Joined threads now terminate, not park forever (and other TSAN-friendly fixes)#12
Open
gc00 wants to merge 5 commits into
Open
Joined threads now terminate, not park forever (and other TSAN-friendly fixes)#12gc00 wants to merge 5 commits into
gc00 wants to merge 5 commits into
Conversation
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.
📝 WalkthroughWalkthroughAdds per-thread exit permission semaphores, makes child-thread termination wait for permission, and changes target-branch joins to perform real pthread joins. Abnormal termination reporting and DPOR backtracking now handle replay edge cases. ChangesThread exit and join synchronization
Abnormal termination handling
Estimated code review effort: 3 (Moderate) | ~20 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 exit permission
mc_exit_thread_in_child->>exit_permission_sem: wait for permission
mc_exit_thread_in_child-->>libpthread_pthread_join: thread can terminate
mc_pthread_join->>libpthread_pthread_join: perform real join
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Comment on lines
+366
to
+368
| sem_t *exit_permission = find_exit_permission_sem(pthread_self()); | ||
| assert(exit_permission != NULL); | ||
| libpthread_sem_wait(exit_permission); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/lib/wrappers.c --items all
rg -n -C4 '\blibpthread_sem_wait\s*\(|\bsem_wait\s*\(' src includeRepository: mcminickpt/deep-debug
Length of output: 19086
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '350,375p' src/lib/wrappers.c
sed -n '170,195p' src/lib/sem-wrappers.c
sed -n '282,303p' src/lib/interception.c
sed -n '45,60p' include/mcmini/spy/checkpointing/record.h
sgrep --version 2>/dev/null || trueRepository: mcminickpt/deep-debug
Length of output: 3290
Retry interrupted exit-permission semaphore waits.
libpthread_sem_wait() can return -1/EINTR before acquiring join permission; here that makes the exiting child skip the assert() and continue past the guarded termination boundary. Let libpthread_sem_wait()/mc_sem_wait() retry EINTR internally, or wrap this site with the existing retry helper before proceeding after failure.
🤖 Prompt for 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.
In `@src/lib/wrappers.c` around lines 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.
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.
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
changed the title
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.
gc00
force-pushed
the
mcmini-thread-exit-fix
branch
from
July 27, 2026 05:26
2b0d27d to
d222a73
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.
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