[PATCH] [compiler-rt] bug 23600 - sanitizer stack trace pc off by 1

[Alexey Samsonov]

================
Comment at: lib/sanitizer_common/sanitizer_stacktrace.h:84
@@ -78,3 +83,3 @@
   // Cancel Thumb bit.
-  pc = pc & (~1);
+  return pc & (~1);
 #endif
----------------
msebor wrote:
> kcc wrote:
> > I think the old code was intentional. 
> > It cancels the thumb bit (whatever that is) and then executes the rest of the function.
> > Did you test it on arm? 
> This change isn't necessary for the fix, but I included it because it was trivial and obvious (the function otherwise is missing a return statement), and because I didn't manage to create a test case that made the bug manifest in an observable way.  I tested the same change to the GCC sanitizer on arm with no regressions.
I also think the original code was written like that intentionally - we're first canceling the thumb bit, and then subtract one. I agree that it's probably should be rewritten as
  return (pc & (~1)) - 1;

================
Comment at: lib/sanitizer_common/sanitizer_stacktrace_libcdep.cc:37
@@ -33,1 +36,3 @@
+    const int adjust = (0 == i) && signaled ? 0 : 1;
+    const uptr pc = trace[i] - adjust;
     SymbolizedStack *frames = Symbolizer::GetOrInit()->SymbolizePC(pc);
----------------
msebor wrote:
> kcc wrote:
> > I frankly don't understand this. 
> > The old code was to call GetPreviousInstructionPc(), which on some archs subtracted 4 or 8.
> > Now you just subtract 1. Why? 
> DWARF line number programs are represented as ranges of addresses that correspond to a given source line.  When looking up a line number for a given PC value, the lookup compares the PC value to the beginning and end address of each range and considers it a match when the PC is anywhere in that range.  It doesn't matter if the PC value points to the beginning of an instruction in the range or some byte past it.  So subtracting 1 byte is just as good as subtracting the size of an instruction.
We've added architecture-specific `GetPreviousInstructionPc` for a reason: we don't care at all about actual instruction sizes and addresses, we just need PC's that would be correctly handled by symbolizer, and subtracting one provided to be insufficient. As, for the line programs, suppose I have two consecutive instructions:
<instruction 1> at PC [0x100, 0x102], source line 42.
<instruction 2> at PC [0x104, 0x105], source line 43.

Note that instructions are 4-byte aligned. I can imagine the line table for this program matching [0x100,0x102] to line 42, and [0x104,0x105] to line 43. The lookup of 0x103 would not find any line table entry, and thus return no line number at all.



================
Comment at: test/sanitizer_common/TestCases/print-stack-trace-pc.cc:7
@@ +6,3 @@
+// RUN: %clangxx -O0 %s -o %t && %tool_symbolizer_path=%llvm_obj_root/bin/llvm-symbolizer %tool_options='stack_trace_format="#%%n %%p %%f:%%l"' %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx -O3 %s -o %t && %tool_symbolizer_path=%llvm_obj_root/bin/llvm-symbolizer %tool_options='stack_trace_format="#%%n %%p %%f:%%l"' %run %t 2>&1 | FileCheck %s
+
----------------
msebor wrote:
> kcc wrote:
> > This is strange. The test should know where the symbolizer is, 
> > other tests don't require this. 
> As far as I can tell, no other ASan test depends on the symbolizer and the other sanitizers use their own symbolizer. I didn't spend too much time debugging this beyond simply making it work.
All tests under `compiler-rt/test/sanitizer_common` depend on the llvm-symbolizer (the dependency is added in compiler-rt/test/CMakeLists.txt). It should be available in $PATH, and thus picked up by ASan/TSan/MSan.

http://reviews.llvm.org/D10065

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/