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

[Sergey Matveev]

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:257
@@ +256,3 @@
+    guard_size_ = GetPageSizeCached();
+    guard_start_ = MmapOrDie(stack_size_ + guard_size_,
+                             "ScopedStackWithGuard");
----------------
Alexander Potapenko wrote:
> You're silently omitting MAP_STACK, which is currently a no-op but may be implemented later in the kernel.
> It's fine to keep it for now, but please add a comment regarding that.
Done.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:259
@@ +258,3 @@
+                             "ScopedStackWithGuard");
+    void *res = Mprotect((uptr)guard_start_, guard_size_);
+    CHECK_EQ(res, guard_start_);
----------------
Alexander Potapenko wrote:
> Kostya Serebryany wrote:
> > CHECK_EQ(guard_size, Mprotect(...))
> Please be consistent with spaces when you're declaring pointers: either "void *res" or "void* res"
> I know it's different across the codebase, but please choose one within a single file.
Done (assuming you mean guard_start_).

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:259
@@ +258,3 @@
+                             "ScopedStackWithGuard");
+    void *res = Mprotect((uptr)guard_start_, guard_size_);
+    CHECK_EQ(res, guard_start_);
----------------
Sergey Matveev wrote:
> Alexander Potapenko wrote:
> > Kostya Serebryany wrote:
> > > CHECK_EQ(guard_size, Mprotect(...))
> > Please be consistent with spaces when you're declaring pointers: either "void *res" or "void* res"
> > I know it's different across the codebase, but please choose one within a single file.
> Done (assuming you mean guard_start_).
Fixed.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:265
@@ +264,3 @@
+  }
+  void *Bottom() {
+    return (void *)((uptr)guard_start_ + stack_size_ + guard_size_);
----------------
Alexander Potapenko wrote:
> const
Ok.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:272
@@ +271,3 @@
+  uptr guard_size_;
+  void* guard_start_;
+};
----------------
Kostya Serebryany wrote:
> make it uptr?
Ok.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:281
@@ -259,1 +280,3 @@
   // which modify errno (which is shared between the two threads).
+  sigset_t blocked_sigset;
+  sigset_t old_sigset;
----------------
Alexey Samsonov wrote:
> We've turned these into globals because sanitizer tools (well, TSan) have strict limitations on the stack frame size (512 bytes). sizeof(sigset_t) and sizeof(old_sigactions) are large enough. To test your changes:
> $ cd projects/compiler-rt/lib/tsan
> $ make -f Makefile.old presubmit
I see.
I implemented a hack that allows us to keep those globals. Unfortunately the only alternative is to reimplement sigaction, and that requires some magical assembler inlines that have to be copied from somewhere else.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:276
@@ +275,3 @@
+    // Omitting MAP_STACK here works in current kernels but might break later.
+    guard_start_ = (uptr) MmapOrDie(stack_size_ + guard_size_,
+                                   "ScopedStackWithGuard");
----------------
Alexey Samsonov wrote:
> (uptr)MmapOrDir (w/o space) for consistency
Done.

================
Comment at: lib/sanitizer_common/sanitizer_stoptheworld_linux.cc:74
@@ -73,1 +73,3 @@
 namespace __sanitizer {
+static const uptr kTracerStackSize = 2 * 1024 * 1024;
+
----------------
Alexey Samsonov wrote:
> Please move this to the place of its usage. This doesn't have to be a global constant
Done.


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