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

[Mark Wielaard <mjw at redhat dot com>]

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

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.

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

> +/* 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)?

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

> +# 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,

Mark