ESP is zero in dumps created by CrashGenerationClient::RequestDump on i386 Linux

If a crash dump is created by calling CrashGenerationClient::RequestDump then
getcontext() is called and on Linux this sets gregs[REG_ESP] to zero. Once the
stack pointer is retrieved from gregs[REG_UESP] another problem is hit because
the local variables in ExceptionHandler::WriteMinidump() consume more space than
the 32 KB of stack saved.

Fixing these can be done by setting gregs[REG_ESP] to gregs[REG_EBP] in order to
get a good stack value that steps past the current function's local variables.

A fix is available and has been in use at Valve for a few months now. The
problem and fix are only known to exist/work on i386 so the code is restricted
to that architecture.

[bruce.dawson]

I finally got around to creating an issue/patch for this bug. We've been running
a local fix for a few months now and it has been extremely helpful.

[Ted Mielczarek]

This LGTM with a few nits that I'll fix on checkin. Sorry this took so long, I
kept forgetting about it.

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
src/client/linux/handler/exception_handler.cc:555: // is supported on GCC but
not on clang.
nit: We try to stay away from first-person in comments.

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
src/client/linux/handler/exception_handler.cc:557:
__attribute__((optimize("no-omit-frame-pointer")))
It seems unfortunate that this won't work in clang, but it's not actually any
worse than the existing scenario...

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
src/client/linux/handler/exception_handler.cc:586: if (
!context.context.uc_mcontext.gregs[REG_UESP] )
nit: no spaces inside the if conditional to match local style here, open bracket
goes on the same line as the if.

[Ted Mielczarek]

Pushed as r1149:
http://code.google.com/p/google-breakpad/source/detail?r=1149

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception...
src/client/linux/handler/exception_handler.cc:582: // In CPUFillFromUContext in
minidumpwriter.cc the stack pointer is retried from REG_UESP instead of from
One other nit: we still use 80-character line lengths.

[bruce.dawson]

Thanks Ted.

BTW, I found an old comment from you (on one of my altdevblogaday posts)
where you suggested some good ways of catching more crashes (pure-call,
etc.) Unfortunately the HANDLER_ALL suggestion doesn't work for us because
that sets the pure-call and invalid-parameter handlers for the current
version of the CRT. For most programs that is good enough but we statically
link the CRT into each DLL. Weird, I know, but it means that we have to
call those functions once per DLL. Oh well, we've done it, and it works.

You also had some good thoughts on handling /GS and direct calls to
_invoke_watson. And hashing of symbols to get them from graphics
vendors. I'm going to try implementing some of those ideas.


On Tue, Apr 16, 2013 at 10:42 AM, <ted.mielczarek@gmail.com> wrote:

> This LGTM with a few nits that I'll fix on checkin. Sorry this took so
> long, I kept forgetting about it.
>
>
>
> https://breakpad.appspot.com/**533002/diff/1/src/client/**
>
linux/handler/exception_**handler.cc<https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception_handler.cc>
> File src/client/linux/handler/**exception_handler.cc (right):
>
> https://breakpad.appspot.com/**533002/diff/1/src/client/**
>
linux/handler/exception_**handler.cc#newcode555<https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception_handler.cc#newcode555>
> src/client/linux/handler/**exception_handler.cc:555: // is supported on
> GCC but not on clang.
> nit: We try to stay away from first-person in comments.
>
> https://breakpad.appspot.com/**533002/diff/1/src/client/**
>
linux/handler/exception_**handler.cc#newcode557<https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception_handler.cc#newcode557>
> src/client/linux/handler/**exception_handler.cc:557:
> __attribute__((optimize("no-**omit-frame-pointer")))
> It seems unfortunate that this won't work in clang, but it's not
> actually any worse than the existing scenario...
>
> https://breakpad.appspot.com/**533002/diff/1/src/client/**
>
linux/handler/exception_**handler.cc#newcode586<https://breakpad.appspot.com/533002/diff/1/src/client/linux/handler/exception_handler.cc#newcode586>
> src/client/linux/handler/**exception_handler.cc:586: if (
> !context.context.uc_mcontext.**gregs[REG_UESP] )
> nit: no spaces inside the if conditional to match local style here, open
> bracket goes on the same line as the if.
>
> https://breakpad.appspot.com/**533002/<https://breakpad.appspot.com/533002/>
>