Allow StackwalkerARM to scan much farther to find caller of context frame

We're hitting a lot of crashes on ARM where the top frame is not in a known
function, so there's no unwind info. Unfortunately the caller frame is more than
120 bytes up the stack, so stack scanning fails to find it and we get a useless
crash signature. This patch allows StackwalkerARM to scan farther *only* to find
the caller of the context frame. This should help avoid getting bad frames
elsewhere in the stack while still fixing this case.

[jimb]

LGTM.

http://breakpad.appspot.com/501002/diff/1/src/processor/stackwalker_arm.cc
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/501002/diff/1/src/processor/stackwalker_arm.cc#ne...
src/processor/stackwalker_arm.cc:170: // farther down the stack.
I read this comment to mean, "When the entire stack is only one frame deep"; how
about "When searching for the caller of the youngest frame, ..."?

[Michael Krebs]

Coincidentally, I'm seeing a problem on x86 where a stack frame is 40 words big.
 It would be nice to increase kRASearchWords in general.  Is there a reason not
to?  I.e. How slow are the address lookups?

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

http://breakpad.appspot.com/501002/diff/1/src/processor/stackwalker.cc#newcode58
src/processor/stackwalker.cc:58: const int Stackwalker::kRASearchWords = 30;
Can we increase this number?  Maybe to 100?

[Ted Mielczarek]

On 2012/12/05 01:07:09, Michael Krebs wrote:
> Coincidentally, I'm seeing a problem on x86 where a stack frame is 40 words
big.
>  It would be nice to increase kRASearchWords in general.  Is there a reason
not
> to?  I.e. How slow are the address lookups?
> 
> http://breakpad.appspot.com/501002/diff/1/src/processor/stackwalker.cc
> File src/processor/stackwalker.cc (right):
> 
>
http://breakpad.appspot.com/501002/diff/1/src/processor/stackwalker.cc#newcode58
> src/processor/stackwalker.cc:58: const int Stackwalker::kRASearchWords = 30;
> Can we increase this number?  Maybe to 100?

This has been floated before. I don't think there's really much of a perf cost
(it's just looking through the block of stack memory). My biggest worry has been
that it will lead to more bogus stack frames being included in stack traces.
Perhaps this isn't a really valid concern and we should just let the scanner
look through all available memory? You will probably get some really screwed up
stacks in certain cases with that behavior, but only in cases where you might
otherwise not get frames at all, so maybe it's the right tradeoff.

[Michael Krebs]

On 2012/12/05 01:41:41, Ted Mielczarek wrote:
> This has been floated before. I don't think there's really much of a perf cost
> (it's just looking through the block of stack memory). My biggest worry has
been
> that it will lead to more bogus stack frames being included in stack traces.
> Perhaps this isn't a really valid concern and we should just let the scanner
> look through all available memory? You will probably get some really screwed
up
> stacks in certain cases with that behavior, but only in cases where you might
> otherwise not get frames at all, so maybe it's the right tradeoff.

That would be my thinking as well.  At least when I look at stack traces, I can
pretty quickly weed out what looks like bad stack frames (if I even look that
far down the trace), but it's painful if there aren't enough frames.

And I feel that with C++, at least, there can naturally be bigger stacks.

I just did a fairly unscientific experiment, where I ran minidump_stackwalk with
kRASearchWords set to 30 vs 10000.  With my test case, it took about 14-19 secs
with it set to 30, and took about 15-19 seconds with it set to 10000.  This was
basically just running it a dozen times each way, without much care to maintain
consistent machine load.  But it might still indicate a sub-5% increase in total
time.

Breakpad currently limits stack memory for each thread to 32k, which on a 32-bit
machine amounts to a max of 8k words.  So I'd be fine with letting it scan
through all available memory.