[compiler-rt] d1e08b1 - Revert "tsan: refactor fork handling"
Tres Popp via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 28 05:08:54 PDT 2021
Author: Tres Popp
Date: 2021-04-28T14:08:33+02:00
New Revision: d1e08b124cf9ff0660119b804653fd2c28c53379
URL: https://github.com/llvm/llvm-project/commit/d1e08b124cf9ff0660119b804653fd2c28c53379
DIFF: https://github.com/llvm/llvm-project/commit/d1e08b124cf9ff0660119b804653fd2c28c53379.diff
LOG: Revert "tsan: refactor fork handling"
This reverts commit e1021dd1fdfebff77cfb205892ada6b6a900865f.
Added:
Modified:
compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
compiler-rt/lib/tsan/rtl/tsan_mman.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl.h
compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
compiler-rt/test/tsan/pthread_atfork_deadlock.c
compiler-rt/test/tsan/pthread_atfork_deadlock2.c
Removed:
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 ab9ee3d97e19a..3db470b5b963f 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 45a39f0f8ec9a..743e67bf2f7d9 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 f6672e9310264..ed6cc83450d90 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 fef8182f6fe87..04d474e044e15 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 6c4bf58c07769..27897f0592b0e 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
diff erent 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 cedf1a3ca7c9c..208d0df44df7b 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 97ab2059988ae..d80146735ea79 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<ThreadLeak> leaks;
diff --git a/compiler-rt/test/tsan/pthread_atfork_deadlock.c b/compiler-rt/test/tsan/pthread_atfork_deadlock.c
index b002d7d956fc7..01107ee6692c1 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 43f24f0eebac6..700507c1e637c 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 2ced0067de5c7..0000000000000
--- 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 <sys/types.h>
-#include <sys/wait.h>
-#include <errno.h>
-#include <string.h>
-#include <signal.h>
-
-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
More information about the llvm-commits
mailing list