Page MenuHomeFreeBSD

Avoid mapping the current directory with -fdebug-prefix-map.
ClosedPublic

Authored by markj on Mar 25 2019, 9:22 PM.

Details

Summary

I added those maps to work around what turned out to be a deficiency in
elftoolchain libdwarf which manifested in addr2line's use of
dwarf_srclines(3). This change fixes the deficiency and removes the
mapping.

Basically, each CU contains a table of file names (mostly headers)
referenced in the associated line number information. Each entry in the
file name table references an entry in the directory table;
concatenating the directory and file strings may give an absolute path
or a relative path. The CU also contains a DW_AT_comp_dir attribute,
the compilation directory.

Consider an autogenerated header, vnode_if.h, referenced by a CU.
Without the debug map for ".", there exists a file table entry,
"vnode_if.h", which references "." since that was the include path used
to find vnode_if.h. Thus, libdwarf internally caches "./vnode_if.h",
and if any PCs map to lines in that file, addr2line prints
"./vnode_if.h". Binutils addr2line on the other hand prints
"$OBJDIR/./vnode_if.h". That is, it prepends the compilation directory
to a relative path in the directory table, which I think is a reasonable
thing to do. The change just implements this logic.

Test Plan

Before:

$ addr2line -afi -e vfs_subr.o 0x00000d6d                                                                                        
0x0000000000000d6d                                                                                                                           
VOP_LOCK1                                                                                                             
./vnode_if.h:854                                                                                                                             
vtryrecycle                                             
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1278                              
vnlru_free_locked                                                                                                      
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1042

After:

$ echo 0x00000d6d | addr2line -afi -e vfs_subr.o                                                                                 
0x0000000000000d6d
VOP_LOCK1
/home/mark/src/freebsd-obj/home/mark/src/freebsd-dev/amd64.amd64/sys/MARKJ-NODEBUG/./vnode_if.h:854                                          
vtryrecycle
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1278
vnlru_free_locked
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1042

Binutils:

$ echo 0x00000d6d | /usr/local/bin/addr2line -afi -e vfs_subr.o                                                                  
0x0000000000000d6d
VOP_LOCK1
/home/mark/src/freebsd-obj/home/mark/src/freebsd-dev/amd64.amd64/sys/MARKJ-NODEBUG/./vnode_if.h:854                                          
vtryrecycle
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1278
vnlru_free_locked
/home/mark/src/freebsd-dev/sys/kern/vfs_subr.c:1042

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj added reviewers: jhb, emaste, brooks, imp.
markj edited the test plan for this revision. (Show Details)
markj added a reviewer: kaiw.
markj added a subscriber: tuexen.
contrib/elftoolchain/libdwarf/libdwarf_lineno.c
62 ↗(On Diff #55439)

While I probably prefer the '[0]' syntax, the existing style on line 58 seems to want if (*incdir == '/') instead.

78 ↗(On Diff #55439)

Something like asprintf(3) sure would make these changes smaller and easier to read. I guess asprintf(3) isn't portable enough? open_memstream() is probably just as many lines of code as what is there now (perhaps even more) so not worth doing (though see my comment below which isn't a straight replacement).

84 ↗(On Diff #55439)

So if I'm reading this right, it will always prepend the build path, including if the debug map is a relative directory (such as for 'machine' and 'x86' symlinks)? Using a string builder like open_memstream would let you perhaps do this more incrementally, something like:

if (*lf->lf_fname != '/') {
    size_t dummy;
    fp = open_memstream(&lf->lf_fullpath, &dummy);
    if (fp == NULL) {
        /* error */
    }
    if (lf->lf_dirndx > 0)
        incdir = li->li_incdirs[lf->lf_dirndx - 1];
    else
        incdir = NULL;

    /* Prepend the compile directory unless the include directory is an absolute path. */
    if (incdir == NULL || *incdir != '/')
        fprintf(fp, "%s/", compdir);
    if (incdir != NULL)
        fprintf(fp, "%s/", incdir);
    fprintf(fp, "%s", lf->lf_fname);
    if (fclose(fp) != 0) {
        /* error */
    }

I guess this doesn't really reduce the amount of error handling code since both fopen() and fclose() have to be checked. :-/

I seem to recall a report of this somewhere but can't locate it now (in ELF Tool Chain tickets or bugzilla)

markj marked an inline comment as done.

Use open_memstream(3) instead.

contrib/elftoolchain/libdwarf/libdwarf_lineno.c
78 ↗(On Diff #55439)

I think the version using open_memstream() is much easier to read, so I went ahead and used it.

84 ↗(On Diff #55439)

Right, we prepend compdir if:

  1. the incdir index is 0, which means "use DW_AT_comp_dir," or
  2. the incdir is relative.
This revision is now accepted and ready to land.Mar 25 2019, 11:56 PM

fine with me, although I'd prefer the elftoolchain change and the build changes committed separately

fine with me, although I'd prefer the elftoolchain change and the build changes committed separately

Yeah, I wasn't going to commit them together. Meant to note that in the description.

Committing to src is ok? Or did you want a PR on github, or both?

This revision was automatically updated to reflect the committed changes.