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@xxxxxxxxxx> > > --- > [...] > > > > -#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
