Re: [PATCH v1 11/11] perf build: Rename PERF_HAVE_DWARF_REGS to PERF_HAVE_LIBDW_REGS

[Google]

On Thu, 3 Oct 2024 22:12:25 -0700
Namhyung Kim <namhyung@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>