linux-kernel - Re: [PATCH v5 11/21] perf x86: Define arch_fetch_insn in NO

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6e6bd96f-912a-40e4-b43e-53f6a99f4338@intel.com>
Date: Thu, 31 Oct 2024 11:14:15 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Ian Rogers <irogers@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
 James Clark <james.clark@...aro.org>, Howard Chu <howardchu95@...il.com>,
 Athira Jajeev <atrajeev@...ux.vnet.ibm.com>,
 Michael Petlan <mpetlan@...hat.com>, Veronika Molnarova
 <vmolnaro@...hat.com>, Dapeng Mi <dapeng1.mi@...ux.intel.com>,
 Thomas Richter <tmricht@...ux.ibm.com>, Ilya Leoshkevich
 <iii@...ux.ibm.com>, Colin Ian King <colin.i.king@...il.com>,
 Weilin Wang <weilin.wang@...el.com>, Andi Kleen <ak@...ux.intel.com>,
 Josh Poimboeuf <jpoimboe@...hat.com>, linux-kernel@...r.kernel.org,
 linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v5 11/21] perf x86: Define arch_fetch_insn in NO_AUXTRACE
 builds

On 31/10/24 03:42, Ian Rogers wrote:
> archinsn.c containing arch_fetch_insn was only enabled with
> CONFIG_AUXTRACE, but this meant that a NO_AUXTRACE build on x86 would
> use the empty weak version of arch_fetch_insn - weak symbols are a
> frequent source of errors like this and are outside of the C
> specification. Change it so that archinsn.c is always built on x86 and
> make the weak symbol empty version of arch_fetch_insn a strong one
> guarded by ifdefs.
> 
> arch_fetch_insn on x86 depends on insn_decode which is a function
> included then built into
> intel-pt-insn-decoder.c. intel-pt-insn-decoder.c isn't built in a
> NO_AUXTRACE=1 build. Separate the insn_decode function from
> intel-pt-insn-decoder.c by just directly compiling the relevant
> file. Guard this compilation to be for either always on x86 (because
> of the use in arch_fetch_insn) or when auxtrace is enabled. Apply the
> CFLAGS overrides as necessary, reducing the amount of code where
> warnings are disabled.
> 
> Signed-off-by: Ian Rogers <irogers@...gle.com>

For Intel PT

Tested-by: Adrian Hunter <adrian.hunter@...el.com>

> ---
>  tools/perf/arch/x86/util/Build                 |  2 +-
>  tools/perf/util/Build                          |  2 +-
>  tools/perf/util/intel-pt-decoder/Build         | 18 ++++++++++++++----
>  .../intel-pt-decoder/intel-pt-insn-decoder.c   |  3 ---
>  tools/perf/util/trace-event-scripting.c        |  4 +++-
>  5 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
> index bc56a8e70f34..c5df4a2cf180 100644
> --- a/tools/perf/arch/x86/util/Build
> +++ b/tools/perf/arch/x86/util/Build
> @@ -18,6 +18,6 @@ perf-util-$(CONFIG_LOCAL_LIBUNWIND)    += unwind-libunwind.o
>  perf-util-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
>  
>  perf-util-$(CONFIG_AUXTRACE) += auxtrace.o
> -perf-util-$(CONFIG_AUXTRACE) += archinsn.o
> +perf-util-y += archinsn.o
>  perf-util-$(CONFIG_AUXTRACE) += intel-pt.o
>  perf-util-$(CONFIG_AUXTRACE) += intel-bts.o
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 650974413849..340544a6f5ec 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -122,7 +122,7 @@ perf-util-y += iostat.o
>  perf-util-y += stream.o
>  perf-util-y += kvm-stat.o
>  perf-util-$(CONFIG_AUXTRACE) += auxtrace.o
> -perf-util-$(CONFIG_AUXTRACE) += intel-pt-decoder/
> +perf-util-y += intel-pt-decoder/
>  perf-util-$(CONFIG_AUXTRACE) += intel-pt.o
>  perf-util-$(CONFIG_AUXTRACE) += intel-bts.o
>  perf-util-$(CONFIG_AUXTRACE) += arm-spe.o
> diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> index 30793d08c6d4..f99d150059b9 100644
> --- a/tools/perf/util/intel-pt-decoder/Build
> +++ b/tools/perf/util/intel-pt-decoder/Build
> @@ -9,14 +9,24 @@ $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_table
>  
>  # Busybox's diff doesn't have -I, avoid warning in the case
>  
> -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +ifeq ($(SRCARCH),x86)
> +  perf-util-y += inat.o insn.o
> +else
> +  perf-util-$(CONFIG_AUXTRACE) += inat.o insn.o
> +endif
> +
> +$(OUTPUT)util/intel-pt-decoder/inat.o: $(srctree)/tools/arch/x86/lib/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
>  	$(call rule_mkdir)
>  	$(call if_changed_dep,cc_o_c)
>  
> -CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder
> +CFLAGS_inat.o += -I$(OUTPUT)util/intel-pt-decoder
> +
> +$(OUTPUT)util/intel-pt-decoder/insn.o: $(srctree)/tools/arch/x86/lib/insn.c
> +	$(call rule_mkdir)
> +	$(call if_changed_dep,cc_o_c)
>  
>  ifeq ($(CC_NO_CLANG), 1)
> -  CFLAGS_intel-pt-insn-decoder.o += -Wno-override-init
> +  CFLAGS_insn.o += -Wno-override-init
>  endif
>  
> -CFLAGS_intel-pt-insn-decoder.o += -Wno-packed
> +CFLAGS_insn.o += -Wno-packed
> diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
> index 47cf35799a4d..8fabddc1c0da 100644
> --- a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
> +++ b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
> @@ -11,9 +11,6 @@
>  #include <byteswap.h>
>  #include "../../../arch/x86/include/asm/insn.h"
>  
> -#include "../../../arch/x86/lib/inat.c"
> -#include "../../../arch/x86/lib/insn.c"
> -
>  #include "event.h"
>  
>  #include "intel-pt-insn-decoder.h"
> diff --git a/tools/perf/util/trace-event-scripting.c b/tools/perf/util/trace-event-scripting.c
> index 2e9da0b089ef..8d71998a9ecd 100644
> --- a/tools/perf/util/trace-event-scripting.c
> +++ b/tools/perf/util/trace-event-scripting.c
> @@ -272,11 +272,13 @@ void setup_perl_scripting(void)
>  #endif
>  #endif
>  
> -__weak void arch_fetch_insn(struct perf_sample *sample __maybe_unused,
> +#if !defined(__i386__) && !defined(__x86_64__)
> +void arch_fetch_insn(struct perf_sample *sample __maybe_unused,
>  		     struct thread *thread __maybe_unused,
>  		     struct machine *machine __maybe_unused)
>  {
>  }
> +#endif
>  
>  void script_fetch_insn(struct perf_sample *sample, struct thread *thread,
>  		       struct machine *machine, bool native_arch)