[PATCH] libdw: Try .gnu_debugaltlink paths relative to the debug file
Josh Stone
jistone at redhat.com
Mon Dec 16 19:16:47 UTC 2013
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
More information about the elfutils-devel
mailing list