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
