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

[Ian Rogers]

On Mon, Feb 10, 2025 at 10:06 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Thu, Jan 23, 2025 at 3:36 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > On Thu, Jan 23, 2025 at 1:59 PM Namhyung Kim <namhyung@xxxxxxxxxx> 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@xxxxxxxxxx/
> 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

Ping.

Thanks,
Ian