linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] perf tools: Some improvements on data type profiler
@ 2025-08-25 19:54 Zecheng Li
  2025-08-25 19:54 ` [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset Zecheng Li
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

Hi all,

I've identified several missing data type annotations within the perf
tools when annotating the Linux kernel. This patch series improves the
coverage and correctness of data type annotations.

Here's a breakdown of the changes:

Patches 1-3 fix type matching from DWARF. They address cases with
negative offsets (e.g., from intrusive linked lists) and distinguish
DWARF expressions for variable value or address.

Patch 4 skips annotations for LEA instructions in x86, as these do not
involve memory access.

Patches 5-7 implement a basic approach for register offset tracking
based on add, sub, and lea operations. The register is invalidated when
an unsupported arithmetic instruction on that register is encountered.
While this feature has known limitations and may regress in rare cases
compared to the original, it generally improves offset tracking in most
scenarios.

Patch 8 skips check_variable when the type is found directly by
register, since sufficient checking is already performed in
match_var_offset. check_variable lacks some DWARF information to
correctly determine if a variable is valid.

Patch 9 fixes __die_find_scope_cb for namespaces. I found this issue
when trying to annotate a Rust program. The Die for a namespace doesn't
have a PC range, so it would be skipped. Therefore, we should check a
namespace's siblings and children.

Patch 10 implements support for DW_OP_piece. Currently, it is allowed in
check_allowed_ops but is handled like other single location expressions.
We should split any expression containing DW_OP_piece into multiple
parts and handle them separately.

I have tested each patch on a vmlinux and manually checked the results.
After applying all patches, there are less missing or incorrect
annotations. No obvious regressions were observed.

v2:
1. update the match_var_offset function signature to s64
2. correct the comment for is_breg_access_indirect. Use simpler logic to
match the expressions we support.
3. add is_reg_var_addr to indicate whether a register holds an address
of the variable. This defers the type dereference logic to
update_var_state.
4. invalidate register state for unsupported instructions.
5. include two new patches related to improving data type profiler.

v1: https://lore.kernel.org/linux-perf-users/20250725202809.1230085-1-zecheng@google.com/

Zecheng Li (10):
  perf dwarf-aux: Use signed variable types in match_var_offset
  perf dwarf-aux: More accurate variable type match for breg
  perf dwarf-aux: Better variable collection for insn tracking
  perf annotate: Skip annotating data types to lea instructions
  perf dwarf-aux: Find pointer type to a type
  perf annotate: Track arithmetic instructions on pointers
  perf annotate: Invalidate register states for unsupported instructions
  perf dwarf-aux: Skip check_variable for die_find_variable_by_reg
  perf dwarf-aux: fix __die_find_scope_cb for namespaces
  perf dwarf-aux: support DW_OP_piece expressions

 tools/perf/arch/x86/annotate/instructions.c | 155 +++++++-
 tools/perf/util/annotate-data.c             |  35 +-
 tools/perf/util/annotate-data.h             |   6 +
 tools/perf/util/annotate.c                  |  18 +
 tools/perf/util/dwarf-aux.c                 | 372 ++++++++++++++++----
 tools/perf/util/dwarf-aux.h                 |   8 +-
 6 files changed, 519 insertions(+), 75 deletions(-)

-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset
  2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
@ 2025-08-25 19:54 ` Zecheng Li
  2025-08-28  6:52   ` Namhyung Kim
  2025-08-25 19:54 ` [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg Zecheng Li
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

match_var_offset compares address offsets to determine if an access
falls within a variable's bounds. The offsets involved for those
relative to base registers from DW_OP_breg can be negative.

The current implementation uses unsigned types (u64) for these offsets,
which rejects almost all negative values.

Change the signature of match_var_offset to use signed types (s64). This
ensures correct behavior when addr_offset or addr_type are negative.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/dwarf-aux.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 559c953ca172..920054425578 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1388,18 +1388,19 @@ struct find_var_data {
 #define DWARF_OP_DIRECT_REGS  32
 
 static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
-			     u64 addr_offset, u64 addr_type, bool is_pointer)
+			     s64 addr_offset, s64 addr_type, bool is_pointer)
 {
 	Dwarf_Die type_die;
 	Dwarf_Word size;
+	s64 offset = addr_offset - addr_type;
 
-	if (addr_offset == addr_type) {
+	if (offset == 0) {
 		/* Update offset relative to the start of the variable */
 		data->offset = 0;
 		return true;
 	}
 
-	if (addr_offset < addr_type)
+	if (offset < 0)
 		return false;
 
 	if (die_get_real_type(die_mem, &type_die) == NULL)
@@ -1414,11 +1415,11 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
 	if (dwarf_aggregate_size(&type_die, &size) < 0)
 		return false;
 
-	if (addr_offset >= addr_type + size)
+	if ((u64)offset >= size)
 		return false;
 
 	/* Update offset relative to the start of the variable */
-	data->offset = addr_offset - addr_type;
+	data->offset = offset;
 	return true;
 }
 
-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg
  2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
  2025-08-25 19:54 ` [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset Zecheng Li
@ 2025-08-25 19:54 ` Zecheng Li
  2025-08-28  7:18   ` Namhyung Kim
  2025-08-25 19:54 ` [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking Zecheng Li
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

Introduces the function is_breg_access_indirect to determine whether a
memory access involving a DW_OP_breg* operation refers to the variable's
value directly or requires dereferencing the variable's type as a
pointer based on the DWARF expression. Previously, all breg based
accesses were assumed to directly access the variable's value
(is_pointer = false).

The is_breg_access_indirect function handles three cases:

1. Base register + offset only: (e.g., DW_OP_breg7 RSP+88) The
   calculated address is the location of the variable. The access is
   direct, so no type dereference is needed. Returns false.

2. Base register + offset, followed by other operations ending in
   DW_OP_stack_value, including DW_OP_deref: (e.g., DW_OP_breg*,
   DW_OP_deref, DW_OP_stack_value) The DWARF expression computes the
   variable's value, but that value requires a dereference. The memory
   access is fetching that value, so no type dereference is needed.
   Returns false.

3. Base register + offset, followed only by DW_OP_stack_value: (e.g.,
   DW_OP_breg13 R13+256, DW_OP_stack_value) This indicates the value at
   the base + offset is the variable's value. Since this value is being
   used as an address in the memory access, the variable's type is
   treated as a pointer and requires a type dereference. Returns true.

The is_pointer argument passed to match_var_offset is now set by
is_breg_access_indirect for breg accesses.

There are more complex expressions that includes multiple operations and
may require additional handling, such as DW_OP_deref without a
DW_OP_stack_value, or including multiple base registers. They are less
common in the Linux kernel dwarf and are skipped in check_allowed_ops.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/dwarf-aux.c | 38 ++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 920054425578..449bc9ad7aff 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1423,6 +1423,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
 	return true;
 }
 
+/**
+ * is_breg_access_indirect - Check if breg based access implies type
+ * dereference
+ * @ops: DWARF operations array
+ * @nops: Number of operations in @ops
+ *
+ * Returns true if the DWARF expression evaluates to the variable's
+ * value, so the memory access on that register needs type dereference.
+ * Returns false if the expression evaluates to the variable's address.
+ * This is called after check_allowed_ops.
+ */
+static bool is_breg_access_indirect(Dwarf_Op *ops, size_t nops)
+{
+	/* only the base register */
+	if (nops == 1)
+		return false;
+
+	if (nops == 2 && ops[1].atom == DW_OP_stack_value)
+		return true;
+
+	if (nops == 3 && (ops[1].atom == DW_OP_deref ||
+		ops[1].atom == DW_OP_deref_size) &&
+		ops[2].atom == DW_OP_stack_value)
+		return false;
+	/* unreachable, OP not supported */
+	return false;
+}
+
 /* Only checks direct child DIEs in the given scope. */
 static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 {
@@ -1451,7 +1479,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 		if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
 		    check_allowed_ops(ops, nops) &&
 		    match_var_offset(die_mem, data, data->offset, ops->number,
-				     /*is_pointer=*/false))
+				     is_breg_access_indirect(ops, nops)))
 			return DIE_FIND_CB_END;
 
 		/* Only match with a simple case */
@@ -1463,11 +1491,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 					     /*is_pointer=*/true))
 				return DIE_FIND_CB_END;
 
-			/* Local variables accessed by a register + offset */
+			/* variables accessed by a register + offset */
 			if (ops->atom == (DW_OP_breg0 + data->reg) &&
 			    check_allowed_ops(ops, nops) &&
 			    match_var_offset(die_mem, data, data->offset, ops->number,
-					     /*is_pointer=*/false))
+					     is_breg_access_indirect(ops, nops)))
 				return DIE_FIND_CB_END;
 		} else {
 			/* pointer variables saved in a register 32 or above */
@@ -1477,11 +1505,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 					     /*is_pointer=*/true))
 				return DIE_FIND_CB_END;
 
-			/* Local variables accessed by a register + offset */
+			/* variables accessed by a register + offset */
 			if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
 			    check_allowed_ops(ops, nops) &&
 			    match_var_offset(die_mem, data, data->offset, ops->number2,
-					     /*is_poitner=*/false))
+					     is_breg_access_indirect(ops, nops)))
 				return DIE_FIND_CB_END;
 		}
 	}
-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking
  2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
  2025-08-25 19:54 ` [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset Zecheng Li
  2025-08-25 19:54 ` [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg Zecheng Li
@ 2025-08-25 19:54 ` Zecheng Li
  2025-08-30  1:22   ` Namhyung Kim
  2025-08-25 19:54 ` [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions Zecheng Li
  2025-08-25 19:54 ` [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type Zecheng Li
  4 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

Utilizes the previous is_breg_access_indirect function to determine if
the register + offset stores the variable itself or the struct it points
to, save the information in die_var_type.is_reg_var_addr.

Since we are storing the real types in the stack state, we need to do a
type dereference when is_reg_var_addr is set to false for stack/frame
registers.

For other gp registers, skip the variable when the register is a pointer
to the type. If we want to accept these variables, we might also utilize
is_reg_var_addr in a different way, we need to mark that register as a
pointer to the type.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/annotate-data.c |  9 +++++++++
 tools/perf/util/dwarf-aux.c     | 11 ++++++++++-
 tools/perf/util/dwarf-aux.h     |  2 ++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 1ef2edbc71d9..258157cc43c2 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -868,6 +868,11 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
 			int offset = var->offset;
 			struct type_state_stack *stack;
 
+			/* If the reg location holds the pointer value, dereference the type */
+			if (!var->is_reg_var_addr && is_pointer_type(&mem_die) &&
+				__die_get_real_type(&mem_die, &mem_die) == NULL)
+				continue;
+
 			if (var->reg != DWARF_REG_FB)
 				offset -= fb_offset;
 
@@ -893,6 +898,10 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
 
 			reg = &state->regs[var->reg];
 
+			/* For gp registers, skip the address registers for now */
+			if (var->is_reg_var_addr)
+				continue;
+
 			if (reg->ok && reg->kind == TSR_KIND_TYPE &&
 			    !is_better_type(&reg->type, &mem_die))
 				continue;
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 449bc9ad7aff..6fd2db5d9381 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1627,13 +1627,22 @@ static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
 	if (!check_allowed_ops(ops, nops))
 		return DIE_FIND_CB_SIBLING;
 
-	if (die_get_real_type(die_mem, &type_die) == NULL)
+	if (__die_get_real_type(die_mem, &type_die) == NULL)
 		return DIE_FIND_CB_SIBLING;
 
 	vt = malloc(sizeof(*vt));
 	if (vt == NULL)
 		return DIE_FIND_CB_END;
 
+	/* Usually a register holds the value of a variable */
+	vt->is_reg_var_addr = false;
+
+	if (((ops->atom >= DW_OP_breg0 && ops->atom <= DW_OP_breg31) ||
+	      ops->atom == DW_OP_bregx || ops->atom == DW_OP_fbreg) &&
+	      !is_breg_access_indirect(ops, nops))
+		/* The register contains an address of the variable. */
+		vt->is_reg_var_addr = true;
+
 	vt->die_off = dwarf_dieoffset(&type_die);
 	vt->addr = start;
 	vt->reg = reg_from_dwarf_op(ops);
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 892c8c5c23fc..cd481ec9c5a1 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -148,6 +148,8 @@ struct die_var_type {
 	u64 addr;
 	int reg;
 	int offset;
+	/* Whether the register holds a address to the type */
+	bool is_reg_var_addr;
 };
 
 /* Return type info of a member at offset */
-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions
  2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
                   ` (2 preceding siblings ...)
  2025-08-25 19:54 ` [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking Zecheng Li
@ 2025-08-25 19:54 ` Zecheng Li
  2025-08-30  6:41   ` Namhyung Kim
  2025-08-25 19:54 ` [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type Zecheng Li
  4 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

Introduce a helper function is_address_gen_insn() to check
arch-dependent address generation instructions like lea in x86. Remove
type annotation on these instructions since they are not accessing
memory. It should be counted as `no_mem_ops`.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/annotate.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 0dd475a744b6..9d36709d867d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -2621,6 +2621,19 @@ static bool is_stack_canary(struct arch *arch, struct annotated_op_loc *loc)
 	return false;
 }
 
+/**
+ * Returns true if the instruction has a memory operand without
+ * performing a load/store
+ */
+static bool is_address_gen_insn(struct arch *arch, struct disasm_line *dl)
+{
+	if (arch__is(arch, "x86"))
+		if (!strncmp(dl->ins.name, "lea", 3))
+			return true;
+
+	return false;
+}
+
 static struct disasm_line *
 annotation__prev_asm_line(struct annotation *notes, struct disasm_line *curr)
 {
@@ -2729,6 +2742,11 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
 		return &stackop_type;
 	}
 
+	if (is_address_gen_insn(arch, dl)) {
+		istat->bad++;
+		return NULL;
+	}
+
 	for_each_insn_op_loc(&loc, i, op_loc) {
 		struct data_loc_info dloc = {
 			.arch = arch,
-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type
  2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
                   ` (3 preceding siblings ...)
  2025-08-25 19:54 ` [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions Zecheng Li
@ 2025-08-25 19:54 ` Zecheng Li
  2025-08-30  6:48   ` Namhyung Kim
  4 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-25 19:54 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, Liang, Kan, Masami Hiramatsu
  Cc: Xu Liu, linux-perf-users, linux-kernel, Zecheng Li

Implement die_find_pointer_to_type that searches for the pointer type to
a given type in the CU. There's no guarantee that a pointer DIE exists
for every possible base type. Compilers only generate DIEs for types
actually used or defined. Returns NULL if no pointer type is found.

It prepares the support for tracking LEA instructions. When we load the
effective address from a stack position to a register, the register now
holds a pointer type to the type at that stack position.

The performance could be improved by adding a cache for the pointer
types. Currently its impact on the annotation time for vmlinux is low.

Signed-off-by: Zecheng Li <zecheng@google.com>
---
 tools/perf/util/dwarf-aux.c | 69 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/dwarf-aux.h |  4 +++
 2 files changed, 73 insertions(+)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 6fd2db5d9381..6e8877ff2172 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -2117,3 +2117,72 @@ Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset,
 
 	return die_get_member_type(&type_die, offset, die_mem);
 }
+
+struct find_pointer_type_data {
+	/* DIE offset of the type we want to point to */
+	Dwarf_Off target_type_offset;
+	Dwarf_Die *found_die;
+};
+
+static int __die_find_pointer_to_type_cb(Dwarf_Die *die_mem, void *arg)
+{
+	struct find_pointer_type_data *data = arg;
+	Dwarf_Attribute type_attr;
+	Dwarf_Die type_die;
+	Dwarf_Off ref_type_offset;
+
+	if (dwarf_tag(die_mem) != DW_TAG_pointer_type)
+		return DIE_FIND_CB_CONTINUE;
+
+	if (!dwarf_attr(die_mem, DW_AT_type, &type_attr))
+		return DIE_FIND_CB_SIBLING;
+
+	/* Get the DIE this pointer points to */
+	if (!dwarf_formref_die(&type_attr, &type_die))
+		return DIE_FIND_CB_SIBLING;
+
+	ref_type_offset = dwarf_dieoffset(&type_die);
+
+	if (ref_type_offset != 0 && ref_type_offset == data->target_type_offset) {
+		/* This die_mem is a pointer to the target type */
+		if (data->found_die)
+			*data->found_die = *die_mem;
+		return DIE_FIND_CB_END;
+	}
+
+	return DIE_FIND_CB_SIBLING;
+}
+
+/**
+ * die_find_pointer_to_type - Find a DIE for a pointer to a given type
+ * @cu_die: The compilation unit to search within.
+ * @target_type: The DIE of the type you want to find a pointer to.
+ * @result_die: Buffer to store the found DW_TAG_pointer_type DIE.
+ *
+ * Scans the children of the @cu_die for a DW_TAG_pointer_type DIE
+ * whose DW_AT_type attribute references the @target_type.
+ *
+ * Return: @result_die if found, NULL otherwise.
+ */
+Dwarf_Die *die_find_pointer_to_type(Dwarf_Die *cu_die, Dwarf_Die *target_type,
+				   Dwarf_Die *result_die)
+{
+	struct find_pointer_type_data data;
+	Dwarf_Die search_mem;
+
+	if (!cu_die || !target_type || !result_die)
+		return NULL;
+
+	data.target_type_offset = dwarf_dieoffset(target_type);
+	if (data.target_type_offset == 0) {
+		pr_debug("Target type DIE has no offset\n");
+		return NULL;
+	}
+	data.found_die = result_die;
+
+	if (die_find_child(cu_die, __die_find_pointer_to_type_cb, &data, &search_mem))
+		return result_die;
+
+	return NULL;
+}
+
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index cd481ec9c5a1..f20319eb97a9 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -158,6 +158,10 @@ Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset, Dwarf_Die *die_m
 /* Return type info where the pointer and offset point to */
 Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset, Dwarf_Die *die_mem);
 
+/* Find a DIE for a pointer to a given type */
+Dwarf_Die *die_find_pointer_to_type(Dwarf_Die *cu_die, Dwarf_Die *target_type,
+				   Dwarf_Die *result_die);
+
 /* Get byte offset range of given variable DIE */
 int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf);
 
-- 
2.51.0.261.g7ce5a0a67e-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset
  2025-08-25 19:54 ` [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset Zecheng Li
@ 2025-08-28  6:52   ` Namhyung Kim
  2025-09-03 15:49     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2025-08-28  6:52 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Mon, Aug 25, 2025 at 07:54:03PM +0000, Zecheng Li wrote:
> match_var_offset compares address offsets to determine if an access
> falls within a variable's bounds. The offsets involved for those
> relative to base registers from DW_OP_breg can be negative.
> 
> The current implementation uses unsigned types (u64) for these offsets,
> which rejects almost all negative values.

Right, I thought it cannot get negative offsets except for stack access
(e.g. fbreg).  But it turns out that container_of() trick can generate
them with optimizing compilers.

> 
> Change the signature of match_var_offset to use signed types (s64). This
> ensures correct behavior when addr_offset or addr_type are negative.
> 
> Signed-off-by: Zecheng Li <zecheng@google.com>

I've confirmed it produced slightly better results on my test sets.

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/util/dwarf-aux.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 559c953ca172..920054425578 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1388,18 +1388,19 @@ struct find_var_data {
>  #define DWARF_OP_DIRECT_REGS  32
>  
>  static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
> -			     u64 addr_offset, u64 addr_type, bool is_pointer)
> +			     s64 addr_offset, s64 addr_type, bool is_pointer)
>  {
>  	Dwarf_Die type_die;
>  	Dwarf_Word size;
> +	s64 offset = addr_offset - addr_type;
>  
> -	if (addr_offset == addr_type) {
> +	if (offset == 0) {
>  		/* Update offset relative to the start of the variable */
>  		data->offset = 0;
>  		return true;
>  	}
>  
> -	if (addr_offset < addr_type)
> +	if (offset < 0)
>  		return false;
>  
>  	if (die_get_real_type(die_mem, &type_die) == NULL)
> @@ -1414,11 +1415,11 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
>  	if (dwarf_aggregate_size(&type_die, &size) < 0)
>  		return false;
>  
> -	if (addr_offset >= addr_type + size)
> +	if ((u64)offset >= size)
>  		return false;
>  
>  	/* Update offset relative to the start of the variable */
> -	data->offset = addr_offset - addr_type;
> +	data->offset = offset;
>  	return true;
>  }
>  
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg
  2025-08-25 19:54 ` [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg Zecheng Li
@ 2025-08-28  7:18   ` Namhyung Kim
  2025-08-28 18:36     ` Zecheng Li
  0 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2025-08-28  7:18 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Mon, Aug 25, 2025 at 07:54:04PM +0000, Zecheng Li wrote:
> Introduces the function is_breg_access_indirect to determine whether a
> memory access involving a DW_OP_breg* operation refers to the variable's
> value directly or requires dereferencing the variable's type as a
> pointer based on the DWARF expression. Previously, all breg based
> accesses were assumed to directly access the variable's value
> (is_pointer = false).
> 
> The is_breg_access_indirect function handles three cases:
> 
> 1. Base register + offset only: (e.g., DW_OP_breg7 RSP+88) The
>    calculated address is the location of the variable. The access is
>    direct, so no type dereference is needed. Returns false.

I'm afraid there may be cases that the base register doesn't point to
the stack.  In that case it may return true, right?

I think struct find_var_data already has 'is_fbreg' field.  Maybe you
can add 'is_stack' or 'is_stack_reg' field if the target.  Currently we
hardcoded X86_REG_SP but it should be arch-dependent.

> 
> 2. Base register + offset, followed by other operations ending in
>    DW_OP_stack_value, including DW_OP_deref: (e.g., DW_OP_breg*,
>    DW_OP_deref, DW_OP_stack_value) The DWARF expression computes the
>    variable's value, but that value requires a dereference. The memory
>    access is fetching that value, so no type dereference is needed.
>    Returns false.
> 
> 3. Base register + offset, followed only by DW_OP_stack_value: (e.g.,
>    DW_OP_breg13 R13+256, DW_OP_stack_value) This indicates the value at
>    the base + offset is the variable's value. Since this value is being
>    used as an address in the memory access, the variable's type is
>    treated as a pointer and requires a type dereference. Returns true.
> 
> The is_pointer argument passed to match_var_offset is now set by
> is_breg_access_indirect for breg accesses.
> 
> There are more complex expressions that includes multiple operations and
> may require additional handling, such as DW_OP_deref without a
> DW_OP_stack_value, or including multiple base registers. They are less
> common in the Linux kernel dwarf and are skipped in check_allowed_ops.
> 
> Signed-off-by: Zecheng Li <zecheng@google.com>
> ---
>  tools/perf/util/dwarf-aux.c | 38 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 920054425578..449bc9ad7aff 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1423,6 +1423,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
>  	return true;
>  }
>  
> +/**
> + * is_breg_access_indirect - Check if breg based access implies type
> + * dereference
> + * @ops: DWARF operations array
> + * @nops: Number of operations in @ops
> + *
> + * Returns true if the DWARF expression evaluates to the variable's
> + * value, so the memory access on that register needs type dereference.
> + * Returns false if the expression evaluates to the variable's address.
> + * This is called after check_allowed_ops.
> + */
> +static bool is_breg_access_indirect(Dwarf_Op *ops, size_t nops)
> +{
> +	/* only the base register */
> +	if (nops == 1)
> +		return false;

Then it could be like below:

	if (nops == 1) {
		int reg = reg_from_dwarf_op(ops);
		return !(reg == DWARF_REG_FB || data->is_fbreg || reg == data->is_stack);
	}

Thanks,
Namhyung

> +
> +	if (nops == 2 && ops[1].atom == DW_OP_stack_value)
> +		return true;
> +
> +	if (nops == 3 && (ops[1].atom == DW_OP_deref ||
> +		ops[1].atom == DW_OP_deref_size) &&
> +		ops[2].atom == DW_OP_stack_value)
> +		return false;
> +	/* unreachable, OP not supported */
> +	return false;
> +}
> +
>  /* Only checks direct child DIEs in the given scope. */
>  static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  {
> @@ -1451,7 +1479,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  		if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
>  		    check_allowed_ops(ops, nops) &&
>  		    match_var_offset(die_mem, data, data->offset, ops->number,
> -				     /*is_pointer=*/false))
> +				     is_breg_access_indirect(ops, nops)))
>  			return DIE_FIND_CB_END;
>  
>  		/* Only match with a simple case */
> @@ -1463,11 +1491,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  					     /*is_pointer=*/true))
>  				return DIE_FIND_CB_END;
>  
> -			/* Local variables accessed by a register + offset */
> +			/* variables accessed by a register + offset */
>  			if (ops->atom == (DW_OP_breg0 + data->reg) &&
>  			    check_allowed_ops(ops, nops) &&
>  			    match_var_offset(die_mem, data, data->offset, ops->number,
> -					     /*is_pointer=*/false))
> +					     is_breg_access_indirect(ops, nops)))
>  				return DIE_FIND_CB_END;
>  		} else {
>  			/* pointer variables saved in a register 32 or above */
> @@ -1477,11 +1505,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  					     /*is_pointer=*/true))
>  				return DIE_FIND_CB_END;
>  
> -			/* Local variables accessed by a register + offset */
> +			/* variables accessed by a register + offset */
>  			if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
>  			    check_allowed_ops(ops, nops) &&
>  			    match_var_offset(die_mem, data, data->offset, ops->number2,
> -					     /*is_poitner=*/false))
> +					     is_breg_access_indirect(ops, nops)))
>  				return DIE_FIND_CB_END;
>  		}
>  	}
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg
  2025-08-28  7:18   ` Namhyung Kim
@ 2025-08-28 18:36     ` Zecheng Li
  2025-08-30  0:53       ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Zecheng Li @ 2025-08-28 18:36 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Thu, Aug 28, 2025 at 3:19 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Aug 25, 2025 at 07:54:04PM +0000, Zecheng Li wrote:
> > Introduces the function is_breg_access_indirect to determine whether a
> > memory access involving a DW_OP_breg* operation refers to the variable's
> > value directly or requires dereferencing the variable's type as a
> > pointer based on the DWARF expression. Previously, all breg based
> > accesses were assumed to directly access the variable's value
> > (is_pointer = false).
> >
> > The is_breg_access_indirect function handles three cases:
> >
> > 1. Base register + offset only: (e.g., DW_OP_breg7 RSP+88) The
> >    calculated address is the location of the variable. The access is
> >    direct, so no type dereference is needed. Returns false.
>
> I'm afraid there may be cases that the base register doesn't point to
> the stack.  In that case it may return true, right?

Hi Namhyung,

In this case, the DWARF specification for a DW_OP_breg* operation is
to always calculate a memory address. So, even if the base register
isn't the stack pointer, the expression still resolves to the
variable's location, meaning the access is direct (is_pointer =
false).

> I think struct find_var_data already has 'is_fbreg' field.  Maybe you
> can add 'is_stack' or 'is_stack_reg' field if the target.  Currently we
> hardcoded X86_REG_SP but it should be arch-dependent.

Therefore we don't need to check if the register is a stack or frame base.

> >
> > 2. Base register + offset, followed by other operations ending in
> >    DW_OP_stack_value, including DW_OP_deref: (e.g., DW_OP_breg*,
> >    DW_OP_deref, DW_OP_stack_value) The DWARF expression computes the
> >    variable's value, but that value requires a dereference. The memory
> >    access is fetching that value, so no type dereference is needed.
> >    Returns false.
> >
> > 3. Base register + offset, followed only by DW_OP_stack_value: (e.g.,
> >    DW_OP_breg13 R13+256, DW_OP_stack_value) This indicates the value at
> >    the base + offset is the variable's value. Since this value is being
> >    used as an address in the memory access, the variable's type is
> >    treated as a pointer and requires a type dereference. Returns true.
> >
> > The is_pointer argument passed to match_var_offset is now set by
> > is_breg_access_indirect for breg accesses.
> >
> > There are more complex expressions that includes multiple operations and
> > may require additional handling, such as DW_OP_deref without a
> > DW_OP_stack_value, or including multiple base registers. They are less
> > common in the Linux kernel dwarf and are skipped in check_allowed_ops.
> >
> > Signed-off-by: Zecheng Li <zecheng@google.com>
> > ---
> >  tools/perf/util/dwarf-aux.c | 38 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index 920054425578..449bc9ad7aff 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -1423,6 +1423,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
> >       return true;
> >  }
> >
> > +/**
> > + * is_breg_access_indirect - Check if breg based access implies type
> > + * dereference
> > + * @ops: DWARF operations array
> > + * @nops: Number of operations in @ops
> > + *
> > + * Returns true if the DWARF expression evaluates to the variable's
> > + * value, so the memory access on that register needs type dereference.
> > + * Returns false if the expression evaluates to the variable's address.
> > + * This is called after check_allowed_ops.
> > + */
> > +static bool is_breg_access_indirect(Dwarf_Op *ops, size_t nops)
> > +{
> > +     /* only the base register */
> > +     if (nops == 1)
> > +             return false;
>
> Then it could be like below:
>
>         if (nops == 1) {
>                 int reg = reg_from_dwarf_op(ops);
>                 return !(reg == DWARF_REG_FB || data->is_fbreg || reg == data->is_stack);
>         }
>
> Thanks,
> Namhyung
>
> > +
> > +     if (nops == 2 && ops[1].atom == DW_OP_stack_value)
> > +             return true;
> > +
> > +     if (nops == 3 && (ops[1].atom == DW_OP_deref ||
> > +             ops[1].atom == DW_OP_deref_size) &&
> > +             ops[2].atom == DW_OP_stack_value)
> > +             return false;
> > +     /* unreachable, OP not supported */
> > +     return false;
> > +}
> > +
> >  /* Only checks direct child DIEs in the given scope. */
> >  static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> >  {
> > @@ -1451,7 +1479,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> >               if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
> >                   check_allowed_ops(ops, nops) &&
> >                   match_var_offset(die_mem, data, data->offset, ops->number,
> > -                                  /*is_pointer=*/false))
> > +                                  is_breg_access_indirect(ops, nops)))
> >                       return DIE_FIND_CB_END;
> >
> >               /* Only match with a simple case */
> > @@ -1463,11 +1491,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> >                                            /*is_pointer=*/true))
> >                               return DIE_FIND_CB_END;
> >
> > -                     /* Local variables accessed by a register + offset */
> > +                     /* variables accessed by a register + offset */
> >                       if (ops->atom == (DW_OP_breg0 + data->reg) &&
> >                           check_allowed_ops(ops, nops) &&
> >                           match_var_offset(die_mem, data, data->offset, ops->number,
> > -                                          /*is_pointer=*/false))
> > +                                          is_breg_access_indirect(ops, nops)))
> >                               return DIE_FIND_CB_END;
> >               } else {
> >                       /* pointer variables saved in a register 32 or above */
> > @@ -1477,11 +1505,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> >                                            /*is_pointer=*/true))
> >                               return DIE_FIND_CB_END;
> >
> > -                     /* Local variables accessed by a register + offset */
> > +                     /* variables accessed by a register + offset */
> >                       if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
> >                           check_allowed_ops(ops, nops) &&
> >                           match_var_offset(die_mem, data, data->offset, ops->number2,
> > -                                          /*is_poitner=*/false))
> > +                                          is_breg_access_indirect(ops, nops)))
> >                               return DIE_FIND_CB_END;
> >               }
> >       }
> > --
> > 2.51.0.261.g7ce5a0a67e-goog
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg
  2025-08-28 18:36     ` Zecheng Li
@ 2025-08-30  0:53       ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2025-08-30  0:53 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

Hello,

On Thu, Aug 28, 2025 at 02:36:32PM -0400, Zecheng Li wrote:
> On Thu, Aug 28, 2025 at 3:19 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Aug 25, 2025 at 07:54:04PM +0000, Zecheng Li wrote:
> > > Introduces the function is_breg_access_indirect to determine whether a
> > > memory access involving a DW_OP_breg* operation refers to the variable's
> > > value directly or requires dereferencing the variable's type as a
> > > pointer based on the DWARF expression. Previously, all breg based
> > > accesses were assumed to directly access the variable's value
> > > (is_pointer = false).
> > >
> > > The is_breg_access_indirect function handles three cases:
> > >
> > > 1. Base register + offset only: (e.g., DW_OP_breg7 RSP+88) The
> > >    calculated address is the location of the variable. The access is
> > >    direct, so no type dereference is needed. Returns false.
> >
> > I'm afraid there may be cases that the base register doesn't point to
> > the stack.  In that case it may return true, right?
> 
> Hi Namhyung,
> 
> In this case, the DWARF specification for a DW_OP_breg* operation is
> to always calculate a memory address. So, even if the base register
> isn't the stack pointer, the expression still resolves to the
> variable's location, meaning the access is direct (is_pointer =
> false).

I've re-read the DWARF spec and I think you're right. :)

> 
> > I think struct find_var_data already has 'is_fbreg' field.  Maybe you
> > can add 'is_stack' or 'is_stack_reg' field if the target.  Currently we
> > hardcoded X86_REG_SP but it should be arch-dependent.
> 
> Therefore we don't need to check if the register is a stack or frame base.

Fair enough.

> 
> > >
> > > 2. Base register + offset, followed by other operations ending in
> > >    DW_OP_stack_value, including DW_OP_deref: (e.g., DW_OP_breg*,
> > >    DW_OP_deref, DW_OP_stack_value) The DWARF expression computes the
> > >    variable's value, but that value requires a dereference. The memory
> > >    access is fetching that value, so no type dereference is needed.
> > >    Returns false.
> > >
> > > 3. Base register + offset, followed only by DW_OP_stack_value: (e.g.,
> > >    DW_OP_breg13 R13+256, DW_OP_stack_value) This indicates the value at
> > >    the base + offset is the variable's value. Since this value is being
> > >    used as an address in the memory access, the variable's type is
> > >    treated as a pointer and requires a type dereference. Returns true.
> > >
> > > The is_pointer argument passed to match_var_offset is now set by
> > > is_breg_access_indirect for breg accesses.
> > >
> > > There are more complex expressions that includes multiple operations and
> > > may require additional handling, such as DW_OP_deref without a
> > > DW_OP_stack_value, or including multiple base registers. They are less
> > > common in the Linux kernel dwarf and are skipped in check_allowed_ops.
> > >
> > > Signed-off-by: Zecheng Li <zecheng@google.com>

This also improved the data quality!

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> > > ---
> > >  tools/perf/util/dwarf-aux.c | 38 ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 33 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > > index 920054425578..449bc9ad7aff 100644
> > > --- a/tools/perf/util/dwarf-aux.c
> > > +++ b/tools/perf/util/dwarf-aux.c
> > > @@ -1423,6 +1423,34 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
> > >       return true;
> > >  }
> > >
> > > +/**
> > > + * is_breg_access_indirect - Check if breg based access implies type
> > > + * dereference
> > > + * @ops: DWARF operations array
> > > + * @nops: Number of operations in @ops
> > > + *
> > > + * Returns true if the DWARF expression evaluates to the variable's
> > > + * value, so the memory access on that register needs type dereference.
> > > + * Returns false if the expression evaluates to the variable's address.
> > > + * This is called after check_allowed_ops.
> > > + */
> > > +static bool is_breg_access_indirect(Dwarf_Op *ops, size_t nops)
> > > +{
> > > +     /* only the base register */
> > > +     if (nops == 1)
> > > +             return false;
> >
> > Then it could be like below:
> >
> >         if (nops == 1) {
> >                 int reg = reg_from_dwarf_op(ops);
> >                 return !(reg == DWARF_REG_FB || data->is_fbreg || reg == data->is_stack);
> >         }
> >
> > Thanks,
> > Namhyung
> >
> > > +
> > > +     if (nops == 2 && ops[1].atom == DW_OP_stack_value)
> > > +             return true;
> > > +
> > > +     if (nops == 3 && (ops[1].atom == DW_OP_deref ||
> > > +             ops[1].atom == DW_OP_deref_size) &&
> > > +             ops[2].atom == DW_OP_stack_value)
> > > +             return false;
> > > +     /* unreachable, OP not supported */
> > > +     return false;
> > > +}
> > > +
> > >  /* Only checks direct child DIEs in the given scope. */
> > >  static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > >  {
> > > @@ -1451,7 +1479,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > >               if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
> > >                   check_allowed_ops(ops, nops) &&
> > >                   match_var_offset(die_mem, data, data->offset, ops->number,
> > > -                                  /*is_pointer=*/false))
> > > +                                  is_breg_access_indirect(ops, nops)))
> > >                       return DIE_FIND_CB_END;
> > >
> > >               /* Only match with a simple case */
> > > @@ -1463,11 +1491,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > >                                            /*is_pointer=*/true))
> > >                               return DIE_FIND_CB_END;
> > >
> > > -                     /* Local variables accessed by a register + offset */
> > > +                     /* variables accessed by a register + offset */
> > >                       if (ops->atom == (DW_OP_breg0 + data->reg) &&
> > >                           check_allowed_ops(ops, nops) &&
> > >                           match_var_offset(die_mem, data, data->offset, ops->number,
> > > -                                          /*is_pointer=*/false))
> > > +                                          is_breg_access_indirect(ops, nops)))
> > >                               return DIE_FIND_CB_END;
> > >               } else {
> > >                       /* pointer variables saved in a register 32 or above */
> > > @@ -1477,11 +1505,11 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
> > >                                            /*is_pointer=*/true))
> > >                               return DIE_FIND_CB_END;
> > >
> > > -                     /* Local variables accessed by a register + offset */
> > > +                     /* variables accessed by a register + offset */
> > >                       if (ops->atom == DW_OP_bregx && data->reg == ops->number &&
> > >                           check_allowed_ops(ops, nops) &&
> > >                           match_var_offset(die_mem, data, data->offset, ops->number2,
> > > -                                          /*is_poitner=*/false))
> > > +                                          is_breg_access_indirect(ops, nops)))
> > >                               return DIE_FIND_CB_END;
> > >               }
> > >       }
> > > --
> > > 2.51.0.261.g7ce5a0a67e-goog
> > >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking
  2025-08-25 19:54 ` [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking Zecheng Li
@ 2025-08-30  1:22   ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2025-08-30  1:22 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Mon, Aug 25, 2025 at 07:54:05PM +0000, Zecheng Li wrote:
> Utilizes the previous is_breg_access_indirect function to determine if
> the register + offset stores the variable itself or the struct it points
> to, save the information in die_var_type.is_reg_var_addr.
> 
> Since we are storing the real types in the stack state, we need to do a
> type dereference when is_reg_var_addr is set to false for stack/frame
> registers.
> 
> For other gp registers, skip the variable when the register is a pointer
> to the type. If we want to accept these variables, we might also utilize
> is_reg_var_addr in a different way, we need to mark that register as a
> pointer to the type.
> 
> Signed-off-by: Zecheng Li <zecheng@google.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/util/annotate-data.c |  9 +++++++++
>  tools/perf/util/dwarf-aux.c     | 11 ++++++++++-
>  tools/perf/util/dwarf-aux.h     |  2 ++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 1ef2edbc71d9..258157cc43c2 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -868,6 +868,11 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
>  			int offset = var->offset;
>  			struct type_state_stack *stack;
>  
> +			/* If the reg location holds the pointer value, dereference the type */
> +			if (!var->is_reg_var_addr && is_pointer_type(&mem_die) &&
> +				__die_get_real_type(&mem_die, &mem_die) == NULL)
> +				continue;
> +
>  			if (var->reg != DWARF_REG_FB)
>  				offset -= fb_offset;
>  
> @@ -893,6 +898,10 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
>  
>  			reg = &state->regs[var->reg];
>  
> +			/* For gp registers, skip the address registers for now */
> +			if (var->is_reg_var_addr)
> +				continue;
> +
>  			if (reg->ok && reg->kind == TSR_KIND_TYPE &&
>  			    !is_better_type(&reg->type, &mem_die))
>  				continue;
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 449bc9ad7aff..6fd2db5d9381 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1627,13 +1627,22 @@ static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
>  	if (!check_allowed_ops(ops, nops))
>  		return DIE_FIND_CB_SIBLING;
>  
> -	if (die_get_real_type(die_mem, &type_die) == NULL)
> +	if (__die_get_real_type(die_mem, &type_die) == NULL)
>  		return DIE_FIND_CB_SIBLING;
>  
>  	vt = malloc(sizeof(*vt));
>  	if (vt == NULL)
>  		return DIE_FIND_CB_END;
>  
> +	/* Usually a register holds the value of a variable */
> +	vt->is_reg_var_addr = false;
> +
> +	if (((ops->atom >= DW_OP_breg0 && ops->atom <= DW_OP_breg31) ||
> +	      ops->atom == DW_OP_bregx || ops->atom == DW_OP_fbreg) &&
> +	      !is_breg_access_indirect(ops, nops))
> +		/* The register contains an address of the variable. */
> +		vt->is_reg_var_addr = true;
> +
>  	vt->die_off = dwarf_dieoffset(&type_die);
>  	vt->addr = start;
>  	vt->reg = reg_from_dwarf_op(ops);
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index 892c8c5c23fc..cd481ec9c5a1 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -148,6 +148,8 @@ struct die_var_type {
>  	u64 addr;
>  	int reg;
>  	int offset;
> +	/* Whether the register holds a address to the type */
> +	bool is_reg_var_addr;
>  };
>  
>  /* Return type info of a member at offset */
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions
  2025-08-25 19:54 ` [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions Zecheng Li
@ 2025-08-30  6:41   ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2025-08-30  6:41 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Mon, Aug 25, 2025 at 07:54:06PM +0000, Zecheng Li wrote:
> Introduce a helper function is_address_gen_insn() to check
> arch-dependent address generation instructions like lea in x86. Remove
> type annotation on these instructions since they are not accessing
> memory. It should be counted as `no_mem_ops`.
> 
> Signed-off-by: Zecheng Li <zecheng@google.com>
> ---
>  tools/perf/util/annotate.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 0dd475a744b6..9d36709d867d 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -2621,6 +2621,19 @@ static bool is_stack_canary(struct arch *arch, struct annotated_op_loc *loc)
>  	return false;
>  }
>  
> +/**
> + * Returns true if the instruction has a memory operand without
> + * performing a load/store
> + */
> +static bool is_address_gen_insn(struct arch *arch, struct disasm_line *dl)
> +{
> +	if (arch__is(arch, "x86"))
> +		if (!strncmp(dl->ins.name, "lea", 3))
> +			return true;
> +
> +	return false;
> +}
> +
>  static struct disasm_line *
>  annotation__prev_asm_line(struct annotation *notes, struct disasm_line *curr)
>  {
> @@ -2729,6 +2742,11 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
>  		return &stackop_type;
>  	}
>  
> +	if (is_address_gen_insn(arch, dl)) {
> +		istat->bad++;
> +		return NULL;

Do you return NULL because you want to handle it with no_mem_ops case?
I suggest you add it here and return NO_TYPE.  There are places to call
__hist_entry__get_data_type() directly and I'd like to update the stat
in this function as much as possible.

Thanks,
Namhyung

> +	}
> +
>  	for_each_insn_op_loc(&loc, i, op_loc) {
>  		struct data_loc_info dloc = {
>  			.arch = arch,
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type
  2025-08-25 19:54 ` [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type Zecheng Li
@ 2025-08-30  6:48   ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2025-08-30  6:48 UTC (permalink / raw)
  To: Zecheng Li
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Liang, Kan, Masami Hiramatsu, Xu Liu,
	linux-perf-users, linux-kernel

On Mon, Aug 25, 2025 at 07:54:07PM +0000, Zecheng Li wrote:
> Implement die_find_pointer_to_type that searches for the pointer type to
> a given type in the CU. There's no guarantee that a pointer DIE exists
> for every possible base type. Compilers only generate DIEs for types
> actually used or defined. Returns NULL if no pointer type is found.
> 
> It prepares the support for tracking LEA instructions. When we load the
> effective address from a stack position to a register, the register now
> holds a pointer type to the type at that stack position.
> 
> The performance could be improved by adding a cache for the pointer
> types. Currently its impact on the annotation time for vmlinux is low.

I'm afraid the cache hit ratio can be low, but I have no data.  Anyway
can you please share the difference of annotation time before and after
this patchset?  It'd be nice if you have numbers for a large perf.data.

Thanks,
Namhyung

> 
> Signed-off-by: Zecheng Li <zecheng@google.com>
> ---
>  tools/perf/util/dwarf-aux.c | 69 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/dwarf-aux.h |  4 +++
>  2 files changed, 73 insertions(+)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 6fd2db5d9381..6e8877ff2172 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -2117,3 +2117,72 @@ Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset,
>  
>  	return die_get_member_type(&type_die, offset, die_mem);
>  }
> +
> +struct find_pointer_type_data {
> +	/* DIE offset of the type we want to point to */
> +	Dwarf_Off target_type_offset;
> +	Dwarf_Die *found_die;
> +};
> +
> +static int __die_find_pointer_to_type_cb(Dwarf_Die *die_mem, void *arg)
> +{
> +	struct find_pointer_type_data *data = arg;
> +	Dwarf_Attribute type_attr;
> +	Dwarf_Die type_die;
> +	Dwarf_Off ref_type_offset;
> +
> +	if (dwarf_tag(die_mem) != DW_TAG_pointer_type)
> +		return DIE_FIND_CB_CONTINUE;
> +
> +	if (!dwarf_attr(die_mem, DW_AT_type, &type_attr))
> +		return DIE_FIND_CB_SIBLING;
> +
> +	/* Get the DIE this pointer points to */
> +	if (!dwarf_formref_die(&type_attr, &type_die))
> +		return DIE_FIND_CB_SIBLING;
> +
> +	ref_type_offset = dwarf_dieoffset(&type_die);
> +
> +	if (ref_type_offset != 0 && ref_type_offset == data->target_type_offset) {
> +		/* This die_mem is a pointer to the target type */
> +		if (data->found_die)
> +			*data->found_die = *die_mem;
> +		return DIE_FIND_CB_END;
> +	}
> +
> +	return DIE_FIND_CB_SIBLING;
> +}
> +
> +/**
> + * die_find_pointer_to_type - Find a DIE for a pointer to a given type
> + * @cu_die: The compilation unit to search within.
> + * @target_type: The DIE of the type you want to find a pointer to.
> + * @result_die: Buffer to store the found DW_TAG_pointer_type DIE.
> + *
> + * Scans the children of the @cu_die for a DW_TAG_pointer_type DIE
> + * whose DW_AT_type attribute references the @target_type.
> + *
> + * Return: @result_die if found, NULL otherwise.
> + */
> +Dwarf_Die *die_find_pointer_to_type(Dwarf_Die *cu_die, Dwarf_Die *target_type,
> +				   Dwarf_Die *result_die)
> +{
> +	struct find_pointer_type_data data;
> +	Dwarf_Die search_mem;
> +
> +	if (!cu_die || !target_type || !result_die)
> +		return NULL;
> +
> +	data.target_type_offset = dwarf_dieoffset(target_type);
> +	if (data.target_type_offset == 0) {
> +		pr_debug("Target type DIE has no offset\n");
> +		return NULL;
> +	}
> +	data.found_die = result_die;
> +
> +	if (die_find_child(cu_die, __die_find_pointer_to_type_cb, &data, &search_mem))
> +		return result_die;
> +
> +	return NULL;
> +}
> +
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index cd481ec9c5a1..f20319eb97a9 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -158,6 +158,10 @@ Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset, Dwarf_Die *die_m
>  /* Return type info where the pointer and offset point to */
>  Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset, Dwarf_Die *die_mem);
>  
> +/* Find a DIE for a pointer to a given type */
> +Dwarf_Die *die_find_pointer_to_type(Dwarf_Die *cu_die, Dwarf_Die *target_type,
> +				   Dwarf_Die *result_die);
> +
>  /* Get byte offset range of given variable DIE */
>  int die_get_var_range(Dwarf_Die *sp_die, Dwarf_Die *vr_die, struct strbuf *buf);
>  
> -- 
> 2.51.0.261.g7ce5a0a67e-goog
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset
  2025-08-28  6:52   ` Namhyung Kim
@ 2025-09-03 15:49     ` Arnaldo Carvalho de Melo
  2025-09-03 22:05       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-09-03 15:49 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Zecheng Li, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	Liang, Kan, Masami Hiramatsu, Xu Liu, linux-perf-users,
	linux-kernel

On Wed, Aug 27, 2025 at 11:52:02PM -0700, Namhyung Kim wrote:
> On Mon, Aug 25, 2025 at 07:54:03PM +0000, Zecheng Li wrote:
> > match_var_offset compares address offsets to determine if an access
> > falls within a variable's bounds. The offsets involved for those
> > relative to base registers from DW_OP_breg can be negative.

> > The current implementation uses unsigned types (u64) for these offsets,
> > which rejects almost all negative values.
 
> Right, I thought it cannot get negative offsets except for stack access
> (e.g. fbreg).  But it turns out that container_of() trick can generate
> them with optimizing compilers.
 
> > Change the signature of match_var_offset to use signed types (s64). This
> > ensures correct behavior when addr_offset or addr_type are negative.

> > Signed-off-by: Zecheng Li <zecheng@google.com>
 
> I've confirmed it produced slightly better results on my test sets.
 
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Cherry picked this first patch to make a bit of progress in the
perf-tools-next front.

- Arnaldo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset
  2025-09-03 15:49     ` Arnaldo Carvalho de Melo
@ 2025-09-03 22:05       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-09-03 22:05 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Zecheng Li, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	Liang, Kan, Masami Hiramatsu, Xu Liu, linux-perf-users,
	linux-kernel

On Wed, Sep 03, 2025 at 12:49:49PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Aug 27, 2025 at 11:52:02PM -0700, Namhyung Kim wrote:
> > On Mon, Aug 25, 2025 at 07:54:03PM +0000, Zecheng Li wrote:
> > > match_var_offset compares address offsets to determine if an access
> > > falls within a variable's bounds. The offsets involved for those
> > > relative to base registers from DW_OP_breg can be negative.
 
> > > The current implementation uses unsigned types (u64) for these offsets,
> > > which rejects almost all negative values.
  
> > Right, I thought it cannot get negative offsets except for stack access
> > (e.g. fbreg).  But it turns out that container_of() trick can generate
> > them with optimizing compilers.
  
> > > Change the signature of match_var_offset to use signed types (s64). This
> > > ensures correct behavior when addr_offset or addr_type are negative.
 
> > > Signed-off-by: Zecheng Li <zecheng@google.com>
  
> > I've confirmed it produced slightly better results on my test sets.
  
> > Reviewed-by: Namhyung Kim <namhyung@kernel.org>
 
> Cherry picked this first patch to make a bit of progress in the
> perf-tools-next front.

It is in perf-tools-next/perf-tools-next now (this first reviewed one):

https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next

- Arnaldo

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-09-03 22:05 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-25 19:54 [PATCH v2 00/10] perf tools: Some improvements on data type profiler Zecheng Li
2025-08-25 19:54 ` [PATCH v2 01/10] perf dwarf-aux: Use signed variable types in match_var_offset Zecheng Li
2025-08-28  6:52   ` Namhyung Kim
2025-09-03 15:49     ` Arnaldo Carvalho de Melo
2025-09-03 22:05       ` Arnaldo Carvalho de Melo
2025-08-25 19:54 ` [PATCH v2 02/10] perf dwarf-aux: More accurate variable type match for breg Zecheng Li
2025-08-28  7:18   ` Namhyung Kim
2025-08-28 18:36     ` Zecheng Li
2025-08-30  0:53       ` Namhyung Kim
2025-08-25 19:54 ` [PATCH v2 03/10] perf dwarf-aux: Better variable collection for insn tracking Zecheng Li
2025-08-30  1:22   ` Namhyung Kim
2025-08-25 19:54 ` [PATCH v2 04/10] perf annotate: Skip annotating data types to lea instructions Zecheng Li
2025-08-30  6:41   ` Namhyung Kim
2025-08-25 19:54 ` [PATCH v2 05/10] perf dwarf-aux: Find pointer type to a type Zecheng Li
2025-08-30  6:48   ` Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).