| Message ID | 20241005195541.380070-17-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Libdw/dwarf build clean up | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
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. 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 >
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>
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; }
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(-)