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

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
>

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

On Thu, Sep 04, 2025 at 06:53:33PM +0100, Sam James wrote:
> 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?

I don't have much to add. If no functionality is lost by building with BUILD_NONDISTRO=0,
then we can disable it for 6.17 and later, since it's deprecated in any case.

Best regards,
-Guilherme

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

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

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>

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

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

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
> 

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

On 9/9/25 01:47, Ian Rogers wrote:
> 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?

I'm happy to do that. I'll be largely unavailable for the next 4 weeks 
anyway, and I've still got clean-up/improvements to make to the PoC 
before it would be ready for any kind of submission. So I'm happy to 
wait and then modify my patches accordingly.

> 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

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