Message-ID: <3e6eb24f-5557-4045-8593-bfd12e2b9cec@codeweavers.com>
Date: Fri, 12 Sep 2025 09:23:42 +1000
From: Brendan McGrath <bmcgrath@...eweavers.com>
To: Ian Rogers <irogers@...gle.com>, Namhyung Kim <namhyung@...nel.org>,
 Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: James Clark <james.clark@...aro.org>,
 Rémi Bernon <rbernon@...eweavers.com>,
 Sam James <sam@...too.org>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 Leo Yan <leo.yan@....com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] perf symbols: Fix HAVE_LIBBFD_BUILDID_SUPPORT build



On 9/9/25 01:47, Ian Rogers wrote:
> On Mon, Sep 8, 2025 at 3:24 AM Brendan McGrath <bmcgrath@...eweavers.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

Powered by blists - more mailing lists