[PATCH] Core file build-ids
Keith Seitz
keiths@redhat.com
Tue Oct 29 15:26:00 GMT 2019
Ping?
On 10/16/19 8:22 AM, Keith Seitz wrote:
> Hi,
>
> Has anyone had an opportunity to (or plan to) review?
>
> Keith
>
> On 10/1/19 6:07 AM, Keith Seitz wrote:
>> Hi, Nick,
>>
>> Wowser, it's been a busy quarter! I'm anxious to get back to this patch.
>> I appreciate your patience.
>>
>> On 6/7/19 9:16 AM, Nick Clifton wrote:
>>>> +void
>>>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)> +{> + return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)> + (templ, offset);
>>>
>>> I would recommend being a little bit paranoid here and checking
>>> that templ is an elf format bfd before attempting to access the
>>> backend data.
>>
>> Done.
>>
>>>> - return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>>>> + if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>>>> + return FALSE;
>>>> + if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>>>> + _bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>>>> + return TRUE;
>>>
>>> If _bfd_elf_core_find_build_id can fail (see above and below) then
>>> it would be worth checking the return value for an error result.
>>
>> My concern here is what happens if we fail to find a build-id in the
>> core file (I/O error jumping through a header or it's just plain missing).
>> If _bfd_elf_core_find_build_id returns FALSE and we bail here, this will
>> abort constructing any more phdrs.
>>
>> After implementing your recommendation, I can actually demonstrate
>> this scenario with one of gdb's tests: gdb.arch/i386-biarch-core.exp. This
>> test will FAIL with a memory read error while attempting to print the
>> contents of some memory location in the core file.
>>
>> This location exists in a section that is internalized after we attempt
>> to find a (missing) build-id.
>>
>> That's why I made this an optional (as in, we don't care if it fails)
>> step when constructing the section.
>>
>> Maybe I've misunderstood what you were really asking me to do?
>>
>>>> +/* Attempt to find a build-id in a core file from the core file BFD.
>>>> + OFFSET is the file offset to a PT_LOAD segment that may contain
>>>> + the build-id note. */
>>>> +
>>>> +void
>>>> +NAME(_bfd_elf,core_find_build_id)
>>>
>>> Given that things can wrong when attempting to find a build_id, I think
>>> that this function should return a bfd_boolean. I appreciate that the
>>> caller could just check abfd->build_id != NULL, (assuming that it was NULL
>>> to start with), but I think that it is cleaner that the function itself
>>> should tell its caller whether ot not it succeeded.
>>
>> I've made this change in this version, but at the one place it is called,
>> the return value is ignored (see above). Nonetheless, I could imagine a
>> use for this outside of my/gdb's specific use case, and I think returning
>> bfd_boolean is a better/safer/more-forward-thinking option, so I've left
>> it (updated per your request) in this version.
>>
>>>> + Elf_External_Ehdr x_ehdr; /* Elf file header, external form */
>>>> + Elf_Internal_Ehdr i_ehdr; /* Elf file header, internal form */
>>>
>>> Minor formatting nit: comments should end with a full stop and two spaces.
>>
>> Fixed.
>>
>> New version below.
>>
>> Keith
>>
>> Core file build-ids
>>
>> This patch adds support for reading build-ids from core files. I've
>> opted to maintain the current interfaces that BFD and GDB already use.
>>
>> The approach taken is fairly straightforward (YMMV). When a core file
>> is opened, BFD loops over the various segments in the ELF file and
>> program headers. During this time, whenever a segment whose type is
>> PT_LOAD is encountered, and the build-id is unknown, I've added a call
>> to inspect the segment for an ELF header. If a valid ELF file header
>> is found, it attempts to read in the program headers and scan for any
>> notes.
>>
>> For those unfamiliar with build-ids, please see
>> https://fedoraproject.org/wiki/Releases/FeatureBuildId .
>>
>> Yes, this feature has been in Fedora GDB for over a decade, but it exists
>> as a copy-n-paste of a lot of BFD's internals. For the brave, you can
>> read the original submission at
>> https://sourceware.org/ml/gdb-patches/2010-11/msg00353.html .
>>
>> This is an attempt to clean that up.
>>
>> I have run this patch through GDB's buildbot, and it has detected
>> no problems.
>>
>> bfd/ChangeLog:
>>
>> * elf-bfd.h (elf_backend_data) <elf_backend_core_find_build_id>: New
>> field.
>> (_bfd_elf32_core_find_build_id, _bfd_elf64_core_find_build_id): New
>> functions.
>> (elf_read_notes): Add declaration.
>> * elf.c (elf_read_notes): Move elf-bfd.h.
>> (_bfd_elf_core_find_build_id): New function.
>> (bfd_section_from_phdr): Scan core file PT_LOAD segments for
>> build-id if none is known.
>> (elf_parse_notes): For core files, scan for notes.
>> * elfcore.h (elf_core_file_matches_executable_p): If both
>> BFDs have identical build-ids, then they match.
>> (_bfd_elf_core_find_build_id): New function.
>> * elfxx-target.h (elf_backend_core_find_build_id): Define.
>> (elfNN_bed): Add elf_backend_core_find_build_id.
>> ---
>> bfd/elf-bfd.h | 8 ++++
>> bfd/elf.c | 23 +++++++---
>> bfd/elfcore.h | 106 +++++++++++++++++++++++++++++++++++++++++++++
>> bfd/elfxx-target.h | 4 ++
>> 4 files changed, 136 insertions(+), 5 deletions(-)
>>
>> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
>> index 0a83c17332..2d214be87f 100644
>> --- a/bfd/elf-bfd.h
>> +++ b/bfd/elf-bfd.h
>> @@ -1377,6 +1377,8 @@ struct elf_backend_data
>> int (*target_read_memory) (bfd_vma vma, bfd_byte *myaddr,
>> bfd_size_type len));
>>
>> + bfd_boolean (*elf_backend_core_find_build_id) (bfd *, bfd_vma);
>> +
>> /* This function is used by `_bfd_elf_get_synthetic_symtab';
>> see elf.c. */
>> bfd_vma (*plt_sym_val) (bfd_vma, const asection *, const arelent *);
>> @@ -2398,6 +2400,8 @@ extern bfd_boolean bfd_elf32_core_file_matches_executable_p
>> (bfd *, bfd *);
>> extern int bfd_elf32_core_file_pid
>> (bfd *);
>> +extern bfd_boolean _bfd_elf32_core_find_build_id
>> + (bfd *, bfd_vma);
>>
>> extern bfd_boolean bfd_elf32_swap_symbol_in
>> (bfd *, const void *, const void *, Elf_Internal_Sym *);
>> @@ -2444,6 +2448,8 @@ extern bfd_boolean bfd_elf64_core_file_matches_executable_p
>> (bfd *, bfd *);
>> extern int bfd_elf64_core_file_pid
>> (bfd *);
>> +extern bfd_boolean _bfd_elf64_core_find_build_id
>> + (bfd *, bfd_vma);
>>
>> extern bfd_boolean bfd_elf64_swap_symbol_in
>> (bfd *, const void *, const void *, Elf_Internal_Sym *);
>> @@ -2768,6 +2774,8 @@ extern bfd_boolean _bfd_elf_merge_object_attributes
>> extern bfd_boolean _bfd_elf_merge_unknown_attribute_low (bfd *, bfd *, int);
>> extern bfd_boolean _bfd_elf_merge_unknown_attribute_list (bfd *, bfd *);
>> extern Elf_Internal_Shdr *_bfd_elf_single_rel_hdr (asection *sec);
>> +extern bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
>> + size_t align) ;
>>
>> extern bfd_boolean _bfd_elf_parse_gnu_properties
>> (bfd *, Elf_Internal_Note *);
>> diff --git a/bfd/elf.c b/bfd/elf.c
>> index 664eae5c66..2ae3ec53c8 100644
>> --- a/bfd/elf.c
>> +++ b/bfd/elf.c
>> @@ -53,8 +53,6 @@ static int elf_sort_sections (const void *, const void *);
>> static bfd_boolean assign_file_positions_except_relocs (bfd *, struct bfd_link_info *);
>> static bfd_boolean prep_headers (bfd *);
>> static bfd_boolean swap_out_syms (bfd *, struct elf_strtab_hash **, int) ;
>> -static bfd_boolean elf_read_notes (bfd *, file_ptr, bfd_size_type,
>> - size_t align) ;
>> static bfd_boolean elf_parse_notes (bfd *abfd, char *buf, size_t size,
>> file_ptr offset, size_t align);
>>
>> @@ -3060,6 +3058,16 @@ _bfd_elf_make_section_from_phdr (bfd *abfd,
>> return TRUE;
>> }
>>
>> +static bfd_boolean
>> +_bfd_elf_core_find_build_id (bfd *templ, bfd_vma offset)
>> +{
>> + /* The return value is ignored. Build-ids are considered optional. */
>> + if (templ->xvec->flavour == bfd_target_elf_flavour)
>> + return (*get_elf_backend_data (templ)->elf_backend_core_find_build_id)
>> + (templ, offset);
>> + return FALSE;
>> +}
>> +
>> bfd_boolean
>> bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>> {
>> @@ -3071,7 +3079,11 @@ bfd_section_from_phdr (bfd *abfd, Elf_Internal_Phdr *hdr, int hdr_index)
>> return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "null");
>>
>> case PT_LOAD:
>> - return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load");
>> + if (! _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "load"))
>> + return FALSE;
>> + if (bfd_get_format (abfd) == bfd_core && abfd->build_id == NULL)
>> + _bfd_elf_core_find_build_id (abfd, hdr->p_offset);
>> + return TRUE;
>>
>> case PT_DYNAMIC:
>> return _bfd_elf_make_section_from_phdr (abfd, hdr, hdr_index, "dynamic");
>> @@ -11790,7 +11802,8 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>> GROKER_ELEMENT ("NetBSD-CORE", elfcore_grok_netbsd_note),
>> GROKER_ELEMENT ( "OpenBSD", elfcore_grok_openbsd_note),
>> GROKER_ELEMENT ("QNX", elfcore_grok_nto_note),
>> - GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note)
>> + GROKER_ELEMENT ("SPU/", elfcore_grok_spu_note),
>> + GROKER_ELEMENT ("GNU", elfobj_grok_gnu_note)
>> };
>> #undef GROKER_ELEMENT
>> int i;
>> @@ -11830,7 +11843,7 @@ elf_parse_notes (bfd *abfd, char *buf, size_t size, file_ptr offset,
>> return TRUE;
>> }
>>
>> -static bfd_boolean
>> +bfd_boolean
>> elf_read_notes (bfd *abfd, file_ptr offset, bfd_size_type size,
>> size_t align)
>> {
>> diff --git a/bfd/elfcore.h b/bfd/elfcore.h
>> index 3550eaac27..751e831914 100644
>> --- a/bfd/elfcore.h
>> +++ b/bfd/elfcore.h
>> @@ -49,6 +49,13 @@ elf_core_file_matches_executable_p (bfd *core_bfd, bfd *exec_bfd)
>> return FALSE;
>> }
>>
>> + /* If both BFDs have identical build-ids, then they match. */
>> + if (core_bfd->build_id != NULL && exec_bfd->build_id != NULL
>> + && core_bfd->build_id->size == exec_bfd->build_id->size
>> + && memcmp (core_bfd->build_id->data, exec_bfd->build_id->data,
>> + core_bfd->build_id->size) == 0)
>> + return TRUE;
>> +
>> /* See if the name in the corefile matches the executable name. */
>> corename = elf_tdata (core_bfd)->core->program;
>> if (corename != NULL)
>> @@ -313,3 +320,102 @@ wrong:
>> fail:
>> return NULL;
>> }
>> +
>> +/* Attempt to find a build-id in a core file from the core file BFD.
>> + OFFSET is the file offset to a PT_LOAD segment that may contain
>> + the build-id note. Returns TRUE upon success, FALSE otherwise. */
>> +
>> +bfd_boolean
>> +NAME(_bfd_elf,core_find_build_id)
>> + (bfd *abfd,
>> + bfd_vma offset)
>> +{
>> + Elf_External_Ehdr x_ehdr; /* Elf file header, external form. */
>> + Elf_Internal_Ehdr i_ehdr; /* Elf file header, internal form. */
>> + Elf_Internal_Phdr *i_phdr;
>> + unsigned int i;
>> +
>> + /* Seek to the position of the segment at OFFSET. */
>> + if (bfd_seek (abfd, offset, SEEK_SET) != 0)
>> + goto fail;
>> +
>> + /* Read in the ELF header in external format. */
>> + if (bfd_bread (&x_ehdr, sizeof (x_ehdr), abfd) != sizeof (x_ehdr))
>> + {
>> + if (bfd_get_error () != bfd_error_system_call)
>> + goto wrong;
>> + else
>> + goto fail;
>> + }
>> +
>> + /* Now check to see if we have a valid ELF file, and one that BFD can
>> + make use of. The magic number must match, the address size ('class')
>> + and byte-swapping must match our XVEC entry, and it must have a
>> + section header table (FIXME: See comments re sections at top of this
>> + file). */
>> +
>> + if (! elf_file_p (&x_ehdr)
>> + || x_ehdr.e_ident[EI_VERSION] != EV_CURRENT
>> + || x_ehdr.e_ident[EI_CLASS] != ELFCLASS)
>> + goto wrong;
>> +
>> + /* Check that file's byte order matches xvec's */
>> + switch (x_ehdr.e_ident[EI_DATA])
>> + {
>> + case ELFDATA2MSB: /* Big-endian */
>> + if (! bfd_header_big_endian (abfd))
>> + goto wrong;
>> + break;
>> + case ELFDATA2LSB: /* Little-endian */
>> + if (! bfd_header_little_endian (abfd))
>> + goto wrong;
>> + break;
>> + case ELFDATANONE: /* No data encoding specified */
>> + default: /* Unknown data encoding specified */
>> + goto wrong;
>> + }
>> +
>> + elf_swap_ehdr_in (abfd, &x_ehdr, &i_ehdr);
>> +#if DEBUG & 1
>> + elf_debug_file (&i_ehdr);
>> +#endif
>> +
>> + if (i_ehdr.e_phentsize != sizeof (Elf_External_Phdr) || i_ehdr.e_phnum == 0)
>> + goto fail;
>> +
>> + /* Read in program headers. */
>> + i_phdr = (Elf_Internal_Phdr *) bfd_alloc2 (abfd, i_ehdr.e_phnum,
>> + sizeof (*i_phdr));
>> + if (i_phdr == NULL)
>> + goto fail;
>> +
>> + if (bfd_seek (abfd, (file_ptr) (offset + i_ehdr.e_phoff), SEEK_SET) != 0)
>> + goto fail;
>> +
>> + /* Read in program headers and parse notes. */
>> + for (i = 0; i < i_ehdr.e_phnum; ++i, ++i_phdr)
>> + {
>> + Elf_External_Phdr x_phdr;
>> +
>> + if (bfd_bread (&x_phdr, sizeof (x_phdr), abfd) != sizeof (x_phdr))
>> + goto fail;
>> + elf_swap_phdr_in (abfd, &x_phdr, i_phdr);
>> +
>> + if (i_phdr->p_type == PT_NOTE && i_phdr->p_filesz > 0)
>> + {
>> + elf_read_notes (abfd, offset + i_phdr->p_offset,
>> + i_phdr->p_filesz, i_phdr->p_align);
>> + if (abfd->build_id != NULL)
>> + return TRUE;
>> + }
>> + }
>> +
>> + /* Having gotten this far, we have a valid ELF section, but no
>> + build-id was found. */
>> + goto fail;
>> +
>> +wrong:
>> + bfd_set_error (bfd_error_wrong_format);
>> +fail:
>> + return FALSE;
>> +}
>> diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
>> index 78a1f6314d..29ad2e8481 100644
>> --- a/bfd/elfxx-target.h
>> +++ b/bfd/elfxx-target.h
>> @@ -521,6 +521,9 @@
>> #ifndef elf_backend_bfd_from_remote_memory
>> #define elf_backend_bfd_from_remote_memory _bfd_elfNN_bfd_from_remote_memory
>> #endif
>> +#ifndef elf_backend_core_find_build_id
>> +#define elf_backend_core_find_build_id _bfd_elfNN_core_find_build_id
>> +#endif
>> #ifndef elf_backend_got_header_size
>> #define elf_backend_got_header_size 0
>> #endif
>> @@ -860,6 +863,7 @@ static struct elf_backend_data elfNN_bed =
>> elf_backend_mips_rtype_to_howto,
>> elf_backend_ecoff_debug_swap,
>> elf_backend_bfd_from_remote_memory,
>> + elf_backend_core_find_build_id,
>> elf_backend_plt_sym_val,
>> elf_backend_common_definition,
>> elf_backend_common_section_index,
>>
>
>
More information about the Binutils
mailing list