[PATCHSET 0/9] perf annotate-data: Update data-type profiling quality (v1)

[PATCHSET 0/9] perf annotate-data: Update data-type profiling quality (v1)

[Namhyung Kim]

Hello,

I've found a couple of bugs in the DWARF location handling and had
some updates to improve the quality of the type resolution.

The current code only checks the first variable it found in the
closest scope but sometimes it's not good because macro expansions
like container_of (which is used by many list/rb tree manipulation
routines) only gives a very limited information (like void *) with
type cast.  So it needs to lookup other variables in the upper scope.
See the patch 8 for more details.

Also sometimes it can have more information for the parent type if the
pointer is for an embedded type.  For example, a list_head is
typically a part of bigger struct.  Even if it found a variable for
the list_head, it'd be nice if it can tell which list it is.

To compare two type information in general, I've added a heuristic to
take a pointer to a bigger data type.

This is an example data, the portion of unknown type went down a bit
and the atomic_t turned out to be _mapcount in the struct page.

Before:
  #
  # Overhead  Data Type
  # ........  .........
  #
      37.24%  (unknown)
      14.40%  atomic_t 
       8.81%  (stack operation)
       5.54%  struct psi_group_cpu
       3.40%  struct task_struct
       2.99%  struct pcpu_hot
       2.99%  struct cfs_rq
       2.18%  struct audit_krule
       1.93%  struct psi_group
       1.62%  struct sched_entity

After:
  #
  # Overhead  Data Type
  # ........  .........
  #
      36.87%  (unknown)
      14.40%  struct page
       8.81%  (stack operation)
       6.00%  struct psi_group_cpu
       3.40%  struct task_struct
       3.36%  struct cfs_rq
       2.99%  struct pcpu_hot
       2.18%  struct audit_krule
       1.93%  struct psi_group
       1.62%  struct sched_entity

Also updated the debug message and the statistics to help debugging.

The code is available at 'perf/data-profile-update-v1' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git


Thanks,
Namhyung


Namhyung Kim (9):
  perf dwarf-aux: Check allowed location expressions when collecting
    variables
  perf annotate-data: Fix off-by-one in location range check
  perf annotate-data: Add enum type_match_result
  perf annotate-data: Add variable_state_str()
  perf annotate-data: Change return type of find_data_type_block()
  perf annotate-data: Add is_pointer_type() helper
  perf annotate-data: Add is_better_type() helper
  perf annotate-data: Check variables in every scope
  perf annotate-data: Update type stat at the end of
    find_data_type_die()

 tools/perf/util/annotate-data.c | 359 ++++++++++++++++++++------------
 tools/perf/util/dwarf-aux.c     |   5 +-
 2 files changed, 230 insertions(+), 134 deletions(-)

-- 
2.46.0.184.g6999bdac58-goog

[PATCH 1/9] perf dwarf-aux: Check allowed location expressions when collecting variables

[Namhyung Kim]

It missed to call check_allowed_ops() in __die_collect_vars_cb() so it
can take variables with complex location expression incorrectly.

For example, I found some variable has this expression.

    015d8df8 ffffffff81aacfb3 (base address)
    015d8e01 v000000000000004 v000000000000000 views at 015d8df2 for:
             ffffffff81aacfb3 ffffffff81aacfd2 (DW_OP_fbreg: -176; DW_OP_deref;
						DW_OP_plus_uconst: 332; DW_OP_deref_size: 4;
						DW_OP_lit1; DW_OP_shra; DW_OP_const1u: 64;
						DW_OP_minus; DW_OP_stack_value)
    015d8e14 v000000000000000 v000000000000000 views at 015d8df4 for:
             ffffffff81aacfd2 ffffffff81aacfd7 (DW_OP_reg3 (rbx))
    015d8e19 v000000000000000 v000000000000000 views at 015d8df6 for:
             ffffffff81aacfd7 ffffffff81aad020 (DW_OP_fbreg: -176; DW_OP_deref;
						DW_OP_plus_uconst: 332; DW_OP_deref_size: 4;
						DW_OP_lit1; DW_OP_shra; DW_OP_const1u: 64;
						DW_OP_minus; DW_OP_stack_value)
    015d8e2c <End of list>

It looks like '((int *)(-176(%rbp) + 332) >> 1) - 64' but the current
code thought it's just -176(%rbp) and processed the variable incorrectly.
It should reject such a complex expression if check_allowed_ops()
doesn't like it. :)

Fixes: 932dcc2c39ae ("perf dwarf-aux: Add die_collect_vars()")
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 5e080d7e22c2..beb632153a74 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1598,6 +1598,9 @@ static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
 	if (dwarf_getlocations(&attr, 0, &base, &start, &end, &ops, &nops) <= 0)
 		return DIE_FIND_CB_SIBLING;
 
+	if (!check_allowed_ops(ops, nops))
+		return DIE_FIND_CB_SIBLING;
+
 	if (die_get_real_type(die_mem, &type_die) == NULL)
 		return DIE_FIND_CB_SIBLING;
 
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 2/9] perf annotate-data: Fix off-by-one in location range check

[Namhyung Kim]

The location list will have entries with half-open addressing like
[start, end) which means it doesn't include the end address.  So it
should skip entries at the end address and match to the next entry.

An example location list looks like this:

    00237876 ffffffff8110d32b (base address)
    0023787f v000000000000000 v000000000000002 views at 00237868 for:
             ffffffff8110d32b ffffffff8110d4eb (DW_OP_reg3 (rbx))     <<<--- 1
    00237885 v000000000000002 v000000000000000 views at 0023786a for:
             ffffffff8110d4eb ffffffff8110d50b (DW_OP_reg14 (r14))    <<<--- 2
    0023788c v000000000000000 v000000000000001 views at 0023786c for:
             ffffffff8110d50b ffffffff8110d7c4 (DW_OP_reg3 (rbx))
    00237893 v000000000000000 v000000000000000 views at 0023786e for:
             ffffffff8110d806 ffffffff8110d854 (DW_OP_reg3 (rbx))
    0023789a v000000000000000 v000000000000000 views at 00237870 for:
             ffffffff8110d876 ffffffff8110d88e (DW_OP_reg3 (rbx))

The first entry at 0023787f has [8110d32b, 8110d4eb) (omitting the
ffffffff at the beginning), and the second one has [8110d4eb, 8110d50b).

Fixes: 2bc3cf575a16 ("perf annotate-data: Improve debug message with location info")
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 2 +-
 tools/perf/util/dwarf-aux.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index ff85d190e3ac..fd8d3cdead5a 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -95,7 +95,7 @@ static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
 		return;
 
 	while ((off = dwarf_getlocations(&attr, off, &base, &start, &end, &ops, &nops)) > 0) {
-		if (reg != DWARF_REG_PC && end < pc)
+		if (reg != DWARF_REG_PC && end <= pc)
 			continue;
 		if (reg != DWARF_REG_PC && start > pc)
 			break;
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index beb632153a74..0151a8d14350 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1444,7 +1444,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
 
 	while ((off = dwarf_getlocations(&attr, off, &base, &start, &end, &ops, &nops)) > 0) {
 		/* Assuming the location list is sorted by address */
-		if (end < data->pc)
+		if (end <= data->pc)
 			continue;
 		if (start > data->pc)
 			break;
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 3/9] perf annotate-data: Add enum type_match_result

[Namhyung Kim]

And let check_variable() return the enum value so that callers can know
what was the problem.  This will be used by the later patch to update
the statistics correctly and print the error message in a right place.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index fd8d3cdead5a..8e3b422eca22 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -345,9 +345,20 @@ static bool find_cu_die(struct debuginfo *di, u64 pc, Dwarf_Die *cu_die)
 	return false;
 }
 
+enum type_match_result {
+	PERF_TMR_UNKNOWN = 0,
+	PERF_TMR_OK,
+	PERF_TMR_NO_TYPE,
+	PERF_TMR_NO_POINTER,
+	PERF_TMR_NO_SIZE,
+	PERF_TMR_BAD_OFFSET,
+};
+
 /* The type info will be saved in @type_die */
-static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
-			  Dwarf_Die *type_die, int reg, int offset, bool is_fbreg)
+static enum type_match_result 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;
@@ -364,7 +375,7 @@ static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
 	if (__die_get_real_type(var_die, type_die) == NULL) {
 		pr_debug_dtp("variable has no type\n");
 		ann_data_stat.no_typeinfo++;
-		return -1;
+		return PERF_TMR_NO_TYPE;
 	}
 
 	/*
@@ -378,7 +389,7 @@ static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
 		    __die_get_real_type(type_die, type_die) == NULL) {
 			pr_debug_dtp("no pointer or no type\n");
 			ann_data_stat.no_typeinfo++;
-			return -1;
+			return PERF_TMR_NO_POINTER;
 		}
 	}
 
@@ -391,7 +402,7 @@ static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
 	if (dwarf_aggregate_size(&sized_type, &size) < 0) {
 		pr_debug_dtp("type size is unknown\n");
 		ann_data_stat.invalid_size++;
-		return -1;
+		return PERF_TMR_NO_SIZE;
 	}
 
 	/* Minimal sanity check */
@@ -399,10 +410,10 @@ static int check_variable(struct data_loc_info *dloc, Dwarf_Die *var_die,
 		pr_debug_dtp("offset: %d is bigger than size: %"PRIu64"\n",
 			     offset, size);
 		ann_data_stat.bad_offset++;
-		return -1;
+		return PERF_TMR_BAD_OFFSET;
 	}
 
-	return 0;
+	return PERF_TMR_OK;
 }
 
 struct type_state_stack *find_stack_state(struct type_state *state,
@@ -652,7 +663,7 @@ 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(dloc, &var_die, type_die, DWARF_REG_PC, offset,
-			   /*is_fbreg=*/false) == 0) {
+			   /*is_fbreg=*/false) == PERF_TMR_OK) {
 		var_name = dwarf_diename(&var_die);
 		*var_offset = offset;
 		goto ok;
@@ -666,7 +677,7 @@ 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(dloc, &var_die, type_die, DWARF_REG_PC, *var_offset,
-			   /*is_fbreg=*/false) == 0)
+			   /*is_fbreg=*/false) == PERF_TMR_OK)
 		goto ok;
 
 	return false;
@@ -1205,6 +1216,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	bool is_fbreg = false;
 	u64 pc;
 	char buf[64];
+	enum type_match_result result;
 
 	if (dloc->op->multi_regs)
 		snprintf(buf, sizeof(buf), "reg%d, reg%d", dloc->op->reg1, dloc->op->reg2);
@@ -1299,8 +1311,8 @@ 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(dloc, &var_die, type_die, reg, offset, is_fbreg);
-		if (ret == 0) {
+		result = check_variable(dloc, &var_die, type_die, reg, offset, is_fbreg);
+		if (result == PERF_TMR_OK) {
 			pr_debug_dtp("found \"%s\" in scope=%d/%d (die: %#lx) ",
 				     dwarf_diename(&var_die), i+1, nr_scopes,
 				     (long)dwarf_dieoffset(&scopes[i]));
@@ -1315,12 +1327,14 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			}
 			pr_debug_location(&var_die, pc, reg);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
+			ret = 0;
 		} 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);
+			ret = -1;
 		}
 		dloc->type_offset = offset;
 		goto out;
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 4/9] perf annotate-data: Add variable_state_str()

[Namhyung Kim]

So that it can show a proper debug message in the right place.  The
check_variable() is used in other places which don't want to print the
message.

  $ perf --debug type-profile annotate --data-type

Before:
  -----------------------------------------------------------
  find data type for 0x140(reg14) at update_blocked_averages+0x2db
  CU for kernel/sched/fair.c (die:0x12dd892)
  frame base: cfa=1 fbreg=7
  no pointer or no type                                         <<<--- removed
  check variable "__mptr" failed (die: 0x13022f1)
   variable location: base=reg14, offset=0x140
   type='void*' size=0x8 (die:0x12dd8f9)

After:
  -----------------------------------------------------------
  find data type for 0x140(reg14) at update_blocked_averages+0x2db
  CU for kernel/sched/fair.c (die:0x12dd892)
  frame base: cfa=1 fbreg=7
  found "__mptr" (die: 0x13022f1) in scope=4/4 (die: 0x13022e8) failed: no/void pointer  <<<--- here
   variable location: base=reg14, offset=0x140
   type='void*' size=0x8 (die:0x12dd8f9)

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 8e3b422eca22..332254da49be 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -354,6 +354,25 @@ enum type_match_result {
 	PERF_TMR_BAD_OFFSET,
 };
 
+static const char *match_result_str(enum type_match_result tmr)
+{
+	switch (tmr) {
+	case PERF_TMR_OK:
+		return "Good!";
+	case PERF_TMR_NO_TYPE:
+		return "no type information";
+	case PERF_TMR_NO_POINTER:
+		return "no/void pointer";
+	case PERF_TMR_NO_SIZE:
+		return "type size is unknown";
+	case PERF_TMR_BAD_OFFSET:
+		return "offset bigger than size";
+	case PERF_TMR_UNKNOWN:
+	default:
+		return "invalid state";
+	}
+}
+
 /* The type info will be saved in @type_die */
 static enum type_match_result check_variable(struct data_loc_info *dloc,
 					     Dwarf_Die *var_die,
@@ -373,7 +392,6 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 
 	/* Get the type of the variable */
 	if (__die_get_real_type(var_die, type_die) == NULL) {
-		pr_debug_dtp("variable has no type\n");
 		ann_data_stat.no_typeinfo++;
 		return PERF_TMR_NO_TYPE;
 	}
@@ -387,7 +405,6 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 		if ((dwarf_tag(type_die) != DW_TAG_pointer_type &&
 		     dwarf_tag(type_die) != DW_TAG_array_type) ||
 		    __die_get_real_type(type_die, type_die) == NULL) {
-			pr_debug_dtp("no pointer or no type\n");
 			ann_data_stat.no_typeinfo++;
 			return PERF_TMR_NO_POINTER;
 		}
@@ -400,15 +417,12 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 
 	/* Get the size of the actual type */
 	if (dwarf_aggregate_size(&sized_type, &size) < 0) {
-		pr_debug_dtp("type size is unknown\n");
 		ann_data_stat.invalid_size++;
 		return PERF_TMR_NO_SIZE;
 	}
 
 	/* Minimal sanity check */
 	if ((unsigned)offset >= size) {
-		pr_debug_dtp("offset: %d is bigger than size: %"PRIu64"\n",
-			     offset, size);
 		ann_data_stat.bad_offset++;
 		return PERF_TMR_BAD_OFFSET;
 	}
@@ -1310,12 +1324,13 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 				continue;
 		}
 
+		pr_debug_dtp("found \"%s\" (die: %#lx) in scope=%d/%d (die: %#lx) ",
+			     dwarf_diename(&var_die), (long)dwarf_dieoffset(&var_die),
+			     i+1, nr_scopes, (long)dwarf_dieoffset(&scopes[i]));
+
 		/* Found a variable, see if it's correct */
 		result = check_variable(dloc, &var_die, type_die, reg, offset, is_fbreg);
 		if (result == PERF_TMR_OK) {
-			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("addr=%#"PRIx64" type_offset=%#x\n",
 					     dloc->var_addr, offset);
@@ -1325,17 +1340,13 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			} 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);
 			ret = 0;
 		} 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);
+			pr_debug_dtp("failed: %s\n", match_result_str(result));
 			ret = -1;
 		}
+		pr_debug_location(&var_die, pc, reg);
+		pr_debug_type_name(type_die, TSR_KIND_TYPE);
 		dloc->type_offset = offset;
 		goto out;
 	}
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 5/9] perf annotate-data: Change return type of find_data_type_block()

[Namhyung Kim]

So that it can return enum variable_match_type to be propagated to the
find_data_type_die().  Also update the debug message to show the result
of the check_matching_type().

  chk [dd] reg0 offset=0 ok=1 kind=1  : Good!
or
  chk [177] reg4 offset=0x138 ok=0 kind=0 cfa : no type information

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 332254da49be..5548cd8e84ba 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -352,6 +352,7 @@ enum type_match_result {
 	PERF_TMR_NO_POINTER,
 	PERF_TMR_NO_SIZE,
 	PERF_TMR_BAD_OFFSET,
+	PERF_TMR_BAIL_OUT,
 };
 
 static const char *match_result_str(enum type_match_result tmr)
@@ -368,6 +369,7 @@ static const char *match_result_str(enum type_match_result tmr)
 	case PERF_TMR_BAD_OFFSET:
 		return "offset bigger than size";
 	case PERF_TMR_UNKNOWN:
+	case PERF_TMR_BAIL_OUT:
 	default:
 		return "invalid state";
 	}
@@ -868,19 +870,19 @@ static void setup_stack_canary(struct data_loc_info *dloc)
 
 /*
  * It's at the target address, check if it has a matching type.
- * It returns 1 if found, 0 if not or -1 if not found but no need to
- * repeat the search.  The last case is for per-cpu variables which
+ * It returns PERF_TMR_BAIL_OUT when it looks up per-cpu variables which
  * are similar to global variables and no additional info is needed.
  */
-static int check_matching_type(struct type_state *state,
-			       struct data_loc_info *dloc,
-			       Dwarf_Die *cu_die, Dwarf_Die *type_die)
+static enum type_match_result check_matching_type(struct type_state *state,
+						  struct data_loc_info *dloc,
+						  Dwarf_Die *cu_die,
+						  Dwarf_Die *type_die)
 {
 	Dwarf_Word size;
 	u32 insn_offset = dloc->ip - dloc->ms->sym->start;
 	int reg = dloc->op->reg1;
 
-	pr_debug_dtp("chk [%x] reg%d offset=%#x ok=%d kind=%d",
+	pr_debug_dtp("chk [%x] reg%d offset=%#x ok=%d kind=%d ",
 		     insn_offset, reg, dloc->op->offset,
 		     state->regs[reg].ok, state->regs[reg].kind);
 
@@ -896,15 +898,12 @@ static int check_matching_type(struct type_state *state,
 			if (dloc->op->offset < 0 && reg != state->stack_reg)
 				goto check_kernel;
 
-			pr_debug_dtp("\n");
-			return -1;
+			return PERF_TMR_NO_POINTER;
 		}
 
-		pr_debug_dtp("\n");
-
 		/* Remove the pointer and get the target type */
 		if (__die_get_real_type(&state->regs[reg].type, type_die) == NULL)
-			return -1;
+			return PERF_TMR_NO_POINTER;
 
 		dloc->type_offset = dloc->op->offset;
 
@@ -916,33 +915,33 @@ static int check_matching_type(struct type_state *state,
 		/* Get the size of the actual type */
 		if (dwarf_aggregate_size(&sized_type, &size) < 0 ||
 		    (unsigned)dloc->type_offset >= size)
-			return -1;
+			return PERF_TMR_BAD_OFFSET;
 
-		return 1;
+		return PERF_TMR_OK;
 	}
 
 	if (reg == dloc->fbreg) {
 		struct type_state_stack *stack;
 
-		pr_debug_dtp(" fbreg\n");
+		pr_debug_dtp("fbreg");
 
 		stack = find_stack_state(state, dloc->type_offset);
 		if (stack == NULL)
-			return 0;
+			return PERF_TMR_NO_TYPE;
 
 		if (stack->kind == TSR_KIND_CANARY) {
 			setup_stack_canary(dloc);
-			return -1;
+			return PERF_TMR_BAIL_OUT;
 		}
 
 		if (stack->kind != TSR_KIND_TYPE)
-			return 0;
+			return PERF_TMR_NO_TYPE;
 
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= stack->offset;
 
-		return 1;
+		return PERF_TMR_OK;
 	}
 
 	if (dloc->fb_cfa) {
@@ -950,38 +949,38 @@ static int check_matching_type(struct type_state *state,
 		u64 pc = map__rip_2objdump(dloc->ms->map, dloc->ip);
 		int fbreg, fboff;
 
-		pr_debug_dtp(" cfa\n");
+		pr_debug_dtp("cfa");
 
 		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
 			fbreg = -1;
 
 		if (reg != fbreg)
-			return 0;
+			return PERF_TMR_NO_TYPE;
 
 		stack = find_stack_state(state, dloc->type_offset - fboff);
 		if (stack == NULL)
-			return 0;
+			return PERF_TMR_NO_TYPE;
 
 		if (stack->kind == TSR_KIND_CANARY) {
 			setup_stack_canary(dloc);
-			return -1;
+			return PERF_TMR_BAIL_OUT;
 		}
 
 		if (stack->kind != TSR_KIND_TYPE)
-			return 0;
+			return PERF_TMR_NO_TYPE;
 
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= fboff + stack->offset;
 
-		return 1;
+		return PERF_TMR_OK;
 	}
 
 	if (state->regs[reg].kind == TSR_KIND_PERCPU_BASE) {
 		u64 var_addr = dloc->op->offset;
 		int var_offset;
 
-		pr_debug_dtp(" percpu var\n");
+		pr_debug_dtp("percpu var");
 
 		if (dloc->op->multi_regs) {
 			int reg2 = dloc->op->reg2;
@@ -997,14 +996,14 @@ static int check_matching_type(struct type_state *state,
 		if (get_global_var_type(cu_die, dloc, dloc->ip, var_addr,
 					&var_offset, type_die)) {
 			dloc->type_offset = var_offset;
-			return 1;
+			return PERF_TMR_OK;
 		}
 		/* No need to retry per-cpu (global) variables */
-		return -1;
+		return PERF_TMR_BAIL_OUT;
 	}
 
 	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_POINTER) {
-		pr_debug_dtp(" percpu ptr\n");
+		pr_debug_dtp("percpu ptr");
 
 		/*
 		 * It's actaully pointer but the address was calculated using
@@ -1017,13 +1016,13 @@ static int check_matching_type(struct type_state *state,
 		/* Get the size of the actual type */
 		if (dwarf_aggregate_size(type_die, &size) < 0 ||
 		    (unsigned)dloc->type_offset >= size)
-			return -1;
+			return PERF_TMR_BAIL_OUT;
 
-		return 1;
+		return PERF_TMR_OK;
 	}
 
 	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_CANARY) {
-		pr_debug_dtp(" stack canary\n");
+		pr_debug_dtp("stack canary");
 
 		/*
 		 * This is a saved value of the stack canary which will be handled
@@ -1032,7 +1031,7 @@ static int check_matching_type(struct type_state *state,
 		 */
 		setup_stack_canary(dloc);
 
-		return -1;
+		return PERF_TMR_BAIL_OUT;
 	}
 
 check_kernel:
@@ -1043,16 +1042,16 @@ static int check_matching_type(struct type_state *state,
 		/* Direct this-cpu access like "%gs:0x34740" */
 		if (dloc->op->segment == INSN_SEG_X86_GS && dloc->op->imm &&
 		    arch__is(dloc->arch, "x86")) {
-			pr_debug_dtp(" this-cpu var\n");
+			pr_debug_dtp("this-cpu var");
 
 			addr = dloc->op->offset;
 
 			if (get_global_var_type(cu_die, dloc, dloc->ip, addr,
 						&offset, type_die)) {
 				dloc->type_offset = offset;
-				return 1;
+				return PERF_TMR_OK;
 			}
-			return -1;
+			return PERF_TMR_BAIL_OUT;
 		}
 
 		/* Access to global variable like "-0x7dcf0500(,%rdx,8)" */
@@ -1061,31 +1060,30 @@ static int check_matching_type(struct type_state *state,
 
 			if (get_global_var_type(cu_die, dloc, dloc->ip, addr,
 						&offset, type_die)) {
-				pr_debug_dtp(" global var\n");
+				pr_debug_dtp("global var");
 
 				dloc->type_offset = offset;
-				return 1;
+				return PERF_TMR_OK;
 			}
-			pr_debug_dtp(" negative offset\n");
-			return -1;
+			return PERF_TMR_BAIL_OUT;
 		}
 	}
 
-	pr_debug_dtp("\n");
-	return 0;
+	return PERF_TMR_UNKNOWN;
 }
 
 /* Iterate instructions in basic blocks and update type table */
-static int find_data_type_insn(struct data_loc_info *dloc,
-			       struct list_head *basic_blocks,
-			       struct die_var_type *var_types,
-			       Dwarf_Die *cu_die, Dwarf_Die *type_die)
+static enum type_match_result find_data_type_insn(struct data_loc_info *dloc,
+						  struct list_head *basic_blocks,
+						  struct die_var_type *var_types,
+						  Dwarf_Die *cu_die,
+						  Dwarf_Die *type_die)
 {
 	struct type_state state;
 	struct symbol *sym = dloc->ms->sym;
 	struct annotation *notes = symbol__annotation(sym);
 	struct annotated_basic_block *bb;
-	int ret = 0;
+	enum type_match_result ret = PERF_TMR_UNKNOWN;
 
 	init_type_state(&state, dloc->arch);
 
@@ -1111,6 +1109,7 @@ static int find_data_type_insn(struct data_loc_info *dloc,
 			if (this_ip == dloc->ip) {
 				ret = check_matching_type(&state, dloc,
 							  cu_die, type_die);
+				pr_debug_dtp(" : %s\n", match_result_str(ret));
 				goto out;
 			}
 
@@ -1137,24 +1136,25 @@ static int arch_supports_insn_tracking(struct data_loc_info *dloc)
  * Construct a list of basic blocks for each scope with variables and try to find
  * the data type by updating a type state table through instructions.
  */
-static int find_data_type_block(struct data_loc_info *dloc,
-				Dwarf_Die *cu_die, Dwarf_Die *scopes,
-				int nr_scopes, Dwarf_Die *type_die)
+static enum type_match_result find_data_type_block(struct data_loc_info *dloc,
+						   Dwarf_Die *cu_die,
+						   Dwarf_Die *scopes,
+						   int nr_scopes,
+						   Dwarf_Die *type_die)
 {
 	LIST_HEAD(basic_blocks);
 	struct die_var_type *var_types = NULL;
 	u64 src_ip, dst_ip, prev_dst_ip;
-	int ret = -1;
+	enum type_match_result ret = PERF_TMR_UNKNOWN;
 
 	/* TODO: other architecture support */
 	if (!arch_supports_insn_tracking(dloc))
-		return -1;
+		return PERF_TMR_BAIL_OUT;
 
 	prev_dst_ip = dst_ip = dloc->ip;
 	for (int i = nr_scopes - 1; i >= 0; i--) {
 		Dwarf_Addr base, start, end;
 		LIST_HEAD(this_blocks);
-		int found;
 
 		if (dwarf_ranges(&scopes[i], 0, &base, &start, &end) < 0)
 			break;
@@ -1185,9 +1185,9 @@ static int find_data_type_block(struct data_loc_info *dloc,
 		fixup_var_address(var_types, start);
 
 		/* Find from start of this scope to the target instruction */
-		found = find_data_type_insn(dloc, &basic_blocks, var_types,
+		ret = find_data_type_insn(dloc, &basic_blocks, var_types,
 					    cu_die, type_die);
-		if (found > 0) {
+		if (ret == PERF_TMR_OK) {
 			char buf[64];
 
 			if (dloc->op->multi_regs)
@@ -1199,11 +1199,10 @@ static int find_data_type_block(struct data_loc_info *dloc,
 			pr_debug_dtp("found by insn track: %#x(%s) type-offset=%#x\n",
 				     dloc->op->offset, buf, dloc->type_offset);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
-			ret = 0;
 			break;
 		}
 
-		if (found < 0)
+		if (ret == PERF_TMR_BAIL_OUT)
 			break;
 
 		/* Go up to the next scope and find blocks to the start */
@@ -1357,11 +1356,11 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	}
 
 	if (reg != DWARF_REG_PC) {
-		ret = find_data_type_block(dloc, &cu_die, scopes,
+		result = find_data_type_block(dloc, &cu_die, scopes,
 					   nr_scopes, type_die);
-		if (ret == 0) {
+		if (result == PERF_TMR_OK) {
 			ann_data_stat.insn_track++;
-			goto out;
+			ret = 0;
 		}
 	}
 
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 6/9] perf annotate-data: Add is_pointer_type() helper

[Namhyung Kim]

It treats pointers and arrays in the same way.  Let's add the helper and
use it when it checks if it needs a pointer.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 5548cd8e84ba..b2414747cdeb 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -375,6 +375,13 @@ static const char *match_result_str(enum type_match_result tmr)
 	}
 }
 
+static bool is_pointer_type(Dwarf_Die *type_die)
+{
+	int tag = dwarf_tag(type_die);
+
+	return tag == DW_TAG_pointer_type || tag == DW_TAG_array_type;
+}
+
 /* The type info will be saved in @type_die */
 static enum type_match_result check_variable(struct data_loc_info *dloc,
 					     Dwarf_Die *var_die,
@@ -382,15 +389,15 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 					     int offset, bool is_fbreg)
 {
 	Dwarf_Word size;
-	bool is_pointer = true;
+	bool needs_pointer = true;
 	Dwarf_Die sized_type;
 
 	if (reg == DWARF_REG_PC)
-		is_pointer = false;
+		needs_pointer = false;
 	else if (reg == dloc->fbreg || is_fbreg)
-		is_pointer = false;
+		needs_pointer = false;
 	else if (arch__is(dloc->arch, "x86") && reg == X86_REG_SP)
-		is_pointer = false;
+		needs_pointer = false;
 
 	/* Get the type of the variable */
 	if (__die_get_real_type(var_die, type_die) == NULL) {
@@ -403,9 +410,8 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 	 * Convert to a real type it points to.  But global variables
 	 * and local variables are accessed directly without a pointer.
 	 */
-	if (is_pointer) {
-		if ((dwarf_tag(type_die) != DW_TAG_pointer_type &&
-		     dwarf_tag(type_die) != DW_TAG_array_type) ||
+	if (needs_pointer) {
+		if (!is_pointer_type(type_die) ||
 		    __die_get_real_type(type_die, type_die) == NULL) {
 			ann_data_stat.no_typeinfo++;
 			return PERF_TMR_NO_POINTER;
@@ -887,14 +893,13 @@ static enum type_match_result check_matching_type(struct type_state *state,
 		     state->regs[reg].ok, state->regs[reg].kind);
 
 	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_TYPE) {
-		int tag = dwarf_tag(&state->regs[reg].type);
 		Dwarf_Die sized_type;
 
 		/*
 		 * Normal registers should hold a pointer (or array) to
 		 * dereference a memory location.
 		 */
-		if (tag != DW_TAG_pointer_type && tag != DW_TAG_array_type) {
+		if (!is_pointer_type(&state->regs[reg].type)) {
 			if (dloc->op->offset < 0 && reg != state->stack_reg)
 				goto check_kernel;
 
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 7/9] perf annotate-data: Add is_better_type() helper

[Namhyung Kim]

Sometimes more than one variables are located in the same register or a
stack slot.  Or it can overwrite existing information with others.  I
found this is not helpful in some cases so it needs to update the type
information from the variable only if it's better.

But it's hard to know which one is better, so we needs heuristics. :)

As it deals with memory accesses, the location should have a pointer or
something similar (like array or reference).  So if it had an integer
type and a variable is a pointer, we can take the variable's type to
resolve the target of the access.

If it has a pointer type and a variable with the same location has a
different pointer type, it'll take one with bigger target type.  This
can be useful when the target type embeds a smaller type (like list
header or RB-tree node) at the beginning so their location is same.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index b2414747cdeb..916d26bfb9eb 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -382,6 +382,38 @@ static bool is_pointer_type(Dwarf_Die *type_die)
 	return tag == DW_TAG_pointer_type || tag == DW_TAG_array_type;
 }
 
+/* returns if Type B has better information than Type A */
+static bool is_better_type(Dwarf_Die *type_a, Dwarf_Die *type_b)
+{
+	Dwarf_Word size_a, size_b;
+	Dwarf_Die die_a, die_b;
+
+	/* pointer type is preferred */
+	if (is_pointer_type(type_a) != is_pointer_type(type_b))
+		return is_pointer_type(type_b);
+
+	if (is_pointer_type(type_b)) {
+		/*
+		 * We want to compare the target type, but 'void *' can fail to
+		 * get the target type.
+		 */
+		if (die_get_real_type(type_a, &die_a) == NULL)
+			return true;
+		if (die_get_real_type(type_b, &die_b) == NULL)
+			return false;
+
+		type_a = &die_a;
+		type_b = &die_b;
+	}
+
+	/* bigger type is preferred */
+	if (dwarf_aggregate_size(type_a, &size_a) < 0 ||
+	    dwarf_aggregate_size(type_b, &size_b) < 0)
+		return false;
+
+	return size_a < size_b;
+}
+
 /* The type info will be saved in @type_die */
 static enum type_match_result check_variable(struct data_loc_info *dloc,
 					     Dwarf_Die *var_die,
@@ -741,24 +773,33 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
 		if (!dwarf_offdie(dloc->di->dbg, var->die_off, &mem_die))
 			continue;
 
-		if (var->reg == DWARF_REG_FB) {
-			findnew_stack_state(state, var->offset, TSR_KIND_TYPE,
-					    &mem_die);
+		if (var->reg == DWARF_REG_FB || var->reg == fbreg) {
+			int offset = var->offset;
+			struct type_state_stack *stack;
 
-			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
-				     insn_offset, -var->offset);
-			pr_debug_type_name(&mem_die, TSR_KIND_TYPE);
-		} else if (var->reg == fbreg) {
-			findnew_stack_state(state, var->offset - fb_offset,
-					    TSR_KIND_TYPE, &mem_die);
+			if (var->reg != DWARF_REG_FB)
+				offset -= fb_offset;
+
+			stack = find_stack_state(state, offset);
+			if (stack && stack->kind == TSR_KIND_TYPE &&
+			    !is_better_type(&stack->type, &mem_die))
+				continue;
+
+			findnew_stack_state(state, offset, TSR_KIND_TYPE,
+					    &mem_die);
 
 			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
-				     insn_offset, -var->offset + fb_offset);
+				     insn_offset, -offset);
 			pr_debug_type_name(&mem_die, TSR_KIND_TYPE);
 		} else if (has_reg_type(state, var->reg) && var->offset == 0) {
 			struct type_state_reg *reg;
 
 			reg = &state->regs[var->reg];
+
+			if (reg->ok && reg->kind == TSR_KIND_TYPE &&
+			    !is_better_type(&reg->type, &mem_die))
+				continue;
+
 			reg->type = mem_die;
 			reg->kind = TSR_KIND_TYPE;
 			reg->ok = true;
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 8/9] perf annotate-data: Check variables in every scope

[Namhyung Kim]

Sometimes it matches a variable in the inner scope but it fails because
the actual access can be on a different type.  Let's try variables in
every scope and choose the best one using is_better_type().

I have an example with update_blocked_averages(), at first it found a
variable (__mptr) but it's a void pointer.  So it moved on to the upper
scope and found another variable (cfs_rq).

  $ perf --debug type-profile annotate --data-type --stdio
  ...
  -----------------------------------------------------------
  find data type for 0x140(reg14) at update_blocked_averages+0x2db
  CU for kernel/sched/fair.c (die:0x12dd892)
  frame base: cfa=1 fbreg=7
  found "__mptr" (die: 0x13022f1) in scope=4/4 (die: 0x13022e8) failed: no/void pointer
   variable location: base=reg14, offset=0x140
   type='void*' size=0x8 (die:0x12dd8f9)
  found "cfs_rq" (die: 0x1301721) in scope=3/4 (die: 0x130171c) type_offset=0x140
   variable location: reg14
   type='struct cfs_rq' size=0x1c0 (die:0x12e37e5)
  final type: type='struct cfs_rq' size=0x1c0 (die:0x12e37e5)

IIUC the scope is like below:
  1: update_blocked_averages
  2:   __update_blocked_fair
  3:     for_each_leaf_cfs_rq_safe
  4:       list_entry -> (container_of)

The container_of is implemented like:

  #define container_of(ptr, type, member) ({				\
  	void *__mptr = (void *)(ptr);					\
  	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
  		      __same_type(*(ptr), void),			\
  		      "pointer type mismatch in container_of()");	\
  	((type *)(__mptr - offsetof(type, member))); })

That's why we see the __mptr variable first but it failed since it has
no type information.

Then for_each_leaf_cfs_rq_safe() is defined as

  #define for_each_leaf_cfs_rq_safe(rq, cfs_rq, pos)			\
  	list_for_each_entry_safe(cfs_rq, pos, &rq->leaf_cfs_rq_list,	\
  				 leaf_cfs_rq_list)

Note that the access was 0x140(r14).  And the cfs_rq has
leaf_cfs_rq_list at the 0x140.  So it converts the list_head pointer to
a pointer to struct cfs_rq here.

  $ pahole --hex -C cfs_rq vmlinux | grep 140
  struct cfs_rq 	struct list_head           leaf_cfs_rq_list;     /* 0x140  0x10 */

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 916d26bfb9eb..e86f40fed323 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -1244,7 +1244,6 @@ static enum type_match_result find_data_type_block(struct data_loc_info *dloc,
 
 			pr_debug_dtp("found by insn track: %#x(%s) type-offset=%#x\n",
 				     dloc->op->offset, buf, dloc->type_offset);
-			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 			break;
 		}
 
@@ -1273,6 +1272,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	int fbreg = -1;
 	int fb_offset = 0;
 	bool is_fbreg = false;
+	bool found = false;
 	u64 pc;
 	char buf[64];
 	enum type_match_result result;
@@ -1358,14 +1358,17 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 
 	/* Search from the inner-most scope to the outer */
 	for (i = nr_scopes - 1; i >= 0; i--) {
+		Dwarf_Die mem_die;
+		int type_offset = offset;
+
 		if (reg == DWARF_REG_PC) {
 			if (!die_find_variable_by_addr(&scopes[i], dloc->var_addr,
-						       &var_die, &offset))
+						       &var_die, &type_offset))
 				continue;
 		} else {
 			/* Look up variables/parameters in this scope */
 			if (!die_find_variable_by_reg(&scopes[i], pc, reg,
-						      &offset, is_fbreg, &var_die))
+						      &type_offset, is_fbreg, &var_die))
 				continue;
 		}
 
@@ -1374,43 +1377,50 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			     i+1, nr_scopes, (long)dwarf_dieoffset(&scopes[i]));
 
 		/* Found a variable, see if it's correct */
-		result = check_variable(dloc, &var_die, type_die, reg, offset, is_fbreg);
+		result = check_variable(dloc, &var_die, &mem_die, reg, type_offset, is_fbreg);
 		if (result == PERF_TMR_OK) {
 			if (reg == DWARF_REG_PC) {
 				pr_debug_dtp("addr=%#"PRIx64" type_offset=%#x\n",
-					     dloc->var_addr, offset);
+					     dloc->var_addr, type_offset);
 			} else if (reg == DWARF_REG_FB || is_fbreg) {
 				pr_debug_dtp("stack_offset=%#x type_offset=%#x\n",
-					     fb_offset, offset);
+					     fb_offset, type_offset);
 			} else {
-				pr_debug_dtp("type_offset=%#x\n", offset);
+				pr_debug_dtp("type_offset=%#x\n", type_offset);
+			}
+
+			if (!found || is_better_type(type_die, &mem_die)) {
+				*type_die = mem_die;
+				dloc->type_offset = type_offset;
+				found = true;
 			}
-			ret = 0;
 		} else {
 			pr_debug_dtp("failed: %s\n", match_result_str(result));
-			ret = -1;
 		}
+
 		pr_debug_location(&var_die, pc, reg);
-		pr_debug_type_name(type_die, TSR_KIND_TYPE);
-		dloc->type_offset = offset;
-		goto out;
+		pr_debug_type_name(&mem_die, TSR_KIND_TYPE);
 	}
 
-	if (loc->multi_regs && reg == loc->reg1 && loc->reg1 != loc->reg2) {
+	if (!found && loc->multi_regs && reg == loc->reg1 && loc->reg1 != loc->reg2) {
 		reg = loc->reg2;
 		goto retry;
 	}
 
-	if (reg != DWARF_REG_PC) {
+	if (!found && reg != DWARF_REG_PC) {
 		result = find_data_type_block(dloc, &cu_die, scopes,
-					   nr_scopes, type_die);
+					      nr_scopes, type_die);
 		if (result == PERF_TMR_OK) {
 			ann_data_stat.insn_track++;
-			ret = 0;
+			found = true;
 		}
 	}
 
-	if (ret < 0) {
+	if (found) {
+		pr_debug_dtp("final type:");
+		pr_debug_type_name(type_die, TSR_KIND_TYPE);
+		ret = 0;
+	} else {
 		pr_debug_dtp("no variable found\n");
 		ann_data_stat.no_var++;
 	}
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 9/9] perf annotate-data: Update type stat at the end of find_data_type_die()

[Namhyung Kim]

after trying all possibilities with DWARF and instruction tracking.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index e86f40fed323..aa330c7d8edd 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -432,10 +432,8 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 		needs_pointer = false;
 
 	/* Get the type of the variable */
-	if (__die_get_real_type(var_die, type_die) == NULL) {
-		ann_data_stat.no_typeinfo++;
+	if (__die_get_real_type(var_die, type_die) == NULL)
 		return PERF_TMR_NO_TYPE;
-	}
 
 	/*
 	 * Usually it expects a pointer type for a memory access.
@@ -444,10 +442,8 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 	 */
 	if (needs_pointer) {
 		if (!is_pointer_type(type_die) ||
-		    __die_get_real_type(type_die, type_die) == NULL) {
-			ann_data_stat.no_typeinfo++;
+		    __die_get_real_type(type_die, type_die) == NULL)
 			return PERF_TMR_NO_POINTER;
-		}
 	}
 
 	if (dwarf_tag(type_die) == DW_TAG_typedef)
@@ -456,16 +452,12 @@ static enum type_match_result check_variable(struct data_loc_info *dloc,
 		sized_type = *type_die;
 
 	/* Get the size of the actual type */
-	if (dwarf_aggregate_size(&sized_type, &size) < 0) {
-		ann_data_stat.invalid_size++;
+	if (dwarf_aggregate_size(&sized_type, &size) < 0)
 		return PERF_TMR_NO_SIZE;
-	}
 
 	/* Minimal sanity check */
-	if ((unsigned)offset >= size) {
-		ann_data_stat.bad_offset++;
+	if ((unsigned)offset >= size)
 		return PERF_TMR_BAD_OFFSET;
-	}
 
 	return PERF_TMR_OK;
 }
@@ -1275,7 +1267,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	bool found = false;
 	u64 pc;
 	char buf[64];
-	enum type_match_result result;
+	enum type_match_result result = PERF_TMR_UNKNOWN;
 
 	if (dloc->op->multi_regs)
 		snprintf(buf, sizeof(buf), "reg%d, reg%d", dloc->op->reg1, dloc->op->reg2);
@@ -1317,7 +1309,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			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;
+			found = true;
 			goto out;
 		}
 	}
@@ -1416,16 +1408,37 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 		}
 	}
 
+out:
 	if (found) {
 		pr_debug_dtp("final type:");
 		pr_debug_type_name(type_die, TSR_KIND_TYPE);
 		ret = 0;
 	} else {
-		pr_debug_dtp("no variable found\n");
-		ann_data_stat.no_var++;
+		switch (result) {
+		case PERF_TMR_NO_TYPE:
+		case PERF_TMR_NO_POINTER:
+			pr_debug_dtp("%s\n", match_result_str(result));
+			ann_data_stat.no_typeinfo++;
+			break;
+		case PERF_TMR_NO_SIZE:
+			pr_debug_dtp("%s\n", match_result_str(result));
+			ann_data_stat.invalid_size++;
+			break;
+		case PERF_TMR_BAD_OFFSET:
+			pr_debug_dtp("%s\n", match_result_str(result));
+			ann_data_stat.bad_offset++;
+			break;
+		case PERF_TMR_UNKNOWN:
+		case PERF_TMR_BAIL_OUT:
+		case PERF_TMR_OK:  /* should not reach here */
+		default:
+			pr_debug_dtp("no variable found\n");
+			ann_data_stat.no_var++;
+			break;
+		}
+		ret = -1;
 	}
 
-out:
 	free(scopes);
 	return ret;
 }
-- 
2.46.0.184.g6999bdac58-goog

Re: [PATCH 1/9] perf dwarf-aux: Check allowed location expressions when collecting variables

[Masami Hiramatsu]

On Fri, 16 Aug 2024 16:58:31 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> It missed to call check_allowed_ops() in __die_collect_vars_cb() so it
> can take variables with complex location expression incorrectly.
> 
> For example, I found some variable has this expression.
> 
>     015d8df8 ffffffff81aacfb3 (base address)
>     015d8e01 v000000000000004 v000000000000000 views at 015d8df2 for:
>              ffffffff81aacfb3 ffffffff81aacfd2 (DW_OP_fbreg: -176; DW_OP_deref;
> 						DW_OP_plus_uconst: 332; DW_OP_deref_size: 4;
> 						DW_OP_lit1; DW_OP_shra; DW_OP_const1u: 64;
> 						DW_OP_minus; DW_OP_stack_value)
>     015d8e14 v000000000000000 v000000000000000 views at 015d8df4 for:
>              ffffffff81aacfd2 ffffffff81aacfd7 (DW_OP_reg3 (rbx))
>     015d8e19 v000000000000000 v000000000000000 views at 015d8df6 for:
>              ffffffff81aacfd7 ffffffff81aad020 (DW_OP_fbreg: -176; DW_OP_deref;
> 						DW_OP_plus_uconst: 332; DW_OP_deref_size: 4;
> 						DW_OP_lit1; DW_OP_shra; DW_OP_const1u: 64;
> 						DW_OP_minus; DW_OP_stack_value)
>     015d8e2c <End of list>
> 
> It looks like '((int *)(-176(%rbp) + 332) >> 1) - 64' but the current
> code thought it's just -176(%rbp) and processed the variable incorrectly.
> It should reject such a complex expression if check_allowed_ops()
> doesn't like it. :)

Looks good to me. Hmm, I should reconsider to support this complex variable
on ftrace probe events...

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> 
> Fixes: 932dcc2c39ae ("perf dwarf-aux: Add die_collect_vars()")
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 5e080d7e22c2..beb632153a74 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1598,6 +1598,9 @@ static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
>  	if (dwarf_getlocations(&attr, 0, &base, &start, &end, &ops, &nops) <= 0)
>  		return DIE_FIND_CB_SIBLING;
>  
> +	if (!check_allowed_ops(ops, nops))
> +		return DIE_FIND_CB_SIBLING;
> +
>  	if (die_get_real_type(die_mem, &type_die) == NULL)
>  		return DIE_FIND_CB_SIBLING;
>  
> -- 
> 2.46.0.184.g6999bdac58-goog
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 2/9] perf annotate-data: Fix off-by-one in location range check

[Masami Hiramatsu]

On Fri, 16 Aug 2024 16:58:32 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> The location list will have entries with half-open addressing like
> [start, end) which means it doesn't include the end address.  So it
> should skip entries at the end address and match to the next entry.
> 
> An example location list looks like this:
> 
>     00237876 ffffffff8110d32b (base address)
>     0023787f v000000000000000 v000000000000002 views at 00237868 for:
>              ffffffff8110d32b ffffffff8110d4eb (DW_OP_reg3 (rbx))     <<<--- 1
>     00237885 v000000000000002 v000000000000000 views at 0023786a for:
>              ffffffff8110d4eb ffffffff8110d50b (DW_OP_reg14 (r14))    <<<--- 2
>     0023788c v000000000000000 v000000000000001 views at 0023786c for:
>              ffffffff8110d50b ffffffff8110d7c4 (DW_OP_reg3 (rbx))
>     00237893 v000000000000000 v000000000000000 views at 0023786e for:
>              ffffffff8110d806 ffffffff8110d854 (DW_OP_reg3 (rbx))
>     0023789a v000000000000000 v000000000000000 views at 00237870 for:
>              ffffffff8110d876 ffffffff8110d88e (DW_OP_reg3 (rbx))
> 
> The first entry at 0023787f has [8110d32b, 8110d4eb) (omitting the
> ffffffff at the beginning), and the second one has [8110d4eb, 8110d50b).
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks!

> Fixes: 2bc3cf575a16 ("perf annotate-data: Improve debug message with location info")
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 2 +-
>  tools/perf/util/dwarf-aux.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index ff85d190e3ac..fd8d3cdead5a 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -95,7 +95,7 @@ static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
>  		return;
>  
>  	while ((off = dwarf_getlocations(&attr, off, &base, &start, &end, &ops, &nops)) > 0) {
> -		if (reg != DWARF_REG_PC && end < pc)
> +		if (reg != DWARF_REG_PC && end <= pc)
>  			continue;
>  		if (reg != DWARF_REG_PC && start > pc)
>  			break;
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index beb632153a74..0151a8d14350 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1444,7 +1444,7 @@ static int __die_find_var_reg_cb(Dwarf_Die *die_mem, void *arg)
>  
>  	while ((off = dwarf_getlocations(&attr, off, &base, &start, &end, &ops, &nops)) > 0) {
>  		/* Assuming the location list is sorted by address */
> -		if (end < data->pc)
> +		if (end <= data->pc)
>  			continue;
>  		if (start > data->pc)
>  			break;
> -- 
> 2.46.0.184.g6999bdac58-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCHSET 0/9] perf annotate-data: Update data-type profiling quality (v1)

[Arnaldo Carvalho de Melo]

On Fri, Aug 16, 2024 at 04:58:30PM -0700, Namhyung Kim wrote:
> Hello,
> 
> I've found a couple of bugs in the DWARF location handling and had
> some updates to improve the quality of the type resolution.
> 
> The current code only checks the first variable it found in the
> closest scope but sometimes it's not good because macro expansions
> like container_of (which is used by many list/rb tree manipulation
> routines) only gives a very limited information (like void *) with
> type cast.  So it needs to lookup other variables in the upper scope.
> See the patch 8 for more details.
> 
> Also sometimes it can have more information for the parent type if the
> pointer is for an embedded type.  For example, a list_head is
> typically a part of bigger struct.  Even if it found a variable for
> the list_head, it'd be nice if it can tell which list it is.
> 
> To compare two type information in general, I've added a heuristic to
> take a pointer to a bigger data type.
> 
> This is an example data, the portion of unknown type went down a bit
> and the atomic_t turned out to be _mapcount in the struct page.
> 
> Before:
>   #
>   # Overhead  Data Type
>   # ........  .........
>   #
>       37.24%  (unknown)
>       14.40%  atomic_t 
>        8.81%  (stack operation)
>        5.54%  struct psi_group_cpu
>        3.40%  struct task_struct
>        2.99%  struct pcpu_hot
>        2.99%  struct cfs_rq
>        2.18%  struct audit_krule
>        1.93%  struct psi_group
>        1.62%  struct sched_entity
> 
> After:
>   #
>   # Overhead  Data Type
>   # ........  .........
>   #
>       36.87%  (unknown)
>       14.40%  struct page
>        8.81%  (stack operation)
>        6.00%  struct psi_group_cpu
>        3.40%  struct task_struct
>        3.36%  struct cfs_rq
>        2.99%  struct pcpu_hot
>        2.18%  struct audit_krule
>        1.93%  struct psi_group
>        1.62%  struct sched_entity
> 
> Also updated the debug message and the statistics to help debugging.
> 
> The code is available at 'perf/data-profile-update-v1' branch in
> git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 

Thanks, applied to perf-tools-next,

- Arnaldo