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

[Kostya Serebryany]

================
Comment at: lib/sanitizer_common/sanitizer_stacktrace.h:84
@@ -78,3 +83,3 @@
   // Cancel Thumb bit.
-  pc = pc & (~1);
+  return pc & (~1);
 #endif
----------------
samsonov wrote:
> 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;
>> the function otherwise is missing a return statement
No it is not. There is an #else clause. 

It's very sad that none of our tests cover this use case (although you can't be sure w/o running clang tests on arm),
but it is not a good reason to change the logic of the code. 

================
Comment at: test/sanitizer_common/TestCases/print-stack-trace-pc.cc:1
@@ +1,2 @@
+// Run each sanitizer test with XSAN_SYMBOLIZER_PATH pointing at
+// bin/llvm-symbolizer and with XSAN_OPTIONS set to a stack trace
----------------
msebor wrote:
> kcc wrote:
> > That's a great test to have, but it is not testing the case with signals, is it? 
> No, it isn't. It didn't seem too important since the code that generates the stack trace is the same in either case.
> 
> What seemed more important to me and what's not covered is testing the PC value for the active frame.  Unfortunately, I'm not aware of a straightforward and portable way of getting the PC value in this case.  I considered two approaches to verifying that the PC value for the active frame when the stack trace is generated in response to a signal:
> 
> 1) Invoke GDB and have it print the stack trace and use it for comparison.
> 
> 2) Handle the signal, extract and print the PC value in it, and raise it again to let the sanitizer generate its own.
> 
> I rejected (1) because I couldn't find any other tests that relied on GDB and introducing a dependency for just this one test seemed like overkill.  Adding the machinery to invoke GDB also seemed like a nontrivial effort that I cannot justify.
> 
> (2) should be doable on POSIX using sigaction and siginfo_t but wouldn't work on non-POSIX targets.  Since I don't have access to such targets I decided to keep the test simple at the expense of not exercising this case.
> 
> If you can suggest a portable way that's not overly involved I'd be happy to code it up.
Yea, I'd like to keep gdb out of our tests, it's too heavy. 
But I am totally find with having a posix-only or even a linux-only test that uses sigaction.
(put it into test/sanitizer_common/TestCases/Linux/)
Since the patch aims exactly at signals, it sounds like having such a test would be nice. 

================
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
----------------
msebor wrote:
> 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.
[[@LINE]] matches the current line number
[[@LINE-1]] matches the previous, and so on. 
So, do something like this

DoSomething(__LINE__); 
// CHECK: ExpexctedFoo:[[@LINE-1]]
// CHECK: UnrelatedStuff1
// CHECK: UnrelatedStuff2
// CHECK: ExpexctedBar:[[@LINE-4]]

You will not need #define FOO_LINE nor the #line directives

http://reviews.llvm.org/D10065

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