Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 14:49:04 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Ian Rogers <irogers@google.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        John Garry <john.g.garry@oracle.com>,
        Will Deacon <will@kernel.org>,
        James Clark <james.clark@arm.com>,
        Mike Leach <mike.leach@linaro.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Guo Ren <guoren@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Eric Lin <eric.lin@sifive.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Qi Liu <liuqi115@huawei.com>,
        Sandipan Das <sandipan.das@amd.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-perf-users@vger.kernel.org, linux-csky@vger.kernel.org,
        linux-riscv@lists.infradead.org
Subject: Re: [PATCH v1 2/5] perf parse-regs: Introduce functions
 arch__reg_{ip|sp}()
Message-ID: <20230523064904.GA1969788@leoy-yangtze.lan>
References: <20230520025537.1811986-1-leo.yan@linaro.org>
 <20230520025537.1811986-3-leo.yan@linaro.org>
 <CAP-5=fU-+8Sdw0tfQNYN40tXanrzUgqv0=6jsHqGR9cgjkQa2A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP-5=fU-+8Sdw0tfQNYN40tXanrzUgqv0=6jsHqGR9cgjkQa2A@mail.gmail.com>
Precedence: bulk

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