tools/perf/util/annotate.c | 61 +++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 31 deletions(-)
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-googOn 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
>
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
> >
> 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.
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
© 2016 - 2026 Red Hat, Inc.