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

Derek Bruening via llvm-commits llvm-commits at lists.llvm.org
Wed May 25 10:04:57 PDT 2016

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.


More information about the llvm-commits mailing list