[compiler-rt] r230874 - Revert r229678 "tsan: fix signal handling during stop-the-world"
Hans Wennborg
hans at hanshq.net
Sat Feb 28 14:18:11 PST 2015
Author: hans
Date: Sat Feb 28 16:18:11 2015
New Revision: 230874
URL: http://llvm.org/viewvc/llvm-project?rev=230874&view=rev
Log:
Revert r229678 "tsan: fix signal handling during stop-the-world"
This was causing Chromium's browser_tests to flakily segfault during
leak check.
Removed:
compiler-rt/trunk/test/tsan/signal_segv_handler.cc
Modified:
compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc?rev=230874&r1=230873&r2=230874&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc Sat Feb 28 16:18:11 2015
@@ -184,9 +184,10 @@ bool ThreadSuspender::SuspendAllThreads(
// Pointer to the ThreadSuspender instance for use in signal handler.
static ThreadSuspender *thread_suspender_instance = NULL;
-// Synchronous signals that should not be blocked.
-static const int kSyncSignals[] = { SIGABRT, SIGILL, SIGFPE, SIGSEGV, SIGBUS,
- SIGXCPU, SIGXFSZ };
+// Signals that should not be blocked (this is used in the parent thread as well
+// as the tracer thread).
+static const int kUnblockedSignals[] = { SIGABRT, SIGILL, SIGFPE, SIGSEGV,
+ SIGBUS, SIGXCPU, SIGXFSZ };
// Structure for passing arguments into the tracer thread.
struct TracerThreadArgument {
@@ -201,7 +202,7 @@ struct TracerThreadArgument {
static DieCallbackType old_die_callback;
// Signal handler to wake up suspended threads when the tracer thread dies.
-static void TracerThreadSignalHandler(int signum, void *siginfo, void *) {
+void TracerThreadSignalHandler(int signum, void *siginfo, void *) {
if (thread_suspender_instance != NULL) {
if (signum == SIGABRT)
thread_suspender_instance->KillAllThreads();
@@ -241,7 +242,6 @@ static int TracerThread(void* argument)
tracer_thread_argument->mutex.Lock();
tracer_thread_argument->mutex.Unlock();
- old_die_callback = GetDieCallback();
SetDieCallback(TracerThreadDieCallback);
ThreadSuspender thread_suspender(internal_getppid());
@@ -256,14 +256,17 @@ static int TracerThread(void* argument)
handler_stack.ss_size = kHandlerStackSize;
internal_sigaltstack(&handler_stack, NULL);
- // Install our handler for synchronous signals. Other signals should be
- // blocked by the mask we inherited from the parent thread.
- for (uptr i = 0; i < ARRAY_SIZE(kSyncSignals); i++) {
- __sanitizer_sigaction act;
- internal_memset(&act, 0, sizeof(act));
- act.sigaction = TracerThreadSignalHandler;
- act.sa_flags = SA_ONSTACK | SA_SIGINFO;
- internal_sigaction_norestorer(kSyncSignals[i], &act, 0);
+ // Install our handler for fatal signals. Other signals should be blocked by
+ // the mask we inherited from the caller thread.
+ for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
+ signal_index++) {
+ __sanitizer_sigaction new_sigaction;
+ internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
+ new_sigaction.sigaction = TracerThreadSignalHandler;
+ new_sigaction.sa_flags = SA_ONSTACK | SA_SIGINFO;
+ internal_sigfillset(&new_sigaction.sa_mask);
+ internal_sigaction_norestorer(kUnblockedSignals[signal_index],
+ &new_sigaction, NULL);
}
int exit_code = 0;
@@ -276,11 +279,9 @@ static int TracerThread(void* argument)
thread_suspender.ResumeAllThreads();
exit_code = 0;
}
- // Note, this is a bad race. If TracerThreadDieCallback is already started
- // in another thread and observed that thread_suspender_instance != 0,
- // it can call KillAllThreads on the destroyed variable.
- SetDieCallback(old_die_callback);
thread_suspender_instance = NULL;
+ handler_stack.ss_flags = SS_DISABLE;
+ internal_sigaltstack(&handler_stack, NULL);
return exit_code;
}
@@ -312,21 +313,53 @@ class ScopedStackSpaceWithGuard {
// into globals.
static __sanitizer_sigset_t blocked_sigset;
static __sanitizer_sigset_t old_sigset;
+static __sanitizer_sigaction old_sigactions
+ [ARRAY_SIZE(kUnblockedSignals)];
class StopTheWorldScope {
public:
StopTheWorldScope() {
+ // Block all signals that can be blocked safely, and install
+ // default handlers for the remaining signals.
+ // We cannot allow user-defined handlers to run while the ThreadSuspender
+ // thread is active, because they could conceivably call some libc functions
+ // which modify errno (which is shared between the two threads).
+ internal_sigfillset(&blocked_sigset);
+ for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
+ signal_index++) {
+ // Remove the signal from the set of blocked signals.
+ internal_sigdelset(&blocked_sigset, kUnblockedSignals[signal_index]);
+ // Install the default handler.
+ __sanitizer_sigaction new_sigaction;
+ internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
+ new_sigaction.handler = SIG_DFL;
+ internal_sigfillset(&new_sigaction.sa_mask);
+ internal_sigaction_norestorer(kUnblockedSignals[signal_index],
+ &new_sigaction, &old_sigactions[signal_index]);
+ }
+ int sigprocmask_status =
+ internal_sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
+ CHECK_EQ(sigprocmask_status, 0); // sigprocmask should never fail
// Make this process dumpable. Processes that are not dumpable cannot be
// attached to.
process_was_dumpable_ = internal_prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
if (!process_was_dumpable_)
internal_prctl(PR_SET_DUMPABLE, 1, 0, 0, 0);
+ old_die_callback = GetDieCallback();
}
~StopTheWorldScope() {
+ SetDieCallback(old_die_callback);
// Restore the dumpable flag.
if (!process_was_dumpable_)
internal_prctl(PR_SET_DUMPABLE, 0, 0, 0, 0);
+ // Restore the signal handlers.
+ for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
+ signal_index++) {
+ internal_sigaction_norestorer(kUnblockedSignals[signal_index],
+ &old_sigactions[signal_index], NULL);
+ }
+ internal_sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);
}
private:
@@ -359,36 +392,11 @@ void StopTheWorld(StopTheWorldCallback c
// Block the execution of TracerThread until after we have set ptrace
// permissions.
tracer_thread_argument.mutex.Lock();
- // Signal handling story.
- // We don't want async signals to be delivered to the tracer thread,
- // so we block all async signals before creating the thread. An async signal
- // handler can temporary modify errno, which is shared with this thread.
- // We ought to use pthread_sigmask here, because sigprocmask has undefined
- // behavior in multithreaded programs. However, on linux sigprocmask is
- // equivalent to pthread_sigmask with the exception that pthread_sigmask
- // does not allow to block some signals used internally in pthread
- // implementation. We are fine with blocking them here, we are really not
- // going to pthread_cancel the thread.
- // The tracer thread should not raise any synchronous signals. But in case it
- // does, we setup a special handler for sync signals that properly kills the
- // parent as well. Note: we don't pass CLONE_SIGHAND to clone, so handlers
- // in the tracer thread won't interfere with user program. Double note: if a
- // user does something along the lines of 'kill -11 pid', that can kill the
- // process even if user setup own handler for SEGV.
- // Thing to watch out for: this code should not change behavior of user code
- // in any observable way. In particular it should not override user signal
- // handlers.
- internal_sigfillset(&blocked_sigset);
- for (uptr i = 0; i < ARRAY_SIZE(kSyncSignals); i++)
- internal_sigdelset(&blocked_sigset, kSyncSignals[i]);
- int rv = internal_sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
- CHECK_EQ(rv, 0);
uptr tracer_pid = internal_clone(
TracerThread, tracer_stack.Bottom(),
CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_UNTRACED,
&tracer_thread_argument, 0 /* parent_tidptr */, 0 /* newtls */, 0
/* child_tidptr */);
- internal_sigprocmask(SIG_SETMASK, &old_sigset, 0);
int local_errno = 0;
if (internal_iserror(tracer_pid, &local_errno)) {
VReport(1, "Failed spawning a tracer thread (errno %d).\n", local_errno);
Removed: compiler-rt/trunk/test/tsan/signal_segv_handler.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/signal_segv_handler.cc?rev=230873&view=auto
==============================================================================
--- compiler-rt/trunk/test/tsan/signal_segv_handler.cc (original)
+++ compiler-rt/trunk/test/tsan/signal_segv_handler.cc (removed)
@@ -1,39 +0,0 @@
-// RUN: %clang_tsan -O1 %s -o %t && TSAN_OPTIONS="flush_memory_ms=1 memory_limit_mb=1" %run %t 2>&1 | FileCheck %s
-
-// JVM uses SEGV to preempt threads. All threads do a load from a known address
-// periodically. When runtime needs to preempt threads, it unmaps the page.
-// Threads start triggering SEGV one by one. The signal handler blocks
-// threads while runtime does its thing. Then runtime maps the page again
-// and resumes the threads.
-// Previously this pattern conflicted with stop-the-world machinery,
-// because it briefly reset SEGV handler to SIG_DFL.
-// As the consequence JVM just silently died.
-
-// This test sets memory flushing rate to maximum, then does series of
-// "benign" SEGVs that are handled by signal handler, and ensures that
-// the process survive.
-
-#include "test.h"
-#include <signal.h>
-#include <sys/mman.h>
-
-void *guard;
-
-void handler(int signo, siginfo_t *info, void *uctx) {
- mprotect(guard, 4096, PROT_READ | PROT_WRITE);
-}
-
-int main() {
- struct sigaction a;
- a.sa_sigaction = handler;
- a.sa_flags = SA_SIGINFO;
- sigaction(SIGSEGV, &a, 0);
- guard = mmap(0, 4096, PROT_NONE, MAP_ANON | MAP_PRIVATE, -1, 0);
- for (int i = 0; i < 1000000; i++) {
- mprotect(guard, 4096, PROT_NONE);
- *(int*)guard = 1;
- }
- fprintf(stderr, "DONE\n");
-}
-
-// CHECK: DONE
More information about the llvm-commits
mailing list