[PATCH] Run the callback on a separate stack in StopTheWorld.

[Sergey Matveev]

    - TidySigaction
    - minor changes

Hi kcc, glider, samsonov,

http://llvm-reviews.chandlerc.com/D592

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D592?vs=1438&id=1452#toc

Files:
  lib/sanitizer_common/sanitizer_stoptheworld_linux.cc

Index: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
===================================================================
--- lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
+++ lib/sanitizer_common/sanitizer_stoptheworld_linux.cc
@@ -71,6 +71,26 @@
 COMPILER_CHECK(sizeof(SuspendedThreadID) == sizeof(pid_t));
 
 namespace __sanitizer {
+static const uptr kTracerStackSize = 2 * 1024 * 1024;
+
+// At least in some libc implementations, sigaction() has a side-effect where
+// it copies 120 bytes from stack directly into oldact without ever touching
+// them. This happens because the size of sigset_t is larger in libc than it is
+// in kernel, and the syscall ignores the extra space. The net effect is that
+// out-of-scope stack values end up in oldact, which can cause problems
+// in LSan. To fix this we provide a wrapper that cleans up after itself.
+int TidySigaction(int signum, const struct sigaction *act,
+                  struct sigaction *oldact) {
+  int res = sigaction(signum, act, oldact);
+  if (oldact) {
+    // The size of kernel_sigset_t is 8 bytes on every platform we care about.
+    // The rest of sa_mask is unused.
+    internal_memset((void *)((uptr)&oldact->sa_mask + 8), 0,
+                    sizeof(oldact->sa_mask) - 8);
+  }
+  return res;
+}
+
 // This class handles thread suspending/unsuspending in the tracer thread.
 class ThreadSuspender {
  public:
@@ -247,6 +267,29 @@
   return exit_code;
 }
 
+class ScopedStackSpaceWithGuard {
+ public:
+  explicit ScopedStackSpaceWithGuard(uptr stack_size) {
+    stack_size_ = stack_size;
+    guard_size_ = GetPageSizeCached();
+    // Omitting MAP_STACK here works in current kernels but might break later.
+    guard_start_ = (uptr) MmapOrDie(stack_size_ + guard_size_,
+                                   "ScopedStackWithGuard");
+    CHECK_EQ(guard_start_, (uptr)Mprotect((uptr)guard_start_, guard_size_));
+  }
+  ~ScopedStackSpaceWithGuard() {
+    UnmapOrDie((void *)guard_start_, stack_size_ + guard_size_);
+  }
+  void *Bottom() const {
+    return (void *)(guard_start_ + stack_size_ + guard_size_);
+  }
+
+ private:
+  uptr stack_size_;
+  uptr guard_size_;
+  uptr guard_start_;
+};
+
 static sigset_t blocked_sigset;
 static sigset_t old_sigset;
 static struct sigaction old_sigactions[ARRAY_SIZE(kUnblockedSignals)];
@@ -267,11 +310,10 @@
     internal_memset(&new_sigaction, 0, sizeof(new_sigaction));
     new_sigaction.sa_handler = SIG_DFL;
     sigfillset(&new_sigaction.sa_mask);
-    sigaction(kUnblockedSignals[signal_index], &new_sigaction,
-                    &old_sigactions[signal_index]);
+    CHECK_EQ(0, TidySigaction(kUnblockedSignals[signal_index], &new_sigaction,
+                              &old_sigactions[signal_index]));
   }
-  int sigprocmask_status = sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset);
-  CHECK_EQ(sigprocmask_status, 0); // sigprocmask should never fail
+  CHECK_EQ(0, sigprocmask(SIG_BLOCK, &blocked_sigset, &old_sigset));
   // Make this process dumpable. Processes that are not dumpable cannot be
   // attached to.
   int process_was_dumpable = internal_prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
@@ -281,16 +323,11 @@
   struct TracerThreadArgument tracer_thread_argument;
   tracer_thread_argument.callback = callback;
   tracer_thread_argument.callback_argument = argument;
+  ScopedStackSpaceWithGuard tracer_stack(kTracerStackSize);
   // Block the execution of TracerThread until after we have set ptrace
   // permissions.
   tracer_thread_argument.mutex.Lock();
-  // The tracer thread will run on the same stack, so we must reserve some
-  // stack space for the caller thread to run in as it waits on the tracer.
-  const uptr kReservedStackSize = 4096;
-  // Get a 16-byte aligned pointer for stack.
-  int a_local_variable __attribute__((__aligned__(16)));
-  pid_t tracer_pid = clone(TracerThread,
-                          (char *)&a_local_variable - kReservedStackSize,
+  pid_t tracer_pid = clone(TracerThread, tracer_stack.Bottom(),
                           CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_UNTRACED,
                           &tracer_thread_argument, 0, 0, 0);
   if (tracer_pid < 0) {
@@ -318,8 +355,8 @@
   // Restore the signal handlers.
   for (uptr signal_index = 0; signal_index < ARRAY_SIZE(kUnblockedSignals);
        signal_index++) {
-    sigaction(kUnblockedSignals[signal_index],
-              &old_sigactions[signal_index], NULL);
+    CHECK_EQ(0, TidySigaction(kUnblockedSignals[signal_index],
+                              &old_sigactions[signal_index], NULL));
   }
   sigprocmask(SIG_SETMASK, &old_sigset, &old_sigset);
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D592.2.patch
Type: text/x-patch
Size: 4634 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130330/47281576/attachment.bin>