notsan nomultifork - #14
Draft
gc00 wants to merge 22 commits into
Draft
Conversation
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens DeepDebug/McMini’s checkpoint/restart + model-checking integration around condition variables, thread lifecycle handling (join/exit), and SIGCHLD/process reaping, with supporting documentation and a new example test.
Changes:
- Improve process/SIGCHLD handling during runner execution and teardown to avoid misattributed child-death reporting.
- Refine CV/mutex modeling and restart behavior (policy handling, mutex owner restoration, avoid touching real pthread_cond_t in post-restart modes).
- Add/extend wrappers and interception to better preserve POSIX semantics across restart (thread join/exit,
__libc_start_mainhook), plus new example(s) and docs.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mcmini/real_world/local_linux_process.cpp | Adds SIGCHLD draining/reaping logic and waitpid-based confirmation in runner wait loop. |
| src/mcmini/real_world/dmtcp_process_source.cpp | Removes dmtcp cleanup destructor; uses dmtcp_target-based restart launch. |
| src/mcmini/model/transitions/mutex.cpp | Ensures observed mutex model includes its real location. |
| src/mcmini/model/transitions/condition_variables.cpp | Simplifies CV init to rely on constructor defaults for policy creation. |
| src/mcmini/model_checking/algorithms/classic_dpor.cpp | Catches process termination/nonzero exit during backtracking replay and routes to callbacks. |
| src/mcmini/mcmini.cpp | Restores mutex owner from checkpoints; improves termination/nonzero-exit reporting callbacks. |
| src/lib/wrappers.c | Tracks mutex owner for checkpointing; fixes timed-wait deadlines; changes thread exit/join semantics; avoids real pthread_cond_* calls post-restart; fixes exit protocol waits. |
| src/lib/interception.c | Adds libpthread timedjoin handle; hooks __libc_start_main to route plain main return through mc_transparent_exit(). |
| src/lib/dmtcp-callback.c | Replaces /proc/self/task thread counting with record-list counting; skips post-restart checkpoint loop for one-shot restarts; updates mutex logging. |
| src/examples/producer-consumer-park.c | New example to exercise “threads parked/unjoined when main returns” behavior. |
| src/examples/CMakeLists.txt | Builds new producer-consumer-park variants including a TSAN build. |
| src/common/runner_mailbox.c | Replaces child-side semaphore with a raw futex-based counting semaphore to avoid glibc desync. |
| src/common/multithreaded_fork.c | Updates to DMTCP v4 pid-translation API names. |
| include/mcmini/spy/intercept/interception.h | Declares libpthread timedjoin handle for RECORD-mode join loop. |
| include/mcmini/spy/checkpointing/objects.h | Changes mutex state representation to include status + owner for checkpoint restore. |
| include/mcmini/real_world/process/dmtcp_process_source.hpp | Switches to dmtcp_target and removes coordinator member/destructor. |
| include/mcmini/real_world/mailbox/runner_mailbox.h | Documents and changes child-side semaphore storage to a futex word. |
| include/mcmini/model/transitions/process/exit.hpp | Marks executor thread exited on process-exit transition to avoid DPOR reselecting it. |
| include/mcmini/model/transitions/mutex/mutex_unlock.hpp | Updates mutex state creation to preserve location and default owner. |
| include/mcmini/model/transitions/mutex/mutex_init.hpp | Preserves mutex location across init transition. |
| include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp | Switches CV waiter handling to policy cloning (but currently calls nonexistent setter). |
| include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp | Switches signal handling to policy cloning (but currently calls nonexistent setter). |
| include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp | Switches enqueue handling to policy cloning and preserves mutex location (but currently calls nonexistent setter). |
| include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp | Switches broadcast handling to policy cloning (but currently calls nonexistent setter). |
| include/mcmini/model/objects/mutex.hpp | Consolidates mutex ctor to require location; defaults owner to RID_INVALID. |
| include/mcmini/model/objects/condition_variables.hpp | Refactors CV ctor/policy initialization (introduces unsafe default construction as written). |
| include/dmtcp.h | Updates vendored DMTCP API to v4 additions/renames and plugin ABI version. |
| doc/glibc-sem-desync.txt | New design note explaining semaphore desync and futex-based fix for mailbox. |
| doc/glibc-cond-var-desync.txt | New design note explaining CV desync/asymmetry and the post-restart pthread_cond_* avoidance strategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+59
to
+61
| condition_variable* new_cv = new condition_variable(new_state, executor, m->get_location(), new_waiting_count); | ||
| new_cv->set_policy(new_policy); | ||
| s.add_state_for_obj(cond_id, new_cv); |
Comment on lines
+74
to
+78
| condition_variable* new_cv = new condition_variable(new_state, executor, | ||
| cv->get_mutex(), | ||
| new_waiting_count)); | ||
| new_waiting_count); | ||
| new_cv->set_policy(new_policy); | ||
| s.add_state_for_obj(cond_id, new_cv); |
Comment on lines
+94
to
+98
| condition_variable* new_cv = new condition_variable(new_state, RID_INVALID, nullptr, new_waiting_count); | ||
| new_cv->set_policy(new_policy); | ||
| s.add_state_for_obj(cond_id, new_cv); | ||
| condition_variable mutable_cv(new_state, RID_INVALID, nullptr, new_waiting_count); | ||
| mutable_cv.check_for_lost_wakeup(true, prev_waiting_count); // Check for lost wakeup if this was a signal |
Comment on lines
28
to
32
| bool hadwaiters; | ||
| mutable unsigned int numRemainingSpuriousWakeups = 0; | ||
| runner_id_t running_thread; | ||
| pthread_mutex_t* associated_mutex; | ||
| int waiting_count = 0; |
Comment on lines
+110
to
+114
| while (true) { | ||
| errno = 0; | ||
| signal_tracker::sig_semwait((sem_t *)&rmb->model_side_sem); | ||
| if (!signal_tracker::instance().try_consume_signal(SIGCHLD)) { | ||
| break; |
Comment on lines
+54
to
+56
| condition_variable* new_cv = new condition_variable(condition_variable::cv_waiting, executor, m->get_location(), new_waiting_count); | ||
| new_cv->set_policy(new_policy); | ||
| s.add_state_for_obj(cond_id, new_cv); |
gc00
force-pushed
the
notsan-nomultifork
branch
2 times, most recently
from
July 31, 2026 06:00
73b1b99 to
1ed319c
Compare
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.
A DMTCP_RESTART_INTO_BRANCH process explores exactly one trace, then gets discarded -- it never legitimately needs to checkpoint again. Left alone, the checkpoint thread resumes its normal sleep-checkpoint-resume loop and blocks forever waiting for a checkpoint request from this restart's one-shot, otherwise-idle coordinator (dmtcp_process_source spins up a fresh coordinator per branch). Call the new dmtcp_skip_post_restart_checkpoint_loop() to tell DMTCP not to resume that loop. Verified: the checkpoint thread now parks in pause() right after restart instead of hanging in read() waiting on the coordinator.
Its destructor unconditionally ran `dmtcp_command -q --port <coordinator_target.get_port()>`, but coordinator_target (a dmtcp_coordinator member) never had launch_and_wait() called anywhere in this class, so its port field stayed at its default-constructed 0 forever -- this call always failed, since no coordinator ever listens on port 0. It's also unnecessary: each branch's own `dmtcp_restart --new-coordinator --port 0` call spawns its own private coordinator with --exit-on-last baked in by DMTCP itself, so it already self-terminates once its sole client disconnects. Nothing in this class needs explicit shutdown. Confirmed via a restart run: the bogus `dmtcp_command -q --port 0` [...] exited with status 2` error no longer appears, with no other change in behavior.
local_linux_process::execute_runner() previously treated any pending SIGCHLD as proof that *this* branch had just died, and reported it with a hardcoded SIGTERM regardless of the true cause. But signal_tracker's SIGCHLD count is a global counter, not tied to a pid, and ~local_linux_process() (invoked by coordinator::assign_new_process_handle() to tear down the previous branch immediately before spawning a new one, e.g. from return_to_depth() on every DPOR backtrack) kills that old process and reaps it without ever consuming the SIGCHLD it generates. That leftover count then lingers until the *next* branch process's very first execute_runner() call, which sees try_consume_signal() return true and wrongly concludes the brand-new process just died -- even though it's alive and simply hasn't responded yet. Confirmed via a waitpid() on the supposedly-dead pid: it blocked forever, proving the process was never actually dead. Fix: ~local_linux_process() now consumes the SIGCHLD from its own deliberate kill, then drains (non-blocking) any other already-reapable zombies -- e.g. the old branch's own private DMTCP coordinator -- consuming one signal per reap, since none of them are tied to a specific pid either. execute_runner() itself also loops on a pending SIGCHLD instead of treating it as automatic proof of death: since the counter isn't pid-scoped, a signal here can still belong to some other descendant. It confirms via a non-blocking waitpid() on this->pid specifically, resuming the wait if that comes back empty, and only reports the real signal/exit code once waitpid() actually confirms this->pid died. Verified: 9/10 fresh-checkpoint restart runs now fully explore all 9 traces and complete cleanly, with zero false "Abnormally Termination" reports across every run (previously: every run failed after 1-2 branches). One rarer, separate hang remains -- a genuine restart-synchronization stall, not a signal-tracking issue -- tracked separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mc_transparent_exit()'s TARGET_BRANCH case used the wait-only thread_await_scheduler() instead of thread_wake_scheduler_and_wait(), unlike every other wrapper. A thread that reaches this case directly (e.g. main() calling exit() as its own first wrapped call, in classic mode or after a restart) never posts model_side_sem, so the coordinator's execute_runner() hangs forever. Verified this is not --multithreaded-fork-specific: reverting just this hunk hangs plain classic mode identically, no DMTCP involved. Also mark the executor exited in process_exit::modify(), mirroring thread_exit: otherwise is_active() keeps reporting true and classic_dpor -- shared across every mode -- either re-selects the transition forever (when program_exit_code() > 0 doesn't already stop exploration, e.g. exit code 0) or reports a false DEADLOCK (confirmed with a trivial single-threaded exit(0) program in classic mode). Adds producer-consumer-park(-tsan), modeled on multithreaded-fork-tsan-2.0's test_park.c, to exercise main() exiting explicitly while other threads are still parked -- the scenario that exposed both bugs under --multithreaded-fork. Verified: a full --multithreaded-fork restart cycle now completes in under a second with a correct DEADLOCK verdict, instead of hanging. Regression-checked producer-consumer(-safe/-exit)-tsan and cv-producer-consumer(-safe)-tsan restart cycles, all still clean.
Two separate, previously-noted gaps in the same area: 1. callbacks.nonzero_exit_code was never wired up in mcmini.cpp (in either classic or DMTCP mode), so a target program exiting with a nonzero code -- exactly the kind of bug the model checker exists to catch -- was caught internally in classic_dpor.cpp, then silently discarded: `if (callbacks.nonzero_exit_code)` was always false, so the run just stopped with no verdict printed at all. Added found_nonzero_exit_code(), mirroring found_abnormal_termination()'s existing trace-printing pattern, and wired it into both callback setups. Verified live with a trivial `exit(1);`-only classic-mode target: now prints "NONZERO EXIT CODE (1)" plus the trace instead of silently completing. 2. local_linux_process.cpp's SIGCHLD-based dead-child detection threw nonzero_exit_code_error unconditionally on WIFEXITED(status), regardless of whether WEXITSTATUS(status) was actually nonzero -- misleading given the exception's own name/contract. A clean (code 0) exit reaching this path is a different situation entirely: it means the whole process fully terminated on its own while a transition was still pending on this runner, bypassing the model-driven exit protocol (mc_transparent_exit(), 6bed56a) entirely -- a McMini-side protocol violation to investigate, not a target-program bug. Now throws execution_error for that case instead of mislabeling it. Regression-checked clean against all 6 existing TSan targets.
A DMTCP-restarted target thread can be genuinely blocked at the kernel level on this exact futex word while glibc's own userspace "is anyone really waiting" bookkeeping (packed into the same memory sem_post() checks before deciding whether to skip the underlying FUTEX_WAKE syscall) is desynced from that -- because mc_runner_mailbox_init()/ _destroy() reinitialize this memory before every new DMTCP-restarted branch, independent of whatever kernel-level futex wait state a resurrected thread still has queued. When that happens, sem_post() silently skips the wake and the branch hangs forever. A plain futex word has no such bookkeeping: mc_raw_sem_post() always calls FUTEX_WAKE unconditionally, and mc_raw_sem_wait() only ever blocks after re-checking the atomic counter, so a wake can never be lost regardless of what order post/wait actually race in. Diagnosed via Gemini-assisted research into glibc's NPTL sem_t internals (packed nwaiters/value in one 64-bit word on 64-bit architectures) plus a direct FUTEX_WAKE(INT_MAX) probe confirming exactly one real waiter was queued when a hang occurred -- ruling out stale/multiple waiters as the cause and pointing squarely at the userspace-bookkeeping desync. model_side_sem is untouched: mcmini itself is never checkpointed, so glibc's bookkeeping for it can never desync. Condition variables (pthread_cond_wait/pthread_cond_signal) have the same class of vulnerability via glibc's G1/G2 waiter-group bookkeeping and will need an analogous fix separately. See doc/glibc-sem-desync.txt for a fuller explanation of the desync mechanism and why it is specific to checkpoint/restart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A target whose main() returns via `return N;` (rather than an explicit exit()/pthread_exit() call) never checked in with mc_transparent_exit()'s restart-quiescence protocol: glibc's __libc_start_call_main calls exit() internally via the hidden __GI_exit alias, bypassing exit()'s own interposition entirely -- confirmed this also holds for _exit() one level deeper (test_implicit_exit_interposition.c). __libc_start_main()'s own call site, in contrast, is in _start (crt1.o, not glibc-internal code), so it resolves through ordinary dynamic symbol resolution and is interposable (test_libc_start_main_hook.c). Wraps the target's main with a version that, after the real main() returns, calls mc_transparent_exit() directly instead of returning to glibc's own (uninterposable) exit machinery -- treating a plain return exactly like an explicit exit(rc) call, in every mode. Verified live: producer-consumer-park(-tsan), whose main() now returns plainly instead of calling exit(0) explicitly (removing that previously- required workaround), correctly reaches INITIAL STATE after a --multithreaded-fork restart with all 3 threads represented (main's pending exit(2), both workers' pending sem_wait), finds the expected DEADLOCK, and completes cleanly -- instead of the previous "Failed to create a new process (template process died)" failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
condition_variable_enqueue_thread::modify() also transitions the mutex to unlocked (cond_wait atomically releases it), but did so via mutex(state) -- the 1-arg constructor, which has no default member initializer for `location` (unlike condition_variable's policy field), so it was left completely uninitialized. Every later mutex_lock/unlock faithfully forwards whatever garbage ms->get_location() reads back, corrupting the mutex's identity for the rest of the run. This is what made condition_variable_wait::modify()'s `m->get_location() == cv->get_mutex()` check fail forever, even after the earlier policy-cloning and mutex-association fixes -- traced by adding a diagnostic directly at state_sequence::follow()'s commit point, the one unambiguous place that distinguishes a genuine commit from transition::is_enabled_in()'s throwaway speculative check. Fixed the same way as condition_variable_signal's associated_mutex fix: pass the existing location through explicitly. Also fixed the identical pattern in mutex_init::modify() and its callback (mutex.cpp) for consistency, though neither is exercised by the CV repro that found this (mutex_init only ever runs once, before any checkpoint, in every target tested so far). With this fix, a checkpoint taken mid-pthread_cond_wait() finally restarts and completes correctly under --multithreaded-fork: DPOR explores all 8 valid interleavings of a producer/consumer/main scenario with zero deadlocks, down from an immediate false DEADLOCK before this session's whole condition-variable investigation started. The core diff_state/state_sequence replay machinery investigated along the way (element indexing, slice()/consume_into_subsequence(), follow()'s commit path) turned out to be architecturally sound -- every symptom traced back to CV/mutex-specific constructors, not the shared state machinery itself. Regression-checked clean against all 6 existing TSan targets.
Its restart case called the one-time-per-thread thread_handle_after_dmtcp_restart() twice; the second call aborts once mode has already advanced past DMTCP_RESTART_INTO_BRANCH/ TEMPLATE (confirmed via dmesg: real SIGABRT). Use the ordinary thread_wake_scheduler_and_wait() for the second round instead, matching every other wrapper's second-and-later round with the coordinator.
Removed the 1-arg mutex(state) and 2-arg mutex(state, location) constructors -- confirmed unused after the recent fixes (every real call site already passes location) -- leaving one 3-arg constructor with location mandatory and tid defaulted to RID_INVALID. This turns "forgot to pass location" from a silent, uninitialized-memory correctness bug (as fixed in 9bd9ecf) into a compile error, and lets mutex_init/enqueue_thread/unlock express "no specific owner" without passing a misleading literal 0 (a real, valid runner id) or leaving `owner` uninitialized. Regression-checked clean against all 6 existing TSan targets.
Collapsed 6 constructors (1-arg through 5-arg, plus a
(state, ConditionVariablePolicy*) overload) into one: state is
mandatory, everything else -- tid, mutex, count, thread_states,
policy -- gets a sensible default (RID_INVALID, nullptr, 0, {},
nullptr-meaning-fresh-policy respectively). `tid`/`mutex` previously
had no default member initializer at all, so any of the shorter
constructors left them uninitialized -- the same bug class fixed for
mutex's `location` in 9bd9ecf, just not yet triggered here.
Simplified two call sites that fell out of the old overloads directly:
cond_init_callback no longer needs to construct its own policy just to
hand it to the constructor (the default already does that), and
condition_variable_signal's replacement object now passes its cloned
policy and the mutex association straight through the constructor
instead of via set_policy()/set_associated_mutex() afterward. Also
fixed signal's throwaway lost-wakeup-check object to be stack-allocated
instead of a leaked `new`, since its constructor call needed rewriting
anyway.
Regression-checked clean against all 6 existing TSan targets, plus the
cv-repro deadlock scenario from 9bd9ecf (still 0 deadlocks).
mc_pthread_cond_wait() never calls the real libpthread_cond_wait()/ libpthread_cond_timedwait() in any post-restart mode (DMTCP_RESTART_ INTO_BRANCH/TEMPLATE, TARGET_BRANCH, TARGET_BRANCH_AFTER_RESTART): the wait is entirely simulated via the mailbox handshake, true even in classic (non-DMTCP) mode. But mc_pthread_cond_signal()/_broadcast()/ _init()/_destroy() still called the real libpthread_cond_signal()/ broadcast()/init()/destroy() in those same modes -- an asymmetry. That asymmetry is dangerous specifically under DMTCP restart: a clone()-recreated thread can still be genuinely, kernel-level blocked inside a pre-restart real pthread_cond_timedwait() call (from RECORD mode, if the checkpoint landed mid-call). A real signal/broadcast reaching that thread wakes it for real, letting it resume running application code without ever going through the model checker's own scheduling -- breaking DPOR's single-stepping invariant. Unlike the child_side_sem fix, this isn't a lost-wakeup story: it's a real, uncontrolled wakeup escaping the model checker entirely, which is arguably worse than a hang. Fix: remove the real libpthread_cond_signal()/broadcast()/init()/ destroy() calls from all four post-restart cases, mirroring what mc_pthread_cond_wait() already did -- once wait never consults the real object's state, touching it from signal/broadcast/init/destroy serves no purpose, only risk. RECORD/PRE_CHECKPOINT mode is untouched: those real calls remain necessary and safe there, since it's one continuous execution before any checkpoint exists. Verified classic-mode cv-test produces byte-for-byte-equivalent output (modulo debug-log pid/interleaving) before and after, confirming no behavior change for the already-real-call-free wait path this mirrors. See doc/glibc-cond-var-desync.txt for the full analysis, including why a normally single-process (pshared=0) condition variable is affected by the same class of bug as the pshared=1 child_side_sem mailbox semaphore despite the different pshared requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
producer-consumer has no condition variables at all, so it can't exercise the "CV desync fix: stop touching cond_t after restart" fix. Add a structurally identical producer-consumer variant that uses a mutex + condition variable (count-based bounded buffer, single shared cond, classic while-loop predicate wait) instead of the two semaphores. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mc_pthread_mutex_lock(), mc_pthread_join_impl(), and
mc_pthread_cond_wait() each hoist a struct timespec {.tv_sec = 2,
...} once, outside their RECORD-mode retry loop, and pass it to
libpthread_mutex_timedlock()/libpthread_timedjoin_np()/
libpthread_cond_timedwait() as an absolute deadline every iteration.
Since it's never computed from the current time, it means "2 seconds
past the epoch" -- decades in the past -- so every call returns
ETIMEDOUT immediately instead of ever genuinely blocking.
mc_sem_wait() (sem-wrappers.c) already does this correctly, calling
clock_gettime(CLOCK_REALTIME, &ts); ts.tv_sec++; fresh on each
iteration.
Found while trying to live-test the pthread_cond_t desync fix
(f54bb73) under a real DMTCP+TSan checkpoint/restart cycle: doing so
requires a thread to be genuinely, kernel-level blocked inside a
real pthread_cond_wait() at checkpoint time, which this bug made
impossible -- the RECORD-mode wait was actually a tight busy-poll,
never blocking long enough for anything to observe.
Fix: recompute the deadline via clock_gettime() on every iteration
in all three loops, matching mc_sem_wait()'s existing pattern.
Verified: cv-test, deadly-embrace, and classic-mode
producer-consumer give identical results; 20 consecutive
fresh-checkpoint --multithreaded-fork cycles against
producer-consumer-tsan still complete cleanly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
condition_variable_enqueue_thread::modify() requires the mutex to be locked_by(executor) before the "enter wait" transition can be enabled, but the recorded mutex_state carried only LOCKED/UNLOCKED, no owner -- translate_recorded_object_to_model() built the restored mutex with the 2-arg constructor, leaving owner default-constructed/unset. Every restart with a thread mid-cond_wait therefore deadlocked immediately: the mutex looked locked by no one that matched, so neither the producer's lock nor the consumer's own wait-entry could ever become enabled. Added an owner field to mutex_state (objects.h), set it to tid_self on every successful RECORD-mode mutex_lock and clear it on unlock, and pass it through in mcmini.cpp's mutex reconstruction. Verified: the same restart-from-checkpoint scenario that previously deadlocked instantly now correctly treats the consumer's cond_wait as enabled at the initial state. producer-consumer-tsan (mutex-only, no CVs) regression-checked clean, 9 traces, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
transition::is_enabled_in() (transition.hpp) runs modify() against a throwaway diff_state purely to check the returned status, discarding the diff afterward. But condition_variable_enqueue_thread/_wait/ _signal/_brdcast's modify() all called mutating policy methods (add_waiter_with_state, wake_thread, add_to_wake_groups, receive_broadcast_message) directly on cv->get_policy() -- a pointer into the *previous*, already-committed state's object, not anything scoped to the throwaway diff. Every such "is this enabled" check, even ones never actually applied, permanently corrupted the committed policy. Confirmed live: a waiter that should appear once in a CV's wait queue was appearing 4 times. Fixed by cloning the policy (ConditionVariablePolicy::clone(), already implemented but unused by these four call sites) and performing every mutation on the clone, attaching it to the replacement object via the existing set_policy() setter instead. condition_variable_signal's replacement object additionally used a constructor that never sets associated_mutex at all, leaving it uninitialized -- condition_variable_wait::modify()'s mutex-location check then never matches again for the rest of the run. Preserve it via the existing set_associated_mutex() setter, same call site as the set_policy() fix above. Reduced the duplicate-waiter count in the live repro from 4 to 2 (real progress, not yet a full fix) and didn't affect a second, still-open bug in the same repro: a mutex object's location field reads back corrupted partway through the same run, which looks like a separate, deeper issue in state_sequence's replay/backtracking bookkeeping rather than anything CV-specific -- not yet root-caused. Regression-checked clean against all 6 existing TSan targets.
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.
@aayushi363 , These are the commits that I'd like you to review. All of them are: no TSAN, no multithreaded-fork, but they do involve condition variables. So, skip all commits through "Harden mutex constructor against dropped location", and begin reviewing starting at "Harden condition_variable constructor like mutex's". So, you only have to review the last 5 commits.
I'm also requesting a review from copilot to help you. It would be great to have a human read it before we push it into main.
And these commits will be pushed in after PR #3, #4, #5, #7, #13 (where PR #3 and #4 are commits that you'll redo to do them the right way.
@maxwellpirtle is reviewing PR #13 (the commits before yours).
I'm labelling this a Draft PR, since it's included in the full PR #11. After we converge on this PR #14, I'll map it back into PR #11.