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
>
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
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.
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.
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
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.
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
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