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.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/Makefile.config | 4 ++--
tools/perf/arch/arm/Makefile | 2 +-
tools/perf/arch/arm64/Makefile | 2 +-
tools/perf/arch/csky/Makefile | 2 +-
tools/perf/arch/loongarch/Makefile | 2 +-
tools/perf/arch/mips/Makefile | 2 +-
tools/perf/arch/powerpc/Makefile | 2 +-
tools/perf/arch/riscv/Makefile | 2 +-
tools/perf/arch/s390/Makefile | 2 +-
tools/perf/arch/sh/Makefile | 2 +-
tools/perf/arch/sparc/Makefile | 2 +-
tools/perf/arch/x86/Makefile | 2 +-
tools/perf/arch/xtensa/Makefile | 2 +-
13 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b14430ab2898..ea9b2b315064 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -552,7 +552,7 @@ ifndef NO_LIBELF
endif
ifndef NO_LIBDW
- ifeq ($(origin PERF_HAVE_DWARF_REGS), undefined)
+ ifeq ($(origin PERF_HAVE_LIBDW_REGS), undefined)
$(warning DWARF register mappings have not been defined for architecture $(SRCARCH), DWARF support disabled)
NO_LIBDW := 1
else
@@ -560,7 +560,7 @@ ifndef NO_LIBELF
LDFLAGS += $(LIBDW_LDFLAGS)
EXTLIBS += ${DWARFLIBS}
$(call detected,CONFIG_LIBDW)
- endif # PERF_HAVE_DWARF_REGS
+ endif # PERF_HAVE_LIBDW_REGS
endif # NO_LIBDW
ifndef NO_LIBBPF
diff --git a/tools/perf/arch/arm/Makefile b/tools/perf/arch/arm/Makefile
index 9b164d379548..5c712fa2347b 100644
--- a/tools/perf/arch/arm/Makefile
+++ b/tools/perf/arch/arm/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
PERF_HAVE_JITDUMP := 1
diff --git a/tools/perf/arch/arm64/Makefile b/tools/perf/arch/arm64/Makefile
index 8a5ffbfe809f..66456aef91bc 100644
--- a/tools/perf/arch/arm64/Makefile
+++ b/tools/perf/arch/arm64/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
PERF_HAVE_JITDUMP := 1
PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
diff --git a/tools/perf/arch/csky/Makefile b/tools/perf/arch/csky/Makefile
index 119b06a64bed..f9e270940919 100644
--- a/tools/perf/arch/csky/Makefile
+++ b/tools/perf/arch/csky/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
diff --git a/tools/perf/arch/loongarch/Makefile b/tools/perf/arch/loongarch/Makefile
index 1cc5eb01f32b..fb0c55bf59f8 100644
--- a/tools/perf/arch/loongarch/Makefile
+++ b/tools/perf/arch/loongarch/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
PERF_HAVE_JITDUMP := 1
diff --git a/tools/perf/arch/mips/Makefile b/tools/perf/arch/mips/Makefile
index 733f7b76f52d..5892bc310127 100644
--- a/tools/perf/arch/mips/Makefile
+++ b/tools/perf/arch/mips/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
# Syscall table generation for perf
diff --git a/tools/perf/arch/powerpc/Makefile b/tools/perf/arch/powerpc/Makefile
index 7672d555f6cd..86911b7505ca 100644
--- a/tools/perf/arch/powerpc/Makefile
+++ b/tools/perf/arch/powerpc/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
HAVE_KVM_STAT_SUPPORT := 1
diff --git a/tools/perf/arch/riscv/Makefile b/tools/perf/arch/riscv/Makefile
index 4664a78a1afd..d97fd00e43f2 100644
--- a/tools/perf/arch/riscv/Makefile
+++ b/tools/perf/arch/riscv/Makefile
@@ -1,5 +1,5 @@
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
PERF_HAVE_JITDUMP := 1
diff --git a/tools/perf/arch/s390/Makefile b/tools/perf/arch/s390/Makefile
index 3f66e2ede3f7..4b83e66c2dfb 100644
--- a/tools/perf/arch/s390/Makefile
+++ b/tools/perf/arch/s390/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
HAVE_KVM_STAT_SUPPORT := 1
PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
diff --git a/tools/perf/arch/sh/Makefile b/tools/perf/arch/sh/Makefile
index 119b06a64bed..f9e270940919 100644
--- a/tools/perf/arch/sh/Makefile
+++ b/tools/perf/arch/sh/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
diff --git a/tools/perf/arch/sparc/Makefile b/tools/perf/arch/sparc/Makefile
index 7741184894c8..39108adc18f1 100644
--- a/tools/perf/arch/sparc/Makefile
+++ b/tools/perf/arch/sparc/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
PERF_HAVE_JITDUMP := 1
diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
index 9aa58acb5564..901d74e85470 100644
--- a/tools/perf/arch/x86/Makefile
+++ b/tools/perf/arch/x86/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
HAVE_KVM_STAT_SUPPORT := 1
PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
diff --git a/tools/perf/arch/xtensa/Makefile b/tools/perf/arch/xtensa/Makefile
index 119b06a64bed..f9e270940919 100644
--- a/tools/perf/arch/xtensa/Makefile
+++ b/tools/perf/arch/xtensa/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
ifndef NO_LIBDW
-PERF_HAVE_DWARF_REGS := 1
+PERF_HAVE_LIBDW_REGS := 1
endif
--
2.46.0.792.g87dc391469-googOn 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. Thanks, Namhyung
On Wed, Sep 25, 2024 at 8:27 PM Namhyung Kim <namhyung@kernel.org> 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? Thanks, Ian
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@kernel.org> 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. Thanks, Namhyung
On Thu, Sep 26, 2024 at 12:40 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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. Thanks, Ian
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@kernel.org> 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@kernel.org> 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. Thanks, Namhyung
On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. Thanks, Ian
On Fri, 27 Sep 2024 11:15:21 -0700 Ian Rogers <irogers@google.com> wrote: > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. 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@kernel.org>
On Sat, Sep 28, 2024 at 7:35 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Fri, 27 Sep 2024 11:15:21 -0700 > Ian Rogers <irogers@google.com> wrote: > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. 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@kernel.org>
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@kernel.org> wrote: > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > Ian Rogers <irogers@google.com> wrote: > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. :) 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@kernel.org>
On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. 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@kernel.org>
On Tue, 1 Oct 2024 16:17:34 -0700 Ian Rogers <irogers@google.com> wrote: > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. 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@kernel.org> > -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Tue, 1 Oct 2024 16:17:34 -0700 > Ian Rogers <irogers@google.com> wrote: > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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@kernel.org> > > > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Tue, 1 Oct 2024 18:31:43 -0700 Ian Rogers <irogers@google.com> wrote: > On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Tue, 1 Oct 2024 16:17:34 -0700 > > Ian Rogers <irogers@google.com> wrote: > > > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. It will be only used with the libdw, but I don't care. "HAVE_DWARF_REG" (internal config, just indicates the arch implemented feature) simply means there is `arch/XXX/util/dwarf-regs.c`. Also the APIs provided by the dwarf-regs.c are still based on DWARF standard, which defines registers by number like DW_OP_reg[0-31]. So the mapping of these suffix number and actual register must be defined for each architecture. That is why I had introduced dwarf-regs.c and call it "dwarf"-regs. Even if the implementation depends on libdw, this dwarf-regs.c is still based on DWARF standard. > 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. Oh, wait, I think we can apply other patches. I just don't like this patch. I think the other patches are good. But this is Thank you, > > 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@kernel.org> > > > > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Wed, Oct 2, 2024 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Tue, 1 Oct 2024 18:31:43 -0700 > Ian Rogers <irogers@google.com> wrote: > > > On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > On Tue, 1 Oct 2024 16:17:34 -0700 > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. > > It will be only used with the libdw, but I don't care. > "HAVE_DWARF_REG" (internal config, just indicates the arch implemented > feature) simply means there is `arch/XXX/util/dwarf-regs.c`. > Also the APIs provided by the dwarf-regs.c are still based on DWARF > standard, which defines registers by number like DW_OP_reg[0-31]. > So the mapping of these suffix number and actual register must be > defined for each architecture. > > That is why I had introduced dwarf-regs.c and call it "dwarf"-regs. > Even if the implementation depends on libdw, this dwarf-regs.c is > still based on DWARF standard. You seem to be missing the point of the series which is to clean up inconsistencies where dwarf is used to mean libdw. Here we have libdw guarding a #define with DWARF in the name, it should have libdw in the name as the patch cleans up. This is a coding thing and not a dwarf specificatin thing. > > 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. > > Oh, wait, I think we can apply other patches. I just don't like this > patch. I think the other patches are good. But this is Then we are intentionally aiming to be inconsistent, with libdw meaning dwarf with a #define that just states a truism. Arguably the code is better with none of the series applied. Thanks, Ian > Thank you, > > > > > 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@kernel.org> > > > > > > > > > > > > > -- > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Wed, Oct 02, 2024 at 07:27:16AM -0700, Ian Rogers wrote: > On Wed, Oct 2, 2024 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Tue, 1 Oct 2024 18:31:43 -0700 > > Ian Rogers <irogers@google.com> wrote: > > > > > On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > On Tue, 1 Oct 2024 16:17:34 -0700 > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. > > > > It will be only used with the libdw, but I don't care. > > "HAVE_DWARF_REG" (internal config, just indicates the arch implemented > > feature) simply means there is `arch/XXX/util/dwarf-regs.c`. > > Also the APIs provided by the dwarf-regs.c are still based on DWARF > > standard, which defines registers by number like DW_OP_reg[0-31]. > > So the mapping of these suffix number and actual register must be > > defined for each architecture. > > > > That is why I had introduced dwarf-regs.c and call it "dwarf"-regs. > > Even if the implementation depends on libdw, this dwarf-regs.c is > > still based on DWARF standard. > > You seem to be missing the point of the series which is to clean up > inconsistencies where dwarf is used to mean libdw. Here we have libdw > guarding a #define with DWARF in the name, it should have libdw in the > name as the patch cleans up. This is a coding thing and not a dwarf > specificatin thing. > > > > 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. > > > > Oh, wait, I think we can apply other patches. I just don't like this > > patch. I think the other patches are good. But this is > > Then we are intentionally aiming to be inconsistent, with libdw > meaning dwarf with a #define that just states a truism. Arguably the > code is better with none of the series applied. 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. Thanks, Namhyung
On Thu, Oct 3, 2024 at 3:48 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Wed, Oct 02, 2024 at 07:27:16AM -0700, Ian Rogers wrote: > > On Wed, Oct 2, 2024 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > On Tue, 1 Oct 2024 18:31:43 -0700 > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > > On Tue, 1 Oct 2024 16:17:34 -0700 > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. > > > > > > It will be only used with the libdw, but I don't care. > > > "HAVE_DWARF_REG" (internal config, just indicates the arch implemented > > > feature) simply means there is `arch/XXX/util/dwarf-regs.c`. > > > Also the APIs provided by the dwarf-regs.c are still based on DWARF > > > standard, which defines registers by number like DW_OP_reg[0-31]. > > > So the mapping of these suffix number and actual register must be > > > defined for each architecture. > > > > > > That is why I had introduced dwarf-regs.c and call it "dwarf"-regs. > > > Even if the implementation depends on libdw, this dwarf-regs.c is > > > still based on DWARF standard. > > > > You seem to be missing the point of the series which is to clean up > > inconsistencies where dwarf is used to mean libdw. Here we have libdw > > guarding a #define with DWARF in the name, it should have libdw in the > > name as the patch cleans up. This is a coding thing and not a dwarf > > specificatin thing. > > > > > > 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. > > > > > > Oh, wait, I think we can apply other patches. I just don't like this > > > patch. I think the other patches are good. But this is > > > > Then we are intentionally aiming to be inconsistent, with libdw > > meaning dwarf with a #define that just states a truism. Arguably the > > code is better with none of the series applied. > > 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 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. 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. 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? Thanks, Ian
On Thu, 3 Oct 2024 17:58:13 -0700 Ian Rogers <irogers@google.com> wrote: > On Thu, Oct 3, 2024 at 3:48 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Wed, Oct 02, 2024 at 07:27:16AM -0700, Ian Rogers wrote: > > > On Wed, Oct 2, 2024 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > On Tue, 1 Oct 2024 18:31:43 -0700 > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > On Tue, Oct 1, 2024 at 4:29 PM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > > > > On Tue, 1 Oct 2024 16:17:34 -0700 > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > On Tue, Oct 1, 2024 at 4:10 PM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 27 Sep 2024 11:15:21 -0700 > > > > > > > > > > Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > > > > > > > > > > > > On Fri, Sep 27, 2024 at 10:16 AM Namhyung Kim <namhyung@kernel.org> 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@kernel.org> 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@kernel.org> 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. > > > > > > > > It will be only used with the libdw, but I don't care. > > > > "HAVE_DWARF_REG" (internal config, just indicates the arch implemented > > > > feature) simply means there is `arch/XXX/util/dwarf-regs.c`. > > > > Also the APIs provided by the dwarf-regs.c are still based on DWARF > > > > standard, which defines registers by number like DW_OP_reg[0-31]. > > > > So the mapping of these suffix number and actual register must be > > > > defined for each architecture. > > > > > > > > That is why I had introduced dwarf-regs.c and call it "dwarf"-regs. > > > > Even if the implementation depends on libdw, this dwarf-regs.c is > > > > still based on DWARF standard. > > > > > > You seem to be missing the point of the series which is to clean up > > > inconsistencies where dwarf is used to mean libdw. Here we have libdw > > > guarding a #define with DWARF in the name, it should have libdw in the > > > name as the patch cleans up. This is a coding thing and not a dwarf > > > specificatin thing. > > > > > > > > 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. > > > > > > > > Oh, wait, I think we can apply other patches. I just don't like this > > > > patch. I think the other patches are good. But this is > > > > > > Then we are intentionally aiming to be inconsistent, with libdw > > > meaning dwarf with a #define that just states a truism. Arguably the > > > code is better with none of the series applied. > > > > 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 > > 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. OK. > > 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. Yeah, I think that is OK to me. > 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? You missed to cut the word, PERF_HAVE_DWARF_REGS means "perf have 'dwarf-regs.c'". If dwarf-regs.c is not there, we can not enable libdw support because it causes linker error. So, if the file name simply changed to libdw-helper.c, I think we can change it. Thank you, > > Thanks, > Ian > -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
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@kernel.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.
>
> 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 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.
ifndef NO_LIBDW
ifeq ($(origin PERF_HAVE_DWARF_REGS), undefined)
$(warning DWARF register mappings have not been defined for architecture $(SRCARCH), DWARF support disabled)
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
On Thu, 3 Oct 2024 22:12:25 -0700
Namhyung Kim <namhyung@kernel.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@kernel.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@kernel.org>
On Fri, Oct 4, 2024 at 7:45 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Thu, 3 Oct 2024 22:12:25 -0700
> Namhyung Kim <namhyung@kernel.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@kernel.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."
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
On Fri, Oct 04, 2024 at 08:15:22AM -0700, Ian Rogers wrote:
> On Fri, Oct 4, 2024 at 7:45 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Thu, 3 Oct 2024 22:12:25 -0700
> > Namhyung Kim <namhyung@kernel.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@kernel.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."
>
> 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.
Sounds good, it'd be easier if we merge patch 1-10 first and you would
work on the register thing separately.
Thanks,
Namhyung
© 2016 - 2024 Red Hat, Inc.