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 Thanks, Ian
