[Asan] Pack signal context into a structure

[kutuzov.viktor.84]

asan/asan_internal.h

65 ↗

(On Diff #15851)

Umm, we probably don't need this line.

[samsonov]

asan/asan_internal.h

73 ↗

(On Diff #15851)

explicit is not necessary for two-argument ctors.
Why not make the body empty and just have a single initializer list?

SignalContext(void *context, uptr addr) : context(context), addr(addr), pc(0), sp(0), bp(0) {}

You can also make pc/sp/bp optional ctor arguments (looks like you can use that on Windows)

93 ↗

(On Diff #15851)

We generally don't use "struct" keyword for passing our structs around.

asan/asan_report.h

55 ↗

(On Diff #15851)

You can make these function take

const SignalContext &sig;

[kutuzov.viktor.84]

Updated as suggested.

[samsonov]

asan/asan_internal.h

91 ↗

(On Diff #15983)

Ok, now it becomes somewhat confusing - on POSIX the user must create SignalContext that would initialize half of the fields, and then call GetPcSpBp, that would initialize the rest of them. This looks... weird. It seems that the work done in GetPcSpBp should be done in SignalContext ctor. How do you feel about this: replace SignalContext ctor with a factory function (SignalContext::Create or whatever), that would take two opaque pointers and return a fully-constructed SignalContext object. The meaning of pointers will be different on different platforms:

• on POSIX that would be context_t* (used to initialize context, pc, sp and bp) and siginfo_t* (used to initialize addr).
• on Windows that would be CONTEXT* and EXCEPTION_RECORD*

asan/asan_stack.h

84 ↗

(On Diff #15983)

Why not (sig).pc ?

[kutuzov.viktor.84]

Updated. Thanks, Alexey.

[samsonov]

LGTM (but please the problem for powerpc before commit)

lib/asan/asan_posix.cc

61 ↗

(On Diff #16553)

Please fix the code under this #if: s/pc/sig.pc.

Closed by commit rL222756 (authored by vkutuzov).

compiler-rt/trunk/lib/asan/asan_internal.h

 // to avoid namespace collisions with the user programs.
 // Separate namespace also makes it simpler to distinguish the asan run-time
 // functions from the instrumented user code in a profile.
 namespace __asan {
 
 class AsanThread;
 using __sanitizer::StackTrace;
 
+struct SignalContext {
+  void *context;
+  uptr addr;
+  uptr pc;
+  uptr sp;
+  uptr bp;
+
+  SignalContext(void *context, uptr addr, uptr pc, uptr sp, uptr bp) :
+      context(context), addr(addr), pc(pc), sp(sp), bp(bp) {
+  }
+
+  // Creates signal context in a platform-specific manner.
+  static SignalContext Create(void *siginfo, void *context);
+};
+
 void AsanInitFromRtl();
 
 // asan_rtl.cc
 void NORETURN ShowStatsAndAbort();
 
 // asan_malloc_linux.cc / asan_malloc_mac.cc
 void ReplaceSystemMalloc();
 

compiler-rt/trunk/lib/asan/asan_posix.cc

 #include <signal.h>
 #include <stdlib.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include <unistd.h>
 
 namespace __asan {
 
+SignalContext SignalContext::Create(void *siginfo, void *context) {
+  uptr addr = (uptr)((siginfo_t*)siginfo)->si_addr;
+  uptr pc, sp, bp;
+  GetPcSpBp(context, &pc, &sp, &bp);
+  return SignalContext(context, addr, pc, sp, bp);
+}
+
 void AsanOnSIGSEGV(int, void *siginfo, void *context) {
   ScopedDeadlySignal signal_scope(GetCurrentThread());
-  uptr addr = (uptr)((siginfo_t*)siginfo)->si_addr;
   int code = (int)((siginfo_t*)siginfo)->si_code;
   // Write the first message using the bullet-proof write.
   if (13 != internal_write(2, "ASAN:SIGSEGV\n", 13)) Die();
-  uptr pc, sp, bp;
-  GetPcSpBp(context, &pc, &sp, &bp);
+  SignalContext sig = SignalContext::Create(siginfo, context);
 
   // Access at a reasonable offset above SP, or slightly below it (to account
   // for x86_64 or PowerPC redzone, ARM push of multiple registers, etc) is
   // probably a stack overflow.
-  bool IsStackAccess = addr + 512 > sp && addr < sp + 0xFFFF;
+  bool IsStackAccess = sig.addr + 512 > sig.sp && sig.addr < sig.sp + 0xFFFF;
 
 #if __powerpc__
   // Large stack frames can be allocated with e.g.
   //   lis r0,-10000
   //   stdux r1,r1,r0 # store sp to [sp-10000] and update sp by -10000
   // If the store faults then sp will not have been updated, so test above
   // will not work, becase the fault address will be more than just "slightly"
   // below sp.
-  if (!IsStackAccess && IsAccessibleMemoryRange(pc, 4)) {
-    u32 inst = *(unsigned *)pc;
+  if (!IsStackAccess && IsAccessibleMemoryRange(sig.pc, 4)) {
+    u32 inst = *(unsigned *)sig.pc;
     u32 ra = (inst >> 16) & 0x1F;
     u32 opcd = inst >> 26;
     u32 xo = (inst >> 1) & 0x3FF;
     // Check for store-with-update to sp. The instructions we accept are:
     //   stbu rs,d(ra)          stbux rs,ra,rb
     //   sthu rs,d(ra)          sthux rs,ra,rb
     //   stwu rs,d(ra)          stwux rs,ra,rb
     //   stdu rs,ds(ra)         stdux rs,ra,rb
     // where ra is r1 (the stack pointer).
     if (ra == 1 &&
         (opcd == 39 || opcd == 45 || opcd == 37 || opcd == 62 ||
          (opcd == 31 && (xo == 247 || xo == 439 || xo == 183 || xo == 181))))
       IsStackAccess = true;
   }
 #endif // __powerpc__
 
   // We also check si_code to filter out SEGV caused by something else other
   // then hitting the guard page or unmapped memory, like, for example,
   // unaligned memory access.
   if (IsStackAccess && (code == si_SEGV_MAPERR || code == si_SEGV_ACCERR))
-    ReportStackOverflow(pc, sp, bp, context, addr);
+    ReportStackOverflow(sig);
   else
-    ReportSIGSEGV("SEGV", pc, sp, bp, context, addr);
+    ReportSIGSEGV("SEGV", sig);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
 static pthread_key_t tsd_key;
 static bool tsd_key_inited = false;
 void AsanTSDInit(void (*destructor)(void *tsd)) {
   CHECK(!tsd_key_inited);

compiler-rt/trunk/lib/asan/asan_report.h

bool ParseFrameDescription(const char *frame_descr,
                            InternalMmapVector<StackVarDescr> *vars);
 bool DescribeAddressIfStack(uptr addr, uptr access_size);
 // Determines memory type on its own.
 void DescribeAddress(uptr addr, uptr access_size);
 
 void DescribeThread(AsanThreadContext *context);
 
 // Different kinds of error reports.
-void NORETURN
-    ReportStackOverflow(uptr pc, uptr sp, uptr bp, void *context, uptr addr);
-void NORETURN ReportSIGSEGV(const char *description, uptr pc, uptr sp, uptr bp,
-                            void *context, uptr addr);
+void NORETURN ReportStackOverflow(const SignalContext &sig);
+void NORETURN ReportSIGSEGV(const char *description, const SignalContext &sig);
 void NORETURN ReportNewDeleteSizeMismatch(uptr addr, uptr delete_size,
                                           BufferedStackTrace *free_stack);
 void NORETURN ReportDoubleFree(uptr addr, BufferedStackTrace *free_stack);
 void NORETURN ReportFreeNotMalloced(uptr addr, BufferedStackTrace *free_stack);
 void NORETURN ReportAllocTypeMismatch(uptr addr, BufferedStackTrace *free_stack,
                                       AllocType alloc_type,
                                       AllocType dealloc_type);
 void NORETURN

compiler-rt/trunk/lib/asan/asan_report.cc

NORETURN ~ScopedInErrorReport() {
     if (error_report_callback) {
       error_report_callback(error_message_buffer);
     }
     Report("ABORTING\n");
     Die();
   }
 };
 
-void ReportStackOverflow(uptr pc, uptr sp, uptr bp, void *context, uptr addr) {
+void ReportStackOverflow(const SignalContext &sig) {
   ScopedInErrorReport in_report;
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: stack-overflow on address %p"
       " (pc %p bp %p sp %p T%d)\n",
-      (void *)addr, (void *)pc, (void *)bp, (void *)sp,
+      (void *)sig.addr, (void *)sig.pc, (void *)sig.bp, (void *)sig.sp,
       GetCurrentTidOrInvalid());
   Printf("%s", d.EndWarning());
-  GET_STACK_TRACE_SIGNAL(pc, bp, context);
+  GET_STACK_TRACE_SIGNAL(sig);
   stack.Print();
   ReportErrorSummary("stack-overflow", &stack);
 }
 
-void ReportSIGSEGV(const char *description, uptr pc, uptr sp, uptr bp,
-                   void *context, uptr addr) {
+void ReportSIGSEGV(const char *description, const SignalContext &sig) {
   ScopedInErrorReport in_report;
   Decorator d;
   Printf("%s", d.Warning());
   Report(
       "ERROR: AddressSanitizer: %s on unknown address %p"
       " (pc %p bp %p sp %p T%d)\n",
-      description, (void *)addr, (void *)pc, (void *)bp, (void *)sp,
-      GetCurrentTidOrInvalid());
-  if (pc < GetPageSizeCached()) {
+      description, (void *)sig.addr, (void *)sig.pc, (void *)sig.bp,
+      (void *)sig.sp, GetCurrentTidOrInvalid());
+  if (sig.pc < GetPageSizeCached()) {
     Report("Hint: pc points to the zero page.\n");
   }
   Printf("%s", d.EndWarning());
-  GET_STACK_TRACE_SIGNAL(pc, bp, context);
+  GET_STACK_TRACE_SIGNAL(sig);
   stack.Print();
-  MaybeDumpInstructionBytes(pc);
+  MaybeDumpInstructionBytes(sig.pc);
   Printf("AddressSanitizer can not provide additional info.\n");
   ReportErrorSummary("SEGV", &stack);
 }
 
 void ReportDoubleFree(uptr addr, BufferedStackTrace *free_stack) {
   ScopedInErrorReport in_report;
   Decorator d;
   Printf("%s", d.Warning());

compiler-rt/trunk/lib/asan/asan_stack.h

GetStackTraceWithPcBpAndContext(&stack, max_size,                          \
                                     GET_CURRENT_FRAME(), 0, fast);             \
   }
 
 #define GET_STACK_TRACE_FATAL(pc, bp)                                          \
   BufferedStackTrace stack;                                                    \
   GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax, pc, bp, 0,           \
                                   common_flags()->fast_unwind_on_fatal)
 
-#define GET_STACK_TRACE_SIGNAL(pc, bp, context)                                \
+#define GET_STACK_TRACE_SIGNAL(sig)                                            \
   BufferedStackTrace stack;                                                    \
-  GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax, pc, bp, context,     \
+  GetStackTraceWithPcBpAndContext(&stack, kStackTraceMax,                      \
+                                  (sig).pc, (sig).bp, (sig).context,           \
                                   common_flags()->fast_unwind_on_fatal)
 
 #define GET_STACK_TRACE_FATAL_HERE                                \
   GET_STACK_TRACE(kStackTraceMax, common_flags()->fast_unwind_on_fatal)
 
 #define GET_STACK_TRACE_CHECK_HERE                                \
   GET_STACK_TRACE(kStackTraceMax, common_flags()->fast_unwind_on_check)
 

compiler-rt/trunk/lib/asan/asan_win.cc

 }
 
 void AsanOnSIGSEGV(int, void *siginfo, void *context) {
   UNIMPLEMENTED();
 }
 
 static LPTOP_LEVEL_EXCEPTION_FILTER default_seh_handler;
 
-static long WINAPI SEHHandler(EXCEPTION_POINTERS *info) {
-  EXCEPTION_RECORD *exception_record = info->ExceptionRecord;
-  CONTEXT *context = info->ContextRecord;
+SignalContext SignalContext::Create(void *siginfo, void *context) {
+  EXCEPTION_RECORD *exception_record = (EXCEPTION_RECORD*)siginfo;
+  CONTEXT *context_record = (CONTEXT*)context;
+
   uptr pc = (uptr)exception_record->ExceptionAddress;
 #ifdef _WIN64
-  uptr bp = (uptr)context->Rbp, sp = (uptr)context->Rsp;
+  uptr bp = (uptr)context_record->Rbp;
+  uptr sp = (uptr)context_record->Rsp;
 #else
-  uptr bp = (uptr)context->Ebp, sp = (uptr)context->Esp;
+  uptr bp = (uptr)context_record->Ebp;
+  uptr sp = (uptr)context_record->Esp;
 #endif
+  uptr access_addr = exception_record->ExceptionInformation[1];
+
+  return SignalContext(context, access_addr, pc, sp, bp);
+}
+
+static long WINAPI SEHHandler(EXCEPTION_POINTERS *info) {
+  EXCEPTION_RECORD *exception_record = info->ExceptionRecord;
+  CONTEXT *context = info->ContextRecord;
 
   if (exception_record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION ||
       exception_record->ExceptionCode == EXCEPTION_IN_PAGE_ERROR) {
     const char *description =
         (exception_record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION)
             ? "access-violation"
             : "in-page-error";
-    uptr access_addr = exception_record->ExceptionInformation[1];
-    ReportSIGSEGV(description, pc, sp, bp, context, access_addr);
+    SignalContext sig = SignalContext::Create(exception_record, context);
+    ReportSIGSEGV(description, sig);
   }
 
   // FIXME: Handle EXCEPTION_STACK_OVERFLOW here.
 
   return default_seh_handler(info);
 }
 
 // We want to install our own exception handler (EH) to print helpful reports