From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Remi Bernon <rbernon@codeweavers.com>
Cc: linux-kernel@vger.kernel.org,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Ingo Molnar <mingo@redhat.com>, Jiri Olsa <jolsa@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Namhyung Kim <namhyung@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Jacek Caban <jacek@codeweavers.com>
Subject: Re: [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section
Date: Fri, 4 Sep 2020 16:03:37 -0300 [thread overview]
Message-ID: <20200904190337.GB3753976@kernel.org> (raw)
In-Reply-To: <76cc6da5-bf1e-4de3-7b79-b9759dc6e5b9@codeweavers.com>
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@codeweavers.com>
> > > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > > Cc: Ingo Molnar <mingo@redhat.com>
> > > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Jacek Caban <jacek@codeweavers.com>
> > > > ---
> > > >
> > > > 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@redhat.com>
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@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jacek Caban <jacek@codeweavers.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Remi Bernon <rbernon@codeweavers.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
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
prev parent reply other threads:[~2020-09-04 19:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-21 16:52 [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section Remi Bernon
2020-08-21 16:52 ` [PATCH v3 2/3] perf symbols: Try reading the symbol table with libbfd Remi Bernon
2020-08-21 16:52 ` [PATCH v3 3/3] perf tests: Add test for PE binary format support Remi Bernon
2020-08-26 11:47 ` [PATCH v3 1/3] perf dso: Use libbfd to read build_id and .gnu_debuglink section Jiri Olsa
2020-09-03 0:25 ` Arnaldo Carvalho de Melo
2020-09-03 16:51 ` Arnaldo Carvalho de Melo
2020-09-04 7:57 ` Remi Bernon
2020-09-04 19:03 ` Arnaldo Carvalho de Melo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200904190337.GB3753976@kernel.org \
--to=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=jacek@codeweavers.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rbernon@codeweavers.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.