| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fV0w9tLFr7xYHFUH=UUq+tr+o5EYUik0d74rMWa9=Qi+A@mail.gmail.com>
Date: Fri, 24 Jan 2025 16:59:21 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>,
Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
Aditya Gupta <adityag@...ux.ibm.com>, "Steinar H. Gunderson" <sesse@...gle.com>,
Charlie Jenkins <charlie@...osinc.com>, Changbin Du <changbin.du@...wei.com>,
"Masami Hiramatsu (Google)" <mhiramat@...nel.org>, James Clark <james.clark@...aro.org>,
Kajol Jain <kjain@...ux.ibm.com>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
Li Huafei <lihuafei1@...wei.com>, Dmitry Vyukov <dvyukov@...gle.com>,
Andi Kleen <ak@...ux.intel.com>, Chaitanya S Prakash <chaitanyas.prakash@....com>,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
llvm@...ts.linux.dev, Song Liu <song@...nel.org>, bpf@...r.kernel.org
Subject: Re: [PATCH v3 03/18] perf capstone: Move capstone functionality into
its own file
On Fri, Jan 24, 2025 at 1:58 PM Namhyung Kim <namhyung@...nel.org> 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@...gle.com>
> > ---
> > 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.
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
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.
Thanks,
Ian
Powered by blists - more mailing lists