Re: [PATCH v2 00/17] Support dynamic opening of capstone/llvm remove BUILD_NONDISTRO

[Ian Rogers]

On Fri, Mar 14, 2025 at 10:06 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> On Thu, Mar 13, 2025 at 03:24:22PM -0700, Namhyung Kim wrote:
> > 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().
>
> My two cents: if one wants to support some library, then have its devel
> packages available at build time.
>
> Then, perf nowadays has lots of dependencies, we need to rein on that,
> making the good to have but not always used things to be dlopen'ed.
>
> Like we did with gtk (that at this point I think is really deprecated,
> BTW).
>
> gdb has prior art in this area that we could use, it is not even a TUI
> but it asks if debuginfo should be used and if so it goes on on
> potentially lenghty updates of the local buildid cache they keep (which
> is not the one we use, it should be).
>
> And in the recent discussion with Dmitry Vyukov the possibility doing a
> question to the user about a default behaviour to be set and then using
> .perfconfig not to bother anymore the user about things is part of
> helping the user to deal with the myriad possibilites perf offers.
>
> gdb could use that as well, why ask at every session if debuginfod
> should be used? Annoying.
>
> I think perf should try to use what is available, both at build and at
> run time, and it shouldn't change the way it output things, but should
> warn the user about recent developments, things we over time figured out
> are problematic and thus a new default would be better, but then obtain
> consent if the user cares about it, and allow for backtracking, to go
> and change .perfconfig when the user realises the old output/behaviour
> is not really nice.
>
> But keeping the grass green as it used to be should be the priority.

So I don't understand what you are saying. If I have libcapstone
installed should the build link against it or just use its headers for
dlopen? Is declaring structs/constants to avoid needing the library
acceptable?

The patches as they are will link against libcapstone if it's
installed (just as currently happens) if not it will try to use dlopen
at runtime, if that fails then you get the current no libcapstone not
available at build time behavior. To support this a minimal amount of
structs and constants are necessary. See here:
https://github.com/googleprodkernel/linux-perf/blob/google_tools_master/tools/perf/util/llvm.c#L21-L58
https://github.com/googleprodkernel/linux-perf/blob/google_tools_master/tools/perf/util/capstone.c#L23-L132

The patches try to make it look as though the same function exists
with dlopen or directly calling by turning for example "cs_disasm"
into:
```
static size_t perf_cs_disasm(csh handle, const uint8_t *code, size_t code_size,
uint64_t address, size_t count, struct cs_insn **insn)
{
#ifdef HAVE_LIBCAPSTONE_SUPPORT
return cs_disasm(handle, code, code_size, address, count, insn);
#else
static bool fn_init;
static enum cs_err (*fn)(csh handle, const uint8_t *code, size_t code_size,
uint64_t address, size_t count, struct cs_insn **insn);

if (!fn_init) {
fn = dlsym(perf_cs_dll_handle(), "cs_disasm");
if (!fn)
pr_debug("dlsym failed for cs_disasm\n");
fn_init = true;
}
if (!fn)
return CS_ERR_HANDLE;
return fn(handle, code, code_size, address, count, insn);
#endif
}
```
That is with the library linked it is just a direct call otherwise
dlopen/dlsym are used. This means that the perf code using the library
is unchanged except turning "cs_disasm" into "perf_cs_disasm" and we
can unconditionally assume the perf_cs_disasm function will be
available.

If we don't create the structs/constants ourselves, which I think is
where controversy lies, then any function that calls cs_disasm needs
to handle a situation where there is no cs_disasm at build time. I was
trying to avoid this clutter in the code and just have things at the
library boundary. The structs/constants become necessary as we can't
assume we have access to the header file to support perf's current
build.

Thanks,
Ian