Re: [PATCH 2/2] perf symbols: Fix HAVE_LIBBFD_BUILDID_SUPPORT build

[Brendan McGrath <bmcgrath@codeweavers.com>]

On 9/5/25 01:53, Ian Rogers wrote:
> On Thu, Sep 4, 2025 at 7:18 AM James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 04/09/2025 9:27 am, Rémi Bernon wrote:
>>> Hi!
>>>
>>> On 9/4/25 10:13, James Clark wrote:
>>>>
>>>>
>>>> On 03/09/2025 5:07 pm, Ian Rogers wrote:
>>>>> On Wed, Sep 3, 2025 at 8:15 AM James Clark <james.clark@linaro.org>
>>>>> wrote:
>>>>>>
>>>>>> read_build_id() now has a blocking argument, but libbfd uses fopen()
>>>>>> internally which doesn't support O_NONBLOCK. Fix the build by adding
>>>>>> the
>>>>>> argument and ignoring it:
>>>>>>
>>>>>>     util/symbol-elf.c:964:8: error: too many arguments to function
>>>>>> ‘read_build_id’
>>>>>>       964 |  err = read_build_id(filename, bid, block);
>>>>>>
>>>>>> Fixes: 2c369d91d093 ("perf symbol: Add blocking argument to
>>>>>> filename__read_build_id")
>>>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>>>
>>>>> Libbfd should go away:
>>>>> https://lore.kernel.org/lkml/20250823003216.733941-14-
>>>>> irogers@google.com/
>>>>> but I can imagine that currently this is hit in a build test - sorry
>>>>> for missing that and thanks for the fix!
>>>>>
>>>>
>>>> Yeah just one of the build tests, I'm not actually using it.
>>>>
>>>> Remi are you still using this? To be fair the addition for PE support
>>>> is fairly recent and even includes a binary for testing it so I'm not
>>>> sure if we should be so quick to remove it.
>>>>
>>> Yes, I'm still using it occasionally, and I think it's generally useful
>>> for Wine profiling purposes and I would rather prefer that it's not
>>> removed.
>>>
>>> I know it's not built by default because of license conflicts. I didn't
>>> realize that was an issue when contributing the changes, and it is quite
>>> unfortunate (and silly IMO).
>>>
>>> Then I'm not particularly attached to libbfd and any other option that
>>> would let perf read PE files would be alright, as long as PE support is
>>> kept.
>>>
>>> Cheers,
>>
>> It looks like libLLVM might work. Looking at the doxygen there are vague
>> references to PE binaries around the getBuildID() function. But as
>> mentioned in the linked thread, it's huge at 100+ MB.
>>
>> WRT that thread, I think maybe re-writing some of this in Perf wouldn't
>> be so bad. Surely getting the buildID is trivial. For PE binaries it's
>> hard to tell what's supported currently, what's being used and what's
>> being done by what library or tool. addr2line, libbfd, symbols,
>> disassembly etc.
> 
> I know some people who work on LLVM for Windows for the sake of having
> a Chrome build from Linux. It should be possible to migrate the libbfd
> use cases to LLVM.

Just wanted to let you know that I've been able to put together a PoC 
that does just this. It allows the pe-file-parsing test to pass using 
LLVM in place of libbfd.

If there's interest, I would be happy to try to shape this in to 
something that can be accepted upstream.

> If I remember John Levine's Linkers and Loaders
> book correctly (contents available by way of your favorite search
> engine) everything is just a variant of COFF anyway.
> 
> It is a shame that the PE testing in buildid.sh (and the testing in
> general) is requiring `cc` as it'd be much nicer to have the tests in
> a form similar to the perf test workloads (e.g. perf test -w noploop).
> I don't have a good idea on how to fix this but just wanted to note
> it.
> 
> I'll write a non-blocking patch for read_build_id with libbfd that
> matches what the others do and should avoid the hang in the meantime.
> 
> Thanks,
> Ian
>