More robust stack walks when EIP in the context frame is invalid (ot not in a known module).

More robust stack walks when the IP address in the context frame is invalid (or
not in a known module).

This is achieved by:
1. Extending the span of the scan for return address in the conext frame. 
Initially, I wanted to extend the span of the scan for all frames but then I
noticed that there is code for ARM already that is extending the search only for
the context frame.  This kind of makes sense so I decided to reuse the same idea
everywhere.
2. Attempting to restore the EBP chain after a successful scan for return
address so that the stackwalker can switch back to FRAME_TRUST_CFI for the rest
of the frames when possible.

I also fixed the lint errors in the files touched.
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1193

[Ivan Penkov]

.

[Mark Mentovai]

In the CL description, you wrote:

> This kind of makes sense so I decided to reuse the same idea
> everywhere.

How does it make sense? Why would you scan further in the context frame than in
any other?

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:55: // The default stack reservation size used
by the linker is 1 MB, so 2 MB
This whole explanation sounds very Windows-specific, but this stackwalker is not
Windows-specific.

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:58: // thread, however using a conservative
approximation should be sifficient for
sufficient

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:60: static const uint32_t
kMaxReasonableGapBetweenFrames = 2 << 20;  // 2 MB
I think 2MB is way too big. Real-life stack frames are much smaller than this.

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:558: // caller %ebp should be at a location
above the current %ebp location
Writing comments in the middle of code like this makes it harder to read the
code.

[Ivan Penkov]

Regarding the larger scans in the context frame.

We usually resort to scanning for a return address when we can't use the caller
information from the current frame.  This is quite typical for context frames
for crashes where EIP is bad or EBP/ESP are way off.  Once the first frame is
properly located, chances are we don't need to scan anymore (restored EBP chain)
or if we do, consequent frames will be closer to each other (hence shorter
scans).  However, if the first return address that is found by a scan is not
correct (some leftover execution residue), then it will make sense to do larger
scans for the consequent frames as well.  Larger scans can produce more frames
for broken stack walks but at the same time we risk to introduce too much noise.
 I guess an argument can be made both ways.  I'm open to simplify this and allow
larger scans for all frames

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:55: // The default stack reservation size used
by the linker is 1 MB, so 2 MB
On 2013/06/24 15:14:08, Mark Mentovai wrote:
> This whole explanation sounds very Windows-specific, but this stackwalker is
not
> Windows-specific.

Good catch.  I provided a more generic description and a better value which
should cover 99% of all frames.

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:58: // thread, however using a conservative
approximation should be sifficient for
On 2013/06/24 15:14:08, Mark Mentovai wrote:
> sufficient

Done.

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:60: static const uint32_t
kMaxReasonableGapBetweenFrames = 2 << 20;  // 2 MB
On 2013/06/24 15:14:08, Mark Mentovai wrote:
> I think 2MB is way too big. Real-life stack frames are much smaller than this.

Done.

https://breakpad.appspot.com/605002/diff/4001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:558: // caller %ebp should be at a location
above the current %ebp location
On 2013/06/24 15:14:08, Mark Mentovai wrote:
> Writing comments in the middle of code like this makes it harder to read the
> code.

Done.

[Mark Mentovai]

LGTM otherwise. I’m still not really sold on the “treat context frame
differently” aspect, but that should be harmless.

https://breakpad.appspot.com/605002/diff/9001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

https://breakpad.appspot.com/605002/diff/9001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:58: static const uint32_t
kMaxReasonableGapBetweenFrames = 128 << 10;  // 128 KB
Write 128 * 1024. Some people might recognize << 10 as * 1024, but not everyone
will. I’m one of the people who recognize << 10 for what it is, and I still
think it’s clearer to communicate intent (“kilo = 1024”) than to be cute. If a
left shift is the best way to implement multiplication by a power of two, the
compiler will generally emit a left shift, but since this is a static const,
there shouldn’t even be any machine code doing the multiplication at all.

[Ivan Penkov]

Done.

https://breakpad.appspot.com/605002/diff/9001/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

https://breakpad.appspot.com/605002/diff/9001/src/processor/stackwalker_x86.c...
src/processor/stackwalker_x86.cc:58: static const uint32_t
kMaxReasonableGapBetweenFrames = 128 << 10;  // 128 KB
On 2013/06/24 21:44:03, Mark Mentovai wrote:
> Write 128 * 1024. Some people might recognize << 10 as * 1024, but not
everyone
> will. I’m one of the people who recognize << 10 for what it is, and I still
> think it’s clearer to communicate intent (“kilo = 1024”) than to be cute. If a
> left shift is the best way to implement multiplication by a power of two, the
> compiler will generally emit a left shift, but since this is a static const,
> there shouldn’t even be any machine code doing the multiplication at all.

Done.