[PATCH] D9259: [PowerPC]Adapt fast stack unwinding to work for Power.

[Bill Schmidt]

wschmidt added a comment.

OK, I've been looking at a small example.  I've taken the portion of Bill Seurer's patch that modifies the offset to the return address in the stack frame, but omitted the part that pops extra stack frames, so that I can get a look at why we get a bogus extra frame.

In the example, we produce this:

freed by thread T0 here:

  #0 0x1010d988 in GetStackTraceWithPcBpAndContext /home/wschmidt/llvm/llvm-test2/projects/compiler-rt/lib/asan/asan_stack.h:50
  #1 0x1010d988 in operator delete(void*) /home/wschmidt/llvm/llvm-test2/projects/compiler-rt/lib/asan/asan_new_delete.cc:94
  #2 0x10110474 in main /home/wschmidt/seurer/tests/stack/crash-small.cpp:13:3
  #3 0x3fffb7ad4cfc in generic_start_main /build/buildd/eglibc-2.19/csu/../csu/libc-start.c:287
  #4 0x3fffb7ad4ef4 in __libc_start_main /build/buildd/eglibc-2.19/csu/../sysdeps/unix/sysv/linux/powerpc/libc-start.c:93

The issue is "frames" #0 and #1, which share an identical call address, but claim to be in different functions.

Let's start with a picture of the stack:

...callers of main...
0x3ffffffff410:  stack frame for main()
0x3fffffffeb40:  address of the BufferedStackTrace object created during operator delete(void*)
0x3fffffffeb20:  stack frame for operator delete(void*)

Looking through all the various macro definitions, this is the order of events I see:

- Inside operator delete(void*), we call StackTrace::GetCurrentPc(), which correctly returns the address following the call to StackTrace::GetCurrentPc().  This is 0x1010d910.  It also calls GET_CURRENT_FRAME(), which equates to __builtin_frame_address(0), to obtain the address of the stack frame for operator delete(void*), which is 0x3fffffffeb20.
- Operator delete(void*) then calls GetStackTraceWithPcBpAndContext().  This is inlined as expected, so no frame is stacked.  The callee is passed 0x1010d910 as "pc", and 0x3fffffffeb20 as "bp".
- GetStackTraceWithPcBpAndContext() calls stack->Unwind() on behalf of the uninitialized BufferedStackTrace object at 0x3fffffffeb40, passing the "pc" and "bp" values as above.  This has the effect of storing the address following the call to Unwind() into operator delete(void*)'s stack frame at offset 16, i.e., at 0x3fffffffeb30.  That address is 0x1010d98c.  Observe that both 0x1010d910 ("pc") and 0x1010d98c (the saved return address) are addresses within the text of operator delete(void*).
- BufferedStackTrace::Unwind() calls BufferedStackTrace::FastUnwindStack() with the same pc and bp parameters.  This code pulls "pc1" out of the frame "bp" at offset 16, which is 0x3fffffffeb30.  Thus pc1 = 0x1010d98c, whereas pc = 0x1010d910.  There is code within FastUnwindStack() to check whether pc and pc1 are identical, presumably to determine whether they are in the same stack frame.  This is a fallacious test, at least with the Power ABI.  Even though both pc and pc1 are addresses within operator delete(void*), this test fails to detect this, and thus both addresses are included in the stack frame to be reported.

The only remaining question is why the two stack frames name different functions.  This is simply a matter of the debug data attempting to track inlining.  This library code is optimized, and the statement number information thus bounces around quite a lot.  While stepping through the code with gdb, I saw gdb reporting that it was inside operator delete() for a few instructions, then that it was inside GetStackTraceWithPcBpAndContext() for a few instructions, and back and forth.  It just so happens that 0x1010d910 was associated with a statement number from the inlined function, so we got the different name.

My conclusion is that the code in BufferedStackTrace::FastUnwindStack() that compares pc with pc1 isn't doing what it intends to do.  I am frankly not entirely sure what it is intending to do.  If that can be fixed, then perhaps Bill Seurer's code to change the amount of frame popping is not necessary.  If it can't, then it seems to me to be as good a solution as any to avoid misreporting the same stack frame twice.


http://reviews.llvm.org/D9259