On Fri, Jun 27, 2025 at 12:26 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > On Fri, Jun 27, 2025 at 09:44:02AM -0700, Ian Rogers wrote: > > On Thu, Jun 26, 2025 at 9:53 PM Ian Rogers <irogers@xxxxxxxxxx> wrote: > > > > > > On Thu, Jun 26, 2025 at 4:19 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote: > > > > > > > > On Thu, Apr 17, 2025 at 04:07:27PM -0700, Ian Rogers wrote: > > > > > If perf wasn't built against libcapstone, no HAVE_LIBCAPSTONE_SUPPORT, > > > > > support dlopen-ing libcapstone.so and then calling the necessary > > > > > functions by looking them up using dlsym. Reverse engineer the types > > > > > in the API using pahole, adding only what's used in the perf code or > > > > > necessary for the sake of struct size and alignment. > > > > > > > > I still think it's simpler to require capstone headers at build time and > > > > add LIBCAPSTONE_DYNAMIC=1 or something to support dlopen. > > > > > > I agree, having a header file avoids the need to declare the header > > > file values. This is simpler. Can we make the build require > > > libcapstone and libLLVM in the same way that libtraceevent is > > > required? That is you have to explicitly build with NO_LIBTRACEEVENT=1 > > > to get a no libtraceevent build to succeed. If we don't do this then > > > having LIBCAPSTONE_DYNAMIC will most likely be an unused option and > > > not worth carrying in the code base, I think that's sad. If we require > > > the libraries I don't like the idea of people arguing, "why do I need > > > to install libcapstone and libLLVM just to get the kernel/perf to > > > build now?" The non-simple, but still not very complex, approach taken > > > here was taken as a compromise to get the best result (a perf that > > > gets faster, BPF support, .. when libraries are available without > > > explicitly depending on them) while trying not to offend kernel > > > developers who are often trying to build on minimal systems. > > > > Fwiw, a situation that I think is analogous (and was playing on my > > mind while writing the code) is that we don't require python to build > > perf and carry around empty-pmu-events.c: > > https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/empty-pmu-events.c?h=perf-tools-next > > It would be simpler (in the code base and in general) to require > > everyone building perf to have python. > > Having python on a system seems less of a stretch than requiring > > libcapstone and libLLVM. > > > > If we keep the existing build approach, optional capstone and libLLVM > > by detecting it as a feature, then just linking against the libraries > > is natural. Someone would need to know they care about optionality and > > enable LIBCAPSTONE_DYNAMIC=1. An average build where the libraries > > weren't present would lose the libcapstone and libLLVM support. We > > could warn about this situation but some people are upset about build > > warnings, and if we do warn we could be pushing people into just > > linking against libcapstone and libLLVM which seems like we'll fall > > foul of the, "perf has too many library dependencies," complaint. We > > could warn about linking against libraries when there is a _DYNAMIC > > alternative like this available, but again people don't like build > > warnings and they could legitimately want to link against libcapstone > > or libLLVM. > > > > Anyway, that's why I ended up with the code in this state, to best try > > to play off all the different compromises and complaints that have > > been dealt with in the past. > > I can see your point. Adding new build flags is likely to be unused and > forgotten. There's also more code to support the neither linked or nor dlopened approach. > But I also think is that this dlopen support is mostly useful to distro > package managers who want to support more flexible environment and > regular dynamic linking is preferred to local builds over dlopen. Then > adding a note to a pull request and contacting them directly (if needed) > might work? If you want to run with this then I don't mind. Thanks, Ian > Thanks, > Namhyung >
