From d1e08b124cf9ff0660119b804653fd2c28c53379 Mon Sep 17 00:00:00 2001 From: Tres Popp Date: Wed, 28 Apr 2021 14:06:57 +0200 Subject: [PATCH] Revert "tsan: refactor fork handling" This reverts commit e1021dd1fdfebff77cfb205892ada6b6a900865f. --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 53 +++++------- compiler-rt/lib/tsan/rtl/tsan_mman.cpp | 2 +- compiler-rt/lib/tsan/rtl/tsan_rtl.cpp | 18 ++-- compiler-rt/lib/tsan/rtl/tsan_rtl.h | 1 - compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp | 6 +- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp | 36 +------- compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp | 2 +- compiler-rt/test/tsan/pthread_atfork_deadlock.c | 2 +- compiler-rt/test/tsan/pthread_atfork_deadlock2.c | 10 +-- compiler-rt/test/tsan/pthread_atfork_deadlock3.c | 98 ---------------------- 10 files changed, 38 insertions(+), 190 deletions(-) delete mode 100644 compiler-rt/test/tsan/pthread_atfork_deadlock3.c diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index ab9ee3d..3db470b 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -81,8 +81,6 @@ extern "C" int pthread_attr_init(void *attr); extern "C" int pthread_attr_destroy(void *attr); DECLARE_REAL(int, pthread_attr_getdetachstate, void *, void *) extern "C" int pthread_attr_setstacksize(void *attr, uptr stacksize); -extern "C" int pthread_atfork(void (*prepare)(void), void (*parent)(void), - void (*child)(void)); extern "C" int pthread_key_create(unsigned *key, void (*destructor)(void* v)); extern "C" int pthread_setspecific(unsigned key, const void *v); DECLARE_REAL(int, pthread_mutexattr_gettype, void *, void *) @@ -1978,8 +1976,7 @@ static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire, // because in async signal processing case (when handler is called directly // from rtl_generic_sighandler) we have not yet received the reraised // signal; and it looks too fragile to intercept all ways to reraise a signal. - if (ShouldReport(thr, ReportTypeErrnoInSignal) && !sync && sig != SIGTERM && - errno != 99) { + if (flags()->report_bugs && !sync && sig != SIGTERM && errno != 99) { VarSizeStackTrace stack; // StackTrace::GetNestInstructionPc(pc) is used because return address is // expected, OutputReport() will undo this. @@ -2149,32 +2146,26 @@ TSAN_INTERCEPTOR(int, fork, int fake) { if (in_symbolizer()) return REAL(fork)(fake); SCOPED_INTERCEPTOR_RAW(fork, fake); - return REAL(fork)(fake); -} - -void atfork_prepare() { - if (in_symbolizer()) - return; - ThreadState *thr = cur_thread(); - const uptr pc = StackTrace::GetCurrentPc(); ForkBefore(thr, pc); -} - -void atfork_parent() { - if (in_symbolizer()) - return; - ThreadState *thr = cur_thread(); - const uptr pc = StackTrace::GetCurrentPc(); - ForkParentAfter(thr, pc); -} - -void atfork_child() { - if (in_symbolizer()) - return; - ThreadState *thr = cur_thread(); - const uptr pc = StackTrace::GetCurrentPc(); - ForkChildAfter(thr, pc); - FdOnFork(thr, pc); + int pid; + { + // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and + // we'll assert in CheckNoLocks() unless we ignore interceptors. + ScopedIgnoreInterceptors ignore; + pid = REAL(fork)(fake); + } + if (pid == 0) { + // child + ForkChildAfter(thr, pc); + FdOnFork(thr, pc); + } else if (pid > 0) { + // parent + ForkParentAfter(thr, pc); + } else { + // error + ForkParentAfter(thr, pc); + } + return pid; } TSAN_INTERCEPTOR(int, vfork, int fake) { @@ -2849,10 +2840,6 @@ void InitializeInterceptors() { Printf("ThreadSanitizer: failed to setup atexit callback\n"); Die(); } - if (pthread_atfork(atfork_prepare, atfork_parent, atfork_child)) { - Printf("ThreadSanitizer: failed to setup atfork callbacks\n"); - Die(); - } #if !SANITIZER_MAC && !SANITIZER_NETBSD && !SANITIZER_FREEBSD if (pthread_key_create(&interceptor_ctx()->finalize_key, &thread_finalize)) { diff --git a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp index 45a39f0..743e67bf 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp @@ -145,7 +145,7 @@ void AllocatorPrintStats() { static void SignalUnsafeCall(ThreadState *thr, uptr pc) { if (atomic_load_relaxed(&thr->in_signal_handler) == 0 || - !ShouldReport(thr, ReportTypeSignalUnsafe)) + !flags()->report_signal_unsafe) return; VarSizeStackTrace stack; ObtainCurrentStack(thr, pc, &stack); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp index f6672e9..ed6cc83 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp @@ -519,27 +519,23 @@ int Finalize(ThreadState *thr) { void ForkBefore(ThreadState *thr, uptr pc) { ctx->thread_registry->Lock(); ctx->report_mtx.Lock(); - // Suppress all reports in the pthread_atfork callbacks. - // Reports will deadlock on the report_mtx. - // We could ignore sync operations as well, + // Ignore memory accesses in the pthread_atfork callbacks. + // If any of them triggers a data race we will deadlock + // on the report_mtx. + // We could ignore interceptors and sync operations as well, // but so far it's unclear if it will do more good or harm. // Unnecessarily ignoring things can lead to false positives later. - thr->suppress_reports++; - // On OS X, REAL(fork) can call intercepted functions (OSSpinLockLock), and - // we'll assert in CheckNoLocks() unless we ignore interceptors. - thr->ignore_interceptors++; + ThreadIgnoreBegin(thr, pc); } void ForkParentAfter(ThreadState *thr, uptr pc) { - thr->suppress_reports--; // Enabled in ForkBefore. - thr->ignore_interceptors--; + ThreadIgnoreEnd(thr, pc); // Begin is in ForkBefore. ctx->report_mtx.Unlock(); ctx->thread_registry->Unlock(); } void ForkChildAfter(ThreadState *thr, uptr pc) { - thr->suppress_reports--; // Enabled in ForkBefore. - thr->ignore_interceptors--; + ThreadIgnoreEnd(thr, pc); // Begin is in ForkBefore. ctx->report_mtx.Unlock(); ctx->thread_registry->Unlock(); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index fef8182..04d474e 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -624,7 +624,6 @@ class ScopedReport : public ScopedReportBase { ScopedErrorReportLock lock_; }; -bool ShouldReport(ThreadState *thr, ReportType typ); ThreadContext *IsThreadStackOrTls(uptr addr, bool *is_stack); void RestoreStack(int tid, const u64 epoch, VarSizeStackTrace *stk, MutexSet *mset, uptr *tag = nullptr); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp index 6c4bf58..27897f0 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -51,8 +51,6 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ, // or false positives (e.g. unlock in a different thread). if (SANITIZER_GO) return; - if (!ShouldReport(thr, typ)) - return; ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(typ); rep.AddMutex(mid); @@ -109,7 +107,7 @@ void MutexDestroy(ThreadState *thr, uptr pc, uptr addr, u32 flagz) { if (!unlock_locked) s->Reset(thr->proc()); // must not reset it before the report is printed s->mtx.Unlock(); - if (unlock_locked && ShouldReport(thr, ReportTypeMutexDestroyLocked)) { + if (unlock_locked) { ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(ReportTypeMutexDestroyLocked); rep.AddMutex(mid); @@ -536,7 +534,7 @@ void AcquireReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c) { } void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) { - if (r == 0 || !ShouldReport(thr, ReportTypeDeadlock)) + if (r == 0) return; ThreadRegistryLock l(ctx->thread_registry); ScopedReport rep(ReportTypeDeadlock); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp index cedf1a3..208d0df4 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -142,34 +142,6 @@ static ReportStack *SymbolizeStack(StackTrace trace) { return stack; } -bool ShouldReport(ThreadState *thr, ReportType typ) { - // We set thr->suppress_reports in the fork context. - // Taking any locking in the fork context can lead to deadlocks. - // If any locks are already taken, it's too late to do this check. - CheckNoLocks(thr); - // For the same reason check we didn't lock thread_registry yet. - if (SANITIZER_DEBUG) - ThreadRegistryLock l(ctx->thread_registry); - if (!flags()->report_bugs || thr->suppress_reports) - return false; - switch (typ) { - case ReportTypeSignalUnsafe: - return flags()->report_signal_unsafe; - case ReportTypeThreadLeak: -#if !SANITIZER_GO - // It's impossible to join phantom threads - // in the child after fork. - if (ctx->after_multithreaded_fork) - return false; -#endif - return flags()->report_thread_leaks; - case ReportTypeMutexDestroyLocked: - return flags()->report_destroy_locked; - default: - return true; - } -} - ScopedReportBase::ScopedReportBase(ReportType typ, uptr tag) { ctx->thread_registry->CheckLocked(); void *mem = internal_alloc(MBlockReport, sizeof(ReportDesc)); @@ -525,10 +497,8 @@ static bool HandleRacyAddress(ThreadState *thr, uptr addr_min, uptr addr_max) { } bool OutputReport(ThreadState *thr, const ScopedReport &srep) { - // These should have been checked in ShouldReport. - // It's too late to check them here, we have already taken locks. - CHECK(flags()->report_bugs); - CHECK(!thr->suppress_reports); + if (!flags()->report_bugs || thr->suppress_reports) + return false; atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime()); const ReportDesc *rep = srep.GetReport(); CHECK_EQ(thr->current_report, nullptr); @@ -619,7 +589,7 @@ void ReportRace(ThreadState *thr) { // at best it will cause deadlocks on internal mutexes. ScopedIgnoreInterceptors ignore; - if (!ShouldReport(thr, ReportTypeRace)) + if (!flags()->report_bugs) return; if (!flags()->report_atomic_races && !RaceBetweenAtomicAndFree(thr)) return; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index 97ab205..d801467 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -210,7 +210,7 @@ static void ThreadCheckIgnore(ThreadState *thr) {} void ThreadFinalize(ThreadState *thr) { ThreadCheckIgnore(thr); #if !SANITIZER_GO - if (!ShouldReport(thr, ReportTypeThreadLeak)) + if (!flags()->report_thread_leaks) return; ThreadRegistryLock l(ctx->thread_registry); Vector leaks; diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock.c b/compiler-rt/test/tsan/pthread_atfork_deadlock.c index b002d7d..01107ee 100644 --- a/compiler-rt/test/tsan/pthread_atfork_deadlock.c +++ b/compiler-rt/test/tsan/pthread_atfork_deadlock.c @@ -21,7 +21,7 @@ void atfork() { int main() { barrier_init(&barrier, 2); - pthread_atfork(atfork, atfork, atfork); + pthread_atfork(atfork, NULL, NULL); pthread_t t; pthread_create(&t, NULL, worker, NULL); glob++; diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock2.c b/compiler-rt/test/tsan/pthread_atfork_deadlock2.c index 43f24f0..700507c 100644 --- a/compiler-rt/test/tsan/pthread_atfork_deadlock2.c +++ b/compiler-rt/test/tsan/pthread_atfork_deadlock2.c @@ -1,4 +1,4 @@ -// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s +// RUN: %clang_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s // Regression test for // https://groups.google.com/d/msg/thread-sanitizer/e_zB9gYqFHM/DmAiTsrLAwAJ // pthread_atfork() callback triggers a data race and we deadlocked @@ -22,7 +22,7 @@ void atfork() { int main() { barrier_init(&barrier, 2); - pthread_atfork(atfork, atfork, atfork); + pthread_atfork(atfork, NULL, NULL); pthread_t t; pthread_create(&t, NULL, worker, NULL); barrier_wait(&barrier); @@ -44,10 +44,6 @@ int main() { return 0; } -// CHECK: ThreadSanitizer: data race -// CHECK: Write of size 4 -// CHECK: #0 atfork -// CHECK: Previous write of size 4 -// CHECK: #0 worker +// CHECK-NOT: ThreadSanitizer: data race // CHECK: CHILD // CHECK: PARENT diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock3.c b/compiler-rt/test/tsan/pthread_atfork_deadlock3.c deleted file mode 100644 index 2ced006..0000000 --- a/compiler-rt/test/tsan/pthread_atfork_deadlock3.c +++ /dev/null @@ -1,98 +0,0 @@ -// RUN: %clang_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s -// Regression test for -// https://groups.google.com/g/thread-sanitizer/c/TQrr4-9PRYo/m/HFR4FMi6AQAJ -#include "test.h" -#include -#include -#include -#include -#include - -long glob = 0; - -void *worker(void *main) { - glob++; - // synchronize with main - barrier_wait(&barrier); - // synchronize with atfork - barrier_wait(&barrier); - pthread_kill((pthread_t)main, SIGPROF); - barrier_wait(&barrier); - // synchronize with afterfork - barrier_wait(&barrier); - pthread_kill((pthread_t)main, SIGPROF); - barrier_wait(&barrier); - return NULL; -} - -void atfork() { - barrier_wait(&barrier); - barrier_wait(&barrier); - write(2, "in atfork\n", strlen("in atfork\n")); - static volatile long a; - __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE); -} - -void afterfork() { - barrier_wait(&barrier); - barrier_wait(&barrier); - write(2, "in afterfork\n", strlen("in afterfork\n")); - static volatile long a; - __atomic_fetch_add(&a, 1, __ATOMIC_RELEASE); -} - -void afterfork_child() { - // Can't synchronize with barriers because we are - // in the new process, but want consistent output. - sleep(1); - write(2, "in afterfork_child\n", strlen("in afterfork_child\n")); - glob++; -} - -void handler(int sig) { - write(2, "in handler\n", strlen("in handler\n")); - glob++; -} - -int main() { - barrier_init(&barrier, 2); - struct sigaction act = {}; - act.sa_handler = &handler; - if (sigaction(SIGPROF, &act, 0)) { - perror("sigaction"); - exit(1); - } - pthread_atfork(atfork, afterfork, afterfork_child); - pthread_t t; - pthread_create(&t, NULL, worker, (void*)pthread_self()); - barrier_wait(&barrier); - pid_t pid = fork(); - if (pid < 0) { - fprintf(stderr, "fork failed: %d\n", errno); - return 1; - } - if (pid == 0) { - fprintf(stderr, "CHILD\n"); - return 0; - } - if (pid != waitpid(pid, NULL, 0)) { - fprintf(stderr, "waitpid failed: %d\n", errno); - return 1; - } - pthread_join(t, NULL); - fprintf(stderr, "PARENT\n"); - return 0; -} - -// CHECK: in atfork -// CHECK: in handler -// CHECK: ThreadSanitizer: data race -// CHECK: Write of size 8 -// CHECK: #0 handler -// CHECK: Previous write of size 8 -// CHECK: #0 worker -// CHECK: afterfork -// CHECK: in handler -// CHECK: afterfork_child -// CHECK: CHILD -// CHECK: PARENT -- 2.7.4