Re: [PATCH] libdw: Try .gnu_debugaltlink paths relative to the debug file

[Mark Wielaard <mjw at redhat dot com>]

On Mon, 2013-12-16 at 11:16 -0800, Josh Stone wrote:
> On 12/16/2013 06:17 AM, Mark Wielaard wrote:
> > On Sat, 2013-12-14 at 13:51 -0800, Josh Stone wrote:
> >> In Fedora, these paths are relative to the debug file itself, so that's
> >> a better thing to try first.  We don't actually have the debug path in
> >> libdw, but we can usually find it from elf->fildes and /proc/self/fd/.
> > 
> > It is a smart trick (using Elf internals though).
> 
> Is that a problem to use libelfP.h?  I see this going on elsewhere, in
> elflint, readelf, libdw/cfi.h, and a few places in libdwfl.  At least
> fildes is a pretty unassuming detail of the implementation, IMO.

No it isn't on itself. But it is a warning flag to check why we need to
use any internals and why a user couldn't do something similar
themselves. In some cases there are very good reasons to use and hide
implementation details like these. In this case however I do think it
shows something "fishy" is going on. An Elf might or might not be backed
by a fildes (anymore). Like when elf_cntl (elf, ELF_C_FDREAD) has
already been called. So this "trick" might or might not work depending
on internal state of the Elf.

> > I actually think the automatic finding of debugaltlink files should move
> > to libdwfl. And libdw should just provide a dwarf_set_altlink and
> > dwarf_get_altlink on a Dwarf (my original proposal had that, but I
> > removed them later). It might have been a mistake on my part to do the
> > magic in libdw directly. Roland was opposed to it back then, which is
> > why dwz support is not included by default. Although I don't fully
> > understand why he was against including it by default, I do think moving
> > the magic altlinking from libdw to libdwfl might be part of it.
> 
> That would be similar to how even the primary .gnu_debuglink is only
> handled in libdwfl, I suppose.

Right. In hindsight I think we should have viewed debugaltlink in the
same way. Also because libdwfl does have the search and file path based
mechanism already. While in libdw there is no such mechanism and
possibly no way to retrieve the file paths in the first place.

> You can argue both ways.  By making this "magic", direct libdw clients
> don't even need to know that dwz is playing this new game with binaries
> - it just works.

Except that it doesn't really, it might magically work most of the time.
But if it doesn't we don't provide any mechanism to make it work
reliably.

> But even that magic can be broken if the consumer makes any assumptions
> about offset numbers, which can now overlap as we've seen.  It's also a
> bit ugly that you can't use dwarf_offdie at all.  At least there's
> dwarf_offdie_types if you realize you followed a ref_sig8; perhaps
> dwarf_offdie_alt is needed too.  Hmm, or use it on dwarf_get_altlink.
> Even that requires you to know the context, yet something like
> dwarf_getfuncs might have fed you that alt die without you knowing.
> Perhaps it also needs:
> 
>   Dwarf *dwarf_diedwarf (Dwarf_Die *die, bool *is_type_p);
> 
> so you can know the context of any die.

Yes, users might use offsets directly. But I do think that is a bit of a
leaky abstraction. Those functions really are only there as "backup" in
case we forgot to provide a more appropriate DIE referencing function
(dwarf_formref_die). So I would be hesitant to introduce even much such
low-level indexing/offset functions unless there is a strong use case
for them.

> > Note that in libdwfl the find_debuginfo callback deals with file names
> > (not raw Elf or Dwarf handles) making things easier there. Also the
> > logic is reversed there. First build-ids are tried, then file based
> > search paths. Which feels like a better order to try things.
> 
> I don't know that the order really matters, since the internal buildids
> are confirmed anyway.  Do what you like. :)

My non-scientific feeling (I didn't do any measurements) was that doing
the build-id lookup is cheaper and more likely to work the first time.
But yeah, it shouldn't really matter which order they are in. At least
it would be slightly more consistent to do the lookups in the same
order.

> > I am not 100% against this patch, since we do currently do the magic in
> > libdw and the code isn't included by default anyway. But would prefer to
> > fix this by moving things fully into libdwfl and introducing a libdw
> > getter and setter.
> 
> I'm unlikely to volunteer for moving stuff right now. :)  So let's
> proceed with this patch for now, and it can be morphed into libdwfl with
> the rest of the magic later.

My hope is that in libdwfl we don't need the extra trickery because we
can use file path based lookups directly.

Thanks,

Mark