notsan nomultifork nocondvar - #13
Conversation
mc_pthread_join's RECORD loop called pthread_timedjoin_np directly, which resolves to libtsan's interceptor under a TSAN target. Its ConsumeThreadUserId trips a thread-registry CHECK (sanitizer_thread_registry.cpp:348) and aborts. Add a libpthread_timedjoin_np handle (dlsym'd from libpthread, like the mutex/cond/sem wrappers) that bypasses libtsan, and call it from mc_pthread_join's RECORD loop instead of the raw symbol. This completes end-to-end TSAN-target checkpointing under deep-debug (mcmini record mode), alongside 5be8500 (DMTCP plugin API v3->v4) and 4bf2720 (TSan-safe RECORD prologue). Verified: `mcmini -i 3 ~/dmtcp.git/test/tsan_target` runs with no SEGV/ThreadSanitizer errors, producing a valid checkpoint matching the no-mcmini baseline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TSAN's pthread_join() interceptor delegates to a genuine OS-level join and blocks via the kernel until the target thread actually dies -- it does not rely on its own creation-time bookkeeping. But mc_pthread_join()'s TARGET_BRANCH case only simulated success at the model level, while the joined thread was kept parked in thread_block_indefinitely() forever, so a real join on it (e.g. from TSan) could never complete. Give each thread its own exit_permission_sem (alongside its existing pthread_map entry). A finishing thread waits on it before returning; mc_pthread_join() posts it and performs a real libpthread_pthread_join() before returning, and as a bonus, pthread_join returns a return value.
classic_dpor::verify_using()'s forward-exploration path catches real_world::process::termination_error and reports it via the abnormal_termination callback, letting the run end cleanly. The backtrack-replay path (coordinator::return_to_depth(), which replays prior transitions against a freshly restarted process) had no such handling, so the same exception there escaped all the way to the top-level catch-all instead. Wrap return_to_depth() the same way. found_abnormal_termination() also needed a null check: return_to_depth()'s target thread may have no pending transition in the model's current view (unlike the forward path, where the culprit is always the runner DPOR just selected as enabled). The report then falls back to a plain "no longer pending" line instead of dereferencing a null transition.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change updates DMTCP public interfaces, replaces the child mailbox semaphore with a raw futex counter, adds a parked producer-consumer example, improves thread join and exit handling, and adds detailed process termination diagnostics. ChangesRuntime integration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant mc_pthread_join
participant exit_permission_sem
participant child_thread
participant libpthread
mc_pthread_join->>exit_permission_sem: post join permission
exit_permission_sem-->>child_thread: permit termination
mc_pthread_join->>libpthread: perform real pthread join
libpthread-->>mc_pthread_join: joined thread result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/wrappers.c (1)
43-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBoth lookups dereference / return interior pointers after releasing the read lock.
search_pthread_map()readscur->valueafterpthread_rwlock_unlock(), andfind_exit_permission_sem()hands out&cur->exit_permission_sem— a pointer into amalloc()ed node — to a caller that will use it arbitrarily later (Line 820). This is only sound if map nodes are never removed or freed for the lifetime of the process. That happens to be true today (I see no erase path), but it is an unstated invariant guarding a use-after-free.Read the value inside the critical section, and document the node-immortality invariant on the
find_exit_permission_sem()declaration.♻️ Read under the lock
runner_id_t search_pthread_map(pthread_t t) { pthread_rwlock_rdlock(&pthread_map_lock); pthread_map_t *cur = head; while (cur) { if (pthread_equal(cur->thread, t)) { break; } cur = cur->next; } - pthread_rwlock_unlock(&pthread_map_lock); - return cur == NULL ? RID_INVALID : cur->value; + runner_id_t result = cur == NULL ? RID_INVALID : cur->value; + pthread_rwlock_unlock(&pthread_map_lock); + return result; }🤖 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 43 - 69, Update search_pthread_map() to copy cur->value into a local result while pthread_map_lock is held, then unlock and return that copied value. Document on the find_exit_permission_sem() declaration that returned semaphore pointers reference map nodes that must remain allocated and are intentionally never removed or freed for the process lifetime.src/mcmini/mcmini.cpp (1)
132-214: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate reporting logic between
found_abnormal_terminationandfound_nonzero_exit_code.Both functions independently rebuild the trace string, resolve the (possibly-null) pending transition for the culprit, and print "NEXT THREAD OPERATIONS" + transition count with identical logic. Extracting a shared helper (parameterized on the culprit id, exception message, and header text) would prevent the two from drifting apart on the next fix.
♻️ Sketch of a shared helper
static void report_trace_and_culprit(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(); }Then
found_abnormal_termination/found_nonzero_exit_codewould just print their own header line and call this helper.🤖 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 132 - 214, Extract the duplicated trace, culprit-transition, pending-operation, and transition-count reporting from found_abnormal_termination and found_nonzero_exit_code into a shared helper parameterized by coordinator, stats, and culprit ID. Keep each function’s distinct error header and message output, then call the helper to preserve the existing reporting behavior.src/mcmini/model_checking/algorithms/classic_dpor.cpp (1)
223-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBacktrack-phase re-expansion is missing the same termination/nonzero-exit catches added elsewhere.
continue_dpor_by_expanding_trace_withis demonstrably capable of throwingtermination_error/nonzero_exit_code_error(per the catches around its forward-exploration use at lines 171-179). This second call site, used when following a backtrack thread, only catchesundefined_behavior_exception. If the backtracked thread's replay hits a process termination or nonzero exit here, it propagates uncaught out ofverify_using()and crashes the checker instead of invokingcallbacks.abnormal_termination/callbacks.nonzero_exit_codelike every other path in this function now does.🐛 Proposed fix
try { this->continue_dpor_by_expanding_trace_with( dpor_stack.back().backtrack_set_pop_first(), context); } catch (const model::undefined_behavior_exception &ube) { if (callbacks.undefined_behavior) callbacks.undefined_behavior(coordinator, model_checking_stats, ube); return; + } catch (const real_world::process::termination_error &te) { + if (callbacks.abnormal_termination) + callbacks.abnormal_termination(coordinator, model_checking_stats, te); + return; + } catch (const real_world::process::nonzero_exit_code_error &nzec) { + if (callbacks.nonzero_exit_code) + callbacks.nonzero_exit_code(coordinator, model_checking_stats, nzec); + return; }🤖 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/model_checking/algorithms/classic_dpor.cpp` around lines 223 - 230, Update the backtrack-phase call to continue_dpor_by_expanding_trace_with so its catch handling also covers termination_error and nonzero_exit_code_error, matching the forward-exploration handling in verify_using(). Invoke callbacks.abnormal_termination or callbacks.nonzero_exit_code with the existing coordinator and model_checking_stats context, then return instead of allowing either exception to escape.
🧹 Nitpick comments (5)
doc/glibc-sem-desync.txt (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAbbreviated commit SHA will likely dangle.
1a4b3d9refers to a commit on this un-merged branch; if the PR is squashed or rebased the reference dies. Prefer describing the change (as the following section already does) without the hash.🤖 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 57 - 60, Update the paragraph in doc/glibc-sem-desync.txt to remove the abbreviated commit reference “1a4b3d9” and describe the child_side_sem replacement directly, matching the explanatory wording in the following section.include/dmtcp.h (1)
248-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the anonymous-union placeholder.
int32_t _;alongsidepid_t _pidconveys no intent (on Linuxpid_tis alreadyint32_t), and anonymous unions in a struct are C11/GNU-only. If the goal is a fixed-width on-disk layout forDmtcpCkptHeader, consider naming it (e.g._pid_placeholder) with a comment stating the wire-format intent.🤖 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 248 - 255, Update the anonymous union in DmtcpUniqueProcessId by replacing the ambiguous int32_t _ member with a descriptive placeholder name such as _pid_placeholder, and document that it preserves the fixed-width wire/on-disk layout expected by DmtcpCkptHeader. Keep the existing pid_t _pid member and union layout unchanged.include/mcmini/real_world/mailbox/runner_mailbox.h (1)
11-25: 🗄️ Data Integrity & Integration | 🔵 TrivialShared-memory layout change: stale checkpoint images / libmcmini builds now mismatch silently.
runner_mailboxis mapped by both the verifier and the target, andsem_t→uint32_tshifts every subsequent field. A checkpoint image produced by a pre-changelibmcmini.soand restarted under a post-changemcminiwill readtype/cntsat the wrong offsets with no diagnostic. Consider bumping/adding a layout version word validated atmc_runner_mailbox_init()time, or documenting that existing.dmtcpimages must be regenerated.The rationale comment itself is excellent and worth keeping.
🤖 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/real_world/mailbox/runner_mailbox.h` around lines 11 - 25, Add an explicit mailbox layout version field to runner_mailbox and validate it during mc_runner_mailbox_init(), rejecting or clearly diagnosing checkpoints created with an incompatible layout before accessing shifted fields such as type or cnts. Update the initialization path consistently for newly created mailboxes while preserving the existing semaphore rationale comment and synchronization behavior.src/common/runner_mailbox.c (1)
3-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
#include "string.h"should be<string.h>.Quoted form searches the local directory first; this is a system header.
🤖 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/runner_mailbox.c` around lines 3 - 18, Change the string header include in the runner mailbox implementation from quoted form to angle-bracket form, leaving the futex-related includes and mc_futex function unchanged.src/examples/CMakeLists.txt (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PRIVATEfor executable targets, and considerRUNTIME_OUTPUT_DIRECTORYover a copy command.
PUBLICon an executable exports usage requirements no one consumes;PRIVATEis the idiomatic choice. The POST_BUILD copy also singles this target out from every other example — setting the output directory achieves the same thing declaratively and avoids a self-copy error ifCMAKE_BINARY_DIRever equals this target's output dir.♻️ Suggested form
add_executable(producer-consumer-park-tsan producer-consumer-park.c) -target_compile_options(producer-consumer-park-tsan PUBLIC -fsanitize=thread) -target_link_options(producer-consumer-park-tsan PUBLIC -fsanitize=thread) -target_link_libraries(producer-consumer-park-tsan PUBLIC -pthread libmcmini) - -add_custom_command(TARGET producer-consumer-park-tsan POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy - $<TARGET_FILE:producer-consumer-park-tsan> - ${CMAKE_BINARY_DIR}/producer-consumer-park-tsan) +target_compile_options(producer-consumer-park-tsan PRIVATE -fsanitize=thread) +target_link_options(producer-consumer-park-tsan PRIVATE -fsanitize=thread) +target_link_libraries(producer-consumer-park-tsan PRIVATE -pthread libmcmini) +set_target_properties(producer-consumer-park-tsan PROPERTIES + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR})🤖 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 20 - 28, Update the producer-consumer-park-tsan target to use PRIVATE for its compile options, link options, and pthread/libmcmini dependencies instead of PUBLIC. Replace its POST_BUILD copy command with the target’s RUNTIME_OUTPUT_DIRECTORY property configured to place the executable in CMAKE_BINARY_DIR.
🤖 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 `@include/dmtcp.h`:
- Around line 317-318: Update the sizeof check for DmtcpCkptHeader in dmtcp.h so
the assertion is available under C++ or C11-and-later only, preserving the
4096-byte validation without requiring C11 from older C consumers. Guard the
existing static_assert with the specified language-version condition, or use a
compatible _Static_assert form for C.
In `@include/mcmini/model/objects/mutex.hpp`:
- Around line 21-33: Remove the zero-argument mutex constructor from class
mutex, preserving only constructors that require an explicit state and location.
Keep the existing parameterized constructor and its RID_INVALID default for
owner unchanged, and do not introduce placeholder semantics.
In `@src/common/runner_mailbox.c`:
- Around line 20-37: Update mc_raw_sem_wait() so an interrupted FUTEX_WAIT
returns an EINTR-like failure instead of looping back to wait; preserve retry
behavior for EAGAIN and successful waits, and retain -1 for other unhandled
futex errors. Remove the unnecessary errno = 0 reset before mc_futex.
In `@src/lib/interception.c`:
- Around line 343-351: Update __libc_start_main to validate the result of
dlsym(RTLD_NEXT, "__libc_start_main") before invoking it. If real_start_main is
NULL, handle the failure safely and return without calling through the null
function pointer; preserve the existing wrapped_main invocation when resolution
succeeds.
In `@src/lib/wrappers.c`:
- Around line 359-368: Replace the assert-only NULL handling for
find_exit_permission_sem() in src/lib/wrappers.c:359-368 with an explicit
runtime check that logs and aborts or preserves the prior indefinite-park
behavior before libpthread_sem_wait(). Apply the same check at
src/lib/wrappers.c:812-821 before libpthread_sem_post(), propagating an error
back through mc_pthread_join() when the semaphore is unavailable.
- Around line 812-821: Replace the blocking libpthread_pthread_join call in
thread_join with a timed or otherwise bounded join using the coordinator’s
established timeout contract. Preserve posting exit_permission first, propagate
successful join results through rv, and fail loudly when the timeout expires so
the model cannot remain waiting indefinitely after THREAD_JOIN_TYPE is consumed.
In `@src/mcmini/real_world/local_linux_process.cpp`:
- Around line 110-129: Update the wait loop around sig_semwait(),
try_consume_signal(), and waitpid() so continuation depends on the mailbox reply
slot remaining unread, not solely on the global SIGCHLD counter. Preserve
processing when waitpid(this->pid, ..., WNOHANG) reports the target changed
state, and when it returns 0 only continue waiting if no mailbox reply is
available. Handle waitpid() returning -1 by treating ECHILD separately and
propagating other errors.
- Around line 66-78: Replace the blanket waitpid(-1, ..., WNOHANG) loop in the
local_linux_process cleanup path with a non-blocking reap restricted to
this->pid, preserving signal_tracker consumption only when that specific child
is reaped. Do not consume or reap unrelated McMini-managed children; handle any
additional SIGCHLD sources explicitly without stealing descendants owned by
fork_process_source.
---
Outside diff comments:
In `@src/lib/wrappers.c`:
- Around line 43-69: Update search_pthread_map() to copy cur->value into a local
result while pthread_map_lock is held, then unlock and return that copied value.
Document on the find_exit_permission_sem() declaration that returned semaphore
pointers reference map nodes that must remain allocated and are intentionally
never removed or freed for the process lifetime.
In `@src/mcmini/mcmini.cpp`:
- Around line 132-214: Extract the duplicated trace, culprit-transition,
pending-operation, and transition-count reporting from
found_abnormal_termination and found_nonzero_exit_code into a shared helper
parameterized by coordinator, stats, and culprit ID. Keep each function’s
distinct error header and message output, then call the helper to preserve the
existing reporting behavior.
In `@src/mcmini/model_checking/algorithms/classic_dpor.cpp`:
- Around line 223-230: Update the backtrack-phase call to
continue_dpor_by_expanding_trace_with so its catch handling also covers
termination_error and nonzero_exit_code_error, matching the forward-exploration
handling in verify_using(). Invoke callbacks.abnormal_termination or
callbacks.nonzero_exit_code with the existing coordinator and
model_checking_stats context, then return instead of allowing either exception
to escape.
---
Nitpick comments:
In `@doc/glibc-sem-desync.txt`:
- Around line 57-60: Update the paragraph in doc/glibc-sem-desync.txt to remove
the abbreviated commit reference “1a4b3d9” and describe the child_side_sem
replacement directly, matching the explanatory wording in the following section.
In `@include/dmtcp.h`:
- Around line 248-255: Update the anonymous union in DmtcpUniqueProcessId by
replacing the ambiguous int32_t _ member with a descriptive placeholder name
such as _pid_placeholder, and document that it preserves the fixed-width
wire/on-disk layout expected by DmtcpCkptHeader. Keep the existing pid_t _pid
member and union layout unchanged.
In `@include/mcmini/real_world/mailbox/runner_mailbox.h`:
- Around line 11-25: Add an explicit mailbox layout version field to
runner_mailbox and validate it during mc_runner_mailbox_init(), rejecting or
clearly diagnosing checkpoints created with an incompatible layout before
accessing shifted fields such as type or cnts. Update the initialization path
consistently for newly created mailboxes while preserving the existing semaphore
rationale comment and synchronization behavior.
In `@src/common/runner_mailbox.c`:
- Around line 3-18: Change the string header include in the runner mailbox
implementation from quoted form to angle-bracket form, leaving the futex-related
includes and mc_futex function unchanged.
In `@src/examples/CMakeLists.txt`:
- Around line 20-28: Update the producer-consumer-park-tsan target to use
PRIVATE for its compile options, link options, and pthread/libmcmini
dependencies instead of PUBLIC. Replace its POST_BUILD copy command with the
target’s RUNTIME_OUTPUT_DIRECTORY property configured to place the executable in
CMAKE_BINARY_DIR.
🪄 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: ee242865-f0fa-4c17-b0ad-eb5906eb132d
📒 Files selected for processing (22)
doc/glibc-sem-desync.txtinclude/dmtcp.hinclude/mcmini/model/objects/mutex.hppinclude/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.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/intercept/interception.hsrc/common/multithreaded_fork.csrc/common/runner_mailbox.csrc/examples/CMakeLists.txtsrc/examples/producer-consumer-park.csrc/lib/dmtcp-callback.csrc/lib/interception.csrc/lib/wrappers.csrc/mcmini/mcmini.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.cpp
💤 Files with no reviewable changes (1)
- src/mcmini/real_world/dmtcp_process_source.cpp
|
|
||
| static_assert(sizeof(DmtcpCkptHeader) == 4096, "DmtcpCkptHeader must be 4096 bytes"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the C standard the build uses and whether any C target includes dmtcp.h
fd -e txt -e cmake --full-path 'CMakeLists.txt' --exec rg -n 'C_STANDARD|std=(gnu|c)[0-9]+|CMAKE_C_FLAGS' {} \;
rg -n '`#include`\s*[<"].*dmtcp\.h' --glob '*.c' --glob '*.h'Repository: mcminickpt/deep-debug
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -u
echo "== repository files of interest =="
git ls-files | rg '(^|/)CMakeLists\.txt$|include/dmtcp\.h$|\.h$|\.c$' | sed -n '1,160p'
echo
echo "== dmtcp.h relevant section =="
if [ -f include/dmtcp.h ]; then
nl -ba include/dmtcp.h | sed -n '270,330p'
else
echo "include/dmtcp.h not found"
fi
echo
echo "== C standard settings =="
for f in CMakeLists.txt `fd -e txt -e cmake 'CMakeLists.txt'`; do
echo "--- $f"
rg -n 'C_STANDARD|C_STANDARD_REQUIRED|CMAKE_C_STANDARD|std=(gnu|c)[0-9]+|CMAKE_C_FLAGS|CXX_STANDARD|CXX_STANDARD_REQUIRED' "$f" || true
done
echo
echo "== dmtcp.h includes and static_assert usages nearby =="
rg -n '`#include`\s*[<"].*dmtcp\.h|static_assert|_Static_assert|RESTORE_BUF_TOTAL_SIZE|DmtcpCkptHeader' -g '*.c' -g '*.h' -g '*.cpp' -g '*.cc' .Repository: mcminickpt/deep-debug
Length of output: 2809
🏁 Script executed:
#!/bin/bash
set -u
echo "== include/dmtcp.h relevant section =="
awk 'NR>=270 && NR<=340 { printf "%6d %s\n", NR, $0 }' include/dmtcp.h
echo
echo "== current plugin source includes around dmtcp.h =="
for f in src/common/multithreaded_fork.c src/lib/interception.c src/lib/dmtcp-callback.c src/lib/main.c src/lib/log.c src/lib/template/loop.c src/lib/wrappers.c; do
echo "--- $f"
sed -n '1,25p' "$f" | awk '{ printf "%4d %s\n", NR, $0 }'
doneRepository: mcminickpt/deep-debug
Length of output: 9505
Avoid forcing C11 for pre-existing dmtcp.h consumers.
The project build sets C11, but include/dmtcp.h is a public header included by plugin code, so bare static_assert can break downstream non-C11 consumers. Keep the check, but guard it with defined(__cplusplus) || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L) or use _Static_assert for C compatibility.
🤖 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 317 - 318, Update the sizeof check for
DmtcpCkptHeader in dmtcp.h so the assertion is available under C++ or
C11-and-later only, preserving the 4096-byte validation without requiring C11
from older C consumers. Guard the existing static_assert with the specified
language-version condition, or use a compatible _Static_assert form for C.
| mutex() = default; | ||
| ~mutex() = default; | ||
| mutex(const mutex &) = default; | ||
| mutex(state s) : current_state(s) {} | ||
| mutex(state s, pthread_mutex_t* loc) : current_state(s), location(loc) {} | ||
| mutex(state s, pthread_mutex_t* loc, runner_id_t tid): current_state(s), location(loc), owner(tid) {} | ||
| // `location` has no default member initializer (unlike, say, | ||
| // condition_variable's `policy`), so it must always be passed explicitly | ||
| // -- a mutex without its real address is meaningless, and every | ||
| // mutex_lock/unlock faithfully forwards whatever get_location() returns, | ||
| // so an uninitialized one corrupts every subsequent state derived from | ||
| // it (see doc history around commit 9bd9ecf). `tid` defaults to | ||
| // RID_INVALID: "unlocked, no owner" is exactly what callers that omit it | ||
| // (mutex_init, condition_variable_enqueue_thread) mean. | ||
| mutex(state s, pthread_mutex_t* loc, runner_id_t tid = RID_INVALID) | ||
| : current_state(s), location(loc), owner(tid) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for zero-arg construction/default-construction of model::objects::mutex.
set -euo pipefail
rg -nP '\bmutex\s+\w+\s*;' --type=cpp -g '!**/build/**'
rg -nP 'new\s+mutex\s*\(\s*\)' --type=cpp
ast-grep run --pattern 'mutex $NAME;' --lang cpp include srcRepository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching mutex.hpp =="
fd -a 'mutex\.hpp$' . | sed 's#^\./##'
echo "== outline for candidate include/mcmini/model/objects/mutex.hpp =="
if [ -f include/mcmini/model/objects/mutex.hpp ]; then
ast-grep outline include/mcmini/model/objects/mutex.hpp || true
echo "== relevant source =="
cat -n include/mcmini/model/objects/mutex.hpp | sed -n '1,120p'
fi
echo "== broad searches for object declarations/usages =="
rg -n '\bmutex\b' src include tests -g '*.cpp' -g '*.hpp' -g '*.h' || true
echo "== searches for default object variables containing mutex in name/type context =="
rg -n 'mutex\s+\w+\s*;' src include tests -g '*.cpp' -g '*.hpp' -g '*.h' || trueRepository: mcminickpt/deep-debug
Length of output: 15182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== constructors/calls =="
rg -n 'mutex\s*\(' src include tests -g '*.cpp' -g '*.hpp' -g '*.h' || true
echo "== default-constructible data structures containing mutex =="
rg -n 'std::(vector|array|map|set|deque|list|queue|stack)<.*mutex|std::unordered_(map|set)<.*mutex' include src tests -g '*.cpp' -g '*.hpp' -g '*.h' || trueRepository: mcminickpt/deep-debug
Length of output: 2930
Remove the zero-argument mutex constructor.
location and owner still have no default member initializers, so mutex() can leave both uninitialized until add_state_for_obj creates a new object. That violates the “a mutex without its real address is meaningless” invariant and can corrupt lock/unlock state derived through get_location(). Delete the default constructor or provide explicit invalid-member semantics for placeholder objects.
🤖 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 21 - 33, Remove the
zero-argument mutex constructor from class mutex, preserving only constructors
that require an explicit state and location. Keep the existing parameterized
constructor and its RID_INVALID default for owner unchanged, and do not
introduce placeholder semantics.
| static int mc_raw_sem_wait(volatile uint32_t *sem) { | ||
| while (1) { | ||
| uint32_t cur = __atomic_load_n(sem, __ATOMIC_SEQ_CST); | ||
| if (cur > 0) { | ||
| uint32_t expected = cur; | ||
| if (__atomic_compare_exchange_n(sem, &expected, cur - 1, 0, | ||
| __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)) { | ||
| return 0; | ||
| } | ||
| continue; | ||
| } | ||
| errno = 0; | ||
| long rc = mc_futex(sem, FUTEX_WAIT, 0); | ||
| if (rc == -1 && errno != EAGAIN && errno != EINTR) { | ||
| return -1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What does McMini's SIGUSR1 handler do, and do callers depend on EINTR from mc_wait_for_scheduler?
rg -n -C 8 'SIGUSR1'
rg -n -C 8 'mc_wait_for_scheduler'Repository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repo files (sample):\n'
git ls-files | sed -n '1,120p'
printf '\nFind mailbox / wrappers / process files:\n'
git ls-files | rg 'runner_mailbox|wrappers|process|mailbox|signal|SIGUSR1|wait_for_scheduler' || true
printf '\nSearch relevant identifiers:\n'
rg -n -C 6 'mc_raw_sem_wait|mc_wait_for_scheduler|sigaction|signal\\(.*SIGUSR1|SIGUSR1|kill\\(.*MSG|local_linux_process|futex|sem_wait' || trueRepository: mcminickpt/deep-debug
Length of output: 6948
🏁 Script executed:
#!/bin/bash
set -u
printf 'Relevant identifiers:\n'
rg -n -C 6 'mc_raw_sem_wait|mc_wait_for_scheduler|sigaction|signal|SIGUSR1|SIGKILL|kill|mc_futex|FUTEX_WAIT|sem_wait|errno' src include || true
printf '\nrunner_mailbox.c:\n'
cat -n src/common/runner_mailbox.c
printf '\nwrappers.c relevant section:\n'
cat -n src/lib/wrappers.c | sed -n '80,120p'
printf '\nlocal_linux_process.cpp relevant destructor/process section:\n'
cat -n src/mcmini/real_world/local_linux_process.cpp | sed -n '1,220p'
printf '\nsignal.cpp:\n'
cat -n src/mcmini/signal.cpp
printf '\nlocal_linux_process.hpp:\n'
cat -n include/mcmini/real_world/process/local_linux_process.hpp | sed -n '1,220p'Repository: mcminickpt/deep-debug
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -u
printf 'runner_mailbox.c:\n'
cat -n src/common/runner_mailbox.c
printf '\nwrappers.c around libpthread_sem_wait_loop:\n'
cat -n src/lib/wrappers.c | sed -n '80,130p'
printf '\nsem-wrappers.c:\n'
cat -n src/lib/sem-wrappers.c | sed -n '1,130p'
printf '\nlocal_linux_process headers and destructor/kill usages:\n'
cat -n include/mcmini/real_world/process/local_linux_process.hpp | sed -n '1,220p'
cat -n src/mcmini/real_world/local_linux_process.cpp | sed -n '1,240p'
printf '\nsignal.cpp:\n'
cat -n src/mcmini/signal.cpp | sed -n '1,260p'
printf '\nLocal grep all files for SIGUSR1/userr1/wait_for_scheduler:\n'
rg -n --hidden --glob '!build/**' --glob '!dist/**' 'SIGUSR1|sigusr1|SIGUSR1|SIG_USR1|mc_wait_for_scheduler|mc_raw_sem_wait|FUTEX_WAIT|errno = 0|ERRNO_SET|EINTR' . || true
printf '\nStatic verifier of wait loop control flow and wrapper call sites:\n'
python3 - <<'PY'
from pathlib import Path
import re
for p in [Path('src/common/runner_mailbox.c'), Path('src/lib/wrappers.c'), Path('src/mcmini/real_world/local_linux_process.cpp')]:
print(f'== {p} ==')
data = p.read_text()
if 'mc_raw_sem_wait' in p.name or 'wrappers' in p.name:
for m in re.finditer(r'int\s+mc_raw_sem_wait[\s\S]+?^\}', data, re.M):
s=m.start(); e=m.end(); print(f'@{data.count("\n",0,m.start())+1}-{data.count("\n",0,m.end())+1}')
body=data[s:e]
print(body)
for name in ['EINTR','continue','FUTEX_WAIT','errno = 0','return -1']:
print(f'contains {name!r}:', name in body)
elif 'local_linux_process.cpp' in p.name:
for txt in ['kill(', 'mc_wait_for_scheduler', 'mc_wait_for_thread', 'SIGUSR1']:
print(f'contains {txt!r}:', txt in data)
for i,line in enumerate(data.splitlines(),1):
if 'kill' in line or 'SIGUSR1' in line or 'mc_wait_for' in line:
print(f'{i}: {line.strip()}')
PYRepository: mcminickpt/deep-debug
Length of output: 35094
🏁 Script executed:
#!/bin/bash
set -u
printf 'loop.c around template SIGUSR1 handling/futex wait:\n'
cat -n src/lib/template/loop.c | sed -n '130,220p'
printf '\ninterception.c relevant syscall interception section:\n'
cat -n src/lib/interception.c | sed -n '260,330p'
printf '\nStatic control-flow check (no f-string):\n'
python3 - <<'PY'
from pathlib import Path
for path in ["src/common/runner_mailbox.c", "src/lib/wrappers.c", "src/mcmini/real_world/local_linux_process.cpp"]:
text = Path(path).read_text()
print(f"== {path} ==")
for needle in ["mc_raw_sem_wait", "mc_wait_for_scheduler", "kill(pid, SIGUSR1)", "while (1)", "FUTEX_WAIT", "EINTR"]:
print(f"{needle}: {needle in text}")
PYRepository: mcminickpt/deep-debug
Length of output: 7829
Make mc_raw_sem_wait() return on interruption.
mc_wait_for_scheduler() is now mc_raw_sem_wait() directly, and its callers retry only on EINTR; with the unconditional retry inside mc_raw_sem_wait(), the child-side semaphore no longer lets signals unblock the parked thread. Return an EINTR-like value for unhandled FUTEX_WAIT interruptions instead of always looping back to the atomic wait.
The dangling errno = 0 line is only a minor cleanup, since failed FUTEX_WAIT already leaves errno set.
🤖 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/runner_mailbox.c` around lines 20 - 37, Update mc_raw_sem_wait()
so an interrupted FUTEX_WAIT returns an EINTR-like failure instead of looping
back to wait; preserve retry behavior for EAGAIN and successful waits, and
retain -1 for other unhandled futex errors. Remove the unnecessary errno = 0
reset before mc_futex.
| int __libc_start_main(main_fn main, int argc, char **argv, void (*init)(void), | ||
| void (*fini)(void), void (*rtld_fini)(void), | ||
| void *stack_end) { | ||
| real_main = main; | ||
| libc_start_main_fn real_start_main = | ||
| (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); | ||
| return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, | ||
| stack_end); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which processes get libmcmini.so preloaded, and is the hook gated anywhere?
rg -n -C 5 'LD_PRELOAD'
rg -n -C 10 'mc_transparent_exit' --glob '*.c'Repository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repository files matching mcmini/dmtcp/libc_start_main:\n'
git ls-files | rg -n '(^|/)(src/lib/interception\.c|.*mcmini.*|.*dmtcp.*|.*launch.*|.*restart.*|.*coordinator.*|.*test.*|README|.*\.md)$' || true
printf '\nFind interception.c and related symbols:\n'
fd -a 'interception\.c$|mcmini|dmtcp' . 2>/dev/null | sed 's#^\./##' | head -200
printf '\nInterception file stats and outline:\n'
if [ -f src/lib/interception.c ]; then
wc -l src/lib/interception.c
ast-grep outline src/lib/interception.c || true
printf '\nRelevant sections:\n'
sed -n '1,120p' src/lib/interception.c | cat -n
sed -n '300,380p' src/lib/interception.c | cat -n
fi
printf '\nSearch for dlopen/dlsym/RTLD_NEXT/preload-related code:\n'
rg -n -C 4 'dlopen|dlsym|RTLD_NEXT|RTLD_LAZY|LD_PRELOAD|__libc_start_main|libmcmini_init|libc_abort|mc_transparent_exit|MCMINI|MC_MINI|mcm' . --glob '!*.png' --glob '!*.jpg' || trueRepository: mcminickpt/deep-debug
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -u
printf 'Files likely defining wrappers/exit/mode:\n'
fd -a 'wrappers\.(c|h)$|.*exit.*\.c$|.*mode.*\.(c|h)$|.*interception.*\.(c|h)$' src include . 2>/dev/null | sed 's#^\./##' | head -200
printf '\nmc_transparent_exit definitions/usages:\n'
rg -n -C 8 'mc_transparent_exit|libc_abort|dlopen\(.*lib(pthread|c)|enum.*mode|TARGET_|RECORD|DMTCP|libmcmini_mode|get_current_mode|is_.*mode|MODE_.*' src --glob '*.c' --glob '*.h' | head -400
printf '\nLocate libmcmini_init/libpthread handles declarations:\n'
rg -n -C 4 'static.*libpthread_handle|void libmcmini_init|typedef.*libc_start_main|real_start_main|wrapped_main\(' src include --glob '*.c' --glob '*.h'Repository: mcminickpt/deep-debug
Length of output: 31588
🏁 Script executed:
#!/bin/bash
set -ul
# Use exact search from result while avoiding the large-output failure mode.
printf 'LD_PRELOAD references:\n'
rg -n -C 5 'LD_PRELOAD' . --glob '!*.png' --glob '!*.jpg' || true
printf '\nDmtcp launcher/restart references in repository:\n'
rg -n -C 4 'dmtcp_(launch|restart|coordinator)|DMTCP_(LAUNCH|RERUN|RESTART)|MCMINI_PROCESS|TARGET|premain|PRE_DMTCP-init|MCMINI_MODE' . --glob '!*.png' --glob '!*.jpg' --glob '!*build*' --glob '!*.o' --glob '!*.a' | head -400Repository: mcminickpt/deep-debug
Length of output: 31681
🏁 Script executed:
#!/bin/bash
set -u
printf 'Exit wrappers implementation:\n'
wc -l src/lib/wrappers.c src/common/exit.c
sed -n '1,220p' src/common/exit.c | cat -n
sed -n '220,340p' src/lib/wrappers.c | cat -n
printf '\nPreload setup sites:\n'
rg -n -C 8 'set_preload_libmcmini|LD_PRELOAD|with-plugin|--with-plugin' src include --glob '*.cpp' --glob '*.hpp' --glob '*.c' --glob '*.h'
printf '\nMode enum definitions:\n'
rg -n -C 6 'typedef enum|enum libmcmini_mode|PRE_DMTCP_INIT|RECORD|TARGET_BRANCH|TARGET_TEMPLATE|DMTCP_RESTART_INTO|PRE_CHECKPOINT' src include --glob '*.cpp' --glob '*.hpp' --glob '*.c' --glob '*.h'
printf '\nStatic behavioral probe for dlsym NULL fallback and process preload reachability:\n'
python3 - <<'PY'
from pathlib import Path
import re
interception = Path("src/lib/interception.c").read_text()
wrappers = Path("src/lib/wrappers.c").read_text()
target_hpp = Path("include/mcmini/real_world/target.hpp").read_text()
callsite = re.search(r'libc_start_main_fn real_start_main =\s*\([\s\S]*?dlsym\(RTLD_NEXT,\s*"__libc_start_main"\);\s*return\s+real_start_main', interception)
has_null_guard = bool(re.search(r'real_start_main\s*==\s*NULL|NULL\s*==\s*real_start_main', callsite.group(0) if callsite else ""))
exit_called_after_lib_main = bool(re.search(r'int\s+real_main\s*[=;].*?\n.*?libmcmini_init\(\);\s*\n.*?mc_transparent_exit\s*\(', interception, re.S))
preload_function = re.search(r'void\s+set_preload_libmcmini\s*\([^)]*\)\s*\{(?P<body>.*?}\n\}|\{\s*)', target_hpp, re.S)
uses_env = bool(preload_function and re.search(r'getenv\(["'']LD_PRELOAD["'']\)', preload_function.group(0)))
print({"interception_dlsym_called": bool(callsite),
"interception_returns_unchecked_dlsym_result": not has_null_guard and bool(callsite),
"exit_called_after_libmcmini_init_from_wrapped_main": exit_called_after_lib_main,
"target_preload_uses_LD_PRELOAD_env": uses_env})
dmtcp_main = Path("include/mcmini/real_world/dmtcp_target.hpp").read_text() if Path("include/mcmini/real_world/dmtcp_target.hpp").exists() else ""
# External DMTCP implementation is not in this repo, but the repository explicitly constructs
# "dmtcp_launch" as the exec target before letting libmcmini interpose the target.
print("local_code_constructs_dmtcp_launch_target", "dmtcp_launch" in Path("src/mcmini/mcmini.cpp").read_text())
print("local_code_sets_libmcmini_preload_for_target", bool(set_preload_libmcmini := re.search(r'set_preload_libmcmini\s*\(', target_hpp)))
PYRepository: mcminickpt/deep-debug
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -u
printf 'Locate dmtcp target setup files:\n'
git ls-files | rg '(^|/)(target|dmtcp_target)\.(cpp|h)$' || true
for f in $(git ls-files | rg '(^|/)(target|dmtcp_target)\.(cpp|h)$'); do
echo "--- $f"
wc -l "$f"
sed -n '1,240p' "$f" | cat -n
rg -n -C 8 'exec|dlopen|plugin|preload|LD_PRELOAD|MCMINI|target_executable|set_preload_libmcmini' "$f" || true
done
printf '\nCheck whether dmtcp_main.cpp is vendored/configured:\n'
git ls-files | rg 'dmtcp_main\.(cpp|c)$|dmtcp_launch' || trueRepository: mcminickpt/deep-debug
Length of output: 17781
Guard dlsym(RTLD_NEXT, "__libc_start_main") before calling it.
__libc_start_main is a real target symbol, but resolving it with RTLD_NEXT can return NULL; calling that null pointer from an interposed entry point crashes the process before normal DMTCP/McMini initialization or logging can happen.
🛡️ Minutimal guard
real_main = main;
libc_start_main_fn real_start_main =
(libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main");
+ if (real_start_main == NULL) {
+ fprintf(stderr,
+ "libmcmini: dlsym(RTLD_NEXT, \"__libc_start_main\") failed: %s\n",
+ dlerror());
+ fflush(stderr);
+ libc_abort();
+ }
return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini,
stack_end);📝 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.
| int __libc_start_main(main_fn main, int argc, char **argv, void (*init)(void), | |
| void (*fini)(void), void (*rtld_fini)(void), | |
| void *stack_end) { | |
| real_main = main; | |
| libc_start_main_fn real_start_main = | |
| (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); | |
| return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, | |
| stack_end); | |
| } | |
| int __libc_start_main(main_fn main, int argc, char **argv, void (*init)(void), | |
| void (*fini)(void), void (*rtld_fini)(void), | |
| void *stack_end) { | |
| real_main = main; | |
| libc_start_main_fn real_start_main = | |
| (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); | |
| if (real_start_main == NULL) { | |
| fprintf(stderr, | |
| "libmcmini: dlsym(RTLD_NEXT, \"__libc_start_main\") failed: %s\n", | |
| dlerror()); | |
| fflush(stderr); | |
| libc_abort(); | |
| } | |
| return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, | |
| stack_end); | |
| } |
🤖 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 343 - 351, Update __libc_start_main to
validate the result of dlsym(RTLD_NEXT, "__libc_start_main") before invoking it.
If real_start_main is NULL, handle the failure safely and return without calling
through the null function pointer; preserve the existing wrapped_main invocation
when resolution succeeds.
|
|
||
| // Wait for whichever thread eventually calls pthread_join() on this one | ||
| // (see mc_pthread_join()'s TARGET_BRANCH case) 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); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
assert() is the sole NULL guard on both exit_permission_sem lookups. Both new call sites establish the pointer with find_exit_permission_sem() and validate it only via assert(), which compiles out under NDEBUG — leaving a NULL semaphore passed straight to libpthread. Since the function returns NULL for any thread not (yet) registered in the pthread map, this is a reachable path, not an impossible one.
src/lib/wrappers.c#L359-L368: replace theassertwith a real check beforelibpthread_sem_wait(); on NULL, log and abort (or fall back to the previous indefinite park) rather than dereferencing.src/lib/wrappers.c#L812-L821: apply the same explicit check beforelibpthread_sem_post(), returning an error to the caller ofmc_pthread_join()instead of relying onassert.
📍 Affects 1 file
src/lib/wrappers.c#L359-L368(this comment)src/lib/wrappers.c#L812-L821
🤖 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 359 - 368, Replace the assert-only NULL
handling for find_exit_permission_sem() in src/lib/wrappers.c:359-368 with an
explicit runtime check that logs and aborts or preserves the prior
indefinite-park behavior before libpthread_sem_wait(). Apply the same check at
src/lib/wrappers.c:812-821 before libpthread_sem_post(), propagating an error
back through mc_pthread_join() when the semaphore is unavailable.
|
|
||
| // Grant the target thread permission to really terminate now that | ||
| // it's been joined (see mc_exit_thread_in_child()), then perform a | ||
| // genuine join, so its OS thread actually dies -- and *rv is | ||
| // genuinely populated -- before we return, instead of only | ||
| // simulating success at the model level. | ||
| sem_t *exit_permission = find_exit_permission_sem(t); | ||
| assert(exit_permission != NULL); | ||
| libpthread_sem_post(exit_permission); | ||
| return libpthread_pthread_join(t, rv); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the model only enable a thread-join transition after the target thread's exit transition?
rg -n -C 20 'thread_join' include/mcmini/model/transitions/ src/mcmini/model/
rg -n -C 10 'THREAD_JOIN_TYPE|THREAD_EXIT_TYPE'Repository: mcminickpt/deep-debug
Length of output: 11801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate wrappers.c and related thread/semaphore implementation files.
git ls-files | rg '(^|/)wrappers\.c$|(^|/)wrapper|libpthread|pthread|thread|semaphore|runner_mailbox' | head -200
printf '\n--- snippets around lines 330-400 and 760-850 in src/lib/wrappers.c ---\n'
wc -l src/lib/wrappers.c
sed -n '330,410p' src/lib/wrappers.c
sed -n '760,870p' src/lib/wrappers.c
printf '\n--- scheduler/wait/wake definitions/usages in src/lib/wrappers.c ---\n'
rg -n -C 8 'mc_wait_for_scheduler|mc_post_event|mc_post|mc_wake|pthread_join|THREAD_JOIN_TYPE|exit_permission|sem_t|libpthread_sem_post|sem_post' src/lib/wrappers.c
printf '\n--- candidate files containing libpthread_pthread_join and event handling ---\n'
rg -n -C 5 'libpthread_pthread_join|pthread_join|THREAD_JOIN_TYPE|mc_wait_for_scheduler|mc_post_event|mc_wake_thread' src includeRepository: mcminickpt/deep-debug
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the model transition shapes and thread exit/exit_permission state.
wc -l src/mcmini/model/transitions/thread/thread_exit.hpp src/mcmini/model/objects/thread.hpp
sed -n '1,220p' src/mcmini/model/transitions/thread/thread_exit.hpp
sed -n '1,220p' src/mcmini/model/threads/objects/thread.hpp
sed -n '1,180p' src/mcmini/model/transitions/thread/thread.cpp
printf '\n--- transition.cpp thread exit/join code ---\n'
sed -n '1,220p' src/mcmini/model/transitions/thread.cpp
printf '\n--- model state references to exit_permission_sem / has_exited / THREAD_JOIN_TYPE ---\n'
rg -n -C 4 'has_exited|exit_transition|exit_permission_sem|has_exited\(\)|THREAD_JOIN_TYPE' src/mcmini/model src/lib/wrappers.c includeRepository: mcminickpt/deep-debug
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Add targeted searches for scheduler/mailbox wake behavior and DMTCP restart handlers.
rg -n -C 6 'void .*handle_after_dmtcp_restart|handle_after_dmtcp_restart\(' src include
rg -n -C 6 'model_side_|child_side_sem|mc_handle_after|run.*transition|THREAD_EXIT_TYPE' src includeRepository: mcminickpt/deep-debug
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/lib/dmtcp-callback.c thread_handle_after_dmtcp_restart ---'
sed -n '220,265p' src/lib/dmtcp-callback.c
printf '%s\n' '--- src/mcmini/mcmini.cpp thread exit/join dispatch ---'
sed -n '300,370p' src/mcmini/mcmini.cpp
printf '%s\n' '--- src/mcmini/model/transitions/thread.cpp ---'
cat -n src/mcmini/model/transitions/thread.cpp
printf '%s\n' '--- src/mcmini/model/objects/semaphore.hpp ---'
wc -l src/mcmini/model/objects/semaphore.hpp
cat -n src/mcmini/model/objects/semaphore.hpp
printf '%s\n' '--- src/mcmini/model/transitions/semaphore/sem_post.hpp ---'
wc -l src/mcmini/model/transitions/semaphore/sem_post.hpp
cat -n src/mcmini/model/transitions/semaphore/sem_post.hpp
printf '%s\n' '--- targeted thread model/transition includes ---'
find src/mcmini/model -path '*thread*' -type f -print -o -name 'thread*.hpp' -type f -print | sortRepository: mcminickpt/deep-debug
Length of output: 7234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- git tracked object/transition files relevant to thread/semaphore ---'
git ls-files | rg 'src/mcmini/model/(objects/transitions).*\.(hpp|cpp)$|src/mcmini/mcmini\.cpp$|src/lib/dmtcp-callback\.c$|src/common/runner_mailbox\.c$|src/lib/wrappers\.c$|src/lib/dmtcp-callback\.c$' | sed -n '1,220p'
printf '%s\n' '--- src/mcmini/model/objects/*.hpp files ---'
git ls-files 'src/mcmini/model/objects/*.hpp' | while read -r f; do
echo "### $f"
wc -l "$f"
sed -n '1,240p' "$f"
done
printf '%s\n' '--- src/mcmini/model/transitions/thread/*.hpp files ---'
git ls-files 'src/mcmini/model/transitions/thread/*.hpp' | while read -r f; do
echo "### $f"
wc -l "$f"
sed -n '1,240p' "$f"
done
printf '%s\n' '--- semantic ordering probe: thread_join dependency/coenabled ---'
python3 - <<'PY'
from pathlib import Path
p = Path('src/mcmini/model/transitions/thread/thread_join.hpp')
src = p.read_text()
print('thread_join modify has_exited:', 'has_exited()' in src)
print('depends target == executor:', 'this->target == t->get_executor()' in src)
print('coenabled excludes executor:', 'this->target != t->get_executor()' in src)
PY
printf '%s\n' '--- common/runner_mailbox waiting model side ---'
sed -n '1,130p' src/common/runner_mailbox.cRepository: mcminickpt/deep-debug
Length of output: 1055
Use a timed join with the coordinator’s timeout contract.
thread_join is enabled by target->has_exited(), so this does not depend on a co-enabled THREAD_EXIT_TYPE transition. exit_permission ensures mc_exit_thread_in_child() cannot actually return until mc_pthread_join() posts it, but the blocking pthread_join() can still stall the real OS thread outside the model while the joiner’s THREAD_JOIN_TYPE transition has been consumed. Replace this with a bounded join / timeout path that fails loudly instead of leaving the model waiting.
🤖 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 812 - 821, Replace the blocking
libpthread_pthread_join call in thread_join with a timed or otherwise bounded
join using the coordinator’s established timeout contract. Preserve posting
exit_permission first, propagate successful join results through rv, and fail
loudly when the timeout expires so the model cannot remain waiting indefinitely
after THREAD_JOIN_TYPE is consumed.
|
|
||
| // The process we just killed may itself have left behind other | ||
| // now-orphaned descendants (e.g. its own private DMTCP coordinator, | ||
| // spawned by `dmtcp_restart --new-coordinator`) that die/reparent to | ||
| // us -- as the PR_SET_CHILD_SUBREAPER subreaper -- around the same | ||
| // time, generating their own SIGCHLDs. signal_tracker's counter isn't | ||
| // tied to a specific pid, so drain every zombie that's already | ||
| // reapable right now (non-blocking: this must never wait on a | ||
| // descendant that hasn't died yet) and consume one signal per reap, | ||
| // so none of them linger to be misattributed later either. | ||
| 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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every waitpid/wait call site to see who else depends on unreaped statuses.
rg -n -C 8 '\bwait(pid|id)?\s*\(' --glob '*.cpp' --glob '*.hpp'
rg -n -C 5 'PR_SET_CHILD_SUBREAPER'Repository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -u
printf 'Tracked files matching relevant names:\n'
git ls-files | grep -E 'local_linux_process\.(cpp|hpp)$|dmtcp_process_source|local_linux' || true
printf '\nRepository wait/pr_set_subreaper occurrences:\n'
rg -n -C 5 'wait(pid|i)?\s*\(|PR_SET_CHILD_SUBREAPER|child_subreaper|subreaper' . || true
printf '\nCandidate file snippets:\n'
for f in $(git ls-files | grep -E 'local_linux_process\.(cpp|hpp)$' | head -20); do
echo "--- $f $(wc -l < "$f")"
sed -n '1,180p' "$f"
doneRepository: mcminickpt/deep-debug
Length of output: 50378
🌐 Web query:
POSIX waitpid pid = -1 waits for any child process terminates man page
💡 Result:
Under POSIX standards, the waitpid function is used to wait for child processes to change state (such as terminating, stopping, or continuing) [1][2][3]. To wait for any child process, you must pass -1 as the pid argument [2]. The POSIX signature is: pid_t waitpid(pid_t pid, int *stat_loc, int options); When the pid argument is set to -1, waitpid requests that the system wait for any child process of the calling process [2]. If multiple children have changed state, one will be selected to report its status [2]. It is important to note that waitpid can only be used to wait for direct child processes [2][4][5]. If you attempt to wait for a process that is not a child of the caller, the function will fail and return -1 with errno set to ECHILD [4].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/7908799/xsh/waitpid.html
- 2: https://linux.die.net/man/2/waitpid
- 3: https://man7.org/linux/man-pages/man3/waitpid.3p.html
- 4: https://unix.stackexchange.com/questions/214908/why-can-the-waitpid-system-call-only-be-used-with-child-processes
- 5: https://docs.rs/waitpid-any/latest/waitpid_any/
🌐 Web query:
Linux setoption PR_SET_CHILD_SUBREAPER waitpid -1 descendant pid not direct child
💡 Result:
Yes, setting the PR_SET_CHILD_SUBREAPER attribute on a Linux process allows it to act as a subreaper, enabling it to wait on orphaned descendants that would otherwise be reparented to init [1][2]. When a process is marked as a subreaper using prctl(PR_SET_CHILD_SUBREAPER, 1), it fulfills the role of init(1) for its process tree [1][2]. If any descendant process becomes orphaned—meaning its immediate parent terminates—it will be reparented to the nearest ancestor that has the subreaper attribute set, rather than to the system init process [1][2]. Once an orphan is reparented to the subreaper, the subreaper process receives a SIGCHLD signal upon the orphan's termination [1][2]. The subreaper can then use wait(2) (or waitpid(2)) to collect the exit status of that process, even though it was not the process's original direct parent [1][3]. Key considerations: - The subreaper attribute is not inherited by children created via fork(2) or clone(2), though it is preserved across execve(2) [1]. - This mechanism is primarily intended for process supervisors or session managers (like systemd) that need to track the lifecycle of a hierarchy of processes [1][4]. - Because this feature makes the subreaper the "parent" for all orphaned descendants in its tree, the subreaper must be prepared to periodically call wait(2) to reap these processes; otherwise, it will accumulate zombie processes [3][5]. - You cannot typically use waitpid(pid,...) for a specific descendant unless you know its PID, and it must have been reparented to you [6]. Many implementations use a loop with wait(NULL) or waitpid(-1,...) to catch any orphaned child that terminates [3][4].
Citations:
- 1: https://man7.org/linux/man-pages/man2/pr_set_child_subreaper.2const.html
- 2: https://linux.die.net/man/2/prctl
- 3: https://jmmv.dev/2019/11/wait-for-process-group-linux.html
- 4: https://github.com/haileys/fork-wrapper
- 5: https://lwn.net/Articles/965891/
- 6: https://community.unix.com/t/waitpid-and-grandchildren/310254
🌐 Web query:
source dmtcp_prepare_daemon_daemonizes waitsForPortFd dmtcp_coordinator child_subreaper pr_init
💡 Result:
The terms mentioned are related to the internals of the Distributed MultiThreaded Checkpointing (DMTCP) project, specifically its coordinator and process management logic [1][2]. DMTCP_coordinator is the central control process in a DMTCP checkpoint/restart environment, responsible for coordinating checkpointing and restarting of distributed applications [1][3][4]. The specific terms appear to be functions or internal implementation details related to how the DMTCP coordinator or launcher manages processes and network ports: 1. dmtcp_prepare_daemon_daemonizes: This likely refers to an internal routine or script logic used when the coordinator or a DMTCP-managed process needs to properly daemonize itself (detach from the controlling terminal) during startup [5]. 2. waitsForPortFd: This suggests a synchronization mechanism where a process waits for a specific file descriptor (associated with a port) to be ready before proceeding, a common pattern to ensure a daemon is fully listening before the parent process continues [6][7]. 3. child_subreaper and pr_init: These relate to Linux process management [8]. A child_subreaper (set via prctl(PR_SET_CHILD_SUBREAPER,...)) allows a process to act as a sub-init for its process tree, ensuring that orphaned children are re-parented to it rather than PID 1 [8]. This is critical for supervisors like the DMTCP coordinator to maintain control over the processes they are monitoring, even if those processes daemonize or fork multiple times [8]. In summary, these components enable the DMTCP coordinator to reliably manage, supervise, and synchronize the lifecycle of complex, potentially multi-threaded or distributed applications without losing track of their child processes or network state [1][8][4].
Citations:
- 1: https://installati.one/centos/8/dmtcp/
- 2: https://screenshots.debian.net/package/dmtcp
- 3: https://packagehub.suse.com/packages/dmtcp/2_5_2-bp150_2_3/
- 4: https://www.onworks.net/software/app-distributed-multithreaded-checkpointing-to-run-in-linux-online
- 5: https://sourceforge.net/p/dmtcp/mailman/dmtcp-forum/?page=1
- 6: https://lists.libreplanet.org/archive/html/qemu-devel/2021-03/msg00264.html
- 7: https://patchew.org/QEMU/20210301172728.135331-1-stefanha@redhat.com/20210301172728.135331-2-stefanha@redhat.com/
- 8: LineageOS/android_kernel_lge_msm8996@ebec18a
🏁 Script executed:
#!/bin/bash
set -u
printf 'local_linux_process relevant lines:\n'
sed -n '1,180p' src/mcmini/real_world/local_linux_process.cpp
printf '\nprocess_source/dmtcp_process_source relevant lines:\n'
sed -n '1,130p' src/mcmini/real_world/dmtcp_process_source.cpp
sed -n '1,180p' src/mcmini/real_world/fork_process_source.cpp
printf '\nlocal_linux_process header:\n'
sed -n '1,180p' include/mcmini/real_world/process/local_linux_process.hpp
printf '\nTarget process source relevant lines:\n'
sed -n '1,230p' src/mcmini/real_world/target.cpp
printf '\nAll waitpid calls with surrounding context:\n'
rg -n -C 4 'waitpid\s*\(' src include --glob '*.{cpp,hpp,c,h}' || true
printf '\nAll child_subreaper/prctl child subreaper mentions:\n'
rg -n -C 4 'child_subreaper|PR_SET_CHILD_SUBREAPER|PR_SET_SUBREAPER|MCMINI.*SUBREAPER' src include --glob '*.{cpp,hpp,c,h}' || trueRepository: mcminickpt/deep-debug
Length of output: 37551
🌐 Web query:
DMTCP dmtcp_restart --new-coordinator coordinator child_subreaper prctl source
💡 Result:
DMTCP (Distributed MultiThreaded Checkpointing) does not have a specific command-line flag or feature named "child_subreaper" or "prctl" in its standard documentation [1][2][3][4]. The --new-coordinator flag used with dmtcp_restart is a standard command-line option that directs the restarting process to initialize a new DMTCP coordinator [1][2]. If the coordinator is not explicitly specified, DMTCP defaults to using a coordinator on localhost port 7779 [5][6]. The --new-coordinator option is often used to ensure a unique coordinator instance for a specific job, particularly in batch or HPC environments where isolating processes is necessary [7][8]. Regarding the terms "child_subreaper" and "prctl": 1. Child Subreaper (prctl(PR_SET_CHILD_SUBREAPER,...)): This is a Linux kernel feature, not a native DMTCP command-line parameter [9]. It allows a process to act as an init process for its orphaned descendants, ensuring that when they terminate, they are reparented to the subreaper rather than init [9]. 2. Relationship to DMTCP: While DMTCP does not have a "child_subreaper" command-line option, users managing complex process hierarchies or container-like environments might employ the Linux prctl(PR_SET_CHILD_SUBREAPER,...) system call within their own application wrappers or job scripts to manage process lifecycles. DMTCP itself manages child process checkpointing transparently through its internal mechanisms [10][6], independent of the host's use of child subreapers. If you are encountering issues where a process is not being correctly tracked or cleaned up, it may be due to the interaction between DMTCP's internal process tracking and the environment's process management. Using --new-coordinator ensures that a fresh coordinator is listening for your specific restarted processes, which is a common best practice for preventing cross-job interference [7][8].
Citations:
- 1: https://manpages.debian.org/unstable/dmtcp/dmtcp_restart.1.en.html
- 2: https://man.archlinux.org/man/dmtcp_restart.1.en.txt
- 3: https://dmtcp.sourceforge.io/docs/index.html
- 4: https://man.archlinux.org/man/dmtcp_launch.1.en
- 5: https://github.com/dmtcp/dmtcp/blob/main/QUICK-START.md
- 6: https://man.archlinux.org/man/dmtcp.1.en
- 7: --new-coordinator doesn't work with dmtcp_restart? dmtcp/dmtcp#635
- 8: https://nersc.gitlab.io/development/checkpoint-restart/dmtcp/
- 9: https://man7.org/linux/man-pages/man2/PR_SET_CHILD_SUBREAPER.2const.html
- 10: https://dmtcp.sourceforge.io/dmtcp.1.manpage.html
Restrict the zombie drain to descendants that McMini owns.
local_linux_process is built on the same McMini subreaper process, but McMini already calls waitpid(-1, ..., WNOHANG) in fork_process_source error handling. If any McMini-managed child dies around the same time, this blanket loop can reap it instead, so subsequent waitpid(pid, ...) calls for that specific process fail with ECHILD. Reap only this->pid here and handle the other SIGCHLD sources more explicitly.
🤖 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 66 - 78, Replace
the blanket waitpid(-1, ..., WNOHANG) loop in the local_linux_process cleanup
path with a non-blocking reap restricted to this->pid, preserving signal_tracker
consumption only when that specific child is reaped. Do not consume or reap
unrelated McMini-managed children; handle any additional SIGCHLD sources
explicitly without stealing descendants owned by fork_process_source.
| while (true) { | ||
| errno = 0; | ||
| signal_tracker::sig_semwait((sem_t *)&rmb->model_side_sem); | ||
| if (!signal_tracker::instance().try_consume_signal(SIGCHLD)) { | ||
| break; | ||
| } | ||
| // signal_tracker's SIGCHLD count is process-wide, not tied to a pid, so | ||
| // a pending count doesn't necessarily mean *this->pid* is the one that | ||
| // died -- some other descendant (e.g. a previous branch's own private | ||
| // DMTCP coordinator) may have generated it instead. Confirm with a | ||
| // non-blocking waitpid() specifically on this->pid (PR_SET_CHILD_SUBREAPER, | ||
| // set in target::prepare_mcmini_targets(), lets us wait on it even though | ||
| // it isn't a direct child): if it hasn't actually changed state, this | ||
| // SIGCHLD wasn't ours, so just resume waiting instead of misreporting a | ||
| // termination that didn't happen. | ||
| int status; | ||
| int rc = waitpid(this->pid, &status, WNOHANG); | ||
| if (rc == 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect signal_tracker's coupling between the SIGCHLD counter and model_side_sem.
rg -n -C 20 'class signal_tracker|try_consume_signal|sig_semwait|set_sem'Repository: mcminickpt/deep-debug
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files sample:\n'
git ls-files | sed -n '1,120p'
printf '\nFind local_linux_process.cpp:\n'
fd -a 'local_linux_process\.cpp|signal_tracker|.*sem.*' . | sed -n '1,120p'
printf '\nSearch relevant identifiers (case-insensitive):\n'
rg -n -i 'signal_tracker|try_consume_signal|sig_semwait|set_sem|model_side_sem|waitpid|SIGCHLD|sem_post|sem_wait' . || trueRepository: mcminickpt/deep-debug
Length of output: 28896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'local_linux_process.hpp outline:\n'
ast-grep outline include/mcmini/real_world/process/local_linux_process.hpp || true
printf '\nlocal_linux_process.hpp:\n'
cat -n include/mcmini/real_world/process/local_linux_process.hpp
printf '\nlocal_linux_process.cpp relevant sections:\n'
sed -n '1,190p' src/mcmini/real_world/local_linux_process.cpp | cat -n
printf '\nsignal_tracker header:\n'
sed -n '1,60p' include/mcmini/signal.hpp | cat -n
printf '\nsignal_tracker implementation relevant:\n'
sed -n '1,90p' src/mcmini/signal.cpp | cat -n
printf '\nrunner_mailbox relevant:\n'
cat -n include/mcmini/real_world/mailbox/runner_mailbox.h
sed -n '1,120p' src/common/runner_mailbox.c | cat -nRepository: mcminickpt/deep-debug
Length of output: 21144
Base the loop continuation on the mailbox state, not on the global SIGCHLD counter.
sig_semwait() and try_consume_signal(SIGCHLD) are independent: a target reply can post model_side_sem while a stale SIGCHLD count is already pending. In that case this loop consumes the reply, decrements the stale SIGCHLD count, waitpid(this->pid, ..., WNOHANG) returns 0, and continue re-waits on model_side_sem although the target already woke the checker, causing a hang. Decide continuation from waitpid plus whether the mailbox reply slot is still unread; also split the rc == -1 handling for plain ECHILD from errors that need to throw.
🤖 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 110 - 129, Update
the wait loop around sig_semwait(), try_consume_signal(), and waitpid() so
continuation depends on the mailbox reply slot remaining unread, not solely on
the global SIGCHLD counter. Preserve processing when waitpid(this->pid, ...,
WNOHANG) reports the target changed state, and when it returns 0 only continue
waiting if no mailbox reply is available. Handle waitpid() returning -1 by
treating ECHILD separately and propagating other errors.
There was a problem hiding this comment.
🟡 Not ready to approve
Several new paths introduce correctness/portability hazards (notably SIGCHLD + waitpid(ECHILD) handling, EINTR-unsafe semaphore waits, and a C header using static_assert) that can lead to misreported terminations or broken builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves McMini’s robustness around process/thread lifecycle handling during checkpoint/restart and abnormal termination scenarios (notably around DMTCP and TSan-related workflows), and updates internal modeling to avoid state corruption.
Changes:
- Make SIGCHLD/termination reporting more precise during replay/backtracking and improve diagnostics for nonzero exit codes.
- Fix several correctness issues in model state updates (mutex location preservation, process-exit runner state) and mailbox synchronization (raw futex for child-side semaphore).
- Add/adjust DMTCP/TSan support plumbing (API v4 header updates, join/timedjoin interception,
__libc_start_mainhook) and add a new parked-thread example/test.
File summaries
| File | Description |
|---|---|
| src/mcmini/real_world/local_linux_process.cpp | Refines SIGCHLD handling and zombie draining when runners die. |
| src/mcmini/real_world/dmtcp_process_source.cpp | Removes explicit DMTCP cleanup step and relies on new restart behavior. |
| src/mcmini/model/transitions/mutex.cpp | Ensures observed mutex objects preserve their address/location. |
| src/mcmini/model_checking/algorithms/classic_dpor.cpp | Converts certain replay failures into structured callbacks instead of escaping. |
| src/mcmini/mcmini.cpp | Improves termination reporting and adds nonzero-exit callback wiring. |
| src/lib/wrappers.c | Changes target-thread exit/join behavior to allow real termination after join. |
| src/lib/interception.c | Adds libpthread timed-join handle and hooks __libc_start_main to route main returns through exit protocol. |
| src/lib/dmtcp-callback.c | Fixes restart barrier thread counting and skips unnecessary post-restart checkpoint loop in one-shot restarts. |
| src/examples/producer-consumer-park.c | New example exercising “main returns with parked threads still alive”. |
| src/examples/CMakeLists.txt | Builds new example and a tsan-instrumented variant. |
| src/common/runner_mailbox.c | Replaces child-side sem_t usage with a raw futex-based counting semaphore. |
| src/common/multithreaded_fork.c | Updates to DMTCP API v4 pid-translation names. |
| include/mcmini/spy/intercept/interception.h | Declares libpthread timed-join handle. |
| include/mcmini/real_world/process/dmtcp_process_source.hpp | Switches to dmtcp_target and removes destructor declaration. |
| include/mcmini/real_world/mailbox/runner_mailbox.h | Changes child-side mailbox semaphore representation and documents rationale. |
| include/mcmini/model/transitions/process/exit.hpp | Marks executor thread exited on process exit to avoid DPOR reselect loops. |
| include/mcmini/model/transitions/mutex/mutex_unlock.hpp | Preserves mutex location when updating unlock state. |
| include/mcmini/model/transitions/mutex/mutex_init.hpp | Preserves mutex location when initializing model state. |
| include/mcmini/model/transitions/condition_variables/condition_variable_enqueue_thread.hpp | Preserves mutex location when enqueueing on condition variable. |
| include/mcmini/model/objects/mutex.hpp | Makes mutex location mandatory and defaults owner to RID_INVALID. |
| include/dmtcp.h | Updates to DMTCP plugin API v4 and adds new helpers/macros. |
| doc/glibc-sem-desync.txt | New documentation explaining glibc sem_t desync and the futex-based fix. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 5
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| sem_t *exit_permission = find_exit_permission_sem(pthread_self()); | ||
| assert(exit_permission != NULL); | ||
| libpthread_sem_wait(exit_permission); |
| int status; | ||
| int rc = waitpid(this->pid, &status, WNOHANG); | ||
| if (rc == 0) { | ||
| continue; | ||
| } | ||
| if (rc == -1) { | ||
| throw process::execution_error( | ||
| "Error attempting to determine the failure causing the child " | ||
| "process to abnormally exit (or possibly an internal error of " | ||
| "McMini): " + std::string(strerror(errno))); |
| static int wrapped_main(int argc, char **argv, char **envp) { | ||
| int rc = real_main(argc, argv, envp); | ||
| // libmcmini_init() itself is safe to call any time after main() has run | ||
| // (pthreads are certainly initialized by now), unlike at the top of | ||
| // __libc_start_main() below, which runs before glibc's own internal | ||
| // pthread-subsystem setup and must stay free of any pthread_once/mutex | ||
| // use until the real __libc_start_main() has had a chance to run. | ||
| libmcmini_init(); | ||
| // Route a plain return from main() through the exact same model-checking | ||
| // exit protocol as an explicit exit(rc) call: mc_transparent_exit() | ||
| // itself performs the real, final process termination in every mode | ||
| // (see wrappers.c), so this call never returns. | ||
| mc_transparent_exit(rc); | ||
| } |
| real_main = main; | ||
| libc_start_main_fn real_start_main = | ||
| (libc_start_main_fn)dlsym(RTLD_NEXT, "__libc_start_main"); | ||
| return real_start_main(wrapped_main, argc, argv, init, fini, rtld_fini, | ||
| stack_end); |
| char padding[1792]; | ||
| } DmtcpCkptHeader; | ||
|
|
||
| static_assert(sizeof(DmtcpCkptHeader) == 4096, "DmtcpCkptHeader must be 4096 bytes"); |
0f4cb92 to
8caef0a
Compare
The TSAN-supporting DMTCP branch (tsan-phased-init) bumped the
plugin API from v3 to v4, an ABI change (DmtcpPluginDescriptor_t /
DmtcpUniqueProcessId, new DmtcpCkptHeader etc.). DMTCP refused to
load libmcmini.so:
ASSERT pluginmanager.cpp:228: incompatible DMTCP plugin API version:
plugin_api=3 expected=4
Sync the vendored include/dmtcp.h to DMTCP's v4 header (correct
version string and descriptor ABI), and carry forward the only
McMini-specific additions -- the mcmini_virtual_pid / mcmini_real_pid
macros -- updated to the v4 function names
(dmtcp_{real_to_virtual,virtual_to_real}_pid became
dmtcp_pid_{real_to_virtual,virtual_to_real}). Also update the two
direct callers in multithreaded_fork.c. The unused
dmtcp_restore_buf_* decls are dropped (not referenced by libmcmini,
and gone from v4).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
template_thread() computed the number of threads to wait for by scanning /proc/self/task live, at restart time, then subtracting a blanket 2 (for itself and the checkpoint thread). DMTCP recreates checkpointed threads asynchronously via clone(), so a scan that runs before it has finished recreating all of them undercounts -- this barrier then declares "consistent state" and lets the template thread proceed before every thread has actually restarted. Confirmed via added diagnostic logging: one thread's own restart-completion signal could arrive after the barrier already released. Fix: count ALIVE THREAD entries in head_record_mode instead. That list only ever gets entries for genuine target threads (the template thread and checkpoint thread never go through libmcmini's wrapped pthread_create(), so neither is ever recorded there), and since a DMTCP checkpoint is a full memory snapshot, it's preserved exactly as-is across every restart -- immune to any restart-time scheduling race. Also remove fast_multithreaded_fork()'s #if 1/#else wrapper: the #else side held an early, never-compiled clone()-based fork prototype that predates this file's current _Fork()-based approach and was always dead code. Also remove the needless signal-mask check in threaded fork: getcontext()/setcontext() already restore a thread's blocked-signal set via uc_sigmask, even across the raw clone() used to recreate a checkpointed thread, so the thread_sigmask field and abort were unnecessary. That check ran on every restarted thread regardless of mode, so it also blocked plain (non-multithreaded-fork) restart of any target with a blocked-signal thread.
A DMTCP_RESTART_INTO_BRANCH process explores exactly one trace, then gets discarded -- it never legitimately needs to checkpoint again. Left alone, the checkpoint thread resumes its normal sleep-checkpoint-resume loop and blocks forever waiting for a checkpoint request from this restart's one-shot, otherwise-idle coordinator (dmtcp_process_source spins up a fresh coordinator per branch). Call the new dmtcp_skip_post_restart_checkpoint_loop() to tell DMTCP not to resume that loop. Verified: the checkpoint thread now parks in pause() right after restart instead of hanging in read() waiting on the coordinator.
Its destructor unconditionally ran `dmtcp_command -q --port <coordinator_target.get_port()>`, but coordinator_target (a dmtcp_coordinator member) never had launch_and_wait() called anywhere in this class, so its port field stayed at its default-constructed 0 forever -- this call always failed, since no coordinator ever listens on port 0. It's also unnecessary: each branch's own `dmtcp_restart --new-coordinator --port 0` call spawns its own private coordinator with --exit-on-last baked in by DMTCP itself, so it already self-terminates once its sole client disconnects. Nothing in this class needs explicit shutdown. Confirmed via a restart run: the bogus `dmtcp_command -q --port 0` [...] exited with status 2` error no longer appears, with no other change in behavior.
local_linux_process::execute_runner() previously treated any pending SIGCHLD as proof that *this* branch had just died, and reported it with a hardcoded SIGTERM regardless of the true cause. But signal_tracker's SIGCHLD count is a global counter, not tied to a pid, and ~local_linux_process() (invoked by coordinator::assign_new_process_handle() to tear down the previous branch immediately before spawning a new one, e.g. from return_to_depth() on every DPOR backtrack) kills that old process and reaps it without ever consuming the SIGCHLD it generates. That leftover count then lingers until the *next* branch process's very first execute_runner() call, which sees try_consume_signal() return true and wrongly concludes the brand-new process just died -- even though it's alive and simply hasn't responded yet. Confirmed via a waitpid() on the supposedly-dead pid: it blocked forever, proving the process was never actually dead. Fix: ~local_linux_process() now consumes the SIGCHLD from its own deliberate kill, then drains (non-blocking) any other already-reapable zombies -- e.g. the old branch's own private DMTCP coordinator -- consuming one signal per reap, since none of them are tied to a specific pid either. execute_runner() itself also loops on a pending SIGCHLD instead of treating it as automatic proof of death: since the counter isn't pid-scoped, a signal here can still belong to some other descendant. It confirms via a non-blocking waitpid() on this->pid specifically, resuming the wait if that comes back empty, and only reports the real signal/exit code once waitpid() actually confirms this->pid died. Verified: 9/10 fresh-checkpoint restart runs now fully explore all 9 traces and complete cleanly, with zero false "Abnormally Termination" reports across every run (previously: every run failed after 1-2 branches). One rarer, separate hang remains -- a genuine restart-synchronization stall, not a signal-tracking issue -- tracked separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mc_transparent_exit()'s TARGET_BRANCH case used the wait-only thread_await_scheduler() instead of thread_wake_scheduler_and_wait(), unlike every other wrapper. A thread that reaches this case directly (e.g. main() calling exit() as its own first wrapped call, in classic mode or after a restart) never posts model_side_sem, so the coordinator's execute_runner() hangs forever. Verified this is not --multithreaded-fork-specific: reverting just this hunk hangs plain classic mode identically, no DMTCP involved. Also mark the executor exited in process_exit::modify(), mirroring thread_exit: otherwise is_active() keeps reporting true and classic_dpor -- shared across every mode -- either re-selects the transition forever (when program_exit_code() > 0 doesn't already stop exploration, e.g. exit code 0) or reports a false DEADLOCK (confirmed with a trivial single-threaded exit(0) program in classic mode). Adds producer-consumer-park(-tsan), modeled on multithreaded-fork-tsan-2.0's test_park.c, to exercise main() exiting explicitly while other threads are still parked -- the scenario that exposed both bugs under --multithreaded-fork. Verified: a full --multithreaded-fork restart cycle now completes in under a second with a correct DEADLOCK verdict, instead of hanging. Regression-checked producer-consumer(-safe/-exit)-tsan and cv-producer-consumer(-safe)-tsan restart cycles, all still clean.
Two separate, previously-noted gaps in the same area: 1. callbacks.nonzero_exit_code was never wired up in mcmini.cpp (in either classic or DMTCP mode), so a target program exiting with a nonzero code -- exactly the kind of bug the model checker exists to catch -- was caught internally in classic_dpor.cpp, then silently discarded: `if (callbacks.nonzero_exit_code)` was always false, so the run just stopped with no verdict printed at all. Added found_nonzero_exit_code(), mirroring found_abnormal_termination()'s existing trace-printing pattern, and wired it into both callback setups. Verified live with a trivial `exit(1);`-only classic-mode target: now prints "NONZERO EXIT CODE (1)" plus the trace instead of silently completing. 2. local_linux_process.cpp's SIGCHLD-based dead-child detection threw nonzero_exit_code_error unconditionally on WIFEXITED(status), regardless of whether WEXITSTATUS(status) was actually nonzero -- misleading given the exception's own name/contract. A clean (code 0) exit reaching this path is a different situation entirely: it means the whole process fully terminated on its own while a transition was still pending on this runner, bypassing the model-driven exit protocol (mc_transparent_exit(), 6bed56a) entirely -- a McMini-side protocol violation to investigate, not a target-program bug. Now throws execution_error for that case instead of mislabeling it. Regression-checked clean against all 6 existing TSan targets.
A DMTCP-restarted target thread can be genuinely blocked at the kernel level on this exact futex word while glibc's own userspace "is anyone really waiting" bookkeeping (packed into the same memory sem_post() checks before deciding whether to skip the underlying FUTEX_WAKE syscall) is desynced from that -- because mc_runner_mailbox_init()/ _destroy() reinitialize this memory before every new DMTCP-restarted branch, independent of whatever kernel-level futex wait state a resurrected thread still has queued. When that happens, sem_post() silently skips the wake and the branch hangs forever. A plain futex word has no such bookkeeping: mc_raw_sem_post() always calls FUTEX_WAKE unconditionally, and mc_raw_sem_wait() only ever blocks after re-checking the atomic counter, so a wake can never be lost regardless of what order post/wait actually race in. Diagnosed via Gemini-assisted research into glibc's NPTL sem_t internals (packed nwaiters/value in one 64-bit word on 64-bit architectures) plus a direct FUTEX_WAKE(INT_MAX) probe confirming exactly one real waiter was queued when a hang occurred -- ruling out stale/multiple waiters as the cause and pointing squarely at the userspace-bookkeeping desync. model_side_sem is untouched: mcmini itself is never checkpointed, so glibc's bookkeeping for it can never desync. Condition variables (pthread_cond_wait/pthread_cond_signal) have the same class of vulnerability via glibc's G1/G2 waiter-group bookkeeping and will need an analogous fix separately. See doc/glibc-sem-desync.txt for a fuller explanation of the desync mechanism and why it is specific to checkpoint/restart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A target whose main() returns via `return N;` (rather than an explicit exit()/pthread_exit() call) never checked in with mc_transparent_exit()'s restart-quiescence protocol: glibc's __libc_start_call_main calls exit() internally via the hidden __GI_exit alias, bypassing exit()'s own interposition entirely -- confirmed this also holds for _exit() one level deeper (test_implicit_exit_interposition.c). __libc_start_main()'s own call site, in contrast, is in _start (crt1.o, not glibc-internal code), so it resolves through ordinary dynamic symbol resolution and is interposable (test_libc_start_main_hook.c). Wraps the target's main with a version that, after the real main() returns, calls mc_transparent_exit() directly instead of returning to glibc's own (uninterposable) exit machinery -- treating a plain return exactly like an explicit exit(rc) call, in every mode. Verified live: producer-consumer-park(-tsan), whose main() now returns plainly instead of calling exit(0) explicitly (removing that previously- required workaround), correctly reaches INITIAL STATE after a --multithreaded-fork restart with all 3 threads represented (main's pending exit(2), both workers' pending sem_wait), finds the expected DEADLOCK, and completes cleanly -- instead of the previous "Failed to create a new process (template process died)" failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
condition_variable_enqueue_thread::modify() also transitions the mutex to unlocked (cond_wait atomically releases it), but did so via mutex(state) -- the 1-arg constructor, which has no default member initializer for `location` (unlike condition_variable's policy field), so it was left completely uninitialized. Every later mutex_lock/unlock faithfully forwards whatever garbage ms->get_location() reads back, corrupting the mutex's identity for the rest of the run. This is what made condition_variable_wait::modify()'s `m->get_location() == cv->get_mutex()` check fail forever, even after the earlier policy-cloning and mutex-association fixes -- traced by adding a diagnostic directly at state_sequence::follow()'s commit point, the one unambiguous place that distinguishes a genuine commit from transition::is_enabled_in()'s throwaway speculative check. Fixed the same way as condition_variable_signal's associated_mutex fix: pass the existing location through explicitly. Also fixed the identical pattern in mutex_init::modify() and its callback (mutex.cpp) for consistency, though neither is exercised by the CV repro that found this (mutex_init only ever runs once, before any checkpoint, in every target tested so far). With this fix, a checkpoint taken mid-pthread_cond_wait() finally restarts and completes correctly under --multithreaded-fork: DPOR explores all 8 valid interleavings of a producer/consumer/main scenario with zero deadlocks, down from an immediate false DEADLOCK before this session's whole condition-variable investigation started. The core diff_state/state_sequence replay machinery investigated along the way (element indexing, slice()/consume_into_subsequence(), follow()'s commit path) turned out to be architecturally sound -- every symptom traced back to CV/mutex-specific constructors, not the shared state machinery itself. Regression-checked clean against all 6 existing TSan targets.
Its restart case called the one-time-per-thread thread_handle_after_dmtcp_restart() twice; the second call aborts once mode has already advanced past DMTCP_RESTART_INTO_BRANCH/ TEMPLATE (confirmed via dmesg: real SIGABRT). Use the ordinary thread_wake_scheduler_and_wait() for the second round instead, matching every other wrapper's second-and-later round with the coordinator.
Removed the 1-arg mutex(state) and 2-arg mutex(state, location) constructors -- confirmed unused after the recent fixes (every real call site already passes location) -- leaving one 3-arg constructor with location mandatory and tid defaulted to RID_INVALID. This turns "forgot to pass location" from a silent, uninitialized-memory correctness bug (as fixed in 9bd9ecf) into a compile error, and lets mutex_init/enqueue_thread/unlock express "no specific owner" without passing a misleading literal 0 (a real, valid runner id) or leaving `owner` uninitialized. Regression-checked clean against all 6 existing TSan targets.
8caef0a to
9d64042
Compare
@maxwellpirtle , These are the commits that I'd like you to review. All of them are: no TSAN, no multithreaded-fork, no condition variable
I'm also requesting a review from copilot to help you. For commits #2 through #6, you've already seen and reviewed those commits before. This is the full "deck" where it would be great to have a human read it before we push it into main.
And these commits will be pushed in after PR #3, #4, #5, #7 (where PR #3 and #4 are commits that you'll redo to do them the right way.
Please make sure that you've also reviewed PR #3, #4, #5, #7.
@aayushi363 will then review the next batch of commits, related to condition variables, and I'll then review all the remaining commits (multithreaded-fork, TSAN, etc.). Thanks.
I'm labelling this a Draft PR, since it's included in the full PR #11. After we converge on this PR #13, I'll map it back into PR #11.
Summary by CodeRabbit
Bug Fixes
Diagnostics
Examples and Testing
Documentation