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

[Ian Rogers]

On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Tue, 1 Oct 2024 16:17:34 -0700
> Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 30, 2024 at 09:02:36PM -0700, Ian Rogers wrote:
> > > > On Sat, Sep 28, 2024 at 7:35 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 27 Sep 2024 11:15:21 -0700
> > > > > Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > > >
> > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Thu, Sep 26, 2024 at 12:55:18PM -0700, Ian Rogers wrote:
> > > > > > > > On Thu, Sep 26, 2024 at 12:40 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Sep 26, 2024 at 05:47:16AM -0700, Ian Rogers wrote:
> > > > > > > > > > On Wed, Sep 25, 2024 at 8:27 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Sep 24, 2024 at 09:04:18AM -0700, Ian Rogers wrote:
> > > > > > > > > > > > The name dwarf can imply libunwind support, whereas
> > > > > > > > > > > > PERF_HAVE_DWARF_REGS is only enabled with libdw support. Rename to
> > > > > > > > > > > > make it clearer there is a libdw connection.
> > > > > > > > > > >
> > > > > > > > > > > While it only covers libdw, I think the idea of this macro is whether
> > > > > > > > > > > the arch has register mappings defined in DWARF standard.  So I think
> > > > > > > > > > > it's better to keep the name for this case.
> > > > > > > > > >
> > > > > > > > > > How can the dwarf standard exist for an arch but not define registers?
> > > > > > > > >
> > > > > > > > > I meant it's about the arch code in the perf tools to have the mapping,
> > > > > > > > > not the DWARF standard itself.
> > > > > > > >
> > > > > > > > But we guard those definitions behind having libdw:
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/Makefile?h=perf-tools-next#n3
> > > > > > > > So we only have the regs if libdw is present, not if dwarf is in use
> > > > > > > > for libunwind/libdw. Hence wanting to be specific that they are just a
> > > > > > > > libdw and not a dwarf thing. Trying to use the regs in libunwind code
> > > > > > > > would be broken. That could change but I wanted to make the code clear
> > > > > > > > for the way things are at the moment.
> > > > > > >
> > > > > > > I understand your point but calling it LIBDW_REGS looks unnatural to me.
> > > > > >
> > > > > > I don't follow. Wouldn't it be unnatural to see PERF_HAVE_DWARF_REGS
> > > > > > in libunwind code but you are to some how know that the code only had
> > > > > > meaning if libdw was present? I don't like the implication that DWARF
> > > > > > means LIBDW as throughout the code it doesn't. I think the name
> > > > > > PERF_HAVE_LIBDW_REGS better captures how the code is, makes the code
> > > > > > more intention revealing and so readable, etc.
> > > > >
> > > > > I agree with Namhyung this point. dwarf-regs is defined only by the
> > > > > DWARF standard, not libdw only. The standard encode registers by a digit
> > > > > number and the dwarf-regs decode the number to actual register name.
> > > >
> > > > The code is not making a statement about the DWARF standard, take arch/csky:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/csky/Makefile?h=perf-tools-next
> > > > ```
> > > > # SPDX-License-Identifier: GPL-2.0-only
> > > > ifndef NO_DWARF
> > > > PERF_HAVE_DWARF_REGS := 1
> > > > endif
> > > > ```
> > > > in the patch series NO_DWARF becomes NO_LIBDW, so it is now:
> > > > ```
> > > > # SPDX-License-Identifier: GPL-2.0-only
> > > > ifndef NO_LIBDW
> > > > PERF_HAVE_DWARF_REGS := 1
> > > > endif
> > > > ```
> > > > So the Makefile says that PERF_HAVE_DWARF_REGS is dependent on having
> > > > NO_LIBDW, that is having libdw implies PERF_HAVE_DWARF_REGS is defined
> > > > for csky.
> > >
> > > I think this is totally fine and we can change the condition later if
> > > needed.
> > >
> > > After all, I don't think it's a big deal.  Let's just call DWARF
> > > registers DWARF_REGS. :)
> >
> > The define is called PERF_HAVE_DWARF_REGS, notice the HAVE, but we're
> > not setting it while supporting call-graph dwarf with libunwind. It is
> > actively confusing.
>
> Does libunwind requires the dwarf regs? I think the dwarf regs information
> is only needed for analyzing dwarf register assignment, not stack unwinding.

So you are saying the #define is guarding a libdw feature?
perf record/report --call-graph=dwarf is supported with either libdw
or libunwind. The dwarf support in the tool may come from more sources
hence wanting in this patch set to be clear what variable is guarding
what. PERF_HAVE_DWARF_REGS is set to 1 for a specific set of
architectures and only when libdw is present. The variable is saying
that libdw supports the notion of registers needed for the #define
HAVE_DWARF_SUPPORT that patch 9 in the series renamed to
HAVE_LIBDW_SUPPORT. So I want the makefile variable
PERF_HAVE_LIBDW_REGS to guard the #define HAVE_LIBDW_SUPPORT, rather
than what is being argued by yourself and Namhyung that the #define
HAVE_LIBDW_SUPPORT be guarded by a variable called
PERF_HAVE_DWARF_REGS and that is only set when NO_LIBDW isn't set.
We've made a digression into the name dwarf for a reason I can't
fathom, at best it is inconsistent. Having dwarf registers is like
having a bright sun or numeric numbers, it is a truism (playing devils
advocate maybe if there were an ELF file format for postscript we
could have a dwarf specification without registers). Anyway, I'm
trying to connect the dots that libdw support controls the libdw type
variables and defines hence not wanting 10 out of 11 patches applied.

Thanks,
Ian

> Thank you,
>
> >
> > Thanks,
> > Ian
> >
> > > Thanks,
> > > Namhyung
> > >
> > > >
> > > > Dwarf in the code base implies libdw, libunwind and potentially other
> > > > dwarf capable things like llvm. If we don't have libdw then NO_LIBDW
> > > > will be set and PERF_HAVE_DWARF_REGS won't be set. That is the more
> > > > general dwarf thing will not be set because of missing libdw. This
> > > > goes contrary to wanting this to be true whenever a dwarf thing is
> > > > present - something that reflecting what the standard says would
> > > > achieve.
> > > >
> > > > In the code base right now PERF_HAVE_DWARF_REGS isn't a dwarf
> > > > dependent thing, it is a libdw dependent thing, this is why
> > > > PERF_HAVE_LIBDW_REGS is a more intention revealing name as it makes
> > > > the connection explicit.
> > > >
> > > > We could change the code so that in Makefile.config we set something like:
> > > > ```
> > > > ...
> > > > ifndef NO_LIBDW
> > > >   PERF_HAVE_DWARF := 1
> > > > ...
> > > > ```
> > > > and in the arch/.../Makefiles change them to be:
> > > > ```
> > > > if PERF_HAVE_DWARF
> > > > PERF_HAVE_DWARF_REGS := 1
> > > > endif
> > > > ```
> > > > but this is going beyond the clean up this patch series was trying to
> > > > achieve. I also don't know of an architecture where dwarf is present
> > > > but registers are not, so having a definition for this case feels
> > > > redundant.
> > > >
> > > > Thanks,
> > > > Ian
> > > >
> > > > > Actually, there is a histrical reason I had called it is DWARF. I used to
> > > > > use "libdwarf", and switched to "libdw" for required features. So I know
> > > > > there are more than 1 implementation of DWARF library, and the libdwarf
> > > > > also uses the same operation number because it depends on the same standard.
> > > > >
> > > > > https://github.com/davea42/libdwarf-code/blob/main/src/lib/libdwarf/dwarf.h#L809
> > > > >
> > > > > So I think we'd better keep it call as DWARF_REGS.
> > > > >
> > > > > Thank you,
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Ian
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>