[Mesa-dev] [PATCH 1/2] util: Add utility build-id code.

[Chad Versace]

On Tue 14 Feb 2017, Matt Turner wrote:
> Provides the ability to read the .note.gnu.build-id section of ELF
> binaries, which is inserted by the --build-id=... flag to ld.
> ---
>  configure.ac              |   2 +
>  src/util/Makefile.sources |   2 +
>  src/util/build_id.c       | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/build_id.h       |  56 ++++++++++++++++++++++++
>  4 files changed, 169 insertions(+)
>  create mode 100644 src/util/build_id.c
>  create mode 100644 src/util/build_id.h


> +AC_CHECK_FUNC([dl_iterate_phdr], [DEFINES="$DEFINES -DHAVE_DL_ITERATE_PHDR"])

Nice. I wasn't aware of dl_iterate_phdr(). My code for querying the
build-id was less slick. It used open(2) on the library, then manually
parsed the ElfW(Ehdr) and ElfW(Shdr) to find the build-id node.


> diff --git a/src/util/build_id.c b/src/util/build_id.c
> new file mode 100644
> index 0000000..a2e21b7
> --- /dev/null
> +++ b/src/util/build_id.c


> +#define ALIGN(val, align)      (((val) + (align) - 1) & ~((align) - 1))
> +
> +struct note {
> +   ElfW(Nhdr) nhdr;
> +
> +   char name[4];

Because nothing requires ElfW(Nhdr).n_namesz to be 4, please a comment
here explaining that we hardcoded 4 because the note name is "GNU".

> +   uint8_t build_id[0];
> +};
> +
> +struct callback_data {
> +   const char *name;
> +   struct note *note;
> +};
> +
> +static int
> +build_id_find_nhdr_callback(struct dl_phdr_info *info, size_t size, void *data_)
> +{

This function looks correct to me.

> +   struct callback_data *data = data_;
> +
> +   char *ptr = strstr(info->dlpi_name, data->name);
> +   if (ptr == NULL || ptr[strlen(data->name)] != '\0')
> +      return 0;
> +
> +   for (unsigned i = 0; i < info->dlpi_phnum; i++) {
> +      if (info->dlpi_phdr[i].p_type != PT_NOTE)
> +         continue;
> +
> +      struct note *note = (void *)(info->dlpi_addr +
> +                                   info->dlpi_phdr[i].p_vaddr);

> +      ptrdiff_t len = info->dlpi_phdr[i].p_filesz;

I was initially worried that the len should use p_memsz instead of
p_filesz. But the elf manpage says it's ok; if p_memsz > p_filesz, then
the excess bytes are mere padding.

> +
> +      while (len >= sizeof(struct note)) {
> +         if (note->nhdr.n_type == NT_GNU_BUILD_ID &&
> +            note->nhdr.n_descsz != 0 &&
> +            note->nhdr.n_namesz == 4 &&
> +            memcmp(note->name, "GNU", 4) == 0) {
> +            data->note = note;
> +            return 1;
> +         }
> +
> +         size_t offset = sizeof(ElfW(Nhdr)) +
> +                         ALIGN(note->nhdr.n_namesz, 4) +
> +                         ALIGN(note->nhdr.n_descsz, 4);
> +         note = (struct note *)((char *)note + offset);
> +         len -= offset;
> +      }
> +   }
> +
> +   return 0;
> +}
> +
> +const struct note *
> +build_id_find_nhdr(const char *name)

Please rename the 'name' parameter to be clearer. I was confused,
thinking that name was the name of ELF section for the note. It wasn't
until I read patch 2, and saw name="libvulkan.so" instead
name=".note.gnu.build-id" that I was able to understand this patch.

> +{
> +   struct callback_data data = {
> +      .name = name,
> +      .note = NULL,
> +   };
> +
> +   if (dl_iterate_phdr(build_id_find_nhdr_callback, &data)) {
> +      return data.note;
> +   } else {
> +      return NULL;
> +   }
> +}
> +
> +unsigned
> +build_id_length(const struct note *note)
> +{
> +   return note->nhdr.n_descsz;
> +}
> +
> +void
> +build_id_read(const struct note *note, unsigned char *build_id)
> +{

As I explain in patch 2, I think this function needs a 'size' parameter,
à la snprintf, due to Vulkan weirdness regarding restrictons on usage of
malloc.

> +   memcpy(build_id, note->build_id, note->nhdr.n_descsz);
> +}

> diff --git a/src/util/build_id.h b/src/util/build_id.h
> new file mode 100644
> index 0000000..0eaecf9
> --- /dev/null
> +++ b/src/util/build_id.h


> +struct note;

Please namespace struct note. Please :)

My only hard request is the size_t parameter for build_id_read(). My
other comments are just minor nits, that you can ignore.