Message-Id: <20241004234544.76317dc8c7027bcd4b70fda3@kernel.org>
Date: Fri, 4 Oct 2024 23:45:44 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Ian Rogers <irogers@...gle.com>, Masami Hiramatsu <mhiramat@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland
 <mark.rutland@....com>, Alexander Shishkin
 <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, Adrian
 Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>,
 John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>, James
 Clark <james.clark@...aro.org>, Mike Leach <mike.leach@...aro.org>, Leo Yan
 <leo.yan@...ux.dev>, Guo Ren <guoren@...nel.org>, Paul Walmsley
 <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
 <aou@...s.berkeley.edu>, Nick Terrell <terrelln@...com>, Guilherme Amadio
 <amadio@...too.org>, Changbin Du <changbin.du@...wei.com>,
 "Steinar H. Gunderson" <sesse@...gle.com>, Aditya Gupta
 <adityag@...ux.ibm.com>, Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
 Masahiro Yamada <masahiroy@...nel.org>, Huacai Chen
 <chenhuacai@...nel.org>, Bibo Mao <maobibo@...ngson.cn>, Kajol Jain
 <kjain@...ux.ibm.com>, Anup Patel <anup@...infault.org>, Shenlin Liang
 <liangshenlin@...incomputing.com>, Atish Patra <atishp@...osinc.com>,
 Oliver Upton <oliver.upton@...ux.dev>, Chen Pei <cp0613@...ux.alibaba.com>,
 Dima Kogan <dima@...retsauce.net>, Alexander Lobakin
 <aleksander.lobakin@...el.com>, "David S. Miller" <davem@...emloft.net>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, Yang Jihong
 <yangjihong@...edance.com>
Subject: Re: [PATCH v1 11/11] perf build: Rename PERF_HAVE_DWARF_REGS to
 PERF_HAVE_LIBDW_REGS

On Thu, 3 Oct 2024 22:12:25 -0700
Namhyung Kim <namhyung@...nel.org> wrote:

> On Thu, Oct 03, 2024 at 05:58:13PM -0700, Ian Rogers wrote:
> > On Thu, Oct 3, 2024 at 3:48 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > > I agree renaming libdw-specific parts.  But the register is for DWARF,
> > > not libdw even if it's currently used by libdw only.   So I don't want
> > > to rename it.
> > 
> > So your objection is that we have files called:
> > tools/perf/arch/*/util/dwarf-regs.c
> > and PERF_HAVE_DRWARF_REGS is an indication that this file exists. This
> > file declares a single get_arch_regnum function. The building of the
> > file after this series is:
> > perf-util-$(CONFIG_LIBDW)     += dwarf-regs.o
> 
> Well.. I think we can even make it
> 
> perf-util-y += dwarf-regs.o
> 
> since it doesn't have any dependency on libdw.  But it'd be inefficent
> to ship the dead code and data.  Anyway we may remove the condition to
> define the PERF_HAVE_DWARF_REGS like below.
> 
> diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
> index 67b4969a673836eb..f1eb1ee1ea25ca53 100644
> --- a/tools/perf/arch/x86/Makefile
> +++ b/tools/perf/arch/x86/Makefile
> @@ -1,7 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -ifndef NO_DWARF
>  PERF_HAVE_DWARF_REGS := 1
> -endif
>  HAVE_KVM_STAT_SUPPORT := 1
>  PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
>  PERF_HAVE_JITDUMP := 1
> 
> > 
> > My objection is that PERF_HAVE_DWARF_REGS is controlling the #define
> > HAVE_LIBDW_SUPPORT, so dwarf (that can mean libunwind, libdw, etc.) is
> > guarding having libdw which is backward and part of what this series
> > has been trying to clean up.
> 
> Why not?  If the arch doesn't define DWARF registers, it can refuse
> libdw support because it won't work well.

It actually does not DWARF registers, but just "dwarf-regs.c" file
since arch should define DWARF registers if the compiler generates
the DWARF.
Here the flag means only "we implemented dwarf-regs.c file for this
arch." So if it is called as "libdw-helper.c" then we can rename the
flag as PERF_HAVE_ARCH_LIBDW_HELPER simply.

> > If we rename tools/perf/arch/*/util/dwarf-regs.c to
> > tools/perf/arch/*/util/libdw-helpers.c the PERF_HAVE_DWARF_REGS can be
> > renamed to PERF_HAVE_LIBDW_HELPERS to align. Then
> > PERF_HAVE_LIBDW_HELPERS guarding the #define PERF_HAVE_LIBDW makes
> > sense to me and I think we achieve the filename alignment you are
> > looking for.
> 
> I don't think it's a good idea.  The logic is not specific to libdw.

Yes, the logic (DWARF register mapping to the ISA register name) is
not libdw. But I think we can also implement it in "libdw-helper.c".
(In fact, this implementation does not depend only on Dwarf, but 
 rather on the convenience of ftrace.)

> > 
> > Yes get_arch_regnum could make sense out of libdw and needn't just be
> > a helper for it, but let's worry about that when there's a need.
> > What's confusing at the moment is does libdw provide dwarf support,
> > which I'd say is expected, or does dwarf provide libdw support?
> 
> As I said, it's about refusing libdw.

I think Ian pointed this part, even if libdw is available, dwarf-regs.c
controls its usage, but libunwind is not.

> 
>   ifndef NO_LIBDW
>     ifeq ($(origin PERF_HAVE_DWARF_REGS), undefined)
>       $(warning DWARF register mappings have not been defined for architecture $(SRCARCH), DWARF support disabled)

I think *this message* is the root cause of this discussion. It should be
changed to 

"DWARF register mappings have not been defined for architecture $(SRCARCH), libdw support disabled."

or (if changed to libdw-helper)

"libdw-helper.c is not implemented for architecture $(SRCARCH), libdw support disabled."

Thank you,

>       NO_LIBDW := 1
>     else
>       CFLAGS += -DHAVE_DWARF_SUPPORT $(LIBDW_CFLAGS)
>       LDFLAGS += $(LIBDW_LDFLAGS)
>       EXTLIBS += ${DWARFLIBS}
>       $(call detected,CONFIG_DWARF)
>     endif # PERF_HAVE_DWARF_REGS
>   endif # NO_LIBDW
> 
> Thanks,
> Namhyung
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists