[PATCH v2 0/3] perf annotate: Introduce the new source code view

[PATCH v2 0/3] perf annotate: Introduce the new source code view

[Taeung Song]

Hi,

Currently perf-annotate have several problems.

  - Wrong line numbers on perf-annotate (both stdio and TUI)
  - Wrong sum of overhead(percent) matching source lines
  - It's so confusing that the output is mixed with
    both source code and assembly code. (related to depending on 'objdump -S')

So fix them regarding wrong line numbers
and Introduce new source_code collecting actual code,
not depending on 'objdump -S'.

I'd appreciate some feedback.

Thanks,
Taeung

v2:
- contains the new source code view (Namhyung)

Taeung Song (3):
  perf annotate: Get correct line numbers matched with addr
  perf annotate: Introduce the new source code view
  perf annotate: Support the new source code view for TUI

 tools/perf/builtin-annotate.c     |   2 +-
 tools/perf/ui/browsers/annotate.c | 175 ++++++++++++++++-------
 tools/perf/util/annotate.c        | 292 ++++++++++++++++++++++++++++++++++----
 tools/perf/util/annotate.h        |  28 ++++
 4 files changed, 419 insertions(+), 78 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

[Taeung Song]

Currently perf-annotate show wrong line numbers.

For example,
Actual source code is as below

...
  21 };
  22
  23 unsigned int limited_wgt;
  24
  25 unsigned int get_cond_maxprice(int wgt)
  26 {
...

However, the output of perf-annotate is as below.

  4   Disassembly of section .text:

  6   0000000000400966 <get_cond_maxprice>:
  7   get_cond_maxprice():
  26  };

  28  unsigned int limited_wgt;

  30  unsigned int get_cond_maxprice(int wgt)
  31  {

The cause is the wrong way counting line numbers
in symbol__parse_objdump_line().
So remove wrong current code counting line number and
use other method for it using functions related to addr2line
instead of the output of '-l' of objdump.

However, despite the correct line numbers,
we can't show proper source code view
because of limitations from output of 'objdump -S'.

So, next commit will resolve the limitations from 'objdump -S'
with the new source code view.

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/util/annotate.c | 59 +++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 273f21f..42b752e 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -19,14 +19,12 @@
 #include "evsel.h"
 #include "block-range.h"
 #include "arch/common.h"
-#include <regex.h>
 #include <pthread.h>
 #include <linux/bitops.h>
 #include <sys/utsname.h>
 
 const char 	*disassembler_style;
 const char	*objdump_path;
-static regex_t	 file_lineno;
 
 static struct ins_ops *ins__find(struct arch *arch, const char *name);
 static void ins__sort(struct arch *arch);
@@ -814,7 +812,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 }
 
 static struct disasm_line *disasm_line__new(s64 offset, char *line,
-					    size_t privsize, int line_nr,
+					    size_t privsize,
 					    struct arch *arch,
 					    struct map *map)
 {
@@ -823,7 +821,6 @@ static struct disasm_line *disasm_line__new(s64 offset, char *line,
 	if (dl != NULL) {
 		dl->offset = offset;
 		dl->line = strdup(line);
-		dl->line_nr = line_nr;
 		if (dl->line == NULL)
 			goto out_delete;
 
@@ -1121,6 +1118,29 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
 	return 0;
 }
 
+static int parse_srcline(char *srcline, char **path, int *line_nr)
+{
+	char *sep;
+
+	if (path != NULL)
+		*path = NULL;
+
+	if (!strcmp(srcline, SRCLINE_UNKNOWN))
+		return -1;
+
+	sep = strchr(srcline, ':');
+	if (sep) {
+		*sep = '\0';
+		if (path != NULL)
+			*path = srcline;
+		if (line_nr != NULL)
+			*line_nr = strtoul(++sep, NULL, 0);
+	} else
+		return -1;
+
+	return 0;
+}
+
 /*
  * symbol__parse_objdump_line() parses objdump output (with -d --no-show-raw)
  * which looks like following
@@ -1143,15 +1163,14 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
  */
 static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 				      struct arch *arch,
-				      FILE *file, size_t privsize,
-				      int *line_nr)
+				      FILE *file, size_t privsize)
 {
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *dl;
 	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
 	size_t line_len;
 	s64 line_ip, offset = -1;
-	regmatch_t match[2];
+	bool has_src_code = true;
 
 	if (getline(&line, &line_len, file) < 0)
 		return -1;
@@ -1169,12 +1188,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	line_ip = -1;
 	parsed_line = line;
 
-	/* /filename:linenr ? Save line number and ignore. */
-	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
-		*line_nr = atoi(line + match[1].rm_so);
-		return 0;
-	}
-
 	/*
 	 * Strip leading spaces:
 	 */
@@ -1205,13 +1218,18 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 			parsed_line = tmp2 + 1;
 	}
 
-	dl = disasm_line__new(offset, parsed_line, privsize, *line_nr, arch, map);
+	dl = disasm_line__new(offset, parsed_line, privsize, arch, map);
 	free(line);
-	(*line_nr)++;
 
 	if (dl == NULL)
 		return -1;
 
+	if (has_src_code && dl->offset != -1 && map->dso->symsrc_filename) {
+		if (parse_srcline(get_srcline(map->dso, line_ip, NULL, false),
+				  NULL, &dl->line_nr) < 0)
+			has_src_code = false;
+	}
+
 	if (!disasm_line__has_offset(dl)) {
 		dl->ops.target.offset = dl->ops.target.addr -
 					map__rip_2objdump(map, sym->start);
@@ -1235,11 +1253,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	return 0;
 }
 
-static __attribute__((constructor)) void symbol__init_regexpr(void)
-{
-	regcomp(&file_lineno, "^/[^:]+:([0-9]+)", REG_EXTENDED);
-}
-
 static void delete_last_nop(struct symbol *sym)
 {
 	struct annotation *notes = symbol__annotation(sym);
@@ -1360,7 +1373,6 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	struct kcore_extract kce;
 	bool delete_extract = false;
 	int stdout_fd[2];
-	int lineno = 0;
 	int nline;
 	pid_t pid;
 	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
@@ -1435,7 +1447,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	snprintf(command, sizeof(command),
 		 "%s %s%s --start-address=0x%016" PRIx64
 		 " --stop-address=0x%016" PRIx64
-		 " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
+		 " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
 		 objdump_path ? objdump_path : "objdump",
 		 disassembler_style ? "-M " : "",
 		 disassembler_style ? disassembler_style : "",
@@ -1482,8 +1494,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 
 	nline = 0;
 	while (!feof(file)) {
-		if (symbol__parse_objdump_line(sym, map, arch, file, privsize,
-			    &lineno) < 0)
+		if (symbol__parse_objdump_line(sym, map, arch, file, privsize) < 0)
 			break;
 		nline++;
 	}
-- 
2.7.4

[PATCH v2 2/3] perf annotate: Introduce the new source code view

[Taeung Song]

The current source code view using 'objdump -S'
has several limitations regarding line numbers of each soure
code line and confusing mixed soure code & diassembly view.

So not use the '-S' option any more and
add the new reable source code view with
correct line numbers and source code per each symbol(function)
using new struct source_code.

Before:

    $ perf annotate --stdio -s pack_knapsack
 Percent |      Source code & Disassembly of old for cycles:ppp (82 samples)
----------------------------------------------------------------------------
...
         :      void pack_knapsack(struct jewelry *jewelry)
         :      {
    0.00 :        40089e:       push   %rbp
    0.00 :        40089f:       mov    %rsp,%rbp
    0.00 :        4008a2:       sub    $0x18,%rsp
    0.00 :        4008a6:       mov    %rdi,-0x18(%rbp)
         :               * price per limited weight.
         :               */
         :              int wgt;
         :              unsigned int maxprice;
         :
         :              if (limited_wgt < jewelry->wgt)
    0.00 :        4008aa:       mov    -0x18(%rbp),%rax
...

After:

    $ perf annotate --stdio -s pack_knapsack
 Percent |      Source code of old_pack_knapsack.c for cycles:ppp (82 samples)
------------------------------------------------------------------------------
    0.00 : 43           return maxprice;
    0.00 : 44   }
    0.00 : 45
    0.00 : 46   void pack_knapsack(struct jewelry *jewelry)
    0.00 : 47   {
    0.00 : 48           /* Case by case pack knapsack following maximum
    0.00 : 49            * price per limited weight.
    0.00 : 50            */
    0.00 : 51           int wgt;
    0.00 : 52           unsigned int maxprice;
    0.00 : 53
    0.00 : 54           if (limited_wgt < jewelry->wgt)
    0.00 : 55                   return;
    0.00 : 56
   52.44 : 57           for (wgt = 0; wgt <= limited_wgt; wgt++) {
    9.76 : 58                   if (jewelry->wgt <= wgt) {
   25.61 : 59                           maxprice = get_cond_maxprice(wgt, jewelry);
    0.00 : 60
   12.20 : 61                           if (knapsack_list[wgt].maxprice < maxprice)
    0.00 : 62                                   knapsack_list[wgt].maxprice = maxprice;
    0.00 : 63                   }
    0.00 : 64           }
    0.00 : 65   }

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-annotate.c     |   2 +-
 tools/perf/ui/browsers/annotate.c |   5 -
 tools/perf/util/annotate.c        | 235 +++++++++++++++++++++++++++++++++++++-
 tools/perf/util/annotate.h        |  27 +++++
 4 files changed, 257 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 4f52d85..5ecc73c 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -431,7 +431,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		     "Look for files with symbols relative to this directory",
 		     symbol__config_symfs),
 	OPT_BOOLEAN(0, "source", &symbol_conf.annotate_src,
-		    "Interleave source code with assembly code (default)"),
+		    "Display source code for each symbol (default)"),
 	OPT_BOOLEAN(0, "asm-raw", &symbol_conf.annotate_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index ba36aac..03b2012 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -11,11 +11,6 @@
 #include "../../util/config.h"
 #include <pthread.h>
 
-struct disasm_line_samples {
-	double		percent;
-	u64		nr;
-};
-
 #define IPC_WIDTH 6
 #define CYCLES_WIDTH 6
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 42b752e..d7826d3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1363,6 +1363,203 @@ static const char *annotate__norm_arch(const char *arch_name)
 	return normalize_arch((char *)arch_name);
 }
 
+bool symbol__has_source_code(struct symbol *sym, struct map *map)
+{
+	u64 start = map__rip_2objdump(map, sym->start);
+
+	if (map->dso->symsrc_filename &&
+	    parse_srcline(get_srcline(map->dso, start, NULL, false),
+			  NULL, NULL) != -1)
+		return true;
+
+	return false;
+}
+
+int symbol__free_source_code(struct symbol *sym)
+{
+	struct annotation *notes = symbol__annotation(sym);
+	struct source_code *code = notes->src->code;
+	struct code_line *pos, *tmp;
+
+	if (code == NULL)
+		return -1;
+
+	list_for_each_entry_safe(pos, tmp, &code->lines, node) {
+		list_del_init(&pos->node);
+		zfree(&pos->line);
+		zfree(&pos->samples_sum);
+	}
+	zfree(&code->path);
+	zfree(&notes->src->code);
+	return 0;
+}
+
+static void source_code__print(struct code_line *cl, int nr_events)
+{
+	int i;
+	const char *color;
+	double percent, max_percent = 0.0;
+
+	for (i = 0; i < nr_events; i++) {
+		percent = cl->samples_sum[i].percent;
+		color = get_percent_color(percent);
+		if (max_percent < percent)
+			max_percent = percent;
+
+		if (symbol_conf.show_total_period)
+			color_fprintf(stdout, color, " %7" PRIu64,
+				      cl->samples_sum[i].nr);
+		else
+			color_fprintf(stdout, color, " %7.2f", percent);
+	}
+	color = get_percent_color(max_percent);
+	color_fprintf(stdout, color, " : %d	%s\n",
+		      cl->line_nr, cl->line);
+}
+
+static int source_code__collect(struct source_code *code, char *path,
+				int start_linenr, int end_linenr)
+{
+	FILE *file;
+	int ret = -1, linenr = 0;
+	char *line = NULL, *c, *parsed_line;
+	size_t len;
+	struct code_line *cl;
+
+	if (access(path, R_OK) != 0)
+		return -1;
+
+	file = fopen(path, "r");
+	if (!file)
+		return -1;
+
+	if (srcline_full_filename)
+		code->path = strdup(path);
+	else
+		code->path = strdup(basename(path));
+
+	INIT_LIST_HEAD(&code->lines);
+	while (!feof(file)) {
+		if (getline(&line, &len, file) < 0)
+			break;
+
+		if (++linenr < start_linenr)
+			continue;
+
+		parsed_line = rtrim(line);
+		c = strchr(parsed_line, '\n');
+		if (c)
+			*c = '\0';
+
+		cl = zalloc(sizeof(*cl));
+		if (!cl)
+			break;
+
+		cl->line_nr = linenr;
+		cl->line = strdup(parsed_line);
+		cl->samples_sum =
+			zalloc(sizeof(struct disasm_line_samples) * code->nr_events);
+		if (!cl->samples_sum)
+			break;
+
+		list_add_tail(&cl->node, &code->lines);
+		if (linenr == end_linenr) {
+			ret = 0;
+			break;
+		}
+	}
+
+	free(line);
+	fclose(file);
+	return ret;
+}
+
+int symbol__get_source_code(struct symbol *sym, struct map *map,
+			    struct perf_evsel *evsel)
+{
+	struct annotation *notes = symbol__annotation(sym);
+	struct source_code *code;
+	int start_linenr, end_linenr, ret = 0;
+	char *path, *start_srcline, *end_srcline;
+	u64 start = map__rip_2objdump(map, sym->start);
+	u64 end = map__rip_2objdump(map, sym->end - 1);
+	bool bef_fullpath = srcline_full_filename;
+
+	srcline_full_filename = true;
+	start_srcline = get_srcline(map->dso, start, NULL, false);
+	end_srcline = get_srcline(map->dso, end, NULL, false);
+	srcline_full_filename = bef_fullpath;
+
+	if (parse_srcline(start_srcline, &path, &start_linenr) < 0)
+		return -1;
+	if (parse_srcline(end_srcline, &path, &end_linenr) < 0)
+		return -1;
+
+	code = zalloc(sizeof(struct source_code));
+	if (code == NULL)
+		return -1;
+
+	if (perf_evsel__is_group_event(evsel))
+		code->nr_events = evsel->nr_members;
+	else
+		code->nr_events = 1;
+
+	/* To read a function header for the sym */
+	if (start_linenr > 4)
+		start_linenr -= 4;
+	else
+		start_linenr = 1;
+
+	if (source_code__collect(code, path, start_linenr, end_linenr) < 0) {
+		zfree(&code);
+		ret = -1;
+	}
+	notes->src->code = code;
+
+	free_srcline(start_srcline);
+	free_srcline(end_srcline);
+	return ret;
+}
+
+static struct code_line *source_code__find_line(struct list_head *lines, int linenr)
+{
+	struct code_line *pos;
+
+	list_for_each_entry(pos, lines, node) {
+		if (pos->line_nr == linenr)
+			return pos;
+	}
+	return NULL;
+}
+
+void source_code__set_samples(struct disasm_line *dl, struct annotation *notes,
+			      struct perf_evsel *evsel)
+{
+	int i;
+	double percent;
+	u64 nr_samples;
+	struct sym_hist *h;
+	struct source_code *code;
+	struct code_line *cl;
+
+	code = notes->src->code;
+	for (i = 0; i < code->nr_events; i++) {
+		h = annotation__histogram(notes, evsel->idx + i);
+		nr_samples = h->addr[dl->offset];
+
+		if (nr_samples == 0)
+			percent = 0.0;
+		else
+			percent = 100.0 * nr_samples / h->sum;
+
+		cl = source_code__find_line(&code->lines, dl->line_nr);
+		if (cl == NULL)
+			continue;
+		cl->samples_sum[i].percent += percent;
+		cl->samples_sum[i].nr += nr_samples;
+	}
+}
+
 int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_name, size_t privsize)
 {
 	struct dso *dso = map->dso;
@@ -1447,14 +1644,13 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 	snprintf(command, sizeof(command),
 		 "%s %s%s --start-address=0x%016" PRIx64
 		 " --stop-address=0x%016" PRIx64
-		 " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
+		 " -d %s -C %s 2>/dev/null|grep -v %s|expand",
 		 objdump_path ? objdump_path : "objdump",
 		 disassembler_style ? "-M " : "",
 		 disassembler_style ? disassembler_style : "",
 		 map__rip_2objdump(map, sym->start),
 		 map__rip_2objdump(map, sym->end),
 		 symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
-		 symbol_conf.annotate_src ? "-S" : "",
 		 symfs_filename, symfs_filename);
 
 	pr_debug("Executing: %s\n", command);
@@ -1501,7 +1697,6 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
 
 	if (nline == 0)
 		pr_err("No output from %s\n", command);
-
 	/*
 	 * kallsyms does not have symbol sizes so there may a nop at the end.
 	 * Remove it.
@@ -1753,6 +1948,7 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 	struct sym_hist *h = annotation__histogram(notes, evsel->idx);
 	struct disasm_line *pos, *queue = NULL;
 	u64 start = map__rip_2objdump(map, sym->start);
+	bool has_src_code = false;
 	int printed = 2, queue_len = 0;
 	int more = 0;
 	u64 len;
@@ -1773,8 +1969,14 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 	if (perf_evsel__is_group_event(evsel))
 		width *= evsel->nr_members;
 
-	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
-	       width, width, "Percent", d_filename, evsel_name, h->sum);
+	if (symbol_conf.annotate_src && symbol__has_source_code(sym, map))
+		has_src_code = true;
+
+	graph_dotted_len = printf(" %-*.*s|	%s of %s for %s (%" PRIu64 " samples)\n",
+				  width, width, "Percent",
+				  has_src_code ? "Source code" : "Disassembly",
+				  has_src_code ? notes->src->code->path : d_filename,
+				  evsel_name, h->sum);
 
 	printf("%-*.*s----\n",
 	       graph_dotted_len, graph_dotted_len, graph_dotted_line);
@@ -1782,6 +1984,22 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 	if (verbose > 0)
 		symbol__annotate_hits(sym, evsel);
 
+	if (has_src_code) {
+		struct source_code *code = notes->src->code;
+		struct code_line *cl;
+
+		list_for_each_entry(pos, &notes->src->source, node) {
+			if (pos->offset == -1)
+				continue;
+			source_code__set_samples(pos, notes, evsel);
+		}
+
+		list_for_each_entry(cl, &code->lines, node)
+			source_code__print(cl, code->nr_events);
+
+		goto out;
+	}
+
 	list_for_each_entry(pos, &notes->src->source, node) {
 		if (context && queue == NULL) {
 			queue = pos;
@@ -1818,7 +2036,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
 			break;
 		}
 	}
-
+out:
+	printf("\n");
 	free(filename);
 
 	return more;
@@ -1902,11 +2121,15 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 		print_summary(&source_line, dso->long_name);
 	}
 
+	if (symbol_conf.annotate_src && symbol__has_source_code(sym, map))
+		symbol__get_source_code(sym, map, evsel);
+
 	symbol__annotate_printf(sym, map, evsel, full_paths,
 				min_pcnt, max_lines, 0);
 	if (print_lines)
 		symbol__free_source_line(sym, len);
 
+	symbol__free_source_code(sym);
 	disasm__purge(&symbol__annotation(sym)->src->source);
 
 	return 0;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 09776b5..6375012 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -56,6 +56,11 @@ int ins__scnprintf(struct ins *ins, char *bf, size_t size, struct ins_operands *
 
 struct annotation;
 
+struct disasm_line_samples {
+	double		percent;
+	u64		nr;
+};
+
 struct disasm_line {
 	struct list_head    node;
 	s64		    offset;
@@ -95,6 +100,22 @@ struct cyc_hist {
 	u16	reset;
 };
 
+struct code_line {
+	struct list_head    node;
+	int		    line_nr;
+	char		    *line;
+	struct disasm_line_samples *samples_sum;
+};
+
+struct source_code {
+	char		 *path;
+	int		 nr_events;
+	struct list_head lines;
+};
+
+void source_code__set_samples(struct disasm_line *dl, struct annotation *notes,
+			      struct perf_evsel *evsel);
+
 struct source_line_samples {
 	double		percent;
 	double		percent_sum;
@@ -123,6 +144,7 @@ struct source_line {
  */
 struct annotated_source {
 	struct list_head   source;
+	struct source_code *code;
 	struct source_line *lines;
 	int    		   nr_histograms;
 	size_t		   sizeof_sym_hist;
@@ -158,6 +180,11 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr);
 int symbol__alloc_hist(struct symbol *sym);
 void symbol__annotate_zero_histograms(struct symbol *sym);
 
+bool symbol__has_source_code(struct symbol *sym, struct map *map);
+int symbol__free_source_code(struct symbol *sym);
+int symbol__get_source_code(struct symbol *sym, struct map *map,
+			    struct perf_evsel *evsel);
+
 int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_name, size_t privsize);
 
 enum symbol_disassemble_errno {
-- 
2.7.4

[PATCH v2 3/3] perf annotate: Support the new source code view for TUI

[Taeung Song]

Current source code view in TUI also has
problems showing wrong line number and
confusing mixed source code & dissambly view.

So fix it with the new source code view.
We can toggle the new source code view by a 's' key.

Before:

       │11  void pack_knapsack(struct jewelry *jewelry)
       │12  {
       │      push   %rbp
       │      mov    %rsp,%rbp
       │      sub    $0x18,%rsp
       │      mov    %rdi,-0x18(%rbp)
       │17           * price per limited weight.
       │18           */
       │19          int wgt;
       │20          unsigned int maxprice;
       │
       │22          if (limited_wgt < jewelry->wgt)
       │      mov    -0x18(%rbp),%rax
       │      mov    (%rax),%edx
       │      mov    limited_wgt,%eax
       │      cmp    %eax,%edx
       │    ↓ ja     8d
       │28                  return;
       │
       │30          for (wgt = 0; wgt <= limited_wgt; wgt++) {
       │      movl   $0x0,-0x8(%rbp)
       │    ↓ jmp    7e
       │33                  if (jewelry->wgt <= wgt) {
  4.88 │25:   mov    -0x18(%rbp),%rax
       │      mov    (%rax),%edx
  1.22 │      mov    -0x8(%rbp),%eax
  1.22 │      cmp    %eax,%edx
  2.44 │    ↓ ja     7a
       │39                          maxprice = get_cond_maxprice(wgt, jewelry);

After:

       │46   void pack_knapsack(struct jewelry *jewelry)
       │47   {
       │48      /* Case by case pack knapsack following maximum
       │49       * price per limited weight.
       │50       */
       │51      int wgt;
       │52      unsigned int maxprice;
       │53
       │54      if (limited_wgt < jewelry->wgt)
       │55              return;
       │56
 52.44 │57      for (wgt = 0; wgt <= limited_wgt; wgt++) {
  9.76 │58              if (jewelry->wgt <= wgt) {
 25.61 │59                      maxprice = get_cond_maxprice(wgt, jewelry);

Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/ui/browsers/annotate.c | 170 ++++++++++++++++++++++++++++----------
 tools/perf/util/annotate.h        |   1 +
 2 files changed, 128 insertions(+), 43 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 03b2012..d6d072b 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -39,7 +39,7 @@ static struct annotate_browser_opt {
 };
 
 struct annotate_browser {
-	struct ui_browser b;
+	struct ui_browser b, cb;
 	struct rb_root	  entries;
 	struct rb_node	  *curr_hot;
 	struct disasm_line  *selection;
@@ -52,6 +52,7 @@ struct annotate_browser {
 	int		    nr_jumps;
 	bool		    searching_backwards;
 	bool		    have_cycles;
+	bool		    has_src_code;
 	u8		    addr_width;
 	u8		    jumps_width;
 	u8		    target_width;
@@ -104,6 +105,47 @@ static int annotate_browser__pcnt_width(struct annotate_browser *ab)
 	return w;
 }
 
+static void annotate_code_browser__write(struct ui_browser *browser, void *entry, int row)
+{
+	struct annotate_browser *ab = container_of(browser, struct annotate_browser, cb);
+	struct code_line *cl = list_entry(entry, struct code_line, node);
+	bool current_entry = ui_browser__is_current_entry(browser, row);
+	int i, printed;
+	double percent, max_percent = 0.0;
+	char line[256];
+
+	for (i = 0; i < ab->nr_events; i++) {
+		percent = cl->samples_sum[i].percent;
+		ui_browser__set_percent_color(browser, percent, current_entry);
+		if (percent == 0.0) {
+			ui_browser__write_nstring(browser, " ", 7 * ab->nr_events);
+			continue;
+		}
+
+		if (annotate_browser__opts.show_total_period)
+			ui_browser__printf(browser, "%6" PRIu64 " ",
+					   cl->samples_sum[i].nr);
+		else
+			ui_browser__printf(browser, "%6.2f ", percent);
+
+		if (max_percent < percent)
+			max_percent = percent;
+	}
+
+	SLsmg_write_char(' ');
+
+	if (annotate_browser__opts.show_linenr)
+		printed = scnprintf(line, sizeof(line), "%-*d ",
+				    ab->addr_width + 2, cl->line_nr);
+	else
+		printed = scnprintf(line, sizeof(line), "%*s  ",
+				    ab->addr_width, " ");
+
+	ui_browser__set_percent_color(browser, percent, current_entry);
+	ui_browser__write_nstring(browser, line, printed);
+	ui_browser__write_nstring(browser, cl->line, browser->width);
+}
+
 static void annotate_browser__write(struct ui_browser *browser, void *entry, int row)
 {
 	struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
@@ -284,6 +326,17 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
 				 from, to);
 }
 
+static unsigned int annotate_code_browser__refresh(struct ui_browser *browser)
+{
+	struct annotate_browser *ab = container_of(browser, struct annotate_browser, cb);
+	int ret = ui_browser__list_head_refresh(browser);
+	int pcnt_width = annotate_browser__pcnt_width(ab);
+
+	ui_browser__set_color(browser, HE_COLORSET_NORMAL);
+	__ui_browser__vline(browser, pcnt_width, 0, browser->height - 1);
+	return ret;
+}
+
 static unsigned int annotate_browser__refresh(struct ui_browser *browser)
 {
 	struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
@@ -394,6 +447,9 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
 			continue;
 		}
 
+		if (browser->has_src_code)
+			source_code__set_samples(pos, notes, evsel);
+
 		next = disasm__get_next_ip_line(&notes->src->source, pos);
 
 		for (i = 0; i < browser->nr_events; i++) {
@@ -422,45 +478,6 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
 	browser->curr_hot = rb_last(&browser->entries);
 }
 
-static bool annotate_browser__toggle_source(struct annotate_browser *browser)
-{
-	struct disasm_line *dl;
-	struct browser_disasm_line *bdl;
-	off_t offset = browser->b.index - browser->b.top_idx;
-
-	browser->b.seek(&browser->b, offset, SEEK_CUR);
-	dl = list_entry(browser->b.top, struct disasm_line, node);
-	bdl = disasm_line__browser(dl);
-
-	if (annotate_browser__opts.hide_src_code) {
-		if (bdl->idx_asm < offset)
-			offset = bdl->idx;
-
-		browser->b.nr_entries = browser->nr_entries;
-		annotate_browser__opts.hide_src_code = false;
-		browser->b.seek(&browser->b, -offset, SEEK_CUR);
-		browser->b.top_idx = bdl->idx - offset;
-		browser->b.index = bdl->idx;
-	} else {
-		if (bdl->idx_asm < 0) {
-			ui_helpline__puts("Only available for assembly lines.");
-			browser->b.seek(&browser->b, -offset, SEEK_CUR);
-			return false;
-		}
-
-		if (bdl->idx_asm < offset)
-			offset = bdl->idx_asm;
-
-		browser->b.nr_entries = browser->nr_asm_entries;
-		annotate_browser__opts.hide_src_code = true;
-		browser->b.seek(&browser->b, -offset, SEEK_CUR);
-		browser->b.top_idx = bdl->idx_asm - offset;
-		browser->b.index = bdl->idx_asm;
-	}
-
-	return true;
-}
-
 static void annotate_browser__init_asm_mode(struct annotate_browser *browser)
 {
 	ui_browser__reset_index(&browser->b);
@@ -696,6 +713,50 @@ static void annotate_browser__update_addr_width(struct annotate_browser *browser
 		browser->addr_width += browser->jumps_width + 1;
 }
 
+static int annotate_code_browser__run(struct annotate_browser *browser,
+				      char *title, const char *help)
+{
+	int key;
+
+	if (ui_browser__show(&browser->cb, title, help) < 0)
+		return -1;
+
+	while (1) {
+		key = ui_browser__run(&browser->cb, 0);
+
+		switch (key) {
+		case K_F1:
+		case 'h':
+			ui_browser__help_window(&browser->cb,
+		"UP/DOWN/PGUP\n"
+		"PGDN/SPACE    Navigate\n"
+		"q/ESC/CTRL+C  Return to dissembly view\n\n"
+		"ENTER         Show \n"
+		"s             Toggle source code view\n"
+		"t             Toggle total period view\n"
+		"k             Toggle line numbers\n");
+			continue;
+		case 't':
+			annotate_browser__opts.show_total_period =
+				!annotate_browser__opts.show_total_period;
+			continue;
+		case 'k':
+			annotate_browser__opts.show_linenr =
+				!annotate_browser__opts.show_linenr;
+			continue;
+		case 's':
+		case K_LEFT:
+		case K_ESC:
+		case 'q':
+		case CTRL('c'):
+			return 0;
+		default:
+			continue;
+		}
+	}
+	return 0;
+}
+
 static int annotate_browser__run(struct annotate_browser *browser,
 				 struct perf_evsel *evsel,
 				 struct hist_browser_timer *hbt)
@@ -775,7 +836,6 @@ static int annotate_browser__run(struct annotate_browser *browser,
 		"s             Toggle source code view\n"
 		"t             Toggle total period view\n"
 		"/             Search string\n"
-		"k             Toggle line numbers\n"
 		"r             Run available scripts\n"
 		"?             Search string backwards\n");
 			continue;
@@ -792,8 +852,10 @@ static int annotate_browser__run(struct annotate_browser *browser,
 			nd = browser->curr_hot;
 			break;
 		case 's':
-			if (annotate_browser__toggle_source(browser))
-				ui_helpline__puts(help);
+			if (browser->has_src_code)
+				annotate_code_browser__run(browser, title, help);
+			else
+				ui_helpline__puts("No source code for the symbol");
 			continue;
 		case 'o':
 			annotate_browser__opts.use_offset = !annotate_browser__opts.use_offset;
@@ -1095,6 +1157,27 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
 	browser.b.entries = &notes->src->source,
 	browser.b.width += 18; /* Percentage */
 
+	browser.has_src_code = symbol__has_source_code(sym, map);
+	if (browser.has_src_code &&
+	    symbol__get_source_code(sym, map, evsel) != -1) {
+		struct source_code *code = notes->src->code;
+		struct code_line *cl;
+
+		browser.cb.refresh = annotate_code_browser__refresh;
+		browser.cb.seek = ui_browser__list_head_seek;
+		browser.cb.write = annotate_code_browser__write;
+		browser.cb.use_navkeypressed = true;
+		browser.cb.entries = &code->lines;
+
+		list_for_each_entry(cl, &code->lines, node) {
+			size_t line_len = strlen(cl->line);
+
+			if (browser.cb.width < line_len)
+				browser.cb.width = line_len;
+			browser.cb.nr_entries++;
+		}
+	}
+
 	if (annotate_browser__opts.hide_src_code)
 		annotate_browser__init_asm_mode(&browser);
 
@@ -1105,6 +1188,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map,
 		list_del(&pos->node);
 		disasm_line__free(pos);
 	}
+	symbol__free_source_code(sym);
 
 out_free_offsets:
 	free(browser.offsets);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 6375012..c4e4c08 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -146,6 +146,7 @@ struct annotated_source {
 	struct list_head   source;
 	struct source_code *code;
 	struct source_line *lines;
+	bool		   has_src_code;
 	int    		   nr_histograms;
 	size_t		   sizeof_sym_hist;
 	struct cyc_hist	   *cycles_hist;
-- 
2.7.4

Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

[Namhyung Kim]

Hi Taeung,

On Wed, Mar 01, 2017 at 04:59:51AM +0900, Taeung Song wrote:
> Currently perf-annotate show wrong line numbers.
> 
> For example,
> Actual source code is as below
> 
> ...
>   21 };
>   22
>   23 unsigned int limited_wgt;
>   24
>   25 unsigned int get_cond_maxprice(int wgt)
>   26 {
> ...
> 
> However, the output of perf-annotate is as below.
> 
>   4   Disassembly of section .text:
> 
>   6   0000000000400966 <get_cond_maxprice>:
>   7   get_cond_maxprice():
>   26  };
> 
>   28  unsigned int limited_wgt;
> 
>   30  unsigned int get_cond_maxprice(int wgt)
>   31  {
> 
> The cause is the wrong way counting line numbers
> in symbol__parse_objdump_line().
> So remove wrong current code counting line number and
> use other method for it using functions related to addr2line
> instead of the output of '-l' of objdump.

Hmm.. do you think it's a bug of objdump or it's perf failing to parse
the line number correctly?  I'd like to see the output of `objdump -l`


> 
> However, despite the correct line numbers,
> we can't show proper source code view
> because of limitations from output of 'objdump -S'.
> 
> So, next commit will resolve the limitations from 'objdump -S'
> with the new source code view.

It seems not related with this commit..

Thanks,
Namhyung

> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/util/annotate.c | 59 +++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 273f21f..42b752e 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -19,14 +19,12 @@
>  #include "evsel.h"
>  #include "block-range.h"
>  #include "arch/common.h"
> -#include <regex.h>
>  #include <pthread.h>
>  #include <linux/bitops.h>
>  #include <sys/utsname.h>
>  
>  const char 	*disassembler_style;
>  const char	*objdump_path;
> -static regex_t	 file_lineno;
>  
>  static struct ins_ops *ins__find(struct arch *arch, const char *name);
>  static void ins__sort(struct arch *arch);
> @@ -814,7 +812,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>  }
>  
>  static struct disasm_line *disasm_line__new(s64 offset, char *line,
> -					    size_t privsize, int line_nr,
> +					    size_t privsize,
>  					    struct arch *arch,
>  					    struct map *map)
>  {
> @@ -823,7 +821,6 @@ static struct disasm_line *disasm_line__new(s64 offset, char *line,
>  	if (dl != NULL) {
>  		dl->offset = offset;
>  		dl->line = strdup(line);
> -		dl->line_nr = line_nr;
>  		if (dl->line == NULL)
>  			goto out_delete;
>  
> @@ -1121,6 +1118,29 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  	return 0;
>  }
>  
> +static int parse_srcline(char *srcline, char **path, int *line_nr)
> +{
> +	char *sep;
> +
> +	if (path != NULL)
> +		*path = NULL;
> +
> +	if (!strcmp(srcline, SRCLINE_UNKNOWN))
> +		return -1;
> +
> +	sep = strchr(srcline, ':');
> +	if (sep) {
> +		*sep = '\0';
> +		if (path != NULL)
> +			*path = srcline;
> +		if (line_nr != NULL)
> +			*line_nr = strtoul(++sep, NULL, 0);
> +	} else
> +		return -1;
> +
> +	return 0;
> +}
> +
>  /*
>   * symbol__parse_objdump_line() parses objdump output (with -d --no-show-raw)
>   * which looks like following
> @@ -1143,15 +1163,14 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>   */
>  static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  				      struct arch *arch,
> -				      FILE *file, size_t privsize,
> -				      int *line_nr)
> +				      FILE *file, size_t privsize)
>  {
>  	struct annotation *notes = symbol__annotation(sym);
>  	struct disasm_line *dl;
>  	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
>  	size_t line_len;
>  	s64 line_ip, offset = -1;
> -	regmatch_t match[2];
> +	bool has_src_code = true;
>  
>  	if (getline(&line, &line_len, file) < 0)
>  		return -1;
> @@ -1169,12 +1188,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  	line_ip = -1;
>  	parsed_line = line;
>  
> -	/* /filename:linenr ? Save line number and ignore. */
> -	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
> -		*line_nr = atoi(line + match[1].rm_so);
> -		return 0;
> -	}
> -
>  	/*
>  	 * Strip leading spaces:
>  	 */
> @@ -1205,13 +1218,18 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  			parsed_line = tmp2 + 1;
>  	}
>  
> -	dl = disasm_line__new(offset, parsed_line, privsize, *line_nr, arch, map);
> +	dl = disasm_line__new(offset, parsed_line, privsize, arch, map);
>  	free(line);
> -	(*line_nr)++;
>  
>  	if (dl == NULL)
>  		return -1;
>  
> +	if (has_src_code && dl->offset != -1 && map->dso->symsrc_filename) {
> +		if (parse_srcline(get_srcline(map->dso, line_ip, NULL, false),
> +				  NULL, &dl->line_nr) < 0)
> +			has_src_code = false;
> +	}
> +
>  	if (!disasm_line__has_offset(dl)) {
>  		dl->ops.target.offset = dl->ops.target.addr -
>  					map__rip_2objdump(map, sym->start);
> @@ -1235,11 +1253,6 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  	return 0;
>  }
>  
> -static __attribute__((constructor)) void symbol__init_regexpr(void)
> -{
> -	regcomp(&file_lineno, "^/[^:]+:([0-9]+)", REG_EXTENDED);
> -}
> -
>  static void delete_last_nop(struct symbol *sym)
>  {
>  	struct annotation *notes = symbol__annotation(sym);
> @@ -1360,7 +1373,6 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  	struct kcore_extract kce;
>  	bool delete_extract = false;
>  	int stdout_fd[2];
> -	int lineno = 0;
>  	int nline;
>  	pid_t pid;
>  	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
> @@ -1435,7 +1447,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  	snprintf(command, sizeof(command),
>  		 "%s %s%s --start-address=0x%016" PRIx64
>  		 " --stop-address=0x%016" PRIx64
> -		 " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
> +		 " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
>  		 objdump_path ? objdump_path : "objdump",
>  		 disassembler_style ? "-M " : "",
>  		 disassembler_style ? disassembler_style : "",
> @@ -1482,8 +1494,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  
>  	nline = 0;
>  	while (!feof(file)) {
> -		if (symbol__parse_objdump_line(sym, map, arch, file, privsize,
> -			    &lineno) < 0)
> +		if (symbol__parse_objdump_line(sym, map, arch, file, privsize) < 0)
>  			break;
>  		nline++;
>  	}
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Namhyung Kim]

On Wed, Mar 01, 2017 at 04:59:52AM +0900, Taeung Song wrote:
> The current source code view using 'objdump -S'
> has several limitations regarding line numbers of each soure
> code line and confusing mixed soure code & diassembly view.
> 
> So not use the '-S' option any more and
> add the new reable source code view with
> correct line numbers and source code per each symbol(function)
> using new struct source_code.

I like this "source-only" annotation, but some people still might want
to see the "mixed" annotation.  So I think you need to keep it and
provide another option for the source-only mode.  And then we can
change the default later.

> 
> Before:
> 
>     $ perf annotate --stdio -s pack_knapsack
>  Percent |      Source code & Disassembly of old for cycles:ppp (82 samples)
> ----------------------------------------------------------------------------
> ...
>          :      void pack_knapsack(struct jewelry *jewelry)
>          :      {
>     0.00 :        40089e:       push   %rbp
>     0.00 :        40089f:       mov    %rsp,%rbp
>     0.00 :        4008a2:       sub    $0x18,%rsp
>     0.00 :        4008a6:       mov    %rdi,-0x18(%rbp)
>          :               * price per limited weight.
>          :               */
>          :              int wgt;
>          :              unsigned int maxprice;
>          :
>          :              if (limited_wgt < jewelry->wgt)
>     0.00 :        4008aa:       mov    -0x18(%rbp),%rax
> ...
> 
> After:
> 
>     $ perf annotate --stdio -s pack_knapsack
>  Percent |      Source code of old_pack_knapsack.c for cycles:ppp (82 samples)
> ------------------------------------------------------------------------------
>     0.00 : 43           return maxprice;
>     0.00 : 44   }
>     0.00 : 45
>     0.00 : 46   void pack_knapsack(struct jewelry *jewelry)
>     0.00 : 47   {
>     0.00 : 48           /* Case by case pack knapsack following maximum
>     0.00 : 49            * price per limited weight.
>     0.00 : 50            */
>     0.00 : 51           int wgt;
>     0.00 : 52           unsigned int maxprice;
>     0.00 : 53
>     0.00 : 54           if (limited_wgt < jewelry->wgt)
>     0.00 : 55                   return;
>     0.00 : 56
>    52.44 : 57           for (wgt = 0; wgt <= limited_wgt; wgt++) {
>     9.76 : 58                   if (jewelry->wgt <= wgt) {
>    25.61 : 59                           maxprice = get_cond_maxprice(wgt, jewelry);
>     0.00 : 60
>    12.20 : 61                           if (knapsack_list[wgt].maxprice < maxprice)
>     0.00 : 62                                   knapsack_list[wgt].maxprice = maxprice;
>     0.00 : 63                   }
>     0.00 : 64           }
>     0.00 : 65   }
> 
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-annotate.c     |   2 +-
>  tools/perf/ui/browsers/annotate.c |   5 -
>  tools/perf/util/annotate.c        | 235 +++++++++++++++++++++++++++++++++++++-
>  tools/perf/util/annotate.h        |  27 +++++
>  4 files changed, 257 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 4f52d85..5ecc73c 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -431,7 +431,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
>  		     "Look for files with symbols relative to this directory",
>  		     symbol__config_symfs),
>  	OPT_BOOLEAN(0, "source", &symbol_conf.annotate_src,
> -		    "Interleave source code with assembly code (default)"),
> +		    "Display source code for each symbol (default)"),
>  	OPT_BOOLEAN(0, "asm-raw", &symbol_conf.annotate_asm_raw,
>  		    "Display raw encoding of assembly instructions (default)"),
>  	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index ba36aac..03b2012 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -11,11 +11,6 @@
>  #include "../../util/config.h"
>  #include <pthread.h>
>  
> -struct disasm_line_samples {
> -	double		percent;
> -	u64		nr;
> -};
> -
>  #define IPC_WIDTH 6
>  #define CYCLES_WIDTH 6
>  
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 42b752e..d7826d3 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1363,6 +1363,203 @@ static const char *annotate__norm_arch(const char *arch_name)
>  	return normalize_arch((char *)arch_name);
>  }
>  
> +bool symbol__has_source_code(struct symbol *sym, struct map *map)
> +{
> +	u64 start = map__rip_2objdump(map, sym->start);
> +
> +	if (map->dso->symsrc_filename &&
> +	    parse_srcline(get_srcline(map->dso, start, NULL, false),
> +			  NULL, NULL) != -1)
> +		return true;
> +
> +	return false;
> +}
> +
> +int symbol__free_source_code(struct symbol *sym)
> +{
> +	struct annotation *notes = symbol__annotation(sym);
> +	struct source_code *code = notes->src->code;
> +	struct code_line *pos, *tmp;
> +
> +	if (code == NULL)
> +		return -1;
> +
> +	list_for_each_entry_safe(pos, tmp, &code->lines, node) {
> +		list_del_init(&pos->node);
> +		zfree(&pos->line);
> +		zfree(&pos->samples_sum);

No need to free 'pos' itself?

> +	}
> +	zfree(&code->path);
> +	zfree(&notes->src->code);
> +	return 0;
> +}
> +
> +static void source_code__print(struct code_line *cl, int nr_events)
> +{
> +	int i;
> +	const char *color;
> +	double percent, max_percent = 0.0;
> +
> +	for (i = 0; i < nr_events; i++) {
> +		percent = cl->samples_sum[i].percent;
> +		color = get_percent_color(percent);
> +		if (max_percent < percent)
> +			max_percent = percent;
> +
> +		if (symbol_conf.show_total_period)
> +			color_fprintf(stdout, color, " %7" PRIu64,
> +				      cl->samples_sum[i].nr);
> +		else
> +			color_fprintf(stdout, color, " %7.2f", percent);
> +	}
> +	color = get_percent_color(max_percent);
> +	color_fprintf(stdout, color, " : %d	%s\n",
> +		      cl->line_nr, cl->line);
> +}
> +
> +static int source_code__collect(struct source_code *code, char *path,
> +				int start_linenr, int end_linenr)
> +{
> +	FILE *file;
> +	int ret = -1, linenr = 0;
> +	char *line = NULL, *c, *parsed_line;
> +	size_t len;
> +	struct code_line *cl;
> +
> +	if (access(path, R_OK) != 0)
> +		return -1;
> +
> +	file = fopen(path, "r");
> +	if (!file)
> +		return -1;
> +
> +	if (srcline_full_filename)
> +		code->path = strdup(path);
> +	else
> +		code->path = strdup(basename(path));
> +
> +	INIT_LIST_HEAD(&code->lines);
> +	while (!feof(file)) {
> +		if (getline(&line, &len, file) < 0)
> +			break;
> +
> +		if (++linenr < start_linenr)
> +			continue;
> +
> +		parsed_line = rtrim(line);
> +		c = strchr(parsed_line, '\n');
> +		if (c)
> +			*c = '\0';

I guess rtrim() already removed the newline, no?

> +
> +		cl = zalloc(sizeof(*cl));
> +		if (!cl)
> +			break;
> +
> +		cl->line_nr = linenr;
> +		cl->line = strdup(parsed_line);
> +		cl->samples_sum =
> +			zalloc(sizeof(struct disasm_line_samples) * code->nr_events);
> +		if (!cl->samples_sum)
> +			break;
> +
> +		list_add_tail(&cl->node, &code->lines);
> +		if (linenr == end_linenr) {
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	free(line);
> +	fclose(file);
> +	return ret;
> +}
> +
> +int symbol__get_source_code(struct symbol *sym, struct map *map,
> +			    struct perf_evsel *evsel)
> +{
> +	struct annotation *notes = symbol__annotation(sym);
> +	struct source_code *code;
> +	int start_linenr, end_linenr, ret = 0;
> +	char *path, *start_srcline, *end_srcline;
> +	u64 start = map__rip_2objdump(map, sym->start);
> +	u64 end = map__rip_2objdump(map, sym->end - 1);
> +	bool bef_fullpath = srcline_full_filename;
> +
> +	srcline_full_filename = true;
> +	start_srcline = get_srcline(map->dso, start, NULL, false);
> +	end_srcline = get_srcline(map->dso, end, NULL, false);
> +	srcline_full_filename = bef_fullpath;
> +
> +	if (parse_srcline(start_srcline, &path, &start_linenr) < 0)
> +		return -1;
> +	if (parse_srcline(end_srcline, &path, &end_linenr) < 0)
> +		return -1;
> +
> +	code = zalloc(sizeof(struct source_code));
> +	if (code == NULL)
> +		return -1;

Will leak {start,end}_srcline.

> +
> +	if (perf_evsel__is_group_event(evsel))
> +		code->nr_events = evsel->nr_members;
> +	else
> +		code->nr_events = 1;
> +
> +	/* To read a function header for the sym */
> +	if (start_linenr > 4)
> +		start_linenr -= 4;
> +	else
> +		start_linenr = 1;
> +
> +	if (source_code__collect(code, path, start_linenr, end_linenr) < 0) {
> +		zfree(&code);
> +		ret = -1;
> +	}
> +	notes->src->code = code;
> +
> +	free_srcline(start_srcline);
> +	free_srcline(end_srcline);
> +	return ret;
> +}
> +
> +static struct code_line *source_code__find_line(struct list_head *lines, int linenr)
> +{
> +	struct code_line *pos;
> +
> +	list_for_each_entry(pos, lines, node) {
> +		if (pos->line_nr == linenr)
> +			return pos;
> +	}
> +	return NULL;
> +}
> +
> +void source_code__set_samples(struct disasm_line *dl, struct annotation *notes,
> +			      struct perf_evsel *evsel)
> +{
> +	int i;
> +	double percent;
> +	u64 nr_samples;
> +	struct sym_hist *h;
> +	struct source_code *code;
> +	struct code_line *cl;
> +
> +	code = notes->src->code;
> +	for (i = 0; i < code->nr_events; i++) {
> +		h = annotation__histogram(notes, evsel->idx + i);
> +		nr_samples = h->addr[dl->offset];
> +
> +		if (nr_samples == 0)
> +			percent = 0.0;
> +		else
> +			percent = 100.0 * nr_samples / h->sum;
> +
> +		cl = source_code__find_line(&code->lines, dl->line_nr);
> +		if (cl == NULL)
> +			continue;
> +		cl->samples_sum[i].percent += percent;
> +		cl->samples_sum[i].nr += nr_samples;
> +	}
> +}
> +
>  int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_name, size_t privsize)
>  {
>  	struct dso *dso = map->dso;
> @@ -1447,14 +1644,13 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  	snprintf(command, sizeof(command),
>  		 "%s %s%s --start-address=0x%016" PRIx64
>  		 " --stop-address=0x%016" PRIx64
> -		 " -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
> +		 " -d %s -C %s 2>/dev/null|grep -v %s|expand",
>  		 objdump_path ? objdump_path : "objdump",
>  		 disassembler_style ? "-M " : "",
>  		 disassembler_style ? disassembler_style : "",
>  		 map__rip_2objdump(map, sym->start),
>  		 map__rip_2objdump(map, sym->end),
>  		 symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
> -		 symbol_conf.annotate_src ? "-S" : "",
>  		 symfs_filename, symfs_filename);
>  
>  	pr_debug("Executing: %s\n", command);
> @@ -1501,7 +1697,6 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na
>  
>  	if (nline == 0)
>  		pr_err("No output from %s\n", command);
> -
>  	/*
>  	 * kallsyms does not have symbol sizes so there may a nop at the end.
>  	 * Remove it.
> @@ -1753,6 +1948,7 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  	struct sym_hist *h = annotation__histogram(notes, evsel->idx);
>  	struct disasm_line *pos, *queue = NULL;
>  	u64 start = map__rip_2objdump(map, sym->start);
> +	bool has_src_code = false;
>  	int printed = 2, queue_len = 0;
>  	int more = 0;
>  	u64 len;
> @@ -1773,8 +1969,14 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  	if (perf_evsel__is_group_event(evsel))
>  		width *= evsel->nr_members;
>  
> -	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
> -	       width, width, "Percent", d_filename, evsel_name, h->sum);
> +	if (symbol_conf.annotate_src && symbol__has_source_code(sym, map))
> +		has_src_code = true;
> +
> +	graph_dotted_len = printf(" %-*.*s|	%s of %s for %s (%" PRIu64 " samples)\n",
> +				  width, width, "Percent",
> +				  has_src_code ? "Source code" : "Disassembly",
> +				  has_src_code ? notes->src->code->path : d_filename,
> +				  evsel_name, h->sum);
>  
>  	printf("%-*.*s----\n",
>  	       graph_dotted_len, graph_dotted_len, graph_dotted_line);
> @@ -1782,6 +1984,22 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  	if (verbose > 0)
>  		symbol__annotate_hits(sym, evsel);
>  
> +	if (has_src_code) {
> +		struct source_code *code = notes->src->code;
> +		struct code_line *cl;
> +
> +		list_for_each_entry(pos, &notes->src->source, node) {
> +			if (pos->offset == -1)
> +				continue;
> +			source_code__set_samples(pos, notes, evsel);
> +		}
> +
> +		list_for_each_entry(cl, &code->lines, node)
> +			source_code__print(cl, code->nr_events);
> +
> +		goto out;
> +	}
> +
>  	list_for_each_entry(pos, &notes->src->source, node) {
>  		if (context && queue == NULL) {
>  			queue = pos;
> @@ -1818,7 +2036,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  			break;
>  		}
>  	}
> -
> +out:
> +	printf("\n");
>  	free(filename);
>  
>  	return more;
> @@ -1902,11 +2121,15 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
>  		print_summary(&source_line, dso->long_name);
>  	}
>  
> +	if (symbol_conf.annotate_src && symbol__has_source_code(sym, map))
> +		symbol__get_source_code(sym, map, evsel);

What if it fails?

Thanks,
Namhyung


> +
>  	symbol__annotate_printf(sym, map, evsel, full_paths,
>  				min_pcnt, max_lines, 0);
>  	if (print_lines)
>  		symbol__free_source_line(sym, len);
>  
> +	symbol__free_source_code(sym);
>  	disasm__purge(&symbol__annotation(sym)->src->source);
>  
>  	return 0;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 09776b5..6375012 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -56,6 +56,11 @@ int ins__scnprintf(struct ins *ins, char *bf, size_t size, struct ins_operands *
>  
>  struct annotation;
>  
> +struct disasm_line_samples {
> +	double		percent;
> +	u64		nr;
> +};
> +
>  struct disasm_line {
>  	struct list_head    node;
>  	s64		    offset;
> @@ -95,6 +100,22 @@ struct cyc_hist {
>  	u16	reset;
>  };
>  
> +struct code_line {
> +	struct list_head    node;
> +	int		    line_nr;
> +	char		    *line;
> +	struct disasm_line_samples *samples_sum;
> +};
> +
> +struct source_code {
> +	char		 *path;
> +	int		 nr_events;
> +	struct list_head lines;
> +};
> +
> +void source_code__set_samples(struct disasm_line *dl, struct annotation *notes,
> +			      struct perf_evsel *evsel);
> +
>  struct source_line_samples {
>  	double		percent;
>  	double		percent_sum;
> @@ -123,6 +144,7 @@ struct source_line {
>   */
>  struct annotated_source {
>  	struct list_head   source;
> +	struct source_code *code;
>  	struct source_line *lines;
>  	int    		   nr_histograms;
>  	size_t		   sizeof_sym_hist;
> @@ -158,6 +180,11 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr);
>  int symbol__alloc_hist(struct symbol *sym);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
>  
> +bool symbol__has_source_code(struct symbol *sym, struct map *map);
> +int symbol__free_source_code(struct symbol *sym);
> +int symbol__get_source_code(struct symbol *sym, struct map *map,
> +			    struct perf_evsel *evsel);
> +
>  int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_name, size_t privsize);
>  
>  enum symbol_disassemble_errno {
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Peter Zijlstra]

On Wed, Mar 01, 2017 at 10:58:05PM +0900, Namhyung Kim wrote:
> On Wed, Mar 01, 2017 at 04:59:52AM +0900, Taeung Song wrote:
> > The current source code view using 'objdump -S'
> > has several limitations regarding line numbers of each soure
> > code line and confusing mixed soure code & diassembly view.
> > 
> > So not use the '-S' option any more and
> > add the new reable source code view with
> > correct line numbers and source code per each symbol(function)
> > using new struct source_code.
> 
> I like this "source-only" annotation, but some people still might want
> to see the "mixed" annotation.  So I think you need to keep it and
> provide another option for the source-only mode.  And then we can
> change the default later.

What's the point of source-only? You can't even see what it is that is
expensive.

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Namhyung Kim]

Hi Peter,

On Wed, Mar 01, 2017 at 03:08:33PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 10:58:05PM +0900, Namhyung Kim wrote:
> > On Wed, Mar 01, 2017 at 04:59:52AM +0900, Taeung Song wrote:
> > > The current source code view using 'objdump -S'
> > > has several limitations regarding line numbers of each soure
> > > code line and confusing mixed soure code & diassembly view.
> > > 
> > > So not use the '-S' option any more and
> > > add the new reable source code view with
> > > correct line numbers and source code per each symbol(function)
> > > using new struct source_code.
> > 
> > I like this "source-only" annotation, but some people still might want
> > to see the "mixed" annotation.  So I think you need to keep it and
> > provide another option for the source-only mode.  And then we can
> > change the default later.
> 
> What's the point of source-only? You can't even see what it is that is
> expensive.

???

This is to show source code + overhead (for each line).  People can
see which line of their source is expensive.  I think it's way more
intuitive for most developers..

Thanks,
Namhyung

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Peter Zijlstra]

On Wed, Mar 01, 2017 at 11:21:33PM +0900, Namhyung Kim wrote:

> > What's the point of source-only? You can't even see what it is that is
> > expensive.
> 
> ???

>From a line like:

		a = b ? ptr->c : 0;

How do you tell what it is that causes the problem, the branch miss or
the pointer deref?

Typically looking at the asm this is fairly clear; even without
recording with more specific events.

> This is to show source code + overhead (for each line).  People can
> see which line of their source is expensive.  I think it's way more
> intuitive for most developers..

And how pray are you going to write better code if you have no clue what
the problem is?

I'm really wondering how you can even think about performance if you're
scared of asm.

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Namhyung Kim]

On Wed, Mar 01, 2017 at 03:30:11PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 11:21:33PM +0900, Namhyung Kim wrote:
> 
> > > What's the point of source-only? You can't even see what it is that is
> > > expensive.
> > 
> > ???
> 
> From a line like:
> 
> 		a = b ? ptr->c : 0;
> 
> How do you tell what it is that causes the problem, the branch miss or
> the pointer deref?

I can't.  But I think it's a problem of granularity.  If someone wants
to see the cause, he/she will need to dig into the asm.  But before
that, looking at the source level can give a hint or clue for the
problem IMHO.

> 
> Typically looking at the asm this is fairly clear; even without
> recording with more specific events.

But I think there're many developers don't see asm code at least
usually.  And even for someone who is familiar with the asm, it's
easier to see the source code to try to identify the problem IMHO.


> 
> > This is to show source code + overhead (for each line).  People can
> > see which line of their source is expensive.  I think it's way more
> > intuitive for most developers..
> 
> And how pray are you going to write better code if you have no clue what
> the problem is?
> 
> I'm really wondering how you can even think about performance if you're
> scared of asm.

I'm not saying I'm scared of asm. :)

For extreme performance, we need to see the asm.  But I guess
sometimes it's enough to see it in the source level.

It's a kind of user experience issue.  We provide the asm-only and
asm+source annotation, and I think it'd be nice to add source-only
option.  And I remember that it was requested some time ago..

Thanks,
Namhyung

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Peter Zijlstra]

On Wed, Mar 01, 2017 at 11:56:39PM +0900, Namhyung Kim wrote:

> It's a kind of user experience issue.  We provide the asm-only and
> asm+source annotation, and I think it'd be nice to add source-only
> option.  And I remember that it was requested some time ago..

Thing is, an optimizing compiler -- that same beast that ensures your
objdump -S output is such a garbled mess -- can generate code that
becomes very hard to relate to the original source code.

I'm really sceptical the source line only view is very useful; maybe if
you build with -O0, but then, if you do that you're not bothered with
performance.

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Taeung Song]

Hi, Peter

On 03/02/2017 12:07 AM, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 11:56:39PM +0900, Namhyung Kim wrote:
>
>> It's a kind of user experience issue.  We provide the asm-only and
>> asm+source annotation, and I think it'd be nice to add source-only
>> option.  And I remember that it was requested some time ago..
>
> Thing is, an optimizing compiler -- that same beast that ensures your
> objdump -S output is such a garbled mess -- can generate code that
> becomes very hard to relate to the original source code.
>
> I'm really sceptical the source line only view is very useful; maybe if
> you build with -O0, but then, if you do that you're not bothered with
> performance.
>

I think there is a fundamental difference between the two points of view.

   1) Based on assembly code view, we can show parts of actual source code.
   2) Based on source code view, we can show parts of assembly code for 
a particular source code line.

I think current compilers have some limitations about the optimization.
what they can't optimize is the logic or process.
So, many developers should optimize the logic or process.
And to do that, I think we need to the source code view with overhead.

And I think we can show asm lines for a particular in the source view,
when the user press ENTER key at a source code line.

For example,
If a current line is line 47 and
the user press ENTER key, we can show asm per the source code line.

            │46   void pack_knapsack(struct jewelry *jewelry)
            │47   {
            │      push   %rbp
            │      mov    %rsp,%rbp
            │      sub    $0x18,%rsp
            │      mov    %rdi,-0x18(%rbp)
            │48      /* Case by case pack knapsack following maximum
...

And I think we don't need to only show actual source code.
But we can show assembly code based on source code view.

Thanks,
Taeung

Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

[Taeung Song]

On 03/01/2017 10:17 PM, Namhyung Kim wrote:
> Hi Taeung,
>
> On Wed, Mar 01, 2017 at 04:59:51AM +0900, Taeung Song wrote:
>> Currently perf-annotate show wrong line numbers.
>>
>> For example,
>> Actual source code is as below
>>
>> ...
>>   21 };
>>   22
>>   23 unsigned int limited_wgt;
>>   24
>>   25 unsigned int get_cond_maxprice(int wgt)
>>   26 {
>> ...
>>
>> However, the output of perf-annotate is as below.
>>
>>   4   Disassembly of section .text:
>>
>>   6   0000000000400966 <get_cond_maxprice>:
>>   7   get_cond_maxprice():
>>   26  };
>>
>>   28  unsigned int limited_wgt;
>>
>>   30  unsigned int get_cond_maxprice(int wgt)
>>   31  {
>>
>> The cause is the wrong way counting line numbers
>> in symbol__parse_objdump_line().
>> So remove wrong current code counting line number and
>> use other method for it using functions related to addr2line
>> instead of the output of '-l' of objdump.
>
> Hmm.. do you think it's a bug of objdump or it's perf failing to parse
> the line number correctly?  I'd like to see the output of `objdump -l`
>

Both are ok.
'objdump -l' hasn't a bug related to line number
and perf's method parsing the line number is ok.

But symbol__parse_objdump_line() wrongly count line numbers
after parsing it as below.

1172         /* /filename:linenr ? Save line number and ignore. */
1173         if (regexec(&file_lineno, line, 2, match, 0) == 0) {
1174                 *line_nr = atoi(line + match[1].rm_so);
1175                 return 0;
1176         }
...
1208         dl = disasm_line__new(offset, parsed_line, privsize, 
*line_nr, arch, map);
1209         free(line);
1210         (*line_nr)++;

Increasing line_nr each asm line is wrong method.
Because 'line_nr' means actual source code line number.

Sure, I can fix only the wrong counting way.
But the above parsing method(1172~1176) is never used because of 'grep -v'
in command as below.
(the grep already remove lines containing filename:linenr of output)

1435         snprintf(command, sizeof(command),
1436                  "%s %s%s --start-address=0x%016" PRIx64
1437                  " --stop-address=0x%016" PRIx64
1438                  " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
1439                  objdump_path ? objdump_path : "objdump",
1440                  disassembler_style ? "-M " : "",
1441                  disassembler_style ? disassembler_style : "",
1442                  map__rip_2objdump(map, sym->start),
1443                  map__rip_2objdump(map, sym->end),
1444                  symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
1445                  symbol_conf.annotate_src ? "-S" : "",
1446                  symfs_filename, symfs_filename);

Therefore, I think it is better to do three things

   1) fix the wrong counting line number problem
   2) remove unused the line number parsing method
   3) In addtion, a bit reduce objdump dependency
      using functions related to addr2line of perf.

What do you think about that ?
Is it bad idea ?

>>
>> However, despite the correct line numbers,
>> we can't show proper source code view
>> because of limitations from output of 'objdump -S'.
>>
>> So, next commit will resolve the limitations from 'objdump -S'
>> with the new source code view.
>
> It seems not related with this commit..
>

Okey, will remove the mention.

Thanks,
Taeung

Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

[Namhyung Kim]

+ Andi Kleen who wrote the code.

On Thu, Mar 02, 2017 at 03:05:14PM +0900, Taeung Song wrote:
> 
> 
> On 03/01/2017 10:17 PM, Namhyung Kim wrote:
> > Hi Taeung,
> > 
> > On Wed, Mar 01, 2017 at 04:59:51AM +0900, Taeung Song wrote:
> > > Currently perf-annotate show wrong line numbers.
> > > 
> > > For example,
> > > Actual source code is as below
> > > 
> > > ...
> > >   21 };
> > >   22
> > >   23 unsigned int limited_wgt;
> > >   24
> > >   25 unsigned int get_cond_maxprice(int wgt)
> > >   26 {
> > > ...
> > > 
> > > However, the output of perf-annotate is as below.
> > > 
> > >   4   Disassembly of section .text:
> > > 
> > >   6   0000000000400966 <get_cond_maxprice>:
> > >   7   get_cond_maxprice():
> > >   26  };
> > > 
> > >   28  unsigned int limited_wgt;
> > > 
> > >   30  unsigned int get_cond_maxprice(int wgt)
> > >   31  {
> > > 
> > > The cause is the wrong way counting line numbers
> > > in symbol__parse_objdump_line().
> > > So remove wrong current code counting line number and
> > > use other method for it using functions related to addr2line
> > > instead of the output of '-l' of objdump.
> > 
> > Hmm.. do you think it's a bug of objdump or it's perf failing to parse
> > the line number correctly?  I'd like to see the output of `objdump -l`
> > 
> 
> Both are ok.
> 'objdump -l' hasn't a bug related to line number
> and perf's method parsing the line number is ok.
> 
> But symbol__parse_objdump_line() wrongly count line numbers
> after parsing it as below.
> 
> 1172         /* /filename:linenr ? Save line number and ignore. */
> 1173         if (regexec(&file_lineno, line, 2, match, 0) == 0) {
> 1174                 *line_nr = atoi(line + match[1].rm_so);
> 1175                 return 0;
> 1176         }
> ...
> 1208         dl = disasm_line__new(offset, parsed_line, privsize, *line_nr,
> arch, map);
> 1209         free(line);
> 1210         (*line_nr)++;
> 
> Increasing line_nr each asm line is wrong method.
> Because 'line_nr' means actual source code line number.

Hmm.. ok.  It looks like that it should reuse the old line_nr as is.

> 
> Sure, I can fix only the wrong counting way.
> But the above parsing method(1172~1176) is never used because of 'grep -v'
> in command as below.
> (the grep already remove lines containing filename:linenr of output)

Right, but only if filename is same as binary name.

> 
> 1435         snprintf(command, sizeof(command),
> 1436                  "%s %s%s --start-address=0x%016" PRIx64
> 1437                  " --stop-address=0x%016" PRIx64
> 1438                  " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
> 1439                  objdump_path ? objdump_path : "objdump",
> 1440                  disassembler_style ? "-M " : "",
> 1441                  disassembler_style ? disassembler_style : "",
> 1442                  map__rip_2objdump(map, sym->start),
> 1443                  map__rip_2objdump(map, sym->end),
> 1444                  symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
> 1445                  symbol_conf.annotate_src ? "-S" : "",
> 1446                  symfs_filename, symfs_filename);
> 
> Therefore, I think it is better to do three things
> 
>   1) fix the wrong counting line number problem
>   2) remove unused the line number parsing method
>   3) In addtion, a bit reduce objdump dependency
>      using functions related to addr2line of perf.
> 
> What do you think about that ?
> Is it bad idea ?

I think we need to fix 1) definitely, but not sure about 2) and 3).
If objdump could do all necessary works, why not use it? :)

Thanks,
Namhyung


> 
> > > 
> > > However, despite the correct line numbers,
> > > we can't show proper source code view
> > > because of limitations from output of 'objdump -S'.
> > > 
> > > So, next commit will resolve the limitations from 'objdump -S'
> > > with the new source code view.
> > 
> > It seems not related with this commit..
> > 
> 
> Okey, will remove the mention.
> 
> Thanks,
> Taeung

Re: [PATCH v2 1/3] perf annotate: Get correct line numbers matched with addr

[Taeung Song]

On 03/03/2017 11:40 AM, Namhyung Kim wrote:
> + Andi Kleen who wrote the code.
>
> On Thu, Mar 02, 2017 at 03:05:14PM +0900, Taeung Song wrote:
>>
>>
>> On 03/01/2017 10:17 PM, Namhyung Kim wrote:
>>> Hi Taeung,
>>>
>>> On Wed, Mar 01, 2017 at 04:59:51AM +0900, Taeung Song wrote:
>>>> Currently perf-annotate show wrong line numbers.
>>>>
>>>> For example,
>>>> Actual source code is as below
>>>>
>>>> ...
>>>>   21 };
>>>>   22
>>>>   23 unsigned int limited_wgt;
>>>>   24
>>>>   25 unsigned int get_cond_maxprice(int wgt)
>>>>   26 {
>>>> ...
>>>>
>>>> However, the output of perf-annotate is as below.
>>>>
>>>>   4   Disassembly of section .text:
>>>>
>>>>   6   0000000000400966 <get_cond_maxprice>:
>>>>   7   get_cond_maxprice():
>>>>   26  };
>>>>
>>>>   28  unsigned int limited_wgt;
>>>>
>>>>   30  unsigned int get_cond_maxprice(int wgt)
>>>>   31  {
>>>>
>>>> The cause is the wrong way counting line numbers
>>>> in symbol__parse_objdump_line().
>>>> So remove wrong current code counting line number and
>>>> use other method for it using functions related to addr2line
>>>> instead of the output of '-l' of objdump.
>>>
>>> Hmm.. do you think it's a bug of objdump or it's perf failing to parse
>>> the line number correctly?  I'd like to see the output of `objdump -l`
>>>
>>
>> Both are ok.
>> 'objdump -l' hasn't a bug related to line number
>> and perf's method parsing the line number is ok.
>>
>> But symbol__parse_objdump_line() wrongly count line numbers
>> after parsing it as below.
>>
>> 1172         /* /filename:linenr ? Save line number and ignore. */
>> 1173         if (regexec(&file_lineno, line, 2, match, 0) == 0) {
>> 1174                 *line_nr = atoi(line + match[1].rm_so);
>> 1175                 return 0;
>> 1176         }
>> ...
>> 1208         dl = disasm_line__new(offset, parsed_line, privsize, *line_nr,
>> arch, map);
>> 1209         free(line);
>> 1210         (*line_nr)++;
>>
>> Increasing line_nr each asm line is wrong method.
>> Because 'line_nr' means actual source code line number.
>
> Hmm.. ok.  It looks like that it should reuse the old line_nr as is.
>
>>
>> Sure, I can fix only the wrong counting way.
>> But the above parsing method(1172~1176) is never used because of 'grep -v'
>> in command as below.
>> (the grep already remove lines containing filename:linenr of output)
>
> Right, but only if filename is same as binary name.
>
>>
>> 1435         snprintf(command, sizeof(command),
>> 1436                  "%s %s%s --start-address=0x%016" PRIx64
>> 1437                  " --stop-address=0x%016" PRIx64
>> 1438                  " -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand",
>> 1439                  objdump_path ? objdump_path : "objdump",
>> 1440                  disassembler_style ? "-M " : "",
>> 1441                  disassembler_style ? disassembler_style : "",
>> 1442                  map__rip_2objdump(map, sym->start),
>> 1443                  map__rip_2objdump(map, sym->end),
>> 1444                  symbol_conf.annotate_asm_raw ? "" : "--no-show-raw",
>> 1445                  symbol_conf.annotate_src ? "-S" : "",
>> 1446                  symfs_filename, symfs_filename);
>>
>> Therefore, I think it is better to do three things
>>
>>   1) fix the wrong counting line number problem
>>   2) remove unused the line number parsing method
>>   3) In addtion, a bit reduce objdump dependency
>>      using functions related to addr2line of perf.
>>
>> What do you think about that ?
>> Is it bad idea ?
>
> I think we need to fix 1) definitely, but not sure about 2) and 3).
> If objdump could do all necessary works, why not use it? :)
>
> Thanks,
> Namhyung
>
>

Okey! I'll concentrate on fixing 1)
,not removing objdump -l :)

Thanks,
Taeung

Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view

[Namhyung Kim]

Hi Peter,

On Wed, Mar 01, 2017 at 04:07:46PM +0100, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 11:56:39PM +0900, Namhyung Kim wrote:
> 
> > It's a kind of user experience issue.  We provide the asm-only and
> > asm+source annotation, and I think it'd be nice to add source-only
> > option.  And I remember that it was requested some time ago..
> 
> Thing is, an optimizing compiler -- that same beast that ensures your
> objdump -S output is such a garbled mess -- can generate code that
> becomes very hard to relate to the original source code.

I understand that.  Maybe it's not 100% accurate, but it still has
valuable information.  And I think the source-only view can give more
readable outputs using the info.  Also I guess many developers already
aware of the effect of optimizing compilers.

> 
> I'm really sceptical the source line only view is very useful; maybe if
> you build with -O0, but then, if you do that you're not bothered with
> performance.

Even with an optimizing compiler, it can be helpful to overview which
parts of the code are bottlenecks IMHO.  After that, one can see the
asm to identify the problem deeply, if needed.

Thanks,
Namhyung