Re: [PATCH v2 16/31] perf dwarf-regs: Pass accurate disassembly machine to get_dwarf_regnum

[Ian Rogers]

On Mon, Oct 7, 2024 at 1:08 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Sat,  5 Oct 2024 12:55:26 -0700
> Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> > Rather than pass 0/EM_NONE, use the value computed in the disasm
> > struct arch. Switch the EM_NONE case to EM_HOST, rewriting EM_NONE if
> > it were passed to get_dwarf_regnum. Pass a flags value as
> > architectures like csky need the flags to determine the ABI variant.
> >
>
> Does this change the command output when we use it for cross-build
> environment? E.g. remote arch is different from host arch? If so,
> please add output examples with/without this change.

The cases where this would apply are small as get_arch_regnum is only
implemented for x86. get_dwarf_regnum likewise only works for x86 and
it is only called by annotate.
In this code without this patch the behavior is to return -ENOTSUP, ie
the code is set up to fail and this code just makes it not fail for
the x86 case (when not on x86) with code that is well tested on x86.
The code exists as x86 registers may be the same dwarf number but have
different names: e.g. rax, eax, ax, al. I'm not sure this reaches a
high complexity level for extensive testing. I'll see if I can grab an
x86 perf.data file to analyze on ARM, but I don't think doing this
should gate the series.

Thanks,
Ian

> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/annotate.c           | 6 +++---
> >  tools/perf/util/dwarf-regs.c         | 8 ++++++--
> >  tools/perf/util/include/dwarf-regs.h | 5 +++--
> >  3 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 37ce43c4eb8f..b1d98da79be8 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -2292,7 +2292,7 @@ static int extract_reg_offset(struct arch *arch, const char *str,
> >       if (regname == NULL)
> >               return -1;
> >
> > -     op_loc->reg1 = get_dwarf_regnum(regname, 0);
> > +     op_loc->reg1 = get_dwarf_regnum(regname, arch->e_machine, arch->e_flags);
> >       free(regname);
> >
> >       /* Get the second register */
> > @@ -2305,7 +2305,7 @@ static int extract_reg_offset(struct arch *arch, const char *str,
> >               if (regname == NULL)
> >                       return -1;
> >
> > -             op_loc->reg2 = get_dwarf_regnum(regname, 0);
> > +             op_loc->reg2 = get_dwarf_regnum(regname, arch->e_machine, arch->e_flags);
> >               free(regname);
> >       }
> >       return 0;
> > @@ -2405,7 +2405,7 @@ int annotate_get_insn_location(struct arch *arch, struct disasm_line *dl,
> >                               return -1;
> >
> >                       if (*s == arch->objdump.register_char)
> > -                             op_loc->reg1 = get_dwarf_regnum(s, 0);
> > +                             op_loc->reg1 = get_dwarf_regnum(s, arch->e_machine, arch->e_flags);
> >                       else if (*s == arch->objdump.imm_char) {
> >                               op_loc->offset = strtol(s + 1, &p, 0);
> >                               if (p && p != s + 1)
> > diff --git a/tools/perf/util/dwarf-regs.c b/tools/perf/util/dwarf-regs.c
> > index 7c01bc4d7e5b..1321387f6948 100644
> > --- a/tools/perf/util/dwarf-regs.c
> > +++ b/tools/perf/util/dwarf-regs.c
> > @@ -70,7 +70,7 @@ __weak int get_arch_regnum(const char *name __maybe_unused)
> >  }
> >
> >  /* Return DWARF register number from architecture register name */
> > -int get_dwarf_regnum(const char *name, unsigned int machine)
> > +int get_dwarf_regnum(const char *name, unsigned int machine, unsigned int flags __maybe_unused)
> >  {
> >       char *regname = strdup(name);
> >       int reg = -1;
> > @@ -84,8 +84,12 @@ int get_dwarf_regnum(const char *name, unsigned int machine)
> >       if (p)
> >               *p = '\0';
> >
> > +     if (machine == EM_NONE) {
> > +             /* Generic arch - use host arch */
> > +             machine = EM_HOST;
> > +     }
> >       switch (machine) {
> > -     case EM_NONE:   /* Generic arch - use host arch */
> > +     case EM_HOST:
> >               reg = get_arch_regnum(regname);
> >               break;
> >       default:
> > diff --git a/tools/perf/util/include/dwarf-regs.h b/tools/perf/util/include/dwarf-regs.h
> > index f4f87ded5e3d..ee0a734564c7 100644
> > --- a/tools/perf/util/include/dwarf-regs.h
> > +++ b/tools/perf/util/include/dwarf-regs.h
> > @@ -93,12 +93,13 @@ int get_arch_regnum(const char *name);
> >   * name: architecture register name
> >   * machine: ELF machine signature (EM_*)
> >   */
> > -int get_dwarf_regnum(const char *name, unsigned int machine);
> > +int get_dwarf_regnum(const char *name, unsigned int machine, unsigned int flags);
> >
> >  #else /* HAVE_LIBDW_SUPPORT */
> >
> >  static inline int get_dwarf_regnum(const char *name __maybe_unused,
> > -                                unsigned int machine __maybe_unused)
> > +                                unsigned int machine __maybe_unused,
> > +                                unsigned int flags __maybe_unused)
> >  {
> >       return -1;
> >  }
> > --
> > 2.47.0.rc0.187.ge670bccf7e-goog
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>