Let x86 stackwalker scan stack in cases where program evaluation fails

In Mozilla's crash reports, it's not uncommon to have crashes with a completely
bogus instruction pointer, called from code without a useful frame pointer. We
see this a lot from third-party code (plugins etc). Currently, if there's no
call frame info and dereferencing %ebp fails in program evaluation, the stack
walker gives up. This results in some really terrible stacks, like this one,
which simply shows "0x0" as the stack for the crashing thread:
http://crash-stats.mozilla.com/report/index/d6eddf4e-a71e-4efe-b796-924112090928

Jeff Muizelaar added some logic to the x86 stackwalker to scan the stack in
these cases, which gets us a more useful stack in this case. I added some other
logic (the InstructionAddressSeemsValid bit) which doesn't seem to help much
here, since we're generally in modules without symbols anyway. Jeff also added a
"trust" field to StackFrameX86, so we could identify how much the stack walker
is guessing in each case.

The stack produced in cases like these isn't perfect, but it gives us something,
as opposed to the nothing we currently have. You can follow our train of thought
here:
https://bugzilla.mozilla.org/show_bug.cgi?id=519616
Additionally, I ran minidump_stackwalk on a sampling of dumps from our
production crash reporting system, and diffed the output with/without this
patch. The output is available here:
http://people.mozilla.org/~tmielczarek/breakpad-stacks/

Mark, since you're responsible for the entire x86 stackwalker, could you take a
look at this?

[Mark Mentovai]

Some info is better than no info.  I like where this is headed.

Most of the comments are quick style fixes, but the last one might involve some
light refactoring.

http://breakpad.appspot.com/32003/diff/1/7
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/32003/diff/1/7#newcode74
Line 74: StackFrameX86() : context(), context_validity(CONTEXT_VALID_NONE),
Fix indentation.

http://breakpad.appspot.com/32003/diff/1/2
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/32003/diff/1/2#newcode195
Line 195: // not inside any loaded module
Wrap me in { }.

http://breakpad.appspot.com/32003/diff/1/2#newcode198
Line 198: StackFrame frame;
You don't need this until after the next check.  Try to keep variable
declarations closest to their first use.

http://breakpad.appspot.com/32003/diff/1/2#newcode203
Line 203: // we don't have a resolver and or symbol supplier,
Wrap me in { } too.

http://breakpad.appspot.com/32003/diff/1/2#newcode216
Line 216: // we don't have symbols, but we're inside a loaded module
{ } again

http://breakpad.appspot.com/32003/diff/1/3
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/32003/diff/1/3#newcode320
Line 320: // This seems like a reasonable return address. Since program string
Indent ought to be 2, not 4.

http://breakpad.appspot.com/32003/diff/1/3#newcode380
Line 380: // TODO(mmentovai): The return-address check can be made even
See my TODO here?  It seems like now that we have Instruction AddressSeemsValid,
we can resolve this one too.

Actually, since this whole section is so similar to the new code you're adding
above, it seems like there should be a way for the two to share.  Would that be
possible?

[Ted Mielczarek]

On 2009/10/08 13:33:58, mark wrote:
> http://breakpad.appspot.com/32003/diff/1/3#newcode380
> Line 380: // TODO(mmentovai): The return-address check can be made even
> See my TODO here?  It seems like now that we have Instruction
AddressSeemsValid,
> we can resolve this one too.
> 
> Actually, since this whole section is so similar to the new code you're adding
> above, it seems like there should be a way for the two to share.  Would that
be
> possible?

I factored those both out into a StackwalkerX86::ScanForReturnAddress method,
which resolves the TODO along the way. Fixed all the other nits and committed at
r409. Thanks for the review!