[PATCH v5 0/7] perf: Cross arch annotate + few miscellaneous fixes

[PATCH v5 0/7] perf: Cross arch annotate + few miscellaneous 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 enables cross arch annotate. Currently I've used x86
and arm instructions which are already available and added support
for powerpc.

Additionally this patch series also contains few other related fixes.

Patches are prepared on top of acme/perf/core and tested it with x86
and powerpc only.

Note for arm:
Few instructions were defined under #if __arm__ which I've used as a
table for arm. But I'm not sure whether instruction defined outside of
that also contains arm instructions. Apart from that, 'call__parse()'
and 'move__parse()' contains #ifdef __arm__ directive. I've changed it
to  if (!strcmp(norm_arch, arm)). I don't have a arm machine to test
these changes.

Example:

  Record on powerpc:
  $ ./perf record -a

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

Changes in v5:
  - Replaced symbol__annotate with symbol__disassemble.
  - Removed hacks for jump and call instructions like bctr and bctrl
    respectively from generic patch that enables support for powerpc
    and made separate patch for that.
  - v4 was not annotating powerpc 'btar' instruction. Included that.
  - Added few generic fixes.

v4 link:
  https://lkml.org/lkml/2016/7/8/10

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

Ravi Bangoria (6):
  perf: Define macro for normalized arch names
  perf annotate: Add cross arch annotate support
  perf annotate: Do not ignore call instruction with indirect target
  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/arch/common.c           |  36 ++---
 tools/perf/arch/common.h           |  11 ++
 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         | 276 +++++++++++++++++++++++++++++--------
 tools/perf/util/annotate.h         |  10 +-
 tools/perf/util/unwind-libunwind.c |   4 +-
 8 files changed, 262 insertions(+), 87 deletions(-)

-- 
2.5.5

[PATCH v5 1/7] perf: Define macro for normalized arch names

[Ravi Bangoria]

Define macro for each normalized arch name and use them instead
of using arch name as string.

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

 tools/perf/arch/common.c           | 36 ++++++++++++++++++------------------
 tools/perf/arch/common.h           | 11 +++++++++++
 tools/perf/util/unwind-libunwind.c |  4 ++--
 3 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/tools/perf/arch/common.c b/tools/perf/arch/common.c
index 886dd2a..f763666 100644
--- a/tools/perf/arch/common.c
+++ b/tools/perf/arch/common.c
@@ -123,25 +123,25 @@ static int lookup_triplets(const char *const *triplets, const char *name)
 const char *normalize_arch(char *arch)
 {
 	if (!strcmp(arch, "x86_64"))
-		return "x86";
+		return NORM_X86;
 	if (arch[0] == 'i' && arch[2] == '8' && arch[3] == '6')
-		return "x86";
+		return NORM_X86;
 	if (!strcmp(arch, "sun4u") || !strncmp(arch, "sparc", 5))
-		return "sparc";
+		return NORM_SPARC;
 	if (!strcmp(arch, "aarch64") || !strcmp(arch, "arm64"))
-		return "arm64";
+		return NORM_ARM64;
 	if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110"))
-		return "arm";
+		return NORM_ARM;
 	if (!strncmp(arch, "s390", 4))
-		return "s390";
+		return NORM_S390;
 	if (!strncmp(arch, "parisc", 6))
-		return "parisc";
+		return NORM_PARISC;
 	if (!strncmp(arch, "powerpc", 7) || !strncmp(arch, "ppc", 3))
-		return "powerpc";
+		return NORM_POWERPC;
 	if (!strncmp(arch, "mips", 4))
-		return "mips";
+		return NORM_MIPS;
 	if (!strncmp(arch, "sh", 2) && isdigit(arch[2]))
-		return "sh";
+		return NORM_SH;
 
 	return arch;
 }
@@ -181,21 +181,21 @@ static int perf_env__lookup_binutils_path(struct perf_env *env,
 		zfree(&buf);
 	}
 
-	if (!strcmp(arch, "arm"))
+	if (!strcmp(arch, NORM_ARM))
 		path_list = arm_triplets;
-	else if (!strcmp(arch, "arm64"))
+	else if (!strcmp(arch, NORM_ARM64))
 		path_list = arm64_triplets;
-	else if (!strcmp(arch, "powerpc"))
+	else if (!strcmp(arch, NORM_POWERPC))
 		path_list = powerpc_triplets;
-	else if (!strcmp(arch, "sh"))
+	else if (!strcmp(arch, NORM_SH))
 		path_list = sh_triplets;
-	else if (!strcmp(arch, "s390"))
+	else if (!strcmp(arch, NORM_S390))
 		path_list = s390_triplets;
-	else if (!strcmp(arch, "sparc"))
+	else if (!strcmp(arch, NORM_SPARC))
 		path_list = sparc_triplets;
-	else if (!strcmp(arch, "x86"))
+	else if (!strcmp(arch, NORM_X86))
 		path_list = x86_triplets;
-	else if (!strcmp(arch, "mips"))
+	else if (!strcmp(arch, NORM_MIPS))
 		path_list = mips_triplets;
 	else {
 		ui__error("binutils for %s not supported.\n", arch);
diff --git a/tools/perf/arch/common.h b/tools/perf/arch/common.h
index 6b01c73..14ca8ca 100644
--- a/tools/perf/arch/common.h
+++ b/tools/perf/arch/common.h
@@ -5,6 +5,17 @@
 
 extern const char *objdump_path;
 
+/* Macro for normalized arch names */
+#define NORM_X86	"x86"
+#define NORM_SPARC	"sparc"
+#define NORM_ARM64	"arm64"
+#define NORM_ARM	"arm"
+#define NORM_S390	"s390"
+#define NORM_PARISC	"parisc"
+#define NORM_POWERPC	"powerpc"
+#define NORM_MIPS	"mips"
+#define NORM_SH		"sh"
+
 int perf_env__lookup_objdump(struct perf_env *env);
 const char *normalize_arch(char *arch);
 
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index 6d542a4..6199102 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -40,10 +40,10 @@ int unwind__prepare_access(struct thread *thread, struct map *map,
 
 	arch = normalize_arch(thread->mg->machine->env->arch);
 
-	if (!strcmp(arch, "x86")) {
+	if (!strcmp(arch, NORM_X86)) {
 		if (dso_type != DSO__TYPE_64BIT)
 			ops = x86_32_unwind_libunwind_ops;
-	} else if (!strcmp(arch, "arm64") || !strcmp(arch, "arm")) {
+	} else if (!strcmp(arch, NORM_ARM64) || !strcmp(arch, NORM_ARM)) {
 		if (dso_type == DSO__TYPE_64BIT)
 			ops = arm64_unwind_libunwind_ops;
 	}
-- 
2.5.5

[PATCH v5 2/7] 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.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v5:
  - Replaced symbol__annotate with symbol__disassemble.

 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        | 133 ++++++++++++++++++++++++--------------
 tools/perf/util/annotate.h        |   5 +-
 5 files changed, 92 insertions(+), 53 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index a3223aa..fdd4203 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 2e2d100..21c5e10 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 25a9259..deb9af0 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -20,12 +20,14 @@
 #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 struct ins *ins__find(const char *name);
+static struct ins *ins__find(const char *name, const char *norm_arch);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
 
 static void ins__delete(struct ins_operands *ops)
@@ -53,7 +55,7 @@ int ins__scnprintf(struct ins *ins, char *bf, size_t size,
 	return ins__raw_scnprintf(ins, bf, size, ops);
 }
 
-static int call__parse(struct ins_operands *ops)
+static int call__parse(struct ins_operands *ops, const char *norm_arch)
 {
 	char *endptr, *tok, *name;
 
@@ -65,10 +67,8 @@ static int call__parse(struct ins_operands *ops)
 
 	name++;
 
-#ifdef __arm__
-	if (strchr(name, '+'))
+	if (!strcmp(norm_arch, NORM_ARM) && strchr(name, '+'))
 		return -1;
-#endif
 
 	tok = strchr(name, '>');
 	if (tok == NULL)
@@ -117,7 +117,8 @@ bool ins__is_call(const struct ins *ins)
 	return ins->ops == &call_ops;
 }
 
-static int jump__parse(struct ins_operands *ops)
+static int jump__parse(struct ins_operands *ops,
+		       const char *norm_arch __maybe_unused)
 {
 	const char *s = strchr(ops->raw, '+');
 
@@ -172,7 +173,7 @@ static int comment__symbol(char *raw, char *comment, u64 *addrp, char **namep)
 	return 0;
 }
 
-static int lock__parse(struct ins_operands *ops)
+static int lock__parse(struct ins_operands *ops, const char *norm_arch)
 {
 	char *name;
 
@@ -183,7 +184,7 @@ static int lock__parse(struct ins_operands *ops)
 	if (disasm_line__parse(ops->raw, &name, &ops->locked.ops->raw) < 0)
 		goto out_free_ops;
 
-	ops->locked.ins = ins__find(name);
+	ops->locked.ins = ins__find(name, norm_arch);
 	free(name);
 
 	if (ops->locked.ins == NULL)
@@ -193,7 +194,7 @@ static int lock__parse(struct ins_operands *ops)
 		return 0;
 
 	if (ops->locked.ins->ops->parse &&
-	    ops->locked.ins->ops->parse(ops->locked.ops) < 0)
+	    ops->locked.ins->ops->parse(ops->locked.ops, norm_arch) < 0)
 		goto out_free_ops;
 
 	return 0;
@@ -236,7 +237,7 @@ static struct ins_ops lock_ops = {
 	.scnprintf = lock__scnprintf,
 };
 
-static int mov__parse(struct ins_operands *ops)
+static int mov__parse(struct ins_operands *ops, const char *norm_arch)
 {
 	char *s = strchr(ops->raw, ','), *target, *comment, prev;
 
@@ -251,11 +252,11 @@ static int mov__parse(struct ins_operands *ops)
 		return -1;
 
 	target = ++s;
-#ifdef __arm__
-	comment = strchr(s, ';');
-#else
-	comment = strchr(s, '#');
-#endif
+
+	if (!strcmp(norm_arch, NORM_ARM))
+		comment = strchr(s, ';');
+	else
+		comment = strchr(s, '#');
 
 	if (comment != NULL)
 		s = comment - 1;
@@ -303,7 +304,8 @@ static struct ins_ops mov_ops = {
 	.scnprintf = mov__scnprintf,
 };
 
-static int dec__parse(struct ins_operands *ops)
+static int dec__parse(struct ins_operands *ops,
+		      const char *norm_arch __maybe_unused)
 {
 	char *target, *comment, *s, prev;
 
@@ -363,26 +365,12 @@ 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 = "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, },
-#endif
 	{ .name = "bts",   .ops  = &mov_ops, },
 	{ .name = "call",  .ops  = &call_ops, },
 	{ .name = "callq", .ops  = &call_ops, },
@@ -456,6 +444,21 @@ static struct ins instructions[] = {
 	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
+static struct ins instructions_arm[] = {
+	{ .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, },
+};
+
 static int ins__key_cmp(const void *name, const void *insp)
 {
 	const struct ins *ins = insp;
@@ -471,24 +474,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 struct ins *ins__find(const char *name)
+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 char *norm_arch)
 {
-	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, NORM_X86)) {
+		instructions = instructions_x86;
+		nmemb = ARRAY_SIZE(instructions_x86);
+	} else if (!strcmp(norm_arch, NORM_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__annotate_init(struct map *map __maybe_unused, struct symbol *sym)
@@ -715,17 +742,24 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip)
 	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip);
 }
 
-static void disasm_line__init_ins(struct disasm_line *dl)
+static void disasm_line__init_ins(struct disasm_line *dl, char *arch)
 {
-	dl->ins = ins__find(dl->name);
+	const char *norm_arch = annotate__norm_arch(arch);
+
+	if (!norm_arch) {
+		pr_err("Can not annotate. Could not determine architecture.");
+		return;
+	}
 
+	dl->ins = ins__find(dl->name, norm_arch);
 	if (dl->ins == NULL)
 		return;
 
 	if (!dl->ins->ops)
 		return;
 
-	if (dl->ins->ops->parse && dl->ins->ops->parse(&dl->ops) < 0)
+	if (dl->ins->ops->parse &&
+	    dl->ins->ops->parse(&dl->ops, norm_arch) < 0)
 		dl->ins = NULL;
 }
 
@@ -767,7 +801,8 @@ out_free_name:
 }
 
 static struct disasm_line *disasm_line__new(s64 offset, char *line,
-					size_t privsize, int line_nr)
+					size_t privsize, int line_nr,
+					char *arch)
 {
 	struct disasm_line *dl = zalloc(sizeof(*dl) + privsize);
 
@@ -782,7 +817,7 @@ static struct disasm_line *disasm_line__new(s64 offset, char *line,
 			if (disasm_line__parse(dl->line, &dl->name, &dl->ops.raw) < 0)
 				goto out_free_line;
 
-			disasm_line__init_ins(dl);
+			disasm_line__init_ins(dl, arch);
 		}
 	}
 
@@ -1005,7 +1040,7 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
  */
 static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 				      FILE *file, size_t privsize,
-				      int *line_nr)
+				      int *line_nr, char *arch)
 {
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *dl;
@@ -1066,7 +1101,8 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 			parsed_line = tmp2 + 1;
 	}
 
-	dl = disasm_line__new(offset, parsed_line, privsize, *line_nr);
+	dl = disasm_line__new(offset, parsed_line, privsize,
+			      *line_nr, arch);
 	free(line);
 	(*line_nr)++;
 
@@ -1197,7 +1233,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];
@@ -1313,7 +1350,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, size_t privsize)
 	nline = 0;
 	while (!feof(file)) {
 		if (symbol__parse_objdump_line(sym, map, file, privsize,
-			    &lineno) < 0)
+			    &lineno, arch) < 0)
 			break;
 		nline++;
 	}
@@ -1710,7 +1747,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 f67ccb0..5cfad4e 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -36,7 +36,7 @@ struct ins_operands {
 
 struct ins_ops {
 	void (*free)(struct ins_operands *ops);
-	int (*parse)(struct ins_operands *ops);
+	int (*parse)(struct ins_operands *ops, const char *norm_arch);
 	int (*scnprintf)(struct ins *ins, char *bf, size_t size,
 			 struct ins_operands *ops);
 };
@@ -155,7 +155,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 v5 3/7] 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 v5:
  - Removed hacks for instructions like bctr and bctrl from this patch.

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

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index deb9af0..0b64841 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -459,6 +459,11 @@ static struct ins instructions_arm[] = {
 	{ .name = "bne",   .ops  = &jump_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;
@@ -474,6 +479,115 @@ 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;
+	static bool list_initialized;
+
+	/*
+	 * - 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;
+
+	if (!list_initialized) {
+		INIT_LIST_HEAD(&head.list);
+		list_initialized = true;
+	}
+
+	/*
+	 * 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);
@@ -509,6 +623,8 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
 	} else if (!strcmp(norm_arch, NORM_ARM)) {
 		instructions = instructions_arm;
 		nmemb = ARRAY_SIZE(instructions_arm);
+	} else if (!strcmp(norm_arch, NORM_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 v5 4/7] perf annotate: Do not ignore call instruction with indirect target

[Ravi Bangoria]

Do not ignore call instruction with indirect target when its already
identified as a call. This is an extension of commit e8ea1561952b
("perf annotate: Use raw form for register indirect call instructions")
to generalize annotation for all instructions with indirect calls.

This is needed for certain powerpc call instructions that use address
in a register (such as bctrl, btarl, ...).

Apart from that, when kcore is used to disassemble function, all call
instructions were ignored. This patch will fix it as a side effect by
not ignoring them. For example,

Before (with kcore):
       mov    %r13,%rdi
       callq  0xffffffff811a7e70
     ^ jmpq   64
       mov    %gs:0x7ef41a6e(%rip),%al

After (with kcore):
       mov    %r13,%rdi
     > callq  0xffffffff811a7e70
     ^ jmpq   64
       mov    %gs:0x7ef41a6e(%rip),%al

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
[Suggested about 'bctrl' instruction]
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v5:
  - New patch, introduced to annotate all indirect call instructions.

 tools/perf/util/annotate.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 0b64841..6368ba9 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -81,16 +81,12 @@ static int call__parse(struct ins_operands *ops, const char *norm_arch)
 	return ops->target.name == NULL ? -1 : 0;
 
 indirect_call:
-	tok = strchr(endptr, '(');
-	if (tok != NULL) {
+	tok = strchr(endptr, '*');
+	if (tok == NULL) {
 		ops->target.addr = 0;
 		return 0;
 	}
 
-	tok = strchr(endptr, '*');
-	if (tok == NULL)
-		return -1;
-
 	ops->target.addr = strtoull(tok + 1, NULL, 16);
 	return 0;
 }
-- 
2.5.5

[PATCH v5 5/7] 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, ...).

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v5:
  - New patch introduced to annotate jump instruction with indirect target

 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 6368ba9..4a4a583 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -131,6 +131,9 @@ static int jump__parse(struct ins_operands *ops,
 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 v5 6/7] 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, 'beq  cr7,10173e60'.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
Changes in v5:
  - New patch

 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 4a4a583..678fb81 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -117,8 +117,12 @@ static int jump__parse(struct ins_operands *ops,
 		       const char *norm_arch __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 v5 7/7] 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 v5:
  - New patch

 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 21c5e10..c13df5b 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 678fb81..c8b017c 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -124,10 +124,12 @@ static int jump__parse(struct ins_operands *ops,
 	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;
 }
@@ -135,7 +137,7 @@ static int jump__parse(struct ins_operands *ops,
 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);
@@ -1228,9 +1230,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 5cfad4e..5787ed8 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 v5 2/7] perf annotate: Add cross arch annotate support

[Russell King - ARM Linux]

On Fri, Aug 19, 2016 at 10:59:01AM +0530, Ravi Bangoria wrote:
> -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 = "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, },
> -#endif

Notice that ARM includes a lot of other instructions from this table,
not just those above.

>  	{ .name = "bts",   .ops  = &mov_ops, },
>  	{ .name = "call",  .ops  = &call_ops, },
>  	{ .name = "callq", .ops  = &call_ops, },
> @@ -456,6 +444,21 @@ static struct ins instructions[] = {
>  	{ .name = "retq",  .ops  = &ret_ops, },
>  };
>  
> +static struct ins instructions_arm[] = {
> +	{ .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, },
> +};
> +
...
> +	if (!strcmp(norm_arch, NORM_X86)) {
> +		instructions = instructions_x86;
> +		nmemb = ARRAY_SIZE(instructions_x86);
> +	} else if (!strcmp(norm_arch, NORM_ARM)) {
> +		instructions = instructions_arm;
> +		nmemb = ARRAY_SIZE(instructions_arm);

But these changes result in _only_ the ones that were in the #if __arm__
being matched.  This is wrong.

If we want to go that way, we need to add _all_ arm instructions to
instructions_arm, not just those within the #if.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v5 2/7] perf annotate: Add cross arch annotate support

[Ravi Bangoria]

Thanks Russell for reviewing.

On Friday 19 August 2016 01:20 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 19, 2016 at 10:59:01AM +0530, Ravi Bangoria wrote:
>> -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 = "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, },
>> -#endif
> Notice that ARM includes a lot of other instructions from this table,
> not just those above.
>
>>  	{ .name = "bts",   .ops  = &mov_ops, },
>>  	{ .name = "call",  .ops  = &call_ops, },
>>  	{ .name = "callq", .ops  = &call_ops, },
>> @@ -456,6 +444,21 @@ static struct ins instructions[] = {
>>  	{ .name = "retq",  .ops  = &ret_ops, },
>>  };
>>  
>> +static struct ins instructions_arm[] = {
>> +	{ .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, },
>> +};
>> +
> ...
>> +	if (!strcmp(norm_arch, NORM_X86)) {
>> +		instructions = instructions_x86;
>> +		nmemb = ARRAY_SIZE(instructions_x86);
>> +	} else if (!strcmp(norm_arch, NORM_ARM)) {
>> +		instructions = instructions_arm;
>> +		nmemb = ARRAY_SIZE(instructions_arm);
> But these changes result in _only_ the ones that were in the #if __arm__
> being matched.  This is wrong.
>
> If we want to go that way, we need to add _all_ arm instructions to
> instructions_arm, not just those within the #if.

Yes, I've mentioned same in cover letter as well.

Can I add all x86 instructions for arm as well? If not, can you please provide
a list of arm instructions that needs to be added here.

-Ravi

Re: [PATCH v5 2/7] perf annotate: Add cross arch annotate support

[Russell King - ARM Linux]

On Fri, Aug 19, 2016 at 04:09:51PM +0530, Ravi Bangoria wrote:
> Thanks Russell for reviewing.
> 
> On Friday 19 August 2016 01:20 PM, Russell King - ARM Linux wrote:
> > On Fri, Aug 19, 2016 at 10:59:01AM +0530, Ravi Bangoria wrote:
> >> -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 = "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, },
> >> -#endif
> > Notice that ARM includes a lot of other instructions from this table,
> > not just those above.
> >
> >>  	{ .name = "bts",   .ops  = &mov_ops, },
> >>  	{ .name = "call",  .ops  = &call_ops, },
> >>  	{ .name = "callq", .ops  = &call_ops, },
> >> @@ -456,6 +444,21 @@ static struct ins instructions[] = {
> >>  	{ .name = "retq",  .ops  = &ret_ops, },
> >>  };
> >>  
> >> +static struct ins instructions_arm[] = {
> >> +	{ .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, },
> >> +};
> >> +
> > ...
> >> +	if (!strcmp(norm_arch, NORM_X86)) {
> >> +		instructions = instructions_x86;
> >> +		nmemb = ARRAY_SIZE(instructions_x86);
> >> +	} else if (!strcmp(norm_arch, NORM_ARM)) {
> >> +		instructions = instructions_arm;
> >> +		nmemb = ARRAY_SIZE(instructions_arm);
> > But these changes result in _only_ the ones that were in the #if __arm__
> > being matched.  This is wrong.
> >
> > If we want to go that way, we need to add _all_ arm instructions to
> > instructions_arm, not just those within the #if.
> 
> Yes, I've mentioned same in cover letter as well.
> 
> Can I add all x86 instructions for arm as well? If not, can you please
> provide a list of arm instructions that needs to be added here.

If it were me doing a change like this, I'd be trying to preserve the
current behaviour to avoid causing regressions, which would mean
ensuring that all the instructions that were visible before the change
remain visible after the change, even those which are obviously x86
specific but were still in the table anyway.  It then becomes a cleanup
matter later to remove those which aren't relevent, rather than having
to chase around wondering why the tool broke.

I'm afraid I don't have time to look at this (I'm chasing regressions
and bugs in the kernel) so I'd suggest you try to avoid causing
regressions in this tool...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v5 2/7] perf annotate: Add cross arch annotate support

[Ravi Bangoria]

On Friday 19 August 2016 04:18 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 19, 2016 at 04:09:51PM +0530, Ravi Bangoria wrote:
>> Thanks Russell for reviewing.
>>
>> On Friday 19 August 2016 01:20 PM, Russell King - ARM Linux wrote:
>>> On Fri, Aug 19, 2016 at 10:59:01AM +0530, Ravi Bangoria wrote:
>>>> -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 = "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, },
>>>> -#endif
>>> Notice that ARM includes a lot of other instructions from this table,
>>> not just those above.
>>>
>>>>  	{ .name = "bts",   .ops  = &mov_ops, },
>>>>  	{ .name = "call",  .ops  = &call_ops, },
>>>>  	{ .name = "callq", .ops  = &call_ops, },
>>>> @@ -456,6 +444,21 @@ static struct ins instructions[] = {
>>>>  	{ .name = "retq",  .ops  = &ret_ops, },
>>>>  };
>>>>  
>>>> +static struct ins instructions_arm[] = {
>>>> +	{ .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, },
>>>> +};
>>>> +
>>> ...
>>>> +	if (!strcmp(norm_arch, NORM_X86)) {
>>>> +		instructions = instructions_x86;
>>>> +		nmemb = ARRAY_SIZE(instructions_x86);
>>>> +	} else if (!strcmp(norm_arch, NORM_ARM)) {
>>>> +		instructions = instructions_arm;
>>>> +		nmemb = ARRAY_SIZE(instructions_arm);
>>> But these changes result in _only_ the ones that were in the #if __arm__
>>> being matched.  This is wrong.
>>>
>>> If we want to go that way, we need to add _all_ arm instructions to
>>> instructions_arm, not just those within the #if.
>> Yes, I've mentioned same in cover letter as well.
>>
>> Can I add all x86 instructions for arm as well? If not, can you please
>> provide a list of arm instructions that needs to be added here.
> If it were me doing a change like this, I'd be trying to preserve the
> current behaviour to avoid causing regressions, which would mean
> ensuring that all the instructions that were visible before the change
> remain visible after the change, even those which are obviously x86
> specific but were still in the table anyway.  It then becomes a cleanup
> matter later to remove those which aren't relevent, rather than having
> to chase around wondering why the tool broke.
>
> I'm afraid I don't have time to look at this (I'm chasing regressions
> and bugs in the kernel) so I'd suggest you try to avoid causing
> regressions in this tool...
>

Yes Russell, Fair point. Will send a next series.

-Ravi