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.
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-googOn 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.
Thank you,
> 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>
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.
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@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>
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>
© 2016 - 2024 Red Hat, Inc.