perf symbols: Follow .gnu_debuglink section to find separate symbols
PATCH 1/1 perf symbols: Follow .gnu_debuglink section to find separate symbols
The .gnu_debuglink section is specified to contain the filename of the debug info file, as well as a CRC that can be used to validate it.
This doesn't currently use the checksum and relies on the usual build-id matching for validation.
This provides more context:
http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
--- tools/perf/util/symbol.c | 65 +++++++++++++++++++++++++++++++++++++++++++++- tools/perf/util/symbol.h | 1 + 2 files changed, 65 insertions(+), 1 deletion(-)
tools/perf/util/symbol.c +64 -1
@@ -1590,11 +1590,62 @@ out:return err; } +static int filename__read_debuglink (const char *filename, +char *path,size_t size) +{ +int fd, err =-1 ; +Elf *elf; +GElf_Ehdr ehdr; +GElf_Shdr shdr; +Elf_Data *data; +Elf_Scn *sec; +Elf_Kind ek; + + fd =open (filename,O_RDONLY ); +if (fd <0 ) + goto out; + + elf =elf_begin (fd,PERF_ELF_C_READ_MMAP ,NULL ); +if (elf ==NULL ) { +pr_debug2 ("%s: cannot read %s ELF file.\n" , __func__, filename); + goto out_close; + } + + ek =elf_kind (elf); +if (ek !=ELF_K_ELF ) + goto out_close; + +if (gelf_getehdr (elf, &ehdr) ==NULL ) { +pr_err ("%s: cannot get elf header.\n" , __func__); + goto out_close; + } + + sec =elf_section_by_name (elf, &ehdr, &shdr, +".gnu_debuglink" ,NULL ); +if (sec ==NULL ) + goto out_close; + + data =elf_getdata (sec,NULL ); +if (data ==NULL ) + goto out_close; + +/* the start of this section is a zero-terminated string */ +strncpy (path, data->d_buf, size); + +elf_end (elf); + +out_close: +close (fd); +out: +return err; +} +char dso__symtab_origin (const struct dso *dso) {static const char origin[] = { [SYMTAB__KALLSYMS ] ='k' , [SYMTAB__JAVA_JIT ] ='j' , + [SYMTAB__DEBUGLINK ] ='l' , [SYMTAB__BUILD_ID_CACHE ] ='B' , [SYMTAB__FEDORA_DEBUGINFO ] ='f' , [SYMTAB__UBUNTU_DEBUGINFO ] ='u' ,
@@ -1662,10 +1713,22 @@ int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter)*/ want_symtab =1 ; restart: -for (dso->symtab_type =SYMTAB__BUILD_ID_CACHE ; +for (dso->symtab_type =SYMTAB__DEBUGLINK ; dso->symtab_type !=SYMTAB__NOT_FOUND ; dso->symtab_type++) {switch (dso->symtab_type) { +case SYMTAB__DEBUGLINK : { +char *last_slash; +strncpy (name, dso->long_name, size); + last_slash = name + dso->long_name_len; +while (last_slash && *last_slash !='/' ) + last_slash--; +if (last_slash) + last_slash++; +filename__read_debuglink (dso->long_name, last_slash, + size - (last_slash - name)); + } +break ;case SYMTAB__BUILD_ID_CACHE :/* skip the locally configured cache if a symfs is given */ if (symbol_conf.symfs[0] ||
tools/perf/util/symbol.h +1 -0
@@ -257,6 +257,7 @@ enum symtab_type {SYMTAB__KALLSYMS =0 ,SYMTAB__GUEST_KALLSYMS ,SYMTAB__JAVA_JIT , +SYMTAB__DEBUGLINK ,SYMTAB__BUILD_ID_CACHE ,SYMTAB__FEDORA_DEBUGINFO , SYMTAB__UBUNTU_DEBUGINFO,
Signature · 1 line
1.7.9.5
Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
tools/perf/util/symbol.c out: · line 1595
Quoted 10 lines
The .gnu_debuglink section is specified to contain the filename of the debug info file, as well as a CRC that can be used to validate it.
This provides more context:
http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html+++
b/tools/perf/util/symbol.ctools/perf/util/symbol.c +3
-0+static int filename__read_debuglink (const char *filename, +char *path,size_t size) +{
Isn't there any other function that opens an ELF file, looks for an specific session to then read its contents?
Couldn't it be reused here?
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1725
Quoted 6 lines
tools/perf/util/symbol.c +6
-0+case SYMTAB__DEBUGLINK : { +char *last_slash; +strncpy (name, dso->long_name, size); + last_slash = name + dso->long_name_len; +while (last_slash && *last_slash !='/' ) + last_slash--;
Why the test for last_slash to be != NULL? How could it ever be? This is an optimization since we have the dso->long_name_len so that we avoid using strrchr that in turn would do an strlen?
So the test should be:
while (last_slash != name && *last_slash != '/')
To avoid underflowing, right?
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1728
Quoted 3 lines
tools/perf/util/symbol.c +3
-0+if (last_slash) + last_slash++; + filename__read_debuglink(dso->long_name, last_slash,
How last_slash can point to the path? It looks like it points to the basename, no?
Yeah, it is, and then your algorithm will work because last_slash doesn't point to the _path_, but to a string _preceded_ by the path, so for /bin/ls, the debuglink content would be ls.debug and that is what will be stashed there, ending up with:
name = "/bin/\0"
last_slash---^
name = "/bin/ls.debug\0"
I.e. its not really the last slash, but where the debuglink content has to be stashed, concatenating with the same dirname as the associated binary.
Can you please rename "last_slash" to "debuglink"? And "path" to "debuglink" as well in the routine that reads the debuglink.
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1730
Quoted 2 lines
tools/perf/util/symbol.c +2
-0+ size - (last_slash - name)); + }
Other than that, thanks a lot for working on this, surely we have to support this feature!
- ARnaldo
Thanks a lot for taking a look, Arnaldo; inline:
Quoted 15 lines · Arnaldo Carvalho de Melo
Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
tools/perf/util/symbol.c out: · line 1595
Quoted 10 lines
The .gnu_debuglink section is specified to contain the filename of the debug info file, as well as a CRC that can be used to validate it.
This provides more context:
http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html+++
b/tools/perf/util/symbol.ctools/perf/util/symbol.c +3
-0+static int filename__read_debuglink (const char *filename, +char *path,size_t size) +{Isn't there any other function that opens an ELF file, looks for an specific session to then read its contents?
Quoted 1 line
Couldn't it be reused here?
There are plenty, but they're all different enough that trying to fold them into common code seemed involved and risky. The closest looks like elf_read_build_id(), but in order to leverage that one the common function would need to take a list of sections to try in sequence. Unless you have strong feelings against using filename__read_debuglink() as-is I would recommend that any such work happen outside of that patch in the interest of minimizing risk.
Quoted 12 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1725
Quoted 7 lines
tools/perf/util/symbol.c +6
-0+case SYMTAB__DEBUGLINK : { +char *last_slash; +strncpy (name, dso->long_name, size); + last_slash = name + dso->long_name_len; +while (last_slash && *last_slash !='/' ) + last_slash--;Why the test for
last_slashto be != NULL? How could it ever be? This is an optimization since we have thedso->long_name_lenso that we avoid using strrchr that in turn would do an strlen?
Quoted 5 lines
So the test should be:
while (last_slash != name && *last_slash != '/')To avoid underflowing, right?
Oops, yeah; not what I had in mind. Thanks for catching that.
Quoted 12 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1728
Quoted 3 lines
tools/perf/util/symbol.c +3
-0+if (last_slash) + last_slash++; + filename__read_debuglink(dso->long_name, last_slash,How
last_slashcan point to the path? It looks like it points to the basename, no?Yeah, it is, and then your algorithm will work because
last_slashdoesn't point to the _path_, but to a string _preceded_ by the path, so for/bin/ls, the debuglink content would be ls.debug and that is what will be stashed there, ending up with:
Quoted 7 lines
name = "/bin/\0"
last_slash---^
name = "/bin/ls.debug\0"I.e. its not really the last slash, but where the debuglink content has to be stashed, concatenating with the same dirname as the associated binary.
Yes, that's correct; sorry for the ill-named variables.
Quoted 3 lines
Can you please rename "
last_slash" to "debuglink"? And "path" to "debuglink" as well in the routine that reads the debuglink.
Done. I'm proposing to resend with:
while (debuglink != name && *debuglink != '/') debuglink--; if (*debuglink == '/') debuglink++;
The underflow and subsequent "*debuglink == '/'" checks are only there to do the right thing in case dso->long_name happens to just be a local filename without any slashes in it. I realize that's probably never supposed to happen, so I could remove them if you want. I'd rather err on the side of caution, though.
Quoted 6 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1730
Quoted 3 lines
tools/perf/util/symbol.c +2
-0+ size - (last_slash - name)); + }Other than that, thanks a lot for working on this, surely we have to support this feature!
Thanks a lot for walking through the logic. Looking forward to your comments before I resend!
- Pierre-Loup
Quoted 2 lines
- ARnaldo
Em Thu, Jun 21, 2012 at 11:41:05AM -0700, Pierre-Loup A. Griffais escreveu:
Quoted 11 lines · Arnaldo Carvalho de Melo
Quoted 9 lines
Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
Quoted 5 lines
The .gnu_debuglink section is specified to contain the filename of the
http://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
+static int filename__read_debuglink(const char *filename,
Isn't there any other function that opens an ELF file, looks for an specific session to then read its contents?
Quoted 6 lines
Quoted 1 line
Couldn't it be reused here?
There are plenty, but they're all different enough that trying to fold them into common code seemed involved and risky. The closest looks like
elf_read_build_id(), but in order to leverage that one the common function would need to take a list of sections to try in sequence.
But we can have helper routines to do what is done to open the elf file, etc, right?
Quoted 3 lines
Unless you have strong feelings against using
filename__read_debuglink()as-is I would recommend that any such work happen outside of that patch in the interest of minimizing risk.
Agreed, that could be done later.
Quoted 50 lines
Quoted 11 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1725
Quoted 6 lines
tools/perf/util/symbol.c +6
-0+case SYMTAB__DEBUGLINK : { +char *last_slash; +strncpy (name, dso->long_name, size); + last_slash = name + dso->long_name_len; +while (last_slash && *last_slash !='/' ) + last_slash--;Why the test for
last_slashto be != NULL? How could it ever be? This is an optimization since we have thedso->long_name_lenso that we avoid using strrchr that in turn would do an strlen?Quoted 4 lines
So the test should be:
while (last_slash != name && *last_slash != '/')To avoid underflowing, right?
Oops, yeah; not what I had in mind. Thanks for catching that.
Quoted 12 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1728
Quoted 3 lines
tools/perf/util/symbol.c +3
-0+if (last_slash) + last_slash++; + filename__read_debuglink(dso->long_name, last_slash,How
last_slashcan point to the path? It looks like it points to the basename, no?Yeah, it is, and then your algorithm will work because
last_slashdoesn't point to the _path_, but to a string _preceded_ by the path, so for/bin/ls, the debuglink content would be ls.debug and that is what will be stashed there, ending up with:Quoted 7 lines
name = "/bin/\0"
last_slash---^
name = "/bin/ls.debug\0"I.e. its not really the last slash, but where the debuglink content has to be stashed, concatenating with the same dirname as the associated binary.
Yes, that's correct; sorry for the ill-named variables.
Quoted 2 lines
Can you please rename "
last_slash" to "debuglink"? And "path" to "debuglink" as well in the routine that reads the debuglink.Done. I'm proposing to resend with:
while (debuglink != name && *debuglink != '/') debuglink--; if (*debuglink == '/') debuglink++;
Quoted 5 lines
The underflow and subsequent "*debuglink == '/'" checks are only there to do the right thing in case
dso->long_namehappens to just be a local filename without any slashes in it. I realize that's probably never supposed to happen, so I could remove them if you want. I'd rather err on the side of caution, though.
That is the right mindset, code has to be robust.
Quoted 8 lines
Quoted 5 lines
tools/perf/util/symbol.c int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) · line 1730
Quoted 2 lines
tools/perf/util/symbol.c +2
-0+ size - (last_slash - name)); + }Other than that, thanks a lot for working on this, surely we have to support this feature!
Thanks a lot for walking through the logic. Looking forward to your comments before I resend!
Please resend! :-)
- Arnaldo