[PATCH 0/2] perf: tests: Fix object code reading test for riscv

linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/2] perf: tests: Fix object code reading test for riscv
@ 2024-12-16 23:12 Charlie Jenkins
  2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
  2024-12-16 23:12 ` [PATCH 2/2] tools: perf: tests: Fix code reading for riscv Charlie Jenkins
  0 siblings, 2 replies; 10+ messages in thread
From: Charlie Jenkins @ 2024-12-16 23:12 UTC (permalink / raw)
  To: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu
  Cc: linux-kernel, linux-kbuild, linux-riscv, llvm, linux-perf-users,
	linux-security-module, Charlie Jenkins

There was a breaking change to binutils objdump that causes partial
instructions to no longer be dumped. This behavior is different from
what the "Object code reading" test expects. Add a Kconfig variable that
checks the version of objdump and conditionally enables the perf test
fix for riscv objdump versions effected by this issue.

A binutils patch has been sent as well to fix this in objdump [1].

Link:
https://sourceware.org/pipermail/binutils/2024-December/138139.html [1]

To: 

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Charlie Jenkins (2):
      kbuild: Check version of objdump
      tools: perf: tests: Fix code reading for riscv

 arch/riscv/Kconfig              |  5 +++
 init/Kconfig                    | 10 ++++++
 scripts/Kconfig.include         |  6 ++++
 scripts/objdump-version.sh      | 69 +++++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/code-reading.c | 17 +++++++++-
 5 files changed, 106 insertions(+), 1 deletion(-)
---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85
-- 
- Charlie


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] kbuild: Check version of objdump
  2024-12-16 23:12 [PATCH 0/2] perf: tests: Fix object code reading test for riscv Charlie Jenkins
@ 2024-12-16 23:12 ` Charlie Jenkins
  2024-12-18 15:14   ` Conor Dooley
                     ` (2 more replies)
  2024-12-16 23:12 ` [PATCH 2/2] tools: perf: tests: Fix code reading for riscv Charlie Jenkins
  1 sibling, 3 replies; 10+ messages in thread
From: Charlie Jenkins @ 2024-12-16 23:12 UTC (permalink / raw)
  To: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu
  Cc: linux-kernel, linux-kbuild, linux-riscv, llvm, linux-perf-users,
	linux-security-module, Charlie Jenkins

Similar to ld-version, add a way to check the version of objdump. This
should most of the time end up being the binutils version or the llvm
version.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 init/Kconfig               | 10 +++++++
 scripts/Kconfig.include    |  6 ++++
 scripts/objdump-version.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index a20e6efd3f0fbdd7f0df2448854cc30734a0ee4f..0b5d36f939e1de89c12ebdd61e4815015314d4f1 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -60,6 +60,16 @@ config LLD_VERSION
 	default $(ld-version) if LD_IS_LLD
 	default 0
 
+config OBJDUMP_IS_GNU
+	def_bool $(success,test "$(objdump-name)" = objdump)
+
+config OBJDUMP_IS_LLVM
+	def_bool $(success,test "$(objdump-name)" = llvm-objdump)
+
+config OBJDUMP_VERSION
+	int
+	default $(objdump-version)
+
 config RUSTC_VERSION
 	int
 	default $(rustc-version)
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index 33193ca6e8030e659d6b321acaea1acd42c387a4..cb3e2d2564fea8cce780adb3be672c9596b7ccf2 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -58,6 +58,12 @@ $(error-if,$(success,test -z "$(ld-info)"),Sorry$(comma) this linker is not supp
 ld-name := $(shell,set -- $(ld-info) && echo $1)
 ld-version := $(shell,set -- $(ld-info) && echo $2)
 
+# Get the objdump name, version, and error out if it is not supported.
+objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
+$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
+objdump-name := $(shell,set -- $(objdump-info) && echo $1)
+objdump-version := $(shell,set -- $(objdump-info) && echo $2)
+
 # machine bit flags
 #  $(m32-flag): -m32 if the compiler supports it, or an empty string otherwise.
 #  $(m64-flag): -m64 if the compiler supports it, or an empty string otherwise.
diff --git a/scripts/objdump-version.sh b/scripts/objdump-version.sh
new file mode 100755
index 0000000000000000000000000000000000000000..fa24f8dc2d3c42fd1195fceb3c96b27f7127db25
--- /dev/null
+++ b/scripts/objdump-version.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Print the objdump name and its version in a 5 or 6-digit form.
+# Also, perform the minimum version check.
+
+set -e
+
+# Convert the version string x.y.z to a canonical 5 or 6-digit form.
+get_canonical_version()
+{
+	IFS=.
+	set -- $1
+
+	# If the 2nd or 3rd field is missing, fill it with a zero.
+	#
+	# The 4th field, if present, is ignored.
+	# This occurs in development snapshots as in 2.35.1.20201116
+	echo $((10000 * $1 + 100 * ${2:-0} + ${3:-0}))
+}
+
+orig_args="$@"
+
+# Get the first line of the --version output.
+IFS='
+'
+set -- $(LC_ALL=C "$@" --version)
+
+# Split the line on spaces.
+IFS=' '
+set -- $1
+
+min_tool_version=$(dirname $0)/min-tool-version.sh
+
+if [ "$1" = GNU -a "$2" = objdump ]; then
+	shift $(($# - 1))
+	version=$1
+	min_version=$($min_tool_version binutils)
+	disp_name="GNU objdump"
+else
+	while [ $# -gt 1 -a "$1" != "LLVM" ]; do
+		shift
+	done
+
+	if [ "$1" = LLVM ]; then
+		version=$3
+		min_version=$($min_tool_version llvm)
+		disp_name="llvm-objdump"
+	else
+		echo "$orig_args: unknown objdump" >&2
+		exit 1
+	fi
+fi
+
+version=${version%%[!0-9.]*}
+
+cversion=$(get_canonical_version $version)
+min_cversion=$(get_canonical_version $min_version)
+
+if [ "$cversion" -lt "$min_cversion" ]; then
+	echo >&2 "***"
+	echo >&2 "*** objdump is too old."
+	echo >&2 "***   Your $disp_name version:    $version"
+	echo >&2 "***   Minimum $disp_name version: $min_version"
+	echo >&2 "***"
+	exit 1
+fi
+
+echo objdump $cversion

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] kbuild: Check version of objdump
  2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
@ 2024-12-18 15:14   ` Conor Dooley
  2024-12-18 15:40     ` Conor Dooley
  2024-12-21  6:49   ` Masahiro Yamada
  2024-12-23 16:33   ` kernel test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Conor Dooley @ 2024-12-18 15:14 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu, linux-kernel, linux-kbuild,
	linux-riscv, llvm, linux-perf-users, linux-security-module

[-- Attachment #1: Type: text/plain, Size: 4449 bytes --]

On Mon, Dec 16, 2024 at 03:12:51PM -0800, Charlie Jenkins wrote:
> Similar to ld-version, add a way to check the version of objdump. This
> should most of the time end up being the binutils version or the llvm
> version.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

This fails for allmodconfig and rv32_defconfig with clang. 19.1.1
according to Bjorn :)

Cheers,
Conor.

> ---
>  init/Kconfig               | 10 +++++++
>  scripts/Kconfig.include    |  6 ++++
>  scripts/objdump-version.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index a20e6efd3f0fbdd7f0df2448854cc30734a0ee4f..0b5d36f939e1de89c12ebdd61e4815015314d4f1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -60,6 +60,16 @@ config LLD_VERSION
>  	default $(ld-version) if LD_IS_LLD
>  	default 0
>  
> +config OBJDUMP_IS_GNU
> +	def_bool $(success,test "$(objdump-name)" = objdump)
> +
> +config OBJDUMP_IS_LLVM
> +	def_bool $(success,test "$(objdump-name)" = llvm-objdump)
> +
> +config OBJDUMP_VERSION
> +	int
> +	default $(objdump-version)
> +
>  config RUSTC_VERSION
>  	int
>  	default $(rustc-version)
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index 33193ca6e8030e659d6b321acaea1acd42c387a4..cb3e2d2564fea8cce780adb3be672c9596b7ccf2 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -58,6 +58,12 @@ $(error-if,$(success,test -z "$(ld-info)"),Sorry$(comma) this linker is not supp
>  ld-name := $(shell,set -- $(ld-info) && echo $1)
>  ld-version := $(shell,set -- $(ld-info) && echo $2)
>  
> +# Get the objdump name, version, and error out if it is not supported.
> +objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
> +$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
> +objdump-name := $(shell,set -- $(objdump-info) && echo $1)
> +objdump-version := $(shell,set -- $(objdump-info) && echo $2)
> +
>  # machine bit flags
>  #  $(m32-flag): -m32 if the compiler supports it, or an empty string otherwise.
>  #  $(m64-flag): -m64 if the compiler supports it, or an empty string otherwise.
> diff --git a/scripts/objdump-version.sh b/scripts/objdump-version.sh
> new file mode 100755
> index 0000000000000000000000000000000000000000..fa24f8dc2d3c42fd1195fceb3c96b27f7127db25
> --- /dev/null
> +++ b/scripts/objdump-version.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Print the objdump name and its version in a 5 or 6-digit form.
> +# Also, perform the minimum version check.
> +
> +set -e
> +
> +# Convert the version string x.y.z to a canonical 5 or 6-digit form.
> +get_canonical_version()
> +{
> +	IFS=.
> +	set -- $1
> +
> +	# If the 2nd or 3rd field is missing, fill it with a zero.
> +	#
> +	# The 4th field, if present, is ignored.
> +	# This occurs in development snapshots as in 2.35.1.20201116
> +	echo $((10000 * $1 + 100 * ${2:-0} + ${3:-0}))
> +}
> +
> +orig_args="$@"
> +
> +# Get the first line of the --version output.
> +IFS='
> +'
> +set -- $(LC_ALL=C "$@" --version)
> +
> +# Split the line on spaces.
> +IFS=' '
> +set -- $1
> +
> +min_tool_version=$(dirname $0)/min-tool-version.sh
> +
> +if [ "$1" = GNU -a "$2" = objdump ]; then
> +	shift $(($# - 1))
> +	version=$1
> +	min_version=$($min_tool_version binutils)
> +	disp_name="GNU objdump"
> +else
> +	while [ $# -gt 1 -a "$1" != "LLVM" ]; do
> +		shift
> +	done
> +
> +	if [ "$1" = LLVM ]; then
> +		version=$3
> +		min_version=$($min_tool_version llvm)
> +		disp_name="llvm-objdump"
> +	else
> +		echo "$orig_args: unknown objdump" >&2
> +		exit 1
> +	fi
> +fi
> +
> +version=${version%%[!0-9.]*}
> +
> +cversion=$(get_canonical_version $version)
> +min_cversion=$(get_canonical_version $min_version)
> +
> +if [ "$cversion" -lt "$min_cversion" ]; then
> +	echo >&2 "***"
> +	echo >&2 "*** objdump is too old."
> +	echo >&2 "***   Your $disp_name version:    $version"
> +	echo >&2 "***   Minimum $disp_name version: $min_version"
> +	echo >&2 "***"
> +	exit 1
> +fi
> +
> +echo objdump $cversion
> 
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] kbuild: Check version of objdump
  2024-12-18 15:14   ` Conor Dooley
@ 2024-12-18 15:40     ` Conor Dooley
  2024-12-18 21:55       ` Charlie Jenkins
  0 siblings, 1 reply; 10+ messages in thread
From: Conor Dooley @ 2024-12-18 15:40 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu, linux-kernel, linux-kbuild,
	linux-riscv, llvm, linux-perf-users, linux-security-module

[-- Attachment #1: Type: text/plain, Size: 5310 bytes --]

On Wed, Dec 18, 2024 at 03:14:46PM +0000, Conor Dooley wrote:
> On Mon, Dec 16, 2024 at 03:12:51PM -0800, Charlie Jenkins wrote:
> > Similar to ld-version, add a way to check the version of objdump. This
> > should most of the time end up being the binutils version or the llvm
> > version.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> 
> This fails for allmodconfig and rv32_defconfig with clang. 19.1.1
> according to Bjorn :)

Some additional info from Bjorn:
https://paste.debian.net/1340410
and the steps to reproduce:
https://paste.debian.net/1340408

That should not be reporting 13.0.1, it should be 19.1.x, there's one
included in the toolchains we use from https://mirrors.edge.kernel.org/pub/tools/llvm/

13.0.1 looks like a host toolchain?

> 
> Cheers,
> Conor.
> 
> > ---
> >  init/Kconfig               | 10 +++++++
> >  scripts/Kconfig.include    |  6 ++++
> >  scripts/objdump-version.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 85 insertions(+)
> > 
> > diff --git a/init/Kconfig b/init/Kconfig
> > index a20e6efd3f0fbdd7f0df2448854cc30734a0ee4f..0b5d36f939e1de89c12ebdd61e4815015314d4f1 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -60,6 +60,16 @@ config LLD_VERSION
> >  	default $(ld-version) if LD_IS_LLD
> >  	default 0
> >  
> > +config OBJDUMP_IS_GNU
> > +	def_bool $(success,test "$(objdump-name)" = objdump)
> > +
> > +config OBJDUMP_IS_LLVM
> > +	def_bool $(success,test "$(objdump-name)" = llvm-objdump)
> > +
> > +config OBJDUMP_VERSION
> > +	int
> > +	default $(objdump-version)
> > +
> >  config RUSTC_VERSION
> >  	int
> >  	default $(rustc-version)
> > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> > index 33193ca6e8030e659d6b321acaea1acd42c387a4..cb3e2d2564fea8cce780adb3be672c9596b7ccf2 100644
> > --- a/scripts/Kconfig.include
> > +++ b/scripts/Kconfig.include
> > @@ -58,6 +58,12 @@ $(error-if,$(success,test -z "$(ld-info)"),Sorry$(comma) this linker is not supp
> >  ld-name := $(shell,set -- $(ld-info) && echo $1)
> >  ld-version := $(shell,set -- $(ld-info) && echo $2)
> >  
> > +# Get the objdump name, version, and error out if it is not supported.
> > +objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
> > +$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
> > +objdump-name := $(shell,set -- $(objdump-info) && echo $1)
> > +objdump-version := $(shell,set -- $(objdump-info) && echo $2)
> > +
> >  # machine bit flags
> >  #  $(m32-flag): -m32 if the compiler supports it, or an empty string otherwise.
> >  #  $(m64-flag): -m64 if the compiler supports it, or an empty string otherwise.
> > diff --git a/scripts/objdump-version.sh b/scripts/objdump-version.sh
> > new file mode 100755
> > index 0000000000000000000000000000000000000000..fa24f8dc2d3c42fd1195fceb3c96b27f7127db25
> > --- /dev/null
> > +++ b/scripts/objdump-version.sh
> > @@ -0,0 +1,69 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Print the objdump name and its version in a 5 or 6-digit form.
> > +# Also, perform the minimum version check.
> > +
> > +set -e
> > +
> > +# Convert the version string x.y.z to a canonical 5 or 6-digit form.
> > +get_canonical_version()
> > +{
> > +	IFS=.
> > +	set -- $1
> > +
> > +	# If the 2nd or 3rd field is missing, fill it with a zero.
> > +	#
> > +	# The 4th field, if present, is ignored.
> > +	# This occurs in development snapshots as in 2.35.1.20201116
> > +	echo $((10000 * $1 + 100 * ${2:-0} + ${3:-0}))
> > +}
> > +
> > +orig_args="$@"
> > +
> > +# Get the first line of the --version output.
> > +IFS='
> > +'
> > +set -- $(LC_ALL=C "$@" --version)
> > +
> > +# Split the line on spaces.
> > +IFS=' '
> > +set -- $1
> > +
> > +min_tool_version=$(dirname $0)/min-tool-version.sh
> > +
> > +if [ "$1" = GNU -a "$2" = objdump ]; then
> > +	shift $(($# - 1))
> > +	version=$1
> > +	min_version=$($min_tool_version binutils)
> > +	disp_name="GNU objdump"
> > +else
> > +	while [ $# -gt 1 -a "$1" != "LLVM" ]; do
> > +		shift
> > +	done
> > +
> > +	if [ "$1" = LLVM ]; then
> > +		version=$3
> > +		min_version=$($min_tool_version llvm)
> > +		disp_name="llvm-objdump"
> > +	else
> > +		echo "$orig_args: unknown objdump" >&2
> > +		exit 1
> > +	fi
> > +fi
> > +
> > +version=${version%%[!0-9.]*}
> > +
> > +cversion=$(get_canonical_version $version)
> > +min_cversion=$(get_canonical_version $min_version)
> > +
> > +if [ "$cversion" -lt "$min_cversion" ]; then
> > +	echo >&2 "***"
> > +	echo >&2 "*** objdump is too old."
> > +	echo >&2 "***   Your $disp_name version:    $version"
> > +	echo >&2 "***   Minimum $disp_name version: $min_version"
> > +	echo >&2 "***"
> > +	exit 1
> > +fi
> > +
> > +echo objdump $cversion
> > 
> > -- 
> > 2.34.1
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv



> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] kbuild: Check version of objdump
  2024-12-18 15:40     ` Conor Dooley
@ 2024-12-18 21:55       ` Charlie Jenkins
  0 siblings, 0 replies; 10+ messages in thread
From: Charlie Jenkins @ 2024-12-18 21:55 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu, linux-kernel, linux-kbuild,
	linux-riscv, llvm, linux-perf-users, linux-security-module

On Wed, Dec 18, 2024 at 03:40:25PM +0000, Conor Dooley wrote:
> On Wed, Dec 18, 2024 at 03:14:46PM +0000, Conor Dooley wrote:
> > On Mon, Dec 16, 2024 at 03:12:51PM -0800, Charlie Jenkins wrote:
> > > Similar to ld-version, add a way to check the version of objdump. This
> > > should most of the time end up being the binutils version or the llvm
> > > version.
> > > 
> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > 
> > This fails for allmodconfig and rv32_defconfig with clang. 19.1.1
> > according to Bjorn :)
> 
> Some additional info from Bjorn:
> https://paste.debian.net/1340410
> and the steps to reproduce:
> https://paste.debian.net/1340408
> 
> That should not be reporting 13.0.1, it should be 19.1.x, there's one
> included in the toolchains we use from https://mirrors.edge.kernel.org/pub/tools/llvm/
> 
> 13.0.1 looks like a host toolchain?

I ended up sending a v2 that dropped this in favor of detecting this at
runtime [1] so this is no longer needed.

- Charlie

Link:
https://lore.kernel.org/lkml/20241217-perf_fix_riscv_obj_reading-v2-1-58f81b7b4c7d@rivosinc.com/
[1]

> 
> > 
> > Cheers,
> > Conor.
> > 
> > > ---
> > >  init/Kconfig               | 10 +++++++
> > >  scripts/Kconfig.include    |  6 ++++
> > >  scripts/objdump-version.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 85 insertions(+)
> > > 
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index a20e6efd3f0fbdd7f0df2448854cc30734a0ee4f..0b5d36f939e1de89c12ebdd61e4815015314d4f1 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -60,6 +60,16 @@ config LLD_VERSION
> > >  	default $(ld-version) if LD_IS_LLD
> > >  	default 0
> > >  
> > > +config OBJDUMP_IS_GNU
> > > +	def_bool $(success,test "$(objdump-name)" = objdump)
> > > +
> > > +config OBJDUMP_IS_LLVM
> > > +	def_bool $(success,test "$(objdump-name)" = llvm-objdump)
> > > +
> > > +config OBJDUMP_VERSION
> > > +	int
> > > +	default $(objdump-version)
> > > +
> > >  config RUSTC_VERSION
> > >  	int
> > >  	default $(rustc-version)
> > > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> > > index 33193ca6e8030e659d6b321acaea1acd42c387a4..cb3e2d2564fea8cce780adb3be672c9596b7ccf2 100644
> > > --- a/scripts/Kconfig.include
> > > +++ b/scripts/Kconfig.include
> > > @@ -58,6 +58,12 @@ $(error-if,$(success,test -z "$(ld-info)"),Sorry$(comma) this linker is not supp
> > >  ld-name := $(shell,set -- $(ld-info) && echo $1)
> > >  ld-version := $(shell,set -- $(ld-info) && echo $2)
> > >  
> > > +# Get the objdump name, version, and error out if it is not supported.
> > > +objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
> > > +$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
> > > +objdump-name := $(shell,set -- $(objdump-info) && echo $1)
> > > +objdump-version := $(shell,set -- $(objdump-info) && echo $2)
> > > +
> > >  # machine bit flags
> > >  #  $(m32-flag): -m32 if the compiler supports it, or an empty string otherwise.
> > >  #  $(m64-flag): -m64 if the compiler supports it, or an empty string otherwise.
> > > diff --git a/scripts/objdump-version.sh b/scripts/objdump-version.sh
> > > new file mode 100755
> > > index 0000000000000000000000000000000000000000..fa24f8dc2d3c42fd1195fceb3c96b27f7127db25
> > > --- /dev/null
> > > +++ b/scripts/objdump-version.sh
> > > @@ -0,0 +1,69 @@
> > > +#!/bin/sh
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +#
> > > +# Print the objdump name and its version in a 5 or 6-digit form.
> > > +# Also, perform the minimum version check.
> > > +
> > > +set -e
> > > +
> > > +# Convert the version string x.y.z to a canonical 5 or 6-digit form.
> > > +get_canonical_version()
> > > +{
> > > +	IFS=.
> > > +	set -- $1
> > > +
> > > +	# If the 2nd or 3rd field is missing, fill it with a zero.
> > > +	#
> > > +	# The 4th field, if present, is ignored.
> > > +	# This occurs in development snapshots as in 2.35.1.20201116
> > > +	echo $((10000 * $1 + 100 * ${2:-0} + ${3:-0}))
> > > +}
> > > +
> > > +orig_args="$@"
> > > +
> > > +# Get the first line of the --version output.
> > > +IFS='
> > > +'
> > > +set -- $(LC_ALL=C "$@" --version)
> > > +
> > > +# Split the line on spaces.
> > > +IFS=' '
> > > +set -- $1
> > > +
> > > +min_tool_version=$(dirname $0)/min-tool-version.sh
> > > +
> > > +if [ "$1" = GNU -a "$2" = objdump ]; then
> > > +	shift $(($# - 1))
> > > +	version=$1
> > > +	min_version=$($min_tool_version binutils)
> > > +	disp_name="GNU objdump"
> > > +else
> > > +	while [ $# -gt 1 -a "$1" != "LLVM" ]; do
> > > +		shift
> > > +	done
> > > +
> > > +	if [ "$1" = LLVM ]; then
> > > +		version=$3
> > > +		min_version=$($min_tool_version llvm)
> > > +		disp_name="llvm-objdump"
> > > +	else
> > > +		echo "$orig_args: unknown objdump" >&2
> > > +		exit 1
> > > +	fi
> > > +fi
> > > +
> > > +version=${version%%[!0-9.]*}
> > > +
> > > +cversion=$(get_canonical_version $version)
> > > +min_cversion=$(get_canonical_version $min_version)
> > > +
> > > +if [ "$cversion" -lt "$min_cversion" ]; then
> > > +	echo >&2 "***"
> > > +	echo >&2 "*** objdump is too old."
> > > +	echo >&2 "***   Your $disp_name version:    $version"
> > > +	echo >&2 "***   Minimum $disp_name version: $min_version"
> > > +	echo >&2 "***"
> > > +	exit 1
> > > +fi
> > > +
> > > +echo objdump $cversion
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] kbuild: Check version of objdump
  2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
  2024-12-18 15:14   ` Conor Dooley
@ 2024-12-21  6:49   ` Masahiro Yamada
  2024-12-23 16:33   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: Masahiro Yamada @ 2024-12-21  6:49 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Nathan Chancellor,
	Nick Desaulniers, Bill Wendling, Justin Stitt, Peter Zijlstra,
	Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	Mickaël Salaün, Günther Noack, Nelson Chu,
	linux-kernel, linux-kbuild, linux-riscv, llvm, linux-perf-users,
	linux-security-module

On Tue, Dec 17, 2024 at 8:13 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> Similar to ld-version, add a way to check the version of objdump. This
> should most of the time end up being the binutils version or the llvm
> version.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>

This patch has no point because 2/2 includes CONFIG option
from the userspace "perf".




> ---
>  init/Kconfig               | 10 +++++++
>  scripts/Kconfig.include    |  6 ++++
>  scripts/objdump-version.sh | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index a20e6efd3f0fbdd7f0df2448854cc30734a0ee4f..0b5d36f939e1de89c12ebdd61e4815015314d4f1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -60,6 +60,16 @@ config LLD_VERSION
>         default $(ld-version) if LD_IS_LLD
>         default 0
>
> +config OBJDUMP_IS_GNU
> +       def_bool $(success,test "$(objdump-name)" = objdump)
> +
> +config OBJDUMP_IS_LLVM
> +       def_bool $(success,test "$(objdump-name)" = llvm-objdump)
> +
> +config OBJDUMP_VERSION
> +       int
> +       default $(objdump-version)
> +
>  config RUSTC_VERSION
>         int
>         default $(rustc-version)
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index 33193ca6e8030e659d6b321acaea1acd42c387a4..cb3e2d2564fea8cce780adb3be672c9596b7ccf2 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -58,6 +58,12 @@ $(error-if,$(success,test -z "$(ld-info)"),Sorry$(comma) this linker is not supp
>  ld-name := $(shell,set -- $(ld-info) && echo $1)
>  ld-version := $(shell,set -- $(ld-info) && echo $2)
>
> +# Get the objdump name, version, and error out if it is not supported.
> +objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
> +$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
> +objdump-name := $(shell,set -- $(objdump-info) && echo $1)
> +objdump-version := $(shell,set -- $(objdump-info) && echo $2)
> +
>  # machine bit flags
>  #  $(m32-flag): -m32 if the compiler supports it, or an empty string otherwise.
>  #  $(m64-flag): -m64 if the compiler supports it, or an empty string otherwise.
> diff --git a/scripts/objdump-version.sh b/scripts/objdump-version.sh
> new file mode 100755
> index 0000000000000000000000000000000000000000..fa24f8dc2d3c42fd1195fceb3c96b27f7127db25
> --- /dev/null
> +++ b/scripts/objdump-version.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Print the objdump name and its version in a 5 or 6-digit form.
> +# Also, perform the minimum version check.
> +
> +set -e
> +
> +# Convert the version string x.y.z to a canonical 5 or 6-digit form.
> +get_canonical_version()
> +{
> +       IFS=.
> +       set -- $1
> +
> +       # If the 2nd or 3rd field is missing, fill it with a zero.
> +       #
> +       # The 4th field, if present, is ignored.
> +       # This occurs in development snapshots as in 2.35.1.20201116
> +       echo $((10000 * $1 + 100 * ${2:-0} + ${3:-0}))
> +}
> +
> +orig_args="$@"
> +
> +# Get the first line of the --version output.
> +IFS='
> +'
> +set -- $(LC_ALL=C "$@" --version)
> +
> +# Split the line on spaces.
> +IFS=' '
> +set -- $1
> +
> +min_tool_version=$(dirname $0)/min-tool-version.sh
> +
> +if [ "$1" = GNU -a "$2" = objdump ]; then
> +       shift $(($# - 1))
> +       version=$1
> +       min_version=$($min_tool_version binutils)
> +       disp_name="GNU objdump"
> +else
> +       while [ $# -gt 1 -a "$1" != "LLVM" ]; do
> +               shift
> +       done
> +
> +       if [ "$1" = LLVM ]; then
> +               version=$3
> +               min_version=$($min_tool_version llvm)
> +               disp_name="llvm-objdump"
> +       else
> +               echo "$orig_args: unknown objdump" >&2
> +               exit 1
> +       fi
> +fi
> +
> +version=${version%%[!0-9.]*}
> +
> +cversion=$(get_canonical_version $version)
> +min_cversion=$(get_canonical_version $min_version)
> +
> +if [ "$cversion" -lt "$min_cversion" ]; then
> +       echo >&2 "***"
> +       echo >&2 "*** objdump is too old."
> +       echo >&2 "***   Your $disp_name version:    $version"
> +       echo >&2 "***   Minimum $disp_name version: $min_version"
> +       echo >&2 "***"
> +       exit 1
> +fi
> +
> +echo objdump $cversion
>
> --
> 2.34.1
>


--
Best Regards


Masahiro Yamada

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] kbuild: Check version of objdump
  2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
  2024-12-18 15:14   ` Conor Dooley
  2024-12-21  6:49   ` Masahiro Yamada
@ 2024-12-23 16:33   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2024-12-23 16:33 UTC (permalink / raw)
  To: Charlie Jenkins, Masahiro Yamada, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, Nathan Chancellor, Nick Desaulniers, Bill Wendling,
	Justin Stitt, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	Mickaël Salaün, Günther Noack, Nelson Chu
  Cc: llvm, oe-kbuild-all, linux-kernel, linux-kbuild, linux-riscv,
	linux-perf-users, linux-security-module, Charlie Jenkins

Hi Charlie,

kernel test robot noticed the following build errors:

[auto build test ERROR on fac04efc5c793dccbd07e2d59af9f90b7fc0dca4]

url:    https://github.com/intel-lab-lkp/linux/commits/Charlie-Jenkins/kbuild-Check-version-of-objdump/20241217-071632
base:   fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
patch link:    https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-1-b75962660a9b%40rivosinc.com
patch subject: [PATCH 1/2] kbuild: Check version of objdump
config: x86_64-randconfig-003-20241223 (attached as .config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241224/202412240044.wVXW8Eg1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412240044.wVXW8Eg1-lkp@intel.com/

All errors (new ones prefixed by >>):

   ***
   *** objdump is too old.
   ***   Your llvm-objdump version:    
   ***   Minimum llvm-objdump version: 13.0.1
   ***
>> scripts/Kconfig.include:63: Sorry, this objdump is not supported.
   make[3]: *** [scripts/kconfig/Makefile:85: oldconfig] Error 1 shuffle=1831414175
   make[2]: *** [Makefile:733: oldconfig] Error 2 shuffle=1831414175
   make[1]: *** [Makefile:251: __sub-make] Error 2 shuffle=1831414175
   make[1]: Target 'oldconfig' not remade because of errors.
   make: *** [Makefile:251: __sub-make] Error 2 shuffle=1831414175
   make: Target 'oldconfig' not remade because of errors.
--
   ***
   *** objdump is too old.
   ***   Your llvm-objdump version:    
   ***   Minimum llvm-objdump version: 13.0.1
   ***
>> scripts/Kconfig.include:63: Sorry, this objdump is not supported.
   make[3]: *** [scripts/kconfig/Makefile:85: olddefconfig] Error 1 shuffle=1831414175
   make[2]: *** [Makefile:733: olddefconfig] Error 2 shuffle=1831414175
   make[1]: *** [Makefile:251: __sub-make] Error 2 shuffle=1831414175
   make[1]: Target 'olddefconfig' not remade because of errors.
   make: *** [Makefile:251: __sub-make] Error 2 shuffle=1831414175
   make: Target 'olddefconfig' not remade because of errors.


vim +63 scripts/Kconfig.include

    60	
    61	# Get the objdump name, version, and error out if it is not supported.
    62	objdump-info := $(shell,$(srctree)/scripts/objdump-version.sh $(OBJDUMP))
  > 63	$(error-if,$(success,test -z "$(objdump-info)"),Sorry$(comma) this objdump is not supported.)
    64	objdump-name := $(shell,set -- $(objdump-info) && echo $1)
    65	objdump-version := $(shell,set -- $(objdump-info) && echo $2)
    66	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] tools: perf: tests: Fix code reading for riscv
  2024-12-16 23:12 [PATCH 0/2] perf: tests: Fix object code reading test for riscv Charlie Jenkins
  2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
@ 2024-12-16 23:12 ` Charlie Jenkins
  2024-12-17  4:57   ` Ian Rogers
  1 sibling, 1 reply; 10+ messages in thread
From: Charlie Jenkins @ 2024-12-16 23:12 UTC (permalink / raw)
  To: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Mickaël Salaün,
	Günther Noack, Nelson Chu
  Cc: linux-kernel, linux-kbuild, linux-riscv, llvm, linux-perf-users,
	linux-security-module, Charlie Jenkins

After binutils commit e43d876 which was first included in binutils 2.41,
riscv no longer supports dumping in the middle of instructions. Increase
the objdump window by 2-bytes to ensure that any instruction that sits
on the boundary of the specified stop-address is not cut in half.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/Kconfig              |  5 +++++
 tools/perf/tests/code-reading.c | 17 ++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d4a7ca0388c071b536df59c0eb11d55f9080c7cd..f164047471267936bc62389b7d7d9a7cbdca8f97 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -229,6 +229,11 @@ config GCC_SUPPORTS_DYNAMIC_FTRACE
 	def_bool CC_IS_GCC
 	depends on $(cc-option,-fpatchable-function-entry=8)
 
+config RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION
+	# Some versions of objdump do not support dumping partial instructions
+	def_bool y
+	depends on !(OBJDUMP_IS_GNU && OBJDUMP_VERSION > 24100)
+
 config HAVE_SHADOW_CALL_STACK
 	def_bool $(cc-option,-fsanitize=shadow-call-stack)
 	# https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/a484e843e6eeb51f0cb7b8819e50da6d2444d769
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..605f4a8e1dbc00d8a572503f45053c2f30ad19e3 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
+#include <linux/kconfig.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <inttypes.h>
@@ -183,9 +184,23 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
 	const char *fmt;
 	FILE *f;
 	int ret;
+	u64 stop_address = addr + len;
+
+	if (IS_ENABLED(__riscv) && !IS_ENABLED(CONFIG_RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION)) {
+		/*
+		 * On some versions of riscv objdump, dumping in the middle of
+		 * instructions is not supported. riscv instructions are aligned along
+		 * 2-byte intervals and can be either 2-bytes or 4-bytes. This makes it
+		 * possible that the stop-address lands in the middle of a 4-byte
+		 * instruction. Increase the stop_address by two to ensure an
+		 * instruction is not cut in half, but leave the len as-is so only the
+		 * expected number of bytes are collected.
+		 */
+		stop_address += 2;
+	}
 
 	fmt = "%s -z -d --start-address=0x%"PRIx64" --stop-address=0x%"PRIx64" %s";
-	ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, addr + len,
+	ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, stop_address,
 		       filename);
 	if (ret <= 0 || (size_t)ret >= sizeof(cmd))
 		return -1;

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] tools: perf: tests: Fix code reading for riscv
  2024-12-16 23:12 ` [PATCH 2/2] tools: perf: tests: Fix code reading for riscv Charlie Jenkins
@ 2024-12-17  4:57   ` Ian Rogers
  2024-12-17  6:44     ` Charlie Jenkins
  0 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2024-12-17  4:57 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Mickaël Salaün, Günther Noack,
	Nelson Chu, linux-kernel, linux-kbuild, linux-riscv, llvm,
	linux-perf-users, linux-security-module

On Mon, Dec 16, 2024 at 3:13 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> After binutils commit e43d876 which was first included in binutils 2.41,
> riscv no longer supports dumping in the middle of instructions. Increase
> the objdump window by 2-bytes to ensure that any instruction that sits
> on the boundary of the specified stop-address is not cut in half.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/Kconfig              |  5 +++++
>  tools/perf/tests/code-reading.c | 17 ++++++++++++++++-

Files under tools use a different Build system than the kernel. The
Kconfig value won't have an effect. Check out Makefile.config:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Makefile.config?h=perf-tools-next
which is included into the build here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Makefile.perf?h=perf-tools-next#n313

>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d4a7ca0388c071b536df59c0eb11d55f9080c7cd..f164047471267936bc62389b7d7d9a7cbdca8f97 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -229,6 +229,11 @@ config GCC_SUPPORTS_DYNAMIC_FTRACE
>         def_bool CC_IS_GCC
>         depends on $(cc-option,-fpatchable-function-entry=8)
>
> +config RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION
> +       # Some versions of objdump do not support dumping partial instructions
> +       def_bool y
> +       depends on !(OBJDUMP_IS_GNU && OBJDUMP_VERSION > 24100)
> +
>  config HAVE_SHADOW_CALL_STACK
>         def_bool $(cc-option,-fsanitize=shadow-call-stack)
>         # https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/a484e843e6eeb51f0cb7b8819e50da6d2444d769
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..605f4a8e1dbc00d8a572503f45053c2f30ad19e3 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <errno.h>
> +#include <linux/kconfig.h>
>  #include <linux/kernel.h>
>  #include <linux/types.h>
>  #include <inttypes.h>
> @@ -183,9 +184,23 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
>         const char *fmt;
>         FILE *f;
>         int ret;
> +       u64 stop_address = addr + len;
> +
> +       if (IS_ENABLED(__riscv) && !IS_ENABLED(CONFIG_RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION)) {

It would be nice if this could be a runtime rather than build time detected.

Thanks,
Ian

> +               /*
> +                * On some versions of riscv objdump, dumping in the middle of
> +                * instructions is not supported. riscv instructions are aligned along
> +                * 2-byte intervals and can be either 2-bytes or 4-bytes. This makes it
> +                * possible that the stop-address lands in the middle of a 4-byte
> +                * instruction. Increase the stop_address by two to ensure an
> +                * instruction is not cut in half, but leave the len as-is so only the
> +                * expected number of bytes are collected.
> +                */
> +               stop_address += 2;
> +       }
>
>         fmt = "%s -z -d --start-address=0x%"PRIx64" --stop-address=0x%"PRIx64" %s";
> -       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, addr + len,
> +       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, stop_address,
>                        filename);
>         if (ret <= 0 || (size_t)ret >= sizeof(cmd))
>                 return -1;
>
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] tools: perf: tests: Fix code reading for riscv
  2024-12-17  4:57   ` Ian Rogers
@ 2024-12-17  6:44     ` Charlie Jenkins
  0 siblings, 0 replies; 10+ messages in thread
From: Charlie Jenkins @ 2024-12-17  6:44 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Masahiro Yamada, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
	Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Mickaël Salaün, Günther Noack,
	Nelson Chu, linux-kernel, linux-kbuild, linux-riscv, llvm,
	linux-perf-users, linux-security-module

On Mon, Dec 16, 2024 at 08:57:20PM -0800, Ian Rogers wrote:
> On Mon, Dec 16, 2024 at 3:13 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > After binutils commit e43d876 which was first included in binutils 2.41,
> > riscv no longer supports dumping in the middle of instructions. Increase
> > the objdump window by 2-bytes to ensure that any instruction that sits
> > on the boundary of the specified stop-address is not cut in half.
> >
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/Kconfig              |  5 +++++
> >  tools/perf/tests/code-reading.c | 17 ++++++++++++++++-
> 
> Files under tools use a different Build system than the kernel. The
> Kconfig value won't have an effect. Check out Makefile.config:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Makefile.config?h=perf-tools-next
> which is included into the build here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/Makefile.perf?h=perf-tools-next#n313
> 

Ahh okay, thank you. It was properly enabling when I was testing, is
there some bleeding over?

> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index d4a7ca0388c071b536df59c0eb11d55f9080c7cd..f164047471267936bc62389b7d7d9a7cbdca8f97 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -229,6 +229,11 @@ config GCC_SUPPORTS_DYNAMIC_FTRACE
> >         def_bool CC_IS_GCC
> >         depends on $(cc-option,-fpatchable-function-entry=8)
> >
> > +config RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION
> > +       # Some versions of objdump do not support dumping partial instructions
> > +       def_bool y
> > +       depends on !(OBJDUMP_IS_GNU && OBJDUMP_VERSION > 24100)
> > +
> >  config HAVE_SHADOW_CALL_STACK
> >         def_bool $(cc-option,-fsanitize=shadow-call-stack)
> >         # https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/a484e843e6eeb51f0cb7b8819e50da6d2444d769
> > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..605f4a8e1dbc00d8a572503f45053c2f30ad19e3 100644
> > --- a/tools/perf/tests/code-reading.c
> > +++ b/tools/perf/tests/code-reading.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <errno.h>
> > +#include <linux/kconfig.h>
> >  #include <linux/kernel.h>
> >  #include <linux/types.h>
> >  #include <inttypes.h>
> > @@ -183,9 +184,23 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
> >         const char *fmt;
> >         FILE *f;
> >         int ret;
> > +       u64 stop_address = addr + len;
> > +
> > +       if (IS_ENABLED(__riscv) && !IS_ENABLED(CONFIG_RISCV_OBJDUMP_SUPPORTS_SPLIT_INSTRUCTION)) {
> 
> It would be nice if this could be a runtime rather than build time detected.

Hmm that is a good point. I will change this to check the version at
runtime.

- Charlie

> 
> Thanks,
> Ian
> 
> > +               /*
> > +                * On some versions of riscv objdump, dumping in the middle of
> > +                * instructions is not supported. riscv instructions are aligned along
> > +                * 2-byte intervals and can be either 2-bytes or 4-bytes. This makes it
> > +                * possible that the stop-address lands in the middle of a 4-byte
> > +                * instruction. Increase the stop_address by two to ensure an
> > +                * instruction is not cut in half, but leave the len as-is so only the
> > +                * expected number of bytes are collected.
> > +                */
> > +               stop_address += 2;
> > +       }
> >
> >         fmt = "%s -z -d --start-address=0x%"PRIx64" --stop-address=0x%"PRIx64" %s";
> > -       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, addr + len,
> > +       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, stop_address,
> >                        filename);
> >         if (ret <= 0 || (size_t)ret >= sizeof(cmd))
> >                 return -1;
> >
> > --
> > 2.34.1
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-12-23 16:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-16 23:12 [PATCH 0/2] perf: tests: Fix object code reading test for riscv Charlie Jenkins
2024-12-16 23:12 ` [PATCH 1/2] kbuild: Check version of objdump Charlie Jenkins
2024-12-18 15:14   ` Conor Dooley
2024-12-18 15:40     ` Conor Dooley
2024-12-18 21:55       ` Charlie Jenkins
2024-12-21  6:49   ` Masahiro Yamada
2024-12-23 16:33   ` kernel test robot
2024-12-16 23:12 ` [PATCH 2/2] tools: perf: tests: Fix code reading for riscv Charlie Jenkins
2024-12-17  4:57   ` Ian Rogers
2024-12-17  6:44     ` Charlie Jenkins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).