DeepDebug: McMini, DMTCP and TSAN

CMakeLists.txt

@@ -14,6 +14,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED TRUE)
 # Project configuration
 option(BUILD_TESTS OFF)
 option(VERBOSE_TESTING OFF)
+option(MCMINI_LIBMCMINI_TSAN "Build libmcmini.so itself with -fsanitize=thread" OFF)
 set(MCMINI_DIR "${CMAKE_SOURCE_DIR}")
 set(MCMINI_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/include")
 set(MCMINI_CMAKE_MODULE_DIR "${CMAKE_SOURCE_DIR}/cmake")
@@ -74,6 +75,7 @@ set(LIBMCMINI_C_SRC
   src/lib/sem-wrappers.c
   src/lib/template/loop.c
   src/lib/template/sig.c
+  src/lib/tsan_support.c
   src/lib/wrappers.c
   src/mcmini/Thread_queue.c
 )
@@ -91,6 +93,13 @@ set(LIBMCMINI_EXTRA_COMPILER_DEFINITIONS MC_SHARED_LIBRARY=1 DMTCP=1 LOGGING_ROO
 # -ldl -> dlsym etc.
 set(LIBMCMINI_EXTRA_LINK_FLAGS -lrt -pthread -lm -ldl)
 
+# libmcmini.so is normally uninstrumented even when the target links libtsan
+# (see PLAN.txt); MCMINI_LIBMCMINI_TSAN is an experiment to instrument it too.
+if(MCMINI_LIBMCMINI_TSAN)
+  list(APPEND LIBMCMINI_EXTRA_COMPILER_FLAGS -fsanitize=thread)
+  list(APPEND LIBMCMINI_EXTRA_LINK_FLAGS -fsanitize=thread)
+endif()
+
 # libmcmini.so -> the dylib which is loaded
 add_library(libmcmini SHARED "${LIBMCMINI_C_SRC}")
 set_target_properties(libmcmini PROPERTIES OUTPUT_NAME "mcmini")
@@ -120,3 +129,19 @@ target_link_libraries(mcmini
 
 add_subdirectory(src/examples)
 add_subdirectory(test)
+
+# `make check-tsan` builds and runs the TSan example targets that exercise
+# McMini's DMTCP+TSan integration through a checkpoint/restart cycle (see
+# script/check-tsan.sh). Needs dmtcp_launch/dmtcp_restart on PATH, or a local
+# DMTCP build's bin/ dir passed as DMTCP_BIN_DIR (defaults to dmtcp.git/bin
+# alongside this CMakeLists.txt, if present).
+if(NOT DEFINED DMTCP_BIN_DIR AND EXISTS "${CMAKE_SOURCE_DIR}/dmtcp.git/bin")
+  set(DMTCP_BIN_DIR "${CMAKE_SOURCE_DIR}/dmtcp.git/bin")
+endif()
+
+add_custom_target(check-tsan
+  COMMAND ${CMAKE_SOURCE_DIR}/script/check-tsan.sh
+          ${CMAKE_BINARY_DIR} ${DMTCP_BIN_DIR}
+  DEPENDS mcmini libmcmini cv-producer-consumer-multi-tsan
+  WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+  USES_TERMINAL)

doc/classic-mode-thread-registration-segv.txt

@@ -0,0 +1,108 @@
+Classic-mode SEGV: new application threads never registered with TSan
+=============================================================================
+
+Symptom
+--------
+Running any TSan-instrumented target (producer-consumer-tsan,
+cv-producer-consumer-tsan) under classic (non-DMTCP) model checking --
+`mcmini ./target`, no `-i`/`--from-checkpoint` involved at all -- crashed
+100% of the time, deterministically, the moment the target's first
+non-main thread reached its first `libpthread_*`-wrapped call:
+
+  ThreadSanitizer:DEADLYSIGNAL
+  ==PID==ERROR: ThreadSanitizer: SEGV on unknown address 0x000000000000
+  ==PID==The signal is caused by a READ memory access.
+  ==PID==Hint: address points to the zero page.
+  ThreadSanitizer: nested bug in the same thread, aborting.
+
+TSan's own crash handler could not even produce a symbolicated backtrace
+(a second fault while handling the first -- itself a symptom of the same
+root cause: the crashing thread has no valid per-thread TSan state, and
+neither does TSan's own signal-handling code when it tries to use that
+same state to report the crash).
+
+Root cause
+-----------
+Caught live via `gdb --args ./mcmini ./producer-consumer-tsan` with
+`set follow-fork-mode child`, `set detach-on-fork off`, and
+`set schedule-multiple on` (needed, so the parent `mcmini` process keeps
+running -- classic mode requires it to actively coordinate with the
+branch process, so freezing it while continuing only the child deadlocks
+instead of crashing). `handle SIGSEGV stop nopass` lets gdb catch the
+fault before TSan's own handler consumes it. Full backtrace of the
+crashing thread:
+
+  #0  __tsan::SlotLock (thr=0x...) at tsan_rtl.cpp:366
+  #1  __tsan::SlotLocker::SlotLocker (...) at tsan_rtl.h:641
+  #2  __tsan::Acquire (thr=..., pc=..., addr=...) at tsan_rtl_mutex.cpp:448
+  #3  guard_acquire (thr=..., g=<libmcini_init>, ...) at tsan_interceptors_posix.cpp:891
+  #4  ___interceptor_pthread_once (o=<libmcini_init>, f=mc_load_intercepted_pthread_functions)
+  #5  libmcmini_init () from ./libmcmini.so
+  #6  libpthread_mutex_lock () from ./libmcmini.so
+  #7  mc_register_this_thread () from ./libmcmini.so
+  #8  mc_thread_routine_wrapper () from ./libmcmini.so
+  #9  start_thread
+  #10 clone3
+
+The chain: `mc_thread_routine_wrapper()` (the new thread's actual OS-level
+entry point) calls `mc_register_this_thread()` as its first statement,
+which calls `libpthread_mutex_lock()`, which calls `libmcmini_init()`,
+which lazily resolves all the `libpthread_*` bypass handles exactly once
+via `pthread_once()`. `pthread_once` is a public, dynamically-resolved
+symbol that TSan intercepts -- and TSan's interceptor needs this thread's
+own `ThreadState` to do its own bookkeeping. But this thread was never
+registered with TSan in the first place: `mc_pthread_create()`'s
+TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case (src/lib/wrappers.c)
+created it via `libpthread_pthread_create()` -- a handle resolved via
+`dlopen("libpthread")` + `dlsym()`, which always finds real, raw glibc,
+bypassing *any* interceptor (TSan's or libmcmini's own) unconditionally,
+regardless of load order. TSan's own `pthread_create` interceptor -- the
+thing that would normally set up this thread's `ThreadState` -- never ran
+for this thread at all.
+
+The comment above the original call ("Calling libpthread_pthread_create
+simplifies all this") suggests this was chosen for convenience, not as a
+deliberate TSan-avoidance measure -- unlike the `__clone`-based recreation
+work for DMTCP-restarted threads (doc files elsewhere in this directory),
+which deliberately bypasses TSan's interceptor for well-documented,
+load-order-specific reasons. This one had no such reason and was simply a
+bug: classic mode has no DMTCP/checkpoint-restart concerns at all, so
+there was never a reason to avoid TSan's own registration for these
+threads.
+
+The fix
+--------
+Added `tsan_or_real_pthread_create()` (src/lib/interception.c), resolved
+via `dlsym(RTLD_NEXT, "pthread_create")` instead of the libpthread-handle
+dlsym used by `libpthread_pthread_create_ptr`. `RTLD_NEXT` finds whatever
+comes *after* libmcmini.so in this process's actual, specific load order:
+
+  - Classic mode: libmcmini loads ahead of libtsan (see
+    TSAN-McMini-DMTCP.txt's own explanation of this load order), so
+    "next after libmcmini" correctly lands on TSan's own pthread_create
+    interceptor -- the fix.
+  - DMTCP mode: libtsan loads ahead of libmcmini, so "next after
+    libmcmini" still resolves past both and lands on real glibc, exactly
+    like libpthread_pthread_create's behavior today -- no change, no
+    regression risk for the already-extensively-verified DMTCP+TSan
+    restart flow (which handles TSan registration for its own
+    clone()-recreated threads via an entirely separate mechanism: R3/R4's
+    __clone + fresh-fiber approach, not pthread_create at all).
+
+`mc_pthread_create()`'s TARGET_BRANCH/TARGET_BRANCH_AFTER_RESTART case
+(src/lib/wrappers.c) now calls `tsan_or_real_pthread_create()` instead of
+`libpthread_pthread_create()`.
+
+Verification
+-------------
+- producer-consumer-tsan and cv-producer-consumer-tsan, classic mode: 100%
+  reproducible SEGV before the fix, zero crashes across multiple runs
+  after (cv-producer-consumer-tsan takes longer to fully explore -- more
+  interleavings from the mutex+cond-based synchronization -- but completes
+  cleanly given enough time).
+- Classic-mode regression check: cv-test, deadly-embrace,
+  producer-consumer, and cv-producer-consumer (non-TSan) all give
+  unchanged results.
+- DMTCP+TSan --multithreaded-fork restart flow (producer-consumer-tsan):
+  batch-verified unaffected, consistent with the RTLD_NEXT resolution
+  falling through to real glibc in that load order exactly as before.

doc/cond-wait-tsan-interceptor-bypass.txt

@@ -0,0 +1,72 @@
+pthread_cond_wait bypasses libmcmini under DMTCP+TSan
+=======================================================
+
+Symptom
+-------
+A DMTCP-restart-from-checkpoint of a TSan-instrumented target hangs
+forever as soon as any thread is genuinely blocked in pthread_cond_wait()
+at checkpoint time. mcmini's own trace output never even reaches
+"INITIAL STATE" for that thread; it just waits on a mailbox message that
+never arrives.
+
+Root cause
+----------
+Under DMTCP, libtsan.so loads ahead of libmcmini.so (the reverse of
+classic mode), so the target's own calls to the public pthread_cond_wait()
+symbol resolve to TSan's interceptor first, via the PLT/GOT -- confirmed
+empirically by reading the resolved GOT entry out of /proc/pid/mem.
+
+This is the same load-order problem pthread_join already had (see
+pthread_join_wrap.c), but pthread_cond_wait can't be fixed the same way
+sem_wait/pthread_mutex_lock/pthread_cond_signal/broadcast are handled:
+those TSan interceptors do their own bookkeeping and then delegate the
+actual operation to whatever's next in the interposition chain (landing
+on libmcmini's mc_* wrapper, since it's next after libtsan under DMTCP).
+pthread_cond_wait must atomically release the mutex and suspend the
+thread, then atomically reacquire the mutex on wakeup; TSan implements
+that whole sequence itself, inside its own runtime, rather than risk a
+race window from splitting the atomicity across an interposed call. So
+the target's pthread_cond_wait() calls never reach mc_pthread_cond_wait()
+at all -- the thread genuinely, permanently blocks inside TSan's own real,
+untimed wait, with nothing left to wake it (see glibc-cond-var-desync.txt
+for why the earlier CV desync fix removed the only thing that could have).
+
+Fix
+---
+-Wl,--wrap=pthread_cond_wait, applied when linking the TARGET (not
+libmcmini.so), rewrites the target's own pthread_cond_wait() calls to
+__wrap_pthread_cond_wait() at link time -- before the dynamic linker (and
+hence TSan's interceptor) is ever involved. See pthread_cond_wait_wrap.c.
+Unlike pthread_join, there's no "sometimes a real wait is still needed"
+case: mc_pthread_cond_wait() is already fully self-contained in every
+libmcmini_mode, so the wrapper forwards unconditionally with no
+__real_pthread_cond_wait() fallback.
+
+Verification
+------------
+Confirmed via GOT inspection that pthread_mutex_lock, pthread_mutex_unlock,
+sem_post, sem_wait, and pthread_cond_wait all resolve to libtsan.so
+directly with no exception -- the asymmetry is in TSan's internal
+delegation behavior per function, not in symbol resolution.
+
+Confirmed live: before the fix, restarting cv-producer-consumer-tsan from
+a checkpoint taken mid-consumer-cond_wait hung indefinitely (trace log
+truncated right after "waiting for them to get into a consistent state").
+After the fix, the same restart completes promptly, and the consumer's
+cond_wait shows up as a genuine, modeled NEXT THREAD OPERATIONS entry
+instead of an invisible black-box call.
+
+Follow-on issue (separate from this fix)
+-----------------------------------------
+That same verification run surfaced a second, previously-unreachable bug:
+once the consumer's cond_wait is actually visible to the model, the
+restart reports an immediate DEADLOCK. The reconstructed state shows the
+mutex still "locked" by the consumer and the condition_variable still at
+cv_initialized (never advanced to cv_waiting), so neither the producer's
+pthread_mutex_lock nor the consumer's own resume-from-wait transition
+(condition_variables_wait.hpp's modify()) can ever become enabled. This
+looks like a bug in how the checkpoint/restart path reconstructs CV
+waiter-queue state (the CV_PREWAITING/CV_WAITING bookkeeping described in
+Cond_Var_Readme.md) rather than anything to do with the TSan-bypass fix
+above, since this code path was never reachable before this fix landed.
+Tracked as a separate, follow-on investigation.