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

Ian Rogers
2025-01-23T23:36:26Z

On Thu, Jan 23, 2025 at 1:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Jan 21, 2025 at 10:23:15PM -0800, Ian Rogers wrote:
> > Linking against libcapstone and libLLVM can be a significant increase
> > in dependencies and size of memory footprint. For something like `perf
> > record` the disassembler and addr2line functionality won't be
> > used. Support dynamically loading these libraries using dlopen and
> > then calling the appropriate functions found using dlsym.
>
> It's not clear from the description how you would use dlopen/dlsym.
> Based on an offline discussion, you want to leave the current linking
> model as is, and to support dlopen/dlsym when it's NOT detected at
> build-time, right?

Yep. Current behavior is no header file than these options fail, new
behavior is that we try to use dlopen/dlsym and fail if the dlopen
fails.

> For that, you need to carry some definitions of the functions and types
> for the used APIs. But I'm not sure if it's right to carry them in the
> perf code base.

Right. I mention that here:
https://lore.kernel.org/lkml/CAP-5=fUhNuybCU-2_5EgcCwgwXnxvyFMvyhzKe=ZP1bssQwXHw@mail.gmail.com/
For LLVM we need 3 typedefs and 5 #defines, for capstone we need 2
structs and 5 enums (if we #ifdef some x86 only formatting code).

The problem in not carrying those definitions is:
1) if the header file isn't present a build won't support
LLVM/capstone even by dlopen - everything falls through to
objdump/addr2line that have known performance issues;
2) package maintainers either need to spot a warning message to
realize they've done this by having a missing header file (hard to
spot and brittle) or we require the build to fail and people without
capstone.h opt out of the build error with NO_CAPSTONE=1 - something
perf developers will probably not like;
3) the LLVM/capstone code needs #ifdefs and __maybe_unused to suppress
compiler warnings, or perhaps we have a minimal version of those
files, leading to extra code complexity.

I believe the approach here is no worse than what we do with vmlinux.h
for BPF code and is robust as depending on dlsym being able to look up
the function names. It is not perfect but I think it is more perfect
and less complex than the alternative.

> >
> > BUILD_NONDISTRO is used to build perf against the license incompatible
> > libbfd and libiberty libraries. As this has been opt-in for nearly 2
> > years, commit dd317df07207 ("perf build: Make binutil libraries opt
> > in"), remove the code to simplify the code base.
>
> This part can be a separate series.

Right, I posted it as a series here:
https://lore.kernel.org/lkml/20250111202851.1075338-1-irogers@google.com/
as mentioned in the v2 notes below. The issue was that Arnaldo pointed
out removing BUILD_NONDISTRO removed disassemble_bpf that had only
been implemented for libbfd. This series adds the LLVM/capstone
definitions built into the symbol__disassemble refactor. Merging that
series would conflict with this series, so I posted everything
together to avoid having series of patches depending upon one another.
I also wanted to check that what is in disasm.c in the end is
reasonable, which I believe it is with significantly reduced
ifdef-ery.

> >
> > The patch series:
> > 1) does some initial clean up;
> > 2) moves the capstone and LLVM code to their own C files,
> > 3) simplifies a little the capstone code;
>
> I like changes up to this in general. Let me take a look at the
> patches.

Thanks,
Ian
Ian

> Thanks,
> Namhyung