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

[Derek Bruening via llvm-commits]

bruening marked an inline comment as done.

================
Comment at: lib/esan/working_set_posix.cpp:68
@@ +67,3 @@
+static void handleShadowFault(int SigNum, void *Info, void *Ctx) {
+  if (SigNum == SIGSEGV) {
+
----------------
filcab wrote:
> But it handles any and all SEGV, right? Not just in the shadow memory.
> A "shadow fault" doesn't really mean much by itself either (unless you're familiar with this code, of course).
> Maybe we can agree on `handleMemoryFault`? I'm ok with not adding the POSIX-related names, since it's generic. But I'd rather not mention `Shadow` like that in a function that will be called for things outside the shadow memory.
> But it handles any and all SEGV, right? Not just in the shadow memory.

It doesn't take its own action on non-shadow faults: it passes them through to the app.  Thus, a non-shadow fault remains un-handled: we let the app handle it.  It really depends on how you think of "handle".

> A "shadow fault" doesn't really mean much by itself either (unless you're familiar with this code, of course).

I'll assume that what you mean is "I myself have never seen this phrase before".  That does not make it a poor term.  It is descriptive and self-explanatory in the context of a shadow memory tool.

I am ok changing these names, as it is true that other faults will pass through these routines, but the original names nicely captured what the code is trying to accomplish: to take action on shadow memory faults.  We would really prefer to ignore all other faults, but the signal mechanism forces us to chain a general handler with the app's and to pass through the non-shadow faults.

================
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:
> bruening wrote:
> > 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.
> Fair enough. No need casting argc twice, btw.
> I cringe looking at that line, but couldn't get you an easier way to trigger the fault off the top of my head.
Two casts are needed to avoid a warning about casting a non-pointer-sized integer to a pointer.


http://reviews.llvm.org/D20577