Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754908AbcJELTm (ORCPT ); Wed, 5 Oct 2016 07:19:42 -0400 Received: from mail.kernel.org ([198.145.29.136]:46170 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752328AbcJELTk (ORCPT ); Wed, 5 Oct 2016 07:19:40 -0400 Date: Wed, 5 Oct 2016 08:19:29 -0300 From: Arnaldo Carvalho de Melo To: Ravi Bangoria Cc: linux-kernel@vger.kernel.org, kim.phillips@arm.com, linuxppc-dev@lists.ozlabs.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, treeze.taeung@gmail.com, naveen.n.rao@linux.vnet.ibm.com, markus@trippelsdorf.de, namhyung@kernel.org, pawel.moll@arm.com, chris.ryder@arm.com, jolsa@kernel.org, mhiramat@kernel.org Subject: Re: [PATCH v7 1/6] perf annotate: Add cross arch annotate support Message-ID: <20161005111928.GQ7143@kernel.org> References: <1474472876-2706-1-git-send-email-ravi.bangoria@linux.vnet.ibm.com> <1474472876-2706-2-git-send-email-ravi.bangoria@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1474472876-2706-2-git-send-email-ravi.bangoria@linux.vnet.ibm.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12064 Lines: 333 Em Wed, Sep 21, 2016 at 09:17:51PM +0530, Ravi Bangoria escreveu: > Change current data structures and function to enable cross arch > annotate. > > Current perf implementation does not support cross arch annotate. > To make it truly cross arch, instruction table of all arch should > be present in perf binary. And use appropriate table based on arch > where perf.data was recorded. > > Record on arm: > $ ./perf record -a > > Report -> Annotate on x86: > $ ./perf report -i perf.data.arm --vmlinux vmlinux.arm > > Signed-off-by: Ravi Bangoria > --- > Changes in v7: > - Make norm_arch as global var instead of passing them to each parser. > - Address other review comments. > > tools/perf/builtin-top.c | 2 +- > tools/perf/ui/browsers/annotate.c | 3 +- > tools/perf/ui/gtk/annotate.c | 2 +- > tools/perf/util/annotate.c | 151 ++++++++++++++++++++++++++++++++------ > tools/perf/util/annotate.h | 3 +- > 5 files changed, 134 insertions(+), 27 deletions(-) > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index 4007857..41ecdd6 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -129,7 +129,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he) > return err; > } > > - err = symbol__disassemble(sym, map, 0); > + err = symbol__disassemble(sym, map, 0, NULL); > if (err == 0) { > out_assign: > top->sym_filter_entry = he; > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c > index 4c18271..214a14a 100644 > --- a/tools/perf/ui/browsers/annotate.c > +++ b/tools/perf/ui/browsers/annotate.c > @@ -1050,7 +1050,8 @@ int symbol__tui_annotate(struct symbol *sym, struct map *map, > (nr_pcnt - 1); > } > > - err = symbol__disassemble(sym, map, sizeof_bdl); > + err = symbol__disassemble(sym, map, sizeof_bdl, > + perf_evsel__env_arch(evsel)); > if (err) { > char msg[BUFSIZ]; > symbol__strerror_disassemble(sym, map, err, msg, sizeof(msg)); > diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c > index 42d3199..c127aba 100644 > --- a/tools/perf/ui/gtk/annotate.c > +++ b/tools/perf/ui/gtk/annotate.c > @@ -167,7 +167,7 @@ static int symbol__gtk_annotate(struct symbol *sym, struct map *map, > if (map->dso->annotate_warned) > return -1; > > - err = symbol__disassemble(sym, map, 0); > + err = symbol__disassemble(sym, map, 0, perf_evsel__env_arch(evsel)); > if (err) { > char msg[BUFSIZ]; > symbol__strerror_disassemble(sym, map, err, msg, sizeof(msg)); > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index aeb5a44..816aa2c 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -21,10 +21,13 @@ > #include > #include > #include > +#include > +#include "../arch/common.h" > > const char *disassembler_style; > const char *objdump_path; > static regex_t file_lineno; > +static char *norm_arch; > > static struct ins *ins__find(const char *name); > static int disasm_line__parse(char *line, char **namep, char **rawp); > @@ -66,10 +69,8 @@ static int call__parse(struct ins_operands *ops, struct map *map) > > name++; > > -#ifdef __arm__ > - if (strchr(name, '+')) > + if (!strcmp(norm_arch, "arm") && strchr(name, '+')) > return -1; > -#endif > > tok = strchr(name, '>'); > if (tok == NULL) > @@ -252,16 +253,12 @@ static int mov__parse(struct ins_operands *ops, struct map *map __maybe_unused) > return -1; > > target = ++s; > -#ifdef __arm__ > + > comment = strchr(s, ';'); > -#else > - comment = strchr(s, '#'); > -#endif > + if (comment == NULL) > + comment = strchr(s, '#'); > > - if (comment != NULL) > - s = comment - 1; > - else > - s = strchr(s, '\0') - 1; > + s = (comment != NULL) ? comment - 1 : strchr(s, '\0') - 1; Why have you touched the above 4 lines? The code you provided is equivalent, i.e. has no value for this patch you're working on, just a distraction for reviewers, please don't do that. I'll remove it and continue processing the patchkit. > > while (s > target && isspace(s[0])) > --s; > @@ -364,14 +361,92 @@ bool ins__is_ret(const struct ins *ins) > return ins->ops == &ret_ops; > } > > -static struct ins instructions[] = { > +static struct ins instructions_x86[] = { > { .name = "add", .ops = &mov_ops, }, > { .name = "addl", .ops = &mov_ops, }, > { .name = "addq", .ops = &mov_ops, }, > { .name = "addw", .ops = &mov_ops, }, > { .name = "and", .ops = &mov_ops, }, > -#ifdef __arm__ > - { .name = "b", .ops = &jump_ops, }, // might also be a call > + { .name = "bts", .ops = &mov_ops, }, > + { .name = "call", .ops = &call_ops, }, > + { .name = "callq", .ops = &call_ops, }, > + { .name = "cmp", .ops = &mov_ops, }, > + { .name = "cmpb", .ops = &mov_ops, }, > + { .name = "cmpl", .ops = &mov_ops, }, > + { .name = "cmpq", .ops = &mov_ops, }, > + { .name = "cmpw", .ops = &mov_ops, }, > + { .name = "cmpxch", .ops = &mov_ops, }, > + { .name = "dec", .ops = &dec_ops, }, > + { .name = "decl", .ops = &dec_ops, }, > + { .name = "imul", .ops = &mov_ops, }, > + { .name = "inc", .ops = &dec_ops, }, > + { .name = "incl", .ops = &dec_ops, }, > + { .name = "ja", .ops = &jump_ops, }, > + { .name = "jae", .ops = &jump_ops, }, > + { .name = "jb", .ops = &jump_ops, }, > + { .name = "jbe", .ops = &jump_ops, }, > + { .name = "jc", .ops = &jump_ops, }, > + { .name = "jcxz", .ops = &jump_ops, }, > + { .name = "je", .ops = &jump_ops, }, > + { .name = "jecxz", .ops = &jump_ops, }, > + { .name = "jg", .ops = &jump_ops, }, > + { .name = "jge", .ops = &jump_ops, }, > + { .name = "jl", .ops = &jump_ops, }, > + { .name = "jle", .ops = &jump_ops, }, > + { .name = "jmp", .ops = &jump_ops, }, > + { .name = "jmpq", .ops = &jump_ops, }, > + { .name = "jna", .ops = &jump_ops, }, > + { .name = "jnae", .ops = &jump_ops, }, > + { .name = "jnb", .ops = &jump_ops, }, > + { .name = "jnbe", .ops = &jump_ops, }, > + { .name = "jnc", .ops = &jump_ops, }, > + { .name = "jne", .ops = &jump_ops, }, > + { .name = "jng", .ops = &jump_ops, }, > + { .name = "jnge", .ops = &jump_ops, }, > + { .name = "jnl", .ops = &jump_ops, }, > + { .name = "jnle", .ops = &jump_ops, }, > + { .name = "jno", .ops = &jump_ops, }, > + { .name = "jnp", .ops = &jump_ops, }, > + { .name = "jns", .ops = &jump_ops, }, > + { .name = "jnz", .ops = &jump_ops, }, > + { .name = "jo", .ops = &jump_ops, }, > + { .name = "jp", .ops = &jump_ops, }, > + { .name = "jpe", .ops = &jump_ops, }, > + { .name = "jpo", .ops = &jump_ops, }, > + { .name = "jrcxz", .ops = &jump_ops, }, > + { .name = "js", .ops = &jump_ops, }, > + { .name = "jz", .ops = &jump_ops, }, > + { .name = "lea", .ops = &mov_ops, }, > + { .name = "lock", .ops = &lock_ops, }, > + { .name = "mov", .ops = &mov_ops, }, > + { .name = "movb", .ops = &mov_ops, }, > + { .name = "movdqa", .ops = &mov_ops, }, > + { .name = "movl", .ops = &mov_ops, }, > + { .name = "movq", .ops = &mov_ops, }, > + { .name = "movslq", .ops = &mov_ops, }, > + { .name = "movzbl", .ops = &mov_ops, }, > + { .name = "movzwl", .ops = &mov_ops, }, > + { .name = "nop", .ops = &nop_ops, }, > + { .name = "nopl", .ops = &nop_ops, }, > + { .name = "nopw", .ops = &nop_ops, }, > + { .name = "or", .ops = &mov_ops, }, > + { .name = "orl", .ops = &mov_ops, }, > + { .name = "test", .ops = &mov_ops, }, > + { .name = "testb", .ops = &mov_ops, }, > + { .name = "testl", .ops = &mov_ops, }, > + { .name = "xadd", .ops = &mov_ops, }, > + { .name = "xbeginl", .ops = &jump_ops, }, > + { .name = "xbeginq", .ops = &jump_ops, }, > + { .name = "retq", .ops = &ret_ops, }, > +}; > + > +static struct ins instructions_arm[] = { > + { .name = "add", .ops = &mov_ops, }, > + { .name = "addl", .ops = &mov_ops, }, > + { .name = "addq", .ops = &mov_ops, }, > + { .name = "addw", .ops = &mov_ops, }, > + { .name = "and", .ops = &mov_ops, }, > + { .name = "b", .ops = &jump_ops, }, /* might also be a call */ > { .name = "bcc", .ops = &jump_ops, }, > { .name = "bcs", .ops = &jump_ops, }, > { .name = "beq", .ops = &jump_ops, }, > @@ -383,7 +458,6 @@ static struct ins instructions[] = { > { .name = "blt", .ops = &jump_ops, }, > { .name = "blx", .ops = &call_ops, }, > { .name = "bne", .ops = &jump_ops, }, > -#endif > { .name = "bts", .ops = &mov_ops, }, > { .name = "call", .ops = &call_ops, }, > { .name = "callq", .ops = &call_ops, }, > @@ -472,24 +546,48 @@ static int ins__cmp(const void *a, const void *b) > return strcmp(ia->name, ib->name); > } > > -static void ins__sort(void) > +static void ins__sort(struct ins *instructions, int nmemb) > { > - const int nmemb = ARRAY_SIZE(instructions); > - > qsort(instructions, nmemb, sizeof(struct ins), ins__cmp); > } > > +static const char *annotate__norm_arch(char *arch) > +{ > + struct utsname uts; > + > + if (!arch) { /* Assume we are annotating locally. */ > + if (uname(&uts) < 0) > + return NULL; > + arch = uts.machine; > + } > + return normalize_arch(arch); > +} > + > static struct ins *ins__find(const char *name) > { > - const int nmemb = ARRAY_SIZE(instructions); > static bool sorted; > + struct ins *instructions; > + int nmemb; > > if (!sorted) { > - ins__sort(); > + ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86)); > + ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm)); > sorted = true; > } > > - return bsearch(name, instructions, nmemb, sizeof(struct ins), ins__key_cmp); > + if (!strcmp(norm_arch, "x86")) { > + instructions = instructions_x86; > + nmemb = ARRAY_SIZE(instructions_x86); > + } else if (!strcmp(norm_arch, "arm")) { > + instructions = instructions_arm; > + nmemb = ARRAY_SIZE(instructions_arm); > + } else { > + pr_err("perf annotate not supported by %s arch\n", norm_arch); > + return NULL; > + } > + > + return bsearch(name, instructions, nmemb, sizeof(struct ins), > + ins__key_cmp); > } > > int symbol__alloc_hist(struct symbol *sym) > @@ -1280,7 +1378,8 @@ fallback: > return 0; > } > > -int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize) > +int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize, > + char *arch) > { > struct dso *dso = map->dso; > char command[PATH_MAX * 2]; > @@ -1297,6 +1396,12 @@ int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize) > if (err) > return err; > > + norm_arch = (char *) annotate__norm_arch(arch); > + if (!norm_arch) { > + pr_err("Can not annotate. Could not determine architecture."); > + return -1; > + } > + > pr_debug("%s: filename=%s, sym=%s, start=%#" PRIx64 ", end=%#" PRIx64 "\n", __func__, > symfs_filename, sym->name, map->unmap_ip(map, sym->start), > map->unmap_ip(map, sym->end)); > @@ -1793,7 +1898,7 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map, > struct rb_root source_line = RB_ROOT; > u64 len; > > - if (symbol__disassemble(sym, map, 0) < 0) > + if (symbol__disassemble(sym, map, 0, perf_evsel__env_arch(evsel)) < 0) > return -1; > > len = symbol__size(sym); > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h > index 5bbcec1..4400269 100644 > --- a/tools/perf/util/annotate.h > +++ b/tools/perf/util/annotate.h > @@ -156,7 +156,8 @@ 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); > > -int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize); > +int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize, > + char *arch); > > enum symbol_disassemble_errno { > SYMBOL_ANNOTATE_ERRNO__SUCCESS = 0, > -- > 2.5.5