* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.