Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issue 1688743002: Switch the Linux minidump writer to use MDCVInfoELF for CV data. (Closed)

Created:
4 years, 10 months ago by Ted Mielczarek
Modified:
4 years, 8 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Rework to handle arbitrary size build ids #

Unified diffs Side-by-side diffs Delta from patch set Stats (+459 lines, -227 lines) Patch
M Makefile.am View 1 1 chunk +3 lines, -1 line 0 comments Download
M Makefile.in View 1 1 chunk +4 lines, -1 line 0 comments Download
M src/client/linux/handler/exception_handler_unittest.cc View 1 2 chunks +1 line, -14 lines 0 comments Download
M src/client/linux/microdump_writer/microdump_writer.cc View 1 4 chunks +14 lines, -3 lines 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.h View 1 2 chunks +5 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.cc View 1 3 chunks +5 lines, -4 lines 0 comments Download
M src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc View 1 4 chunks +20 lines, -13 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer.cc View 1 3 chunks +25 lines, -23 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer_unittest.cc View 1 5 chunks +52 lines, -35 lines 0 comments Download
M src/common/linux/dump_symbols.cc View 1 5 chunks +13 lines, -23 lines 0 comments Download
M src/common/linux/file_id.h View 1 2 chunks +15 lines, -12 lines 0 comments Download
M src/common/linux/file_id.cc View 1 10 chunks +36 lines, -34 lines 0 comments Download
M src/common/linux/file_id_unittest.cc View 1 10 chunks +117 lines, -59 lines 0 comments Download
M src/common/memory.h View 1 7 chunks +48 lines, -4 lines 0 comments Download
M src/common/memory_unittest.cc View 1 5 chunks +27 lines, -0 lines 0 comments Download
M src/processor/minidump_unittest.cc View 1 1 chunk +74 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (3 generated)
Ted Mielczarek
4 years, 10 months ago (2016-02-10 14:16:44 UTC) #1
Lei Zhang
I did not look at the previous MDCVInfoELF CL, so I'll let Mark have the ...
4 years, 10 months ago (2016-02-10 23:08:50 UTC) #2
Mark Mentovai
> the maximum build id length supported is 20 > bytes, with longer values being ...
4 years, 10 months ago (2016-02-19 19:15:29 UTC) #3
Ted Mielczarek
On 2016/02/19 19:15:29, Mark Mentovai wrote: > > the maximum build id length supported is ...
4 years, 10 months ago (2016-02-19 20:22:12 UTC) #4
Ted Mielczarek
Okay, I came back around to this and fixed it to handle arbitrarily-long build ids. ...
4 years, 8 months ago (2016-04-01 17:07:28 UTC) #5
Mark Mentovai
Ted Mielczarek wrote: > Okay, I came back around to this and fixed it to ...
4 years, 8 months ago (2016-04-05 00:19:35 UTC) #7
Ted Mielczarek
On 2016/04/05 00:19:35, Mark Mentovai wrote: > I thought that at some point in the ...
4 years, 8 months ago (2016-04-05 01:35:27 UTC) #8
Ted Mielczarek
Committed patchset #2 (id:20001) manually as 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 (presubmit successful).
4 years, 8 months ago (2016-04-05 13:34:27 UTC) #10
David Yen
On 2016/04/05 13:34:27, Ted Mielczarek wrote: > Committed patchset #2 (id:20001) manually as > 6c8f80aa8b3ba8120c4158c069bb298c044dedf9 ...
4 years, 8 months ago (2016-04-08 19:02:07 UTC) #11
Ted Mielczarek
On 2016/04/08 19:02:07, David Yen wrote: > On 2016/04/05 13:34:27, Ted Mielczarek wrote: > > ...
4 years, 8 months ago (2016-04-08 23:33:15 UTC) #12
David Yen
On 2016/04/08 23:33:15, Ted Mielczarek wrote: > On 2016/04/08 19:02:07, David Yen wrote: > > ...
4 years, 8 months ago (2016-04-11 17:23:09 UTC) #13
David Yen
On 2016/04/11 17:23:09, David Yen wrote: > On 2016/04/08 23:33:15, Ted Mielczarek wrote: > > ...
4 years, 8 months ago (2016-04-11 20:06:03 UTC) #15
Ted Mielczarek
On 2016/04/11 20:06:03, David Yen wrote: > On 2016/04/11 17:23:09, David Yen wrote: > > ...
4 years, 8 months ago (2016-04-11 20:21:36 UTC) #16
Ted Mielczarek
On 2016/04/11 20:06:03, David Yen wrote: > On 2016/04/11 17:23:09, David Yen wrote: > > ...
4 years, 8 months ago (2016-04-11 20:23:34 UTC) #17
David Yen
4 years, 8 months ago (2016-04-11 20:38:20 UTC) #18
Message was sent while issue was closed.
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!

Powered by Google App Engine
This is Rietveld 408576698