| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fXn2jaPPBHfFLvZy-O2OXZbjOFHatDGdVm4b5q3C8jmnA@mail.gmail.com>
Date: Thu, 5 Feb 2026 21:00:43 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, James Clark <james.clark@...aro.org>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org,
Zecheng Li <zli94@...u.edu>, Dmitry Dolgov <9erthalion6@...il.com>
Subject: Re: [PATCH] perf annotate: Fix register usage in data type profiling
On Thu, Feb 5, 2026 at 5:27 PM Namhyung Kim <namhyung@...nel.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@...il.com>
> Closes: 7d3n23li6drroxrdlpxn7ixehdeszkjdftah3zyngjl2qs22ef@...cjv53v42o
> Signed-off-by: Namhyung Kim <namhyung@...nel.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@...gle.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
>
Powered by blists - more mailing lists