[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

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

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

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>