* [PATCH] perf annotate: Fix register usage in data type profiling @ 2026-02-06 1:27 Namhyung Kim 2026-02-06 5:00 ` Ian Rogers 0 siblings, 1 reply; 6+ messages in thread From: Namhyung Kim @ 2026-02-06 1:27 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, James Clark Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li, Dmitry Dolgov On data type profiling, it tried to match register name with a partial string. For example, it allowed to match with "%rbp)" or "%rdi,8)". But with recent change in the area, it doesn't match anymore and break the data type profiling. Let's pass the correct register name by removing the unwanted part. Add arch__dwarf_regnum() to handle it in a single place. Reported-by: Dmitry Dolgov <9erthalion6@gmail.com> Closes: 7d3n23li6drroxrdlpxn7ixehdeszkjdftah3zyngjl2qs22ef@yelcjv53v42o Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/util/annotate.c | 61 +++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 880b1bd300c21e67..2e3522905046c1ec 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -2447,6 +2447,29 @@ int annotate_check_args(void) return 0; } +static int arch__dwarf_regnum(const struct arch *arch, const char *str) +{ + const char *p; + char *regname, *q; + int reg; + + p = strchr(str, arch->objdump.register_char); + if (p == NULL) + return -1; + + regname = strdup(p); + if (regname == NULL) + return -1; + + q = strpbrk(regname, ",) "); + if (q) + *q = '\0'; + + reg = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); + free(regname); + return reg; +} + /* * Get register number and access offset from the given instruction. * It assumes AT&T x86 asm format like OFFSET(REG). Maybe it needs @@ -2457,7 +2480,6 @@ static int extract_reg_offset(const struct arch *arch, const char *str, struct annotated_op_loc *op_loc) { char *p; - char *regname; if (arch->objdump.register_char == 0) return -1; @@ -2482,31 +2504,14 @@ static int extract_reg_offset(const struct arch *arch, const char *str, } op_loc->offset = strtol(str, &p, 0); - - p = strchr(p, arch->objdump.register_char); - if (p == NULL) + op_loc->reg1 = arch__dwarf_regnum(arch, p); + if (op_loc->reg1 == -1) return -1; - regname = strdup(p); - if (regname == NULL) - return -1; - - op_loc->reg1 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); - free(regname); - /* Get the second register */ - if (op_loc->multi_regs) { - p = strchr(p + 1, arch->objdump.register_char); - if (p == NULL) - return -1; - - regname = strdup(p); - if (regname == NULL) - return -1; + if (op_loc->multi_regs) + op_loc->reg2 = arch__dwarf_regnum(arch, p + 1); - op_loc->reg2 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); - free(regname); - } return 0; } @@ -2585,7 +2590,8 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, op_loc->multi_regs = multi_regs; extract_reg_offset(arch, insn_str, op_loc); } else { - char *s, *p = NULL; + const char *s = insn_str; + char *p = NULL; if (arch__is_x86(arch)) { /* FIXME: Handle other segment registers */ @@ -2599,21 +2605,14 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, } } - s = strdup(insn_str); - if (s == NULL) - return -1; - if (*s == arch->objdump.register_char) { - op_loc->reg1 = get_dwarf_regnum(s, - arch->id.e_machine, - arch->id.e_flags); + op_loc->reg1 = arch__dwarf_regnum(arch, s); } else if (*s == arch->objdump.imm_char) { op_loc->offset = strtol(s + 1, &p, 0); if (p && p != s + 1) op_loc->imm = true; } - free(s); } } -- 2.53.0.rc2.204.g2597b5adb4-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf annotate: Fix register usage in data type profiling 2026-02-06 1:27 [PATCH] perf annotate: Fix register usage in data type profiling Namhyung Kim @ 2026-02-06 5:00 ` Ian Rogers 2026-02-06 9:08 ` Dmitry Dolgov 2026-02-06 21:18 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 6+ messages in thread From: Ian Rogers @ 2026-02-06 5:00 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li, Dmitry Dolgov On Thu, Feb 5, 2026 at 5:27 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On data type profiling, it tried to match register name with a partial > string. For example, it allowed to match with "%rbp)" or "%rdi,8)". > But with recent change in the area, it doesn't match anymore and break > the data type profiling. Hmm.. the blamed patch was a strcmp before and after, so maybe something nearby changed causing this: https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=c31040085914f1188720073baa43d1483693c0a3 ``` - for (i = 0; i < ARRAY_SIZE(x86_regidx_table); i++) - if (!strcmp(x86_regidx_table[i].name, name + 1)) - return x86_regidx_table[i].idx; + name++; + for (size_t i = 0; i < num_entries; i++) { + if (!strcmp(entries[i].name, name)) + return entries[i].dwarf_regnum; ``` > Let's pass the correct register name by removing the unwanted part. Add > arch__dwarf_regnum() to handle it in a single place. > > Reported-by: Dmitry Dolgov <9erthalion6@gmail.com> > Closes: 7d3n23li6drroxrdlpxn7ixehdeszkjdftah3zyngjl2qs22ef@yelcjv53v42o > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/util/annotate.c | 61 +++++++++++++++++++------------------- > 1 file changed, 30 insertions(+), 31 deletions(-) > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 880b1bd300c21e67..2e3522905046c1ec 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -2447,6 +2447,29 @@ int annotate_check_args(void) > return 0; > } > > +static int arch__dwarf_regnum(const struct arch *arch, const char *str) > +{ > + const char *p; > + char *regname, *q; > + int reg; > + > + p = strchr(str, arch->objdump.register_char); > + if (p == NULL) > + return -1; > + > + regname = strdup(p); nit: Given register names are known to be so short it feels a shame to do a full strdup for this. > + if (regname == NULL) > + return -1; > + > + q = strpbrk(regname, ",) "); > + if (q) > + *q = '\0'; > + > + reg = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); nit: Perhaps pass a regname_len to avoid the strdup. Reviewed-by: Ian Rogers <irogers@google.com> It'd be nice to have a test on this to avoid a regression. Dmitrii's patch: https://lore.kernel.org/linux-perf-users/20260127083030.5909-1-9erthalion6@gmail.com/ does that for a Rust struct Buf, but it'd be nice to do something similar for C. Thanks, Ian > + free(regname); > + return reg; > +} > + > /* > * Get register number and access offset from the given instruction. > * It assumes AT&T x86 asm format like OFFSET(REG). Maybe it needs > @@ -2457,7 +2480,6 @@ static int extract_reg_offset(const struct arch *arch, const char *str, > struct annotated_op_loc *op_loc) > { > char *p; > - char *regname; > > if (arch->objdump.register_char == 0) > return -1; > @@ -2482,31 +2504,14 @@ static int extract_reg_offset(const struct arch *arch, const char *str, > } > > op_loc->offset = strtol(str, &p, 0); > - > - p = strchr(p, arch->objdump.register_char); > - if (p == NULL) > + op_loc->reg1 = arch__dwarf_regnum(arch, p); > + if (op_loc->reg1 == -1) > return -1; > > - regname = strdup(p); > - if (regname == NULL) > - return -1; > - > - op_loc->reg1 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); > - free(regname); > - > /* Get the second register */ > - if (op_loc->multi_regs) { > - p = strchr(p + 1, arch->objdump.register_char); > - if (p == NULL) > - return -1; > - > - regname = strdup(p); > - if (regname == NULL) > - return -1; > + if (op_loc->multi_regs) > + op_loc->reg2 = arch__dwarf_regnum(arch, p + 1); > > - op_loc->reg2 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); > - free(regname); > - } > return 0; > } > > @@ -2585,7 +2590,8 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, > op_loc->multi_regs = multi_regs; > extract_reg_offset(arch, insn_str, op_loc); > } else { > - char *s, *p = NULL; > + const char *s = insn_str; > + char *p = NULL; > > if (arch__is_x86(arch)) { > /* FIXME: Handle other segment registers */ > @@ -2599,21 +2605,14 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, > } > } > > - s = strdup(insn_str); > - if (s == NULL) > - return -1; > - > if (*s == arch->objdump.register_char) { > - op_loc->reg1 = get_dwarf_regnum(s, > - arch->id.e_machine, > - arch->id.e_flags); > + op_loc->reg1 = arch__dwarf_regnum(arch, s); > } > else if (*s == arch->objdump.imm_char) { > op_loc->offset = strtol(s + 1, &p, 0); > if (p && p != s + 1) > op_loc->imm = true; > } > - free(s); > } > } > > -- > 2.53.0.rc2.204.g2597b5adb4-goog > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf annotate: Fix register usage in data type profiling 2026-02-06 5:00 ` Ian Rogers @ 2026-02-06 9:08 ` Dmitry Dolgov 2026-02-06 21:19 ` Arnaldo Carvalho de Melo 2026-02-06 21:18 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 6+ messages in thread From: Dmitry Dolgov @ 2026-02-06 9:08 UTC (permalink / raw) To: Ian Rogers Cc: Namhyung Kim, Arnaldo Carvalho de Melo, James Clark, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li > On Thu, Feb 05, 2026 at 09:00:43PM -0800, Ian Rogers wrote: > It'd be nice to have a test on this to avoid a regression. Dmitrii's patch: > https://lore.kernel.org/linux-perf-users/20260127083030.5909-1-9erthalion6@gmail.com/ > does that for a Rust struct Buf, but it'd be nice to do something similar for C. Agree, and I was planning to extend the tests in the patch above with a C workload as well. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf annotate: Fix register usage in data type profiling 2026-02-06 9:08 ` Dmitry Dolgov @ 2026-02-06 21:19 ` Arnaldo Carvalho de Melo 2026-02-08 12:24 ` Dmitry Dolgov 0 siblings, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2026-02-06 21:19 UTC (permalink / raw) To: Dmitry Dolgov Cc: Ian Rogers, Namhyung Kim, James Clark, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li On Fri, Feb 06, 2026 at 10:08:55AM +0100, Dmitry Dolgov wrote: > > On Thu, Feb 05, 2026 at 09:00:43PM -0800, Ian Rogers wrote: > > It'd be nice to have a test on this to avoid a regression. Dmitrii's patch: > > https://lore.kernel.org/linux-perf-users/20260127083030.5909-1-9erthalion6@gmail.com/ > > does that for a Rust struct Buf, but it'd be nice to do something similar for C. > Agree, and I was planning to extend the tests in the patch above with a > C workload as well. Hey, did you test the patch? Having a Tested-by you in addition to the Reported-by would be nice. - Arnaldo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf annotate: Fix register usage in data type profiling 2026-02-06 21:19 ` Arnaldo Carvalho de Melo @ 2026-02-08 12:24 ` Dmitry Dolgov 0 siblings, 0 replies; 6+ messages in thread From: Dmitry Dolgov @ 2026-02-08 12:24 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ian Rogers, Namhyung Kim, James Clark, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li > On Fri, Feb 06, 2026 at 06:19:41PM -0300, Arnaldo Carvalho de Melo wrote: > On Fri, Feb 06, 2026 at 10:08:55AM +0100, Dmitry Dolgov wrote: > > > On Thu, Feb 05, 2026 at 09:00:43PM -0800, Ian Rogers wrote: > > > It'd be nice to have a test on this to avoid a regression. Dmitrii's patch: > > > https://lore.kernel.org/linux-perf-users/20260127083030.5909-1-9erthalion6@gmail.com/ > > > does that for a Rust struct Buf, but it'd be nice to do something similar for C. > > > Agree, and I was planning to extend the tests in the patch above with a > > C workload as well. > > Hey, did you test the patch? Having a Tested-by you in addition to the > Reported-by would be nice. Yep, I did some testing, all looked fine. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf annotate: Fix register usage in data type profiling 2026-02-06 5:00 ` Ian Rogers 2026-02-06 9:08 ` Dmitry Dolgov @ 2026-02-06 21:18 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2026-02-06 21:18 UTC (permalink / raw) To: Ian Rogers Cc: Namhyung Kim, James Clark, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Zecheng Li, Dmitry Dolgov On Thu, Feb 05, 2026 at 09:00:43PM -0800, Ian Rogers wrote: > On Thu, Feb 5, 2026 at 5:27 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On data type profiling, it tried to match register name with a partial > > string. For example, it allowed to match with "%rbp)" or "%rdi,8)". > > But with recent change in the area, it doesn't match anymore and break > > the data type profiling. > > Hmm.. the blamed patch was a strcmp before and after, so maybe > something nearby changed causing this: > https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=c31040085914f1188720073baa43d1483693c0a3 > ``` > - for (i = 0; i < ARRAY_SIZE(x86_regidx_table); i++) > - if (!strcmp(x86_regidx_table[i].name, name + 1)) > - return x86_regidx_table[i].idx; > + name++; > + for (size_t i = 0; i < num_entries; i++) { > + if (!strcmp(entries[i].name, name)) > + return entries[i].dwarf_regnum; > ``` > > > Let's pass the correct register name by removing the unwanted part. Add > > arch__dwarf_regnum() to handle it in a single place. > > > > Reported-by: Dmitry Dolgov <9erthalion6@gmail.com> > > Closes: 7d3n23li6drroxrdlpxn7ixehdeszkjdftah3zyngjl2qs22ef@yelcjv53v42o > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/util/annotate.c | 61 +++++++++++++++++++------------------- > > 1 file changed, 30 insertions(+), 31 deletions(-) > > > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > > index 880b1bd300c21e67..2e3522905046c1ec 100644 > > --- a/tools/perf/util/annotate.c > > +++ b/tools/perf/util/annotate.c > > @@ -2447,6 +2447,29 @@ int annotate_check_args(void) > > return 0; > > } > > > > +static int arch__dwarf_regnum(const struct arch *arch, const char *str) > > +{ > > + const char *p; > > + char *regname, *q; > > + int reg; > > + > > + p = strchr(str, arch->objdump.register_char); > > + if (p == NULL) > > + return -1; > > + > > + regname = strdup(p); > > nit: Given register names are known to be so short it feels a shame to > do a full strdup for this. > > > + if (regname == NULL) > > + return -1; > > + > > + q = strpbrk(regname, ",) "); > > + if (q) > > + *q = '\0'; > > + > > + reg = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); > > nit: Perhaps pass a regname_len to avoid the strdup. > > Reviewed-by: Ian Rogers <irogers@google.com> Thanks, applied to perf-tools-next, these extra nits can be done later, - Arnaldo > It'd be nice to have a test on this to avoid a regression. Dmitrii's patch: > https://lore.kernel.org/linux-perf-users/20260127083030.5909-1-9erthalion6@gmail.com/ > does that for a Rust struct Buf, but it'd be nice to do something similar for C. > > Thanks, > Ian > > > + free(regname); > > + return reg; > > +} > > + > > /* > > * Get register number and access offset from the given instruction. > > * It assumes AT&T x86 asm format like OFFSET(REG). Maybe it needs > > @@ -2457,7 +2480,6 @@ static int extract_reg_offset(const struct arch *arch, const char *str, > > struct annotated_op_loc *op_loc) > > { > > char *p; > > - char *regname; > > > > if (arch->objdump.register_char == 0) > > return -1; > > @@ -2482,31 +2504,14 @@ static int extract_reg_offset(const struct arch *arch, const char *str, > > } > > > > op_loc->offset = strtol(str, &p, 0); > > - > > - p = strchr(p, arch->objdump.register_char); > > - if (p == NULL) > > + op_loc->reg1 = arch__dwarf_regnum(arch, p); > > + if (op_loc->reg1 == -1) > > return -1; > > > > - regname = strdup(p); > > - if (regname == NULL) > > - return -1; > > - > > - op_loc->reg1 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); > > - free(regname); > > - > > /* Get the second register */ > > - if (op_loc->multi_regs) { > > - p = strchr(p + 1, arch->objdump.register_char); > > - if (p == NULL) > > - return -1; > > - > > - regname = strdup(p); > > - if (regname == NULL) > > - return -1; > > + if (op_loc->multi_regs) > > + op_loc->reg2 = arch__dwarf_regnum(arch, p + 1); > > > > - op_loc->reg2 = get_dwarf_regnum(regname, arch->id.e_machine, arch->id.e_flags); > > - free(regname); > > - } > > return 0; > > } > > > > @@ -2585,7 +2590,8 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, > > op_loc->multi_regs = multi_regs; > > extract_reg_offset(arch, insn_str, op_loc); > > } else { > > - char *s, *p = NULL; > > + const char *s = insn_str; > > + char *p = NULL; > > > > if (arch__is_x86(arch)) { > > /* FIXME: Handle other segment registers */ > > @@ -2599,21 +2605,14 @@ int annotate_get_insn_location(const struct arch *arch, struct disasm_line *dl, > > } > > } > > > > - s = strdup(insn_str); > > - if (s == NULL) > > - return -1; > > - > > if (*s == arch->objdump.register_char) { > > - op_loc->reg1 = get_dwarf_regnum(s, > > - arch->id.e_machine, > > - arch->id.e_flags); > > + op_loc->reg1 = arch__dwarf_regnum(arch, s); > > } > > else if (*s == arch->objdump.imm_char) { > > op_loc->offset = strtol(s + 1, &p, 0); > > if (p && p != s + 1) > > op_loc->imm = true; > > } > > - free(s); > > } > > } > > > > -- > > 2.53.0.rc2.204.g2597b5adb4-goog > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-02-08 12:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-02-06 1:27 [PATCH] perf annotate: Fix register usage in data type profiling Namhyung Kim 2026-02-06 5:00 ` Ian Rogers 2026-02-06 9:08 ` Dmitry Dolgov 2026-02-06 21:19 ` Arnaldo Carvalho de Melo 2026-02-08 12:24 ` Dmitry Dolgov 2026-02-06 21:18 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox