[PATCHSET 00/23] Remaining bits of data type profiling (v7)

[PATCHSET 00/23] Remaining bits of data type profiling (v7)

[Namhyung Kim]

Hello,

This is the last part of the data type profiling series.
So far we added the basic pointer variable support, and direct access to
global/local variables.  Now it's time to add instruction tracking. :)

For the history and background, you can refer to the previous version
[1] and the LWN article [2].

 * Changes from v6
  - add a new debug option for data type profiling
  - fix return value look up for function calls
  - handle general per-cpu access (by tracking ADD instructions)
  - add cache for global variables
  - fail insn tracking early when no further info is expected
  - and more small bug fixes

 * Changes from v5
  - rename register state from scratch to caller_saved
  - change comment in map__objdump_2rip()


Basically it needs to track variable (and its type) assignment to get
a type of memory access at the sampled instruction.  Compilers don't
generate DWARF information for every memory accesses so it cannot find
all the necessary information from DWARF.  Therefore, it follows the
path to the sample in the function, and update type information at
each location when the instruction moves it.

For the DWARF search, it has a list of scope entries (subroutines or
blocks) that covers the sample already.  So it can use the scopes to
find the shortest path to the sample instruction.

Let's say we have this.  It got 5 scopes but couldn't find a matching
variable for the sample.

      +----------------  scope[0] subprogram
      |
      | +--------------  scope[1] lexical_block
      | |
      | | +------------  scope[2] inlined_subroutine
      | | |
      | | | +----------  scope[3] inlined_subroutine
      | | | |
      | | | | +--------  scope[4] lexical_block
      | | | | |
      | | | | |     ***  target instruction
      ...

Then it starts with the closest scope (at index 4), and find the
shortest path from the start of the scope to the target instruction.
Along the way, it updates type information in the scope and see if the
location at the target instruction has the type.  If so, it can
return with the type.

Otherwise, it goes to the scope[3] and find the shortest path from the
start of scope[3] to the start of scope[4].  And then it can combine
the existing shortest path from the scope[4] to the target with the
new path.  Now it can start from the scope[3] with new variables and
types.  It can repeat this algorithm for the outer scopes.

I did it this way because mostly it was able to find a type in the
closest scope.  So it can avoid unnecessary work for outer scopes.

And it added a basic per-cpu variable support for this CPU on x86_64
which uses %gs segment register.  Also it can detect the stack-canary
pattern which is added by compiler to detect stack overflow.

The code is available at 'perf/data-profile-v7' branch in the tree
below.  I've dropped the debug patch at the end in this series but you
can find it in the git branch.

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Cc: Ben Woodard <woodard@redhat.com>
Cc: Joe Mario <jmario@redhat.com>
CC: Kees Cook <keescook@chromium.org>
Cc: David Blaikie <blaikie@google.com>
Cc: Xu Liu <xliuprof@google.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Mark Wielaard <mark@klomp.org>
Cc: Jason Merrill <jason@redhat.com>
Cc: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: William Huang <williamjhuang@google.com>


[1] https://lore.kernel.org/linux-perf-users/20231110000012.3538610-1-namhyung@kernel.org/
[2] https://lwn.net/Articles/955709/


Namhyung Kim (23):
  perf dwarf-aux: Remove unused pc argument
  perf dwarf-aux: Add die_collect_vars()
  perf dwarf-aux: Handle type transfer for memory access
  perf dwarf-aux: Add die_find_func_rettype()
  perf map: Add map__objdump_2rip()
  perf annotate-data: Introduce struct data_loc_info
  perf annotate: Add annotate_get_basic_blocks()
  perf annotate-data: Add debug messages
  perf annotate-data: Maintain variable type info
  perf annotate-data: Add update_insn_state()
  perf annotate-data: Add get_global_var_type()
  perf annotate-data: Handle global variable access
  perf annotate-data: Handle call instructions
  perf annotate-data: Implement instruction tracking
  perf annotate-data: Check register state for type
  perf annotate: Parse x86 segment register location
  perf annotate-data: Handle this-cpu variables in kernel
  perf annotate-data: Track instructions with a this-cpu variable
  perf annotate-data: Support general per-cpu access
  perf annotate-data: Handle ADD instructions
  perf annotate-data: Add stack canary type
  perf annotate-data: Add a cache for global variable types
  perf annotate-data: Do not retry for invalid types

 tools/perf/builtin-annotate.c   |    1 +
 tools/perf/util/annotate-data.c | 1320 +++++++++++++++++++++++++++++--
 tools/perf/util/annotate-data.h |   52 +-
 tools/perf/util/annotate.c      |  334 +++++++-
 tools/perf/util/annotate.h      |   31 +
 tools/perf/util/debug.c         |    3 +
 tools/perf/util/debug.h         |    1 +
 tools/perf/util/dso.c           |    2 +
 tools/perf/util/dso.h           |    6 +-
 tools/perf/util/dwarf-aux.c     |  282 ++++++-
 tools/perf/util/dwarf-aux.h     |   33 +-
 tools/perf/util/map.c           |   17 +
 tools/perf/util/map.h           |    3 +
 13 files changed, 1955 insertions(+), 130 deletions(-)


base-commit: 2ebf53c2894623d1ef3bcecf1464dbcc3fe1cb72
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 01/23] perf dwarf-aux: Remove unused pc argument

[Namhyung Kim]

It's not used, let's get rid of it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 4 ++--
 tools/perf/util/dwarf-aux.c     | 7 ++-----
 tools/perf/util/dwarf-aux.h     | 6 ++----
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 30c4d19fcf11..59ce5f4f4a40 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -263,7 +263,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 	offset = loc->offset;
 
 	if (reg == DWARF_REG_PC) {
-		if (die_find_variable_by_addr(&cu_die, pc, addr, &var_die, &offset)) {
+		if (die_find_variable_by_addr(&cu_die, addr, &var_die, &offset)) {
 			ret = check_variable(&var_die, type_die, offset,
 					     /*is_pointer=*/false);
 			loc->offset = offset;
@@ -312,7 +312,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 	/* Search from the inner-most scope to the outer */
 	for (i = nr_scopes - 1; i >= 0; i--) {
 		if (reg == DWARF_REG_PC) {
-			if (!die_find_variable_by_addr(&scopes[i], pc, addr,
+			if (!die_find_variable_by_addr(&scopes[i], addr,
 						       &var_die, &offset))
 				continue;
 		} else {
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 2791126069b4..e84d0d6a7750 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1456,7 +1456,6 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
 /**
  * die_find_variable_by_addr - Find variable located at given address
  * @sc_die: a scope DIE
- * @pc: the program address to find
  * @addr: the data address to find
  * @die_mem: a buffer to save the resulting DIE
  * @offset: the offset in the resulting type
@@ -1464,12 +1463,10 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
  * Find the variable DIE located at the given address (in PC-relative mode).
  * This is usually for global variables.
  */
-Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
-				     Dwarf_Addr addr, Dwarf_Die *die_mem,
-				     int *offset)
+Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
+				     Dwarf_Die *die_mem, int *offset)
 {
 	struct find_var_data data = {
-		.pc = pc,
 		.addr = addr,
 	};
 	Dwarf_Die *result;
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 85dd527ae1f7..9973801a20c1 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -146,9 +146,8 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
 				    Dwarf_Die *die_mem);
 
 /* Find a (global) variable located in the 'addr' */
-Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
-				     Dwarf_Addr addr, Dwarf_Die *die_mem,
-				     int *offset);
+Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
+				     Dwarf_Die *die_mem, int *offset);
 
 #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
 
@@ -170,7 +169,6 @@ static inline Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die __maybe_unus
 }
 
 static inline Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die __maybe_unused,
-						   Dwarf_Addr pc __maybe_unused,
 						   Dwarf_Addr addr __maybe_unused,
 						   Dwarf_Die *die_mem __maybe_unused,
 						   int *offset __maybe_unused)
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 02/23] perf dwarf-aux: Add die_collect_vars()

[Namhyung Kim]

The die_collect_vars() is to find all variable information in the scope
including function parameters.  The struct die_var_type is to save the
type of the variable with the location (reg and offset) as well as where
it's defined in the code (addr).

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 118 +++++++++++++++++++++++++++---------
 tools/perf/util/dwarf-aux.h |  17 ++++++
 2 files changed, 107 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index e84d0d6a7750..785aa7a3d725 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1136,6 +1136,40 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
 	return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
 }
 
+#if defined(HAVE_DWARF_GETLOCATIONS_SUPPORT) || defined(HAVE_DWARF_CFI_SUPPORT)
+static int reg_from_dwarf_op(Dwarf_Op *op)
+{
+	switch (op->atom) {
+	case DW_OP_reg0 ... DW_OP_reg31:
+		return op->atom - DW_OP_reg0;
+	case DW_OP_breg0 ... DW_OP_breg31:
+		return op->atom - DW_OP_breg0;
+	case DW_OP_regx:
+	case DW_OP_bregx:
+		return op->number;
+	default:
+		break;
+	}
+	return -1;
+}
+
+static int offset_from_dwarf_op(Dwarf_Op *op)
+{
+	switch (op->atom) {
+	case DW_OP_reg0 ... DW_OP_reg31:
+	case DW_OP_regx:
+		return 0;
+	case DW_OP_breg0 ... DW_OP_breg31:
+		return op->number;
+	case DW_OP_bregx:
+		return op->number2;
+	default:
+		break;
+	}
+	return -1;
+}
+#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT || HAVE_DWARF_CFI_SUPPORT */
+
 #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
 /**
  * die_get_var_innermost_scope - Get innermost scope range of given variable DIE
@@ -1476,41 +1510,69 @@ Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
 		*offset = data.offset;
 	return result;
 }
-#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
 
-#ifdef HAVE_DWARF_CFI_SUPPORT
-static int reg_from_dwarf_op(Dwarf_Op *op)
+static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
 {
-	switch (op->atom) {
-	case DW_OP_reg0 ... DW_OP_reg31:
-		return op->atom - DW_OP_reg0;
-	case DW_OP_breg0 ... DW_OP_breg31:
-		return op->atom - DW_OP_breg0;
-	case DW_OP_regx:
-	case DW_OP_bregx:
-		return op->number;
-	default:
-		break;
-	}
-	return -1;
+	struct die_var_type **var_types = arg;
+	Dwarf_Die type_die;
+	int tag = dwarf_tag(die_mem);
+	Dwarf_Attribute attr;
+	Dwarf_Addr base, start, end;
+	Dwarf_Op *ops;
+	size_t nops;
+	struct die_var_type *vt;
+
+	if (tag != DW_TAG_variable && tag != DW_TAG_formal_parameter)
+		return DIE_FIND_CB_SIBLING;
+
+	if (dwarf_attr(die_mem, DW_AT_location, &attr) == NULL)
+		return DIE_FIND_CB_SIBLING;
+
+	/*
+	 * Only collect the first location as it can reconstruct the
+	 * remaining state by following the instructions.
+	 * start = 0 means it covers the whole range.
+	 */
+	if (dwarf_getlocations(&attr, 0, &base, &start, &end, &ops, &nops) <= 0)
+		return DIE_FIND_CB_SIBLING;
+
+	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;
+
+	vt->die_off = dwarf_dieoffset(&type_die);
+	vt->addr = start;
+	vt->reg = reg_from_dwarf_op(ops);
+	vt->offset = offset_from_dwarf_op(ops);
+	vt->next = *var_types;
+	*var_types = vt;
+
+	return DIE_FIND_CB_SIBLING;
 }
 
-static int offset_from_dwarf_op(Dwarf_Op *op)
+/**
+ * die_collect_vars - Save all variables and parameters
+ * @sc_die: a scope DIE
+ * @var_types: a pointer to save the resulting list
+ *
+ * Save all variables and parameters in the @sc_die and save them to @var_types.
+ * The @var_types is a singly-linked list containing type and location info.
+ * Actual type can be retrieved using dwarf_offdie() with 'die_off' later.
+ *
+ * Callers should free @var_types.
+ */
+void die_collect_vars(Dwarf_Die *sc_die, struct die_var_type **var_types)
 {
-	switch (op->atom) {
-	case DW_OP_reg0 ... DW_OP_reg31:
-	case DW_OP_regx:
-		return 0;
-	case DW_OP_breg0 ... DW_OP_breg31:
-		return op->number;
-	case DW_OP_bregx:
-		return op->number2;
-	default:
-		break;
-	}
-	return -1;
+	Dwarf_Die die_mem;
+
+	die_find_child(sc_die, __die_collect_vars_cb, (void *)var_types, &die_mem);
 }
+#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
 
+#ifdef HAVE_DWARF_CFI_SUPPORT
 /**
  * die_get_cfa - Get frame base information
  * @dwarf: a Dwarf info
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 9973801a20c1..cd171b06fd4c 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -135,6 +135,15 @@ void die_skip_prologue(Dwarf_Die *sp_die, Dwarf_Die *cu_die,
 /* Get the list of including scopes */
 int die_get_scopes(Dwarf_Die *cu_die, Dwarf_Addr pc, Dwarf_Die **scopes);
 
+/* Variable type information */
+struct die_var_type {
+	struct die_var_type *next;
+	u64 die_off;
+	u64 addr;
+	int reg;
+	int offset;
+};
+
 #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
 
 /* Get byte offset range of given variable DIE */
@@ -149,6 +158,9 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
 Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
 				     Dwarf_Die *die_mem, int *offset);
 
+/* Save all variables and parameters in this scope */
+void die_collect_vars(Dwarf_Die *sc_die, struct die_var_type **var_types);
+
 #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
 
 static inline int die_get_var_range(Dwarf_Die *sp_die __maybe_unused,
@@ -176,6 +188,11 @@ static inline Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die __maybe_unu
 	return NULL;
 }
 
+static inline void die_collect_vars(Dwarf_Die *sc_die __maybe_unused,
+				    struct die_var_type **var_types __maybe_unused)
+{
+}
+
 #endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
 
 #ifdef HAVE_DWARF_CFI_SUPPORT
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 03/23] perf dwarf-aux: Handle type transfer for memory access

[Namhyung Kim]

We want to track type states as instructions are executed.  Each
instruction can access compound types like struct or union and load/
store its members to a different location.

The die_deref_ptr_type() is to find a type of memory access with a
pointer variable.  If it points to a compound type like struct, the
target memory is a member in the struct.  The access will happen
with an offset indicating which member it refers.  Let's follow the
DWARF info to figure out the type of the pointer target.

For example, say we have the following code.

  struct foo {
    int a;
    int b;
  };

  struct foo *p = malloc(sizeof(*p));
  p->b = 0;

The last pointer access should produce x86 asm like below:

  mov  0x0, 4(%rbx)

And we know %rbx register has a pointer to struct foo.  Then offset 4
should return the debug info of member 'b'.

Also variables of compound types can be accessed directly without a
pointer.  The die_get_member_type() is to handle a such case.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 110 ++++++++++++++++++++++++++++++++++++
 tools/perf/util/dwarf-aux.h |   6 ++
 2 files changed, 116 insertions(+)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 785aa7a3d725..cd9364d296b6 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1838,3 +1838,113 @@ int die_get_scopes(Dwarf_Die *cu_die, Dwarf_Addr pc, Dwarf_Die **scopes)
 	*scopes = data.scopes;
 	return data.nr;
 }
+
+static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
+{
+	Dwarf_Die type_die;
+	Dwarf_Word size, loc;
+	Dwarf_Word offset = (long)arg;
+	int tag = dwarf_tag(die_mem);
+
+	if (tag != DW_TAG_member)
+		return DIE_FIND_CB_SIBLING;
+
+	/* Unions might not have location */
+	if (die_get_data_member_location(die_mem, &loc) < 0)
+		loc = 0;
+
+	if (offset == loc)
+		return DIE_FIND_CB_END;
+
+	die_get_real_type(die_mem, &type_die);
+
+	if (dwarf_aggregate_size(&type_die, &size) < 0)
+		size = 0;
+
+	if (loc < offset && offset < (loc + size))
+		return DIE_FIND_CB_END;
+
+	return DIE_FIND_CB_SIBLING;
+}
+
+/**
+ * die_get_member_type - Return type info of struct member
+ * @type_die: a type DIE
+ * @offset: offset in the type
+ * @die_mem: a buffer to save the resulting DIE
+ *
+ * This function returns a type of a member in @type_die where it's located at
+ * @offset if it's a struct.  For now, it just returns the first matching
+ * member in a union.  For other types, it'd return the given type directly
+ * if it's within the size of the type or NULL otherwise.
+ */
+Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset,
+			       Dwarf_Die *die_mem)
+{
+	Dwarf_Die *member;
+	Dwarf_Die mb_type;
+	int tag;
+
+	tag = dwarf_tag(type_die);
+	/* If it's not a compound type, return the type directly */
+	if (tag != DW_TAG_structure_type && tag != DW_TAG_union_type) {
+		Dwarf_Word size;
+
+		if (dwarf_aggregate_size(type_die, &size) < 0)
+			size = 0;
+
+		if ((unsigned)offset >= size)
+			return NULL;
+
+		*die_mem = *type_die;
+		return die_mem;
+	}
+
+	mb_type = *type_die;
+	/* TODO: Handle union types better? */
+	while (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
+		member = die_find_child(&mb_type, __die_find_member_offset_cb,
+					(void *)(long)offset, die_mem);
+		if (member == NULL)
+			return NULL;
+
+		if (die_get_real_type(member, &mb_type) == NULL)
+			return NULL;
+
+		tag = dwarf_tag(&mb_type);
+
+		if (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
+			Dwarf_Word loc;
+
+			/* Update offset for the start of the member struct */
+			if (die_get_data_member_location(member, &loc) == 0)
+				offset -= loc;
+		}
+	}
+	*die_mem = mb_type;
+	return die_mem;
+}
+
+/**
+ * die_deref_ptr_type - Return type info for pointer access
+ * @ptr_die: a pointer type DIE
+ * @offset: access offset for the pointer
+ * @die_mem: a buffer to save the resulting DIE
+ *
+ * This function follows the pointer in @ptr_die with given @offset
+ * and saves the resulting type in @die_mem.  If the pointer points
+ * a struct type, actual member at the offset would be returned.
+ */
+Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset,
+			      Dwarf_Die *die_mem)
+{
+	Dwarf_Die type_die;
+
+	if (dwarf_tag(ptr_die) != DW_TAG_pointer_type)
+		return NULL;
+
+	if (die_get_real_type(ptr_die, &type_die) == NULL)
+		return NULL;
+
+	return die_get_member_type(&type_die, offset, die_mem);
+}
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index cd171b06fd4c..16c916311bc0 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -144,6 +144,12 @@ struct die_var_type {
 	int offset;
 };
 
+/* Return type info of a member at offset */
+Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset, Dwarf_Die *die_mem);
+
+/* 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);
+
 #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
 
 /* Get byte offset range of given variable DIE */
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 04/23] perf dwarf-aux: Add die_find_func_rettype()

[Namhyung Kim]

The die_find_func_rettype() is to find a debug entry for the given
function name and sets the type information of the return value.  By
convention, it'd return the pointer to the type die (should be the
same as the given mem_die argument) if found, or NULL otherwise.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dwarf-aux.c | 43 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/dwarf-aux.h |  4 ++++
 2 files changed, 47 insertions(+)

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index cd9364d296b6..9080119a258c 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -696,6 +696,49 @@ Dwarf_Die *die_find_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
 	return die_mem;
 }
 
+static int __die_find_func_rettype_cb(Dwarf_Die *die_mem, void *data)
+{
+	const char *func_name;
+
+	if (dwarf_tag(die_mem) != DW_TAG_subprogram)
+		return DIE_FIND_CB_SIBLING;
+
+	func_name = dwarf_diename(die_mem);
+	if (func_name && !strcmp(func_name, data))
+		return DIE_FIND_CB_END;
+
+	return DIE_FIND_CB_SIBLING;
+}
+
+/**
+ * die_find_func_rettype - Search a return type of function
+ * @cu_die: a CU DIE
+ * @name: target function name
+ * @die_mem: a buffer for result DIE
+ *
+ * Search a non-inlined function which matches to @name and stores the
+ * return type of the function to @die_mem and returns it if found.
+ * Returns NULL if failed.  Note that it doesn't needs to find a
+ * definition of the function, so it doesn't match with address.
+ * Most likely, it can find a declaration at the top level.  Thus the
+ * callback function continues to sibling entries only.
+ */
+Dwarf_Die *die_find_func_rettype(Dwarf_Die *cu_die, const char *name,
+				 Dwarf_Die *die_mem)
+{
+	Dwarf_Die tmp_die;
+
+	cu_die = die_find_child(cu_die, __die_find_func_rettype_cb,
+				(void *)name, &tmp_die);
+	if (!cu_die)
+		return NULL;
+
+	if (die_get_real_type(&tmp_die, die_mem) == NULL)
+		return NULL;
+
+	return die_mem;
+}
+
 struct __instance_walk_param {
 	void    *addr;
 	int	(*callback)(Dwarf_Die *, void *);
diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
index 16c916311bc0..b0f25fbf9668 100644
--- a/tools/perf/util/dwarf-aux.h
+++ b/tools/perf/util/dwarf-aux.h
@@ -94,6 +94,10 @@ Dwarf_Die *die_find_top_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
 Dwarf_Die *die_find_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
 			       Dwarf_Die *die_mem);
 
+/* Search a non-inlined function by name and returns its return type */
+Dwarf_Die *die_find_func_rettype(Dwarf_Die *sp_die, const char *name,
+				 Dwarf_Die *die_mem);
+
 /* Walk on the instances of given DIE */
 int die_walk_instances(Dwarf_Die *in_die,
 		       int (*callback)(Dwarf_Die *, void *), void *data);
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 05/23] perf map: Add map__objdump_2rip()

[Namhyung Kim]

Sometimes we want to convert an address in objdump output to
map-relative address to match with a sample data.  Let's add
map__objdump_2rip() for that.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/map.c | 17 +++++++++++++++++
 tools/perf/util/map.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 14a5ea70d81e..a5d57c201a30 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -587,6 +587,23 @@ u64 map__objdump_2mem(struct map *map, u64 ip)
 	return ip + map__reloc(map);
 }
 
+/* convert objdump address to relative address.  (To be removed) */
+u64 map__objdump_2rip(struct map *map, u64 ip)
+{
+	const struct dso *dso = map__dso(map);
+
+	if (!dso->adjust_symbols)
+		return ip;
+
+	if (dso->rel)
+		return ip + map__pgoff(map);
+
+	if (dso->kernel == DSO_SPACE__USER)
+		return ip - dso->text_offset;
+
+	return map__map_ip(map, ip + map__reloc(map));
+}
+
 bool map__contains_symbol(const struct map *map, const struct symbol *sym)
 {
 	u64 ip = map__unmap_ip(map, sym->start);
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 49756716cb13..65e2609fa1b1 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -132,6 +132,9 @@ u64 map__rip_2objdump(struct map *map, u64 rip);
 /* objdump address -> memory address */
 u64 map__objdump_2mem(struct map *map, u64 ip);
 
+/* objdump address -> rip */
+u64 map__objdump_2rip(struct map *map, u64 ip);
+
 struct symbol;
 struct thread;
 
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 06/23] perf annotate-data: Introduce struct data_loc_info

[Namhyung Kim]

The find_data_type() needs many information to describe the location of
the data.  Add the new struct data_loc_info to pass those information at
once.

No functional changes intended.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 83 +++++++++++++++++----------------
 tools/perf/util/annotate-data.h | 38 ++++++++++++---
 tools/perf/util/annotate.c      | 30 ++++++------
 3 files changed, 91 insertions(+), 60 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 59ce5f4f4a40..ff81d164aa57 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -239,21 +239,28 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
 }
 
 /* The result will be saved in @type_die */
-static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
-			      const char *var_name, struct annotated_op_loc *loc,
-			      Dwarf_Die *type_die)
+static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 {
+	struct annotated_op_loc *loc = dloc->op;
 	Dwarf_Die cu_die, var_die;
 	Dwarf_Die *scopes = NULL;
 	int reg, offset;
 	int ret = -1;
 	int i, nr_scopes;
 	int fbreg = -1;
-	bool is_fbreg = false;
 	int fb_offset = 0;
+	bool is_fbreg = false;
+	u64 pc;
+
+	/*
+	 * IP is a relative instruction address from the start of the map, as
+	 * it can be randomized/relocated, it needs to translate to PC which is
+	 * a file address for DWARF processing.
+	 */
+	pc = map__rip_2objdump(dloc->ms->map, dloc->ip);
 
 	/* Get a compile_unit for this address */
-	if (!find_cu_die(di, pc, &cu_die)) {
+	if (!find_cu_die(dloc->di, pc, &cu_die)) {
 		pr_debug("cannot find CU for address %" PRIx64 "\n", pc);
 		ann_data_stat.no_cuinfo++;
 		return -1;
@@ -263,18 +270,19 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 	offset = loc->offset;
 
 	if (reg == DWARF_REG_PC) {
-		if (die_find_variable_by_addr(&cu_die, addr, &var_die, &offset)) {
+		if (die_find_variable_by_addr(&cu_die, dloc->var_addr, &var_die,
+					      &offset)) {
 			ret = check_variable(&var_die, type_die, offset,
 					     /*is_pointer=*/false);
-			loc->offset = offset;
+			dloc->type_offset = offset;
 			goto out;
 		}
 
-		if (var_name && die_find_variable_at(&cu_die, var_name, pc,
-						     &var_die)) {
-			ret = check_variable(&var_die, type_die, 0,
+		if (dloc->var_name &&
+		    die_find_variable_at(&cu_die, dloc->var_name, pc, &var_die)) {
+			ret = check_variable(&var_die, type_die, dloc->type_offset,
 					     /*is_pointer=*/false);
-			/* loc->offset will be updated by the caller */
+			/* dloc->type_offset was updated by the caller */
 			goto out;
 		}
 	}
@@ -291,10 +299,11 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 		    dwarf_formblock(&attr, &block) == 0 && block.length == 1) {
 			switch (*block.data) {
 			case DW_OP_reg0 ... DW_OP_reg31:
-				fbreg = *block.data - DW_OP_reg0;
+				fbreg = dloc->fbreg = *block.data - DW_OP_reg0;
 				break;
 			case DW_OP_call_frame_cfa:
-				if (die_get_cfa(di->dbg, pc, &fbreg,
+				dloc->fb_cfa = true;
+				if (die_get_cfa(dloc->di->dbg, pc, &fbreg,
 						&fb_offset) < 0)
 					fbreg = -1;
 				break;
@@ -312,7 +321,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 	/* Search from the inner-most scope to the outer */
 	for (i = nr_scopes - 1; i >= 0; i--) {
 		if (reg == DWARF_REG_PC) {
-			if (!die_find_variable_by_addr(&scopes[i], addr,
+			if (!die_find_variable_by_addr(&scopes[i], dloc->var_addr,
 						       &var_die, &offset))
 				continue;
 		} else {
@@ -325,7 +334,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 		/* Found a variable, see if it's correct */
 		ret = check_variable(&var_die, type_die, offset,
 				     reg != DWARF_REG_PC && !is_fbreg);
-		loc->offset = offset;
+		dloc->type_offset = offset;
 		goto out;
 	}
 
@@ -344,50 +353,46 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
 
 /**
  * find_data_type - Return a data type at the location
- * @ms: map and symbol at the location
- * @ip: instruction address of the memory access
- * @loc: instruction operand location
- * @addr: data address of the memory access
- * @var_name: global variable name
+ * @dloc: data location
  *
  * This functions searches the debug information of the binary to get the data
- * type it accesses.  The exact location is expressed by (@ip, reg, offset)
- * for pointer variables or (@ip, @addr) for global variables.  Note that global
- * variables might update the @loc->offset after finding the start of the variable.
- * If it cannot find a global variable by address, it tried to fine a declaration
- * of the variable using @var_name.  In that case, @loc->offset won't be updated.
+ * type it accesses.  The exact location is expressed by (ip, reg, offset)
+ * for pointer variables or (ip, addr) for global variables.  Note that global
+ * variables might update the @dloc->type_offset after finding the start of the
+ * variable.  If it cannot find a global variable by address, it tried to find
+ * a declaration of the variable using var_name.  In that case, @dloc->offset
+ * won't be updated.
  *
  * It return %NULL if not found.
  */
-struct annotated_data_type *find_data_type(struct map_symbol *ms, u64 ip,
-					   struct annotated_op_loc *loc, u64 addr,
-					   const char *var_name)
+struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
 {
 	struct annotated_data_type *result = NULL;
-	struct dso *dso = map__dso(ms->map);
-	struct debuginfo *di;
+	struct dso *dso = map__dso(dloc->ms->map);
 	Dwarf_Die type_die;
-	u64 pc;
 
-	di = debuginfo__new(dso->long_name);
-	if (di == NULL) {
+	dloc->di = debuginfo__new(dso->long_name);
+	if (dloc->di == NULL) {
 		pr_debug("cannot get the debug info\n");
 		return NULL;
 	}
 
 	/*
-	 * IP is a relative instruction address from the start of the map, as
-	 * it can be randomized/relocated, it needs to translate to PC which is
-	 * a file address for DWARF processing.
+	 * The type offset is the same as instruction offset by default.
+	 * But when finding a global variable, the offset won't be valid.
 	 */
-	pc = map__rip_2objdump(ms->map, ip);
-	if (find_data_type_die(di, pc, addr, var_name, loc, &type_die) < 0)
+	if (dloc->var_name == NULL)
+		dloc->type_offset = dloc->op->offset;
+
+	dloc->fbreg = -1;
+
+	if (find_data_type_die(dloc, &type_die) < 0)
 		goto out;
 
 	result = dso__findnew_data_type(dso, &type_die);
 
 out:
-	debuginfo__delete(di);
+	debuginfo__delete(dloc->di);
 	return result;
 }
 
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 1b0db8e8c40e..ad6493ea2c8e 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 
 struct annotated_op_loc;
+struct debuginfo;
 struct evsel;
 struct map_symbol;
 
@@ -72,6 +73,35 @@ struct annotated_data_type {
 extern struct annotated_data_type unknown_type;
 extern struct annotated_data_type stackop_type;
 
+/**
+ * struct data_loc_info - Data location information
+ * @ms: Map and Symbol info
+ * @ip: Instruction address
+ * @var_addr: Data address (for global variables)
+ * @var_name: Variable name (for global variables)
+ * @op: Instruction operand location (regs and offset)
+ * @di: Debug info
+ * @fbreg: Frame base register
+ * @fb_cfa: Whether the frame needs to check CFA
+ * @type_offset: Final offset in the type
+ */
+struct data_loc_info {
+	/* These are input field, should be filled by caller */
+	struct map_symbol *ms;
+	u64 ip;
+	u64 var_addr;
+	const char *var_name;
+	struct annotated_op_loc *op;
+
+	/* These are used internally */
+	struct debuginfo *di;
+	int fbreg;
+	bool fb_cfa;
+
+	/* This is for the result */
+	int type_offset;
+};
+
 /**
  * struct annotated_data_stat - Debug statistics
  * @total: Total number of entry
@@ -106,9 +136,7 @@ extern struct annotated_data_stat ann_data_stat;
 #ifdef HAVE_DWARF_SUPPORT
 
 /* Returns data type at the location (ip, reg, offset) */
-struct annotated_data_type *find_data_type(struct map_symbol *ms, u64 ip,
-					   struct annotated_op_loc *loc, u64 addr,
-					   const char *var_name);
+struct annotated_data_type *find_data_type(struct data_loc_info *dloc);
 
 /* Update type access histogram at the given offset */
 int annotated_data_type__update_samples(struct annotated_data_type *adt,
@@ -121,9 +149,7 @@ void annotated_data_type__tree_delete(struct rb_root *root);
 #else /* HAVE_DWARF_SUPPORT */
 
 static inline struct annotated_data_type *
-find_data_type(struct map_symbol *ms __maybe_unused, u64 ip __maybe_unused,
-	       struct annotated_op_loc *loc __maybe_unused,
-	       u64 addr __maybe_unused, const char *var_name __maybe_unused)
+find_data_type(struct data_loc_info *dloc __maybe_unused)
 {
 	return NULL;
 }
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index ac002d907d81..a15ff6758210 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3816,9 +3816,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 	struct annotated_op_loc *op_loc;
 	struct annotated_data_type *mem_type;
 	struct annotated_item_stat *istat;
-	u64 ip = he->ip, addr = 0;
-	const char *var_name = NULL;
-	int var_offset;
+	u64 ip = he->ip;
 	int i;
 
 	ann_data_stat.total++;
@@ -3871,51 +3869,53 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 	}
 
 	for_each_insn_op_loc(&loc, i, op_loc) {
+		struct data_loc_info dloc = {
+			.ms = ms,
+			/* Recalculate IP for LOCK prefix or insn fusion */
+			.ip = ms->sym->start + dl->al.offset,
+			.op = op_loc,
+		};
+
 		if (!op_loc->mem_ref)
 			continue;
 
 		/* Recalculate IP because of LOCK prefix or insn fusion */
 		ip = ms->sym->start + dl->al.offset;
 
-		var_offset = op_loc->offset;
-
 		/* PC-relative addressing */
 		if (op_loc->reg1 == DWARF_REG_PC) {
 			struct addr_location al;
 			struct symbol *var;
 			u64 map_addr;
 
-			addr = annotate_calc_pcrel(ms, ip, op_loc->offset, dl);
+			dloc.var_addr = annotate_calc_pcrel(ms, ip, op_loc->offset, dl);
 			/* Kernel symbols might be relocated */
-			map_addr = addr + map__reloc(ms->map);
+			map_addr = dloc.var_addr + map__reloc(ms->map);
 
 			addr_location__init(&al);
 			var = thread__find_symbol_fb(he->thread, he->cpumode,
 						     map_addr, &al);
 			if (var) {
-				var_name = var->name;
+				dloc.var_name = var->name;
 				/* Calculate type offset from the start of variable */
-				var_offset = map_addr - map__unmap_ip(al.map, var->start);
+				dloc.type_offset = map_addr - map__unmap_ip(al.map, var->start);
 			}
 			addr_location__exit(&al);
 		}
 
-		mem_type = find_data_type(ms, ip, op_loc, addr, var_name);
+		mem_type = find_data_type(&dloc);
 		if (mem_type)
 			istat->good++;
 		else
 			istat->bad++;
 
-		if (mem_type && var_name)
-			op_loc->offset = var_offset;
-
 		if (symbol_conf.annotate_data_sample) {
 			annotated_data_type__update_samples(mem_type, evsel,
-							    op_loc->offset,
+							    dloc.type_offset,
 							    he->stat.nr_events,
 							    he->stat.period);
 		}
-		he->mem_type_off = op_loc->offset;
+		he->mem_type_off = dloc.type_offset;
 		return mem_type;
 	}
 
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 07/23] perf annotate: Add annotate_get_basic_blocks()

[Namhyung Kim]

The annotate_get_basic_blocks() is to find a list of basic blocks from
the source instruction to the destination instruction in a function.

It'll be used to find variables in a scope.  Use BFS (Breadth First
Search) to find a shortest path to carry the variable/register state
minimally.

Also change find_disasm_line() to be used in annotate_get_basic_blocks()
and add 'allow_update' argument to control if it can update the IP.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 222 ++++++++++++++++++++++++++++++++++++-
 tools/perf/util/annotate.h |  16 +++
 2 files changed, 235 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a15ff6758210..aa005c13ff67 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3714,7 +3714,8 @@ static void symbol__ensure_annotate(struct map_symbol *ms, struct evsel *evsel)
 	}
 }
 
-static struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip)
+static struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip,
+					    bool allow_update)
 {
 	struct disasm_line *dl;
 	struct annotation *notes;
@@ -3727,7 +3728,8 @@ static struct disasm_line *find_disasm_line(struct symbol *sym, u64 ip)
 			 * llvm-objdump places "lock" in a separate line and
 			 * in that case, we want to get the next line.
 			 */
-			if (!strcmp(dl->ins.name, "lock") && *dl->ops.raw == '\0') {
+			if (!strcmp(dl->ins.name, "lock") &&
+			    *dl->ops.raw == '\0' && allow_update) {
 				ip++;
 				continue;
 			}
@@ -3843,7 +3845,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 	 * Get a disasm to extract the location from the insn.
 	 * This is too slow...
 	 */
-	dl = find_disasm_line(ms->sym, ip);
+	dl = find_disasm_line(ms->sym, ip, /*allow_update=*/true);
 	if (dl == NULL) {
 		ann_data_stat.no_insn++;
 		return NULL;
@@ -3937,3 +3939,217 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 	istat->bad++;
 	return NULL;
 }
+
+/* Basic block traversal (BFS) data structure */
+struct basic_block_data {
+	struct list_head queue;
+	struct list_head visited;
+};
+
+/*
+ * During the traversal, it needs to know the parent block where the current
+ * block block started from.  Note that single basic block can be parent of
+ * two child basic blocks (in case of condition jump).
+ */
+struct basic_block_link {
+	struct list_head node;
+	struct basic_block_link *parent;
+	struct annotated_basic_block *bb;
+};
+
+/* Check any of basic block in the list already has the offset */
+static bool basic_block_has_offset(struct list_head *head, s64 offset)
+{
+	struct basic_block_link *link;
+
+	list_for_each_entry(link, head, node) {
+		s64 begin_offset = link->bb->begin->al.offset;
+		s64 end_offset = link->bb->end->al.offset;
+
+		if (begin_offset <= offset && offset <= end_offset)
+			return true;
+	}
+	return false;
+}
+
+static bool is_new_basic_block(struct basic_block_data *bb_data,
+			       struct disasm_line *dl)
+{
+	s64 offset = dl->al.offset;
+
+	if (basic_block_has_offset(&bb_data->visited, offset))
+		return false;
+	if (basic_block_has_offset(&bb_data->queue, offset))
+		return false;
+	return true;
+}
+
+/* Add a basic block starting from dl and link it to the parent */
+static int add_basic_block(struct basic_block_data *bb_data,
+			   struct basic_block_link *parent,
+			   struct disasm_line *dl)
+{
+	struct annotated_basic_block *bb;
+	struct basic_block_link *link;
+
+	if (dl == NULL)
+		return -1;
+
+	if (!is_new_basic_block(bb_data, dl))
+		return 0;
+
+	bb = zalloc(sizeof(*bb));
+	if (bb == NULL)
+		return -1;
+
+	bb->begin = dl;
+	bb->end = dl;
+	INIT_LIST_HEAD(&bb->list);
+
+	link = malloc(sizeof(*link));
+	if (link == NULL) {
+		free(bb);
+		return -1;
+	}
+
+	link->bb = bb;
+	link->parent = parent;
+	list_add_tail(&link->node, &bb_data->queue);
+	return 0;
+}
+
+/* Returns true when it finds the target in the current basic block */
+static bool process_basic_block(struct basic_block_data *bb_data,
+				struct basic_block_link *link,
+				struct symbol *sym, u64 target)
+{
+	struct disasm_line *dl, *next_dl, *last_dl;
+	struct annotation *notes = symbol__annotation(sym);
+	bool found = false;
+
+	dl = link->bb->begin;
+	/* Check if it's already visited */
+	if (basic_block_has_offset(&bb_data->visited, dl->al.offset))
+		return false;
+
+	last_dl = list_last_entry(&notes->src->source,
+				  struct disasm_line, al.node);
+
+	list_for_each_entry_from(dl, &notes->src->source, al.node) {
+		/* Found the target instruction */
+		if (sym->start + dl->al.offset == target) {
+			found = true;
+			break;
+		}
+		/* End of the function, finish the block */
+		if (dl == last_dl)
+			break;
+		/* 'return' instruction finishes the block */
+		if (dl->ins.ops == &ret_ops)
+			break;
+		/* normal instructions are part of the basic block */
+		if (dl->ins.ops != &jump_ops)
+			continue;
+		/* jump to a different function, tail call or return */
+		if (dl->ops.target.outside)
+			break;
+		/* jump instruction creates new basic block(s) */
+		next_dl = find_disasm_line(sym, sym->start + dl->ops.target.offset,
+					   /*allow_update=*/false);
+		add_basic_block(bb_data, link, next_dl);
+
+		/*
+		 * FIXME: determine conditional jumps properly.
+		 * Conditional jumps create another basic block with the
+		 * next disasm line.
+		 */
+		if (!strstr(dl->ins.name, "jmp")) {
+			next_dl = list_next_entry(dl, al.node);
+			add_basic_block(bb_data, link, next_dl);
+		}
+		break;
+
+	}
+	link->bb->end = dl;
+	return found;
+}
+
+/*
+ * It founds a target basic block, build a proper linked list of basic blocks
+ * by following the link recursively.
+ */
+static void link_found_basic_blocks(struct basic_block_link *link,
+				    struct list_head *head)
+{
+	while (link) {
+		struct basic_block_link *parent = link->parent;
+
+		list_move(&link->bb->list, head);
+		list_del(&link->node);
+		free(link);
+
+		link = parent;
+	}
+}
+
+static void delete_basic_blocks(struct basic_block_data *bb_data)
+{
+	struct basic_block_link *link, *tmp;
+
+	list_for_each_entry_safe(link, tmp, &bb_data->queue, node) {
+		list_del(&link->node);
+		free(link->bb);
+		free(link);
+	}
+
+	list_for_each_entry_safe(link, tmp, &bb_data->visited, node) {
+		list_del(&link->node);
+		free(link->bb);
+		free(link);
+	}
+}
+
+/**
+ * annotate_get_basic_blocks - Get basic blocks for given address range
+ * @sym: symbol to annotate
+ * @src: source address
+ * @dst: destination address
+ * @head: list head to save basic blocks
+ *
+ * This function traverses disasm_lines from @src to @dst and save them in a
+ * list of annotated_basic_block to @head.  It uses BFS to find the shortest
+ * path between two.  The basic_block_link is to maintain parent links so
+ * that it can build a list of blocks from the start.
+ */
+int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
+			      struct list_head *head)
+{
+	struct basic_block_data bb_data = {
+		.queue = LIST_HEAD_INIT(bb_data.queue),
+		.visited = LIST_HEAD_INIT(bb_data.visited),
+	};
+	struct basic_block_link *link;
+	struct disasm_line *dl;
+	int ret = -1;
+
+	dl = find_disasm_line(sym, src, /*allow_update=*/false);
+	if (dl == NULL)
+		return -1;
+
+	if (add_basic_block(&bb_data, /*parent=*/NULL, dl) < 0)
+		return -1;
+
+	/* Find shortest path from src to dst using BFS */
+	while (!list_empty(&bb_data.queue)) {
+		link = list_first_entry(&bb_data.queue, struct basic_block_link, node);
+
+		if (process_basic_block(&bb_data, link, sym, dst)) {
+			link_found_basic_blocks(link, head);
+			ret = 0;
+			break;
+		}
+		list_move(&link->node, &bb_data.visited);
+	}
+	delete_basic_blocks(&bb_data);
+	return ret;
+}
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 13cc659e508c..0928663fddee 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -561,4 +561,20 @@ extern struct list_head ann_insn_stat;
 u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
 			struct disasm_line *dl);
 
+/**
+ * struct annotated_basic_block - Basic block of instructions
+ * @list: List node
+ * @begin: start instruction in the block
+ * @end: end instruction in the block
+ */
+struct annotated_basic_block {
+	struct list_head list;
+	struct disasm_line *begin;
+	struct disasm_line *end;
+};
+
+/* Get a list of basic blocks from src to dst addresses */
+int annotate_get_basic_blocks(struct symbol *sym, s64 src, s64 dst,
+			      struct list_head *head);
+
 #endif	/* __PERF_ANNOTATE_H */
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 08/23] perf annotate-data: Add debug messages

[Namhyung Kim]

Add a new debug option "type-profile" to enable the detailed info during
the type analysis especially for instruction tracking.  You can use this
before the command name like 'report' or 'annotate'.

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

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 74 +++++++++++++++++++++++++++++----
 tools/perf/util/debug.c         |  3 ++
 tools/perf/util/debug.h         |  1 +
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index ff81d164aa57..f482ccfdaa91 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -23,6 +23,29 @@
 #include "symbol.h"
 #include "symbol_conf.h"
 
+#define pr_debug_dtp(fmt, ...)					\
+do {								\
+	if (debug_type_profile)					\
+		pr_info(fmt, ##__VA_ARGS__);			\
+	else							\
+		pr_debug3(fmt, ##__VA_ARGS__);			\
+} while (0)
+
+static void pr_debug_type_name(Dwarf_Die *die)
+{
+	struct strbuf sb;
+	char *str;
+
+	if (!debug_type_profile && verbose < 3)
+		return;
+
+	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));
+	free(str);
+}
+
 /*
  * Compare type name and size to maintain them in a tree.
  * I'm not sure if DWARF would have information of a single type in many
@@ -201,7 +224,7 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
 
 	/* Get the type of the variable */
 	if (die_get_real_type(var_die, type_die) == NULL) {
-		pr_debug("variable has no type\n");
+		pr_debug_dtp("variable has no type\n");
 		ann_data_stat.no_typeinfo++;
 		return -1;
 	}
@@ -215,7 +238,7 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
 		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("no pointer or no type\n");
+			pr_debug_dtp("no pointer or no type\n");
 			ann_data_stat.no_typeinfo++;
 			return -1;
 		}
@@ -223,14 +246,15 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
 
 	/* Get the size of the actual type */
 	if (dwarf_aggregate_size(type_die, &size) < 0) {
-		pr_debug("type size is unknown\n");
+		pr_debug_dtp("type size is unknown\n");
 		ann_data_stat.invalid_size++;
 		return -1;
 	}
 
 	/* Minimal sanity check */
 	if ((unsigned)offset >= size) {
-		pr_debug("offset: %d is bigger than size: %" PRIu64 "\n", offset, size);
+		pr_debug_dtp("offset: %d is bigger than size: %"PRIu64"\n",
+			     offset, size);
 		ann_data_stat.bad_offset++;
 		return -1;
 	}
@@ -251,6 +275,19 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	int fb_offset = 0;
 	bool is_fbreg = false;
 	u64 pc;
+	char buf[64];
+
+	if (dloc->op->multi_regs)
+		snprintf(buf, sizeof(buf), " or reg%d", dloc->op->reg2);
+	else if (dloc->op->reg1 == DWARF_REG_PC)
+		snprintf(buf, sizeof(buf), " (PC)");
+	else
+		buf[0] = '\0';
+
+	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);
 
 	/*
 	 * IP is a relative instruction address from the start of the map, as
@@ -261,7 +298,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 
 	/* Get a compile_unit for this address */
 	if (!find_cu_die(dloc->di, pc, &cu_die)) {
-		pr_debug("cannot find CU for address %" PRIx64 "\n", pc);
+		pr_debug_dtp("cannot find CU for address %"PRIx64"\n", pc);
 		ann_data_stat.no_cuinfo++;
 		return -1;
 	}
@@ -269,12 +306,17 @@ 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));
+
 	if (reg == DWARF_REG_PC) {
 		if (die_find_variable_by_addr(&cu_die, dloc->var_addr, &var_die,
 					      &offset)) {
 			ret = check_variable(&var_die, type_die, offset,
 					     /*is_pointer=*/false);
 			dloc->type_offset = offset;
+
+			pr_debug_dtp("found PC-rel by addr=%#"PRIx64" offset=%#x\n",
+				     dloc->var_addr, offset);
 			goto out;
 		}
 
@@ -310,6 +352,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 			default:
 				break;
 			}
+
+			pr_debug_dtp("frame base: cfa=%d fbreg=%d\n",
+				     dloc->fb_cfa, fbreg);
 		}
 	}
 
@@ -334,6 +379,19 @@ 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);
+		if (ret == 0) {
+			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);
+			pr_debug_type_name(type_die);
+		}
 		dloc->type_offset = offset;
 		goto out;
 	}
@@ -343,8 +401,10 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 		goto retry;
 	}
 
-	if (ret < 0)
+	if (ret < 0) {
+		pr_debug_dtp("no variable found\n");
 		ann_data_stat.no_var++;
+	}
 
 out:
 	free(scopes);
@@ -373,7 +433,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
 
 	dloc->di = debuginfo__new(dso->long_name);
 	if (dloc->di == NULL) {
-		pr_debug("cannot get the debug info\n");
+		pr_debug_dtp("cannot get the debug info\n");
 		return NULL;
 	}
 
diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index c39ee0fcb8cf..d633d15329fa 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -41,6 +41,7 @@ static int redirect_to_stderr;
 int debug_data_convert;
 static FILE *_debug_file;
 bool debug_display_time;
+int debug_type_profile;
 
 FILE *debug_file(void)
 {
@@ -231,6 +232,7 @@ static struct sublevel_option debug_opts[] = {
 	{ .name = "data-convert",	.value_ptr = &debug_data_convert },
 	{ .name = "perf-event-open",	.value_ptr = &debug_peo_args },
 	{ .name = "kmaps",		.value_ptr = &debug_kmaps },
+	{ .name = "type-profile",	.value_ptr = &debug_type_profile },
 	{ .name = NULL, }
 };
 
@@ -270,6 +272,7 @@ int perf_quiet_option(void)
 	redirect_to_stderr = 0;
 	debug_peo_args = 0;
 	debug_kmaps = 0;
+	debug_type_profile = 0;
 
 	return 0;
 }
diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
index 35a7a5ae762e..a4026d1fd6a3 100644
--- a/tools/perf/util/debug.h
+++ b/tools/perf/util/debug.h
@@ -14,6 +14,7 @@ extern int debug_peo_args;
 extern bool quiet, dump_trace;
 extern int debug_ordered_events;
 extern int debug_data_convert;
+extern int debug_type_profile;
 
 #ifndef pr_fmt
 #define pr_fmt(fmt) fmt
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 09/23] perf annotate-data: Maintain variable type info

[Namhyung Kim]

As it collected basic block and variable information in each scope, it
now can build a state table to find matching variable at the location.

The struct type_state is to keep the type info saved in each register
and stack slot.  The update_var_state() updates the table when it finds
variables in the current address.  It expects die_collect_vars() filled
a list of variables with type info and starting address.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 173 ++++++++++++++++++++++++++++++++
 tools/perf/util/dwarf-aux.c     |   4 +
 2 files changed, 177 insertions(+)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index f482ccfdaa91..8eaa06f1cee5 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -46,6 +46,62 @@ static void pr_debug_type_name(Dwarf_Die *die)
 	free(str);
 }
 
+/* Type information in a register, valid when ok is true */
+struct type_state_reg {
+	Dwarf_Die type;
+	bool ok;
+};
+
+/* Type information in a stack location, dynamically allocated */
+struct type_state_stack {
+	struct list_head list;
+	Dwarf_Die type;
+	int offset;
+	int size;
+	bool compound;
+};
+
+/* FIXME: This should be arch-dependent */
+#define TYPE_STATE_MAX_REGS  16
+
+/*
+ * State table to maintain type info in each register and stack location.
+ * It'll be updated when new variable is allocated or type info is moved
+ * to a new location (register or stack).  As it'd be used with the
+ * shortest path of basic blocks, it only maintains a single table.
+ */
+struct type_state {
+	struct type_state_reg regs[TYPE_STATE_MAX_REGS];
+	struct list_head stack_vars;
+};
+
+static bool has_reg_type(struct type_state *state, int reg)
+{
+	return (unsigned)reg < ARRAY_SIZE(state->regs);
+}
+
+/* These declarations will be remove once they are changed to static */
+void init_type_state(struct type_state *state, struct arch *arch __maybe_unused);
+void exit_type_state(struct type_state *state);
+void update_var_state(struct type_state *state, struct data_loc_info *dloc,
+		      u64 addr, u64 insn_offset, struct die_var_type *var_types);
+
+void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
+{
+	memset(state, 0, sizeof(*state));
+	INIT_LIST_HEAD(&state->stack_vars);
+}
+
+void exit_type_state(struct type_state *state)
+{
+	struct type_state_stack *stack, *tmp;
+
+	list_for_each_entry_safe(stack, tmp, &state->stack_vars, list) {
+		list_del(&stack->list);
+		free(stack);
+	}
+}
+
 /*
  * Compare type name and size to maintain them in a tree.
  * I'm not sure if DWARF would have information of a single type in many
@@ -262,6 +318,123 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
 	return 0;
 }
 
+static struct type_state_stack *find_stack_state(struct type_state *state,
+						 int offset)
+{
+	struct type_state_stack *stack;
+
+	list_for_each_entry(stack, &state->stack_vars, list) {
+		if (offset == stack->offset)
+			return stack;
+
+		if (stack->compound && stack->offset < offset &&
+		    offset < stack->offset + stack->size)
+			return stack;
+	}
+	return NULL;
+}
+
+static void set_stack_state(struct type_state_stack *stack, int offset,
+			    Dwarf_Die *type_die)
+{
+	int tag;
+	Dwarf_Word size;
+
+	if (dwarf_aggregate_size(type_die, &size) < 0)
+		size = 0;
+
+	tag = dwarf_tag(type_die);
+
+	stack->type = *type_die;
+	stack->size = size;
+	stack->offset = offset;
+
+	switch (tag) {
+	case DW_TAG_structure_type:
+	case DW_TAG_union_type:
+		stack->compound = true;
+		break;
+	default:
+		stack->compound = false;
+		break;
+	}
+}
+
+static struct type_state_stack *findnew_stack_state(struct type_state *state,
+						    int offset, Dwarf_Die *type_die)
+{
+	struct type_state_stack *stack = find_stack_state(state, offset);
+
+	if (stack) {
+		set_stack_state(stack, offset, type_die);
+		return stack;
+	}
+
+	stack = malloc(sizeof(*stack));
+	if (stack) {
+		set_stack_state(stack, offset, type_die);
+		list_add(&stack->list, &state->stack_vars);
+	}
+	return stack;
+}
+
+/**
+ * update_var_state - Update type state using given variables
+ * @state: type state table
+ * @dloc: data location info
+ * @addr: instruction address to match with variable
+ * @insn_offset: instruction offset (for debug)
+ * @var_types: list of variables with type info
+ *
+ * This function fills the @state table using @var_types info.  Each variable
+ * is used only at the given location and updates an entry in the table.
+ */
+void update_var_state(struct type_state *state, struct data_loc_info *dloc,
+		      u64 addr, u64 insn_offset, struct die_var_type *var_types)
+{
+	Dwarf_Die mem_die;
+	struct die_var_type *var;
+	int fbreg = dloc->fbreg;
+	int fb_offset = 0;
+
+	if (dloc->fb_cfa) {
+		if (die_get_cfa(dloc->di->dbg, addr, &fbreg, &fb_offset) < 0)
+			fbreg = -1;
+	}
+
+	for (var = var_types; var != NULL; var = var->next) {
+		if (var->addr != addr)
+			continue;
+		/* Get the type DIE using the offset */
+		if (!dwarf_offdie(dloc->di->dbg, var->die_off, &mem_die))
+			continue;
+
+		if (var->reg == DWARF_REG_FB) {
+			findnew_stack_state(state, var->offset, &mem_die);
+
+			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
+				     insn_offset, -var->offset);
+			pr_debug_type_name(&mem_die);
+		} else if (var->reg == fbreg) {
+			findnew_stack_state(state, var->offset - fb_offset, &mem_die);
+
+			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
+				     insn_offset, -var->offset + fb_offset);
+			pr_debug_type_name(&mem_die);
+		} else if (has_reg_type(state, var->reg) && var->offset == 0) {
+			struct type_state_reg *reg;
+
+			reg = &state->regs[var->reg];
+			reg->type = mem_die;
+			reg->ok = true;
+
+			pr_debug_dtp("var [%"PRIx64"] reg%d type=",
+				     insn_offset, var->reg);
+			pr_debug_type_name(&mem_die);
+		}
+	}
+}
+
 /* The result will be saved in @type_die */
 static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 {
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 9080119a258c..41dbbb25b256 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include "debug.h"
 #include "dwarf-aux.h"
+#include "dwarf-regs.h"
 #include "strbuf.h"
 #include "string2.h"
 
@@ -1190,6 +1191,8 @@ static int reg_from_dwarf_op(Dwarf_Op *op)
 	case DW_OP_regx:
 	case DW_OP_bregx:
 		return op->number;
+	case DW_OP_fbreg:
+		return DWARF_REG_FB;
 	default:
 		break;
 	}
@@ -1203,6 +1206,7 @@ static int offset_from_dwarf_op(Dwarf_Op *op)
 	case DW_OP_regx:
 		return 0;
 	case DW_OP_breg0 ... DW_OP_breg31:
+	case DW_OP_fbreg:
 		return op->number;
 	case DW_OP_bregx:
 		return op->number2;
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 10/23] perf annotate-data: Add update_insn_state()

[Namhyung Kim]

The update_insn_state() function is to update the type state table after
processing each instruction.  For now, it handles MOV (on x86) insn
to transfer type info from the source location to the target.

The location can be a register or a stack slot.  Check carefully when
memory reference happens and fetch the type correctly.  It basically
ignores write to a memory since it doesn't change the type info.  One
exception is writes to (new) stack slots for register spilling.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 161 +++++++++++++++++++++++++++++++-
 tools/perf/util/annotate-data.h |   2 +
 tools/perf/util/annotate.c      |   1 +
 3 files changed, 161 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 8eaa06f1cee5..592437b6c097 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -71,7 +71,9 @@ struct type_state_stack {
  * shortest path of basic blocks, it only maintains a single table.
  */
 struct type_state {
+	/* state of general purpose registers */
 	struct type_state_reg regs[TYPE_STATE_MAX_REGS];
+	/* state of stack location */
 	struct list_head stack_vars;
 };
 
@@ -85,6 +87,8 @@ void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
 void exit_type_state(struct type_state *state);
 void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 		      u64 addr, u64 insn_offset, struct die_var_type *var_types);
+void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
+		       struct disasm_line *dl);
 
 void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
 {
@@ -412,13 +416,13 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 		if (var->reg == DWARF_REG_FB) {
 			findnew_stack_state(state, var->offset, &mem_die);
 
-			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
+			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
 				     insn_offset, -var->offset);
 			pr_debug_type_name(&mem_die);
 		} else if (var->reg == fbreg) {
 			findnew_stack_state(state, var->offset - fb_offset, &mem_die);
 
-			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
+			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
 				     insn_offset, -var->offset + fb_offset);
 			pr_debug_type_name(&mem_die);
 		} else if (has_reg_type(state, var->reg) && var->offset == 0) {
@@ -428,13 +432,164 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 			reg->type = mem_die;
 			reg->ok = true;
 
-			pr_debug_dtp("var [%"PRIx64"] reg%d type=",
+			pr_debug_dtp("var [%"PRIx64"] reg%d",
 				     insn_offset, var->reg);
 			pr_debug_type_name(&mem_die);
 		}
 	}
 }
 
+static void update_insn_state_x86(struct type_state *state,
+				  struct data_loc_info *dloc,
+				  struct disasm_line *dl)
+{
+	struct annotated_insn_loc loc;
+	struct annotated_op_loc *src = &loc.ops[INSN_OP_SOURCE];
+	struct annotated_op_loc *dst = &loc.ops[INSN_OP_TARGET];
+	struct type_state_reg *tsr;
+	Dwarf_Die type_die;
+	u32 insn_offset = dl->al.offset;
+	int fbreg = dloc->fbreg;
+	int fboff = 0;
+
+	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
+		return;
+
+	if (strncmp(dl->ins.name, "mov", 3))
+		return;
+
+	if (dloc->fb_cfa) {
+		u64 ip = dloc->ms->sym->start + dl->al.offset;
+		u64 pc = map__rip_2objdump(dloc->ms->map, ip);
+
+		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
+			fbreg = -1;
+	}
+
+	/* Case 1. register to register transfers */
+	if (!src->mem_ref && !dst->mem_ref) {
+		if (!has_reg_type(state, dst->reg1))
+			return;
+
+		tsr = &state->regs[dst->reg1];
+		if (!has_reg_type(state, src->reg1) ||
+		    !state->regs[src->reg1].ok) {
+			tsr->ok = false;
+			return;
+		}
+
+		tsr->type = state->regs[src->reg1].type;
+		tsr->ok = true;
+
+		pr_debug_dtp("mov [%x] reg%d -> reg%d",
+			     insn_offset, src->reg1, dst->reg1);
+		pr_debug_type_name(&tsr->type);
+	}
+	/* Case 2. memory to register transers */
+	if (src->mem_ref && !dst->mem_ref) {
+		int sreg = src->reg1;
+
+		if (!has_reg_type(state, dst->reg1))
+			return;
+
+		tsr = &state->regs[dst->reg1];
+
+retry:
+		/* Check stack variables with offset */
+		if (sreg == fbreg) {
+			struct type_state_stack *stack;
+			int offset = src->offset - fboff;
+
+			stack = find_stack_state(state, offset);
+			if (stack == NULL) {
+				tsr->ok = false;
+				return;
+			} else if (!stack->compound) {
+				tsr->type = stack->type;
+				tsr->ok = true;
+			} else if (die_get_member_type(&stack->type,
+						       offset - stack->offset,
+						       &type_die)) {
+				tsr->type = type_die;
+				tsr->ok = true;
+			} else {
+				tsr->ok = false;
+				return;
+			}
+
+			pr_debug_dtp("mov [%x] -%#x(stack) -> reg%d",
+				     insn_offset, -offset, dst->reg1);
+			pr_debug_type_name(&tsr->type);
+		}
+		/* And then dereference the pointer if it has one */
+		else if (has_reg_type(state, sreg) && state->regs[sreg].ok &&
+			 die_deref_ptr_type(&state->regs[sreg].type,
+					    src->offset, &type_die)) {
+			tsr->type = type_die;
+			tsr->ok = true;
+
+			pr_debug_dtp("mov [%x] %#x(reg%d) -> reg%d",
+				     insn_offset, src->offset, sreg, dst->reg1);
+			pr_debug_type_name(&tsr->type);
+		}
+		/* Or try another register if any */
+		else if (src->multi_regs && sreg == src->reg1 &&
+			 src->reg1 != src->reg2) {
+			sreg = src->reg2;
+			goto retry;
+		}
+		/* It failed to get a type info, mark it as invalid */
+		else {
+			tsr->ok = false;
+		}
+	}
+	/* Case 3. register to memory transfers */
+	if (!src->mem_ref && dst->mem_ref) {
+		if (!has_reg_type(state, src->reg1) ||
+		    !state->regs[src->reg1].ok)
+			return;
+
+		/* Check stack variables with offset */
+		if (dst->reg1 == fbreg) {
+			struct type_state_stack *stack;
+			int offset = dst->offset - fboff;
+
+			stack = find_stack_state(state, offset);
+			if (stack) {
+				/*
+				 * The source register is likely to hold a type
+				 * of member if it's a compound type.  Do not
+				 * update the stack variable type since we can
+				 * get the member type later by using the
+				 * die_get_member_type().
+				 */
+				if (!stack->compound)
+					set_stack_state(stack, offset,
+							&state->regs[src->reg1].type);
+			} else {
+				findnew_stack_state(state, offset,
+						    &state->regs[src->reg1].type);
+			}
+
+			pr_debug_dtp("mov [%x] reg%d -> -%#x(stack)",
+				     insn_offset, src->reg1, -offset);
+			pr_debug_type_name(&state->regs[src->reg1].type);
+		}
+		/*
+		 * Ignore other transfers since it'd set a value in a struct
+		 * and won't change the type.
+		 */
+	}
+	/* Case 4. memory to memory transfers (not handled for now) */
+}
+
+void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
+		       struct disasm_line *dl)
+{
+	if (arch__is(dloc->arch, "x86"))
+		update_insn_state_x86(state, dloc, dl);
+}
+
 /* The result will be saved in @type_die */
 static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 {
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index ad6493ea2c8e..7324cafe2c7b 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -75,6 +75,7 @@ extern struct annotated_data_type stackop_type;
 
 /**
  * struct data_loc_info - Data location information
+ * @arch: CPU architecture info
  * @ms: Map and Symbol info
  * @ip: Instruction address
  * @var_addr: Data address (for global variables)
@@ -87,6 +88,7 @@ extern struct annotated_data_type stackop_type;
  */
 struct data_loc_info {
 	/* These are input field, should be filled by caller */
+	struct arch *arch;
 	struct map_symbol *ms;
 	u64 ip;
 	u64 var_addr;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index aa005c13ff67..9777df5dc2e3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3872,6 +3872,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 
 	for_each_insn_op_loc(&loc, i, op_loc) {
 		struct data_loc_info dloc = {
+			.arch = arch,
 			.ms = ms,
 			/* Recalculate IP for LOCK prefix or insn fusion */
 			.ip = ms->sym->start + dl->al.offset,
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 11/23] perf annotate-data: Add get_global_var_type()

[Namhyung Kim]

Accessing global variable is common when it tracks execution later.
Factor out the common code into a function for later use.

It adds thread and cpumode to struct data_loc_info to find (global)
symbols if needed.  Also remove var_name as it's retrieved in the
helper function.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 62 +++++++++++++++++++++++++--------
 tools/perf/util/annotate-data.h |  7 ++--
 tools/perf/util/annotate.c      | 21 +++--------
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 592437b6c097..3b661e693410 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -22,6 +22,7 @@
 #include "strbuf.h"
 #include "symbol.h"
 #include "symbol_conf.h"
+#include "thread.h"
 
 #define pr_debug_dtp(fmt, ...)					\
 do {								\
@@ -382,6 +383,50 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
 	return stack;
 }
 
+static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
+				u64 ip, u64 var_addr, int *var_offset,
+				Dwarf_Die *type_die)
+{
+	u64 pc, mem_addr;
+	int offset;
+	bool is_pointer = false;
+	const char *var_name = NULL;
+	Dwarf_Die var_die;
+	struct addr_location al;
+	struct symbol *sym;
+
+	/* 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) {
+		*var_offset = offset;
+		return true;
+	}
+
+	/* Kernel symbols might be relocated */
+	mem_addr = var_addr + map__reloc(dloc->ms->map);
+
+	addr_location__init(&al);
+	sym = thread__find_symbol_fb(dloc->thread, dloc->cpumode,
+				     mem_addr, &al);
+	if (sym) {
+		var_name = sym->name;
+		/* Calculate type offset from the start of variable */
+		*var_offset = mem_addr - map__unmap_ip(al.map, sym->start);
+	}
+	addr_location__exit(&al);
+	if (var_name == NULL)
+		return false;
+
+	pc = map__rip_2objdump(dloc->ms->map, ip);
+
+	/* 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)
+		return true;
+
+	return false;
+}
+
 /**
  * update_var_state - Update type state using given variables
  * @state: type state table
@@ -637,24 +682,14 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 	pr_debug_dtp("CU die offset: %#lx\n", (long)dwarf_dieoffset(&cu_die));
 
 	if (reg == DWARF_REG_PC) {
-		if (die_find_variable_by_addr(&cu_die, dloc->var_addr, &var_die,
-					      &offset)) {
-			ret = check_variable(&var_die, type_die, offset,
-					     /*is_pointer=*/false);
+		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\n",
 				     dloc->var_addr, offset);
 			goto out;
 		}
-
-		if (dloc->var_name &&
-		    die_find_variable_at(&cu_die, dloc->var_name, pc, &var_die)) {
-			ret = check_variable(&var_die, type_die, dloc->type_offset,
-					     /*is_pointer=*/false);
-			/* dloc->type_offset was updated by the caller */
-			goto out;
-		}
 	}
 
 	/* Get a list of nested scopes - i.e. (inlined) functions and blocks. */
@@ -769,8 +804,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
 	 * The type offset is the same as instruction offset by default.
 	 * But when finding a global variable, the offset won't be valid.
 	 */
-	if (dloc->var_name == NULL)
-		dloc->type_offset = dloc->op->offset;
+	dloc->type_offset = dloc->op->offset;
 
 	dloc->fbreg = -1;
 
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 7324cafe2c7b..acfbd1748d02 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -11,6 +11,7 @@ struct annotated_op_loc;
 struct debuginfo;
 struct evsel;
 struct map_symbol;
+struct thread;
 
 /**
  * struct annotated_member - Type of member field
@@ -76,10 +77,11 @@ extern struct annotated_data_type stackop_type;
 /**
  * struct data_loc_info - Data location information
  * @arch: CPU architecture info
+ * @thread: Thread info
  * @ms: Map and Symbol info
  * @ip: Instruction address
  * @var_addr: Data address (for global variables)
- * @var_name: Variable name (for global variables)
+ * @cpumode: CPU execution mode
  * @op: Instruction operand location (regs and offset)
  * @di: Debug info
  * @fbreg: Frame base register
@@ -89,10 +91,11 @@ extern struct annotated_data_type stackop_type;
 struct data_loc_info {
 	/* These are input field, should be filled by caller */
 	struct arch *arch;
+	struct thread *thread;
 	struct map_symbol *ms;
 	u64 ip;
 	u64 var_addr;
-	const char *var_name;
+	u8 cpumode;
 	struct annotated_op_loc *op;
 
 	/* These are used internally */
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 9777df5dc2e3..abb641aa8ec0 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3873,9 +3873,11 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 	for_each_insn_op_loc(&loc, i, op_loc) {
 		struct data_loc_info dloc = {
 			.arch = arch,
+			.thread = he->thread,
 			.ms = ms,
 			/* Recalculate IP for LOCK prefix or insn fusion */
 			.ip = ms->sym->start + dl->al.offset,
+			.cpumode = he->cpumode,
 			.op = op_loc,
 		};
 
@@ -3887,23 +3889,8 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 
 		/* PC-relative addressing */
 		if (op_loc->reg1 == DWARF_REG_PC) {
-			struct addr_location al;
-			struct symbol *var;
-			u64 map_addr;
-
-			dloc.var_addr = annotate_calc_pcrel(ms, ip, op_loc->offset, dl);
-			/* Kernel symbols might be relocated */
-			map_addr = dloc.var_addr + map__reloc(ms->map);
-
-			addr_location__init(&al);
-			var = thread__find_symbol_fb(he->thread, he->cpumode,
-						     map_addr, &al);
-			if (var) {
-				dloc.var_name = var->name;
-				/* Calculate type offset from the start of variable */
-				dloc.type_offset = map_addr - map__unmap_ip(al.map, var->start);
-			}
-			addr_location__exit(&al);
+			dloc.var_addr = annotate_calc_pcrel(ms, dloc.ip,
+							    op_loc->offset, dl);
 		}
 
 		mem_type = find_data_type(&dloc);
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 12/23] perf annotate-data: Handle global variable access

[Namhyung Kim]

When updating the instruction states, it also needs to handle global
variable accesses.  Same as it does for PC-relative addressing, it can
look up the type by address (if it's defined in the same file), or by
name after finding the symbol by address (for declarations).

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 3b661e693410..2cc9f56e3eea 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -89,7 +89,7 @@ void exit_type_state(struct type_state *state);
 void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 		      u64 addr, u64 insn_offset, struct die_var_type *var_types);
 void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
-		       struct disasm_line *dl);
+		       Dwarf_Die *cu_die, struct disasm_line *dl);
 
 void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
 {
@@ -485,7 +485,7 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 }
 
 static void update_insn_state_x86(struct type_state *state,
-				  struct data_loc_info *dloc,
+				  struct data_loc_info *dloc, Dwarf_Die *cu_die,
 				  struct disasm_line *dl)
 {
 	struct annotated_insn_loc loc;
@@ -577,6 +577,29 @@ static void update_insn_state_x86(struct type_state *state,
 				     insn_offset, src->offset, sreg, dst->reg1);
 			pr_debug_type_name(&tsr->type);
 		}
+		/* Or check if it's a global variable */
+		else if (sreg == DWARF_REG_PC) {
+			struct map_symbol *ms = dloc->ms;
+			u64 ip = ms->sym->start + dl->al.offset;
+			u64 addr;
+			int offset;
+
+			addr = annotate_calc_pcrel(ms, ip, src->offset, dl);
+
+			if (!get_global_var_type(cu_die, dloc, ip, addr, &offset,
+						 &type_die) ||
+			    !die_get_member_type(&type_die, offset, &type_die)) {
+				tsr->ok = false;
+				return;
+			}
+
+			tsr->type = type_die;
+			tsr->ok = true;
+
+			pr_debug_dtp("mov [%x] global addr=%"PRIx64" -> reg%d",
+				     insn_offset, addr, dst->reg1);
+			pr_debug_type_name(&type_die);
+		}
 		/* Or try another register if any */
 		else if (src->multi_regs && sreg == src->reg1 &&
 			 src->reg1 != src->reg2) {
@@ -628,11 +651,26 @@ static void update_insn_state_x86(struct type_state *state,
 	/* Case 4. memory to memory transfers (not handled for now) */
 }
 
+/**
+ * update_insn_state - Update type state for an instruction
+ * @state: type state table
+ * @dloc: data location info
+ * @cu_die: compile unit debug entry
+ * @dl: disasm line for the instruction
+ *
+ * This function updates the @state table for the target operand of the
+ * instruction at @dl if it transfers the type like MOV on x86.  Since it
+ * tracks the type, it won't care about the values like in arithmetic
+ * instructions like ADD/SUB/MUL/DIV and INC/DEC.
+ *
+ * Note that ops->reg2 is only available when both mem_ref and multi_regs
+ * are true.
+ */
 void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
-		       struct disasm_line *dl)
+		       Dwarf_Die *cu_die, struct disasm_line *dl)
 {
 	if (arch__is(dloc->arch, "x86"))
-		update_insn_state_x86(state, dloc, dl);
+		update_insn_state_x86(state, dloc, cu_die, dl);
 }
 
 /* The result will be saved in @type_die */
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 13/23] perf annotate-data: Handle call instructions

[Namhyung Kim]

When updating instruction states, the call instruction should play a
role since it changes the register states.  For simplicity, mark some
registers as caller-saved registers (should be arch-dependent), and
invalidate them all after a function call.

If the function returns something, the designated register (ret_reg)
will have the type info.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 2cc9f56e3eea..6bcf22e523cb 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -47,10 +47,14 @@ static void pr_debug_type_name(Dwarf_Die *die)
 	free(str);
 }
 
-/* Type information in a register, valid when ok is true */
+/*
+ * Type information in a register, valid when @ok is true.
+ * The @caller_saved registers are invalidated after a function call.
+ */
 struct type_state_reg {
 	Dwarf_Die type;
 	bool ok;
+	bool caller_saved;
 };
 
 /* Type information in a stack location, dynamically allocated */
@@ -76,6 +80,8 @@ struct type_state {
 	struct type_state_reg regs[TYPE_STATE_MAX_REGS];
 	/* state of stack location */
 	struct list_head stack_vars;
+	/* return value register */
+	int ret_reg;
 };
 
 static bool has_reg_type(struct type_state *state, int reg)
@@ -91,10 +97,23 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
 void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
 		       Dwarf_Die *cu_die, struct disasm_line *dl);
 
-void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
+void init_type_state(struct type_state *state, struct arch *arch)
 {
 	memset(state, 0, sizeof(*state));
 	INIT_LIST_HEAD(&state->stack_vars);
+
+	if (arch__is(arch, "x86")) {
+		state->regs[0].caller_saved = true;
+		state->regs[1].caller_saved = true;
+		state->regs[2].caller_saved = true;
+		state->regs[4].caller_saved = true;
+		state->regs[5].caller_saved = true;
+		state->regs[8].caller_saved = true;
+		state->regs[9].caller_saved = true;
+		state->regs[10].caller_saved = true;
+		state->regs[11].caller_saved = true;
+		state->ret_reg = 0;
+	}
 }
 
 void exit_type_state(struct type_state *state)
@@ -500,6 +519,37 @@ static void update_insn_state_x86(struct type_state *state,
 	if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
 		return;
 
+	if (ins__is_call(&dl->ins)) {
+		struct symbol *func = dl->ops.target.sym;
+
+		if (func == NULL)
+			return;
+
+		/* __fentry__ will preserve all registers */
+		if (!strcmp(func->name, "__fentry__"))
+			return;
+
+		pr_debug_dtp("call [%x] %s\n", insn_offset, func->name);
+
+		/* Otherwise invalidate caller-saved registers after call */
+		for (unsigned i = 0; i < ARRAY_SIZE(state->regs); i++) {
+			if (state->regs[i].caller_saved)
+				state->regs[i].ok = false;
+		}
+
+		/* Update register with the return type (if any) */
+		if (die_find_func_rettype(cu_die, func->name, &type_die)) {
+			tsr = &state->regs[state->ret_reg];
+			tsr->type = type_die;
+			tsr->ok = true;
+
+			pr_debug_dtp("call [%x] return -> reg%d",
+				     insn_offset, state->ret_reg);
+			pr_debug_type_name(&type_die);
+		}
+		return;
+	}
+
 	if (strncmp(dl->ins.name, "mov", 3))
 		return;
 
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 14/23] perf annotate-data: Implement instruction tracking

[Namhyung Kim]

If it failed to find a variable for the location directly, it might be
due to a missing variable in the source code.  For example, accessing
pointer variables in a chain can result in the case like below:

  struct foo *foo = ...;

  int i = foo->bar->baz;

The DWARF debug information is created for each variable so it'd have
one for 'foo'.  But there's no variable for 'foo->bar' and then it
cannot know the type of 'bar' and 'baz'.

The above source code can be compiled to the follow x86 instructions:

  mov  0x8(%rax), %rcx
  mov  0x4(%rcx), %rdx   <=== PMU sample
  mov  %rdx, -4(%rbp)

Let's say 'foo' is located in the %rax and it has a pointer to struct
foo.  But perf sample is captured in the second instruction and there
is no variable or type info for the %rcx.

It'd be great if compiler could generate debug info for %rcx, but we
should handle it on our side.  So this patch implements the logic to
iterate instructions and update the type table for each location.

As it already collected a list of scopes including the target
instruction, we can use it to construct the type table smartly.

  +----------------  scope[0] subprogram
  |
  | +--------------  scope[1] lexical_block
  | |
  | | +------------  scope[2] inlined_subroutine
  | | |
  | | | +----------  scope[3] inlined_subroutine
  | | | |
  | | | | +--------  scope[4] lexical_block
  | | | | |
  | | | | |     ***  target instruction
  ...

Image the target instruction has 5 scopes, each scope will have its own
variables and parameters.  Then it can start with the innermost scope
(4).  So it'd search the shortest path from the start of scope[4] to
the target address and build a list of basic blocks.  Then it iterates
the basic blocks with the variables in the scope and update the table.
If it finds a type at the target instruction, then returns it.

Otherwise, it moves to the upper scope[3].  Now it'd search the shortest
path from the start of scope[3] to the start of scope[4].  Then connect
it to the existing basic block list.  Then it'd iterate the blocks with
variables for both scopes.  It can repeat this until it finds a type at
the target instruction or reaches to the top scope[0].

As the basic blocks contain the shortest path, it won't worry about
branches and can update the table simply.

The final check will be done by find_matching_type() in the next patch.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c   |   1 +
 tools/perf/util/annotate-data.c | 223 ++++++++++++++++++++++++++++++--
 tools/perf/util/annotate-data.h |   1 +
 3 files changed, 211 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 6c1cc797692d..f677671409b1 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -430,6 +430,7 @@ static void print_annotate_data_stat(struct annotated_data_stat *s)
 	PRINT_STAT(no_typeinfo);
 	PRINT_STAT(invalid_size);
 	PRINT_STAT(bad_offset);
+	PRINT_STAT(insn_track);
 	printf("\n");
 
 #undef PRINT_STAT
diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 6bcf22e523cb..13ba65693367 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -89,15 +89,7 @@ static bool has_reg_type(struct type_state *state, int reg)
 	return (unsigned)reg < ARRAY_SIZE(state->regs);
 }
 
-/* These declarations will be remove once they are changed to static */
-void init_type_state(struct type_state *state, struct arch *arch __maybe_unused);
-void exit_type_state(struct type_state *state);
-void update_var_state(struct type_state *state, struct data_loc_info *dloc,
-		      u64 addr, u64 insn_offset, struct die_var_type *var_types);
-void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
-		       Dwarf_Die *cu_die, struct disasm_line *dl);
-
-void init_type_state(struct type_state *state, struct arch *arch)
+static void init_type_state(struct type_state *state, struct arch *arch)
 {
 	memset(state, 0, sizeof(*state));
 	INIT_LIST_HEAD(&state->stack_vars);
@@ -116,7 +108,7 @@ void init_type_state(struct type_state *state, struct arch *arch)
 	}
 }
 
-void exit_type_state(struct type_state *state)
+static void exit_type_state(struct type_state *state)
 {
 	struct type_state_stack *stack, *tmp;
 
@@ -457,8 +449,8 @@ static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
  * This function fills the @state table using @var_types info.  Each variable
  * is used only at the given location and updates an entry in the table.
  */
-void update_var_state(struct type_state *state, struct data_loc_info *dloc,
-		      u64 addr, u64 insn_offset, struct die_var_type *var_types)
+static void update_var_state(struct type_state *state, struct data_loc_info *dloc,
+			     u64 addr, u64 insn_offset, struct die_var_type *var_types)
 {
 	Dwarf_Die mem_die;
 	struct die_var_type *var;
@@ -716,13 +708,207 @@ static void update_insn_state_x86(struct type_state *state,
  * Note that ops->reg2 is only available when both mem_ref and multi_regs
  * are true.
  */
-void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
-		       Dwarf_Die *cu_die, struct disasm_line *dl)
+static void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
+			      Dwarf_Die *cu_die, struct disasm_line *dl)
 {
 	if (arch__is(dloc->arch, "x86"))
 		update_insn_state_x86(state, dloc, cu_die, dl);
 }
 
+/*
+ * Prepend this_blocks (from the outer scope) to full_blocks, removing
+ * duplicate disasm line.
+ */
+static void prepend_basic_blocks(struct list_head *this_blocks,
+				 struct list_head *full_blocks)
+{
+	struct annotated_basic_block *first_bb, *last_bb;
+
+	last_bb = list_last_entry(this_blocks, typeof(*last_bb), list);
+	first_bb = list_first_entry(full_blocks, typeof(*first_bb), list);
+
+	if (list_empty(full_blocks))
+		goto out;
+
+	/* Last insn in this_blocks should be same as first insn in full_blocks */
+	if (last_bb->end != first_bb->begin) {
+		pr_debug("prepend basic blocks: mismatched disasm line %"PRIx64" -> %"PRIx64"\n",
+			 last_bb->end->al.offset, first_bb->begin->al.offset);
+		goto out;
+	}
+
+	/* Is the basic block have only one disasm_line? */
+	if (last_bb->begin == last_bb->end) {
+		list_del(&last_bb->list);
+		free(last_bb);
+		goto out;
+	}
+
+	/* Point to the insn before the last when adding this block to full_blocks */
+	last_bb->end = list_prev_entry(last_bb->end, al.node);
+
+out:
+	list_splice(this_blocks, full_blocks);
+}
+
+static void delete_basic_blocks(struct list_head *basic_blocks)
+{
+	struct annotated_basic_block *bb, *tmp;
+
+	list_for_each_entry_safe(bb, tmp, basic_blocks, list) {
+		list_del(&bb->list);
+		free(bb);
+	}
+}
+
+/* Make sure all variables have a valid start address */
+static void fixup_var_address(struct die_var_type *var_types, u64 addr)
+{
+	while (var_types) {
+		/*
+		 * Some variables have no address range meaning it's always
+		 * available in the whole scope.  Let's adjust the start
+		 * address to the start of the scope.
+		 */
+		if (var_types->addr == 0)
+			var_types->addr = addr;
+
+		var_types = var_types->next;
+	}
+}
+
+static void delete_var_types(struct die_var_type *var_types)
+{
+	while (var_types) {
+		struct die_var_type *next = var_types->next;
+
+		free(var_types);
+		var_types = next;
+	}
+}
+
+/* It's at the target address, check if it has a matching type */
+static bool find_matching_type(struct type_state *state __maybe_unused,
+			       struct data_loc_info *dloc __maybe_unused,
+			       int reg __maybe_unused,
+			       Dwarf_Die *type_die __maybe_unused)
+{
+	/* TODO */
+	return false;
+}
+
+/* Iterate instructions in basic blocks and update type table */
+static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
+				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;
+	bool found = false;
+
+	init_type_state(&state, dloc->arch);
+
+	list_for_each_entry(bb, basic_blocks, list) {
+		struct disasm_line *dl = bb->begin;
+
+		pr_debug_dtp("bb: [%"PRIx64" - %"PRIx64"]\n",
+			     bb->begin->al.offset, bb->end->al.offset);
+
+		list_for_each_entry_from(dl, &notes->src->source, al.node) {
+			u64 this_ip = sym->start + dl->al.offset;
+			u64 addr = map__rip_2objdump(dloc->ms->map, this_ip);
+
+			/* Update variable type at this address */
+			update_var_state(&state, dloc, addr, dl->al.offset, var_types);
+
+			if (this_ip == dloc->ip) {
+				found = find_matching_type(&state, dloc, reg,
+							   type_die);
+				goto out;
+			}
+
+			/* Update type table after processing the instruction */
+			update_insn_state(&state, dloc, cu_die, dl);
+			if (dl == bb->end)
+				break;
+		}
+	}
+
+out:
+	exit_type_state(&state);
+	return found;
+}
+
+/*
+ * 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, int reg,
+				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;
+
+	/* TODO: other architecture support */
+	if (!arch__is(dloc->arch, "x86"))
+		return -1;
+
+	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);
+
+		if (dwarf_ranges(&scopes[i], 0, &base, &start, &end) < 0)
+			break;
+
+		pr_debug_dtp("scope: [%d/%d] (die:%lx)\n",
+			     i + 1, nr_scopes, (long)dwarf_dieoffset(&scopes[i]));
+		src_ip = map__objdump_2rip(dloc->ms->map, start);
+
+again:
+		/* Get basic blocks for this scope */
+		if (annotate_get_basic_blocks(dloc->ms->sym, src_ip, dst_ip,
+					      &this_blocks) < 0) {
+			/* Try previous block if they are not connected */
+			if (prev_dst_ip != dst_ip) {
+				dst_ip = prev_dst_ip;
+				goto again;
+			}
+
+			pr_debug_dtp("cannot find a basic block from %"PRIx64" to %"PRIx64"\n",
+				     src_ip - dloc->ms->sym->start,
+				     dst_ip - dloc->ms->sym->start);
+			continue;
+		}
+		prepend_basic_blocks(&this_blocks, &basic_blocks);
+
+		/* Get variable info for this scope and add to var_types list */
+		die_collect_vars(&scopes[i], &var_types);
+		fixup_var_address(var_types, start);
+
+		/* Find from start of this scope to the target instruction */
+		if (find_data_type_insn(dloc, reg, &basic_blocks, var_types,
+					cu_die, type_die)) {
+			ret = 0;
+			break;
+		}
+
+		/* Go up to the next scope and find blocks to the start */
+		prev_dst_ip = dst_ip;
+		dst_ip = src_ip;
+	}
+
+	delete_basic_blocks(&basic_blocks);
+	delete_var_types(var_types);
+	return ret;
+}
+
 /* The result will be saved in @type_die */
 static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 {
@@ -847,6 +1033,15 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 		goto out;
 	}
 
+	if (reg != DWARF_REG_PC) {
+		ret = find_data_type_block(dloc, reg, &cu_die, scopes,
+					   nr_scopes, type_die);
+		if (ret == 0) {
+			ann_data_stat.insn_track++;
+			goto out;
+		}
+	}
+
 	if (loc->multi_regs && reg == loc->reg1 && loc->reg1 != loc->reg2) {
 		reg = loc->reg2;
 		goto retry;
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index acfbd1748d02..ae0f87aed804 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -135,6 +135,7 @@ struct annotated_data_stat {
 	int no_typeinfo;
 	int invalid_size;
 	int bad_offset;
+	int insn_track;
 };
 extern struct annotated_data_stat ann_data_stat;
 
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 15/23] perf annotate-data: Check register state for type

[Namhyung Kim]

As instruction tracking updates the type state for each register, check
the final type info for the target register at the given instruction.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 13ba65693367..f5329a78a97d 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -788,12 +788,83 @@ static void delete_var_types(struct die_var_type *var_types)
 }
 
 /* It's at the target address, check if it has a matching type */
-static bool find_matching_type(struct type_state *state __maybe_unused,
-			       struct data_loc_info *dloc __maybe_unused,
-			       int reg __maybe_unused,
-			       Dwarf_Die *type_die __maybe_unused)
+static bool check_matching_type(struct type_state *state,
+				struct data_loc_info *dloc, int reg,
+				Dwarf_Die *type_die)
 {
-	/* TODO */
+	Dwarf_Word size;
+	u32 insn_offset = dloc->ip - dloc->ms->sym->start;
+
+	pr_debug_dtp("chk [%x] reg%d offset=%#x ok=%d",
+		     insn_offset, reg, dloc->op->offset, state->regs[reg].ok);
+
+	if (state->regs[reg].ok) {
+		int tag = dwarf_tag(&state->regs[reg].type);
+
+		pr_debug_dtp("\n");
+
+		/*
+		 * Normal registers should hold a pointer (or array) to
+		 * dereference a memory location.
+		 */
+		if (tag != DW_TAG_pointer_type && tag != DW_TAG_array_type)
+			return false;
+
+		/* Remove the pointer and get the target type */
+		if (die_get_real_type(&state->regs[reg].type, type_die) == NULL)
+			return false;
+
+		dloc->type_offset = dloc->op->offset;
+
+		/* Get the size of the actual type */
+		if (dwarf_aggregate_size(type_die, &size) < 0 ||
+		    (unsigned)dloc->type_offset >= size)
+			return false;
+
+		return true;
+	}
+
+	if (reg == dloc->fbreg) {
+		struct type_state_stack *stack;
+
+		pr_debug_dtp(" fbreg\n");
+
+		stack = find_stack_state(state, dloc->type_offset);
+		if (stack == NULL)
+			return false;
+
+		*type_die = stack->type;
+		/* Update the type offset from the start of slot */
+		dloc->type_offset -= stack->offset;
+
+		return true;
+	}
+
+	if (dloc->fb_cfa) {
+		struct type_state_stack *stack;
+		u64 pc = map__rip_2objdump(dloc->ms->map, dloc->ip);
+		int fbreg, fboff;
+
+		pr_debug_dtp(" cfa\n");
+
+		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
+			fbreg = -1;
+
+		if (reg != fbreg)
+			return false;
+
+		stack = find_stack_state(state, dloc->type_offset - fboff);
+		if (stack == NULL)
+			return false;
+
+		*type_die = stack->type;
+		/* Update the type offset from the start of slot */
+		dloc->type_offset -= fboff + stack->offset;
+
+		return true;
+	}
+
+	pr_debug_dtp("\n");
 	return false;
 }
 
@@ -825,8 +896,8 @@ static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
 			update_var_state(&state, dloc, addr, dl->al.offset, var_types);
 
 			if (this_ip == dloc->ip) {
-				found = find_matching_type(&state, dloc, reg,
-							   type_die);
+				found = check_matching_type(&state, dloc, reg,
+							    type_die);
 				goto out;
 			}
 
@@ -896,6 +967,9 @@ static int find_data_type_block(struct data_loc_info *dloc, int reg,
 		if (find_data_type_insn(dloc, reg, &basic_blocks, var_types,
 					cu_die, type_die)) {
 			ret = 0;
+			pr_debug_dtp("found by insn track: %#x(reg%d) type-offset=%#x",
+				     dloc->op->offset, reg, dloc->type_offset);
+			pr_debug_type_name(type_die);
 			break;
 		}
 
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 16/23] perf annotate: Parse x86 segment register location

[Namhyung Kim]

Add a segment field in the struct annotated_insn_loc and save it for the
segment based addressing like %gs:0x28.  For simplicity it now handles
%gs register only.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 36 ++++++++++++++++++++++++++++++++----
 tools/perf/util/annotate.h | 15 +++++++++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index abb641aa8ec0..3aa3a3b987ad 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -94,6 +94,7 @@ struct arch {
 		char skip_functions_char;
 		char register_char;
 		char memory_ref_char;
+		char imm_char;
 	} objdump;
 };
 
@@ -211,6 +212,7 @@ static struct arch architectures[] = {
 			.comment_char = '#',
 			.register_char = '%',
 			.memory_ref_char = '(',
+			.imm_char = '$',
 		},
 	},
 	{
@@ -3585,6 +3587,12 @@ static int extract_reg_offset(struct arch *arch, const char *str,
 	 * %gs:0x18(%rbx).  In that case it should skip the part.
 	 */
 	if (*str == arch->objdump.register_char) {
+		if (arch__is(arch, "x86")) {
+			/* FIXME: Handle other segment registers */
+			if (!strncmp(str, "%gs:", 4))
+				op_loc->segment = INSN_SEG_X86_GS;
+		}
+
 		while (*str && !isdigit(*str) &&
 		       *str != arch->objdump.memory_ref_char)
 			str++;
@@ -3681,12 +3689,32 @@ int annotate_get_insn_location(struct arch *arch, struct disasm_line *dl,
 			op_loc->multi_regs = multi_regs;
 			extract_reg_offset(arch, insn_str, op_loc);
 		} else {
-			char *s = strdup(insn_str);
+			char *s, *p = NULL;
+
+			if (arch__is(arch, "x86")) {
+				/* FIXME: Handle other segment registers */
+				if (!strncmp(insn_str, "%gs:", 4)) {
+					op_loc->segment = INSN_SEG_X86_GS;
+					op_loc->offset = strtol(insn_str + 4,
+								&p, 0);
+					if (p && p != insn_str + 4)
+						op_loc->imm = true;
+					continue;
+				}
+			}
+
+			s = strdup(insn_str);
+			if (s == NULL)
+				return -1;
 
-			if (s) {
+			if (*s == arch->objdump.register_char)
 				op_loc->reg1 = get_dwarf_regnum(s, 0);
-				free(s);
+			else if (*s == arch->objdump.imm_char) {
+				op_loc->offset = strtol(s + 1, &p, 0);
+				if (p && p != s + 1)
+					op_loc->imm = true;
 			}
+			free(s);
 		}
 	}
 
@@ -3881,7 +3909,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 			.op = op_loc,
 		};
 
-		if (!op_loc->mem_ref)
+		if (!op_loc->mem_ref && op_loc->segment == INSN_SEG_NONE)
 			continue;
 
 		/* Recalculate IP because of LOCK prefix or insn fusion */
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 0928663fddee..14980b65f812 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -511,15 +511,19 @@ int annotate_check_args(void);
  * @reg1: First register in the operand
  * @reg2: Second register in the operand
  * @offset: Memory access offset in the operand
+ * @segment: Segment selector register
  * @mem_ref: Whether the operand accesses memory
  * @multi_regs: Whether the second register is used
+ * @imm: Whether the operand is an immediate value (in offset)
  */
 struct annotated_op_loc {
 	int reg1;
 	int reg2;
 	int offset;
+	u8 segment;
 	bool mem_ref;
 	bool multi_regs;
+	bool imm;
 };
 
 enum annotated_insn_ops {
@@ -529,6 +533,17 @@ enum annotated_insn_ops {
 	INSN_OP_MAX,
 };
 
+enum annotated_x86_segment {
+	INSN_SEG_NONE = 0,
+
+	INSN_SEG_X86_CS,
+	INSN_SEG_X86_DS,
+	INSN_SEG_X86_ES,
+	INSN_SEG_X86_FS,
+	INSN_SEG_X86_GS,
+	INSN_SEG_X86_SS,
+};
+
 /**
  * struct annotated_insn_loc - Location info of instruction
  * @ops: Array of location info for source and target operands
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 17/23] perf annotate-data: Handle this-cpu variables in kernel

[Namhyung Kim]

On x86, the kernel gets the current task using the current macro like
below:

  #define current  get_current()

  static __always_inline struct task_struct *get_current(void)
  {
      return this_cpu_read_stable(pcpu_hot.current_task);
  }

So it returns the current_task field of struct pcpu_hot which is the
first member.  On my build, it's located at 0x32940.

  $ nm vmlinux | grep pcpu_hot
  0000000000032940 D pcpu_hot

And the current macro generates the instructions like below:

  mov  %gs:0x32940, %rcx

So the %gs segment register points to the beginning of the per-cpu
region of this cpu and it points the variable with a constant.

Let's update the instruction location info to have a segment register
and handle %gs in kernel to look up a global variable.  Pretend it as
a global variable by changing the register number to DWARF_REG_PC.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 21 +++++++++++++++++++--
 tools/perf/util/annotate.c      |  7 +++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index f5329a78a97d..d57622ddd5d3 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -790,7 +790,7 @@ static void delete_var_types(struct die_var_type *var_types)
 /* It's at the target address, check if it has a matching type */
 static bool check_matching_type(struct type_state *state,
 				struct data_loc_info *dloc, int reg,
-				Dwarf_Die *type_die)
+				Dwarf_Die *cu_die, Dwarf_Die *type_die)
 {
 	Dwarf_Word size;
 	u32 insn_offset = dloc->ip - dloc->ms->sym->start;
@@ -864,6 +864,23 @@ static bool check_matching_type(struct type_state *state,
 		return true;
 	}
 
+	if (map__dso(dloc->ms->map)->kernel && arch__is(dloc->arch, "x86")) {
+		u64 addr;
+		int offset;
+
+		if (dloc->op->segment == INSN_SEG_X86_GS && dloc->op->imm) {
+			pr_debug_dtp(" this-cpu var\n");
+
+			addr = dloc->op->offset;
+
+			if (get_global_var_type(cu_die, dloc, dloc->ip, addr,
+						&offset, type_die)) {
+				dloc->type_offset = offset;
+				return true;
+			}
+		}
+	}
+
 	pr_debug_dtp("\n");
 	return false;
 }
@@ -897,7 +914,7 @@ static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
 
 			if (this_ip == dloc->ip) {
 				found = check_matching_type(&state, dloc, reg,
-							    type_die);
+							    cu_die, type_die);
 				goto out;
 			}
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 3aa3a3b987ad..e4121acb4f88 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3921,6 +3921,13 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 							    op_loc->offset, dl);
 		}
 
+		/* This CPU access in kernel - pretend PC-relative addressing */
+		if (map__dso(ms->map)->kernel && arch__is(arch, "x86") &&
+		    op_loc->segment == INSN_SEG_X86_GS && op_loc->imm) {
+			dloc.var_addr = op_loc->offset;
+			op_loc->reg1 = DWARF_REG_PC;
+		}
+
 		mem_type = find_data_type(&dloc);
 		if (mem_type)
 			istat->good++;
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 18/23] perf annotate-data: Track instructions with a this-cpu variable

[Namhyung Kim]

Like global variables, this per-cpu variables should be tracked
correctly.  Factor our get_global_var_type() to handle both global
and per-cpu (for this cpu) variables in the same manner.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index d57622ddd5d3..48fea0c716ef 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -553,12 +553,41 @@ static void update_insn_state_x86(struct type_state *state,
 			fbreg = -1;
 	}
 
-	/* Case 1. register to register transfers */
+	/* Case 1. register to register or segment:offset to register transfers */
 	if (!src->mem_ref && !dst->mem_ref) {
 		if (!has_reg_type(state, dst->reg1))
 			return;
 
 		tsr = &state->regs[dst->reg1];
+		if (map__dso(dloc->ms->map)->kernel &&
+		    src->segment == INSN_SEG_X86_GS && src->imm) {
+			u64 ip = dloc->ms->sym->start + dl->al.offset;
+			u64 var_addr;
+			int offset;
+
+			/*
+			 * In kernel, %gs points to a per-cpu region for the
+			 * current CPU.  Access with a constant offset should
+			 * be treated as a global variable access.
+			 */
+			var_addr = src->offset;
+
+			if (!get_global_var_type(cu_die, dloc, ip, var_addr,
+						 &offset, &type_die) ||
+			    !die_get_member_type(&type_die, offset, &type_die)) {
+				tsr->ok = false;
+				return;
+			}
+
+			tsr->type = type_die;
+			tsr->ok = true;
+
+			pr_debug_dtp("mov [%x] this-cpu addr=%#"PRIx64" -> reg%d",
+				     insn_offset, var_addr, dst->reg1);
+			pr_debug_type_name(&tsr->type);
+			return;
+		}
+
 		if (!has_reg_type(state, src->reg1) ||
 		    !state->regs[src->reg1].ok) {
 			tsr->ok = false;
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 19/23] perf annotate-data: Support general per-cpu access

[Namhyung Kim]

This is to support per-cpu variable access often without a matching
DWARF entry.  For some reason, I cannot find debug info of per-cpu
variables sometimes.  They have more complex pattern to calculate the
address of per-cpu variables like below.

  2b7d:  mov    -0x1e0(%rbp),%rax           ; rax = cpu
  2b84:  mov    -0x7da0f7e0(,%rax,8),%rcx   ; rcx = __per_cpu_offset[cpu]
* 2b8c:  mov    0x34870(%rcx),%rax          ; *(__per_cpu_offset[cpu] + 0x34870)

Let's assume the rax register has a number for a CPU at 2b7d.  The next
instruction is to get the per-cpu offset' for that cpu.  The offset
-0x7da0f7e0 is 0xffffffff825f0820 in u64 which is the address of the
'__per_cpu_offset' array in my system.  So it'd get the actual offset
of that CPU's per-cpu region and save it to the rcx register.

Then, at 2b8c, accesses using rcx can be handled same as the global
variable access.  To handle this case, it should check if the offset
of the instruction matches to the address of '__per_cpu_offset'.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 48fea0c716ef..83b5aa00f01c 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -24,6 +24,12 @@
 #include "symbol_conf.h"
 #include "thread.h"
 
+enum type_state_kind {
+	TSR_KIND_INVALID = 0,
+	TSR_KIND_TYPE,
+	TSR_KIND_PERCPU_BASE,
+};
+
 #define pr_debug_dtp(fmt, ...)					\
 do {								\
 	if (debug_type_profile)					\
@@ -32,7 +38,7 @@ do {								\
 		pr_debug3(fmt, ##__VA_ARGS__);			\
 } while (0)
 
-static void pr_debug_type_name(Dwarf_Die *die)
+static void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
 {
 	struct strbuf sb;
 	char *str;
@@ -40,6 +46,18 @@ static void pr_debug_type_name(Dwarf_Die *die)
 	if (!debug_type_profile && verbose < 3)
 		return;
 
+	switch (kind) {
+	case TSR_KIND_INVALID:
+		pr_info("\n");
+		return;
+	case TSR_KIND_PERCPU_BASE:
+		pr_info(" percpu base\n");
+		return;
+	case TSR_KIND_TYPE:
+	default:
+		break;
+	}
+
 	strbuf_init(&sb, 32);
 	die_get_typename_from_type(die, &sb);
 	str = strbuf_detach(&sb, NULL);
@@ -53,8 +71,10 @@ static void pr_debug_type_name(Dwarf_Die *die)
  */
 struct type_state_reg {
 	Dwarf_Die type;
+	u32 imm_value;
 	bool ok;
 	bool caller_saved;
+	u8 kind;
 };
 
 /* Type information in a stack location, dynamically allocated */
@@ -64,6 +84,7 @@ struct type_state_stack {
 	int offset;
 	int size;
 	bool compound;
+	u8 kind;
 };
 
 /* FIXME: This should be arch-dependent */
@@ -82,6 +103,8 @@ struct type_state {
 	struct list_head stack_vars;
 	/* return value register */
 	int ret_reg;
+	/* stack pointer register */
+	int stack_reg;
 };
 
 static bool has_reg_type(struct type_state *state, int reg)
@@ -105,6 +128,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;
 	}
 }
 
@@ -350,7 +374,7 @@ static struct type_state_stack *find_stack_state(struct type_state *state,
 	return NULL;
 }
 
-static void set_stack_state(struct type_state_stack *stack, int offset,
+static void set_stack_state(struct type_state_stack *stack, int offset, u8 kind,
 			    Dwarf_Die *type_die)
 {
 	int tag;
@@ -364,6 +388,7 @@ static void set_stack_state(struct type_state_stack *stack, int offset,
 	stack->type = *type_die;
 	stack->size = size;
 	stack->offset = offset;
+	stack->kind = kind;
 
 	switch (tag) {
 	case DW_TAG_structure_type:
@@ -377,34 +402,60 @@ static void set_stack_state(struct type_state_stack *stack, int offset,
 }
 
 static struct type_state_stack *findnew_stack_state(struct type_state *state,
-						    int offset, Dwarf_Die *type_die)
+						    int offset, u8 kind,
+						    Dwarf_Die *type_die)
 {
 	struct type_state_stack *stack = find_stack_state(state, offset);
 
 	if (stack) {
-		set_stack_state(stack, offset, type_die);
+		set_stack_state(stack, offset, kind, type_die);
 		return stack;
 	}
 
 	stack = malloc(sizeof(*stack));
 	if (stack) {
-		set_stack_state(stack, offset, type_die);
+		set_stack_state(stack, offset, kind, type_die);
 		list_add(&stack->list, &state->stack_vars);
 	}
 	return stack;
 }
 
+static bool get_global_var_info(struct data_loc_info *dloc, u64 addr,
+				const char **var_name, int *var_offset)
+{
+	struct addr_location al;
+	struct symbol *sym;
+	u64 mem_addr;
+
+	/* Kernel symbols might be relocated */
+	mem_addr = addr + map__reloc(dloc->ms->map);
+
+	addr_location__init(&al);
+	sym = thread__find_symbol_fb(dloc->thread, dloc->cpumode,
+				     mem_addr, &al);
+	if (sym) {
+		*var_name = sym->name;
+		/* Calculate type offset from the start of variable */
+		*var_offset = mem_addr - map__unmap_ip(al.map, sym->start);
+	} else {
+		*var_name = NULL;
+	}
+	addr_location__exit(&al);
+	if (*var_name == NULL)
+		return false;
+
+	return true;
+}
+
 static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
 				u64 ip, u64 var_addr, int *var_offset,
 				Dwarf_Die *type_die)
 {
-	u64 pc, mem_addr;
+	u64 pc;
 	int offset;
 	bool is_pointer = false;
-	const char *var_name = NULL;
+	const char *var_name;
 	Dwarf_Die var_die;
-	struct addr_location al;
-	struct symbol *sym;
 
 	/* Try to get the variable by address first */
 	if (die_find_variable_by_addr(cu_die, var_addr, &var_die, &offset) &&
@@ -413,19 +464,7 @@ static bool get_global_var_type(Dwarf_Die *cu_die, struct data_loc_info *dloc,
 		return true;
 	}
 
-	/* Kernel symbols might be relocated */
-	mem_addr = var_addr + map__reloc(dloc->ms->map);
-
-	addr_location__init(&al);
-	sym = thread__find_symbol_fb(dloc->thread, dloc->cpumode,
-				     mem_addr, &al);
-	if (sym) {
-		var_name = sym->name;
-		/* Calculate type offset from the start of variable */
-		*var_offset = mem_addr - map__unmap_ip(al.map, sym->start);
-	}
-	addr_location__exit(&al);
-	if (var_name == NULL)
+	if (!get_global_var_info(dloc, var_addr, &var_name, var_offset))
 		return false;
 
 	pc = map__rip_2objdump(dloc->ms->map, ip);
@@ -470,27 +509,30 @@ static void update_var_state(struct type_state *state, struct data_loc_info *dlo
 			continue;
 
 		if (var->reg == DWARF_REG_FB) {
-			findnew_stack_state(state, var->offset, &mem_die);
+			findnew_stack_state(state, var->offset, TSR_KIND_TYPE,
+					    &mem_die);
 
 			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
 				     insn_offset, -var->offset);
-			pr_debug_type_name(&mem_die);
+			pr_debug_type_name(&mem_die, TSR_KIND_TYPE);
 		} else if (var->reg == fbreg) {
-			findnew_stack_state(state, var->offset - fb_offset, &mem_die);
+			findnew_stack_state(state, var->offset - fb_offset,
+					    TSR_KIND_TYPE, &mem_die);
 
 			pr_debug_dtp("var [%"PRIx64"] -%#x(stack)",
 				     insn_offset, -var->offset + fb_offset);
-			pr_debug_type_name(&mem_die);
+			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];
 			reg->type = mem_die;
+			reg->kind = TSR_KIND_TYPE;
 			reg->ok = true;
 
 			pr_debug_dtp("var [%"PRIx64"] reg%d",
 				     insn_offset, var->reg);
-			pr_debug_type_name(&mem_die);
+			pr_debug_type_name(&mem_die, TSR_KIND_TYPE);
 		}
 	}
 }
@@ -533,11 +575,12 @@ static void update_insn_state_x86(struct type_state *state,
 		if (die_find_func_rettype(cu_die, func->name, &type_die)) {
 			tsr = &state->regs[state->ret_reg];
 			tsr->type = type_die;
+			tsr->kind = TSR_KIND_TYPE;
 			tsr->ok = true;
 
 			pr_debug_dtp("call [%x] return -> reg%d",
 				     insn_offset, state->ret_reg);
-			pr_debug_type_name(&type_die);
+			pr_debug_type_name(&type_die, tsr->kind);
 		}
 		return;
 	}
@@ -580,11 +623,12 @@ static void update_insn_state_x86(struct type_state *state,
 			}
 
 			tsr->type = type_die;
+			tsr->kind = TSR_KIND_TYPE;
 			tsr->ok = true;
 
 			pr_debug_dtp("mov [%x] this-cpu addr=%#"PRIx64" -> reg%d",
 				     insn_offset, var_addr, dst->reg1);
-			pr_debug_type_name(&tsr->type);
+			pr_debug_type_name(&tsr->type, tsr->kind);
 			return;
 		}
 
@@ -595,11 +639,12 @@ static void update_insn_state_x86(struct type_state *state,
 		}
 
 		tsr->type = state->regs[src->reg1].type;
+		tsr->kind = state->regs[src->reg1].kind;
 		tsr->ok = true;
 
 		pr_debug_dtp("mov [%x] reg%d -> reg%d",
 			     insn_offset, src->reg1, dst->reg1);
-		pr_debug_type_name(&tsr->type);
+		pr_debug_type_name(&tsr->type, tsr->kind);
 	}
 	/* Case 2. memory to register transers */
 	if (src->mem_ref && !dst->mem_ref) {
@@ -622,11 +667,13 @@ static void update_insn_state_x86(struct type_state *state,
 				return;
 			} else if (!stack->compound) {
 				tsr->type = stack->type;
+				tsr->kind = stack->kind;
 				tsr->ok = true;
 			} else if (die_get_member_type(&stack->type,
 						       offset - stack->offset,
 						       &type_die)) {
 				tsr->type = type_die;
+				tsr->kind = TSR_KIND_TYPE;
 				tsr->ok = true;
 			} else {
 				tsr->ok = false;
@@ -635,18 +682,20 @@ static void update_insn_state_x86(struct type_state *state,
 
 			pr_debug_dtp("mov [%x] -%#x(stack) -> reg%d",
 				     insn_offset, -offset, dst->reg1);
-			pr_debug_type_name(&tsr->type);
+			pr_debug_type_name(&tsr->type, tsr->kind);
 		}
 		/* And then dereference the pointer if it has one */
 		else if (has_reg_type(state, sreg) && state->regs[sreg].ok &&
+			 state->regs[sreg].kind == TSR_KIND_TYPE &&
 			 die_deref_ptr_type(&state->regs[sreg].type,
 					    src->offset, &type_die)) {
 			tsr->type = type_die;
+			tsr->kind = TSR_KIND_TYPE;
 			tsr->ok = true;
 
 			pr_debug_dtp("mov [%x] %#x(reg%d) -> reg%d",
 				     insn_offset, src->offset, sreg, dst->reg1);
-			pr_debug_type_name(&tsr->type);
+			pr_debug_type_name(&tsr->type, tsr->kind);
 		}
 		/* Or check if it's a global variable */
 		else if (sreg == DWARF_REG_PC) {
@@ -665,11 +714,37 @@ static void update_insn_state_x86(struct type_state *state,
 			}
 
 			tsr->type = type_die;
+			tsr->kind = TSR_KIND_TYPE;
 			tsr->ok = true;
 
 			pr_debug_dtp("mov [%x] global addr=%"PRIx64" -> reg%d",
 				     insn_offset, addr, dst->reg1);
-			pr_debug_type_name(&type_die);
+			pr_debug_type_name(&type_die, tsr->kind);
+		}
+		/* And check percpu access with base register */
+		else if (has_reg_type(state, sreg) &&
+			 state->regs[sreg].kind == TSR_KIND_PERCPU_BASE) {
+			u64 ip = dloc->ms->sym->start + dl->al.offset;
+			int offset;
+
+			/*
+			 * In kernel, %gs points to a per-cpu region for the
+			 * current CPU.  Access with a constant offset should
+			 * be treated as a global variable access.
+			 */
+			if (get_global_var_type(cu_die, dloc, ip, src->offset,
+						&offset, &type_die) &&
+			    die_get_member_type(&type_die, offset, &type_die)) {
+				tsr->type = type_die;
+				tsr->kind = TSR_KIND_TYPE;
+				tsr->ok = true;
+
+				pr_debug_dtp("mov [%x] percpu %#x(reg%d) -> reg%d type=",
+					     insn_offset, src->offset, sreg, dst->reg1);
+				pr_debug_type_name(&tsr->type, tsr->kind);
+			} else {
+				tsr->ok = false;
+			}
 		}
 		/* Or try another register if any */
 		else if (src->multi_regs && sreg == src->reg1 &&
@@ -677,8 +752,22 @@ static void update_insn_state_x86(struct type_state *state,
 			sreg = src->reg2;
 			goto retry;
 		}
-		/* It failed to get a type info, mark it as invalid */
 		else {
+			int offset;
+			const char *var_name = NULL;
+
+			/* it might be per-cpu variable (in kernel) access */
+			if (src->offset < 0) {
+				if (get_global_var_info(dloc, (s64)src->offset,
+							&var_name, &offset) &&
+				    !strcmp(var_name, "__per_cpu_offset")) {
+					tsr->kind = TSR_KIND_PERCPU_BASE;
+
+					pr_debug_dtp("mov [%x] percpu base reg%d\n",
+						     insn_offset, dst->reg1);
+				}
+			}
+
 			tsr->ok = false;
 		}
 	}
@@ -693,6 +782,8 @@ static void update_insn_state_x86(struct type_state *state,
 			struct type_state_stack *stack;
 			int offset = dst->offset - fboff;
 
+			tsr = &state->regs[src->reg1];
+
 			stack = find_stack_state(state, offset);
 			if (stack) {
 				/*
@@ -703,16 +794,16 @@ static void update_insn_state_x86(struct type_state *state,
 				 * die_get_member_type().
 				 */
 				if (!stack->compound)
-					set_stack_state(stack, offset,
-							&state->regs[src->reg1].type);
+					set_stack_state(stack, offset, tsr->kind,
+							&tsr->type);
 			} else {
-				findnew_stack_state(state, offset,
-						    &state->regs[src->reg1].type);
+				findnew_stack_state(state, offset, tsr->kind,
+						    &tsr->type);
 			}
 
 			pr_debug_dtp("mov [%x] reg%d -> -%#x(stack)",
 				     insn_offset, src->reg1, -offset);
-			pr_debug_type_name(&state->regs[src->reg1].type);
+			pr_debug_type_name(&tsr->type, tsr->kind);
 		}
 		/*
 		 * Ignore other transfers since it'd set a value in a struct
@@ -824,10 +915,11 @@ static bool check_matching_type(struct type_state *state,
 	Dwarf_Word size;
 	u32 insn_offset = dloc->ip - dloc->ms->sym->start;
 
-	pr_debug_dtp("chk [%x] reg%d offset=%#x ok=%d",
-		     insn_offset, reg, dloc->op->offset, state->regs[reg].ok);
+	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);
 
-	if (state->regs[reg].ok) {
+	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_TYPE) {
 		int tag = dwarf_tag(&state->regs[reg].type);
 
 		pr_debug_dtp("\n");
@@ -893,10 +985,25 @@ static bool check_matching_type(struct type_state *state,
 		return true;
 	}
 
+	if (state->regs[reg].kind == TSR_KIND_PERCPU_BASE) {
+		u64 var_addr = dloc->op->offset;
+		int var_offset;
+
+		pr_debug_dtp(" percpu var\n");
+
+		if (get_global_var_type(cu_die, dloc, dloc->ip, var_addr,
+					&var_offset, type_die)) {
+			dloc->type_offset = var_offset;
+			return true;
+		}
+		return false;
+	}
+
 	if (map__dso(dloc->ms->map)->kernel && arch__is(dloc->arch, "x86")) {
 		u64 addr;
 		int offset;
 
+		/* Direct this-cpu access like "%gs:0x34740" */
 		if (dloc->op->segment == INSN_SEG_X86_GS && dloc->op->imm) {
 			pr_debug_dtp(" this-cpu var\n");
 
@@ -907,6 +1014,24 @@ static bool check_matching_type(struct type_state *state,
 				dloc->type_offset = offset;
 				return true;
 			}
+			return false;
+		}
+
+		/* Access to per-cpu base like "-0x7dcf0500(,%rdx,8)" */
+		if (dloc->op->offset < 0 && reg != state->stack_reg) {
+			const char *var_name = NULL;
+
+			addr = (s64) dloc->op->offset;
+
+			if (get_global_var_info(dloc, addr, &var_name, &offset) &&
+			    !strcmp(var_name, "__per_cpu_offset") && offset == 0 &&
+			    get_global_var_type(cu_die, dloc, dloc->ip, addr,
+						&offset, type_die)) {
+				pr_debug_dtp(" percpu base\n");
+
+				dloc->type_offset = offset;
+				return true;
+			}
 		}
 	}
 
@@ -1015,7 +1140,7 @@ static int find_data_type_block(struct data_loc_info *dloc, int reg,
 			ret = 0;
 			pr_debug_dtp("found by insn track: %#x(reg%d) type-offset=%#x",
 				     dloc->op->offset, reg, dloc->type_offset);
-			pr_debug_type_name(type_die);
+			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 			break;
 		}
 
@@ -1147,7 +1272,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
 					     loc->offset, reg, fb_offset, offset);
 			else
 				pr_debug_dtp("%#x(reg%d)", loc->offset, reg);
-			pr_debug_type_name(type_die);
+			pr_debug_type_name(type_die, TSR_KIND_TYPE);
 		}
 		dloc->type_offset = offset;
 		goto out;
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 20/23] perf annotate-data: Handle ADD instructions

[Namhyung Kim]

There are different patterns for percpu variable access using a constant
value added to the base.

  2aeb:  mov    -0x7da0f7e0(,%rax,8),%r14  # r14 = __per_cpu_offset[cpu]
  2af3:  mov    $0x34740,%rax              # rax = address of runqueues
* 2afa:  add    %rax,%r14                  # r14 = &per_cpu(runqueues, cpu)
  2bfd:  cmpl   $0x0,0x10(%r14)            # cpu_rq(cpu)->has_blocked_load
  2b03:  je     0x2b36

At the first instruction, r14 has the __per_cpu_offset.  And then rax
has an immediate value and then added to r14 to calculate the address of
a per-cpu variable.  So it needs to track the immediate values and ADD
instructions.

Similar but a little different case is to use "this_cpu_off" instead of
"__per_cpu_offset" for the current CPU.  This time the variable address
comes with PC-rel addressing.

  89:  mov     $0x34740,%rax                # rax = address of runqueues
* 90:  add     %gs:0x7f015f60(%rip),%rax    # 19a78  <this_cpu_off>
  98:  incl    0xd8c(%rax)                  # cpu_rq(cpu)->sched_count

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 83b5aa00f01c..bd10a576cfbf 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -28,6 +28,8 @@ enum type_state_kind {
 	TSR_KIND_INVALID = 0,
 	TSR_KIND_TYPE,
 	TSR_KIND_PERCPU_BASE,
+	TSR_KIND_CONST,
+	TSR_KIND_POINTER,
 };
 
 #define pr_debug_dtp(fmt, ...)					\
@@ -53,6 +55,13 @@ static void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
 	case TSR_KIND_PERCPU_BASE:
 		pr_info(" percpu base\n");
 		return;
+	case TSR_KIND_CONST:
+		pr_info(" constant\n");
+		return;
+	case TSR_KIND_POINTER:
+		pr_info(" pointer");
+		/* it also prints the type info */
+		break;
 	case TSR_KIND_TYPE:
 	default:
 		break;
@@ -393,7 +402,7 @@ static void set_stack_state(struct type_state_stack *stack, int offset, u8 kind,
 	switch (tag) {
 	case DW_TAG_structure_type:
 	case DW_TAG_union_type:
-		stack->compound = true;
+		stack->compound = (kind != TSR_KIND_POINTER);
 		break;
 	default:
 		stack->compound = false;
@@ -585,6 +594,58 @@ static void update_insn_state_x86(struct type_state *state,
 		return;
 	}
 
+	if (!strncmp(dl->ins.name, "add", 3)) {
+		u64 imm_value = -1ULL;
+		int offset;
+		const char *var_name = NULL;
+		struct map_symbol *ms = dloc->ms;
+		u64 ip = ms->sym->start + dl->al.offset;
+
+		if (!has_reg_type(state, dst->reg1))
+			return;
+
+		tsr = &state->regs[dst->reg1];
+
+		if (src->imm)
+			imm_value = src->offset;
+		else if (has_reg_type(state, src->reg1) &&
+			 state->regs[src->reg1].kind == TSR_KIND_CONST)
+			imm_value = state->regs[src->reg1].imm_value;
+		else if (src->reg1 == DWARF_REG_PC) {
+			u64 var_addr = annotate_calc_pcrel(dloc->ms, ip,
+							   src->offset, dl);
+
+			if (get_global_var_info(dloc, var_addr,
+						&var_name, &offset) &&
+			    !strcmp(var_name, "this_cpu_off") &&
+			    tsr->kind == TSR_KIND_CONST) {
+				tsr->kind = TSR_KIND_PERCPU_BASE;
+				imm_value = tsr->imm_value;
+			}
+		}
+		else
+			return;
+
+		if (tsr->kind != TSR_KIND_PERCPU_BASE)
+			return;
+
+		if (get_global_var_type(cu_die, dloc, ip, imm_value, &offset,
+					&type_die) && offset == 0) {
+			/*
+			 * This is not a pointer type, but it should be treated
+			 * as a pointer.
+			 */
+			tsr->type = type_die;
+			tsr->kind = TSR_KIND_POINTER;
+			tsr->ok = true;
+
+			pr_debug_dtp("add [%x] percpu %#"PRIx64" -> reg%d",
+				     insn_offset, imm_value, dst->reg1);
+			pr_debug_type_name(&tsr->type, tsr->kind);
+		}
+		return;
+	}
+
 	if (strncmp(dl->ins.name, "mov", 3))
 		return;
 
@@ -632,6 +693,16 @@ static void update_insn_state_x86(struct type_state *state,
 			return;
 		}
 
+		if (src->imm) {
+			tsr->kind = TSR_KIND_CONST;
+			tsr->imm_value = src->offset;
+			tsr->ok = true;
+
+			pr_debug_dtp("mov [%x] imm=%#x -> reg%d\n",
+				     insn_offset, tsr->imm_value, dst->reg1);
+			return;
+		}
+
 		if (!has_reg_type(state, src->reg1) ||
 		    !state->regs[src->reg1].ok) {
 			tsr->ok = false;
@@ -739,13 +810,26 @@ static void update_insn_state_x86(struct type_state *state,
 				tsr->kind = TSR_KIND_TYPE;
 				tsr->ok = true;
 
-				pr_debug_dtp("mov [%x] percpu %#x(reg%d) -> reg%d type=",
+				pr_debug_dtp("mov [%x] percpu %#x(reg%d) -> reg%d",
 					     insn_offset, src->offset, sreg, dst->reg1);
 				pr_debug_type_name(&tsr->type, tsr->kind);
 			} else {
 				tsr->ok = false;
 			}
 		}
+		/* And then dereference the calculated pointer if it has one */
+		else if (has_reg_type(state, sreg) && state->regs[sreg].ok &&
+			 state->regs[sreg].kind == TSR_KIND_POINTER &&
+			 die_get_member_type(&state->regs[sreg].type,
+					     src->offset, &type_die)) {
+			tsr->type = type_die;
+			tsr->kind = TSR_KIND_TYPE;
+			tsr->ok = true;
+
+			pr_debug_dtp("mov [%x] pointer %#x(reg%d) -> reg%d",
+				     insn_offset, src->offset, sreg, dst->reg1);
+			pr_debug_type_name(&tsr->type, tsr->kind);
+		}
 		/* Or try another register if any */
 		else if (src->multi_regs && sreg == src->reg1 &&
 			 src->reg1 != src->reg2) {
@@ -999,6 +1083,25 @@ static bool check_matching_type(struct type_state *state,
 		return false;
 	}
 
+	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_POINTER) {
+		pr_debug_dtp(" percpu ptr\n");
+
+		/*
+		 * It's actaully pointer but the address was calculated using
+		 * some arithmetic.  So it points to the actual type already.
+		 */
+		*type_die = state->regs[reg].type;
+
+		dloc->type_offset = dloc->op->offset;
+
+		/* Get the size of the actual type */
+		if (dwarf_aggregate_size(type_die, &size) < 0 ||
+		    (unsigned)dloc->type_offset >= size)
+			return false;
+
+		return true;
+	}
+
 	if (map__dso(dloc->ms->map)->kernel && arch__is(dloc->arch, "x86")) {
 		u64 addr;
 		int offset;
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 21/23] perf annotate-data: Add stack canary type

[Namhyung Kim]

When the stack protector is enabled, compiler would generate code to
check stack overflow with a special value called 'stack carary' at
runtime.  On x86_64, GCC hard-codes the stack canary as %gs:40.

While there's a definition of fixed_percpu_data in asm/processor.h,
it seems that the header is not included everywhere and many places
it cannot find the type info.  As it's in the well-known location (at
%gs:40), let's add a pseudo stack canary type to handle it specially.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 46 +++++++++++++++++++++++++++++++++
 tools/perf/util/annotate-data.h |  1 +
 tools/perf/util/annotate.c      | 25 ++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index bd10a576cfbf..633fe125fcd8 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -30,6 +30,7 @@ enum type_state_kind {
 	TSR_KIND_PERCPU_BASE,
 	TSR_KIND_CONST,
 	TSR_KIND_POINTER,
+	TSR_KIND_CANARY,
 };
 
 #define pr_debug_dtp(fmt, ...)					\
@@ -62,6 +63,9 @@ static void pr_debug_type_name(Dwarf_Die *die, enum type_state_kind kind)
 		pr_info(" pointer");
 		/* it also prints the type info */
 		break;
+	case TSR_KIND_CANARY:
+		pr_info(" stack canary\n");
+		return;
 	case TSR_KIND_TYPE:
 	default:
 		break;
@@ -676,6 +680,15 @@ static void update_insn_state_x86(struct type_state *state,
 			 */
 			var_addr = src->offset;
 
+			if (var_addr == 40) {
+				tsr->kind = TSR_KIND_CANARY;
+				tsr->ok = true;
+
+				pr_debug_dtp("mov [%x] stack canary -> reg%d\n",
+					     insn_offset, dst->reg1);
+				return;
+			}
+
 			if (!get_global_var_type(cu_die, dloc, ip, var_addr,
 						 &offset, &type_die) ||
 			    !die_get_member_type(&type_die, offset, &type_die)) {
@@ -991,6 +1004,16 @@ static void delete_var_types(struct die_var_type *var_types)
 	}
 }
 
+/* should match to is_stack_canary() in util/annotate.c */
+static void setup_stack_canary(struct data_loc_info *dloc)
+{
+	if (arch__is(dloc->arch, "x86")) {
+		dloc->op->segment = INSN_SEG_X86_GS;
+		dloc->op->imm = true;
+		dloc->op->offset = 40;
+	}
+}
+
 /* It's at the target address, check if it has a matching type */
 static bool check_matching_type(struct type_state *state,
 				struct data_loc_info *dloc, int reg,
@@ -1038,6 +1061,11 @@ static bool check_matching_type(struct type_state *state,
 		if (stack == NULL)
 			return false;
 
+		if (stack->kind == TSR_KIND_CANARY) {
+			setup_stack_canary(dloc);
+			return false;
+		}
+
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= stack->offset;
@@ -1062,6 +1090,11 @@ static bool check_matching_type(struct type_state *state,
 		if (stack == NULL)
 			return false;
 
+		if (stack->kind == TSR_KIND_CANARY) {
+			setup_stack_canary(dloc);
+			return false;
+		}
+
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= fboff + stack->offset;
@@ -1102,6 +1135,19 @@ static bool check_matching_type(struct type_state *state,
 		return true;
 	}
 
+	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_CANARY) {
+		pr_debug_dtp(" stack canary\n");
+
+		/*
+		 * This is a saved value of the stack canary which will be handled
+		 * in the outer logic when it returns failure here.  Pretend it's
+		 * from the stack canary directly.
+		 */
+		setup_stack_canary(dloc);
+
+		return false;
+	}
+
 	if (map__dso(dloc->ms->map)->kernel && arch__is(dloc->arch, "x86")) {
 		u64 addr;
 		int offset;
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index ae0f87aed804..1b5a152163b5 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -73,6 +73,7 @@ struct annotated_data_type {
 
 extern struct annotated_data_type unknown_type;
 extern struct annotated_data_type stackop_type;
+extern struct annotated_data_type canary_type;
 
 /**
  * struct data_loc_info - Data location information
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e4121acb4f88..64e54ff1aa1d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -118,6 +118,13 @@ struct annotated_data_type stackop_type = {
 	},
 };
 
+struct annotated_data_type canary_type = {
+	.self = {
+		.type_name = (char *)"(stack canary)",
+		.children = LIST_HEAD_INIT(canary_type.self.children),
+	},
+};
+
 static int arch__grow_instructions(struct arch *arch)
 {
 	struct ins *new_instructions;
@@ -3803,6 +3810,18 @@ static bool is_stack_operation(struct arch *arch, struct disasm_line *dl)
 	return false;
 }
 
+static bool is_stack_canary(struct arch *arch, struct annotated_op_loc *loc)
+{
+	/* On x86_64, %gs:40 is used for stack canary */
+	if (arch__is(arch, "x86")) {
+		if (loc->segment == INSN_SEG_X86_GS && loc->imm &&
+		    loc->offset == 40)
+			return true;
+	}
+
+	return false;
+}
+
 u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
 			struct disasm_line *dl)
 {
@@ -3929,6 +3948,12 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
 		}
 
 		mem_type = find_data_type(&dloc);
+
+		if (mem_type == NULL && is_stack_canary(arch, op_loc)) {
+			mem_type = &canary_type;
+			dloc.type_offset = 0;
+		}
+
 		if (mem_type)
 			istat->good++;
 		else
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 22/23] perf annotate-data: Add a cache for global variable types

[Namhyung Kim]

They are often searched by many different places.  Let's add a cache
for them to reduce the duplicate DWARF access.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
 tools/perf/util/annotate-data.h |   7 +++
 tools/perf/util/dso.c           |   2 +
 tools/perf/util/dso.h           |   6 +-
 4 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 633fe125fcd8..4b3184b7c799 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
 	return stack;
 }
 
+/* Maintain a cache for quick global variable lookup */
+struct global_var_entry {
+	struct rb_node node;
+	char *name;
+	u64 start;
+	u64 end;
+	u64 die_offset;
+};
+
+static int global_var_cmp(const void *_key, const struct rb_node *node)
+{
+	const u64 addr = (uintptr_t)_key;
+	struct global_var_entry *gvar;
+
+	gvar = rb_entry(node, struct global_var_entry, node);
+
+	if (gvar->start <= addr && addr < gvar->end)
+		return 0;
+	return gvar->start > addr ? -1 : 1;
+}
+
+static bool global_var_less(struct rb_node *node_a, const struct rb_node *node_b)
+{
+	struct global_var_entry *gvar_a, *gvar_b;
+
+	gvar_a = rb_entry(node_a, struct global_var_entry, node);
+	gvar_b = rb_entry(node_b, struct global_var_entry, node);
+
+	return gvar_a->start < gvar_b->start;
+}
+
+static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64 addr)
+{
+	struct dso *dso = map__dso(dloc->ms->map);
+	struct rb_node *node;
+
+	node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);
+	if (node == NULL)
+		return NULL;
+
+	return rb_entry(node, struct global_var_entry, node);
+}
+
+static bool global_var__add(struct data_loc_info *dloc, u64 addr,
+			    const char *name, Dwarf_Die *type_die)
+{
+	struct dso *dso = map__dso(dloc->ms->map);
+	struct global_var_entry *gvar;
+	Dwarf_Word size;
+
+	if (dwarf_aggregate_size(type_die, &size) < 0)
+		return false;
+
+	gvar = malloc(sizeof(*gvar));
+	if (gvar == NULL)
+		return false;
+
+	gvar->name = strdup(name);
+	if (gvar->name == NULL) {
+		free(gvar);
+		return false;
+	}
+
+	gvar->start = addr;
+	gvar->end = addr + size;
+	gvar->die_offset = dwarf_dieoffset(type_die);
+
+	rb_add(&gvar->node, &dso->global_vars, global_var_less);
+	return true;
+}
+
+void global_var_type__tree_delete(struct rb_root *root)
+{
+	struct global_var_entry *gvar;
+
+	while (!RB_EMPTY_ROOT(root)) {
+		struct rb_node *node = rb_first(root);
+
+		rb_erase(node, root);
+		gvar = rb_entry(node, struct global_var_entry, node);
+		free(gvar->name);
+		free(gvar);
+	}
+}
+
 static bool get_global_var_info(struct data_loc_info *dloc, u64 addr,
 				const char **var_name, int *var_offset)
 {
@@ -467,14 +552,25 @@ 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;
+	const char *var_name = NULL;
+	struct global_var_entry *gvar;
 	Dwarf_Die var_die;
 
+	gvar = global_var__find(dloc, var_addr);
+	if (gvar) {
+		if (!dwarf_offdie(dloc->di->dbg, gvar->die_offset, type_die))
+			return false;
+
+		*var_offset = var_addr - gvar->start;
+		return true;
+	}
+
 	/* 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) {
+		var_name = dwarf_diename(&var_die);
 		*var_offset = offset;
-		return true;
+		goto ok;
 	}
 
 	if (!get_global_var_info(dloc, var_addr, &var_name, var_offset))
@@ -485,9 +581,14 @@ 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)
-		return true;
+		goto ok;
 
 	return false;
+
+ok:
+	/* The address should point to the start of the variable */
+	global_var__add(dloc, var_addr - *var_offset, var_name, type_die);
+	return true;
 }
 
 /**
diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 1b5a152163b5..fe1e53d6e8c7 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -153,6 +153,9 @@ int annotated_data_type__update_samples(struct annotated_data_type *adt,
 /* Release all data type information in the tree */
 void annotated_data_type__tree_delete(struct rb_root *root);
 
+/* Release all global variable information in the tree */
+void global_var_type__tree_delete(struct rb_root *root);
+
 #else /* HAVE_DWARF_SUPPORT */
 
 static inline struct annotated_data_type *
@@ -175,6 +178,10 @@ static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe
 {
 }
 
+static inline void global_var_type__tree_delete(struct rb_root *root __maybe_unused)
+{
+}
+
 #endif /* HAVE_DWARF_SUPPORT */
 
 #endif /* _PERF_ANNOTATE_DATA_H */
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 22fd5fa806ed..6e2a7198b382 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1329,6 +1329,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
 		dso->inlined_nodes = RB_ROOT_CACHED;
 		dso->srclines = RB_ROOT_CACHED;
 		dso->data_types = RB_ROOT;
+		dso->global_vars = RB_ROOT;
 		dso->data.fd = -1;
 		dso->data.status = DSO_DATA_STATUS_UNKNOWN;
 		dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
@@ -1373,6 +1374,7 @@ void dso__delete(struct dso *dso)
 	dso->symbol_names_len = 0;
 	zfree(&dso->symbol_names);
 	annotated_data_type__tree_delete(&dso->data_types);
+	global_var_type__tree_delete(&dso->global_vars);
 
 	if (dso->short_name_allocated) {
 		zfree((char **)&dso->short_name);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index ce9f3849a773..2cdcd1e2ef8b 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -154,7 +154,8 @@ struct dso {
 	size_t		 symbol_names_len;
 	struct rb_root_cached inlined_nodes;
 	struct rb_root_cached srclines;
-	struct rb_root	data_types;
+	struct rb_root	 data_types;
+	struct rb_root	 global_vars;
 
 	struct {
 		u64		addr;
@@ -411,4 +412,7 @@ int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
 
 void reset_fd_limit(void);
 
+u64 dso__find_global_type(struct dso *dso, u64 addr);
+u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
+
 #endif /* __PERF_DSO */
-- 
2.44.0.291.gc1ea87d7ee-goog

[PATCH 23/23] perf annotate-data: Do not retry for invalid types

[Namhyung Kim]

In some cases, it was able to find a type or location info (for per-cpu
variable) but cannot match because of invalid offset or missing global
information.  In those cases, it's meaningless to go to the outer scope
and retry because there will be no additional information.

Let's change the return type of find_matching_type() and bail out if it
returns -1 for the cases.

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

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 4b3184b7c799..de035db9d9b4 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -1115,10 +1115,15 @@ static void setup_stack_canary(struct data_loc_info *dloc)
 	}
 }
 
-/* It's at the target address, check if it has a matching type */
-static bool check_matching_type(struct type_state *state,
-				struct data_loc_info *dloc, int reg,
-				Dwarf_Die *cu_die, Dwarf_Die *type_die)
+/*
+ * 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
+ * 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, int reg,
+			       Dwarf_Die *cu_die, Dwarf_Die *type_die)
 {
 	Dwarf_Word size;
 	u32 insn_offset = dloc->ip - dloc->ms->sym->start;
@@ -1137,20 +1142,20 @@ static bool check_matching_type(struct type_state *state,
 		 * dereference a memory location.
 		 */
 		if (tag != DW_TAG_pointer_type && tag != DW_TAG_array_type)
-			return false;
+			return -1;
 
 		/* Remove the pointer and get the target type */
 		if (die_get_real_type(&state->regs[reg].type, type_die) == NULL)
-			return false;
+			return -1;
 
 		dloc->type_offset = dloc->op->offset;
 
 		/* Get the size of the actual type */
 		if (dwarf_aggregate_size(type_die, &size) < 0 ||
 		    (unsigned)dloc->type_offset >= size)
-			return false;
+			return -1;
 
-		return true;
+		return 1;
 	}
 
 	if (reg == dloc->fbreg) {
@@ -1160,18 +1165,18 @@ static bool check_matching_type(struct type_state *state,
 
 		stack = find_stack_state(state, dloc->type_offset);
 		if (stack == NULL)
-			return false;
+			return 0;
 
 		if (stack->kind == TSR_KIND_CANARY) {
 			setup_stack_canary(dloc);
-			return false;
+			return -1;
 		}
 
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= stack->offset;
 
-		return true;
+		return 1;
 	}
 
 	if (dloc->fb_cfa) {
@@ -1185,22 +1190,22 @@ static bool check_matching_type(struct type_state *state,
 			fbreg = -1;
 
 		if (reg != fbreg)
-			return false;
+			return 0;
 
 		stack = find_stack_state(state, dloc->type_offset - fboff);
 		if (stack == NULL)
-			return false;
+			return 0;
 
 		if (stack->kind == TSR_KIND_CANARY) {
 			setup_stack_canary(dloc);
-			return false;
+			return -1;
 		}
 
 		*type_die = stack->type;
 		/* Update the type offset from the start of slot */
 		dloc->type_offset -= fboff + stack->offset;
 
-		return true;
+		return 1;
 	}
 
 	if (state->regs[reg].kind == TSR_KIND_PERCPU_BASE) {
@@ -1212,9 +1217,10 @@ static bool 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 true;
+			return 1;
 		}
-		return false;
+		/* No need to retry per-cpu (global) variables */
+		return -1;
 	}
 
 	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_POINTER) {
@@ -1231,9 +1237,9 @@ static bool 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 false;
+			return -1;
 
-		return true;
+		return 1;
 	}
 
 	if (state->regs[reg].ok && state->regs[reg].kind == TSR_KIND_CANARY) {
@@ -1246,7 +1252,7 @@ static bool check_matching_type(struct type_state *state,
 		 */
 		setup_stack_canary(dloc);
 
-		return false;
+		return -1;
 	}
 
 	if (map__dso(dloc->ms->map)->kernel && arch__is(dloc->arch, "x86")) {
@@ -1262,9 +1268,9 @@ static bool check_matching_type(struct type_state *state,
 			if (get_global_var_type(cu_die, dloc, dloc->ip, addr,
 						&offset, type_die)) {
 				dloc->type_offset = offset;
-				return true;
+				return 1;
 			}
-			return false;
+			return -1;
 		}
 
 		/* Access to per-cpu base like "-0x7dcf0500(,%rdx,8)" */
@@ -1280,26 +1286,28 @@ static bool check_matching_type(struct type_state *state,
 				pr_debug_dtp(" percpu base\n");
 
 				dloc->type_offset = offset;
-				return true;
+				return 1;
 			}
+			pr_debug_dtp(" negative offset\n");
+			return -1;
 		}
 	}
 
 	pr_debug_dtp("\n");
-	return false;
+	return 0;
 }
 
 /* Iterate instructions in basic blocks and update type table */
-static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
-				struct list_head *basic_blocks,
-				struct die_var_type *var_types,
-				Dwarf_Die *cu_die, Dwarf_Die *type_die)
+static int find_data_type_insn(struct data_loc_info *dloc, int reg,
+			       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;
-	bool found = false;
+	int ret = 0;
 
 	init_type_state(&state, dloc->arch);
 
@@ -1317,8 +1325,8 @@ static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
 			update_var_state(&state, dloc, addr, dl->al.offset, var_types);
 
 			if (this_ip == dloc->ip) {
-				found = check_matching_type(&state, dloc, reg,
-							    cu_die, type_die);
+				ret = check_matching_type(&state, dloc, reg,
+							  cu_die, type_die);
 				goto out;
 			}
 
@@ -1331,7 +1339,7 @@ static bool find_data_type_insn(struct data_loc_info *dloc, int reg,
 
 out:
 	exit_type_state(&state);
-	return found;
+	return ret;
 }
 
 /*
@@ -1355,6 +1363,7 @@ static int find_data_type_block(struct data_loc_info *dloc, int reg,
 	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;
@@ -1385,15 +1394,19 @@ static int find_data_type_block(struct data_loc_info *dloc, int reg,
 		fixup_var_address(var_types, start);
 
 		/* Find from start of this scope to the target instruction */
-		if (find_data_type_insn(dloc, reg, &basic_blocks, var_types,
-					cu_die, type_die)) {
-			ret = 0;
+		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",
 				     dloc->op->offset, reg, dloc->type_offset);
 			pr_debug_type_name(type_die, TSR_KIND_TYPE);
+			ret = 0;
 			break;
 		}
 
+		if (found < 0)
+			break;
+
 		/* Go up to the next scope and find blocks to the start */
 		prev_dst_ip = dst_ip;
 		dst_ip = src_ip;
-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 01/23] perf dwarf-aux: Remove unused pc argument

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:50:53PM -0700, Namhyung Kim wrote:
> It's not used, let's get rid of it.

I looked for data->pc, there is usage somewhere else, but not via
__die_find_var_addr_cb() nor match_var_offset().

I think renaming:

  struct find_var_data data;

to:

  struct find_var_data var;

And:

  struct find_scope_data data;

to:

  struct find_scope_data scope;

Helps with grepping for 'data->pc', but that can be left for later.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 4 ++--
>  tools/perf/util/dwarf-aux.c     | 7 ++-----
>  tools/perf/util/dwarf-aux.h     | 6 ++----
>  3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 30c4d19fcf11..59ce5f4f4a40 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -263,7 +263,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
>  	offset = loc->offset;
>  
>  	if (reg == DWARF_REG_PC) {
> -		if (die_find_variable_by_addr(&cu_die, pc, addr, &var_die, &offset)) {
> +		if (die_find_variable_by_addr(&cu_die, addr, &var_die, &offset)) {
>  			ret = check_variable(&var_die, type_die, offset,
>  					     /*is_pointer=*/false);
>  			loc->offset = offset;
> @@ -312,7 +312,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
>  	/* Search from the inner-most scope to the outer */
>  	for (i = nr_scopes - 1; i >= 0; i--) {
>  		if (reg == DWARF_REG_PC) {
> -			if (!die_find_variable_by_addr(&scopes[i], pc, addr,
> +			if (!die_find_variable_by_addr(&scopes[i], addr,
>  						       &var_die, &offset))
>  				continue;
>  		} else {
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 2791126069b4..e84d0d6a7750 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1456,7 +1456,6 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
>  /**
>   * die_find_variable_by_addr - Find variable located at given address
>   * @sc_die: a scope DIE
> - * @pc: the program address to find
>   * @addr: the data address to find
>   * @die_mem: a buffer to save the resulting DIE
>   * @offset: the offset in the resulting type
> @@ -1464,12 +1463,10 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
>   * Find the variable DIE located at the given address (in PC-relative mode).
>   * This is usually for global variables.
>   */
> -Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
> -				     Dwarf_Addr addr, Dwarf_Die *die_mem,
> -				     int *offset)
> +Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
> +				     Dwarf_Die *die_mem, int *offset)
>  {
>  	struct find_var_data data = {
> -		.pc = pc,
>  		.addr = addr,
>  	};
>  	Dwarf_Die *result;
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index 85dd527ae1f7..9973801a20c1 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -146,9 +146,8 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
>  				    Dwarf_Die *die_mem);
>  
>  /* Find a (global) variable located in the 'addr' */
> -Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
> -				     Dwarf_Addr addr, Dwarf_Die *die_mem,
> -				     int *offset);
> +Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
> +				     Dwarf_Die *die_mem, int *offset);
>  
>  #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
>  
> @@ -170,7 +169,6 @@ static inline Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die __maybe_unus
>  }
>  
>  static inline Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die __maybe_unused,
> -						   Dwarf_Addr pc __maybe_unused,
>  						   Dwarf_Addr addr __maybe_unused,
>  						   Dwarf_Die *die_mem __maybe_unused,
>  						   int *offset __maybe_unused)
> -- 
> 2.44.0.291.gc1ea87d7ee-goog
> 

Re: [PATCH 02/23] perf dwarf-aux: Add die_collect_vars()

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:50:54PM -0700, Namhyung Kim wrote:
> The die_collect_vars() is to find all variable information in the scope
> including function parameters.  The struct die_var_type is to save the
> type of the variable with the location (reg and offset) as well as where
> it's defined in the code (addr).
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c | 118 +++++++++++++++++++++++++++---------
>  tools/perf/util/dwarf-aux.h |  17 ++++++
>  2 files changed, 107 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index e84d0d6a7750..785aa7a3d725 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1136,6 +1136,40 @@ int die_get_varname(Dwarf_Die *vr_die, struct strbuf *buf)
>  	return ret < 0 ? ret : strbuf_addf(buf, "\t%s", dwarf_diename(vr_die));
>  }
>  
> +#if defined(HAVE_DWARF_GETLOCATIONS_SUPPORT) || defined(HAVE_DWARF_CFI_SUPPORT)
> +static int reg_from_dwarf_op(Dwarf_Op *op)
> +{
> +	switch (op->atom) {
> +	case DW_OP_reg0 ... DW_OP_reg31:
> +		return op->atom - DW_OP_reg0;
> +	case DW_OP_breg0 ... DW_OP_breg31:
> +		return op->atom - DW_OP_breg0;
> +	case DW_OP_regx:
> +	case DW_OP_bregx:
> +		return op->number;
> +	default:
> +		break;
> +	}
> +	return -1;
> +}
> +
> +static int offset_from_dwarf_op(Dwarf_Op *op)
> +{
> +	switch (op->atom) {
> +	case DW_OP_reg0 ... DW_OP_reg31:
> +	case DW_OP_regx:
> +		return 0;
> +	case DW_OP_breg0 ... DW_OP_breg31:
> +		return op->number;
> +	case DW_OP_bregx:
> +		return op->number2;
> +	default:
> +		break;
> +	}
> +	return -1;
> +}
> +#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT || HAVE_DWARF_CFI_SUPPORT */
> +
>  #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
>  /**
>   * die_get_var_innermost_scope - Get innermost scope range of given variable DIE
> @@ -1476,41 +1510,69 @@ Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
>  		*offset = data.offset;
>  	return result;
>  }
> -#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
>  
> -#ifdef HAVE_DWARF_CFI_SUPPORT
> -static int reg_from_dwarf_op(Dwarf_Op *op)
> +static int __die_collect_vars_cb(Dwarf_Die *die_mem, void *arg)
>  {
> -	switch (op->atom) {
> -	case DW_OP_reg0 ... DW_OP_reg31:
> -		return op->atom - DW_OP_reg0;
> -	case DW_OP_breg0 ... DW_OP_breg31:
> -		return op->atom - DW_OP_breg0;
> -	case DW_OP_regx:
> -	case DW_OP_bregx:
> -		return op->number;
> -	default:
> -		break;
> -	}
> -	return -1;
> +	struct die_var_type **var_types = arg;
> +	Dwarf_Die type_die;
> +	int tag = dwarf_tag(die_mem);
> +	Dwarf_Attribute attr;
> +	Dwarf_Addr base, start, end;
> +	Dwarf_Op *ops;
> +	size_t nops;
> +	struct die_var_type *vt;
> +
> +	if (tag != DW_TAG_variable && tag != DW_TAG_formal_parameter)
> +		return DIE_FIND_CB_SIBLING;
> +
> +	if (dwarf_attr(die_mem, DW_AT_location, &attr) == NULL)
> +		return DIE_FIND_CB_SIBLING;
> +
> +	/*
> +	 * Only collect the first location as it can reconstruct the
> +	 * remaining state by following the instructions.
> +	 * start = 0 means it covers the whole range.
> +	 */
> +	if (dwarf_getlocations(&attr, 0, &base, &start, &end, &ops, &nops) <= 0)
> +		return DIE_FIND_CB_SIBLING;
> +
> +	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;
> +
> +	vt->die_off = dwarf_dieoffset(&type_die);
> +	vt->addr = start;
> +	vt->reg = reg_from_dwarf_op(ops);
> +	vt->offset = offset_from_dwarf_op(ops);
> +	vt->next = *var_types;
> +	*var_types = vt;
> +
> +	return DIE_FIND_CB_SIBLING;
>  }
>  
> -static int offset_from_dwarf_op(Dwarf_Op *op)
> +/**
> + * die_collect_vars - Save all variables and parameters
> + * @sc_die: a scope DIE
> + * @var_types: a pointer to save the resulting list
> + *
> + * Save all variables and parameters in the @sc_die and save them to @var_types.
> + * The @var_types is a singly-linked list containing type and location info.
> + * Actual type can be retrieved using dwarf_offdie() with 'die_off' later.
> + *
> + * Callers should free @var_types.
> + */
> +void die_collect_vars(Dwarf_Die *sc_die, struct die_var_type **var_types)
>  {
> -	switch (op->atom) {
> -	case DW_OP_reg0 ... DW_OP_reg31:
> -	case DW_OP_regx:
> -		return 0;
> -	case DW_OP_breg0 ... DW_OP_breg31:
> -		return op->number;
> -	case DW_OP_bregx:
> -		return op->number2;
> -	default:
> -		break;
> -	}
> -	return -1;
> +	Dwarf_Die die_mem;
> +
> +	die_find_child(sc_die, __die_collect_vars_cb, (void *)var_types, &die_mem);
>  }
> +#endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
>  
> +#ifdef HAVE_DWARF_CFI_SUPPORT
>  /**
>   * die_get_cfa - Get frame base information
>   * @dwarf: a Dwarf info
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index 9973801a20c1..cd171b06fd4c 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -135,6 +135,15 @@ void die_skip_prologue(Dwarf_Die *sp_die, Dwarf_Die *cu_die,
>  /* Get the list of including scopes */
>  int die_get_scopes(Dwarf_Die *cu_die, Dwarf_Addr pc, Dwarf_Die **scopes);
>  
> +/* Variable type information */
> +struct die_var_type {
> +	struct die_var_type *next;
> +	u64 die_off;
> +	u64 addr;
> +	int reg;
> +	int offset;
> +};
> +
>  #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
>  
>  /* Get byte offset range of given variable DIE */
> @@ -149,6 +158,9 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
>  Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
>  				     Dwarf_Die *die_mem, int *offset);
>  
> +/* Save all variables and parameters in this scope */
> +void die_collect_vars(Dwarf_Die *sc_die, struct die_var_type **var_types);
> +
>  #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
>  
>  static inline int die_get_var_range(Dwarf_Die *sp_die __maybe_unused,
> @@ -176,6 +188,11 @@ static inline Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die __maybe_unu
>  	return NULL;
>  }
>  
> +static inline void die_collect_vars(Dwarf_Die *sc_die __maybe_unused,
> +				    struct die_var_type **var_types __maybe_unused)
> +{
> +}
> +
>  #endif /* HAVE_DWARF_GETLOCATIONS_SUPPORT */
>  
>  #ifdef HAVE_DWARF_CFI_SUPPORT
> -- 
> 2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 03/23] perf dwarf-aux: Handle type transfer for memory access

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:50:55PM -0700, Namhyung Kim wrote:
> We want to track type states as instructions are executed.  Each
> instruction can access compound types like struct or union and load/
> store its members to a different location.
> 
> The die_deref_ptr_type() is to find a type of memory access with a
> pointer variable.  If it points to a compound type like struct, the
> target memory is a member in the struct.  The access will happen
> with an offset indicating which member it refers.  Let's follow the
> DWARF info to figure out the type of the pointer target.
> 
> For example, say we have the following code.
> 
>   struct foo {
>     int a;
>     int b;
>   };
> 
>   struct foo *p = malloc(sizeof(*p));
>   p->b = 0;
> 
> The last pointer access should produce x86 asm like below:
> 
>   mov  0x0, 4(%rbx)
> 
> And we know %rbx register has a pointer to struct foo.  Then offset 4
> should return the debug info of member 'b'.
> 
> Also variables of compound types can be accessed directly without a
> pointer.  The die_get_member_type() is to handle a such case.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c | 110 ++++++++++++++++++++++++++++++++++++
>  tools/perf/util/dwarf-aux.h |   6 ++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 785aa7a3d725..cd9364d296b6 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1838,3 +1838,113 @@ int die_get_scopes(Dwarf_Die *cu_die, Dwarf_Addr pc, Dwarf_Die **scopes)
>  	*scopes = data.scopes;
>  	return data.nr;
>  }
> +
> +static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
> +{
> +	Dwarf_Die type_die;
> +	Dwarf_Word size, loc;
> +	Dwarf_Word offset = (long)arg;
> +	int tag = dwarf_tag(die_mem);
> +
> +	if (tag != DW_TAG_member)
> +		return DIE_FIND_CB_SIBLING;
> +
> +	/* Unions might not have location */
> +	if (die_get_data_member_location(die_mem, &loc) < 0)
> +		loc = 0;
> +
> +	if (offset == loc)
> +		return DIE_FIND_CB_END;
> +
> +	die_get_real_type(die_mem, &type_die);

Don't we have to check that the type_die was initialized? From what I
can see there is the possibility it isn't:

  Dwarf_Die type_die;
  die_get_real_type(die_mem, &type_die);
        do {
                vr_die = __die_get_real_type(vr_die, die_mem);
        } while (vr_die && dwarf_tag(vr_die) == DW_TAG_typedef);
        
        return vr_die;

    static Dwarf_Die *__die_get_real_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)

        do {
                vr_die = die_get_type(vr_die, die_mem);
                if (!vr_die)
                        break;

   Dwarf_Die *die_get_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)

        Dwarf_Attribute attr;

        if (dwarf_attr_integrate(vr_die, DW_AT_type, &attr) &&
            dwarf_formref_die(&attr, die_mem))
                return die_mem;
        else
                return NULL;
  } 

> +	if (dwarf_aggregate_size(&type_die, &size) < 0)
> +		size = 0;
> +
> +	if (loc < offset && offset < (loc + size))
> +		return DIE_FIND_CB_END;
> +
> +	return DIE_FIND_CB_SIBLING;
> +}
> +
> +/**
> + * die_get_member_type - Return type info of struct member
> + * @type_die: a type DIE
> + * @offset: offset in the type
> + * @die_mem: a buffer to save the resulting DIE
> + *
> + * This function returns a type of a member in @type_die where it's located at
> + * @offset if it's a struct.  For now, it just returns the first matching
> + * member in a union.  For other types, it'd return the given type directly
> + * if it's within the size of the type or NULL otherwise.
> + */
> +Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset,
> +			       Dwarf_Die *die_mem)
> +{
> +	Dwarf_Die *member;
> +	Dwarf_Die mb_type;
> +	int tag;
> +
> +	tag = dwarf_tag(type_die);
> +	/* If it's not a compound type, return the type directly */
> +	if (tag != DW_TAG_structure_type && tag != DW_TAG_union_type) {
> +		Dwarf_Word size;
> +
> +		if (dwarf_aggregate_size(type_die, &size) < 0)
> +			size = 0;
> +
> +		if ((unsigned)offset >= size)
> +			return NULL;
> +
> +		*die_mem = *type_die;
> +		return die_mem;
> +	}
> +
> +	mb_type = *type_die;
> +	/* TODO: Handle union types better? */
> +	while (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
> +		member = die_find_child(&mb_type, __die_find_member_offset_cb,
> +					(void *)(long)offset, die_mem);
> +		if (member == NULL)
> +			return NULL;
> +
> +		if (die_get_real_type(member, &mb_type) == NULL)
> +			return NULL;
> +
> +		tag = dwarf_tag(&mb_type);
> +
> +		if (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
> +			Dwarf_Word loc;
> +
> +			/* Update offset for the start of the member struct */
> +			if (die_get_data_member_location(member, &loc) == 0)
> +				offset -= loc;
> +		}
> +	}
> +	*die_mem = mb_type;
> +	return die_mem;
> +}
> +
> +/**
> + * die_deref_ptr_type - Return type info for pointer access
> + * @ptr_die: a pointer type DIE
> + * @offset: access offset for the pointer
> + * @die_mem: a buffer to save the resulting DIE
> + *
> + * This function follows the pointer in @ptr_die with given @offset
> + * and saves the resulting type in @die_mem.  If the pointer points
> + * a struct type, actual member at the offset would be returned.
> + */
> +Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset,
> +			      Dwarf_Die *die_mem)
> +{
> +	Dwarf_Die type_die;
> +
> +	if (dwarf_tag(ptr_die) != DW_TAG_pointer_type)
> +		return NULL;
> +
> +	if (die_get_real_type(ptr_die, &type_die) == NULL)
> +		return NULL;
> +
> +	return die_get_member_type(&type_die, offset, die_mem);
> +}
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index cd171b06fd4c..16c916311bc0 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -144,6 +144,12 @@ struct die_var_type {
>  	int offset;
>  };
>  
> +/* Return type info of a member at offset */
> +Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset, Dwarf_Die *die_mem);
> +
> +/* 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);
> +
>  #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
>  
>  /* Get byte offset range of given variable DIE */
> -- 
> 2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 04/23] perf dwarf-aux: Add die_find_func_rettype()

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:50:56PM -0700, Namhyung Kim wrote:
> The die_find_func_rettype() is to find a debug entry for the given
> function name and sets the type information of the return value.  By
> convention, it'd return the pointer to the type die (should be the
> same as the given mem_die argument) if found, or NULL otherwise.
> 
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/dwarf-aux.c | 43 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/dwarf-aux.h |  4 ++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index cd9364d296b6..9080119a258c 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -696,6 +696,49 @@ Dwarf_Die *die_find_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
>  	return die_mem;
>  }
>  
> +static int __die_find_func_rettype_cb(Dwarf_Die *die_mem, void *data)
> +{
> +	const char *func_name;
> +
> +	if (dwarf_tag(die_mem) != DW_TAG_subprogram)
> +		return DIE_FIND_CB_SIBLING;
> +
> +	func_name = dwarf_diename(die_mem);
> +	if (func_name && !strcmp(func_name, data))
> +		return DIE_FIND_CB_END;
> +
> +	return DIE_FIND_CB_SIBLING;
> +}
> +
> +/**
> + * die_find_func_rettype - Search a return type of function
> + * @cu_die: a CU DIE
> + * @name: target function name
> + * @die_mem: a buffer for result DIE
> + *
> + * Search a non-inlined function which matches to @name and stores the
> + * return type of the function to @die_mem and returns it if found.
> + * Returns NULL if failed.  Note that it doesn't needs to find a
> + * definition of the function, so it doesn't match with address.
> + * Most likely, it can find a declaration at the top level.  Thus the
> + * callback function continues to sibling entries only.
> + */
> +Dwarf_Die *die_find_func_rettype(Dwarf_Die *cu_die, const char *name,
> +				 Dwarf_Die *die_mem)
> +{
> +	Dwarf_Die tmp_die;
> +
> +	cu_die = die_find_child(cu_die, __die_find_func_rettype_cb,
> +				(void *)name, &tmp_die);
> +	if (!cu_die)
> +		return NULL;
> +
> +	if (die_get_real_type(&tmp_die, die_mem) == NULL)
> +		return NULL;


Here you check die_get_real_type() return, may I go and do the same for
the previous patch to address my review comment?

- Arnaldo

> +
> +	return die_mem;
> +}
> +
>  struct __instance_walk_param {
>  	void    *addr;
>  	int	(*callback)(Dwarf_Die *, void *);
> diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> index 16c916311bc0..b0f25fbf9668 100644
> --- a/tools/perf/util/dwarf-aux.h
> +++ b/tools/perf/util/dwarf-aux.h
> @@ -94,6 +94,10 @@ Dwarf_Die *die_find_top_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
>  Dwarf_Die *die_find_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
>  			       Dwarf_Die *die_mem);
>  
> +/* Search a non-inlined function by name and returns its return type */
> +Dwarf_Die *die_find_func_rettype(Dwarf_Die *sp_die, const char *name,
> +				 Dwarf_Die *die_mem);
> +
>  /* Walk on the instances of given DIE */
>  int die_walk_instances(Dwarf_Die *in_die,
>  		       int (*callback)(Dwarf_Die *, void *), void *data);
> -- 
> 2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 08/23] perf annotate-data: Add debug messages

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:51:00PM -0700, Namhyung Kim wrote:
> Add a new debug option "type-profile" to enable the detailed info during
> the type analysis especially for instruction tracking.  You can use this
> before the command name like 'report' or 'annotate'.
> 
>   $ perf --debug type-profile annotate --data-type

Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

First get some memory events:

  $ perf mem record ls

Then, without data-type profiling debug:

  $ perf annotate --data-type | head
  Annotate type: 'struct rtld_global' in /usr/lib64/ld-linux-x86-64.so.2 (1 samples):
  ============================================================================
      samples     offset       size  field
            1          0       4336  struct rtld_global  {
            0          0          0      struct link_namespaces*        _dl_ns;
            0       2560          8      size_t _dl_nns;
            0       2568         40      __rtld_lock_recursive_t        _dl_load_lock {
            0       2568         40          pthread_mutex_t    mutex {
            0       2568         40              struct __pthread_mutex_s       __data {
            0       2568          4                  int        __lock;
  $

And with only data-type profiling:

  $ perf --debug type-profile annotate --data-type | head
  -----------------------------------------------------------
  find_data_type_die [1e67] for reg13873052 (PC) offset=0x150e2 in dl_main
  CU die offset: 0x29cd3
  found PC-rel by addr=0x34020 offset=0x20
  -----------------------------------------------------------
  find_data_type_die [2e] for reg12 offset=0 in __GI___readdir64
  CU die offset: 0x137a45
  frame base: cfa=1 fbreg=-1
  found "__futex" in scope=2/2 (die: 0x137ad5) 0(reg12) type=int (die:2a)
  -----------------------------------------------------------
  find_data_type_die [52] for reg5 offset=0 in __memmove_avx_unaligned_erms
  CU die offset: 0x1124ed
  no variable found
  Annotate type: 'struct rtld_global' in /usr/lib64/ld-linux-x86-64.so.2 (1 samples):
  ============================================================================
      samples     offset       size  field
            1          0       4336  struct rtld_global  {
            0          0          0      struct link_namespaces*        _dl_ns;
            0       2560          8      size_t _dl_nns;
            0       2568         40      __rtld_lock_recursive_t        _dl_load_lock {
            0       2568         40          pthread_mutex_t    mutex {
            0       2568         40              struct __pthread_mutex_s       __data {
            0       2568          4                  int        __lock;
  $
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 74 +++++++++++++++++++++++++++++----
>  tools/perf/util/debug.c         |  3 ++
>  tools/perf/util/debug.h         |  1 +
>  3 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index ff81d164aa57..f482ccfdaa91 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -23,6 +23,29 @@
>  #include "symbol.h"
>  #include "symbol_conf.h"
>  
> +#define pr_debug_dtp(fmt, ...)					\
> +do {								\
> +	if (debug_type_profile)					\
> +		pr_info(fmt, ##__VA_ARGS__);			\
> +	else							\
> +		pr_debug3(fmt, ##__VA_ARGS__);			\
> +} while (0)
> +
> +static void pr_debug_type_name(Dwarf_Die *die)
> +{
> +	struct strbuf sb;
> +	char *str;
> +
> +	if (!debug_type_profile && verbose < 3)
> +		return;
> +
> +	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));
> +	free(str);
> +}
> +
>  /*
>   * Compare type name and size to maintain them in a tree.
>   * I'm not sure if DWARF would have information of a single type in many
> @@ -201,7 +224,7 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
>  
>  	/* Get the type of the variable */
>  	if (die_get_real_type(var_die, type_die) == NULL) {
> -		pr_debug("variable has no type\n");
> +		pr_debug_dtp("variable has no type\n");
>  		ann_data_stat.no_typeinfo++;
>  		return -1;
>  	}
> @@ -215,7 +238,7 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
>  		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("no pointer or no type\n");
> +			pr_debug_dtp("no pointer or no type\n");
>  			ann_data_stat.no_typeinfo++;
>  			return -1;
>  		}
> @@ -223,14 +246,15 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
>  
>  	/* Get the size of the actual type */
>  	if (dwarf_aggregate_size(type_die, &size) < 0) {
> -		pr_debug("type size is unknown\n");
> +		pr_debug_dtp("type size is unknown\n");
>  		ann_data_stat.invalid_size++;
>  		return -1;
>  	}
>  
>  	/* Minimal sanity check */
>  	if ((unsigned)offset >= size) {
> -		pr_debug("offset: %d is bigger than size: %" PRIu64 "\n", offset, size);
> +		pr_debug_dtp("offset: %d is bigger than size: %"PRIu64"\n",
> +			     offset, size);
>  		ann_data_stat.bad_offset++;
>  		return -1;
>  	}
> @@ -251,6 +275,19 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  	int fb_offset = 0;
>  	bool is_fbreg = false;
>  	u64 pc;
> +	char buf[64];
> +
> +	if (dloc->op->multi_regs)
> +		snprintf(buf, sizeof(buf), " or reg%d", dloc->op->reg2);
> +	else if (dloc->op->reg1 == DWARF_REG_PC)
> +		snprintf(buf, sizeof(buf), " (PC)");
> +	else
> +		buf[0] = '\0';
> +
> +	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);
>  
>  	/*
>  	 * IP is a relative instruction address from the start of the map, as
> @@ -261,7 +298,7 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  
>  	/* Get a compile_unit for this address */
>  	if (!find_cu_die(dloc->di, pc, &cu_die)) {
> -		pr_debug("cannot find CU for address %" PRIx64 "\n", pc);
> +		pr_debug_dtp("cannot find CU for address %"PRIx64"\n", pc);
>  		ann_data_stat.no_cuinfo++;
>  		return -1;
>  	}
> @@ -269,12 +306,17 @@ 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));
> +
>  	if (reg == DWARF_REG_PC) {
>  		if (die_find_variable_by_addr(&cu_die, dloc->var_addr, &var_die,
>  					      &offset)) {
>  			ret = check_variable(&var_die, type_die, offset,
>  					     /*is_pointer=*/false);
>  			dloc->type_offset = offset;
> +
> +			pr_debug_dtp("found PC-rel by addr=%#"PRIx64" offset=%#x\n",
> +				     dloc->var_addr, offset);
>  			goto out;
>  		}
>  
> @@ -310,6 +352,9 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  			default:
>  				break;
>  			}
> +
> +			pr_debug_dtp("frame base: cfa=%d fbreg=%d\n",
> +				     dloc->fb_cfa, fbreg);
>  		}
>  	}
>  
> @@ -334,6 +379,19 @@ 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);
> +		if (ret == 0) {
> +			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);
> +			pr_debug_type_name(type_die);
> +		}
>  		dloc->type_offset = offset;
>  		goto out;
>  	}
> @@ -343,8 +401,10 @@ static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  		goto retry;
>  	}
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pr_debug_dtp("no variable found\n");
>  		ann_data_stat.no_var++;
> +	}
>  
>  out:
>  	free(scopes);
> @@ -373,7 +433,7 @@ struct annotated_data_type *find_data_type(struct data_loc_info *dloc)
>  
>  	dloc->di = debuginfo__new(dso->long_name);
>  	if (dloc->di == NULL) {
> -		pr_debug("cannot get the debug info\n");
> +		pr_debug_dtp("cannot get the debug info\n");
>  		return NULL;
>  	}
>  
> diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
> index c39ee0fcb8cf..d633d15329fa 100644
> --- a/tools/perf/util/debug.c
> +++ b/tools/perf/util/debug.c
> @@ -41,6 +41,7 @@ static int redirect_to_stderr;
>  int debug_data_convert;
>  static FILE *_debug_file;
>  bool debug_display_time;
> +int debug_type_profile;
>  
>  FILE *debug_file(void)
>  {
> @@ -231,6 +232,7 @@ static struct sublevel_option debug_opts[] = {
>  	{ .name = "data-convert",	.value_ptr = &debug_data_convert },
>  	{ .name = "perf-event-open",	.value_ptr = &debug_peo_args },
>  	{ .name = "kmaps",		.value_ptr = &debug_kmaps },
> +	{ .name = "type-profile",	.value_ptr = &debug_type_profile },
>  	{ .name = NULL, }
>  };
>  
> @@ -270,6 +272,7 @@ int perf_quiet_option(void)
>  	redirect_to_stderr = 0;
>  	debug_peo_args = 0;
>  	debug_kmaps = 0;
> +	debug_type_profile = 0;
>  
>  	return 0;
>  }
> diff --git a/tools/perf/util/debug.h b/tools/perf/util/debug.h
> index 35a7a5ae762e..a4026d1fd6a3 100644
> --- a/tools/perf/util/debug.h
> +++ b/tools/perf/util/debug.h
> @@ -14,6 +14,7 @@ extern int debug_peo_args;
>  extern bool quiet, dump_trace;
>  extern int debug_ordered_events;
>  extern int debug_data_convert;
> +extern int debug_type_profile;
>  
>  #ifndef pr_fmt
>  #define pr_fmt(fmt) fmt
> -- 
> 2.44.0.291.gc1ea87d7ee-goog
> 

Re: [PATCH 09/23] perf annotate-data: Maintain variable type info

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:51:01PM -0700, Namhyung Kim wrote:
> As it collected basic block and variable information in each scope, it
> now can build a state table to find matching variable at the location.
> 
> The struct type_state is to keep the type info saved in each register
> and stack slot.  The update_var_state() updates the table when it finds
> variables in the current address.  It expects die_collect_vars() filled
> a list of variables with type info and starting address.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 173 ++++++++++++++++++++++++++++++++
>  tools/perf/util/dwarf-aux.c     |   4 +
>  2 files changed, 177 insertions(+)
> 
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index f482ccfdaa91..8eaa06f1cee5 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -46,6 +46,62 @@ static void pr_debug_type_name(Dwarf_Die *die)
>  	free(str);
>  }
>  
> +/* Type information in a register, valid when ok is true */
> +struct type_state_reg {
> +	Dwarf_Die type;
> +	bool ok;
> +};
> +
> +/* Type information in a stack location, dynamically allocated */
> +struct type_state_stack {
> +	struct list_head list;
> +	Dwarf_Die type;
> +	int offset;
> +	int size;
> +	bool compound;
> +};
> +
> +/* FIXME: This should be arch-dependent */
> +#define TYPE_STATE_MAX_REGS  16
> +
> +/*
> + * State table to maintain type info in each register and stack location.
> + * It'll be updated when new variable is allocated or type info is moved
> + * to a new location (register or stack).  As it'd be used with the
> + * shortest path of basic blocks, it only maintains a single table.
> + */
> +struct type_state {
> +	struct type_state_reg regs[TYPE_STATE_MAX_REGS];
> +	struct list_head stack_vars;
> +};
> +
> +static bool has_reg_type(struct type_state *state, int reg)
> +{
> +	return (unsigned)reg < ARRAY_SIZE(state->regs);
> +}
> +
> +/* These declarations will be remove once they are changed to static */
> +void init_type_state(struct type_state *state, struct arch *arch __maybe_unused);
> +void exit_type_state(struct type_state *state);
> +void update_var_state(struct type_state *state, struct data_loc_info *dloc,
> +		      u64 addr, u64 insn_offset, struct die_var_type *var_types);
> +
> +void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
> +{
> +	memset(state, 0, sizeof(*state));
> +	INIT_LIST_HEAD(&state->stack_vars);
> +}
> +
> +void exit_type_state(struct type_state *state)
> +{
> +	struct type_state_stack *stack, *tmp;
> +
> +	list_for_each_entry_safe(stack, tmp, &state->stack_vars, list) {
> +		list_del(&stack->list);

list_del_init()?

> +		free(stack);
> +	}
> +}
> +
>  /*
>   * Compare type name and size to maintain them in a tree.
>   * I'm not sure if DWARF would have information of a single type in many
> @@ -262,6 +318,123 @@ static int check_variable(Dwarf_Die *var_die, Dwarf_Die *type_die, int offset,
>  	return 0;
>  }
>  
> +static struct type_state_stack *find_stack_state(struct type_state *state,
> +						 int offset)
> +{
> +	struct type_state_stack *stack;
> +
> +	list_for_each_entry(stack, &state->stack_vars, list) {
> +		if (offset == stack->offset)
> +			return stack;
> +
> +		if (stack->compound && stack->offset < offset &&
> +		    offset < stack->offset + stack->size)
> +			return stack;
> +	}
> +	return NULL;
> +}
> +
> +static void set_stack_state(struct type_state_stack *stack, int offset,
> +			    Dwarf_Die *type_die)
> +{
> +	int tag;
> +	Dwarf_Word size;
> +
> +	if (dwarf_aggregate_size(type_die, &size) < 0)
> +		size = 0;
> +
> +	tag = dwarf_tag(type_die);
> +
> +	stack->type = *type_die;
> +	stack->size = size;
> +	stack->offset = offset;
> +
> +	switch (tag) {
> +	case DW_TAG_structure_type:
> +	case DW_TAG_union_type:
> +		stack->compound = true;
> +		break;
> +	default:
> +		stack->compound = false;
> +		break;
> +	}
> +}
> +
> +static struct type_state_stack *findnew_stack_state(struct type_state *state,
> +						    int offset, Dwarf_Die *type_die)
> +{
> +	struct type_state_stack *stack = find_stack_state(state, offset);
> +
> +	if (stack) {
> +		set_stack_state(stack, offset, type_die);
> +		return stack;
> +	}
> +
> +	stack = malloc(sizeof(*stack));
> +	if (stack) {
> +		set_stack_state(stack, offset, type_die);
> +		list_add(&stack->list, &state->stack_vars);
> +	}
> +	return stack;
> +}
> +
> +/**
> + * update_var_state - Update type state using given variables
> + * @state: type state table
> + * @dloc: data location info
> + * @addr: instruction address to match with variable
> + * @insn_offset: instruction offset (for debug)
> + * @var_types: list of variables with type info
> + *
> + * This function fills the @state table using @var_types info.  Each variable
> + * is used only at the given location and updates an entry in the table.
> + */
> +void update_var_state(struct type_state *state, struct data_loc_info *dloc,
> +		      u64 addr, u64 insn_offset, struct die_var_type *var_types)
> +{
> +	Dwarf_Die mem_die;
> +	struct die_var_type *var;
> +	int fbreg = dloc->fbreg;
> +	int fb_offset = 0;
> +
> +	if (dloc->fb_cfa) {
> +		if (die_get_cfa(dloc->di->dbg, addr, &fbreg, &fb_offset) < 0)
> +			fbreg = -1;
> +	}
> +
> +	for (var = var_types; var != NULL; var = var->next) {
> +		if (var->addr != addr)
> +			continue;
> +		/* Get the type DIE using the offset */
> +		if (!dwarf_offdie(dloc->di->dbg, var->die_off, &mem_die))
> +			continue;
> +
> +		if (var->reg == DWARF_REG_FB) {
> +			findnew_stack_state(state, var->offset, &mem_die);
> +
> +			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
> +				     insn_offset, -var->offset);
> +			pr_debug_type_name(&mem_die);
> +		} else if (var->reg == fbreg) {
> +			findnew_stack_state(state, var->offset - fb_offset, &mem_die);
> +
> +			pr_debug_dtp("var [%"PRIx64"] -%#x(stack) type=",
> +				     insn_offset, -var->offset + fb_offset);
> +			pr_debug_type_name(&mem_die);
> +		} else if (has_reg_type(state, var->reg) && var->offset == 0) {
> +			struct type_state_reg *reg;
> +
> +			reg = &state->regs[var->reg];
> +			reg->type = mem_die;
> +			reg->ok = true;
> +
> +			pr_debug_dtp("var [%"PRIx64"] reg%d type=",
> +				     insn_offset, var->reg);
> +			pr_debug_type_name(&mem_die);
> +		}
> +	}
> +}
> +
>  /* The result will be saved in @type_die */
>  static int find_data_type_die(struct data_loc_info *dloc, Dwarf_Die *type_die)
>  {
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 9080119a258c..41dbbb25b256 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -9,6 +9,7 @@
>  #include <stdlib.h>
>  #include "debug.h"
>  #include "dwarf-aux.h"
> +#include "dwarf-regs.h"
>  #include "strbuf.h"
>  #include "string2.h"
>  
> @@ -1190,6 +1191,8 @@ static int reg_from_dwarf_op(Dwarf_Op *op)
>  	case DW_OP_regx:
>  	case DW_OP_bregx:
>  		return op->number;
> +	case DW_OP_fbreg:
> +		return DWARF_REG_FB;
>  	default:
>  		break;
>  	}
> @@ -1203,6 +1206,7 @@ static int offset_from_dwarf_op(Dwarf_Op *op)
>  	case DW_OP_regx:
>  		return 0;
>  	case DW_OP_breg0 ... DW_OP_breg31:
> +	case DW_OP_fbreg:
>  		return op->number;
>  	case DW_OP_bregx:
>  		return op->number2;
> -- 
> 2.44.0.291.gc1ea87d7ee-goog

Re: [PATCHSET 00/23] Remaining bits of data type profiling (v7)

[Arnaldo Carvalho de Melo]

On Mon, Mar 18, 2024 at 10:50:52PM -0700, Namhyung Kim wrote:
> 
> Hello,
> 
> This is the last part of the data type profiling series.
> So far we added the basic pointer variable support, and direct access to
> global/local variables.  Now it's time to add instruction tracking. :)
> 
> For the history and background, you can refer to the previous version
> [1] and the LWN article [2].

Most of my comments are minor and can be addressed later, I've applied
in my local repo, will go thru the usual build tests and then I'll do
further usage tests.

Thanks,

- Arnaldo

Re: [PATCH 01/23] perf dwarf-aux: Remove unused pc argument

[Namhyung Kim]

On Tue, Mar 19, 2024 at 6:43 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 10:50:53PM -0700, Namhyung Kim wrote:
> > It's not used, let's get rid of it.
>
> I looked for data->pc, there is usage somewhere else, but not via
> __die_find_var_addr_cb() nor match_var_offset().
>
> I think renaming:
>
>   struct find_var_data data;
>
> to:
>
>   struct find_var_data var;
>
> And:
>
>   struct find_scope_data data;
>
> to:
>
>   struct find_scope_data scope;
>
> Helps with grepping for 'data->pc', but that can be left for later.

Ok, I can rename them later.

>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Thanks for the review!
Namhyung

>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/annotate-data.c | 4 ++--
> >  tools/perf/util/dwarf-aux.c     | 7 ++-----
> >  tools/perf/util/dwarf-aux.h     | 6 ++----
> >  3 files changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index 30c4d19fcf11..59ce5f4f4a40 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> > @@ -263,7 +263,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
> >       offset = loc->offset;
> >
> >       if (reg == DWARF_REG_PC) {
> > -             if (die_find_variable_by_addr(&cu_die, pc, addr, &var_die, &offset)) {
> > +             if (die_find_variable_by_addr(&cu_die, addr, &var_die, &offset)) {
> >                       ret = check_variable(&var_die, type_die, offset,
> >                                            /*is_pointer=*/false);
> >                       loc->offset = offset;
> > @@ -312,7 +312,7 @@ static int find_data_type_die(struct debuginfo *di, u64 pc, u64 addr,
> >       /* Search from the inner-most scope to the outer */
> >       for (i = nr_scopes - 1; i >= 0; i--) {
> >               if (reg == DWARF_REG_PC) {
> > -                     if (!die_find_variable_by_addr(&scopes[i], pc, addr,
> > +                     if (!die_find_variable_by_addr(&scopes[i], addr,
> >                                                      &var_die, &offset))
> >                               continue;
> >               } else {
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index 2791126069b4..e84d0d6a7750 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -1456,7 +1456,6 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
> >  /**
> >   * die_find_variable_by_addr - Find variable located at given address
> >   * @sc_die: a scope DIE
> > - * @pc: the program address to find
> >   * @addr: the data address to find
> >   * @die_mem: a buffer to save the resulting DIE
> >   * @offset: the offset in the resulting type
> > @@ -1464,12 +1463,10 @@ static int __die_find_var_addr_cb(Dwarf_Die *die_mem, void *arg)
> >   * Find the variable DIE located at the given address (in PC-relative mode).
> >   * This is usually for global variables.
> >   */
> > -Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
> > -                                  Dwarf_Addr addr, Dwarf_Die *die_mem,
> > -                                  int *offset)
> > +Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
> > +                                  Dwarf_Die *die_mem, int *offset)
> >  {
> >       struct find_var_data data = {
> > -             .pc = pc,
> >               .addr = addr,
> >       };
> >       Dwarf_Die *result;
> > diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> > index 85dd527ae1f7..9973801a20c1 100644
> > --- a/tools/perf/util/dwarf-aux.h
> > +++ b/tools/perf/util/dwarf-aux.h
> > @@ -146,9 +146,8 @@ Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die, Dwarf_Addr pc, int reg,
> >                                   Dwarf_Die *die_mem);
> >
> >  /* Find a (global) variable located in the 'addr' */
> > -Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
> > -                                  Dwarf_Addr addr, Dwarf_Die *die_mem,
> > -                                  int *offset);
> > +Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr addr,
> > +                                  Dwarf_Die *die_mem, int *offset);
> >
> >  #else /*  HAVE_DWARF_GETLOCATIONS_SUPPORT */
> >
> > @@ -170,7 +169,6 @@ static inline Dwarf_Die *die_find_variable_by_reg(Dwarf_Die *sc_die __maybe_unus
> >  }
> >
> >  static inline Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die __maybe_unused,
> > -                                                Dwarf_Addr pc __maybe_unused,
> >                                                  Dwarf_Addr addr __maybe_unused,
> >                                                  Dwarf_Die *die_mem __maybe_unused,
> >                                                  int *offset __maybe_unused)
> > --
> > 2.44.0.291.gc1ea87d7ee-goog
> >

Re: [PATCH 03/23] perf dwarf-aux: Handle type transfer for memory access

[Namhyung Kim]

On Tue, Mar 19, 2024 at 6:55 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 10:50:55PM -0700, Namhyung Kim wrote:
> > We want to track type states as instructions are executed.  Each
> > instruction can access compound types like struct or union and load/
> > store its members to a different location.
> >
> > The die_deref_ptr_type() is to find a type of memory access with a
> > pointer variable.  If it points to a compound type like struct, the
> > target memory is a member in the struct.  The access will happen
> > with an offset indicating which member it refers.  Let's follow the
> > DWARF info to figure out the type of the pointer target.
> >
> > For example, say we have the following code.
> >
> >   struct foo {
> >     int a;
> >     int b;
> >   };
> >
> >   struct foo *p = malloc(sizeof(*p));
> >   p->b = 0;
> >
> > The last pointer access should produce x86 asm like below:
> >
> >   mov  0x0, 4(%rbx)
> >
> > And we know %rbx register has a pointer to struct foo.  Then offset 4
> > should return the debug info of member 'b'.
> >
> > Also variables of compound types can be accessed directly without a
> > pointer.  The die_get_member_type() is to handle a such case.
> >
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/dwarf-aux.c | 110 ++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/dwarf-aux.h |   6 ++
> >  2 files changed, 116 insertions(+)
> >
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index 785aa7a3d725..cd9364d296b6 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -1838,3 +1838,113 @@ int die_get_scopes(Dwarf_Die *cu_die, Dwarf_Addr pc, Dwarf_Die **scopes)
> >       *scopes = data.scopes;
> >       return data.nr;
> >  }
> > +
> > +static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
> > +{
> > +     Dwarf_Die type_die;
> > +     Dwarf_Word size, loc;
> > +     Dwarf_Word offset = (long)arg;
> > +     int tag = dwarf_tag(die_mem);
> > +
> > +     if (tag != DW_TAG_member)
> > +             return DIE_FIND_CB_SIBLING;
> > +
> > +     /* Unions might not have location */
> > +     if (die_get_data_member_location(die_mem, &loc) < 0)
> > +             loc = 0;
> > +
> > +     if (offset == loc)
> > +             return DIE_FIND_CB_END;
> > +
> > +     die_get_real_type(die_mem, &type_die);
>
> Don't we have to check that the type_die was initialized? From what I
> can see there is the possibility it isn't:

Right, it might return NULL if the entry doesn't have a valid
DW_AT_type.  I'll check the return value and skip.

Thanks,
Namhyung

>
>   Dwarf_Die type_die;
>   die_get_real_type(die_mem, &type_die);
>         do {
>                 vr_die = __die_get_real_type(vr_die, die_mem);
>         } while (vr_die && dwarf_tag(vr_die) == DW_TAG_typedef);
>
>         return vr_die;
>
>     static Dwarf_Die *__die_get_real_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)
>
>         do {
>                 vr_die = die_get_type(vr_die, die_mem);
>                 if (!vr_die)
>                         break;
>
>    Dwarf_Die *die_get_type(Dwarf_Die *vr_die, Dwarf_Die *die_mem)
>
>         Dwarf_Attribute attr;
>
>         if (dwarf_attr_integrate(vr_die, DW_AT_type, &attr) &&
>             dwarf_formref_die(&attr, die_mem))
>                 return die_mem;
>         else
>                 return NULL;
>   }
>
> > +     if (dwarf_aggregate_size(&type_die, &size) < 0)
> > +             size = 0;
> > +
> > +     if (loc < offset && offset < (loc + size))
> > +             return DIE_FIND_CB_END;
> > +
> > +     return DIE_FIND_CB_SIBLING;
> > +}
> > +
> > +/**
> > + * die_get_member_type - Return type info of struct member
> > + * @type_die: a type DIE
> > + * @offset: offset in the type
> > + * @die_mem: a buffer to save the resulting DIE
> > + *
> > + * This function returns a type of a member in @type_die where it's located at
> > + * @offset if it's a struct.  For now, it just returns the first matching
> > + * member in a union.  For other types, it'd return the given type directly
> > + * if it's within the size of the type or NULL otherwise.
> > + */
> > +Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset,
> > +                            Dwarf_Die *die_mem)
> > +{
> > +     Dwarf_Die *member;
> > +     Dwarf_Die mb_type;
> > +     int tag;
> > +
> > +     tag = dwarf_tag(type_die);
> > +     /* If it's not a compound type, return the type directly */
> > +     if (tag != DW_TAG_structure_type && tag != DW_TAG_union_type) {
> > +             Dwarf_Word size;
> > +
> > +             if (dwarf_aggregate_size(type_die, &size) < 0)
> > +                     size = 0;
> > +
> > +             if ((unsigned)offset >= size)
> > +                     return NULL;
> > +
> > +             *die_mem = *type_die;
> > +             return die_mem;
> > +     }
> > +
> > +     mb_type = *type_die;
> > +     /* TODO: Handle union types better? */
> > +     while (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
> > +             member = die_find_child(&mb_type, __die_find_member_offset_cb,
> > +                                     (void *)(long)offset, die_mem);
> > +             if (member == NULL)
> > +                     return NULL;
> > +
> > +             if (die_get_real_type(member, &mb_type) == NULL)
> > +                     return NULL;
> > +
> > +             tag = dwarf_tag(&mb_type);
> > +
> > +             if (tag == DW_TAG_structure_type || tag == DW_TAG_union_type) {
> > +                     Dwarf_Word loc;
> > +
> > +                     /* Update offset for the start of the member struct */
> > +                     if (die_get_data_member_location(member, &loc) == 0)
> > +                             offset -= loc;
> > +             }
> > +     }
> > +     *die_mem = mb_type;
> > +     return die_mem;
> > +}
> > +
> > +/**
> > + * die_deref_ptr_type - Return type info for pointer access
> > + * @ptr_die: a pointer type DIE
> > + * @offset: access offset for the pointer
> > + * @die_mem: a buffer to save the resulting DIE
> > + *
> > + * This function follows the pointer in @ptr_die with given @offset
> > + * and saves the resulting type in @die_mem.  If the pointer points
> > + * a struct type, actual member at the offset would be returned.
> > + */
> > +Dwarf_Die *die_deref_ptr_type(Dwarf_Die *ptr_die, int offset,
> > +                           Dwarf_Die *die_mem)
> > +{
> > +     Dwarf_Die type_die;
> > +
> > +     if (dwarf_tag(ptr_die) != DW_TAG_pointer_type)
> > +             return NULL;
> > +
> > +     if (die_get_real_type(ptr_die, &type_die) == NULL)
> > +             return NULL;
> > +
> > +     return die_get_member_type(&type_die, offset, die_mem);
> > +}
> > diff --git a/tools/perf/util/dwarf-aux.h b/tools/perf/util/dwarf-aux.h
> > index cd171b06fd4c..16c916311bc0 100644
> > --- a/tools/perf/util/dwarf-aux.h
> > +++ b/tools/perf/util/dwarf-aux.h
> > @@ -144,6 +144,12 @@ struct die_var_type {
> >       int offset;
> >  };
> >
> > +/* Return type info of a member at offset */
> > +Dwarf_Die *die_get_member_type(Dwarf_Die *type_die, int offset, Dwarf_Die *die_mem);
> > +
> > +/* 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);
> > +
> >  #ifdef HAVE_DWARF_GETLOCATIONS_SUPPORT
> >
> >  /* Get byte offset range of given variable DIE */
> > --
> > 2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH 04/23] perf dwarf-aux: Add die_find_func_rettype()

[Namhyung Kim]

On Tue, Mar 19, 2024 at 6:56 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 10:50:56PM -0700, Namhyung Kim wrote:
> > The die_find_func_rettype() is to find a debug entry for the given
> > function name and sets the type information of the return value.  By
> > convention, it'd return the pointer to the type die (should be the
> > same as the given mem_die argument) if found, or NULL otherwise.
> >
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/dwarf-aux.c | 43 +++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/dwarf-aux.h |  4 ++++
> >  2 files changed, 47 insertions(+)
> >
> > diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> > index cd9364d296b6..9080119a258c 100644
> > --- a/tools/perf/util/dwarf-aux.c
> > +++ b/tools/perf/util/dwarf-aux.c
> > @@ -696,6 +696,49 @@ Dwarf_Die *die_find_inlinefunc(Dwarf_Die *sp_die, Dwarf_Addr addr,
> >       return die_mem;
> >  }
> >
> > +static int __die_find_func_rettype_cb(Dwarf_Die *die_mem, void *data)
> > +{
> > +     const char *func_name;
> > +
> > +     if (dwarf_tag(die_mem) != DW_TAG_subprogram)
> > +             return DIE_FIND_CB_SIBLING;
> > +
> > +     func_name = dwarf_diename(die_mem);
> > +     if (func_name && !strcmp(func_name, data))
> > +             return DIE_FIND_CB_END;
> > +
> > +     return DIE_FIND_CB_SIBLING;
> > +}
> > +
> > +/**
> > + * die_find_func_rettype - Search a return type of function
> > + * @cu_die: a CU DIE
> > + * @name: target function name
> > + * @die_mem: a buffer for result DIE
> > + *
> > + * Search a non-inlined function which matches to @name and stores the
> > + * return type of the function to @die_mem and returns it if found.
> > + * Returns NULL if failed.  Note that it doesn't needs to find a
> > + * definition of the function, so it doesn't match with address.
> > + * Most likely, it can find a declaration at the top level.  Thus the
> > + * callback function continues to sibling entries only.
> > + */
> > +Dwarf_Die *die_find_func_rettype(Dwarf_Die *cu_die, const char *name,
> > +                              Dwarf_Die *die_mem)
> > +{
> > +     Dwarf_Die tmp_die;
> > +
> > +     cu_die = die_find_child(cu_die, __die_find_func_rettype_cb,
> > +                             (void *)name, &tmp_die);
> > +     if (!cu_die)
> > +             return NULL;
> > +
> > +     if (die_get_real_type(&tmp_die, die_mem) == NULL)
> > +             return NULL;
>
>
> Here you check die_get_real_type() return, may I go and do the same for
> the previous patch to address my review comment?

Sure thing! :)

Re: [PATCH 09/23] perf annotate-data: Maintain variable type info

[Namhyung Kim]

On Tue, Mar 19, 2024 at 7:07 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 10:51:01PM -0700, Namhyung Kim wrote:
> > As it collected basic block and variable information in each scope, it
> > now can build a state table to find matching variable at the location.
> >
> > The struct type_state is to keep the type info saved in each register
> > and stack slot.  The update_var_state() updates the table when it finds
> > variables in the current address.  It expects die_collect_vars() filled
> > a list of variables with type info and starting address.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/annotate-data.c | 173 ++++++++++++++++++++++++++++++++
> >  tools/perf/util/dwarf-aux.c     |   4 +
> >  2 files changed, 177 insertions(+)
> >
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index f482ccfdaa91..8eaa06f1cee5 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> > @@ -46,6 +46,62 @@ static void pr_debug_type_name(Dwarf_Die *die)
> >       free(str);
> >  }
> >
> > +/* Type information in a register, valid when ok is true */
> > +struct type_state_reg {
> > +     Dwarf_Die type;
> > +     bool ok;
> > +};
> > +
> > +/* Type information in a stack location, dynamically allocated */
> > +struct type_state_stack {
> > +     struct list_head list;
> > +     Dwarf_Die type;
> > +     int offset;
> > +     int size;
> > +     bool compound;
> > +};
> > +
> > +/* FIXME: This should be arch-dependent */
> > +#define TYPE_STATE_MAX_REGS  16
> > +
> > +/*
> > + * State table to maintain type info in each register and stack location.
> > + * It'll be updated when new variable is allocated or type info is moved
> > + * to a new location (register or stack).  As it'd be used with the
> > + * shortest path of basic blocks, it only maintains a single table.
> > + */
> > +struct type_state {
> > +     struct type_state_reg regs[TYPE_STATE_MAX_REGS];
> > +     struct list_head stack_vars;
> > +};
> > +
> > +static bool has_reg_type(struct type_state *state, int reg)
> > +{
> > +     return (unsigned)reg < ARRAY_SIZE(state->regs);
> > +}
> > +
> > +/* These declarations will be remove once they are changed to static */
> > +void init_type_state(struct type_state *state, struct arch *arch __maybe_unused);
> > +void exit_type_state(struct type_state *state);
> > +void update_var_state(struct type_state *state, struct data_loc_info *dloc,
> > +                   u64 addr, u64 insn_offset, struct die_var_type *var_types);
> > +
> > +void init_type_state(struct type_state *state, struct arch *arch __maybe_unused)
> > +{
> > +     memset(state, 0, sizeof(*state));
> > +     INIT_LIST_HEAD(&state->stack_vars);
> > +}
> > +
> > +void exit_type_state(struct type_state *state)
> > +{
> > +     struct type_state_stack *stack, *tmp;
> > +
> > +     list_for_each_entry_safe(stack, tmp, &state->stack_vars, list) {
> > +             list_del(&stack->list);
>
> list_del_init()?

Maybe.. but I'm not sure how much value it'd have as we free it right after.

Thanks,
Namhyung

>
> > +             free(stack);
> > +     }
> > +}
> > +

Re: [PATCH 22/23] perf annotate-data: Add a cache for global variable types

[Namhyung Kim]

On Mon, Mar 18, 2024 at 10:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> They are often searched by many different places.  Let's add a cache
> for them to reduce the duplicate DWARF access.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
>  tools/perf/util/annotate-data.h |   7 +++
>  tools/perf/util/dso.c           |   2 +
>  tools/perf/util/dso.h           |   6 +-
>  4 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 633fe125fcd8..4b3184b7c799 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
>         return stack;
>  }
>
> +/* Maintain a cache for quick global variable lookup */
> +struct global_var_entry {
> +       struct rb_node node;
> +       char *name;
> +       u64 start;
> +       u64 end;
> +       u64 die_offset;
> +};
> +
> +static int global_var_cmp(const void *_key, const struct rb_node *node)
> +{
> +       const u64 addr = (uintptr_t)_key;
> +       struct global_var_entry *gvar;
> +
> +       gvar = rb_entry(node, struct global_var_entry, node);
> +
> +       if (gvar->start <= addr && addr < gvar->end)
> +               return 0;
> +       return gvar->start > addr ? -1 : 1;
> +}
> +
> +static bool global_var_less(struct rb_node *node_a, const struct rb_node *node_b)
> +{
> +       struct global_var_entry *gvar_a, *gvar_b;
> +
> +       gvar_a = rb_entry(node_a, struct global_var_entry, node);
> +       gvar_b = rb_entry(node_b, struct global_var_entry, node);
> +
> +       return gvar_a->start < gvar_b->start;
> +}
> +
> +static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64 addr)
> +{
> +       struct dso *dso = map__dso(dloc->ms->map);
> +       struct rb_node *node;
> +
> +       node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);

It seems to cause a build error on 32-bit systems.  It needs one
more cast to suppress the "pointer cast w/ different size" warning.

    node = rb_find(void *)(uintptr_tr)addr, ...);

Thanks,
Namhyung



> +       if (node == NULL)
> +               return NULL;
> +
> +       return rb_entry(node, struct global_var_entry, node);
> +}
> +
> +static bool global_var__add(struct data_loc_info *dloc, u64 addr,
> +                           const char *name, Dwarf_Die *type_die)
> +{
> +       struct dso *dso = map__dso(dloc->ms->map);
> +       struct global_var_entry *gvar;
> +       Dwarf_Word size;
> +
> +       if (dwarf_aggregate_size(type_die, &size) < 0)
> +               return false;
> +
> +       gvar = malloc(sizeof(*gvar));
> +       if (gvar == NULL)
> +               return false;
> +
> +       gvar->name = strdup(name);
> +       if (gvar->name == NULL) {
> +               free(gvar);
> +               return false;
> +       }
> +
> +       gvar->start = addr;
> +       gvar->end = addr + size;
> +       gvar->die_offset = dwarf_dieoffset(type_die);
> +
> +       rb_add(&gvar->node, &dso->global_vars, global_var_less);
> +       return true;
> +}
> +
> +void global_var_type__tree_delete(struct rb_root *root)
> +{
> +       struct global_var_entry *gvar;
> +
> +       while (!RB_EMPTY_ROOT(root)) {
> +               struct rb_node *node = rb_first(root);
> +
> +               rb_erase(node, root);
> +               gvar = rb_entry(node, struct global_var_entry, node);
> +               free(gvar->name);
> +               free(gvar);
> +       }
> +}
> +
>  static bool get_global_var_info(struct data_loc_info *dloc, u64 addr,
>                                 const char **var_name, int *var_offset)
>  {
> @@ -467,14 +552,25 @@ 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;
> +       const char *var_name = NULL;
> +       struct global_var_entry *gvar;
>         Dwarf_Die var_die;
>
> +       gvar = global_var__find(dloc, var_addr);
> +       if (gvar) {
> +               if (!dwarf_offdie(dloc->di->dbg, gvar->die_offset, type_die))
> +                       return false;
> +
> +               *var_offset = var_addr - gvar->start;
> +               return true;
> +       }
> +
>         /* 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) {
> +               var_name = dwarf_diename(&var_die);
>                 *var_offset = offset;
> -               return true;
> +               goto ok;
>         }
>
>         if (!get_global_var_info(dloc, var_addr, &var_name, var_offset))
> @@ -485,9 +581,14 @@ 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)
> -               return true;
> +               goto ok;
>
>         return false;
> +
> +ok:
> +       /* The address should point to the start of the variable */
> +       global_var__add(dloc, var_addr - *var_offset, var_name, type_die);
> +       return true;
>  }
>
>  /**
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index 1b5a152163b5..fe1e53d6e8c7 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -153,6 +153,9 @@ int annotated_data_type__update_samples(struct annotated_data_type *adt,
>  /* Release all data type information in the tree */
>  void annotated_data_type__tree_delete(struct rb_root *root);
>
> +/* Release all global variable information in the tree */
> +void global_var_type__tree_delete(struct rb_root *root);
> +
>  #else /* HAVE_DWARF_SUPPORT */
>
>  static inline struct annotated_data_type *
> @@ -175,6 +178,10 @@ static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe
>  {
>  }
>
> +static inline void global_var_type__tree_delete(struct rb_root *root __maybe_unused)
> +{
> +}
> +
>  #endif /* HAVE_DWARF_SUPPORT */
>
>  #endif /* _PERF_ANNOTATE_DATA_H */
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 22fd5fa806ed..6e2a7198b382 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1329,6 +1329,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
>                 dso->inlined_nodes = RB_ROOT_CACHED;
>                 dso->srclines = RB_ROOT_CACHED;
>                 dso->data_types = RB_ROOT;
> +               dso->global_vars = RB_ROOT;
>                 dso->data.fd = -1;
>                 dso->data.status = DSO_DATA_STATUS_UNKNOWN;
>                 dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
> @@ -1373,6 +1374,7 @@ void dso__delete(struct dso *dso)
>         dso->symbol_names_len = 0;
>         zfree(&dso->symbol_names);
>         annotated_data_type__tree_delete(&dso->data_types);
> +       global_var_type__tree_delete(&dso->global_vars);
>
>         if (dso->short_name_allocated) {
>                 zfree((char **)&dso->short_name);
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index ce9f3849a773..2cdcd1e2ef8b 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -154,7 +154,8 @@ struct dso {
>         size_t           symbol_names_len;
>         struct rb_root_cached inlined_nodes;
>         struct rb_root_cached srclines;
> -       struct rb_root  data_types;
> +       struct rb_root   data_types;
> +       struct rb_root   global_vars;
>
>         struct {
>                 u64             addr;
> @@ -411,4 +412,7 @@ int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
>
>  void reset_fd_limit(void);
>
> +u64 dso__find_global_type(struct dso *dso, u64 addr);
> +u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> +
>  #endif /* __PERF_DSO */
> --
> 2.44.0.291.gc1ea87d7ee-goog
>
>

Re: [PATCH 22/23] perf annotate-data: Add a cache for global variable types

[Arnaldo Carvalho de Melo]

On Tue, Mar 19, 2024 at 11:05:04AM -0700, Namhyung Kim wrote:
> On Mon, Mar 18, 2024 at 10:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > They are often searched by many different places.  Let's add a cache
> > for them to reduce the duplicate DWARF access.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
> >  tools/perf/util/annotate-data.h |   7 +++
> >  tools/perf/util/dso.c           |   2 +
> >  tools/perf/util/dso.h           |   6 +-
> >  4 files changed, 118 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index 633fe125fcd8..4b3184b7c799 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> > @@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
> >         return stack;
> >  }
> >
> > +/* Maintain a cache for quick global variable lookup */
> > +struct global_var_entry {
> > +       struct rb_node node;
> > +       char *name;
> > +       u64 start;
> > +       u64 end;
> > +       u64 die_offset;
> > +};
> > +
> > +static int global_var_cmp(const void *_key, const struct rb_node *node)
> > +{
> > +       const u64 addr = (uintptr_t)_key;
> > +       struct global_var_entry *gvar;
> > +
> > +       gvar = rb_entry(node, struct global_var_entry, node);
> > +
> > +       if (gvar->start <= addr && addr < gvar->end)
> > +               return 0;
> > +       return gvar->start > addr ? -1 : 1;
> > +}
> > +
> > +static bool global_var_less(struct rb_node *node_a, const struct rb_node *node_b)
> > +{
> > +       struct global_var_entry *gvar_a, *gvar_b;
> > +
> > +       gvar_a = rb_entry(node_a, struct global_var_entry, node);
> > +       gvar_b = rb_entry(node_b, struct global_var_entry, node);
> > +
> > +       return gvar_a->start < gvar_b->start;
> > +}
> > +
> > +static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64 addr)
> > +{
> > +       struct dso *dso = map__dso(dloc->ms->map);
> > +       struct rb_node *node;
> > +
> > +       node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);
> 
> It seems to cause a build error on 32-bit systems.  It needs one
> more cast to suppress the "pointer cast w/ different size" warning.
> 
>     node = rb_find(void *)(uintptr_tr)addr, ...);

I can add that, to speed up the process, ok?

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> 
> > +       if (node == NULL)
> > +               return NULL;
> > +
> > +       return rb_entry(node, struct global_var_entry, node);
> > +}
> > +
> > +static bool global_var__add(struct data_loc_info *dloc, u64 addr,
> > +                           const char *name, Dwarf_Die *type_die)
> > +{
> > +       struct dso *dso = map__dso(dloc->ms->map);
> > +       struct global_var_entry *gvar;
> > +       Dwarf_Word size;
> > +
> > +       if (dwarf_aggregate_size(type_die, &size) < 0)
> > +               return false;
> > +
> > +       gvar = malloc(sizeof(*gvar));
> > +       if (gvar == NULL)
> > +               return false;
> > +
> > +       gvar->name = strdup(name);
> > +       if (gvar->name == NULL) {
> > +               free(gvar);
> > +               return false;
> > +       }
> > +
> > +       gvar->start = addr;
> > +       gvar->end = addr + size;
> > +       gvar->die_offset = dwarf_dieoffset(type_die);
> > +
> > +       rb_add(&gvar->node, &dso->global_vars, global_var_less);
> > +       return true;
> > +}
> > +
> > +void global_var_type__tree_delete(struct rb_root *root)
> > +{
> > +       struct global_var_entry *gvar;
> > +
> > +       while (!RB_EMPTY_ROOT(root)) {
> > +               struct rb_node *node = rb_first(root);
> > +
> > +               rb_erase(node, root);
> > +               gvar = rb_entry(node, struct global_var_entry, node);
> > +               free(gvar->name);
> > +               free(gvar);
> > +       }
> > +}
> > +
> >  static bool get_global_var_info(struct data_loc_info *dloc, u64 addr,
> >                                 const char **var_name, int *var_offset)
> >  {
> > @@ -467,14 +552,25 @@ 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;
> > +       const char *var_name = NULL;
> > +       struct global_var_entry *gvar;
> >         Dwarf_Die var_die;
> >
> > +       gvar = global_var__find(dloc, var_addr);
> > +       if (gvar) {
> > +               if (!dwarf_offdie(dloc->di->dbg, gvar->die_offset, type_die))
> > +                       return false;
> > +
> > +               *var_offset = var_addr - gvar->start;
> > +               return true;
> > +       }
> > +
> >         /* 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) {
> > +               var_name = dwarf_diename(&var_die);
> >                 *var_offset = offset;
> > -               return true;
> > +               goto ok;
> >         }
> >
> >         if (!get_global_var_info(dloc, var_addr, &var_name, var_offset))
> > @@ -485,9 +581,14 @@ 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)
> > -               return true;
> > +               goto ok;
> >
> >         return false;
> > +
> > +ok:
> > +       /* The address should point to the start of the variable */
> > +       global_var__add(dloc, var_addr - *var_offset, var_name, type_die);
> > +       return true;
> >  }
> >
> >  /**
> > diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> > index 1b5a152163b5..fe1e53d6e8c7 100644
> > --- a/tools/perf/util/annotate-data.h
> > +++ b/tools/perf/util/annotate-data.h
> > @@ -153,6 +153,9 @@ int annotated_data_type__update_samples(struct annotated_data_type *adt,
> >  /* Release all data type information in the tree */
> >  void annotated_data_type__tree_delete(struct rb_root *root);
> >
> > +/* Release all global variable information in the tree */
> > +void global_var_type__tree_delete(struct rb_root *root);
> > +
> >  #else /* HAVE_DWARF_SUPPORT */
> >
> >  static inline struct annotated_data_type *
> > @@ -175,6 +178,10 @@ static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe
> >  {
> >  }
> >
> > +static inline void global_var_type__tree_delete(struct rb_root *root __maybe_unused)
> > +{
> > +}
> > +
> >  #endif /* HAVE_DWARF_SUPPORT */
> >
> >  #endif /* _PERF_ANNOTATE_DATA_H */
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 22fd5fa806ed..6e2a7198b382 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -1329,6 +1329,7 @@ struct dso *dso__new_id(const char *name, struct dso_id *id)
> >                 dso->inlined_nodes = RB_ROOT_CACHED;
> >                 dso->srclines = RB_ROOT_CACHED;
> >                 dso->data_types = RB_ROOT;
> > +               dso->global_vars = RB_ROOT;
> >                 dso->data.fd = -1;
> >                 dso->data.status = DSO_DATA_STATUS_UNKNOWN;
> >                 dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
> > @@ -1373,6 +1374,7 @@ void dso__delete(struct dso *dso)
> >         dso->symbol_names_len = 0;
> >         zfree(&dso->symbol_names);
> >         annotated_data_type__tree_delete(&dso->data_types);
> > +       global_var_type__tree_delete(&dso->global_vars);
> >
> >         if (dso->short_name_allocated) {
> >                 zfree((char **)&dso->short_name);
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index ce9f3849a773..2cdcd1e2ef8b 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -154,7 +154,8 @@ struct dso {
> >         size_t           symbol_names_len;
> >         struct rb_root_cached inlined_nodes;
> >         struct rb_root_cached srclines;
> > -       struct rb_root  data_types;
> > +       struct rb_root   data_types;
> > +       struct rb_root   global_vars;
> >
> >         struct {
> >                 u64             addr;
> > @@ -411,4 +412,7 @@ int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
> >
> >  void reset_fd_limit(void);
> >
> > +u64 dso__find_global_type(struct dso *dso, u64 addr);
> > +u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> > +
> >  #endif /* __PERF_DSO */
> > --
> > 2.44.0.291.gc1ea87d7ee-goog
> >
> >

Re: [PATCH 22/23] perf annotate-data: Add a cache for global variable types

[Arnaldo Carvalho de Melo]

On Tue, Mar 19, 2024 at 03:07:19PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Mar 19, 2024 at 11:05:04AM -0700, Namhyung Kim wrote:
> > On Mon, Mar 18, 2024 at 10:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > They are often searched by many different places.  Let's add a cache
> > > for them to reduce the duplicate DWARF access.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
> > >  tools/perf/util/annotate-data.h |   7 +++
> > >  tools/perf/util/dso.c           |   2 +
> > >  tools/perf/util/dso.h           |   6 +-
> > >  4 files changed, 118 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > > index 633fe125fcd8..4b3184b7c799 100644
> > > --- a/tools/perf/util/annotate-data.c
> > > +++ b/tools/perf/util/annotate-data.c
> > > @@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
> > >         return stack;
> > >  }
> > >
> > > +/* Maintain a cache for quick global variable lookup */
> > > +struct global_var_entry {
> > > +       struct rb_node node;
> > > +       char *name;
> > > +       u64 start;
> > > +       u64 end;
> > > +       u64 die_offset;
> > > +};
> > > +
> > > +static int global_var_cmp(const void *_key, const struct rb_node *node)
> > > +{
> > > +       const u64 addr = (uintptr_t)_key;
> > > +       struct global_var_entry *gvar;
> > > +
> > > +       gvar = rb_entry(node, struct global_var_entry, node);
> > > +
> > > +       if (gvar->start <= addr && addr < gvar->end)
> > > +               return 0;
> > > +       return gvar->start > addr ? -1 : 1;
> > > +}
> > > +
> > > +static bool global_var_less(struct rb_node *node_a, const struct rb_node *node_b)
> > > +{
> > > +       struct global_var_entry *gvar_a, *gvar_b;
> > > +
> > > +       gvar_a = rb_entry(node_a, struct global_var_entry, node);
> > > +       gvar_b = rb_entry(node_b, struct global_var_entry, node);
> > > +
> > > +       return gvar_a->start < gvar_b->start;
> > > +}
> > > +
> > > +static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64 addr)
> > > +{
> > > +       struct dso *dso = map__dso(dloc->ms->map);
> > > +       struct rb_node *node;
> > > +
> > > +       node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);
> > 
> > It seems to cause a build error on 32-bit systems.  It needs one
> > more cast to suppress the "pointer cast w/ different size" warning.
> > 
> >     node = rb_find(void *)(uintptr_tr)addr, ...);

                               uintptr_t
> 
> I can add that, to speed up the process, ok?

Done

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 4b3184b7c79942b4..969e2f82079cdec5 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -469,7 +469,7 @@ static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64
 	struct dso *dso = map__dso(dloc->ms->map);
 	struct rb_node *node;
 
-	node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);
+	node = rb_find((void *)(uintptr_t)addr, &dso->global_vars, global_var_cmp);
 	if (node == NULL)
 		return NULL;
 

Re: [PATCH 22/23] perf annotate-data: Add a cache for global variable types

[Namhyung Kim]

On Tue, Mar 19, 2024 at 11:07 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Mar 19, 2024 at 11:05:04AM -0700, Namhyung Kim wrote:
> > On Mon, Mar 18, 2024 at 10:56 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > They are often searched by many different places.  Let's add a cache
> > > for them to reduce the duplicate DWARF access.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/annotate-data.c | 107 +++++++++++++++++++++++++++++++-
> > >  tools/perf/util/annotate-data.h |   7 +++
> > >  tools/perf/util/dso.c           |   2 +
> > >  tools/perf/util/dso.h           |   6 +-
> > >  4 files changed, 118 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > > index 633fe125fcd8..4b3184b7c799 100644
> > > --- a/tools/perf/util/annotate-data.c
> > > +++ b/tools/perf/util/annotate-data.c
> > > @@ -433,6 +433,91 @@ static struct type_state_stack *findnew_stack_state(struct type_state *state,
> > >         return stack;
> > >  }
> > >
> > > +/* Maintain a cache for quick global variable lookup */
> > > +struct global_var_entry {
> > > +       struct rb_node node;
> > > +       char *name;
> > > +       u64 start;
> > > +       u64 end;
> > > +       u64 die_offset;
> > > +};
> > > +
> > > +static int global_var_cmp(const void *_key, const struct rb_node *node)
> > > +{
> > > +       const u64 addr = (uintptr_t)_key;
> > > +       struct global_var_entry *gvar;
> > > +
> > > +       gvar = rb_entry(node, struct global_var_entry, node);
> > > +
> > > +       if (gvar->start <= addr && addr < gvar->end)
> > > +               return 0;
> > > +       return gvar->start > addr ? -1 : 1;
> > > +}
> > > +
> > > +static bool global_var_less(struct rb_node *node_a, const struct rb_node *node_b)
> > > +{
> > > +       struct global_var_entry *gvar_a, *gvar_b;
> > > +
> > > +       gvar_a = rb_entry(node_a, struct global_var_entry, node);
> > > +       gvar_b = rb_entry(node_b, struct global_var_entry, node);
> > > +
> > > +       return gvar_a->start < gvar_b->start;
> > > +}
> > > +
> > > +static struct global_var_entry *global_var__find(struct data_loc_info *dloc, u64 addr)
> > > +{
> > > +       struct dso *dso = map__dso(dloc->ms->map);
> > > +       struct rb_node *node;
> > > +
> > > +       node = rb_find((void *)addr, &dso->global_vars, global_var_cmp);
> >
> > It seems to cause a build error on 32-bit systems.  It needs one
> > more cast to suppress the "pointer cast w/ different size" warning.
> >
> >     node = rb_find(void *)(uintptr_tr)addr, ...);
>
> I can add that, to speed up the process, ok?

That would be great!

Thanks,
Namhyung

Re: [PATCH 09/23] perf annotate-data: Maintain variable type info

[Arnaldo Carvalho de Melo]

On Tue, Mar 19, 2024 at 10:44:31AM -0700, Namhyung Kim wrote:
> On Tue, Mar 19, 2024 at 7:07 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > +void exit_type_state(struct type_state *state)
> > > +{
> > > +     struct type_state_stack *stack, *tmp;
> > > +
> > > +     list_for_each_entry_safe(stack, tmp, &state->stack_vars, list) {
> > > +             list_del(&stack->list);

> > list_del_init()?
 
> Maybe.. but I'm not sure how much value it'd have as we free it right after.

Usually the value is in catching use after free more quickly, i.e.
someone may have a pointer to a freed list and then it would be able to
traverse the list of freed elements.

- Arnaldo

Re: [PATCH 04/23] perf dwarf-aux: Add die_find_func_rettype()

[Arnaldo Carvalho de Melo]

On Tue, Mar 19, 2024 at 10:42:00AM -0700, Namhyung Kim wrote:
> On Tue, Mar 19, 2024 at 6:56 AM Arnaldo Carvalho de Melo
> > > +             return NULL;
> > > +
> > > +     if (die_get_real_type(&tmp_die, die_mem) == NULL)
> > > +             return NULL;
> >
> >
> > Here you check die_get_real_type() return, may I go and do the same for
> > the previous patch to address my review comment?
> 
> Sure thing! :)

Ack?

- Arnaldo

diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index cd9364d296b6bf42..09fd6f1f0ed8eac3 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1856,7 +1856,10 @@ static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
 	if (offset == loc)
 		return DIE_FIND_CB_END;
 
-	die_get_real_type(die_mem, &type_die);
+	if (die_get_real_type(die_mem, &type_die) == NULL) {
+		// TODO: add a pr_debug_dtp() later for this unlikely failure
+		return DIE_FIND_CB_SIBLING;
+	}
 
 	if (dwarf_aggregate_size(&type_die, &size) < 0)
 		size = 0;

Re: [PATCH 04/23] perf dwarf-aux: Add die_find_func_rettype()

[Namhyung Kim]

On Tue, Mar 19, 2024 at 11:19 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Mar 19, 2024 at 10:42:00AM -0700, Namhyung Kim wrote:
> > On Tue, Mar 19, 2024 at 6:56 AM Arnaldo Carvalho de Melo
> > > > +             return NULL;
> > > > +
> > > > +     if (die_get_real_type(&tmp_die, die_mem) == NULL)
> > > > +             return NULL;
> > >
> > >
> > > Here you check die_get_real_type() return, may I go and do the same for
> > > the previous patch to address my review comment?
> >
> > Sure thing! :)
>
> Ack?

Yep, looks good!

Thanks,
Namhyung

>
> - Arnaldo
>
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index cd9364d296b6bf42..09fd6f1f0ed8eac3 100644
> --- a/tools/perf/util/dwarf-aux.c
> +++ b/tools/perf/util/dwarf-aux.c
> @@ -1856,7 +1856,10 @@ static int __die_find_member_offset_cb(Dwarf_Die *die_mem, void *arg)
>         if (offset == loc)
>                 return DIE_FIND_CB_END;
>
> -       die_get_real_type(die_mem, &type_die);
> +       if (die_get_real_type(die_mem, &type_die) == NULL) {
> +               // TODO: add a pr_debug_dtp() later for this unlikely failure
> +               return DIE_FIND_CB_SIBLING;
> +       }
>
>         if (dwarf_aggregate_size(&type_die, &size) < 0)
>                 size = 0;

Re: [PATCH 09/23] perf annotate-data: Maintain variable type info

[Namhyung Kim]

On Tue, Mar 19, 2024 at 11:12 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Mar 19, 2024 at 10:44:31AM -0700, Namhyung Kim wrote:
> > On Tue, Mar 19, 2024 at 7:07 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > > +void exit_type_state(struct type_state *state)
> > > > +{
> > > > +     struct type_state_stack *stack, *tmp;
> > > > +
> > > > +     list_for_each_entry_safe(stack, tmp, &state->stack_vars, list) {
> > > > +             list_del(&stack->list);
>
> > > list_del_init()?
>
> > Maybe.. but I'm not sure how much value it'd have as we free it right after.
>
> Usually the value is in catching use after free more quickly, i.e.
> someone may have a pointer to a freed list and then it would be able to
> traverse the list of freed elements.

Ok, then. :)