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

[Remi Bernon <rbernon@codeweavers.com>]

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?
-- 
Rémi Bernon <rbernon@codeweavers.com>