[PATCH 0/1] Support .gnu_debugdata for symbols in perf

[PATCH 0/1] Support .gnu_debugdata for symbols in perf

[Stephen Brennan]

Hello all,

A while back, Fedora implemented a "MiniDebugInfo" feature[1]. This allows the
full symbol table of a binary to be compressed into a fake ELF file and embedded
within the ".gnu_debugdata" section. The standard RPMs thus include the full
symbol information, at least enough to provide symbol names for stack traces. As
part of the project, support for .gnu_debugdata was implemented in GDB. However,
perf does not yet have support for it.

For Fedora, RHEL, and all the derivatives, there is already the ".eh_frame"
section in ELF files, which allows perf to unwind stacks with "--call-graph
dwarf", and in some cases, there may be frame pointers to make it even easier
and more efficient to gather stack traces.

But once the data is recorded, perf may still be missing the symbol names for
many functions, because it doesn't have support for .gnu_debugdata.

This patch is a first stab at implementing that support. To demonstrate, here is
a diff between a sample output by "perf script" before and after the change:

@@ -16,9 +16,9 @@
            55fdc45c837f rl_read_key+0x17f (/usr/bin/bash)
            55fdc45a744b readline_internal_char+0x6b (/usr/bin/bash)
            55fdc45a776c readline+0x4c (/usr/bin/bash)
-           55fdc4509dbe [unknown] (/usr/bin/bash)
-           55fdc450ce28 [unknown] (/usr/bin/bash)
-           55fdc450ebb9 [unknown] (/usr/bin/bash)
+           55fdc4509dbe yy_readline_get+0xae (/usr/bin/bash)
+           55fdc450ce28 shell_getc.lto_priv.0+0x278 (/usr/bin/bash)
+           55fdc450ebb9 read_token.part.0+0x49 (/usr/bin/bash)
            55fdc4512739 yyparse+0x3f9 (/usr/bin/bash)
            55fdc4515bbc parse_command+0x3c (/usr/bin/bash)
            55fdc4515d8c read_command+0x6c (/usr/bin/bash)

I believe this support will be even more important in the future, if and when
SFrame becomes supported: then we will have an efficient unwinder that doesn't
require debuginfo to be installed. But users won't be able to take full
advantage of their perf recordings without having the best symbol information
available.

My approach almost certainly can be improved: I'm not sure whether there is a
strong preference to decompress the ".gnu_debugdata" into a temporary file, or
if keeping it in-memory is good enough. I'd love feedback, since I know this
approach is a bit rough around the edges.

Thanks,
Stephen

[1]: https://fedoraproject.org/wiki/Features/MiniDebugInfo

Stephen Brennan (1):
  tools: perf: support .gnu_debugdata for symbols

 tools/perf/util/dso.c        |   2 +
 tools/perf/util/dso.h        |   1 +
 tools/perf/util/symbol-elf.c | 141 ++++++++++++++++++++++++++++++++++-
 tools/perf/util/symbol.c     |   2 +
 tools/perf/util/symsrc.h     |   1 +
 5 files changed, 143 insertions(+), 4 deletions(-)

-- 
2.43.5

[PATCH 1/1] tools: perf: support .gnu_debugdata for symbols

[Stephen Brennan]

Fedora introduced a "MiniDebuginfo" feature, in which an LZMA-compressed
ELF file is placed inside a section named ".gnu_debugdata". This file
contains nothing but a symbol table, which can be used to supplement the
.dynsym section which only contains required symbols for runtime.

It is supported by GDB for stack traces, but it should be useful for
tracing as well. Implement support for loading symbols from
.gnu_debugdata.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 tools/perf/util/dso.c        |   2 +
 tools/perf/util/dso.h        |   1 +
 tools/perf/util/symbol-elf.c | 141 ++++++++++++++++++++++++++++++++++-
 tools/perf/util/symbol.c     |   2 +
 tools/perf/util/symsrc.h     |   1 +
 5 files changed, 143 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5c6e85fdae0de..67c21999628d4 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -93,6 +93,7 @@ bool dso__is_object_file(const struct dso *dso)
 	case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
 	case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
 	case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
+	case DSO_BINARY_TYPE__GNU_DEBUGDATA:
 	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
 	case DSO_BINARY_TYPE__GUEST_KMODULE:
 	case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
@@ -224,6 +225,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 	case DSO_BINARY_TYPE__VMLINUX:
 	case DSO_BINARY_TYPE__GUEST_VMLINUX:
 	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
+	case DSO_BINARY_TYPE__GNU_DEBUGDATA:
 		__symbol__join_symfs(filename, size, dso__long_name(dso));
 		break;
 
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index bb8e8f444054d..84d5aac666aa1 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -33,6 +33,7 @@ enum dso_binary_type {
 	DSO_BINARY_TYPE__UBUNTU_DEBUGINFO,
 	DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO,
 	DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
+	DSO_BINARY_TYPE__GNU_DEBUGDATA,
 	DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
 	DSO_BINARY_TYPE__GUEST_KMODULE,
 	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 66fd1249660a3..e578b7d406a69 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -6,6 +6,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <inttypes.h>
+#include <lzma.h>
 
 #include "dso.h"
 #include "map.h"
@@ -1214,7 +1215,9 @@ void symsrc__destroy(struct symsrc *ss)
 {
 	zfree(&ss->name);
 	elf_end(ss->elf);
-	close(ss->fd);
+	free(ss->image);
+	if (ss->fd != -1)
+		close(ss->fd);
 }
 
 bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
@@ -1228,12 +1231,115 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
 	       ehdr.e_type == ET_DYN;
 }
 
+static Elf *read_gnu_debugdata(struct dso *dso, Elf *elf, const char *name, void **image_ret)
+{
+	Elf *elf_embedded;
+	GElf_Ehdr ehdr;
+	GElf_Shdr shdr;
+	Elf_Scn *scn;
+	Elf_Data *scn_data;
+	void *data = NULL;
+	size_t shndx, datasize, bytes_decoded;
+	lzma_stream stream = LZMA_STREAM_INIT;
+	lzma_ret ret;
+
+	if (gelf_getehdr(elf, &ehdr) == NULL) {
+		pr_debug("%s: cannot read %s ELF file.\n", __func__, name);
+		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+		return NULL;
+	}
+
+	scn = elf_section_by_name(elf, &ehdr, &shdr, ".gnu_debugdata", &shndx);
+	if (!scn) {
+		pr_debug("%s: ELF file %s has no .gnu_debugdata\n", __func__, name);
+		*dso__load_errno(dso) = -ENOENT;
+		return NULL;
+	}
+
+	if (shdr.sh_type == SHT_NOBITS) {
+		pr_debug("%s: .gnu_debugdata of ELF file %s has no data.\n", __func__, name);
+		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+		return NULL;
+	}
+
+	scn_data = elf_rawdata(scn, NULL);
+	if (!scn_data) {
+		pr_debug("%s: error reading .gnu_debugdata of %s: %s\n", __func__,
+			 name, elf_errmsg(-1));
+		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+		return NULL;
+	}
+
+	ret = lzma_stream_decoder(&stream, UINT64_MAX, 0);
+	if (ret != LZMA_OK) {
+		pr_debug("%s: error decompressing .gnu_debugdata of %s: %d\n", __func__,
+			 name, ret);
+		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+		lzma_end(&stream);
+		return NULL;
+	}
+
+	stream.next_in = scn_data->d_buf;
+	stream.avail_in = scn_data->d_size;
+
+	datasize = 2 * scn_data->d_size;
+	data = malloc(datasize);
+
+	stream.next_out = data;
+	stream.avail_out = scn_data->d_size;
+	while (1) {
+		void *data_resized;
+
+		ret = lzma_code(&stream, LZMA_RUN);
+		if (ret != LZMA_OK && ret != LZMA_STREAM_END) {
+			pr_debug("%s: error decompressing .gnu_debugdata of %s: %d\n", __func__,
+				name, ret);
+			*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+			free(data);
+			lzma_end(&stream);
+			return NULL;
+		}
+		bytes_decoded = (void *)stream.next_out - data;
+		if (ret == LZMA_STREAM_END)
+			datasize = bytes_decoded;
+		else
+			datasize *= 2;
+		data_resized = realloc(data, datasize);
+		if (!data_resized) {
+			pr_debug("%s: allocation error accessing .gnu_debugdata of %s\n",
+				 __func__, name);
+			*dso__load_errno(dso) = -ENOMEM;
+			free(data);
+			lzma_end(&stream);
+			return NULL;
+		}
+		data = data_resized;
+		stream.next_out = data + bytes_decoded;
+		stream.avail_out = datasize - bytes_decoded;
+		if (ret == LZMA_STREAM_END)
+			break;
+	}
+	lzma_end(&stream);
+
+	elf_embedded = elf_memory(data, bytes_decoded);
+	if (!elf_embedded) {
+		pr_debug("%s: error reading .gnu_debugdata of %s: %s\n", __func__,
+			 name, elf_errmsg(-1));
+		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
+		free(data);
+		return NULL;
+	}
+	*image_ret = data;
+	return elf_embedded;
+}
+
 int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 		 enum dso_binary_type type)
 {
 	GElf_Ehdr ehdr;
 	Elf *elf;
 	int fd;
+	ss->image = NULL;
 
 	if (dso__needs_decompress(dso)) {
 		fd = dso__decompress_kmodule_fd(dso, name);
@@ -1256,6 +1362,18 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 		goto out_close;
 	}
 
+	if (type == DSO_BINARY_TYPE__GNU_DEBUGDATA) {
+		Elf *embedded = read_gnu_debugdata(dso, elf, name, &ss->image);
+
+		if (!embedded)
+			goto out_close;
+
+		elf_end(elf);
+		close(fd);
+		fd = -1;
+		elf = embedded;
+	}
+
 	if (gelf_getehdr(elf, &ehdr) == NULL) {
 		*dso__load_errno(dso) = DSO_LOAD_ERRNO__INVALID_ELF;
 		pr_debug("%s: cannot get elf header.\n", __func__);
@@ -1328,7 +1446,9 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 out_elf_end:
 	elf_end(elf);
 out_close:
-	close(fd);
+	if (fd != -1)
+		close(fd);
+	free(ss->image);
 	return -1;
 }
 
@@ -1854,10 +1974,23 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 					     kmodule, 1);
 		if (err < 0)
 			return err;
-		err += nr;
+		nr += err;
 	}
 
-	return err;
+	/*
+	 * The .gnu_debugdata is a special situation: it contains a symbol
+	 * table, but the runtime file may also contain dynsym entries which are
+	 * not present there. We need to load both.
+	 */
+	if (syms_ss->type == DSO_BINARY_TYPE__GNU_DEBUGDATA && runtime_ss->dynsym) {
+		err = dso__load_sym_internal(dso, map, runtime_ss, runtime_ss,
+					     kmodule, 1);
+		if (err < 0)
+			return err;
+		nr += err;
+	}
+
+	return nr;
 }
 
 static int elf_read_maps(Elf *elf, bool exe, mapfn_t mapfn, void *data)
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 49b08adc6ee34..a0767762d4d73 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -84,6 +84,7 @@ static enum dso_binary_type binary_type_symtab[] = {
 	DSO_BINARY_TYPE__FEDORA_DEBUGINFO,
 	DSO_BINARY_TYPE__UBUNTU_DEBUGINFO,
 	DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
+	DSO_BINARY_TYPE__GNU_DEBUGDATA,
 	DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
 	DSO_BINARY_TYPE__GUEST_KMODULE,
 	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
@@ -1716,6 +1717,7 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
 	case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
 	case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
 	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
+	case DSO_BINARY_TYPE__GNU_DEBUGDATA:
 		return !kmod && dso__kernel(dso) == DSO_SPACE__USER;
 
 	case DSO_BINARY_TYPE__KALLSYMS:
diff --git a/tools/perf/util/symsrc.h b/tools/perf/util/symsrc.h
index edf82028c9e62..38b628f12aaa5 100644
--- a/tools/perf/util/symsrc.h
+++ b/tools/perf/util/symsrc.h
@@ -15,6 +15,7 @@
 struct symsrc {
 	char		     *name;
 	int		     fd;
+	void		     *image;
 	enum dso_binary_type type;
 
 #ifdef HAVE_LIBELF_SUPPORT
-- 
2.43.5

Re: [PATCH 1/1] tools: perf: support .gnu_debugdata for symbols

[Andi Kleen]

Stephen Brennan <stephen.s.brennan@oracle.com> writes:
>  	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 66fd1249660a3..e578b7d406a69 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -6,6 +6,7 @@
>  #include <string.h>
>  #include <unistd.h>
>  #include <inttypes.h>
> +#include <lzma.h>

This needs some ifdefs in case lzma.h is not available?

-Andi

Re: [PATCH 1/1] tools: perf: support .gnu_debugdata for symbols

[Namhyung Kim]

Hello,

On Fri, Feb 14, 2025 at 11:30:26AM -0800, Andi Kleen wrote:
> Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> >  	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index 66fd1249660a3..e578b7d406a69 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -6,6 +6,7 @@
> >  #include <string.h>
> >  #include <unistd.h>
> >  #include <inttypes.h>
> > +#include <lzma.h>
> 
> This needs some ifdefs in case lzma.h is not available?

Right, should be guarded by HAVE_LZMA_SUPPORT.

Thanks,
Namhyung

Re: [PATCH 1/1] tools: perf: support .gnu_debugdata for symbols

[Stephen Brennan]

Namhyung Kim <namhyung@kernel.org> writes:
> Hello,
>
> On Fri, Feb 14, 2025 at 11:30:26AM -0800, Andi Kleen wrote:
>> Stephen Brennan <stephen.s.brennan@oracle.com> writes:
>> >  	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
>> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> > index 66fd1249660a3..e578b7d406a69 100644
>> > --- a/tools/perf/util/symbol-elf.c
>> > +++ b/tools/perf/util/symbol-elf.c
>> > @@ -6,6 +6,7 @@
>> >  #include <string.h>
>> >  #include <unistd.h>
>> >  #include <inttypes.h>
>> > +#include <lzma.h>
>> 
>> This needs some ifdefs in case lzma.h is not available?
>
> Right, should be guarded by HAVE_LZMA_SUPPORT.

Thank you both. Do you happen to have a preference on whether to do this
inline with the HAVE_LZMA_SUPPORT guards, or whether to use a helper
like lzma_decompress_to_file() with a temporary file (presumably one
which is already unlinked...)?

I suppose the more I ask the question, the more it feels obvious that an
unlinked temporary file would require less new code. Both because I
could use the existing helper, and also because there would be no need
to free() the data which we decompressed into memory. So unless you have
strong opinions I guess I'll switch it over :)

Thanks,
Stephen

Re: [PATCH 1/1] tools: perf: support .gnu_debugdata for symbols

[Namhyung Kim]

On Wed, Feb 19, 2025 at 01:57:25PM -0800, Stephen Brennan wrote:
> Namhyung Kim <namhyung@kernel.org> writes:
> > Hello,
> >
> > On Fri, Feb 14, 2025 at 11:30:26AM -0800, Andi Kleen wrote:
> >> Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> >> >  	DSO_BINARY_TYPE__GUEST_KMODULE_COMP,
> >> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> >> > index 66fd1249660a3..e578b7d406a69 100644
> >> > --- a/tools/perf/util/symbol-elf.c
> >> > +++ b/tools/perf/util/symbol-elf.c
> >> > @@ -6,6 +6,7 @@
> >> >  #include <string.h>
> >> >  #include <unistd.h>
> >> >  #include <inttypes.h>
> >> > +#include <lzma.h>
> >> 
> >> This needs some ifdefs in case lzma.h is not available?
> >
> > Right, should be guarded by HAVE_LZMA_SUPPORT.
> 
> Thank you both. Do you happen to have a preference on whether to do this
> inline with the HAVE_LZMA_SUPPORT guards, or whether to use a helper
> like lzma_decompress_to_file() with a temporary file (presumably one
> which is already unlinked...)?
> 
> I suppose the more I ask the question, the more it feels obvious that an
> unlinked temporary file would require less new code. Both because I
> could use the existing helper, and also because there would be no need
> to free() the data which we decompressed into memory. So unless you have
> strong opinions I guess I'll switch it over :)

Should be ok to use the existing helper but I guess you still need to
call close() once it's done with the data.  Also I think it's better
to add dummy functions in the compress.h when LZMA is not supported so
that the symbol code can be unaware of the configurations.

Thanks,
Namhyung