[PATCH] D20577: [esan|wset] Intercept and chain signal handlers

[Derek Bruening via llvm-commits]

bruening marked 3 inline comments as done.

================
Comment at: lib/esan/working_set_posix.cpp:67
@@ +66,3 @@
+
+static void handleShadowFault(int SigNum, void *Info, void *Ctx) {
+  if (SigNum == SIGSEGV) {
----------------
filcab wrote:
> `handleSEGV` for now. Possibly `handleSignal` later.
> This will be called for any SEGV, and `ShadowFault` isn't descriptive.
The purpose of this routine is to handle shadow page write faults.

================
Comment at: lib/esan/working_set_posix.cpp:84
@@ +83,3 @@
+
+void registerShadowFault() {
+  // We do not use an alternate signal stack, as doing so would require
----------------
filcab wrote:
> `registerSEGVHandler`
This routine will have the same name on every platform.  Using a UNIX-specific name is not ideal: "fault" will translate to Windows.

================
Comment at: lib/esan/working_set_posix.cpp:99
@@ +98,3 @@
+  SigAct.sa_flags = SA_SIGINFO | SA_NODEFER;
+  int Res = internal_sigaction(SIGSEGV, &SigAct, &AppSigAct);
+  CHECK(Res == 0);
----------------
aizatsky wrote:
> Could a signal happen before this? AppSigAct would be uninitialized in process* functions.
I think what you mean is could a call to signal() or sigaction() happen before this?  Yes, that is a general issue with all interceptors, and it will be fixed in a separate CL that ensures our whole runtime lib is initialized at the first interceptor that is encountered.

================
Comment at: test/esan/TestCases/workingset-fault.cpp:1
@@ +1,2 @@
+// RUN: %clang_esan_wset -O0 %s -o %t 2>&1
+// RUN: %run %t 2>&1 | FileCheck %s
----------------
aizatsky wrote:
> This test as it is doesn't test faults, but tests signal handling propagation, right? Maybe rename it?
> 
> (Optionally include "posix" in the name?)
It's both, testing propagation of faults (SIGSEGV) as no other signals need propagation.  Sure, posix sounds good.

================
Comment at: test/esan/TestCases/workingset-fault.cpp:32
@@ +31,3 @@
+  if (sigsetjmp(mark, 1) == 0)
+    *((volatile int *)(ssize_t)argc) = 42; // Raise SIGSEGV
+  fprintf(stderr, "Past longjmp for signal\n");
----------------
filcab wrote:
> Would `kill(SIGSEGV, getpid())` work?
If POSIX guarantees synchronous delivery before kill returns (and platforms implement that), which I believe is true for a single-threaded app, yes, but a true fault just feels like a more authentic test.


http://reviews.llvm.org/D20577