[PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

[PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

[Pierre-Loup A. Griffais]

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

Signed-off-by: Pierre-Loup A. Griffais <pgriffais@nvidia.com>
Reported-by: Mike Sartain <mikesart@valvesoftware.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 tools/perf/util/symbol.c |   65 +++++++++++++++++++++++++++++++++++++++++++++-
 tools/perf/util/symbol.h |    1 +
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 3e2e5ea..4788092 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -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] ||
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index af0752b..a884b99 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -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,
-- 
1.7.9.5

Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

[Arnaldo Carvalho de Melo]

Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
> 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.c
> +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?

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

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

> +						 size - (last_slash - name));
> +			}

Other than that, thanks a lot for working on this, surely we have to
support this feature!

- ARnaldo

Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

[Pierre-Loup A. Griffais]

Thanks a lot for taking a look, Arnaldo; inline:

On 06/21/2012 10:28 AM, Arnaldo Carvalho de Melo wrote:

> Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
>> 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.c
>> +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?


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.

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


Oops, yeah; not what I had in mind. Thanks for catching that.


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


Yes, that's correct; sorry for the ill-named variables.

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

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

> 
> - ARnaldo

Re: [PATCH] perf symbols: Follow .gnu_debuglink section to find separate symbols

[Arnaldo Carvalho de Melo]

Em Thu, Jun 21, 2012 at 11:41:05AM -0700, Pierre-Loup A. Griffais escreveu:
> On 06/21/2012 10:28 AM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jun 20, 2012 at 03:22:41PM -0700, Pierre-Loup A. Griffais escreveu:
> >> 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?

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

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

> >> +		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?
> 
> Oops, yeah; not what I had in mind. Thanks for catching that.
> 
> >> +			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.
> 
> Yes, that's correct; sorry for the ill-named variables.
> 
> > 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.

That is the right mindset, code has to be robust.
 
> >> +						 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