Re: [PATCH v2 00/17] Support dynamic opening of capstone/llvm remove BUILD_NONDISTRO

[Namhyung Kim <namhyung@kernel.org>]

Hi Ian,

On Wed, Mar 12, 2025 at 02:04:30PM -0700, Ian Rogers wrote:
> On Mon, Feb 10, 2025 at 10:06 AM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Jan 23, 2025 at 3:36 PM Ian Rogers <irogers@google.com> wrote:
> > > On Thu, Jan 23, 2025 at 1:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > I like changes up to this in general.  Let me take a look at the
> > > > patches.
> >
> > So it would be nice to make progress with this series given some level
> > of happiness, I don't see any actions currently on the patch series as
> > is. If I may be so bold as to recap the issues that have come up:
> >
> > 1) Andi Kleen mentions that dlopen is inferior to linking against
> > libraries and those libraries aren't a memory overhead if unused.
> >
> > I agree but pointed-out the data center use case means that saving
> > size on binaries can be important to some (me). We've also been trying
> > to reduce perf's dependencies for distributions as perf dragging in
> > say the whole of libLLVM can be annoying for making minimal
> > distributions that contain perf. Perhaps somebody (Arnaldo?) more
> > involved with distributions can confirm or deny the distribution
> > problem, I'm hoping it is self-evident.
> >
> > 2) Namhyung Kim was uncomfortable with the code defining
> > types/constants that were in header files as the two may drift over
> > time
> >
> > I agree but in the same way as a function name is an ABI for dlysym,
> > the types/constants are too. Yes a header file may change, but in
> > doing so the ABI has changed and so it would be an incompatible change
> > and everything would be broken. We'd need to fix the code for this,
> > say as we did when libbpf moved to version 1.0, but using a header
> > file would only weakly guard against this problem. The problem with
> > including the header files is that then the build either breaks
> > without the header or we need to support a no linking against a
> > library and not using dlopen case. I suspect a lot of distributions
> > wouldn't understand the build subtlety in this, the necessary build
> > options and things installed, and we'd end up not using things like
> > libLLVM even when it is known to be a large performance win. I also
> > hope one day we can move from parsing text out of forked commands, as
> > it is slower and more brittle, to just directly using libraries.
> > Making dlopen the fallback (probably with a warning on failure) seems
> > like the right direction for this except we won't get it if we need to
> > drag in extra dependency header files for the build to succeed (well
> > we could have a no library or dlopen option, but then we'd probably
> > find distributions packaging this and things like perf annotate
> > getting broken as they don't even know how to dlopen a library).
> >
> > 3) Namhyung Kim (and I) also raises that the libcapstone patch can be
> > smaller by dropping the print_capstone_detail support on x86
> >
> > Note, given the similarity between capstone and libLLVM for
> > disassembly, it is curious that only capstone gives the extra detail.
> >
> > I agree. Given the capstone disassembly output will be compromised we
> > should warn for this, probably in Makefile.config to avoid running
> > afoul of -Werror. It isn't clear that having a warning is a good move
> > given the handful of structs needed to support print_capstone_detail.
> > I'd prefer to keep the structs so that we haven't got a warning that
> > looks like it needs cleaning up.
> >
> > 4) Namhyung Kim raised concerns over #if placement
> >
> > Namhyung raised that he'd prefer:
> > ```
> > #if HAVE_LIBCAPSTONE_SUPPORT
> > // lots of code
> > #else
> > // lots of code
> > #endif
> > ```
> > rather than the #ifs being inside or around individual functions. I
> > raised that the large #ifs is a problem in the current code as you
> > lose context when trying to understand a function. You may look at a
> > function but not realize it isn't being used because of a #if 10s or
> > 100s of lines above. Namhyung raised that the large #ifs is closer to
> > kernel style, I disagreed as I think kernel style is only doing this
> > when it stubs out a bunch of API functions, not when more context
> > would be useful. Hopefully as the person writing the patches the style
> > choice I've made can be respected.
> >
> > 5) Daniel Xu raised issues with the removal of libbfd for Rust
> > support, as the code implies libbfd C++ demangling is a pre-requisite
> > of legacy rust symbol demangling
> >
> > A separate patch was posted adding Rust v0 symbol demangling with no
> > libbfd dependency:
> > https://lore.kernel.org/lkml/20250129193037.573431-1-irogers@google.com/
> > The legacy support should work with the non-libbfd demanglers as
> > that's what we have today. We should really clean up Rust demangling
> > and have tests. This is blocked on the Rust community responding to:
> > https://github.com/rust-lang/rust/issues/60705

I think #ifdef placements is not a big deal, but I still don't want to
pull libcapstone details into the perf tree.

For LLVM, I think you should to build llvm-c-helpers anyway which means
you still need LLVM headers and don't need to redefine the structures.

Can we do the same for capstone?  I think it's best to use capstone
headers directly and add a build option to use dlopen().

Thanks,
Namhyung