Joined threads now terminate, not park forever (and other TSAN-friendly fixes; 5 commits) - #12
Open
gc00 wants to merge 6 commits into
Open
Joined threads now terminate, not park forever (and other TSAN-friendly fixes; 5 commits)#12gc00 wants to merge 6 commits into
gc00 wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes update DMTCP interfaces, PID translation, checkpoint thread accounting, pthread exit coordination, timed joins, and replay termination handling. They also handle missing model transitions during abnormal termination reporting. ChangesRuntime synchronization and replay handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant mc_pthread_join
participant exit_permission_sem
participant mc_exit_thread_in_child
participant libpthread_pthread_join
mc_pthread_join->>exit_permission_sem: post target exit permission
mc_exit_thread_in_child->>exit_permission_sem: wait for exit permission
mc_pthread_join->>libpthread_pthread_join: perform real join
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/wrappers.c`:
- Around line 366-368: Update the exit-permission wait in the code surrounding
find_exit_permission_sem so an EINTR result from libpthread_sem_wait cannot
allow execution to continue without acquiring the semaphore. Prefer making
libpthread_sem_wait or mc_sem_wait retry EINTR internally; otherwise use the
existing retry helper at this call site before proceeding past the guarded
termination boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Pull request overview
This PR updates the thread-join/exit interaction in the McMini pthread wrappers so that a thread that is joined in the TARGET_BRANCH mode can actually terminate at the OS level (allowing real pthread_join()—including TSAN’s interceptor—to complete), instead of remaining parked indefinitely.
Changes:
- Add a per-thread semaphore (
exit_permission_sem) in the pthread→runner-id map to coordinate when an exiting thread is allowed to truly terminate. - Update
mc_exit_thread_in_child()to wait on that semaphore (instead of parking forever), and updatemc_pthread_join()inTARGET_BRANCHto post the semaphore and then perform a realpthread_join()(populating*rv). - Fix a
pthread_map_lockread-lock leak insearch_pthread_map()by ensuring the unlock occurs on all paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gc00
changed the title
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 27, 2026 05:26
2b0d27d to
d222a73
Compare
maxwellpirtle
approved these changes
Jul 28, 2026
maxwellpirtle
left a comment
maxwellpirtle
left a comment
Contributor
There was a problem hiding this comment.
I think this should be OK. We should just check eventually against cases where the pthread_t is reused by libpthread in Phase I mode, although we might be storing based on the unique thread ID but we'd have to double check
| // pthread_create(), so neither is ever recorded here), and -- since a | ||
| // DMTCP checkpoint is a full memory snapshot -- this list is preserved | ||
| // exactly as it was at record time across every restart, with no | ||
| // dependence on restart-time scheduling. |
mc_pthread_join's RECORD loop called pthread_timedjoin_np directly, which resolves to libtsan's interceptor under a TSAN target. Its ConsumeThreadUserId trips a thread-registry CHECK (sanitizer_thread_registry.cpp:348) and aborts. Add a libpthread_timedjoin_np handle (dlsym'd from libpthread, like the mutex/cond/sem wrappers) that bypasses libtsan, and call it from mc_pthread_join's RECORD loop instead of the raw symbol. This completes end-to-end TSAN-target checkpointing under deep-debug (mcmini record mode), alongside 5be8500 (DMTCP plugin API v3->v4) and 4bf2720 (TSan-safe RECORD prologue). Verified: `mcmini -i 3 ~/dmtcp.git/test/tsan_target` runs with no SEGV/ThreadSanitizer errors, producing a valid checkpoint matching the no-mcmini baseline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TSAN's pthread_join() interceptor delegates to a genuine OS-level join and blocks via the kernel until the target thread actually dies -- it does not rely on its own creation-time bookkeeping. But mc_pthread_join()'s TARGET_BRANCH case only simulated success at the model level, while the joined thread was kept parked in thread_block_indefinitely() forever, so a real join on it (e.g. from TSan) could never complete. Give each thread its own exit_permission_sem (alongside its existing pthread_map entry). A finishing thread waits on it before returning; mc_pthread_join() posts it and performs a real libpthread_pthread_join() before returning, and as a bonus, pthread_join returns a return value.
classic_dpor::verify_using()'s forward-exploration path catches real_world::process::termination_error and reports it via the abnormal_termination callback, letting the run end cleanly. The backtrack-replay path (coordinator::return_to_depth(), which replays prior transitions against a freshly restarted process) had no such handling, so the same exception there escaped all the way to the top-level catch-all instead. Wrap return_to_depth() the same way. found_abnormal_termination() also needed a null check: return_to_depth()'s target thread may have no pending transition in the model's current view (unlike the forward path, where the culprit is always the runner DPOR just selected as enabled). The report then falls back to a plain "no longer pending" line instead of dereferencing a null transition.
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 30, 2026 22:15
d222a73 to
e98dd6c
Compare
This was referenced Jul 30, 2026
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 31, 2026 04:29
e98dd6c to
d222a73
Compare
gc00
changed the title
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 31, 2026 05:58
d222a73 to
0e0e810
Compare
maxwellpirtle
approved these changes
Jul 31, 2026
| // genuinely populated -- before we return, instead of only | ||
| // simulating success at the model level. | ||
| sem_t *exit_permission = find_exit_permission_sem(t); | ||
| assert(exit_permission != NULL); |
Contributor
There was a problem hiding this comment.
I think the semaphores are correct but unnecessary. Why do we block the threads until we need to join on them?
Contributor
Author
There was a problem hiding this comment.
@maxwellpirtle , I can add a more informative comment if you like. Tell me what you'd like to emphasize. As for the reason that it's necessary, Claude says:
- The semaphores aren't just bookkeeping — they're what prevents pthread_t reuse from aliasing two different threads in pthread_map. mc_exit_thread_in_child() reports the model-visible EXIT transition before waiting on exit_permission_sem; what the wait actually delays is the real OS exit, until the corresponding join happens. glibc can recycle a joinable thread's pthread_t/TCB the moment it's reaped, and our lookups (find_pthread_map_entry et al.) identify threads only by pthread_equal() on that value — so if we let the OS thread really die before the model processes its join, a brand-new thread could get assigned the same pthread_t and get misidentified as the "exited" one. Blocking until join keeps the model's "not yet joined" belief and the OS's "not yet reaped" reality in sync, same as it stays in sync for any real, un-joined joinable thread. This is the same aliasing risk you flagged in your other comment about pthread_t reuse in Phase I mode.
The TSAN-supporting DMTCP branch (tsan-phased-init) bumped the
plugin API from v3 to v4, an ABI change (DmtcpPluginDescriptor_t /
DmtcpUniqueProcessId, new DmtcpCkptHeader etc.). DMTCP refused to
load libmcmini.so:
ASSERT pluginmanager.cpp:228: incompatible DMTCP plugin API version:
plugin_api=3 expected=4
Sync the vendored include/dmtcp.h to DMTCP's v4 header (correct
version string and descriptor ABI), and carry forward the only
McMini-specific additions -- the mcmini_virtual_pid / mcmini_real_pid
macros -- updated to the v4 function names
(dmtcp_{real_to_virtual,virtual_to_real}_pid became
dmtcp_pid_{real_to_virtual,virtual_to_real}). Also update the two
direct callers in multithreaded_fork.c. The unused
dmtcp_restore_buf_* decls are dropped (not referenced by libmcmini,
and gone from v4).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
template_thread() computed the number of threads to wait for by scanning /proc/self/task live, at restart time, then subtracting a blanket 2 (for itself and the checkpoint thread). DMTCP recreates checkpointed threads asynchronously via clone(), so a scan that runs before it has finished recreating all of them undercounts -- this barrier then declares "consistent state" and lets the template thread proceed before every thread has actually restarted. Confirmed via added diagnostic logging: one thread's own restart-completion signal could arrive after the barrier already released. Fix: count ALIVE THREAD entries in head_record_mode instead. That list only ever gets entries for genuine target threads (the template thread and checkpoint thread never go through libmcmini's wrapped pthread_create(), so neither is ever recorded there), and since a DMTCP checkpoint is a full memory snapshot, it's preserved exactly as-is across every restart -- immune to any restart-time scheduling race. Also remove fast_multithreaded_fork()'s #if 1/#else wrapper: the #else side held an early, never-compiled clone()-based fork prototype that predates this file's current _Fork()-based approach and was always dead code. Also remove the needless signal-mask check in threaded fork: getcontext()/setcontext() already restore a thread's blocked-signal set via uc_sigmask, even across the raw clone() used to recreate a checkpointed thread, so the thread_sigmask field and abort were unnecessary. That check ran on every restarted thread regardless of mode, so it also blocked plain (non-multithreaded-fork) restart of any target with a blocked-signal thread.
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 31, 2026 22:17
0e0e810 to
9bd964d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@maxwellpirtle, Please review this very small PR. The first commit is for what we originally talked about when you decided to let a joined thread park forever instead of exiting. The second commit is trivial. The next three are fixing original bugs in McMini (or updating to DMTCP's newer v4 plugin API). Thanks for reviewing this. It simplifies my work, since I always have to rebase on this PR before creating new commits.
And now there are 2 more McMini bugs that I'll be adding here (currently in the mcmini-dmtcp-tsan branch). And Claude seems to have even discovered a known bug/misfeature of semaphores and condition variables in the GLIBC implementation. So, there will be additional commits to fix the GLIBC bugs! Weird.
Before pushing in these commits, we should push in PR #3, PR #4, PR #5, PR #7. And this PR #12 (which already includes PR #6). After that, we can push in the HUGE PR #11 (which will contain this PR #12 as the first few commits).
For PR #3 and PR #4, @maxwellpirtle is still updating those PR's with a better commit.
6 commits in total:
Zeroth commit: Bypass libtsan in mc_pthread_join's timed join
First commit: TSAN's pthread_join() interceptor delegates to a genuine OS-level
join and blocks via the kernel until the target thread actually
dies -- it does not rely on its own creation-time bookkeeping. But
mc_pthread_join()'s TARGET_BRANCH case only simulated success at the
model level, while the joined thread was kept parked in
thread_block_indefinitely() forever, so a real join on it (e.g. from
TSan) could never complete.
Give each thread its own exit_permission_sem (alongside its existing
pthread_map entry). A finishing thread waits on it before returning;
mc_pthread_join() posts it and performs a real libpthread_pthread_join()
before returning, and as a bonus, pthread_join returns a return value.
Second commit: Also fix a pthread_map_lock leak in search_pthread_map() found along
the way: on a match, the loop returned directly, skipping the unlock
below it, so a later insert_pthread_map() write-lock would block
forever. Break out of the loop and unlock on every path instead.
Third commit: DPOR backtrack replay: Report abnormal termination
classic_dpor::verify_using()'s forward-exploration path catches
real_world::process::termination_error and reports it via the
abnormal_termination callback, letting the run end cleanly. The
backtrack-replay path (coordinator::return_to_depth(), which replays
prior transitions against a freshly restarted process) had no such
handling, so the same exception there escaped all the way to the
top-level catch-all instead.
Fourth commit: Port libmcmini DMTCP plugin from plugin API v3 to v4
We will target DMTCP-5.0 (soon to be released). From now on, we use the
new plugin API.
Fifth commit: Fix TOCTOU race in template_thread()'s restart barrier
Fixes a bug in McMini, concerning a thread moving past a barrier (see commit msg)
Summary by CodeRabbit
Bug Fixes
Compatibility