Patchwork [RFC] Enable libsanitizer on powerpc{,64}

login
register
mail settings
Submitter Peter Bergner
Date Nov. 20, 2012, 8:16 p.m.
Message ID <1353442604.17833.131.camel@otta>
Download mbox | patch
Permalink /patch/200503/
State New
Headers show

Comments

Peter Bergner - Nov. 20, 2012, 8:16 p.m.
On Tue, 2012-11-20 at 23:24 +0400, Konstantin Serebryany wrote:
> On Tue, Nov 20, 2012 at 11:08 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h
> > --- gcc-fsf-mainline-kcc/libsanitizer/sanitizer_common/sanitizer_stacktrace.h   2012-11-20 11:42:08.821389243 -0600
> > +++ gcc-fsf-mainline-asan/libsanitizer/sanitizer_common/sanitizer_stacktrace.h  2012-11-20 11:12:44.551390980 -0600
> > @@ -23,6 +23,7 @@ struct StackTrace {
> >    uptr size;
> >    uptr max_size;
> >    uptr trace[kStackTraceMax];
> > +  uptr frame[kStackTraceMax];  // For use by _Unwind_Backtrace architectures.
> 
> This is bad. The objects of this type are already too big, we should
> not make them 2x larger.
> Hopefully Evgeniy can handle this tomorrow.

Ok, here's another attempt that doesn't require storing the frame pointers.
In this case, we pass down the frame pointer we're looking for into the
unwind code and if we come across it while building up the trace, we immediately
empty the trace and start over, effectively popping the ASAN functions from
the trace.  If we never encounter the passed down frame pointer, then the code
will just behave as before.  Thoughts?

Peter
Richard Henderson - Nov. 20, 2012, 8:36 p.m.
On 11/20/2012 12:16 PM, Peter Bergner wrote:
> +uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
> +  return _Unwind_GetCFA(ctx);
> +}
> +
> +struct Unwind_Trace_Info {
> +  StackTrace *stack;
> +  uptr bp;
> +};
> +
>  _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
>      void *param) {
> -  StackTrace *b = (StackTrace*)param;
> -  CHECK(b->size < b->max_size);
> +  Unwind_Trace_Info *p = (Unwind_Trace_Info *)param;
> +  StackTrace *b = p->stack;
>    uptr pc = Unwind_GetIP(ctx);
> +  if (Unwind_GetBP(ctx) == p->bp) {

BP will only equal the CFA on some targets.  It really depends on how the target sets up the stack frame.  It will also depend on the target actually using frame pointers.

On the other hand, CFA = SP on the next frame up.  And that's rather more reliable based on how we work with dwarf2 and define the CFA.  Only very unusual functions have CFA != the incoming SP -- asm versions of longjmp for example.


r~
Peter Bergner - Nov. 20, 2012, 10:14 p.m.
On Tue, 2012-11-20 at 12:36 -0800, Richard Henderson wrote:
> BP will only equal the CFA on some targets.  It really depends on how the
> target sets up the stack frame.

Are you talking about leaf routines like on ppc64 where we don't
decrement the stack pointer?  If so, that's not a concern here
because the ASAN tests insert a call so none of the instrumented
functions will be leaf routines.


> It will also depend on the target actually using frame pointers.

That is problematical, except for...


> On the other hand, CFA = SP on the next frame up.  And that's rather more
> reliable based on how we work with dwarf2 and define the CFA.  Only very
> unusual functions have CFA != the incoming SP -- asm versions of longjmp
> for example.

Doesn't this save us, since the bottom frame in the backtrace will always
be an ASAN functionand the frame we're interested in will always be higher
in the backtrace?

I guess I'm wondering, in this specific use case, do you think using
the CFA to compare against is safe or not?

Peter
Richard Henderson - Nov. 20, 2012, 10:27 p.m.
On 11/20/2012 02:14 PM, Peter Bergner wrote:
> Doesn't this save us, since the bottom frame in the backtrace will always
> be an ASAN functionand the frame we're interested in will always be higher
> in the backtrace?
> 
> I guess I'm wondering, in this specific use case, do you think using
> the CFA to compare against is safe or not?

Yes it saves us.  I believe using the value of __builtin_dwarf_cfa from
the outermost ASAN function will reliably match the SP value of the
interesting user function outer of ASAN.


r~
Evgeniy Stepanov - Nov. 21, 2012, 9:46 a.m.
The ARM/Android failure is due to libstdc++ in android-ndk-r8b not
containing debug info. As a result, stack unwinding breaks in
"operator new", after exactly 2 frames. I guess we can simply tweak
the assert to be OK with empty stack traces when user code stack can
not be unwinded.

Matching FP or SP also sounds good, and perhaps more reliable than
just popping 2 frames from the top of the stack.

AFAIK, the debug info issue is fixed in the latest NDK release.


On Wed, Nov 21, 2012 at 2:27 AM, Richard Henderson <rth@redhat.com> wrote:
> On 11/20/2012 02:14 PM, Peter Bergner wrote:
>> Doesn't this save us, since the bottom frame in the backtrace will always
>> be an ASAN functionand the frame we're interested in will always be higher
>> in the backtrace?
>>
>> I guess I'm wondering, in this specific use case, do you think using
>> the CFA to compare against is safe or not?
>
> Yes it saves us.  I believe using the value of __builtin_dwarf_cfa from
> the outermost ASAN function will reliably match the SP value of the
> interesting user function outer of ASAN.
>
>
> r~
Peter Bergner - Nov. 21, 2012, 4:16 p.m.
On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
> Matching FP or SP also sounds good, and perhaps more reliable than
> just popping 2 frames from the top of the stack.

Agreed.  Can you try my second patch that searches for the frame
address we want our backtrace to start with and see if that works
for ARM?  The nice thing about that patch is that we won't have
to play any games with forcing or disabling inlining of any of
the ASAN functions which me might have to do if we always pop
2 frames off the stack.  It would also be tolerant of adding
any number of new function calls in between the current two
ASAN function at the top of the backtrace.

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html

Bah, ignore that first diff of the LAST_UPDATED file. :(

Peter
Konstantin Serebryany - Nov. 21, 2012, 4:22 p.m.
On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>> Matching FP or SP also sounds good, and perhaps more reliable than
>> just popping 2 frames from the top of the stack.
>
> Agreed.  Can you try my second patch that searches for the frame
> address we want our backtrace to start with and see if that works
> for ARM?  The nice thing about that patch is that we won't have
> to play any games with forcing or disabling inlining of any of
> the ASAN functions which me might have to do if we always pop
> 2 frames off the stack.  It would also be tolerant of adding
> any number of new function calls in between the current two
> ASAN function at the top of the backtrace.
>
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>
> Bah, ignore that first diff of the LAST_UPDATED file. :(

I'd actually prefer to keep the current code that pops two frames (if
it works for you)
Evgeniy seems to know how to fix the ARM case.

--kcc

>
> Peter
>
>
>
Peter Bergner - Nov. 21, 2012, 5:07 p.m.
On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
> On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
> >> Matching FP or SP also sounds good, and perhaps more reliable than
> >> just popping 2 frames from the top of the stack.
> >
> > Agreed.  Can you try my second patch that searches for the frame
> > address we want our backtrace to start with and see if that works
> > for ARM?  The nice thing about that patch is that we won't have
> > to play any games with forcing or disabling inlining of any of
> > the ASAN functions which me might have to do if we always pop
> > 2 frames off the stack.  It would also be tolerant of adding
> > any number of new function calls in between the current two
> > ASAN function at the top of the backtrace.
> >
> > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
> >
> > Bah, ignore that first diff of the LAST_UPDATED file. :(
> 
> I'd actually prefer to keep the current code that pops two frames
> (if it works for you)

Well it does work for me, since I wrote it. :)  That being said, the
change where I always pop two frames off of the backtrace was more of
a proof of concept that if I can remove those ASAN functions from the
backtrace, do we pass the test case in the testsuite.  It did, but I
have to admit that code is extremely fragile.  It is dependent not
only on the inlining heuristics of one compiler, but of two compilers!
Not to mention people building debugable compilers with -O0 -fno-inline,
etc. etc.  We'd also have to make sure no one in the future adds any
ASAN function calls in between the report function and the GetBackTrace
calls.  It just seems like there are so many things that could go wrong,
that something is bound to.


> Evgeniy seems to know how to fix the ARM case.

His fix was to do:

 void StackTrace::PopStackFrames(uptr count) {
-  CHECK(size > count);
+  CHECK(size >= count);
   size -= count;
   for (uptr i = 0; i < size; i++) {
     trace[i] = trace[i + count];

Basically, that is allowing for us to pop off all of the frames from
the backtrace leaving an empty backtrace.  That can't be helpful in
tracking down the address violation, can it?  With my patch above,
either we find the frame we want to start our backtrace with, or
it returns the entire backtrace, ASAN functions and all.  Isn't that
better from a diagnostic point of view?

That being said, I'd still like to hear from Evgeniy whether my
patch above helps ARM or not.

Peter
Evgeniy Stepanov - Nov. 22, 2012, 8:38 a.m.
Yes, it has 3 asan-rtl frames on top. I'm not sure why this does not
happen on ppc.

    #0 0x40122cdb in __asan::GetStackTrace(__sanitizer::StackTrace*,
unsigned long, unsigned long, unsigned long)
    #1 0x40125167 in __asan_report_error
    #2 0x40125af3 in __asan_report_load1


On Thu, Nov 22, 2012 at 12:10 AM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> I'm looking into the empty stack issue, at this point it looks like a weird
> linker bug. But its completely orthogonal to this discussion.
>
> I recall that the stack trace of the offending memory access has in fact
> three extra frames on top. I'll verify tomorrow. If so, FP/SP matching
> solution is preferable.
>
> On Nov 21, 2012 9:08 PM, "Peter Bergner" <bergner@vnet.ibm.com> wrote:
>>
>> On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
>> > On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com>
>> > wrote:
>> > > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>> > >> Matching FP or SP also sounds good, and perhaps more reliable than
>> > >> just popping 2 frames from the top of the stack.
>> > >
>> > > Agreed.  Can you try my second patch that searches for the frame
>> > > address we want our backtrace to start with and see if that works
>> > > for ARM?  The nice thing about that patch is that we won't have
>> > > to play any games with forcing or disabling inlining of any of
>> > > the ASAN functions which me might have to do if we always pop
>> > > 2 frames off the stack.  It would also be tolerant of adding
>> > > any number of new function calls in between the current two
>> > > ASAN function at the top of the backtrace.
>> > >
>> > > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>> > >
>> > > Bah, ignore that first diff of the LAST_UPDATED file. :(
>> >
>> > I'd actually prefer to keep the current code that pops two frames
>> > (if it works for you)
>>
>> Well it does work for me, since I wrote it. :)  That being said, the
>> change where I always pop two frames off of the backtrace was more of
>> a proof of concept that if I can remove those ASAN functions from the
>> backtrace, do we pass the test case in the testsuite.  It did, but I
>> have to admit that code is extremely fragile.  It is dependent not
>> only on the inlining heuristics of one compiler, but of two compilers!
>> Not to mention people building debugable compilers with -O0 -fno-inline,
>> etc. etc.  We'd also have to make sure no one in the future adds any
>> ASAN function calls in between the report function and the GetBackTrace
>> calls.  It just seems like there are so many things that could go wrong,
>> that something is bound to.
>>
>>
>> > Evgeniy seems to know how to fix the ARM case.
>>
>> His fix was to do:
>>
>>  void StackTrace::PopStackFrames(uptr count) {
>> -  CHECK(size > count);
>> +  CHECK(size >= count);
>>    size -= count;
>>    for (uptr i = 0; i < size; i++) {
>>      trace[i] = trace[i + count];
>>
>> Basically, that is allowing for us to pop off all of the frames from
>> the backtrace leaving an empty backtrace.  That can't be helpful in
>> tracking down the address violation, can it?  With my patch above,
>> either we find the frame we want to start our backtrace with, or
>> it returns the entire backtrace, ASAN functions and all.  Isn't that
>> better from a diagnostic point of view?
>>
>> That being said, I'd still like to hear from Evgeniy whether my
>> patch above helps ARM or not.
>>
>> Peter
>>
>>
>>
>
Konstantin Serebryany - Nov. 22, 2012, 5:36 p.m.
On Thu, Nov 22, 2012 at 12:38 PM, Evgeniy Stepanov
<eugeni.stepanov@gmail.com> wrote:
> Yes, it has 3 asan-rtl frames on top. I'm not sure why this does not
> happen on ppc.

Different inlining inside run-time.
I still prefer a simple PopFrames to anything else, I am sure we can
make it reliable.
the asan run-time should always be build with optimizations on and we
can force or (better) forbid inlining for any function.

--kcc

>
>     #0 0x40122cdb in __asan::GetStackTrace(__sanitizer::StackTrace*,
> unsigned long, unsigned long, unsigned long)
>     #1 0x40125167 in __asan_report_error
>     #2 0x40125af3 in __asan_report_load1
>
>
> On Thu, Nov 22, 2012 at 12:10 AM, Evgeniy Stepanov
> <eugeni.stepanov@gmail.com> wrote:
>> I'm looking into the empty stack issue, at this point it looks like a weird
>> linker bug. But its completely orthogonal to this discussion.
>>
>> I recall that the stack trace of the offending memory access has in fact
>> three extra frames on top. I'll verify tomorrow. If so, FP/SP matching
>> solution is preferable.
>>
>> On Nov 21, 2012 9:08 PM, "Peter Bergner" <bergner@vnet.ibm.com> wrote:
>>>
>>> On Wed, 2012-11-21 at 20:22 +0400, Konstantin Serebryany wrote:
>>> > On Wed, Nov 21, 2012 at 8:16 PM, Peter Bergner <bergner@vnet.ibm.com>
>>> > wrote:
>>> > > On Wed, 2012-11-21 at 13:46 +0400, Evgeniy Stepanov wrote:
>>> > >> Matching FP or SP also sounds good, and perhaps more reliable than
>>> > >> just popping 2 frames from the top of the stack.
>>> > >
>>> > > Agreed.  Can you try my second patch that searches for the frame
>>> > > address we want our backtrace to start with and see if that works
>>> > > for ARM?  The nice thing about that patch is that we won't have
>>> > > to play any games with forcing or disabling inlining of any of
>>> > > the ASAN functions which me might have to do if we always pop
>>> > > 2 frames off the stack.  It would also be tolerant of adding
>>> > > any number of new function calls in between the current two
>>> > > ASAN function at the top of the backtrace.
>>> > >
>>> > > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01711.html
>>> > >
>>> > > Bah, ignore that first diff of the LAST_UPDATED file. :(
>>> >
>>> > I'd actually prefer to keep the current code that pops two frames
>>> > (if it works for you)
>>>
>>> Well it does work for me, since I wrote it. :)  That being said, the
>>> change where I always pop two frames off of the backtrace was more of
>>> a proof of concept that if I can remove those ASAN functions from the
>>> backtrace, do we pass the test case in the testsuite.  It did, but I
>>> have to admit that code is extremely fragile.  It is dependent not
>>> only on the inlining heuristics of one compiler, but of two compilers!
>>> Not to mention people building debugable compilers with -O0 -fno-inline,
>>> etc. etc.  We'd also have to make sure no one in the future adds any
>>> ASAN function calls in between the report function and the GetBackTrace
>>> calls.  It just seems like there are so many things that could go wrong,
>>> that something is bound to.
>>>
>>>
>>> > Evgeniy seems to know how to fix the ARM case.
>>>
>>> His fix was to do:
>>>
>>>  void StackTrace::PopStackFrames(uptr count) {
>>> -  CHECK(size > count);
>>> +  CHECK(size >= count);
>>>    size -= count;
>>>    for (uptr i = 0; i < size; i++) {
>>>      trace[i] = trace[i + count];
>>>
>>> Basically, that is allowing for us to pop off all of the frames from
>>> the backtrace leaving an empty backtrace.  That can't be helpful in
>>> tracking down the address violation, can it?  With my patch above,
>>> either we find the frame we want to start our backtrace with, or
>>> it returns the entire backtrace, ASAN functions and all.  Isn't that
>>> better from a diagnostic point of view?
>>>
>>> That being said, I'd still like to hear from Evgeniy whether my
>>> patch above helps ARM or not.
>>>
>>> Peter
>>>
>>>
>>>
>>

Patch

diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/LAST_UPDATED gcc-fsf-mainline-asan/LAST_UPDATED
--- gcc-fsf-mainline-kcc/LAST_UPDATED	2012-11-20 11:40:17.232777673 -0600
+++ gcc-fsf-mainline-asan/LAST_UPDATED	2012-11-19 10:33:36.362778406 -0600
@@ -1,2 +1,2 @@ 
-Tue Nov 20 11:40:17 CST 2012
-Tue Nov 20 17:40:17 UTC 2012 (revision 193626)
+Mon Nov 19 10:33:36 CST 2012
+Mon Nov 19 16:33:36 UTC 2012 (revision 193626)
diff -urpN -X /home/bergner/cvs/dontdiff gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc
--- gcc-fsf-mainline-kcc/libsanitizer/asan/asan_linux.cc	2012-11-20 12:52:33.961664485 -0600
+++ gcc-fsf-mainline-asan/libsanitizer/asan/asan_linux.cc	2012-11-20 14:12:31.231751746 -0600
@@ -141,11 +141,27 @@  uptr Unwind_GetIP(struct _Unwind_Context
 #endif
 }
 
+uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
+  return _Unwind_GetCFA(ctx);
+}
+
+struct Unwind_Trace_Info {
+  StackTrace *stack;
+  uptr bp;
+};
+
 _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
     void *param) {
-  StackTrace *b = (StackTrace*)param;
-  CHECK(b->size < b->max_size);
+  Unwind_Trace_Info *p = (Unwind_Trace_Info *)param;
+  StackTrace *b = p->stack;
   uptr pc = Unwind_GetIP(ctx);
+  if (Unwind_GetBP(ctx) == p->bp) {
+    // We just encountered the frame pointer we want to start
+    // our backtrace with, so empty the backtrace before adding
+    // this frame to the backtrace.
+    b->size = 0;
+  }
+  CHECK(b->size < b->max_size);
   b->trace[b->size++] = pc;
   if (b->size == b->max_size) return UNWIND_STOP;
   return UNWIND_CONTINUE;
@@ -157,9 +173,10 @@  void GetStackTrace(StackTrace *stack, up
   if ((max_s) > 1) {
     stack->max_size = max_s;
 #if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
-    _Unwind_Backtrace(Unwind_Trace, stack);
-    // Pop off the two ASAN functions from the backtrace.
-    stack->PopStackFrames(2);
+    Unwind_Trace_Info param;
+    param.stack = stack;
+    param.bp = bp;
+    _Unwind_Backtrace(Unwind_Trace, &param);
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())