Support generating minidumps from live processes on Linux

This patch adds two new things to the Linux ExceptionHandler:
1) The ability to write an exception record in on-demand minidumps written using
ExceptionHandler::WriteMinidump.
2) An ExceptionHandler::WriteMinidumpForChild method that can write a useful
minidump for a child process.

Originally filed as https://breakpad.appspot.com/123001/

[digit]

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:567: #elif defined(__ARMEL__)
why is this __ARMEL__ instead of __arm__?

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:569:
reinterpret_cast<void*>(context.context.uc_mcontext.arm_ip);
I'm sure you meant arm_pc here :)

[Ted Mielczarek]

If you'd like to review the rest of this patch feel free. I just spent a full
day rebasing it and I'd desperately like to get it landed.

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:567: #elif defined(__ARMEL__)
On 2012/09/12 15:28:01, digit wrote:
> why is this __ARMEL__ instead of __arm__?

Not sure, I didn't actually write this patch, I just rebased it to current
trunk. This part applied pretty cleanly so I didn't think about it much.

http://breakpad.appspot.com/449003/diff/1/src/client/linux/handler/exception_...
src/client/linux/handler/exception_handler.cc:569:
reinterpret_cast<void*>(context.context.uc_mcontext.arm_ip);
On 2012/09/12 15:28:01, digit wrote:
> I'm sure you meant arm_pc here :)

*sigh* thanks! I actually fixed another instance of this in minidump_writer.cc
in this patch, but I missed this one.

[Ben Chan]

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:38: pid_t pid() const { return
pid_; }
with the addition of this getter, should ClientInfo be a class and pid_ be
private?

[Ted Mielczarek]

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:38: pid_t pid() const { return
pid_; }
On 2012/09/12 16:25:49, Ben Chan wrote:
> with the addition of this getter, should ClientInfo be a class and pid_ be
> private?

Probably. I suspect this change was just to bring this in line with the Mac and
Windows versions of this struct.

[Mark Mentovai]

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:1: // Copyright (c) 2010 Google
Inc.
The change description is not a change description, it’s just a message to
reviewers. Please fix that.

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.cc:616: void* callback_context)
Use Google style: { on this line.

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
File src/client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.h:175: void* callback_context);
Four versions of WriteMinidump? That’s getting out of hand. What’s the bool
argument about, and why isn’t what it controls just exposed by
WriteMinidumpWithArtificalExceptionStream instead of a bool overload? Or,
alternatively, why isn’t the bool retained in all cases and the other callers
adjusted?

[Ted Mielczarek]

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
File src/client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/449003/diff/6001/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.h:175: void* callback_context);
On 2012/09/12 19:04:44, Mark Mentovai wrote:
> Four versions of WriteMinidump? That’s getting out of hand. What’s the bool
> argument about, and why isn’t what it controls just exposed by
> WriteMinidumpWithArtificalExceptionStream instead of a bool overload? Or,
> alternatively, why isn’t the bool retained in all cases and the other callers
> adjusted?

Agreed. After updating the Windows patch I noted that you made the default
behavior of WriteMinidump on Windows be to add an exception stream. I think we
should probably just do the same thing here, and make the new behavior here the
default.

[Ted Mielczarek]

This patch addresses Ben's comment as well as removes the proliferation of
ExceptionHandler::WriteMinidump methods by making the default WriteMinidump
behavior be to add an exception stream (like Windows).

[digit]

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:37: class ClientInfo {
shouldn't you use public: below, since the default visibility of C++ members is
private anyway?

[Ted Mielczarek]

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:37: class ClientInfo {
On 2012/09/14 11:55:47, digit wrote:
> shouldn't you use public: below, since the default visibility of C++ members
is
> private anyway?

Yeah, I totally screwed that up and realized it when I compiled. Fixed locally.
Juggling too many patches here.

[Mark Mentovai]

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
File src/client/linux/crash_generation/client_info.h (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/crash_generatio...
src/client/linux/crash_generation/client_info.h:37: class ClientInfo {
On 2012/09/14 12:02:36, Ted Mielczarek wrote:
> On 2012/09/14 11:55:47, digit wrote:
> > shouldn't you use public: below, since the default visibility of C++ members
> is
> > private anyway?
> 
> Yeah, I totally screwed that up and realized it when I compiled. Fixed
locally.
> Juggling too many patches here.

“Fixed locally” doesn’t really help a reviewer. Another upload is cheap.

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.cc:549: #if defined(__i386)
__i386__

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.cc:552: #elif defined(__x86_64)
__x86_64__

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.cc:604: {
Use Google style, putting this on the preceding line.

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
File src/client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/handler/excepti...
src/client/linux/handler/exception_handler.h:172: // WARNING: the return of this
function *must* be ordered
“must be ordered happens-before” could benefit from a reduction in nonsense
factor.

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1460:
dumper.set_crash_signal(SIGSTOP);
Some sort of magic number? Seems confusing. 0 or some other nonsignal value
would be better.

[Ted Mielczarek]

Thanks for the review. Just one question, I'll fix the rest in a new patch
tomorrow.

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1460:
dumper.set_crash_signal(SIGSTOP);
On 2012/09/17 21:17:38, Mark Mentovai wrote:
> Some sort of magic number? Seems confusing. 0 or some other nonsignal value
> would be better.

Is this any different than the Windows code using
STATUS_NONCONTINUABLE_EXCEPTION?
http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/windo...

I suppose we could make up a matching set of "dump requested" exception codes if
that would be cleaner.

[Mark Mentovai]

The only difference is that I can catch things like this written to a
POSIXish API by eye without having to look at some reference on the Windows
side.

I think that a 0 or -1 type of value here would be fine.


On Mon, Sep 17, 2012 at 8:53 PM, <ted.mielczarek@gmail.com> wrote:

> Thanks for the review. Just one question, I'll fix the rest in a new
> patch tomorrow.
>
>
>
> http://breakpad.appspot.com/**449003/diff/5003/src/client/**
>
linux/minidump_writer/**minidump_writer.cc<http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer/minidump_writer.cc>
> File src/client/linux/minidump_**writer/minidump_writer.cc (right):
>
> http://breakpad.appspot.com/**449003/diff/5003/src/client/**
>
linux/minidump_writer/**minidump_writer.cc#newcode1460<http://breakpad.appspot.com/449003/diff/5003/src/client/linux/minidump_writer/minidump_writer.cc#newcode1460>
> src/client/linux/minidump_**writer/minidump_writer.cc:**1460:
> dumper.set_crash_signal(**SIGSTOP);
> On 2012/09/17 21:17:38, Mark Mentovai wrote:
>
>> Some sort of magic number? Seems confusing. 0 or some other nonsignal
>>
> value
>
>> would be better.
>>
>
> Is this any different than the Windows code using
> STATUS_NONCONTINUABLE_**EXCEPTION?
> http://code.google.com/p/**google-breakpad/source/browse/**
>
trunk/src/client/windows/**handler/exception_handler.cc#**708<http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/windows/handler/exception_handler.cc#708>
>
> I suppose we could make up a matching set of "dump requested" exception
> codes if that would be cleaner.
>
> http://breakpad.appspot.com/**449003/<http://breakpad.appspot.com/449003/>
>

[Ted Mielczarek]

On 2012/09/18 02:54:24, Mark Mentovai wrote:
> The only difference is that I can catch things like this written to a
> POSIXish API by eye without having to look at some reference on the Windows
> side.
> 
> I think that a 0 or -1 type of value here would be fine.

Okay, that's fine with me as long as we can stick it in the MDExceptionCodeLinux
enum and make MinidumpProcessor stringify it in a sane manner so that our crash
reports look sensible. I'll post a patch with that change in a bit.

[Mark Mentovai]

LGTM