generate full callchain cursor entries for inlined frames

The follow-up commits will make inline frames first-class citizens in the callchain, thereby obsoleting all of this special code.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Yao Jin <[email protected]>

Signed-off-by Milian Wolff <[email protected]>

---
 tools/perf/ui/browsers/hists.c  | 180 +++-------------------------------------
 tools/perf/ui/stdio/hist.c      |  77 +----------------
 tools/perf/util/evsel_fprintf.c |  32 -------
 tools/perf/util/hist.c          |   5 --
 tools/perf/util/sort.h          |   1 -
 5 files changed, 13 insertions(+), 282 deletions(-)

tools/perf/ui/browsers/hists.c +12 -168

index f4bc2462bc2c..48d056bb3747 100644
@@ -154,57 +154,9 @@ static void callchain_list__set_folding(struct callchain_list *cl, bool unfold)
 	cl->unfolded = unfold ? cl->has_children : false;
 }
 
-static struct inline_node *inline_node__create(struct map *map, u64 ip)
-{
-	struct dso *dso;
-	struct inline_node *node;
-
-	if (map == NULL)
-		return NULL;
-
-	dso = map->dso;
-	if (dso == NULL)
-		return NULL;
-
-	node = dso__parse_addr_inlines(dso,
-				       map__rip_2objdump(map, ip));
-
-	return node;
-}
-
-static int inline__count_rows(struct inline_node *node)
-{
-	struct inline_list *ilist;
-	int i = 0;
-
-	if (node == NULL)
-		return 0;
-
-	list_for_each_entry(ilist, &node->val, list) {
-		if ((ilist->filename != NULL) || (ilist->funcname != NULL))
-			i++;
-	}
-
-	return i;
-}
-
-static int callchain_list__inline_rows(struct callchain_list *chain)
-{
-	struct inline_node *node;
-	int rows;
-
-	node = inline_node__create(chain->ms.map, chain->ip);
-	if (node == NULL)
-		return 0;
-
-	rows = inline__count_rows(node);
-	inline_node__delete(node);
-	return rows;
-}
-
 static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
 {
-	int n = 0, inline_rows;
+	int n = 0;
 	struct rb_node *nd;
 
 	for (nd = rb_first(&node->rb_root); nd; nd = rb_next(nd)) {

@@ -215,12 +167,6 @@ static int callchain_node__count_rows_rb_tree(struct callchain_node *node)
 		list_for_each_entry(chain, &child->val, list) {
 			++n;
 
-			if (symbol_conf.inline_name) {
-				inline_rows =
-					callchain_list__inline_rows(chain);
-				n += inline_rows;
-			}
-
 			/* We need this because we may not have children */
 			folded_sign = callchain_list__folded(chain);
 			if (folded_sign == '+')

@@ -272,7 +218,7 @@ static int callchain_node__count_rows(struct callchain_node *node)
 {
 	struct callchain_list *chain;
 	bool unfolded = false;
-	int n = 0, inline_rows;
+	int n = 0;
 
 	if (callchain_param.mode == CHAIN_FLAT)
 		return callchain_node__count_flat_rows(node);

@@ -281,10 +227,6 @@ static int callchain_node__count_rows(struct callchain_node *node)
 
 	list_for_each_entry(chain, &node->val, list) {
 		++n;
-		if (symbol_conf.inline_name) {
-			inline_rows = callchain_list__inline_rows(chain);
-			n += inline_rows;
-		}
 
 		unfolded = chain->unfolded;
 	}

@@ -432,19 +374,6 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 	he->init_have_children = true;
 }
 
-static void hist_entry_init_inline_node(struct hist_entry *he)
-{
-	if (he->inline_node)
-		return;
-
-	he->inline_node = inline_node__create(he->ms.map, he->ip);
-
-	if (he->inline_node == NULL)
-		return;
-
-	he->has_children = true;
-}
-
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
 	struct hist_entry *he = browser->he_selection;

@@ -476,12 +405,8 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 
 		if (he->unfolded) {
 			if (he->leaf)
-				if (he->inline_node)
-					he->nr_rows = inline__count_rows(
-							he->inline_node);
-				else
-					he->nr_rows = callchain__count_rows(
-							&he->sorted_chain);
+				he->nr_rows = callchain__count_rows(
+						&he->sorted_chain);
 			else
 				he->nr_rows = hierarchy_count_rows(browser, he, false);
 

@@ -841,71 +766,6 @@ static bool hist_browser__check_dump_full(struct hist_browser *browser __maybe_u
 
 #define LEVEL_OFFSET_STEP 3
 
-static int hist_browser__show_inline(struct hist_browser *browser,
-				     struct inline_node *node,
-				     unsigned short row,
-				     int offset)
-{
-	struct inline_list *ilist;
-	char buf[1024];
-	int color, width, first_row;
-
-	first_row = row;
-	width = browser->b.width - (LEVEL_OFFSET_STEP + 2);
-	list_for_each_entry(ilist, &node->val, list) {
-		if ((ilist->filename != NULL) || (ilist->funcname != NULL)) {
-			color = HE_COLORSET_NORMAL;
-			if (ui_browser__is_current_entry(&browser->b, row))
-				color = HE_COLORSET_SELECTED;
-
-			if (callchain_param.key == CCKEY_ADDRESS ||
-			    callchain_param.key == CCKEY_SRCLINE) {
-				if (ilist->filename != NULL)
-					scnprintf(buf, sizeof(buf),
-						  "%s:%d (inline)",
-						  ilist->filename,
-						  ilist->line_nr);
-				else
-					scnprintf(buf, sizeof(buf), "??");
-			} else if (ilist->funcname != NULL)
-				scnprintf(buf, sizeof(buf), "%s (inline)",
-					  ilist->funcname);
-			else if (ilist->filename != NULL)
-				scnprintf(buf, sizeof(buf),
-					  "%s:%d (inline)",
-					  ilist->filename,
-					  ilist->line_nr);
-			else
-				scnprintf(buf, sizeof(buf), "??");
-
-			ui_browser__set_color(&browser->b, color);
-			hist_browser__gotorc(browser, row, 0);
-			ui_browser__write_nstring(&browser->b, " ",
-				LEVEL_OFFSET_STEP + offset);
-			ui_browser__write_nstring(&browser->b, buf, width);
-			row++;
-		}
-	}
-
-	return row - first_row;
-}
-
-static size_t show_inline_list(struct hist_browser *browser, struct map *map,
-			       u64 ip, int row, int offset)
-{
-	struct inline_node *node;
-	int ret;
-
-	node = inline_node__create(map, ip);
-	if (node == NULL)
-		return 0;
-
-	ret = hist_browser__show_inline(browser, node, row, offset);
-
-	inline_node__delete(node);
-	return ret;
-}
-
 static int hist_browser__show_callchain_list(struct hist_browser *browser,
 					     struct callchain_node *node,
 					     struct callchain_list *chain,

@@ -917,7 +777,7 @@ static int hist_browser__show_callchain_list(struct hist_browser *browser,
 	char bf[1024], *alloc_str;
 	char buf[64], *alloc_str2;
 	const char *str;
-	int inline_rows = 0, ret = 1;
+	int ret = 1;
 
 	if (arg->row_offset != 0) {
 		arg->row_offset--;

@@ -958,12 +818,7 @@ static int hist_browser__show_callchain_list(struct hist_browser *browser,
 	free(alloc_str);
 	free(alloc_str2);
 
-	if (symbol_conf.inline_name) {
-		inline_rows = show_inline_list(browser, chain->ms.map,
-					       chain->ip, row + 1, offset);
-	}
-
-	return ret + inline_rows;
+	return ret;
 }
 
 static bool check_percent_display(struct rb_node *node, u64 parent_total)

@@ -1387,12 +1242,6 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		folded_sign = hist_entry__folded(entry);
 	}
 
-	if (symbol_conf.inline_name &&
-	    (!entry->has_children)) {
-		hist_entry_init_inline_node(entry);
-		folded_sign = hist_entry__folded(entry);
-	}
-
 	if (row_offset == 0) {
 		struct hpp_arg arg = {
 			.b		= &browser->b,

@@ -1424,8 +1273,7 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 			}
 
 			if (first) {
-				if (symbol_conf.use_callchain ||
-					symbol_conf.inline_name) {
+				if (symbol_conf.use_callchain) {
 					ui_browser__printf(&browser->b, "%c ", folded_sign);
 					width -= 2;
 				}

@@ -1467,15 +1315,11 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 			.is_current_entry = current_entry,
 		};
 
-		if (entry->inline_node)
-			printed += hist_browser__show_inline(browser,
-					entry->inline_node, row, 0);
-		else
-			printed += hist_browser__show_callchain(browser,
-					entry, 1, row,
-					hist_browser__show_callchain_entry,
-					&arg,
-					hist_browser__check_output_full);
+		printed += hist_browser__show_callchain(browser,
+				entry, 1, row,
+				hist_browser__show_callchain_entry,
+				&arg,
+				hist_browser__check_output_full);
 	}
 
 	return printed;

tools/perf/ui/stdio/hist.c +1 -76

index 5c95b8301c67..8585bc628e0c 100644
@@ -21,64 +21,6 @@ static size_t callchain__fprintf_left_margin(FILE *fp, int left_margin)
 	return ret;
 }
 
-static size_t inline__fprintf(struct map *map, u64 ip, int left_margin,
-			      int depth, int depth_mask, FILE *fp)
-{
-	struct dso *dso;
-	struct inline_node *node;
-	struct inline_list *ilist;
-	int ret = 0, i;
-
-	if (map == NULL)
-		return 0;
-
-	dso = map->dso;
-	if (dso == NULL)
-		return 0;
-
-	node = dso__parse_addr_inlines(dso,
-				       map__rip_2objdump(map, ip));
-	if (node == NULL)
-		return 0;
-
-	list_for_each_entry(ilist, &node->val, list) {
-		if ((ilist->filename != NULL) || (ilist->funcname != NULL)) {
-			ret += callchain__fprintf_left_margin(fp, left_margin);
-
-			for (i = 0; i < depth; i++) {
-				if (depth_mask & (1 << i))
-					ret += fprintf(fp, "|");
-				else
-					ret += fprintf(fp, " ");
-				ret += fprintf(fp, "          ");
-			}
-
-			if (callchain_param.key == CCKEY_ADDRESS ||
-			    callchain_param.key == CCKEY_SRCLINE) {
-				if (ilist->filename != NULL)
-					ret += fprintf(fp, "%s:%d (inline)",
-						       ilist->filename,
-						       ilist->line_nr);
-				else
-					ret += fprintf(fp, "??");
-			} else if (ilist->funcname != NULL)
-				ret += fprintf(fp, "%s (inline)",
-					       ilist->funcname);
-			else if (ilist->filename != NULL)
-				ret += fprintf(fp, "%s:%d (inline)",
-					       ilist->filename,
-					       ilist->line_nr);
-			else
-				ret += fprintf(fp, "??");
-
-			ret += fprintf(fp, "\n");
-		}
-	}
-
-	inline_node__delete(node);
-	return ret;
-}
-
 static size_t ipchain__fprintf_graph_line(FILE *fp, int depth, int depth_mask,
 					  int left_margin)
 {

@@ -141,9 +83,6 @@ static size_t ipchain__fprintf_graph(FILE *fp, struct callchain_node *node,
 	fputc('\n', fp);
 	free(alloc_str);
 
-	if (symbol_conf.inline_name)
-		ret += inline__fprintf(chain->ms.map, chain->ip,
-				       left_margin, depth, depth_mask, fp);
 	return ret;
 }
 

@@ -318,13 +257,6 @@ static size_t callchain__fprintf_graph(FILE *fp, struct rb_root *root,
 
 			if (++entries_printed == callchain_param.print_limit)
 				break;
-
-			if (symbol_conf.inline_name)
-				ret += inline__fprintf(chain->ms.map,
-						       chain->ip,
-						       left_margin,
-						       0, 0,
-						       fp);
 		}
 		root = &cnode->rb_root;
 	}

@@ -604,7 +536,6 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 {
 	int ret;
 	int callchain_ret = 0;
-	int inline_ret = 0;
 	struct perf_hpp hpp = {
 		.buf		= bf,
 		.size		= size,

@@ -626,13 +557,7 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 		callchain_ret = hist_entry_callchain__fprintf(he, total_period,
 							      0, fp);
 
-	if (callchain_ret == 0 && symbol_conf.inline_name) {
-		inline_ret = inline__fprintf(he->ms.map, he->ip, 0, 0, 0, fp);
-		ret += inline_ret;
-		if (inline_ret > 0)
-			ret += fprintf(fp, "\n");
-	} else
-		ret += callchain_ret;
+	ret += callchain_ret;
 
 	return ret;
 }

tools/perf/util/evsel_fprintf.c +0 -32

index 583f3a602506..f2c6c5ee11e8 100644
@@ -169,38 +169,6 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 			if (!print_oneline)
 				printed += fprintf(fp, "\n");
 
-			if (symbol_conf.inline_name && node->map) {
-				struct inline_node *inode;
-
-				addr = map__rip_2objdump(node->map, node->ip),
-				inode = dso__parse_addr_inlines(node->map->dso, addr);
-
-				if (inode) {
-					struct inline_list *ilist;
-
-					list_for_each_entry(ilist, &inode->val, list) {
-						if (print_arrow)
-							printed += fprintf(fp, " <-");
-
-						/* IP is same, just skip it */
-						if (print_ip)
-							printed += fprintf(fp, "%c%16s",
-									   s, "");
-						if (print_sym)
-							printed += fprintf(fp, " %s",
-									   ilist->funcname);
-						if (print_srcline)
-							printed += fprintf(fp, "\n  %s:%d",
-									   ilist->filename,
-									   ilist->line_nr);
-						if (!print_oneline)
-							printed += fprintf(fp, "\n");
-					}
-
-					inline_node__delete(inode);
-				}
-			}
-
 			if (symbol_conf.bt_stop_list &&
 			    node->sym &&
 			    strlist__has_entry(symbol_conf.bt_stop_list,

tools/perf/util/hist.c +0 -5

index 9453b2e27015..043a54871276 100644
@@ -1137,11 +1137,6 @@ void hist_entry__delete(struct hist_entry *he)
 		zfree(&he->mem_info);
 	}
 
-	if (he->inline_node) {
-		inline_node__delete(he->inline_node);
-		he->inline_node = NULL;
-	}
-
 	zfree(&he->stat_acc);
 	free_srcline(he->srcline);
 	if (he->srcfile && he->srcfile[0])

tools/perf/util/sort.h +0 -1

index b7c75597e18f..2f41416f01fa 100644
@@ -129,7 +129,6 @@ struct hist_entry {
 	};
 	char			*srcline;
 	char			*srcfile;
-	struct inline_node	*inline_node;
 	struct symbol		*parent;
 	struct branch_info	*branch_info;
 	struct hists		*hists;

Signature · 1 line

2.13.3

Em Sun, Aug 06, 2017 at 11:24:33PM +0200, Milian Wolff escreveu:

Quoted 2 lines

The follow-up commits will make inline frames first-class citizens in the callchain, thereby obsoleting all of this special code.

So you are removing the feature to then reintroduce it, is that it? That is not usual :-\

Normally we go on replacing bit by bit or have some ifdef, etc, to then phase out the old code.

Perhaps in this case your approach is the best one, still have to look at all of it, and it would help if the people behind the original code could review this, Yao Jin, can you take a look at this patch series, please?

Quoted 9 lines

Em Sun, Aug 06, 2017 at 11:24:33PM +0200, Milian Wolff escreveu:

Quoted 2 lines

The follow-up commits will make inline frames first-class citizens in the callchain, thereby obsoleting all of this special code.

So you are removing the feature to then reintroduce it, is that it? That is not usual :-\

Normally we go on replacing bit by bit or have some ifdef, etc, to then phase out the old code.

Quoted 4 lines

Perhaps in this case your approach is the best one, still have to look at all of it, and it would help if the people behind the original code could review this, Yao Jin, can you take a look at this patch series, please?

Yes, I also did that in v1 of this patch series. Note that I can easily squash this, if needed. But I personally think for reviewing purposes, having it as separate patches is far better.

The input string is not modified and thus can be passed in as a pointer to const data.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Yao Jin <[email protected]>

Signed-off-by Milian Wolff <[email protected]>

---
 tools/perf/util/symbol-elf.c     | 2 +-
 tools/perf/util/symbol-minimal.c | 2 +-
 tools/perf/util/symbol.h         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

tools/perf/util/symbol-elf.c +1 -1

index 502505cf236a..7cf18f14e152 100644
@@ -391,7 +391,7 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss, struct map *
 	return 0;
 }
 
-char *dso__demangle_sym(struct dso *dso, int kmodule, char *elf_name)
+char *dso__demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
 {
 	return demangle_sym(dso, kmodule, elf_name);
 }

tools/perf/util/symbol-minimal.c +1 -1

index 40bf5d4c0bfd..1a5aa35b0100 100644
@@ -377,7 +377,7 @@ void symbol__elf_init(void)
 
 char *dso__demangle_sym(struct dso *dso __maybe_unused,
 			int kmodule __maybe_unused,
-			char *elf_name __maybe_unused)
+			const char *elf_name __maybe_unused)
 {
 	return NULL;
 }

tools/perf/util/symbol.h +1 -1

index 41ebba9a2eb2..f0b08810d7fa 100644
@@ -306,7 +306,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss,
 				struct map *map);
 
-char *dso__demangle_sym(struct dso *dso, int kmodule, char *elf_name);
+char *dso__demangle_sym(struct dso *dso, int kmodule, const char *elf_name);
 
 void __symbols__insert(struct rb_root *symbols, struct symbol *sym, bool kernel);
 void symbols__insert(struct rb_root *symbols, struct symbol *sym);

Signature · 1 line

2.13.3

The inlined frames use a fake symbol that is tracked in a special map inside the dso, which is always sorted by name. All other

entries of the symbol beside the function name are unused for inline frames. The advantage of this approach is that all existing users of the callchain API can now transparently display inlined frames without having to patch their code.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Yao Jin <[email protected]>

Signed-off-by Milian Wolff <[email protected]>

---
 tools/perf/util/callchain.c |  31 +++-----
 tools/perf/util/callchain.h |   6 +-
 tools/perf/util/dso.c       |   2 +
 tools/perf/util/dso.h       |   1 +
 tools/perf/util/machine.c   |  56 +++++++++++++-
 tools/perf/util/srcline.c   | 183 ++++++++++++++++++++++++++++++++++----------
 tools/perf/util/srcline.h   |  19 ++++-
 tools/perf/util/symbol.h    |   1 +
 8 files changed, 230 insertions(+), 69 deletions(-)

tools/perf/util/callchain.c +9 -22

index f320b0777e0d..9854adb06ac1 100644
@@ -559,6 +559,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		call->ip = cursor_node->ip;
 		call->ms.sym = cursor_node->sym;
 		call->ms.map = map__get(cursor_node->map);
+		call->srcline = cursor_node->srcline;
 
 		if (cursor_node->branch) {
 			call->branch_count = 1;

@@ -640,20 +641,11 @@ enum match_result {
 static enum match_result match_chain_srcline(struct callchain_cursor_node *node,
 					     struct callchain_list *cnode)
 {
-	char *left = NULL;
-	char *right = NULL;
+	const char *left = cnode->srcline;
+	const char *right = node->srcline;
 	enum match_result ret = MATCH_EQ;
 	int cmp;
 
-	if (cnode->ms.map)
-		left = get_srcline(cnode->ms.map->dso,
-				 map__rip_2objdump(cnode->ms.map, cnode->ip),
-				 cnode->ms.sym, true, false);
-	if (node->map)
-		right = get_srcline(node->map->dso,
-				  map__rip_2objdump(node->map, node->ip),
-				  node->sym, true, false);
-
 	if (left && right)
 		cmp = strcmp(left, right);
 	else if (!left && right)

@@ -668,8 +660,6 @@ static enum match_result match_chain_srcline(struct callchain_cursor_node *node,
 	if (cmp != 0)
 		ret = cmp < 0 ? MATCH_LT : MATCH_GT;
 
-	free_srcline(left);
-	free_srcline(right);
 	return ret;
 }
 

@@ -958,7 +948,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
 	list_for_each_entry_safe(list, next_list, &src->val, list) {
 		callchain_cursor_append(cursor, list->ip,
 					list->ms.map, list->ms.sym,
-					false, NULL, 0, 0, 0);
+					false, NULL, 0, 0, 0, list->srcline);
 		list_del(&list->list);
 		map__zput(list->ms.map);
 		free(list);

@@ -998,7 +988,8 @@ int callchain_merge(struct callchain_cursor *cursor,
 int callchain_cursor_append(struct callchain_cursor *cursor,
 			    u64 ip, struct map *map, struct symbol *sym,
 			    bool branch, struct branch_flags *flags,
-			    int nr_loop_iter, int samples, u64 branch_from)
+			    int nr_loop_iter, int samples, u64 branch_from,
+			    const char *srcline)
 {
 	struct callchain_cursor_node *node = *cursor->last;
 

@@ -1017,6 +1008,7 @@ int callchain_cursor_append(struct callchain_cursor *cursor,
 	node->branch = branch;
 	node->nr_loop_iter = nr_loop_iter;
 	node->samples = samples;
+	node->srcline = srcline;
 
 	if (flags)
 		memcpy(&node->branch_flags, flags,

@@ -1104,12 +1096,7 @@ char *callchain_list__sym_name(struct callchain_list *cl,
 	int printed;
 
 	if (cl->ms.sym) {
-		if (show_srcline && cl->ms.map && !cl->srcline)
-			cl->srcline = get_srcline(cl->ms.map->dso,
-						  map__rip_2objdump(cl->ms.map,
-								    cl->ip),
-						  cl->ms.sym, false, show_addr);
-		if (cl->srcline)
+		if (show_srcline && cl->srcline)
 			printed = scnprintf(bf, bfsize, "%s %s",
 					cl->ms.sym->name, cl->srcline);
 		else

@@ -1524,7 +1511,7 @@ int callchain_cursor__copy(struct callchain_cursor *dst,
 		rc = callchain_cursor_append(dst, node->ip, node->map, node->sym,
 					     node->branch, &node->branch_flags,
 					     node->nr_loop_iter, node->samples,
-					     node->branch_from);
+					     node->branch_from, node->srcline);
 		if (rc)
 			break;
 

tools/perf/util/callchain.h +2 -1

index 97738201464a..bf81b56f34c3 100644
@@ -121,7 +121,7 @@ struct callchain_list {
 	u64			iter_count;
 	u64			samples_count;
 	struct branch_type_stat brtype_stat;
-	char		       *srcline;
+	const char		*srcline;
 	struct list_head	list;
 };
 

@@ -135,6 +135,7 @@ struct callchain_cursor_node {
 	u64				ip;
 	struct map			*map;
 	struct symbol			*sym;
+	const char			*srcline;
 	bool				branch;
 	struct branch_flags		branch_flags;
 	u64				branch_from;

tools/perf/util/dso.c +1 -0

index b9e087fb8247..72e6e390fd26 100644
@@ -9,6 +9,7 @@
 #include "compress.h"
 #include "path.h"
 #include "symbol.h"
+#include "srcline.h"
 #include "dso.h"
 #include "machine.h"
 #include "auxtrace.h"

tools/perf/util/dso.c +0 -0

 	if (dso->short_name_allocated) {
 		zfree((char **)&dso->short_name);

tools/perf/util/dso.h +1 -0

index f886141678eb..7d1e2b3c1f10 100644
@@ -141,6 +141,7 @@ struct dso {
 	struct rb_root	 *root;		/* root of rbtree that rb_node is in */
 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
+	struct rb_root	 inlined_nodes;
 	struct {
 		u64		addr;
 		struct symbol	*symbol;

tools/perf/util/machine.c +0 -0

index d4df353051af..a7f8499c8756 100644

tools/perf/util/srcline.c +34 -15

index ebc88a74e67b..a1fdf035d1dd 100644
@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)
 	return dso_name;
 }
 
-static int inline_list__append(char *filename, char *funcname, int line_nr,
-			       struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+			       struct inline_node *node)
 {
 	struct inline_list *ilist;
-	char *demangled;
 
 	ilist = zalloc(sizeof(*ilist));
 	if (ilist == NULL)
 		return -1;
 
-	ilist->filename = filename;
-	ilist->line_nr = line_nr;
-
-	if (dso != NULL) {
-		demangled = dso__demangle_sym(dso, 0, funcname);
-		if (demangled == NULL) {
-			ilist->funcname = funcname;
-		} else {
-			ilist->funcname = demangled;
-			free(funcname);
-		}
-	}
+	ilist->symbol = symbol;
+	ilist->srcline = srcline;
 
 	if (callchain_param.order == ORDER_CALLEE)
 		list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char *funcname, int line_nr,
 	return 0;
 }
 
+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+	const char *base = strrchr(path, '/');
+
+	return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+	char *srcline;
+
+	if (!file)
+		return NULL;
+
+	if (!srcline_full_filename)
+		file = gnu_basename(file);
+
+	if (asprintf(&srcline, "%s:%u", file, line) < 0)
+		return NULL;
+
+	return srcline;
+}
+
 #ifdef HAVE_LIBBFD_SUPPORT
 
 /*

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data *a2l)
 
 #define MAX_INLINE_NEST 1024
 
+static struct symbol *new_inline_sym(struct dso *dso,
+				     struct symbol *base_sym,
+				     const char *funcname)
+{
+	struct symbol *inline_sym;
+	char *demangled = NULL;

tools/perf/util/srcline.c +23 -4

+	} else {
+		// create a fake symbol for the inline frame
+		inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+					 base_sym ? base_sym->end : 0,
+					 base_sym ? base_sym->binding : 0,
+					 funcname);
+		if (inline_sym)
+			inline_sym->inlined = 1;
+	}
+
+	free(demangled);
+
+	return inline_sym;
+}
+
 static int inline_list__append_dso_a2l(struct dso *dso,
-				       struct inline_node *node)
+				       struct inline_node *node,
+				       struct symbol *sym)
 {
 	struct a2l_data *a2l = dso->a2l;
-	char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-	char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+	struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+	char *srcline = NULL;
+
+	if (a2l->filename)
+		srcline = srcline_from_fileline(a2l->filename, a2l->line);
 
-	return inline_list__append(filename, funcname, a2l->line, node, dso);
+	return inline_list__append(inline_sym, srcline, node);
 }
 

tools/perf/util/srcline.c +51 -0

 		free(ilist);
 	}
 
 	free(node);
 }
+
+void inlines__tree_insert(struct rb_root *tree, struct inline_node *inlines)
+{
+	struct rb_node **p = &tree->rb_node;
+	struct rb_node *parent = NULL;
+	const u64 addr = inlines->addr;
+	struct inline_node *i;
+
+	while (*p != NULL) {
+		parent = *p;
+		i = rb_entry(parent, struct inline_node, rb_node);
+		if (addr < i->addr)
+			p = &(*p)->rb_left;
+		else
+			p = &(*p)->rb_right;
+	}
+	rb_link_node(&inlines->rb_node, parent, p);
+	rb_insert_color(&inlines->rb_node, tree);
+}
+
+struct inline_node *inlines__tree_find(struct rb_root *tree, u64 addr)
+{
+	struct rb_node *n = tree->rb_node;
+
+	while (n) {
+		struct inline_node *i = rb_entry(n, struct inline_node,
+						 rb_node);
+
+		if (addr < i->addr)
+			n = n->rb_left;
+		else if (addr > i->addr)
+			n = n->rb_right;
+		else
+			return i;
+	}
+
+	return NULL;
+}
+
+void inlines__tree_delete(struct rb_root *tree)
+{
+	struct inline_node *pos;
+	struct rb_node *next = rb_first(tree);
+
+	while (next) {
+		pos = rb_entry(next, struct inline_node, rb_node);
+		next = rb_next(&pos->rb_node);
+		rb_erase(&pos->rb_node, tree);
+		inline_node__delete(pos);
+	}
+}

tools/perf/util/srcline.h +15 -4

index 7b52ba88676e..0d2aca92e8c7 100644
@@ -2,6 +2,7 @@
 #define PERF_SRCLINE_H
 
 #include <linux/list.h>
+#include <linux/rbtree.h>
 #include <linux/types.h>
 
 struct dso;

@@ -17,18 +18,28 @@ void free_srcline(char *srcline);
 #define SRCLINE_UNKNOWN  ((char *) "??:0")
 
 struct inline_list {
-	char			*filename;
-	char			*funcname;
-	unsigned int		line_nr;
+	struct symbol		*symbol;
+	char			*srcline;
 	struct list_head	list;
 };
 
 struct inline_node {
 	u64			addr;
 	struct list_head	val;
+	struct rb_node		rb_node;
 };
 
-struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr);
+// parse inlined frames for the given address
+struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr,
+					    struct symbol *sym);
+// free resources associated to the inline node list
 void inline_node__delete(struct inline_node *node);
 
+// insert the inline node list into the DSO, which will take ownership
+void inlines__tree_insert(struct rb_root *tree, struct inline_node *inlines);
+// find previously inserted inline node list
+struct inline_node *inlines__tree_find(struct rb_root *tree, u64 addr);
+// delete all nodes within the tree of inline_node s
+void inlines__tree_delete(struct rb_root *tree);
+
 #endif /* PERF_SRCLINE_H */

tools/perf/util/symbol.h +1 -0

index f0b08810d7fa..b358570ce615 100644
@@ -59,6 +59,7 @@ struct symbol {
 	u8		binding;
 	u8		idle:1;
 	u8		ignore:1;
+	u8		inlined:1;
 	u8		arch_sym;
 	char		name[0];
 };

Signature · 1 line

2.13.3

Hi Milian,

Quoted 2 lines · Milian Wolff

The inlined frames use a fake symbol that is tracked in a special map inside the dso, which is always sorted by name. All other

It seems the above is not true. Fake symbols are maintained by inline_node which in turn maintained by dso->inlines tree.

Quoted 11 lines

(quoted diff) +0 -0

@@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct

callchain_cursor *cursor)>
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,

tools/perf/util/callchain.h +2 -1

 			    struct map *map, struct symbol *sym,
 			    bool branch, struct branch_flags *flags,
 
-			    int nr_loop_iter, int samples, u64 branch_from);
+			    int nr_loop_iter, int samples, u64 branch_from,
+			    const char *srcline);

Quoted 3 lines

/* Close a cursor writing session. Initialize for the reader */ static inline void callchain_cursor_commit(struct callchain_cursor *cursor)

I think it'd be better splitting srcline change into a separate commit.

Quoted 17 lines

tools/perf/util/dso.c +1 -0

index b9e087fb8247..72e6e390fd26 100644
@@ -9,6 +9,7 @@
 
 #include "compress.h"
 #include "path.h"
 #include "symbol.h"
 
+#include "srcline.h"
 

#include "dso.h"
#include "machine.h"
#include "auxtrace.h"

tools/perf/util/dso.c void dso__delete(struct dso *dso)

Quoted 8 lines

tools/perf/util/dso.c +1 -0

@@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso)
 
 		       dso->long_name);
 
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 
 		symbols__delete(&dso->symbols[i]);
 
+	inlines__tree_delete(&dso->inlined_nodes);

Hmm.. inline_node is released after symbol but it seems to have a problem. Please see below.

Quoted 4 lines

tools/perf/util/dso.c +0 -0

 	if (dso->short_name_allocated) {
 
 		zfree((char **)&dso->short_name);

tools/perf/util/dso.h struct dso { · line 142

Quoted 7 lines

tools/perf/util/dso.h +0 -0

index f886141678eb..7d1e2b3c1f10 100644
@@ -141,6 +141,7 @@ struct dso {
 
 	struct rb_root	 *root;		/* root of rbtree that rb_node is in 

Quoted 9 lines

tools/perf/util/dso.h +1 -0

 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
 
+	struct rb_root	 inlined_nodes;
 

tools/perf/util/dso.h +0 -0

 	struct {
 
 		u64		addr;
 		struct symbol	*symbol;

[SNIP]

Quoted 98 lines

tools/perf/util/machine.c +0 -0

index d4df353051af..a7f8499c8756 100644

tools/perf/util/srcline.c +28 -14

index ebc88a74e67b..a1fdf035d1dd 100644
@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)
 
 	return dso_name;
 
 }
 
-static int inline_list__append(char *filename, char *funcname, int
 line_nr, -			       struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+			       struct inline_node *node)
 
 {
 
 	struct inline_list *ilist;
 
-	char *demangled;
 
 	ilist = zalloc(sizeof(*ilist));
 	if (ilist == NULL)
 
 		return -1;
 
-	ilist->filename = filename;
-	ilist->line_nr = line_nr;
-
-	if (dso != NULL) {
-		demangled = dso__demangle_sym(dso, 0, funcname);
-		if (demangled == NULL) {
-			ilist->funcname = funcname;
-		} else {
-			ilist->funcname = demangled;
-			free(funcname);
-		}
-	}
+	ilist->symbol = symbol;
+	ilist->srcline = srcline;
 
 	if (callchain_param.order == ORDER_CALLEE)
 
 		list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char
 *funcname, int line_nr,> 
 	return 0;
 
 }
 
+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+	const char *base = strrchr(path, '/');
+
+	return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+	char *srcline;
+
+	if (!file)
+		return NULL;
+
+	if (!srcline_full_filename)
+		file = gnu_basename(file);
+
+	if (asprintf(&srcline, "%s:%u", file, line) < 0)
+		return NULL;
+
+	return srcline;
+}
+
 

#ifdef HAVE_LIBBFD_SUPPORT

/*

tools/perf/util/srcline.c +10 -0

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data *a2l)
 
 #define MAX_INLINE_NEST 1024
 
+static struct symbol *new_inline_sym(struct dso *dso,
+				     struct symbol *base_sym,
+				     const char *funcname)
+{
+	struct symbol *inline_sym;
+	char *demangled = NULL;
+
+	if (dso) {
+		demangled = dso__demangle_sym(dso, 0, funcname);
+		if (demangled)

tools/perf/util/srcline.c static void addr2line_cleanup(struct a2l_data *a2l) · line 234

Quoted 6 lines

tools/perf/util/srcline.c +6 -0

+			funcname = demangled;
+	}
+
+	if (strcmp(funcname, base_sym->name) == 0) {
+		// reuse the real, existing symbol
+		inline_sym = base_sym;

So inline_node could refer the existing symbol.

Quoted 38 lines

tools/perf/util/srcline.c +23 -4

+	} else {
+		// create a fake symbol for the inline frame
+		inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+					 base_sym ? base_sym->end : 0,
+					 base_sym ? base_sym->binding : 0,
+					 funcname);
+		if (inline_sym)
+			inline_sym->inlined = 1;
+	}
+
+	free(demangled);
+
+	return inline_sym;
+}
+
 
 static int inline_list__append_dso_a2l(struct dso *dso,
 
-				       struct inline_node *node)
+				       struct inline_node *node,
+				       struct symbol *sym)
 
 {
 
 	struct a2l_data *a2l = dso->a2l;
 
-	char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-	char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+	struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+	char *srcline = NULL;
+
+	if (a2l->filename)
+		srcline = srcline_from_fileline(a2l->filename, a2l->line);
 
-	return inline_list__append(filename, funcname, a2l->line, node, dso);
+	return inline_list__append(inline_sym, srcline, node);
 
 }

[SNIP]

Quoted 6 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node)
 
 	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
 		list_del_init(&ilist->list);
 

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-		zfree(&ilist->filename);
-		zfree(&ilist->funcname);
+		zfree(&ilist->srcline);
+		// only the inlined symbols are owned by the list
+		if (ilist->symbol && ilist->symbol->inlined)
+			symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Quoted 3 lines · On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:

Hi Milian,

On Sun, Aug 06, 2017 at 11:24:35PM +0200, Milian Wolff wrote:

Quoted 5 lines

Quoted 2 lines

The inlined frames use a fake symbol that is tracked in a special map inside the dso, which is always sorted by name. All other

It seems the above is not true. Fake symbols are maintained by inline_node which in turn maintained by dso->inlines tree.

OK, I'll rephrase this to make it more explicit. But what I call "map" is what you call "tree", no?

<snip>

Quoted 11 lines

Quoted 11 lines

(quoted diff) +0 -0

@@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct

callchain_cursor *cursor)>
int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,

struct map *map, struct symbol *sym, bool branch, struct branch_flags *flags,

tools/perf/util/callchain.h +2 -1

-                       int nr_loop_iter, int samples, u64 branch_from);
+                       int nr_loop_iter, int samples, u64 branch_from,
+                       const char *srcline);

Quoted 6 lines

Quoted 3 lines

/* Close a cursor writing session. Initialize for the reader */ static inline void callchain_cursor_commit(struct callchain_cursor *cursor)

I think it'd be better splitting srcline change into a separate commit.

OK, I'll try to see how this goes. It would simply add the boiler plate code to pass the srcline around, and then set it from within the callchain code, right?

Quoted 34 lines

Quoted 18 lines

tools/perf/util/dso.c +1 -0

index b9e087fb8247..72e6e390fd26 100644
@@ -9,6 +9,7 @@
 
 #include "compress.h"
 #include "path.h"
 #include "symbol.h"
 
+#include "srcline.h"
 

#include "dso.h"
#include "machine.h"
#include "auxtrace.h"

tools/perf/util/dso.c void dso__delete(struct dso *dso)

Quoted 8 lines

tools/perf/util/dso.c +1 -0

@@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso)
 
                   dso->long_name);
 
    for (i = 0; i < MAP__NR_TYPES; ++i)
 
            symbols__delete(&dso->symbols[i]);
 
+   inlines__tree_delete(&dso->inlined_nodes);

Hmm.. inline_node is released after symbol but it seems to have a problem. Please see below.

Quoted 4 lines

    if (dso->short_name_allocated) {

            zfree((char **)&dso->short_name);

Quoted 7 lines

tools/perf/util/dso.h struct dso { · line 142

Quoted 7 lines

tools/perf/util/dso.h +0 -0

index f886141678eb..7d1e2b3c1f10 100644
@@ -141,6 +141,7 @@ struct dso {
 
    struct rb_root   *root;         /* root of rbtree that rb_node is in

*/

Quoted 110 lines

Quoted 9 lines

tools/perf/util/dso.h +1 -0

    struct rb_root   symbols[MAP__NR_TYPES];
    struct rb_root   symbol_names[MAP__NR_TYPES];
 
+   struct rb_root   inlined_nodes;
 

    struct {

            u64             addr;
            struct symbol   *symbol;

[SNIP]

Quoted 98 lines

tools/perf/util/machine.c +0 -0

index d4df353051af..a7f8499c8756 100644

tools/perf/util/srcline.c +28 -14

index ebc88a74e67b..a1fdf035d1dd 100644
@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)
 
    return dso_name;
 
 }
 
-static int inline_list__append(char *filename, char *funcname, int
 line_nr, -                         struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+                          struct inline_node *node)
 
 {
 
    struct inline_list *ilist;
 
-   char *demangled;
 
    ilist = zalloc(sizeof(*ilist));
    if (ilist == NULL)
 
            return -1;
 
-   ilist->filename = filename;
-   ilist->line_nr = line_nr;
-
-   if (dso != NULL) {
-           demangled = dso__demangle_sym(dso, 0, funcname);
-           if (demangled == NULL) {
-                   ilist->funcname = funcname;
-           } else {
-                   ilist->funcname = demangled;
-                   free(funcname);
-           }
-   }
+   ilist->symbol = symbol;
+   ilist->srcline = srcline;
 
    if (callchain_param.order == ORDER_CALLEE)
 
            list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char
 *funcname, int line_nr,>
    return 0;
 
 }
 
+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+   const char *base = strrchr(path, '/');
+
+   return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+   char *srcline;
+
+   if (!file)
+           return NULL;
+
+   if (!srcline_full_filename)
+           file = gnu_basename(file);
+
+   if (asprintf(&srcline, "%s:%u", file, line) < 0)
+           return NULL;
+
+   return srcline;
+}
+
 

#ifdef HAVE_LIBBFD_SUPPORT

/*

tools/perf/util/srcline.c +10 -0

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data *a2l)
 
 #define MAX_INLINE_NEST 1024
 
+static struct symbol *new_inline_sym(struct dso *dso,
+                                struct symbol *base_sym,
+                                const char *funcname)
+{
+   struct symbol *inline_sym;
+   char *demangled = NULL;
+
+   if (dso) {
+           demangled = dso__demangle_sym(dso, 0, funcname);
+           if (demangled)

Quoted 8 lines

tools/perf/util/srcline.c static void addr2line_cleanup(struct a2l_data *a2l) · line 234

Quoted 6 lines

tools/perf/util/srcline.c +6 -0

+                   funcname = demangled;
+   }
+
+   if (strcmp(funcname, base_sym->name) == 0) {
+           // reuse the real, existing symbol
+           inline_sym = base_sym;

So inline_node could refer the existing symbol.

Yes, that does indeed seem to be an issue. I wonder why I'm not seeing any crashes or valgrind/ASAN reports though. I'll have to dig into this.

Quoted 47 lines

Quoted 38 lines

tools/perf/util/srcline.c +23 -4

+   } else {
+           // create a fake symbol for the inline frame
+           inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+                                    base_sym ? base_sym->end : 0,
+                                    base_sym ? base_sym->binding : 0,
+                                    funcname);
+           if (inline_sym)
+                   inline_sym->inlined = 1;
+   }
+
+   free(demangled);
+
+   return inline_sym;
+}
+
 
 static int inline_list__append_dso_a2l(struct dso *dso,
 
-                                  struct inline_node *node)
+                                  struct inline_node *node,
+                                  struct symbol *sym)
 
 {
 
    struct a2l_data *a2l = dso->a2l;
 
-   char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-   char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+   struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+   char *srcline = NULL;
+
+   if (a2l->filename)
+           srcline = srcline_from_fileline(a2l->filename, a2l->line);
 
-   return inline_list__append(filename, funcname, a2l->line, node, dso);
+   return inline_list__append(inline_sym, srcline, node);
 
 }

[SNIP]

Quoted 6 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node)
 
    list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
            list_del_init(&ilist->list);
 

Quoted 8 lines

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-           zfree(&ilist->filename);
-           zfree(&ilist->funcname);
+           zfree(&ilist->srcline);
+           // only the inlined symbols are owned by the list
+           if (ilist->symbol && ilist->symbol->inlined)
+                   symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Thanks for the review, I'll try to look into these issues once I have more time again.

Cheers

--
Milian Wolff | [email protected] | Senior Software Engineer KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Quoted 8 lines · On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:

Hi Milian,

Quoted 2 lines · Milian Wolff

The inlined frames use a fake symbol that is tracked in a special map inside the dso, which is always sorted by name. All other

It seems the above is not true. Fake symbols are maintained by inline_node which in turn maintained by dso->inlines tree.

OK, I'll rephrase this to make it more explicit. But what I call "map" is what you call "tree", no?

<snip>

Quoted 12 lines

Quoted 12 lines

(quoted diff) +0 -0

@@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct

callchain_cursor *cursor)>

int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,

tools/perf/util/callchain.h +2 -1

 			    struct map *map, struct symbol *sym,
 			    bool branch, struct branch_flags *flags,
 
-			    int nr_loop_iter, int samples, u64 branch_from);
+			    int nr_loop_iter, int samples, u64 branch_from,
+			    const char *srcline);

Quoted 6 lines

Quoted 3 lines

/* Close a cursor writing session. Initialize for the reader */ static inline void callchain_cursor_commit(struct callchain_cursor *cursor)

I think it'd be better splitting srcline change into a separate commit.

OK, I'll try to see how this goes. It would simply add the boiler plate code to pass the srcline around, and then set it from within the callchain code, right?

Quoted 33 lines

Quoted 17 lines

tools/perf/util/dso.c +1 -0

index b9e087fb8247..72e6e390fd26 100644
@@ -9,6 +9,7 @@
 
 #include "compress.h"
 #include "path.h"
 #include "symbol.h"
 
+#include "srcline.h"
 

#include "dso.h"
#include "machine.h"
#include "auxtrace.h"

tools/perf/util/dso.c void dso__delete(struct dso *dso)

Quoted 8 lines

tools/perf/util/dso.c +1 -0

@@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso)
 
 		       dso->long_name);
 
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 
 		symbols__delete(&dso->symbols[i]);
 
+	inlines__tree_delete(&dso->inlined_nodes);

Hmm.. inline_node is released after symbol but it seems to have a problem. Please see below.

Quoted 4 lines

tools/perf/util/dso.c +0 -0

 	if (dso->short_name_allocated) {
 
 		zfree((char **)&dso->short_name);

Quoted 7 lines

tools/perf/util/dso.h struct dso { · line 142

Quoted 7 lines

tools/perf/util/dso.h +0 -0

index f886141678eb..7d1e2b3c1f10 100644
@@ -141,6 +141,7 @@ struct dso {
 
 	struct rb_root	 *root;		/* root of rbtree that rb_node is in

*/

Quoted 112 lines

Quoted 9 lines

tools/perf/util/dso.h +1 -0

 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
 
+	struct rb_root	 inlined_nodes;
 

tools/perf/util/dso.h +0 -0

 	struct {
 
 		u64		addr;
 		struct symbol	*symbol;

[SNIP]

Quoted 100 lines

tools/perf/util/machine.c +0 -0

index d4df353051af..a7f8499c8756 100644

tools/perf/util/srcline.c +28 -14

index ebc88a74e67b..a1fdf035d1dd 100644
@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)
 
 	return dso_name;
 
 }
 
-static int inline_list__append(char *filename, char *funcname, int
 line_nr, -			       struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+			       struct inline_node *node)
 
 {
 
 	struct inline_list *ilist;
 
-	char *demangled;
 
 	ilist = zalloc(sizeof(*ilist));
 	if (ilist == NULL)
 
 		return -1;
 
-	ilist->filename = filename;
-	ilist->line_nr = line_nr;
-
-	if (dso != NULL) {
-		demangled = dso__demangle_sym(dso, 0, funcname);
-		if (demangled == NULL) {
-			ilist->funcname = funcname;
-		} else {
-			ilist->funcname = demangled;
-			free(funcname);
-		}
-	}
+	ilist->symbol = symbol;
+	ilist->srcline = srcline;
 
 	if (callchain_param.order == ORDER_CALLEE)
 
 		list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char
 *funcname, int line_nr,>
 
 	return 0;
 
 }
 
+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+	const char *base = strrchr(path, '/');
+
+	return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+	char *srcline;
+
+	if (!file)
+		return NULL;
+
+	if (!srcline_full_filename)
+		file = gnu_basename(file);
+
+	if (asprintf(&srcline, "%s:%u", file, line) < 0)
+		return NULL;
+
+	return srcline;
+}
+

#ifdef HAVE_LIBBFD_SUPPORT

/*

tools/perf/util/srcline.c +10 -0

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data
 *a2l)
 
 #define MAX_INLINE_NEST 1024
 
+static struct symbol *new_inline_sym(struct dso *dso,
+				     struct symbol *base_sym,
+				     const char *funcname)
+{
+	struct symbol *inline_sym;
+	char *demangled = NULL;
+
+	if (dso) {
+		demangled = dso__demangle_sym(dso, 0, funcname);
+		if (demangled)

Quoted 8 lines

tools/perf/util/srcline.c static void addr2line_cleanup(struct a2l_data *a2l) · line 234

Quoted 6 lines

tools/perf/util/srcline.c +6 -0

+			funcname = demangled;
+	}
+
+	if (strcmp(funcname, base_sym->name) == 0) {
+		// reuse the real, existing symbol
+		inline_sym = base_sym;

So inline_node could refer the existing symbol.

Yes, that does indeed seem to be an issue. I wonder why I'm not seeing any crashes or valgrind/ASAN reports though. I'll have to dig into this.

Quoted 47 lines

Quoted 38 lines

tools/perf/util/srcline.c +23 -4

+	} else {
+		// create a fake symbol for the inline frame
+		inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+					 base_sym ? base_sym->end : 0,
+					 base_sym ? base_sym->binding : 0,
+					 funcname);
+		if (inline_sym)
+			inline_sym->inlined = 1;
+	}
+
+	free(demangled);
+
+	return inline_sym;
+}
+
 
 static int inline_list__append_dso_a2l(struct dso *dso,
 
-				       struct inline_node *node)
+				       struct inline_node *node,
+				       struct symbol *sym)
 
 {
 
 	struct a2l_data *a2l = dso->a2l;
 
-	char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-	char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+	struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+	char *srcline = NULL;
+
+	if (a2l->filename)
+		srcline = srcline_from_fileline(a2l->filename, a2l->line);
 
-	return inline_list__append(filename, funcname, a2l->line, node, dso);
+	return inline_list__append(inline_sym, srcline, node);
 
 }

[SNIP]

Quoted 6 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node)
 
 	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
 		list_del_init(&ilist->list);
 

Quoted 8 lines

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-		zfree(&ilist->filename);
-		zfree(&ilist->funcname);
+		zfree(&ilist->srcline);
+		// only the inlined symbols are owned by the list
+		if (ilist->symbol && ilist->symbol->inlined)
+			symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Thanks for the review, I'll try to look into these issues once I have more time again.

Quoted 261 lines · On Sonntag, 20. August 2017 22:57:17 CEST Milian Wolff wrote:

Quoted 8 lines · On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:

Hi Milian,

Quoted 2 lines · Milian Wolff

The inlined frames use a fake symbol that is tracked in a special map inside the dso, which is always sorted by name. All other

It seems the above is not true. Fake symbols are maintained by inline_node which in turn maintained by dso->inlines tree.

OK, I'll rephrase this to make it more explicit. But what I call "map" is what you call "tree", no?

<snip>

Quoted 12 lines

Quoted 12 lines

(quoted diff) +0 -0

@@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct

callchain_cursor *cursor)>

int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip,

tools/perf/util/callchain.h +2 -1

 			    struct map *map, struct symbol *sym,
 			    bool branch, struct branch_flags *flags,
 
-			    int nr_loop_iter, int samples, u64 branch_from);
+			    int nr_loop_iter, int samples, u64 branch_from,
+			    const char *srcline);

Quoted 6 lines

Quoted 3 lines

/* Close a cursor writing session. Initialize for the reader */ static inline void callchain_cursor_commit(struct callchain_cursor *cursor)

I think it'd be better splitting srcline change into a separate commit.

OK, I'll try to see how this goes. It would simply add the boiler plate code to pass the srcline around, and then set it from within the callchain code, right?

Quoted 33 lines

Quoted 17 lines

tools/perf/util/dso.c +1 -0

index b9e087fb8247..72e6e390fd26 100644
@@ -9,6 +9,7 @@
 
 #include "compress.h"
 #include "path.h"
 #include "symbol.h"
 
+#include "srcline.h"
 

#include "dso.h"
#include "machine.h"
#include "auxtrace.h"

tools/perf/util/dso.c void dso__delete(struct dso *dso)

Quoted 8 lines

tools/perf/util/dso.c +1 -0

@@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso)
 
 		       dso->long_name);
 
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 
 		symbols__delete(&dso->symbols[i]);
 
+	inlines__tree_delete(&dso->inlined_nodes);

Hmm.. inline_node is released after symbol but it seems to have a problem. Please see below.

Quoted 4 lines

tools/perf/util/dso.c +0 -0

 	if (dso->short_name_allocated) {
 
 		zfree((char **)&dso->short_name);

Quoted 7 lines

tools/perf/util/dso.h struct dso { · line 142

Quoted 7 lines

tools/perf/util/dso.h +0 -0

index f886141678eb..7d1e2b3c1f10 100644
@@ -141,6 +141,7 @@ struct dso {
 
 	struct rb_root	 *root;		/* root of rbtree that rb_node is in

*/

Quoted 112 lines

Quoted 9 lines

tools/perf/util/dso.h +1 -0

 	struct rb_root	 symbols[MAP__NR_TYPES];
 	struct rb_root	 symbol_names[MAP__NR_TYPES];
 
+	struct rb_root	 inlined_nodes;
 

tools/perf/util/dso.h +0 -0

 	struct {
 
 		u64		addr;
 		struct symbol	*symbol;

[SNIP]

Quoted 100 lines

tools/perf/util/machine.c +0 -0

index d4df353051af..a7f8499c8756 100644

tools/perf/util/srcline.c +28 -14

index ebc88a74e67b..a1fdf035d1dd 100644
@@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso)
 
 	return dso_name;
 
 }
 
-static int inline_list__append(char *filename, char *funcname, int
 line_nr, -			       struct inline_node *node, struct dso *dso)
+static int inline_list__append(struct symbol *symbol, char *srcline,
+			       struct inline_node *node)
 
 {
 
 	struct inline_list *ilist;
 
-	char *demangled;
 
 	ilist = zalloc(sizeof(*ilist));
 	if (ilist == NULL)
 
 		return -1;
 
-	ilist->filename = filename;
-	ilist->line_nr = line_nr;
-
-	if (dso != NULL) {
-		demangled = dso__demangle_sym(dso, 0, funcname);
-		if (demangled == NULL) {
-			ilist->funcname = funcname;
-		} else {
-			ilist->funcname = demangled;
-			free(funcname);
-		}
-	}
+	ilist->symbol = symbol;
+	ilist->srcline = srcline;
 
 	if (callchain_param.order == ORDER_CALLEE)
 
 		list_add_tail(&ilist->list, &node->val);

@@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char
 *funcname, int line_nr,>
 
 	return 0;
 
 }
 
+// basename version that takes a const input string
+static const char *gnu_basename(const char *path)
+{
+	const char *base = strrchr(path, '/');
+
+	return base ? base + 1 : path;
+}
+
+static char *srcline_from_fileline(const char *file, unsigned int line)
+{
+	char *srcline;
+
+	if (!file)
+		return NULL;
+
+	if (!srcline_full_filename)
+		file = gnu_basename(file);
+
+	if (asprintf(&srcline, "%s:%u", file, line) < 0)
+		return NULL;
+
+	return srcline;
+}
+

#ifdef HAVE_LIBBFD_SUPPORT

/*

tools/perf/util/srcline.c +10 -0

@@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data
 *a2l)
 
 #define MAX_INLINE_NEST 1024
 
+static struct symbol *new_inline_sym(struct dso *dso,
+				     struct symbol *base_sym,
+				     const char *funcname)
+{
+	struct symbol *inline_sym;
+	char *demangled = NULL;
+
+	if (dso) {
+		demangled = dso__demangle_sym(dso, 0, funcname);
+		if (demangled)

Quoted 8 lines

tools/perf/util/srcline.c static void addr2line_cleanup(struct a2l_data *a2l) · line 234

Quoted 6 lines

tools/perf/util/srcline.c +6 -0

+			funcname = demangled;
+	}
+
+	if (strcmp(funcname, base_sym->name) == 0) {
+		// reuse the real, existing symbol
+		inline_sym = base_sym;

So inline_node could refer the existing symbol.

Yes, that does indeed seem to be an issue. I wonder why I'm not seeing any crashes or valgrind/ASAN reports though. I'll have to dig into this.

Quoted 47 lines

Quoted 38 lines

tools/perf/util/srcline.c +23 -4

+	} else {
+		// create a fake symbol for the inline frame
+		inline_sym = symbol__new(base_sym ? base_sym->start : 0,
+					 base_sym ? base_sym->end : 0,
+					 base_sym ? base_sym->binding : 0,
+					 funcname);
+		if (inline_sym)
+			inline_sym->inlined = 1;
+	}
+
+	free(demangled);
+
+	return inline_sym;
+}
+
 
 static int inline_list__append_dso_a2l(struct dso *dso,
 
-				       struct inline_node *node)
+				       struct inline_node *node,
+				       struct symbol *sym)
 
 {
 
 	struct a2l_data *a2l = dso->a2l;
 
-	char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL;
-	char *filename = a2l->filename ? strdup(a2l->filename) : NULL;
+	struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname);
+	char *srcline = NULL;
+
+	if (a2l->filename)
+		srcline = srcline_from_fileline(a2l->filename, a2l->line);
 
-	return inline_list__append(filename, funcname, a2l->line, node, dso);
+	return inline_list__append(inline_sym, srcline, node);
 
 }

[SNIP]

Quoted 6 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node)
 
 	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
 		list_del_init(&ilist->list);
 

Quoted 8 lines

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-		zfree(&ilist->filename);
-		zfree(&ilist->funcname);
+		zfree(&ilist->srcline);
+		// only the inlined symbols are owned by the list
+		if (ilist->symbol && ilist->symbol->inlined)
+			symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Quoted 2 lines

Thanks for the review, I'll try to look into these issues once I have more time again.

OK, so I just dug into this part of the patch again. I don't think it's actually a problem after all:

When an inline node reuses the real symbol, that symbol won't have its inlined member set to true. Thus these symbols will never get deleted by inline_node__delete.

If you have suggestions on how to make this clearer, I'm
all ears. For now, I'll add a comment to where we alias/reuse the symbol.

I'll try to split up the patch now to make it somehow easier to review.

Hi Milian,

Quoted 23 lines

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node
 *node)
 
 	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
 		list_del_init(&ilist->list);
 

Quoted 8 lines

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-		zfree(&ilist->filename);
-		zfree(&ilist->funcname);
+		zfree(&ilist->srcline);
+		// only the inlined symbols are owned by the list
+		if (ilist->symbol && ilist->symbol->inlined)
+			symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Quoted 3 lines

Thanks for the review, I'll try to look into these issues once I have more
time again.

OK, so I just dug into this part of the patch again. I don't think it's actually a problem after all:

Quoted 3 lines

When an inline node reuses the real symbol, that symbol won't have its inlined member set to true. Thus these symbols will never get deleted by inline_node__delete.

But ilist->symbol is a dangling pointer so accessing ->inlined would be a problem, no?

Quoted 4 lines

If you have suggestions on how to make this clearer, I'm
all ears. For now, I'll add a comment to where we alias/reuse the symbol.

I'll try to split up the patch now to make it somehow easier to review.

Thanks for doing this, but I'm afraid I don't have time to review before going to OSS-NA.

Quoted 1 line · CEST Namhyung Kim

Hi Milian,

Hey Namhyung!

Quoted 27 lines

Quoted 26 lines

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

tools/perf/util/srcline.c +0 -0

@@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node
 *node)
 
      list_for_each_entry_safe(ilist, tmp, &node->val, list) {
 
              list_del_init(&ilist->list);
 

Quoted 8 lines

tools/perf/util/srcline.c void inline_node__delete(struct inline_node *node) · line 569

Quoted 6 lines

tools/perf/util/srcline.c +4 -2

-             zfree(&ilist->filename);
-             zfree(&ilist->funcname);
+             zfree(&ilist->srcline);
+             // only the inlined symbols are owned by the list
+             if (ilist->symbol && ilist->symbol->inlined)
+                     symbol__delete(ilist->symbol);

Existing symbols are released at this moment.

Quoted 3 lines

Thanks for the review, I'll try to look into these issues once I have more
time again.

OK, so I just dug into this part of the patch again. I don't think it's actually a problem after all:

When an inline node reuses the real symbol, that symbol won't have its inlined member set to true. Thus these symbols will never get deleted by inline_node__delete.

Quoted 2 lines

But ilist->symbol is a dangling pointer so accessing ->inlined would be a problem, no?

Sorry, but I can't follow. Why would it be a dangling pointer? Note, again, that I've tested this with both valgrind and ASAN and neither reports any issues about this code.