[PATCH] perf annotate: Fix register usage in data type profiling

[PATCH] perf annotate: Fix register usage in data type profiling

[Namhyung Kim]

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

Re: [PATCH] perf annotate: Fix register usage in data type profiling

[Ian Rogers]

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
>

Re: [PATCH] perf annotate: Fix register usage in data type profiling

[Dmitry Dolgov]

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

Re: [PATCH] perf annotate: Fix register usage in data type profiling

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf annotate: Fix register usage in data type profiling

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf annotate: Fix register usage in data type profiling

[Dmitry Dolgov]

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