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

[Josh Stone <jistone at redhat dot com>]

On 12/16/2013 06:17 AM, Mark Wielaard wrote:
> On Sat, 2013-12-14 at 13:51 -0800, Josh Stone wrote:
>> The existing code was first trying .gnu_debugaltlink paths directly, so
>> relative paths would start from the current program's working directory.
>> That's unlikely to be useful, so it was almost always falling back to a
>> build-id based path.
>>
>> 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.

> 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.

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.

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.

> 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. :)

> 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.

>> +/* Make relative links into an absolute path.  */
>> +static char *
>> +build_rel_debugaltlink (Dwarf *result, const char *alt_name)
>> +{
>> +  if (alt_name[0] == '/' || result->elf->fildes < 0)
>> +    return NULL;
>> +
>> +#define SELFFD_PREFIX "/proc/self/fd/"
>> +  char fd_name[sizeof SELFFD_PREFIX + 12];
> 
> Why 12? Should that be 11 (maxuint digits + 1)?

Just habit, for maxint digits (10) + sign (1) + NIL (1).  But since I
already checked for negative, it doesn't need room for the sign after
all.  Maybe I should just asprintf this too and not worry about it.

>> +  sprintf (fd_name, "%s%d", SELFFD_PREFIX, result->elf->fildes);
>> +
>> +  /* Note, readlink(2) recommends sizing the buffer based on lstat(2), but
>> +     procfs always reports st_size=64, even if it's really longer.  */
>> +  char fd_link[PATH_MAX + 1];
>> +  ssize_t len_link = readlink (fd_name, fd_link, sizeof fd_link);
>> +  if (len_link == -1 || (size_t) len_link > sizeof fd_link - 1)
>> +    return NULL;
>> +  fd_link[len_link] = '\0';
> 
> The PATH_MAX fixed buffer seems somewhat ugly, might not work on really
> large/deep file systems and might depending on PATH_MAX use lots of
> stack. I would just use the glibc canonicalize_file_name (or realpath
> with NULL as second argument) and let that handle the memory and free
> the result below.

Good idea.  I guess this just didn't occur to me because realpath does
more than just links.  AFAICT /proc/self/fd/ targets are already
canonical anyway, so realpath here will simply be like a readlink that
can allocate for us.  Nice.

On using lots of stack, it's fun to note that glibc realpath does an
alloca (PATH_MAX) for its readlinks, up to SYMLOOP_MAX times.  But we
can let them worry about this. :)

>> +# Use head to capture output because the output could be infinite...
>> +testrun ${abs_builddir}/allfcts test-offset-loop | head -n 20 > allfcts.out
>> +testrun_compare cat allfcts.out <allfcts.in
>> +
>> +# Repeat from a different directory for relative .gnu_debugaltlink
>> +(cd / && testrun ${abs_builddir}/allfcts $OLDPWD/test-offset-loop) | head -n 20 > allfcts.out
>> +testrun_compare cat allfcts.out <allfcts.in
> 
> Cute test.

Thanks. :)


Josh