Message-ID: <CAP-5=fX33kGxfHzqVGzusMBiHJM6G75TbLyZazjp37yohwscGg@mail.gmail.com>
Date: Mon, 8 Sep 2025 08:47:12 -0700
From: Ian Rogers <irogers@...gle.com>
To: Brendan McGrath <bmcgrath@...eweavers.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 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? 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