Message-ID: <Z9NbFqaDQMjvYxcc@google.com>
Date: Thu, 13 Mar 2025 15:24:22 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
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

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().

Thanks,
Namhyung

Powered by blists - more mailing lists