Re: [PATCH v3 3/6] libdwelf: Add dwelf_dwarf_gnu_debugaltlink

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2014-04-22 at 09:33 +0200, Florian Weimer wrote:
> On 04/18/2014 01:02 PM, Mark Wielaard wrote:
> > On Tue, 2014-04-15 at 14:49 +0200, Florian Weimer wrote:
> >> +2014-04-15  Florian Weimer  <fweimer@redhat.com>
> >> +
> >> +	* dwelf_dwarf_gnu_debugaltlink.c: New file.
> >> +	* Makefile.am (libdwelf_a_SOURCES): Add it.
> >> +	* libdwelf.h (dwelf_dwarf_gnu_debugaltlink): Declare new function.
> >> +	* libdwelfP.h (dwelf_dwarf_gnu_debugaltlink): Add internal
> >> +	declaration.
> >
> > The code looks fine. But I still like the "tri-state" return more to
> > indicate why/how things went. Precisely because then you don't have to
> > do:
> >
> >> +  Elf_Data *data = dwarf->sectiondata[IDX_gnu_debugaltlink];
> >> +  if (data == NULL)
> >> +    {
> >> +      /* Missing data is not actually an error. */
> >> +      __libdw_seterrno (0);
> >> +      return NULL;
> >> +    }
> >> [...]
> >> +/* Returns the name of the alternate debug information file from the
> >> +   .gnu_debugaltlink section if found in the ELF.  On success, writes
> >> +   a pointer to the build ID to *BUILD_IDP, and its length to
> >> +   *BUILD_ID_LENP.  Returns NULL if the ELF file didn't have a
> >> +   .gnu_debuglink section (and sets the dwarf_errcode error code to
> >> +   0), had malformed data in the section or some other error occured.
> >> +   The returned pointers are valid while the ELF handle is valid.  */
> >> +extern const char *dwelf_dwarf_gnu_debugaltlink (Dwarf *dwarf,
> >> +						 const void **build_idp,
> >> +						 size_t *build_id_lenp);
> 
> I'm not sure if this addresses this completely addresses the issue.  A 
> return value of 0 could mean "no debugaltlink data" or "the build ID has 
> zero length".

But a zero length build ID isn't valid, so then you would just return -1
and set DWARF_E_INVALID_ELF.

> > Thanks for adding the testcase. run-debugaltlink.sh should also be added
> > to EXTRA_DIST (I also forgot that in my dwelf_elf_gnu_debuglink patch
> > (fixed on the mjw/dwelf branch).
> 
> Can't we handle this in a better way with make variables, so that the 
> file needs to be listed in just one place?

Yeah we probably should, I have forgotten more than once.

Cheers,

Mark