[PATCH 0/2] perf tools: read_build_id() blocking argument fixes

[PATCH 0/2] perf tools: read_build_id() blocking argument fixes

[James Clark]

The function now takes an argument for O_NONBLOCK. The first fix seems
straightforward. The second one is _probably_ fine, but I can't really
see any easy way to fix it because libbfd handles all its own IO. Maybe
we need to compile in both versions of read_build_id() and only call the
libbfd one on regular files? Or maybe in that specific use case it
doesn't care, the commit message for adding libbfd there mentioned Wine
PE binaries.

Signed-off-by: James Clark <james.clark@linaro.org>
---
James Clark (2):
      perf tests: Fix "PE file support" test build
      perf symbols: Fix HAVE_LIBBFD_BUILDID_SUPPORT build

 tools/perf/tests/pe-file-parsing.c | 4 ++--
 tools/perf/util/symbol-elf.c       | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
---
base-commit: 07d9df80082b8d1f37e05658371b087cb6738770
change-id: 20250903-james-perf-read-build-id-fix-0ef6fbce0432

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH 1/2] perf tests: Fix "PE file support" test build

[James Clark]

filename__read_build_id() now takes a blocking/non-blocking argument.
The original behavior of filename__read_build_id() was blocking so add
block=true to fix the build.

Fixes: 2c369d91d093 ("perf symbol: Add blocking argument to filename__read_build_id")
Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/tests/pe-file-parsing.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 30c7da79e109..8b31d1d05f90 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -37,7 +37,7 @@ static int run_dir(const char *d)
 	size_t idx;
 
 	scnprintf(filename, PATH_MAX, "%s/pe-file.exe", d);
-	ret = filename__read_build_id(filename, &bid);
+	ret = filename__read_build_id(filename, &bid, /*block=*/true);
 	TEST_ASSERT_VAL("Failed to read build_id",
 			ret == sizeof(expect_build_id));
 	TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
@@ -49,7 +49,7 @@ static int run_dir(const char *d)
 			!strcmp(debuglink, expect_debuglink));
 
 	scnprintf(debugfile, PATH_MAX, "%s/%s", d, debuglink);
-	ret = filename__read_build_id(debugfile, &bid);
+	ret = filename__read_build_id(debugfile, &bid, /*block=*/true);
 	TEST_ASSERT_VAL("Failed to read debug file build_id",
 			ret == sizeof(expect_build_id));
 	TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,

-- 
2.34.1

Re: [PATCH 1/2] perf tests: Fix "PE file support" test build

[Ian Rogers]

On Wed, Sep 3, 2025 at 8:15 AM James Clark <james.clark@linaro.org> wrote:
>
> filename__read_build_id() now takes a blocking/non-blocking argument.
> The original behavior of filename__read_build_id() was blocking so add
> block=true to fix the build.
>
> Fixes: 2c369d91d093 ("perf symbol: Add blocking argument to filename__read_build_id")
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks for the fix!
Ian

> ---
>  tools/perf/tests/pe-file-parsing.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 30c7da79e109..8b31d1d05f90 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -37,7 +37,7 @@ static int run_dir(const char *d)
>         size_t idx;
>
>         scnprintf(filename, PATH_MAX, "%s/pe-file.exe", d);
> -       ret = filename__read_build_id(filename, &bid);
> +       ret = filename__read_build_id(filename, &bid, /*block=*/true);
>         TEST_ASSERT_VAL("Failed to read build_id",
>                         ret == sizeof(expect_build_id));
>         TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
> @@ -49,7 +49,7 @@ static int run_dir(const char *d)
>                         !strcmp(debuglink, expect_debuglink));
>
>         scnprintf(debugfile, PATH_MAX, "%s/%s", d, debuglink);
> -       ret = filename__read_build_id(debugfile, &bid);
> +       ret = filename__read_build_id(debugfile, &bid, /*block=*/true);
>         TEST_ASSERT_VAL("Failed to read debug file build_id",
>                         ret == sizeof(expect_build_id));
>         TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
>
> --
> 2.34.1
>

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

[James Clark]

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>
---
 tools/perf/util/symbol-elf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 033c79231a54..e0d6ff7d0acf 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -873,7 +873,8 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
 
 #ifdef HAVE_LIBBFD_BUILDID_SUPPORT
 
-static int read_build_id(const char *filename, struct build_id *bid)
+static int read_build_id(const char *filename, struct build_id *bid,
+			 bool block __maybe_unused)
 {
 	size_t size = sizeof(bid->data);
 	int err = -1;

-- 
2.34.1

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

[Ian Rogers]

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!

We should probably honor the blocking argument (use fdopen) as the
probe perf tests will invoke perf record system wide with data pages
and predictably hang on this for files like mmap-ed in sound devices.
That said, maybe this hanging will serve as an indication not to use
the deprecated libbfd code. From the sounds of things this will break
gentoo :-(
https://lore.kernel.org/lkml/87ldnacz33.fsf@gentoo.org/

Thanks,
Ian

> ---
>  tools/perf/util/symbol-elf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 033c79231a54..e0d6ff7d0acf 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -873,7 +873,8 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
>
>  #ifdef HAVE_LIBBFD_BUILDID_SUPPORT
>
> -static int read_build_id(const char *filename, struct build_id *bid)
> +static int read_build_id(const char *filename, struct build_id *bid,
> +                        bool block __maybe_unused)
>  {
>         size_t size = sizeof(bid->data);
>         int err = -1;
>
> --
> 2.34.1
>

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

[James Clark]

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.

Having said that, it is a bit weird that it's basically not compiled in 
anywhere. And the current array of supported disassemblers etc is 
completely bewildering and hard to maintain.

> We should probably honor the blocking argument (use fdopen) as the
> probe perf tests will invoke perf record system wide with data pages
> and predictably hang on this for files like mmap-ed in sound devices.
> That said, maybe this hanging will serve as an indication not to use
> the deprecated libbfd code. From the sounds of things this will break
> gentoo :-(
> https://lore.kernel.org/lkml/87ldnacz33.fsf@gentoo.org/

If it's just going to be a test issue we can probably live with it. I'm 
not sure how to honor the blocking argument, I did have a look into 
libbfd but it didn't seem possible after looking for a few minutes. 
Probably not worth spending any more time on it until we decide whether 
it's staying or not.

> 
> Thanks,
> Ian
> 
>> ---
>>   tools/perf/util/symbol-elf.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 033c79231a54..e0d6ff7d0acf 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -873,7 +873,8 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
>>
>>   #ifdef HAVE_LIBBFD_BUILDID_SUPPORT
>>
>> -static int read_build_id(const char *filename, struct build_id *bid)
>> +static int read_build_id(const char *filename, struct build_id *bid,
>> +                        bool block __maybe_unused)
>>   {
>>          size_t size = sizeof(bid->data);
>>          int err = -1;
>>
>> --
>> 2.34.1
>>

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

[Rémi Bernon]

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

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

[James Clark]

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.

James

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

[Ian Rogers]

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. 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

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

[Brendan McGrath]

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
> 

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

[Ian Rogers]

On Mon, Sep 8, 2025 at 3:24 AM Brendan McGrath <bmcgrath@codeweavers.com> wrote:
> 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.

IMO that'd be great! In this series:
https://lore.kernel.org/lkml/20250823003216.733941-1-irogers@google.com/
I wanted to pull apart the disasm vs the srcline vs .. uses of
libraries like llvm, capstone, let's delete libbfd, objdump, etc. The
idea being to have the API defined somewhere like disasm.h and then
based on compile time and runtime options select which implementation
to use. Things have evolved in perf, with a lot of stuff just globbed
into places like symbol without clear separation of APIs. Separating
things by library used allows reuse of things like library handles.

That series has 2 issues I'm aware of:
1) the last 6 patches remove libbfd support (rather than refactor) and
some people may care. I suspect with your fix it could be down to ~1
person caring. I removed rather than refactored as there is a very
real risk that when you do refactor you break, and as this stuff is
next to always disabled then it's easy for regressions to creep in at
which point that ~1 person would probably complain. I'd much rather we
had a good solution that everyone was working toward having work well,
your patches would pull in this direction :-)

2) Namhyung didn't like that I'd reversed the struct definitions for
the the capstone/LLVM APIs using pahole and would prefer that the
definitions came from capstone.h/llvm.h. My reasons for not doing that
are:
2.1) if you have say capstone.h or llvm.h then why not just link
against the library? But doing that avoids the decoupling the patch
series is trying to set up. We'd need more build options, which option
to make the default, etc. which is kind of messy.
2.2) to support that you'd need to bring back a "what if no
capstone.h/llvm.h" option in the code, I'd wanted that to be the
dlopen/dlsym case which must already handle libcapstone.so or
libLLVM.so being missing. Supporting the "no anything" option brings
with it a lot of ifdef-ery I was hoping to avoid and it would again be
one of those seldom used and often broken build options (like symbol
minimal (no libelf) today).
2.3) in my build environment (bazel) depending on headers means
linking against the library and the global initializers mean that even
though no code (in say libLLVM) is directly used you can't strip out
the library again. I'd need to rework my build environment to try to
get the headers without the library and that'd be a larger undertaking
than the reverse engineering of the structs using pahole (as is
already done in the series). So changing the patches would mean
creating a patch series that I'd need to then do more work on to have
work in my build environment, and I'm not sure things are any better
by doing that. pahole was my compromise to just sidestep all of this
without copy-pasting from header files and introducing licensing
issues.

Anyway, what does this mean to fixing PE executables in LLVM? Perhaps
the first 12 patches of:
https://lore.kernel.org/lkml/20250823003216.733941-1-irogers@google.com/
can land and then you put your changes into llvm.c there? We can
always clean up the issues of problem (2) above as later patches -
don't let perfect be the enemy of good. If libbfd must live-on then we
can move it to libbfd.c so that going through the generic source
doesn't mean wading through the libbfd code.

Anyway, I know I'm hijacking your fix to advance my own patch series.
I'm happy with your work landing independent of it, it just seemed we
could do the APIs more cleanly if both series landed together. I don't
object to (I'd also be positive for) just getting PE working without
my series. Isolating your code in the llvm.c in my series may make
things a bit easier for you. Having both series together would allow
the library decoupling, BPF JIT disassembly along with LLVM PE
support.

Namhyung/Arnaldo as the barriers to entry, could you comment?

Thanks,
Ian

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

[Arnaldo Carvalho de Melo]

On Mon, Sep 08, 2025 at 08:47:12AM -0700, Ian Rogers wrote:
> Anyway, what does this mean to fixing PE executables in LLVM? Perhaps
> the first 12 patches of:

Lots to digest, but to make progress, if you think you can resubmit a
trimmed down series after making sure it applies to perf-tools-next,
please do so and mention that in the cover letter.

- Arnaldo

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

[Sam James]

Ian Rogers <irogers@google.com> writes:

> 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!
>
> We should probably honor the blocking argument (use fdopen) as the
> probe perf tests will invoke perf record system wide with data pages
> and predictably hang on this for files like mmap-ed in sound devices.
> That said, maybe this hanging will serve as an indication not to use
> the deprecated libbfd code. From the sounds of things this will break
> gentoo :-(
> https://lore.kernel.org/lkml/87ldnacz33.fsf@gentoo.org/

Just want to say I haven't forgot about this, I need to find a moment to
compare the bfd and nonbfd builds to see if everything works OK
now. Will try do that in the next few days.

The disassembler/objdump use was definitely the biggest problem so if
support for binutils is here to say there, that puts my mind at ease.

Has Andi mentioned what issue he had? amadio/dlan, can you weigh in as well?

>
> Thanks,
> Ian
>
>> ---
>>  tools/perf/util/symbol-elf.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 033c79231a54..e0d6ff7d0acf 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -873,7 +873,8 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
>>
>>  #ifdef HAVE_LIBBFD_BUILDID_SUPPORT
>>
>> -static int read_build_id(const char *filename, struct build_id *bid)
>> +static int read_build_id(const char *filename, struct build_id *bid,
>> +                        bool block __maybe_unused)
>>  {
>>         size_t size = sizeof(bid->data);
>>         int err = -1;
>>
>> --
>> 2.34.1
>>

Re: [PATCH 0/2 v6.17-rc] perf tools: read_build_id() blocking argument fixes

[Arnaldo Carvalho de Melo]

On Wed, Sep 03, 2025 at 04:15:25PM +0100, James Clark wrote:
> The function now takes an argument for O_NONBLOCK. The first fix seems
> straightforward. The second one is _probably_ fine, but I can't really
> see any easy way to fix it because libbfd handles all its own IO. Maybe
> we need to compile in both versions of read_build_id() and only call the
> libbfd one on regular files? Or maybe in that specific use case it
> doesn't care, the commit message for adding libbfd there mentioned Wine
> PE binaries.

I noticed that yesterday and have this in the tmp.perf-tools-next
(thought that had sent to the list but didn't) branch:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=tmp.perf-tools-next&id=4bfe653aa3fefd429671aa27413a1124fe65b9d1

But since this affects 6.17, even being opt-in, I think it should go
there together with other patches that Namhyung is collecting in
perf-tools.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> James Clark (2):
>       perf tests: Fix "PE file support" test build
>       perf symbols: Fix HAVE_LIBBFD_BUILDID_SUPPORT build
> 
>  tools/perf/tests/pe-file-parsing.c | 4 ++--
>  tools/perf/util/symbol-elf.c       | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> ---
> base-commit: 07d9df80082b8d1f37e05658371b087cb6738770
> change-id: 20250903-james-perf-read-build-id-fix-0ef6fbce0432
> 
> Best regards,
> -- 
> James Clark <james.clark@linaro.org>

Re: [PATCH 0/2 v6.17-rc] perf tools: read_build_id() blocking argument fixes

[Namhyung Kim]

Hello,

On Wed, Sep 03, 2025 at 12:34:15PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Sep 03, 2025 at 04:15:25PM +0100, James Clark wrote:
> > The function now takes an argument for O_NONBLOCK. The first fix seems
> > straightforward. The second one is _probably_ fine, but I can't really
> > see any easy way to fix it because libbfd handles all its own IO. Maybe
> > we need to compile in both versions of read_build_id() and only call the
> > libbfd one on regular files? Or maybe in that specific use case it
> > doesn't care, the commit message for adding libbfd there mentioned Wine
> > PE binaries.
> 
> I noticed that yesterday and have this in the tmp.perf-tools-next
> (thought that had sent to the list but didn't) branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=tmp.perf-tools-next&id=4bfe653aa3fefd429671aa27413a1124fe65b9d1
> 
> But since this affects 6.17, even being opt-in, I think it should go
> there together with other patches that Namhyung is collecting in
> perf-tools.
> 
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Yep, sure.  I'll queue them to perf-tools.

Thanks,
Namhyung