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

[Ian Rogers]

On Fri, Oct 4, 2024 at 7:45 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> 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."

So looking at the code I think the whole thing looks wrong. The
get_arch_regnum function is used by get_dwarf_regnum which is used in
2 places in annotate.c:
```
static int extract_reg_offset(struct arch *arch, const char *str,
                             struct annotated_op_loc *op_loc)
...
int annotate_get_insn_location(struct arch *arch, struct disasm_line *dl,
                              struct annotated_insn_loc *loc)
```
So these functions are passing in an architecture. In get_dwarf_regnum:
```
/* Return DWARF register number from architecture register name */
int get_dwarf_regnum(const char *name, unsigned int machine)
{
       char *regname = strdup(name);
       int reg = -1;
       char *p;

       if (regname == NULL)
               return -EINVAL;

       /* For convenience, remove trailing characters */
       p = strpbrk(regname, " ,)");
       if (p)
               *p = '\0';

       switch (machine) {
       case EM_NONE:   /* Generic arch - use host arch */
               reg = get_arch_regnum(regname);
               break;
       default:
               pr_err("ELF MACHINE %x is not supported.\n", machine);
       }
       free(regname);
       return reg;
}
```
But why, if the machine is EM_X86_64 and I'm on an x86-64, can't I
call get_arch_regnum? The code should be something like:
```
if (machine == EM_NONE) {
#ifdef __x86_64__
  machine = EM_X86_64;
#elf...
```
Once we have an architecture specific machine then instead of
get_arch_regnum it should call get_x86_64_regnum or
get_aarch64_regnum.
```
switch(machine) }
case EM_X86_64:
         reg = get_x86_64_regnum(regname);
         break;
...
```
Is this better? Yes, it means that the annotate logic can work if,
say, annotating/disassembling an ARM binary on an x86-64.

So we need to pull all the tools/perf/arch/<arch>/util/dwarf-regs.c
files into tools/perf/util/dwarf-regs-<arch>.c files. We need to
rename the get_arch_regnum to reflect the <arch> in the file name. The
Makefile logic can include all of this unconditionally and
PERF_HAVE_DWARF_REGS can just be removed. In the process the ability
to annotate binaries from one architecture on another is improved. It
needn't be the case that we have dwarf regs for the architecture perf
is being run on as we may be annotating an x86-64 binary where there
is support.

What's strange is that get_dwarf_regstr in the common code already
does things pretty much this way. This whole Makefile, arch, weak
function, PERF_HAVE... logic just looks like a mistake that is making
the tool worse than it needs to be. I think this is frequently the
case with code in arch/, a lot of the functionality there can be moved
into pmu.c and doing things conditional on the pmu, which is
inherently architecture dependent. This can fix unusual cases of say
running the perf tool on user land qemu, where we may have an ARM perf
binary but see the PMUs of an x86.

I can work to put this into a v2 so please scream if my reasoning
doesn't make sense.

Thanks,
Ian