| Message ID | 20241217-perf_fix_riscv_obj_reading-v2-1-58f81b7b4c7d@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tools: perf: tests: Fix code reading for riscv | expand |
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 >
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 > >
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
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;
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(-) --- base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85