Re: [PATCH v7 1/6] perf annotate: Add cross arch annotate support

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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 <ravi.bangoria@linux.vnet.ibm.com>
> ---
> 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 <regex.h>
>  #include <pthread.h>
>  #include <linux/bitops.h>
> +#include <sys/utsname.h>
> +#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