Message-ID: <CAP-5=fUHLP-vtktodVuvMEbOd+TfQPPndkajT=WNf3Mc4VEZaA@mail.gmail.com>
Date: Mon, 10 Feb 2025 10:06:34 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: 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, 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

Thanks,
Ian

Powered by blists - more mailing lists