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

[Namhyung Kim <namhyung@kernel.org>]

Hello,

Sorry for the late reply.

On Mon, Oct 07, 2024 at 08:46:59AM -0700, Ian Rogers wrote:
> On Mon, Oct 7, 2024 at 1:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Sat,  5 Oct 2024 12:55:26 -0700
> > Ian Rogers <irogers@google.com> 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.

Yep, it's used only in the data type profiling.  I don't expect it to
work in the cross environment yet.  This change moves it closer to the
working state though. :)


> 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.

Yep, as I said it's not supported yet and I don't think that's the goal
of this patch series.  So probably it's ok to merge it without the
extensive testing.

Thanks,
Namhyung

> 
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  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@kernel.org>