[PATCH 0/4] perf annotate-data: A couple of small updates

[PATCH 0/4] perf annotate-data: A couple of small updates

[Namhyung Kim]

Hello,

I am working on improving quality of the data type profiling and I
found some issues.  One is when more than one variables are placed at
the same location.  Then it should find the correct one based on the
given info rather than checking the first one and bailing out.  This
one (patch 2) can go to the perf-tools tree.

Another issue is use of the stack pointe register when it's not the
frame base register.  I found a case where rbp is used as the frame
base but rsp is also used to point some stack variables.  And it
confuses itself how to interpret the type of the variable.

I think these are rare cases but it would depends on the code pattern
and compiler behavior.  Anyway I can see a tiny improvement in my data
with this change. :)

Thanks,
Namhyung


Namhyung Kim (4):
  perf annotate-data: Improve debug message with location info
  perf dwarf-aux: Check pointer offset when checking variables
  perf dwarf-aux: Check variable address range properly
  perf annotate-data: Handle RSP if it's not the FB register

 tools/perf/util/annotate-data.c | 126 +++++++++++++++++++++++++-------
 tools/perf/util/dwarf-aux.c     |  35 ++++++---
 2 files changed, 125 insertions(+), 36 deletions(-)


base-commit: 0ffc8fca5c15a70f32c8aff12c566bbd3991bd0a
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 1/4] perf annotate-data: Improve debug message with location info

[Namhyung Kim]

To verify it found the correct variable, let's add the location
expression to the debug message.

  $ perf --debug type-profile annotate --data-type
  ...
  -----------------------------------------------------------
  find data type for 0xaf0(reg15) at schedule+0xeb
  CU for kernel/sched/core.c (die:0x1180523)
  frame base: cfa=0 fbreg=6
  found "rq" in scope=3/4 (die: 0x11b6a00) type_offset=0xaf0
   variable location: reg15
   type='struct rq' size=0xfc0 (die:0x11892e2)
  -----------------------------------------------------------
  find data type for 0x7bc(reg3) at tcp_get_info+0x62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  offset: 1980 is bigger than size: 760
  check variable "sk" failed (die: 0x7b92b2c)
   variable location: reg3
   type='struct sock' size=0x2f8 (die:0x7b63c3a)
  -----------------------------------------------------------
  ...

The first case is fine.  It looked up a data type in r15 with offset of
0xaf0 at the schedule+0xeb.  It found CU die and the frame base info and
the variable "rq" was found in the scope 3/4.  Its location is the r15
register and the type size is 0xfc0 which includes 0xaf0.

But the second case is not good.  It looked up a data type in rbx (reg3)
with offset 0x7bc.  It found a CU and the frame base which is good so
far.  And it also found a variable "sk" but the access offset is bigger
than the type size (1980 vs. 760 or 0x7bc vs. 0x2f8).  The variable has
the right location (reg3) but I need to figure out why it accesses
beyond what it's supposed to.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 99 +++++++++++++++++++++++++++------
 1 file changed, 82 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 1cd857400038..c6eb5b2cc4d5 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -46,6 +46,7 @@ static void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
 {
 	struct strbuf sb;
 	char *str;
+	Dwarf_Word size = 0;
 
 	if (!debug_type_profile && verbose < 3)
 		return;
@@ -72,13 +73,67 @@ static void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
 		break;
 	}
 
+	dwarf_aggregate_size(die, &size);
+
 	strbuf_init(&sb, 32);
 	die_get_typename_from_type(die, &sb);
 	str = strbuf_detach(&sb, NULL);
-	pr_info(" type=%s (die:%lx)\n", str, (long)dwarf_dieoffset(die));
+	pr_info(" type='%s' size=%#lx (die:%#lx)\n",
+		str, (long)size, (long)dwarf_dieoffset(die));
 	free(str);
 }
 
+static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
+{
+	ptrdiff_t off = 0;
+	Dwarf_Attribute attr;
+	Dwarf_Addr base, start, end;
+	Dwarf_Op *ops;
+	size_t nops;
+
+	if (!debug_type_profile && verbose < 3)
+		return;
+
+	if (dwarf_attr(die, DW_AT_location, &attr) == NULL)
+		return;
+
+	while ((off = dwarf_getlocations(&attr, off, &base, &start, &end, &ops, &nops)) > 0) {
+		if (reg != DWARF_REG_PC && end < pc)
+			continue;
+		if (reg != DWARF_REG_PC && start > pc)
+			break;
+
+		pr_info(" variable location: ");
+		switch (ops->atom) {
+		case DW_OP_reg0 ...DW_OP_reg31:
+			pr_info("reg%d\n", ops->atom - DW_OP_reg0);
+			break;
+		case DW_OP_breg0 ...DW_OP_breg31:
+			pr_info("base=reg%d, offset=%#lx\n",
+				ops->atom - DW_OP_breg0, ops->number);
+			break;
+		case DW_OP_regx:
+			pr_info("reg%ld\n", ops->number);
+			break;
+		case DW_OP_bregx:
+			pr_info("base=reg%ld, offset=%#lx\n",
+				ops->number, ops->number2);
+			break;
+		case DW_OP_fbreg:
+			pr_info("use frame base, offset=%#lx\n", ops->number);
+			break;
+		case DW_OP_addr:
+			pr_info("address=%#lx\n", ops->number);
+			break;
+		default:
+			pr_info("unknown: code=%#x, number=%#lx\n",
+				ops->atom, ops->number);
+			break;
+		}
+		break;
+	}
+}
+
 /*
  * Type information in a register, valid when @ok is true.
  * The @caller_saved registers are invalidated after a function call.
@@ -1404,7 +1459,7 @@ static int find_data_type_block(struct data_loc_info *dloc, int reg,
 		found = find_data_type_insn(dloc, reg, &basic_blocks, var_types,
 					    cu_die, type_die);
 		if (found > 0) {
-			pr_debug_dtp("found by insn track: %#x(reg%d) type-offset=%#x",
+			pr_debug_dtp("found by insn track: %#x(reg%d) type-offset=%#x\n",
 				     dloc->op->offset, reg, dloc->type_offset);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 			ret = 0;
@@ -1440,16 +1495,16 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	char buf[64];
 
 	if (dloc->op->multi_regs)
-		snprintf(buf, sizeof(buf), " or reg%d", dloc->op->reg2);
+		snprintf(buf, sizeof(buf), "reg%d, reg%d", dloc->op->reg1, dloc->op->reg2);
 	else if (dloc->op->reg1 == DWARF_REG_PC)
-		snprintf(buf, sizeof(buf), " (PC)");
+		snprintf(buf, sizeof(buf), "PC");
 	else
-		buf[0] = '\0';
+		snprintf(buf, sizeof(buf), "reg%d", dloc->op->reg1);
 
 	pr_debug_dtp("-----------------------------------------------------------\n");
-	pr_debug_dtp("%s [%"PRIx64"] for reg%d%s offset=%#x in %s\n",
-		     __func__, dloc->ip - dloc->ms->sym->start,
-		     dloc->op->reg1, buf, dloc->op->offset, dloc->ms->sym->name);
+	pr_debug_dtp("find data type for %#x(%s) at %s+%#"PRIx64"\n",
+		     dloc->op->offset, buf, dloc->ms->sym->name,
+		     dloc->ip - dloc->ms->sym->start);
 
 	/*
 	 * IP is a relative instruction address from the start of the map, as
@@ -1468,14 +1523,15 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	reg = loc->reg1;
 	offset = loc->offset;
 
-	pr_debug_dtp("CU die offset: %#lx\n", (long)dwarf_dieoffset(&cu_die));
+	pr_debug_dtp("CU for %s (die:%#lx)\n",
+		     dwarf_diename(&cu_die), (long)dwarf_dieoffset(&cu_die));
 
 	if (reg == DWARF_REG_PC) {
 		if (get_global_var_type(&cu_die, dloc, dloc->ip, dloc->var_addr,
 					&offset, type_die)) {
 			dloc->type_offset = offset;
 
-			pr_debug_dtp("found PC-rel by addr=%#"PRIx64" offset=%#x",
+			pr_debug_dtp("found by addr=%#"PRIx64" type_offset=%#x\n",
 				     dloc->var_addr, offset);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 			ret = 0;
@@ -1537,13 +1593,22 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			pr_debug_dtp("found \"%s\" in scope=%d/%d (die: %#lx) ",
 				     dwarf_diename(&var_die), i+1, nr_scopes,
 				     (long)dwarf_dieoffset(&scopes[i]));
-			if (reg == DWARF_REG_PC)
-				pr_debug_dtp("%#x(PC) offset=%#x", loc->offset, offset);
-			else if (reg == DWARF_REG_FB || is_fbreg)
-				pr_debug_dtp("%#x(reg%d) stack fb_offset=%#x offset=%#x",
-					     loc->offset, reg, fb_offset, offset);
-			else
-				pr_debug_dtp("%#x(reg%d)", loc->offset, reg);
+			if (reg == DWARF_REG_PC) {
+				pr_debug_dtp("addr=%#"PRIx64" type_offset=%#x\n",
+					     dloc->var_addr, offset);
+			} else if (reg == DWARF_REG_FB || is_fbreg) {
+				pr_debug_dtp("stack_offset=%#x type_offset=%#x\n",
+					     fb_offset, offset);
+			} else {
+				pr_debug_dtp("type_offset=%#x\n", offset);
+			}
+			pr_debug_location(&var_die, pc, reg);
+			pr_debug_type_name(type_die, TSR_KIND_TYPE);
+		} else {
+			pr_debug_dtp("check variable \"%s\" failed (die: %#lx)\n",
+				     dwarf_diename(&var_die),
+				     (long)dwarf_dieoffset(&var_die));
+			pr_debug_location(&var_die, pc, reg);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 		}
 		dloc->type_offset = offset;
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH 1/4] perf annotate-data: Improve debug message with location info

[Arnaldo Carvalho de Melo]

On Fri, Apr 12, 2024 at 11:33:07AM -0700, Namhyung Kim wrote:
> To verify it found the correct variable, let's add the location
> expression to the debug message.

Added the patch below, following your existing convention of casting
Dwarf_Offset/Darf_Word to long.

- Arnaldo

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index c6eb5b2cc4d50d79..e53d66c46c540b75 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -110,24 +110,24 @@ static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
 			break;
 		case DW_OP_breg0 ...DW_OP_breg31:
 			pr_info("base=reg%d, offset=%#lx\n",
-				ops->atom - DW_OP_breg0, ops->number);
+				ops->atom - DW_OP_breg0, (long)ops->number);
 			break;
 		case DW_OP_regx:
-			pr_info("reg%ld\n", ops->number);
+			pr_info("reg%ld\n", (long)ops->number);
 			break;
 		case DW_OP_bregx:
 			pr_info("base=reg%ld, offset=%#lx\n",
-				ops->number, ops->number2);
+				(long)ops->number, (long)ops->number2);
 			break;
 		case DW_OP_fbreg:
-			pr_info("use frame base, offset=%#lx\n", ops->number);
+			pr_info("use frame base, offset=%#lx\n", (long)ops->number);
 			break;
 		case DW_OP_addr:
-			pr_info("address=%#lx\n", ops->number);
+			pr_info("address=%#lx\n", (long)ops->number);
 			break;
 		default:
 			pr_info("unknown: code=%#x, number=%#lx\n",
-				ops->atom, ops->number);
+				ops->atom, (long)ops->number);
 			break;
 		}
 		break;

[PATCH 2/4] perf dwarf-aux: Check pointer offset when checking variables

[Namhyung Kim]

In match_var_offset(), it checks the offset range with the target type
only for non-pointer types.  But it also needs to check the pointer
types with the target type.

This is because there can be more than one pointer variables located in
the same register.  Let's look at the following example.  It's looking
up a variable for reg3 at tcp_get_info+0x62.  It found "sk" variable but
it wasn't the right one since it accesses beyond the target type (struct
'sock' in this case) size.

  -----------------------------------------------------------
  find data type for 0x7bc(reg3) at tcp_get_info+0x62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  offset: 1980 is bigger than size: 760
  check variable "sk" failed (die: 0x7b92b2c)
   variable location: reg3
   type='struct sock' size=0x2f8 (die:0x7b63c3a)

Actually there was another variable "tp" in the function and it's
located at the same (reg3) because it's just type-casted like below.

  void tcp_get_info(struct sock *sk, struct tcp_info *info)
  {
      const struct tcp_sock *tp = tcp_sk(sk);
      ...

The struct tcp_sock contains the struct sock at offset 0 so it can
just use the same address as a pointer to tcp_sock.  That means it
should match variables correctly by checking the offset and size.
Actually it cannot distinguish if the offset was smaller than the size
of the original struct sock.  But I think it's fine as they are the
same at that part.

So let's check the target type size and retry if it doesn't match.
Now it succeeded to find the correct variable.

  -----------------------------------------------------------
  find data type for 0x7bc(reg3) at tcp_get_info+0x62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  found "tp" in scope=1/1 (die: 0x7b92b16) type_offset=0x7bc
   variable location: reg3
   type='struct tcp_sock' size=0xa68 (die:0x7b81380)

Fixes: bc10db8eb895 ("perf annotate-data: Support stack variables")
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 7dad99ee3ff3..b361fd7ebd56 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1361,7 +1361,7 @@ 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)
+			     u64 addr_offset, u64 addr_type, bool is_pointer)
 {
 	Dwarf_Die type_die;
 	Dwarf_Word size;
@@ -1375,6 +1375,12 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
 	if (die_get_real_type(die_mem, &type_die) == NULL)
 		return false;
 
+	if (is_pointer && dwarf_tag(&type_die) == DW_TAG_pointer_type) {
+		/* Get the target type of the pointer */
+		if (die_get_real_type(&type_die, &type_die) == NULL)
+			return false;
+	}
+
 	if (dwarf_aggregate_size(&type_die, &size) < 0)
 		return false;
 
@@ -1442,31 +1448,38 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 		if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
 		    data->offset >= (int)ops->number &&
 		    check_allowed_ops(ops, nops) &&
-		    match_var_offset(die_mem, data, data->offset, ops->number))
+		    match_var_offset(die_mem, data, data->offset, ops->number,
+				     /*is_pointer=*/false))
 			return DIE_FIND_CB_END;
 
 		/* Only match with a simple case */
 		if (data->reg < DWARF_OP_DIRECT_REGS) {
 			/* pointer variables saved in a register 0 to 31 */
 			if (ops->atom == (DW_OP_reg0 + data->reg) &&
-			    check_allowed_ops(ops, nops))
+			    check_allowed_ops(ops, nops) &&
+			    match_var_offset(die_mem, data, data->offset, 0,
+					     /*is_pointer=*/true))
 				return DIE_FIND_CB_END;
 
 			/* Local 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))
+			    match_var_offset(die_mem, data, data->offset, ops->number,
+					     /*is_pointer=*/false))
 				return DIE_FIND_CB_END;
 		} else {
 			/* pointer variables saved in a register 32 or above */
 			if (ops->atom == DW_OP_regx && ops->number == data->reg &&
-			    check_allowed_ops(ops, nops))
+			    check_allowed_ops(ops, nops) &&
+			    match_var_offset(die_mem, data, data->offset, 0,
+					     /*is_pointer=*/true))
 				return DIE_FIND_CB_END;
 
 			/* Local 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))
+			    match_var_offset(die_mem, data, data->offset, ops->number2,
+					     /*is_poitner=*/false))
 				return DIE_FIND_CB_END;
 		}
 	}
@@ -1528,7 +1541,8 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
 			continue;
 
 		if (check_allowed_ops(ops, nops) &&
-		    match_var_offset(die_mem, data, data->addr, ops->number))
+		    match_var_offset(die_mem, data, data->addr, ops->number,
+				     /*is_pointer=*/false))
 			return DIE_FIND_CB_END;
 	}
 	return DIE_FIND_CB_SIBLING;
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 3/4] perf dwarf-aux: Check variable address range properly

[Namhyung Kim]

In match_var_offset(), it just checked the end address of the variable
with the given offset because it assumed the register holds a pointer
to the data type and the offset starts from the base.

But I found some cases that the stack pointer (rsp = reg7) register is
used to pointer a stack variable while the frame base is maintained by a
different register (rbp = reg6).  In that case, it cannot simply use the
stack pointer as it cannot guarantee that it points to the frame base.
So it needs to check the both boundaries of the variable location.

Before:
  -----------------------------------------------------------
  find data type for 0x7c(reg7) at tcp_getsockopt+0xb62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  no pointer or no type
  check variable "tss" failed (die: 0x7b95801)
   variable location: base reg7, offset=0x110
   type='struct scm_timestamping_internal' size=0x30 (die:0x7b8c126)

So the current code just checks register number for the non-PC and
non-FB registers and assuming it has offset 0.  But this variable has
offset 0x110 so it should not match to this.

After:
  -----------------------------------------------------------
  find data type for 0x7c(reg7) at tcp_getsockopt+0xb62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  no pointer or no type
  check variable "zc" failed (die: 0x7b9580a)
   variable location: base=reg7, offset=0x40
   type='struct tcp_zerocopy_receive' size=0x40 (die:7b947f4)

Now it find the correct variable "zc".  It was located at reg7 + 0x40
and the size if 0x40 which means it should cover [0x40, 0x80).  And the
access was for reg7 + 0x7c so it found the right one.  But it still
failed to use the variable and it would be handled in the next patch.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index b361fd7ebd56..40cfbdfe2d75 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1372,6 +1372,9 @@ static bool match_var_offset(Dwarf_Die *die_mem, struct find_var_data *data,
 		return true;
 	}
 
+	if (addr_offset < addr_type)
+		return false;
+
 	if (die_get_real_type(die_mem, &type_die) == NULL)
 		return false;
 
@@ -1446,7 +1449,6 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 
 		/* Local variables accessed using frame base register */
 		if (data->is_fbreg && ops->atom == DW_OP_fbreg &&
-		    data->offset >= (int)ops->number &&
 		    check_allowed_ops(ops, nops) &&
 		    match_var_offset(die_mem, data, data->offset, ops->number,
 				     /*is_pointer=*/false))
@@ -1537,9 +1539,6 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
 		if (ops->atom != DW_OP_addr)
 			continue;
 
-		if (data->addr < ops->number)
-			continue;
-
 		if (check_allowed_ops(ops, nops) &&
 		    match_var_offset(die_mem, data, data->addr, ops->number,
 				     /*is_pointer=*/false))
-- 
2.44.0.683.g7961c838ac-goog

[PATCH 4/4] perf annotate-data: Handle RSP if it's not the FB register

[Namhyung Kim]

In some cases, the stack pointer on x86 (rsp = reg7) is used to point
variables on stack but it's not the frame base register.  Then it
should handle the register like normal registers (IOW not to access
the other stack variables using offset calculation) but it should not
assume it would have a pointer.

Before:
  -----------------------------------------------------------
  find data type for 0x7c(reg7) at tcp_getsockopt+0xb62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  no pointer or no type
  check variable "zc" failed (die: 0x7b9580a)
   variable location: base=reg7, offset=0x40
   type='struct tcp_zerocopy_receive' size=0x40 (die:0x7b947f4)

After:
  -----------------------------------------------------------
  find data type for 0x7c(reg7) at tcp_getsockopt+0xb62
  CU for net/ipv4/tcp.c (die:0x7b5f516)
  frame base: cfa=0 fbreg=6
  found "zc" in scope=3/3 (die: 0x7b957fc) type_offset=0x3c
   variable location: base=reg7, offset=0x40
   type='struct tcp_zerocopy_receive' size=0x40 (die:0x7b947f4)

Note that the type-offset was properly calculated to 0x3c as the
variable starts at 0x40.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index c6eb5b2cc4d5..2dfbdd804222 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -25,6 +25,9 @@
 #include "symbol_conf.h"
 #include "thread.h"
 
+/* register number of the stack pointer */
+#define X86_REG_SP 7
+
 enum type_state_kind {
 	TSR_KIND_INVALID = 0,
 	TSR_KIND_TYPE,
@@ -197,7 +200,7 @@ static void init_type_state(struct type_state *state, struct arch *arch)
 		state->regs[10].caller_saved = true;
 		state->regs[11].caller_saved = true;
 		state->ret_reg = 0;
-		state->stack_reg = 7;
+		state->stack_reg = X86_REG_SP;
 	}
 }
 
@@ -382,10 +385,18 @@ static bool find_cu_die(struct debuginfo *di, u64 pc, Dwarf_Die *cu_die)
 }
 
 /* The type info will be saved in @type_die */
-static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
-			  bool is_pointer)
+static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
+			  Dwarf_Die *type_die, int reg, int offset, bool is_fbreg)
 {
 	Dwarf_Word size;
+	bool is_pointer = true;
+
+	if (reg == DWARF_REG_PC)
+		is_pointer = false;
+	else if (reg == dloc->fbreg || is_fbreg)
+		is_pointer = false;
+	else if (arch__is(dloc->arch, "x86") && reg == X86_REG_SP)
+		is_pointer = false;
 
 	/* Get the type of the variable */
 	if (die_get_real_type(var_die, type_die) == NULL) {
@@ -607,7 +618,6 @@ static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
 {
 	u64 pc;
 	int offset;
-	bool is_pointer = false;
 	const char *var_name = NULL;
 	struct global_var_entry *gvar;
 	Dwarf_Die var_die;
@@ -623,7 +633,8 @@ static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
 
 	/* Try to get the variable by address first */
 	if (die_find_variable_by_addr(cu_die, var_addr, &var_die, &offset) &&
-	    check_variable(&var_die, type_die, offset, is_pointer) == 0) {
+	    check_variable(dloc, &var_die, type_die, DWARF_REG_PC, offset,
+			   /*is_fbreg=*/false) == 0) {
 		var_name = dwarf_diename(&var_die);
 		*var_offset = offset;
 		goto ok;
@@ -636,7 +647,8 @@ static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
 
 	/* Try to get the name of global variable */
 	if (die_find_variable_at(cu_die, var_name, pc, &var_die) &&
-	    check_variable(&var_die, type_die, *var_offset, is_pointer) == 0)
+	    check_variable(dloc, &var_die, type_die, DWARF_REG_PC, *var_offset,
+			   /*is_fbreg=*/false) == 0)
 		goto ok;
 
 	return false;
@@ -1587,8 +1599,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 		}
 
 		/* Found a variable, see if it's correct */
-		ret = check_variable(&var_die, type_die, offset,
-				     reg != DWARF_REG_PC && !is_fbreg);
+		ret = check_variable(dloc, &var_die, type_die, reg, offset, is_fbreg);
 		if (ret == 0) {
 			pr_debug_dtp("found \"%s\" in scope=%d/%d (die: %#lx) ",
 				     dwarf_diename(&var_die), i+1, nr_scopes,
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH 0/4] perf annotate-data: A couple of small updates

[Arnaldo Carvalho de Melo]

On Fri, Apr 12, 2024 at 11:33:06AM -0700, Namhyung Kim wrote:
> Hello,
> 
> I am working on improving quality of the data type profiling and I
> found some issues.  One is when more than one variables are placed at
> the same location.  Then it should find the correct one based on the
> given info rather than checking the first one and bailing out.  This
> one (patch 2) can go to the perf-tools tree.
> 
> Another issue is use of the stack pointe register when it's not the
> frame base register.  I found a case where rbp is used as the frame
> base but rsp is also used to point some stack variables.  And it
> confuses itself how to interpret the type of the variable.
> 
> I think these are rare cases but it would depends on the code pattern
> and compiler behavior.  Anyway I can see a tiny improvement in my data

Thanks, applied to perf-tools-next,

- Arnaldo
> with this change. :)
 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (4):
>   perf annotate-data: Improve debug message with location info
>   perf dwarf-aux: Check pointer offset when checking variables
>   perf dwarf-aux: Check variable address range properly
>   perf annotate-data: Handle RSP if it's not the FB register
> 
>  tools/perf/util/annotate-data.c | 126 +++++++++++++++++++++++++-------
>  tools/perf/util/dwarf-aux.c     |  35 ++++++---
>  2 files changed, 125 insertions(+), 36 deletions(-)
> 
> 
> base-commit: 0ffc8fca5c15a70f32c8aff12c566bbd3991bd0a
> -- 
> 2.44.0.683.g7961c838ac-goog