| 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: <aYZalTZ1AWpCp_ae@x1>
Date: Fri, 6 Feb 2026 18:18:13 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Namhyung Kim <namhyung@...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 05, 2026 at 09:00:43PM -0800, Ian Rogers wrote:
> 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>
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
> >
Powered by blists - more mailing lists