Re: [PATCH, RFC] Enable libsanitizer on powerpc{,64}

[Konstantin Serebryany <konstantin.s.serebryany@gmail.com>]

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
>>>
>>>
>>>
>>