[PATCH 0/9] perf tools: Enhance and correct srcline behavior

[PATCH 0/9] perf tools: Enhance and correct srcline behavior

[Namhyung Kim]

Hello,

This patchset tries to fix and enhance current srcline behavior.

Firstly it doesn't actually sort by srcline info but by ip.  I suspect
it was because of a performance reason to run external addr2line
utility.  It showed the srcline info after hist entries were
collapsed.  Thanks to Roberto, we now have internal implementation of
addr2line using libbfd so can sort/compare by srcline of entries.

Second problem for me was it sometimes printed "??:0" and sometimes
printed a raw ip value for unknown srcline info.  I changed to print
the former consistently.

While at it, I found some bugs/leaks in srcline handling.  Patch 1-3
are fixes for those and can be merged separately.

You can get this series on my 'perf/srcline-v1' branch in my tree at:

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

Any comments are welcome, thanks.
Namhyung


Namhyung Kim (8):
  perf sort: Fix a memory leak on srcline
  perf annotate: Reuse path from the result of addr2line
  perf hists: Free srcline when freeing hist_entry
  perf tools: Factor out get/free_srcline()
  perf tools: Do not try to call addr2line for non-binary files
  perf tools: Pass dso instead of dso_name to get_srcline()
  perf tools: Save failed result of get_srcline()
  perf tools: Fix srcline sort key behavior

Roberto Vitillo (1):
  perf tools: Implement addr2line directly using libbfd

 tools/perf/Makefile        |   1 +
 tools/perf/config/Makefile |   4 +
 tools/perf/util/annotate.c |  33 +-----
 tools/perf/util/dso.c      |   1 +
 tools/perf/util/dso.h      |   2 +
 tools/perf/util/hist.c     |   1 +
 tools/perf/util/sort.c     |  58 ++++------
 tools/perf/util/srcline.c  | 265 +++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/util.h     |   8 ++
 9 files changed, 308 insertions(+), 65 deletions(-)
 create mode 100644 tools/perf/util/srcline.c

-- 
1.7.11.7

[PATCH 1/9] perf sort: Fix a memory leak on srcline

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

In the hist_entry__srcline_snprintf(), path and self->srcline are
pointing the same memory region, but they are doubly allocated.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5f118a089519..5ba29fc679c5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -269,10 +269,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 	if (!fp)
 		goto out_ip;
 
-	if (getline(&path, &line_len, fp) < 0 || !line_len)
-		goto out_ip;
-	self->srcline = strdup(path);
-	if (self->srcline == NULL)
+	if (getline(&self->srcline, &line_len, fp) < 0 || !line_len)
 		goto out_ip;
 
 	nl = strchr(self->srcline, '\n');
-- 
1.7.11.7

[PATCH 2/9] perf annotate: Reuse path from the result of addr2line

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

In the symbol__get_source_line(), path and src_line->path will have
same value, but they were allocated separately, and leaks one.
Just share path to src_line->path.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index d102716c43a1..8a5babc71d53 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1091,11 +1091,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 		if (getline(&path, &line_len, fp) < 0 || !line_len)
 			goto next_close;
 
-		src_line->path = malloc(sizeof(char) * line_len + 1);
-		if (!src_line->path)
-			goto next_close;
-
-		strcpy(src_line->path, path);
+		src_line->path = path;
 		insert_source_line(&tmp_root, src_line);
 
 	next_close:
-- 
1.7.11.7

[PATCH 3/9] perf hists: Free srcline when freeing hist_entry

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

We've been leaked srcline of hist_entry, it should be freed also.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 46a0d35a05e1..e6c11ccf138c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -530,6 +530,7 @@ void hist_entry__free(struct hist_entry *he)
 {
 	free(he->branch_info);
 	free(he->mem_info);
+	free(he->srcline);
 	free(he);
 }
 
-- 
1.7.11.7

[PATCH 4/9] perf tools: Factor out get/free_srcline()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Currently external addr2line tool is used for srcline sort key and
annotate with srcline info.  Separate the common code to prepare
upcoming enhancements.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Makefile        |  1 +
 tools/perf/util/annotate.c | 20 ++---------
 tools/perf/util/hist.c     |  2 +-
 tools/perf/util/sort.c     | 17 ++--------
 tools/perf/util/srcline.c  | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/util.h     |  6 ++++
 6 files changed, 97 insertions(+), 32 deletions(-)
 create mode 100644 tools/perf/util/srcline.c

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index bfd12d02a304..e9e496e01914 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -360,6 +360,7 @@ LIB_OBJS += $(OUTPUT)util/rblist.o
 LIB_OBJS += $(OUTPUT)util/intlist.o
 LIB_OBJS += $(OUTPUT)util/vdso.o
 LIB_OBJS += $(OUTPUT)util/stat.o
+LIB_OBJS += $(OUTPUT)util/srcline.o
 
 LIB_OBJS += $(OUTPUT)ui/setup.o
 LIB_OBJS += $(OUTPUT)ui/helpline.o
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 8a5babc71d53..c8a0d5762b1d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1018,7 +1018,7 @@ static void symbol__free_source_line(struct symbol *sym, int len)
 			  (sizeof(src_line->p) * (src_line->nr_pcnt - 1));
 
 	for (i = 0; i < len; i++) {
-		free(src_line->path);
+		free_srcline(src_line->path);
 		src_line = (void *)src_line + sizeof_src_line;
 	}
 
@@ -1035,7 +1035,6 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 	u64 start;
 	int i, k;
 	int evidx = evsel->idx;
-	char cmd[PATH_MAX * 2];
 	struct source_line *src_line;
 	struct annotation *notes = symbol__annotation(sym);
 	struct sym_hist *h = annotation__histogram(notes, evidx);
@@ -1063,10 +1062,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 	start = map__rip_2objdump(map, sym->start);
 
 	for (i = 0; i < len; i++) {
-		char *path = NULL;
-		size_t line_len;
 		u64 offset;
-		FILE *fp;
 		double percent_max = 0.0;
 
 		src_line->nr_pcnt = nr_pcnt;
@@ -1083,19 +1079,9 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 			goto next;
 
 		offset = start + i;
-		sprintf(cmd, "addr2line -e %s %016" PRIx64, filename, offset);
-		fp = popen(cmd, "r");
-		if (!fp)
-			goto next;
-
-		if (getline(&path, &line_len, fp) < 0 || !line_len)
-			goto next_close;
-
-		src_line->path = path;
+		src_line->path = get_srcline(filename, offset);
 		insert_source_line(&tmp_root, src_line);
 
-	next_close:
-		pclose(fp);
 	next:
 		src_line = (void *)src_line + sizeof_src_line;
 	}
@@ -1136,7 +1122,7 @@ static void print_summary(struct rb_root *root, const char *filename)
 
 		path = src_line->path;
 		color = get_percent_color(percent_max);
-		color_fprintf(stdout, color, " %s", path);
+		color_fprintf(stdout, color, " %s\n", path);
 
 		node = rb_next(node);
 	}
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index e6c11ccf138c..fa695ce1662a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -530,7 +530,7 @@ void hist_entry__free(struct hist_entry *he)
 {
 	free(he->branch_info);
 	free(he->mem_info);
-	free(he->srcline);
+	free_srcline(he->srcline);
 	free(he);
 }
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5ba29fc679c5..0d633a059df4 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -251,8 +251,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 					unsigned int width __maybe_unused)
 {
 	FILE *fp = NULL;
-	char cmd[PATH_MAX + 2], *path = self->srcline, *nl;
-	size_t line_len;
+	char *path = self->srcline;
 
 	if (path != NULL)
 		goto out_path;
@@ -263,19 +262,9 @@ static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 	if (!strncmp(self->ms.map->dso->long_name, "/tmp/perf-", 10))
 		goto out_ip;
 
-	snprintf(cmd, sizeof(cmd), "addr2line -e %s %016" PRIx64,
-		 self->ms.map->dso->long_name, self->ip);
-	fp = popen(cmd, "r");
-	if (!fp)
-		goto out_ip;
-
-	if (getline(&self->srcline, &line_len, fp) < 0 || !line_len)
-		goto out_ip;
+	path = get_srcline(self->ms.map->dso->long_name, self->ip);
+	self->srcline = path;
 
-	nl = strchr(self->srcline, '\n');
-	if (nl != NULL)
-		*nl = '\0';
-	path = self->srcline;
 out_path:
 	if (fp)
 		pclose(fp);
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
new file mode 100644
index 000000000000..7e92cca6f502
--- /dev/null
+++ b/tools/perf/util/srcline.c
@@ -0,0 +1,83 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <linux/kernel.h>
+
+#include "util/util.h"
+#include "util/debug.h"
+
+static int addr2line(const char *dso_name, unsigned long addr,
+		     char **file, unsigned int *line_nr)
+{
+	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,
+		  dso_name, addr);
+
+	fp = popen(cmd, "r");
+	if (fp == NULL) {
+		pr_warning("popen failed for %s\n", dso_name);
+		return 0;
+	}
+
+	if (getline(&filename, &len, fp) < 0 || !len) {
+		pr_warning("addr2line has no output for %s\n", dso_name);
+		goto out;
+	}
+
+	sep = strchr(filename, '\n');
+	if (sep)
+		*sep = '\0';
+
+	if (!strcmp(filename, "??:0")) {
+		pr_debug("no debugging info in %s\n", dso_name);
+		free(filename);
+		goto out;
+	}
+
+	sep = strchr(filename, ':');
+	if (sep) {
+		*sep++ = '\0';
+		*file = filename;
+		*line_nr = strtoul(sep, NULL, 0);
+		ret = 1;
+	}
+out:
+	pclose(fp);
+	return ret;
+}
+
+char *get_srcline(const char *dso_name, unsigned long addr)
+{
+	char *file;
+	unsigned line;
+	char *srcline;
+	size_t size;
+
+	if (!addr2line(dso_name, addr, &file, &line))
+		return SRCLINE_UNKNOWN;
+
+	/* just calculate actual length */
+	size = snprintf(NULL, 0, "%s:%u", file, line) + 1;
+
+	srcline = malloc(size);
+	if (srcline)
+		snprintf(srcline, size, "%s:%u", file, line);
+	else
+		srcline = SRCLINE_UNKNOWN;
+
+	free(file);
+	return srcline;
+}
+
+void free_srcline(char *srcline)
+{
+	if (srcline && strcmp(srcline, SRCLINE_UNKNOWN) != 0)
+		free(srcline);
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index cc1574edcd9a..9a0bbe45f642 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -279,4 +279,10 @@ void dump_stack(void);
 extern unsigned int page_size;
 
 void get_term_dimensions(struct winsize *ws);
+
+#define SRCLINE_UNKNOWN  ((char *) "??:0")
+
+char *get_srcline(const char *dso_name, unsigned long addr);
+void free_srcline(char *srcline);
+
 #endif /* GIT_COMPAT_UTIL_H */
-- 
1.7.11.7

[PATCH 5/9] perf tools: Do not try to call addr2line for non-binary files

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

No need to call addr2line since they don't have such information.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c    |  3 ---
 tools/perf/util/srcline.c | 11 +++++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 0d633a059df4..113aa92b1191 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -259,9 +259,6 @@ static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 	if (!self->ms.map)
 		goto out_ip;
 
-	if (!strncmp(self->ms.map->dso->long_name, "/tmp/perf-", 10))
-		goto out_ip;
-
 	path = get_srcline(self->ms.map->dso->long_name, self->ip);
 	self->srcline = path;
 
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 7e92cca6f502..777f91880cdb 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -57,11 +57,17 @@ char *get_srcline(const char *dso_name, unsigned long addr)
 {
 	char *file;
 	unsigned line;
-	char *srcline;
+	char *srcline = SRCLINE_UNKNOWN;
 	size_t size;
 
+	if (dso_name[0] == '[')
+		goto out;
+
+	if (!strncmp(dso_name, "/tmp/perf-", 10))
+		goto out;
+
 	if (!addr2line(dso_name, addr, &file, &line))
-		return SRCLINE_UNKNOWN;
+		goto out;
 
 	/* just calculate actual length */
 	size = snprintf(NULL, 0, "%s:%u", file, line) + 1;
@@ -73,6 +79,7 @@ char *get_srcline(const char *dso_name, unsigned long addr)
 		srcline = SRCLINE_UNKNOWN;
 
 	free(file);
+out:
 	return srcline;
 }
 
-- 
1.7.11.7

[PATCH 6/9] perf tools: Pass dso instead of dso_name to get_srcline()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

This is a preparation of next change.  No functional changes are
intended.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 11 ++++-------
 tools/perf/util/dso.h      |  1 +
 tools/perf/util/sort.c     |  2 +-
 tools/perf/util/srcline.c  |  4 +++-
 tools/perf/util/util.h     |  4 +++-
 5 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c8a0d5762b1d..0137f028c0d6 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1029,8 +1029,7 @@ static void symbol__free_source_line(struct symbol *sym, int len)
 /* Get the filename:line for the colored entries */
 static int symbol__get_source_line(struct symbol *sym, struct map *map,
 				   struct perf_evsel *evsel,
-				   struct rb_root *root, int len,
-				   const char *filename)
+				   struct rb_root *root, int len)
 {
 	u64 start;
 	int i, k;
@@ -1079,7 +1078,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
 			goto next;
 
 		offset = start + i;
-		src_line->path = get_srcline(filename, offset);
+		src_line->path = get_srcline(map->dso, offset);
 		insert_source_line(&tmp_root, src_line);
 
 	next:
@@ -1286,7 +1285,6 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 			 bool full_paths, int min_pcnt, int max_lines)
 {
 	struct dso *dso = map->dso;
-	const char *filename = dso->long_name;
 	struct rb_root source_line = RB_ROOT;
 	u64 len;
 
@@ -1296,9 +1294,8 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 	len = symbol__size(sym);
 
 	if (print_lines) {
-		symbol__get_source_line(sym, map, evsel, &source_line,
-					len, filename);
-		print_summary(&source_line, filename);
+		symbol__get_source_line(sym, map, evsel, &source_line, len);
+		print_summary(&source_line, dso->long_name);
 	}
 
 	symbol__annotate_printf(sym, map, evsel, full_paths,
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index d51aaf272c68..94c2f1a8b116 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -5,6 +5,7 @@
 #include <linux/rbtree.h>
 #include "types.h"
 #include "map.h"
+#include "build-id.h"
 
 enum dso_binary_type {
 	DSO_BINARY_TYPE__KALLSYMS = 0,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 113aa92b1191..58cfe69d3a6c 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -259,7 +259,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 	if (!self->ms.map)
 		goto out_ip;
 
-	path = get_srcline(self->ms.map->dso->long_name, self->ip);
+	path = get_srcline(self->ms.map->dso, self->ip);
 	self->srcline = path;
 
 out_path:
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 777f91880cdb..c736d9428cf2 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -4,6 +4,7 @@
 
 #include <linux/kernel.h>
 
+#include "util/dso.h"
 #include "util/util.h"
 #include "util/debug.h"
 
@@ -53,11 +54,12 @@ out:
 	return ret;
 }
 
-char *get_srcline(const char *dso_name, unsigned long addr)
+char *get_srcline(struct dso *dso, unsigned long addr)
 {
 	char *file;
 	unsigned line;
 	char *srcline = SRCLINE_UNKNOWN;
+	char *dso_name = dso->long_name;
 	size_t size;
 
 	if (dso_name[0] == '[')
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 9a0bbe45f642..20dd24d8f338 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -282,7 +282,9 @@ void get_term_dimensions(struct winsize *ws);
 
 #define SRCLINE_UNKNOWN  ((char *) "??:0")
 
-char *get_srcline(const char *dso_name, unsigned long addr);
+struct dso;
+
+char *get_srcline(struct dso *dso, unsigned long addr);
 void free_srcline(char *srcline);
 
 #endif /* GIT_COMPAT_UTIL_H */
-- 
1.7.11.7

[PATCH 7/9] perf tools: Save failed result of get_srcline()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Some dso's lack srcline info, so there's no point to keep trying on
them.  Just save failture status and skip them.

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

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index c4374f07603c..cc959d2fa5da 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -421,6 +421,7 @@ struct dso *dso__new(const char *name)
 		dso->loaded = 0;
 		dso->sorted_by_name = 0;
 		dso->has_build_id = 0;
+		dso->has_srcline = 1;
 		dso->kernel = DSO_TYPE_USER;
 		dso->needs_swap = DSO_SWAP__UNSET;
 		INIT_LIST_HEAD(&dso->node);
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 94c2f1a8b116..2a1ae2a8d39c 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -79,6 +79,7 @@ struct dso {
 	enum dso_binary_type	data_type;
 	u8		 adjust_symbols:1;
 	u8		 has_build_id:1;
+	u8		 has_srcline:1;
 	u8		 hit:1;
 	u8		 annotate_warned:1;
 	u8		 sname_alloc:1;
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index c736d9428cf2..dcff10bed7da 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -58,10 +58,13 @@ char *get_srcline(struct dso *dso, unsigned long addr)
 {
 	char *file;
 	unsigned line;
-	char *srcline = SRCLINE_UNKNOWN;
+	char *srcline;
 	char *dso_name = dso->long_name;
 	size_t size;
 
+	if (!dso->has_srcline)
+		return SRCLINE_UNKNOWN;
+
 	if (dso_name[0] == '[')
 		goto out;
 
@@ -81,8 +84,11 @@ char *get_srcline(struct dso *dso, unsigned long addr)
 		srcline = SRCLINE_UNKNOWN;
 
 	free(file);
-out:
 	return srcline;
+
+out:
+	dso->has_srcline = 0;
+	return SRCLINE_UNKNOWN;
 }
 
 void free_srcline(char *srcline)
-- 
1.7.11.7

[PATCH 8/9] perf tools: Implement addr2line directly using libbfd

[Namhyung Kim]

From: Roberto Vitillo <ravitillo@lbl.gov>

When the srcline sort key is used , the external addr2line utility
needs to be run for each hist entry to get the srcline info.  This can
consume quite a time if one has a huge perf.data file.

So rather than executing the external utility, implement it internally
and just call it.  We can do it since we've linked with libbfd
already.

Signed-off-by: Roberto Agostino Vitillo <ravitillo@lbl.gov>
[namhyung@kernel.org: Use a2l_data struct instead of static globals]
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/config/Makefile |   4 ++
 tools/perf/util/srcline.c  | 167 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)

diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index 214e17e97e5c..128d580ca397 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -396,6 +396,10 @@ else
   endif
 endif
 
+ifndef ($(filter -lbfd,$(EXTLIBS)),)
+  CFLAGS += -DLIBBFD_SUPPORT
+endif
+
 ifndef NO_STRLCPY
   ifeq ($(call try-cc,$(SOURCE_STRLCPY),,-DHAVE_STRLCPY),y)
     CFLAGS += -DHAVE_STRLCPY
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index dcff10bed7da..10983a92bb5e 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -8,6 +8,172 @@
 #include "util/util.h"
 #include "util/debug.h"
 
+#ifdef LIBBFD_SUPPORT
+
+/*
+ * Implement addr2line using libbfd.
+ */
+#define PACKAGE "perf"
+#include <bfd.h>
+
+struct a2l_data {
+	const char 	*input;
+	unsigned long 	addr;
+
+	bool 		found;
+	const char 	*filename;
+	const char 	*funcname;
+	unsigned 	line;
+
+	bfd 		*abfd;
+	asymbol 	**syms;
+};
+
+static int bfd_error(const char *string)
+{
+	const char *errmsg;
+
+	errmsg = bfd_errmsg(bfd_get_error());
+	fflush(stdout);
+
+	if (string)
+		pr_debug("%s: %s\n", string, errmsg);
+	else
+		pr_debug("%s\n", errmsg);
+
+	return -1;
+}
+
+static int slurp_symtab(bfd *abfd, struct a2l_data *a2l)
+{
+	long storage;
+	long symcount;
+	asymbol **syms;
+	bfd_boolean dynamic = FALSE;
+
+	if ((bfd_get_file_flags(abfd) & HAS_SYMS) == 0)
+		return bfd_error(bfd_get_filename(abfd));
+
+	storage = bfd_get_symtab_upper_bound(abfd);
+	if (storage == 0L) {
+		storage = bfd_get_dynamic_symtab_upper_bound(abfd);
+		dynamic = TRUE;
+	}
+	if (storage < 0L)
+		return bfd_error(bfd_get_filename(abfd));
+
+	syms = malloc(storage);
+	if (dynamic)
+		symcount = bfd_canonicalize_dynamic_symtab(abfd, syms);
+	else
+		symcount = bfd_canonicalize_symtab(abfd, syms);
+
+	if (symcount < 0) {
+		free(syms);
+		return bfd_error(bfd_get_filename(abfd));
+	}
+
+	a2l->syms = syms;
+	return 0;
+}
+
+static void find_address_in_section(bfd *abfd, asection *section, void *data)
+{
+	bfd_vma pc, vma;
+	bfd_size_type size;
+	struct a2l_data *a2l = data;
+
+	if (a2l->found)
+		return;
+
+	if ((bfd_get_section_flags(abfd, section) & SEC_ALLOC) == 0)
+		return;
+
+	pc = a2l->addr;
+	vma = bfd_get_section_vma(abfd, section);
+	size = bfd_get_section_size(section);
+
+	if (pc < vma || pc >= vma + size)
+		return;
+
+	a2l->found = bfd_find_nearest_line(abfd, section, a2l->syms, pc - vma,
+					   &a2l->filename, &a2l->funcname,
+					   &a2l->line);
+}
+
+static struct a2l_data *addr2line_init(const char *path)
+{
+	bfd *abfd;
+	struct a2l_data *a2l = NULL;
+
+	abfd = bfd_openr(path, NULL);
+	if (abfd == NULL)
+		return NULL;
+
+	if (!bfd_check_format(abfd, bfd_object))
+		goto out;
+
+	a2l = zalloc(sizeof(*a2l));
+	if (a2l == NULL)
+		goto out;
+
+	a2l->abfd = abfd;
+	a2l->input = strdup(path);
+	if (a2l->input == NULL)
+		goto out;
+
+	if (slurp_symtab(abfd, a2l))
+		goto out;
+
+	return a2l;
+
+out:
+	if (a2l) {
+		free((void *)a2l->input);
+		free(a2l);
+	}
+	bfd_close(abfd);
+	return NULL;
+}
+
+static void addr2line_cleanup(struct a2l_data *a2l)
+{
+	if (a2l->abfd)
+		bfd_close(a2l->abfd);
+	free((void *)a2l->input);
+	free(a2l->syms);
+	free(a2l);
+}
+
+static int addr2line(const char *dso_name, unsigned long addr,
+		     char **file, unsigned int *line)
+{
+	int ret = 0;
+	struct a2l_data *a2l;
+
+	a2l = addr2line_init(dso_name);
+	if (a2l == NULL) {
+		pr_warning("addr2line_init failed for %s\n", dso_name);
+		return 0;
+	}
+
+	a2l->addr = addr;
+	bfd_map_over_sections(a2l->abfd, find_address_in_section, a2l);
+
+	if (a2l->found && a2l->filename) {
+		*file = strdup(a2l->filename);
+		*line = a2l->line;
+
+		if (*file)
+			ret = 1;
+	}
+
+	addr2line_cleanup(a2l);
+	return ret;
+}
+
+#else /* LIBBFD_SUPPORT */
+
 static int addr2line(const char *dso_name, unsigned long addr,
 		     char **file, unsigned int *line_nr)
 {
@@ -53,6 +219,7 @@ out:
 	pclose(fp);
 	return ret;
 }
+#endif /* LIBBFD_SUPPORT */
 
 char *get_srcline(struct dso *dso, unsigned long addr)
 {
-- 
1.7.11.7

[PATCH 9/9] perf tools: Fix srcline sort key behavior

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Currently the srcline sort key compares ip rather than srcline info.
I guess this was due to a performance reason to run external addr2line
utility.  Now we have implemented the functionality inside, use the
srcline info when comparing hist entries.

Also constantly print "??:0" string for unknown srcline rather than
printing ip.

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

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 58cfe69d3a6c..082480579861 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -243,33 +243,32 @@ struct sort_entry sort_sym = {
 static int64_t
 sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	return (int64_t)(right->ip - left->ip);
+	if (!left->srcline) {
+		if (!left->ms.map)
+			left->srcline = SRCLINE_UNKNOWN;
+		else {
+			struct map *map = left->ms.map;
+			left->srcline = get_srcline(map->dso,
+					    map__rip_2objdump(map, left->ip));
+		}
+	}
+	if (!right->srcline) {
+		if (!right->ms.map)
+			right->srcline = SRCLINE_UNKNOWN;
+		else {
+			struct map *map = right->ms.map;
+			right->srcline = get_srcline(map->dso,
+					    map__rip_2objdump(map, right->ip));
+		}
+	}
+	return strcmp(left->srcline, right->srcline);
 }
 
 static int hist_entry__srcline_snprintf(struct hist_entry *self, char *bf,
 					size_t size,
 					unsigned int width __maybe_unused)
 {
-	FILE *fp = NULL;
-	char *path = self->srcline;
-
-	if (path != NULL)
-		goto out_path;
-
-	if (!self->ms.map)
-		goto out_ip;
-
-	path = get_srcline(self->ms.map->dso, self->ip);
-	self->srcline = path;
-
-out_path:
-	if (fp)
-		pclose(fp);
-	return repsep_snprintf(bf, size, "%s", path);
-out_ip:
-	if (fp)
-		pclose(fp);
-	return repsep_snprintf(bf, size, "%-#*llx", BITS_PER_LONG / 4, self->ip);
+	return repsep_snprintf(bf, size, "%s", self->srcline);
 }
 
 struct sort_entry sort_srcline = {
-- 
1.7.11.7

Re: [PATCH 0/9] perf tools: Enhance and correct srcline behavior

[Peter Zijlstra]

On Thu, Aug 01, 2013 at 11:55:35AM +0900, Namhyung Kim wrote:
> Hello,
> 
> This patchset tries to fix and enhance current srcline behavior.
> 
> Firstly it doesn't actually sort by srcline info but by ip.  I suspect
> it was because of a performance reason to run external addr2line
> utility.  It showed the srcline info after hist entries were
> collapsed.  Thanks to Roberto, we now have internal implementation of
> addr2line using libbfd so can sort/compare by srcline of entries.

Sweet..

Re: [PATCH 0/9] perf tools: Enhance and correct srcline behavior

[Jiri Olsa]

On Thu, Aug 01, 2013 at 11:55:35AM +0900, Namhyung Kim wrote:
> Hello,
> 
> This patchset tries to fix and enhance current srcline behavior.
> 
> Firstly it doesn't actually sort by srcline info but by ip.  I suspect
> it was because of a performance reason to run external addr2line
> utility.  It showed the srcline info after hist entries were
> collapsed.  Thanks to Roberto, we now have internal implementation of
> addr2line using libbfd so can sort/compare by srcline of entries.
> 
> Second problem for me was it sometimes printed "??:0" and sometimes
> printed a raw ip value for unknown srcline info.  I changed to print
> the former consistently.
> 
> While at it, I found some bugs/leaks in srcline handling.  Patch 1-3
> are fixes for those and can be merged separately.
> 
> You can get this series on my 'perf/srcline-v1' branch in my tree at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Any comments are welcome, thanks.
> Namhyung
> 
> 
> Namhyung Kim (8):
>   perf sort: Fix a memory leak on srcline
>   perf annotate: Reuse path from the result of addr2line
>   perf hists: Free srcline when freeing hist_entry
>   perf tools: Factor out get/free_srcline()
>   perf tools: Do not try to call addr2line for non-binary files
>   perf tools: Pass dso instead of dso_name to get_srcline()
>   perf tools: Save failed result of get_srcline()
>   perf tools: Fix srcline sort key behavior
> 
> Roberto Vitillo (1):
>   perf tools: Implement addr2line directly using libbfd
> 

nice..

Reviewed-by: Jiri Olsa <jolsa@redhat.com>