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 (file)
--- 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..743e67b 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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..208d0df 100644 (file)
--- 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 (file)
--- 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<ThreadLeak> 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 (file)
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 <sys/types.h>
-#include <sys/wait.h>
-#include <errno.h>
-#include <string.h>
-#include <signal.h>
-
-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