Change MDCVInfoELF into something usable.

Change MDCVInfoELF into something usable.

This patch changes MDCVInfoELF (which is currently unused, apparently
a vestigal bit of code landed as part of Solaris support) into a supported
CodeView format that simply contains a build id as raw bytes.

Modern ELF toolchains support build ids nicely:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Developer_Guide/compiling-build-id.html

It would be useful to have the original build ids of loaded modules in
Linux minidumps, since tools like Fedora's darkserver allow querying by build
id and the current Breakpad code truncates the build id to the size of a GUID,
which loses information:
https://darkserver.fedoraproject.org/

A follow-up patch will change the Linux minidump generation code to produce
MDCVInfoELF in minidumps instead of MDCVInfoPDB70. This patch should be landed
first to ensure that crash processors are able to handle this format before
dumps are generated containing it.

The full build id is exposed as the return value of Minidump::code_identifier(),
which currently just returns "id" for modules in Linux dumps. For
backwards-compatibility, Minidump::debug_identifier() continues to treat
the build id as a GUID, so debug identifiers for existing modules will not
change.

BUG=
R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/7a364b1262df4fe6f861033547f177f8dd17fa30

[Ted Mielczarek, 2016-02-08 20:13:27]

Description was changed from

==========
Change MDCVInfoELF into something usable.

This patch changes MDCVInfoELF (which is currently unused, apparently
a vestigal bit of code landed as part of Solaris support) into a supported
CodeView format that simply contains a build id as raw bytes.

Modern ELF toolchains support build ids nicely:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html...

It would be useful to have the original build ids of loaded modules in
Linux minidumps, since tools like Fedora's darkserver allow querying by build
id and the current Breakpad code truncates the build id to the size of a GUID,
which loses information:
https://darkserver.fedoraproject.org/

A follow-up patch will change the Linux minidump generation code to produce
MDCVInfoELF in minidumps instead of MDCVInfoPDB70. This patch should be landed
first to ensure that crash processors are able to handle this format before
dumps are generated containing it.

The full build id is exposed as the return value of Minidump::code_identifier(),
which currently just returns "id" for modules in Linux dumps. For
backwards-compatibility, Minidump::debug_identifier() continues to treat
the build id as a GUID, so debug identifiers for existing modules will not
change.

BUG=
==========

to

==========
Change MDCVInfoELF into something usable.

This patch changes MDCVInfoELF (which is currently unused, apparently
a vestigal bit of code landed as part of Solaris support) into a supported
CodeView format that simply contains a build id as raw bytes.

Modern ELF toolchains support build ids nicely:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html...

It would be useful to have the original build ids of loaded modules in
Linux minidumps, since tools like Fedora's darkserver allow querying by build
id and the current Breakpad code truncates the build id to the size of a GUID,
which loses information:
https://darkserver.fedoraproject.org/

A follow-up patch will change the Linux minidump generation code to produce
MDCVInfoELF in minidumps instead of MDCVInfoPDB70. This patch should be landed
first to ensure that crash processors are able to handle this format before
dumps are generated containing it.

The full build id is exposed as the return value of Minidump::code_identifier(),
which currently just returns "id" for modules in Linux dumps. For
backwards-compatibility, Minidump::debug_identifier() continues to treat
the build id as a GUID, so debug identifiers for existing modules will not
change.

BUG=
==========

[Ted Mielczarek, 2016-02-08 20:13:27]

ted.mielczarek@gmail.com changed reviewers:
+ mark@chromium.org

[Ted Mielczarek, 2016-02-08 20:14:55]

Here's an example dump produced from the unit tests containing MDCVInfoELF:
https://people.mozilla.org/~tmielczarek/linux-cvinfoelf.dmp

(I have the follow-up patch to change the Linux minidump writer code to use this
done already.)

[Mark Mentovai, 2016-02-09 15:55:04]

You mean we weren’t doing this all along? I thought we were. Oh boy.

https://codereview.chromium.org/1675413002/diff/1/src/google_breakpad/common/...
File src/google_breakpad/common/minidump_format.h (right):

https://codereview.chromium.org/1675413002/diff/1/src/google_breakpad/common/...
src/google_breakpad/common/minidump_format.h:460: uint8_t  build_id[1];  /*
bytes of build id */
How many?

[Ted Mielczarek, 2016-02-09 18:01:52]

I knew we were using the MDCVInfoPDB70 struct for Linux (because I had run into
this issue previously) but I didn't realize MDCVInfoELF was even defined until
just recently.

https://codereview.chromium.org/1675413002/diff/1/src/google_breakpad/common/...
File src/google_breakpad/common/minidump_format.h (right):

https://codereview.chromium.org/1675413002/diff/1/src/google_breakpad/common/...
src/google_breakpad/common/minidump_format.h:460: uint8_t  build_id[1];  /*
bytes of build id */
On 2016/02/09 15:55:04, Mark Mentovai wrote:
> How many?

There isn't really a spec for it, but it allows for an arbitrary number of
bytes. The binutils default is a 160-bit sha-1 hash of the rest of the binary,
so 20 bytes, but ld will let you use a uuid, md5, or an arbitrary hex string:
https://sourceware.org/binutils/docs-2.26/ld/Options.html#index-g_t_002d_002d...

Since the CV record is already variable-length, supporting arbitrary sizes here
isn't particularly hard. For the client-side patch I have to use this, I just
clamp it to a max of 20 bytes since that covers the common case and avoids
having to allocate memory in the minidump writer.

Should I just mention that it's variable-length, but usually 20 bytes?

[Ted Mielczarek, 2016-02-09 18:03:33]

Also, in fairness to us, the Build ID work didn't ship in Fedora until late
2007, so it's not like we missed something obvious when the initial Linux
Breakpad work happened.

[Mark Mentovai, 2016-02-09 18:52:54]

Yeah, could you say something else about the expected length? Or mention
GNU_BUILD_ID by name, maybe. If this isn’t supposed to follow a specific format
(UUID, hash algorithm, random number), it’s useful to know that. Breakpad always
treats these IDs as opaque already, but consumers need to know how to allocate
enough space to deal with them (sticking these IDs into a database, for
example).

[Ted Mielczarek, 2016-02-09 19:21:00]

On 2016/02/09 18:52:54, Mark Mentovai wrote:
> Yeah, could you say something else about the expected length? Or mention
> GNU_BUILD_ID by name, maybe. If this isn’t supposed to follow a specific
format
> (UUID, hash algorithm, random number), it’s useful to know that. Breakpad
always
> treats these IDs as opaque already, but consumers need to know how to allocate
> enough space to deal with them (sticking these IDs into a database, for
> example).

OK. There's more to this when I get to the next patch--I would upload it now if
I could figure out how to convince `git cl upload` to only upload that one
changeset and not also upload these changes as well. I have
`Minidump::debug_identifier` treating the bytes like a GUID to preserve
backwards-compatibility, so that we don't have to deal with having the same
libraries wind up with different debug IDs, and `Minidump::code_identifier`
(currently not useful for Linux dumps) returning the full debug ID.

[Mark Mentovai, 2016-02-09 19:27:05]

OK, fair enough. LGTM

https://codereview.chromium.org/1675413002/diff/1/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://codereview.chromium.org/1675413002/diff/1/src/processor/minidump.cc#n...
src/processor/minidump.cc:1867: char byte[3];
build_id[build_id_index] is the byte, this is a string with the byte’s value. Is
there a better name?

[Ted Mielczarek, 2016-02-10 14:00:04]

Description was changed from

==========
Change MDCVInfoELF into something usable.

This patch changes MDCVInfoELF (which is currently unused, apparently
a vestigal bit of code landed as part of Solaris support) into a supported
CodeView format that simply contains a build id as raw bytes.

Modern ELF toolchains support build ids nicely:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html...

It would be useful to have the original build ids of loaded modules in
Linux minidumps, since tools like Fedora's darkserver allow querying by build
id and the current Breakpad code truncates the build id to the size of a GUID,
which loses information:
https://darkserver.fedoraproject.org/

A follow-up patch will change the Linux minidump generation code to produce
MDCVInfoELF in minidumps instead of MDCVInfoPDB70. This patch should be landed
first to ensure that crash processors are able to handle this format before
dumps are generated containing it.

The full build id is exposed as the return value of Minidump::code_identifier(),
which currently just returns "id" for modules in Linux dumps. For
backwards-compatibility, Minidump::debug_identifier() continues to treat
the build id as a GUID, so debug identifiers for existing modules will not
change.

BUG=
==========

to

==========
Change MDCVInfoELF into something usable.

This patch changes MDCVInfoELF (which is currently unused, apparently
a vestigal bit of code landed as part of Solaris support) into a supported
CodeView format that simply contains a build id as raw bytes.

Modern ELF toolchains support build ids nicely:
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html...

It would be useful to have the original build ids of loaded modules in
Linux minidumps, since tools like Fedora's darkserver allow querying by build
id and the current Breakpad code truncates the build id to the size of a GUID,
which loses information:
https://darkserver.fedoraproject.org/

A follow-up patch will change the Linux minidump generation code to produce
MDCVInfoELF in minidumps instead of MDCVInfoPDB70. This patch should be landed
first to ensure that crash processors are able to handle this format before
dumps are generated containing it.

The full build id is exposed as the return value of Minidump::code_identifier(),
which currently just returns "id" for modules in Linux dumps. For
backwards-compatibility, Minidump::debug_identifier() continues to treat
the build id as a GUID, so debug identifiers for existing modules will not
change.

BUG=
R=mark@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/7a364b1262df4fe6f861033...
==========

[Ted Mielczarek, 2016-02-10 14:00:05]

Committed patchset #1 (id:1) manually as
7a364b1262df4fe6f861033547f177f8dd17fa30 (presubmit successful).

[ivanpe, 2016-02-17 02:28:00]

ivanpe@chromium.org changed reviewers:
+ ivanpe@chromium.org

[ivanpe, 2016-02-17 02:28:00]

Hi Ted, this change is breaking minidump_unittest when compiled for 64-bit
platforms.  Here is how to reproduce:

./configure
make
make check
src/processor/minidump_unittest

[ RUN      ] Dump.OneModuleCVELF
2016-02-16 18:26:54: minidump.cc:4249: INFO: Minidump reopening minidump 
2016-02-16 18:26:54: minidump.cc:4385: INFO: Minidump not byte-swapping minidump
*** buffer overflow detected ***: src/processor/minidump_unittest terminated
...

[vapier, 2016-02-17 04:17:10]

vapier@chromium.org changed reviewers:
+ vapier@chromium.org

[vapier, 2016-02-17 04:17:10]

yeah, we tried updating in CrOS, but this broke on all our bots :(

[Ted Mielczarek, 2016-02-17 10:58:14]

On 2016/02/17 02:28:00, ivanpe wrote:
> Hi Ted, this change is breaking minidump_unittest when compiled for 64-bit
> platforms.  Here is how to reproduce:
> 
> ./configure
> make
> make check
> src/processor/minidump_unittest
> 
> [ RUN      ] Dump.OneModuleCVELF
> 2016-02-16 18:26:54: minidump.cc:4249: INFO: Minidump reopening minidump 
> 2016-02-16 18:26:54: minidump.cc:4385: INFO: Minidump not byte-swapping
minidump
> *** buffer overflow detected ***: src/processor/minidump_unittest terminated
> ...

The tests pass on my 64-bit Ubuntu with GCC 4.8, are you using any extra
CXXFLAGS or a different toolchain? I just tried running minidump_unittest under
Valgrind locally and didn't see any errors either.

[Ted Mielczarek, 2016-02-17 11:05:48]

Oh, apparently I had configured my objdir to build without optimization at some
point. Rebuilding with -O2 gives me the same error locally now. I'll get that
sorted ASAP.

[Ted Mielczarek, 2016-02-17 11:46:25]

I landed a simple fix for this:
https://chromium.googlesource.com/breakpad/breakpad/+/c05ee946049cbf964b668dc...

Sorry for not catching it before landing. :-(

How much effort would it take to get try bots/CI setup for Breakpad? I'd offer
to help but I suspect it's entirely Google-internal work. I looked into setting
up a GitHub mirror with Travis-CI at one point but the 100MB file in Breakpad's
git history prevents pushing a clone to GitHub.

[vapier, 2016-02-17 20:30:17]

seems to work for me in CrOS, so i've sent it to the bots.  thanks!