DeepDebug: McMini, DMTCP and TSAN - #11
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:
📝 WalkthroughWalkthroughThis change adds optional TSan instrumentation, TSan-aware thread and fork handling, futex mailbox synchronization, checkpoint model-state preservation, process-status handling, example targets, tests, and diagnostic documentation. ChangesTSan and checkpoint/restart integration
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
9f80a5d to
a917800
Compare
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.
3310863 to
6513b37
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends DeepDebug/McMini’s DMTCP integration to work with ThreadSanitizer (TSAN), addressing multiple interposition/order-of-initialization conflicts between McMini wrappers, TSAN interceptors, and DMTCP restart semantics.
Changes:
- Hardened restart/branch execution paths to correctly handle thread/process termination, SIGCHLD attribution, and exit/join semantics under TSAN + DMTCP.
- Added TSAN-specific support in libmcmini (fiber switching, annotations, TSAN-safe allocation/initialization, and target-side
--wrapshims for interceptors that bypass interposition). - Added new TSAN-focused examples, tests, and extensive documentation of discovered failure modes and fixes.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/tsan_support/test_thread_blocks_signal.c | Standalone test for /proc-based signal-block detection helper. |
| src/mcmini/real_world/local_linux_process.cpp | Improves SIGCHLD handling and distinguishes unrelated descendant deaths. |
| src/mcmini/real_world/dmtcp_process_source.cpp | Removes DMTCP coordinator cleanup logic from destructor. |
| src/mcmini/model/transitions/mutex.cpp | Preserves mutex location/id when creating model objects. |
| src/mcmini/model/transitions/condition_variables.cpp | Simplifies condvar init and relies on constructor defaults for policy. |
| src/mcmini/model_checking/algorithms/classic_dpor.cpp | Reports termination/nonzero exits during backtrack replay via callbacks. |
| src/mcmini/mcmini.cpp | Restores mutex owner from checkpoints; adds nonzero-exit reporting callback. |
| src/lib/wrappers.c | Major TSAN/DMTCP compatibility work: join/exit semantics, TSAN annotations, init ordering, CV behavior, etc. |
| src/lib/tsan_support.c | Adds TSAN-safe helpers for thread classification and real TID detection. |
| src/lib/sem-wrappers.c | Renames CHECKPOINT_THREAD handling to EXTERNAL_THREAD forwarding. |
| src/lib/record.c | Adds TSAN-safe record entry allocation and TSAN-internal-thread gating in mode detection. |
| src/lib/pthread_join_wrap.c | Target-side --wrap=pthread_join shim to bypass TSAN join hangs. |
| src/lib/pthread_cond_wait_wrap.c | Target-side --wrap=pthread_cond_wait shim because TSAN bypasses interposition. |
| src/lib/pthread_cond_signal_wrap.c | Target-side --wrap=pthread_cond_signal shim because TSAN bypasses interposition. |
| src/lib/main.c | Adds early runtime warning for TSAN targets missing required --wrap join shim. |
| src/lib/log.c | Makes logging thread-safe (tz init, lock) and adds post-_Fork reset hook; TSAN annotations. |
| src/lib/interception.c | Adds TSAN-aware init fast path and new interposed functions (pthread_exit, timedjoin, clone, libc_start_main). |
| src/lib/dmtcp-callback.c | TSAN fiber + fork syscalls hooks; safer clone usage; improved thread counting barrier; one-shot checkpoint thread behavior. |
| src/examples/producer-consumer-safe.c | Race-free variant of producer/consumer example for TSAN signal-to-noise. |
| src/examples/producer-consumer-park.c | Test target with alive-but-parked threads at process exit. |
| src/examples/producer-consumer-exit.c | Test target explicitly exercising pthread_exit() paths. |
| src/examples/exit-stress.c | Stress test for pthread_exit behavior at higher thread counts. |
| src/examples/exit-stress-noop.c | Reliable reproducer for checkpoint-thread/TSAN background-thread init race. |
| src/examples/cv-producer-consumer.c | Condition-variable based example target for restart/TSAN scenarios. |
| src/examples/cv-producer-consumer-safe.c | Race-free condvar example variant for cleaner TSAN runs. |
| src/examples/CMakeLists.txt | Adds many TSAN-instrumented example targets with required --wrap link flags and post-build copies. |
| src/common/runner_mailbox.c | Replaces child-side glibc sem_t with raw futex-based counting semaphore to avoid desync. |
| src/common/multithreaded_fork.c | Updates DMTCP API calls and resets log mutex post-_Fork in shared-library path. |
| src/common/mem.c | Implements TSAN-safe bump allocator (mc_ts_alloc) for pre-TSAN-registration allocations. |
| include/mcmini/spy/intercept/wrappers.h | Exposes new helpers: internal-thread creation flag, recreated-thread query, deferred join API, mc_pthread_exit. |
| include/mcmini/spy/intercept/interception.h | Declares tsan_or_real_pthread_create, timedjoin handle, pthread_exit forwarding, and raw libc_clone. |
| include/mcmini/spy/checkpointing/tsan_support.h | Declares TSAN support helpers used by mode detection and restart logic. |
| include/mcmini/spy/checkpointing/record.h | Renames CHECKPOINT_THREAD to EXTERNAL_THREAD; exposes checkpoint-thread window helpers and TSAN-safe record allocation. |
| include/mcmini/spy/checkpointing/objects.h | Extends mutex state to include owner for correct post-checkpoint reconstruction. |
| include/mcmini/real_world/process/dmtcp_process_source.hpp | Updates includes and removes coordinator member / destructor. |
| include/mcmini/real_world/mailbox/runner_mailbox.h | Changes child-side mailbox semaphore from sem_t to raw futex word. |
| include/mcmini/model/transitions/process/exit.hpp | Marks exiting thread as exited to avoid DPOR re-selecting exit transitions. |
| include/mcmini/model/transitions/mutex/mutex_unlock.hpp | Aligns mutex unlock transition with new mutex constructor signature. |
| include/mcmini/model/transitions/mutex/mutex_init.hpp | Preserves mutex location when initializing model state. |
| include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp | Avoids in-place policy mutation; clones policy for diff_state correctness. |
| include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp | Avoids in-place policy mutation; preserves mutex association and policy. |
| include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp | Avoids in-place policy mutation; preserves mutex location; carries cloned policy forward. |
| include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp | Avoids in-place policy mutation; uses cloned policy and preserves it in new CV object. |
| include/mcmini/model/objects/mutex.hpp | Consolidates mutex constructors; enforces passing mutex location and tracks owner. |
| include/mcmini/model/objects/condition_variables.hpp | Refactors constructors/policy handling to support policy cloning and state reconstruction. |
| include/mcmini/mem.h | Declares TSAN-safe allocator API. |
| include/mcmini/lib/log.h | Declares post-_Fork log mutex reset helper. |
| include/dmtcp.h | Updates DMTCP plugin API version and adds/renames several API definitions and helpers. |
| doc/tsan-mutex-annotation-false-positive.txt | Documents mutex annotation false positives and the annotation-based fix. |
| doc/pthread-exit-abort-and-fiber-crash.txt | Documents pthread_exit failures and the fiber + raw-exit workaround. |
| doc/log-mutex-fork-desync.txt | Documents log mutex deadlock across _Fork and the reset fix. |
| doc/glibc-sem-desync.txt | Documents glibc semaphore desync and rationale for raw futex usage. |
| doc/glibc-cond-var-desync.txt | Documents condvar desync risk and removal of real condvar ops post-restart. |
| doc/cond-wait-tsan-interceptor-bypass.txt | Documents TSAN bypass of pthread_cond_wait and the need for target-side wraps. |
| doc/classic-mode-thread-registration-segv.txt | Documents classic-mode TSAN thread registration crash and RTLD_NEXT create fix. |
| CMakeLists.txt | Adds option to build libmcmini with TSAN and includes new TSAN support source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (libmcmini_init_done) { | ||
| return; | ||
| } | ||
| pthread_once(&libmcini_init, &mc_load_intercepted_pthread_functions); | ||
| libmcmini_init_done = true; |
| char padding[1792]; | ||
| } DmtcpCkptHeader; | ||
|
|
||
| static_assert(sizeof(DmtcpCkptHeader) == 4096, "DmtcpCkptHeader must be 4096 bytes"); |
| ConditionVariablePolicy* policy; | ||
|
|
||
| public: | ||
| condition_variable() = default; | ||
| ~condition_variable() = default; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
test/tsan_support/test_thread_blocks_signal.c-49-51 (1)
49-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a deterministically invalid task ID.
Task ID
999999can exist. If this process receives that ID, the test fails intermittently. Use-1;thread_blocks_signal()maps it to/proc/self/task/0/status, which cannot exist.Proposed fix
- assert(thread_blocks_signal(999999 /* bogus tid, should not exist */, SIGUSR1) == 0); + assert(thread_blocks_signal(-1, SIGUSR1) == 0);🤖 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 `@test/tsan_support/test_thread_blocks_signal.c` around lines 49 - 51, Replace the hard-coded bogus task ID in the thread_blocks_signal() invalid-ID assertion with -1, preserving the expected return value of 0 and ensuring the test always targets a nonexistent task status path.src/examples/exit-stress.c-27-29 (1)
27-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the pre-exit checkpoint window signal-resilient.
POSIX
sleep()returns the unslept seconds when interrupted by a signal, andsrc/examples/producer-consumer-park.cdocuments DMTCP pre-checkpoint signals returning early from blocking syscalls. If thissleep()wakes early, workers incrementcounterand exit before the checkpoint is taken, so the restart may no longer exercise the recreated-threadpthread_exit()path under this scenario.Use a monotonic deadline loop such as the existing busy wait for the worker delay.
🤖 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/examples/exit-stress.c` around lines 27 - 29, Replace the single sleep(10) delay in the exit-stress scenario with a monotonic deadline loop, following the existing worker-delay busy-wait pattern, so pre-checkpoint signals cannot shorten the window before workers exit. Preserve the ten-second delay and ensure the loop continues until the deadline is reached.doc/glibc-sem-desync.txt-88-93 (1)
88-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the condition-variable status.
These lines state that no condition-variable fix exists.
doc/glibc-cond-var-desync.txtdocuments the condition-variable fix in this PR. Replace this stale status with a reference to that document and list only remaining limitations.Proposed documentation update
-Condition variables (pthread_cond_wait/pthread_cond_signal) have the same -class of vulnerability via glibc's G1/G2 waiter-group bookkeeping, for the -same reason (checkpoint/restart + externally-managed reinitialization). An -analogous fix has not yet been applied there. +Condition variables have the same class of vulnerability via glibc's G1/G2 +waiter-group bookkeeping. The post-restart signal, broadcast, init, and +destroy paths are handled in doc/glibc-cond-var-desync.txt.🤖 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 `@doc/glibc-sem-desync.txt` around lines 88 - 93, Update the condition-variable section in glibc-sem-desync.txt to remove the stale “Not yet fixed” claim, reference glibc-cond-var-desync.txt as documenting the applied fix, and retain only the limitations that still remain.src/examples/producer-consumer-park.c-54-54 (1)
54-54: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRetry only
EINTRfromsem_wait().The loop retries every nonzero return value, so permanent
sem_wait()failures such asEINVALspin indefinitely and hide semaphore setup failures. Retry only whensem_wait()returns-1witherrno == EINTR, then abort on other errors.Proposed fix
+#include <errno.h> + -static void sem_wait_retry(sem_t *s) { while (sem_wait(s) != 0) /* EINTR */; } +static void sem_wait_retry(sem_t *s) { + while (sem_wait(s) == -1) { + if (errno != EINTR) { + perror("sem_wait"); + abort(); + } + } +}🤖 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/examples/producer-consumer-park.c` at line 54, Update sem_wait_retry so it loops only when sem_wait returns -1 with errno equal to EINTR; for any other failure, abort immediately and preserve the semaphore error visibility instead of retrying indefinitely.src/lib/wrappers.c-232-241 (1)
232-241: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset
owneron a successful re-initialization.Line 233 sets
.owner = RID_INVALIDonly when the record is created. For an already-known mutex, line 241 setsstatus = UNLOCKEDand leavesownerat its previous value. A mutex that was locked beforepthread_mutex_init()therefore keeps a stale owner while the status says unlocked.condition_variable_enqueue_thread::modify()checksmutex->is_locked_by(executor), so the model can read an inconsistent pair.🐛 Proposed fix
mutex_record->vo.mut_state.status = UNLOCKED; + mutex_record->vo.mut_state.owner = RID_INVALID; libpthread_mutex_unlock(&rec_list_lock);🤖 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 232 - 241, In the successful initialization branch after libpthread_mutex_init returns zero, update the existing mutex_record state so owner is reset to RID_INVALID alongside status = UNLOCKED. Apply this to both newly created and already-known mutex records, using mutex_record->vo.mut_state.include/mcmini/spy/checkpointing/record.h-198-202 (1)
198-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign stale TSAN design-document references
The code references
TSAN-McMini-DMTCP.txtandTSAN-pthread-join.md, but those exact files are not indoc/. Update these references to the actual added doc files, or add the referenced documents with stable names.🤖 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 `@include/mcmini/spy/checkpointing/record.h` around lines 198 - 202, Update the stale documentation references associated with add_rec_entry_record_mode_ts and the related TSAN pthread-join references to point to the actual documents present in doc/, or add the missing documents using stable names; ensure no references remain to nonexistent TSAN-McMini-DMTCP.txt or TSAN-pthread-join.md files.src/common/multithreaded_fork.c-181-183 (1)
181-183: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse virtual TIDs consistently before sending SIG_MULTITHREADED_FORK.
multithreaded_fork()passes the virtualized/proc/self/taskentries directly totgkill(), but DMTCP comments saytgkill()needs the same virtual identifier. Sincectlis already virtualized in both callers, make the directSys_gettid()value virtual too before thetids[i] != mytidcomparison.🤖 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/common/multithreaded_fork.c` around lines 181 - 183, Virtualize the ctid value obtained from Sys_gettid() using dmtcp_pid_real_to_virtual() before the tids[i] != mytid comparison in the multithreaded_fork function, ensuring that the real thread ID from Sys_gettid() is converted to a virtual TID to match the virtual identifiers already present in the tids array from /proc/self/task before the comparison is performed.src/lib/main.c-30-39 (1)
30-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid probing
__wrap_pthread_joinfromlibmcmini.so.
producer-consumer-tsanand the other TSan link lines compilesrc/lib/pthread_join_wrap.cwith-Wl,--wrap=pthread_joinbut do not use-rdynamicor-Wl,--export-dynamic. The shared library’s weak undefined reference can therefore stay NULL even though the executable has the wrapper, and the constructor prints the warning incorrectly. Do not rely on this dynamic lookup unless the TSan targets export symbols, or switch to an explicit registration call from the wrapper.🤖 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/main.c` around lines 30 - 39, Remove the __wrap_pthread_join probe and warning from warn_if_tsan_target_missing_wrap, since libmcmini.so cannot reliably observe the executable’s linker wrapper. Use an explicit registration call from pthread_join_wrap.c instead, or otherwise ensure TSan targets export the wrapper before retaining the lookup.include/mcmini/model/objects/mutex.hpp-24-33 (1)
24-33: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequiring
locis correct, but the defaulted default constructor still bypasses it.The single constructor now forces every caller to supply the real address. Line 21 still declares
mutex() = default;, which leaveslocationandownerindeterminate. That reintroduces exactly the failure the comment describes, becauseget_location()then returns garbage.Add default member initializers for
locationandowner, or delete the default constructor if no caller needs it.🛡️ Proposed fix
state current_state = state::uninitialized; - pthread_mutex_t* location; - runner_id_t owner; + pthread_mutex_t* location = nullptr; + runner_id_t owner = RID_INVALID;🤖 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 `@include/mcmini/model/objects/mutex.hpp` around lines 24 - 33, Fix the default construction path in the mutex class by removing mutex() = default if it is unused, or initialize location and owner to safe values through default member initializers. Ensure every mutex instance has a valid location state and RID_INVALID ownership before get_location(), mutex_lock, or mutex_unlock can use it, while preserving the explicit constructor’s behavior.src/lib/interception.c-392-400 (1)
392-400: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a failed
__libc_start_mainresolution.
dlsym(RTLD_NEXT, "__libc_start_main")can returnNULL. The code then calls aNULLpointer, and every target process crashes beforemain()with no diagnostic. Report the failure explicitly.🛡️ Proposed fix
libc_start_main_fn real_start_main = (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); + if (real_start_main == NULL) { + fprintf(stderr, "mcmini: dlsym(RTLD_NEXT, \"__libc_start_main\") failed: %s\n", + dlerror()); + fflush(stderr); + _exit(1); + } return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, stack_end);Note: use
_exit(2)here, notlibc_exit(), because this path runs before initialization completes.🤖 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/interception.c` around lines 392 - 400, Update __libc_start_main to validate the result of dlsym before invoking real_start_main; when resolution fails, report the failure explicitly and terminate with _exit(2), avoiding libc_exit because initialization is incomplete. Preserve the existing delegation to wrapped_main when the symbol resolves successfully.src/lib/interception.c-125-125 (1)
125-125: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck
clone_ptrbefore callinglibc_clone().
dlsym(libc_handle, "__clone")can returnNULL, andlibc_clone()later invokes(*clone_ptr). Add aNULLcheck around this resolve with a diagnostic, thenlibc_abort()to avoid crashing during restart.🤖 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/interception.c` at line 125, Validate the result assigned to clone_ptr immediately after resolving "__clone" with dlsym in libc_clone(); if it is NULL, emit a diagnostic and call libc_abort() before any indirect invocation. Preserve the existing libc_clone restart flow when clone_ptr resolves successfully.
🧹 Nitpick comments (12)
src/examples/CMakeLists.txt (1)
24-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated TSan-target boilerplate into one CMake function.
The eight TSan target blocks (
producer-consumer-tsan,producer-consumer-safe-tsan,producer-consumer-exit-tsan,producer-consumer-park-tsan,exit-stress-tsan,exit-stress-noop-tsan,cv-producer-consumer-tsan,cv-producer-consumer-safe-tsan) repeat the same five steps:add_executable,target_compile_options(... -fsanitize=thread),target_link_options(... -fsanitize=thread [+ --wrap flags]),target_link_libraries(... -pthread libmcmini), and theadd_custom_command(POST_BUILD copy ...). Each new TSan example requires copying all five steps and keeping them in sync by hand.Extract a function that takes the target name, source list, and wrap symbols, and generates all five steps once.
♻️ Proposed refactor
function(add_tsan_example name) cmake_parse_arguments(ARG "" "" "SOURCES;WRAP" ${ARGN}) add_executable(${name} ${ARG_SOURCES}) target_compile_options(${name} PUBLIC -fsanitize=thread) set(wrap_flags "") foreach(sym ${ARG_WRAP}) list(APPEND wrap_flags -Wl,--wrap=${sym}) endforeach() target_link_options(${name} PUBLIC -fsanitize=thread ${wrap_flags}) target_link_libraries(${name} PUBLIC -pthread libmcmini) add_custom_command(TARGET ${name} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:${name}> ${CMAKE_BINARY_DIR}/${name}) endfunction() add_tsan_example(producer-consumer-tsan SOURCES producer-consumer.c ../lib/pthread_join_wrap.c WRAP pthread_join) add_tsan_example(cv-producer-consumer-tsan SOURCES cv-producer-consumer.c ../lib/pthread_join_wrap.c ../lib/pthread_cond_wait_wrap.c ../lib/pthread_cond_signal_wrap.c WRAP pthread_join pthread_cond_wait pthread_cond_signal) add_tsan_example(producer-consumer-park-tsan SOURCES producer-consumer-park.c) # ... remaining targets follow the same pattern🤖 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/examples/CMakeLists.txt` around lines 24 - 168, Extract the repeated setup from the eight TSan targets into a shared CMake function, such as add_tsan_example, accepting the target name, source files, and pthread wrap symbols. Have it generate the executable, ThreadSanitizer compile/link options, wrap flags, libmcmini linkage, and POST_BUILD copy command; then replace each producer-consumer, exit-stress, and cv-producer-consumer block with function calls while preserving each target’s existing sources and wrap symbols.src/lib/tsan_support.c (2)
30-31: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTwo small cleanups in
thread_blocks_signal.
- Line 31 discards
signowith(void)signo, but line 89 usessigno. Remove the stale cast.- The read loop stops on any negative return, including
EINTR. A partial mask read then yields a wrong answer instead of a retry. Retry onEINTR.♻️ Proposed cleanup
int thread_blocks_signal(pid_t tid, int signo) { - (void)signo; char path[64]; @@ while (total < (ssize_t)sizeof(buf) - 1 && - (n = syscall(SYS_read, fd, buf + total, sizeof(buf) - 1 - total)) > 0) { - total += n; + ((n = syscall(SYS_read, fd, buf + total, sizeof(buf) - 1 - total)) > 0 || + (n < 0 && errno == EINTR))) { + if (n > 0) total += n; }Also applies to: 56-59
🤖 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/tsan_support.c` around lines 30 - 31, Update thread_blocks_signal by removing the stale (void)signo cast, since signo is used later, and adjust the signal-mask read loop to retry when the read operation returns EINTR while preserving the existing handling for other negative returns and partial reads.
92-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace the readlink/
atoipath with a directgettidsyscall.
mc_real_tid()reads/proc/thread-self, then parses the result withatoi.atoiperforms no error detection and has undefined behavior on out-of-range values.syscall(SYS_gettid)returns the same kernel thread ID with one raw syscall, no string parsing, and no procfs dependency. It is also interceptor-free, which is the property this file needs.src/lib/wrappers.cline 832 already usessyscall(SYS_gettid)for the same purpose.♻️ Proposed simplification
pid_t mc_real_tid(void) { - char linkbuf[64]; - ssize_t n = syscall(SYS_readlink, "/proc/thread-self", linkbuf, - sizeof(linkbuf) - 1); - if (n < 0) { - return -1; - } - linkbuf[n] = '\0'; - const char *task = strstr(linkbuf, "/task/"); - if (task == NULL) { - return -1; - } - return (pid_t)atoi(task + strlen("/task/")); + return (pid_t)syscall(SYS_gettid); }🤖 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/tsan_support.c` around lines 92 - 105, Replace the procfs readlink and atoi logic in mc_real_tid() with a direct syscall(SYS_gettid) return, matching the existing usage in wrappers.c and preserving the function’s pid_t return type.Source: Linters/SAST tools
src/common/mem.c (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the requested size, and stop the offset from wrapping past the arena.
Two small robustness points on the exhaustion path:
mc_ts_offkeeps increasing on every failed call. With repeated large requests the counter can wrap and let a later call pass the bounds check and return an in-range but already-used pointer. The current path callsexit_group, so this is not reachable today. It becomes reachable if the failure policy ever changes.- The message does not report the requested size or the arena size, which makes a real exhaustion hard to diagnose.
Consider a compare-exchange loop that leaves
mc_ts_offunchanged on 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/common/mem.c` around lines 16 - 23, The exhaustion handling in mc_ts_alloc must avoid advancing mc_ts_off when a request exceeds the arena and include both the requested size and MC_TS_ARENA_SIZE in the raw-syscall diagnostic. Update the offset reservation logic to use a compare-exchange loop that commits the new offset only when it fits, leaving mc_ts_off unchanged on failure while preserving the interceptor-free exit path.include/dmtcp.h (1)
506-523: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard DMTCP API names from macro capture.
dmtcp_tsan_background_thread_virtual_tid()anddmtcp_skip_post_restart_checkpoint_loop()expand to guarded calls, but any definition site that includesinclude/dmtcp.hstill has those names replaced. Add#undefbefore defining them, or use non-macro names for macros so the definitions keep the exported function names intact.🤖 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 `@include/dmtcp.h` around lines 506 - 523, Prevent the fallback macros for dmtcp_tsan_background_thread_virtual_tid and dmtcp_skip_post_restart_checkpoint_loop from rewriting the corresponding exported function declarations or definitions. Undefine each function-name macro before its declaration/definition, or rename the macro wrappers while preserving the public API names and guarded-call behavior.include/mcmini/spy/checkpointing/tsan_support.h (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
#includedirectives above theextern "C"block.The header opens
extern "C"at line 4 and then includes<stdbool.h>and<sys/types.h>at lines 7-8. Any declaration those headers pull in then gets C language linkage. System headers usually guard themselves, but the portable convention is to include first and then open the linkage block.♻️ Proposed reordering
`#pragma` once +#include <stdbool.h> +#include <sys/types.h> + `#ifdef` __cplusplus extern "C" { `#endif` - -#include <stdbool.h> -#include <sys/types.h>🤖 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 `@include/mcmini/spy/checkpointing/tsan_support.h` around lines 3 - 8, Move the <stdbool.h> and <sys/types.h> include directives before the __cplusplus extern "C" block in the checkpointing header, leaving only this header’s declarations inside the C-linkage wrapper.src/lib/dmtcp-callback.c (2)
226-229: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCheck the
libc_clone()return value.If
libc_clone()fails, the thread is never recreated. The template thread then waits ondmtcp_restart_semfor a thread that does not exist and the barrier at lines 421-423 blocks forever. Fail loudly instead.♻️ Proposed error check
- libc_clone(child_setcontext_fast, - stack, - clone_flags, - (void *)&threadInfos[i], ptid, (void *)threadInfos[i].fs, ctid); + int clone_rc = libc_clone(child_setcontext_fast, + stack, + clone_flags, + (void *)&threadInfos[i], ptid, + (void *)threadInfos[i].fs, ctid); + if (clone_rc == -1) { + // A missing thread makes the restart barrier block forever. + libc_abort(); + }🤖 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/dmtcp-callback.c` around lines 226 - 229, Check the return value of libc_clone() in the thread recreation path and fail loudly when it indicates failure, rather than continuing to wait on dmtcp_restart_sem. Preserve the existing successful clone flow and ensure the failure path prevents the barrier from blocking indefinitely.
272-274: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
__tsan_create_fiberas well as__tsan_switch_to_fiber.Both sites test only
__tsan_switch_to_fiberand then call the weak__tsan_create_fiberunconditionally. If only one symbol resolves, the call dereferences a null function pointer. Test both symbols, and also reject a null fiber.🛡️ Proposed guard (apply at both sites)
- if (__tsan_switch_to_fiber != NULL) { - __tsan_switch_to_fiber(__tsan_create_fiber(0), 0); - } + if (__tsan_switch_to_fiber != NULL && __tsan_create_fiber != NULL) { + void *fiber = __tsan_create_fiber(0); + if (fiber != NULL) __tsan_switch_to_fiber(fiber, 0); + }Also applies to: 329-331
🤖 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/dmtcp-callback.c` around lines 272 - 274, Update both TSan fiber-switch call sites in the callback logic to require __tsan_switch_to_fiber and __tsan_create_fiber to be non-null before invoking either; store the result of __tsan_create_fiber(0), verify the fiber is non-null, and only then call __tsan_switch_to_fiber.src/mcmini/mcmini.cpp (1)
182-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared trace-reporting body.
found_nonzero_exit_codeduplicatesfound_abnormal_terminationfrom thestd::stringstreamonward. Only the firststd::cerrline and the error type differ. Extract a helper that takes the coordinator, the stats, and the culpritrunner_id_t, then call it from both callbacks.♻️ Sketch of the shared helper
static void report_trace_and_pending(const coordinator& c, const stats& stats, runner_id_t culprit) { std::stringstream ss; const auto& program_model = c.get_current_program_model(); ss << "TRACE " << stats.trace_id << "\n"; for (const auto& t : program_model.get_trace()) { ss << "thread " << t->get_executor() << ": " << t->to_string() << "\n"; } const transition* culprit_transition = program_model.get_pending_transition_for(culprit); if (culprit_transition != nullptr) { ss << "thread " << culprit_transition->get_executor() << ": " << culprit_transition->to_string() << "\n"; } else { ss << "thread " << culprit << ": (no longer pending)\n"; } ss << "\nNEXT THREAD OPERATIONS\n"; for (const auto& tpair : program_model.get_pending_transitions()) { if (culprit_transition != nullptr && tpair.first == culprit_transition->get_executor()) { ss << "thread " << tpair.first << ": executing\n"; } else { ss << "thread " << tpair.first << ": " << tpair.second->to_string() << "\n"; } } ss << stats.total_transitions + 1 << " total transitions executed\n"; std::cout << ss.str(); std::cout.flush(); }🤖 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/mcmini/mcmini.cpp` around lines 182 - 221, Extract the duplicated trace and pending-transition reporting logic from found_nonzero_exit_code and found_abnormal_termination into a shared helper accepting const coordinator&, const stats&, and runner_id_t culprit. Replace both callbacks’ duplicated bodies with calls to that helper, while preserving their distinct std::cerr error messages and passing nzec.culprit or the corresponding abnormal-termination culprit.include/mcmini/model/objects/condition_variables.hpp (1)
57-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCombine the two passes over
thread_states.The first loop registers prewaiting and waiting threads. The second loop collects signaled threads. One pass can do both.
♻️ Proposed refactor
- // Initialize the policy according to the states of the threads in waiting queue - for (const auto& thread_with_state : thread_states) { - if (thread_with_state.second == CV_PREWAITING || thread_with_state.second == CV_WAITING) { - this->policy->add_waiter_with_state(thread_with_state.first,thread_with_state.second); - } - } std::vector<runner_id_t> signaled_threads; + // Initialize the policy according to the states of the threads in waiting queue for (const auto& thread_with_state : thread_states) { - if (thread_with_state.second == CV_SIGNALED) { + if (thread_with_state.second == CV_PREWAITING || + thread_with_state.second == CV_WAITING) { + this->policy->add_waiter_with_state(thread_with_state.first, + thread_with_state.second); + } else if (thread_with_state.second == CV_SIGNALED) { signaled_threads.push_back(thread_with_state.first); } }🤖 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 `@include/mcmini/model/objects/condition_variables.hpp` around lines 57 - 73, Combine the two iterations over thread_states in the policy initialization logic: keep registering CV_PREWAITING and CV_WAITING entries through add_waiter_with_state, while collecting CV_SIGNALED thread IDs for the existing add_to_wake_groups call after the single pass.src/lib/interception.c (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the ignored
__noreturn__attribute on the pointer object.GCC and Clang ignore
__attribute__((__noreturn__))on an object declaration. The attribute belongs to the type, andtypeof(&pthread_exit)already carries glibc'snoreturnattribute. Clang also reports a parse error on this line.♻️ Proposed change
-__attribute__((__noreturn__)) typeof(&pthread_exit) libpthread_pthread_exit_ptr; +typeof(&pthread_exit) libpthread_pthread_exit_ptr;🤖 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/interception.c` at line 29, Update the libpthread_pthread_exit_ptr declaration to remove the object-level __attribute__((__noreturn__)) annotation, retaining typeof(&pthread_exit) as the pointer type.Source: Linters/SAST tools
src/mcmini/real_world/local_linux_process.cpp (1)
141-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState the
_exit(0)case in the code-0 message.A target that calls
_exit(0)directly bypasses the exit interposition, as noted insrc/lib/interception.c. That target reaches this branch and reports a McMini protocol violation, which misleads the user. Add the alternative cause to the message.♻️ Proposed change
throw process::execution_error( "Runner " + std::to_string(id) + "'s process exited normally (code 0) while a transition was " - "still pending on it, bypassing the model-driven exit protocol."); + "still pending on it, bypassing the model-driven exit protocol. " + "This happens if the target calls _exit(2) directly, which McMini " + "cannot intercept, or if McMini's own exit protocol was violated.");🤖 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/mcmini/real_world/local_linux_process.cpp` around lines 141 - 154, Update the code-0 execution_error message in the SIGCHLD handling branch to explicitly include direct target calls to _exit(0) as an alternative cause, alongside bypassing the model-driven exit protocol. Keep the existing distinction from nonzero target exits and preserve the current error behavior.
🤖 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 `@doc/cond-wait-tsan-interceptor-bypass.txt`:
- Around line 59-72: Fix the checkpoint/restart reconstruction path so
condition-variable waiters are restored with the correct
CV_PREWAITING/CV_WAITING state and waiter queue, while preserving the consumer’s
mutex ownership until its wait transition resumes. Ensure the reconstructed
state enables both the producer’s pthread_mutex_lock and
condition_variables_wait.hpp modify() consumer-resume transition, eliminating
the immediate DEADLOCK after restart.
In `@include/mcmini/model/objects/condition_variables.hpp`:
- Around line 27-56: Update the condition_variable data members to provide safe
default member initializers, at minimum setting policy to nullptr and hadwaiters
to false; also initialize running_thread to RID_INVALID and associated_mutex to
nullptr to match the default-constructor contract. Preserve the parameterized
constructor’s policy allocation behavior for non-null policy arguments.
- Around line 38-56: The policy member variable is currently a raw owning
pointer that causes memory leaks (allocated in the parameterized constructor but
never freed) and unintended aliasing via the defaulted copy constructor. Replace
the raw pointer policy member with std::unique_ptr<ConditionVariablePolicy> to
enable automatic cleanup. Remove the defaulted copy constructor and implement an
explicit copy constructor that deep-copies the policy by creating a new
ConditionVariablePolicy instance for the copied object. Update the parameterized
constructor to initialize the unique_ptr correctly, allocating a fresh policy
when p is nullptr or wrapping the provided policy pointer.
In
`@include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp`:
- Around line 54-55: Condition-variable transitions leak the default policy by
replacing it after construction. In
include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp
lines 54-55, condition_variable_brdcast.hpp lines 52-78, and
condition_variables_wait.hpp lines 59-60, pass new_policy as the constructor’s
sixth argument and remove set_policy. In condition_variables_signal.hpp lines
92-98, pass cv->get_mutex() and new_policy to the constructor, then remove both
set_policy and set_associated_mutex.
- Around line 44-52: Update the enqueue path around
ConditionVariablePolicy::add_waiter_with_state to detect and remove or replace
any existing executor waiter before adding its CV_WAITING entry, especially when
the current state is CV_PREWAITING. Ensure the queue contains at most one entry
for executor so new_waiting_count and cv_waiting_count remain accurate.
In
`@include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp`:
- Around line 100-101: Update the signal lost-wakeup handling to call
check_for_lost_wakeup(true, prev_waiting_count) on new_cv, which owns
new_policy, instead of the temporary mutable_cv. Remove the throwaway
condition_variable construction so no empty policy is allocated or leaked, while
preserving the existing state and waiting-count updates.
- Around line 92-98: Update the condition_variable constructor call to pass the
associated mutex and policy directly as constructor parameters instead of using
nullptr and then calling set_associated_mutex() and set_policy() separately.
Replace the nullptr argument with cv->get_mutex() and add new_policy as the
appropriate constructor parameter, then remove the subsequent set_policy() and
set_associated_mutex() calls to eliminate the extra steps and prevent leaking
the policy allocated by the constructor.
In `@src/common/mem.c`:
- Around line 9-24: Declare the static arena used by mc_ts_alloc with an
explicit 16-byte alignment, preserving its existing size and allocation logic.
Update the mc_ts_arena declaration so every returned address remains suitably
aligned for the objects allocated by this function.
In `@src/lib/dmtcp-callback.c`:
- Around line 411-416: Protect the traversal of head_record_mode in the
thread-counting logic with rec_list_lock. Update the surrounding function so the
lock is acquired before iterating through rec_list entries and released after
the count is complete, while preserving the existing THREAD and ALIVE filtering
and subsequent barrier behavior.
- Around line 195-204: The mc_pthread_is_recreated_thread function reads
threadInfos entries without acquire semantics, creating a race where it observes
maxThreadIdx incremented before the entry is fully written. Add an atomic
published field (or convert origTid to atomic) in struct threadinfo, set it last
with memory_order_release in saveThreadStateBeforeFork after all fields are
populated, and check it here with memory_order_acquire instead of testing
origTid != 0 to establish the release/acquire pairing between
thread_handle_after_dmtcp_restart's writer and this reader.
In `@src/lib/interception.c`:
- Around line 15-21: Update libmcmini_init_done and its accesses in
libmcmini_init to use atomic acquire/release builtins: publish the flag with
release ordering only after mc_load_intercepted_pthread_functions() completes,
and read it with acquire ordering on the pthread-free fast path. Preserve the
existing plain-memory path without introducing pthread_once or TSAN-intercepted
operations.
In `@src/lib/pthread_cond_signal_wrap.c`:
- Around line 1-19: Add the matching pthread_cond_broadcast wrapper alongside
__wrap_pthread_cond_signal, forwarding to mc_pthread_cond_broadcast; compile it
into the target and add -Wl,--wrap=pthread_cond_broadcast to the TSan target
link options. Update doc/cond-wait-tsan-interceptor-bypass.txt to state that
TSan bypasses interposition for broadcast as well as signal.
In `@src/lib/tsan_support.c`:
- Around line 48-66: Replace the procfs open/read logic in
mc_is_current_thread_tsan_internal() with syscall(SYS_rt_sigprocmask, SIG_BLOCK,
NULL, &set, sizeof(set)) for current-thread checks, while retaining the procfs
path only for explicit arbitrary-tid queries. In src/lib/record.c lines 186-208,
remove the tsan_internal_check_applies mode gate and invoke
mc_is_current_thread_tsan_internal() for every mode, as required by the record.h
contract.
- Line 48: Add a feature-test macro enabling POSIX.1-2008 or GNU extensions at
the very top of the tsan_support.c translation unit, before every include, so
<fcntl.h> exposes AT_FDCWD for the syscall(SYS_openat, ...) call. Preserve the
existing openat behavior.
In `@src/lib/wrappers.c`:
- Around line 787-807: Update the call site around mark_ckpt_window_candidate to
pass the thread’s DMTCP virtual thread ID rather than the real kernel ID
returned by syscall(SYS_gettid), using mcmini_virtual_pid() for the translation.
Preserve resolve_ckpt_window_candidate_if_pending() and its virtual-ID
comparison unchanged.
- Around line 796-803: Update the TSan background-thread lookup in
get_current_mode to use a bounded retry/timeout and an explicit failure path
when dmtcp_tsan_background_thread_virtual_tid remains unavailable, rather than
spinning indefinitely. Distinguish the guard-macro result of 0 from valid
virtual thread IDs, report that the symbol is absent, and do not call
record_checkpoint_thread in that case. Preserve checkpoint-thread recording only
when a valid published ID differs from ckpt_window_candidate_virtual_tid.
- Around line 86-97: The insert_pthread_map function calls get_current_mode()
while holding pthread_map_lock, which blocks other threads trying to access the
map during the unbounded spin inside get_current_mode(). Move the
get_current_mode() call before libpthread_mutex_lock is acquired, store the
result in a local variable, and then use that variable to set
n->registered_post_restart after creating the node. This ensures the lock
protects only the actual map manipulation, not the mode resolution.
- Around line 1447-1454: Guard the find_thread_record_mode result in the
condition-variable initialization path before accessing
thrd_record->vo.thrd_state.id, using a safe fallback identity or a specific
abort when no record exists. Apply the same protection in
mc_pthread_cond_broadcast and preserve the existing behavior when a thread
record is present.
- Around line 458-474: Update the exit-permission wait in the thread termination
path to retry libpthread_sem_wait(exit_permission) when it returns -1 with errno
== EINTR, and only continue after the semaphore wait succeeds. Apply the same
EINTR retry behavior to the really_exited semaphore wait in
mc_pthread_join_impl, preserving existing handling for successful waits and
other errors.
In `@src/mcmini/real_world/local_linux_process.cpp`:
- Around line 76-78: In src/mcmini/real_world/local_linux_process.cpp lines
76-78, update the destructor’s waitpid drain to reap only child PIDs spawned by
this handle, or use the handle’s process group, instead of waitpid(-1, ...). In
lines 130-134, update the execute_runner waitpid error handling so errno ==
ECHILD is reported as the child status already being consumed, separately from
genuine waitpid failures.
---
Minor comments:
In `@doc/glibc-sem-desync.txt`:
- Around line 88-93: Update the condition-variable section in
glibc-sem-desync.txt to remove the stale “Not yet fixed” claim, reference
glibc-cond-var-desync.txt as documenting the applied fix, and retain only the
limitations that still remain.
In `@include/mcmini/model/objects/mutex.hpp`:
- Around line 24-33: Fix the default construction path in the mutex class by
removing mutex() = default if it is unused, or initialize location and owner to
safe values through default member initializers. Ensure every mutex instance has
a valid location state and RID_INVALID ownership before get_location(),
mutex_lock, or mutex_unlock can use it, while preserving the explicit
constructor’s behavior.
In `@include/mcmini/spy/checkpointing/record.h`:
- Around line 198-202: Update the stale documentation references associated with
add_rec_entry_record_mode_ts and the related TSAN pthread-join references to
point to the actual documents present in doc/, or add the missing documents
using stable names; ensure no references remain to nonexistent
TSAN-McMini-DMTCP.txt or TSAN-pthread-join.md files.
In `@src/common/multithreaded_fork.c`:
- Around line 181-183: Virtualize the ctid value obtained from Sys_gettid()
using dmtcp_pid_real_to_virtual() before the tids[i] != mytid comparison in the
multithreaded_fork function, ensuring that the real thread ID from Sys_gettid()
is converted to a virtual TID to match the virtual identifiers already present
in the tids array from /proc/self/task before the comparison is performed.
In `@src/examples/exit-stress.c`:
- Around line 27-29: Replace the single sleep(10) delay in the exit-stress
scenario with a monotonic deadline loop, following the existing worker-delay
busy-wait pattern, so pre-checkpoint signals cannot shorten the window before
workers exit. Preserve the ten-second delay and ensure the loop continues until
the deadline is reached.
In `@src/examples/producer-consumer-park.c`:
- Line 54: Update sem_wait_retry so it loops only when sem_wait returns -1 with
errno equal to EINTR; for any other failure, abort immediately and preserve the
semaphore error visibility instead of retrying indefinitely.
In `@src/lib/interception.c`:
- Around line 392-400: Update __libc_start_main to validate the result of dlsym
before invoking real_start_main; when resolution fails, report the failure
explicitly and terminate with _exit(2), avoiding libc_exit because
initialization is incomplete. Preserve the existing delegation to wrapped_main
when the symbol resolves successfully.
- Line 125: Validate the result assigned to clone_ptr immediately after
resolving "__clone" with dlsym in libc_clone(); if it is NULL, emit a diagnostic
and call libc_abort() before any indirect invocation. Preserve the existing
libc_clone restart flow when clone_ptr resolves successfully.
In `@src/lib/main.c`:
- Around line 30-39: Remove the __wrap_pthread_join probe and warning from
warn_if_tsan_target_missing_wrap, since libmcmini.so cannot reliably observe the
executable’s linker wrapper. Use an explicit registration call from
pthread_join_wrap.c instead, or otherwise ensure TSan targets export the wrapper
before retaining the lookup.
In `@src/lib/wrappers.c`:
- Around line 232-241: In the successful initialization branch after
libpthread_mutex_init returns zero, update the existing mutex_record state so
owner is reset to RID_INVALID alongside status = UNLOCKED. Apply this to both
newly created and already-known mutex records, using mutex_record->vo.mut_state.
In `@test/tsan_support/test_thread_blocks_signal.c`:
- Around line 49-51: Replace the hard-coded bogus task ID in the
thread_blocks_signal() invalid-ID assertion with -1, preserving the expected
return value of 0 and ensuring the test always targets a nonexistent task status
path.
---
Nitpick comments:
In `@include/dmtcp.h`:
- Around line 506-523: Prevent the fallback macros for
dmtcp_tsan_background_thread_virtual_tid and
dmtcp_skip_post_restart_checkpoint_loop from rewriting the corresponding
exported function declarations or definitions. Undefine each function-name macro
before its declaration/definition, or rename the macro wrappers while preserving
the public API names and guarded-call behavior.
In `@include/mcmini/model/objects/condition_variables.hpp`:
- Around line 57-73: Combine the two iterations over thread_states in the policy
initialization logic: keep registering CV_PREWAITING and CV_WAITING entries
through add_waiter_with_state, while collecting CV_SIGNALED thread IDs for the
existing add_to_wake_groups call after the single pass.
In `@include/mcmini/spy/checkpointing/tsan_support.h`:
- Around line 3-8: Move the <stdbool.h> and <sys/types.h> include directives
before the __cplusplus extern "C" block in the checkpointing header, leaving
only this header’s declarations inside the C-linkage wrapper.
In `@src/common/mem.c`:
- Around line 16-23: The exhaustion handling in mc_ts_alloc must avoid advancing
mc_ts_off when a request exceeds the arena and include both the requested size
and MC_TS_ARENA_SIZE in the raw-syscall diagnostic. Update the offset
reservation logic to use a compare-exchange loop that commits the new offset
only when it fits, leaving mc_ts_off unchanged on failure while preserving the
interceptor-free exit path.
In `@src/examples/CMakeLists.txt`:
- Around line 24-168: Extract the repeated setup from the eight TSan targets
into a shared CMake function, such as add_tsan_example, accepting the target
name, source files, and pthread wrap symbols. Have it generate the executable,
ThreadSanitizer compile/link options, wrap flags, libmcmini linkage, and
POST_BUILD copy command; then replace each producer-consumer, exit-stress, and
cv-producer-consumer block with function calls while preserving each target’s
existing sources and wrap symbols.
In `@src/lib/dmtcp-callback.c`:
- Around line 226-229: Check the return value of libc_clone() in the thread
recreation path and fail loudly when it indicates failure, rather than
continuing to wait on dmtcp_restart_sem. Preserve the existing successful clone
flow and ensure the failure path prevents the barrier from blocking
indefinitely.
- Around line 272-274: Update both TSan fiber-switch call sites in the callback
logic to require __tsan_switch_to_fiber and __tsan_create_fiber to be non-null
before invoking either; store the result of __tsan_create_fiber(0), verify the
fiber is non-null, and only then call __tsan_switch_to_fiber.
In `@src/lib/interception.c`:
- Line 29: Update the libpthread_pthread_exit_ptr declaration to remove the
object-level __attribute__((__noreturn__)) annotation, retaining
typeof(&pthread_exit) as the pointer type.
In `@src/lib/tsan_support.c`:
- Around line 30-31: Update thread_blocks_signal by removing the stale
(void)signo cast, since signo is used later, and adjust the signal-mask read
loop to retry when the read operation returns EINTR while preserving the
existing handling for other negative returns and partial reads.
- Around line 92-105: Replace the procfs readlink and atoi logic in
mc_real_tid() with a direct syscall(SYS_gettid) return, matching the existing
usage in wrappers.c and preserving the function’s pid_t return type.
In `@src/mcmini/mcmini.cpp`:
- Around line 182-221: Extract the duplicated trace and pending-transition
reporting logic from found_nonzero_exit_code and found_abnormal_termination into
a shared helper accepting const coordinator&, const stats&, and runner_id_t
culprit. Replace both callbacks’ duplicated bodies with calls to that helper,
while preserving their distinct std::cerr error messages and passing
nzec.culprit or the corresponding abnormal-termination culprit.
In `@src/mcmini/real_world/local_linux_process.cpp`:
- Around line 141-154: Update the code-0 execution_error message in the SIGCHLD
handling branch to explicitly include direct target calls to _exit(0) as an
alternative cause, alongside bypassing the model-driven exit protocol. Keep the
existing distinction from nonzero target exits and preserve the current error
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c606018-fc24-4e4f-b520-057e65ebf39f
📒 Files selected for processing (56)
CMakeLists.txtdoc/classic-mode-thread-registration-segv.txtdoc/cond-wait-tsan-interceptor-bypass.txtdoc/glibc-cond-var-desync.txtdoc/glibc-sem-desync.txtdoc/log-mutex-fork-desync.txtdoc/pthread-exit-abort-and-fiber-crash.txtdoc/tsan-mutex-annotation-false-positive.txtinclude/dmtcp.hinclude/mcmini/lib/log.hinclude/mcmini/mem.hinclude/mcmini/model/objects/condition_variables.hppinclude/mcmini/model/objects/mutex.hppinclude/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hppinclude/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hppinclude/mcmini/model/transitions/condition_variables/condition_variables_signal.hppinclude/mcmini/model/transitions/condition_variables/condition_variables_wait.hppinclude/mcmini/model/transitions/mutex/mutex_init.hppinclude/mcmini/model/transitions/mutex/mutex_unlock.hppinclude/mcmini/model/transitions/process/exit.hppinclude/mcmini/real_world/mailbox/runner_mailbox.hinclude/mcmini/real_world/process/dmtcp_process_source.hppinclude/mcmini/spy/checkpointing/objects.hinclude/mcmini/spy/checkpointing/record.hinclude/mcmini/spy/checkpointing/tsan_support.hinclude/mcmini/spy/intercept/interception.hinclude/mcmini/spy/intercept/wrappers.hsrc/common/mem.csrc/common/multithreaded_fork.csrc/common/runner_mailbox.csrc/examples/CMakeLists.txtsrc/examples/cv-producer-consumer-safe.csrc/examples/cv-producer-consumer.csrc/examples/exit-stress-noop.csrc/examples/exit-stress.csrc/examples/producer-consumer-exit.csrc/examples/producer-consumer-park.csrc/examples/producer-consumer-safe.csrc/lib/dmtcp-callback.csrc/lib/interception.csrc/lib/log.csrc/lib/main.csrc/lib/pthread_cond_signal_wrap.csrc/lib/pthread_cond_wait_wrap.csrc/lib/pthread_join_wrap.csrc/lib/record.csrc/lib/sem-wrappers.csrc/lib/tsan_support.csrc/lib/wrappers.csrc/mcmini/mcmini.cppsrc/mcmini/model/transitions/condition_variables.cppsrc/mcmini/model/transitions/mutex.cppsrc/mcmini/model_checking/algorithms/classic_dpor.cppsrc/mcmini/real_world/dmtcp_process_source.cppsrc/mcmini/real_world/local_linux_process.cpptest/tsan_support/test_thread_blocks_signal.c
💤 Files with no reviewable changes (1)
- src/mcmini/real_world/dmtcp_process_source.cpp
| Follow-on issue (separate from this fix) | ||
| ----------------------------------------- | ||
| That same verification run surfaced a second, previously-unreachable bug: | ||
| once the consumer's cond_wait is actually visible to the model, the | ||
| restart reports an immediate DEADLOCK. The reconstructed state shows the | ||
| mutex still "locked" by the consumer and the condition_variable still at | ||
| cv_initialized (never advanced to cv_waiting), so neither the producer's | ||
| pthread_mutex_lock nor the consumer's own resume-from-wait transition | ||
| (condition_variables_wait.hpp's modify()) can ever become enabled. This | ||
| looks like a bug in how the checkpoint/restart path reconstructs CV | ||
| waiter-queue state (the CV_PREWAITING/CV_WAITING bookkeeping described in | ||
| Cond_Var_Readme.md) rather than anything to do with the TSan-bypass fix | ||
| above, since this code path was never reachable before this fix landed. | ||
| Tracked as a separate, follow-on investigation. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the restored condition-variable state before merge.
Line 61 reports an immediate deadlock after the bypass succeeds. The restored mutex remains owned, and the condition variable never enters cv_waiting. A DMTCP+TSan target checkpointed in pthread_cond_wait() cannot restart successfully.
Restore the waiter queue and mutex ownership so the producer lock and consumer resume transitions become enabled.
🤖 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 `@doc/cond-wait-tsan-interceptor-bypass.txt` around lines 59 - 72, Fix the
checkpoint/restart reconstruction path so condition-variable waiters are
restored with the correct CV_PREWAITING/CV_WAITING state and waiter queue, while
preserving the consumer’s mutex ownership until its wait transition resumes.
Ensure the reconstructed state enables both the producer’s pthread_mutex_lock
and condition_variables_wait.hpp modify() consumer-resume transition,
eliminating the immediate DEADLOCK after restart.
| state current_state = state::cv_uninitialized; | ||
| bool hadwaiters; | ||
| mutable unsigned int numRemainingSpuriousWakeups = 0; | ||
| runner_id_t running_thread; | ||
| pthread_mutex_t* associated_mutex; | ||
| int waiting_count = 0; | ||
| int prev_waiting_count = 0; | ||
| int lost_wakeups = 0; | ||
| ConditionVariablePolicy* policy = new ConditionVariableArbitraryPolicy(); | ||
| ConditionVariablePolicy* policy; | ||
|
|
||
| public: | ||
| condition_variable() = default; | ||
| ~condition_variable() = default; | ||
| condition_variable(const condition_variable &) = default; | ||
| condition_variable(state s) : current_state(s) {} | ||
| condition_variable(state s, ConditionVariablePolicy* p) : current_state(s), policy(p) {} | ||
| condition_variable(state s, int count) : current_state(s) {} | ||
| condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count){} | ||
|
|
||
| condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count, | ||
| const std::vector<std::pair<runner_id_t, condition_variable_status>>& thread_states) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count) { | ||
| // Initialize the policy according to the states of the threads in waiting queue | ||
| for (const auto& thread_with_state : thread_states) { | ||
| if (thread_with_state.second == CV_PREWAITING || thread_with_state.second == CV_WAITING) { | ||
| this->policy->add_waiter_with_state(thread_with_state.first,thread_with_state.second); | ||
| } | ||
| } | ||
| std::vector<runner_id_t> signaled_threads; | ||
| for (const auto& thread_with_state : thread_states) { | ||
| if (thread_with_state.second == CV_SIGNALED) { | ||
| signaled_threads.push_back(thread_with_state.first); | ||
| } | ||
| } | ||
| if (!signaled_threads.empty()) { | ||
| // If there are any threads that have been signaled, we should | ||
| // add them to the wake groups in the policy. | ||
| this->policy->add_to_wake_groups(signaled_threads); | ||
| } | ||
| } | ||
| // The only non-copy constructor: `tid`/`mutex` get sentinel defaults | ||
| // (RID_INVALID/nullptr) rather than being left uninitialized (there was | ||
| // previously no default member initializer for either, unlike `waiting_count`) | ||
| // -- e.g. mutex(state, location) needed the identical fix for `location` | ||
| // after commit 9bd9ecf. `p` (policy) defaults to nullptr meaning "create a | ||
| // fresh one"; every transition that advances an *existing* condition | ||
| // variable's state must still pass its own (possibly cloned) policy | ||
| // explicitly here or via set_policy() afterward -- see that setter's | ||
| // comment for why an omitted policy silently discards in-progress waiter | ||
| // tracking rather than just being harmlessly empty. | ||
| condition_variable(state s, runner_id_t tid = RID_INVALID, | ||
| pthread_mutex_t* mutex = nullptr, int count = 0, | ||
| const std::vector<std::pair<runner_id_t, condition_variable_status>>& thread_states = {}, | ||
| ConditionVariablePolicy* p = nullptr) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count), | ||
| policy(p ? p : new ConditionVariableArbitraryPolicy()) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Give policy and hadwaiters default member initializers.
The policy field lost its default initializer, but line 38 still declares a public defaulted default constructor. condition_variable() therefore leaves policy indeterminate. Every accessor that dereferences it (has_waiters(), get_policy(), waiter_can_exit(), remove_waiter()) then reads an invalid pointer. hadwaiters, running_thread, and associated_mutex have the same gap.
Set policy = nullptr at minimum, so the failure is a null dereference and not an arbitrary one.
🛡️ Proposed fix
state current_state = state::cv_uninitialized;
- bool hadwaiters;
+ bool hadwaiters = false;
mutable unsigned int numRemainingSpuriousWakeups = 0;
- runner_id_t running_thread;
- pthread_mutex_t* associated_mutex;
+ runner_id_t running_thread = RID_INVALID;
+ pthread_mutex_t* associated_mutex = nullptr;
int waiting_count = 0;
int prev_waiting_count = 0;
int lost_wakeups = 0;
- ConditionVariablePolicy* policy;
+ ConditionVariablePolicy* policy = nullptr;Run the following script to check whether the default constructor is reachable:
#!/bin/bash
# Find default-constructed condition_variable objects.
rg -nP --type=cpp --type=cc -C3 '\bcondition_variable\s+\w+\s*(;|\{\s*\})' -g '!**/build/**' || true
ast-grep run --pattern 'new condition_variable()' --lang cpp .
ast-grep outline include/mcmini/model/objects/condition_variables.hpp --items all🤖 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 `@include/mcmini/model/objects/condition_variables.hpp` around lines 27 - 56,
Update the condition_variable data members to provide safe default member
initializers, at minimum setting policy to nullptr and hadwaiters to false; also
initialize running_thread to RID_INVALID and associated_mutex to nullptr to
match the default-constructor contract. Preserve the parameterized constructor’s
policy allocation behavior for non-null policy arguments.
Source: Linters/SAST tools
| condition_variable() = default; | ||
| ~condition_variable() = default; | ||
| condition_variable(const condition_variable &) = default; | ||
| condition_variable(state s) : current_state(s) {} | ||
| condition_variable(state s, ConditionVariablePolicy* p) : current_state(s), policy(p) {} | ||
| condition_variable(state s, int count) : current_state(s) {} | ||
| condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count){} | ||
|
|
||
| condition_variable(state s, runner_id_t tid, pthread_mutex_t* mutex, int count, | ||
| const std::vector<std::pair<runner_id_t, condition_variable_status>>& thread_states) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count) { | ||
| // Initialize the policy according to the states of the threads in waiting queue | ||
| for (const auto& thread_with_state : thread_states) { | ||
| if (thread_with_state.second == CV_PREWAITING || thread_with_state.second == CV_WAITING) { | ||
| this->policy->add_waiter_with_state(thread_with_state.first,thread_with_state.second); | ||
| } | ||
| } | ||
| std::vector<runner_id_t> signaled_threads; | ||
| for (const auto& thread_with_state : thread_states) { | ||
| if (thread_with_state.second == CV_SIGNALED) { | ||
| signaled_threads.push_back(thread_with_state.first); | ||
| } | ||
| } | ||
| if (!signaled_threads.empty()) { | ||
| // If there are any threads that have been signaled, we should | ||
| // add them to the wake groups in the policy. | ||
| this->policy->add_to_wake_groups(signaled_threads); | ||
| } | ||
| } | ||
| // The only non-copy constructor: `tid`/`mutex` get sentinel defaults | ||
| // (RID_INVALID/nullptr) rather than being left uninitialized (there was | ||
| // previously no default member initializer for either, unlike `waiting_count`) | ||
| // -- e.g. mutex(state, location) needed the identical fix for `location` | ||
| // after commit 9bd9ecf. `p` (policy) defaults to nullptr meaning "create a | ||
| // fresh one"; every transition that advances an *existing* condition | ||
| // variable's state must still pass its own (possibly cloned) policy | ||
| // explicitly here or via set_policy() afterward -- see that setter's | ||
| // comment for why an omitted policy silently discards in-progress waiter | ||
| // tracking rather than just being harmlessly empty. | ||
| condition_variable(state s, runner_id_t tid = RID_INVALID, | ||
| pthread_mutex_t* mutex = nullptr, int count = 0, | ||
| const std::vector<std::pair<runner_id_t, condition_variable_status>>& thread_states = {}, | ||
| ConditionVariablePolicy* p = nullptr) | ||
| : current_state(s), running_thread(tid), associated_mutex(mutex), waiting_count(count), | ||
| policy(p ? p : new ConditionVariableArbitraryPolicy()) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
policy is an owning raw pointer with a defaulted copy constructor and destructor.
The parameterized constructor allocates a policy with new. The defaulted destructor never frees it, so every constructed condition_variable leaks one policy. The defaulted copy constructor copies the pointer, so clone() returns an object that shares one policy with the source object.
This shared policy defeats the cloning added in the transitions. condition_variable_enqueue_thread::modify() clones the policy so that a mutation cannot escape a throwaway diff_state, but a state-level clone() still aliases the policy of the cloned state. A later wake_thread() or add_to_wake_groups() through either object mutates the state seen by both.
Hold the policy in a std::unique_ptr<ConditionVariablePolicy> and deep-copy it in the copy constructor.
🤖 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 `@include/mcmini/model/objects/condition_variables.hpp` around lines 38 - 56,
The policy member variable is currently a raw owning pointer that causes memory
leaks (allocated in the parameterized constructor but never freed) and
unintended aliasing via the defaulted copy constructor. Replace the raw pointer
policy member with std::unique_ptr<ConditionVariablePolicy> to enable automatic
cleanup. Remove the defaulted copy constructor and implement an explicit copy
constructor that deep-copies the policy by creating a new
ConditionVariablePolicy instance for the copied object. Update the parameterized
constructor to initialize the unique_ptr correctly, allocating a fresh policy
when p is nullptr or wrapping the provided policy pointer.
| ConditionVariablePolicy* new_policy = cv->get_policy()->clone(); | ||
| condition_variable_status current_state = new_policy->get_thread_cv_state(executor); | ||
| if (current_state == CV_PREWAITING) { | ||
| // Thread not fully in wait state - update to WAITING before proceeding | ||
| cv->get_policy()->update_thread_cv_state(executor, CV_WAITING); | ||
| new_policy->update_thread_cv_state(executor, CV_WAITING); | ||
| } | ||
|
|
||
| cv->get_policy()->add_waiter_with_state(executor, CV_WAITING); | ||
| const int new_waiting_count = cv->get_policy()->return_wait_queue().size(); | ||
|
|
||
| s.add_state_for_obj(cond_id, new condition_variable(condition_variable::cv_waiting, executor, m->get_location(), new_waiting_count)); | ||
| s.add_state_for_obj(mutex_id, new mutex(mutex::unlocked)); | ||
| new_policy->add_waiter_with_state(executor, CV_WAITING); | ||
| const int new_waiting_count = new_policy->return_wait_queue().size(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the policy class and inspect its waiter-queue mutators.
fd -i 'condition_variable*polic*' -e hpp -e cpp -e h
rg -nP -C10 '\b(add_waiter_with_state|update_thread_cv_state)\s*\(' -g '*.hpp' -g '*.cpp' -g '*.h'Repository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository root:"
pwd
echo "Tracked files matching condition_variables/policy names:"
git ls-files | rg -n 'condition_variables|condition_variable|polic' | head -200 || true
echo
echo "Search for file/comment context:"
git ls-files | rg 'condition_variable_enqueue_thread\.hpp$|condition_variable.*policy|condition_variable.*policy.*hpp|condition_variable.*policy.*hpp' || trueRepository: mcminickpt/deep-debug
Length of output: 1459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "enqueue_thread.hpp:"
cat -n include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp | sed -n '1,120p'
echo
echo "Policy headers:"
for f in include/mcmini/misc/cond/cond_var_default_policy.hpp include/mcmini/misc/Cond_var_arbitrary_policy.hpp include/mcmini/misc/cond/cond_var_policy.hpp include/mcmini/misc/cond/cond_var_single_grp_policy.hpp; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f" | sed -n '1,240p'
fi
done
echo
echo "Policy source mutators:"
for f in src/mcmini/model/cond_var_default_policy.cpp src/mcmini/model/cond_var_arbitrary_policy.cpp src/mcmini/model/cond_var_single_grp_policy.cpp include/mcmini/model/objects/condition_variables.hpp include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n -C 8 '\b(add_waiter_with_state|update_thread_cv_state|get_thread_cv_state|return_wait_queue|waiter|Wait|waiting|CV_)\b' "$f"
fi
done
echo
echo "Search all tracked files:"
rg -n -C 8 '\b(add_waiter_with_state|update_thread_cv_state|get_thread_cv_state|return_wait_queue)\b' -g '*.hpp' -g '*.cpp' -g '*.h' . || trueRepository: mcminickpt/deep-debug
Length of output: 15836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All match output for relevant symbols:"
rg -n -C 12 '\b(add_waiter_with_state|add_waiter\(|update_thread_cv_state|get_thread_cv_state|return_wait_queue|invalid_thread_addition|invalid_thread_wakeup_exception)\b' -g '*.hpp' -g '*.cpp' . || true
echo
echo "Full single group policy source:"
if [ -f src/mcmini/model/cond_var_single_grp_policy.cpp ]; then
cat -n src/mcmini/model/cond_var_single_grp_policy.cpp | sed -n '1,200p'
fi
echo
echo "Full arbitrary policy source:"
if [ -f src/mcmini/model/cond_var_arbitrary_policy.cpp ]; then
cat -n src/mcmini/model/cond_var_arbitrary_policy.cpp | sed -n '1,220p'
fiRepository: mcminickpt/deep-debug
Length of output: 49833
Check and clear the waiter queue before calling add_waiter_with_state.
add_waiter_with_state calls add_waiter, and the single-group policy implementation pushes tid into wait_queue unconditionally:
void ConditionVariableSingleGroupPolicy::add_waiter(runner_id_t tid) {
this->wait_queue.push_back(tid);
}If CV_PREWAITING already has an entry for executor, the subsequent CV_WAITING update leaves that entry and appends a duplicate, inflating new_waiting_count and corrupting cv_waiting_count. Handle the pre-existing waiter in this enqueue path, or make add_waiter_with_state reject/replace existing waiters if that is the intended invariant.
🤖 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
`@include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp`
around lines 44 - 52, Update the enqueue path around
ConditionVariablePolicy::add_waiter_with_state to detect and remove or replace
any existing executor waiter before adding its CV_WAITING entry, especially when
the current state is CV_PREWAITING. Ensure the queue contains at most one entry
for executor so new_waiting_count and cv_waiting_count remain accurate.
|
|
||
| // Wait for whichever thread eventually calls pthread_join() on this one | ||
| // (see mc_pthread_join()'s TARGET_BRANCH*/DMTCP_RESTART_INTO_BRANCH-ish | ||
| // cases) to grant permission before this thread is allowed to really | ||
| // terminate. This keeps this thread's pthread_t/tid valid for exactly as | ||
| // long as a real, unjoined POSIX thread's would be -- no longer -- | ||
| // rather than parking it forever regardless of whether anyone ever | ||
| // joins it. | ||
| sem_t *exit_permission = find_exit_permission_sem(pthread_self()); | ||
| assert(exit_permission != NULL); | ||
| libpthread_sem_wait(exit_permission); | ||
|
|
||
| // Signal genuine completion, in case the joiner can't use a real join | ||
| // (see mc_pthread_join()'s TARGET_BRANCH_AFTER_RESTART case). | ||
| sem_t *really_exited = find_really_exited_sem(pthread_self()); | ||
| assert(really_exited != NULL); | ||
| libpthread_sem_post(really_exited); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Retry the exit-permission wait on EINTR.
libpthread_sem_wait(exit_permission) resolves to glibc's sem_wait, which returns -1 with errno == EINTR when a signal handler runs. This code ignores the return value. DMTCP and this library both deliver real-time signals to target threads (SIG_MULTITHREADED_FORK, see src/lib/tsan_support.c line 15), so an interrupted wait is reachable.
On EINTR this thread proceeds to post really_exited_sem and then terminates for real, before any joiner granted permission. The joiner in mc_pthread_join_impl's TARGET_BRANCH case (line 1126) then performs a real join on a thread that already exited outside the model's control, and the model's view of the thread's lifetime no longer matches reality.
🔒️ Proposed fix
sem_t *exit_permission = find_exit_permission_sem(pthread_self());
assert(exit_permission != NULL);
- libpthread_sem_wait(exit_permission);
+ while (libpthread_sem_wait(exit_permission) != 0) {
+ assert(errno == EINTR);
+ }Apply the same retry to the really_exited_sem wait in mc_pthread_join_impl (line 1163).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Wait for whichever thread eventually calls pthread_join() on this one | |
| // (see mc_pthread_join()'s TARGET_BRANCH*/DMTCP_RESTART_INTO_BRANCH-ish | |
| // cases) to grant permission before this thread is allowed to really | |
| // terminate. This keeps this thread's pthread_t/tid valid for exactly as | |
| // long as a real, unjoined POSIX thread's would be -- no longer -- | |
| // rather than parking it forever regardless of whether anyone ever | |
| // joins it. | |
| sem_t *exit_permission = find_exit_permission_sem(pthread_self()); | |
| assert(exit_permission != NULL); | |
| libpthread_sem_wait(exit_permission); | |
| // Signal genuine completion, in case the joiner can't use a real join | |
| // (see mc_pthread_join()'s TARGET_BRANCH_AFTER_RESTART case). | |
| sem_t *really_exited = find_really_exited_sem(pthread_self()); | |
| assert(really_exited != NULL); | |
| libpthread_sem_post(really_exited); | |
| // Wait for whichever thread eventually calls pthread_join() on this one | |
| // (see mc_pthread_join()'s TARGET_BRANCH*/DMTCP_RESTART_INTO_BRANCH-ish | |
| // cases) to grant permission before this thread is allowed to really | |
| // terminate. This keeps this thread's pthread_t/tid valid for exactly as | |
| // long as a real, unjoined POSIX thread's would be -- no longer -- | |
| // rather than parking it forever regardless of whether anyone ever | |
| // joins it. | |
| sem_t *exit_permission = find_exit_permission_sem(pthread_self()); | |
| assert(exit_permission != NULL); | |
| while (libpthread_sem_wait(exit_permission) != 0) { | |
| assert(errno == EINTR); | |
| } | |
| // Signal genuine completion, in case the joiner can't use a real join | |
| // (see mc_pthread_join()'s TARGET_BRANCH_AFTER_RESTART case). | |
| sem_t *really_exited = find_really_exited_sem(pthread_self()); | |
| assert(really_exited != NULL); | |
| libpthread_sem_post(really_exited); |
🤖 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 458 - 474, Update the exit-permission wait
in the thread termination path to retry libpthread_sem_wait(exit_permission)
when it returns -1 with errno == EINTR, and only continue after the semaphore
wait succeeds. Apply the same EINTR retry behavior to the really_exited
semaphore wait in mc_pthread_join_impl, preserving existing handling for
successful waits and other errors.
| int tsan_bg_virtual_tid = dmtcp_tsan_background_thread_virtual_tid(); | ||
| while (tsan_bg_virtual_tid == -1) { | ||
| sched_yield(); | ||
| tsan_bg_virtual_tid = dmtcp_tsan_background_thread_virtual_tid(); | ||
| } | ||
| if (tsan_bg_virtual_tid != ckpt_window_candidate_virtual_tid) { | ||
| record_checkpoint_thread(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the spin, and handle the "symbol absent" case explicitly.
Two problems in this loop:
- The loop has no bound and no timeout. If DMTCP never publishes a value other than
-1, the calling thread spins forever insideget_current_mode(), which every wrapper calls. Add a bounded retry with an explicit failure path. - When the DMTCP build does not export
dmtcp_tsan_background_thread_virtual_tid, the guard macro ininclude/dmtcp.hline 511 yields0.0is never a valid thread ID, so the comparison always takes therecord_checkpoint_thread()branch. For a TSan-instrumented target on such a DMTCP build, TSan's background thread is then recorded as the checkpoint thread with no warning. Detect the absent symbol and report it instead of silently misclassifying.
🤖 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 796 - 803, Update the TSan background-thread
lookup in get_current_mode to use a bounded retry/timeout and an explicit
failure path when dmtcp_tsan_background_thread_virtual_tid remains unavailable,
rather than spinning indefinitely. Distinguish the guard-macro result of 0 from
valid virtual thread IDs, report that the symbol is absent, and do not call
record_checkpoint_thread in that case. Preserve checkpoint-thread recording only
when a valid published ID differs from ckpt_window_candidate_virtual_tid.
| pthread_t this_thread = pthread_self(); | ||
| rec_list *thrd_record = find_thread_record_mode(this_thread); | ||
| visible_object vo = { | ||
| .type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED, | ||
| .interacting_thread = thrd_record->vo.thrd_state.id, | ||
| .associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() } | ||
| }; | ||
| cond_record = add_rec_entry_record_mode(&vo); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard thrd_record before you dereference it.
find_thread_record_mode(this_thread) returns NULL when the calling thread has no record-mode entry. Line 1451 dereferences the result without a check. mc_pthread_cond_wait's own RECORD case has the same shape at line 1276-1278, so a record normally exists for a model-checked thread. This new path exists precisely because the call can arrive on a thread whose bookkeeping did not follow the normal order — for example a signaling thread that reaches this wrapper before its own thread record is inserted.
Use a safe fallback identity when the record is absent, or abort with a specific message instead of dereferencing NULL.
The same code appears in mc_pthread_cond_broadcast at lines 1556-1563.
🐛 Proposed fix
pthread_t this_thread = pthread_self();
rec_list *thrd_record = find_thread_record_mode(this_thread);
+ runner_id_t signaler =
+ thrd_record != NULL ? thrd_record->vo.thrd_state.id : RID_INVALID;
visible_object vo = {
.type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED,
- .interacting_thread = thrd_record->vo.thrd_state.id,
+ .interacting_thread = signaler,
.associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pthread_t this_thread = pthread_self(); | |
| rec_list *thrd_record = find_thread_record_mode(this_thread); | |
| visible_object vo = { | |
| .type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED, | |
| .interacting_thread = thrd_record->vo.thrd_state.id, | |
| .associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() } | |
| }; | |
| cond_record = add_rec_entry_record_mode(&vo); | |
| pthread_t this_thread = pthread_self(); | |
| rec_list *thrd_record = find_thread_record_mode(this_thread); | |
| runner_id_t signaler = | |
| thrd_record != NULL ? thrd_record->vo.thrd_state.id : RID_INVALID; | |
| visible_object vo = { | |
| .type = CONDITION_VARIABLE, .location = cond, .cond_state = { .status = CV_INITIALIZED, | |
| .interacting_thread = signaler, | |
| .associated_mutex = NULL, .count = 0, .waiting_threads = create_thread_queue() } | |
| }; | |
| cond_record = add_rec_entry_record_mode(&vo); |
🤖 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 1447 - 1454, Guard the
find_thread_record_mode result in the condition-variable initialization path
before accessing thrd_record->vo.thrd_state.id, using a safe fallback identity
or a specific abort when no record exists. Apply the same protection in
mc_pthread_cond_broadcast and preserve the existing behavior when a thread
record is present.
| while (waitpid(-1, &status, WNOHANG) > 0) { | ||
| signal_tracker::instance().try_consume_signal(SIGCHLD); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Wait-status ownership is process-wide, not per handle. The destructor reaps every reapable zombie in the McMini process and the execute_runner loop then cannot find its own child's status. One handle can therefore consume a status that another handle still needs.
src/mcmini/real_world/local_linux_process.cpp#L76-L78: limit the drain to pids this handle spawned, or reap by process group, instead ofwaitpid(-1, ...).src/mcmini/real_world/local_linux_process.cpp#L130-L134: handleerrno == ECHILDas "status already consumed" and report it separately from a realwaitpidfailure.
📍 Affects 1 file
src/mcmini/real_world/local_linux_process.cpp#L76-L78(this comment)src/mcmini/real_world/local_linux_process.cpp#L130-L134
🤖 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/mcmini/real_world/local_linux_process.cpp` around lines 76 - 78, In
src/mcmini/real_world/local_linux_process.cpp lines 76-78, update the
destructor’s waitpid drain to reap only child PIDs spawned by this handle, or
use the handle’s process group, instead of waitpid(-1, ...). In lines 130-134,
update the execute_runner waitpid error handling so errno == ECHILD is reported
as the child status already being consumed, separately from genuine waitpid
failures.
7d6a1af to
6c4245f
Compare
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/interception.c`:
- Around line 282-285: Update the pthread-exit interception flow around
libpthread_pthread_exit and mc_pthread_exit to add a DMTCP pthread_exit
forwarding pointer alongside the existing libdmtcp_pthread_create/join pointers.
Initialize and select the DMTCP wrapper when DMTCP is enabled, while retaining
the native libpthread pointer otherwise, so pre-checkpoint exits are forwarded
through DMTCP before restart handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fae7b25-7b8c-4bdf-90c3-59a364da8675
📒 Files selected for processing (20)
doc/cond-wait-tsan-interceptor-bypass.txtdoc/log-mutex-fork-desync.txtdoc/pthread-exit-abort-and-fiber-crash.txtdoc/tsan-mutex-annotation-false-positive.txtinclude/mcmini/lib/log.hinclude/mcmini/spy/intercept/interception.hinclude/mcmini/spy/intercept/wrappers.hsrc/common/multithreaded_fork.csrc/examples/CMakeLists.txtsrc/examples/cv-producer-consumer-safe.csrc/examples/exit-stress-noop.csrc/examples/exit-stress.csrc/examples/producer-consumer-exit.csrc/examples/producer-consumer-safe.csrc/lib/dmtcp-callback.csrc/lib/interception.csrc/lib/log.csrc/lib/pthread_cond_signal_wrap.csrc/lib/pthread_cond_wait_wrap.csrc/lib/wrappers.c
🚧 Files skipped from review as they are similar to previous changes (14)
- src/lib/pthread_cond_signal_wrap.c
- include/mcmini/lib/log.h
- src/lib/log.c
- doc/pthread-exit-abort-and-fiber-crash.txt
- doc/cond-wait-tsan-interceptor-bypass.txt
- src/examples/exit-stress-noop.c
- include/mcmini/spy/intercept/wrappers.h
- include/mcmini/spy/intercept/interception.h
- src/examples/exit-stress.c
- src/lib/pthread_cond_wait_wrap.c
- src/common/multithreaded_fork.c
- src/examples/CMakeLists.txt
- src/lib/dmtcp-callback.c
- src/lib/wrappers.c
| MCMINI_NO_RETURN void libpthread_pthread_exit(void *retval) { | ||
| libmcmini_init(); | ||
| (*libpthread_pthread_exit_ptr)(retval); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Trace all local pthread-exit dispatch paths and DMTCP forwarding symbols.
rg -n -C 6 '\b(mc_pthread_exit|pthread_exit|libpthread_pthread_exit|libdmtcp_pthread_exit)\b' src include
rg -n -C 4 'libdmtcp_pthread_(create|join|exit)|dmtcp.*pthread_exit' src includeRepository: mcminickpt/deep-debug
Length of output: 34051
🌐 Web query:
DMTCP pthread_exit DMTCP callback dlsym pthread_exit dmtcp_pthread_exit
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) utilizes symbol interposition to intercept various system and library calls, including POSIX threads functions like pthread_exit [1][2]. When DMTCP intercepts these calls, it often needs to resolve the original, underlying system function to maintain correct application state [3][4]. Technical details regarding the interaction between DMTCP, pthread_exit, and dlsym: 1. Symbol Interposition and Recursion: DMTCP interposes functions such as pthread_create and pthread_exit by implementing wrappers that record state or perform necessary checkpointing operations [1][2]. A common challenge is that these wrappers may need to call the "real" underlying system function [5][4]. Using standard dlsym(RTLD_NEXT,...) within these wrappers can lead to infinite recursion or deadlocks if the dlsym call itself triggers code paths that require the same locks or state management as the DMTCP wrapper [3][5][6]. 2. Custom dlsym Implementation: To mitigate issues like infinite recursion—especially in modern GLIBC versions where libdl/dlsym may call malloc or other intercepted functions—DMTCP includes its own internal implementation of dlsym [3]. This custom dlsym is used to resolve real symbols without triggering the overhead or side effects of the system's dynamic linker-based dlsym in sensitive contexts [3]. 3. Deadlock Risks: Historically, DMTCP has encountered deadlocks during checkpointing events (e.g., DMTCP_EVENT_THREADS_SUSPEND) when a thread is interrupted while inside a dlsym call [6]. If the DMTCP checkpoint thread then calls dlsym (or a function that calls dlsym, such as a pthread wrapper) and attempts to acquire a mutex already held by the suspended thread, a deadlock occurs [6]. Developers have addressed such issues by employing internal wrappers and locking mechanisms (e.g., WRAPPER_EXECUTION_LOCK) to ensure that interposition calls are atomic and safe during checkpoint/restart operations [6]. 4. Practical Usage: While dmtcp_pthread_exit is not typically a public API, it represents the internal handling logic within the DMTCP interception layer to manage thread termination cleanly during migration or checkpointing [1][2]. If you are encountering issues related to these symbols, it is often due to the order of library preloading (LD_PRELOAD) or linker-related symbol resolution conflicts [5][7]. Ensuring that DMTCP's wrappers are correctly positioned in the library load order is essential for stability [5].
Citations:
- 1: https://exa.ai/library/legal/patent/2sl79ght451br2fhzfkks7
- 2: https://www.patents-review.com/a/20240289164-techniques-clearing-internal-state-library.html
- 3: https://dmtcp.sourceforge.io/index.html
- 4: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 5: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 6: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 7: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
🌐 Web query:
dmTCP code pthread_exit dlsym RTLD_NEXT DMTCP wrapper pthread_exit
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) uses library interposition to intercept system and library calls, including POSIX thread functions like pthread_exit [1][2]. The mechanism relies on dlsym(RTLD_NEXT, ...) to locate and call the "real" underlying implementation of a function after the wrapper has performed its necessary checkpointing or virtualization tasks [3][4][5]. When DMTCP wraps a function such as pthread_exit, it typically follows this pattern: 1. Interposition: The wrapper function in the DMTCP library intercepts the call, as it is loaded into the application's address space (often via LD_PRELOAD) [1][6]. 2. Resolution: The wrapper uses dlsym(RTLD_NEXT, "pthread_exit") to retrieve the address of the actual glibc implementation of pthread_exit [4][7][8]. 3. Execution: The wrapper executes its logic (e.g., updating thread state for checkpointing purposes) and then calls the resolved real function [1][9]. Technical challenges associated with this approach include: - Infinite Recursion: Using dlsym within a wrapper can trigger recursion if dlsym itself triggers a call to a wrapped function (like malloc or a thread-related function) [1][9]. To mitigate this, DMTCP often uses its own internal, low-level implementations of dlsym or safeguards like WRAPPER_EXECUTION_LOCK to prevent deadlocks and recursion during sensitive operations [1][9]. - Symbol Resolution: Ensuring RTLD_NEXT correctly finds the next symbol requires careful management of link order and library dependencies [7][8]. If the application or other libraries interfere with the search order, dlsym may fail or return an incorrect symbol [7]. - Architecture and Environment: DMTCP manages differences in dlsym behavior across architectures and glibc versions, sometimes using environment variables (such as DMTCP_DLSYM_OFFSET) or specific build configurations to ensure robust symbol lookup [10][1]. In summary, the use of dlsym(RTLD_NEXT, ...) in DMTCP wrappers is a standard technique for transparently interposing library calls, with specific internal modifications implemented to avoid the common pitfalls of deadlock and infinite recursion that occur when wrapping core system functionality [4][1][9].
Citations:
- 1: https://dmtcp.sourceforge.io/index.html
- 2: https://criu.org/DMTCP
- 3: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 4: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 5: https://repository.library.northeastern.edu/files/neu:336364/fulltext.pdf
- 6: https://www.slideserve.com/vfoley/dmtcp-a-new-linux-checkpointing-mechanism-for-vanilla-universe-jobs-powerpoint-ppt-presentation
- 7: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 8: https://sourceware.org/legacy-ml/glibc-linux/2001-q1/msg00050.html
- 9: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 10: https://github.com/dmtcp/dmtcp/blob/master/doc/multi-arch.txt
🌐 Web query:
DMTCP source pthread_exit dlsym pthread_exit libdmtcp_pthread_exit
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) utilizes symbol interposition to intercept various system and library calls, including thread-related functions like pthread_exit [1][2]. This interposition allows DMTCP to manage checkpointing and thread state synchronization [1][2]. Regarding the specific symbols mentioned: 1. pthread_exit: DMTCP provides wrappers for pthread functions (often found in files like wrappers.cpp or similar components within the source tree) to ensure that thread lifecycle events, such as thread creation and exit, are correctly tracked for consistent checkpointing [3][4][2]. 2. dlsym: DMTCP implements its own internal dlsym mechanism to avoid infinite recursion and deadlocks [1][5]. In many GLIBC versions, calling standard libdl/dlsym functions can trigger further libc/malloc calls, which may conflict with DMTCP's internal state or initialization sequence [1][5][3]. Consequently, DMTCP frequently uses its own dlsym wrapper—often bypassing standard libdl or using RTLD_NEXT to reach the "real" underlying system function—to maintain control without triggering circular dependencies [6][7][5]. 3. libdmtcp_pthread_exit: This notation generally refers to the internal symbol or the wrapped implementation of pthread_exit within the DMTCP library (libdmtcp.so) [8][4]. When an application is run under DMTCP, the linker/loader redirects calls to the original pthread_exit to the DMTCP-provided wrapper, which performs the necessary bookkeeping before potentially invoking the original system pthread_exit [6][7]. The architecture relies on these wrappers to handle the complexities of multi-threaded application state, specifically ensuring that threads are suspended or in a known state during the checkpointing process [5][4]. The use of dlsym within this context is a critical technical pattern in DMTCP to ensure that the interception layer remains robust even when applications or other libraries (like malloc or libdl) attempt to perform dynamic symbol resolution [1][5][3].
Citations:
- 1: https://dmtcp.sourceforge.io/index.html
- 2: https://github.com/dmtcp/dmtcp/blob/main/QUICK-START.md
- 3: https://rohgarg.github.io/notes/2018/11/19/libdmtcp-initialization.html
- 4: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 5: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 6: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 7: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 8: https://github.com/dmtcp/dmtcp/blob/master/doc/multi-arch.txt
🌐 Web query:
github dmtcp dmtcp pthread_exit RTLD_NEXT "pthread_exit"
💡 Result:
The combination of DMTCP (Distributed MultiThreaded Checkpointing), pthread_exit, and RTLD_NEXT is typically associated with the implementation of function wrappers for checkpoint/restart (C/R) functionality [1][2]. DMTCP and similar interception-based tools often use dlsym(RTLD_NEXT, "pthread_exit") to locate the original, unmodified implementation of pthread_exit within the system's libc or libpthread [3][1][2]. This allows the tool to intercept calls made by the application, perform necessary C/R housekeeping (such as updating metadata or managing state), and then invoke the original function to complete the operation [4][2]. Common technical challenges associated with this pattern include: 1. Symbol Resolution and Infinite Loops: When a library is preloaded (using LD_PRELOAD) or linked in a way that it is searched first, using dlsym(RTLD_NEXT, ...) correctly finds the "next" occurrence of the symbol [2][5]. If the lookup is misconfigured or if multiple wrappers conflict, it can lead to symbol resolution errors or infinite recursion if the wrapper accidentally calls itself [2]. 2. The noreturn Attribute: pthread_exit is frequently declared with the attribute((noreturn)) in system headers [4]. This can cause compiler optimizations that make it difficult to reliably wrap or override the function [4]. Developers often use macros to redefine pthread_exit to a wrapper function that lacks the noreturn attribute, ensuring that control can return to the shim after the wrapper code executes [6][4]. 3. Dependency on Unwinding/libgcc_s.so.1: pthread_exit often requires stack unwinding support (for C++ exception cleanup or thread cancellation handlers) [7][8]. Historically, this was managed by libpthread, but in modern glibc (since version 2.34), libpthread has been merged into libc [9][10]. If a wrapper or environment fails to correctly handle the dynamic loading of dependencies like libgcc_s.so.1, applications may crash or exit with errors stating that the library must be installed for pthread_exit to work [7][8][10]. 4. Deadlocks during Checkpointing: If dlsym() is called during a checkpoint event (when threads are suspended), it may trigger a deadlock if the dynamic linker itself requires a mutex that is already held by the suspended process [11]. This is why sophisticated wrappers often require specific synchronization or locking mechanisms (such as WRAPPER_EXECUTION_LOCK) to prevent re-entrancy during critical state-capture operations [11]. For issues involving DMTCP specifically, if you are experiencing failures, ensure that your environment correctly handles symbol resolution and that your wrappers are compiled to accommodate modern glibc symbol structures [2][12]. If you encounter segfaults, checking for -Wl,--no-as-needed during the link phase can sometimes resolve issues where the linker fails to correctly map the symbols at runtime [5].
Citations:
- 1: https://github.com/plasma-umass/dts
- 2: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 3: https://github.com/Mic92/inspector/blob/master/src/tthread/real.cpp
- 4: http://mails.dpdk.org/archives/dev/2015-November/028334.html
- 5: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 6: http://mails.dpdk.org/archives/dev/2015-December/029389.html
- 7: https://sourceware.org/pipermail/libc-alpha/2025-June/168180.html
- 8: https://sourceware.org/legacy-ml/glibc-bugs/2006-08/msg00168.html
- 9: https://sourceware.org/pipermail/glibc-cvs/2021q2/072873.html
- 10: https://lists.ubuntu.com/archives/foundations-bugs/2022-February/468238.html
- 11: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 12: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
🌐 Web query:
DMTCP code dmtcp-pthread.cpp pthread_exit dlsym RTLD_NEXT
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) uses function interposition—typically via LD_PRELOAD—to intercept libc and pthreads library calls [1][2][3]. Within its source code, such as files like dmtcp-pthread.cpp, DMTCP utilizes dlsym(RTLD_NEXT,...) to locate and invoke the "real" underlying libc/pthreads functions [4][5]. Key aspects of this mechanism include: 1. Interposition and dlsym(RTLD_NEXT): When DMTCP intercepts a function like pthread_exit, it must eventually call the original implementation provided by the system's glibc to maintain correctness [2]. Using dlsym(RTLD_NEXT, "pthread_exit") allows DMTCP to bypass its own wrapper and retrieve the address of the next symbol in the search order, which is the actual glibc function [6][5][7]. 2. Handling Recursion and Deadlocks: A significant challenge with this approach is avoiding infinite recursion and deadlocks. If a wrapper calls dlsym, and dlsym itself triggers a call to a function that DMTCP also wraps (such as malloc or a mutex lock), it can lead to deadlocks or infinite loops [1][8]. To mitigate this, DMTCP has implemented several strategies: - Implementing custom dlsym logic to reduce reliance on the system's libdl, which may call libc functions that DMTCP hooks [1]. - Using internal locks (like WRAPPER_EXECUTION_LOCK) to prevent re-entrant calls or deadlocks when the checkpointing thread and user threads compete for internal resources during event hooks [8]. 3. Architectural Variations: The implementation details, such as the exact dlsym offset, can vary between architectures (e.g., 32-bit vs. 64-bit), and DMTCP handles these differences by calculating offsets during launch/restart and storing them in shared data structures [9]. By wrapping these functions, DMTCP creates a "shadow" environment that allows it to track process state, manage thread lifecycle events, and coordinate checkpoints without modifying the target application's source code [1][2][3].
Citations:
- 1: https://dmtcp.sourceforge.io/index.html
- 2: https://criu.org/DMTCP
- 3: https://www.slideserve.com/vfoley/dmtcp-a-new-linux-checkpointing-mechanism-for-vanilla-universe-jobs-powerpoint-ppt-presentation
- 4: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 5: https://sourceware.org/legacy-ml/glibc-linux/2001-q1/msg00050.html
- 6: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 7: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 8: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 9: https://github.com/dmtcp/dmtcp/blob/master/doc/multi-arch.txt
🌐 Web query:
github dmtcp pthread_exit wrapper dlsym RTLD_NEXT
💡 Result:
In DMTCP (Distributed MultiThreaded Checkpointing), interposition of library functions like pthread_exit is a core mechanism used to track state and manage checkpoint/restart operations [1][2][3]. To safely intercept these functions, DMTCP typically uses a wrapper pattern [4][5][2]. When a wrapper function intercepts a call (e.g., pthread_exit), it often needs to invoke the original libc implementation to ensure correct behavior [4][6][7]. Using dlsym(RTLD_NEXT, "function_name") is the standard mechanism to find the address of the next symbol in the search order—specifically, the original version provided by the C library (libc/libpthread) rather than the wrapper itself [4][6][8][9]. Key technical aspects include: 1. Preventing Infinite Recursion: If a wrapper function calls the function it is wrapping (e.g., a wrapper for pthread_exit calls pthread_exit), it creates an infinite loop. Using RTLD_NEXT with dlsym allows the wrapper to bypass itself and call the next available implementation in the symbol search scope [4][9]. 2. Robustness and Cleanup: DMTCP has evolved its approach to handle symbol resolution more robustly, including replacing manual dlsym calls with internal abstractions like dmtcp_dlsym to better manage platform-specific nuances, versioning, and potential errors during symbol lookup [5][10][7]. 3. Configuration: DMTCP provides configuration options (such as --disable-dlsym-wrapper) to allow users to handle special environments where standard symbol interposition might behave unexpectedly or conflict with other libraries [11][10]. 4. Complexities: Overriding functions declared with attribute((noreturn))—like pthread_exit—can be particularly challenging, sometimes requiring macro-based overrides or specific shims to prevent the compiler from optimizing away the wrapper or misinterpreting the control flow [12][13]. In summary, the use of dlsym with RTLD_NEXT in DMTCP wrappers is essential for transparently interposing on standard library functions without causing infinite recursion or breaking the original functionality of the application being checkpointed [4][9][2].
Citations:
- 1: https://lkml.iu.edu/hypermail/linux/kernel/1011.0/02332.html
- 2: https://www.ccs.neu.edu/home/gene/papers/ipdps09.pdf
- 3: https://www.slideserve.com/vfoley/dmtcp-a-new-linux-checkpointing-mechanism-for-vanilla-universe-jobs-powerpoint-ppt-presentation
- 4: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 5: dmtcp/dmtcp@ad118bb
- 6: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 7: https://dmtcp.sourceforge.io/index.html
- 8: https://maskray.me/blog/2022-05-29-glibc
- 9: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 10: https://github.com/dmtcp/dmtcp/releases
- 11: https://github.com/dmtcp/dmtcp/releases/tag/3.0.0
- 12: http://mails.dpdk.org/archives/dev/2015-November/028334.html
- 13: http://mails.dpdk.org/archives/dev/2015-December/029497.html
🌐 Web query:
DMTCP pthread_create pthread_join pthread_exit wrapper dmtcp-pthread.cpp
💡 Result:
DMTCP (Distributed MultiThreaded CheckPointing) uses interposition via wrappers to intercept and manage POSIX thread (pthread) operations transparently [1][2]. While the specific file name in the DMTCP source code may vary by version or refactoring, thread management logic is typically implemented in files such as threadwrappers.cpp or similar components that handle POSIX thread library calls [3][1]. DMTCP wraps critical functions like pthread_create, pthread_join, and pthread_exit to maintain consistency during checkpointing and restart [1][4]: 1. pthread_create: The wrapper intercepts the creation of new threads. This allows DMTCP to track active threads, manage their state, and ensure that if a checkpoint occurs, the process can correctly recreate the threading environment upon restart [4]. 2. pthread_join: By wrapping this function, DMTCP can monitor thread synchronization and dependencies. It ensures that the checkpointing process accounts for threads waiting for others to finish, preventing inconsistent states [5][6]. 3. pthread_exit: The wrapper for pthread_exit allows DMTCP to clean up internal tracking data structures when a thread terminates, ensuring that the process's thread metadata remains accurate during its lifecycle [3][4]. DMTCP operates at the user level, using these wrappers to "spy" on and record the necessary thread state without requiring kernel modifications [7][1][8]. By interposing on libpthread.so, DMTCP can quiesce threads during checkpointing by forcing them to block in a signal handler and then resume them once the checkpoint image is captured [4]. Plugin architectures in DMTCP also allow for additional hooks and user-defined wrappers around these library calls to support virtualization of resources [9].
Citations:
- 1: https://lkml.iu.edu/hypermail/linux/kernel/1011.0/02332.html
- 2: https://github.laiyagushi.com/dmtcp/dmtcp
- 3: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 4: https://iopscience.iop.org/article/10.1088/1749-4699/8/1/014005
- 5: https://www.ccs.neu.edu/home/gene/papers/ipdps09.pdf
- 6: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 7: https://github.com/dmtcp/dmtcp
- 8: https://github.com/dmtcp/dmtcp/blob/main/README.md
- 9: https://github.com/dmtcp/dmtcp/blob/main/QUICK-START.md
🌐 Web query:
dmTCP dmtcp-pthread.cpp pthread_exit dmtcp library dlsym RTLD_NEXT
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) utilizes symbol interposition to intercept various libc and libpthread functions, including pthread_exit, to maintain process state for checkpointing and restoration [1][2]. The file dmtcp-pthread.cpp (or similar files within the DMTCP source tree) implements these wrappers [1][2]. To intercept functions like pthread_exit while still being able to call the original (libc) implementation, DMTCP commonly uses dlsym with RTLD_NEXT [3][4]. This technique allows the DMTCP library to obtain the address of the "next" occurrence of a symbol in the dynamic library search order, effectively bypassing the DMTCP wrapper itself to reach the underlying system implementation [3][4]. Key technical considerations regarding this mechanism include: 1. Infinite Recursion: Using dlsym(RTLD_NEXT, ...) inside a wrapper function is a standard way to call the real function. However, if the implementation of dlsym or other internal DMTCP mechanisms (like libdl calls) inadvertently trigger another intercepted function, it can lead to infinite recursion or deadlocks [5][6]. DMTCP has historically implemented its own internal dlsym or specific guards to mitigate these recursion issues, especially when modern glibc implementations of dlsym internally call malloc or other functions that DMTCP also wraps [5][6]. 2. Thread Safety and Deadlocks: Intercepting pthread functions is complex because these functions are often used by the checkpointing mechanism itself (e.g., when DMTCP needs to suspend or manage threads) [6][7]. If dlsym is called while the process is in a state where internal mutexes are held (or if the suspension logic interacts poorly with the libdl lock), deadlocks can occur [6]. DMTCP employs synchronization primitives, such as WRAPPER_EXECUTION_LOCK, within its wrappers to prevent these race conditions during checkpointing events [6]. 3. Symbol Visibility and Linking: For symbol interposition to function correctly, the DMTCP library must be loaded before the application (typically via LD_PRELOAD) [1]. If the application or its dependencies are linked in a way that ignores these preloaded symbols (e.g., using --as-needed or static linking), interposition may fail [4]. DMTCP's architecture for wrapping is designed to be transparent to the user, acting as a shadow database for process internals while forwarding the ultimate execution request to the glibc/kernel [8]. Over time, DMTCP has refined these wrappers to be more robust, moving away from simple dlsym usage toward custom implementations to ensure reliability across different glibc versions and architectures [9][5].
Citations:
- 1: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 2: https://lkml.iu.edu/hypermail/linux/kernel/1011.0/02332.html
- 3: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 4: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 5: https://dmtcp.sourceforge.io/index.html
- 6: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 7: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 8: https://criu.org/DMTCP
- 9: https://github.com/dmtcp/dmtcp/blob/master/doc/multi-arch.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant interfaces, dispatch paths, and DMTCP loading/use without executing repo code.
sed -n '1,170p' src/lib/interception.c
printf '\n--- wrappers dispatch ---\n'
sed -n '580,635p' src/lib/wrappers.c
sed -n '836,945p' src/lib/wrappers.c
printf '\n--- creation wrapper declaration ---\n'
sed -n '82,115p' include/mcmini/spy/intercept/interception.hRepository: mcminickpt/deep-debug
Length of output: 15434
🌐 Web query:
DMTCP dmtcp-pthread.cpp symbol "pthread_exit" "dlsym" "pthread_create"
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) uses function interposition (via LD_PRELOAD) to intercept POSIX thread (pthread) calls, enabling transparent checkpointing and restoration of multi-threaded applications [1][2][3]. Regarding the symbols and functions mentioned: 1. dlsym and symbol resolution: To avoid infinite recursion caused by the fact that many glibc functions (including libdl functions) internally call malloc or other intercepted functions, DMTCP implements its own version of dlsym [4]. This custom dlsym allows DMTCP to retrieve the addresses of original libc functions without triggering the wrappers it has injected [4]. 2. pthread_create and pthread_exit: DMTCP wraps these functions to maintain awareness of the application's thread state, which is critical for consistent checkpointing [2]. - By intercepting pthread_create, DMTCP can track thread lifecycles, assign internal thread IDs, and ensure that all threads are in a quiescent or consistent state when a checkpoint is triggered [2]. - pthread_exit is wrapped to ensure that when a thread terminates, DMTCP updates its internal thread management structures (e.g., thread lists) correctly [5]. This is necessary to maintain an accurate representation of the process state for restoration [2][5]. While dmtcp-pthread.cpp (or similar source files in the DMTCP repository) contains these wrapper implementations, the mechanism relies on obtaining the true address of the underlying libc/pthread functions (often stored in internal function pointers) and then executing them after DMTCP performs its own necessary tracking or state-saving operations [6][7]. If you are investigating specific behavior related to these functions, note that DMTCP frequently updates its pthread handling to accommodate changes in glibc versions, such as those related to thread stack allocation, signal handling (e.g., SIGCANCEL), and internal thread list management [5][8][9]. Recent versions have focused on robustness improvements for these wrappers [4][8].
Citations:
- 1: https://github.com/dmtcp/dmtcp
- 2: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 3: https://docs.nersc.gov/development/checkpoint-restart/dmtcp/
- 4: https://dmtcp.sourceforge.io/
- 5: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 6: https://doc.dpdk.org/api-16.07/performance-thread_2pthread_shim_2pthread_shim_8c-example.html
- 7: https://github.com/dmtcp/dmtcp/blob/master/src/plugin/timer/timer_create.cpp
- 8: https://github.com/dmtcp/dmtcp/blob/main/NEWS
- 9: https://dmtcp.sourceforge.io/index.html
🌐 Web query:
DMTCP 3.0 dmtcp-pthread.cpp pthread_exit dlsym pthread_exit wrapper
💡 Result:
DMTCP (Distributed MultiThreaded CheckPointing) uses a wrapping mechanism for POSIX thread and dynamic linking functions to intercept calls for checkpointing and state management [1][2]. In DMTCP, the dlsym wrapper is critical for maintaining consistency in dynamically linked applications. Because DMTCP intercepts library calls (like pthread functions) using LD_PRELOAD, it must ensure that subsequent calls to dlsym (e.g., inside an application) correctly resolve to either the real system library or, when necessary, the DMTCP-interposed version to avoid infinite recursion or deadlocks [3][4]. The pthread_exit wrapper in dmtcp-pthread.cpp (or related thread wrapper files) is responsible for handling thread termination within the context of DMTCP's thread management. When a thread calls pthread_exit, the wrapper allows DMTCP to clean up internal thread-local state, update connection lists, or manage thread synchronization events (such as quiescence for checkpointing) before allowing the thread to exit via the real pthread_exit provided by glibc [2][4]. Key technical aspects include: 1. Wrapping Mechanism: DMTCP often uses dlsym(RTLD_NEXT,...) within its wrappers to locate the original libc function once it has performed its own internal monitoring or state update [5][3]. 2. Deadlock Prevention: As noted in past issues, calls to dlsym inside wrappers can lead to deadlocks if they occur during events like thread suspension or checkpointing. Consequently, DMTCP implementations often employ internal locking (e.g., WRAPPER_EXECUTION_LOCK) to ensure that the thread executing the dlsym or wrapper code is not suspended or interrupted in a way that causes recursion or blocking [4]. 3. DMTCP 3.0: This version, released in 2023, continues to maintain these core architectural principles, though it introduced stricter C++14 requirements and specific bug fixes for platform-specific pthread behaviors [6][7][8]. Users encountering issues with these wrappers—such as hanging or unexpected exits—are generally advised to verify their environment's LD_PRELOAD order or, in specific cases, use configuration flags like --disable-dlsym-wrapper if the application's linking behavior conflicts with the standard DMTCP interception logic [6][3].
Citations:
- 1: https://github.com/dmtcp/dmtcp
- 2: https://course.ccs.neu.edu/cs5600f15/dmtcp/syscallwrappers_8h.html
- 3: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 4: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 5: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 6: https://github.com/dmtcp/dmtcp/releases/tag/3.0.0
- 7: https://github.com/dmtcp/dmtcp/blob/main/NEWS
- 8: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
🌐 Web query:
github dmtcp dmtcp-pthread.cpp pthread_exit pthread_create dlsym
💡 Result:
In DMTCP, the interaction between dmtcp-pthread.cpp, pthread_create, pthread_exit, and dlsym is central to its ability to transparently checkpoint and restart multi-threaded applications [1]. DMTCP uses symbol interposition (wrappers) to intercept calls to POSIX thread functions like pthread_create and pthread_exit [1]. These wrappers allow DMTCP to manage thread lifecycle, handle synchronization during checkpointing, and ensure proper state restoration upon restart [2][3]. Key implementation details include: 1. Custom dlsym Implementation: DMTCP implements its own version of dlsym to avoid infinite recursion issues that occur in modern GLIBC versions where libdl may call malloc or other intercepted functions [4][5]. By using this internal dlsym, DMTCP can reliably find the address of the "real" underlying libc functions without triggering intercepted wrappers recursively [6][7]. 2. Handling Interception: When a wrapper function (e.g., pthread_create) is called, it typically uses the NEXT_FUNC macro (or equivalent mechanism) to forward the call to the actual libc implementation [6]. To perform this lookup, DMTCP uses its internal dlsym [4][5]. 3. Multithreading & Deadlocks: Because dlsym may use internal locks, using it during checkpoint events can lead to deadlocks if the main thread is suspended while holding a lock that the checkpoint thread needs [7]. To mitigate this, DMTCP often employs specific locking strategies (e.g., WRAPPER_EXECUTION_LOCK) within its wrappers to safely manage concurrent calls to intercepted functions [7]. 4. Multi-architecture Support: DMTCP calculates dlsym offsets for both 32-bit and 64-bit architectures, storing these as environment variables (e.g., DMTCP_DLSYM_OFFSET) or in a shared data area to ensure that the correct symbol resolution occurs regardless of the process architecture [8]. In essence, DMTCP wraps thread functions to gain control over execution, uses a custom dlsym to bypass GLIBC's own redirection mechanisms and avoid recursion, and carefully manages locking to prevent deadlocks during the quiescence phase of checkpointing [4][7][3].
Citations:
- 1: https://github.com/dmtcp/dmtcp
- 2: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 3: https://iopscience.iop.org/article/10.1088/1749-4699/8/1/014005
- 4: https://dmtcp.sourceforge.io/index.html
- 5: https://dmtcp.sourceforge.io/
- 6: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 7: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 8: https://github.com/dmtcp/dmtcp/blob/master/doc/multi-arch.txt
🌐 Web query:
github dmtcp dmtcp-pthread.cpp dmtcp_pthread_exit dmtcp_pthread_create dlsym RTLD_NEXT
💡 Result:
In the DMTCP (Distributed MultiThreaded CheckPointing) project, the use of dlsym with RTLD_NEXT is a standard technique for intercepting and wrapping system calls like pthread_create and pthread_exit [1][2][3]. DMTCP functions as a transparent checkpointing tool by injecting a library (typically via LD_PRELOAD) that interposes on key libc and pthread symbols [4][5]. By defining its own versions of functions like pthread_create, DMTCP can track thread creation, manage thread-local storage, and ensure correct state restoration during checkpoint and restart operations [4][5]. Technical details regarding this implementation include: 1. Interception and Forwarding: When an application calls a wrapped function like pthread_create, the DMTCP implementation intercepts the call [1][5]. To invoke the actual system library implementation of the function (the "real" version), DMTCP uses dlsym(RTLD_NEXT, "function_name") [1][6][2]. The RTLD_NEXT handle is critical as it instructs the dynamic linker to find the next occurrence of the symbol in the search order—specifically, the version in the actual system libraries (e.g., libpthread) rather than the wrapper itself, thus avoiding infinite recursion [1][2][3]. 2. Handling Recursion and Deadlocks: Because dlsym itself can trigger library calls that might lead to further interceptions (and potential infinite recursion or deadlocks), DMTCP has historically implemented robust wrappers [4]. As noted in project issues, improper use of dlsym can lead to deadlocks if it triggers a call to a function that is already being intercepted or if it competes for internal locks within the dynamic linker [1][7]. To mitigate this, DMTCP often uses its own internal dlsym implementation or employs locking mechanisms (such as WRAPPER_EXECUTION_LOCK) to ensure that the process of resolving the real symbol is thread-safe and does not interfere with the checkpointing events [7][4]. 3. Code Context: The pattern is visible in various source files within the DMTCP repository, such as those related to thread wrappers (e.g., threadwrappers.cpp) and plugin-based system call interception [7][8][9]. The use of dlsym(RTLD_NEXT,...) is the standard pattern for creating these trampolines to the underlying system calls [3][10]. In summary, DMTCP relies on the dlsym(RTLD_NEXT,...) mechanism to safely interpose on pthread and system functions to maintain its checkpointing state while transparently passing execution to the underlying system implementation [1][7][4].
Citations:
- 1: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 2: https://stackoverflow.com/questions/34840510/how-to-replace-pthread-create-during-linkage
- 3: https://fastware.dev/p/intercept-system-library-calls-on-linux
- 4: https://dmtcp.sourceforge.io/index.html
- 5: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 6: https://course.ccs.neu.edu/cs5600f15/dmtcp/nosyscallsreal_8c.html
- 7: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 8: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 9: https://github.com/dmtcp/dmtcp/blob/master/src/plugin/timer/timer_create.cpp
- 10: https://gist.github.com/yalue/681ef4b84a020af195e162d4a47c75c1
🌐 Web query:
github dmtcp dmtcp-pthread.cpp "pthread_exit" "dlsym"
💡 Result:
In the DMTCP (Distributed MultiThreaded Checkpointing) project, the use of dmtcp-pthread.cpp, pthread_exit, and dlsym relates to the project's strategy for interposing POSIX thread functions to support transparent checkpointing [1][2]. DMTCP wraps pthread functions to intercept application behavior, allowing it to capture and restore the state of multi-threaded applications [1]. A key challenge in this interposition is avoiding infinite recursion and deadlock, particularly because DMTCP itself may need to call underlying system functions (like those in libpthread or libdl) that it is also interposing [3][4]. Key aspects regarding these components in DMTCP include: 1. Interposition and dlsym: DMTCP often uses dlsym(RTLD_NEXT, ...) to locate and call the "real" underlying libc or pthread functions [5][6]. However, direct use of dlsym can be problematic in recent GLIBC versions because libdl functions may call malloc or other functions that DMTCP also interposes, creating infinite recursion or circular dependencies [4][7]. 2. Custom dlsym Implementation: To address these issues, DMTCP implemented its own internal dlsym wrapper [4][7]. This custom implementation is designed to be more robust, avoiding reliance on standard libdl functions where possible to prevent deadlocks during critical DMTCP events (e.g., when threads are being suspended for a checkpoint) [3][4]. 3. pthread_exit handling: Like other pthread functions, pthread_exit is intercepted to ensure that when a thread exits, DMTCP can properly update its internal thread accounting and state management before the thread actually terminates [2]. If an application uses pthread_exit, the DMTCP wrapper captures the call, performs necessary bookkeeping (such as cleaning up thread-specific state), and then invokes the original pthread_exit (often retrieved via the internal dlsym mechanism) to allow the thread to exit normally [2]. If you are investigating specific behavior in dmtcp-pthread.cpp (or related files like pthread_override.c), the logic typically follows the pattern of identifying the current thread, recording the state if needed, and safely calling the real library function through DMTCP's internal symbol resolution mechanism [2][3]. If you encounter hangs or crashes, it is often due to the wrapper attempting to perform an operation (like dlsym or a mutex lock) that is not re-entrant or is deadlocking due to the thread suspension state [3].
Citations:
- 1: https://github.com/dmtcp/dmtcp
- 2: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 3: dlopen1 failing on CentOS 7 (and others?) dmtcp/dmtcp#57
- 4: https://dmtcp.sourceforge.io/index.html
- 5: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 6: https://stackoverflow.com/questions/44177595/mysterious-segfaults-when-overriding-pthread-functions-on-glibc-but-not-on-musl
- 7: https://dmtcp.sourceforge.io/
🌐 Web query:
github dmtcp dmtcp-pthread.cpp "dmtcp_intercept_next_func" "pthread_exit"
💡 Result:
In the DMTCP (Distributed MultiThreaded CheckPointing) source code, dmtcp_intercept_next_func is a utility function used within the library's interception mechanism to call the original, underlying system or library function that DMTCP has wrapped [1]. DMTCP operates by transparently intercepting various system calls and library functions (such as POSIX thread functions) to manage checkpointing and restoration state [2][1]. Because DMTCP overrides these standard library symbols (e.g., using LD_PRELOAD), it requires a way to invoke the original implementation (e.g., the actual pthread_exit or other pthread functions) to ensure the application's intended behavior is maintained alongside DMTCP's management tasks [1]. While specific implementation details can change across versions, files like dmtcp-pthread.cpp typically house the wrappers for POSIX thread functions [3]. The dmtcp_intercept_next_func utility is essential in these wrappers to: 1. Allow the application to execute the original logic provided by glibc or other libraries. 2. Maintain the transparency of the checkpointing process by ensuring that intercepting a call does not break the expected behavior of the thread or process. If you are encountering issues related to pthread_exit or other thread-related functions in DMTCP, it is often due to complex interactions with specific glibc versions or thread cleanup handling [3][4][5]. Recent development in the DMTCP GitHub repository has focused on addressing compatibility issues with newer glibc versions, including bug fixes for pthread cancellation and thread management during restart [3][4][6].
Citations:
- 1: https://www.ccs.neu.edu/home/kapil/papers/2009ipdps-dmtcp.pdf
- 2: https://github.com/dmtcp/dmtcp
- 3: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 4: https://github.com/dmtcp/dmtcp/blob/main/NEWS
- 5: https://dmtcp.sourceforge.io/index.html
- 6: Issues with my program using DMTCP_PATH_MAPPING(src/plugin_pathtranslator.cpp) feature during restart dmtcp/dmtcp#1232
🌐 Web query:
"dmtcp-pthread.cpp" "pthread_exit" "pthread_create"
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) uses wrappers for pthread functions, including pthread_create and pthread_exit, to enable transparent checkpointing of multi-threaded applications [1][2]. These wrappers allow DMTCP to intercept thread creation and termination to track the state of threads across checkpoints and restarts [3][4][5]. Key technical details regarding these functions in the context of DMTCP include: Wrappers and Interception: DMTCP intercepts POSIX thread library calls (like pthread_create) via library interposition, often by linking a shared library (dmtcp.so) that overrides the system implementation [6][1]. This allows DMTCP to maintain its own metadata about application threads [4]. Implementation in source code: Within the DMTCP source, thread-related functionality is often handled in files like threadwrappers.cpp [4][5]. For example, when a thread is created, DMTCP typically uses a wrapper that initializes tracking structures before invoking the real system pthread_create (often accessed via dlsym(RTLD_NEXT,...)) [6]. pthread_exit handling: Wrappers for pthread_exit are used to ensure that when a thread terminates, DMTCP correctly updates its internal process/thread state, allowing it to accurately resume the application during a restart [3][4]. Common Challenges: Infinite Recursion: Because DMTCP wraps pthread functions, issues can arise if an overridden function inadvertently calls itself or another wrapped function without correctly bypassing the wrapper (e.g., via dlsym(RTLD_NEXT)) [6]. Thread State Tracking: Bugs have occasionally appeared related to how DMTCP tracks thread state, particularly with detached threads or specific glibc versions, where the internal metadata (like struct pthread) needs careful synchronization with the operating system's thread management [4]. System Integration: DMTCP is designed to work in user space without kernel modifications [1][2]. Consequently, its thread wrappers must rely on standard glibc interfaces (like NPTL) [4][7], which can lead to complexities when the underlying glibc implementation changes or behaves unexpectedly [4]. In summary, dmtcp-pthread.cpp (or corresponding thread wrapper files) acts as a compatibility layer that intercepts standard pthread calls to manage the lifecycle and state of threads for checkpoint/restart purposes [3][4][5].
Citations:
- 1: https://github.com/dmtcp/dmtcp
- 2: https://dmtcp.sourceforge.io/index.html
- 3: https://github.com/dmtcp/dmtcp/blob/master/src/plugin/timer/timer_create.cpp
- 4: Bug in pthread2 (seen on RISC-V, glibc-2.37): detached threads dmtcp/dmtcp#1163
- 5: Dmtcp 3.1.2 crashing with SIGSEGV fault in my application dmtcp/dmtcp#1190
- 6: Troubles with dlsym() + RM dmtcp/dmtcp#604
- 7: https://github.com/bminor/glibc/blob/master/nptl/pthread_create.c
Forward DMTCP thread exits through the DMTCP wrapper.
DMTCp interposes on pthread_exit, but mc_pthread_exit() still calls libpthread_pthread_exit(retval) before DMTCP restart handling for pre-checkpoint paths. Add a DMTCP pthread_exit forwarding pointer like libdmtcp_pthread_{create,join} and select it when DMTCP is enabled.
🤖 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/interception.c` around lines 282 - 285, Update the pthread-exit
interception flow around libpthread_pthread_exit and mc_pthread_exit to add a
DMTCP pthread_exit forwarding pointer alongside the existing
libdmtcp_pthread_create/join pointers. Initialize and select the DMTCP wrapper
when DMTCP is enabled, while retaining the native libpthread pointer otherwise,
so pre-checkpoint exits are forwarded through DMTCP before restart handling.
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>
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.
Off by default. Experiment to see whether instrumenting libmcmini.so itself (as opposed to only the target) changes TSan-registration-order crashes; empirically it did not move the crash site, so the normal path stays uninstrumented per PLAN.txt's suppressions-file design.
When a -fsanitize=thread target runs under DMTCP, dmtcp_launch
prepends libtsan ahead of libmcmini, so a new thread's entry order is
dmtcp thread_start -> mc_thread_routine_wrapper -> libtsan
trampoline -> user routine.
libmcmini's wrapper thus runs BEFORE libtsan has registered the
thread. Its prologue then called libc functions that libtsan
intercepts (malloc, and the raw pthread_rwlock_* in
insert_pthread_map), and those interceptors dereference the
unregistered thread's null ThreadState -> SEGV / TSan
"sanitizer_thread_registry.cpp:348" CHECK. (Full analysis in
TSAN-McMini-DMTCP.txt.)
Make the prologue free of TSan-intercepted libc calls:
- Add mc_ts_alloc (mem.c/mem.h): a bump allocator over a static BSS
arena. No libc call and no syscall on the fast path (only an
atomic bump), so it never enters a TSan interceptor. Fail path
uses raw syscalls only. Never frees (the pthread_map / rec_list
nodes it backs are never freed).
- insert_pthread_map / search_pthread_map: use libmcmini's
libpthread_* handle wrappers (which bypass libtsan) instead of
the raw pthread_rwlock_* symbols, and mc_ts_alloc instead of
malloc. Also fixes a pre-existing missing-unlock bug in
search_pthread_map's found path.
- add_rec_entry_record_mode_ts (record.c): mc_ts_alloc-backed
variant of add_rec_entry_record_mode; the prologue's THREAD-record
insert uses it.
Verified: mcmini -i 3 on ~/dmtcp.git/test/tsan_target (plugin API v4)
no longer crashes in the thread-creation prologue; both worker
threads now run under RECORD mode.
Scope: this fixes the prologue only. mc_pthread_join still calls
pthread_timedjoin_np directly (no bypass handle), tripping libtsan's
ConsumeThreadUserId CHECK -- addressed next.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
libmcmini's template thread was created via libdmtcp_pthread_create,
bypassing libtsan's pthread_create interceptor. When the target is
built with -fsanitize=thread, that left the template thread with no
libtsan ThreadState. On DMTCP restart, libtsan's setjmp/longjmp
restore (reached from threadlist.cpp:stopthisthread) dereferences
the thread's absent ThreadState and crashes with SIGSEGV during
thread restore.
Create the template thread through the public pthread_create
instead, so libtsan's interceptor registers it and wraps its start
routine. A new thread-local flag mc_creating_internal_thread tells
mc_pthread_create to skip the user-thread/model-checking machinery
and still route the actual creation through DMTCP
(libdmtcp_pthread_create), keeping the thread DMTCP-known. When the
target is not instrumented, the public pthread_create resolves
straight to mc_pthread_create and behavior is unchanged.
With this, record -> checkpoint -> restart of a TSAN target survives
thread restore (verified: raw dmtcp_restart restores all threads
with no SIGSEGV, where it previously crashed).
mc_pthread_create()'s other branch, for ordinary application threads
in classic (non-DMTCP) mode, had the same underlying disease: its
TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case created every new
thread via libpthread_pthread_create(), resolved via
dlopen("libpthread")+dlsym() -- always real, raw glibc, bypassing
any interceptor unconditionally regardless of load order. TSan's own
pthread_create interceptor never got a chance to run for these
threads either. The very next libpthread_*-wrapped call from such a
thread (mc_register_this_thread() -> libpthread_mutex_lock() ->
libmcmini_init()'s lazy pthread_once()) hits TSan's own pthread_once
interceptor, which dereferences this thread's (nonexistent)
ThreadState and crashes: 100% reproducible SEGV in classic mode
against any TSan target (see
doc/classic-mode-thread-registration-segv.txt for the full backtrace
and analysis).
Fixed the same way in spirit: added tsan_or_real_pthread_create(),
resolved via dlsym(RTLD_NEXT, "pthread_create") instead of the
libpthread-handle dlsym. RTLD_NEXT finds whatever comes after
libmcmini.so in this process's actual load order: TSan's own
interceptor in classic mode (libmcmini loads ahead of libtsan
there), or real glibc under DMTCP (libtsan loads ahead of libmcmini
there, identical to today's behavior -- no regression risk for the
DMTCP+TSan restart flow, which handles its own recreated threads'
TSan registration via an unrelated mechanism, R3/R4's __clone +
fresh-fiber approach).
Verified: producer-consumer-tsan and cv-producer-consumer-tsan,
classic mode, zero crashes across multiple runs (previously 100%
crash). Classic-mode regression check (cv-test, deadly-embrace,
producer-consumer, cv-producer-consumer) unchanged. 15 consecutive
fresh-checkpoint --multithreaded-fork cycles against
producer-consumer-tsan under DMTCP still complete cleanly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
restart_child_threads_fast() recreates each pre-checkpoint thread via a raw clone() call, and fast_multithreaded_fork() forks the branch process via _Fork() -- both bypass every interceptor libtsan relies on to track threads, corrupting its runtime in three distinct ways: - R2: _Fork() skips libtsan's fork() interceptor entirely, so its BeforeFork/AfterFork pipeline never runs -- the child inherits TSan's internal locks still held and a ThreadRegistry full of now-dead parent threads. Bracket _Fork() with __sanitizer_syscall_pre/post_impl_fork (weak, no-op for non-TSan targets) to re-run that pipeline. - R3: the public clone() symbol hits libtsan's own interceptor, which treats every call as a fork and corrupts its thread-slot state for the CLONE_THREAD clone used here. Route through libc_clone(), a forwarding function dlsym'd from libc once at startup (added to interception.c's existing libc_abort()/libc_fork() table), rather than calling the raw __clone symbol directly -- DMTCP's own libdmtcp.so *also* strongly exports __clone and aborts unless the caller is mid-DMTCP's-own pthread_create, so bypassing libtsan's interceptor alone isn't enough; libc_clone() resolves the real, uninterposed primitive around both. - R4: a clone()-recreated thread never went through libtsan's pthread_create interceptor, so it has no valid TSan ThreadState; the forking thread's own inherited (fork-copied) state also has a shadow call stack that can overflow as it keeps running. Give both a fresh TSan fiber (weak __tsan_create_fiber/__tsan_switch_to_fiber, no-op for non-TSan targets) before either does any TSAN-intercepted work. Verified end-to-end against a real DMTCP checkpoint/restart of producer-consumer (record via `mcmini -i 1`, restart via `mcmini --from-checkpoint ... --multithreaded-fork`): the full pipeline completes cleanly, correct DEADLOCK detection, exit 0, "Deep debugging completed!", zero ASSERT/CHECK-failed/segfault occurrences. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements a helper function to detect if a thread has a signal blocked via its /proc/self/task/<tid>/status SigBlk mask. This will be consumed by Task 2 to identify and skip ThreadSanitizer's internal background thread during DMTCP checkpoint restart. - Add include/mcmini/spy/checkpointing/tsan_support.h with the function declaration - Add src/lib/tsan_support.c with implementation - Add test/tsan_support/test_thread_blocks_signal.c standalone unit test - Wire new source file into CMakeLists.txt LIBMCMINI_C_SRC list Test verified: compiles with -Wall -Werror, test passes (exit code 0). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Under a TSan-instrumented target, TSan's own background thread is spawned as a nested pthread_create() during the same window in which DMTCP creates its checkpoint thread. Both requests reach libmcmini's dmtcp_create_checkpoint_thread_wrapper(), which previously had no way to tell them apart at creation time. The real checkpoint thread could end up misclassified as an ordinary user thread and get recorded into the target's rec_list, corrupting the recording and causing "Expected a callback for 0" on restart. dmtcp_is_tsan_background_thread() resolves this, but classification requires DMTCP's endCkptThreadCreationWindow(), which can't run until the new thread has been registered with TSan -- which only happens once dmtcp_create_checkpoint_thread_wrapper() calls its real routine. Blocking on classification before that call therefore deadlocks. Fix: mark the thread as an unresolved candidate and run its real routine immediately, then resolve classification lazily on this same thread's first subsequent wrapped call (get_current_mode()), by which point TSan registration is guaranteed to have already happened.
Any thread whose libmcmini prologue runs before TSAN's own pthread_create registration trampoline has no TSAN ThreadState yet. This hit two cases: TSAN's own lazily-spawned background thread (nested inside TSAN's pthread_create interceptor while it's still handling DMTCP's checkpoint-thread creation request) and ordinary DMTCP-RECORD-mode worker threads (mc_thread_routine_wrapper's prologue runs before TSAN's trampoline). Both crashed the first time they made any wrapped call, since libmcmini_init()'s pthread_once() reaches TSAN's own interceptor, which dereferences that nonexistent ThreadState. libmcmini_init() now checks a plain, non-intercepted flag first and returns immediately once genuinely initialized, so pthread_once()/ TSAN's interceptor is only ever reached once, from a normal thread. mc_pthread_create() now also eagerly forces libmcmini_init() to complete on the creating thread (already TSAN-registered) before spawning, in both the PRE_CHECKPOINT_THREAD and RECORD/PRE_CHECKPOINT/DMTCP_RESTART_INTO_* cases -- pthread_create() is itself a synchronization point, so this guarantees the fast path applies on the new thread regardless of which of the two cases above it turns out to be. Confirmed live via gdb: both TSAN's background thread and DMTCP's real checkpoint thread now start and run normally. A dedicated reproducer follows in a later commit, once the surrounding TSan example infrastructure exists. Also documents (FIXME, no functional change) a separate, pre-existing, accepted trade-off: mc_pthread_join's RECORD-mode loop bypasses TSAN's join interceptor to avoid a harder abort, which makes TSAN report a (nonfatal by label, but exitcode-affecting) thread-leak warning instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The mode previously meant only "the DMTCP checkpoint thread is calling in". ThreadSanitizer's own internal background thread calls into libmcmini.so's overridden functions too, on a thread that is likewise not part of the target program, so get_current_mode() now reports the same value for it. Renamed to reflect the broader meaning. Detection reuses thread_blocks_signal() (already used by the R1 restart-barrier fix) via two new tsan_support.c helpers: mc_real_tid(), which reads /proc/thread-self since DMTCP virtualizes gettid() (including via the raw syscall path); and mc_is_current_thread_tsan_internal(), a per-thread-cached wrapper around it. Isolating which of thread_blocks_signal()'s calls were themselves TSan-safe required bisecting its RECORD-mode-original fopen/fgets/ sscanf/fclose implementation one call at a time. Findings: raw syscalls for openat/read/readlink are safe from a thread TSan has not yet registered, but even a raw syscall(SYS_close, fd) is not, so the one fd this code opens is deliberately never closed (bounded to one leak per thread, since the result is cached for that thread's lifetime).
mc_is_current_thread_tsan_internal() is only needed to distinguish TSan's internal thread in TARGET_BRANCH(_AFTER_RESTART) and DMTCP_RESTART_INTO_BRANCH/TEMPLATE. Its first call per thread leaks an fd (see tsan_support.c) pointing at that thread's own /proc/<pid>/task/<tid>/status, still open at checkpoint time, so DMTCP must restore a path whose pid/tid cannot exist after restart. Skip the check via a positive inclusion list of the four modes where it actually applies (matching every wrapper's own case-label grouping), rather than an incomplete negative exclusion list that missed PRE_DMTCP_INIT and PRE_CHECKPOINT_THREAD. Confirmed via /proc/<pid>/fd census: zero leaked fds for the whole RECORD phase, and template_thread() now wakes up after restart instead of hanging there. This alone left a separate potential hang open: restart from a checkpoint recorded this way still hung, later, in template_thread()'s dmtcp_restart_sem count loop. Resolved by an earlier commit, "Fix restart-barrier race in template_thread()", which switched that count from a live /proc/self/task scan to head_record_mode's THREAD entries -- a list TSan's own background thread never reaches, since it is spawned via the same PRE_CHECKPOINT_THREAD path already excluded from that list for the checkpoint thread itself. Reverified live: DMTCP+TSan record/restart of producer-consumer-park-tsan now completes cleanly, with no hang in the consistent-state barrier.
Under DMTCP, libtsan.so resolves ahead of libmcmini.so, so a target's own pthread_join() calls reach TSan's interceptor first. For a thread DMTCP resurrected via clone() (bypassing TSan's pthread_create() interceptor), TSan can never resolve its Tid and hangs forever -- see TSAN-pthread-join.md. Fix: link the target with -Wl,--wrap=pthread_join plus the new pthread_join_wrap.c, rewriting the target's own pthread_join() calls at link time, so they reach mc_pthread_join_maybe_defer() before the dynamic linker (and TSan) ever sees them. A DMTCP-resurrected thread's join is handled entirely via a really_exited_sem handshake; any thread with a valid TSan Tid (classic mode, or created via pthread_create() after restart) is still really joined via __real_pthread_join(), so TSan keeps seeing and tracking that join normally.
Without --wrap (see src/lib/pthread_join_wrap.c), a DMTCP-resurrected thread's pthread_join() can hang forever under TSan. Detect both conditions via weak symbols (same idiom as the other TSan/DMTCP optional-symbol checks in this codebase): __tsan_acquire is non-NULL only when libtsan.so is loaded (i.e. a TSan target), and __wrap_pthread_join is non-NULL only if the target itself was linked with --wrap. Warn as early as possible (a constructor) when the former is true and the latter isn't. Verified via `mcmini`: a TSan target built without --wrap prints the warning, producer-consumer-tsan (built with --wrap) and a plain non-TSan target both stay silent.
Intercepts pthread_exit(), routing it through the existing THREAD_EXIT_TYPE model-checking machinery instead of falling through to the real libpthread pthread_exit, which crashes on a __clone()-recreated thread (not registered with libtsan's real interceptor). No example ever called pthread_exit() explicitly, so this path was never exercised. Added producer-consumer-exit(-tsan) to test it, which uncovered two bugs: 1. The TARGET_BRANCH case never actually terminated the thread, falling through to libc_abort() instead. Fixed by calling the real libpthread_pthread_exit() there. 2. That alone crashes DMTCP-restart of a pre-checkpoint thread (SIGSEGV): such a thread runs on a TSan fiber, not a real TSan thread, and glibc's pthread_exit() cleanup-unwind crashes against that fiber's state. Fixed by detecting recreated threads (mc_pthread_is_recreated_thread()) and terminating them via a fiber switch + raw exit syscall instead. mc_pthread_is_recreated_thread(), added for bug 2 above, also fixed a third, unrelated bug: found while stress-testing --multithreaded-fork with a target whose worker threads finish before main ever joins them (i.e. they exit during RECORD mode, well before any checkpoint). mc_pthread_join_impl's TARGET_BRANCH_AFTER_RESTART case treated every pre-restart thread as one DMTCP resurrected via clone(), waiting on that thread's really_exited_sem -- but really_exited_sem is only ever posted by mc_exit_thread_in_child(), which only runs for a thread actually executing post-restart. A thread that already, genuinely exited before the checkpoint never runs that code again, so the wait blocks forever. Fixed using the same mc_pthread_is_recreated_thread() check to distinguish the two cases: if a pre-restart thread was never actually recreated by restart_child_threads_fast(), it's already exited for good, and the model already knows this (see translate_recorded_runner_to_model()) -- the join is trivially satisfied, so return immediately instead of waiting. Regression-checked clean against all 6 existing TSan targets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds exit-stress.c / exit-stress-noop.c and their -tsan CMake targets: stress tests for pthread_exit() robustness at higher thread counts, and the reproducer used to find and confirm the TSAN ThreadState fix from "Fix TSAN ThreadState crashes before registration". Confirmed live via gdb: both TSAN's background thread and DMTCP's real checkpoint thread now start and run normally; exit-stress-noop-tsan completes cleanly across repeated runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mcmini_log() calls localtime() from any thread, unsynchronized. glibc's tzset_internal() (invoked on every localtime_r() call, not just the first) is not safe to run concurrently even when TZ never changes, so under TSan this showed up as a data race and, at least once, a subsequent SEGV. A real pthread mutex around the whole function isn't enough by itself: it's taken via libpthread_mutex_lock(), which resolves straight to libpthread's own symbol (see mc_load_intercepted_pthread_functions() in interception.c), bypassing TSan's interceptor entirely. So, the lock genuinely serializes the calls, but TSan never sees it and keeps reporting a race. Fix that with TSan's own public annotation API (__tsan_acquire/__tsan_release, declared weak, so this is a no-op without a TSan runtime in the process) around the same lock. Also switch localtime() to localtime_r() with a stack-local struct tm, and add tzset() as a constructor, so the timezone database is loaded once, single-threaded, before any thread can race on it. Verified against a freshly-recorded DMTCP checkpoint (rebuilding libmcmini.so does not affect restarts of an already-recorded checkpoint, which pins whatever plugin build was loaded at record time): three consecutive restarts now show zero tzset_internal races, versus 3/3 racing beforehand.
mc_pthread_mutex_lock()/unlock() and mc_pthread_cond_wait()'s real mutex
release/reacquire all go through libpthread_mutex_lock/unlock/timedlock,
resolved via a raw dlopen("libpthread.so")+dlsym() that always bypasses
TSan's interceptor. TSan never sees these events. So it can't establish
a happens-before edge between critical sections in different threads --
reporting a false race on every variable the mutex protects (e.g.
cv-producer-consumer's count/buffer), even though mcmini's own scheduler
correctly serializes access. Same class of bug already fixed once for
mcmini_log()'s own mutex (log.c); this applies the same __tsan_acquire/
__tsan_release annotation fix to the application's mutex.
Dedicated -safe test targets exercising this fix follow in a later
commit, once the surrounding TSan example infrastructure exists.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A CMake-managed, TSan-instrumented build of producer-consumer, wired with -Wl,--wrap=pthread_join and src/lib/pthread_join_wrap.c, so it exercises the DMTCP-restart + TSan pthread_join fix without a manual ad hoc compile step. do_recording() (src/mcmini/mcmini.cpp) resolves the DMTCP plugin path as getcwd()/libmcmini.so, so mcmini must be invoked from the directory holding libmcmini.so -- CMAKE_BINARY_DIR, not this target's own output directory (build/src/examples/). A manually-placed copy in CMAKE_BINARY_DIR from days ago silently went stale relative to rebuilds of libmcmini.so and this target itself, costing significant debugging time chasing what looked like a live TSan/DMTCP hang but was actually just a pre-fix binary missing -Wl,--wrap=pthread_join's effect. Add a POST_BUILD copy, so this can't happen again. Also adds cv-producer-consumer-tsan: same build recipe, for testing McMini's DMTCP-restart + TSan integration against condition variables specifically (see doc/glibc-cond-var-desync.txt). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TSan's pthread_cond_wait interceptor implements the whole wait itself and never delegates to libmcmini, unlike mutex_lock/sem_wait/cond_signal (confirmed via GOT inspection: all resolve to libtsan, but only cond_wait never forwards). Under DMTCP a checkpoint/restart-resurrected thread genuinely blocked inside TSan's own real, untimed wait, with nothing left to wake it after the earlier CV desync fix. --wrap it at the target's link step, same approach as pthread_join_wrap.c. Verified live: restart-from-checkpoint of a thread mid-cond_wait, which previously hung forever, now completes promptly with the wait visible as a real modeled transition. See doc/cond-wait-tsan-interceptor-bypass.txt, which also records a second, separate CV-state-reconstruction bug this verification uncovered (tracked separately, not fixed here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
multithreaded_fork()'s process-duplication step is _Fork() (glibc's
internal, minimal fork primitive), which deliberately skips
pthread_atfork() handlers -- the mechanism a real fork() would use to
let a lock protecting shared state reset itself safely in the child.
mcmini_log()'s own serialization mutex (log_mut) can be left frozen,
permanently locked if _Fork() snapshots memory for a new branch at the
exact instant some other thread (most plausibly the template thread,
which logs constantly) holds it -- every thread in that branch later
deadlocks calling mcmini_log(). Reproduced live via gdb + a raw memory
read: log_mut's lock word was 2 ("locked, with waiter"), a genuinely
correct glibc state nobody alive in the process could ever release.
Full analysis in doc/log-mutex-fork-desync.txt.
Fixed with mcmini_log_reset_after_fork(), which force-reinitializes
log_mut via the libpthread bypass handle (libpthread_mutex_init(), not
the plain pthread_mutex_init() -- that symbol is libmcmini's own
interposed mc_pthread_mutex_init(), which would recursively re-enter
its own restart-mode dispatch and assert on a tid_self that isn't
valid yet this early after _Fork()). Called from
fast_multithreaded_fork() (dmtcp-callback.c)'s childpid==0 branch,
right after the existing TSan fiber switch and before
restart_child_threads_fast() -- while still the only OS thread alive
in the child, so the reset can never race a concurrent lock/unlock
attempt.
(An earlier version of this fix wired the reset into
src/common/multithreaded_fork.c's multithreaded_fork() instead --
confirmed dead code, superseded by fast_multithreaded_fork() per
commit 74679cf, so that version of the fix was never actually active.
This is the corrected version, verified against the real
--multithreaded-fork path.)
Verified: 40 consecutive fresh-checkpoint producer-consumer-tsan
--multithreaded-fork cycles, zero hangs, zero crashes. The originally
observed hang rate was roughly 1-in-13 to 1-in-25.
Adds producer-consumer-safe.c / cv-producer-consumer-safe.c and their -tsan CMake targets to formally exercise the __tsan_acquire/__tsan_release annotation fix from "Fix false TSan races on mutex-protected globals". Verified: producer-consumer-safe-tsan went from 79 false-positive race reports across classic-mode's explored interleavings to zero; cv-producer-consumer-safe-tsan from 208 (all attributable to the unrelated a[] race below) to zero. DMTCP+TSan record/restart cycles for both stayed clean. While chasing this, a live gdb backtrace caught TSan mid-report for a genuine, separate, pre-existing race in the original examples' main() (both pthread_create loops reuse one shared, undersized thread-argument array, letting main() overwrite a producer's slot while it's still being read). That race's report-generation happening to overlap a DMTCP checkpoint signal is what caused a real deadlock against TSan's own internal locking -- a narrow DMTCP/TSan interaction, not a mcmini bug, and left alone in the original files. The new -safe variants fix just that race, so other tests can isolate future bugs from it. Full analysis in doc/tsan-mutex-annotation-false-positive.txt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pthread_cond_signal (and broadcast) never reach mc_pthread_cond_signal() under DMTCP, contrary to doc/cond-wait-tsan-interceptor-bypass.txt's assumption -- confirmed live via a call-count diagnostic. Needs the same -Wl,--wrap treatment already applied to pthread_cond_wait; added pthread_cond_signal_wrap.c and wired it into both cv-producer-consumer TSan targets. Wrapping cond_signal newly exposed a second, pre-existing bug: RECORD mode's mc_pthread_cond_signal()/_broadcast() abort if the condition variable's record doesn't exist yet, but pthread_cond_init() has the same interceptor-bypass problem, so a producer signaling before any consumer has ever waited legitimately hits this. Mirrors mc_pthread_cond_wait()'s own lazy-init instead of aborting. Regression-checked 5 consecutive record+restart cycles each on cv-producer-consumer(-safe)-tsan plus the existing producer-consumer family, all clean.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/examples/producer-consumer-park.c (1)
12-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the busy wait with an absolute monotonic sleep.
This loop consumes a CPU core for the full ten-second test window. Retry
clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, ...)onEINTRso DMTCP signals do not shorten the delay.Proposed change
+#include <errno.h> + -static void busy_wait_seconds(int seconds) { - struct timespec start, now; - clock_gettime(CLOCK_MONOTONIC, &start); - do { - clock_gettime(CLOCK_MONOTONIC, &now); - } while (now.tv_sec - start.tv_sec < seconds); +static void wait_seconds(int seconds) { + struct timespec deadline; + int rc; + + clock_gettime(CLOCK_MONOTONIC, &deadline); + deadline.tv_sec += seconds; + do { + rc = clock_nanosleep(CLOCK_MONOTONIC, TIMER_ABSTIME, &deadline, NULL); + } while (rc == EINTR); } ... - busy_wait_seconds(10); + wait_seconds(10);🤖 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/examples/producer-consumer-park.c` around lines 12 - 23, Replace the CPU-consuming loop in busy_wait_seconds with an absolute CLOCK_MONOTONIC clock_nanosleep targeting the start time plus the requested seconds. Retry the sleep when it returns EINTR so interruptions do not shorten the full delay, while preserving the function’s existing duration behavior.src/lib/tsan_support.c (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale
(void)signo;.
signois used at line 89. The cast-to-void marker states the opposite and misleads a reader into thinking the parameter is ignored.♻️ Proposed change
int thread_blocks_signal(pid_t tid, int signo) { - (void)signo; char path[64];🤖 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/tsan_support.c` at line 31, Remove the stale (void)signo; statement from the signal-handling function, leaving the existing signo usage unchanged.src/examples/cv-producer-consumer.c (1)
73-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the argument-array race is intentional.
Line 80 overwrites
a[i]while the producer thread can still read the same slot throughpnoat line 31. This is the deliberate TSan-detectable race thatcv-producer-consumer-safe.ccontrasts against, and that file carries an explaining comment at its lines 73-76. This file carries none. A future reader can "repair" the race and silently remove the test case.♻️ Proposed comment
+ // INTENTIONAL DATA RACE (the -safe variant of this file removes it): + // one shared argument array means the second loop below overwrites a + // producer's slot while that producer thread may still be reading it + // via pno. Do not "fix" this: it is the TSan-detectable race this + // example exists to produce. int a[NUM_PRODUCERS > NUM_CONSUMERS ? NUM_PRODUCERS : NUM_CONSUMERS];🤖 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/examples/cv-producer-consumer.c` around lines 73 - 82, Add a concise explanatory comment adjacent to the producer/consumer thread setup, especially the consumer-loop assignment to a[i], documenting that overwriting the argument array while producers may still read pno is intentional and preserves this TSan-detectable race. Match the explanatory intent of cv-producer-consumer-safe.c without changing the implementation.
🤖 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 `@doc/glibc-sem-desync.txt`:
- Around line 88-93: Update the condition-variable status documentation: in
doc/glibc-sem-desync.txt lines 88-93, replace the “Not yet fixed” text with a
reference to doc/glibc-cond-var-desync.txt; in doc/glibc-cond-var-desync.txt
lines 123-127, make verification reference cv-producer-consumer-safe-tsan and
its DMTCP+TSan coverage; and in lines 129-140, remove or revise the obsolete
timed-wait warning to reflect that mc_pthread_cond_wait() recomputes an absolute
deadline on each retry.
In `@include/mcmini/model/transitions/process/exit.hpp`:
- Line 25: Update the exit transition around add_state_for_runner to pass a
const model::runner_state* rather than new thread(thread::exited). Create the
appropriate runner_state object for the exited state, or call the existing state
mutator overload that accepts a thread object, while preserving the executor’s
exited state.
In `@src/lib/interception.c`:
- Line 125: Check the dlsym result for clone_ptr in src/lib/interception.c lines
125-125 immediately after resolving __clone; on NULL, print dlerror() and call
libc_abort(), matching nearby dlopen failure handling. Also check
real_start_main in src/lib/interception.c lines 392-400 before invoking it; on
NULL, print dlerror() and call _exit(1), not libc_abort(), because
initialization has not yet run.
In `@src/lib/log.c`:
- Around line 88-93: Synchronize the global_log_level access in mcmini_log by
making it atomic or by guarding the read with level_mut and applying matching
TSan annotations to the read and the write in mcmini_log_set_level. Preserve the
existing log-level filtering behavior while ensuring concurrent level updates do
not race.
---
Nitpick comments:
In `@src/examples/cv-producer-consumer.c`:
- Around line 73-82: Add a concise explanatory comment adjacent to the
producer/consumer thread setup, especially the consumer-loop assignment to a[i],
documenting that overwriting the argument array while producers may still read
pno is intentional and preserves this TSan-detectable race. Match the
explanatory intent of cv-producer-consumer-safe.c without changing the
implementation.
In `@src/examples/producer-consumer-park.c`:
- Around line 12-23: Replace the CPU-consuming loop in busy_wait_seconds with an
absolute CLOCK_MONOTONIC clock_nanosleep targeting the start time plus the
requested seconds. Retry the sleep when it returns EINTR so interruptions do not
shorten the full delay, while preserving the function’s existing duration
behavior.
In `@src/lib/tsan_support.c`:
- Line 31: Remove the stale (void)signo; statement from the signal-handling
function, leaving the existing signo usage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 256ac28e-4bfc-445b-9bd8-c7edf8826ffe
📒 Files selected for processing (56)
CMakeLists.txtdoc/classic-mode-thread-registration-segv.txtdoc/cond-wait-tsan-interceptor-bypass.txtdoc/glibc-cond-var-desync.txtdoc/glibc-sem-desync.txtdoc/log-mutex-fork-desync.txtdoc/pthread-exit-abort-and-fiber-crash.txtdoc/tsan-mutex-annotation-false-positive.txtinclude/dmtcp.hinclude/mcmini/lib/log.hinclude/mcmini/mem.hinclude/mcmini/model/objects/condition_variables.hppinclude/mcmini/model/objects/mutex.hppinclude/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hppinclude/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hppinclude/mcmini/model/transitions/condition_variables/condition_variables_signal.hppinclude/mcmini/model/transitions/condition_variables/condition_variables_wait.hppinclude/mcmini/model/transitions/mutex/mutex_init.hppinclude/mcmini/model/transitions/mutex/mutex_unlock.hppinclude/mcmini/model/transitions/process/exit.hppinclude/mcmini/real_world/mailbox/runner_mailbox.hinclude/mcmini/real_world/process/dmtcp_process_source.hppinclude/mcmini/spy/checkpointing/objects.hinclude/mcmini/spy/checkpointing/record.hinclude/mcmini/spy/checkpointing/tsan_support.hinclude/mcmini/spy/intercept/interception.hinclude/mcmini/spy/intercept/wrappers.hsrc/common/mem.csrc/common/multithreaded_fork.csrc/common/runner_mailbox.csrc/examples/CMakeLists.txtsrc/examples/cv-producer-consumer-safe.csrc/examples/cv-producer-consumer.csrc/examples/exit-stress-noop.csrc/examples/exit-stress.csrc/examples/producer-consumer-exit.csrc/examples/producer-consumer-park.csrc/examples/producer-consumer-safe.csrc/lib/dmtcp-callback.csrc/lib/interception.csrc/lib/log.csrc/lib/main.csrc/lib/pthread_cond_signal_wrap.csrc/lib/pthread_cond_wait_wrap.csrc/lib/pthread_join_wrap.csrc/lib/record.csrc/lib/sem-wrappers.csrc/lib/tsan_support.csrc/lib/wrappers.csrc/mcmini/mcmini.cppsrc/mcmini/model/transitions/condition_variables.cppsrc/mcmini/model/transitions/mutex.cppsrc/mcmini/model_checking/algorithms/classic_dpor.cppsrc/mcmini/real_world/dmtcp_process_source.cppsrc/mcmini/real_world/local_linux_process.cpptest/tsan_support/test_thread_blocks_signal.c
💤 Files with no reviewable changes (1)
- src/mcmini/real_world/dmtcp_process_source.cpp
🚧 Files skipped from review as they are similar to previous changes (41)
- include/mcmini/lib/log.h
- test/tsan_support/test_thread_blocks_signal.c
- src/lib/sem-wrappers.c
- include/mcmini/mem.h
- include/mcmini/model/transitions/condition_variables/condition_variables_wait.hpp
- src/examples/exit-stress-noop.c
- src/mcmini/model/transitions/mutex.cpp
- include/mcmini/spy/checkpointing/objects.h
- src/mcmini/model/transitions/condition_variables.cpp
- src/mcmini/model_checking/algorithms/classic_dpor.cpp
- src/lib/record.c
- src/lib/pthread_cond_wait_wrap.c
- CMakeLists.txt
- include/mcmini/real_world/process/dmtcp_process_source.hpp
- include/mcmini/model/transitions/condition_variables/condition_variables_signal.hpp
- src/lib/pthread_cond_signal_wrap.c
- include/mcmini/real_world/mailbox/runner_mailbox.h
- doc/pthread-exit-abort-and-fiber-crash.txt
- doc/classic-mode-thread-registration-segv.txt
- src/lib/main.c
- include/mcmini/model/objects/mutex.hpp
- include/mcmini/model/transitions/condition_variables/condition_variable_brdcast.hpp
- include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp
- src/common/multithreaded_fork.c
- doc/cond-wait-tsan-interceptor-bypass.txt
- src/mcmini/mcmini.cpp
- src/mcmini/real_world/local_linux_process.cpp
- include/mcmini/spy/checkpointing/tsan_support.h
- src/common/runner_mailbox.c
- include/mcmini/model/transitions/mutex/mutex_unlock.hpp
- include/mcmini/spy/intercept/wrappers.h
- src/common/mem.c
- include/mcmini/spy/checkpointing/record.h
- src/lib/pthread_join_wrap.c
- include/mcmini/spy/intercept/interception.h
- src/examples/CMakeLists.txt
- include/dmtcp.h
- include/mcmini/model/transitions/mutex/mutex_init.hpp
- src/examples/exit-stress.c
- include/mcmini/model/objects/condition_variables.hpp
- src/lib/wrappers.c
| Not yet fixed | ||
| --------------- | ||
| Condition variables (pthread_cond_wait/pthread_cond_signal) have the same | ||
| class of vulnerability via glibc's G1/G2 waiter-group bookkeeping, for the | ||
| same reason (checkpoint/restart + externally-managed reinitialization). An | ||
| analogous fix has not yet been applied there. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale condition-variable status statements.
The diagnostic documents describe condition-variable work as incomplete even though this PR documents and implements the fix, the TSan-safe target, and the absolute-deadline correction.
doc/glibc-sem-desync.txt#L88-L93: replace the “Not yet fixed” statement with a reference todoc/glibc-cond-var-desync.txt.doc/glibc-cond-var-desync.txt#L123-L127: update the verification section to referencecv-producer-consumer-safe-tsanand its DMTCP+TSan coverage.doc/glibc-cond-var-desync.txt#L129-L140: remove or revise the obsolete timed-wait warning because the suppliedmc_pthread_cond_wait()implementation now computes a fresh absolute deadline each retry.
📍 Affects 2 files
doc/glibc-sem-desync.txt#L88-L93(this comment)doc/glibc-cond-var-desync.txt#L123-L127doc/glibc-cond-var-desync.txt#L129-L140
🤖 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 `@doc/glibc-sem-desync.txt` around lines 88 - 93, Update the condition-variable
status documentation: in doc/glibc-sem-desync.txt lines 88-93, replace the “Not
yet fixed” text with a reference to doc/glibc-cond-var-desync.txt; in
doc/glibc-cond-var-desync.txt lines 123-127, make verification reference
cv-producer-consumer-safe-tsan and its DMTCP+TSan coverage; and in lines
129-140, remove or revise the obsolete timed-wait warning to reflect that
mc_pthread_cond_wait() recomputes an absolute deadline on each retry.
| // exit code its program_exit_code() > 0 check doesn't already catch | ||
| // (i.e. exit code 0). | ||
| using namespace model::objects; | ||
| s.add_state_for_runner(executor, new thread(thread::exited)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Pass a model::runner_state to add_state_for_runner.
Line 25 passes model::objects::thread*, but add_state_for_runner requires const model::runner_state*. This prevents the project from compiling. Create the required runner-state object, or use the state mutator that accepts a thread object.
🧰 Tools
🪛 Clang (14.0.6)
[error] 25-25: cannot initialize a parameter of type 'const model::runner_state *' with an rvalue of type 'model::objects::thread *'
(clang-diagnostic-error)
🤖 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 `@include/mcmini/model/transitions/process/exit.hpp` at line 25, Update the
exit transition around add_state_for_runner to pass a const model::runner_state*
rather than new thread(thread::exited). Create the appropriate runner_state
object for the exited state, or call the existing state mutator overload that
accepts a thread object, while preserving the executor’s exited state.
Source: Linters/SAST tools
| exit_ptr = dlsym(libc_handle, "exit"); | ||
| abort_ptr = dlsym(libc_handle, "abort"); | ||
| fork_ptr = dlsym(libc_handle, "fork"); | ||
| clone_ptr = dlsym(libc_handle, "__clone"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked dlsym results in src/lib/interception.c turn a symbol-resolution failure into a NULL call. Both new lookups store the returned pointer without testing it. The failure is then deferred to the call site, where it appears as a NULL function-pointer call with no diagnostic, in contexts that are hard to debug: a freshly forked restart child, and pre-main() startup. The surrounding code already aborts loudly on dlopen failure; apply the same policy to these dlsym results.
src/lib/interception.c#L125-L125: testclone_ptrimmediately after thedlsym(libc_handle, "__clone")call. If it isNULL, printdlerror()and calllibc_abort(), matching thedlopenfailure blocks above.src/lib/interception.c#L392-L400: testreal_start_mainbefore calling it. If it isNULL, printdlerror()and call_exit(1). Do not uselibc_abort()here, becauselibmcmini_init()has deliberately not run yet andabort_ptris stillNULL.
📍 Affects 1 file
src/lib/interception.c#L125-L125(this comment)src/lib/interception.c#L392-L400
🤖 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/interception.c` at line 125, Check the dlsym result for clone_ptr in
src/lib/interception.c lines 125-125 immediately after resolving __clone; on
NULL, print dlerror() and call libc_abort(), matching nearby dlopen failure
handling. Also check real_start_main in src/lib/interception.c lines 392-400
before invoking it; on NULL, print dlerror() and call _exit(1), not
libc_abort(), because initialization has not yet run.
| void mcmini_log(int level, const char *file, int line, const char *fmt, ...) { | ||
| if (level < global_log_level) { | ||
| return; | ||
| } | ||
| libpthread_mutex_lock(&log_mut); | ||
| if (__tsan_acquire) __tsan_acquire(&log_mut); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Synchronize the global_log_level read.
mcmini_log_set_level() writes global_log_level under level_mut, but Line 89 reads it without that mutex or an atomic operation. A concurrent level update races with logging. Make global_log_level atomic, or protect and TSan-annotate both accesses with level_mut.
🤖 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/log.c` around lines 88 - 93, Synchronize the global_log_level access
in mcmini_log by making it atomic or by guarding the read with level_mut and
applying matching TSan annotations to the read and the write in
mcmini_log_set_level. Preserve the existing log-level filtering behavior while
ensuring concurrent level updates do not race.
cv-producer-consumer-multi-tsan: 5 producers + 5 consumers on a shared mutex+condvar, exercising 3+ same-role threads no existing -safe target here does (they're all 1+1) -- this is what exposed the mc_pthread_cond_wait() RECORD-mode annotation gap fixed in "Fix false TSan races on mutex-protected globals". script/check-tsan.sh runs it through mcmini -i and fails on any TSan summary beyond the accepted thread leak. Wired into a new 'make check-tsan' target (top-level CMakeLists.txt), which also picks up a local dmtcp.git/bin build for PATH/LD_LIBRARY_PATH when present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DMTCP now supports TSAN. Working on porting this support to McMini/DeepDebug.
Part of the issue is that McMini, TSAN and DMTCP all have wrapper functions, and they can interfere with each other if we are not careful.
This includes PR #7, PR #12, etc. Then this commit. PR #12 is the first 6 commits of this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation