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