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

[Martin Sebor]

================
Comment at: lib/sanitizer_common/sanitizer_stacktrace.h:84
@@ -78,3 +83,3 @@
   // Cancel Thumb bit.
-  pc = pc & (~1);
+  return pc & (~1);
 #endif
----------------
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.

================
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);
----------------
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.

================
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
+
----------------
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.

================
Comment at: test/sanitizer_common/TestCases/print-stack-trace-pc.cc:22
@@ +21,3 @@
+#define FOO_LINE  101
+#define BAR_LINE  201
+#define MAIN_LINE 301
----------------
kcc wrote:
> I think you can make this test simpler by using e.g. [[@LINE-1]] in the CHECK: line, see e.g. 
> test/asan/TestCases/unaligned_loads_and_stores.cc
> 
> Just add the CHECK lines closer to the code, then use a small constant offset, e.g. [[@LINE-4]]
I'm afraid I don't see  how to simplify the test by making use of the [[@LINE]] expressions.  The test uses the XXX_LINE constants to set the line numbers printed by the first backtrace (the for loop in foo) and to set (via the #line directives) the corresponding line numbers printed by the sanitizer backtrace.  The two sets of numbers must match.  Can you flesh out in a bit more detail what you have in mind?

I tried changing the test to make use of [[@LINE]] and while it seems that it should be possible to do as you suggested I'm having trouble getting FileCheck to match the patterns for the second and subsequent pairs of CHECK directives (i.e., in bar and main).  I spent quite a bit of time figuring out how to get CHECK to work the way I wanted to for the first patch and I'm not sure that investing even more time into changing it to use @LINE would be worth it.

================
Comment at: test/sanitizer_common/TestCases/print-stack-trace-pc.cc:26
@@ +25,3 @@
+void __attribute__((weak)) foo(void) {
+    *pfrm++ = (struct StackFrame) {
+        __builtin_return_address(0), __func__, FOO_LINE
----------------
kcc wrote:
> This code repeats 3 times, right? 
> Maybe put it in a macro?
> 
Sure, I can add a macro for this.

http://reviews.llvm.org/D10065

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