Minidumper: Get the nexe's build ID from PT_NOTE when available

Minidumper: Get the nexe's build ID from PT_NOTE when available

Get the build ID from PT_NOTE in the ELF Program Headers when these
are available at run time.  This currently only works in the NaCl ARM
GCC toolchain, which is the only NaCl toolchain using the new-style
linker layout where the ELF headers are mapped into memory and
accessible via __ehdr_start.

The minidumper will use this build ID by default if it's available,
but the build ID is still overridable by
nacl_minidump_set_module_build_id().

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3424
TEST=run_minidump_test + manual testing of *.dmp files

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=11335

[Roland McGrath, 2013-05-10 16:58:13]

https://codereview.chromium.org/14757008/diff/1/src/untrusted/minidump_genera...
File src/untrusted/minidump_generator/build_id.c (right):

https://codereview.chromium.org/14757008/diff/1/src/untrusted/minidump_genera...
src/untrusted/minidump_generator/build_id.c:36: if (note->n_type ==
NT_GNU_BUILD_ID) {
This needs to check that the name is "GNU" (four bytes including '\0').  The
n_type value that is NT_GNU_BUILD_ID for "GNU" has other meanings for other name
values.

[Mark Seaborn, 2013-05-10 18:25:28]

https://codereview.chromium.org/14757008/diff/1/src/untrusted/minidump_genera...
File src/untrusted/minidump_generator/build_id.c (right):

https://codereview.chromium.org/14757008/diff/1/src/untrusted/minidump_genera...
src/untrusted/minidump_generator/build_id.c:36: if (note->n_type ==
NT_GNU_BUILD_ID) {
On 2013/05/10 16:58:13, Roland McGrath wrote:
> This needs to check that the name is "GNU" (four bytes including '\0').  The
> n_type value that is NT_GNU_BUILD_ID for "GNU" has other meanings for other
name
> values.

Ah, good point.  I hadn't quite realised it was namespaced like that.  Fixed.

Breakpad's file_id.cc also doesn't check this properly.

[bradn, 2013-05-10 18:51:38]

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
File src/untrusted/minidump_generator/build_id.c (right):

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:10: #include
"native_client/src/include/elf32.h"
link.h ?

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:13: extern const Elf32_Ehdr
__ehdr_start __attribute__((weak));
Why does this need the weak attribute?

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:21: __ehdr_start.e_ident[EI_CLASS]
== ELFCLASS32 &&
The deep nesting is kind of undesirable as this is an error case? Return in the
inverted case?

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:25: Elf32_Phdr *phdrs = (Elf32_Phdr
*) (start_addr + __ehdr_start.e_phoff);
Wouldn't using using dl_iterate_phdr be preferable? I.e. using a public
interface?

[Mark Seaborn, 2013-05-10 19:29:41]

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
File src/untrusted/minidump_generator/build_id.c (right):

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:10: #include
"native_client/src/include/elf32.h"
On 2013/05/10 18:51:38, bradn wrote:
> link.h ?

That's a glibc header related to the dynamic linker.  We want this to work with
newlib and with static linking.

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:13: extern const Elf32_Ehdr
__ehdr_start __attribute__((weak));
On 2013/05/10 18:51:38, bradn wrote:
> Why does this need the weak attribute?

Comment added (copied from tls.c).

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:21: __ehdr_start.e_ident[EI_CLASS]
== ELFCLASS32 &&
On 2013/05/10 18:51:38, bradn wrote:
> The deep nesting is kind of undesirable as this is an error case? Return in
the
> inverted case?

It's not really an error, just "not found when searching".

We might change the x86 nacl-gcc toolchains to provide the build ID by defining
a __build_id symbol via the linker script.  In that case, that will be a second
build ID source to check in nacl_get_build_id() after checking __ehdr_start.

https://codereview.chromium.org/14757008/diff/19001/src/untrusted/minidump_ge...
src/untrusted/minidump_generator/build_id.c:25: Elf32_Phdr *phdrs = (Elf32_Phdr
*) (start_addr + __ehdr_start.e_phoff);
On 2013/05/10 18:51:38, bradn wrote:
> Wouldn't using using dl_iterate_phdr be preferable? I.e. using a public
> interface?

dl_iterate_phdr() is a glibc interface.  We want this to work with newlib and
static linking.

[bradn, 2013-05-10 19:34:01]

lgtm

[Mark Seaborn, 2013-05-10 19:56:52]

Committed patchset #3 manually as r11335 (presubmit successful).