This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]