On Tue, Jan 28, 2025 at 12:56 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Fri, Jan 24, 2025 at 04:59:21PM -0800, Ian Rogers wrote:
> > On Fri, Jan 24, 2025 at 1:58 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jan 22, 2025 at 09:42:53AM -0800, Ian Rogers wrote:
> > > > Capstone disassembly support was split between disasm.c and
> > > > print_insn.c. Move support out of these files into capstone.[ch] and
> > > > remove include capstone/capstone.h from those files. As disassembly
> > > > routines can fail, make failure the only option without
> > > > HAVE_LIBCAPSTONE_SUPPORT. For simplicity's sake, duplicate the
> > > > read_symbol utility function.
> > > >
> > > > The intent with moving capstone support into a single file is that
> > > > dynamic support, using dlopen for libcapstone, can be added in later
> > > > patches. This can potentially always succeed or fail, so relying on
> > > > ifdefs isn't sufficient. Using dlopen is a useful option to minimize
> > > > the perf tools dependencies and potentially size.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > > > ---
> > > > tools/perf/builtin-script.c | 2 -
> > > > tools/perf/util/Build | 1 +
> > > > tools/perf/util/capstone.c | 536 +++++++++++++++++++++++++++++++++++
> > > > tools/perf/util/capstone.h | 24 ++
> > > > tools/perf/util/disasm.c | 358 +----------------------
> > > > tools/perf/util/print_insn.c | 117 +-------
> > > > 6 files changed, 569 insertions(+), 469 deletions(-)
> > > > create mode 100644 tools/perf/util/capstone.c
> > > > create mode 100644 tools/perf/util/capstone.h
> > > >
> > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > > index 33667b534634..f05b2b70d5a7 100644
> > > > --- a/tools/perf/builtin-script.c
> > > > +++ b/tools/perf/builtin-script.c
> > > > @@ -1200,7 +1200,6 @@ static int any_dump_insn(struct evsel *evsel __maybe_unused,
> > > > u8 *inbuf, int inlen, int *lenp,
> > > > FILE *fp)
> > > > {
> > > > -#ifdef HAVE_LIBCAPSTONE_SUPPORT
> > > > if (PRINT_FIELD(BRSTACKDISASM)) {
> > > > int printed = fprintf_insn_asm(x->machine, x->thread, x->cpumode, x->is64bit,
> > > > (uint8_t *)inbuf, inlen, ip, lenp,
> > > > @@ -1209,7 +1208,6 @@ static int any_dump_insn(struct evsel *evsel __maybe_unused,
> > > > if (printed > 0)
> > > > return printed;
> > > > }
> > > > -#endif
> > > > return fprintf(fp, "%s", dump_insn(x, ip, inbuf, inlen, lenp));
> > > > }
> > > >
> > > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > > > index 5ec97e8d6b6d..9542decf9625 100644
> > > > --- a/tools/perf/util/Build
> > > > +++ b/tools/perf/util/Build
> > > > @@ -8,6 +8,7 @@ perf-util-y += block-info.o
> > > > perf-util-y += block-range.o
> > > > perf-util-y += build-id.o
> > > > perf-util-y += cacheline.o
> > > > +perf-util-y += capstone.o
> > > > perf-util-y += config.o
> > > > perf-util-y += copyfile.o
> > > > perf-util-y += ctype.o
> > > > diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> > > > new file mode 100644
> > > > index 000000000000..c0a6d94ebc18
> > > > --- /dev/null
> > > > +++ b/tools/perf/util/capstone.c
> > > > @@ -0,0 +1,536 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +#include "capstone.h"
> > > > +#include "annotate.h"
> > > > +#include "addr_location.h"
> > > > +#include "debug.h"
> > > > +#include "disasm.h"
> > > > +#include "dso.h"
> > > > +#include "machine.h"
> > > > +#include "map.h"
> > > > +#include "namespaces.h"
> > > > +#include "print_insn.h"
> > > > +#include "symbol.h"
> > > > +#include "thread.h"
> > > > +#include <fcntl.h>
> > > > +#include <string.h>
> > > > +
> > > > +#ifdef HAVE_LIBCAPSTONE_SUPPORT
> > > > +#include <capstone/capstone.h>
> > > > +#endif
> > >
> > > I think you can use a big #ifdef throughout the file to minimize the
> > > #ifdef dances. Usually it goes to the header to provide dummy static
> > > inlines and make the .c file depends on config. But I know you will
> > > add dlopen code for the #else case later.
> >
> > So I think big ifdefs like:
> >
> > #if HAVE_xyz
> > // 100s of lines
> > #else
> > // 100s of lines
> > #endif
> >
> > are best avoided. It is also the point of the shim-ing that we do
> >
> > ... perf_foobar(...)
> > {
> > #if NO_SHIM
> > ... foobar(...);
> > #else
> > //dlsym code
> > #endif
> > }
> >
> > Having the shimming and not shimming as two separate functions buried
> > in a 100 #ifdef loses that the code is common except for the shimming.
>
> Right, can we split the common part and move it out of #ifdef?
>
> >
> > For example, in the current code we have find_file_offset:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/disasm.c?h=perf-tools-next&id=91b7747dc70d64b5ec56ffe493310f207e7ffc99#n1371
> >
> > It is only possible to understand the use of this seemingly common
> > code by trying to interpret what's going on with the #ifdefs.
> >
> > I think it stylistically it is okay to have multiple stubbed out
> > functions inside a #if or #else, such as:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/perf_event.h?h=perf-tools-next&id=91b7747dc70d64b5ec56ffe493310f207e7ffc99#n1797
>
> I think the convention in the kernel community is that it's better to
> remove #ifdef's in .c files and add a dummy functions under !condition
> in .h files. If that's not possible, I think we should make the code
> less conditional by minimizing the #ifdef's.
>
> >
> > But when the logic is shared and all in one file it becomes next to
> > impossible to determine what's in use and what's not. Other than by
> > tweaking things and trying to get build errors.
> >
> > So for the shims I've placed the #if inside the function to make it
> > clear the function is a shim. For the other functions that are over
> > 100s of lines, for clarity the individual functions have #if
> > HAVE_LIBLLVM_SUPPORT around them to make it clear that the function
> > only has a meaning in that context - ie the source code doesn't make
> > you go on a #ifdef finding expedition to try to understand when the
> > code is in use.
>
> I think the both approaches have their own pros and cons. Some people
> prefer one and others may have different opinions. I think the big
> conditional block is better and easy to follow. Maybe we cannot agree
> on this. Then I believe it'd be better to follow the convention, no?
You are talking about a different convention. We're not doing
something or having a dummy function, we're having a direct call to
the underlying function or doing a shim through a dlsym. Having those
in 2 separate functions and having the #ifdefs separated from the code
it controls will impact the readability and lose the association I'm
very much after having.
Thanks,
Ian
