Re: [PATCH v1 4/5] perf capstone: Support for dlopen-ing libcapstone.so

[Ian Rogers]

On Wed, Jan 22, 2025 at 2:42 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Wed, Jan 22, 2025 at 2:27 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > Hi Ian,
> >
> > On Mon, Jan 20, 2025 at 09:32:07AM -0800, 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 think reverse engineering the types is fragile.  Can we simply change
> > to dlopen if libcapstone is available?
>
> The current behavior is if HAVE_LIBCAPSTONE_SUPPORT then we will link
> against libcapstone and can assume the header files are present.
>
> The reverse engineering of structs and constants I agree is brittle,
> but these things shouldn't change, it is used in this code when the
> capstone/capstone.h isn't present. If capstone/capstone.h is required
> we could change the ifdefs to indicate:
> 1) capstone is present and we're linking against it (this is
> HAVE_LIBCAPSTONE_SUPPORT today);
> 2) capstone is present but use it by way of dlopen;
> 3) capstone isn't present don't #include, don't link and don't dlopen.
>
> My thought was that this is less good than the reverse engineering.
> (1) relies on the builder having libcapstone, which at build time is a
> warning that flies by. (2) also relies on the builder having to have
> libcapstone and to opt into a dlopen behavior, they'll need to read
> Makefiles to discover this. (3) gives a less good perf tool with in
> the source lots of __maybe_unused and ifdef-ed out functions all over
> the place.
>
> With the reverse engineered values if you have libcapstone then
> HAVE_LIBCAPSTONE_SUPPORT works as before - no change. If you don't
> then you will be opted into getting something better than objdump -
> albeit it could break if the reverse engineered values drift from
> those in capstone/capstone.h. Less build options also is less
> complexity and cleaner code.

To be clear, if the header file values/structs drift then libcapstone
will need to version values/structs, we'll need to update the perf
code to use these, etc. Otherwise a tool linked against an older
libcapstone will break when libcapstone is updated. The code handling
not having any libcapstone, dlopen-ed or not, would be significant as
every static function will need an #ifdef to avoid a not used warning,
every parameter will need a __maybe_unused and the overall code
quality in perf will be worse imo because of this. The no libcapstone
default when not detected would be crappy for users. The values/types
added here are fewer than 20, and so are not a huge maintenance
burden. I did a similar thing for the LLVM C++ symbolizer code and its
maintenance would have been a burden and so abandoned it in the
llvm-c-helpers case that mainly affects addr2line and not disassembly.
Hopefully later LLVM C APIs will make this unnecessary.

Anyway, because of this I won't be changing this in later versions. I
will be carrying these changes in Google's tree and think ideally it
would be carried in the regular tree.

Thanks,
Ian