WIP: TSAN record thread fix (and restore threads on restart)#8
Open
gc00 wants to merge 23 commits into
Open
Conversation
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).
Verified: libmcmini.so builds against v4 and DMTCP now loads it without the
version assert (mcmini record mode reaches RECORD-phase execution under
dmtcp_launch).
Known follow-up (out of scope here): checkpointing a -fsanitize=thread
target under deep-debug still crashes in RECORD mode with the TSan null-
ThreadState SEGV, because libmcmini creates threads via
libdmtcp_pthread_create (dlopen'd directly), bypassing libtsan's
pthread_create interceptor -- the same class of issue the classic-mode
RTLD_NEXT fix addresses, but on the DMTCP record path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 -> libmcmini's 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 (DMTCP tsan-phased-init,
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. A separate TSan-interception site remains
downstream -- mc_pthread_join calls pthread_timedjoin_np directly (no bypass
handle), tripping libtsan's ConsumeThreadUserId CHECK -- to be addressed next.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mc_pthread_join's RECORD loop called pthread_timedjoin_np directly. Under a TSAN target, that symbol resolves to libtsan's interceptor, whose 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. With this, the three-fix stack achieves END-TO-END checkpointing of a -fsanitize=thread target under deep-debug (mcmini record mode): 1. 5be8500 DMTCP plugin API v3 -> v4 (DMTCP loads libmcmini.so) 2. 4bf2720 TSan-safe RECORD prologue (worker creation doesn't crash before libtsan registers the thread) 3. this timed-join via bypass handle (join loop doesn't trip the registry CHECK) Verified: `mcmini -i 3 ~/dmtcp.git/test/tsan_target` (DMTCP tsan-phased-init) now runs with no SEGV / no ThreadSanitizer errors and DMTCP produces a valid checkpoint (ckpt_tsan_target_*.dmtcp, ~5 MB, matching the no-mcmini baseline). 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). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the throwaway SIGSEGV-backtrace handler and its install call sites added during TSan-restart investigation, and restore the DMTCP_EVENT_RESTART SIG_DFL reset for SIGSEGV. Keep the R2 (TSan fork syscall hooks around _Fork()) and R4 (fresh TSan fiber for recreated threads) prototypes as the starting point for the multithreaded_fork TSan port described in PLAN.txt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eads from restart barrier Documents the per-tid classification design for template_thread()'s thread-count barrier: skip self, the checkpoint thread (via a new read-only tid lookup), and TSan-internal threads (via a SigBlk probe in a new src/lib/tsan_support.c). Ready for the writing-plans step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bite-sized task plan for docs/superpowers/specs/2026-07-04-tsan-port-phase1-r1-design.md: Task 1 adds thread_blocks_signal() in a new src/lib/tsan_support.c; Task 2 adds a read-only get_tid_from_pthread_descriptor() and rewires template_thread()'s restart barrier to per-tid classification; Task 3 is the environment-gated end-to-end DMTCP+TSan verification. All code snippets were test-compiled in a scratch copy before committing. 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 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>
template_thread() previously counted expected restart-barrier posts by taking /proc/self/task's thread count and subtracting a blanket 2 (for itself and the checkpoint thread). Under TSan, libtsan spawns its own background thread that blocks all signals at creation and never calls into libmcmini's wrappers, so it never posts to dmtcp_restart_sem, causing template_thread() to hang waiting for a post that never comes. Replace the blanket subtraction with per-tid classification: skip the template thread's own tid and the checkpoint thread's tid (read via the new read-only get_tid_from_pthread_descriptor() sibling of patchThreadDescriptor()), and skip any tid that blocks SIG_MULTITHREADED_FORK (thread_blocks_signal(), from Task 1) as a TSan-internal thread. A self-check re-verifies the pthread_t->tid offset against pthread_self() before trusting it for the checkpoint thread's descriptor, aborting via libc_abort() on mismatch. Adds a standalone host-side test (test/tsan_support/test_tid_from_descriptor_offset.c) proving the read-only offset technique works for reading another thread's descriptor, not just pthread_self(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pt tid The per-tid /proc/self/task walk skips self_tid (always reliable, from SYS_gettid) and ckpt_tid (read from ckpt_pthread_descriptor via a raw offset read, which could be stale or point at a since-changed offset). If ckpt_tid doesn't match any live tid, the checkpoint thread's real entry silently falls into the countable branch, corrupting thread_count and hanging the subsequent barrier loop forever with no diagnostic. Track how many times each tid was actually seen and abort with a clear message if either isn't exactly 1, turning a silent hang back into the exact stabilization bug this code was written to fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents that R2 and half of R4 are already committed from Phase 0's keeper prototypes, scoping this phase down to R3 (__clone) and R4's remaining half (forking-thread fiber switch). Also documents that ThreadSanitizer itself works in this sandbox under `setarch -R`, enabling a standalone test harness that mirrors dmtcp-callback.c's actual getcontext-direct-call mechanism (not the vendor package's signal-based one) without needing DMTCP. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bite-sized plan for docs/superpowers/specs/2026-07-04-tsan-port-phase2-r2-r3-r4-design.md: Task 1 writes a standalone harness mirroring dmtcp-callback.c's actual getcontext-direct-call mechanism and captures real RED (TSan CHECK failed in ForkChildAfter) and GREEN evidence via a compile-time MTF_BUGGY toggle; Task 2 applies the identical fix (__clone extern + swap, forking-thread fiber switch) to production; Task 3 is the environment-gated end-to-end DMTCP+TSan verification. Both the harness and the production diff were test-compiled/run in a scratch copy before committing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ber) under TSan Adds test/tsan_support/test_fastpath_fork_clone_fiber.c, a self-contained host-side test (compiled/run directly with gcc, not via CMake) that mirrors dmtcp-callback.c's fast-path fork+clone+fiber resumption mechanism to prove under real ThreadSanitizer that: - R3 (using libc's __clone instead of the public, libtsan-intercepted clone()) and R4's forking-thread fiber switch are both required: with -DMTF_BUGGY (public clone(), no forker fiber switch) the recreated threads crash with "ThreadSanitizer: CHECK failed: tsan_rtl.cpp:253 ((!thr->slot)) != (0)" inside ForkChildAfter, and the child process exits with code 66. - The default (fixed) configuration passes cleanly: both parent and child report shared_counter=300000 (expected 300000), the child exits 0, and no CHECK failures occur across repeated runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ded_fork restart_child_threads_fast() now calls libc's raw __clone() instead of the public clone(), since libtsan's clone() interceptor treats every call as a fork and corrupts its thread-slot state for the CLONE_THREAD clone used to recreate pre-checkpoint threads. Also switch the forking thread itself onto a fresh TSan fiber in fast_multithreaded_fork()'s child branch, since it otherwise keeps its inherited (fork-copied) ThreadState whose shadow call stack can overflow as the thread keeps running. Both fixes were already proven against a standalone TSan repro harness in a prior commit; this applies them to the production DMTCP restart path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rness comment Final review found the harness's forking thread (main()) never does enough post-fork instrumented work to trigger the shadow-call-stack overflow that the forking-thread fiber switch (R4 remainder) guards against, so the RED/GREEN evidence only actually falsifies/confirms R3 (__clone), not R4. Rewrite the header comment to state this accurately; no code logic changes.
…witch The fiber switch is guarded by #ifndef MTF_BUGGY, so it does not execute in the buggy build -- only the default (fixed) one. Doesn't change any of the comment's substantive conclusions (still not independently validated by this harness), per final re-review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Static code-path analysis of mc_pthread_join resolves PLAN.txt's own open question (Q1): the join CHECK-fail cannot reproduce here, since no code path reachable by a recreated thread ever calls a real libc/libpthread join (TARGET_BRANCH* is a pure mailbox handshake with the mcmini scheduler). No join-side change needed. pthread_exit is a real gap (not intercepted at all today) with a second, previously-undocumented consequence found during this investigation: McMini's existing THREAD_EXIT_TYPE machinery is only triggered by a thread routine returning normally, never by an explicit pthread_exit() call, for any thread. Designs a single mc_pthread_exit() interceptor (mirroring mc_transparent_exit/abort's mode-dispatch) that fixes both the TSan-safety gap and the model-checking gap together, without needing the standalone package's retval-stash/fiber-switch machinery -- McMini's own thread-exit never terminates the OS thread at the kernel level, so that machinery would have nothing to protect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bite-sized plan for docs/superpowers/specs/2026-07-05-tsan-port-phase3-r5-design.md: Task 1 proves the dlopen+dlsym forwarding technique standalone; Task 2 adds mc_pthread_exit() and wires it into interception.c/wrappers.c; Task 3 is the environment-gated end-to-end DMTCP+TSan verification with an explicit-pthread_exit() target. The full production diff was test-applied and built clean in a scratch copy, and the standalone forwarding test was run (PASS), before committing this plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…technique This test demonstrates that pthread_exit can be safely resolved via dlopen+dlsym and called from a worker thread, with the return value correctly delivered to pthread_join(). This technique is used in the next task for production code. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Intercepts pthread_exit() and routes it through the existing THREAD_EXIT_TYPE model-checking machinery (mc_exit_thread_in_child() / mc_exit_main_thread_in_child()) uniformly across all restart/branch modes, instead of falling through to the real libpthread pthread_exit and, on a __clone()-recreated thread, tripping libtsan's real interceptor (which is not registered for that "fiber" thread and can crash). Pre-restart modes forward to the real pthread_exit via a new libtsan-bypassing libpthread_pthread_exit() handle, resolved with the same dlopen+dlsym technique already used for pthread_timedjoin_np and validated standalone in the prior commit's harness. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion.h Matches the header's existing convention (libc_exit/libc_abort use the macro two lines below); interception.c's own pointer declarations already use the raw attribute for a different, pre-existing reason (exit_ptr/abort_ptr), so that file is untouched. Per Phase 3 Task 2 review's one Minor finding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…as live DMTCP_RESTART_INTO_BRANCH/TEMPLATE were grouped with TARGET_BRANCH* in a single case, skipping the mandatory thread_handle_after_dmtcp_restart() call that every other wrapper performs for those two restart modes. Split them into their own case that reports THREAD_EXIT_TYPE, calls thread_handle_after_dmtcp_restart(), then falls through into the unchanged TARGET_BRANCH*/TARGET_BRANCH_AFTER_RESTART body, matching the mc_transparent_exit/mc_transparent_abort fallthrough precedent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces ThreadSanitizer (TSan) compatibility across DMTCP checkpoint/restart/fork paths: a TSan-safe bump allocator, /proc-based thread signal-block detection, fiber switching at fork/restart points, __clone-based thread recreation, a pthread_exit interceptor, renamed DMTCP pid-translation APIs, new headers, tests, and design/plan documentation. ChangesTSan/DMTCP Compatibility Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ParentThread
participant Kernel
participant ChildOS
participant TSanFiber
ParentThread->>Kernel: _Fork() wrapped by TSan syscall hooks
Kernel->>ChildOS: forked child process created
ChildOS->>TSanFiber: __tsan_switch_to_fiber(create_fiber)
ChildOS->>ChildOS: restart_child_threads_fast() via __clone
ChildOS->>TSanFiber: recreated thread switches to fresh fiber after getcontext/setcontext
sequenceDiagram
participant UserCode
participant McPthreadExit
participant Scheduler
participant LibpthreadExit
UserCode->>McPthreadExit: pthread_exit(retval)
McPthreadExit->>McPthreadExit: check current mode
alt pre-restart record/checkpoint mode
McPthreadExit->>LibpthreadExit: forward to real pthread_exit
else restart/branch/template/target mode
McPthreadExit->>Scheduler: mark thread/process exit, transition control
else unexpected template mode
McPthreadExit->>McPthreadExit: abort
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
test/tsan_support/test_fastpath_fork_clone_fiber.c (1)
129-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked
mallocresult used as a clone stack pointer.If allocation fails,
stackbecomes an offset ofNULL, and it's later passed straight toclone()/__clone()as the child stack — undefined behavior / likely crash with a confusing failure mode instead of a clear assertion.🔧 Suggested fix
- void *stack = malloc(0x10000) + 0x10000 - 128; // 64 KB, intentionally leaked + void *raw_stack = malloc(0x10000); // 64 KB, intentionally leaked + assert(raw_stack != NULL); + void *stack = raw_stack + 0x10000 - 128;🤖 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_fastpath_fork_clone_fiber.c` at line 129, The clone stack setup in test_fastpath_fork_clone_fiber.c uses the result of malloc() directly in the stack pointer calculation, so an allocation failure would pass an invalid stack to clone()/__clone(). Add an explicit NULL check for the malloc call before deriving stack, and fail fast with a clear assertion or test error in the relevant setup path inside the fiber/clone test helper.Source: Linters/SAST tools
🤖 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 `@docs/superpowers/plans/2026-07-04-tsan-port-phase1-r1-implementation.md`:
- Around line 108-110: The bogus TID used in the thread_blocks_signal test is a
fixed positive value that can collide with a real thread ID and make the check
flaky. Update the assertion in the thread_blocks_signal test case to use a
sentinel value that cannot be a valid task ID, and keep the existing worker_tid
and self_tid coverage intact.
In
`@docs/superpowers/plans/2026-07-04-tsan-port-phase2-r2-r3-r4-implementation.md`:
- Around line 104-107: The TLS restoration in setTLSPointer() is using the wrong
arch_prctl invocation and success check. Update setTLSPointer(struct threadinfo
*ti) to call syscall(SYS_arch_prctl, ARCH_SET_FS, ti->fs) and
syscall(SYS_arch_prctl, ARCH_SET_GS, ti->gs) directly, and assert that each
returns 0 instead of != 0.
In `@docs/superpowers/specs/2026-07-04-tsan-port-phase1-r1-design.md`:
- Around line 117-119: The testing section conflicts with the Phase 1 plan by
saying no new unit-test harness is introduced, while the implementation
explicitly adds test_thread_blocks_signal.c under test/tsan_support. Update the
verification text in the design doc to reflect the new test file and align the
stated testing strategy with the actual files and approach used in this PR,
referencing the Phase 1 testing section and the test_thread_blocks_signal
symbol.
In `@test/tsan_support/test_fastpath_fork_clone_fiber.c`:
- Around line 87-94: The arch_prctl restore calls in setTLSPointer are using the
wrong argument order and inverted success checks, so the TLS/GS state is not
actually restored. Fix setTLSPointer to match getTLSPointer by passing
ARCH_SET_FS and ARCH_SET_GS as the second argument and asserting success with a
zero return; apply the same correction in the matching restore path in
src/lib/dmtcp-callback.c so both test and production code use the proper
arch_prctl invocation.
---
Nitpick comments:
In `@test/tsan_support/test_fastpath_fork_clone_fiber.c`:
- Line 129: The clone stack setup in test_fastpath_fork_clone_fiber.c uses the
result of malloc() directly in the stack pointer calculation, so an allocation
failure would pass an invalid stack to clone()/__clone(). Add an explicit NULL
check for the malloc call before deriving stack, and fail fast with a clear
assertion or test error in the relevant setup path inside the fiber/clone test
helper.
🪄 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: 1b0162a9-8176-403b-86c0-c1275fc99341
📒 Files selected for processing (27)
CMakeLists.txtdocs/superpowers/plans/2026-07-04-tsan-port-phase1-r1-implementation.mddocs/superpowers/plans/2026-07-04-tsan-port-phase2-r2-r3-r4-implementation.mddocs/superpowers/plans/2026-07-05-tsan-port-phase3-r5-implementation.mddocs/superpowers/specs/2026-07-04-tsan-port-phase1-r1-design.mddocs/superpowers/specs/2026-07-04-tsan-port-phase2-r2-r3-r4-design.mddocs/superpowers/specs/2026-07-05-tsan-port-phase3-r5-design.mdinclude/dmtcp.hinclude/mcmini/mem.hinclude/mcmini/model/pending_transitions.hppinclude/mcmini/model/state.hppinclude/mcmini/model_checking/algorithms/classic_dpor/stack_item.hppinclude/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/lib/dmtcp-callback.csrc/lib/interception.csrc/lib/record.csrc/lib/tsan_support.csrc/lib/wrappers.ctest/tsan_support/test_fastpath_fork_clone_fiber.ctest/tsan_support/test_pthread_exit_forwarding.ctest/tsan_support/test_thread_blocks_signal.ctest/tsan_support/test_tid_from_descriptor_offset.c
Comment on lines
+108
to
+110
| assert(thread_blocks_signal(worker_tid, SIGUSR1) == 1); | ||
| assert(thread_blocks_signal(self_tid, SIGUSR1) == 0); | ||
| assert(thread_blocks_signal(999999 /* bogus tid, should not exist */, SIGUSR1) == 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid a fixed positive bogus TID here.
999999 is not guaranteed to be absent; on a high-TID host it can collide with a live thread and make this test flaky. Use a sentinel that cannot exist as a real task ID instead.
Suggested fix
- assert(thread_blocks_signal(999999 /* bogus tid, should not exist */, SIGUSR1) == 0);
+ assert(thread_blocks_signal(-1, SIGUSR1) == 0);📝 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.
Suggested change
| assert(thread_blocks_signal(worker_tid, SIGUSR1) == 1); | |
| assert(thread_blocks_signal(self_tid, SIGUSR1) == 0); | |
| assert(thread_blocks_signal(999999 /* bogus tid, should not exist */, SIGUSR1) == 0); | |
| assert(thread_blocks_signal(worker_tid, SIGUSR1) == 1); | |
| assert(thread_blocks_signal(self_tid, 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 `@docs/superpowers/plans/2026-07-04-tsan-port-phase1-r1-implementation.md`
around lines 108 - 110, The bogus TID used in the thread_blocks_signal test is a
fixed positive value that can collide with a real thread ID and make the check
flaky. Update the assertion in the thread_blocks_signal test case to use a
sentinel value that cannot be a valid task ID, and keep the existing worker_tid
and self_tid coverage intact.
Comment on lines
+104
to
+107
| static void setTLSPointer(struct threadinfo *ti) { | ||
| assert(syscall(SYS_arch_prctl, 2, ARCH_SET_FS, ti->fs) != 0); | ||
| assert(syscall(SYS_arch_prctl, 2, ARCH_SET_GS, ti->gs) != 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target document around the cited lines
FILE="docs/superpowers/plans/2026-07-04-tsan-port-phase2-r2-r3-r4-implementation.md"
wc -l "$FILE"
sed -n '90,120p' "$FILE" | cat -n
# Find the helper and related mentions across the repo
rg -n "setTLSPointer|arch_prctl|ARCH_SET_FS|ARCH_SET_GS|SYS_arch_prctl" .Repository: mcminickpt/deep-debug
Length of output: 5955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding implementation and any local wrappers/comments
sed -n '90,140p' src/lib/dmtcp-callback.c | cat -n
sed -n '70,110p' test/tsan_support/test_fastpath_fork_clone_fiber.c | cat -n
# Check the kernel API constants from system headers, if available
grep -RIn --include='*.h' -E 'ARCH_SET_FS|ARCH_SET_GS|ARCH_GET_FS|ARCH_GET_GS|SYS_arch_prctl' /usr/include 2>/dev/null | head -n 40Repository: mcminickpt/deep-debug
Length of output: 4574
Use the correct arch_prctl call here. setTLSPointer() should pass ARCH_SET_FS / ARCH_SET_GS directly and assert syscall(...) == 0; the extra 2 and != 0 make TLS restoration fail on success.
🤖 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
`@docs/superpowers/plans/2026-07-04-tsan-port-phase2-r2-r3-r4-implementation.md`
around lines 104 - 107, The TLS restoration in setTLSPointer() is using the
wrong arch_prctl invocation and success check. Update setTLSPointer(struct
threadinfo *ti) to call syscall(SYS_arch_prctl, ARCH_SET_FS, ti->fs) and
syscall(SYS_arch_prctl, ARCH_SET_GS, ti->gs) directly, and assert that each
returns 0 instead of != 0.
Comment on lines
+117
to
+119
| No new unit-test harness is introduced — the project has no test harness yet | ||
| (per `CLAUDE.md`), and this is a runtime/scheduling fix best verified against | ||
| a real TSan target, per PLAN.txt's own phase methodology (section 7). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the testing section with the implementation plan.
This says no new unit-test harness is introduced, but Phase 1 explicitly adds test/tsan_support/test_thread_blocks_signal.c. Please update the text so the verification strategy matches the files in this PR.
Suggested fix
- No new unit-test harness is introduced — the project has no test harness yet (per `CLAUDE.md`), and this is a runtime/scheduling fix best verified against a real TSan target, per PLAN.txt's own phase methodology (section 7).
+ Phase 1 adds a small standalone harness (`test/tsan_support/test_thread_blocks_signal.c`) to lock in the `/proc` probe, while the restart path still needs the end-to-end TSan/DMTCP verification.📝 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.
Suggested change
| No new unit-test harness is introduced — the project has no test harness yet | |
| (per `CLAUDE.md`), and this is a runtime/scheduling fix best verified against | |
| a real TSan target, per PLAN.txt's own phase methodology (section 7). | |
| Phase 1 adds a small standalone harness (`test/tsan_support/test_thread_blocks_signal.c`) to lock in the `/proc` probe, while the restart path still needs the end-to-end TSan/DMTCP verification. |
🤖 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 `@docs/superpowers/specs/2026-07-04-tsan-port-phase1-r1-design.md` around lines
117 - 119, The testing section conflicts with the Phase 1 plan by saying no new
unit-test harness is introduced, while the implementation explicitly adds
test_thread_blocks_signal.c under test/tsan_support. Update the verification
text in the design doc to reflect the new test file and align the stated testing
strategy with the actual files and approach used in this PR, referencing the
Phase 1 testing section and the test_thread_blocks_signal symbol.
Comment on lines
+87
to
+94
| static void getTLSPointer(struct threadinfo *ti) { | ||
| assert(syscall(SYS_arch_prctl, ARCH_GET_FS, &ti->fs) == 0); | ||
| assert(syscall(SYS_arch_prctl, ARCH_GET_GS, &ti->gs) == 0); | ||
| } | ||
| static void setTLSPointer(struct threadinfo *ti) { | ||
| assert(syscall(SYS_arch_prctl, 2, ARCH_SET_FS, ti->fs) != 0); | ||
| assert(syscall(SYS_arch_prctl, 2, ARCH_SET_GS, ti->gs) != 0); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether src/lib/dmtcp-callback.c has the same arch_prctl argument/assert pattern.
rg -n -B2 -A2 'ARCH_SET_FS|ARCH_SET_GS' src/lib/dmtcp-callback.cRepository: mcminickpt/deep-debug
Length of output: 422
Fix the arch_prctl argument order and assert in both copies
setTLSPointer passes an extra leading 2 and checks != 0, so the restore call fails and the failure is treated as success. The same pattern is present in src/lib/dmtcp-callback.c, so the production TLS/GS restore path is affected too.
🤖 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_fastpath_fork_clone_fiber.c` around lines 87 - 94, The
arch_prctl restore calls in setTLSPointer are using the wrong argument order and
inverted success checks, so the TLS/GS state is not actually restored. Fix
setTLSPointer to match getTLSPointer by passing ARCH_SET_FS and ARCH_SET_GS as
the second argument and asserting success with a zero return; apply the same
correction in the matching restore path in src/lib/dmtcp-callback.c so both test
and production code use the proper arch_prctl invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is still a WIP. But with Claude (and after about 10 hours), DeepDebug can now checkpoint and restart. Next, we need it to handle multithreaded_fork in the presence of TSAN. Hopefully, the solution will be similar to the solution during DMTCP restart (restore threads).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation