Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751508AbdCAN6k (ORCPT ); Wed, 1 Mar 2017 08:58:40 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:34840 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751009AbdCAN6h (ORCPT ); Wed, 1 Mar 2017 08:58:37 -0500 Date: Wed, 1 Mar 2017 22:58:05 +0900 From: Namhyung Kim To: Taeung Song Cc: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Jiri Olsa , Ingo Molnar , Peter Zijlstra , Wang Nan , Masami Hiramatsu , Jiri Olsa Subject: Re: [PATCH v2 2/3] perf annotate: Introduce the new source code view Message-ID: <20170301135805.GB2690@danjae.aot.lge.com> References: <1488311993-15124-1-git-send-email-treeze.taeung@gmail.com> <1488311993-15124-3-git-send-email-treeze.taeung@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1488311993-15124-3-git-send-email-treeze.taeung@gmail.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15363 Lines: 489 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 > Cc: Jiri Olsa > Signed-off-by: Taeung Song > --- > 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 > > -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(¬es->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, ¬es->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, ¬es->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 >