[PATCH v2] tools: perf: tests: Fix code reading for riscv

[PATCH v2] tools: perf: tests: Fix code reading for riscv

[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>
---
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]
---
Changes in v2:
- Do objdump version detection at runtime (Ian)
- Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
---
 tools/perf/tests/code-reading.c | 84 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..7e24d10a543ac18ac2be70b829d088874e0edfd5 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>
@@ -176,6 +177,66 @@ static int read_objdump_output(FILE *f, void *buf, size_t *len, u64 start_addr)
 	return err;
 }
 
+/*
+ * Only gets GNU objdump version. Returns 0 for llvm-objdump.
+ */
+static int objdump_version(void)
+{
+	size_t line_len;
+	char cmd[PATH_MAX * 2];
+	char *line = NULL;
+	const char *fmt;
+	FILE *f;
+	int ret;
+
+	int version_tmp, version_num = 0;
+	char *version = 0, *token;
+
+	fmt = "%s --version";
+	ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path);
+	if (ret <= 0 || (size_t)ret >= sizeof(cmd))
+		return -1;
+	/* Ignore objdump errors */
+	strcat(cmd, " 2>/dev/null");
+	f = popen(cmd, "r");
+	if (!f) {
+		pr_debug("popen failed\n");
+		return -1;
+	}
+	/* Get first line of objdump --version output */
+	ret = getline(&line, &line_len, f);
+	pclose(f);
+	if (ret < 0) {
+		pr_debug("getline failed\n");
+		return -1;
+	}
+
+	token = strsep(&line, " ");
+	if (token != NULL && !strcmp(token, "GNU")) {
+		// version is last part of first line of objdump --version output.
+		while ((token = strsep(&line, " ")))
+			version = token;
+
+		// Convert version into a format we can compare with
+		token = strsep(&version, ".");
+		version_num = atoi(token);
+		if (version_num)
+			version_num *= 10000;
+
+		token = strsep(&version, ".");
+		version_tmp = atoi(token);
+		if (token)
+			version_num += version_tmp * 100;
+
+		token = strsep(&version, ".");
+		version_tmp = atoi(token);
+		if (token)
+			version_num += version_tmp;
+	}
+
+	return version_num;
+}
+
 static int read_via_objdump(const char *filename, u64 addr, void *buf,
 			    size_t len)
 {
@@ -183,9 +244,30 @@ 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)) {
+		int version = objdump_version();
+
+		/* Default to this workaround if version parsing fails */
+		if (version < 0 || version > 24100) {
+			/*
+			 * Starting at riscv objdump version 2.41, dumping in
+			 * the middle of an instruction 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;

---
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85
-- 
- Charlie

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Tue, Dec 17, 2024 at 3:52 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>

Reviewed-by: Ian Rogers <irogers@google.com>

> ---
> 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]
> ---
> Changes in v2:
> - Do objdump version detection at runtime (Ian)
> - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> ---
>  tools/perf/tests/code-reading.c | 84 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..7e24d10a543ac18ac2be70b829d088874e0edfd5 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>
> @@ -176,6 +177,66 @@ static int read_objdump_output(FILE *f, void *buf, size_t *len, u64 start_addr)
>         return err;
>  }
>
> +/*
> + * Only gets GNU objdump version. Returns 0 for llvm-objdump.
> + */
> +static int objdump_version(void)
> +{
> +       size_t line_len;
> +       char cmd[PATH_MAX * 2];
> +       char *line = NULL;
> +       const char *fmt;
> +       FILE *f;
> +       int ret;
> +
> +       int version_tmp, version_num = 0;
> +       char *version = 0, *token;
> +
> +       fmt = "%s --version";
> +       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path);
> +       if (ret <= 0 || (size_t)ret >= sizeof(cmd))
> +               return -1;
> +       /* Ignore objdump errors */
> +       strcat(cmd, " 2>/dev/null");
> +       f = popen(cmd, "r");
> +       if (!f) {
> +               pr_debug("popen failed\n");
> +               return -1;
> +       }
> +       /* Get first line of objdump --version output */
> +       ret = getline(&line, &line_len, f);
> +       pclose(f);
> +       if (ret < 0) {
> +               pr_debug("getline failed\n");
> +               return -1;
> +       }
> +
> +       token = strsep(&line, " ");
> +       if (token != NULL && !strcmp(token, "GNU")) {
> +               // version is last part of first line of objdump --version output.
> +               while ((token = strsep(&line, " ")))
> +                       version = token;
> +
> +               // Convert version into a format we can compare with
> +               token = strsep(&version, ".");
> +               version_num = atoi(token);
> +               if (version_num)
> +                       version_num *= 10000;
> +
> +               token = strsep(&version, ".");
> +               version_tmp = atoi(token);
> +               if (token)
> +                       version_num += version_tmp * 100;
> +
> +               token = strsep(&version, ".");
> +               version_tmp = atoi(token);
> +               if (token)
> +                       version_num += version_tmp;
> +       }
> +
> +       return version_num;
> +}
> +
>  static int read_via_objdump(const char *filename, u64 addr, void *buf,
>                             size_t len)
>  {
> @@ -183,9 +244,30 @@ 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)) {

Not sure if there is a consistency issue here. Elsewhere we're just
using ifdef, such as:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69

Thanks,
Ian

> +               int version = objdump_version();
> +
> +               /* Default to this workaround if version parsing fails */
> +               if (version < 0 || version > 24100) {
> +                       /*
> +                        * Starting at riscv objdump version 2.41, dumping in
> +                        * the middle of an instruction 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;
>
> ---
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85
> --
> - Charlie
>

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> On Tue, Dec 17, 2024 at 3:52 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>
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> 
> > ---
> > 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]
> > ---
> > Changes in v2:
> > - Do objdump version detection at runtime (Ian)
> > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > ---
> >  tools/perf/tests/code-reading.c | 84 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 83 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..7e24d10a543ac18ac2be70b829d088874e0edfd5 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>
> > @@ -176,6 +177,66 @@ static int read_objdump_output(FILE *f, void *buf, size_t *len, u64 start_addr)
> >         return err;
> >  }
> >
> > +/*
> > + * Only gets GNU objdump version. Returns 0 for llvm-objdump.
> > + */
> > +static int objdump_version(void)
> > +{
> > +       size_t line_len;
> > +       char cmd[PATH_MAX * 2];
> > +       char *line = NULL;
> > +       const char *fmt;
> > +       FILE *f;
> > +       int ret;
> > +
> > +       int version_tmp, version_num = 0;
> > +       char *version = 0, *token;
> > +
> > +       fmt = "%s --version";
> > +       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path);
> > +       if (ret <= 0 || (size_t)ret >= sizeof(cmd))
> > +               return -1;
> > +       /* Ignore objdump errors */
> > +       strcat(cmd, " 2>/dev/null");
> > +       f = popen(cmd, "r");
> > +       if (!f) {
> > +               pr_debug("popen failed\n");
> > +               return -1;
> > +       }
> > +       /* Get first line of objdump --version output */
> > +       ret = getline(&line, &line_len, f);
> > +       pclose(f);
> > +       if (ret < 0) {
> > +               pr_debug("getline failed\n");
> > +               return -1;
> > +       }
> > +
> > +       token = strsep(&line, " ");
> > +       if (token != NULL && !strcmp(token, "GNU")) {
> > +               // version is last part of first line of objdump --version output.
> > +               while ((token = strsep(&line, " ")))
> > +                       version = token;
> > +
> > +               // Convert version into a format we can compare with
> > +               token = strsep(&version, ".");
> > +               version_num = atoi(token);
> > +               if (version_num)
> > +                       version_num *= 10000;
> > +
> > +               token = strsep(&version, ".");
> > +               version_tmp = atoi(token);
> > +               if (token)
> > +                       version_num += version_tmp * 100;
> > +
> > +               token = strsep(&version, ".");
> > +               version_tmp = atoi(token);
> > +               if (token)
> > +                       version_num += version_tmp;
> > +       }
> > +
> > +       return version_num;
> > +}
> > +
> >  static int read_via_objdump(const char *filename, u64 addr, void *buf,
> >                             size_t len)
> >  {
> > @@ -183,9 +244,30 @@ 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)) {
> 
> Not sure if there is a consistency issue here. Elsewhere we're just
> using ifdef, such as:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69

I don't have any strong feelings about that. I can change it to be an
ifdef. On other lists I have been told to use IS_ENABLED whenever
possible, but it's only a small difference.

- Charlie


> 
> Thanks,
> Ian
> 
> > +               int version = objdump_version();
> > +
> > +               /* Default to this workaround if version parsing fails */
> > +               if (version < 0 || version > 24100) {
> > +                       /*
> > +                        * Starting at riscv objdump version 2.41, dumping in
> > +                        * the middle of an instruction 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;
> >
> > ---
> > base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> > change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85
> > --
> > - Charlie
> >

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Tue, Dec 17, 2024 at 4:30 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > On Tue, Dec 17, 2024 at 3:52 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>
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > > ---
> > > 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]
> > > ---
> > > Changes in v2:
> > > - Do objdump version detection at runtime (Ian)
> > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > > ---
> > >  tools/perf/tests/code-reading.c | 84 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 83 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > > index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..7e24d10a543ac18ac2be70b829d088874e0edfd5 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>
> > > @@ -176,6 +177,66 @@ static int read_objdump_output(FILE *f, void *buf, size_t *len, u64 start_addr)
> > >         return err;
> > >  }
> > >
> > > +/*
> > > + * Only gets GNU objdump version. Returns 0 for llvm-objdump.
> > > + */
> > > +static int objdump_version(void)
> > > +{
> > > +       size_t line_len;
> > > +       char cmd[PATH_MAX * 2];
> > > +       char *line = NULL;
> > > +       const char *fmt;
> > > +       FILE *f;
> > > +       int ret;
> > > +
> > > +       int version_tmp, version_num = 0;
> > > +       char *version = 0, *token;
> > > +
> > > +       fmt = "%s --version";
> > > +       ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path);
> > > +       if (ret <= 0 || (size_t)ret >= sizeof(cmd))
> > > +               return -1;
> > > +       /* Ignore objdump errors */
> > > +       strcat(cmd, " 2>/dev/null");
> > > +       f = popen(cmd, "r");
> > > +       if (!f) {
> > > +               pr_debug("popen failed\n");
> > > +               return -1;
> > > +       }
> > > +       /* Get first line of objdump --version output */
> > > +       ret = getline(&line, &line_len, f);
> > > +       pclose(f);
> > > +       if (ret < 0) {
> > > +               pr_debug("getline failed\n");
> > > +               return -1;
> > > +       }
> > > +
> > > +       token = strsep(&line, " ");
> > > +       if (token != NULL && !strcmp(token, "GNU")) {
> > > +               // version is last part of first line of objdump --version output.
> > > +               while ((token = strsep(&line, " ")))
> > > +                       version = token;
> > > +
> > > +               // Convert version into a format we can compare with
> > > +               token = strsep(&version, ".");
> > > +               version_num = atoi(token);
> > > +               if (version_num)
> > > +                       version_num *= 10000;
> > > +
> > > +               token = strsep(&version, ".");
> > > +               version_tmp = atoi(token);
> > > +               if (token)
> > > +                       version_num += version_tmp * 100;
> > > +
> > > +               token = strsep(&version, ".");
> > > +               version_tmp = atoi(token);
> > > +               if (token)
> > > +                       version_num += version_tmp;
> > > +       }
> > > +
> > > +       return version_num;
> > > +}
> > > +
> > >  static int read_via_objdump(const char *filename, u64 addr, void *buf,
> > >                             size_t len)
> > >  {
> > > @@ -183,9 +244,30 @@ 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)) {
> >
> > Not sure if there is a consistency issue here. Elsewhere we're just
> > using ifdef, such as:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
>
> I don't have any strong feelings about that. I can change it to be an
> ifdef. On other lists I have been told to use IS_ENABLED whenever
> possible, but it's only a small difference.

Agreed. Let's see what Arnaldo and Namhyung think.

Thanks,
Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Arnaldo Carvalho de Melo]

On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > On Tue, Dec 17, 2024 at 3:52 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>

> > Reviewed-by: Ian Rogers <irogers@google.com>

> > > 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]

> > > Changes in v2:
> > > - Do objdump version detection at runtime (Ian)
> > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com

> > > --- a/tools/perf/tests/code-reading.c
> > > @@ -183,9 +244,30 @@ 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)) {

> > Not sure if there is a consistency issue here. Elsewhere we're just
> > using ifdef, such as:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
 
> I don't have any strong feelings about that. I can change it to be an
> ifdef. On other lists I have been told to use IS_ENABLED whenever
> possible, but it's only a small difference.

Can't we just use uname here?

So that we don't use kconfig.h since its not used in tools/perf/ and
makes it looks like perf is in lockstep with the kernel source tree
version it was compiled from?

$ git grep kconfig.h tools/perf/
$

BTW, what would happen if I collected a perf.data file on x86_64 and
would read it in a RiscV machine with such a objdump version? The same
problem?

- Arnaldo

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > On Tue, Dec 17, 2024 at 3:52 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>
>
> > > Reviewed-by: Ian Rogers <irogers@google.com>
>
> > > > 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]
>
> > > > Changes in v2:
> > > > - Do objdump version detection at runtime (Ian)
> > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
>
> > > > --- a/tools/perf/tests/code-reading.c
> > > > @@ -183,9 +244,30 @@ 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)) {
>
> > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > using ifdef, such as:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
>
> > I don't have any strong feelings about that. I can change it to be an
> > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > possible, but it's only a small difference.
>
> Can't we just use uname here?
>
> So that we don't use kconfig.h since its not used in tools/perf/ and
> makes it looks like perf is in lockstep with the kernel source tree
> version it was compiled from?
>
> $ git grep kconfig.h tools/perf/
> $
>
> BTW, what would happen if I collected a perf.data file on x86_64 and
> would read it in a RiscV machine with such a objdump version? The same
> problem?

This code is in tests hence thinking that a separate fix is needed for
that problem. Hopefully the use of elf machine/flags tackles it:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
We are getting somewhat disassembler heavy. We have llvm as a library,
capstone as a library, binutils objdump and llvm objdump. Given the
pain with parsing text, could we lose the objdumps? Similarly for
addr2line?

Thanks,
Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > On Tue, Dec 17, 2024 at 3:52 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>
> >
> > > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > > > > 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]
> >
> > > > > Changes in v2:
> > > > > - Do objdump version detection at runtime (Ian)
> > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> >
> > > > > --- a/tools/perf/tests/code-reading.c
> > > > > @@ -183,9 +244,30 @@ 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)) {
> >
> > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > using ifdef, such as:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> >
> > > I don't have any strong feelings about that. I can change it to be an
> > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > possible, but it's only a small difference.
> >
> > Can't we just use uname here?
> >
> > So that we don't use kconfig.h since its not used in tools/perf/ and
> > makes it looks like perf is in lockstep with the kernel source tree
> > version it was compiled from?
> >
> > $ git grep kconfig.h tools/perf/
> > $
> >
> > BTW, what would happen if I collected a perf.data file on x86_64 and
> > would read it in a RiscV machine with such a objdump version? The same
> > problem?
> 
> This code is in tests hence thinking that a separate fix is needed for
> that problem. Hopefully the use of elf machine/flags tackles it:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> We are getting somewhat disassembler heavy. We have llvm as a library,
> capstone as a library, binutils objdump and llvm objdump. Given the
> pain with parsing text, could we lose the objdumps? Similarly for
> addr2line?

Are you suggesting to remove this test case entirely to get rid of the
objdump dependency? The goal of this test case seems to be to check
objdump and perf return the same data, so it doesn't seem like there
would be an alternative to using objdump.

- Charlie

> 
> Thanks,
> Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > On Tue, Dec 17, 2024 at 3:52 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>
> > >
> > > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > >
> > > > > > 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]
> > >
> > > > > > Changes in v2:
> > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > >
> > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > @@ -183,9 +244,30 @@ 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)) {
> > >
> > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > using ifdef, such as:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > >
> > > > I don't have any strong feelings about that. I can change it to be an
> > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > possible, but it's only a small difference.
> > >
> > > Can't we just use uname here?
> > >
> > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > makes it looks like perf is in lockstep with the kernel source tree
> > > version it was compiled from?
> > >
> > > $ git grep kconfig.h tools/perf/
> > > $
> > >
> > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > would read it in a RiscV machine with such a objdump version? The same
> > > problem?
> >
> > This code is in tests hence thinking that a separate fix is needed for
> > that problem. Hopefully the use of elf machine/flags tackles it:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > We are getting somewhat disassembler heavy. We have llvm as a library,
> > capstone as a library, binutils objdump and llvm objdump. Given the
> > pain with parsing text, could we lose the objdumps? Similarly for
> > addr2line?
>
> Are you suggesting to remove this test case entirely to get rid of the
> objdump dependency? The goal of this test case seems to be to check
> objdump and perf return the same data, so it doesn't seem like there
> would be an alternative to using objdump.

I can imagine having an objdump dependency for a test but not for some
more core like `perf annotate`. We have to do weird things when
parsing text, like this code I'm not proud of:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
The issue with that code is that LLVM objdump has changed its output
in newer versions to be closer to binutils objdump. Did that break
perf? Maybe it just broke what our variables think is an LLVM objdump,
but things aren't really broken. This kind of issue doesn't occur with
a library, although the differing needs of library versions is a real
thing.

Thanks,
Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > > <acme@kernel.org> wrote:
> > > >
> > > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > > On Tue, Dec 17, 2024 at 3:52 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>
> > > >
> > > > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > >
> > > > > > > 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]
> > > >
> > > > > > > Changes in v2:
> > > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > > >
> > > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > > @@ -183,9 +244,30 @@ 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)) {
> > > >
> > > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > > using ifdef, such as:
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > > >
> > > > > I don't have any strong feelings about that. I can change it to be an
> > > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > > possible, but it's only a small difference.
> > > >
> > > > Can't we just use uname here?
> > > >
> > > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > > makes it looks like perf is in lockstep with the kernel source tree
> > > > version it was compiled from?
> > > >
> > > > $ git grep kconfig.h tools/perf/
> > > > $
> > > >
> > > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > > would read it in a RiscV machine with such a objdump version? The same
> > > > problem?
> > >
> > > This code is in tests hence thinking that a separate fix is needed for
> > > that problem. Hopefully the use of elf machine/flags tackles it:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > > We are getting somewhat disassembler heavy. We have llvm as a library,
> > > capstone as a library, binutils objdump and llvm objdump. Given the
> > > pain with parsing text, could we lose the objdumps? Similarly for
> > > addr2line?
> >
> > Are you suggesting to remove this test case entirely to get rid of the
> > objdump dependency? The goal of this test case seems to be to check
> > objdump and perf return the same data, so it doesn't seem like there
> > would be an alternative to using objdump.
> 
> I can imagine having an objdump dependency for a test but not for some
> more core like `perf annotate`. We have to do weird things when
> parsing text, like this code I'm not proud of:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
> The issue with that code is that LLVM objdump has changed its output
> in newer versions to be closer to binutils objdump. Did that break
> perf? Maybe it just broke what our variables think is an LLVM objdump,
> but things aren't really broken. This kind of issue doesn't occur with
> a library, although the differing needs of library versions is a real
> thing.

Yeah doing the parsing of the text output is not ideal... For this test
case it should be possible to dynamically link against libbfd. I would
guess something similar could be done with llvm-objdump but I am less
familiar with that. I don't know if that's a good path to go down
though.

- Charlie

> 
> Thanks,
> Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Ian Rogers]

On Wed, Dec 18, 2024 at 2:32 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > > > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > > > <acme@kernel.org> wrote:
> > > > >
> > > > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > > > On Tue, Dec 17, 2024 at 3:52 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>
> > > > >
> > > > > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > > >
> > > > > > > > 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]
> > > > >
> > > > > > > > Changes in v2:
> > > > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > > > >
> > > > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > > > @@ -183,9 +244,30 @@ 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)) {
> > > > >
> > > > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > > > using ifdef, such as:
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > > > >
> > > > > > I don't have any strong feelings about that. I can change it to be an
> > > > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > > > possible, but it's only a small difference.
> > > > >
> > > > > Can't we just use uname here?
> > > > >
> > > > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > > > makes it looks like perf is in lockstep with the kernel source tree
> > > > > version it was compiled from?
> > > > >
> > > > > $ git grep kconfig.h tools/perf/
> > > > > $
> > > > >
> > > > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > > > would read it in a RiscV machine with such a objdump version? The same
> > > > > problem?
> > > >
> > > > This code is in tests hence thinking that a separate fix is needed for
> > > > that problem. Hopefully the use of elf machine/flags tackles it:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > > > We are getting somewhat disassembler heavy. We have llvm as a library,
> > > > capstone as a library, binutils objdump and llvm objdump. Given the
> > > > pain with parsing text, could we lose the objdumps? Similarly for
> > > > addr2line?
> > >
> > > Are you suggesting to remove this test case entirely to get rid of the
> > > objdump dependency? The goal of this test case seems to be to check
> > > objdump and perf return the same data, so it doesn't seem like there
> > > would be an alternative to using objdump.
> >
> > I can imagine having an objdump dependency for a test but not for some
> > more core like `perf annotate`. We have to do weird things when
> > parsing text, like this code I'm not proud of:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
> > The issue with that code is that LLVM objdump has changed its output
> > in newer versions to be closer to binutils objdump. Did that break
> > perf? Maybe it just broke what our variables think is an LLVM objdump,
> > but things aren't really broken. This kind of issue doesn't occur with
> > a library, although the differing needs of library versions is a real
> > thing.
>
> Yeah doing the parsing of the text output is not ideal... For this test
> case it should be possible to dynamically link against libbfd.

I need to write the patch set to delete libbfd from perf. IANAL but
the issue is that libbfd is part of binutils and GPLv3, while perf is
part of the Linux kernel and largely GPLv2. GPLv3 is incompatible with
GPLv2:
https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility
While using dlopen means we're not linking against libbfd, we may
effectively be using it as a plugin which again GPLv3 (in my IANAL
opinion) wouldn't allow:
https://www.gnu.org/licenses/gpl-faq.en.html#GPLPlugins
Currently to get libbfd support in perf you need to be building the
binary yourself and add to the build BUILD_NONDISTRO=1. We do this as
part of our build testing but having all the #ifdef-ed libbfd code if
nothing else makes the code harder to understand.

> I would
> guess something similar could be done with llvm-objdump but I am less
> familiar with that. I don't know if that's a good path to go down
> though.

In the past I perceived there was hostility toward LLVM from the Linux
kernel community. I guess GPL was considered the special sauce as to
why Linux won and the BSDs hadn't, so the preference was to favor a
compiler that used the same license. I don't think that's true any
more and I think there's a lot of sense in using LLVM's libraries
rather than reinventing them in the perf tool, or using perhaps less
orthodox sources like libcapstone. I'm not a fan of the text output
processing stuff so getting rid of objdump and llvm-objdump support
would be good imo.

Another area where I think we could lose a lot of code baggage is with
libunwind, as BPF support requires libelf which brings with it
libdwarves which when present means we don't use libunwind. I've heard
reports that libdwarves is slower, but I'm sure we can add caches to
speed it up which would likely benefit a range of people. I was kind
of hoping with all that deleted we may be able to get rid of the
majority of the arch directory, but the syscalltbl work is adding to
that directory :-)

Thanks,
Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 2:32 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote:
> > > On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > >
> > > > On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > > > > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > > > > <acme@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > > > > On Tue, Dec 17, 2024 at 3:52 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>
> > > > > >
> > > > > > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > > > >
> > > > > > > > > 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]
> > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > > > > >
> > > > > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > > > > @@ -183,9 +244,30 @@ 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)) {
> > > > > >
> > > > > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > > > > using ifdef, such as:
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > > > > >
> > > > > > > I don't have any strong feelings about that. I can change it to be an
> > > > > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > > > > possible, but it's only a small difference.
> > > > > >
> > > > > > Can't we just use uname here?
> > > > > >
> > > > > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > > > > makes it looks like perf is in lockstep with the kernel source tree
> > > > > > version it was compiled from?
> > > > > >
> > > > > > $ git grep kconfig.h tools/perf/
> > > > > > $
> > > > > >
> > > > > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > > > > would read it in a RiscV machine with such a objdump version? The same
> > > > > > problem?
> > > > >
> > > > > This code is in tests hence thinking that a separate fix is needed for
> > > > > that problem. Hopefully the use of elf machine/flags tackles it:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > > > > We are getting somewhat disassembler heavy. We have llvm as a library,
> > > > > capstone as a library, binutils objdump and llvm objdump. Given the
> > > > > pain with parsing text, could we lose the objdumps? Similarly for
> > > > > addr2line?
> > > >
> > > > Are you suggesting to remove this test case entirely to get rid of the
> > > > objdump dependency? The goal of this test case seems to be to check
> > > > objdump and perf return the same data, so it doesn't seem like there
> > > > would be an alternative to using objdump.
> > >
> > > I can imagine having an objdump dependency for a test but not for some
> > > more core like `perf annotate`. We have to do weird things when
> > > parsing text, like this code I'm not proud of:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
> > > The issue with that code is that LLVM objdump has changed its output
> > > in newer versions to be closer to binutils objdump. Did that break
> > > perf? Maybe it just broke what our variables think is an LLVM objdump,
> > > but things aren't really broken. This kind of issue doesn't occur with
> > > a library, although the differing needs of library versions is a real
> > > thing.
> >
> > Yeah doing the parsing of the text output is not ideal... For this test
> > case it should be possible to dynamically link against libbfd.
> 
> I need to write the patch set to delete libbfd from perf. IANAL but
> the issue is that libbfd is part of binutils and GPLv3, while perf is
> part of the Linux kernel and largely GPLv2. GPLv3 is incompatible with
> GPLv2:
> https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility
> While using dlopen means we're not linking against libbfd, we may
> effectively be using it as a plugin which again GPLv3 (in my IANAL
> opinion) wouldn't allow:
> https://www.gnu.org/licenses/gpl-faq.en.html#GPLPlugins
> Currently to get libbfd support in perf you need to be building the
> binary yourself and add to the build BUILD_NONDISTRO=1. We do this as
> part of our build testing but having all the #ifdef-ed libbfd code if
> nothing else makes the code harder to understand.

Licensing is fun ;)

> 
> > I would
> > guess something similar could be done with llvm-objdump but I am less
> > familiar with that. I don't know if that's a good path to go down
> > though.
> 
> In the past I perceived there was hostility toward LLVM from the Linux
> kernel community. I guess GPL was considered the special sauce as to
> why Linux won and the BSDs hadn't, so the preference was to favor a
> compiler that used the same license. I don't think that's true any
> more and I think there's a lot of sense in using LLVM's libraries
> rather than reinventing them in the perf tool, or using perhaps less
> orthodox sources like libcapstone. I'm not a fan of the text output
> processing stuff so getting rid of objdump and llvm-objdump support
> would be good imo.

Yeah I agree. This test case did end up being interesting though as it
unconvered this change in behavior of objdump on riscv, but that's
tangential to the purpose of this test case. We need this patch on riscv
to stop this test from failing, but it is also reasonable to approach
this differently and not use objdump at all.

> 
> Another area where I think we could lose a lot of code baggage is with
> libunwind, as BPF support requires libelf which brings with it
> libdwarves which when present means we don't use libunwind. I've heard
> reports that libdwarves is slower, but I'm sure we can add caches to
> speed it up which would likely benefit a range of people. I was kind
> of hoping with all that deleted we may be able to get rid of the
> majority of the arch directory, but the syscalltbl work is adding to
> that directory :-)

Yeah... The syscalltbl work will add some extra parts to the arch
directory. A lot of the additions are generic and a chunk of
arch-specific ifdefs were able to be removed, but we still have the
"problem" that not every architecture uses the shared syscall table and
supports different syscalls.

- Charlie

> 
> Thanks,
> Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Wed, Dec 18, 2024 at 05:52:24PM -0800, Charlie Jenkins wrote:
> On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote:
> > On Wed, Dec 18, 2024 at 2:32 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > >
> > > On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote:
> > > > On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
> > > > >
> > > > > On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote:
> > > > > > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo
> > > > > > <acme@kernel.org> wrote:
> > > > > > >
> > > > > > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > > > > > > > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > > > > > > > On Tue, Dec 17, 2024 at 3:52 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>
> > > > > > >
> > > > > > > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > > > > >
> > > > > > > > > > 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]
> > > > > > >
> > > > > > > > > > Changes in v2:
> > > > > > > > > > - Do objdump version detection at runtime (Ian)
> > > > > > > > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> > > > > > >
> > > > > > > > > > --- a/tools/perf/tests/code-reading.c
> > > > > > > > > > @@ -183,9 +244,30 @@ 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)) {
> > > > > > >
> > > > > > > > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > > > > > > > using ifdef, such as:
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
> > > > > > >
> > > > > > > > I don't have any strong feelings about that. I can change it to be an
> > > > > > > > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > > > > > > > possible, but it's only a small difference.
> > > > > > >
> > > > > > > Can't we just use uname here?
> > > > > > >
> > > > > > > So that we don't use kconfig.h since its not used in tools/perf/ and
> > > > > > > makes it looks like perf is in lockstep with the kernel source tree
> > > > > > > version it was compiled from?
> > > > > > >
> > > > > > > $ git grep kconfig.h tools/perf/
> > > > > > > $
> > > > > > >
> > > > > > > BTW, what would happen if I collected a perf.data file on x86_64 and
> > > > > > > would read it in a RiscV machine with such a objdump version? The same
> > > > > > > problem?
> > > > > >
> > > > > > This code is in tests hence thinking that a separate fix is needed for
> > > > > > that problem. Hopefully the use of elf machine/flags tackles it:
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n25
> > > > > > We are getting somewhat disassembler heavy. We have llvm as a library,
> > > > > > capstone as a library, binutils objdump and llvm objdump. Given the
> > > > > > pain with parsing text, could we lose the objdumps? Similarly for
> > > > > > addr2line?
> > > > >
> > > > > Are you suggesting to remove this test case entirely to get rid of the
> > > > > objdump dependency? The goal of this test case seems to be to check
> > > > > objdump and perf return the same data, so it doesn't seem like there
> > > > > would be an alternative to using objdump.
> > > >
> > > > I can imagine having an objdump dependency for a test but not for some
> > > > more core like `perf annotate`. We have to do weird things when
> > > > parsing text, like this code I'm not proud of:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523
> > > > The issue with that code is that LLVM objdump has changed its output
> > > > in newer versions to be closer to binutils objdump. Did that break
> > > > perf? Maybe it just broke what our variables think is an LLVM objdump,
> > > > but things aren't really broken. This kind of issue doesn't occur with
> > > > a library, although the differing needs of library versions is a real
> > > > thing.
> > >
> > > Yeah doing the parsing of the text output is not ideal... For this test
> > > case it should be possible to dynamically link against libbfd.
> > 
> > I need to write the patch set to delete libbfd from perf. IANAL but
> > the issue is that libbfd is part of binutils and GPLv3, while perf is
> > part of the Linux kernel and largely GPLv2. GPLv3 is incompatible with
> > GPLv2:
> > https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility
> > While using dlopen means we're not linking against libbfd, we may
> > effectively be using it as a plugin which again GPLv3 (in my IANAL
> > opinion) wouldn't allow:
> > https://www.gnu.org/licenses/gpl-faq.en.html#GPLPlugins
> > Currently to get libbfd support in perf you need to be building the
> > binary yourself and add to the build BUILD_NONDISTRO=1. We do this as
> > part of our build testing but having all the #ifdef-ed libbfd code if
> > nothing else makes the code harder to understand.
> 
> Licensing is fun ;)
> 
> > 
> > > I would
> > > guess something similar could be done with llvm-objdump but I am less
> > > familiar with that. I don't know if that's a good path to go down
> > > though.
> > 
> > In the past I perceived there was hostility toward LLVM from the Linux
> > kernel community. I guess GPL was considered the special sauce as to
> > why Linux won and the BSDs hadn't, so the preference was to favor a
> > compiler that used the same license. I don't think that's true any
> > more and I think there's a lot of sense in using LLVM's libraries
> > rather than reinventing them in the perf tool, or using perhaps less
> > orthodox sources like libcapstone. I'm not a fan of the text output
> > processing stuff so getting rid of objdump and llvm-objdump support
> > would be good imo.
> 
> Yeah I agree. This test case did end up being interesting though as it
> unconvered this change in behavior of objdump on riscv, but that's
> tangential to the purpose of this test case. We need this patch on riscv
> to stop this test from failing, but it is also reasonable to approach
> this differently and not use objdump at all.

What's the next step here? Would you prefer to get rid of this test
entirely? I sent out a v3 that uses uname [1].

Link
https://lore.kernel.org/lkml/20241219-perf_fix_riscv_obj_reading-v3-1-a7d644dcfa50@rivosinc.com/
[1]

- Charlie

> 
> > 
> > Another area where I think we could lose a lot of code baggage is with
> > libunwind, as BPF support requires libelf which brings with it
> > libdwarves which when present means we don't use libunwind. I've heard
> > reports that libdwarves is slower, but I'm sure we can add caches to
> > speed it up which would likely benefit a range of people. I was kind
> > of hoping with all that deleted we may be able to get rid of the
> > majority of the arch directory, but the syscalltbl work is adding to
> > that directory :-)
> 
> Yeah... The syscalltbl work will add some extra parts to the arch
> directory. A lot of the additions are generic and a chunk of
> arch-specific ifdefs were able to be removed, but we still have the
> "problem" that not every architecture uses the shared syscall table and
> supports different syscalls.
> 
> - Charlie
> 
> > 
> > Thanks,
> > Ian

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Arnaldo Carvalho de Melo]

On Thu, Jan 02, 2025 at 05:44:56PM -0800, Charlie Jenkins wrote:
> On Wed, Dec 18, 2024 at 05:52:24PM -0800, Charlie Jenkins wrote:
> > On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote:
> > Yeah I agree. This test case did end up being interesting though as it
> > unconvered this change in behavior of objdump on riscv, but that's
> > tangential to the purpose of this test case. We need this patch on riscv
> > to stop this test from failing, but it is also reasonable to approach
> > this differently and not use objdump at all.
 
> What's the next step here? Would you prefer to get rid of this test
> entirely? I sent out a v3 that uses uname [1].
 
> Link
> https://lore.kernel.org/lkml/20241219-perf_fix_riscv_obj_reading-v3-1-a7d644dcfa50@rivosinc.com/
> [1]

I'm ok with this latest version and added it to perf-tools-next,
changing the patch subject line to:

  perf tests code-reading: Handle change in objdump output from binutils >= 2.41 on riscv

Ok?

- Arnaldo

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Fri, Jan 03, 2025 at 01:51:59PM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Jan 02, 2025 at 05:44:56PM -0800, Charlie Jenkins wrote:
> > On Wed, Dec 18, 2024 at 05:52:24PM -0800, Charlie Jenkins wrote:
> > > On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote:
> > > Yeah I agree. This test case did end up being interesting though as it
> > > unconvered this change in behavior of objdump on riscv, but that's
> > > tangential to the purpose of this test case. We need this patch on riscv
> > > to stop this test from failing, but it is also reasonable to approach
> > > this differently and not use objdump at all.
>  
> > What's the next step here? Would you prefer to get rid of this test
> > entirely? I sent out a v3 that uses uname [1].
>  
> > Link
> > https://lore.kernel.org/lkml/20241219-perf_fix_riscv_obj_reading-v3-1-a7d644dcfa50@rivosinc.com/
> > [1]
> 
> I'm ok with this latest version and added it to perf-tools-next,
> changing the patch subject line to:
> 
>   perf tests code-reading: Handle change in objdump output from binutils >= 2.41 on riscv
> 
> Ok?

Yes that is perfect, thank you!

- Charlie

> 
> - Arnaldo

Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv

[Charlie Jenkins]

On Wed, Dec 18, 2024 at 03:41:32PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote:
> > On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote:
> > > On Tue, Dec 17, 2024 at 3:52 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>
> 
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> 
> > > > 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]
> 
> > > > Changes in v2:
> > > > - Do objdump version detection at runtime (Ian)
> > > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com
> 
> > > > --- a/tools/perf/tests/code-reading.c
> > > > @@ -183,9 +244,30 @@ 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)) {
> 
> > > Not sure if there is a consistency issue here. Elsewhere we're just
> > > using ifdef, such as:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69
>  
> > I don't have any strong feelings about that. I can change it to be an
> > ifdef. On other lists I have been told to use IS_ENABLED whenever
> > possible, but it's only a small difference.
> 
> Can't we just use uname here?
> 
> So that we don't use kconfig.h since its not used in tools/perf/ and
> makes it looks like perf is in lockstep with the kernel source tree
> version it was compiled from?
> 
> $ git grep kconfig.h tools/perf/
> $
> 
> BTW, what would happen if I collected a perf.data file on x86_64 and
> would read it in a RiscV machine with such a objdump version? The same
> problem?

Yes that's a good point, we should be detecting riscv at runtime too.

- Charlie

> 
> - Arnaldo