Stackwalker Fix for AMD64

Certain functions may be missing unwind hints in the symbol files.  In the
general case, functions tend to push their caller's %rbp onto the stack
immediately upon entry.  As an exception, some functions may not push a stack
frame with their caller's %rbp.  This is problematic particularly on OS X where
a number of common functions in libSystem.B.dylib (on 10.6.8 at least) have this
exact behavior, such as mach_msg_trap.  Encountering these functions in Breakpad
dumps would cause the stackwalker to create a bogus backtrace.

This patch, as proposed by Jim Blandy, check first to see if there is a
plausible %rbp on the stack directly below the hypothetical return address.  If
not, we check to see if the previous frame's %rbp could apply to the next
frame's.  If it's still a valid %rbp (ie, %rbp > next %rsp), then we mark the
next frame's %rbp as valid and set it equal to the previous frame's %rbp.

I've updated the unit tests some and added a test specifically to exercise the
%rbp-on-the-stack heuristic.

[jimb]

Thanks! I've made some minor revisions and committed this as r960.

If it looks okay, please mark this issue as closed.

http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
File src/processor/stackwalker_amd64_unittest.cc (right):

http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
src/processor/stackwalker_amd64_unittest.cc:185: Label frame1_sp, frame2_sp,
frame1_bp;
Here and elsewhere, let's call this rbp, consistently. THe only widely used
abbreviations for this are "fp" ("frame pointer") or "rbp" (the architecture's
full name for this register).

http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
src/processor/stackwalker_amd64_unittest.cc:194: .Mark(&frame1_bp)
It's a bit strange to have frame 1's base pointer pointing at the same place as
frame 1's sp. In almost any function that bothers to set up a frame pointer, the
frame pointer points at the higher-addressed end of the stack frame --- more
precisely, at the word pushed after the return address.

I've changed this so that it points to a more realistic place, and ensured that
it still points to a word that should not be treated as a saved RBP.

http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
src/processor/stackwalker_amd64_unittest.cc:296:
StackFrameAMD64::CONTEXT_VALID_RBP),
We're not supposed to use tabs in sources; I've fixed this, and the other tabs
in similar existing code.

http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
src/processor/stackwalker_amd64_unittest.cc:312: u_int64_t stack_end =
stack_section.start().Value() + 6*16;
The idea of labels is that you shouldn't have to compute this sort of thing this
way; you can D64(frame1_bp) before frame1_bp has been defined, and then
Mark(&frame1_bp) later, and everything will get computed and filled in
appropriately.

I've changed the test to work this way.

[mrmiller]

Sounds good to me!  Closed.

On 2012/04/25 21:12:12, jimb wrote:
> Thanks! I've made some minor revisions and committed this as r960.
> 
> If it looks okay, please mark this issue as closed.
> 
>
http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
> File src/processor/stackwalker_amd64_unittest.cc (right):
> 
>
http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
> src/processor/stackwalker_amd64_unittest.cc:185: Label frame1_sp, frame2_sp,
> frame1_bp;
> Here and elsewhere, let's call this rbp, consistently. THe only widely used
> abbreviations for this are "fp" ("frame pointer") or "rbp" (the architecture's
> full name for this register).
> 
>
http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
> src/processor/stackwalker_amd64_unittest.cc:194: .Mark(&frame1_bp)
> It's a bit strange to have frame 1's base pointer pointing at the same place
as
> frame 1's sp. In almost any function that bothers to set up a frame pointer,
the
> frame pointer points at the higher-addressed end of the stack frame --- more
> precisely, at the word pushed after the return address.
> 
> I've changed this so that it points to a more realistic place, and ensured
that
> it still points to a word that should not be treated as a saved RBP.
> 
>
http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
> src/processor/stackwalker_amd64_unittest.cc:296:
> StackFrameAMD64::CONTEXT_VALID_RBP),
> We're not supposed to use tabs in sources; I've fixed this, and the other tabs
> in similar existing code.
> 
>
http://breakpad.appspot.com/375001/diff/1/src/processor/stackwalker_amd64_uni...
> src/processor/stackwalker_amd64_unittest.cc:312: u_int64_t stack_end =
> stack_section.start().Value() + 6*16;
> The idea of labels is that you shouldn't have to compute this sort of thing
this
> way; you can D64(frame1_bp) before frame1_bp has been defined, and then
> Mark(&frame1_bp) later, and everything will get computed and filled in
> appropriately.
> 
> I've changed the test to work this way.