From: Evgenii Stepanov Date: Tue, 27 Apr 2021 19:31:34 +0000 (-0700) Subject: Revert "tsan: fix deadlock in pthread_atfork callbacks" X-Git-Tag: llvmorg-14-init~8287 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5275d772da0555f95ffa6d8f938728be13caa862;p=platform%2Fupstream%2Fllvm.git Revert "tsan: fix deadlock in pthread_atfork callbacks" Tests fail on debug builders. See the forward fix in https://reviews.llvm.org/D101385. This reverts commit efd254b63621de9ce750eddf9e8135154099d261. --- diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index f39f590..3db470b 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -1976,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. 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 85f3a26..ed6cc83 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp @@ -519,22 +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. + // 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++; + ThreadIgnoreBegin(thr, pc); } void ForkParentAfter(ThreadState *thr, uptr pc) { - thr->suppress_reports--; // Enabled in ForkBefore. + 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. + 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 63032be..208d0df4 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -142,28 +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: - 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)); @@ -519,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); @@ -613,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_deadlock3.c b/compiler-rt/test/tsan/pthread_atfork_deadlock3.c deleted file mode 100644 index dfbd16c..0000000 --- a/compiler-rt/test/tsan/pthread_atfork_deadlock3.c +++ /dev/null @@ -1,71 +0,0 @@ -// RUN: %clang_tsan -O1 %s -o %t && %run %t 2>&1 | 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++; - barrier_wait(&barrier); - 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 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, NULL, NULL); - 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 -// Note: There is a race, but we won't report it -// to not deadlock. -// CHECK-NOT: ThreadSanitizer: data race -// CHECK: CHILD -// CHECK: PARENT