Re: [PATCH v2 11/12] perf disasm: Refactor arch__find and initialization of arch structs

Ian Rogers
2026-01-22T19:20:21Z

On Thu, Jan 22, 2026 at 10:45 AM Suchit Karunakaran
<suchitkarunakaran@gmail.com> wrote:
> On Thu, 22 Jan 2026 at 22:24, Ian Rogers <irogers@google.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