Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159013002:
Read Chrome build ID and store it in leak reports (Closed)
DescriptionRead Chrome build ID and store it in leak reports
The build ID reading code is based on:
src/native_client/src/untrusted/minidump_generator/build_id.cc
BUG=chromium:627873
TEST=build successfully
Committed: https://crrev.com/c5b3973dd407938ff256a14610159aa863c35ab8
Cr-Commit-Position: refs/heads/master@{#407619}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Replace GNUBuildIDReader class with namespace #
Total comments: 22
Patch Set 3 : Rebased; Addressed comments #Patch Set 4 : Use std::equal for ELF_NOTE_GNU note string comparison #
Total comments: 1
Patch Set 5 : Remove DCHECK for build ID having been read #
Messages
Total messages: 37 (16 generated)
sque@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.cc:47: if (note->n_type == NT_GNU_BUILD_ID && where is this build id set in the binary? https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.h (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.h:24: void ReadBuildID(); why not just a static function to return the build_id? https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.h:26: const std::vector<uint8_t>& build_id() const { return build_id_; } DCHECK if the build_id_ hasn't been read?
https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.cc:47: if (note->n_type == NT_GNU_BUILD_ID && On 2016/07/18 22:03:53, Will Harris wrote: > where is this build id set in the binary? What do you mean? It's set by the build process. https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.h (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.h:24: void ReadBuildID(); On 2016/07/18 22:03:53, Will Harris wrote: > why not just a static function to return the build_id? Done. https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.h:26: const std::vector<uint8_t>& build_id() const { return build_id_; } On 2016/07/18 22:03:53, Will Harris wrote: > DCHECK if the build_id_ hasn't been read? Done.
https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.cc:47: if (note->n_type == NT_GNU_BUILD_ID && On 2016/07/19 00:29:34, Simon Que wrote: > On 2016/07/18 22:03:53, Will Harris wrote: > > where is this build id set in the binary? > > What do you mean? It's set by the build process. that's what I mean. Where is this set? Is this a standard ELF thing or is there some custom step in our build process setting this?
https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/gnu_build_id_reader.cc:47: if (note->n_type == NT_GNU_BUILD_ID && On 2016/07/19 00:33:42, Will Harris wrote: > On 2016/07/19 00:29:34, Simon Que wrote: > > On 2016/07/18 22:03:53, Will Harris wrote: > > > where is this build id set in the binary? > > > > What do you mean? It's set by the build process. > > that's what I mean. Where is this set? Is this a standard ELF thing or is there > some custom step in our build process setting this? AFAICT it is set as part of the standard build process. When I build it using the CrOS simplechrome workflow, the build ID is a random-looking 20-byte hash that changes each time. I don't think there's a custom value being passed in.
Description was changed from ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully ========== to ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully ==========
wfh@chromium.org changed reviewers: + rickyz@chromium.org
rickyz is the resident ELF expert, can you take a look as well? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:90: for (int i = 0; i < info->dlpi_phnum; ++i) { i masks variable i, is this what you meant? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:104: return 1; "The dl_iterate_phdr() function walks through the list of an application's shared objects and calls the function callback once for each object, until either all shared objects have been processed or callback returns a nonzero value." this callback will only be called once, is this desired?
Looks good - by the way, we have code to do this in breakpad already: https://cs.chromium.org/chromium/src/breakpad/src/common/linux/file_id.h I assume there are some dependency restrictions that prevent us from using those? https://codereview.chromium.org/2159013002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2159013002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:95: DCHECK(leak_detector::gnu_build_id_reader::ReadBuildID(&build_id_)); If you rely on side effects of a function, don't put it in a DCHECK - on builds that disable these, this call will be dropped. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:7: #include <libelf.h> Is just <elf.h> sufficient here? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:13: #error "Getting binary mapping info is not supported on this platform." Any reason to limit this to Chrome OS instead of Linux? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:46: using NoteHeaderPtr = const Elf32_Nhdr*; This is the same for 64 vs. 32, but might as well use ElfW(Nhdr)* https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:50: NoteHeaderPtr name_ptr = ¬e[1]; Wouldn't use NoteHeaderPtr as the type here, since this isn't really pointing to a header. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:51: if (name_ptr > end) { >= https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:93: // Elf64_Nhdr is the same as Elf32_Nhdr so we can use either here. Was this a leftover comment from a previous iteration? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:95: mapping_addr + segment_header.p_offset); segment_header.p_vaddr is probably more accurate here and below.
https://codereview.chromium.org/2159013002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2159013002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:95: DCHECK(leak_detector::gnu_build_id_reader::ReadBuildID(&build_id_)); On 2016/07/19 04:58:22, rickyz wrote: > If you rely on side effects of a function, don't put it in a DCHECK - on builds > that disable these, this call will be dropped. Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:7: #include <libelf.h> On 2016/07/19 04:58:22, rickyz wrote: > Is just <elf.h> sufficient here? Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:13: #error "Getting binary mapping info is not supported on this platform." On 2016/07/19 04:58:22, rickyz wrote: > Any reason to limit this to Chrome OS instead of Linux? Using OS_POSIX or OS_LINUX cause the #else condition to be evaluated. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:46: using NoteHeaderPtr = const Elf32_Nhdr*; On 2016/07/19 04:58:22, rickyz wrote: > This is the same for 64 vs. 32, but might as well use ElfW(Nhdr)* Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:50: NoteHeaderPtr name_ptr = ¬e[1]; On 2016/07/19 04:58:22, rickyz wrote: > Wouldn't use NoteHeaderPtr as the type here, since this isn't really pointing to > a header. Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:90: for (int i = 0; i < info->dlpi_phnum; ++i) { On 2016/07/19 02:51:38, Will Harris wrote: > i masks variable i, is this what you meant? No, accidentally had a duplicate nested loop. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:93: // Elf64_Nhdr is the same as Elf32_Nhdr so we can use either here. On 2016/07/19 04:58:22, rickyz wrote: > Was this a leftover comment from a previous iteration? Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:95: mapping_addr + segment_header.p_offset); On 2016/07/19 04:58:22, rickyz wrote: > segment_header.p_vaddr is probably more accurate here and below. Done. https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:104: return 1; On 2016/07/19 02:51:38, Will Harris wrote: > "The dl_iterate_phdr() function walks through the list of an application's > shared objects and calls the function callback once for each object, until > either all shared objects have been processed or callback returns a nonzero > value." > > this callback will only be called once, is this desired? Yes. We are only interested in the currently running binary, Chrome, which is going to be the first one. Made a comment about it.
On 2016/07/19 04:58:22, rickyz wrote: > Looks good - by the way, we have code to do this in breakpad already: > https://cs.chromium.org/chromium/src/breakpad/src/common/linux/file_id.h > > I assume there are some dependency restrictions that prevent us from using > those? it does look like the breakpad version has to bring in quite a few other files. I don't think we can depend on breakpad from Chrome, so the only option would be to move the code into base, probably overkill here. +mark might have ideas here? https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:8: #include <string.h> why is string needed?
https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:13: #error "Getting binary mapping info is not supported on this platform." On 2016/07/22 at 01:40:06, Simon Que wrote: > On 2016/07/19 04:58:22, rickyz wrote: > > Any reason to limit this to Chrome OS instead of Linux? > > Using OS_POSIX or OS_LINUX cause the #else condition to be evaluated. Yeah, what I meant was - if this works on OS_POSIX or OS_LINUX, is there any reason that we we restrict this code to OS_CHROMEOS only? Not a big deal either way, was just thinking it couldn't hurt to have the function available on non-Chrome OS in case somebody might want to use it in the future.
https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... File components/metrics/leak_detector/gnu_build_id_reader.cc (right): https://codereview.chromium.org/2159013002/diff/20001/components/metrics/leak... components/metrics/leak_detector/gnu_build_id_reader.cc:8: #include <string.h> On 2016/07/22 01:58:02, Will Harris wrote: > why is string needed? For memcmp(). But I wonder if that could be replaced with a string comparison.
Will Harris wrote: > it does look like the breakpad version has to bring in quite a few other files. > I don't think we can depend on breakpad from Chrome, so the only option would be > to move the code into base, probably overkill here. +mark might have ideas here? Can’t depend on Breakpad from chrome/, can’t depend on base from Breakpad. Sorry, no good answers there. Sucky as it is, there are executable format parsers pretty much all over the place, in duplicate and triplicate for ELF, Mach-O, and PE. This is not the first transgression, and I’m sure it won’t be the last.
Updated, PTAL
lgtm
sque@chromium.org changed reviewers: + isherman@chromium.org
+isherman to review components/metrics.gypi
gypi changes lgtm
The CQ bit was checked by sque@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2159013002/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): https://codereview.chromium.org/2159013002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/leak_detector/leak_detector_controller.cc:97: DCHECK(build_id_read); Unit tests are failing this DCHECK because the unit_tests binary does not have a build ID. Perhaps we should just disregard the return value, and allow a binary not to have a build ID. The |build_id_| vector would just be left empty.
On 2016/07/25 21:25:26, Simon Que wrote: > https://codereview.chromium.org/2159013002/diff/60001/chrome/browser/metrics/... > File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right): > > https://codereview.chromium.org/2159013002/diff/60001/chrome/browser/metrics/... > chrome/browser/metrics/leak_detector/leak_detector_controller.cc:97: > DCHECK(build_id_read); > Unit tests are failing this DCHECK because the unit_tests binary does not have a > build ID. > > Perhaps we should just disregard the return value, and allow a binary not to > have a build ID. The |build_id_| vector would just be left empty. sure. sgtm
The CQ bit was checked by sque@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2159013002/#ps80001 (title: "Remove DCHECK for build ID having been read")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully ========== to ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully ========== to ========== Read Chrome build ID and store it in leak reports The build ID reading code is based on: src/native_client/src/untrusted/minidump_generator/build_id.cc BUG=chromium:627873 TEST=build successfully Committed: https://crrev.com/c5b3973dd407938ff256a14610159aa863c35ab8 Cr-Commit-Position: refs/heads/master@{#407619} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c5b3973dd407938ff256a14610159aa863c35ab8 Cr-Commit-Position: refs/heads/master@{#407619} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
