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

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(-)

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

Hi Arnaldo,

Sorry for little late replies, I was off last week.

Please find my comments.

On Wednesday 05 October 2016 04:49 PM, Arnaldo Carvalho de Melo wrote:
> 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.
...
>>  	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.

Sorry about that. I did this change to make code more compact but
yes, you are right, that should be done as separate patch.

-Ravi