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

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes
@ 2016-08-19 12:59 Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 1/7] perf: Define macro for normalized arch names Ravi Bangoria
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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:
I don't have arm test machine. As suggested by Russell in one of the
review comment, I've copied all instructions from default table to
arm table. This way it want break tool on arm but cleanup is needed
for x86 specific instructions added in arm table.

Example:

  Record on powerpc:
  $ ./perf record -a

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

Changes in v6:
  - Instead of adding only those instructions defined in #ifdef __arm__,
    add all instructions from default table to arm table.

v5 link:
  https://lkml.org/lkml/2016/8/19/35

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         | 330 +++++++++++++++++++++++++++++++------
 tools/perf/util/annotate.h         |  10 +-
 tools/perf/util/unwind-libunwind.c |   4 +-
 8 files changed, 327 insertions(+), 76 deletions(-)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 1/7] perf: Define macro for normalized arch names
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 2/7] perf annotate: Add cross arch annotate support Ravi Bangoria
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No change

 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 1/7] perf: Define macro for normalized arch names Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-08-22 23:01   ` Kim Phillips
  2016-08-19 12:59 ` [PATCH v6 3/7] perf annotate: Add support for powerpc Ravi Bangoria
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - Instead of adding only those instructions defined in #ifdef __arm__,
    add all instructions from default table to arm table.

 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        | 187 ++++++++++++++++++++++++++++++--------
 tools/perf/util/annotate.h        |   5 +-
 5 files changed, 157 insertions(+), 42 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..14a8808 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,14 +365,92 @@ bool ins__is_ret(const struct ins *ins)
 	return ins->ops == &ret_ops;
 }
 
-static struct ins instructions[] = {
+static struct ins instructions_x86[] = {
+	{ .name = "add",   .ops  = &mov_ops, },
+	{ .name = "addl",  .ops  = &mov_ops, },
+	{ .name = "addq",  .ops  = &mov_ops, },
+	{ .name = "addw",  .ops  = &mov_ops, },
+	{ .name = "and",   .ops  = &mov_ops, },
+	{ .name = "bts",   .ops  = &mov_ops, },
+	{ .name = "call",  .ops  = &call_ops, },
+	{ .name = "callq", .ops  = &call_ops, },
+	{ .name = "cmp",   .ops  = &mov_ops, },
+	{ .name = "cmpb",  .ops  = &mov_ops, },
+	{ .name = "cmpl",  .ops  = &mov_ops, },
+	{ .name = "cmpq",  .ops  = &mov_ops, },
+	{ .name = "cmpw",  .ops  = &mov_ops, },
+	{ .name = "cmpxch", .ops  = &mov_ops, },
+	{ .name = "dec",   .ops  = &dec_ops, },
+	{ .name = "decl",  .ops  = &dec_ops, },
+	{ .name = "imul",  .ops  = &mov_ops, },
+	{ .name = "inc",   .ops  = &dec_ops, },
+	{ .name = "incl",  .ops  = &dec_ops, },
+	{ .name = "ja",	   .ops  = &jump_ops, },
+	{ .name = "jae",   .ops  = &jump_ops, },
+	{ .name = "jb",	   .ops  = &jump_ops, },
+	{ .name = "jbe",   .ops  = &jump_ops, },
+	{ .name = "jc",	   .ops  = &jump_ops, },
+	{ .name = "jcxz",  .ops  = &jump_ops, },
+	{ .name = "je",	   .ops  = &jump_ops, },
+	{ .name = "jecxz", .ops  = &jump_ops, },
+	{ .name = "jg",	   .ops  = &jump_ops, },
+	{ .name = "jge",   .ops  = &jump_ops, },
+	{ .name = "jl",    .ops  = &jump_ops, },
+	{ .name = "jle",   .ops  = &jump_ops, },
+	{ .name = "jmp",   .ops  = &jump_ops, },
+	{ .name = "jmpq",  .ops  = &jump_ops, },
+	{ .name = "jna",   .ops  = &jump_ops, },
+	{ .name = "jnae",  .ops  = &jump_ops, },
+	{ .name = "jnb",   .ops  = &jump_ops, },
+	{ .name = "jnbe",  .ops  = &jump_ops, },
+	{ .name = "jnc",   .ops  = &jump_ops, },
+	{ .name = "jne",   .ops  = &jump_ops, },
+	{ .name = "jng",   .ops  = &jump_ops, },
+	{ .name = "jnge",  .ops  = &jump_ops, },
+	{ .name = "jnl",   .ops  = &jump_ops, },
+	{ .name = "jnle",  .ops  = &jump_ops, },
+	{ .name = "jno",   .ops  = &jump_ops, },
+	{ .name = "jnp",   .ops  = &jump_ops, },
+	{ .name = "jns",   .ops  = &jump_ops, },
+	{ .name = "jnz",   .ops  = &jump_ops, },
+	{ .name = "jo",	   .ops  = &jump_ops, },
+	{ .name = "jp",	   .ops  = &jump_ops, },
+	{ .name = "jpe",   .ops  = &jump_ops, },
+	{ .name = "jpo",   .ops  = &jump_ops, },
+	{ .name = "jrcxz", .ops  = &jump_ops, },
+	{ .name = "js",	   .ops  = &jump_ops, },
+	{ .name = "jz",	   .ops  = &jump_ops, },
+	{ .name = "lea",   .ops  = &mov_ops, },
+	{ .name = "lock",  .ops  = &lock_ops, },
+	{ .name = "mov",   .ops  = &mov_ops, },
+	{ .name = "movb",  .ops  = &mov_ops, },
+	{ .name = "movdqa", .ops  = &mov_ops, },
+	{ .name = "movl",  .ops  = &mov_ops, },
+	{ .name = "movq",  .ops  = &mov_ops, },
+	{ .name = "movslq", .ops  = &mov_ops, },
+	{ .name = "movzbl", .ops  = &mov_ops, },
+	{ .name = "movzwl", .ops  = &mov_ops, },
+	{ .name = "nop",   .ops  = &nop_ops, },
+	{ .name = "nopl",  .ops  = &nop_ops, },
+	{ .name = "nopw",  .ops  = &nop_ops, },
+	{ .name = "or",    .ops  = &mov_ops, },
+	{ .name = "orl",   .ops  = &mov_ops, },
+	{ .name = "test",  .ops  = &mov_ops, },
+	{ .name = "testb", .ops  = &mov_ops, },
+	{ .name = "testl", .ops  = &mov_ops, },
+	{ .name = "xadd",  .ops  = &mov_ops, },
+	{ .name = "xbeginl", .ops  = &jump_ops, },
+	{ .name = "xbeginq", .ops  = &jump_ops, },
+	{ .name = "retq",  .ops  = &ret_ops, },
+};
+
+static struct ins instructions_arm[] = {
 	{ .name = "add",   .ops  = &mov_ops, },
 	{ .name = "addl",  .ops  = &mov_ops, },
 	{ .name = "addq",  .ops  = &mov_ops, },
 	{ .name = "addw",  .ops  = &mov_ops, },
 	{ .name = "and",   .ops  = &mov_ops, },
-#ifdef __arm__
-	{ .name = "b",     .ops  = &jump_ops, }, // might also be a call
+	{ .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, },
@@ -382,7 +462,6 @@ static struct ins instructions[] = {
 	{ .name = "blt",   .ops  = &jump_ops, },
 	{ .name = "blx",   .ops  = &call_ops, },
 	{ .name = "bne",   .ops  = &jump_ops, },
-#endif
 	{ .name = "bts",   .ops  = &mov_ops, },
 	{ .name = "call",  .ops  = &call_ops, },
 	{ .name = "callq", .ops  = &call_ops, },
@@ -471,24 +550,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 +818,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 +877,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 +893,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 +1116,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 +1177,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 +1309,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 +1426,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 +1823,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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 3/7] perf annotate: Add support for powerpc
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 1/7] perf: Define macro for normalized arch names Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 2/7] perf annotate: Add cross arch annotate support Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-09-19 15:26   ` Arnaldo Carvalho de Melo
  2016-08-19 12:59 ` [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target Ravi Bangoria
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No change

 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 14a8808..ea07588 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -535,6 +535,11 @@ static struct ins instructions_arm[] = {
 	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
+struct instructions_powerpc {
+	struct ins *ins;
+	struct list_head list;
+};
+
 static int ins__key_cmp(const void *name, const void *insp)
 {
 	const struct ins *ins = insp;
@@ -550,6 +555,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);
@@ -585,6 +699,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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
                   ` (2 preceding siblings ...)
  2016-08-19 12:59 ` [PATCH v6 3/7] perf annotate: Add support for powerpc Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-09-19 15:44   ` Arnaldo Carvalho de Melo
  2016-09-20 21:43   ` [tip:perf/core] " tip-bot for Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 5/7] perf annotate: Show raw form for jump " Ravi Bangoria
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No change

 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 ea07588..a05423b 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 5/7] perf annotate: Show raw form for jump instruction with indirect target
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
                   ` (3 preceding siblings ...)
  2016-08-19 12:59 ` [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 6/7] perf annotate: Support jump instruction with target as second operand Ravi Bangoria
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No changes

 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 a05423b..7ecb1b8 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 6/7] perf annotate: Support jump instruction with target as second operand
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
                   ` (4 preceding siblings ...)
  2016-08-19 12:59 ` [PATCH v6 5/7] perf annotate: Show raw form for jump " Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-08-19 12:59 ` [PATCH v6 7/7] perf annotate: Fix jump target outside of function address range Ravi Bangoria
  2016-09-07 15:39 ` [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
  7 siblings, 0 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No changes

 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 7ecb1b8..73c4f48 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 7/7] perf annotate: Fix jump target outside of function address range
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
                   ` (5 preceding siblings ...)
  2016-08-19 12:59 ` [PATCH v6 6/7] perf annotate: Support jump instruction with target as second operand Ravi Bangoria
@ 2016-08-19 12:59 ` Ravi Bangoria
  2016-09-07 15:39 ` [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
  7 siblings, 0 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-19 12:59 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, 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 v6:
  - No changes

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

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 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 73c4f48..9409d54 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);
@@ -1304,9 +1306,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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-19 12:59 ` [PATCH v6 2/7] perf annotate: Add cross arch annotate support Ravi Bangoria
@ 2016-08-22 23:01   ` Kim Phillips
  2016-08-23  2:17     ` Namhyung Kim
  0 siblings, 1 reply; 20+ messages in thread
From: Kim Phillips @ 2016-08-22 23:01 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, linuxppc-dev, acme, peterz, mingo,
	alexander.shishkin, treeze.taeung, naveen.n.rao, markus,
	chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa, mpe,
	hemant, namhyung

On Fri, 19 Aug 2016 18:29:33 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Changes in v6:
>   - Instead of adding only those instructions defined in #ifdef __arm__,
>     add all instructions from default table to arm table.
..
> +static struct ins instructions_arm[] = {
>  	{ .name = "add",   .ops  = &mov_ops, },
>  	{ .name = "addl",  .ops  = &mov_ops, },
>  	{ .name = "addq",  .ops  = &mov_ops, },
>  	{ .name = "addw",  .ops  = &mov_ops, },
>  	{ .name = "and",   .ops  = &mov_ops, },
> -#ifdef __arm__
> -	{ .name = "b",     .ops  = &jump_ops, }, // might also be a call
> +	{ .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, },
> @@ -382,7 +462,6 @@ static struct ins instructions[] = {
>  	{ .name = "blt",   .ops  = &jump_ops, },
>  	{ .name = "blx",   .ops  = &call_ops, },
>  	{ .name = "bne",   .ops  = &jump_ops, },
> -#endif
>  	{ .name = "bts",   .ops  = &mov_ops, },
>  	{ .name = "call",  .ops  = &call_ops, },
>  	{ .name = "callq", .ops  = &call_ops, },
> @@ -471,24 +550,48 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }

Thanks, I've gone through the list and removed all not-ARM
instructions, and added some missing ARM branch instructions:

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b2c6cf3..9d686504 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -451,9 +451,6 @@ static struct ins instructions_x86[] = {
 
 static struct ins instructions_arm[] = {
 	{ .name = "add",   .ops  = &mov_ops, },
-	{ .name = "addl",  .ops  = &mov_ops, },
-	{ .name = "addq",  .ops  = &mov_ops, },
-	{ .name = "addw",  .ops  = &mov_ops, },
 	{ .name = "and",   .ops  = &mov_ops, },
 	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
 	{ .name = "bcc",   .ops  = &jump_ops, },
@@ -463,81 +460,20 @@ static struct ins instructions_arm[] = {
 	{ .name = "bgt",   .ops  = &jump_ops, },
 	{ .name = "bhi",   .ops  = &jump_ops, },
 	{ .name = "bl",    .ops  = &call_ops, },
+	{ .name = "ble",   .ops  = &jump_ops, },
+	{ .name = "bleq",  .ops  = &call_ops, },
+	{ .name = "blne",  .ops  = &call_ops, },
 	{ .name = "bls",   .ops  = &jump_ops, },
 	{ .name = "blt",   .ops  = &jump_ops, },
 	{ .name = "blx",   .ops  = &call_ops, },
+	{ .name = "blxne", .ops  = &call_ops, },
+	{ .name = "bmi",   .ops  = &jump_ops, },
 	{ .name = "bne",   .ops  = &jump_ops, },
-	{ .name = "bts",   .ops  = &mov_ops, },
-	{ .name = "call",  .ops  = &call_ops, },
-	{ .name = "callq", .ops  = &call_ops, },
+	{ .name = "bpl",   .ops  = &jump_ops, },
 	{ .name = "cmp",   .ops  = &mov_ops, },
-	{ .name = "cmpb",  .ops  = &mov_ops, },
-	{ .name = "cmpl",  .ops  = &mov_ops, },
-	{ .name = "cmpq",  .ops  = &mov_ops, },
-	{ .name = "cmpw",  .ops  = &mov_ops, },
-	{ .name = "cmpxch", .ops  = &mov_ops, },
-	{ .name = "dec",   .ops  = &dec_ops, },
-	{ .name = "decl",  .ops  = &dec_ops, },
-	{ .name = "imul",  .ops  = &mov_ops, },
-	{ .name = "inc",   .ops  = &dec_ops, },
-	{ .name = "incl",  .ops  = &dec_ops, },
-	{ .name = "ja",	   .ops  = &jump_ops, },
-	{ .name = "jae",   .ops  = &jump_ops, },
-	{ .name = "jb",	   .ops  = &jump_ops, },
-	{ .name = "jbe",   .ops  = &jump_ops, },
-	{ .name = "jc",	   .ops  = &jump_ops, },
-	{ .name = "jcxz",  .ops  = &jump_ops, },
-	{ .name = "je",	   .ops  = &jump_ops, },
-	{ .name = "jecxz", .ops  = &jump_ops, },
-	{ .name = "jg",	   .ops  = &jump_ops, },
-	{ .name = "jge",   .ops  = &jump_ops, },
-	{ .name = "jl",    .ops  = &jump_ops, },
-	{ .name = "jle",   .ops  = &jump_ops, },
-	{ .name = "jmp",   .ops  = &jump_ops, },
-	{ .name = "jmpq",  .ops  = &jump_ops, },
-	{ .name = "jna",   .ops  = &jump_ops, },
-	{ .name = "jnae",  .ops  = &jump_ops, },
-	{ .name = "jnb",   .ops  = &jump_ops, },
-	{ .name = "jnbe",  .ops  = &jump_ops, },
-	{ .name = "jnc",   .ops  = &jump_ops, },
-	{ .name = "jne",   .ops  = &jump_ops, },
-	{ .name = "jng",   .ops  = &jump_ops, },
-	{ .name = "jnge",  .ops  = &jump_ops, },
-	{ .name = "jnl",   .ops  = &jump_ops, },
-	{ .name = "jnle",  .ops  = &jump_ops, },
-	{ .name = "jno",   .ops  = &jump_ops, },
-	{ .name = "jnp",   .ops  = &jump_ops, },
-	{ .name = "jns",   .ops  = &jump_ops, },
-	{ .name = "jnz",   .ops  = &jump_ops, },
-	{ .name = "jo",	   .ops  = &jump_ops, },
-	{ .name = "jp",	   .ops  = &jump_ops, },
-	{ .name = "jpe",   .ops  = &jump_ops, },
-	{ .name = "jpo",   .ops  = &jump_ops, },
-	{ .name = "jrcxz", .ops  = &jump_ops, },
-	{ .name = "js",	   .ops  = &jump_ops, },
-	{ .name = "jz",	   .ops  = &jump_ops, },
-	{ .name = "lea",   .ops  = &mov_ops, },
-	{ .name = "lock",  .ops  = &lock_ops, },
 	{ .name = "mov",   .ops  = &mov_ops, },
-	{ .name = "movb",  .ops  = &mov_ops, },
-	{ .name = "movdqa",.ops  = &mov_ops, },
-	{ .name = "movl",  .ops  = &mov_ops, },
-	{ .name = "movq",  .ops  = &mov_ops, },
-	{ .name = "movslq", .ops  = &mov_ops, },
-	{ .name = "movzbl", .ops  = &mov_ops, },
-	{ .name = "movzwl", .ops  = &mov_ops, },
 	{ .name = "nop",   .ops  = &nop_ops, },
-	{ .name = "nopl",  .ops  = &nop_ops, },
-	{ .name = "nopw",  .ops  = &nop_ops, },
 	{ .name = "or",    .ops  = &mov_ops, },
-	{ .name = "orl",   .ops  = &mov_ops, },
-	{ .name = "test",  .ops  = &mov_ops, },
-	{ .name = "testb", .ops  = &mov_ops, },
-	{ .name = "testl", .ops  = &mov_ops, },
-	{ .name = "xadd",  .ops  = &mov_ops, },
-	{ .name = "xbeginl", .ops  = &jump_ops, },
-	{ .name = "xbeginq", .ops  = &jump_ops, },
-	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
 struct instructions_powerpc {

Ideally ARM would - like this powerpc implementation - add a
ins__find_arm() to handle its condition codes, but, for now, is
it possible to merge this change into this series?  I can post a
follow-up patch if not.

Thanks,

Kim

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-22 23:01   ` Kim Phillips
@ 2016-08-23  2:17     ` Namhyung Kim
  2016-08-23 20:36       ` Kim Phillips
  0 siblings, 1 reply; 20+ messages in thread
From: Namhyung Kim @ 2016-08-23  2:17 UTC (permalink / raw)
  To: Kim Phillips
  Cc: Ravi Bangoria, linux-kernel, linuxppc-dev,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Taeung Song, Naveen N. Rao, markus,
	chris.ryder, Pawel Moll, Masami Hiramatsu, rmk+kernel, Jiri Olsa,
	Michael Ellerman, Hemant Kumar

Hello,

On Tue, Aug 23, 2016 at 8:01 AM, Kim Phillips <kim.phillips@arm.com> wrote:
> On Fri, 19 Aug 2016 18:29:33 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Changes in v6:
>>   - Instead of adding only those instructions defined in #ifdef __arm__,
>>     add all instructions from default table to arm table.
> ..
>> +static struct ins instructions_arm[] = {
>>       { .name = "add",   .ops  = &mov_ops, },
>>       { .name = "addl",  .ops  = &mov_ops, },
>>       { .name = "addq",  .ops  = &mov_ops, },
>>       { .name = "addw",  .ops  = &mov_ops, },
>>       { .name = "and",   .ops  = &mov_ops, },
>> -#ifdef __arm__
>> -     { .name = "b",     .ops  = &jump_ops, }, // might also be a call
>> +     { .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, },
>> @@ -382,7 +462,6 @@ static struct ins instructions[] = {
>>       { .name = "blt",   .ops  = &jump_ops, },
>>       { .name = "blx",   .ops  = &call_ops, },
>>       { .name = "bne",   .ops  = &jump_ops, },
>> -#endif
>>       { .name = "bts",   .ops  = &mov_ops, },
>>       { .name = "call",  .ops  = &call_ops, },
>>       { .name = "callq", .ops  = &call_ops, },
>> @@ -471,24 +550,48 @@ static int ins__cmp(const void *a, const void *b)
>>       return strcmp(ia->name, ib->name);
>>  }
>
> Thanks, I've gone through the list and removed all not-ARM
> instructions, and added some missing ARM branch instructions:

Can we use regex patterns instead?

Thanks
Namhyung


>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b2c6cf3..9d686504 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -451,9 +451,6 @@ static struct ins instructions_x86[] = {
>
>  static struct ins instructions_arm[] = {
>         { .name = "add",   .ops  = &mov_ops, },
> -       { .name = "addl",  .ops  = &mov_ops, },
> -       { .name = "addq",  .ops  = &mov_ops, },
> -       { .name = "addw",  .ops  = &mov_ops, },
>         { .name = "and",   .ops  = &mov_ops, },
>         { .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
>         { .name = "bcc",   .ops  = &jump_ops, },
> @@ -463,81 +460,20 @@ static struct ins instructions_arm[] = {
>         { .name = "bgt",   .ops  = &jump_ops, },
>         { .name = "bhi",   .ops  = &jump_ops, },
>         { .name = "bl",    .ops  = &call_ops, },
> +       { .name = "ble",   .ops  = &jump_ops, },
> +       { .name = "bleq",  .ops  = &call_ops, },
> +       { .name = "blne",  .ops  = &call_ops, },
>         { .name = "bls",   .ops  = &jump_ops, },
>         { .name = "blt",   .ops  = &jump_ops, },
>         { .name = "blx",   .ops  = &call_ops, },
> +       { .name = "blxne", .ops  = &call_ops, },
> +       { .name = "bmi",   .ops  = &jump_ops, },
>         { .name = "bne",   .ops  = &jump_ops, },
> -       { .name = "bts",   .ops  = &mov_ops, },
> -       { .name = "call",  .ops  = &call_ops, },
> -       { .name = "callq", .ops  = &call_ops, },
> +       { .name = "bpl",   .ops  = &jump_ops, },
>         { .name = "cmp",   .ops  = &mov_ops, },
> -       { .name = "cmpb",  .ops  = &mov_ops, },
> -       { .name = "cmpl",  .ops  = &mov_ops, },
> -       { .name = "cmpq",  .ops  = &mov_ops, },
> -       { .name = "cmpw",  .ops  = &mov_ops, },
> -       { .name = "cmpxch", .ops  = &mov_ops, },
> -       { .name = "dec",   .ops  = &dec_ops, },
> -       { .name = "decl",  .ops  = &dec_ops, },
> -       { .name = "imul",  .ops  = &mov_ops, },
> -       { .name = "inc",   .ops  = &dec_ops, },
> -       { .name = "incl",  .ops  = &dec_ops, },
> -       { .name = "ja",    .ops  = &jump_ops, },
> -       { .name = "jae",   .ops  = &jump_ops, },
> -       { .name = "jb",    .ops  = &jump_ops, },
> -       { .name = "jbe",   .ops  = &jump_ops, },
> -       { .name = "jc",    .ops  = &jump_ops, },
> -       { .name = "jcxz",  .ops  = &jump_ops, },
> -       { .name = "je",    .ops  = &jump_ops, },
> -       { .name = "jecxz", .ops  = &jump_ops, },
> -       { .name = "jg",    .ops  = &jump_ops, },
> -       { .name = "jge",   .ops  = &jump_ops, },
> -       { .name = "jl",    .ops  = &jump_ops, },
> -       { .name = "jle",   .ops  = &jump_ops, },
> -       { .name = "jmp",   .ops  = &jump_ops, },
> -       { .name = "jmpq",  .ops  = &jump_ops, },
> -       { .name = "jna",   .ops  = &jump_ops, },
> -       { .name = "jnae",  .ops  = &jump_ops, },
> -       { .name = "jnb",   .ops  = &jump_ops, },
> -       { .name = "jnbe",  .ops  = &jump_ops, },
> -       { .name = "jnc",   .ops  = &jump_ops, },
> -       { .name = "jne",   .ops  = &jump_ops, },
> -       { .name = "jng",   .ops  = &jump_ops, },
> -       { .name = "jnge",  .ops  = &jump_ops, },
> -       { .name = "jnl",   .ops  = &jump_ops, },
> -       { .name = "jnle",  .ops  = &jump_ops, },
> -       { .name = "jno",   .ops  = &jump_ops, },
> -       { .name = "jnp",   .ops  = &jump_ops, },
> -       { .name = "jns",   .ops  = &jump_ops, },
> -       { .name = "jnz",   .ops  = &jump_ops, },
> -       { .name = "jo",    .ops  = &jump_ops, },
> -       { .name = "jp",    .ops  = &jump_ops, },
> -       { .name = "jpe",   .ops  = &jump_ops, },
> -       { .name = "jpo",   .ops  = &jump_ops, },
> -       { .name = "jrcxz", .ops  = &jump_ops, },
> -       { .name = "js",    .ops  = &jump_ops, },
> -       { .name = "jz",    .ops  = &jump_ops, },
> -       { .name = "lea",   .ops  = &mov_ops, },
> -       { .name = "lock",  .ops  = &lock_ops, },
>         { .name = "mov",   .ops  = &mov_ops, },
> -       { .name = "movb",  .ops  = &mov_ops, },
> -       { .name = "movdqa",.ops  = &mov_ops, },
> -       { .name = "movl",  .ops  = &mov_ops, },
> -       { .name = "movq",  .ops  = &mov_ops, },
> -       { .name = "movslq", .ops  = &mov_ops, },
> -       { .name = "movzbl", .ops  = &mov_ops, },
> -       { .name = "movzwl", .ops  = &mov_ops, },
>         { .name = "nop",   .ops  = &nop_ops, },
> -       { .name = "nopl",  .ops  = &nop_ops, },
> -       { .name = "nopw",  .ops  = &nop_ops, },
>         { .name = "or",    .ops  = &mov_ops, },
> -       { .name = "orl",   .ops  = &mov_ops, },
> -       { .name = "test",  .ops  = &mov_ops, },
> -       { .name = "testb", .ops  = &mov_ops, },
> -       { .name = "testl", .ops  = &mov_ops, },
> -       { .name = "xadd",  .ops  = &mov_ops, },
> -       { .name = "xbeginl", .ops  = &jump_ops, },
> -       { .name = "xbeginq", .ops  = &jump_ops, },
> -       { .name = "retq",  .ops  = &ret_ops, },
>  };
>
>  struct instructions_powerpc {
>
> Ideally ARM would - like this powerpc implementation - add a
> ins__find_arm() to handle its condition codes, but, for now, is
> it possible to merge this change into this series?  I can post a
> follow-up patch if not.
>
> Thanks,
>
> Kim

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-23  2:17     ` Namhyung Kim
@ 2016-08-23 20:36       ` Kim Phillips
  2016-08-26  6:21         ` Namhyung Kim
  2016-08-26  7:26         ` Ravi Bangoria
  0 siblings, 2 replies; 20+ messages in thread
From: Kim Phillips @ 2016-08-23 20:36 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ravi Bangoria, linux-kernel, linuxppc-dev,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Taeung Song, Naveen N. Rao, markus,
	chris.ryder, Pawel Moll, Masami Hiramatsu, rmk+kernel, Jiri Olsa,
	Michael Ellerman, Hemant Kumar

On Tue, 23 Aug 2016 11:17:16 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Tue, Aug 23, 2016 at 8:01 AM, Kim Phillips <kim.phillips@arm.com> wrote:
> > On Fri, 19 Aug 2016 18:29:33 +0530
> > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >
> >> Changes in v6:
> >>   - Instead of adding only those instructions defined in #ifdef __arm__,
> >>     add all instructions from default table to arm table.
> > Thanks, I've gone through the list and removed all not-ARM
> > instructions, and added some missing ARM branch instructions:
> 
> Can we use regex patterns instead?

Yes, that helps prevent mistakes updating instruction lists - how does
this look?:

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b2c6cf3..52316f3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -26,6 +26,7 @@
 const char 	*disassembler_style;
 const char	*objdump_path;
 static regex_t	 file_lineno;
+static regex_t	 arm_call_insn, arm_jump_insn;
 
 static struct ins *ins__find(const char *name, const char *norm_arch);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
@@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
 	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
-static struct ins instructions_arm[] = {
-	{ .name = "add",   .ops  = &mov_ops, },
-	{ .name = "addl",  .ops  = &mov_ops, },
-	{ .name = "addq",  .ops  = &mov_ops, },
-	{ .name = "addw",  .ops  = &mov_ops, },
-	{ .name = "and",   .ops  = &mov_ops, },
-	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
-	{ .name = "bcc",   .ops  = &jump_ops, },
-	{ .name = "bcs",   .ops  = &jump_ops, },
-	{ .name = "beq",   .ops  = &jump_ops, },
-	{ .name = "bge",   .ops  = &jump_ops, },
-	{ .name = "bgt",   .ops  = &jump_ops, },
-	{ .name = "bhi",   .ops  = &jump_ops, },
-	{ .name = "bl",    .ops  = &call_ops, },
-	{ .name = "bls",   .ops  = &jump_ops, },
-	{ .name = "blt",   .ops  = &jump_ops, },
-	{ .name = "blx",   .ops  = &call_ops, },
-	{ .name = "bne",   .ops  = &jump_ops, },
-	{ .name = "bts",   .ops  = &mov_ops, },
-	{ .name = "call",  .ops  = &call_ops, },
-	{ .name = "callq", .ops  = &call_ops, },
-	{ .name = "cmp",   .ops  = &mov_ops, },
-	{ .name = "cmpb",  .ops  = &mov_ops, },
-	{ .name = "cmpl",  .ops  = &mov_ops, },
-	{ .name = "cmpq",  .ops  = &mov_ops, },
-	{ .name = "cmpw",  .ops  = &mov_ops, },
-	{ .name = "cmpxch", .ops  = &mov_ops, },
-	{ .name = "dec",   .ops  = &dec_ops, },
-	{ .name = "decl",  .ops  = &dec_ops, },
-	{ .name = "imul",  .ops  = &mov_ops, },
-	{ .name = "inc",   .ops  = &dec_ops, },
-	{ .name = "incl",  .ops  = &dec_ops, },
-	{ .name = "ja",	   .ops  = &jump_ops, },
-	{ .name = "jae",   .ops  = &jump_ops, },
-	{ .name = "jb",	   .ops  = &jump_ops, },
-	{ .name = "jbe",   .ops  = &jump_ops, },
-	{ .name = "jc",	   .ops  = &jump_ops, },
-	{ .name = "jcxz",  .ops  = &jump_ops, },
-	{ .name = "je",	   .ops  = &jump_ops, },
-	{ .name = "jecxz", .ops  = &jump_ops, },
-	{ .name = "jg",	   .ops  = &jump_ops, },
-	{ .name = "jge",   .ops  = &jump_ops, },
-	{ .name = "jl",    .ops  = &jump_ops, },
-	{ .name = "jle",   .ops  = &jump_ops, },
-	{ .name = "jmp",   .ops  = &jump_ops, },
-	{ .name = "jmpq",  .ops  = &jump_ops, },
-	{ .name = "jna",   .ops  = &jump_ops, },
-	{ .name = "jnae",  .ops  = &jump_ops, },
-	{ .name = "jnb",   .ops  = &jump_ops, },
-	{ .name = "jnbe",  .ops  = &jump_ops, },
-	{ .name = "jnc",   .ops  = &jump_ops, },
-	{ .name = "jne",   .ops  = &jump_ops, },
-	{ .name = "jng",   .ops  = &jump_ops, },
-	{ .name = "jnge",  .ops  = &jump_ops, },
-	{ .name = "jnl",   .ops  = &jump_ops, },
-	{ .name = "jnle",  .ops  = &jump_ops, },
-	{ .name = "jno",   .ops  = &jump_ops, },
-	{ .name = "jnp",   .ops  = &jump_ops, },
-	{ .name = "jns",   .ops  = &jump_ops, },
-	{ .name = "jnz",   .ops  = &jump_ops, },
-	{ .name = "jo",	   .ops  = &jump_ops, },
-	{ .name = "jp",	   .ops  = &jump_ops, },
-	{ .name = "jpe",   .ops  = &jump_ops, },
-	{ .name = "jpo",   .ops  = &jump_ops, },
-	{ .name = "jrcxz", .ops  = &jump_ops, },
-	{ .name = "js",	   .ops  = &jump_ops, },
-	{ .name = "jz",	   .ops  = &jump_ops, },
-	{ .name = "lea",   .ops  = &mov_ops, },
-	{ .name = "lock",  .ops  = &lock_ops, },
-	{ .name = "mov",   .ops  = &mov_ops, },
-	{ .name = "movb",  .ops  = &mov_ops, },
-	{ .name = "movdqa",.ops  = &mov_ops, },
-	{ .name = "movl",  .ops  = &mov_ops, },
-	{ .name = "movq",  .ops  = &mov_ops, },
-	{ .name = "movslq", .ops  = &mov_ops, },
-	{ .name = "movzbl", .ops  = &mov_ops, },
-	{ .name = "movzwl", .ops  = &mov_ops, },
-	{ .name = "nop",   .ops  = &nop_ops, },
-	{ .name = "nopl",  .ops  = &nop_ops, },
-	{ .name = "nopw",  .ops  = &nop_ops, },
-	{ .name = "or",    .ops  = &mov_ops, },
-	{ .name = "orl",   .ops  = &mov_ops, },
-	{ .name = "test",  .ops  = &mov_ops, },
-	{ .name = "testb", .ops  = &mov_ops, },
-	{ .name = "testl", .ops  = &mov_ops, },
-	{ .name = "xadd",  .ops  = &mov_ops, },
-	{ .name = "xbeginl", .ops  = &jump_ops, },
-	{ .name = "xbeginq", .ops  = &jump_ops, },
-	{ .name = "retq",  .ops  = &ret_ops, },
-};
-
-struct instructions_powerpc {
+struct instructions_arch {
 	struct ins *ins;
 	struct list_head list;
 };
@@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
 	return strcmp(ia->name, ib->name);
 }
 
-static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
+static struct ins *list_add__ins_arch(struct instructions_arch *head,
 					 const char *name, struct ins_ops *ops)
 {
-	struct instructions_powerpc *ins_powerpc;
+	struct instructions_arch *ins_arch;
 	struct ins *ins;
 
 	ins = zalloc(sizeof(struct ins));
 	if (!ins)
 		return NULL;
 
-	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
-	if (!ins_powerpc)
+	ins_arch = zalloc(sizeof(struct instructions_arch));
+	if (!ins_arch)
 		goto out_free_ins;
 
 	ins->name = strdup(name);
 	if (!ins->name)
-		goto out_free_ins_power;
+		goto out_free_ins_arch;
 
 	ins->ops = ops;
-	ins_powerpc->ins = ins;
-	list_add_tail(&(ins_powerpc->list), &(head->list));
+	ins_arch->ins = ins;
+	list_add_tail(&(ins_arch->list), &(head->list));
 
 	return ins;
 
-out_free_ins_power:
-	zfree(&ins_powerpc);
+out_free_ins_arch:
+	zfree(&ins_arch);
 out_free_ins:
 	zfree(&ins);
 	return NULL;
 }
 
-static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
+static struct ins *list_search__ins_arch(struct instructions_arch *head,
 					    const char *name)
 {
-	struct instructions_powerpc *pos;
+	struct instructions_arch *pos;
 
 	list_for_each_entry(pos, &head->list, list) {
 		if (!strcmp(pos->ins->name, name))
@@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	int i;
 	struct ins *ins;
 	struct ins_ops *ops;
-	static struct instructions_powerpc head;
+	static struct instructions_arch head;
 	static bool list_initialized;
 
 	/*
@@ -629,7 +539,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	/*
 	 * Return if we already have object of 'struct ins' for this instruction
 	 */
-	ins = list_search__ins_powerpc(&head, name);
+	ins = list_search__ins_arch(&head, name);
 	if (ins)
 		return ins;
 
@@ -666,7 +576,44 @@ static struct ins *ins__find_powerpc(const char *name)
 	 * Add instruction to list so next time no need to
 	 * allocate memory for it.
 	 */
-	return list_add__ins_powerpc(&head, name, ops);
+	return list_add__ins_arch(&head, name, ops);
+}
+
+static struct ins *ins__find_arm(const char *name)
+{
+	struct ins *ins;
+	struct ins_ops *ops = &mov_ops;
+	static struct instructions_arch head;
+	static bool list_initialized;
+	regmatch_t match[2];
+	int ret;
+
+	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_arch(&head, name);
+	if (ins)
+		return ins;
+
+	ret = regexec(&arm_call_insn, name, 2, match, 0);
+	if (!ret) {
+		ops = &call_ops;
+	} else {
+		ret = regexec(&arm_jump_insn, name, 2, match, 0);
+		if (!ret)
+			ops = &jump_ops;
+	}
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_arch(&head, name, ops);
 }
 
 static void ins__sort(struct ins *instructions, int nmemb)
@@ -686,15 +633,26 @@ static const char *annotate__norm_arch(char *arch)
 	return normalize_arch(arch);
 }
 
+#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
+
 static struct ins *ins__find(const char *name, const char *norm_arch)
 {
 	static bool sorted;
 	struct ins *instructions;
-	int nmemb;
+	int nmemb, ret;
 
 	if (!sorted) {
 		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
-		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
+		if (!strcmp(norm_arch, NORM_ARM)) {
+			ret = regcomp(&arm_call_insn,
+				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
+			ret |= regcomp(&arm_jump_insn,
+				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
+			if (ret) {
+				pr_err("regcomp failed\n");
+				return NULL;
+			}
+		}
 		sorted = true;
 	}
 
@@ -702,8 +660,7 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
 		instructions = instructions_x86;
 		nmemb = ARRAY_SIZE(instructions_x86);
 	} else if (!strcmp(norm_arch, NORM_ARM)) {
-		instructions = instructions_arm;
-		nmemb = ARRAY_SIZE(instructions_arm);
+		return ins__find_arm(name);
 	} else if (!strcmp(norm_arch, NORM_POWERPC)) {
 		return ins__find_powerpc(name);
 	} else {

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

Kim

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-23 20:36       ` Kim Phillips
@ 2016-08-26  6:21         ` Namhyung Kim
  2016-08-26  7:26         ` Ravi Bangoria
  1 sibling, 0 replies; 20+ messages in thread
From: Namhyung Kim @ 2016-08-26  6:21 UTC (permalink / raw)
  To: Kim Phillips
  Cc: Ravi Bangoria, linux-kernel, linuxppc-dev,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Taeung Song, Naveen N. Rao, markus,
	chris.ryder, Pawel Moll, Masami Hiramatsu, rmk+kernel, Jiri Olsa,
	Michael Ellerman, Hemant Kumar

Hi,

On Tue, Aug 23, 2016 at 03:36:17PM -0500, Kim Phillips wrote:
> On Tue, 23 Aug 2016 11:17:16 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > On Tue, Aug 23, 2016 at 8:01 AM, Kim Phillips <kim.phillips@arm.com> wrote:
> > > On Fri, 19 Aug 2016 18:29:33 +0530
> > > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> > >
> > >> Changes in v6:
> > >>   - Instead of adding only those instructions defined in #ifdef __arm__,
> > >>     add all instructions from default table to arm table.
> > > Thanks, I've gone through the list and removed all not-ARM
> > > instructions, and added some missing ARM branch instructions:
> > 
> > Can we use regex patterns instead?
> 
> Yes, that helps prevent mistakes updating instruction lists - how does
> this look?:

Much better!

Thanks,
Namhyung


> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b2c6cf3..52316f3 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -26,6 +26,7 @@
>  const char 	*disassembler_style;
>  const char	*objdump_path;
>  static regex_t	 file_lineno;
> +static regex_t	 arm_call_insn, arm_jump_insn;
>  
>  static struct ins *ins__find(const char *name, const char *norm_arch);
>  static int disasm_line__parse(char *line, char **namep, char **rawp);
> @@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
>  	{ .name = "retq",  .ops  = &ret_ops, },
>  };
>  
> -static struct ins instructions_arm[] = {
> -	{ .name = "add",   .ops  = &mov_ops, },
> -	{ .name = "addl",  .ops  = &mov_ops, },
> -	{ .name = "addq",  .ops  = &mov_ops, },
> -	{ .name = "addw",  .ops  = &mov_ops, },
> -	{ .name = "and",   .ops  = &mov_ops, },
> -	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
> -	{ .name = "bcc",   .ops  = &jump_ops, },
> -	{ .name = "bcs",   .ops  = &jump_ops, },
> -	{ .name = "beq",   .ops  = &jump_ops, },
> -	{ .name = "bge",   .ops  = &jump_ops, },
> -	{ .name = "bgt",   .ops  = &jump_ops, },
> -	{ .name = "bhi",   .ops  = &jump_ops, },
> -	{ .name = "bl",    .ops  = &call_ops, },
> -	{ .name = "bls",   .ops  = &jump_ops, },
> -	{ .name = "blt",   .ops  = &jump_ops, },
> -	{ .name = "blx",   .ops  = &call_ops, },
> -	{ .name = "bne",   .ops  = &jump_ops, },
> -	{ .name = "bts",   .ops  = &mov_ops, },
> -	{ .name = "call",  .ops  = &call_ops, },
> -	{ .name = "callq", .ops  = &call_ops, },
> -	{ .name = "cmp",   .ops  = &mov_ops, },
> -	{ .name = "cmpb",  .ops  = &mov_ops, },
> -	{ .name = "cmpl",  .ops  = &mov_ops, },
> -	{ .name = "cmpq",  .ops  = &mov_ops, },
> -	{ .name = "cmpw",  .ops  = &mov_ops, },
> -	{ .name = "cmpxch", .ops  = &mov_ops, },
> -	{ .name = "dec",   .ops  = &dec_ops, },
> -	{ .name = "decl",  .ops  = &dec_ops, },
> -	{ .name = "imul",  .ops  = &mov_ops, },
> -	{ .name = "inc",   .ops  = &dec_ops, },
> -	{ .name = "incl",  .ops  = &dec_ops, },
> -	{ .name = "ja",	   .ops  = &jump_ops, },
> -	{ .name = "jae",   .ops  = &jump_ops, },
> -	{ .name = "jb",	   .ops  = &jump_ops, },
> -	{ .name = "jbe",   .ops  = &jump_ops, },
> -	{ .name = "jc",	   .ops  = &jump_ops, },
> -	{ .name = "jcxz",  .ops  = &jump_ops, },
> -	{ .name = "je",	   .ops  = &jump_ops, },
> -	{ .name = "jecxz", .ops  = &jump_ops, },
> -	{ .name = "jg",	   .ops  = &jump_ops, },
> -	{ .name = "jge",   .ops  = &jump_ops, },
> -	{ .name = "jl",    .ops  = &jump_ops, },
> -	{ .name = "jle",   .ops  = &jump_ops, },
> -	{ .name = "jmp",   .ops  = &jump_ops, },
> -	{ .name = "jmpq",  .ops  = &jump_ops, },
> -	{ .name = "jna",   .ops  = &jump_ops, },
> -	{ .name = "jnae",  .ops  = &jump_ops, },
> -	{ .name = "jnb",   .ops  = &jump_ops, },
> -	{ .name = "jnbe",  .ops  = &jump_ops, },
> -	{ .name = "jnc",   .ops  = &jump_ops, },
> -	{ .name = "jne",   .ops  = &jump_ops, },
> -	{ .name = "jng",   .ops  = &jump_ops, },
> -	{ .name = "jnge",  .ops  = &jump_ops, },
> -	{ .name = "jnl",   .ops  = &jump_ops, },
> -	{ .name = "jnle",  .ops  = &jump_ops, },
> -	{ .name = "jno",   .ops  = &jump_ops, },
> -	{ .name = "jnp",   .ops  = &jump_ops, },
> -	{ .name = "jns",   .ops  = &jump_ops, },
> -	{ .name = "jnz",   .ops  = &jump_ops, },
> -	{ .name = "jo",	   .ops  = &jump_ops, },
> -	{ .name = "jp",	   .ops  = &jump_ops, },
> -	{ .name = "jpe",   .ops  = &jump_ops, },
> -	{ .name = "jpo",   .ops  = &jump_ops, },
> -	{ .name = "jrcxz", .ops  = &jump_ops, },
> -	{ .name = "js",	   .ops  = &jump_ops, },
> -	{ .name = "jz",	   .ops  = &jump_ops, },
> -	{ .name = "lea",   .ops  = &mov_ops, },
> -	{ .name = "lock",  .ops  = &lock_ops, },
> -	{ .name = "mov",   .ops  = &mov_ops, },
> -	{ .name = "movb",  .ops  = &mov_ops, },
> -	{ .name = "movdqa",.ops  = &mov_ops, },
> -	{ .name = "movl",  .ops  = &mov_ops, },
> -	{ .name = "movq",  .ops  = &mov_ops, },
> -	{ .name = "movslq", .ops  = &mov_ops, },
> -	{ .name = "movzbl", .ops  = &mov_ops, },
> -	{ .name = "movzwl", .ops  = &mov_ops, },
> -	{ .name = "nop",   .ops  = &nop_ops, },
> -	{ .name = "nopl",  .ops  = &nop_ops, },
> -	{ .name = "nopw",  .ops  = &nop_ops, },
> -	{ .name = "or",    .ops  = &mov_ops, },
> -	{ .name = "orl",   .ops  = &mov_ops, },
> -	{ .name = "test",  .ops  = &mov_ops, },
> -	{ .name = "testb", .ops  = &mov_ops, },
> -	{ .name = "testl", .ops  = &mov_ops, },
> -	{ .name = "xadd",  .ops  = &mov_ops, },
> -	{ .name = "xbeginl", .ops  = &jump_ops, },
> -	{ .name = "xbeginq", .ops  = &jump_ops, },
> -	{ .name = "retq",  .ops  = &ret_ops, },
> -};
> -
> -struct instructions_powerpc {
> +struct instructions_arch {
>  	struct ins *ins;
>  	struct list_head list;
>  };
> @@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }
>  
> -static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
> +static struct ins *list_add__ins_arch(struct instructions_arch *head,
>  					 const char *name, struct ins_ops *ops)
>  {
> -	struct instructions_powerpc *ins_powerpc;
> +	struct instructions_arch *ins_arch;
>  	struct ins *ins;
>  
>  	ins = zalloc(sizeof(struct ins));
>  	if (!ins)
>  		return NULL;
>  
> -	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
> -	if (!ins_powerpc)
> +	ins_arch = zalloc(sizeof(struct instructions_arch));
> +	if (!ins_arch)
>  		goto out_free_ins;
>  
>  	ins->name = strdup(name);
>  	if (!ins->name)
> -		goto out_free_ins_power;
> +		goto out_free_ins_arch;
>  
>  	ins->ops = ops;
> -	ins_powerpc->ins = ins;
> -	list_add_tail(&(ins_powerpc->list), &(head->list));
> +	ins_arch->ins = ins;
> +	list_add_tail(&(ins_arch->list), &(head->list));
>  
>  	return ins;
>  
> -out_free_ins_power:
> -	zfree(&ins_powerpc);
> +out_free_ins_arch:
> +	zfree(&ins_arch);
>  out_free_ins:
>  	zfree(&ins);
>  	return NULL;
>  }
>  
> -static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
> +static struct ins *list_search__ins_arch(struct instructions_arch *head,
>  					    const char *name)
>  {
> -	struct instructions_powerpc *pos;
> +	struct instructions_arch *pos;
>  
>  	list_for_each_entry(pos, &head->list, list) {
>  		if (!strcmp(pos->ins->name, name))
> @@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	int i;
>  	struct ins *ins;
>  	struct ins_ops *ops;
> -	static struct instructions_powerpc head;
> +	static struct instructions_arch head;
>  	static bool list_initialized;
>  
>  	/*
> @@ -629,7 +539,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	/*
>  	 * Return if we already have object of 'struct ins' for this instruction
>  	 */
> -	ins = list_search__ins_powerpc(&head, name);
> +	ins = list_search__ins_arch(&head, name);
>  	if (ins)
>  		return ins;
>  
> @@ -666,7 +576,44 @@ static struct ins *ins__find_powerpc(const char *name)
>  	 * Add instruction to list so next time no need to
>  	 * allocate memory for it.
>  	 */
> -	return list_add__ins_powerpc(&head, name, ops);
> +	return list_add__ins_arch(&head, name, ops);
> +}
> +
> +static struct ins *ins__find_arm(const char *name)
> +{
> +	struct ins *ins;
> +	struct ins_ops *ops = &mov_ops;
> +	static struct instructions_arch head;
> +	static bool list_initialized;
> +	regmatch_t match[2];
> +	int ret;
> +
> +	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_arch(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ret = regexec(&arm_call_insn, name, 2, match, 0);
> +	if (!ret) {
> +		ops = &call_ops;
> +	} else {
> +		ret = regexec(&arm_jump_insn, name, 2, match, 0);
> +		if (!ret)
> +			ops = &jump_ops;
> +	}
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_arch(&head, name, ops);
>  }
>  
>  static void ins__sort(struct ins *instructions, int nmemb)
> @@ -686,15 +633,26 @@ static const char *annotate__norm_arch(char *arch)
>  	return normalize_arch(arch);
>  }
>  
> +#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
> +
>  static struct ins *ins__find(const char *name, const char *norm_arch)
>  {
>  	static bool sorted;
>  	struct ins *instructions;
> -	int nmemb;
> +	int nmemb, ret;
>  
>  	if (!sorted) {
>  		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
> -		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
> +		if (!strcmp(norm_arch, NORM_ARM)) {
> +			ret = regcomp(&arm_call_insn,
> +				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
> +			ret |= regcomp(&arm_jump_insn,
> +				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
> +			if (ret) {
> +				pr_err("regcomp failed\n");
> +				return NULL;
> +			}
> +		}
>  		sorted = true;
>  	}
>  
> @@ -702,8 +660,7 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
>  		instructions = instructions_x86;
>  		nmemb = ARRAY_SIZE(instructions_x86);
>  	} else if (!strcmp(norm_arch, NORM_ARM)) {
> -		instructions = instructions_arm;
> -		nmemb = ARRAY_SIZE(instructions_arm);
> +		return ins__find_arm(name);
>  	} else if (!strcmp(norm_arch, NORM_POWERPC)) {
>  		return ins__find_powerpc(name);
>  	} else {
> 
> Note that, for ARM, in order to match return instructions, 'lr' must be
> found to be the operand of the branch instruction, which is not
> contained in the 'name' variable passed to ins__find().
> 
> Kim

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 2/7] perf annotate: Add cross arch annotate support
  2016-08-23 20:36       ` Kim Phillips
  2016-08-26  6:21         ` Namhyung Kim
@ 2016-08-26  7:26         ` Ravi Bangoria
  2016-08-27  0:40           ` [PATCH] perf annotate: cross arch annotate support fixes for ARM Kim Phillips
  1 sibling, 1 reply; 20+ messages in thread
From: Ravi Bangoria @ 2016-08-26  7:26 UTC (permalink / raw)
  To: Kim Phillips
  Cc: Namhyung Kim, linux-kernel, linuxppc-dev,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Taeung Song, Naveen N. Rao, markus,
	chris.ryder, Pawel Moll, Masami Hiramatsu, rmk+kernel, Jiri Olsa,
	Michael Ellerman, Hemant Kumar, Ravi Bangoria

Hi Kim,

I've tested your patch on x86 and powerpc and it looks fine to me. Can you please
put your signed-off-by.

Please add Act-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> as well.

Regards,
-Ravi

On Wednesday 24 August 2016 02:06 AM, Kim Phillips wrote:
> On Tue, 23 Aug 2016 11:17:16 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> On Tue, Aug 23, 2016 at 8:01 AM, Kim Phillips <kim.phillips@arm.com> wrote:
>>> On Fri, 19 Aug 2016 18:29:33 +0530
>>> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>
>>>> Changes in v6:
>>>>   - Instead of adding only those instructions defined in #ifdef __arm__,
>>>>     add all instructions from default table to arm table.
>>> Thanks, I've gone through the list and removed all not-ARM
>>> instructions, and added some missing ARM branch instructions:
>> Can we use regex patterns instead?
> Yes, that helps prevent mistakes updating instruction lists - how does
> this look?:
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b2c6cf3..52316f3 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -26,6 +26,7 @@
>  const char 	*disassembler_style;
>  const char	*objdump_path;
>  static regex_t	 file_lineno;
> +static regex_t	 arm_call_insn, arm_jump_insn;
>
>  static struct ins *ins__find(const char *name, const char *norm_arch);
>  static int disasm_line__parse(char *line, char **namep, char **rawp);
> @@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
>  	{ .name = "retq",  .ops  = &ret_ops, },
>  };
>
> -static struct ins instructions_arm[] = {
> -	{ .name = "add",   .ops  = &mov_ops, },
> -	{ .name = "addl",  .ops  = &mov_ops, },
> -	{ .name = "addq",  .ops  = &mov_ops, },
> -	{ .name = "addw",  .ops  = &mov_ops, },
> -	{ .name = "and",   .ops  = &mov_ops, },
> -	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
> -	{ .name = "bcc",   .ops  = &jump_ops, },
> -	{ .name = "bcs",   .ops  = &jump_ops, },
> -	{ .name = "beq",   .ops  = &jump_ops, },
> -	{ .name = "bge",   .ops  = &jump_ops, },
> -	{ .name = "bgt",   .ops  = &jump_ops, },
> -	{ .name = "bhi",   .ops  = &jump_ops, },
> -	{ .name = "bl",    .ops  = &call_ops, },
> -	{ .name = "bls",   .ops  = &jump_ops, },
> -	{ .name = "blt",   .ops  = &jump_ops, },
> -	{ .name = "blx",   .ops  = &call_ops, },
> -	{ .name = "bne",   .ops  = &jump_ops, },
> -	{ .name = "bts",   .ops  = &mov_ops, },
> -	{ .name = "call",  .ops  = &call_ops, },
> -	{ .name = "callq", .ops  = &call_ops, },
> -	{ .name = "cmp",   .ops  = &mov_ops, },
> -	{ .name = "cmpb",  .ops  = &mov_ops, },
> -	{ .name = "cmpl",  .ops  = &mov_ops, },
> -	{ .name = "cmpq",  .ops  = &mov_ops, },
> -	{ .name = "cmpw",  .ops  = &mov_ops, },
> -	{ .name = "cmpxch", .ops  = &mov_ops, },
> -	{ .name = "dec",   .ops  = &dec_ops, },
> -	{ .name = "decl",  .ops  = &dec_ops, },
> -	{ .name = "imul",  .ops  = &mov_ops, },
> -	{ .name = "inc",   .ops  = &dec_ops, },
> -	{ .name = "incl",  .ops  = &dec_ops, },
> -	{ .name = "ja",	   .ops  = &jump_ops, },
> -	{ .name = "jae",   .ops  = &jump_ops, },
> -	{ .name = "jb",	   .ops  = &jump_ops, },
> -	{ .name = "jbe",   .ops  = &jump_ops, },
> -	{ .name = "jc",	   .ops  = &jump_ops, },
> -	{ .name = "jcxz",  .ops  = &jump_ops, },
> -	{ .name = "je",	   .ops  = &jump_ops, },
> -	{ .name = "jecxz", .ops  = &jump_ops, },
> -	{ .name = "jg",	   .ops  = &jump_ops, },
> -	{ .name = "jge",   .ops  = &jump_ops, },
> -	{ .name = "jl",    .ops  = &jump_ops, },
> -	{ .name = "jle",   .ops  = &jump_ops, },
> -	{ .name = "jmp",   .ops  = &jump_ops, },
> -	{ .name = "jmpq",  .ops  = &jump_ops, },
> -	{ .name = "jna",   .ops  = &jump_ops, },
> -	{ .name = "jnae",  .ops  = &jump_ops, },
> -	{ .name = "jnb",   .ops  = &jump_ops, },
> -	{ .name = "jnbe",  .ops  = &jump_ops, },
> -	{ .name = "jnc",   .ops  = &jump_ops, },
> -	{ .name = "jne",   .ops  = &jump_ops, },
> -	{ .name = "jng",   .ops  = &jump_ops, },
> -	{ .name = "jnge",  .ops  = &jump_ops, },
> -	{ .name = "jnl",   .ops  = &jump_ops, },
> -	{ .name = "jnle",  .ops  = &jump_ops, },
> -	{ .name = "jno",   .ops  = &jump_ops, },
> -	{ .name = "jnp",   .ops  = &jump_ops, },
> -	{ .name = "jns",   .ops  = &jump_ops, },
> -	{ .name = "jnz",   .ops  = &jump_ops, },
> -	{ .name = "jo",	   .ops  = &jump_ops, },
> -	{ .name = "jp",	   .ops  = &jump_ops, },
> -	{ .name = "jpe",   .ops  = &jump_ops, },
> -	{ .name = "jpo",   .ops  = &jump_ops, },
> -	{ .name = "jrcxz", .ops  = &jump_ops, },
> -	{ .name = "js",	   .ops  = &jump_ops, },
> -	{ .name = "jz",	   .ops  = &jump_ops, },
> -	{ .name = "lea",   .ops  = &mov_ops, },
> -	{ .name = "lock",  .ops  = &lock_ops, },
> -	{ .name = "mov",   .ops  = &mov_ops, },
> -	{ .name = "movb",  .ops  = &mov_ops, },
> -	{ .name = "movdqa",.ops  = &mov_ops, },
> -	{ .name = "movl",  .ops  = &mov_ops, },
> -	{ .name = "movq",  .ops  = &mov_ops, },
> -	{ .name = "movslq", .ops  = &mov_ops, },
> -	{ .name = "movzbl", .ops  = &mov_ops, },
> -	{ .name = "movzwl", .ops  = &mov_ops, },
> -	{ .name = "nop",   .ops  = &nop_ops, },
> -	{ .name = "nopl",  .ops  = &nop_ops, },
> -	{ .name = "nopw",  .ops  = &nop_ops, },
> -	{ .name = "or",    .ops  = &mov_ops, },
> -	{ .name = "orl",   .ops  = &mov_ops, },
> -	{ .name = "test",  .ops  = &mov_ops, },
> -	{ .name = "testb", .ops  = &mov_ops, },
> -	{ .name = "testl", .ops  = &mov_ops, },
> -	{ .name = "xadd",  .ops  = &mov_ops, },
> -	{ .name = "xbeginl", .ops  = &jump_ops, },
> -	{ .name = "xbeginq", .ops  = &jump_ops, },
> -	{ .name = "retq",  .ops  = &ret_ops, },
> -};
> -
> -struct instructions_powerpc {
> +struct instructions_arch {
>  	struct ins *ins;
>  	struct list_head list;
>  };
> @@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
>  	return strcmp(ia->name, ib->name);
>  }
>
> -static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
> +static struct ins *list_add__ins_arch(struct instructions_arch *head,
>  					 const char *name, struct ins_ops *ops)
>  {
> -	struct instructions_powerpc *ins_powerpc;
> +	struct instructions_arch *ins_arch;
>  	struct ins *ins;
>
>  	ins = zalloc(sizeof(struct ins));
>  	if (!ins)
>  		return NULL;
>
> -	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
> -	if (!ins_powerpc)
> +	ins_arch = zalloc(sizeof(struct instructions_arch));
> +	if (!ins_arch)
>  		goto out_free_ins;
>
>  	ins->name = strdup(name);
>  	if (!ins->name)
> -		goto out_free_ins_power;
> +		goto out_free_ins_arch;
>
>  	ins->ops = ops;
> -	ins_powerpc->ins = ins;
> -	list_add_tail(&(ins_powerpc->list), &(head->list));
> +	ins_arch->ins = ins;
> +	list_add_tail(&(ins_arch->list), &(head->list));
>
>  	return ins;
>
> -out_free_ins_power:
> -	zfree(&ins_powerpc);
> +out_free_ins_arch:
> +	zfree(&ins_arch);
>  out_free_ins:
>  	zfree(&ins);
>  	return NULL;
>  }
>
> -static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
> +static struct ins *list_search__ins_arch(struct instructions_arch *head,
>  					    const char *name)
>  {
> -	struct instructions_powerpc *pos;
> +	struct instructions_arch *pos;
>
>  	list_for_each_entry(pos, &head->list, list) {
>  		if (!strcmp(pos->ins->name, name))
> @@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	int i;
>  	struct ins *ins;
>  	struct ins_ops *ops;
> -	static struct instructions_powerpc head;
> +	static struct instructions_arch head;
>  	static bool list_initialized;
>
>  	/*
> @@ -629,7 +539,7 @@ static struct ins *ins__find_powerpc(const char *name)
>  	/*
>  	 * Return if we already have object of 'struct ins' for this instruction
>  	 */
> -	ins = list_search__ins_powerpc(&head, name);
> +	ins = list_search__ins_arch(&head, name);
>  	if (ins)
>  		return ins;
>
> @@ -666,7 +576,44 @@ static struct ins *ins__find_powerpc(const char *name)
>  	 * Add instruction to list so next time no need to
>  	 * allocate memory for it.
>  	 */
> -	return list_add__ins_powerpc(&head, name, ops);
> +	return list_add__ins_arch(&head, name, ops);
> +}
> +
> +static struct ins *ins__find_arm(const char *name)
> +{
> +	struct ins *ins;
> +	struct ins_ops *ops = &mov_ops;
> +	static struct instructions_arch head;
> +	static bool list_initialized;
> +	regmatch_t match[2];
> +	int ret;
> +
> +	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_arch(&head, name);
> +	if (ins)
> +		return ins;
> +
> +	ret = regexec(&arm_call_insn, name, 2, match, 0);
> +	if (!ret) {
> +		ops = &call_ops;
> +	} else {
> +		ret = regexec(&arm_jump_insn, name, 2, match, 0);
> +		if (!ret)
> +			ops = &jump_ops;
> +	}
> +
> +	/*
> +	 * Add instruction to list so next time no need to
> +	 * allocate memory for it.
> +	 */
> +	return list_add__ins_arch(&head, name, ops);
>  }
>
>  static void ins__sort(struct ins *instructions, int nmemb)
> @@ -686,15 +633,26 @@ static const char *annotate__norm_arch(char *arch)
>  	return normalize_arch(arch);
>  }
>
> +#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
> +
>  static struct ins *ins__find(const char *name, const char *norm_arch)
>  {
>  	static bool sorted;
>  	struct ins *instructions;
> -	int nmemb;
> +	int nmemb, ret;
>
>  	if (!sorted) {
>  		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
> -		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
> +		if (!strcmp(norm_arch, NORM_ARM)) {
> +			ret = regcomp(&arm_call_insn,
> +				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
> +			ret |= regcomp(&arm_jump_insn,
> +				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
> +			if (ret) {
> +				pr_err("regcomp failed\n");
> +				return NULL;
> +			}
> +		}
>  		sorted = true;
>  	}
>
> @@ -702,8 +660,7 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
>  		instructions = instructions_x86;
>  		nmemb = ARRAY_SIZE(instructions_x86);
>  	} else if (!strcmp(norm_arch, NORM_ARM)) {
> -		instructions = instructions_arm;
> -		nmemb = ARRAY_SIZE(instructions_arm);
> +		return ins__find_arm(name);
>  	} else if (!strcmp(norm_arch, NORM_POWERPC)) {
>  		return ins__find_powerpc(name);
>  	} else {
>
> Note that, for ARM, in order to match return instructions, 'lr' must be
> found to be the operand of the branch instruction, which is not
> contained in the 'name' variable passed to ins__find().
>
> Kim
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] perf annotate: cross arch annotate support fixes for ARM
  2016-08-26  7:26         ` Ravi Bangoria
@ 2016-08-27  0:40           ` Kim Phillips
  0 siblings, 0 replies; 20+ messages in thread
From: Kim Phillips @ 2016-08-27  0:40 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: Namhyung Kim, linux-kernel, linuxppc-dev,
	Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Alexander Shishkin, Taeung Song, Naveen N. Rao, markus,
	chris.ryder, Pawel Moll, Masami Hiramatsu, rmk+kernel, Jiri Olsa,
	Michael Ellerman, Hemant Kumar

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

Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Acked-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 177 +++++++++++++++++----------------------------
 1 file changed, 67 insertions(+), 110 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b2c6cf3..52316f3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -26,6 +26,7 @@
 const char 	*disassembler_style;
 const char	*objdump_path;
 static regex_t	 file_lineno;
+static regex_t	 arm_call_insn, arm_jump_insn;
 
 static struct ins *ins__find(const char *name, const char *norm_arch);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
@@ -449,98 +450,7 @@ static struct ins instructions_x86[] = {
 	{ .name = "retq",  .ops  = &ret_ops, },
 };
 
-static struct ins instructions_arm[] = {
-	{ .name = "add",   .ops  = &mov_ops, },
-	{ .name = "addl",  .ops  = &mov_ops, },
-	{ .name = "addq",  .ops  = &mov_ops, },
-	{ .name = "addw",  .ops  = &mov_ops, },
-	{ .name = "and",   .ops  = &mov_ops, },
-	{ .name = "b",     .ops  = &jump_ops, }, /* might also be a call */
-	{ .name = "bcc",   .ops  = &jump_ops, },
-	{ .name = "bcs",   .ops  = &jump_ops, },
-	{ .name = "beq",   .ops  = &jump_ops, },
-	{ .name = "bge",   .ops  = &jump_ops, },
-	{ .name = "bgt",   .ops  = &jump_ops, },
-	{ .name = "bhi",   .ops  = &jump_ops, },
-	{ .name = "bl",    .ops  = &call_ops, },
-	{ .name = "bls",   .ops  = &jump_ops, },
-	{ .name = "blt",   .ops  = &jump_ops, },
-	{ .name = "blx",   .ops  = &call_ops, },
-	{ .name = "bne",   .ops  = &jump_ops, },
-	{ .name = "bts",   .ops  = &mov_ops, },
-	{ .name = "call",  .ops  = &call_ops, },
-	{ .name = "callq", .ops  = &call_ops, },
-	{ .name = "cmp",   .ops  = &mov_ops, },
-	{ .name = "cmpb",  .ops  = &mov_ops, },
-	{ .name = "cmpl",  .ops  = &mov_ops, },
-	{ .name = "cmpq",  .ops  = &mov_ops, },
-	{ .name = "cmpw",  .ops  = &mov_ops, },
-	{ .name = "cmpxch", .ops  = &mov_ops, },
-	{ .name = "dec",   .ops  = &dec_ops, },
-	{ .name = "decl",  .ops  = &dec_ops, },
-	{ .name = "imul",  .ops  = &mov_ops, },
-	{ .name = "inc",   .ops  = &dec_ops, },
-	{ .name = "incl",  .ops  = &dec_ops, },
-	{ .name = "ja",	   .ops  = &jump_ops, },
-	{ .name = "jae",   .ops  = &jump_ops, },
-	{ .name = "jb",	   .ops  = &jump_ops, },
-	{ .name = "jbe",   .ops  = &jump_ops, },
-	{ .name = "jc",	   .ops  = &jump_ops, },
-	{ .name = "jcxz",  .ops  = &jump_ops, },
-	{ .name = "je",	   .ops  = &jump_ops, },
-	{ .name = "jecxz", .ops  = &jump_ops, },
-	{ .name = "jg",	   .ops  = &jump_ops, },
-	{ .name = "jge",   .ops  = &jump_ops, },
-	{ .name = "jl",    .ops  = &jump_ops, },
-	{ .name = "jle",   .ops  = &jump_ops, },
-	{ .name = "jmp",   .ops  = &jump_ops, },
-	{ .name = "jmpq",  .ops  = &jump_ops, },
-	{ .name = "jna",   .ops  = &jump_ops, },
-	{ .name = "jnae",  .ops  = &jump_ops, },
-	{ .name = "jnb",   .ops  = &jump_ops, },
-	{ .name = "jnbe",  .ops  = &jump_ops, },
-	{ .name = "jnc",   .ops  = &jump_ops, },
-	{ .name = "jne",   .ops  = &jump_ops, },
-	{ .name = "jng",   .ops  = &jump_ops, },
-	{ .name = "jnge",  .ops  = &jump_ops, },
-	{ .name = "jnl",   .ops  = &jump_ops, },
-	{ .name = "jnle",  .ops  = &jump_ops, },
-	{ .name = "jno",   .ops  = &jump_ops, },
-	{ .name = "jnp",   .ops  = &jump_ops, },
-	{ .name = "jns",   .ops  = &jump_ops, },
-	{ .name = "jnz",   .ops  = &jump_ops, },
-	{ .name = "jo",	   .ops  = &jump_ops, },
-	{ .name = "jp",	   .ops  = &jump_ops, },
-	{ .name = "jpe",   .ops  = &jump_ops, },
-	{ .name = "jpo",   .ops  = &jump_ops, },
-	{ .name = "jrcxz", .ops  = &jump_ops, },
-	{ .name = "js",	   .ops  = &jump_ops, },
-	{ .name = "jz",	   .ops  = &jump_ops, },
-	{ .name = "lea",   .ops  = &mov_ops, },
-	{ .name = "lock",  .ops  = &lock_ops, },
-	{ .name = "mov",   .ops  = &mov_ops, },
-	{ .name = "movb",  .ops  = &mov_ops, },
-	{ .name = "movdqa",.ops  = &mov_ops, },
-	{ .name = "movl",  .ops  = &mov_ops, },
-	{ .name = "movq",  .ops  = &mov_ops, },
-	{ .name = "movslq", .ops  = &mov_ops, },
-	{ .name = "movzbl", .ops  = &mov_ops, },
-	{ .name = "movzwl", .ops  = &mov_ops, },
-	{ .name = "nop",   .ops  = &nop_ops, },
-	{ .name = "nopl",  .ops  = &nop_ops, },
-	{ .name = "nopw",  .ops  = &nop_ops, },
-	{ .name = "or",    .ops  = &mov_ops, },
-	{ .name = "orl",   .ops  = &mov_ops, },
-	{ .name = "test",  .ops  = &mov_ops, },
-	{ .name = "testb", .ops  = &mov_ops, },
-	{ .name = "testl", .ops  = &mov_ops, },
-	{ .name = "xadd",  .ops  = &mov_ops, },
-	{ .name = "xbeginl", .ops  = &jump_ops, },
-	{ .name = "xbeginq", .ops  = &jump_ops, },
-	{ .name = "retq",  .ops  = &ret_ops, },
-};
-
-struct instructions_powerpc {
+struct instructions_arch {
 	struct ins *ins;
 	struct list_head list;
 };
@@ -560,41 +470,41 @@ static int ins__cmp(const void *a, const void *b)
 	return strcmp(ia->name, ib->name);
 }
 
-static struct ins *list_add__ins_powerpc(struct instructions_powerpc *head,
+static struct ins *list_add__ins_arch(struct instructions_arch *head,
 					 const char *name, struct ins_ops *ops)
 {
-	struct instructions_powerpc *ins_powerpc;
+	struct instructions_arch *ins_arch;
 	struct ins *ins;
 
 	ins = zalloc(sizeof(struct ins));
 	if (!ins)
 		return NULL;
 
-	ins_powerpc = zalloc(sizeof(struct instructions_powerpc));
-	if (!ins_powerpc)
+	ins_arch = zalloc(sizeof(struct instructions_arch));
+	if (!ins_arch)
 		goto out_free_ins;
 
 	ins->name = strdup(name);
 	if (!ins->name)
-		goto out_free_ins_power;
+		goto out_free_ins_arch;
 
 	ins->ops = ops;
-	ins_powerpc->ins = ins;
-	list_add_tail(&(ins_powerpc->list), &(head->list));
+	ins_arch->ins = ins;
+	list_add_tail(&(ins_arch->list), &(head->list));
 
 	return ins;
 
-out_free_ins_power:
-	zfree(&ins_powerpc);
+out_free_ins_arch:
+	zfree(&ins_arch);
 out_free_ins:
 	zfree(&ins);
 	return NULL;
 }
 
-static struct ins *list_search__ins_powerpc(struct instructions_powerpc *head,
+static struct ins *list_search__ins_arch(struct instructions_arch *head,
 					    const char *name)
 {
-	struct instructions_powerpc *pos;
+	struct instructions_arch *pos;
 
 	list_for_each_entry(pos, &head->list, list) {
 		if (!strcmp(pos->ins->name, name))
@@ -608,7 +518,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	int i;
 	struct ins *ins;
 	struct ins_ops *ops;
-	static struct instructions_powerpc head;
+	static struct instructions_arch head;
 	static bool list_initialized;
 
 	/*
@@ -629,7 +539,7 @@ static struct ins *ins__find_powerpc(const char *name)
 	/*
 	 * Return if we already have object of 'struct ins' for this instruction
 	 */
-	ins = list_search__ins_powerpc(&head, name);
+	ins = list_search__ins_arch(&head, name);
 	if (ins)
 		return ins;
 
@@ -666,7 +576,44 @@ static struct ins *ins__find_powerpc(const char *name)
 	 * Add instruction to list so next time no need to
 	 * allocate memory for it.
 	 */
-	return list_add__ins_powerpc(&head, name, ops);
+	return list_add__ins_arch(&head, name, ops);
+}
+
+static struct ins *ins__find_arm(const char *name)
+{
+	struct ins *ins;
+	struct ins_ops *ops = &mov_ops;
+	static struct instructions_arch head;
+	static bool list_initialized;
+	regmatch_t match[2];
+	int ret;
+
+	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_arch(&head, name);
+	if (ins)
+		return ins;
+
+	ret = regexec(&arm_call_insn, name, 2, match, 0);
+	if (!ret) {
+		ops = &call_ops;
+	} else {
+		ret = regexec(&arm_jump_insn, name, 2, match, 0);
+		if (!ret)
+			ops = &jump_ops;
+	}
+
+	/*
+	 * Add instruction to list so next time no need to
+	 * allocate memory for it.
+	 */
+	return list_add__ins_arch(&head, name, ops);
 }
 
 static void ins__sort(struct ins *instructions, int nmemb)
@@ -686,15 +633,26 @@ static const char *annotate__norm_arch(char *arch)
 	return normalize_arch(arch);
 }
 
+#define ARM_CONDS "(cc|cs|eq|ge|gt|hi|le|ls|lt|mi|ne|pl|vc|vs)"
+
 static struct ins *ins__find(const char *name, const char *norm_arch)
 {
 	static bool sorted;
 	struct ins *instructions;
-	int nmemb;
+	int nmemb, ret;
 
 	if (!sorted) {
 		ins__sort(instructions_x86, ARRAY_SIZE(instructions_x86));
-		ins__sort(instructions_arm, ARRAY_SIZE(instructions_arm));
+		if (!strcmp(norm_arch, NORM_ARM)) {
+			ret = regcomp(&arm_call_insn,
+				      "^blx?" ARM_CONDS "?$", REG_EXTENDED);
+			ret |= regcomp(&arm_jump_insn,
+				       "^bx?" ARM_CONDS "?$", REG_EXTENDED);
+			if (ret) {
+				pr_err("regcomp failed\n");
+				return NULL;
+			}
+		}
 		sorted = true;
 	}
 
@@ -702,8 +660,7 @@ static struct ins *ins__find(const char *name, const char *norm_arch)
 		instructions = instructions_x86;
 		nmemb = ARRAY_SIZE(instructions_x86);
 	} else if (!strcmp(norm_arch, NORM_ARM)) {
-		instructions = instructions_arm;
-		nmemb = ARRAY_SIZE(instructions_arm);
+		return ins__find_arm(name);
 	} else if (!strcmp(norm_arch, NORM_POWERPC)) {
 		return ins__find_powerpc(name);
 	} else {
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes
  2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
                   ` (6 preceding siblings ...)
  2016-08-19 12:59 ` [PATCH v6 7/7] perf annotate: Fix jump target outside of function address range Ravi Bangoria
@ 2016-09-07 15:39 ` Ravi Bangoria
  7 siblings, 0 replies; 20+ messages in thread
From: Ravi Bangoria @ 2016-09-07 15:39 UTC (permalink / raw)
  To: linux-kernel, linuxppc-dev, acme
  Cc: peterz, mingo, alexander.shishkin, treeze.taeung, naveen.n.rao,
	markus, chris.ryder, pawel.moll, mhiramat, rmk+kernel, jolsa,
	mpe, hemant, namhyung, Ravi Bangoria

Hello,

Any update on this?

-Ravi

On Friday 19 August 2016 06:29 PM, Ravi Bangoria wrote:
> Currently Perf annotate support code navigation (branches and calls)
> only when run on the same architecture where perf.data was recorded.
> But, for example, record on powerpc server and annotate on client's
> x86 desktop is not supported.
>
> This patchset 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:
> I don't have arm test machine. As suggested by Russell in one of the
> review comment, I've copied all instructions from default table to
> arm table. This way it want break tool on arm but cleanup is needed
> for x86 specific instructions added in arm table.
>
> Example:
>
>   Record on powerpc:
>   $ ./perf record -a
>
>   Report -> Annotate on x86:
>   $ ./perf report -i perf.data.powerpc --vmlinux vmlinux.powerpc
>
> Changes in v6:
>   - Instead of adding only those instructions defined in #ifdef __arm__,
>     add all instructions from default table to arm table.
>
> v5 link:
>   https://lkml.org/lkml/2016/8/19/35
>
> 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         | 330 +++++++++++++++++++++++++++++++------
>  tools/perf/util/annotate.h         |  10 +-
>  tools/perf/util/unwind-libunwind.c |   4 +-
>  8 files changed, 327 insertions(+), 76 deletions(-)
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 3/7] perf annotate: Add support for powerpc
  2016-08-19 12:59 ` [PATCH v6 3/7] perf annotate: Add support for powerpc Ravi Bangoria
@ 2016-09-19 15:26   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-09-19 15:26 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, linuxppc-dev, peterz, mingo, alexander.shishkin,
	treeze.taeung, naveen.n.rao, markus, chris.ryder, pawel.moll,
	mhiramat, rmk+kernel, jolsa, mpe, hemant, namhyung

Em Fri, Aug 19, 2016 at 06:29:34PM +0530, Ravi Bangoria escreveu:
> From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
> 
> +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;
> +	}

Why not ditch list_initialized and instead just declare the list as:

static struct instructions_powerpc head = {
	.list = LIST_HEAD_INIT(head.list),
}

Just like the kernel sources do? See for instance:

  net/core/net_namespace.c

struct net init_net = {
        .dev_base_head = LIST_HEAD_INIT(init_net.dev_base_head),
};

> +
> +	/*
> +	 * 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);
> @@ -585,6 +699,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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target
  2016-08-19 12:59 ` [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target Ravi Bangoria
@ 2016-09-19 15:44   ` Arnaldo Carvalho de Melo
  2016-09-20 14:35     ` Ravi Bangoria
  2016-09-20 21:43   ` [tip:perf/core] " tip-bot for Ravi Bangoria
  1 sibling, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-09-19 15:44 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, linuxppc-dev, peterz, mingo, alexander.shishkin,
	treeze.taeung, naveen.n.rao, markus, chris.ryder, pawel.moll,
	mhiramat, rmk+kernel, jolsa, mpe, hemant, namhyung

Em Fri, Aug 19, 2016 at 06:29:35PM +0530, Ravi Bangoria escreveu:
> 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

Ok, makes sense, but then now I have the -> and can't press enter to go
to that function, in fact for the case I'm using as a test, the
vsnprintf kernel function, I get:

       │ 56:   test   %al,%al                                                                                                                                ▒
       │     ↓ je     81                                                                                                                                     ▒
       │       lea    -0x38(%rbp),%rsi                                                                                                                       ▒
       │       mov    %r15,%rdi                                                                                                                              ▒
       │     → callq  0xffffffff993e3230 

That 0xffffffff993e3230 should've been resolved to:

[root@jouet ~]# grep ffffffff993e3230 /proc/kallsyms 
ffffffff993e3230 t format_decode

Trying to investigate why it doesn't...

- Arnaldo

> 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 v6:
>   - No change
> 
>  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 ea07588..a05423b 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target
  2016-09-19 15:44   ` Arnaldo Carvalho de Melo
@ 2016-09-20 14:35     ` Ravi Bangoria
  2016-09-20 14:56       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 20+ messages in thread
From: Ravi Bangoria @ 2016-09-20 14:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: linux-kernel, linuxppc-dev, peterz, mingo, alexander.shishkin,
	treeze.taeung, naveen.n.rao, markus, chris.ryder, pawel.moll,
	mhiramat, rmk+kernel, jolsa, mpe, hemant, namhyung,
	Ravi Bangoria

Hi Arnaldo,

On Monday 19 September 2016 09:14 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Aug 19, 2016 at 06:29:35PM +0530, Ravi Bangoria escreveu:
>> 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
> Ok, makes sense, but then now I have the -> and can't press enter to go
> to that function, in fact for the case I'm using as a test, the
> vsnprintf kernel function, I get:
>
>        │ 56:   test   %al,%al                                                                                                                                ▒
>        │     ↓ je     81                                                                                                                                     ▒
>        │       lea    -0x38(%rbp),%rsi                                                                                                                       ▒
>        │       mov    %r15,%rdi                                                                                                                              ▒
>        │     → callq  0xffffffff993e3230 
>
> That 0xffffffff993e3230 should've been resolved to:
>
> [root@jouet ~]# grep ffffffff993e3230 /proc/kallsyms 
> ffffffff993e3230 t format_decode
>
> Trying to investigate why it doesn't...

I investigated this.

If this example is with kcore, then it's expected. Because, perf annotate does
not inspect kallsyms when it can't find symbol name from disassembly itself.

For example, disassembly of  finish_task_switch,

with kcore:

ffffffff810cf1b0:       mov    $0x1,%esi
ffffffff810cf1b5:       mov    $0x4,%edi
ffffffff810cf1ba:       callq  0xffffffff811aced0
ffffffff810cf1bf:       andb   $0xfb,0x4c4(%rbx)
ffffffff810cf1c6:       jmpq   0xffffffff810cf0e9
ffffffff810cf1cb:       mov    %rbx,%rsi
ffffffff810cf1ce:       mov    %r13,%rdi
ffffffff810cf1d1:       callq  0xffffffff811a7e70
ffffffff810cf1d6:       jmpq   0xffffffff810cf0e4

with debuginfo:

ffffffff810cf1b0:       mov    $0x1,%esi
ffffffff810cf1b5:       mov    $0x4,%edi
ffffffff810cf1ba:       callq  ffffffff811aced0 <___perf_sw_event>
ffffffff810cf1bf:       andb   $0xfb,0x4c4(%rbx)
ffffffff810cf1c6:       jmpq   ffffffff810cf0e9 <finish_task_switch+0x69>
ffffffff810cf1cb:       mov    %rbx,%rsi
ffffffff810cf1ce:       mov    %r13,%rdi
ffffffff810cf1d1:       callq  ffffffff811a7e70 <__perf_event_task_sched_in>
ffffffff810cf1d6:       jmpq   ffffffff810cf0e4 <finish_task_switch+0x64>

call__parse tries to find symbol from angle brackets which is not present
in case of kcore.

-Ravi


> - Arnaldo
>
>> 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 v6:
>>   - No change
>>
>>  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 ea07588..a05423b 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target
  2016-09-20 14:35     ` Ravi Bangoria
@ 2016-09-20 14:56       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-09-20 14:56 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, linuxppc-dev, peterz, mingo, alexander.shishkin,
	treeze.taeung, naveen.n.rao, markus, chris.ryder, pawel.moll,
	mhiramat, rmk+kernel, jolsa, mpe, hemant, namhyung

Em Tue, Sep 20, 2016 at 08:05:03PM +0530, Ravi Bangoria escreveu:
> On Monday 19 September 2016 09:14 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Aug 19, 2016 at 06:29:35PM +0530, Ravi Bangoria escreveu:
> >> 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
> > Ok, makes sense, but then now I have the -> and can't press enter to go
> > to that function, in fact for the case I'm using as a test, the
> > vsnprintf kernel function, I get:
> >
> >        │ 56:   test   %al,%al
> >        │     ↓ je     81
> >        │       lea    -0x38(%rbp),%rsi
> >        │       mov    %r15,%rdi
> >        │     → callq  0xffffffff993e3230 
> >
> > That 0xffffffff993e3230 should've been resolved to:
> >
> > [root@jouet ~]# grep ffffffff993e3230 /proc/kallsyms 
> > ffffffff993e3230 t format_decode
> >
> > Trying to investigate why it doesn't...
> 
> I investigated this.
> 
> If this example is with kcore, then it's expected. Because, perf annotate does
> not inspect kallsyms when it can't find symbol name from disassembly itself.
> 
> For example, disassembly of  finish_task_switch,

Yeah, that was the case, up to yesterday, please take a look at my
perf/core branch, specifically this patch:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?id=4de86ebeb433aa59764a13ce851cba08d747e0e1

I'll do that for other instructions as well, like with 'mov
$0xabcdef,%rax' that will try to resolve that address to a variable.

Will try to continue processing your (and others) patches first tho :-)

- Arnaldo
 
> with kcore:
> 
> ffffffff810cf1b0:       mov    $0x1,%esi
> ffffffff810cf1b5:       mov    $0x4,%edi
> ffffffff810cf1ba:       callq  0xffffffff811aced0
> ffffffff810cf1bf:       andb   $0xfb,0x4c4(%rbx)
> ffffffff810cf1c6:       jmpq   0xffffffff810cf0e9
> ffffffff810cf1cb:       mov    %rbx,%rsi
> ffffffff810cf1ce:       mov    %r13,%rdi
> ffffffff810cf1d1:       callq  0xffffffff811a7e70
> ffffffff810cf1d6:       jmpq   0xffffffff810cf0e4
> 
> with debuginfo:
 
> ffffffff810cf1b0:       mov    $0x1,%esi
> ffffffff810cf1b5:       mov    $0x4,%edi
> ffffffff810cf1ba:       callq  ffffffff811aced0 <___perf_sw_event>
> ffffffff810cf1bf:       andb   $0xfb,0x4c4(%rbx)
> ffffffff810cf1c6:       jmpq   ffffffff810cf0e9 <finish_task_switch+0x69>
> ffffffff810cf1cb:       mov    %rbx,%rsi
> ffffffff810cf1ce:       mov    %r13,%rdi
> ffffffff810cf1d1:       callq  ffffffff811a7e70 <__perf_event_task_sched_in>
> ffffffff810cf1d6:       jmpq   ffffffff810cf0e4 <finish_task_switch+0x64>
> 
> call__parse tries to find symbol from angle brackets which is not present
> in case of kcore.
> 
> -Ravi
> 
> 
> > - Arnaldo
> >
> >> 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 v6:
> >>   - No change
> >>
> >>  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 ea07588..a05423b 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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [tip:perf/core] perf annotate: Do not ignore call instruction with indirect target
  2016-08-19 12:59 ` [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target Ravi Bangoria
  2016-09-19 15:44   ` Arnaldo Carvalho de Melo
@ 2016-09-20 21:43   ` tip-bot for Ravi Bangoria
  1 sibling, 0 replies; 20+ messages in thread
From: tip-bot for Ravi Bangoria @ 2016-09-20 21:43 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: markus, jolsa, hpa, linux-kernel, rmk+kernel, ravi.bangoria,
	acme, pawel.moll, chris.ryder, tglx, naveen.n.rao,
	alexander.shishkin, mhiramat, namhyung, peterz, mingo, hemant,
	mpe, treeze.taeung

Commit-ID:  88a7fcf961a27fbd248e3c914fc9df8327f7cdbb
Gitweb:     http://git.kernel.org/tip/88a7fcf961a27fbd248e3c914fc9df8327f7cdbb
Author:     Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
AuthorDate: Fri, 19 Aug 2016 18:29:35 +0530
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 20 Sep 2016 12:28:29 -0300

perf annotate: Do not ignore call instruction with indirect target

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>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Chris Riyder <chris.ryder@arm.com>
Cc: Hemant Kumar <hemant@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Markus Trippelsdorf <markus@trippelsdorf.de>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Taeung Song <treeze.taeung@gmail.com>
Link: http://lkml.kernel.org/r/1471611578-11255-5-git-send-email-ravi.bangoria@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 7a80c73..60e915f 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -82,16 +82,12 @@ static int call__parse(struct ins_operands *ops)
 	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;
 }

^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2016-09-20 21:45 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-19 12:59 [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria
2016-08-19 12:59 ` [PATCH v6 1/7] perf: Define macro for normalized arch names Ravi Bangoria
2016-08-19 12:59 ` [PATCH v6 2/7] perf annotate: Add cross arch annotate support Ravi Bangoria
2016-08-22 23:01   ` Kim Phillips
2016-08-23  2:17     ` Namhyung Kim
2016-08-23 20:36       ` Kim Phillips
2016-08-26  6:21         ` Namhyung Kim
2016-08-26  7:26         ` Ravi Bangoria
2016-08-27  0:40           ` [PATCH] perf annotate: cross arch annotate support fixes for ARM Kim Phillips
2016-08-19 12:59 ` [PATCH v6 3/7] perf annotate: Add support for powerpc Ravi Bangoria
2016-09-19 15:26   ` Arnaldo Carvalho de Melo
2016-08-19 12:59 ` [PATCH v6 4/7] perf annotate: Do not ignore call instruction with indirect target Ravi Bangoria
2016-09-19 15:44   ` Arnaldo Carvalho de Melo
2016-09-20 14:35     ` Ravi Bangoria
2016-09-20 14:56       ` Arnaldo Carvalho de Melo
2016-09-20 21:43   ` [tip:perf/core] " tip-bot for Ravi Bangoria
2016-08-19 12:59 ` [PATCH v6 5/7] perf annotate: Show raw form for jump " Ravi Bangoria
2016-08-19 12:59 ` [PATCH v6 6/7] perf annotate: Support jump instruction with target as second operand Ravi Bangoria
2016-08-19 12:59 ` [PATCH v6 7/7] perf annotate: Fix jump target outside of function address range Ravi Bangoria
2016-09-07 15:39 ` [PATCH v6 0/7] perf: Cross arch annotate + few miscellaneous fixes Ravi Bangoria

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).