This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH v2 1/3] libdw: Add dwarf_debugaltlink function
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 10 Apr 2014 18:10:24 +0200
- Subject: Re: [PATCH v2 1/3] libdw: Add dwarf_debugaltlink function
Hi Florian,
On Thu, 2014-04-10 at 13:19 +0200, Florian Weimer wrote:
> +2014-04-10 Florian Weimer <fweimer@redhat.com>
> +
> + * libdw.h (dwarf_debugaltlink): New function.
> + * libdwP.h (IDX_debug_info): Add IDX_gnu_debugaltlink.
> + (dwarf_debugaltlink): Internal declaration.
> + * Makefile.am (libdw_a_SOURCES): Add dwarf_debugaltlink.c.
> + * libdw.map (ELFUTILS_0.159): Export dwarf_debugaltlink.
> + * dwarf_debugaltlink.c: New file.
> + * dwarf_begin_elf.c (dwarf_scnnames): Add IDX_gnu_debugaltlink
> + element.
> + (check_section): Call open_debugaltlink.
I like the helper function, but I think it is a little more low-level
than what we normally provide. We should at least (also) have a
dwarf_get_alt () that returns a Dwarf *.
If we provide this low-level function then it would be good to also
provide a similar one for .gnu_debuglink (which already exists privately
as find_debuglink in libdwfl) and for the build_id of an Elf file (which
also exist privately as __libdwfl_find_elf_build_id in libdwfl, but that
does a bit more by also determining the load address). If we provide
those helper functions because they might indeed be useful to people
wanting to write their own Elf/Dwarf file search functions, I am not
sure how to call them. I don't want people to confuse these lower-level
helper functions (which expose Elf file details) with the higher-level
libdw/Dwarf functionality.
> +#ifdef ENABLE_DWZ
> + /* For dwz multifile support, ignore if it looks wrong. */
> + if (result->sectiondata[IDX_gnu_debugaltlink] != NULL)
> + {
> + const char *alt_name;
> + const void *build_id;
> + size_t id_len;
> + if (INTUSE (dwarf_debugaltlink) (result, &alt_name, &build_id, &id_len)
> + == 0)
> + return open_debugaltlink (result, alt_name, build_id, id_len);
> + }
> +#endif /* ENABLE_DWZ */
Since open_debugaltlink already does the != NULL check you don't need to
repeat it here.
> +int dwarf_debugaltlink (Dwarf *dwarf,
> + const char **alt_name,
> + const void **build_id,
> + size_t *build_id_len)
> +{
> + Elf_Data *data = dwarf->sectiondata[IDX_gnu_debugaltlink];
> + if (data == NULL)
> + {
> + __libdw_seterrno (DWARF_E_NO_ALT_DEBUGLINK);
> + return -1;
> + }
> +
> + const void *ptr = memchr (data->d_buf, '\0', data->d_size);
> + if (ptr == NULL)
> + {
> + __libdw_seterrno (DWARF_E_INVALID_ELF);
> + return -1;
> + }
> + *alt_name = data->d_buf;
> + *build_id = ptr + 1;
> + *build_id_len = data->d_size - (ptr - data->d_buf + 1);
> + return 0;
> +}
I would make build_id_len the return value. Then you can return:
- 0 No debugaltlink found.
- > 0 Length of build_id, success.
- < 0 Error.
> +/* Get the data from the .gnu_debugaltlink section. */
> +extern int dwarf_debugaltlink (Dwarf *dwarf,
> + const char **alt_name,
> + const void **build_id,
> + size_t *build_id_len);
Make sure to document lifetime/ownership. Pointers valid as long as
Dwarf is. Might be obvious, but better be sure.
Thanks,
Mark