Re: [PATCH v2 1/3] libdw: Add dwarf_debugaltlink function

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2014-04-10 at 18:38 +0200, Florian Weimer wrote:
> On 04/10/2014 06:10 PM, Mark Wielaard wrote:
> > 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 *.
> 
> This would just return the stored Dwarf *, and not attempt to open any 
> files?

Yes. dwarf_getalt () would just return the set Dwarf *alt (or NULL when
none was set).

> > 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.
> 
> We could add them at the ELF layer, but then we'd have to iterate over 
> all the ELF section headers in each of these functions.  The section 
> header array in libdw avoids this.  I'm also not sure where to put such 
> convenience functions.

Yeah, I might be designing things. You are right having it in libdw and
using the debug section array is the right thing to use. I am mostly
concerned about the naming because I think this functionality is at a
different level than the rest.

> libebl?  But I currently don't use that, and I 
> think some downstreams assign send-class status to it.

libebl is strictly for internal use, not for public functions.
The utility functions could just be part of libdw, just like the dwfl
functions are, just with a slightly different naming convention (let the
bike-shedding begin).

BTW. I don't know what assigning send-class status means.

> > 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.
> 
> Hmm, I wonder if the interface is similar enough to read(2) so that the 
> tri-state return value isn't a problem.  Usually, it's problematic 
> because callers are tempted to treat the result as boolean, interpreting 
> -1 as success.

The tri-state return value pattern is used with more libdw functions, so
I don't think it will surprise our users. Especially not if we properly
document it :)

Cheers,

Mark