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@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 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
