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 <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Taeung Song <[email protected]>
> ---
> 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(¬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
>
