On Fri, May 19, 2023 at 7:56 PM Leo Yan <leo.yan@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
