2024-02-02 22:05:19

by Namhyung Kim

Subject: [PATCHSET 00/14] perf tools: Remaining bits of data type profiling (v5)

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].

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-v5' 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 <[email protected]>
Cc: Joe Mario <[email protected]>
CC: Kees Cook <[email protected]>
Cc: David Blaikie <[email protected]>
Cc: Xu Liu <[email protected]>
Cc: Kan Liang <[email protected]>
Cc: Ravi Bangoria <[email protected]>
Cc: Mark Wielaard <[email protected]>
Cc: Jason Merrill <[email protected]>
Cc: Jose E. Marchesi <[email protected]>
Cc: William Huang <[email protected]>


[1] https://lore.kernel.org/linux-perf-users/[email protected]/
[2] https://lwn.net/Articles/955709/


Namhyung Kim (14):
perf dwarf-aux: Add die_collect_vars()
perf dwarf-aux: Handle type transfer for memory access
perf annotate-data: Introduce struct data_loc_info
perf map: Add map__objdump_2rip()
perf annotate: Add annotate_get_basic_blocks()
perf annotate-data: Maintain variable type info
perf annotate-data: Add update_insn_state()
perf annotate-data: Handle global variable access
perf annotate-data: Handle call instructions
perf annotate-data: Implement instruction tracking
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: Add stack canary type

tools/perf/util/annotate-data.c | 710 ++++++++++++++++++++++++++++++--
tools/perf/util/annotate-data.h | 87 +++-
tools/perf/util/annotate.c | 366 ++++++++++++++--
tools/perf/util/annotate.h | 38 ++
tools/perf/util/dwarf-aux.c | 232 +++++++++--
tools/perf/util/dwarf-aux.h | 23 ++
tools/perf/util/map.c | 20 +
tools/perf/util/map.h | 3 +
8 files changed, 1373 insertions(+), 106 deletions(-)

--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:06:16

by Namhyung Kim

Subject: [PATCH 01/14] perf dwarf-aux: Add die_collect_vars()

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) <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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 2791126069b4..f878014c9e27 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
@@ -1479,41 +1513,69 @@ Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
*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 85dd527ae1f7..efafd3a1f5b6 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 */
@@ -150,6 +159,9 @@ Dwarf_Die *die_find_variable_by_addr(Dwarf_Die *sc_die, Dwarf_Addr pc,
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,
@@ -178,6 +190,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:06:27

by Namhyung Kim

Subject: [PATCH 03/14] perf annotate-data: Introduce struct data_loc_info

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 <[email protected]>
---
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 30c4d19fcf11..b8e60c42af8c 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, pc, addr, &var_die, &offset)) {
+ if (die_find_variable_by_addr(&cu_die, pc, 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], pc, addr,
+ if (!die_find_variable_by_addr(&scopes[i], pc, 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 107b264fa41e..cb5d4c517a4d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3788,9 +3788,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++;
@@ -3843,51 +3841,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:06:42

by Namhyung Kim

Subject: [PATCH 05/14] perf annotate: Add annotate_get_basic_blocks()

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 <[email protected]>
---
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 cb5d4c517a4d..4ef14b3f49e4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3686,7 +3686,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;
@@ -3699,7 +3700,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;
}
@@ -3815,7 +3817,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;
@@ -3909,3 +3911,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 94435607c958..83afbe294ab7 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -495,4 +495,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:06:43

by Namhyung Kim

Subject: [PATCH 06/14] perf annotate-data: Maintain variable type info

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 <[email protected]>
---
tools/perf/util/annotate-data.c | 155 ++++++++++++++++++++++++++++++++
tools/perf/util/annotate-data.h | 29 ++++++
tools/perf/util/dwarf-aux.c | 4 +
3 files changed, 188 insertions(+)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index b8e60c42af8c..f8768c224bcc 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -23,6 +23,57 @@
#include "symbol.h"
#include "symbol_conf.h"

+/* Type information in a register, valid when ok is true */
+struct type_state_reg {
+ Dwarf_Die type;
+ bool ok;
+ bool scratch;
+};
+
+/* 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);
+}
+
+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
@@ -238,6 +289,110 @@ 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 update
+ * @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, 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);
+ } else if (var->reg == fbreg) {
+ findnew_stack_state(state, var->offset - fb_offset, &mem_die);
+ } else if (has_reg_type(state, var->reg)) {
+ struct type_state_reg *reg;
+
+ reg = &state->regs[var->reg];
+ reg->type = mem_die;
+ reg->ok = true;
+ }
+ }
+}
+
/* 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..7fbb9eb2e96f 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -8,9 +8,12 @@
#include <linux/types.h>

struct annotated_op_loc;
+struct arch;
struct debuginfo;
+struct die_var_type;
struct evsel;
struct map_symbol;
+struct type_state;

/**
* struct annotated_member - Type of member field
@@ -146,6 +149,16 @@ 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);

+/* Initialize type state table */
+void init_type_state(struct type_state *state, struct arch *arch);
+
+/* Destroy type state table */
+void exit_type_state(struct type_state *state);
+
+/* Update type state table using variables */
+void update_var_state(struct type_state *state, struct data_loc_info *dloc,
+ u64 addr, struct die_var_type *var_types);
+
#else /* HAVE_DWARF_SUPPORT */

static inline struct annotated_data_type *
@@ -168,6 +181,22 @@ static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe
{
}

+static inline void init_type_state(struct type_state *state __maybe_unused,
+ struct arch *arch __maybe_unused)
+{
+}
+
+static inline void exit_type_state(struct type_state *state __maybe_unused)
+{
+}
+
+static inline void update_var_state(struct type_state *state __maybe_unused,
+ struct data_loc_info *dloc __maybe_unused,
+ u64 addr __maybe_unused,
+ struct die_var_type *var_types __maybe_unused)
+{
+}
+
#endif /* HAVE_DWARF_SUPPORT */

#endif /* _PERF_ANNOTATE_DATA_H */
diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
index 39851ff1d5c4..f88a8fb4a350 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"

@@ -1147,6 +1148,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;
}
@@ -1160,6 +1163,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:07:11

by Namhyung Kim

Subject: [PATCH 09/14] perf annotate-data: Handle call instructions

When updating instruction states, the call instruction should play a
role since it can change the register states. For simplicity, mark some
registers as scratch 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 <[email protected]>
---
tools/perf/util/annotate-data.c | 45 +++++++++++++++++++++++++++++++--
1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index e46e162c783f..185cb896b9d6 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -23,10 +23,14 @@
#include "symbol.h"
#include "symbol_conf.h"

-/* Type information in a register, valid when ok is true */
+/*
+ * Type information in a register, valid when @ok is true.
+ * The @scratch registers are invalidated after a function call.
+ */
struct type_state_reg {
Dwarf_Die type;
bool ok;
+ bool scratch;
};

/* Type information in a stack location, dynamically allocated */
@@ -50,6 +54,7 @@ struct type_state_stack {
struct type_state {
struct type_state_reg regs[TYPE_STATE_MAX_REGS];
struct list_head stack_vars;
+ int ret_reg;
};

static bool has_reg_type(struct type_state *state, int reg)
@@ -57,10 +62,23 @@ static bool has_reg_type(struct type_state *state, int reg)
return (unsigned)reg < ARRAY_SIZE(state->regs);
}

-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].scratch = true;
+ state->regs[1].scratch = true;
+ state->regs[2].scratch = true;
+ state->regs[4].scratch = true;
+ state->regs[5].scratch = true;
+ state->regs[8].scratch = true;
+ state->regs[9].scratch = true;
+ state->regs[10].scratch = true;
+ state->regs[11].scratch = true;
+ state->ret_reg = 0;
+ }
}

void exit_type_state(struct type_state *state)
@@ -417,6 +435,29 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
int fbreg = dloc->fbreg;
int fboff = 0;

+ if (ins__is_call(&dl->ins)) {
+ Dwarf_Die func_die;
+
+ /* __fentry__ will preserve all registers */
+ if (dl->ops.target.sym &&
+ !strcmp(dl->ops.target.sym->name, "__fentry__"))
+ return;
+
+ /* Otherwise invalidate scratch registers after call */
+ for (unsigned i = 0; i < ARRAY_SIZE(state->regs); i++) {
+ if (state->regs[i].scratch)
+ state->regs[i].ok = false;
+ }
+
+ /* Update register with the return type (if any) */
+ if (die_find_realfunc(cu_die, dl->ops.target.addr, &func_die) &&
+ die_get_real_type(&func_die, &type_die)) {
+ state->regs[state->ret_reg].type = type_die;
+ state->regs[state->ret_reg].ok = true;
+ }
+ return;
+ }
+
/* FIXME: remove x86 specific code and handle more instructions like LEA */
if (!strstr(dl->ins.name, "mov"))
return;
--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:08:31

by Namhyung Kim

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

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.

With this change, the stat now looks like below:

Annotate data type stats:
total 294, ok 185 (62.9%), bad 109 (37.1%)
-----------------------------------------------------------
30 : no_sym
32 : no_mem_ops
27 : no_var
13 : no_typeinfo
7 : bad_offset

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/annotate-data.c | 232 ++++++++++++++++++++++++++++++++
1 file changed, 232 insertions(+)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index 185cb896b9d6..cebbe17de64a 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -597,6 +597,231 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
/* Case 4. memory to memory transfers (not handled for now) */
}

+/* Prepend this_list to full_list, 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;
+
+ if (last_bb->end != first_bb->begin) {
+ pr_debug("prepend basic blocks: mismatched disasm line %lx -> %lx\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;
+ }
+
+ 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,
+ struct data_loc_info *dloc, int reg,
+ Dwarf_Die *type_die)
+{
+ Dwarf_Word size;
+
+ if (state->regs[reg].ok) {
+ int tag = dwarf_tag(&state->regs[reg].type);
+
+ /*
+ * Normal registers should hold a pointer (or array) to
+ * dereference a memory location.
+ */
+ if (tag != DW_TAG_pointer_type && tag != DW_TAG_array_type)
+ return false;
+
+ 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;
+
+ 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;
+
+ 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;
+ }
+
+ 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;
+
+ 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, 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;
+ int ret = -1;
+
+ 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;
+
+ src_ip = map__objdump_2rip(dloc->ms->map, start);
+
+ /* Get basic blocks for this scope */
+ if (annotate_get_basic_blocks(dloc->ms->sym, src_ip, dst_ip,
+ &this_blocks) < 0)
+ 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 */
+ 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)
{
@@ -697,6 +922,13 @@ 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)
+ goto out;
+ }
+
if (loc->multi_regs && reg == loc->reg1 && loc->reg1 != loc->reg2) {
reg = loc->reg2;
goto retry;
--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:13:59

by Namhyung Kim

Subject: [PATCH 02/14] perf dwarf-aux: Handle type transfer for memory access

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) <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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 f878014c9e27..39851ff1d5c4 100644
--- a/tools/perf/util/dwarf-aux.c
+++ b/tools/perf/util/dwarf-aux.c
@@ -1841,3 +1841,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 efafd3a1f5b6..ad4d7322fcbf 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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:14:19

by Namhyung Kim

Subject: [PATCH 04/14] perf map: Add map__objdump_2rip()

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 <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/map.c | 20 ++++++++++++++++++++
tools/perf/util/map.h | 3 +++
2 files changed, 23 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 54c67cb7ecef..66542864b7b5 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -594,6 +594,26 @@ u64 map__objdump_2mem(struct map *map, u64 ip)
return ip + map__reloc(map);
}

+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);
+
+ /*
+ * kernel modules also have DSO_TYPE_USER in dso->kernel,
+ * but all kernel modules are ET_REL, so won't get here.
+ */
+ 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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:15:19

by Namhyung Kim

Subject: [PATCH 07/14] perf annotate-data: Add update_insn_state()

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 <[email protected]>
---
tools/perf/util/annotate-data.c | 128 +++++++++++++++++++++++++++++++-
tools/perf/util/annotate-data.h | 13 ++++
tools/perf/util/annotate.c | 1 +
3 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index f8768c224bcc..b1e921663452 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -27,7 +27,6 @@
struct type_state_reg {
Dwarf_Die type;
bool ok;
- bool scratch;
};

/* Type information in a stack location, dynamically allocated */
@@ -383,7 +382,7 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
findnew_stack_state(state, var->offset, &mem_die);
} else if (var->reg == fbreg) {
findnew_stack_state(state, var->offset - fb_offset, &mem_die);
- } else if (has_reg_type(state, var->reg)) {
+ } else if (has_reg_type(state, var->reg) && var->offset == 0) {
struct type_state_reg *reg;

reg = &state->regs[var->reg];
@@ -393,6 +392,131 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
}
}

+/**
+ * update_insn_state - Update type state for an instruction
+ * @state: type state table
+ * @dloc: data location info
+ * @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)
+{
+ 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];
+ Dwarf_Die type_die;
+ int fbreg = dloc->fbreg;
+ int fboff = 0;
+
+ /* FIXME: remove x86 specific code and handle more instructions like LEA */
+ if (!strstr(dl->ins.name, "mov"))
+ return;
+
+ if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
+ 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;
+
+ if (has_reg_type(state, src->reg1))
+ state->regs[dst->reg1] = state->regs[src->reg1];
+ else
+ state->regs[dst->reg1].ok = false;
+ }
+ /* 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;
+
+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 && die_get_member_type(&stack->type,
+ offset - stack->offset,
+ &type_die)) {
+ state->regs[dst->reg1].type = type_die;
+ state->regs[dst->reg1].ok = true;
+ } else
+ state->regs[dst->reg1].ok = false;
+ }
+ /* 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)) {
+ state->regs[dst->reg1].type = type_die;
+ state->regs[dst->reg1].ok = true;
+ }
+ /* 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 {
+ state->regs[dst->reg1].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);
+ }
+ }
+ /*
+ * 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) */
+}
+
/* 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 7fbb9eb2e96f..ff9acf6ea808 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -11,6 +11,7 @@ struct annotated_op_loc;
struct arch;
struct debuginfo;
struct die_var_type;
+struct disasm_line;
struct evsel;
struct map_symbol;
struct type_state;
@@ -78,6 +79,7 @@ extern struct annotated_data_type stackop_type;

/**
* struct data_loc_info - Data location information
+ * @arch: architecture info
* @ms: Map and Symbol info
* @ip: Instruction address
* @var_addr: Data address (for global variables)
@@ -90,6 +92,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;
@@ -159,6 +162,10 @@ void exit_type_state(struct type_state *state);
void update_var_state(struct type_state *state, struct data_loc_info *dloc,
u64 addr, struct die_var_type *var_types);

+/* Update type state table for an instruction */
+void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
+ struct disasm_line *dl);
+
#else /* HAVE_DWARF_SUPPORT */

static inline struct annotated_data_type *
@@ -197,6 +204,12 @@ static inline void update_var_state(struct type_state *state __maybe_unused,
{
}

+static inline void update_insn_state(struct type_state *state __maybe_unused,
+ struct data_loc_info *dloc __maybe_unused,
+ struct disasm_line *dl __maybe_unused)
+{
+}
+
#endif /* HAVE_DWARF_SUPPORT */

#endif /* _PERF_ANNOTATE_DATA_H */
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 4ef14b3f49e4..44574056d4bd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3844,6 +3844,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:15:29

by Namhyung Kim

Subject: [PATCH 08/14] perf annotate-data: Handle global variable access

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 <[email protected]>
---
tools/perf/util/annotate-data.c | 45 ++++++++++++++++++++++++++++++---
tools/perf/util/annotate-data.h | 10 ++++++--
tools/perf/util/annotate.c | 45 ++++++++++++++++++++-------------
tools/perf/util/annotate.h | 5 ++++
4 files changed, 83 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index b1e921663452..e46e162c783f 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -396,6 +396,7 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
* 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
@@ -407,7 +408,7 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
* are true.
*/
void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
- struct disasm_line *dl)
+ void *cu_die, struct disasm_line *dl)
{
struct annotated_insn_loc loc;
struct annotated_op_loc *src = &loc.ops[INSN_OP_SOURCE];
@@ -449,8 +450,46 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
return;

retry:
- /* Check stack variables with offset */
- if (sreg == fbreg) {
+ /* Check if it's a global variable */
+ if (sreg == DWARF_REG_PC) {
+ Dwarf_Die var_die;
+ struct map_symbol *ms = dloc->ms;
+ int offset = src->offset;
+ u64 ip = ms->sym->start + dl->al.offset;
+ u64 pc, addr;
+ const char *var_name = NULL;
+
+ addr = annotate_calc_pcrel(ms, ip, offset, dl);
+ pc = map__rip_2objdump(ms->map, ip);
+
+ if (die_find_variable_by_addr(cu_die, pc, addr,
+ &var_die, &offset) &&
+ check_variable(&var_die, &type_die, offset,
+ /*is_pointer=*/false) == 0 &&
+ die_get_member_type(&type_die, offset, &type_die)) {
+ state->regs[dst->reg1].type = type_die;
+ state->regs[dst->reg1].ok = true;
+ return;
+ }
+
+ /* Try to get the name of global variable */
+ offset = src->offset;
+ get_global_var_info(dloc->thread, ms, ip, dl,
+ dloc->cpumode, &addr,
+ &var_name, &offset);
+
+ if (var_name && die_find_variable_at(cu_die, var_name,
+ pc, &var_die) &&
+ check_variable(&var_die, &type_die, offset,
+ /*is_pointer=*/false) == 0 &&
+ die_get_member_type(&type_die, offset, &type_die)) {
+ state->regs[dst->reg1].type = type_die;
+ state->regs[dst->reg1].ok = true;
+ } else
+ state->regs[dst->reg1].ok = false;
+ }
+ /* And check stack variables with offset */
+ else if (sreg == fbreg) {
struct type_state_stack *stack;
int offset = src->offset - fboff;

diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index ff9acf6ea808..0bfef29fa52c 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -14,6 +14,7 @@ struct die_var_type;
struct disasm_line;
struct evsel;
struct map_symbol;
+struct thread;
struct type_state;

/**
@@ -79,11 +80,13 @@ extern struct annotated_data_type stackop_type;

/**
* struct data_loc_info - Data location information
- * @arch: architecture info
+ * @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
@@ -94,8 +97,10 @@ struct data_loc_info {
/* These are input field, should be filled by caller */
struct arch *arch;
struct map_symbol *ms;
+ struct thread *thread;
u64 ip;
u64 var_addr;
+ u8 cpumode;
const char *var_name;
struct annotated_op_loc *op;

@@ -164,7 +169,7 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,

/* Update type state table for an instruction */
void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
- struct disasm_line *dl);
+ void *cu_die, struct disasm_line *dl);

#else /* HAVE_DWARF_SUPPORT */

@@ -206,6 +211,7 @@ static inline void update_var_state(struct type_state *state __maybe_unused,

static inline void update_insn_state(struct type_state *state __maybe_unused,
struct data_loc_info *dloc __maybe_unused,
+ void *cu_die __maybe_unused,
struct disasm_line *dl __maybe_unused)
{
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 44574056d4bd..89a8d57b1bf7 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3771,6 +3771,28 @@ u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
return map__rip_2objdump(ms->map, addr);
}

+void get_global_var_info(struct thread *thread, struct map_symbol *ms, u64 ip,
+ struct disasm_line *dl, u8 cpumode, u64 *var_addr,
+ const char **var_name, int *poffset)
+{
+ struct addr_location al;
+ struct symbol *var;
+ u64 map_addr;
+
+ *var_addr = annotate_calc_pcrel(ms, ip, *poffset, dl);
+ /* Kernel symbols might be relocated */
+ map_addr = *var_addr + map__reloc(ms->map);
+
+ addr_location__init(&al);
+ var = thread__find_symbol_fb(thread, cpumode, map_addr, &al);
+ if (var) {
+ *var_name = var->name;
+ /* Calculate type offset from the start of variable */
+ *poffset = map_addr - map__unmap_ip(al.map, var->start);
+ }
+ addr_location__exit(&al);
+}
+
/**
* hist_entry__get_data_type - find data type for given hist entry
* @he: hist entry
@@ -3845,6 +3867,8 @@ 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,
+ .cpumode = he->cpumode,
.ms = ms,
/* Recalculate IP for LOCK prefix or insn fusion */
.ip = ms->sym->start + dl->al.offset,
@@ -3859,23 +3883,10 @@ 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.type_offset = op_loc->offset;
+ get_global_var_info(he->thread, ms, ip, dl, he->cpumode,
+ &dloc.var_addr, &dloc.var_name,
+ &dloc.type_offset);
}

mem_type = find_data_type(&dloc);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 83afbe294ab7..b460785111a1 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -23,6 +23,7 @@ struct option;
struct perf_sample;
struct evsel;
struct symbol;
+struct thread;
struct annotated_data_type;

struct ins {
@@ -495,6 +496,10 @@ extern struct list_head ann_insn_stat;
u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
struct disasm_line *dl);

+void get_global_var_info(struct thread *thread, struct map_symbol *ms, u64 ip,
+ struct disasm_line *dl, u8 cpumode, u64 *var_addr,
+ const char **var_name, int *poffset);
+
/**
* struct annotated_basic_block - Basic block of instructions
* @list: List node
--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:16:16

by Namhyung Kim

Subject: [PATCH 12/14] perf annotate-data: Handle this-cpu variables in kernel

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. The new
get_percpu_var_info() helper is to get information about the variable.
Pretend it as a global variable by changing the register number to
DWARF_REG_PC.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/annotate.c | 31 +++++++++++++++++++++++++++++++
tools/perf/util/annotate.h | 4 ++++
2 files changed, 35 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 86ac44c476bf..5f3136f57c62 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3810,6 +3810,27 @@ void get_global_var_info(struct thread *thread, struct map_symbol *ms, u64 ip,
addr_location__exit(&al);
}

+void get_percpu_var_info(struct thread *thread, struct map_symbol *ms,
+ u8 cpumode, u64 var_addr, const char **var_name,
+ int *poffset)
+{
+ struct addr_location al;
+ struct symbol *var;
+ u64 map_addr;
+
+ /* Kernel symbols might be relocated */
+ map_addr = var_addr + map__reloc(ms->map);
+
+ addr_location__init(&al);
+ var = thread__find_symbol_fb(thread, cpumode, map_addr, &al);
+ if (var) {
+ *var_name = var->name;
+ /* Calculate type offset from the start of variable */
+ *poffset = map_addr - map__unmap_ip(al.map, var->start);
+ }
+ addr_location__exit(&al);
+}
+
/**
* hist_entry__get_data_type - find data type for given hist entry
* @he: hist entry
@@ -3906,6 +3927,16 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
&dloc.type_offset);
}

+ /* This CPU access in kernel - pretend PC-relative addressing */
+ if (op_loc->reg1 < 0 && map__dso(ms->map)->kernel &&
+ arch__is(arch, "x86") && op_loc->segment == INSN_SEG_X86_GS) {
+ dloc.var_addr = op_loc->offset;
+ get_percpu_var_info(he->thread, ms, he->cpumode,
+ dloc.var_addr, &dloc.var_name,
+ &dloc.type_offset);
+ op_loc->reg1 = DWARF_REG_PC;
+ }
+
mem_type = find_data_type(&dloc);
if (mem_type)
istat->good++;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 2bd654620de3..490134d19c9d 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -513,6 +513,10 @@ void get_global_var_info(struct thread *thread, struct map_symbol *ms, u64 ip,
struct disasm_line *dl, u8 cpumode, u64 *var_addr,
const char **var_name, int *poffset);

+void get_percpu_var_info(struct thread *thread, struct map_symbol *ms,
+ u8 cpumode, u64 var_addr, const char **var_name,
+ int *poffset);
+
/**
* struct annotated_basic_block - Basic block of instructions
* @list: List node
--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:16:19

by Namhyung Kim

Subject: [PATCH 11/14] perf annotate: Parse x86 segment register location

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 <[email protected]>
---
tools/perf/util/annotate.c | 21 +++++++++++++++++++--
tools/perf/util/annotate.h | 13 +++++++++++++
2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 89a8d57b1bf7..86ac44c476bf 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3557,6 +3557,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++;
@@ -3653,8 +3659,19 @@ 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;
+
+ 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,
+ NULL, 0);
+ continue;
+ }
+ }

+ s = strdup(insn_str);
if (s) {
op_loc->reg1 = get_dwarf_regnum(s, 0);
free(s);
@@ -3875,7 +3892,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 b460785111a1..2bd654620de3 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -446,6 +446,7 @@ 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
*/
@@ -453,6 +454,7 @@ struct annotated_op_loc {
int reg1;
int reg2;
int offset;
+ u8 segment;
bool mem_ref;
bool multi_regs;
};
@@ -464,6 +466,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.43.0.594.gd9cf4e227d-goog

2024-02-02 22:16:47

by Namhyung Kim

Subject: [PATCH 13/14] perf annotate-data: Track instructions with a this-cpu variable

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 <[email protected]>
---
tools/perf/util/annotate-data.c | 84 +++++++++++++++++++++++----------
1 file changed, 60 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index cebbe17de64a..9d6cc6ac431c 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -410,6 +410,37 @@ void update_var_state(struct type_state *state, struct data_loc_info *dloc,
}
}

+static bool get_global_var_type(Dwarf_Die *cu_die, struct map_symbol *ms, u64 ip,
+ u64 var_addr, const char *var_name, int var_offset,
+ Dwarf_Die *type_die)
+{
+ u64 pc;
+ int offset = var_offset;
+ bool is_pointer = false;
+ Dwarf_Die var_die;
+
+ pc = map__rip_2objdump(ms->map, ip);
+
+ /* Try to get the variable by address first */
+ if (die_find_variable_by_addr(cu_die, pc, var_addr, &var_die, &offset) &&
+ check_variable(&var_die, type_die, offset, is_pointer) == 0 &&
+ die_get_member_type(type_die, offset, type_die))
+ return true;
+
+ if (var_name == NULL)
+ return false;
+
+ offset = var_offset;
+
+ /* 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, offset, is_pointer) == 0 &&
+ die_get_member_type(type_die, offset, type_die))
+ return true;
+
+ return false;
+}
+
/**
* update_insn_state - Update type state for an instruction
* @state: type state table
@@ -473,14 +504,36 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
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;

if (has_reg_type(state, src->reg1))
state->regs[dst->reg1] = state->regs[src->reg1];
- else
+ else if (map__dso(dloc->ms->map)->kernel &&
+ src->segment == INSN_SEG_X86_GS) {
+ struct map_symbol *ms = dloc->ms;
+ int offset = src->offset;
+ u64 ip = ms->sym->start + dl->al.offset;
+ const char *var_name = NULL;
+ u64 var_addr;
+
+ /*
+ * 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;
+ get_percpu_var_info(dloc->thread, ms, dloc->cpumode,
+ var_addr, &var_name, &offset);
+
+ if (get_global_var_type(cu_die, ms, ip, var_addr,
+ var_name, offset, &type_die)) {
+ state->regs[dst->reg1].type = type_die;
+ state->regs[dst->reg1].ok = true;
+ }
+ } else
state->regs[dst->reg1].ok = false;
}
/* Case 2. memory to register transers */
@@ -493,37 +546,20 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
retry:
/* Check if it's a global variable */
if (sreg == DWARF_REG_PC) {
- Dwarf_Die var_die;
struct map_symbol *ms = dloc->ms;
int offset = src->offset;
u64 ip = ms->sym->start + dl->al.offset;
- u64 pc, addr;
const char *var_name = NULL;
+ u64 var_addr;

- addr = annotate_calc_pcrel(ms, ip, offset, dl);
- pc = map__rip_2objdump(ms->map, ip);
-
- if (die_find_variable_by_addr(cu_die, pc, addr,
- &var_die, &offset) &&
- check_variable(&var_die, &type_die, offset,
- /*is_pointer=*/false) == 0 &&
- die_get_member_type(&type_die, offset, &type_die)) {
- state->regs[dst->reg1].type = type_die;
- state->regs[dst->reg1].ok = true;
- return;
- }
+ var_addr = annotate_calc_pcrel(ms, ip, offset, dl);

- /* Try to get the name of global variable */
- offset = src->offset;
get_global_var_info(dloc->thread, ms, ip, dl,
- dloc->cpumode, &addr,
+ dloc->cpumode, &var_addr,
&var_name, &offset);

- if (var_name && die_find_variable_at(cu_die, var_name,
- pc, &var_die) &&
- check_variable(&var_die, &type_die, offset,
- /*is_pointer=*/false) == 0 &&
- die_get_member_type(&type_die, offset, &type_die)) {
+ if (get_global_var_type(cu_die, ms, ip, var_addr,
+ var_name, offset, &type_die)) {
state->regs[dst->reg1].type = type_die;
state->regs[dst->reg1].ok = true;
} else
--
2.43.0.594.gd9cf4e227d-goog

2024-02-02 22:27:18

by Namhyung Kim

Subject: [PATCH 14/14] perf annotate-data: Add stack canary type

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 <[email protected]>
---
tools/perf/util/annotate-data.h | 1 +
tools/perf/util/annotate.c | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+)

diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
index 0bfef29fa52c..e293980eb11b 100644
--- a/tools/perf/util/annotate-data.h
+++ b/tools/perf/util/annotate-data.h
@@ -77,6 +77,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 5f3136f57c62..f2683dadf3cf 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -116,6 +116,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;
@@ -3764,6 +3771,17 @@ 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->offset == 40)
+ return true;
+ }
+
+ return false;
+}
+
u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
struct disasm_line *dl)
{
@@ -3938,6 +3956,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.43.0.594.gd9cf4e227d-goog

2024-02-03 01:42:20

by Ian Rogers

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
>
> 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.

Hi Namhyung,

I think the naming can be better here. Aren't the objdump addresses
DSO relative offsets? Is the relative IP relative to the map or the
DSO?

Thanks,
Ian

> Cc: Adrian Hunter <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/util/map.c | 20 ++++++++++++++++++++
> tools/perf/util/map.h | 3 +++
> 2 files changed, 23 insertions(+)
>
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 54c67cb7ecef..66542864b7b5 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -594,6 +594,26 @@ u64 map__objdump_2mem(struct map *map, u64 ip)
> return ip + map__reloc(map);
> }
>
> +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);
> +
> + /*
> + * kernel modules also have DSO_TYPE_USER in dso->kernel,
> + * but all kernel modules are ET_REL, so won't get here.
> + */
> + 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.43.0.594.gd9cf4e227d-goog
>

2024-02-03 02:59:28

by Ian Rogers

Subject: Re: [PATCH 06/14] perf annotate-data: Maintain variable type info

On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> 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 <[email protected]>
> ---
> tools/perf/util/annotate-data.c | 155 ++++++++++++++++++++++++++++++++
> tools/perf/util/annotate-data.h | 29 ++++++
> tools/perf/util/dwarf-aux.c | 4 +
> 3 files changed, 188 insertions(+)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index b8e60c42af8c..f8768c224bcc 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -23,6 +23,57 @@
> #include "symbol.h"
> #include "symbol_conf.h"
>
> +/* Type information in a register, valid when ok is true */
> +struct type_state_reg {
> + Dwarf_Die type;
> + bool ok;
> + bool scratch;
> +};
> +
> +/* 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

Perhaps 32, presumably 16 won't work on Arm64.

Thanks,
Ian

> +
> +/*
> + * 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);
> +}
> +
> +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
> @@ -238,6 +289,110 @@ 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 update
> + * @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, 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);
> + } else if (var->reg == fbreg) {
> + findnew_stack_state(state, var->offset - fb_offset, &mem_die);
> + } else if (has_reg_type(state, var->reg)) {
> + struct type_state_reg *reg;
> +
> + reg = &state->regs[var->reg];
> + reg->type = mem_die;
> + reg->ok = true;
> + }
> + }
> +}
> +
> /* 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..7fbb9eb2e96f 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -8,9 +8,12 @@
> #include <linux/types.h>
>
> struct annotated_op_loc;
> +struct arch;
> struct debuginfo;
> +struct die_var_type;
> struct evsel;
> struct map_symbol;
> +struct type_state;
>
> /**
> * struct annotated_member - Type of member field
> @@ -146,6 +149,16 @@ 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);
>
> +/* Initialize type state table */
> +void init_type_state(struct type_state *state, struct arch *arch);
> +
> +/* Destroy type state table */
> +void exit_type_state(struct type_state *state);
> +
> +/* Update type state table using variables */
> +void update_var_state(struct type_state *state, struct data_loc_info *dloc,
> + u64 addr, struct die_var_type *var_types);
> +
> #else /* HAVE_DWARF_SUPPORT */
>
> static inline struct annotated_data_type *
> @@ -168,6 +181,22 @@ static inline void annotated_data_type__tree_delete(struct rb_root *root __maybe
> {
> }
>
> +static inline void init_type_state(struct type_state *state __maybe_unused,
> + struct arch *arch __maybe_unused)
> +{
> +}
> +
> +static inline void exit_type_state(struct type_state *state __maybe_unused)
> +{
> +}
> +
> +static inline void update_var_state(struct type_state *state __maybe_unused,
> + struct data_loc_info *dloc __maybe_unused,
> + u64 addr __maybe_unused,
> + struct die_var_type *var_types __maybe_unused)
> +{
> +}
> +
> #endif /* HAVE_DWARF_SUPPORT */
>
> #endif /* _PERF_ANNOTATE_DATA_H */
> diff --git a/tools/perf/util/dwarf-aux.c b/tools/perf/util/dwarf-aux.c
> index 39851ff1d5c4..f88a8fb4a350 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"
>
> @@ -1147,6 +1148,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;
> }
> @@ -1160,6 +1163,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.43.0.594.gd9cf4e227d-goog
>

2024-02-03 02:59:58

by Ian Rogers

Subject: Re: [PATCH 07/14] perf annotate-data: Add update_insn_state()

On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
>
> 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.

Just an aside, it seems a shame objdump can't do this tracking for us.
objdump already augments its output with symbol and relocation data.

Thanks,
Ian

2024-02-03 03:09:26

by Ian Rogers

Subject: Re: [PATCH 09/14] perf annotate-data: Handle call instructions

On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
>
> When updating instruction states, the call instruction should play a
> role since it can change the register states. For simplicity, mark some
> registers as scratch registers (should be arch-dependent), and
> invalidate them all after a function call.

nit: Volatile or caller-save would be a more conventional name than scratch.

Thanks,
Ian

> If the function returns something, the designated register (ret_reg)
> will have the type info.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/util/annotate-data.c | 45 +++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index e46e162c783f..185cb896b9d6 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -23,10 +23,14 @@
> #include "symbol.h"
> #include "symbol_conf.h"
>
> -/* Type information in a register, valid when ok is true */
> +/*
> + * Type information in a register, valid when @ok is true.
> + * The @scratch registers are invalidated after a function call.
> + */
> struct type_state_reg {
> Dwarf_Die type;
> bool ok;
> + bool scratch;
> };
>
> /* Type information in a stack location, dynamically allocated */
> @@ -50,6 +54,7 @@ struct type_state_stack {
> struct type_state {
> struct type_state_reg regs[TYPE_STATE_MAX_REGS];
> struct list_head stack_vars;
> + int ret_reg;
> };
>
> static bool has_reg_type(struct type_state *state, int reg)
> @@ -57,10 +62,23 @@ static bool has_reg_type(struct type_state *state, int reg)
> return (unsigned)reg < ARRAY_SIZE(state->regs);
> }
>
> -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].scratch = true;
> + state->regs[1].scratch = true;
> + state->regs[2].scratch = true;
> + state->regs[4].scratch = true;
> + state->regs[5].scratch = true;
> + state->regs[8].scratch = true;
> + state->regs[9].scratch = true;
> + state->regs[10].scratch = true;
> + state->regs[11].scratch = true;
> + state->ret_reg = 0;
> + }
> }
>
> void exit_type_state(struct type_state *state)
> @@ -417,6 +435,29 @@ void update_insn_state(struct type_state *state, struct data_loc_info *dloc,
> int fbreg = dloc->fbreg;
> int fboff = 0;
>
> + if (ins__is_call(&dl->ins)) {
> + Dwarf_Die func_die;
> +
> + /* __fentry__ will preserve all registers */
> + if (dl->ops.target.sym &&
> + !strcmp(dl->ops.target.sym->name, "__fentry__"))
> + return;
> +
> + /* Otherwise invalidate scratch registers after call */
> + for (unsigned i = 0; i < ARRAY_SIZE(state->regs); i++) {
> + if (state->regs[i].scratch)
> + state->regs[i].ok = false;
> + }
> +
> + /* Update register with the return type (if any) */
> + if (die_find_realfunc(cu_die, dl->ops.target.addr, &func_die) &&
> + die_get_real_type(&func_die, &type_die)) {
> + state->regs[state->ret_reg].type = type_die;
> + state->regs[state->ret_reg].ok = true;
> + }
> + return;
> + }
> +
> /* FIXME: remove x86 specific code and handle more instructions like LEA */
> if (!strstr(dl->ins.name, "mov"))
> return;
> --
> 2.43.0.594.gd9cf4e227d-goog
>

2024-02-03 03:21:57

by Ian Rogers

Subject: Re: [PATCH 14/14] perf annotate-data: Add stack canary type

On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
>
> 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.

I wonder if cases like this can be handled by debug info rather than
special cases in the tool. Special cases are fine too, but are
potentially less portable.

Thanks,
Ian

> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/perf/util/annotate-data.h | 1 +
> tools/perf/util/annotate.c | 24 ++++++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index 0bfef29fa52c..e293980eb11b 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -77,6 +77,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 5f3136f57c62..f2683dadf3cf 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -116,6 +116,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;
> @@ -3764,6 +3771,17 @@ 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->offset == 40)
> + return true;
> + }
> +
> + return false;
> +}
> +
> u64 annotate_calc_pcrel(struct map_symbol *ms, u64 ip, int offset,
> struct disasm_line *dl)
> {
> @@ -3938,6 +3956,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.43.0.594.gd9cf4e227d-goog
>

2024-02-06 23:05:05

by Namhyung Kim

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

Hi Ian,

On Fri, Feb 2, 2024 at 5:42 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> >
> > 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.
>
> Hi Namhyung,
>
> I think the naming can be better here. Aren't the objdump addresses
> DSO relative offsets? Is the relative IP relative to the map or the
> DSO?

AFAIK the objdump addresses are DSO-relative and rip is to map.
They are mostly the same but sometimes different due to kASLR
for the kernel.

>
> > Cc: Adrian Hunter <[email protected]>
> > Signed-off-by: Namhyung Kim <[email protected]>
> > ---
> > tools/perf/util/map.c | 20 ++++++++++++++++++++
> > tools/perf/util/map.h | 3 +++
> > 2 files changed, 23 insertions(+)
> >
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index 54c67cb7ecef..66542864b7b5 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -594,6 +594,26 @@ u64 map__objdump_2mem(struct map *map, u64 ip)
> > return ip + map__reloc(map);
> > }
> >
> > +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);
> > +
> > + /*
> > + * kernel modules also have DSO_TYPE_USER in dso->kernel,
> > + * but all kernel modules are ET_REL, so won't get here.
> > + */

Hmm.. This comment is not true anymore. Will remove
in other places too.

Thanks,
Namhyung


> > + 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.43.0.594.gd9cf4e227d-goog
> >

2024-02-06 23:07:13

by Namhyung Kim

Subject: Re: [PATCH 06/14] perf annotate-data: Maintain variable type info

On Fri, Feb 2, 2024 at 6:45 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> 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 <[email protected]>
> > ---
> > tools/perf/util/annotate-data.c | 155 ++++++++++++++++++++++++++++++++
> > tools/perf/util/annotate-data.h | 29 ++++++
> > tools/perf/util/dwarf-aux.c | 4 +
> > 3 files changed, 188 insertions(+)
> >
> > diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> > index b8e60c42af8c..f8768c224bcc 100644
> > --- a/tools/perf/util/annotate-data.c
> > +++ b/tools/perf/util/annotate-data.c
> > @@ -23,6 +23,57 @@
> > #include "symbol.h"
> > #include "symbol_conf.h"
> >
> > +/* Type information in a register, valid when ok is true */
> > +struct type_state_reg {
> > + Dwarf_Die type;
> > + bool ok;
> > + bool scratch;
> > +};
> > +
> > +/* 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
>
> Perhaps 32, presumably 16 won't work on Arm64.

Right, but right now I'm targeting x86_64 only.
Maybe we can change it later when it supports Arm64.

Thanks,
Namhyung

2024-02-06 23:14:23

by Namhyung Kim

Subject: Re: [PATCH 07/14] perf annotate-data: Add update_insn_state()

On Fri, Feb 2, 2024 at 6:49 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> >
> > 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.
>
> Just an aside, it seems a shame objdump can't do this tracking for us.
> objdump already augments its output with symbol and relocation data.

I'd be more than happy if objdump can display a relevant data type on
each instruction. :)

Thanks,
Namhyung

2024-02-06 23:17:39

by Namhyung Kim

Subject: Re: [PATCH 09/14] perf annotate-data: Handle call instructions

On Fri, Feb 2, 2024 at 7:09 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> >
> > When updating instruction states, the call instruction should play a
> > role since it can change the register states. For simplicity, mark some
> > registers as scratch registers (should be arch-dependent), and
> > invalidate them all after a function call.
>
> nit: Volatile or caller-save would be a more conventional name than scratch.

'volatile' is a keyword and 'caller_saved' seems somewhat verbose.
Maybe 'temporary'?

Thanks,
Namhyung

2024-02-06 23:19:10

by Namhyung Kim

Subject: Re: [PATCH 14/14] perf annotate-data: Add stack canary type

On Fri, Feb 2, 2024 at 7:21 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> >
> > 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.
>
> I wonder if cases like this can be handled by debug info rather than
> special cases in the tool. Special cases are fine too, but are
> potentially less portable.

Agreed, but I couldn't find anything special in DWARF.

Thanks,
Namhyung

2024-02-06 23:34:44

by Ian Rogers

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

On Tue, Feb 6, 2024 at 3:04 PM Namhyung Kim <[email protected]> wrote:
>
> Hi Ian,
>
> On Fri, Feb 2, 2024 at 5:42 PM Ian Rogers <[email protected]> wrote:
> >
> > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > 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.
> >
> > Hi Namhyung,
> >
> > I think the naming can be better here. Aren't the objdump addresses
> > DSO relative offsets? Is the relative IP relative to the map or the
> > DSO?
>
> AFAIK the objdump addresses are DSO-relative and rip is to map.
> They are mostly the same but sometimes different due to kASLR
> for the kernel.

Perhaps we need to use names like map_rip for mapping relative and
dso_rip to clean this up, or to add a different mapping_type to the
enum. For non-kernel maps addresses for map are either the whole
virtual address space (identity) or relative to a dso:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n115
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n20
The dso addresses should work for objdump so perhaps the kernel
addresses need map__pgoff fixing?

Thanks,
Ian

> >
> > > Cc: Adrian Hunter <[email protected]>
> > > Signed-off-by: Namhyung Kim <[email protected]>
> > > ---
> > > tools/perf/util/map.c | 20 ++++++++++++++++++++
> > > tools/perf/util/map.h | 3 +++
> > > 2 files changed, 23 insertions(+)
> > >
> > > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > > index 54c67cb7ecef..66542864b7b5 100644
> > > --- a/tools/perf/util/map.c
> > > +++ b/tools/perf/util/map.c
> > > @@ -594,6 +594,26 @@ u64 map__objdump_2mem(struct map *map, u64 ip)
> > > return ip + map__reloc(map);
> > > }
> > >
> > > +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);
> > > +
> > > + /*
> > > + * kernel modules also have DSO_TYPE_USER in dso->kernel,
> > > + * but all kernel modules are ET_REL, so won't get here.
> > > + */
>
> Hmm.. This comment is not true anymore. Will remove
> in other places too.
>
> Thanks,
> Namhyung
>
>
> > > + 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.43.0.594.gd9cf4e227d-goog
> > >

2024-02-06 23:36:22

by Ian Rogers

Subject: Re: [PATCH 09/14] perf annotate-data: Handle call instructions

On Tue, Feb 6, 2024 at 3:17 PM Namhyung Kim <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 7:09 PM Ian Rogers <[email protected]> wrote:
> >
> > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > When updating instruction states, the call instruction should play a
> > > role since it can change the register states. For simplicity, mark some
> > > registers as scratch registers (should be arch-dependent), and
> > > invalidate them all after a function call.
> >
> > nit: Volatile or caller-save would be a more conventional name than scratch.
>
> 'volatile' is a keyword and 'caller_saved' seems somewhat verbose.
> Maybe 'temporary'?

Sgtm, perhaps temp for brevity and the documentation to call them caller save?

Thanks,
Ian

> Thanks,
> Namhyung

2024-02-06 23:40:53

by Ian Rogers

Subject: Re: [PATCH 14/14] perf annotate-data: Add stack canary type

On Tue, Feb 6, 2024 at 3:19 PM Namhyung Kim <[email protected]> wrote:
>
> On Fri, Feb 2, 2024 at 7:21 PM Ian Rogers <[email protected]> wrote:
> >
> > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > 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.
> >
> > I wonder if cases like this can be handled by debug info rather than
> > special cases in the tool. Special cases are fine too, but are
> > potentially less portable.
>
> Agreed, but I couldn't find anything special in DWARF.

The fs and gs selectors are commonly used for thread local storage, so
could something like DW_OP_form_tls_address be used?
https://dwarfstd.org/issues/110803.1.html

Thanks,
Ian

> Thanks,
> Namhyung

2024-02-07 01:29:56

by Namhyung Kim

Subject: Re: [PATCH 09/14] perf annotate-data: Handle call instructions

On Tue, Feb 6, 2024 at 3:44 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
>
>
> On Tue, Feb 6, 2024, 8:36 PM Ian Rogers <[email protected]> wrote:
>>
>> On Tue, Feb 6, 2024 at 3:17 PM Namhyung Kim <[email protected]> wrote:
>> >
>> > On Fri, Feb 2, 2024 at 7:09 PM Ian Rogers <[email protected]> wrote:
>> > >
>> > > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <namhyung@kernelorg> wrote:
>> > > >
>> > > > When updating instruction states, the call instruction should play a
>> > > > role since it can change the register states. For simplicity, mark some
>> > > > registers as scratch registers (should be arch-dependent), and
>> > > > invalidate them all after a function call.
>> > >
>> > > nit: Volatile or caller-save would be a more conventional name than scratch.
>> >
>> > 'volatile' is a keyword and 'caller_saved' seems somewhat verbose.
>> > Maybe 'temporary'?
>>
>> Sgtm, perhaps temp for brevity and the documentation to call them caller save?
>
>
>
> "caller_saved" seems to be the conventional name doesn't look too long to use to help in reading this code by new people that have read the literature.

Ok, as you both requested, I will use "caller_saved". :)

Thanks,
Namhyung


>
> For instance, from a quick Google search:
>
> https://stackoverflow.com/questions/9268586/what-are-callee-and-caller-saved-registers
>
> - Arnaldo

2024-02-07 19:08:37

by Namhyung Kim

Subject: Re: [PATCH 14/14] perf annotate-data: Add stack canary type

On Tue, Feb 6, 2024 at 3:40 PM Ian Rogers <[email protected]> wrote:
>
> On Tue, Feb 6, 2024 at 3:19 PM Namhyung Kim <[email protected]> wrote:
> >
> > On Fri, Feb 2, 2024 at 7:21 PM Ian Rogers <[email protected]> wrote:
> > >
> > > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > > >
> > > > 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.
> > >
> > > I wonder if cases like this can be handled by debug info rather than
> > > special cases in the tool. Special cases are fine too, but are
> > > potentially less portable.
> >
> > Agreed, but I couldn't find anything special in DWARF.
>
> The fs and gs selectors are commonly used for thread local storage, so
> could something like DW_OP_form_tls_address be used?
> https://dwarfstd.org/issues/110803.1.html

I'm not sure if it's the same thing. Maybe it's for variables with
__thread annotation. I don't know if compilers generate a
(pseudo) variable for stack canary.

Thanks,
Namhyung

2024-02-07 19:10:48

by Namhyung Kim

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

On Tue, Feb 6, 2024 at 3:34 PM Ian Rogers <[email protected]> wrote:
>
> On Tue, Feb 6, 2024 at 3:04 PM Namhyung Kim <[email protected]> wrote:
> >
> > Hi Ian,
> >
> > On Fri, Feb 2, 2024 at 5:42 PM Ian Rogers <[email protected]> wrote:
> > >
> > > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > > >
> > > > 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.
> > >
> > > Hi Namhyung,
> > >
> > > I think the naming can be better here. Aren't the objdump addresses
> > > DSO relative offsets? Is the relative IP relative to the map or the
> > > DSO?
> >
> > AFAIK the objdump addresses are DSO-relative and rip is to map.
> > They are mostly the same but sometimes different due to kASLR
> > for the kernel.
>
> Perhaps we need to use names like map_rip for mapping relative and
> dso_rip to clean this up, or to add a different mapping_type to the
> enum. For non-kernel maps addresses for map are either the whole
> virtual address space (identity) or relative to a dso:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n115
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n20
> The dso addresses should work for objdump so perhaps the kernel
> addresses need map__pgoff fixing?

I'm not sure about the vDSO case.

By the way, I need to take a look if we can make this objdump-rip
thing simpler as you mentioned. My feeling is that it can be done
but I'd like to do it in a separate work and to move this forward.

Thanks,
Namhyung

2024-02-07 19:56:41

by Ian Rogers

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

On Wed, Feb 7, 2024 at 11:04 AM Namhyung Kim <[email protected]> wrote:
>
> On Tue, Feb 6, 2024 at 3:34 PM Ian Rogers <[email protected]> wrote:
> >
> > On Tue, Feb 6, 2024 at 3:04 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Fri, Feb 2, 2024 at 5:42 PM Ian Rogers <[email protected]> wrote:
> > > >
> > > > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > Hi Namhyung,
> > > >
> > > > I think the naming can be better here. Aren't the objdump addresses
> > > > DSO relative offsets? Is the relative IP relative to the map or the
> > > > DSO?
> > >
> > > AFAIK the objdump addresses are DSO-relative and rip is to map.
> > > They are mostly the same but sometimes different due to kASLR
> > > for the kernel.
> >
> > Perhaps we need to use names like map_rip for mapping relative and
> > dso_rip to clean this up, or to add a different mapping_type to the
> > enum. For non-kernel maps addresses for map are either the whole
> > virtual address space (identity) or relative to a dso:
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n115
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n20
> > The dso addresses should work for objdump so perhaps the kernel
> > addresses need map__pgoff fixing?
>
> I'm not sure about the vDSO case.
>
> By the way, I need to take a look if we can make this objdump-rip
> thing simpler as you mentioned. My feeling is that it can be done
> but I'd like to do it in a separate work and to move this forward.

Makes sense. Could we add a comment to the header file definition to
capture some of this? Perhaps a "to be removed" to avoid later patches
adding dependencies to the function.

Thanks,
Ian

> Thanks,
> Namhyung

2024-02-07 20:43:40

by Namhyung Kim

Subject: Re: [PATCH 04/14] perf map: Add map__objdump_2rip()

On Wed, Feb 7, 2024 at 11:56 AM Ian Rogers <[email protected]> wrote:
>
> On Wed, Feb 7, 2024 at 11:04 AM Namhyung Kim <[email protected]> wrote:
> >
> > On Tue, Feb 6, 2024 at 3:34 PM Ian Rogers <[email protected]> wrote:
> > >
> > > On Tue, Feb 6, 2024 at 3:04 PM Namhyung Kim <[email protected]> wrote:
> > > >
> > > > Hi Ian,
> > > >
> > > > On Fri, Feb 2, 2024 at 5:42 PM Ian Rogers <[email protected]> wrote:
> > > > >
> > > > > On Fri, Feb 2, 2024 at 2:05 PM Namhyung Kim <[email protected]> wrote:
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Hi Namhyung,
> > > > >
> > > > > I think the naming can be better here. Aren't the objdump addresses
> > > > > DSO relative offsets? Is the relative IP relative to the map or the
> > > > > DSO?
> > > >
> > > > AFAIK the objdump addresses are DSO-relative and rip is to map.
> > > > They are mostly the same but sometimes different due to kASLR
> > > > for the kernel.
> > >
> > > Perhaps we need to use names like map_rip for mapping relative and
> > > dso_rip to clean this up, or to add a different mapping_type to the
> > > enum. For non-kernel maps addresses for map are either the whole
> > > virtual address space (identity) or relative to a dso:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n115
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/map.h?h=perf-tools-next#n20
> > > The dso addresses should work for objdump so perhaps the kernel
> > > addresses need map__pgoff fixing?
> >
> > I'm not sure about the vDSO case.
> >
> > By the way, I need to take a look if we can make this objdump-rip
> > thing simpler as you mentioned. My feeling is that it can be done
> > but I'd like to do it in a separate work and to move this forward.
>
> Makes sense. Could we add a comment to the header file definition to
> capture some of this? Perhaps a "to be removed" to avoid later patches
> adding dependencies to the function.

Sure, will add that in v6.

Thanks,
Namhyung