Re: [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section

From: Arnaldo Carvalho de Melo
Date: Fri Sep 04 2020 - 15:03:48 EST

Next message: Andrii Nakryiko: "Re: [PATCH bpf-next v2 1/6] bpf: Introduce pseudo_btf_id"
Previous message: Linus Torvalds: "Re: [git pull] drm fixes for 5.9-rc4"
In reply to: Remi Bernon: "Re: [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Fri, Sep 04, 2020 at 09:57:33AM +0200, Remi Bernon escreveu:
> On 2020-09-03 18:51, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Sep 02, 2020 at 09:25:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Aug 21, 2020 at 06:52:36PM +0200, Remi Bernon escreveu:
> > > > Wine generates PE binaries for most of its modules and perf is unable
> > > > to parse these files to get build_id or .gnu_debuglink section.
> > > >
> > > > Using libbfd when available, instead of libelf, makes it possible to
> > > > resolve debug file location regardless of the dso binary format.
> > > >
> > > > Signed-off-by: Remi Bernon <rbernon@xxxxxxxxxxxxxxx>
> > > > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > > > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > Cc: Jacek Caban <jacek@xxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > v3: Rebase and small changes to PATCH 2/3 and and PATCH 3/3.
> > > >
> > > > tools/perf/util/symbol-elf.c | 80 ++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 77 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > > > index 8cc4b0059fb0..f7432c4a4154 100644
> > > > --- a/tools/perf/util/symbol-elf.c
> > > > +++ b/tools/perf/util/symbol-elf.c
> > > > @@ -50,6 +50,10 @@ typedef Elf64_Nhdr GElf_Nhdr;
> > > > #define DMGL_ANSI (1 << 1) /* Include const, volatile, etc */
> > > > #endif
> > > > +#ifdef HAVE_LIBBFD_SUPPORT
> > >
> > > So, the feature test should also test for the buildid struct field, see
> > > below:
> > >
> > > > +#define PACKAGE 'perf'
> > > > +#include <bfd.h>
> > > > +#else
> > > > #ifdef HAVE_CPLUS_DEMANGLE_SUPPORT
> > > > extern char *cplus_demangle(const char *, int);
> > > > @@ -65,9 +69,7 @@ static inline char *bfd_demangle(void __maybe_unused *v,
> > > > {
> > > > return NULL;
> > > > }
> > > > -#else
> > > > -#define PACKAGE 'perf'
> > > > -#include <bfd.h>
> > > > +#endif
> > > > #endif
> > > > #endif
> > > > @@ -530,6 +532,36 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
> > > > return err;
> > > > }
> > > > +#ifdef HAVE_LIBBFD_SUPPORT
> > > > +
> > > > +int filename__read_build_id(const char *filename, void *bf, size_t size)
> > > > +{
> > > > + int err = -1;
> > > > + bfd *abfd;
> > > > +
> > > > + abfd = bfd_openr(filename, NULL);
> > > > + if (!abfd)
> > > > + return -1;
> > > > +
> > > > + if (!bfd_check_format(abfd, bfd_object)) {
> > > > + pr_debug2("%s: cannot read %s bfd file.\n", __func__, filename);
> > > > + goto out_close;
> > > > + }
> > > > +
> > > > + if (!abfd->build_id || abfd->build_id->size > size)
> > > > + goto out_close;
> > >
> > > amazonlinux:1, centos:6, debian:8, mageia:5, oraclelinux:6, ubuntu:14.04
> > > fail, its all old stuff, but adding a reference to abfd->build_id to the
> > > feature test that ends up defining HAVE_LIBBFD_SUPPORT will solve that,
> > > I'll do it tomorrow morning if you don't beat me to it.
> > >
> > > util/symbol-elf.c: In function 'filename__read_build_id':
> > > util/symbol-elf.c:551:11: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > if (!abfd->build_id || abfd->build_id->size > size)
> > > ^~
> > > util/symbol-elf.c:551:29: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > if (!abfd->build_id || abfd->build_id->size > size)
> > > ^~
> > > util/symbol-elf.c:554:17: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > memcpy(bf, abfd->build_id->data, abfd->build_id->size);
> > > ^~
> > > util/symbol-elf.c:554:39: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > memcpy(bf, abfd->build_id->data, abfd->build_id->size);
> > > ^~
> > > util/symbol-elf.c:555:18: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > memset(bf + abfd->build_id->size, 0, size - abfd->build_id->size);
> > > ^~
> > > util/symbol-elf.c:555:50: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > memset(bf + abfd->build_id->size, 0, size - abfd->build_id->size);
> > > ^~
> > > util/symbol-elf.c:556:12: error: 'bfd {aka struct bfd}' has no member named 'build_id'
> > > err = abfd->build_id->size;
> > > ^~
> > > CC /tmp/build/perf/util/cap.o
> > > make[4]: *** [/tmp/build/perf/util/symbol-elf.o] Error 1
> > > make[4]: *** Waiting for unfinished jobs....
> > > LD /tmp/build/perf/util/scripting-engines/perf-in.o
> > > LD /tmp/build/perf/util/intel-pt-decoder/perf-in.o
> > > make[3]: *** [util] Error 2
> > > make[2]: *** [/tmp/build/perf/perf-in.o] Error 2
> > > make[2]: *** Waiting for unfinished jobs....
> > > make[1]: *** [sub-make] Error 2
> > > make: *** [all] Error 2
> > > make: Leaving directory `/git/linux/tools/perf'
> > > + exit 1
> > > [perfbuilder@five ~]$
> >
> > So, I have the cset at the end of this message in front of your series +
> > the following patch applied to your patch, the debuglink part seems ok
> > and can continue depending on just libbfd, having or not abfd->buildid.
> >
>
> Thanks! I may have missed the first changeset you mention -- IIUC the one
> adding the feature check -- but it sounds good to me nonetheless.
>
> Should I do anything?

No need at this time, here is the new feature test that ends up being
used to check if that abfd->build_id is present:

[acme@five perf]$ git log torvalds/master.. --oneline
d39497a94c1672c2 (HEAD -> perf/core) perf: ftrace: Add filter support for option -F/--funcs
6330d59303482407 perf tools: Consolidate close_control_option()'s into one function
bd0c035c66c933ee perf intel-pt: Document snapshot control command
7f5b197269a34f9a perf annotate: Add 'ret' (intel disasm style) as an alias for 'retq'
2481b0089bddb6b6 perf annotate: Allow configuring the 'disassembler_style' knob via 'perf config'
d20aff1512f013fa (quaco/perf/core) perf record: Add 'snapshot' control command
a8fcbd269b4340c3 perf tools: Add FIFO file names as alternative options to --control
1f4390d825cc04e9 perf tools: Use AsciiDoc formatting for --control option documentation
40db8ff59e75af10 perf tools: Handle read errors from ctl_fd
9864a66defeb8df0 perf tools: Consolidate --control option parsing into one function
ed21d6d7c48e6e96 perf tests: Add test for PE binary format support
eac9a4342e5447ca perf symbols: Try reading the symbol table with libbfd
ba0509dcb7f80640 perf dso: Use libbfd to read build_id and .gnu_debuglink section
e71e19a9ea70952a tools features: Add feature test to check if libbfd has buildid support
[acme@five perf]$

$ git show e71e19a9ea70952a
commit e71e19a9ea70952a53d58a99971820ce6c1794a8
Author: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Date: Thu Sep 3 13:44:39 2020 -0300

tools features: Add feature test to check if libbfd has buildid support

Which is needed by the PE executable support, for instance.

Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Cc: Ian Rogers <irogers@xxxxxxxxxx>
Cc: Jacek Caban <jacek@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Mark Rutland <mark.rutland@xxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Remi Bernon <rbernon@xxxxxxxxxxxxxxx>
Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>

diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index c1daf4d57518c579..8b381d8ec9ded098 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -41,6 +41,7 @@ FEATURE_TESTS_BASIC := \
gtk2 \
gtk2-infobar \
libbfd \
+ libbfd-buildid \
libcap \
libelf \
libelf-getphdrnum \
@@ -113,6 +114,7 @@ FEATURE_DISPLAY ?= \
glibc \
gtk2 \
libbfd \
+ libbfd-buildid \
libcap \
libelf \
libnuma \
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index d220fe952747053a..9e5f8db4a1689832 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -15,6 +15,7 @@ FILES= \
test-hello.bin \
test-libaudit.bin \
test-libbfd.bin \
+ test-libbfd-buildid.bin \
test-disassembler-four-args.bin \
test-reallocarray.bin \
test-libbfd-liberty.bin \
@@ -229,6 +230,9 @@ $(OUTPUT)test-libpython-version.bin:
$(OUTPUT)test-libbfd.bin:
$(BUILD) -DPACKAGE='"perf"' -lbfd -ldl

+$(OUTPUT)test-libbfd-buildid.bin:
+ $(BUILD) -DPACKAGE='"perf"' -lbfd -ldl
+
$(OUTPUT)test-disassembler-four-args.bin:
$(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes

diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 5479e543b1947ee9..80c5795f324ba04f 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -90,6 +90,10 @@
# include "test-libbfd.c"
#undef main

+#define main main_test_libbfd_buildid
+# include "test-libbfd-buildid.c"
+#undef main
+
#define main main_test_backtrace
# include "test-backtrace.c"
#undef main
@@ -208,6 +212,7 @@ int main(int argc, char *argv[])
main_test_gtk2(argc, argv);
main_test_gtk2_infobar(argc, argv);
main_test_libbfd();
+ main_test_libbfd_buildid();
main_test_backtrace();
main_test_libnuma();
main_test_numa_num_possible_cpus();
diff --git a/tools/build/feature/test-libbfd-buildid.c b/tools/build/feature/test-libbfd-buildid.c
new file mode 100644
index 0000000000000000..157644b04c052a51
--- /dev/null
+++ b/tools/build/feature/test-libbfd-buildid.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <bfd.h>
+
+int main(void)
+{
+ bfd *abfd = bfd_openr("Pedro", 0);
+ return abfd && (!abfd->build_id || abfd->build_id->size > 0x506564726f);
+}
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 190be4fa5c2187f3..f73a85ea3e7fb20a 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -825,6 +825,12 @@ else
$(call feature_check,disassembler-four-args)
endif

+ifeq ($(feature-libbfd-buildid), 1)
+ CFLAGS += -DHAVE_LIBBFD_BUILDID_SUPPORT
+else
+ msg := $(warning Old version of libbfd/binutils things like PE executable profiling will not be available);
+endif
+
ifdef NO_DEMANGLE
CFLAGS += -DNO_DEMANGLE
else

Next message: Andrii Nakryiko: "Re: [PATCH bpf-next v2 1/6] bpf: Introduce pseudo_btf_id"
Previous message: Linus Torvalds: "Re: [git pull] drm fixes for 5.9-rc4"
In reply to: Remi Bernon: "Re: [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]