Message-ID: <CAP-5=fUdXRv512ZFiQKtxouNzN+HgommWGF3Pje1Tcuxg=J78Q@mail.gmail.com>
Date: Thu, 13 Mar 2025 22:54:57 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.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>, Kan Liang <kan.liang@...ux.intel.com>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	Aditya Gupta <adityag@...ux.ibm.com>, "Steinar H. Gunderson" <sesse@...gle.com>, 
	Charlie Jenkins <charlie@...osinc.com>, Changbin Du <changbin.du@...wei.com>, 
	"Masami Hiramatsu (Google)" <mhiramat@...nel.org>, James Clark <james.clark@...aro.org>, 
	Kajol Jain <kjain@...ux.ibm.com>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, 
	Li Huafei <lihuafei1@...wei.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Andi Kleen <ak@...ux.intel.com>, Chaitanya S Prakash <chaitanyas.prakash@....com>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	llvm@...ts.linux.dev, Song Liu <song@...nel.org>, bpf@...r.kernel.org, 
	Daniel Xu <dxu@...uu.xyz>
Subject: Re: [PATCH v2 00/17] Support dynamic opening of capstone/llvm remove BUILD_NONDISTRO

On Thu, Mar 13, 2025 at 3:24 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> 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@...gle.com> wrote:
> > >
> > > On Thu, Jan 23, 2025 at 3:36 PM Ian Rogers <irogers@...gle.com> wrote:
> > > > On Thu, Jan 23, 2025 at 1:59 PM Namhyung Kim <namhyung@...nel.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().

So I don't disagree. If we have headers but someone wants dlopen,
let's use the headers and let them use dlopen. Unfortunately the way
the build is set up isn't for that. The build assumes either:
1) you have libcapstone and its headers and want to link against it,
2) you don't have libcapstone and support is just removed.

In the changes the options become:
1) unchanged, if you have libcapstone then use the headers and link
against - this is Andi's preferred approach,
2) if you don't have libcapstone dlopen is used with constants derived
from pahole and baked into the sources - much as we do for vmlinux.h
in BPF programs.

One way to achieve what you are asking for is to make the build do:
1) the link against libcapstone approach that needs the headers,
2) the dlopen with capstone.h headers,
but we need a way to build without libcapstone, so we get:
3) possibly dlopen with pahole derived constants - but if we have that
then why do 2?
4) a no support option.

The problem is that if we don't do (3) then (4) will become the no
libcapstone option and frankly people who care will do (1) and so (2)
becomes redundant.
It was intentional doing the changes the way I have so that when you
don't build with libcapstone, were you later to add the library you
would gain support for it. This is with the down side of a small
number of constants being in the code. Potentially these could change
in the header files, but any such change is an ABI breakage and so
unlikely to happen. So I think the way the patches have things set up
is best.

Thanks,
Ian

Powered by blists - more mailing lists