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@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Signed-off-by: Taeung Song <treeze.taeung@xxxxxxxxx>
> ---
>  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
>