| Message ID | 1353442604.17833.131.camel@otta |
|---|---|
| State | New |
| Headers | show |
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~
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
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~
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~
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
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 > > >
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
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
>>
>>
>>
>
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 >>> >>> >>> >>
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, ¶m); #else if (!asan_inited) return; if (AsanThread *t = asanThreadRegistry().GetCurrent())