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

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

Details

Reviewers

jhb
emaste
brooks
imp
kaiw

Commits

rS345594: Stop using -fdebug-prefix-map to map the object directory.
rS345593: Prepend DW_AT_comp_dir to relative line number directory table entries.

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 23297
Build 22329: arc lint + arc unit

Event Timeline

markj created this revision.Mar 25 2019, 9:22 PM

Harbormaster completed remote builds in B23295: Diff 55439.Mar 25 2019, 9:22 PM

markj edited the test plan for this revision. (Show Details)Mar 25 2019, 9:24 PM

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.

jhb added inline comments.Mar 25 2019, 9:53 PM

contrib/elftoolchain/libdwarf/libdwarf_lineno.c
70	While I probably prefer the '[0]' syntax, the existing style on line 58 seems to want `if (*incdir == '/')` instead.
71	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).
77	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)

Use open_memstream(3) instead.

Harbormaster completed remote builds in B23297: Diff 55442.Mar 25 2019, 11:48 PM

markj added inline comments.Mar 25 2019, 11:48 PM

contrib/elftoolchain/libdwarf/libdwarf_lineno.c
71	I think the version using open_memstream() is much easier to read, so I went ahead and used it.
77	Right, we prepend compdir if: the incdir index is 0, which means "use DW_AT_comp_dir," or the incdir is relative.