[PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Ravi Bangoria]

Currently Perf annotate support code navigation (branches and calls)
only when run on the same architecture where perf.data was recorded.
But, for example, record on powerpc server and annotate on client's
x86 desktop is not supported.

This patchset adds supports for that.

Example:

  Record on powerpc:
  $ ./perf record -a

  Report -> Annotate on x86:
  $ ./perf report -i perf.data.powerpc --vmlinux vmlinux.powerpc

Changes in v7:
  - Using string for normalized arch names instread of macros.(i.e.
    removed patch 1/7 of v6)
  - In patch 1/6, make norm_arch as global var instead of passing them
    to each parser.
  - In patch 1/6 and 6/6, little bit change in initializing instruction
    list.
  - patch 4/7 of v6 is already accepted. Removed that in v7.
  - Address other review comments.
  - Added more examples in patch descriptions.

v6 link:
  https://lkml.org/lkml/2016/8/19/411

Kim, I don't have arm test machine. Can you please help me to test
this on arm.


Kim Phillips (1):
  perf annotate: cross arch annotate support fixes for ARM

Naveen N. Rao (1):
  perf annotate: Add support for powerpc

Ravi Bangoria (4):
  perf annotate: Add cross arch annotate support
  perf annotate: Show raw form for jump instruction with indirect target
  perf annotate: Support jump instruction with target as second operand
  perf annotate: Fix jump target outside of function address range

 tools/perf/builtin-top.c          |   2 +-
 tools/perf/ui/browsers/annotate.c |   8 +-
 tools/perf/ui/gtk/annotate.c      |   2 +-
 tools/perf/util/annotate.c        | 259 ++++++++++++++++++++++++++++++++------
 tools/perf/util/annotate.h        |   8 +-
 5 files changed, 232 insertions(+), 47 deletions(-)

-- 
2.5.5

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

[Ravi Bangoria]

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

[PATCH v7 2/6] perf annotate: Add support for powerpc

[Ravi Bangoria]

From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>

Current perf can disassemble annotated function but it does not have
parsing logic for powerpc instructions. So all navigation options are
not available for powerpc.

Apart from that, Powerpc has long list of branch instructions and
hardcoding them in table appears to be error-prone. So, add function
to find instruction instead of creating table. This function dynamically
create table (list of 'struct ins'), and instead of creating object
every time, first check if list already contain object for that
instruction.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v7:
  - Little bit change in initializing instruction list.

 tools/perf/util/annotate.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 816aa2c..5aa72d9 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -531,6 +531,11 @@ static struct ins instructions_arm[] = {
 	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
+struct instructions_powerpc {
+	struct ins *ins;
+	struct list_head list;
+};
+
 static int ins__key_cmp(const void *name, const void *insp)
 {
 	const struct ins *ins = insp;
@@ -546,6 +551,111 @@ static int ins__cmp(const void *a, const void *b)
 	return strcmp(ia->name, ib->name);
 }
 
+static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
+					 const char *name, struct ins_ops *ops)
+{
+	struct instructions_powerpc *ins_powerpc;
+	struct ins *ins;
+
+	ins = zalloc(sizeof(struct ins));
+	if (!ins)
+		return NULL;
+
+	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
+	if (!ins_powerpc)
+		goto out_free_ins;
+
+	ins->name = strdup(name);
+	if (!ins->name)
+		goto out_free_ins_power;
+
+	ins->ops = ops;
+	ins_powerpc->ins = ins;
+	list_add_tail(&(ins_powerpc->list), &(head->list));
+
+	return ins;
+
+out_free_ins_power:
+	zfree(&ins_powerpc);
+out_free_ins:
+	zfree(&ins);
+	return NULL;
+}
+
+static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
+					    const char *name)
+{
+	struct instructions_powerpc *pos;
+
+	list_for_each_entry(pos, &head->list, list) {
+		if (!strcmp(pos->ins->name, name))
+			return pos->ins;
+	}
+	return NULL;
+}
+
+static struct ins *ins__find_powerpc(const char *name)
+{
+	int i;
+	struct ins *ins;
+	struct ins_ops *ops;
+	static struct instructions_powerpc head = {
+		.list = LIST_HEAD_INIT(head.list),
+	};
+
+	/*
+	 * - Interested only if instruction starts with 'b'.
+	 * - Few start with 'b', but aren't branch instructions.
+	 */
+	if (name[0] != 'b'             ||
+	    !strncmp(name, "bcd", 3)   ||
+	    !strncmp(name, "brinc", 5) ||
+	    !strncmp(name, "bper", 4))
+		return NULL;
+
+	/*
+	 * Return if we already have object of 'struct ins' for this instruction
+	 */
+	ins = list_search__ins_powerpc(&head, name);
+	if (ins)
+		return ins;
+
+	ops = &jump_ops;
+
+	i = strlen(name) - 1;
+	if (i < 0)
+		return NULL;
+
+	/* ignore optional hints at the end of the instructions */
+	if (name[i] == '+' || name[i] == '-')
+		i--;
+
+	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
+		/*
+		 * if the instruction ends up with 'l' or 'la', then
+		 * those are considered 'calls' since they update LR.
+		 * ... except for 'bnl' which is branch if not less than
+		 * and the absolute form of the same.
+		 */
+		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
+		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
+		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
+			ops = &call_ops;
+	}
+	if (name[i] == 'r' && name[i-1] == 'l')
+		/*
+		 * instructions ending with 'lr' are considered to be
+		 * return instructions
+		 */
+		ops = &ret_ops;
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_powerpc(&head, name, ops);
+}
+
 static void ins__sort(struct ins *instructions, int nmemb)
 {
 	qsort(instructions, nmemb, sizeof(struct ins), ins__cmp);
@@ -581,6 +691,8 @@ static struct ins *ins__find(const char *name)
 	} else if (!strcmp(norm_arch, "arm")) {
 		instructions = instructions_arm;
 		nmemb = ARRAY_SIZE(instructions_arm);
+	} else if (!strcmp(norm_arch, "powerpc")) {
+		return ins__find_powerpc(name);
 	} else {
 		pr_err("perf annotate not supported by %s arch\n", norm_arch);
 		return NULL;
-- 
2.5.5

[PATCH v7 3/6] perf annotate: Show raw form for jump instruction with indirect target

[Ravi Bangoria]

For jump instructions that does not include target address as direct
operand, use raw value for that. This is needed for certain powerpc
jump instructions that use target address in a register (such as bctr,
btar, ...).

Before:
     ld     r12,32088(r12)
     mtctr  r12
  v  bctr   ffffffffffffca2c
     std    r2,24(r1)
     addis  r12,r2,-1

After:
     ld     r12,32088(r12)
     mtctr  r12
  v  bctr
     std    r2,24(r1)
     addis  r12,r2,-1

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v7:
  - Added example in description

 tools/perf/util/annotate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 5aa72d9..1ccf26a 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -136,6 +136,9 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
 static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
 			   struct ins_operands *ops)
 {
+	if (!ops->target.addr)
+		return ins__raw_scnprintf(ins, bf, size, ops);
+
 	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
 }
 
-- 
2.5.5

[PATCH v7 4/6] perf annotate: Support jump instruction with target as second operand

[Ravi Bangoria]

Current perf is not able to parse jump instruction when second operand
contains target address. Arch like powerpc has such instructions. For
example, 'bne  cr7,0xc0000000000f6154'.

objdump o/p:
  c0000000000f6140:   ld     r9,1032(r31)
  c0000000000f6144:   cmpdi  cr7,r9,0
  c0000000000f6148:   bne    cr7,0xc0000000000f6154
  c0000000000f614c:   ld     r9,2312(r30)
  c0000000000f6150:   std    r9,1032(r31)
  c0000000000f6154:   ld     r9,88(r31)

Before patch:
         ld     r9,1032(r31)
         cmpdi  cr7,r9,0
      v  bne    3ffffffffff09f2c
         ld     r9,2312(r30)
         std    r9,1032(r31)
  74:    ld     r9,88(r31)

After patch:
         ld     r9,1032(r31)
         cmpdi  cr7,r9,0
      v  bne    74
         ld     r9,2312(r30)
         std    r9,1032(r31)
  74:    ld     r9,88(r31)

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v7:
  - Added example in description

 tools/perf/util/annotate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 1ccf26a..a9dbac1 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -122,8 +122,12 @@ bool ins__is_call(const struct ins *ins)
 static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
 {
 	const char *s = strchr(ops->raw, '+');
+	const char *c = strchr(ops->raw, ',');
 
-	ops->target.addr = strtoull(ops->raw, NULL, 16);
+	if (c++ != NULL)
+		ops->target.addr = strtoull(c, NULL, 16);
+	else
+		ops->target.addr = strtoull(ops->raw, NULL, 16);
 
 	if (s++ != NULL)
 		ops->target.offset = strtoull(s, NULL, 16);
-- 
2.5.5

[PATCH v7 5/6] perf annotate: Fix jump target outside of function address range

[Ravi Bangoria]

If jump target is outside of function range, perf is not handling it
correctly. Especially when target address is lesser than function start
address, target offset will be negative. But, target address declared
to be unsigned, converts negative number into 2's complement. See below
example. Here target of 'jumpq' instruction at 34cf8 is 34ac0 which is
lesser than function start address(34cf0).

        34ac0 - 34cf0 = -0x230 = 0xfffffffffffffdd0

Objdump output:

  0000000000034cf0 <__sigaction>:
  __GI___sigaction():
    34cf0: lea    -0x20(%rdi),%eax
    34cf3: cmp    -bashx1,%eax
    34cf6: jbe    34d00 <__sigaction+0x10>
    34cf8: jmpq   34ac0 <__GI___libc_sigaction>
    34cfd: nopl   (%rax)
    34d00: mov    0x386161(%rip),%rax        # 3bae68 <_DYNAMIC+0x2e8>
    34d07: movl   -bashx16,%fs:(%rax)
    34d0e: mov    -bashxffffffff,%eax
    34d13: retq

perf annotate before applying patch:

  __GI___sigaction  /usr/lib64/libc-2.22.so
           lea    -0x20(%rdi),%eax
           cmp    -bashx1,%eax
        v  jbe    10
        v  jmpq   fffffffffffffdd0
           nop
    10:    mov    _DYNAMIC+0x2e8,%rax
           movl   -bashx16,%fs:(%rax)
           mov    -bashxffffffff,%eax
           retq

perf annotate after applying patch:

  __GI___sigaction  /usr/lib64/libc-2.22.so
           lea    -0x20(%rdi),%eax
           cmp    -bashx1,%eax
        v  jbe    10
        ^  jmpq   34ac0 <__GI___libc_sigaction>
           nop
    10:    mov    _DYNAMIC+0x2e8,%rax
           movl   -bashx16,%fs:(%rax)
           mov    -bashxffffffff,%eax
           retq

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v7:
  - No changes

 tools/perf/ui/browsers/annotate.c |  5 +++--
 tools/perf/util/annotate.c        | 14 +++++++++-----
 tools/perf/util/annotate.h        |  5 +++--
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 214a14a..2d04bdf 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -215,7 +215,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
 			ui_browser__set_color(browser, color);
 		if (dl->ins && dl->ins->ops->scnprintf) {
 			if (ins__is_jump(dl->ins)) {
-				bool fwd = dl->ops.target.offset > (u64)dl->offset;
+				bool fwd = dl->ops.target.offset > dl->offset;
 
 				ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR :
 								    SLSMG_UARROW_CHAR);
@@ -245,7 +245,8 @@ static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sy
 {
 	if (!dl || !dl->ins || !ins__is_jump(dl->ins)
 	    || !disasm_line__has_offset(dl)
-	    || dl->ops.target.offset >= symbol__size(sym))
+	    || dl->ops.target.offset < 0
+	    || dl->ops.target.offset >= (s64)symbol__size(sym))
 		return false;
 
 	return true;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a9dbac1..fc44dd1 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -129,10 +129,12 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
 	else
 		ops->target.addr = strtoull(ops->raw, NULL, 16);
 
-	if (s++ != NULL)
+	if (s++ != NULL) {
 		ops->target.offset = strtoull(s, NULL, 16);
-	else
-		ops->target.offset = UINT64_MAX;
+		ops->target.offset_avail = true;
+	} else {
+		ops->target.offset_avail = false;
+	}
 
 	return 0;
 }
@@ -140,7 +142,7 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
 static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
 			   struct ins_operands *ops)
 {
-	if (!ops->target.addr)
+	if (!ops->target.addr || ops->target.offset < 0)
 		return ins__raw_scnprintf(ins, bf, size, ops);
 
 	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
@@ -1373,9 +1375,11 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	if (dl == NULL)
 		return -1;
 
-	if (dl->ops.target.offset == UINT64_MAX)
+	if (!disasm_line__has_offset(dl)) {
 		dl->ops.target.offset = dl->ops.target.addr -
 					map__rip_2objdump(map, sym->start);
+		dl->ops.target.offset_avail = true;
+	}
 
 	/* kcore has no symbols, so add the call target name */
 	if (dl->ins && ins__is_call(dl->ins) && !dl->ops.target.name) {
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 4400269..7ba3579 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -19,7 +19,8 @@ struct ins_operands {
 		char	*raw;
 		char	*name;
 		u64	addr;
-		u64	offset;
+		s64	offset;
+		bool    offset_avail;
 	} target;
 	union {
 		struct {
@@ -67,7 +68,7 @@ struct disasm_line {
 
 static inline bool disasm_line__has_offset(const struct disasm_line *dl)
 {
-	return dl->ops.target.offset != UINT64_MAX;
+	return dl->ops.target.offset_avail;
 }
 
 void disasm_line__free(struct disasm_line *dl);
-- 
2.5.5

[PATCH v7 6/6] perf annotate: cross arch annotate support fixes for ARM

[Ravi Bangoria]

From: Kim Phillips <kim.phillips@arm.com>

For ARM we remove the list that contains non-arm insns, and
instead add more maintainable branch instruction regex logic.

Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v7:
  - Little bit change in initializing instruction list.

 tools/perf/util/annotate.c | 177 +++++++++++++++++----------------------------
 1 file changed, 65 insertions(+), 112 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index fc44dd1..83d5ac8 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -28,6 +28,7 @@ const char 	*disassembler_style;
 const char	*objdump_path;
 static regex_t	 file_lineno;
 static char	*norm_arch;
+static regex_t	arm_call_insn, arm_jump_insn;
 
 static struct ins *ins__find(const char *name);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
@@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
 	{ .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, },
-	{ .name = "bge",   .ops  = &jump_ops, },
-	{ .name = "bgt",   .ops  = &jump_ops, },
-	{ .name = "bhi",   .ops  = &jump_ops, },
-	{ .name = "bl",    .ops  = &call_ops, },
-	{ .name = "bls",   .ops  = &jump_ops, },
-	{ .name = "blt",   .ops  = &jump_ops, },
-	{ .name = "blx",   .ops  = &call_ops, },
-	{ .name = "bne",   .ops  = &jump_ops, },
-	{ .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, },
-};
-
-struct instructions_powerpc {
+struct instructions_arch {
 	struct ins *ins;
 	struct list_head list;
 };
@@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
 	return strcmp(ia->name, ib->name);
 }
 
-static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
-					 const char *name, struct ins_ops *ops)
+static struct ins *list_add__ins_arch(struct instructions_arch *head,
+				      const char *name, struct ins_ops *ops)
 {
-	struct instructions_powerpc *ins_powerpc;
+	struct instructions_arch *ins_arch;
 	struct ins *ins;
 
 	ins = zalloc(sizeof(struct ins));
 	if (!ins)
 		return NULL;
 
-	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
-	if (!ins_powerpc)
+	ins_arch = zalloc(sizeof(struct instructions_arch));
+	if (!ins_arch)
 		goto out_free_ins;
 
 	ins->name = strdup(name);
 	if (!ins->name)
-		goto out_free_ins_power;
+		goto out_free_ins_arch;
 
 	ins->ops = ops;
-	ins_powerpc->ins = ins;
-	list_add_tail(&(ins_powerpc->list), &(head->list));
+	ins_arch->ins = ins;
+	list_add_tail(&(ins_arch->list), &(head->list));
 
 	return ins;
 
-out_free_ins_power:
-	zfree(&ins_powerpc);
+out_free_ins_arch:
+	zfree(&ins_arch);
 out_free_ins:
 	zfree(&ins);
 	return NULL;
 }
 
-static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
-					    const char *name)
+static struct ins *list_search__ins_arch(struct instructions_arch *head,
+					 const char *name)
 {
-	struct instructions_powerpc *pos;
+	struct instructions_arch *pos;
 
 	list_for_each_entry(pos, &head->list, list) {
 		if (!strcmp(pos->ins->name, name))
@@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	int i;
 	struct ins *ins;
 	struct ins_ops *ops;
-	static struct instructions_powerpc head = {
+	static struct instructions_arch head = {
 		.list = LIST_HEAD_INIT(head.list),
 	};
 
@@ -625,7 +535,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	/*
 	 * Return if we already have object of 'struct ins' for this instruction
 	 */
-	ins = list_search__ins_powerpc(&head, name);
+	ins = list_search__ins_arch(&head, name);
 	if (ins)
 		return ins;
 
@@ -662,7 +572,40 @@ static struct ins *ins__find_powerpc(const char *name)
 	 * Add instruction to list so next time no need to
 	 * allocate memory for it.
 	 */
-	return list_add__ins_powerpc(&head, name, ops);
+	return list_add__ins_arch(&head, name, ops);
+}
+
+static struct ins *ins__find_arm(const char *name)
+{
+	struct ins *ins;
+	struct ins_ops *ops = &mov_ops;
+	regmatch_t match[2];
+	int ret;
+	static struct instructions_arch head = {
+		.list = LIST_HEAD_INIT(head.list),
+	};
+
+	/*
+	 * Return if we already have object of 'struct ins' for this instruction
+	 */
+	ins = list_search__ins_arch(&head, name);
+	if (ins)
+		return ins;
+
+	ret = regexec(&arm_call_insn, name, 2, match, 0);
+	if (!ret) {
+		ops = &call_ops;
+	} else {
+		ret = regexec(&arm_jump_insn, name, 2, match, 0);
+		if (!ret)
+			ops = &jump_ops;
+	}
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_arch(&head, name, ops);
 }
 
 static void ins__sort(struct ins *instructions, int nmemb)
@@ -682,15 +625,26 @@ static const char *annotate__norm_arch(char *arch)
 	return normalize_arch(arch);
 }
 
+#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
+
 static struct ins *ins__find(const char *name)
 {
 	static bool sorted;
 	struct ins *instructions;
-	int nmemb;
+	int nmemb, ret;
 
 	if (!sorted) {
 		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
-		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
+		if (!strcmp(norm_arch, "arm")) {
+			ret = regcomp(&arm_call_insn,
+				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
+			ret |= regcomp(&arm_jump_insn,
+				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
+			if (ret) {
+				pr_err("regcomp failed\n");
+				return NULL;
+			}
+		}
 		sorted = true;
 	}
 
@@ -698,8 +652,7 @@ static struct ins *ins__find(const char *name)
 		instructions = instructions_x86;
 		nmemb = ARRAY_SIZE(instructions_x86);
 	} else if (!strcmp(norm_arch, "arm")) {
-		instructions = instructions_arm;
-		nmemb = ARRAY_SIZE(instructions_arm);
+		return ins__find_arm(name);
 	} else if (!strcmp(norm_arch, "powerpc")) {
 		return ins__find_powerpc(name);
 	} else {
-- 
2.5.5

Re: [PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Kim Phillips]

On Wed, 21 Sep 2016 21:17:50 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Kim, I don't have arm test machine. Can you please help me to test
> this on arm.

This works for me:  hitting return on return instructions yields
"Invalid jump offset", but I'll get that later.

Thanks,

Kim

Re: [PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Ravi Bangoria]

On Thursday 22 September 2016 01:04 AM, Kim Phillips wrote:
> On Wed, 21 Sep 2016 21:17:50 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Kim, I don't have arm test machine. Can you please help me to test
>> this on arm.
> This works for me:  hitting return on return instructions yields
> "Invalid jump offset", but I'll get that later.

Thanks Kim.

Hmm.. so, ins__find_arm does not contain logic for return instructions. Navigation
with return instruction is working fine for x86 and powerpc.

-Ravi

> Thanks,
>
> Kim
>

Re: [PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Kim Phillips]

On Thu, 22 Sep 2016 10:48:13 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> On Thursday 22 September 2016 01:04 AM, Kim Phillips wrote:
> > On Wed, 21 Sep 2016 21:17:50 +0530
> > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >
> >> Kim, I don't have arm test machine. Can you please help me to test
> >> this on arm.
> > This works for me:  hitting return on return instructions yields
> > "Invalid jump offset", but I'll get that later.
> 
> Thanks Kim.
> 
> Hmm.. so, ins__find_arm does not contain logic for return instructions. Navigation
> with return instruction is working fine for x86 and powerpc.

Right, for ARM, in order to match return instructions, 'lr' must be
found to be the operand of the branch instruction, which is not
contained in the 'name' variable passed to ins__find().

I don't want this to inhibit acceptance of this series, however: I plan
on addressing it afterwards, unless, of course, it is perceived as a
problem somehow (compatibility-wise, it is not because it is an error
at the moment).

Thanks,

Kim

Re: [PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Ravi Bangoria]

Hello,

Any updates?

Arnaldo, if patches looks good to you, can you please pickup them.

-Ravi

On Wednesday 21 September 2016 09:17 PM, Ravi Bangoria wrote:
> Currently Perf annotate support code navigation (branches and calls)
> only when run on the same architecture where perf.data was recorded.
> But, for example, record on powerpc server and annotate on client's
> x86 desktop is not supported.
>
> This patchset adds supports for that.
>
> Example:
>
>   Record on powerpc:
>   $ ./perf record -a
>
>   Report -> Annotate on x86:
>   $ ./perf report -i perf.data.powerpc --vmlinux vmlinux.powerpc
>
> Changes in v7:
>   - Using string for normalized arch names instread of macros.(i.e.
>     removed patch 1/7 of v6)
>   - In patch 1/6, make norm_arch as global var instead of passing them
>     to each parser.
>   - In patch 1/6 and 6/6, little bit change in initializing instruction
>     list.
>   - patch 4/7 of v6 is already accepted. Removed that in v7.
>   - Address other review comments.
>   - Added more examples in patch descriptions.
>
> v6 link:
>   https://lkml.org/lkml/2016/8/19/411
>
> Kim, I don't have arm test machine. Can you please help me to test
> this on arm.
>
>
> Kim Phillips (1):
>   perf annotate: cross arch annotate support fixes for ARM
>
> Naveen N. Rao (1):
>   perf annotate: Add support for powerpc
>
> Ravi Bangoria (4):
>   perf annotate: Add cross arch annotate support
>   perf annotate: Show raw form for jump instruction with indirect target
>   perf annotate: Support jump instruction with target as second operand
>   perf annotate: Fix jump target outside of function address range
>
>  tools/perf/builtin-top.c          |   2 +-
>  tools/perf/ui/browsers/annotate.c |   8 +-
>  tools/perf/ui/gtk/annotate.c      |   2 +-
>  tools/perf/util/annotate.c        | 259 ++++++++++++++++++++++++++++++++------
>  tools/perf/util/annotate.h        |   8 +-
>  5 files changed, 232 insertions(+), 47 deletions(-)
>

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

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v7 2/6] perf annotate: Add support for powerpc

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2016 at 09:17:52PM +0530, Ravi Bangoria escreveu:
> From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> 
> Current perf can disassemble annotated function but it does not have
> parsing logic for powerpc instructions. So all navigation options are
> not available for powerpc.
> 
> Apart from that, Powerpc has long list of branch instructions and
> hardcoding them in table appears to be error-prone. So, add function
> to find instruction instead of creating table. This function dynamically
> create table (list of 'struct ins'), and instead of creating object
> every time, first check if list already contain object for that
> instruction.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v7:
>   - Little bit change in initializing instruction list.
> 
>  tools/perf/util/annotate.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 816aa2c..5aa72d9 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -531,6 +531,11 @@ static struct ins instructions_arm[] = {
>  	{ .name = "retq",  .ops  = &ret_ops, },
>  };
>  
> +struct instructions_powerpc {
> +	struct ins *ins;
> +	struct list_head list;
> +};
> +

I'm considering accepting even more arch specific stuff in here, but we
need to stop doing that, and getting this as arch neutral as we possibly
can, instead of continuing to sprinkle strcmp(arch, "foo") around.

>  static int ins__key_cmp(const void *name, const void *insp)
>  {
>  	const struct ins *ins = insp;
> @@ -546,6 +551,111 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }
>  
> +static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
> +					 const char *name, struct ins_ops *ops)
> +{
> +	struct instructions_powerpc *ins_powerpc;
> +	struct ins *ins;
> +
> +	ins = zalloc(sizeof(struct ins));
> +	if (!ins)
> +		return NULL;
> +
> +	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
> +	if (!ins_powerpc)
> +		goto out_free_ins;
> +
> +	ins->name = strdup(name);
> +	if (!ins->name)
> +		goto out_free_ins_power;
> +
> +	ins->ops = ops;
> +	ins_powerpc->ins = ins;
> +	list_add_tail(&(ins_powerpc->list), &(head->list));
> +
> +	return ins;
> +
> +out_free_ins_power:
> +	zfree(&ins_powerpc);
> +out_free_ins:
> +	zfree(&ins);
> +	return NULL;
> +}
> +
> +static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
> +					    const char *name)
> +{
> +	struct instructions_powerpc *pos;
> +
> +	list_for_each_entry(pos, &head->list, list) {
> +		if (!strcmp(pos->ins->name, name))
> +			return pos->ins;
> +	}
> +	return NULL;
> +}
> +
> +static struct ins *ins__find_powerpc(const char *name)
> +{
> +	int i;
> +	struct ins *ins;
> +	struct ins_ops *ops;
> +	static struct instructions_powerpc head = {
> +		.list = LIST_HEAD_INIT(head.list),
> +	};
> +
> +	/*
> +	 * - Interested only if instruction starts with 'b'.
> +	 * - Few start with 'b', but aren't branch instructions.
> +	 */
> +	if (name[0] != 'b'             ||
> +	    !strncmp(name, "bcd", 3)   ||
> +	    !strncmp(name, "brinc", 5) ||
> +	    !strncmp(name, "bper", 4))
> +		return NULL;
> +
> +	/*
> +	 * Return if we already have object of 'struct ins' for this instruction
> +	 */
> +	ins = list_search__ins_powerpc(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ops = &jump_ops;
> +
> +	i = strlen(name) - 1;
> +	if (i < 0)
> +		return NULL;
> +
> +	/* ignore optional hints at the end of the instructions */
> +	if (name[i] == '+' || name[i] == '-')
> +		i--;
> +
> +	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
> +		/*
> +		 * if the instruction ends up with 'l' or 'la', then
> +		 * those are considered 'calls' since they update LR.
> +		 * ... except for 'bnl' which is branch if not less than
> +		 * and the absolute form of the same.
> +		 */
> +		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
> +		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
> +		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
> +			ops = &call_ops;
> +	}
> +	if (name[i] == 'r' && name[i-1] == 'l')
> +		/*
> +		 * instructions ending with 'lr' are considered to be
> +		 * return instructions
> +		 */
> +		ops = &ret_ops;
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_powerpc(&head, name, ops);
> +}
> +
>  static void ins__sort(struct ins *instructions, int nmemb)
>  {
>  	qsort(instructions, nmemb, sizeof(struct ins), ins__cmp);
> @@ -581,6 +691,8 @@ static struct ins *ins__find(const char *name)
>  	} else if (!strcmp(norm_arch, "arm")) {
>  		instructions = instructions_arm;
>  		nmemb = ARRAY_SIZE(instructions_arm);
> +	} else if (!strcmp(norm_arch, "powerpc")) {
> +		return ins__find_powerpc(name);
>  	} else {
>  		pr_err("perf annotate not supported by %s arch\n", norm_arch);
>  		return NULL;
> -- 
> 2.5.5

Re: [PATCH v7 3/6] perf annotate: Show raw form for jump instruction with indirect target

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2016 at 09:17:53PM +0530, Ravi Bangoria escreveu:
> For jump instructions that does not include target address as direct
> operand, use raw value for that. This is needed for certain powerpc

  "use raw value" looks vague, as the example below makes is go from
using a value (ffffffffffffca2c) to no value at all, i.e. the output
looks backwards from what you describe, can you instead show the
original disassembled line from objdump, which I think is what you're
calling "raw value" in this case?

- Arnaldo

> jump instructions that use target address in a register (such as bctr,
> btar, ...).
> 
> Before:
>      ld     r12,32088(r12)
>      mtctr  r12
>   v  bctr   ffffffffffffca2c
>      std    r2,24(r1)
>      addis  r12,r2,-1
> 
> After:
>      ld     r12,32088(r12)
>      mtctr  r12
>   v  bctr
>      std    r2,24(r1)
>      addis  r12,r2,-1
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v7:
>   - Added example in description
> 
>  tools/perf/util/annotate.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 5aa72d9..1ccf26a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -136,6 +136,9 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>  static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>  			   struct ins_operands *ops)
>  {
> +	if (!ops->target.addr)
> +		return ins__raw_scnprintf(ins, bf, size, ops);
> +
>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
>  }
>  
> -- 
> 2.5.5

Re: [PATCH v7 4/6] perf annotate: Support jump instruction with target as second operand

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2016 at 09:17:54PM +0530, Ravi Bangoria escreveu:
> Current perf is not able to parse jump instruction when second operand
> contains target address. Arch like powerpc has such instructions. For
> example, 'bne  cr7,0xc0000000000f6154'.
> 
> objdump o/p:
>   c0000000000f6140:   ld     r9,1032(r31)
>   c0000000000f6144:   cmpdi  cr7,r9,0
>   c0000000000f6148:   bne    cr7,0xc0000000000f6154
>   c0000000000f614c:   ld     r9,2312(r30)
>   c0000000000f6150:   std    r9,1032(r31)
>   c0000000000f6154:   ld     r9,88(r31)

So the above is what is parsed to generate the following? Or these
aren't related?
 
> Before patch:
>          ld     r9,1032(r31)
>          cmpdi  cr7,r9,0
>       v  bne    3ffffffffff09f2c
>          ld     r9,2312(r30)
>          std    r9,1032(r31)
>   74:    ld     r9,88(r31)
> 
> After patch:
>          ld     r9,1032(r31)
>          cmpdi  cr7,r9,0
>       v  bne    74
>          ld     r9,2312(r30)
>          std    r9,1032(r31)
>   74:    ld     r9,88(r31)
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v7:
>   - Added example in description
> 
>  tools/perf/util/annotate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 1ccf26a..a9dbac1 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -122,8 +122,12 @@ bool ins__is_call(const struct ins *ins)
>  static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>  {
>  	const char *s = strchr(ops->raw, '+');
> +	const char *c = strchr(ops->raw, ',');
>  
> -	ops->target.addr = strtoull(ops->raw, NULL, 16);
> +	if (c++ != NULL)
> +		ops->target.addr = strtoull(c, NULL, 16);
> +	else
> +		ops->target.addr = strtoull(ops->raw, NULL, 16);
>  
>  	if (s++ != NULL)
>  		ops->target.offset = strtoull(s, NULL, 16);
> -- 
> 2.5.5

Re: [PATCH v7 5/6] perf annotate: Fix jump target outside of function address range

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2016 at 09:17:55PM +0530, Ravi Bangoria escreveu:
> If jump target is outside of function range, perf is not handling it
> correctly. Especially when target address is lesser than function start
> address, target offset will be negative. But, target address declared
> to be unsigned, converts negative number into 2's complement. See below
> example. Here target of 'jumpq' instruction at 34cf8 is 34ac0 which is
> lesser than function start address(34cf0).
> 
>         34ac0 - 34cf0 = -0x230 = 0xfffffffffffffdd0

This one looks ok, but isn't applying.

- Arnaldo
 
> Objdump output:
> 
>   0000000000034cf0 <__sigaction>:
>   __GI___sigaction():
>     34cf0: lea    -0x20(%rdi),%eax
>     34cf3: cmp    -bashx1,%eax
>     34cf6: jbe    34d00 <__sigaction+0x10>
>     34cf8: jmpq   34ac0 <__GI___libc_sigaction>
>     34cfd: nopl   (%rax)
>     34d00: mov    0x386161(%rip),%rax        # 3bae68 <_DYNAMIC+0x2e8>
>     34d07: movl   -bashx16,%fs:(%rax)
>     34d0e: mov    -bashxffffffff,%eax
>     34d13: retq
> 
> perf annotate before applying patch:
> 
>   __GI___sigaction  /usr/lib64/libc-2.22.so
>            lea    -0x20(%rdi),%eax
>            cmp    -bashx1,%eax
>         v  jbe    10
>         v  jmpq   fffffffffffffdd0
>            nop
>     10:    mov    _DYNAMIC+0x2e8,%rax
>            movl   -bashx16,%fs:(%rax)
>            mov    -bashxffffffff,%eax
>            retq
> 
> perf annotate after applying patch:
> 
>   __GI___sigaction  /usr/lib64/libc-2.22.so
>            lea    -0x20(%rdi),%eax
>            cmp    -bashx1,%eax
>         v  jbe    10
>         ^  jmpq   34ac0 <__GI___libc_sigaction>
>            nop
>     10:    mov    _DYNAMIC+0x2e8,%rax
>            movl   -bashx16,%fs:(%rax)
>            mov    -bashxffffffff,%eax
>            retq
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v7:
>   - No changes
> 
>  tools/perf/ui/browsers/annotate.c |  5 +++--
>  tools/perf/util/annotate.c        | 14 +++++++++-----
>  tools/perf/util/annotate.h        |  5 +++--
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 214a14a..2d04bdf 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -215,7 +215,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
>  			ui_browser__set_color(browser, color);
>  		if (dl->ins && dl->ins->ops->scnprintf) {
>  			if (ins__is_jump(dl->ins)) {
> -				bool fwd = dl->ops.target.offset > (u64)dl->offset;
> +				bool fwd = dl->ops.target.offset > dl->offset;
>  
>  				ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR :
>  								    SLSMG_UARROW_CHAR);
> @@ -245,7 +245,8 @@ static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sy
>  {
>  	if (!dl || !dl->ins || !ins__is_jump(dl->ins)
>  	    || !disasm_line__has_offset(dl)
> -	    || dl->ops.target.offset >= symbol__size(sym))
> +	    || dl->ops.target.offset < 0
> +	    || dl->ops.target.offset >= (s64)symbol__size(sym))
>  		return false;
>  
>  	return true;
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index a9dbac1..fc44dd1 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -129,10 +129,12 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>  	else
>  		ops->target.addr = strtoull(ops->raw, NULL, 16);
>  
> -	if (s++ != NULL)
> +	if (s++ != NULL) {
>  		ops->target.offset = strtoull(s, NULL, 16);
> -	else
> -		ops->target.offset = UINT64_MAX;
> +		ops->target.offset_avail = true;
> +	} else {
> +		ops->target.offset_avail = false;
> +	}
>  
>  	return 0;
>  }
> @@ -140,7 +142,7 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>  static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>  			   struct ins_operands *ops)
>  {
> -	if (!ops->target.addr)
> +	if (!ops->target.addr || ops->target.offset < 0)
>  		return ins__raw_scnprintf(ins, bf, size, ops);
>  
>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
> @@ -1373,9 +1375,11 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>  	if (dl == NULL)
>  		return -1;
>  
> -	if (dl->ops.target.offset == UINT64_MAX)
> +	if (!disasm_line__has_offset(dl)) {
>  		dl->ops.target.offset = dl->ops.target.addr -
>  					map__rip_2objdump(map, sym->start);
> +		dl->ops.target.offset_avail = true;
> +	}
>  
>  	/* kcore has no symbols, so add the call target name */
>  	if (dl->ins && ins__is_call(dl->ins) && !dl->ops.target.name) {
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 4400269..7ba3579 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -19,7 +19,8 @@ struct ins_operands {
>  		char	*raw;
>  		char	*name;
>  		u64	addr;
> -		u64	offset;
> +		s64	offset;
> +		bool    offset_avail;
>  	} target;
>  	union {
>  		struct {
> @@ -67,7 +68,7 @@ struct disasm_line {
>  
>  static inline bool disasm_line__has_offset(const struct disasm_line *dl)
>  {
> -	return dl->ops.target.offset != UINT64_MAX;
> +	return dl->ops.target.offset_avail;
>  }
>  
>  void disasm_line__free(struct disasm_line *dl);
> -- 
> 2.5.5

Re: [PATCH v7 6/6] perf annotate: cross arch annotate support fixes for ARM

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2016 at 09:17:56PM +0530, Ravi Bangoria escreveu:
> From: Kim Phillips <kim.phillips@arm.com>
> 
> For ARM we remove the list that contains non-arm insns, and
> instead add more maintainable branch instruction regex logic.

This one looks ok and actually is in the direction of having facilities
for all arches, should've come as infrastructure that then gets used by
ARM and powerpc.

- Arnaldo
 
> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
> Changes in v7:
>   - Little bit change in initializing instruction list.
> 
>  tools/perf/util/annotate.c | 177 +++++++++++++++++----------------------------
>  1 file changed, 65 insertions(+), 112 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index fc44dd1..83d5ac8 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -28,6 +28,7 @@ const char 	*disassembler_style;
>  const char	*objdump_path;
>  static regex_t	 file_lineno;
>  static char	*norm_arch;
> +static regex_t	arm_call_insn, arm_jump_insn;
>  
>  static struct ins *ins__find(const char *name);
>  static int disasm_line__parse(char *line, char **namep, char **rawp);
> @@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
>  	{ .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, },
> -	{ .name = "bge",   .ops  = &jump_ops, },
> -	{ .name = "bgt",   .ops  = &jump_ops, },
> -	{ .name = "bhi",   .ops  = &jump_ops, },
> -	{ .name = "bl",    .ops  = &call_ops, },
> -	{ .name = "bls",   .ops  = &jump_ops, },
> -	{ .name = "blt",   .ops  = &jump_ops, },
> -	{ .name = "blx",   .ops  = &call_ops, },
> -	{ .name = "bne",   .ops  = &jump_ops, },
> -	{ .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, },
> -};
> -
> -struct instructions_powerpc {
> +struct instructions_arch {
>  	struct ins *ins;
>  	struct list_head list;
>  };
> @@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }
>  
> -static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
> -					 const char *name, struct ins_ops *ops)
> +static struct ins *list_add__ins_arch(struct instructions_arch *head,
> +				      const char *name, struct ins_ops *ops)
>  {
> -	struct instructions_powerpc *ins_powerpc;
> +	struct instructions_arch *ins_arch;
>  	struct ins *ins;
>  
>  	ins = zalloc(sizeof(struct ins));
>  	if (!ins)
>  		return NULL;
>  
> -	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
> -	if (!ins_powerpc)
> +	ins_arch = zalloc(sizeof(struct instructions_arch));
> +	if (!ins_arch)
>  		goto out_free_ins;
>  
>  	ins->name = strdup(name);
>  	if (!ins->name)
> -		goto out_free_ins_power;
> +		goto out_free_ins_arch;
>  
>  	ins->ops = ops;
> -	ins_powerpc->ins = ins;
> -	list_add_tail(&(ins_powerpc->list), &(head->list));
> +	ins_arch->ins = ins;
> +	list_add_tail(&(ins_arch->list), &(head->list));
>  
>  	return ins;
>  
> -out_free_ins_power:
> -	zfree(&ins_powerpc);
> +out_free_ins_arch:
> +	zfree(&ins_arch);
>  out_free_ins:
>  	zfree(&ins);
>  	return NULL;
>  }
>  
> -static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
> -					    const char *name)
> +static struct ins *list_search__ins_arch(struct instructions_arch *head,
> +					 const char *name)
>  {
> -	struct instructions_powerpc *pos;
> +	struct instructions_arch *pos;
>  
>  	list_for_each_entry(pos, &head->list, list) {
>  		if (!strcmp(pos->ins->name, name))
> @@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	int i;
>  	struct ins *ins;
>  	struct ins_ops *ops;
> -	static struct instructions_powerpc head = {
> +	static struct instructions_arch head = {
>  		.list = LIST_HEAD_INIT(head.list),
>  	};
>  
> @@ -625,7 +535,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	/*
>  	 * Return if we already have object of 'struct ins' for this instruction
>  	 */
> -	ins = list_search__ins_powerpc(&head, name);
> +	ins = list_search__ins_arch(&head, name);
>  	if (ins)
>  		return ins;
>  
> @@ -662,7 +572,40 @@ static struct ins *ins__find_powerpc(const char *name)
>  	 * Add instruction to list so next time no need to
>  	 * allocate memory for it.
>  	 */
> -	return list_add__ins_powerpc(&head, name, ops);
> +	return list_add__ins_arch(&head, name, ops);
> +}
> +
> +static struct ins *ins__find_arm(const char *name)
> +{
> +	struct ins *ins;
> +	struct ins_ops *ops = &mov_ops;
> +	regmatch_t match[2];
> +	int ret;
> +	static struct instructions_arch head = {
> +		.list = LIST_HEAD_INIT(head.list),
> +	};
> +
> +	/*
> +	 * Return if we already have object of 'struct ins' for this instruction
> +	 */
> +	ins = list_search__ins_arch(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ret = regexec(&arm_call_insn, name, 2, match, 0);
> +	if (!ret) {
> +		ops = &call_ops;
> +	} else {
> +		ret = regexec(&arm_jump_insn, name, 2, match, 0);
> +		if (!ret)
> +			ops = &jump_ops;
> +	}
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_arch(&head, name, ops);
>  }
>  
>  static void ins__sort(struct ins *instructions, int nmemb)
> @@ -682,15 +625,26 @@ static const char *annotate__norm_arch(char *arch)
>  	return normalize_arch(arch);
>  }
>  
> +#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
> +
>  static struct ins *ins__find(const char *name)
>  {
>  	static bool sorted;
>  	struct ins *instructions;
> -	int nmemb;
> +	int nmemb, ret;
>  
>  	if (!sorted) {
>  		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
> -		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
> +		if (!strcmp(norm_arch, "arm")) {
> +			ret = regcomp(&arm_call_insn,
> +				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
> +			ret |= regcomp(&arm_jump_insn,
> +				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
> +			if (ret) {
> +				pr_err("regcomp failed\n");
> +				return NULL;
> +			}
> +		}
>  		sorted = true;
>  	}
>  
> @@ -698,8 +652,7 @@ static struct ins *ins__find(const char *name)
>  		instructions = instructions_x86;
>  		nmemb = ARRAY_SIZE(instructions_x86);
>  	} else if (!strcmp(norm_arch, "arm")) {
> -		instructions = instructions_arm;
> -		nmemb = ARRAY_SIZE(instructions_arm);
> +		return ins__find_arm(name);
>  	} else if (!strcmp(norm_arch, "powerpc")) {
>  		return ins__find_powerpc(name);
>  	} else {
> -- 
> 2.5.5

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

[Ravi Bangoria]

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

Re: [PATCH v7 3/6] perf annotate: Show raw form for jump instruction with indirect target

[Ravi Bangoria]

On Wednesday 05 October 2016 04:57 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2016 at 09:17:53PM +0530, Ravi Bangoria escreveu:
>> For jump instructions that does not include target address as direct
>> operand, use raw value for that. This is needed for certain powerpc
>   "use raw value" looks vague, as the example below makes is go from
> using a value (ffffffffffffca2c) to no value at all, i.e. the output
> looks backwards from what you describe, can you instead show the
> original disassembled line from objdump, which I think is what you're
> calling "raw value" in this case?

Correct, I'm showing that only -- "original disassembled line from objdump".

There is no direct operand with bctr. It uses content of register 'ctr' as target
address.

For example, objdump output:

       100b8fd8:   add    r10,r9,r10
       100b8fdc:   mtctr  r10
       100b8fe0:   bctr

> - Arnaldo
>
>> jump instructions that use target address in a register (such as bctr,
>> btar, ...).
>>
>> Before:
>>      ld     r12,32088(r12)
>>      mtctr  r12
>>   v  bctr   ffffffffffffca2c
>>      std    r2,24(r1)
>>      addis  r12,r2,-1
>>
>> After:
>>      ld     r12,32088(r12)
>>      mtctr  r12
>>   v  bctr
>>      std    r2,24(r1)
>>      addis  r12,r2,-1
>>
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>> Changes in v7:
>>   - Added example in description
>>
>>  tools/perf/util/annotate.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 5aa72d9..1ccf26a 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -136,6 +136,9 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>>  			   struct ins_operands *ops)
>>  {
>> +	if (!ops->target.addr)
>> +		return ins__raw_scnprintf(ins, bf, size, ops);
>> +
>>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
>>  }
>>  
>> -- 
>> 2.5.5

Re: [PATCH v7 4/6] perf annotate: Support jump instruction with target as second operand

[Ravi Bangoria]

On Wednesday 05 October 2016 04:58 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2016 at 09:17:54PM +0530, Ravi Bangoria escreveu:
>> Current perf is not able to parse jump instruction when second operand
>> contains target address. Arch like powerpc has such instructions. For
>> example, 'bne  cr7,0xc0000000000f6154'.
>>
>> objdump o/p:
>>   c0000000000f6140:   ld     r9,1032(r31)
>>   c0000000000f6144:   cmpdi  cr7,r9,0
>>   c0000000000f6148:   bne    cr7,0xc0000000000f6154
>>   c0000000000f614c:   ld     r9,2312(r30)
>>   c0000000000f6150:   std    r9,1032(r31)
>>   c0000000000f6154:   ld     r9,88(r31)
> So the above is what is parsed to generate the following? Or these
> aren't related?

Yes, following is the perf annotate o/p from above objdump o/p.

-Ravi

>
>> Before patch:
>>          ld     r9,1032(r31)
>>          cmpdi  cr7,r9,0
>>       v  bne    3ffffffffff09f2c
>>          ld     r9,2312(r30)
>>          std    r9,1032(r31)
>>   74:    ld     r9,88(r31)
>>
>> After patch:
>>          ld     r9,1032(r31)
>>          cmpdi  cr7,r9,0
>>       v  bne    74
>>          ld     r9,2312(r30)
>>          std    r9,1032(r31)
>>   74:    ld     r9,88(r31)
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>> Changes in v7:
>>   - Added example in description
>>
>>  tools/perf/util/annotate.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 1ccf26a..a9dbac1 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -122,8 +122,12 @@ bool ins__is_call(const struct ins *ins)
>>  static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  {
>>  	const char *s = strchr(ops->raw, '+');
>> +	const char *c = strchr(ops->raw, ',');
>>  
>> -	ops->target.addr = strtoull(ops->raw, NULL, 16);
>> +	if (c++ != NULL)
>> +		ops->target.addr = strtoull(c, NULL, 16);
>> +	else
>> +		ops->target.addr = strtoull(ops->raw, NULL, 16);
>>  
>>  	if (s++ != NULL)
>>  		ops->target.offset = strtoull(s, NULL, 16);
>> -- 
>> 2.5.5

Re: [PATCH v7 5/6] perf annotate: Fix jump target outside of function address range

[Ravi Bangoria]

On Wednesday 05 October 2016 05:01 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2016 at 09:17:55PM +0530, Ravi Bangoria escreveu:
>> If jump target is outside of function range, perf is not handling it
>> correctly. Especially when target address is lesser than function start
>> address, target offset will be negative. But, target address declared
>> to be unsigned, converts negative number into 2's complement. See below
>> example. Here target of 'jumpq' instruction at 34cf8 is 34ac0 which is
>> lesser than function start address(34cf0).
>>
>>         34ac0 - 34cf0 = -0x230 = 0xfffffffffffffdd0
> This one looks ok, but isn't applying.

This is applying fine for me on perf/core. Which branch are you trying?

-Ravi

>
> - Arnaldo
>
>> Objdump output:
>>
>>   0000000000034cf0 <__sigaction>:
>>   __GI___sigaction():
>>     34cf0: lea    -0x20(%rdi),%eax
>>     34cf3: cmp    -bashx1,%eax
>>     34cf6: jbe    34d00 <__sigaction+0x10>
>>     34cf8: jmpq   34ac0 <__GI___libc_sigaction>
>>     34cfd: nopl   (%rax)
>>     34d00: mov    0x386161(%rip),%rax        # 3bae68 <_DYNAMIC+0x2e8>
>>     34d07: movl   -bashx16,%fs:(%rax)
>>     34d0e: mov    -bashxffffffff,%eax
>>     34d13: retq
>>
>> perf annotate before applying patch:
>>
>>   __GI___sigaction  /usr/lib64/libc-2.22.so
>>            lea    -0x20(%rdi),%eax
>>            cmp    -bashx1,%eax
>>         v  jbe    10
>>         v  jmpq   fffffffffffffdd0
>>            nop
>>     10:    mov    _DYNAMIC+0x2e8,%rax
>>            movl   -bashx16,%fs:(%rax)
>>            mov    -bashxffffffff,%eax
>>            retq
>>
>> perf annotate after applying patch:
>>
>>   __GI___sigaction  /usr/lib64/libc-2.22.so
>>            lea    -0x20(%rdi),%eax
>>            cmp    -bashx1,%eax
>>         v  jbe    10
>>         ^  jmpq   34ac0 <__GI___libc_sigaction>
>>            nop
>>     10:    mov    _DYNAMIC+0x2e8,%rax
>>            movl   -bashx16,%fs:(%rax)
>>            mov    -bashxffffffff,%eax
>>            retq
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>> ---
>> Changes in v7:
>>   - No changes
>>
>>  tools/perf/ui/browsers/annotate.c |  5 +++--
>>  tools/perf/util/annotate.c        | 14 +++++++++-----
>>  tools/perf/util/annotate.h        |  5 +++--
>>  3 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>> index 214a14a..2d04bdf 100644
>> --- a/tools/perf/ui/browsers/annotate.c
>> +++ b/tools/perf/ui/browsers/annotate.c
>> @@ -215,7 +215,7 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
>>  			ui_browser__set_color(browser, color);
>>  		if (dl->ins && dl->ins->ops->scnprintf) {
>>  			if (ins__is_jump(dl->ins)) {
>> -				bool fwd = dl->ops.target.offset > (u64)dl->offset;
>> +				bool fwd = dl->ops.target.offset > dl->offset;
>>  
>>  				ui_browser__write_graph(browser, fwd ? SLSMG_DARROW_CHAR :
>>  								    SLSMG_UARROW_CHAR);
>> @@ -245,7 +245,8 @@ static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sy
>>  {
>>  	if (!dl || !dl->ins || !ins__is_jump(dl->ins)
>>  	    || !disasm_line__has_offset(dl)
>> -	    || dl->ops.target.offset >= symbol__size(sym))
>> +	    || dl->ops.target.offset < 0
>> +	    || dl->ops.target.offset >= (s64)symbol__size(sym))
>>  		return false;
>>  
>>  	return true;
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index a9dbac1..fc44dd1 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -129,10 +129,12 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  	else
>>  		ops->target.addr = strtoull(ops->raw, NULL, 16);
>>  
>> -	if (s++ != NULL)
>> +	if (s++ != NULL) {
>>  		ops->target.offset = strtoull(s, NULL, 16);
>> -	else
>> -		ops->target.offset = UINT64_MAX;
>> +		ops->target.offset_avail = true;
>> +	} else {
>> +		ops->target.offset_avail = false;
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -140,7 +142,7 @@ static int jump__parse(struct ins_operands *ops, struct map *map __maybe_unused)
>>  static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>>  			   struct ins_operands *ops)
>>  {
>> -	if (!ops->target.addr)
>> +	if (!ops->target.addr || ops->target.offset < 0)
>>  		return ins__raw_scnprintf(ins, bf, size, ops);
>>  
>>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
>> @@ -1373,9 +1375,11 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
>>  	if (dl == NULL)
>>  		return -1;
>>  
>> -	if (dl->ops.target.offset == UINT64_MAX)
>> +	if (!disasm_line__has_offset(dl)) {
>>  		dl->ops.target.offset = dl->ops.target.addr -
>>  					map__rip_2objdump(map, sym->start);
>> +		dl->ops.target.offset_avail = true;
>> +	}
>>  
>>  	/* kcore has no symbols, so add the call target name */
>>  	if (dl->ins && ins__is_call(dl->ins) && !dl->ops.target.name) {
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index 4400269..7ba3579 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -19,7 +19,8 @@ struct ins_operands {
>>  		char	*raw;
>>  		char	*name;
>>  		u64	addr;
>> -		u64	offset;
>> +		s64	offset;
>> +		bool    offset_avail;
>>  	} target;
>>  	union {
>>  		struct {
>> @@ -67,7 +68,7 @@ struct disasm_line {
>>  
>>  static inline bool disasm_line__has_offset(const struct disasm_line *dl)
>>  {
>> -	return dl->ops.target.offset != UINT64_MAX;
>> +	return dl->ops.target.offset_avail;
>>  }
>>  
>>  void disasm_line__free(struct disasm_line *dl);
>> -- 
>> 2.5.5

Re: [PATCH v7 6/6] perf annotate: cross arch annotate support fixes for ARM

[Ravi Bangoria]

On Wednesday 05 October 2016 05:04 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2016 at 09:17:56PM +0530, Ravi Bangoria escreveu:
>> From: Kim Phillips <kim.phillips@arm.com>
>>
>> For ARM we remove the list that contains non-arm insns, and
>> instead add more maintainable branch instruction regex logic.
> This one looks ok and actually is in the direction of having facilities
> for all arches, should've come as infrastructure that then gets used by
> ARM and powerpc.

This was authored by Kim and I didn't wanted to change that so I kept it
at the end.

I'm sending a cleanup patch that applies on top of this series. That patch
moves most of arch specific stuff from util/annotate.c to
util/annotate/<arch>.c. Please review it.

Please pull this series if you are ok with that patch. Otherwise I'll respin
entire series.

Thanks
-Ravi

[PATCH] perf annotate: Cleanup arch specific stuff

[Ravi Bangoria]

Move arch specific stuff from util/annotate.c to their respective
files in util/annotate directory.

No functionality changes.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/Build              |   1 +
 tools/perf/util/annotate.c         | 259 +++----------------------------------
 tools/perf/util/annotate.h         |  23 ++++
 tools/perf/util/annotate/Build     |   3 +
 tools/perf/util/annotate/arm.c     |  50 +++++++
 tools/perf/util/annotate/powerpc.c |  63 +++++++++
 tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
 7 files changed, 264 insertions(+), 242 deletions(-)
 create mode 100644 tools/perf/util/annotate/Build
 create mode 100644 tools/perf/util/annotate/arm.c
 create mode 100644 tools/perf/util/annotate/powerpc.c
 create mode 100644 tools/perf/util/annotate/x86.c

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index eb60e61..1ee1170 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -1,4 +1,5 @@
 libperf-y += alias.o
+libperf-y += annotate/
 libperf-y += annotate.o
 libperf-y += block-range.o
 libperf-y += build-id.o
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 83d5ac8..a168d1e 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -28,7 +28,6 @@ const char 	*disassembler_style;
 const char	*objdump_path;
 static regex_t	 file_lineno;
 static char	*norm_arch;
-static regex_t	arm_call_insn, arm_jump_insn;
 
 static struct ins *ins__find(const char *name);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
@@ -110,7 +109,7 @@ static int call__scnprintf(struct ins *ins, char *bf, size_t size,
 	return scnprintf(bf, size, "%-6.6s *%" PRIx64, ins->name, ops->target.addr);
 }
 
-static struct ins_ops call_ops = {
+struct ins_ops call_ops = {
 	.parse	   = call__parse,
 	.scnprintf = call__scnprintf,
 };
@@ -149,7 +148,7 @@ static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
 	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
 }
 
-static struct ins_ops jump_ops = {
+struct ins_ops jump_ops = {
 	.parse	   = jump__parse,
 	.scnprintf = jump__scnprintf,
 };
@@ -242,7 +241,7 @@ static void lock__delete(struct ins_operands *ops)
 	zfree(&ops->target.name);
 }
 
-static struct ins_ops lock_ops = {
+struct ins_ops lock_ops = {
 	.free	   = lock__delete,
 	.parse	   = lock__parse,
 	.scnprintf = lock__scnprintf,
@@ -306,7 +305,7 @@ static int mov__scnprintf(struct ins *ins, char *bf, size_t size,
 			 ops->target.name ?: ops->target.raw);
 }
 
-static struct ins_ops mov_ops = {
+struct ins_ops mov_ops = {
 	.parse	   = mov__parse,
 	.scnprintf = mov__scnprintf,
 };
@@ -347,7 +346,7 @@ static int dec__scnprintf(struct ins *ins, char *bf, size_t size,
 			 ops->target.name ?: ops->target.raw);
 }
 
-static struct ins_ops dec_ops = {
+struct ins_ops dec_ops = {
 	.parse	   = dec__parse,
 	.scnprintf = dec__scnprintf,
 };
@@ -358,11 +357,11 @@ static int nop__scnprintf(struct ins *ins __maybe_unused, char *bf, size_t size,
 	return scnprintf(bf, size, "%-6.6s", "nop");
 }
 
-static struct ins_ops nop_ops = {
+struct ins_ops nop_ops = {
 	.scnprintf = nop__scnprintf,
 };
 
-static struct ins_ops ret_ops = {
+struct ins_ops ret_ops = {
 	.scnprintf = ins__raw_scnprintf,
 };
 
@@ -371,107 +370,8 @@ bool ins__is_ret(const struct ins *ins)
 	return ins->ops == &ret_ops;
 }
 
-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, },
-	{ .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, },
-};
-
-struct instructions_arch {
-	struct ins *ins;
-	struct list_head list;
-};
-
-static int ins__key_cmp(const void *name, const void *insp)
-{
-	const struct ins *ins = insp;
-
-	return strcmp(name, ins->name);
-}
-
-static int ins__cmp(const void *a, const void *b)
-{
-	const struct ins *ia = a;
-	const struct ins *ib = b;
-
-	return strcmp(ia->name, ib->name);
-}
-
-static struct ins *list_add__ins_arch(struct instructions_arch *head,
-				      const char *name, struct ins_ops *ops)
+struct ins *list_add__ins_arch(struct instructions_arch *head,
+			       const char *name, struct ins_ops *ops)
 {
 	struct instructions_arch *ins_arch;
 	struct ins *ins;
@@ -501,8 +401,8 @@ static struct ins *list_add__ins_arch(struct instructions_arch *head,
 	return NULL;
 }
 
-static struct ins *list_search__ins_arch(struct instructions_arch *head,
-					 const char *name)
+struct ins *list_search__ins_arch(struct instructions_arch *head,
+				  const char *name)
 {
 	struct instructions_arch *pos;
 
@@ -513,106 +413,6 @@ static struct ins *list_search__ins_arch(struct instructions_arch *head,
 	return NULL;
 }
 
-static struct ins *ins__find_powerpc(const char *name)
-{
-	int i;
-	struct ins *ins;
-	struct ins_ops *ops;
-	static struct instructions_arch head = {
-		.list = LIST_HEAD_INIT(head.list),
-	};
-
-	/*
-	 * - Interested only if instruction starts with 'b'.
-	 * - Few start with 'b', but aren't branch instructions.
-	 */
-	if (name[0] != 'b'             ||
-	    !strncmp(name, "bcd", 3)   ||
-	    !strncmp(name, "brinc", 5) ||
-	    !strncmp(name, "bper", 4))
-		return NULL;
-
-	/*
-	 * Return if we already have object of 'struct ins' for this instruction
-	 */
-	ins = list_search__ins_arch(&head, name);
-	if (ins)
-		return ins;
-
-	ops = &jump_ops;
-
-	i = strlen(name) - 1;
-	if (i < 0)
-		return NULL;
-
-	/* ignore optional hints at the end of the instructions */
-	if (name[i] == '+' || name[i] == '-')
-		i--;
-
-	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
-		/*
-		 * if the instruction ends up with 'l' or 'la', then
-		 * those are considered 'calls' since they update LR.
-		 * ... except for 'bnl' which is branch if not less than
-		 * and the absolute form of the same.
-		 */
-		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
-		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
-		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
-			ops = &call_ops;
-	}
-	if (name[i] == 'r' && name[i-1] == 'l')
-		/*
-		 * instructions ending with 'lr' are considered to be
-		 * return instructions
-		 */
-		ops = &ret_ops;
-
-	/*
-	 * Add instruction to list so next time no need to
-	 * allocate memory for it.
-	 */
-	return list_add__ins_arch(&head, name, ops);
-}
-
-static struct ins *ins__find_arm(const char *name)
-{
-	struct ins *ins;
-	struct ins_ops *ops = &mov_ops;
-	regmatch_t match[2];
-	int ret;
-	static struct instructions_arch head = {
-		.list = LIST_HEAD_INIT(head.list),
-	};
-
-	/*
-	 * Return if we already have object of 'struct ins' for this instruction
-	 */
-	ins = list_search__ins_arch(&head, name);
-	if (ins)
-		return ins;
-
-	ret = regexec(&arm_call_insn, name, 2, match, 0);
-	if (!ret) {
-		ops = &call_ops;
-	} else {
-		ret = regexec(&arm_jump_insn, name, 2, match, 0);
-		if (!ret)
-			ops = &jump_ops;
-	}
-
-	/*
-	 * Add instruction to list so next time no need to
-	 * allocate memory for it.
-	 */
-	return list_add__ins_arch(&head, name, ops);
-}
-
-static void ins__sort(struct ins *instructions, int nmemb)
-{
-	qsort(instructions, nmemb, sizeof(struct ins), ins__cmp);
-}
-
 static const char *annotate__norm_arch(char *arch)
 {
 	struct utsname uts;
@@ -625,43 +425,18 @@ static const char *annotate__norm_arch(char *arch)
 	return normalize_arch(arch);
 }
 
-#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
-
 static struct ins *ins__find(const char *name)
 {
-	static bool sorted;
-	struct ins *instructions;
-	int nmemb, ret;
-
-	if (!sorted) {
-		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
-		if (!strcmp(norm_arch, "arm")) {
-			ret = regcomp(&arm_call_insn,
-				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
-			ret |= regcomp(&arm_jump_insn,
-				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
-			if (ret) {
-				pr_err("regcomp failed\n");
-				return NULL;
-			}
-		}
-		sorted = true;
-	}
-
-	if (!strcmp(norm_arch, "x86")) {
-		instructions = instructions_x86;
-		nmemb = ARRAY_SIZE(instructions_x86);
-	} else if (!strcmp(norm_arch, "arm")) {
+	if (!strcmp(norm_arch, "x86"))
+		return ins__find_x86(name);
+	else if (!strcmp(norm_arch, "arm"))
 		return ins__find_arm(name);
-	} else if (!strcmp(norm_arch, "powerpc")) {
+	else if (!strcmp(norm_arch, "powerpc"))
 		return ins__find_powerpc(name);
-	} else {
+	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);
+	return NULL;
 }
 
 int symbol__alloc_hist(struct symbol *sym)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 7ba3579..91b705a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -42,11 +42,30 @@ struct ins_ops {
 			 struct ins_operands *ops);
 };
 
+/* Defined in annotate.c */
+extern struct ins_ops mov_ops;
+extern struct ins_ops call_ops;
+extern struct ins_ops dec_ops;
+extern struct ins_ops jump_ops;
+extern struct ins_ops lock_ops;
+extern struct ins_ops nop_ops;
+extern struct ins_ops ret_ops;
+
 struct ins {
 	const char     *name;
 	struct ins_ops *ops;
 };
 
+struct instructions_arch {
+	struct ins *ins;
+	struct list_head list;
+};
+
+struct ins *list_add__ins_arch(struct instructions_arch *head,
+			       const char *name, struct ins_ops *ops);
+struct ins *list_search__ins_arch(struct instructions_arch *head,
+				  const char *name);
+
 bool ins__is_jump(const struct ins *ins);
 bool ins__is_call(const struct ins *ins);
 bool ins__is_ret(const struct ins *ins);
@@ -210,4 +229,8 @@ static inline int symbol__tui_annotate(struct symbol *sym __maybe_unused,
 
 extern const char	*disassembler_style;
 
+struct ins *ins__find_x86(const char *name);
+struct ins *ins__find_arm(const char *name);
+struct ins *ins__find_powerpc(const char *name);
+
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/annotate/Build b/tools/perf/util/annotate/Build
new file mode 100644
index 0000000..ed87f1c
--- /dev/null
+++ b/tools/perf/util/annotate/Build
@@ -0,0 +1,3 @@
+libperf-y += x86.o
+libperf-y += arm.o
+libperf-y += powerpc.o
diff --git a/tools/perf/util/annotate/arm.c b/tools/perf/util/annotate/arm.c
new file mode 100644
index 0000000..6bfc866
--- /dev/null
+++ b/tools/perf/util/annotate/arm.c
@@ -0,0 +1,50 @@
+#include "../annotate.h"
+
+static regex_t arm_call_insn, arm_jump_insn;
+
+#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
+
+static __attribute__((constructor)) void init_regex(void)
+{
+	int ret;
+
+	ret = regcomp(&arm_call_insn, "^blx?" ARM_CONDS "?$", REG_EXTENDED);
+	ret |= regcomp(&arm_jump_insn, "^bx?" ARM_CONDS "?$", REG_EXTENDED);
+
+	if (ret)
+		pr_err("regcomp failed\n");
+}
+
+struct ins *ins__find_arm(const char *name)
+{
+	struct ins *ins;
+	struct ins_ops *ops = &mov_ops;
+	regmatch_t match[2];
+	int ret;
+	static struct instructions_arch head = {
+		.list = LIST_HEAD_INIT(head.list),
+	};
+
+	/*
+	 * Return if we already have object of 'struct ins' for this instruction
+	 */
+	ins = list_search__ins_arch(&head, name);
+	if (ins)
+		return ins;
+
+	ret = regexec(&arm_call_insn, name, 2, match, 0);
+	if (!ret) {
+		ops = &call_ops;
+	} else {
+		ret = regexec(&arm_jump_insn, name, 2, match, 0);
+		if (!ret)
+			ops = &jump_ops;
+	}
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_arch(&head, name, ops);
+
+}
diff --git a/tools/perf/util/annotate/powerpc.c b/tools/perf/util/annotate/powerpc.c
new file mode 100644
index 0000000..6088ce2
--- /dev/null
+++ b/tools/perf/util/annotate/powerpc.c
@@ -0,0 +1,63 @@
+#include "../annotate.h"
+
+struct ins *ins__find_powerpc(const char *name)
+{
+	int i;
+	struct ins *ins;
+	struct ins_ops *ops;
+	static struct instructions_arch head = {
+		.list = LIST_HEAD_INIT(head.list),
+	};
+
+	/*
+	 * - Interested only if instruction starts with 'b'.
+	 * - Few start with 'b', but aren't branch instructions.
+	 */
+	if (name[0] != 'b'             ||
+	    !strncmp(name, "bcd", 3)   ||
+	    !strncmp(name, "brinc", 5) ||
+	    !strncmp(name, "bper", 4))
+		return NULL;
+
+	/*
+	 * Return if we already have object of 'struct ins' for this instruction
+	 */
+	ins = list_search__ins_arch(&head, name);
+	if (ins)
+		return ins;
+
+	ops = &jump_ops;
+
+	i = strlen(name) - 1;
+	if (i < 0)
+		return NULL;
+
+	/* ignore optional hints at the end of the instructions */
+	if (name[i] == '+' || name[i] == '-')
+		i--;
+
+	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
+		/*
+		 * if the instruction ends up with 'l' or 'la', then
+		 * those are considered 'calls' since they update LR.
+		 * ... except for 'bnl' which is branch if not less than
+		 * and the absolute form of the same.
+		 */
+		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
+		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
+		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
+			ops = &call_ops;
+	}
+	if (name[i] == 'r' && name[i-1] == 'l')
+		/*
+		 * instructions ending with 'lr' are considered to be
+		 * return instructions
+		 */
+		ops = &ret_ops;
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_arch(&head, name, ops);
+}
diff --git a/tools/perf/util/annotate/x86.c b/tools/perf/util/annotate/x86.c
new file mode 100644
index 0000000..b519197
--- /dev/null
+++ b/tools/perf/util/annotate/x86.c
@@ -0,0 +1,107 @@
+#include "../annotate.h"
+
+static struct ins instructions[] = {
+	{ .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 = "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 int ins__cmp(const void *a, const void *b)
+{
+	const struct ins *ia = a;
+	const struct ins *ib = b;
+
+	return strcmp(ia->name, ib->name);
+}
+
+static __attribute__((constructor)) void ins__sort(void)
+{
+	qsort(instructions, ARRAY_SIZE(instructions), sizeof(struct ins),
+	      ins__cmp);
+}
+
+static int ins__key_cmp(const void *name, const void *insp)
+{
+	const struct ins *ins = insp;
+
+	return strcmp(name, ins->name);
+}
+
+struct ins *ins__find_x86(const char *name)
+{
+	return bsearch(name, instructions, ARRAY_SIZE(instructions),
+		       sizeof(struct ins), ins__key_cmp);
+}
-- 
2.5.5

Re: [PATCH v7 6/6] perf annotate: cross arch annotate support fixes for ARM

[Kim Phillips]

On Mon, 10 Oct 2016 19:16:16 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> On Wednesday 05 October 2016 05:04 PM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Sep 21, 2016 at 09:17:56PM +0530, Ravi Bangoria escreveu:
> >> From: Kim Phillips <kim.phillips@arm.com>
> >>
> >> For ARM we remove the list that contains non-arm insns, and
> >> instead add more maintainable branch instruction regex logic.
> > This one looks ok and actually is in the direction of having facilities
> > for all arches, should've come as infrastructure that then gets used by
> > ARM and powerpc.
> 
> This was authored by Kim and I didn't wanted to change that so I kept it
> at the end.

I don't mind if this gets merged higher up in the series, in fact, I
think it's preferred to adding the arm insn table and then removing it
in the same series.  Keeping my Author: isn't necessary either:
something like "Signed-off-by: Kim Phillips <...> [ARM bits]" will
suffice.

> I'm sending a cleanup patch that applies on top of this series. That patch
> moves most of arch specific stuff from util/annotate.c to
> util/annotate/<arch>.c. Please review it.
> 
> Please pull this series if you are ok with that patch. Otherwise I'll respin
> entire series.

I'll wait for the dust to settle here before submitting an ARM return
insn fix, and aarch64 support originally authored by Chris Ryder
(basically translate table in [1] into regex format).

Thanks,

Kim

[1] https://lkml.org/lkml/2016/5/19/461

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Arnaldo Carvalho de Melo]

Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
> Move arch specific stuff from util/annotate.c to their respective
> files in util/annotate directory.
> 
> No functionality changes.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/Build              |   1 +
>  tools/perf/util/annotate.c         | 259 +++----------------------------------
>  tools/perf/util/annotate.h         |  23 ++++
>  tools/perf/util/annotate/Build     |   3 +
>  tools/perf/util/annotate/arm.c     |  50 +++++++
>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++

We already have a per arch area: tools/perf/arch/

- Arnaldo

>  7 files changed, 264 insertions(+), 242 deletions(-)
>  create mode 100644 tools/perf/util/annotate/Build
>  create mode 100644 tools/perf/util/annotate/arm.c
>  create mode 100644 tools/perf/util/annotate/powerpc.c
>  create mode 100644 tools/perf/util/annotate/x86.c
> 
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index eb60e61..1ee1170 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -1,4 +1,5 @@
>  libperf-y += alias.o
> +libperf-y += annotate/
>  libperf-y += annotate.o
>  libperf-y += block-range.o
>  libperf-y += build-id.o
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 83d5ac8..a168d1e 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -28,7 +28,6 @@ const char 	*disassembler_style;
>  const char	*objdump_path;
>  static regex_t	 file_lineno;
>  static char	*norm_arch;
> -static regex_t	arm_call_insn, arm_jump_insn;
>  
>  static struct ins *ins__find(const char *name);
>  static int disasm_line__parse(char *line, char **namep, char **rawp);
> @@ -110,7 +109,7 @@ static int call__scnprintf(struct ins *ins, char *bf, size_t size,
>  	return scnprintf(bf, size, "%-6.6s *%" PRIx64, ins->name, ops->target.addr);
>  }
>  
> -static struct ins_ops call_ops = {
> +struct ins_ops call_ops = {
>  	.parse	   = call__parse,
>  	.scnprintf = call__scnprintf,
>  };
> @@ -149,7 +148,7 @@ static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>  	return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset);
>  }
>  
> -static struct ins_ops jump_ops = {
> +struct ins_ops jump_ops = {
>  	.parse	   = jump__parse,
>  	.scnprintf = jump__scnprintf,
>  };
> @@ -242,7 +241,7 @@ static void lock__delete(struct ins_operands *ops)
>  	zfree(&ops->target.name);
>  }
>  
> -static struct ins_ops lock_ops = {
> +struct ins_ops lock_ops = {
>  	.free	   = lock__delete,
>  	.parse	   = lock__parse,
>  	.scnprintf = lock__scnprintf,
> @@ -306,7 +305,7 @@ static int mov__scnprintf(struct ins *ins, char *bf, size_t size,
>  			 ops->target.name ?: ops->target.raw);
>  }
>  
> -static struct ins_ops mov_ops = {
> +struct ins_ops mov_ops = {
>  	.parse	   = mov__parse,
>  	.scnprintf = mov__scnprintf,
>  };
> @@ -347,7 +346,7 @@ static int dec__scnprintf(struct ins *ins, char *bf, size_t size,
>  			 ops->target.name ?: ops->target.raw);
>  }
>  
> -static struct ins_ops dec_ops = {
> +struct ins_ops dec_ops = {
>  	.parse	   = dec__parse,
>  	.scnprintf = dec__scnprintf,
>  };
> @@ -358,11 +357,11 @@ static int nop__scnprintf(struct ins *ins __maybe_unused, char *bf, size_t size,
>  	return scnprintf(bf, size, "%-6.6s", "nop");
>  }
>  
> -static struct ins_ops nop_ops = {
> +struct ins_ops nop_ops = {
>  	.scnprintf = nop__scnprintf,
>  };
>  
> -static struct ins_ops ret_ops = {
> +struct ins_ops ret_ops = {
>  	.scnprintf = ins__raw_scnprintf,
>  };
>  
> @@ -371,107 +370,8 @@ bool ins__is_ret(const struct ins *ins)
>  	return ins->ops == &ret_ops;
>  }
>  
> -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, },
> -	{ .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, },
> -};
> -
> -struct instructions_arch {
> -	struct ins *ins;
> -	struct list_head list;
> -};
> -
> -static int ins__key_cmp(const void *name, const void *insp)
> -{
> -	const struct ins *ins = insp;
> -
> -	return strcmp(name, ins->name);
> -}
> -
> -static int ins__cmp(const void *a, const void *b)
> -{
> -	const struct ins *ia = a;
> -	const struct ins *ib = b;
> -
> -	return strcmp(ia->name, ib->name);
> -}
> -
> -static struct ins *list_add__ins_arch(struct instructions_arch *head,
> -				      const char *name, struct ins_ops *ops)
> +struct ins *list_add__ins_arch(struct instructions_arch *head,
> +			       const char *name, struct ins_ops *ops)
>  {
>  	struct instructions_arch *ins_arch;
>  	struct ins *ins;
> @@ -501,8 +401,8 @@ static struct ins *list_add__ins_arch(struct instructions_arch *head,
>  	return NULL;
>  }
>  
> -static struct ins *list_search__ins_arch(struct instructions_arch *head,
> -					 const char *name)
> +struct ins *list_search__ins_arch(struct instructions_arch *head,
> +				  const char *name)
>  {
>  	struct instructions_arch *pos;
>  
> @@ -513,106 +413,6 @@ static struct ins *list_search__ins_arch(struct instructions_arch *head,
>  	return NULL;
>  }
>  
> -static struct ins *ins__find_powerpc(const char *name)
> -{
> -	int i;
> -	struct ins *ins;
> -	struct ins_ops *ops;
> -	static struct instructions_arch head = {
> -		.list = LIST_HEAD_INIT(head.list),
> -	};
> -
> -	/*
> -	 * - Interested only if instruction starts with 'b'.
> -	 * - Few start with 'b', but aren't branch instructions.
> -	 */
> -	if (name[0] != 'b'             ||
> -	    !strncmp(name, "bcd", 3)   ||
> -	    !strncmp(name, "brinc", 5) ||
> -	    !strncmp(name, "bper", 4))
> -		return NULL;
> -
> -	/*
> -	 * Return if we already have object of 'struct ins' for this instruction
> -	 */
> -	ins = list_search__ins_arch(&head, name);
> -	if (ins)
> -		return ins;
> -
> -	ops = &jump_ops;
> -
> -	i = strlen(name) - 1;
> -	if (i < 0)
> -		return NULL;
> -
> -	/* ignore optional hints at the end of the instructions */
> -	if (name[i] == '+' || name[i] == '-')
> -		i--;
> -
> -	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
> -		/*
> -		 * if the instruction ends up with 'l' or 'la', then
> -		 * those are considered 'calls' since they update LR.
> -		 * ... except for 'bnl' which is branch if not less than
> -		 * and the absolute form of the same.
> -		 */
> -		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
> -		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
> -		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
> -			ops = &call_ops;
> -	}
> -	if (name[i] == 'r' && name[i-1] == 'l')
> -		/*
> -		 * instructions ending with 'lr' are considered to be
> -		 * return instructions
> -		 */
> -		ops = &ret_ops;
> -
> -	/*
> -	 * Add instruction to list so next time no need to
> -	 * allocate memory for it.
> -	 */
> -	return list_add__ins_arch(&head, name, ops);
> -}
> -
> -static struct ins *ins__find_arm(const char *name)
> -{
> -	struct ins *ins;
> -	struct ins_ops *ops = &mov_ops;
> -	regmatch_t match[2];
> -	int ret;
> -	static struct instructions_arch head = {
> -		.list = LIST_HEAD_INIT(head.list),
> -	};
> -
> -	/*
> -	 * Return if we already have object of 'struct ins' for this instruction
> -	 */
> -	ins = list_search__ins_arch(&head, name);
> -	if (ins)
> -		return ins;
> -
> -	ret = regexec(&arm_call_insn, name, 2, match, 0);
> -	if (!ret) {
> -		ops = &call_ops;
> -	} else {
> -		ret = regexec(&arm_jump_insn, name, 2, match, 0);
> -		if (!ret)
> -			ops = &jump_ops;
> -	}
> -
> -	/*
> -	 * Add instruction to list so next time no need to
> -	 * allocate memory for it.
> -	 */
> -	return list_add__ins_arch(&head, name, ops);
> -}
> -
> -static void ins__sort(struct ins *instructions, int nmemb)
> -{
> -	qsort(instructions, nmemb, sizeof(struct ins), ins__cmp);
> -}
> -
>  static const char *annotate__norm_arch(char *arch)
>  {
>  	struct utsname uts;
> @@ -625,43 +425,18 @@ static const char *annotate__norm_arch(char *arch)
>  	return normalize_arch(arch);
>  }
>  
> -#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
> -
>  static struct ins *ins__find(const char *name)
>  {
> -	static bool sorted;
> -	struct ins *instructions;
> -	int nmemb, ret;
> -
> -	if (!sorted) {
> -		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
> -		if (!strcmp(norm_arch, "arm")) {
> -			ret = regcomp(&arm_call_insn,
> -				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
> -			ret |= regcomp(&arm_jump_insn,
> -				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
> -			if (ret) {
> -				pr_err("regcomp failed\n");
> -				return NULL;
> -			}
> -		}
> -		sorted = true;
> -	}
> -
> -	if (!strcmp(norm_arch, "x86")) {
> -		instructions = instructions_x86;
> -		nmemb = ARRAY_SIZE(instructions_x86);
> -	} else if (!strcmp(norm_arch, "arm")) {
> +	if (!strcmp(norm_arch, "x86"))
> +		return ins__find_x86(name);
> +	else if (!strcmp(norm_arch, "arm"))
>  		return ins__find_arm(name);
> -	} else if (!strcmp(norm_arch, "powerpc")) {
> +	else if (!strcmp(norm_arch, "powerpc"))
>  		return ins__find_powerpc(name);
> -	} else {
> +	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);
> +	return NULL;
>  }
>  
>  int symbol__alloc_hist(struct symbol *sym)
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 7ba3579..91b705a 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -42,11 +42,30 @@ struct ins_ops {
>  			 struct ins_operands *ops);
>  };
>  
> +/* Defined in annotate.c */
> +extern struct ins_ops mov_ops;
> +extern struct ins_ops call_ops;
> +extern struct ins_ops dec_ops;
> +extern struct ins_ops jump_ops;
> +extern struct ins_ops lock_ops;
> +extern struct ins_ops nop_ops;
> +extern struct ins_ops ret_ops;
> +
>  struct ins {
>  	const char     *name;
>  	struct ins_ops *ops;
>  };
>  
> +struct instructions_arch {
> +	struct ins *ins;
> +	struct list_head list;
> +};
> +
> +struct ins *list_add__ins_arch(struct instructions_arch *head,
> +			       const char *name, struct ins_ops *ops);
> +struct ins *list_search__ins_arch(struct instructions_arch *head,
> +				  const char *name);
> +
>  bool ins__is_jump(const struct ins *ins);
>  bool ins__is_call(const struct ins *ins);
>  bool ins__is_ret(const struct ins *ins);
> @@ -210,4 +229,8 @@ static inline int symbol__tui_annotate(struct symbol *sym __maybe_unused,
>  
>  extern const char	*disassembler_style;
>  
> +struct ins *ins__find_x86(const char *name);
> +struct ins *ins__find_arm(const char *name);
> +struct ins *ins__find_powerpc(const char *name);
> +
>  #endif	/* __PERF_ANNOTATE_H */
> diff --git a/tools/perf/util/annotate/Build b/tools/perf/util/annotate/Build
> new file mode 100644
> index 0000000..ed87f1c
> --- /dev/null
> +++ b/tools/perf/util/annotate/Build
> @@ -0,0 +1,3 @@
> +libperf-y += x86.o
> +libperf-y += arm.o
> +libperf-y += powerpc.o
> diff --git a/tools/perf/util/annotate/arm.c b/tools/perf/util/annotate/arm.c
> new file mode 100644
> index 0000000..6bfc866
> --- /dev/null
> +++ b/tools/perf/util/annotate/arm.c
> @@ -0,0 +1,50 @@
> +#include "../annotate.h"
> +
> +static regex_t arm_call_insn, arm_jump_insn;
> +
> +#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
> +
> +static __attribute__((constructor)) void init_regex(void)
> +{
> +	int ret;
> +
> +	ret = regcomp(&arm_call_insn, "^blx?" ARM_CONDS "?$", REG_EXTENDED);
> +	ret |= regcomp(&arm_jump_insn, "^bx?" ARM_CONDS "?$", REG_EXTENDED);
> +
> +	if (ret)
> +		pr_err("regcomp failed\n");
> +}
> +
> +struct ins *ins__find_arm(const char *name)
> +{
> +	struct ins *ins;
> +	struct ins_ops *ops = &mov_ops;
> +	regmatch_t match[2];
> +	int ret;
> +	static struct instructions_arch head = {
> +		.list = LIST_HEAD_INIT(head.list),
> +	};
> +
> +	/*
> +	 * Return if we already have object of 'struct ins' for this instruction
> +	 */
> +	ins = list_search__ins_arch(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ret = regexec(&arm_call_insn, name, 2, match, 0);
> +	if (!ret) {
> +		ops = &call_ops;
> +	} else {
> +		ret = regexec(&arm_jump_insn, name, 2, match, 0);
> +		if (!ret)
> +			ops = &jump_ops;
> +	}
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_arch(&head, name, ops);
> +
> +}
> diff --git a/tools/perf/util/annotate/powerpc.c b/tools/perf/util/annotate/powerpc.c
> new file mode 100644
> index 0000000..6088ce2
> --- /dev/null
> +++ b/tools/perf/util/annotate/powerpc.c
> @@ -0,0 +1,63 @@
> +#include "../annotate.h"
> +
> +struct ins *ins__find_powerpc(const char *name)
> +{
> +	int i;
> +	struct ins *ins;
> +	struct ins_ops *ops;
> +	static struct instructions_arch head = {
> +		.list = LIST_HEAD_INIT(head.list),
> +	};
> +
> +	/*
> +	 * - Interested only if instruction starts with 'b'.
> +	 * - Few start with 'b', but aren't branch instructions.
> +	 */
> +	if (name[0] != 'b'             ||
> +	    !strncmp(name, "bcd", 3)   ||
> +	    !strncmp(name, "brinc", 5) ||
> +	    !strncmp(name, "bper", 4))
> +		return NULL;
> +
> +	/*
> +	 * Return if we already have object of 'struct ins' for this instruction
> +	 */
> +	ins = list_search__ins_arch(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ops = &jump_ops;
> +
> +	i = strlen(name) - 1;
> +	if (i < 0)
> +		return NULL;
> +
> +	/* ignore optional hints at the end of the instructions */
> +	if (name[i] == '+' || name[i] == '-')
> +		i--;
> +
> +	if (name[i] == 'l' || (name[i] == 'a' && name[i-1] == 'l')) {
> +		/*
> +		 * if the instruction ends up with 'l' or 'la', then
> +		 * those are considered 'calls' since they update LR.
> +		 * ... except for 'bnl' which is branch if not less than
> +		 * and the absolute form of the same.
> +		 */
> +		if (strcmp(name, "bnl") && strcmp(name, "bnl+") &&
> +		    strcmp(name, "bnl-") && strcmp(name, "bnla") &&
> +		    strcmp(name, "bnla+") && strcmp(name, "bnla-"))
> +			ops = &call_ops;
> +	}
> +	if (name[i] == 'r' && name[i-1] == 'l')
> +		/*
> +		 * instructions ending with 'lr' are considered to be
> +		 * return instructions
> +		 */
> +		ops = &ret_ops;
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_arch(&head, name, ops);
> +}
> diff --git a/tools/perf/util/annotate/x86.c b/tools/perf/util/annotate/x86.c
> new file mode 100644
> index 0000000..b519197
> --- /dev/null
> +++ b/tools/perf/util/annotate/x86.c
> @@ -0,0 +1,107 @@
> +#include "../annotate.h"
> +
> +static struct ins instructions[] = {
> +	{ .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 = "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 int ins__cmp(const void *a, const void *b)
> +{
> +	const struct ins *ia = a;
> +	const struct ins *ib = b;
> +
> +	return strcmp(ia->name, ib->name);
> +}
> +
> +static __attribute__((constructor)) void ins__sort(void)
> +{
> +	qsort(instructions, ARRAY_SIZE(instructions), sizeof(struct ins),
> +	      ins__cmp);
> +}
> +
> +static int ins__key_cmp(const void *name, const void *insp)
> +{
> +	const struct ins *ins = insp;
> +
> +	return strcmp(name, ins->name);
> +}
> +
> +struct ins *ins__find_x86(const char *name)
> +{
> +	return bsearch(name, instructions, ARRAY_SIZE(instructions),
> +		       sizeof(struct ins), ins__key_cmp);
> +}
> -- 
> 2.5.5

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Naveen N. Rao]

On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
> > Move arch specific stuff from util/annotate.c to their respective
> > files in util/annotate directory.
> > 
> > No functionality changes.
> > 
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> > ---
> >  tools/perf/util/Build              |   1 +
> >  tools/perf/util/annotate.c         | 259 +++----------------------------------
> >  tools/perf/util/annotate.h         |  23 ++++
> >  tools/perf/util/annotate/Build     |   3 +
> >  tools/perf/util/annotate/arm.c     |  50 +++++++
> >  tools/perf/util/annotate/powerpc.c |  63 +++++++++
> >  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
> 
> We already have a per arch area: tools/perf/arch/

I think this was done to support cross-arch annotate similar to the 
remote unwind support with util/libunwind/

- Naveen

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Ravi Bangoria]

On Monday 10 October 2016 10:09 PM, Naveen N. Rao wrote:
> On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
>> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
>>> Move arch specific stuff from util/annotate.c to their respective
>>> files in util/annotate directory.
>>>
>>> No functionality changes.
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>>> ---
>>>  tools/perf/util/Build              |   1 +
>>>  tools/perf/util/annotate.c         | 259 +++----------------------------------
>>>  tools/perf/util/annotate.h         |  23 ++++
>>>  tools/perf/util/annotate/Build     |   3 +
>>>  tools/perf/util/annotate/arm.c     |  50 +++++++
>>>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
>>>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
>> We already have a per arch area: tools/perf/arch/
> I think this was done to support cross-arch annotate similar to the 
> remote unwind support with util/libunwind/

Yes, because tools/perf/arch/ will only include host arch code.

-Ravi

> - Naveen
>

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Arnaldo Carvalho de Melo]

Em Mon, Oct 10, 2016 at 10:19:28PM +0530, Ravi Bangoria escreveu:
> 
> 
> On Monday 10 October 2016 10:09 PM, Naveen N. Rao wrote:
> > On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
> >> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
> >>> Move arch specific stuff from util/annotate.c to their respective
> >>> files in util/annotate directory.
> >>>
> >>> No functionality changes.
> >>>
> >>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> >>> ---
> >>>  tools/perf/util/Build              |   1 +
> >>>  tools/perf/util/annotate.c         | 259 +++----------------------------------
> >>>  tools/perf/util/annotate.h         |  23 ++++
> >>>  tools/perf/util/annotate/Build     |   3 +
> >>>  tools/perf/util/annotate/arm.c     |  50 +++++++
> >>>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
> >>>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
> >> We already have a per arch area: tools/perf/arch/
> > I think this was done to support cross-arch annotate similar to the 
> > remote unwind support with util/libunwind/
> 
> Yes, because tools/perf/arch/ will only include host arch code.

Ok, thanks for clarifying.

- Arnaldo

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Ravi Bangoria]

On Monday 10 October 2016 10:26 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 10, 2016 at 10:19:28PM +0530, Ravi Bangoria escreveu:
>>
>> On Monday 10 October 2016 10:09 PM, Naveen N. Rao wrote:
>>> On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
>>>> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
>>>>> Move arch specific stuff from util/annotate.c to their respective
>>>>> files in util/annotate directory.
>>>>>
>>>>> No functionality changes.
>>>>>
>>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>>>>> ---
>>>>>  tools/perf/util/Build              |   1 +
>>>>>  tools/perf/util/annotate.c         | 259 +++----------------------------------
>>>>>  tools/perf/util/annotate.h         |  23 ++++
>>>>>  tools/perf/util/annotate/Build     |   3 +
>>>>>  tools/perf/util/annotate/arm.c     |  50 +++++++
>>>>>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
>>>>>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
>>>> We already have a per arch area: tools/perf/arch/
>>> I think this was done to support cross-arch annotate similar to the 
>>> remote unwind support with util/libunwind/
>> Yes, because tools/perf/arch/ will only include host arch code.
> Ok, thanks for clarifying.

Hi Arnaldo,

Are you ok with this patchset. Please let me know if you want to respin it.

-Ravi

> - Arnaldo
>

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Arnaldo Carvalho de Melo]

Em Mon, Oct 17, 2016 at 08:13:38PM +0530, Ravi Bangoria escreveu:
> 
> 
> On Monday 10 October 2016 10:26 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Oct 10, 2016 at 10:19:28PM +0530, Ravi Bangoria escreveu:
> >>
> >> On Monday 10 October 2016 10:09 PM, Naveen N. Rao wrote:
> >>> On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
> >>>> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
> >>>>> Move arch specific stuff from util/annotate.c to their respective
> >>>>> files in util/annotate directory.
> >>>>>
> >>>>> No functionality changes.
> >>>>>
> >>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> >>>>> ---
> >>>>>  tools/perf/util/Build              |   1 +
> >>>>>  tools/perf/util/annotate.c         | 259 +++----------------------------------
> >>>>>  tools/perf/util/annotate.h         |  23 ++++
> >>>>>  tools/perf/util/annotate/Build     |   3 +
> >>>>>  tools/perf/util/annotate/arm.c     |  50 +++++++
> >>>>>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
> >>>>>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
> >>>> We already have a per arch area: tools/perf/arch/
> >>> I think this was done to support cross-arch annotate similar to the 
> >>> remote unwind support with util/libunwind/
> >> Yes, because tools/perf/arch/ will only include host arch code.
> > Ok, thanks for clarifying.
> 
> Hi Arnaldo,
> 
> Are you ok with this patchset. Please let me know if you want to respin it.

We're in the merge window, and I'm having to deal with other patches
requiring more work than I was expecting to invest on them, so no time
to revisit this, sorry, but I have not forgot about it, will get back to
it as time permits,

Thanks,

- Arnaldo
 
> -Ravi
> 
> > - Arnaldo
> >

Re: [PATCH] perf annotate: Cleanup arch specific stuff

[Ravi Bangoria]

On Monday 17 October 2016 08:15 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Oct 17, 2016 at 08:13:38PM +0530, Ravi Bangoria escreveu:
>>
>> On Monday 10 October 2016 10:26 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Oct 10, 2016 at 10:19:28PM +0530, Ravi Bangoria escreveu:
>>>> On Monday 10 October 2016 10:09 PM, Naveen N. Rao wrote:
>>>>> On 2016/10/10 01:24PM, Arnaldo Carvalho de Melo wrote:
>>>>>> Em Mon, Oct 10, 2016 at 07:29:02PM +0530, Ravi Bangoria escreveu:
>>>>>>> Move arch specific stuff from util/annotate.c to their respective
>>>>>>> files in util/annotate directory.
>>>>>>>
>>>>>>> No functionality changes.
>>>>>>>
>>>>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>  tools/perf/util/Build              |   1 +
>>>>>>>  tools/perf/util/annotate.c         | 259 +++----------------------------------
>>>>>>>  tools/perf/util/annotate.h         |  23 ++++
>>>>>>>  tools/perf/util/annotate/Build     |   3 +
>>>>>>>  tools/perf/util/annotate/arm.c     |  50 +++++++
>>>>>>>  tools/perf/util/annotate/powerpc.c |  63 +++++++++
>>>>>>>  tools/perf/util/annotate/x86.c     | 107 +++++++++++++++
>>>>>> We already have a per arch area: tools/perf/arch/
>>>>> I think this was done to support cross-arch annotate similar to the 
>>>>> remote unwind support with util/libunwind/
>>>> Yes, because tools/perf/arch/ will only include host arch code.
>>> Ok, thanks for clarifying.
>> Hi Arnaldo,
>>
>> Are you ok with this patchset. Please let me know if you want to respin it.
> We're in the merge window, and I'm having to deal with other patches
> requiring more work than I was expecting to invest on them, so no time
> to revisit this, sorry, but I have not forgot about it, will get back to
> it as time permits,

Sure, no issues. :)

-Ravi

Re: [PATCH v7 0/6] perf annotate: Cross arch support + few fixes

[Naveen N. Rao]

On 2016/09/21 09:17PM, Ravi Bangoria wrote:
> Currently Perf annotate support code navigation (branches and calls)
> only when run on the same architecture where perf.data was recorded.
> But, for example, record on powerpc server and annotate on client's
> x86 desktop is not supported.
> 
> This patchset adds supports for that.
> 
> Example:
> 
>   Record on powerpc:
>   $ ./perf record -a
> 
>   Report -> Annotate on x86:
>   $ ./perf report -i perf.data.powerpc --vmlinux vmlinux.powerpc

Hi Arnaldo,
Can you please take this in for v4.10? We'd very much like to use this 
for better code annotation and browsing. FWIW, I have tested this on 
powerpc.

Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>


Thanks,
Naveen