[PATCH v3 0/4] perf annotate: Enable cross arch annotate

[PATCH v3 0/4] perf annotate: Enable cross arch annotate

[Ravi Bangoria]

Perf can currently only support code navigation (branches and calls) in
annotate when run on the same architecture where perf.data was recorded.
But cross arch annotate is not supported.

This patchset enables cross arch annotate. Currently I've used x86
and arm instructions which are already available and adding support
for powerpc as well. Adding support for other arch will be easy.

I've created this patch on top of acme/perf/core. And tested it with
x86 and powerpc only.

Example:

  Record on powerpc:
  $ ./perf record -a

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

Changes in v3:
  - Optimized patch that enables annotate on powerpc
  - Corrected one memory leak

v2 link: https://lkml.org/lkml/2016/6/29/278

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

Ravi Bangoria (4):
  perf: Utility function to fetch arch
  perf annotate: Enable cross arch annotate
  perf: Define macro for normalized arch names

 tools/perf/arch/common.c           |  36 ++---
 tools/perf/arch/common.h           |  11 ++
 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         | 260 ++++++++++++++++++++++++++++++-------
 tools/perf/util/annotate.h         |   5 +-
 tools/perf/util/evsel.c            |   7 +
 tools/perf/util/evsel.h            |   2 +
 tools/perf/util/unwind-libunwind.c |   4 +-
 10 files changed, 260 insertions(+), 72 deletions(-)

--
2.5.5

[PATCH v3 1/4] perf: Utility function to fetch arch

[Ravi Bangoria]

Add Utility function to fetch arch using evsel. (evsel->env->arch)

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

 tools/perf/util/evsel.c | 7 +++++++
 tools/perf/util/evsel.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1d8f2bb..0fea724 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2422,3 +2422,10 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			 err, strerror_r(err, sbuf, sizeof(sbuf)),
 			 perf_evsel__name(evsel));
 }
+
+char *perf_evsel__env_arch(struct perf_evsel *evsel)
+{
+	if (evsel && evsel->evlist && evsel->evlist->env)
+		return evsel->evlist->env->arch;
+	return NULL;
+}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 828ddd1..86fed7a 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -435,4 +435,6 @@ typedef int (*attr__fprintf_f)(FILE *, const char *, const char *, void *);
 int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
 			     attr__fprintf_f attr__fprintf, void *priv);
 
+char *perf_evsel__env_arch(struct perf_evsel *evsel);
+
 #endif /* __PERF_EVSEL_H */
-- 
2.5.5

[PATCH v3 2/4] perf annotate: Enable cross arch annotate

[Ravi Bangoria]

Change current data structures and function to enable cross arch
annotate.

Current implementation does not contain logic of record on one arch
and annotating on other. This remote annotate is partially possible
with current implementation for x86 (or may be arm as well) only.
But, to make remote annotation work properly, all architecture
instruction tables need to be included in the perf binary. And while
annotating, look for instruction table where perf.data was recorded.

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)). But I've not
tested this as well.

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

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

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 07fc792..d4fd947 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -128,7 +128,7 @@ static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
 		return err;
 	}
 
-	err = symbol__annotate(sym, map, 0);
+	err = symbol__annotate(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 29dc6d2..3a652a6f 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);
 	}
 
-	if (symbol__annotate(sym, map, sizeof_bdl) < 0) {
+	if (symbol__annotate(sym, map, sizeof_bdl,
+			     perf_evsel__env_arch(evsel)) < 0) {
 		ui__error("%s", ui_helpline__last_msg);
 		goto out_free_offsets;
 	}
diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index 9c7ff8d..d7150b3 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -166,7 +166,7 @@ static int symbol__gtk_annotate(struct symbol *sym, struct map *map,
 	if (map->dso->annotate_warned)
 		return -1;
 
-	if (symbol__annotate(sym, map, 0) < 0) {
+	if (symbol__annotate(sym, map, 0, perf_evsel__env_arch(evsel)) < 0) {
 		ui__error("%s", ui_helpline__current);
 		return -1;
 	}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c385fec..36a5825 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,8 @@ 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,
+		       __maybe_unused const char *norm_arch)
 {
 	char *endptr, *tok, *name;
 
@@ -65,10 +68,8 @@ static int call__parse(struct ins_operands *ops)
 
 	name++;
 
-#ifdef __arm__
-	if (strchr(name, '+'))
+	if (!strcmp(norm_arch, "arm") && strchr(name, '+'))
 		return -1;
-#endif
 
 	tok = strchr(name, '>');
 	if (tok == NULL)
@@ -117,7 +118,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,
+		       __maybe_unused const char *norm_arch)
 {
 	const char *s = strchr(ops->raw, '+');
 
@@ -172,7 +174,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 +185,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 +195,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 +238,8 @@ 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,
+		      __maybe_unused const char *norm_arch)
 {
 	char *s = strchr(ops->raw, ','), *target, *comment, prev;
 
@@ -251,11 +254,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, "arm"))
+		comment = strchr(s, ';');
+	else
+		comment = strchr(s, '#');
 
 	if (comment != NULL)
 		s = comment - 1;
@@ -303,7 +306,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,
+		      __maybe_unused const char *norm_arch)
 {
 	char *target, *comment, *s, prev;
 
@@ -363,26 +367,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 +446,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 +476,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, "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__annotate_init(struct map *map __maybe_unused, struct symbol *sym)
@@ -715,9 +744,16 @@ 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;
@@ -725,7 +761,8 @@ static void disasm_line__init_ins(struct disasm_line *dl)
 	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 +804,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 +820,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 +1043,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 +1104,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)++;
 
@@ -1123,7 +1162,8 @@ static void delete_last_nop(struct symbol *sym)
 	}
 }
 
-int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize)
+int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize,
+		     char *arch)
 {
 	struct dso *dso = map->dso;
 	char *filename = dso__build_id_filename(dso, NULL, 0);
@@ -1271,7 +1311,7 @@ fallback:
 	nline = 0;
 	while (!feof(file)) {
 		if (symbol__parse_objdump_line(sym, map, file, privsize,
-			    &lineno) < 0)
+			    &lineno, arch) < 0)
 			break;
 		nline++;
 	}
@@ -1665,7 +1705,7 @@ int symbol__tty_annotate(struct symbol *sym, struct map *map,
 	struct rb_root source_line = RB_ROOT;
 	u64 len;
 
-	if (symbol__annotate(sym, map, 0) < 0)
+	if (symbol__annotate(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 a23084f..4902138 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__annotate(struct symbol *sym, struct map *map, size_t privsize);
+int symbol__annotate(struct symbol *sym, struct map *map, size_t privsize,
+		     char *arch);
 
 int symbol__annotate_init(struct map *map, struct symbol *sym);
 int symbol__annotate_printf(struct symbol *sym, struct map *map,
-- 
2.5.5

[PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

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

Powerpc has long list of branch instructions and hardcoding them in
table appears to be error-prone. So, add new 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 v3:
  - Optimized code
  - Corrected one memory leak

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

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 36a5825..b87eac7 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -461,6 +461,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;
@@ -476,6 +481,125 @@ 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.
+	 * - Let's also ignore instructions involving 'ctr' and
+	 *   'tar' since target branch addresses for those can't
+	 *   be determined statically.
+	 */
+	if (name[0] != 'b'             ||
+	    !strncmp(name, "bcd", 3)   ||
+	    !strncmp(name, "brinc", 5) ||
+	    !strncmp(name, "bper", 4)  ||
+	    strstr(name, "ctr")        ||
+	    strstr(name, "tar"))
+		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.
+	 */
+	ins = list_add__ins_powerpc(&head, name, ops);
+	if (ins)
+		return ins;
+
+	return NULL;
+}
+
 static void ins__sort(struct ins *instructions, int nmemb)
 {
 	qsort(instructions, nmemb, sizeof(struct ins), ins__cmp);
@@ -511,6 +635,8 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
 	} 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 v3 4/4] 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 v3:
  - No changes

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

diff --git a/tools/perf/arch/common.c b/tools/perf/arch/common.c
index ee69668..feb2113 100644
--- a/tools/perf/arch/common.c
+++ b/tools/perf/arch/common.c
@@ -122,25 +122,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;
 }
@@ -180,21 +180,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/annotate.c b/tools/perf/util/annotate.c
index b87eac7..fce60b4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -68,7 +68,7 @@ static int call__parse(struct ins_operands *ops,
 
 	name++;
 
-	if (!strcmp(norm_arch, "arm") && strchr(name, '+'))
+	if (!strcmp(norm_arch, NORM_ARM) && strchr(name, '+'))
 		return -1;
 
 	tok = strchr(name, '>');
@@ -255,7 +255,7 @@ static int mov__parse(struct ins_operands *ops,
 
 	target = ++s;
 
-	if (!strcmp(norm_arch, "arm"))
+	if (!strcmp(norm_arch, NORM_ARM))
 		comment = strchr(s, ';');
 	else
 		comment = strchr(s, '#');
@@ -629,13 +629,13 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
 		sorted = true;
 	}
 
-	if (!strcmp(norm_arch, "x86")) {
+	if (!strcmp(norm_arch, NORM_X86)) {
 		instructions = instructions_x86;
 		nmemb = ARRAY_SIZE(instructions_x86);
-	} else if (!strcmp(norm_arch, "arm")) {
+	} else if (!strcmp(norm_arch, NORM_ARM)) {
 		instructions = instructions_arm;
 		nmemb = ARRAY_SIZE(instructions_arm);
-	} else if (!strcmp(norm_arch, "powerpc")) {
+	} 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);
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index 8547119..8d4a53f 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -36,10 +36,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

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Michael Ellerman]

On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 36a5825..b87eac7 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
...
> +
> +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.
> +	 * - Let's also ignore instructions involving 'ctr' and
> +	 *   'tar' since target branch addresses for those can't
> +	 *   be determined statically.
> +	 */
> +	if (name[0] != 'b'             ||
> +	    !strncmp(name, "bcd", 3)   ||
> +	    !strncmp(name, "brinc", 5) ||
> +	    !strncmp(name, "bper", 4)  ||
> +	    strstr(name, "ctr")        ||
> +	    strstr(name, "tar"))
> +		return NULL;

It would be good if 'bctr' was at least recognised as a branch, even if we
can't determine the target. They are very common.

It doesn't look like we have the opcode handy here? Could we get it somehow?
That would make this a *lot* more robust.

cheers

[tip:perf/core] perf evsel: Utility function to fetch arch

[tip-bot for Ravi Bangoria]

Commit-ID:  f4e47f9f7b0bcbb1069b93bd719a1d34fb37d933
Gitweb:     http://git.kernel.org/tip/f4e47f9f7b0bcbb1069b93bd719a1d34fb37d933
Author:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
AuthorDate: Thu, 30 Jun 2016 11:44:19 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 30 Jun 2016 08:37:32 -0300

perf evsel: Utility function to fetch arch

Add Utility function to fetch arch using evsel. (evsel->env->arch)

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Anton Blanchard <anton@ozlabs.org>
Cc: Daniel Axtens <dja@axtens.net>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/1467267262-4589-2-git-send-email-ravi.bangoria@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 7 +++++++
 tools/perf/util/evsel.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1d8f2bb..0fea724 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2422,3 +2422,10 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			 err, strerror_r(err, sbuf, sizeof(sbuf)),
 			 perf_evsel__name(evsel));
 }
+
+char *perf_evsel__env_arch(struct perf_evsel *evsel)
+{
+	if (evsel && evsel->evlist && evsel->evlist->env)
+		return evsel->evlist->env->arch;
+	return NULL;
+}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 828ddd1..86fed7a 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -435,4 +435,6 @@ typedef int (*attr__fprintf_f)(FILE *, const char *, const char *, void *);
 int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
 			     attr__fprintf_f attr__fprintf, void *priv);
 
+char *perf_evsel__env_arch(struct perf_evsel *evsel);
+
 #endif /* __PERF_EVSEL_H */

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Thanks Michael for your suggestion.

On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
> On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 36a5825..b87eac7 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
> ...
>> +
>> +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.
>> +	 * - Let's also ignore instructions involving 'ctr' and
>> +	 *   'tar' since target branch addresses for those can't
>> +	 *   be determined statically.
>> +	 */
>> +	if (name[0] != 'b'             ||
>> +	    !strncmp(name, "bcd", 3)   ||
>> +	    !strncmp(name, "brinc", 5) ||
>> +	    !strncmp(name, "bper", 4)  ||
>> +	    strstr(name, "ctr")        ||
>> +	    strstr(name, "tar"))
>> +		return NULL;
> It would be good if 'bctr' was at least recognised as a branch, even if we
> can't determine the target. They are very common.

We can not show arrow for this since we don't know the target location.
can you please suggest how you intends perf to display bctr?

bctr can be classified into two variants -- 'bctr' and 'bctrl'.

'bctr' will be considered as jump instruction but jump__parse() won't
be able to find any target location and hence it will set target to
UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
looks misleading.

bctrl will be considered as call instruction but call_parse() won't
be able to find any target function and hence it won't show any
navigation arrow for this instruction. Which is same as filter it
beforehand.

> It doesn't look like we have the opcode handy here? Could we get it somehow?
> That would make this a *lot* more robust.

objdump prints machine code, but I don't know how difficult that would
be to parse to get opcode.

-Ravi

> cheers
>

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Balbir Singh]

On Fri, 2016-07-01 at 14:13 +0530, Ravi Bangoria wrote:
> Thanks Michael for your suggestion.
> 
> On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
> > 
> > On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
> > > 
> > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > > index 36a5825..b87eac7 100644
> > > --- a/tools/perf/util/annotate.c
> > > +++ b/tools/perf/util/annotate.c
> > > @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
> > ...
> > > 
> > > +
> > > +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.
> > > +	 * - Let's also ignore instructions involving 'ctr' and
> > > +	 *   'tar' since target branch addresses for those can't
> > > +	 *   be determined statically.
> > > +	 */
> > > +	if (name[0] != 'b'             ||
> > > +	    !strncmp(name, "bcd", 3)   ||
> > > +	    !strncmp(name, "brinc", 5) ||
> > > +	    !strncmp(name, "bper", 4)  ||
> > > +	    strstr(name, "ctr")        ||
> > > +	    strstr(name, "tar"))
> > > +		return NULL;
> > It would be good if 'bctr' was at least recognised as a branch, even if we
> > can't determine the target. They are very common.
> We can not show arrow for this since we don't know the target location.
> can you please suggest how you intends perf to display bctr?
> 
> bctr can be classified into two variants -- 'bctr' and 'bctrl'.
> 
> 'bctr' will be considered as jump instruction but jump__parse() won't
> be able to find any target location and hence it will set target to
> UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
> looks misleading.
> 
> bctrl will be considered as call instruction but call_parse() won't
> be able to find any target function and hence it won't show any
> navigation arrow for this instruction. Which is same as filter it
> beforehand.
>

The target location and function are in the counter. Can't we add
this to instruction ops? Is it a major change to add it?
 
Balbir Singh. 

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Hi Balbir,

On Friday 01 July 2016 06:18 PM, Balbir Singh wrote:
> On Fri, 2016-07-01 at 14:13 +0530, Ravi Bangoria wrote:
>> Thanks Michael for your suggestion.
>>   
>> On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
>>>   
>>> On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
>>>>   
>>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>>> index 36a5825..b87eac7 100644
>>>> --- a/tools/perf/util/annotate.c
>>>> +++ b/tools/perf/util/annotate.c
>>>> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
>>> ...
>>>>   
>>>> +
>>>> +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.
>>>> +	 * - Let's also ignore instructions involving 'ctr' and
>>>> +	 *   'tar' since target branch addresses for those can't
>>>> +	 *   be determined statically.
>>>> +	 */
>>>> +	if (name[0] != 'b'             ||
>>>> +	    !strncmp(name, "bcd", 3)   ||
>>>> +	    !strncmp(name, "brinc", 5) ||
>>>> +	    !strncmp(name, "bper", 4)  ||
>>>> +	    strstr(name, "ctr")        ||
>>>> +	    strstr(name, "tar"))
>>>> +		return NULL;
>>> It would be good if 'bctr' was at least recognised as a branch, even if we
>>> can't determine the target. They are very common.
>> We can not show arrow for this since we don't know the target location.
>> can you please suggest how you intends perf to display bctr?
>>   
>> bctr can be classified into two variants -- 'bctr' and 'bctrl'.
>>   
>> 'bctr' will be considered as jump instruction but jump__parse() won't
>> be able to find any target location and hence it will set target to
>> UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
>> looks misleading.
>>   
>> bctrl will be considered as call instruction but call_parse() won't
>> be able to find any target function and hence it won't show any
>> navigation arrow for this instruction. Which is same as filter it
>> beforehand.
>>
> The target location and function are in the counter. Can't we add
> this to instruction ops? Is it a major change to add it?

Of course we can add it.

What I mean is we can not determine target location statically by parsing
objdump output. For example, consider snippet:

objdump output:

   c000000000143848:       lwarx   r8,0,r10
   c00000000014384c:       addic   r8,r8,1
   c000000000143850:       stwcx.  r8,0,r10
   c000000000143854:       bne-    c000000000143848 <.rcu_idle_exit+0x58>

corresponding perf annotate output:

   58:  lwarx  r8,0,r10
          addic  r8,r8,1
          stwcx. r8,0,r10
          bne-   58

tui will show up arrow before 'bne- 58' instruction, that indicate it as
a jump instruction. When we focus on 'bne- 58' instruction, arrow will
span from that instruction to instruction with 58th offset( lwarx ).
By pressing Enter, it will jump focus to the target.

In case of 'bctr', we can not determine target location statically
and hence we can not provide any navigation options. Same for
'bctrl' as well.

Please correct me if I misunderstood anything.

-Ravi

>   
> Balbir Singh.
>

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Hi Michael,

On Friday 01 July 2016 02:13 PM, Ravi Bangoria wrote:
> Thanks Michael for your suggestion.
>
> On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
>> On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>> index 36a5825..b87eac7 100644
>>> --- a/tools/perf/util/annotate.c
>>> +++ b/tools/perf/util/annotate.c
>>> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
>> ...
>>> +
>>> +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.
>>> +     * - Let's also ignore instructions involving 'ctr' and
>>> +     *   'tar' since target branch addresses for those can't
>>> +     *   be determined statically.
>>> +     */
>>> +    if (name[0] != 'b'             ||
>>> +        !strncmp(name, "bcd", 3)   ||
>>> +        !strncmp(name, "brinc", 5) ||
>>> +        !strncmp(name, "bper", 4)  ||
>>> +        strstr(name, "ctr")        ||
>>> +        strstr(name, "tar"))
>>> +        return NULL;
>> It would be good if 'bctr' was at least recognised as a branch, even 
>> if we
>> can't determine the target. They are very common.
>
> We can not show arrow for this since we don't know the target location.
> can you please suggest how you intends perf to display bctr?
>
> bctr can be classified into two variants -- 'bctr' and 'bctrl'.
>
> 'bctr' will be considered as jump instruction but jump__parse() won't
> be able to find any target location and hence it will set target to
> UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
> looks misleading.
>
> bctrl will be considered as call instruction but call_parse() won't
> be able to find any target function and hence it won't show any
> navigation arrow for this instruction. Which is same as filter it
> beforehand.
>
>> It doesn't look like we have the opcode handy here? Could we get it 
>> somehow?
>> That would make this a *lot* more robust.
>
> objdump prints machine code, but I don't know how difficult that would
> be to parse to get opcode.

Perf uses  --no-show-raw with objdump and hence objdump output does not
show opcodes. So change in current  objdump output may requires changes
in current parsing logic. Additionally I need to change tui as well to show
opcodes. This looks quite more work.

And this patchset is about enabling annotate for cross arch. So if you 
really
need opcode with perf anotate, can we do it separately?

Please let me know your thoughts.

-Ravi

>
> -Ravi
>
>> cheers
>>
>

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Hi Michael,

On Wednesday 06 July 2016 03:38 PM, Michael Ellerman wrote:
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:
>
>> On Thursday 30 June 2016 11:51 AM, Michael Ellerman wrote:
>>> On Thu, 2016-06-30 at 11:44 +0530, Ravi Bangoria wrote:
>>>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>>>> index 36a5825..b87eac7 100644
>>>> --- a/tools/perf/util/annotate.c
>>>> +++ b/tools/perf/util/annotate.c
>>>> @@ -476,6 +481,125 @@ static int ins__cmp(const void *a, const void *b)
>>> ...
>>>> +
>>>> +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.
>>>> +	 * - Let's also ignore instructions involving 'ctr' and
>>>> +	 *   'tar' since target branch addresses for those can't
>>>> +	 *   be determined statically.
>>>> +	 */
>>>> +	if (name[0] != 'b'             ||
>>>> +	    !strncmp(name, "bcd", 3)   ||
>>>> +	    !strncmp(name, "brinc", 5) ||
>>>> +	    !strncmp(name, "bper", 4)  ||
>>>> +	    strstr(name, "ctr")        ||
>>>> +	    strstr(name, "tar"))
>>>> +		return NULL;
>>> It would be good if 'bctr' was at least recognised as a branch, even if we
>>> can't determine the target. They are very common.
>> We can not show arrow for this since we don't know the target location.
>> can you please suggest how you intends perf to display bctr?
> Yeah I understand you can't show an arrow.
>
> I guess it could just be an unterminated arrow? But I'm not sure if
> that's easy to do with the way the UI is constructed. eg. something
> like:
>
>      ld      r12,0(r12)
>      mtctr   r12
>      bctrl	------------------>
>      ld      r3,-32704(r2)
>
> But that's just an idea.

I've sent v4 which enables annotate for bctr' instructions.

for 'bctr', it will show down arrow(indicate jump) and 'bctrl' will show
right arrow(indicate call). But no navigation options will be provided.
By pressing Enter key on that, message will be shown that like
"Invalid target"

Please review it.

>> bctr can be classified into two variants -- 'bctr' and 'bctrl'.
>>
>> 'bctr' will be considered as jump instruction but jump__parse() won't
>> be able to find any target location and hence it will set target to
>> UINT64_MAX which transform 'bctr' to 'bctr UINT64_MAX'. This
>> looks misleading.
> Agreed.
>
>> bctrl will be considered as call instruction but call_parse() won't
>> be able to find any target function and hence it won't show any
>> navigation arrow for this instruction. Which is same as filter it
>> beforehand.
> OK.
>
> Maybe what I'm asking for is an enhancement and can be done later.
>
>>> It doesn't look like we have the opcode handy here? Could we get it somehow?
>>> That would make this a *lot* more robust.
>> objdump prints machine code, but I don't know how difficult that would
>> be to parse to get opcode.
> Normal objdump -d output includes the opcode, eg:
>
> c00000000000886c:       2c 2c 00 00     cmpdi   r12,0
>                          ^^^^^^^^^^^
>
> The only thing you need to know is the endian and you can reconstruct
> the raw instruction.
>
> Then you can just decode the opcode, see how we do it in the kernel with
> eg. instr_is_relative_branch().

I'm sorry. I was thinking that you wants to show opcodes with perf
annotate. But you were asking to use opcode instead of parsing
instructions.

This looks like rewrite parsing code. I don't know whether there is any
library already available for this which we can directly use. I'm thinking
about this.

- Ravi

> cheers
>

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

Hi Arnaldo,

On Friday 08 July 2016 02:01 PM, Michael Ellerman wrote:
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:
>
>> On Wednesday 06 July 2016 03:38 PM, Michael Ellerman wrote:
>>
>> I've sent v4 which enables annotate for bctr' instructions.
>>
>> for 'bctr', it will show down arrow(indicate jump) and 'bctrl' will show
>> right arrow(indicate call). But no navigation options will be provided.
>> By pressing Enter key on that, message will be shown that like
>> "Invalid target"
> Great thanks.

I've sent v4 series. Please review it.

-Ravi

>>>>> It doesn't look like we have the opcode handy here? Could we get it somehow?
>>>>> That would make this a *lot* more robust.
>>>> objdump prints machine code, but I don't know how difficult that would
>>>> be to parse to get opcode.
>>> Normal objdump -d output includes the opcode, eg:
>>>
>>> c00000000000886c:       2c 2c 00 00     cmpdi   r12,0
>>>                           ^^^^^^^^^^^
>>>
>>> The only thing you need to know is the endian and you can reconstruct
>>> the raw instruction.
>>>
>>> Then you can just decode the opcode, see how we do it in the kernel with
>>> eg. instr_is_relative_branch().
>> I'm sorry. I was thinking that you wants to show opcodes with perf
>> annotate. But you were asking to use opcode instead of parsing
>> instructions.
> Yeah.
>
>> This looks like rewrite parsing code. I don't know whether there is any
>> library already available for this which we can directly use. I'm thinking
>> about this.
> OK don't worry about it for now. We should get this merged for starters
> and we can always improve it later.
>
> cheers
>

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Arnaldo Carvalho de Melo]

Em Tue, Jul 12, 2016 at 07:51:46AM +0530, Ravi Bangoria escreveu:
> Hi Arnaldo,
> 
> On Friday 08 July 2016 02:01 PM, Michael Ellerman wrote:
> > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:
> > 
> > > On Wednesday 06 July 2016 03:38 PM, Michael Ellerman wrote:
> > > 
> > > I've sent v4 which enables annotate for bctr' instructions.
> > > 
> > > for 'bctr', it will show down arrow(indicate jump) and 'bctrl' will show
> > > right arrow(indicate call). But no navigation options will be provided.
> > > By pressing Enter key on that, message will be shown that like
> > > "Invalid target"
> > Great thanks.
> 
> I've sent v4 series. Please review it.

If somebody else could do it and provide acks/reviewed by, that would
help,

Michael, can I get your comments as such?

Thanks,

- Arnaldo
 
> -Ravi
> 
> > > > > > It doesn't look like we have the opcode handy here? Could we get it somehow?
> > > > > > That would make this a *lot* more robust.
> > > > > objdump prints machine code, but I don't know how difficult that would
> > > > > be to parse to get opcode.
> > > > Normal objdump -d output includes the opcode, eg:
> > > > 
> > > > c00000000000886c:       2c 2c 00 00     cmpdi   r12,0
> > > >                           ^^^^^^^^^^^
> > > > 
> > > > The only thing you need to know is the endian and you can reconstruct
> > > > the raw instruction.
> > > > 
> > > > Then you can just decode the opcode, see how we do it in the kernel with
> > > > eg. instr_is_relative_branch().
> > > I'm sorry. I was thinking that you wants to show opcodes with perf
> > > annotate. But you were asking to use opcode instead of parsing
> > > instructions.
> > Yeah.
> > 
> > > This looks like rewrite parsing code. I don't know whether there is any
> > > library already available for this which we can directly use. I'm thinking
> > > about this.
> > OK don't worry about it for now. We should get this merged for starters
> > and we can always improve it later.
> > 
> > cheers
> > 

Re: [PATCH v3 3/4] perf annotate: add powerpc support

[Ravi Bangoria]

On Wednesday 13 July 2016 01:09 PM, Michael Ellerman wrote:
> Arnaldo Carvalho de Melo <acme@kernel.org> writes:
>
>> Em Tue, Jul 12, 2016 at 07:51:46AM +0530, Ravi Bangoria escreveu:
>>> Hi Arnaldo,
>>>
>>> On Friday 08 July 2016 02:01 PM, Michael Ellerman wrote:
>>>> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> writes:
>>>>
>>>>> On Wednesday 06 July 2016 03:38 PM, Michael Ellerman wrote:
>>>>>
>>>>> I've sent v4 which enables annotate for bctr' instructions.
>>>>>
>>>>> for 'bctr', it will show down arrow(indicate jump) and 'bctrl' will show
>>>>> right arrow(indicate call). But no navigation options will be provided.
>>>>> By pressing Enter key on that, message will be shown that like
>>>>> "Invalid target"
>>>> Great thanks.
>>> I've sent v4 series. Please review it.
>> If somebody else could do it and provide acks/reviewed by, that would
>> help,
>>
>> Michael, can I get your comments as such?
> It looks OK to me. But I don't know the code really, and I haven't had
> time to test it personally.
>
> Ravi, have you tested on a big endian machine?

Yes Michael, I've tested annotate on BE and LE both.

-Ravi

>
> cheers
>