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