[PATCH v1 0/4] perf report: Show inline stack

[PATCH v1 0/4] perf report: Show inline stack

[Jin Yao]

It would be useful for perf to support a mode to query the
inline stack for callgraph addresses. This would simplify
finding the right code in code that does a lot of inlining.

For example, the c code:

static inline void f3(void)
{
        int i;
        for (i = 0; i < 1000;) {

                if(i%2)
                        i++;
                else
                        i++;
        }
        printf("hello f3\n");   /* D */
}

/* < CALLCHAIN: f2 <- f1 > */
static inline void f2(void)
{
        int i;
        for (i = 0; i < 100; i++) {
                f3();   /* C */
        }
}

/* < CALLCHAIN: f1 <- main > */
static inline void f1(void)
{
        int i;
        for (i = 0; i < 100; i++) {
                f2();   /* B */
        }
}

/* < CALLCHAIN: main <- TOP > */
int main()
{
        struct timeval tv;
        time_t start, end;

        gettimeofday(&tv, NULL);
        start = end = tv.tv_sec;
        while((end - start) < 5) {
                f1();   /* A */
                gettimeofday(&tv, NULL);
                end = tv.tv_sec;
        }
        return 0;
}

The printed inline stack is:

0.05%  test2    test2              [.] main
       |
       ---/home/perf-dev/lck-2867/test/test2.c:27 (inline)
          /home/perf-dev/lck-2867/test/test2.c:35 (inline)
          /home/perf-dev/lck-2867/test/test2.c:45 (inline)
          /home/perf-dev/lck-2867/test/test2.c:61 (inline)

I tag A/B/C/D in above c code to indicate the source line,
actually the inline stack is equal to:

0.05%  test2    test2              [.] main
       |
       ---D
          C
          B
          A

Jin Yao (4):
  perf report: Find the inline stack for a given address
  perf report: Create a new option "--inline"
  perf report: Show inline stack in stdio mode
  perf report: Show inline stack in browser mode

 tools/perf/Documentation/perf-report.txt |   4 +
 tools/perf/builtin-report.c              |   2 +
 tools/perf/ui/browsers/hists.c           |  98 ++++++++++++++-
 tools/perf/ui/stdio/hist.c               |  56 ++++++++-
 tools/perf/util/hist.c                   |   5 +
 tools/perf/util/sort.h                   |   1 +
 tools/perf/util/srcline.c                | 206 ++++++++++++++++++++++++++-----
 tools/perf/util/symbol.h                 |   3 +-
 tools/perf/util/util.h                   |  15 +++
 9 files changed, 356 insertions(+), 34 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] perf report: Find the inline stack for a given address

[Jin Yao]

It would be useful for perf to support a mode to query the
inline stack for a given callgraph address. This would simplify
finding the right code in code that does a lot of inlining.

The srcline.c has contained the code which supports to translate
the address to filename:line_nr. This patch just extends the
function to let it support getting the inline stacks.

The results (filename:line_nr) would be saved in a list and
returned to the caller.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/srcline.c | 206 +++++++++++++++++++++++++++++++++++++++-------
 tools/perf/util/util.h    |  15 ++++
 2 files changed, 193 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index b4db3f4..0145625 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -12,6 +12,39 @@
 
 bool srcline_full_filename;
 
+static const char *dso_name_get(struct dso *dso)
+{
+	const char *dso_name;
+
+	if (dso->symsrc_filename)
+		dso_name = dso->symsrc_filename;
+	else
+		dso_name = dso->long_name;
+
+	if (dso_name[0] == '[')
+		return NULL;
+
+	if (!strncmp(dso_name, "/tmp/perf-", 10))
+		return NULL;
+
+	return dso_name;
+}
+
+static int ilist_apend(char *filename, int line_nr, struct inline_node *node)
+{
+	struct inline_list *ilist;
+
+	ilist = zalloc(sizeof(*ilist));
+	if (ilist == NULL)
+		return -1;
+
+	ilist->filename = filename;
+	ilist->line_nr = line_nr;
+	list_add_tail(&ilist->list, &node->val);
+
+	return 0;
+}
+
 #ifdef HAVE_LIBBFD_SUPPORT
 
 /*
@@ -153,7 +186,7 @@ static void addr2line_cleanup(struct a2l_data *a2l)
 
 static int addr2line(const char *dso_name, u64 addr,
 		     char **file, unsigned int *line, struct dso *dso,
-		     bool unwind_inlines)
+		     bool unwind_inlines, struct inline_node *node)
 {
 	int ret = 0;
 	struct a2l_data *a2l = dso->a2l;
@@ -178,8 +211,14 @@ static int addr2line(const char *dso_name, u64 addr,
 
 		while (bfd_find_inliner_info(a2l->abfd, &a2l->filename,
 					     &a2l->funcname, &a2l->line) &&
-		       cnt++ < MAX_INLINE_NEST)
-			;
+		       cnt++ < MAX_INLINE_NEST) {
+
+			if (node != NULL) {
+				if (ilist_apend(strdup(a2l->filename),
+						a2l->line, node) != 0)
+					return 0;
+			}
+		}
 	}
 
 	if (a2l->found && a2l->filename) {
@@ -205,18 +244,68 @@ void dso__free_a2l(struct dso *dso)
 	dso->a2l = NULL;
 }
 
+static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
+	struct dso *dso)
+{
+	char *file = NULL;
+	unsigned int line = 0;
+	struct inline_node *node;
+
+	node = zalloc(sizeof(*node));
+	if (node == NULL) {
+		perror("not enough memory for the inline node");
+		return NULL;
+	}
+
+	INIT_LIST_HEAD(&node->val);
+	node->addr = addr;
+
+	if (!addr2line(dso_name, addr, &file, &line, dso, TRUE, node)) {
+		free_inline_node(node);
+		return NULL;
+	}
+
+	if (list_empty(&node->val)) {
+		free_inline_node(node);
+		return NULL;
+	}
+
+	return node;
+}
+
 #else /* HAVE_LIBBFD_SUPPORT */
 
+static int filename_split(const char *dso_name, char *filename,
+			  unsigned int *line_nr)
+{
+	char *sep;
+
+	sep = strchr(filename, '\n');
+	if (sep)
+		*sep = '\0';
+
+	if (!strcmp(filename, "??:0"))
+		return -1;
+
+	sep = strchr(filename, ':');
+	if (sep) {
+		*sep++ = '\0';
+		*line_nr = strtoul(sep, NULL, 0);
+	}
+
+	return 0;
+}
+
 static int addr2line(const char *dso_name, u64 addr,
 		     char **file, unsigned int *line_nr,
 		     struct dso *dso __maybe_unused,
-		     bool unwind_inlines __maybe_unused)
+		     bool unwind_inlines __maybe_unused,
+		     struct inline_node *node __maybe_unused)
 {
 	FILE *fp;
 	char cmd[PATH_MAX];
 	char *filename = NULL;
 	size_t len;
-	char *sep;
 	int ret = 0;
 
 	scnprintf(cmd, sizeof(cmd), "addr2line -e %s %016"PRIx64,
@@ -233,23 +322,14 @@ static int addr2line(const char *dso_name, u64 addr,
 		goto out;
 	}
 
-	sep = strchr(filename, '\n');
-	if (sep)
-		*sep = '\0';
-
-	if (!strcmp(filename, "??:0")) {
-		pr_debug("no debugging info in %s\n", dso_name);
+	if (filename_split(dso_name, filename, line_nr) != 0) {
 		free(filename);
 		goto out;
 	}
 
-	sep = strchr(filename, ':');
-	if (sep) {
-		*sep++ = '\0';
-		*file = filename;
-		*line_nr = strtoul(sep, NULL, 0);
-		ret = 1;
-	}
+	*file = filename;
+	ret = 1;
+
 out:
 	pclose(fp);
 	return ret;
@@ -259,6 +339,57 @@ void dso__free_a2l(struct dso *dso __maybe_unused)
 {
 }
 
+static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
+	struct dso *dso __maybe_unused)
+{
+	FILE *fp;
+	char cmd[PATH_MAX];
+	struct inline_node *node;
+	char *filename = NULL;
+	size_t len;
+	unsigned int line_nr = 0;
+
+	scnprintf(cmd, sizeof(cmd), "addr2line -e %s -i %016"PRIx64,
+		  dso_name, addr);
+
+	fp = popen(cmd, "r");
+	if (fp == NULL) {
+		pr_warn("popen failed for %s\n", dso_name);
+		return NULL;
+	}
+
+	node = zalloc(sizeof(*node));
+	if (node == NULL) {
+		perror("not enough memory for the inline node");
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&node->val);
+	node->addr = addr;
+
+	while (getline(&filename, &len, fp) != -1) {
+		if (filename_split(dso_name, filename, &line_nr) != 0) {
+			free(filename);
+			goto out;
+		}
+
+		if (ilist_apend(filename, line_nr, node) != 0)
+			goto out;
+
+		filename = NULL;
+	}
+
+out:
+	pclose(fp);
+
+	if (list_empty(&node->val)) {
+		free_inline_node(node);
+		return NULL;
+	}
+
+	return node;
+}
+
 #endif /* HAVE_LIBBFD_SUPPORT */
 
 /*
@@ -278,18 +409,11 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 	if (!dso->has_srcline)
 		goto out;
 
-	if (dso->symsrc_filename)
-		dso_name = dso->symsrc_filename;
-	else
-		dso_name = dso->long_name;
-
-	if (dso_name[0] == '[')
-		goto out;
-
-	if (!strncmp(dso_name, "/tmp/perf-", 10))
+	dso_name = dso_name_get(dso);
+	if (dso_name == NULL)
 		goto out;
 
-	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines))
+	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines, NULL))
 		goto out;
 
 	if (asprintf(&srcline, "%s:%u",
@@ -329,3 +453,29 @@ char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 {
 	return __get_srcline(dso, addr, sym, show_sym, false);
 }
+
+struct inline_node *get_inline_node(struct dso *dso, u64 addr)
+{
+	const char *dso_name;
+
+	dso_name = dso_name_get(dso);
+	if (dso_name == NULL)
+		return NULL;
+
+	return addr2inlines(dso_name, addr, dso);
+}
+
+void free_inline_node(struct inline_node *node)
+{
+	struct inline_list *ilist, *tmp;
+
+	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
+		list_del(&ilist->list);
+		if (ilist->filename != NULL)
+			free(ilist->filename);
+
+		free(ilist);
+	}
+
+	free(node);
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 79662d6..febe8d7 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -78,6 +78,7 @@
 #include <termios.h>
 #include <linux/bitops.h>
 #include <termios.h>
+#include <linux/list.h>
 #include "strlist.h"
 
 extern const char *graph_line;
@@ -365,4 +366,18 @@ int is_printable_array(char *p, unsigned int len);
 
 int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz);
 
+struct inline_list {
+	char			*filename;
+	unsigned int		line_nr;
+	struct list_head	list;
+};
+
+struct inline_node {
+	u64			addr;
+	struct list_head	val;
+};
+
+struct inline_node *get_inline_node(struct dso *dso, u64 addr);
+void free_inline_node(struct inline_node *node);
+
 #endif /* GIT_COMPAT_UTIL_H */
-- 
2.7.4

[PATCH v1 2/4] perf report: Create a new option "--inline"

[Jin Yao]

It takes some time to look for inline stack for callgraph addresses.
So it provides a new option "--inline" to let user decide if enable
this feature.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Documentation/perf-report.txt | 4 ++++
 tools/perf/builtin-report.c              | 2 ++
 tools/perf/util/symbol.h                 | 3 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 2d17462..f1299a7 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -412,6 +412,10 @@ include::itrace.txt[]
 --hierarchy::
 	Enable hierarchical output.
 
+--inline::
+	If a callgraph address belongs to an inlined function, the inline stack
+	will be printed.
+
 include::callchain-overhead-calculation.txt[]
 
 SEE ALSO
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 3dfbfff..ba2f627 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -830,6 +830,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_CALLBACK_DEFAULT(0, "stdio-color", NULL, "mode",
 			     "'always' (default), 'never' or 'auto' only applicable to --stdio mode",
 			     stdio__config_color, "always"),
+	OPT_BOOLEAN(0, "inline", &symbol_conf.show_inline,
+		    "Show inline functions"),
 	OPT_END()
 	};
 	struct perf_data_file file = {
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 1bcbefc..c312759 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -118,7 +118,8 @@ struct symbol_conf {
 			show_ref_callgraph,
 			hide_unresolved,
 			raw_trace,
-			report_hierarchy;
+			report_hierarchy,
+			show_inline;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
2.7.4

[PATCH v1 3/4] perf report: Show inline stack in stdio mode

[Jin Yao]

If the address belongs to an inlined function, the source information
back to the first non-inlined function will be printed.

For example:

0.05%  test2    test2              [.] main
       |
       ---/home/jinyao/perf-dev/test/test2.c:27 (inline)
          /home/jinyao/perf-dev/test/test2.c:35 (inline)
          /home/jinyao/perf-dev/test/test2.c:45 (inline)
          /home/jinyao/perf-dev/test/test2.c:61 (inline)

The tag "inline" indicates these items are the entries in inline stack.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/stdio/hist.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 668f4ae..e74eda0 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -400,6 +400,53 @@ static size_t hist_entry_callchain__fprintf(struct hist_entry *he,
 	return 0;
 }
 
+static size_t hist_entry_inline__fprintf(struct hist_entry *he,
+					 int left_margin,
+					 FILE *fp)
+{
+	struct dso *dso;
+	struct inline_node *node;
+	struct inline_list *ilist;
+	int ret = 0, i = 0;
+
+	if (he->ms.map == NULL)
+		return 0;
+
+	dso = he->ms.map->dso;
+	if (dso == NULL)
+		return 0;
+
+	if (dso->kernel != DSO_TYPE_USER)
+		return 0;
+
+	node = get_inline_node(dso, map__rip_2objdump(he->ms.map, he->ip));
+	if (node == NULL)
+		return 0;
+
+	ret += callchain__fprintf_left_margin(fp, left_margin);
+	ret += fprintf(fp, "|\n");
+	ret += callchain__fprintf_left_margin(fp, left_margin);
+	ret += fprintf(fp, "---");
+	left_margin += 3;
+
+	list_for_each_entry(ilist, &node->val, list) {
+		if (ilist->filename != NULL) {
+			if (i++ > 0)
+				ret = callchain__fprintf_left_margin(fp,
+								left_margin);
+			ret += fprintf(fp, "%s:%d (inline)",
+				       ilist->filename, ilist->line_nr);
+			ret += fprintf(fp, "\n");
+		}
+	}
+
+	if (i > 0)
+		ret += fprintf(fp, "\n");
+
+	free_inline_node(node);
+	return ret;
+}
+
 int __hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp,
 			   struct perf_hpp_list *hpp_list)
 {
@@ -529,6 +576,7 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 			       bool use_callchain)
 {
 	int ret;
+	int callchain_ret = 0;
 	struct perf_hpp hpp = {
 		.buf		= bf,
 		.size		= size,
@@ -547,7 +595,13 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 	ret = fprintf(fp, "%s\n", bf);
 
 	if (use_callchain)
-		ret += hist_entry_callchain__fprintf(he, total_period, 0, fp);
+		callchain_ret = hist_entry_callchain__fprintf(he, total_period,
+							      0, fp);
+
+	if ((callchain_ret == 0) && symbol_conf.show_inline)
+		ret += hist_entry_inline__fprintf(he, 0, fp);
+	else
+		ret += callchain_ret;
 
 	return ret;
 }
-- 
2.7.4

[PATCH v1 4/4] perf report: Show inline stack in browser mode

[Jin Yao]

For example:

-    0.05%  test2    test2              [.] main
     /home/jinyao/perf-dev/test/test2.c:27 (inline)
     /home/jinyao/perf-dev/test/test2.c:35 (inline)
     /home/jinyao/perf-dev/test/test2.c:45 (inline)
     /home/jinyao/perf-dev/test/test2.c:61 (inline)

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/browsers/hists.c | 98 ++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/hist.c         |  5 +++
 tools/perf/util/sort.h         |  1 +
 3 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 641b402..c8c3cae 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -362,6 +362,46 @@ 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)
+{
+	struct dso *dso;
+
+	if (he->inline_node)
+		return;
+
+	if (he->ms.map == NULL)
+		return;
+
+	dso = he->ms.map->dso;
+	if (dso == NULL)
+		return;
+
+	if (dso->kernel != DSO_TYPE_USER)
+		return;
+
+	he->inline_node = get_inline_node(dso,
+		map__rip_2objdump(he->ms.map, he->ip));
+
+	if (he->inline_node == NULL)
+		return;
+
+	he->has_children = true;
+}
+
+static int inline__count_rows(struct hist_entry *he)
+{
+	struct inline_node *node = he->inline_node;
+	struct inline_list *ilist;
+	int i = 0;
+
+	list_for_each_entry(ilist, &node->val, list) {
+		if (ilist->filename != NULL)
+			i++;
+	}
+
+	return i;
+}
+
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
 	struct hist_entry *he = browser->he_selection;
@@ -393,7 +433,11 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 
 		if (he->unfolded) {
 			if (he->leaf)
-				he->nr_rows = callchain__count_rows(&he->sorted_chain);
+				if (he->inline_node)
+					he->nr_rows = inline__count_rows(he);
+				else
+					he->nr_rows = callchain__count_rows(
+						&he->sorted_chain);
 			else
 				he->nr_rows = hierarchy_count_rows(browser, he, false);
 
@@ -1091,6 +1135,40 @@ static int hist_browser__show_callchain(struct hist_browser *browser,
 	return printed;
 }
 
+static int hist_browser__show_inline(struct hist_browser *browser,
+				     struct hist_entry *entry,
+				     unsigned short row)
+{
+	struct inline_node *node;
+	struct inline_list *ilist;
+	char buf[1024];
+	int color, width, first_row;
+
+	first_row = row;
+	node = entry->inline_node;
+	width = browser->b.width - (LEVEL_OFFSET_STEP + 2);
+
+	list_for_each_entry(ilist, &node->val, list) {
+		if (ilist->filename != NULL) {
+			color = HE_COLORSET_NORMAL;
+			if (ui_browser__is_current_entry(&browser->b, row))
+				color = HE_COLORSET_SELECTED;
+
+			scnprintf(buf, sizeof(buf), "%s:%d (inline)",
+				ilist->filename, ilist->line_nr);
+
+			ui_browser__set_color(&browser->b, color);
+			hist_browser__gotorc(browser, row, 0);
+			ui_browser__write_nstring(&browser->b, " ",
+				LEVEL_OFFSET_STEP + 2);
+			ui_browser__write_nstring(&browser->b, buf, width);
+			row++;
+		}
+	}
+
+	return row - first_row;
+}
+
 struct hpp_arg {
 	struct ui_browser *b;
 	char folded_sign;
@@ -1204,6 +1282,11 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		folded_sign = hist_entry__folded(entry);
 	}
 
+	if (symbol_conf.show_inline && (!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,
@@ -1235,7 +1318,8 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 			}
 
 			if (first) {
-				if (symbol_conf.use_callchain) {
+				if (symbol_conf.use_callchain ||
+					symbol_conf.show_inline) {
 					ui_browser__printf(&browser->b, "%c ", folded_sign);
 					width -= 2;
 				}
@@ -1277,8 +1361,14 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 			.is_current_entry = current_entry,
 		};
 
-		printed += hist_browser__show_callchain(browser, entry, 1, row,
-					hist_browser__show_callchain_entry, &arg,
+		if (entry->inline_node)
+			printed += hist_browser__show_inline(browser,
+					entry, row);
+		else
+			printed += hist_browser__show_callchain(browser,
+					entry, 1, row,
+					hist_browser__show_callchain_entry,
+					&arg,
 					hist_browser__check_output_full);
 	}
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6770a96..c18af42 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1122,6 +1122,11 @@ void hist_entry__delete(struct hist_entry *he)
 		zfree(&he->mem_info);
 	}
 
+	if (he->inline_node) {
+		free_inline_node(he->inline_node);
+		he->inline_node = NULL;
+	}
+
 	zfree(&he->stat_acc);
 	free_srcline(he->srcline);
 	if (he->srcfile && he->srcfile[0])
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 7aff317..5dbfdc7 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -122,6 +122,7 @@ struct hist_entry {
 	};
 	char			*srcline;
 	char			*srcfile;
+	struct inline_node	*inline_node;
 	struct symbol		*parent;
 	struct branch_info	*branch_info;
 	struct hists		*hists;
-- 
2.7.4

Re: [PATCH v1 0/4] perf report: Show inline stack

[Jin, Yao]

Any comments on this patch series?

Thanks

Jin Yao


On 11/29/2016 10:55 PM, Jin Yao wrote:
> It would be useful for perf to support a mode to query the
> inline stack for callgraph addresses. This would simplify
> finding the right code in code that does a lot of inlining.
>
> For example, the c code:
>
> static inline void f3(void)
> {
>          int i;
>          for (i = 0; i < 1000;) {
>
>                  if(i%2)
>                          i++;
>                  else
>                          i++;
>          }
>          printf("hello f3\n");   /* D */
> }
>
> /* < CALLCHAIN: f2 <- f1 > */
> static inline void f2(void)
> {
>          int i;
>          for (i = 0; i < 100; i++) {
>                  f3();   /* C */
>          }
> }
>
> /* < CALLCHAIN: f1 <- main > */
> static inline void f1(void)
> {
>          int i;
>          for (i = 0; i < 100; i++) {
>                  f2();   /* B */
>          }
> }
>
> /* < CALLCHAIN: main <- TOP > */
> int main()
> {
>          struct timeval tv;
>          time_t start, end;
>
>          gettimeofday(&tv, NULL);
>          start = end = tv.tv_sec;
>          while((end - start) < 5) {
>                  f1();   /* A */
>                  gettimeofday(&tv, NULL);
>                  end = tv.tv_sec;
>          }
>          return 0;
> }
>
> The printed inline stack is:
>
> 0.05%  test2    test2              [.] main
>         |
>         ---/home/perf-dev/lck-2867/test/test2.c:27 (inline)
>            /home/perf-dev/lck-2867/test/test2.c:35 (inline)
>            /home/perf-dev/lck-2867/test/test2.c:45 (inline)
>            /home/perf-dev/lck-2867/test/test2.c:61 (inline)
>
> I tag A/B/C/D in above c code to indicate the source line,
> actually the inline stack is equal to:
>
> 0.05%  test2    test2              [.] main
>         |
>         ---D
>            C
>            B
>            A
>
> Jin Yao (4):
>    perf report: Find the inline stack for a given address
>    perf report: Create a new option "--inline"
>    perf report: Show inline stack in stdio mode
>    perf report: Show inline stack in browser mode
>
>   tools/perf/Documentation/perf-report.txt |   4 +
>   tools/perf/builtin-report.c              |   2 +
>   tools/perf/ui/browsers/hists.c           |  98 ++++++++++++++-
>   tools/perf/ui/stdio/hist.c               |  56 ++++++++-
>   tools/perf/util/hist.c                   |   5 +
>   tools/perf/util/sort.h                   |   1 +
>   tools/perf/util/srcline.c                | 206 ++++++++++++++++++++++++++-----
>   tools/perf/util/symbol.h                 |   3 +-
>   tools/perf/util/util.h                   |  15 +++
>   9 files changed, 356 insertions(+), 34 deletions(-)
>

Re: [PATCH v1 1/4] perf report: Find the inline stack for a given address

[Arnaldo Carvalho de Melo]

Em Tue, Nov 29, 2016 at 10:55:41PM +0800, Jin Yao escreveu:
> It would be useful for perf to support a mode to query the
> inline stack for a given callgraph address. This would simplify
> finding the right code in code that does a lot of inlining.
> 
> The srcline.c has contained the code which supports to translate
> the address to filename:line_nr. This patch just extends the
> function to let it support getting the inline stacks.
> 
> The results (filename:line_nr) would be saved in a list and
> returned to the caller.

You're doing multiple things in this changeset, which makes reviewing
harder than it could be, so please consider breaking it in at least:

1.  introduce dso_name_get() out of existing code and use it, renaming
it to dso__name(), as "_get()" is usually reserved for refcount
operations (see dso__get(), thread__get(), etc, for instance), just like
with the kernel sources.

2. introduce the inline_list thing and name the functions handling it
using that prefix, and as well separate the method name from the struct
name using __, as done elsewhere in tools/perf/, i.e. that function:

  ilist_apend()

should be renamed to:

  inline_list__append()

Better, follow the list_head model and instead call it:

  inline_list__add_tail()

But as it also allocates space for the node, maybe inline_list__append()
is appropriate :-\

- Arnaldo
 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/srcline.c | 206 +++++++++++++++++++++++++++++++++++++++-------
>  tools/perf/util/util.h    |  15 ++++
>  2 files changed, 193 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index b4db3f4..0145625 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -12,6 +12,39 @@
>  
>  bool srcline_full_filename;
>  
> +static const char *dso_name_get(struct dso *dso)
> +{
> +	const char *dso_name;
> +
> +	if (dso->symsrc_filename)
> +		dso_name = dso->symsrc_filename;
> +	else
> +		dso_name = dso->long_name;
> +
> +	if (dso_name[0] == '[')
> +		return NULL;
> +
> +	if (!strncmp(dso_name, "/tmp/perf-", 10))
> +		return NULL;
> +
> +	return dso_name;
> +}
> +
> +static int ilist_apend(char *filename, int line_nr, struct inline_node *node)
> +{
> +	struct inline_list *ilist;
> +
> +	ilist = zalloc(sizeof(*ilist));
> +	if (ilist == NULL)
> +		return -1;
> +
> +	ilist->filename = filename;
> +	ilist->line_nr = line_nr;
> +	list_add_tail(&ilist->list, &node->val);
> +
> +	return 0;
> +}
> +
>  #ifdef HAVE_LIBBFD_SUPPORT
>  
>  /*
> @@ -153,7 +186,7 @@ static void addr2line_cleanup(struct a2l_data *a2l)
>  
>  static int addr2line(const char *dso_name, u64 addr,
>  		     char **file, unsigned int *line, struct dso *dso,
> -		     bool unwind_inlines)
> +		     bool unwind_inlines, struct inline_node *node)
>  {
>  	int ret = 0;
>  	struct a2l_data *a2l = dso->a2l;
> @@ -178,8 +211,14 @@ static int addr2line(const char *dso_name, u64 addr,
>  
>  		while (bfd_find_inliner_info(a2l->abfd, &a2l->filename,
>  					     &a2l->funcname, &a2l->line) &&
> -		       cnt++ < MAX_INLINE_NEST)
> -			;
> +		       cnt++ < MAX_INLINE_NEST) {
> +
> +			if (node != NULL) {
> +				if (ilist_apend(strdup(a2l->filename),
> +						a2l->line, node) != 0)
> +					return 0;
> +			}
> +		}
>  	}
>  
>  	if (a2l->found && a2l->filename) {
> @@ -205,18 +244,68 @@ void dso__free_a2l(struct dso *dso)
>  	dso->a2l = NULL;
>  }
>  
> +static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
> +	struct dso *dso)
> +{
> +	char *file = NULL;
> +	unsigned int line = 0;
> +	struct inline_node *node;
> +
> +	node = zalloc(sizeof(*node));
> +	if (node == NULL) {
> +		perror("not enough memory for the inline node");
> +		return NULL;
> +	}
> +
> +	INIT_LIST_HEAD(&node->val);
> +	node->addr = addr;
> +
> +	if (!addr2line(dso_name, addr, &file, &line, dso, TRUE, node)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	if (list_empty(&node->val)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	return node;

Perhaps have:

out_free_inline_node:
	inline_node__delete(node);
	return NULL;

and jump to here in those two cases above? That after renaming
free_inline_node(node)  to inline_node__delete(node)  to follow
convention used in most parts of tools/perf/.

> +}
> +
>  #else /* HAVE_LIBBFD_SUPPORT */
>  
> +static int filename_split(const char *dso_name, char *filename,
> +			  unsigned int *line_nr)
> +{
> +	char *sep;
> +
> +	sep = strchr(filename, '\n');
> +	if (sep)
> +		*sep = '\0';
> +
> +	if (!strcmp(filename, "??:0"))
> +		return -1;
> +
> +	sep = strchr(filename, ':');
> +	if (sep) {
> +		*sep++ = '\0';
> +		*line_nr = strtoul(sep, NULL, 0);
> +	}
> +
> +	return 0;
> +}
> +
>  static int addr2line(const char *dso_name, u64 addr,
>  		     char **file, unsigned int *line_nr,
>  		     struct dso *dso __maybe_unused,
> -		     bool unwind_inlines __maybe_unused)
> +		     bool unwind_inlines __maybe_unused,
> +		     struct inline_node *node __maybe_unused)
>  {
>  	FILE *fp;
>  	char cmd[PATH_MAX];
>  	char *filename = NULL;
>  	size_t len;
> -	char *sep;
>  	int ret = 0;
>  
>  	scnprintf(cmd, sizeof(cmd), "addr2line -e %s %016"PRIx64,
> @@ -233,23 +322,14 @@ static int addr2line(const char *dso_name, u64 addr,
>  		goto out;
>  	}
>  
> -	sep = strchr(filename, '\n');
> -	if (sep)
> -		*sep = '\0';
> -
> -	if (!strcmp(filename, "??:0")) {
> -		pr_debug("no debugging info in %s\n", dso_name);
> +	if (filename_split(dso_name, filename, line_nr) != 0) {
>  		free(filename);
>  		goto out;
>  	}
>  
> -	sep = strchr(filename, ':');
> -	if (sep) {
> -		*sep++ = '\0';
> -		*file = filename;
> -		*line_nr = strtoul(sep, NULL, 0);
> -		ret = 1;
> -	}
> +	*file = filename;
> +	ret = 1;
> +
>  out:
>  	pclose(fp);
>  	return ret;
> @@ -259,6 +339,57 @@ void dso__free_a2l(struct dso *dso __maybe_unused)
>  {
>  }
>  
> +static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
> +	struct dso *dso __maybe_unused)
> +{
> +	FILE *fp;
> +	char cmd[PATH_MAX];
> +	struct inline_node *node;
> +	char *filename = NULL;
> +	size_t len;
> +	unsigned int line_nr = 0;
> +
> +	scnprintf(cmd, sizeof(cmd), "addr2line -e %s -i %016"PRIx64,
> +		  dso_name, addr);
> +
> +	fp = popen(cmd, "r");
> +	if (fp == NULL) {
> +		pr_warn("popen failed for %s\n", dso_name);
> +		return NULL;
> +	}
> +
> +	node = zalloc(sizeof(*node));
> +	if (node == NULL) {
> +		perror("not enough memory for the inline node");
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&node->val);
> +	node->addr = addr;
> +
> +	while (getline(&filename, &len, fp) != -1) {
> +		if (filename_split(dso_name, filename, &line_nr) != 0) {
> +			free(filename);
> +			goto out;
> +		}
> +
> +		if (ilist_apend(filename, line_nr, node) != 0)
> +			goto out;
> +
> +		filename = NULL;
> +	}
> +
> +out:
> +	pclose(fp);
> +
> +	if (list_empty(&node->val)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	return node;
> +}
> +
>  #endif /* HAVE_LIBBFD_SUPPORT */
>  
>  /*
> @@ -278,18 +409,11 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  	if (!dso->has_srcline)
>  		goto out;
>  
> -	if (dso->symsrc_filename)
> -		dso_name = dso->symsrc_filename;
> -	else
> -		dso_name = dso->long_name;
> -
> -	if (dso_name[0] == '[')
> -		goto out;
> -
> -	if (!strncmp(dso_name, "/tmp/perf-", 10))
> +	dso_name = dso_name_get(dso);
> +	if (dso_name == NULL)
>  		goto out;
>  
> -	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines))
> +	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines, NULL))
>  		goto out;
>  
>  	if (asprintf(&srcline, "%s:%u",
> @@ -329,3 +453,29 @@ char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  {
>  	return __get_srcline(dso, addr, sym, show_sym, false);
>  }
> +
> +struct inline_node *get_inline_node(struct dso *dso, u64 addr)
> +{

Please rename this to dso__parse_addr_inlines().

> +	const char *dso_name;
> +
> +	dso_name = dso_name_get(dso);
> +	if (dso_name == NULL)
> +		return NULL;
> +
> +	return addr2inlines(dso_name, addr, dso);
> +}
> +
> +void free_inline_node(struct inline_node *node)

void inline_node__delete(struct inline_node *node)

> +{
> +	struct inline_list *ilist, *tmp;
> +
> +	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
> +		list_del(&ilist->list);

list_del_init()

> +		if (ilist->filename != NULL)
> +			free(ilist->filename);
> +
> +		free(ilist);
> +	}
> +
> +	free(node);
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 79662d6..febe8d7 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -78,6 +78,7 @@
>  #include <termios.h>
>  #include <linux/bitops.h>
>  #include <termios.h>
> +#include <linux/list.h>
>  #include "strlist.h"
>  
>  extern const char *graph_line;
> @@ -365,4 +366,18 @@ int is_printable_array(char *p, unsigned int len);
>  
>  int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz);
>  
> +struct inline_list {
> +	char			*filename;
> +	unsigned int		line_nr;
> +	struct list_head	list;
> +};
> +
> +struct inline_node {
> +	u64			addr;
> +	struct list_head	val;
> +};
> +
> +struct inline_node *get_inline_node(struct dso *dso, u64 addr);
> +void free_inline_node(struct inline_node *node);
> +
>  #endif /* GIT_COMPAT_UTIL_H */
> -- 
> 2.7.4