| Message ID | 20230520025537.1811986-3-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | perf parse-regs: Refactor arch related functions | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
On 20/05/2023 03:55, Leo Yan wrote: > Ideally, we want util/perf_regs.c to be general enough and doesn't bind > with specific architecture. > > But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP > which are defined by architecture, thus util/perf_regs.c is dependent on > architecture header (see util/perf_regs.h includes "<perf_regs.h>", here > perf_regs.h is architecture specific header). > > As a step to generalize util/perf_regs.c, this commit introduces weak > functions arch__reg_ip() and arch__reg_sp() and every architecture can > define their own functions; thus, util/perf_regs.c doesn't need to use > PERF_REG_IP and PERF_REG_SP anymore. > > This is a preparation to get rid of architecture specific header from > util/perf_regs.h. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- [...] > > -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP)) > +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp())) > > const char *perf_reg_name(int id, const char *arch); > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id); > diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c > index bdccfc511b7e..f308f2ea512b 100644 > --- a/tools/perf/util/unwind-libdw.c > +++ b/tools/perf/util/unwind-libdw.c > @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, > if (!ui->dwfl) > goto out; > > - err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP); > + err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip()); Shouldn't it be more like this, because the weak symbols are a compile time thing and it's supposed to support cross arch unwinding at runtime (assuming something containing the arch from the file is passed down, like we did with perf_reg_name()): char *arch = perf_env__arch(evsel__env(evsel)); err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip(arch)); Now I'm wondering how cross unwinding ever worked because I see libunwind also has something hard coded too: #define LIBUNWIND__ARCH_REG_SP PERF_REG_SP
On Mon, May 22, 2023 at 09:57:25AM +0100, James Clark wrote: > > > On 20/05/2023 03:55, Leo Yan wrote: > > Ideally, we want util/perf_regs.c to be general enough and doesn't bind > > with specific architecture. > > > > But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP > > which are defined by architecture, thus util/perf_regs.c is dependent on > > architecture header (see util/perf_regs.h includes "<perf_regs.h>", here > > perf_regs.h is architecture specific header). > > > > As a step to generalize util/perf_regs.c, this commit introduces weak > > functions arch__reg_ip() and arch__reg_sp() and every architecture can > > define their own functions; thus, util/perf_regs.c doesn't need to use > > PERF_REG_IP and PERF_REG_SP anymore. > > > > This is a preparation to get rid of architecture specific header from > > util/perf_regs.h. > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > --- > [...] > > > > -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP)) > > +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp())) > > > > const char *perf_reg_name(int id, const char *arch); > > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id); > > diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c > > index bdccfc511b7e..f308f2ea512b 100644 > > --- a/tools/perf/util/unwind-libdw.c > > +++ b/tools/perf/util/unwind-libdw.c > > @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, > > if (!ui->dwfl) > > goto out; > > > > - err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP); > > + err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip()); > > Shouldn't it be more like this, because the weak symbols are a compile > time thing and it's supposed to support cross arch unwinding at runtime > (assuming something containing the arch from the file is passed down, > like we did with perf_reg_name()): > > char *arch = perf_env__arch(evsel__env(evsel)); > err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip(arch)); Thanks for pointing out, James. Agreed that we need to return the IP and SP register based on the arch. I will look into more details and spin for a new patch set for this. > Now I'm wondering how cross unwinding ever worked because I see > libunwind also has something hard coded too: > > #define LIBUNWIND__ARCH_REG_SP PERF_REG_SP Yeah, I also used arch__reg_sp() to replace PERF_REG_SP; but as you suggestion, we should fix this with passing 'arch' parameter for getting SP register based on arch. Another important thing is to find a good test for cross unwinding. Maybe I can use tools/perf/tests/shell/record.sh, function test_register_capture() for testing registers, if you have any other suggesion, please let me know. Thanks, Leo
On 22/05/2023 13:07, Leo Yan wrote: > On Mon, May 22, 2023 at 09:57:25AM +0100, James Clark wrote: >> >> >> On 20/05/2023 03:55, Leo Yan wrote: >>> Ideally, we want util/perf_regs.c to be general enough and doesn't bind >>> with specific architecture. >>> >>> But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP >>> which are defined by architecture, thus util/perf_regs.c is dependent on >>> architecture header (see util/perf_regs.h includes "<perf_regs.h>", here >>> perf_regs.h is architecture specific header). >>> >>> As a step to generalize util/perf_regs.c, this commit introduces weak >>> functions arch__reg_ip() and arch__reg_sp() and every architecture can >>> define their own functions; thus, util/perf_regs.c doesn't need to use >>> PERF_REG_IP and PERF_REG_SP anymore. >>> >>> This is a preparation to get rid of architecture specific header from >>> util/perf_regs.h. >>> >>> Signed-off-by: Leo Yan <leo.yan@linaro.org> >>> --- >> [...] >>> >>> -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP)) >>> +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp())) >>> >>> const char *perf_reg_name(int id, const char *arch); >>> int perf_reg_value(u64 *valp, struct regs_dump *regs, int id); >>> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c >>> index bdccfc511b7e..f308f2ea512b 100644 >>> --- a/tools/perf/util/unwind-libdw.c >>> +++ b/tools/perf/util/unwind-libdw.c >>> @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, >>> if (!ui->dwfl) >>> goto out; >>> >>> - err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP); >>> + err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip()); >> >> Shouldn't it be more like this, because the weak symbols are a compile >> time thing and it's supposed to support cross arch unwinding at runtime >> (assuming something containing the arch from the file is passed down, >> like we did with perf_reg_name()): >> >> char *arch = perf_env__arch(evsel__env(evsel)); >> err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip(arch)); > > Thanks for pointing out, James. > > Agreed that we need to return the IP and SP register based on the > arch. I will look into more details and spin for a new patch set for > this. > You might be able to skip the extra work for now though, seeing as your change is no worse than it was before and it fixes the duplicate declaration issue. >> Now I'm wondering how cross unwinding ever worked because I see >> libunwind also has something hard coded too: >> >> #define LIBUNWIND__ARCH_REG_SP PERF_REG_SP > > Yeah, I also used arch__reg_sp() to replace PERF_REG_SP; but as you > suggestion, we should fix this with passing 'arch' parameter for > getting SP register based on arch. > > Another important thing is to find a good test for cross unwinding. > Maybe I can use tools/perf/tests/shell/record.sh, function > test_register_capture() for testing registers, if you have any other > suggesion, please let me know. The only way I can think is to have pre-recorded perf.data files in the repo, but they'd also need the binary from the recording too. I think this is probably not a very good idea because it's going to make the repo huge if we keep changing them (which we will). Some kind of third party perf test suite hosted somewhere might work but I don't think this exists. > > Thanks, > Leo
On Fri, May 19, 2023 at 7:56 PM Leo Yan <leo.yan@linaro.org> wrote: > > Ideally, we want util/perf_regs.c to be general enough and doesn't bind > with specific architecture. > > But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP > which are defined by architecture, thus util/perf_regs.c is dependent on > architecture header (see util/perf_regs.h includes "<perf_regs.h>", here > perf_regs.h is architecture specific header). > > As a step to generalize util/perf_regs.c, this commit introduces weak > functions arch__reg_ip() and arch__reg_sp() and every architecture can > define their own functions; thus, util/perf_regs.c doesn't need to use > PERF_REG_IP and PERF_REG_SP anymore. > > This is a preparation to get rid of architecture specific header from > util/perf_regs.h. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/arch/arm/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/arm64/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/csky/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/mips/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/powerpc/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/riscv/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/s390/util/perf_regs.c | 10 ++++++++++ > tools/perf/arch/x86/util/perf_regs.c | 10 ++++++++++ > tools/perf/util/perf_regs.c | 10 ++++++++++ > tools/perf/util/perf_regs.h | 4 +++- > tools/perf/util/unwind-libdw.c | 2 +- > tools/perf/util/unwind.h | 4 ++-- > 12 files changed, 96 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/arch/arm/util/perf_regs.c b/tools/perf/arch/arm/util/perf_regs.c > index 2833e101a7c6..37aa3a2091bd 100644 > --- a/tools/perf/arch/arm/util/perf_regs.c > +++ b/tools/perf/arch/arm/util/perf_regs.c > @@ -4,3 +4,13 @@ > const struct sample_reg sample_reg_masks[] = { > SMPL_REG_END > }; > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_ARM_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_ARM_SP; > +} > diff --git a/tools/perf/arch/arm64/util/perf_regs.c b/tools/perf/arch/arm64/util/perf_regs.c > index 006692c9b040..dbe7f00b222b 100644 > --- a/tools/perf/arch/arm64/util/perf_regs.c > +++ b/tools/perf/arch/arm64/util/perf_regs.c > @@ -169,3 +169,13 @@ uint64_t arch__user_reg_mask(void) > } > return PERF_REGS_MASK; > } > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_ARM64_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_ARM64_SP; > +} > diff --git a/tools/perf/arch/csky/util/perf_regs.c b/tools/perf/arch/csky/util/perf_regs.c > index 2864e2e3776d..d230d7e640fd 100644 > --- a/tools/perf/arch/csky/util/perf_regs.c > +++ b/tools/perf/arch/csky/util/perf_regs.c > @@ -4,3 +4,13 @@ > const struct sample_reg sample_reg_masks[] = { > SMPL_REG_END > }; > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_CSKY_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_CSKY_SP; > +} > diff --git a/tools/perf/arch/mips/util/perf_regs.c b/tools/perf/arch/mips/util/perf_regs.c > index 2864e2e3776d..64882ebc9287 100644 > --- a/tools/perf/arch/mips/util/perf_regs.c > +++ b/tools/perf/arch/mips/util/perf_regs.c > @@ -4,3 +4,13 @@ > const struct sample_reg sample_reg_masks[] = { > SMPL_REG_END > }; > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_MIPS_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_MIPS_R29; > +} > diff --git a/tools/perf/arch/powerpc/util/perf_regs.c b/tools/perf/arch/powerpc/util/perf_regs.c > index 8d07a78e742a..c84cd79986a8 100644 > --- a/tools/perf/arch/powerpc/util/perf_regs.c > +++ b/tools/perf/arch/powerpc/util/perf_regs.c > @@ -226,3 +226,13 @@ uint64_t arch__intr_reg_mask(void) > } > return mask; > } > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_POWERPC_NIP; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_POWERPC_R1; > +} > diff --git a/tools/perf/arch/riscv/util/perf_regs.c b/tools/perf/arch/riscv/util/perf_regs.c > index 2864e2e3776d..13bbddd139d0 100644 > --- a/tools/perf/arch/riscv/util/perf_regs.c > +++ b/tools/perf/arch/riscv/util/perf_regs.c > @@ -4,3 +4,13 @@ > const struct sample_reg sample_reg_masks[] = { > SMPL_REG_END > }; > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_RISCV_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_RISCV_SP; > +} > diff --git a/tools/perf/arch/s390/util/perf_regs.c b/tools/perf/arch/s390/util/perf_regs.c > index 2864e2e3776d..9b2297471090 100644 > --- a/tools/perf/arch/s390/util/perf_regs.c > +++ b/tools/perf/arch/s390/util/perf_regs.c > @@ -4,3 +4,13 @@ > const struct sample_reg sample_reg_masks[] = { > SMPL_REG_END > }; > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_S390_PC; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_S390_R15; > +} > diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c > index 0ed177991ad0..c752a6e9cba6 100644 > --- a/tools/perf/arch/x86/util/perf_regs.c > +++ b/tools/perf/arch/x86/util/perf_regs.c > @@ -312,3 +312,13 @@ uint64_t arch__intr_reg_mask(void) > > return PERF_REGS_MASK; > } > + > +uint64_t arch__reg_ip(void) > +{ > + return PERF_REG_X86_IP; > +} > + > +uint64_t arch__reg_sp(void) > +{ > + return PERF_REG_X86_SP; > +} > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 8720ec6cf147..334c9a2b785d 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -20,6 +20,16 @@ uint64_t __weak arch__user_reg_mask(void) > return PERF_REGS_MASK; > } > > +uint64_t __weak arch__reg_ip(void) > +{ > + return 0; > +} > + > +uint64_t __weak arch__reg_sp(void) > +{ > + return 0; > +} > + Is there a need for the weak function if there is a definition for every architecture? A problem with weak definitions is that they are not part of the C standard, so strange things can happen such as inlining - although I think this code is safe. Not having the weak functions means that if someone tries to bring up a new architecture they will get linker failures until they add the definitions. Failing to link seems better than silently succeeding but then having to track down runtime failures because these functions are returning 0. Thanks, Ian > #ifdef HAVE_PERF_REGS_SUPPORT > > const char *perf_reg_name(int id, const char *arch) > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > index ab4ec3f2a170..0a1460aaad37 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -26,13 +26,15 @@ enum { > int arch_sdt_arg_parse_op(char *old_op, char **new_op); > uint64_t arch__intr_reg_mask(void); > uint64_t arch__user_reg_mask(void); > +uint64_t arch__reg_ip(void); > +uint64_t arch__reg_sp(void); > > #ifdef HAVE_PERF_REGS_SUPPORT > extern const struct sample_reg sample_reg_masks[]; > > #include <perf_regs.h> > > -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP)) > +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp())) > > const char *perf_reg_name(int id, const char *arch); > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id); > diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c > index bdccfc511b7e..f308f2ea512b 100644 > --- a/tools/perf/util/unwind-libdw.c > +++ b/tools/perf/util/unwind-libdw.c > @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, > if (!ui->dwfl) > goto out; > > - err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP); > + err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip()); > if (err) > goto out; > > diff --git a/tools/perf/util/unwind.h b/tools/perf/util/unwind.h > index b2a03fa5289b..0a98ea9d8c94 100644 > --- a/tools/perf/util/unwind.h > +++ b/tools/perf/util/unwind.h > @@ -43,11 +43,11 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, > #endif > > #ifndef LIBUNWIND__ARCH_REG_SP > -#define LIBUNWIND__ARCH_REG_SP PERF_REG_SP > +#define LIBUNWIND__ARCH_REG_SP arch__reg_sp() > #endif > > #ifndef LIBUNWIND__ARCH_REG_IP > -#define LIBUNWIND__ARCH_REG_IP PERF_REG_IP > +#define LIBUNWIND__ARCH_REG_IP arch__reg_ip() > #endif > > int LIBUNWIND__ARCH_REG_ID(int regnum); > -- > 2.39.2 >
On Mon, May 22, 2023 at 11:08:12AM -0700, Ian Rogers wrote: [...] > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > > index 8720ec6cf147..334c9a2b785d 100644 > > --- a/tools/perf/util/perf_regs.c > > +++ b/tools/perf/util/perf_regs.c > > @@ -20,6 +20,16 @@ uint64_t __weak arch__user_reg_mask(void) > > return PERF_REGS_MASK; > > } > > > > +uint64_t __weak arch__reg_ip(void) > > +{ > > + return 0; > > +} > > + > > +uint64_t __weak arch__reg_sp(void) > > +{ > > + return 0; > > +} > > + > > Is there a need for the weak function if there is a definition for > every architecture? In current code, some archs don't support register parsing (e.g. arch/alpha, arch/parisc, arch/riscv64, etc), this is why I added weak functions to avoid building breakage for these archs. > A problem with weak definitions is that they are > not part of the C standard, so strange things can happen such as > inlining - although I think this code is safe. Good to know this info, thanks for sharing. > Not having the weak > functions means that if someone tries to bring up a new architecture > they will get linker failures until they add the definitions. Failing > to link seems better than silently succeeding but then having to track > down runtime failures because these functions are returning 0. I agreed that removing weak functions is better way to move forward. If removing the weak functions, we need to handle cases for below archs which don't support register parsing: arch/alpha/ arch/arc/ arch/parisc/ arch/riscv64/ arch/sh/ arch/sparc/ arch/xtensa/ As James pointed out perf fails to support cross unwinding, I will update this patch, the new version's arch__reg_ip() / arch__reg_sp() will return IP and SP registers based on the passed 'arch' parameter; for above unsupported archs, arch__reg_ip() / arch__reg_sp() will return error and architecture developers can extend register parsing in the future. In this way, we also can remove weak definitions, this can give us an extra benefit :) Thanks, Leo
diff --git a/tools/perf/arch/arm/util/perf_regs.c b/tools/perf/arch/arm/util/perf_regs.c index 2833e101a7c6..37aa3a2091bd 100644 --- a/tools/perf/arch/arm/util/perf_regs.c +++ b/tools/perf/arch/arm/util/perf_regs.c @@ -4,3 +4,13 @@ const struct sample_reg sample_reg_masks[] = { SMPL_REG_END }; + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_ARM_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_ARM_SP; +} diff --git a/tools/perf/arch/arm64/util/perf_regs.c b/tools/perf/arch/arm64/util/perf_regs.c index 006692c9b040..dbe7f00b222b 100644 --- a/tools/perf/arch/arm64/util/perf_regs.c +++ b/tools/perf/arch/arm64/util/perf_regs.c @@ -169,3 +169,13 @@ uint64_t arch__user_reg_mask(void) } return PERF_REGS_MASK; } + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_ARM64_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_ARM64_SP; +} diff --git a/tools/perf/arch/csky/util/perf_regs.c b/tools/perf/arch/csky/util/perf_regs.c index 2864e2e3776d..d230d7e640fd 100644 --- a/tools/perf/arch/csky/util/perf_regs.c +++ b/tools/perf/arch/csky/util/perf_regs.c @@ -4,3 +4,13 @@ const struct sample_reg sample_reg_masks[] = { SMPL_REG_END }; + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_CSKY_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_CSKY_SP; +} diff --git a/tools/perf/arch/mips/util/perf_regs.c b/tools/perf/arch/mips/util/perf_regs.c index 2864e2e3776d..64882ebc9287 100644 --- a/tools/perf/arch/mips/util/perf_regs.c +++ b/tools/perf/arch/mips/util/perf_regs.c @@ -4,3 +4,13 @@ const struct sample_reg sample_reg_masks[] = { SMPL_REG_END }; + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_MIPS_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_MIPS_R29; +} diff --git a/tools/perf/arch/powerpc/util/perf_regs.c b/tools/perf/arch/powerpc/util/perf_regs.c index 8d07a78e742a..c84cd79986a8 100644 --- a/tools/perf/arch/powerpc/util/perf_regs.c +++ b/tools/perf/arch/powerpc/util/perf_regs.c @@ -226,3 +226,13 @@ uint64_t arch__intr_reg_mask(void) } return mask; } + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_POWERPC_NIP; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_POWERPC_R1; +} diff --git a/tools/perf/arch/riscv/util/perf_regs.c b/tools/perf/arch/riscv/util/perf_regs.c index 2864e2e3776d..13bbddd139d0 100644 --- a/tools/perf/arch/riscv/util/perf_regs.c +++ b/tools/perf/arch/riscv/util/perf_regs.c @@ -4,3 +4,13 @@ const struct sample_reg sample_reg_masks[] = { SMPL_REG_END }; + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_RISCV_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_RISCV_SP; +} diff --git a/tools/perf/arch/s390/util/perf_regs.c b/tools/perf/arch/s390/util/perf_regs.c index 2864e2e3776d..9b2297471090 100644 --- a/tools/perf/arch/s390/util/perf_regs.c +++ b/tools/perf/arch/s390/util/perf_regs.c @@ -4,3 +4,13 @@ const struct sample_reg sample_reg_masks[] = { SMPL_REG_END }; + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_S390_PC; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_S390_R15; +} diff --git a/tools/perf/arch/x86/util/perf_regs.c b/tools/perf/arch/x86/util/perf_regs.c index 0ed177991ad0..c752a6e9cba6 100644 --- a/tools/perf/arch/x86/util/perf_regs.c +++ b/tools/perf/arch/x86/util/perf_regs.c @@ -312,3 +312,13 @@ uint64_t arch__intr_reg_mask(void) return PERF_REGS_MASK; } + +uint64_t arch__reg_ip(void) +{ + return PERF_REG_X86_IP; +} + +uint64_t arch__reg_sp(void) +{ + return PERF_REG_X86_SP; +} diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c index 8720ec6cf147..334c9a2b785d 100644 --- a/tools/perf/util/perf_regs.c +++ b/tools/perf/util/perf_regs.c @@ -20,6 +20,16 @@ uint64_t __weak arch__user_reg_mask(void) return PERF_REGS_MASK; } +uint64_t __weak arch__reg_ip(void) +{ + return 0; +} + +uint64_t __weak arch__reg_sp(void) +{ + return 0; +} + #ifdef HAVE_PERF_REGS_SUPPORT const char *perf_reg_name(int id, const char *arch) diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h index ab4ec3f2a170..0a1460aaad37 100644 --- a/tools/perf/util/perf_regs.h +++ b/tools/perf/util/perf_regs.h @@ -26,13 +26,15 @@ enum { int arch_sdt_arg_parse_op(char *old_op, char **new_op); uint64_t arch__intr_reg_mask(void); uint64_t arch__user_reg_mask(void); +uint64_t arch__reg_ip(void); +uint64_t arch__reg_sp(void); #ifdef HAVE_PERF_REGS_SUPPORT extern const struct sample_reg sample_reg_masks[]; #include <perf_regs.h> -#define DWARF_MINIMAL_REGS ((1ULL << PERF_REG_IP) | (1ULL << PERF_REG_SP)) +#define DWARF_MINIMAL_REGS ((1ULL << arch__reg_ip()) | (1ULL << arch__reg_sp())) const char *perf_reg_name(int id, const char *arch); int perf_reg_value(u64 *valp, struct regs_dump *regs, int id); diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c index bdccfc511b7e..f308f2ea512b 100644 --- a/tools/perf/util/unwind-libdw.c +++ b/tools/perf/util/unwind-libdw.c @@ -252,7 +252,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, if (!ui->dwfl) goto out; - err = perf_reg_value(&ip, &data->user_regs, PERF_REG_IP); + err = perf_reg_value(&ip, &data->user_regs, arch__reg_ip()); if (err) goto out; diff --git a/tools/perf/util/unwind.h b/tools/perf/util/unwind.h index b2a03fa5289b..0a98ea9d8c94 100644 --- a/tools/perf/util/unwind.h +++ b/tools/perf/util/unwind.h @@ -43,11 +43,11 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg, #endif #ifndef LIBUNWIND__ARCH_REG_SP -#define LIBUNWIND__ARCH_REG_SP PERF_REG_SP +#define LIBUNWIND__ARCH_REG_SP arch__reg_sp() #endif #ifndef LIBUNWIND__ARCH_REG_IP -#define LIBUNWIND__ARCH_REG_IP PERF_REG_IP +#define LIBUNWIND__ARCH_REG_IP arch__reg_ip() #endif int LIBUNWIND__ARCH_REG_ID(int regnum);
Ideally, we want util/perf_regs.c to be general enough and doesn't bind with specific architecture. But since util/perf_regs.c uses the macros PERF_REG_IP and PERF_REG_SP which are defined by architecture, thus util/perf_regs.c is dependent on architecture header (see util/perf_regs.h includes "<perf_regs.h>", here perf_regs.h is architecture specific header). As a step to generalize util/perf_regs.c, this commit introduces weak functions arch__reg_ip() and arch__reg_sp() and every architecture can define their own functions; thus, util/perf_regs.c doesn't need to use PERF_REG_IP and PERF_REG_SP anymore. This is a preparation to get rid of architecture specific header from util/perf_regs.h. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/arch/arm/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/arm64/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/csky/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/mips/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/powerpc/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/riscv/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/s390/util/perf_regs.c | 10 ++++++++++ tools/perf/arch/x86/util/perf_regs.c | 10 ++++++++++ tools/perf/util/perf_regs.c | 10 ++++++++++ tools/perf/util/perf_regs.h | 4 +++- tools/perf/util/unwind-libdw.c | 2 +- tools/perf/util/unwind.h | 4 ++-- 12 files changed, 96 insertions(+), 4 deletions(-)