| 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=fVS90cUNXbs2BD-_cEBb9c=1+UN1BvGF1kvtDKy-=O0ZA@mail.gmail.com>
Date: Thu, 22 Jan 2026 11:20:04 -0800
From: Ian Rogers <irogers@...gle.com>
To: Suchit Karunakaran <suchitkarunakaran@...il.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, James Clark <james.clark@...aro.org>,
John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>, Leo Yan <leo.yan@...ux.dev>,
Guo Ren <guoren@...nel.org>, Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>,
Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
Zecheng Li <zecheng@...gle.com>, Tianyou Li <tianyou.li@...el.com>,
Thomas Falcon <thomas.falcon@...el.com>, Julia Lawall <Julia.Lawall@...ia.fr>,
Athira Rajeev <atrajeev@...ux.ibm.com>, Aditya Bodkhe <aditya.b1@...ux.ibm.com>,
Howard Chu <howardchu95@...il.com>,
Krzysztof Łopatowski <krzysztof.m.lopatowski@...il.com>,
"Dr. David Alan Gilbert" <linux@...blig.org>, Shimin Guo <shimin.guo@...dio.com>,
Sergei Trofimovich <slyich@...il.com>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-csky@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 11/12] perf disasm: Refactor arch__find and
initialization of arch structs
On Thu, Jan 22, 2026 at 10:45 AM Suchit Karunakaran
<suchitkarunakaran@...il.com> wrote:
> On Thu, 22 Jan 2026 at 22:24, Ian Rogers <irogers@...gle.com> wrote:
[snip]
> > --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> > +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> > @@ -8,9 +8,10 @@
> > #include "../annotate.h"
> > #include "../disasm.h"
> >
> > -struct arm64_annotate {
> > - regex_t call_insn,
> > - jump_insn;
> > +struct arch_arm64 {
> > + struct arch arch;
> > + regex_t call_insn;
> > + regex_t jump_insn;
> > };
> >
> > static int arm64_mov__parse(const struct arch *arch __maybe_unused,
> > @@ -70,7 +71,7 @@ static const struct ins_ops arm64_mov_ops = {
> >
> > static const struct ins_ops *arm64__associate_instruction_ops(struct arch *arch, const char *name)
> > {
> > - struct arm64_annotate *arm = arch->priv;
> > + struct arch_arm64 *arm = container_of(arch, struct arch_arm64, arch);
> > const struct ins_ops *ops;
> > regmatch_t match[2];
> >
> > @@ -87,38 +88,44 @@ static const struct ins_ops *arm64__associate_instruction_ops(struct arch *arch,
> > return ops;
> > }
> >
> > -int arm64__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
> > +const struct arch *arch__new_arm64(const struct e_machine_and_e_flags *id,
> > + const char *cpuid __maybe_unused)
> > {
> > - struct arm64_annotate *arm;
> > int err;
> > + struct arch_arm64 *arm = zalloc(sizeof(*arm));
> > + struct arch *arch;
> >
> > - if (arch->initialized)
> > - return 0;
> > + if (!arm)
> > + return NULL;
> > +
> > + arch = &arm->arch;
> > + arch->name = "arm64";
> > + arch->id = *id;
> > + arch->objdump.comment_char = '/';
> > + arch->objdump.skip_functions_char = '+';
> > + arch->associate_instruction_ops = arm64__associate_instruction_ops;
> >
> > arm = zalloc(sizeof(*arm));
> > if (!arm)
> > - return ENOMEM;
> > + return NULL;
>
> Please correct me if I'm wrong, but I think there's double allocation
> here for the variable arm since we've already allocated memory for at
> the start of the function.
Yes, I will fix it in v3. Clearly I was working too late :-)
> >
> > /* bl, blr */
> > err = regcomp(&arm->call_insn, "^blr?$", REG_EXTENDED);
> > if (err)
> > goto out_free_arm;
> > +
> > /* b, b.cond, br, cbz/cbnz, tbz/tbnz */
> > err = regcomp(&arm->jump_insn, "^[ct]?br?\\.?(cc|cs|eq|ge|gt|hi|hs|le|lo|ls|lt|mi|ne|pl|vc|vs)?n?z?$",
> > REG_EXTENDED);
> > if (err)
> > goto out_free_call;
> >
> > - arch->initialized = true;
> > - arch->priv = arm;
> > - arch->associate_instruction_ops = arm64__associate_instruction_ops;
> > - arch->objdump.comment_char = '/';
> > - arch->objdump.skip_functions_char = '+';
> > - return 0;
> > + return arch;
> >
> > out_free_call:
> > regfree(&arm->call_insn);
> > out_free_arm:
> > free(arm);
> > - return SYMBOL_ANNOTATE_ERRNO__ARCH_INIT_REGEXP;
> > + errno = SYMBOL_ANNOTATE_ERRNO__ARCH_INIT_REGEXP;
> > + return NULL;
> > }
[snip]
> > diff --git a/tools/perf/util/annotate-arch/annotate-powerpc.c b/tools/perf/util/annotate-arch/annotate-powerpc.c
> > index 593c138c8104..004e6137aa03 100644
> > --- a/tools/perf/util/annotate-arch/annotate-powerpc.c
> > +++ b/tools/perf/util/annotate-arch/annotate-powerpc.c
> > @@ -390,17 +390,21 @@ static void update_insn_state_powerpc(struct type_state *state,
> > }
> > #endif /* HAVE_LIBDW_SUPPORT */
> >
> > -int powerpc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
> > +const struct arch *arch__new_powerpc(const struct e_machine_and_e_flags *id,
> > + const char *cpuid __maybe_unused)
> > {
> > - if (!arch->initialized) {
> > - arch->initialized = true;
> > - arch->associate_instruction_ops = powerpc__associate_instruction_ops;
> > - arch->objdump.comment_char = '#';
> > - annotate_opts.show_asm_raw = true;
> > + struct arch *arch = zalloc(sizeof(*arch));
> > +
> > + if (!arch)
> > + return NULL;
> > +
> > + arch->name = "powerpc";
> > + arch->id = *id;
> > + arch->objdump.comment_char = '#';
> > + annotate_opts.show_asm_raw = true;
> > + arch->associate_instruction_ops = powerpc__associate_instruction_ops;
> > #ifdef HAVE_LIBDW_SUPPORT
> > - arch->update_insn_state = update_insn_state_powerpc;
> > + arch->update_insn_state = update_insn_state_powerpc;
> > #endif
> > - }
> > -
> > return 0;
>
> Shouldn't we return arch here?
Agreed.
> > }
[snip]
> > diff --git a/tools/perf/util/annotate-arch/annotate-sparc.c b/tools/perf/util/annotate-arch/annotate-sparc.c
> > index 66a0174376dd..fe7a37966261 100644
> > --- a/tools/perf/util/annotate-arch/annotate-sparc.c
> > +++ b/tools/perf/util/annotate-arch/annotate-sparc.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> > #include <string.h>
> > #include <linux/compiler.h>
> > +#include <linux/zalloc.h>
> > #include "../../util/disasm.h"
> >
> > static int is_branch_cond(const char *cond)
> > @@ -160,13 +161,17 @@ static const struct ins_ops *sparc__associate_instruction_ops(struct arch *arch,
> > return ops;
> > }
> >
> > -int sparc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
> > +const struct arch *arch__new_sparc(const struct e_machine_and_e_flags *id,
> > + const char *cpuid __maybe_unused)
> > {
> > - if (!arch->initialized) {
> > - arch->initialized = true;
> > - arch->associate_instruction_ops = sparc__associate_instruction_ops;
> > - arch->objdump.comment_char = '#';
> > - }
> > + struct arch *arch = zalloc(sizeof(*arch));
> > +
> > + if (!arch)
> > + return NULL;
> >
> > + arch->name = "sparc";
> > + arch->id = *id;
> > + arch->associate_instruction_ops = sparc__associate_instruction_ops;
> > + arch->objdump.comment_char = '#';
> > return 0;
>
> Same here as well, 0 is returned instead of the arch pointer.
Agreed.
Thanks,
Ian
Powered by blists - more mailing lists