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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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.

- Arnaldo

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index f7432c4a4154a149..94a156df22d5a8d3 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -532,7 +532,7 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
 	return err;
 }
 
-#ifdef HAVE_LIBBFD_SUPPORT
+#ifdef HAVE_LIBBFD_BUILDID_SUPPORT
 
 int filename__read_build_id(const char *filename, void *bf, size_t size)
 {
@@ -560,7 +560,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
 	return err;
 }
 
-#else
+#else // HAVE_LIBBFD_BUILDID_SUPPORT
 
 int filename__read_build_id(const char *filename, void *bf, size_t size)
 {
@@ -589,7 +589,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size)
 	return err;
 }
 
-#endif
+#endif // HAVE_LIBBFD_BUILDID_SUPPORT
 
 int sysfs__read_build_id(const char *filename, void *build_id, size_t size)
 {