Switch the Linux minidump writer to use MDCVInfoELF for CV data.

Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002dbuild_002did-292

The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/6c8f80aa8b3ba8120c4158c069bb298c044dedf9

[Lei Zhang, 2016-02-10 23:08:50]

I did not look at the previous MDCVInfoELF CL, so I'll let Mark have the final
say. Looks fine to me, with a couple nits:

https://codereview.chromium.org/1688743002/diff/1/src/client/linux/microdump_...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://codereview.chromium.org/1688743002/diff/1/src/client/linux/microdump_...
src/client/linux/microdump_writer/microdump_writer.cc:35: #include <algorithm>
nit: C headers before C++ headers.

https://codereview.chromium.org/1688743002/diff/1/src/common/linux/file_id.h
File src/common/linux/file_id.h (right):

https://codereview.chromium.org/1688743002/diff/1/src/common/linux/file_id.h#...
src/common/linux/file_id.h:46: const size_t kMaxBuildID = 20;
In src/client/linux/microdump_writer/microdump_writer.cc:346, we have:

uint8_t identifier_bytes[kMaxBuildID];
my_memcpy(&identifier_bytes, identifier, sizeof(MDGUID));

Can we static_assert(kMaxBuildID >= sizeof(MDGUID), "kMaxBuildID too small") ?

[Mark Mentovai, 2016-02-19 19:15:29]

> the maximum build id length supported is 20
> bytes, with longer values being truncated to 20 bytes.

I don’t think that this is wise. I think that a change to a longer hash or some
other format is just a couple of keystrokes away, and we’d have no indication
that we were losing anything.

Do we really need to allocate anything for this? Isn’t the data already mapped
contiguously? Can’t we just point to that?

[Ted Mielczarek, 2016-02-19 20:22:12]

On 2016/02/19 19:15:29, Mark Mentovai wrote:
> > the maximum build id length supported is 20
> > bytes, with longer values being truncated to 20 bytes.
> 
> I don’t think that this is wise. I think that a change to a longer hash or
some
> other format is just a couple of keystrokes away, and we’d have no indication
> that we were losing anything.

Understood. This does kind of suck.

> Do we really need to allocate anything for this? Isn’t the data already mapped
> contiguously? Can’t we just point to that?

Unfortunately, no, the ELF headers aren't fully mapped by the dynamic linker,
since the build ID is in a NOTE segment, so we mmap the files to generate the
ID:
https://chromium.googlesource.com/breakpad/breakpad/+/5ecc80d579e356193fcd302...

Now, that being said, the minidump writing code does have the ability to write
arbitrary-length data by way of UntypedMDRVA:
https://chromium.googlesource.com/breakpad/breakpad/+/5ecc80d579e356193fcd302...

...but right now the code as written makes it hard to do that usefully, because
the "get an identifier" code lives in LinuxDumper, which doesn't touch the
UntypedMDRVA types.

I could change this to return false if the build id is longer than the max
buffer, and return the necessary length in the `signature_length` outparam,
similar to how many Win32 APIs work.

If we did that the code would be slightly more complicated, since we'd maybe
have to call `UntypedMDRVA::Allocate` with a guess as to the size, and if we
need more space throw that UntypedMDRVA away and allocate a new one of the
correct size. We'd also have to handle the case where we allocated more bytes
than we needed, since MDCVInfoELF is currently just a signature + variable
length data. We could put a length member in there before the variable-length
data, so that we could allocate the UntypedMDRVA to be larger than we need and
just write the actual length in once we know it.

This all does make me wish the dynamic linker just had the stupid build id
mapped into memory so we could just copy it directly from there.

[Ted Mielczarek, 2016-04-01 17:07:28]

Okay, I came back around to this and fixed it to handle arbitrarily-long build
ids. The majority of the changes here are two things:
1) Change the methods on FileID to return the identifier in a
wasteful_vector<uint8_t> instead of a raw uint8_t*. This makes it so the FileID
code can size the buffer appropriately, and we don't have to pass in an explicit
buffer size and have some awkward API for "the buffer was not long enough".
2) Because I wasn't happy about making the most-likely case for each loaded
module have to use the PageAllocator, which would increase the chances of dump
writing failing in low or fragmented memory situations, I added an
`auto_wasteful_vector` class with a stack buffer and tweaked the PageAllocator
to be able to use the stack buffer if possible. These changes are nice-to-have
but not required. If you're not a fan I could leave them out.

Additionally, since I was already changing all the callers anyway, I renamed
FileID::ConvertIdentifierToString to FileID::ConvertIdentifierToUUIDString, made
it return the ID as a std::string, and changed it to not put dashes in the
output, since all the callers were post-processing the output string to remove
them. (That has bothered me for a while, honestly.)

The actual changes I *wanted* to make in minidump_writer.cc are pretty trivial
on top of all that.

I did realize that this makes one functional change to the output--previously if
a mapping didn't generate a file id (because it was not ELF, say) we would write
a CV record with a zeroed out GUID, so you'd get a debug_identifier that was 33
zeroes. With this change we'll omit the CV record entirely, so you'd get an
empty debug_identifier. I think this is OK and sensible, I just wanted to call
it out.

[Ted Mielczarek, 2016-04-01 17:16:37]

Description was changed from

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The bulk of the changes here are to change the signatures of the
FileID methods to increase the maximum size of the identifier, and add
an outparam to return the actual length of the identifier.

For convenience (to avoid having to allocate variable-sized buffers in
the minidump writer) the maximum build id length supported is 20
bytes, with longer values being truncated to 20 bytes. The default
build id produced by GNU binutils' ld is a 160-bit SHA-1 hash of some
parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=
==========

to

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...
    
The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=
==========

[Mark Mentovai, 2016-04-05 00:19:35]

Ted Mielczarek wrote:
> Okay, I came back around to this and fixed it to handle arbitrarily-long build
> ids. The majority of the changes here are two things:
> 1) Change the methods on FileID to return the identifier in a
> wasteful_vector<uint8_t> instead of a raw uint8_t*. This makes it so the
FileID
> code can size the buffer appropriately, and we don't have to pass in an
explicit
> buffer size and have some awkward API for "the buffer was not long enough".
> 2) Because I wasn't happy about making the most-likely case for each loaded
> module have to use the PageAllocator, which would increase the chances of dump
> writing failing in low or fragmented memory situations, I added an
> `auto_wasteful_vector` class with a stack buffer and tweaked the PageAllocator
> to be able to use the stack buffer if possible. These changes are nice-to-have
> but not required. If you're not a fan I could leave them out.
> 
> Additionally, since I was already changing all the callers anyway, I renamed
> FileID::ConvertIdentifierToString to FileID::ConvertIdentifierToUUIDString,
made
> it return the ID as a std::string, and changed it to not put dashes in the
> output, since all the callers were post-processing the output string to remove
> them. (That has bothered me for a while, honestly.)
> 
> The actual changes I *wanted* to make in minidump_writer.cc are pretty trivial
> on top of all that.
> 
> I did realize that this makes one functional change to the output--previously
if
> a mapping didn't generate a file id (because it was not ELF, say) we would
write
> a CV record with a zeroed out GUID, so you'd get a debug_identifier that was
33
> zeroes. With this change we'll omit the CV record entirely, so you'd get an
> empty debug_identifier. I think this is OK and sensible, I just wanted to call
> it out.

I thought that at some point in the past, if no debuglink was present in a
module, we’d hash it or something. If we don’t do that now, then the omitted
info rather than zeroes sounds fine. The processor should be resilient about
that.

LGTM. Since the generated minidump format is changing and people’s processors
will need to be updated to deal with this format, you may want to send a note to
the list calling the change out.

[Ted Mielczarek, 2016-04-05 01:35:27]

On 2016/04/05 00:19:35, Mark Mentovai wrote:
> I thought that at some point in the past, if no debuglink was present in a
> module, we’d hash it or something. If we don’t do that now, then the omitted
> info rather than zeroes sounds fine. The processor should be resilient about
> that.

We still do that, yes, but I'm talking about things like mmap'ed data files. To
pick an arbitrary crash report from my Linux Firefox from recently:
https://crash-stats.mozilla.com/report/index/a998eb6c-2606-4e63-9a1b-082a4216...

If you look under "Modules" you can see all sorts of nonsense like font files.
Those currently have all-zero debug IDs, they'll wind up having empty debug IDs
after this patch. Shouldn't make a difference either way.

> LGTM. Since the generated minidump format is changing and people’s processors
> will need to be updated to deal with this format, you may want to send a note
to
> the list calling the change out.

Yeah, I actually need to get our stackwalker updated, so I will make sure to do
that.

Thanks!

[Ted Mielczarek, 2016-04-05 13:34:24]

Description was changed from

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...
    
The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=
==========

to

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...

The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/6c8f80aa8b3ba8120c4158c...
==========

[Ted Mielczarek, 2016-04-05 13:34:27]

Committed patchset #2 (id:20001) manually as
6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).

[David Yen, 2016-04-08 19:02:07]

On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> Committed patchset #2 (id:20001) manually as
> 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).

This patch adds a new test which is failing: MinidumpWriterTest.BuildIDLong. Can
someone take a look at this?

[Ted Mielczarek, 2016-04-08 23:33:15]

On 2016/04/08 19:02:07, David Yen wrote:
> On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > Committed patchset #2 (id:20001) manually as
> > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> 
> This patch adds a new test which is failing: MinidumpWriterTest.BuildIDLong.
Can
> someone take a look at this?

This passes on my local machine (and on travis). Are you building with the
automake build wherever this is failing? If not, it's probably just that it
expects the executable to have been linked with the explicit --build-id like in
the Makefile:
https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533

[David Yen, 2016-04-11 17:23:09]

On 2016/04/08 23:33:15, Ted Mielczarek wrote:
> On 2016/04/08 19:02:07, David Yen wrote:
> > On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > > Committed patchset #2 (id:20001) manually as
> > > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> > 
> > This patch adds a new test which is failing: MinidumpWriterTest.BuildIDLong.
> Can
> > someone take a look at this?
> 
> This passes on my local machine (and on travis). Are you building with the
> automake build wherever this is failing? If not, it's probably just that it
> expects the executable to have been linked with the explicit --build-id like
in
> the Makefile:
> https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533

Ah that was it, I added this to the chromium build scripts.

[David Yen, 2016-04-11 17:23:27]

Description was changed from

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...

The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/6c8f80aa8b3ba8120c4158c...
==========

to

==========
Switch the Linux minidump writer to use MDCVInfoELF for CV data.

This preserves full build ids in minidumps, which are useful for
tracking down the right version of system libraries from Linux
distributions.

The default build id produced by GNU binutils' ld is a 160-bit SHA-1
hash of some parts of the binary, which is exactly 20 bytes:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...

The bulk of the changes here are to change the signatures of the
FileID methods to use a wasteful_vector instead of raw pointers, since
build ids can be of arbitrary length.

The previous change that added support for this in the processor code
preserved the return value of `Minidump::debug_identifier()` as the
current `GUID+age` treatment for backwards-compatibility, and exposed
the full build id from `Minidump::code_identifier()`, which was
previously stubbed out for Linux dumps. This change keeps the debug ID
in the `dump_syms` output the same to match.

R=mark@chromium.org, thestig@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/6c8f80aa8b3ba8120c4158c...
==========

[David Yen, 2016-04-11 20:06:03]

On 2016/04/11 17:23:09, David Yen wrote:
> On 2016/04/08 23:33:15, Ted Mielczarek wrote:
> > On 2016/04/08 19:02:07, David Yen wrote:
> > > On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > > > Committed patchset #2 (id:20001) manually as
> > > > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> > > 
> > > This patch adds a new test which is failing:
MinidumpWriterTest.BuildIDLong.
> > Can
> > > someone take a look at this?
> > 
> > This passes on my local machine (and on travis). Are you building with the
> > automake build wherever this is failing? If not, it's probably just that it
> > expects the executable to have been linked with the explicit --build-id like
> in
> > the Makefile:
> > https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533
> 
> Ah that was it, I added this to the chromium build scripts.

Adding that fixed it for Linux, but not for Android. Is that suppose to work for
Android as well?

[Ted Mielczarek, 2016-04-11 20:21:36]

On 2016/04/11 20:06:03, David Yen wrote:
> On 2016/04/11 17:23:09, David Yen wrote:
> > On 2016/04/08 23:33:15, Ted Mielczarek wrote:
> > > On 2016/04/08 19:02:07, David Yen wrote:
> > > > On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > > > > Committed patchset #2 (id:20001) manually as
> > > > > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> > > > 
> > > > This patch adds a new test which is failing:
> MinidumpWriterTest.BuildIDLong.
> > > Can
> > > > someone take a look at this?
> > > 
> > > This passes on my local machine (and on travis). Are you building with the
> > > automake build wherever this is failing? If not, it's probably just that
it
> > > expects the executable to have been linked with the explicit --build-id
like
> > in
> > > the Makefile:
> > >
https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533
> > 
> > Ah that was it, I added this to the chromium build scripts.
> 
> Adding that fixed it for Linux, but not for Android. Is that suppose to work
for
> Android as well?


I did not test on Android. Theoretically the test should work there but it's
plausible that something in the build configuration is not set up properly. If
it's not obvious how to fix it I'd be fine with wrapping the test with an
Android ifdef.

[Ted Mielczarek, 2016-04-11 20:23:34]

On 2016/04/11 20:06:03, David Yen wrote:
> On 2016/04/11 17:23:09, David Yen wrote:
> > On 2016/04/08 23:33:15, Ted Mielczarek wrote:
> > > On 2016/04/08 19:02:07, David Yen wrote:
> > > > On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > > > > Committed patchset #2 (id:20001) manually as
> > > > > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> > > > 
> > > > This patch adds a new test which is failing:
> MinidumpWriterTest.BuildIDLong.
> > > Can
> > > > someone take a look at this?
> > > 
> > > This passes on my local machine (and on travis). Are you building with the
> > > automake build wherever this is failing? If not, it's probably just that
it
> > > expects the executable to have been linked with the explicit --build-id
like
> > in
> > > the Makefile:
> > >
https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533
> > 
> > Ah that was it, I added this to the chromium build scripts.
> 
> Adding that fixed it for Linux, but not for Android. Is that suppose to work
for
> Android as well?


I did not test on Android. Theoretically the test should work there but it's
plausible that something in the build configuration is not set up properly. If
it's not obvious how to fix it I'd be fine with wrapping the test with an
Android ifdef.

[David Yen, 2016-04-11 20:38:20]

On 2016/04/11 20:23:34, Ted Mielczarek wrote:
> On 2016/04/11 20:06:03, David Yen wrote:
> > On 2016/04/11 17:23:09, David Yen wrote:
> > > On 2016/04/08 23:33:15, Ted Mielczarek wrote:
> > > > On 2016/04/08 19:02:07, David Yen wrote:
> > > > > On 2016/04/05 13:34:27, Ted Mielczarek wrote:
> > > > > > Committed patchset #2 (id:20001) manually as
> > > > > > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
> > > > > 
> > > > > This patch adds a new test which is failing:
> > MinidumpWriterTest.BuildIDLong.
> > > > Can
> > > > > someone take a look at this?
> > > > 
> > > > This passes on my local machine (and on travis). Are you building with
the
> > > > automake build wherever this is failing? If not, it's probably just that
> it
> > > > expects the executable to have been linked with the explicit --build-id
> like
> > > in
> > > > the Makefile:
> > > >
> https://chromium.googlesource.com/breakpad/breakpad/+/master/Makefile.am#533
> > > 
> > > Ah that was it, I added this to the chromium build scripts.
> > 
> > Adding that fixed it for Linux, but not for Android. Is that suppose to work
> for
> > Android as well?
> 
> 
> I did not test on Android. Theoretically the test should work there but it's
> plausible that something in the build configuration is not set up properly. If
> it's not obvious how to fix it I'd be fine with wrapping the test with an
> Android ifdef.

It turns out this was an issue with GN in the android configuration. We always
set the build-id to sha1 so I had to hack in a fix to override this behavior. I
think I got it working now thanks!