Re: [RFC 3/6] perf annotate: Enable cross arch annotate

2016-06-28 Thread Ravi Bangoria



On Monday 27 June 2016 10:46 PM, Arnaldo Carvalho de Melo wrote:

Em Fri, Jun 24, 2016 at 05:23:57PM +0530, Ravi Bangoria escreveu:

Change current data structures and function to enable cross arch annotate
and add support for x86 and arm instructions.

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


...

  
+static struct arch_instructions {

+   const char *arch;
+   intnmemb;
+   struct ins *instructions;
+   struct ins *(*ins__find)(const char *);

Why do we need arch specific find functions? Why not pass the
instructions pointer to it, just like you did with ins__sort().

Probably it is not needed to be global, you just pick the right
instructions table + its ARRAY_SIZE and pass it around, again, like you
did in ins__sort().

- Arnaldo


Thanks Arnaldo for suggestion.

To determine arch in ins__find, I need to pass 'arch' till ins__find and 
which

requires changes in definition of many functions. So, I thought about global
var.

Anyway, I've prepared a patch as you suggested and sent it as a [PATCH].
Please review it.

-Ravi



Re: [RFC 3/6] perf annotate: Enable cross arch annotate

2016-06-27 Thread Arnaldo Carvalho de Melo
Em Fri, Jun 24, 2016 at 05:23:57PM +0530, Ravi Bangoria escreveu:
> Change current data structures and function to enable cross arch annotate
> and add support for x86 and arm instructions.
> 
> Current implementation does not contain logic of recording on one arch
> and annotating on other. This remote annotate is partially possible with
> current implementation for x86 (or may be arm as well) only. But, to make
> remote annotation work properly, all architecture instruction tables need
> to be included in the perf binary. And while annotating, look for
> instruction table where perf.data was recorded.
> 
> For arm, few instructions were defined under #if __arm__ which I've used
> as a table for arm. But I'm not sure whether instruction defined outside
> of that also contains arm instructions.
> 
> Note:
> Here arch_ins is global var. And init_arch_ins will be called every
> time when we annotate symbol. So I still need to optimize this.
> May be make arch_ins per session. Please suggest best way to do it.
> 
> Signed-off-by: Ravi Bangoria 
> ---
>  tools/perf/builtin-top.c  |   2 +-
>  tools/perf/ui/browsers/annotate.c |   5 +-
>  tools/perf/ui/gtk/annotate.c  |   6 +-
>  tools/perf/util/annotate.c| 116 
> +-
>  tools/perf/util/annotate.h|   3 +-
>  tools/perf/util/evsel.c   |   7 +++
>  tools/perf/util/evsel.h   |   2 +
>  7 files changed, 108 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 07fc792..d4fd947 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -128,7 +128,7 @@ static int perf_top__parse_source(struct perf_top *top, 
> struct hist_entry *he)
>   return err;
>   }
>  
> - err = symbol__annotate(sym, map, 0);
> + err = symbol__annotate(sym, map, 0, NULL);
>   if (err == 0) {
>  out_assign:
>   top->sym_filter_entry = he;
> diff --git a/tools/perf/ui/browsers/annotate.c 
> b/tools/perf/ui/browsers/annotate.c
> index 0e106bb..b65a979 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -1031,6 +1031,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map 
> *map,
>   int ret = -1;
>   int nr_pcnt = 1;
>   size_t sizeof_bdl = sizeof(struct browser_disasm_line);
> + char *target_arch = NULL;
>  
>   if (sym == NULL)
>   return -1;
> @@ -1052,7 +1053,9 @@ int symbol__tui_annotate(struct symbol *sym, struct map 
> *map,
> (nr_pcnt - 1);
>   }
>  
> - if (symbol__annotate(sym, map, sizeof_bdl) < 0) {
> + target_arch = perf_evsel__env_arch(evsel);
> +
> + if (symbol__annotate(sym, map, sizeof_bdl, target_arch) < 0) {
>   ui__error("%s", ui_helpline__last_msg);
>   goto out_free_offsets;
>   }
> diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
> index 9c7ff8d..e468c1a 100644
> --- a/tools/perf/ui/gtk/annotate.c
> +++ b/tools/perf/ui/gtk/annotate.c
> @@ -4,7 +4,6 @@
>  #include "util/evsel.h"
>  #include "ui/helpline.h"
>  
> -
>  enum {
>   ANN_COL__PERCENT,
>   ANN_COL__OFFSET,
> @@ -162,11 +161,14 @@ static int symbol__gtk_annotate(struct symbol *sym, 
> struct map *map,
>   GtkWidget *notebook;
>   GtkWidget *scrolled_window;
>   GtkWidget *tab_label;
> + char *target_arch = NULL;
>  
>   if (map->dso->annotate_warned)
>   return -1;
>  
> - if (symbol__annotate(sym, map, 0) < 0) {
> + target_arch = perf_evsel__env_arch(evsel);
> +
> + if (symbol__annotate(sym, map, 0, target_arch) < 0) {
>   ui__error("%s", ui_helpline__current);
>   return -1;
>   }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b2c7ae4..e0dc7b2 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -20,6 +20,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include "../arch/common.h"
>  
>  const char   *disassembler_style;
>  const char   *objdump_path;
> @@ -28,6 +30,13 @@ static regex_t  file_lineno;
>  static struct ins *ins__find(const char *name);
>  static int disasm_line__parse(char *line, char **namep, char **rawp);
>  
> +static struct arch_instructions {
> + const char *arch;
> + intnmemb;
> + struct ins *instructions;
> + struct ins *(*ins__find)(const char *);

Why do we need arch specific find functions? Why not pass the
instructions pointer to it, just like you did with ins__sort().

Probably it is not needed to be global, you just pick the right
instructions table + its ARRAY_SIZE and pass it around, again, like you
did in ins__sort().

- Arnaldo

> +} arch_ins;
> +
>  static void ins__delete(struct ins_operands *ops)
>  {
>   if (ops == NULL)
> @@ -183,7 +192,7 @@ static int lock__parse(struct ins_operands *ops)
>   if (disasm_line__parse(ops->raw, &name, &ops->loc

[RFC 3/6] perf annotate: Enable cross arch annotate

2016-06-24 Thread Ravi Bangoria
Change current data structures and function to enable cross arch annotate
and add support for x86 and arm instructions.

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

For arm, few instructions were defined under #if __arm__ which I've used
as a table for arm. But I'm not sure whether instruction defined outside
of that also contains arm instructions.

Note:
Here arch_ins is global var. And init_arch_ins will be called every
time when we annotate symbol. So I still need to optimize this.
May be make arch_ins per session. Please suggest best way to do it.

Signed-off-by: Ravi Bangoria 
---
 tools/perf/builtin-top.c  |   2 +-
 tools/perf/ui/browsers/annotate.c |   5 +-
 tools/perf/ui/gtk/annotate.c  |   6 +-
 tools/perf/util/annotate.c| 116 +-
 tools/perf/util/annotate.h|   3 +-
 tools/perf/util/evsel.c   |   7 +++
 tools/perf/util/evsel.h   |   2 +
 7 files changed, 108 insertions(+), 33 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 07fc792..d4fd947 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -128,7 +128,7 @@ static int perf_top__parse_source(struct perf_top *top, 
struct hist_entry *he)
return err;
}
 
-   err = symbol__annotate(sym, map, 0);
+   err = symbol__annotate(sym, map, 0, NULL);
if (err == 0) {
 out_assign:
top->sym_filter_entry = he;
diff --git a/tools/perf/ui/browsers/annotate.c 
b/tools/perf/ui/browsers/annotate.c
index 0e106bb..b65a979 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1031,6 +1031,7 @@ int symbol__tui_annotate(struct symbol *sym, struct map 
*map,
int ret = -1;
int nr_pcnt = 1;
size_t sizeof_bdl = sizeof(struct browser_disasm_line);
+   char *target_arch = NULL;
 
if (sym == NULL)
return -1;
@@ -1052,7 +1053,9 @@ int symbol__tui_annotate(struct symbol *sym, struct map 
*map,
  (nr_pcnt - 1);
}
 
-   if (symbol__annotate(sym, map, sizeof_bdl) < 0) {
+   target_arch = perf_evsel__env_arch(evsel);
+
+   if (symbol__annotate(sym, map, sizeof_bdl, target_arch) < 0) {
ui__error("%s", ui_helpline__last_msg);
goto out_free_offsets;
}
diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
index 9c7ff8d..e468c1a 100644
--- a/tools/perf/ui/gtk/annotate.c
+++ b/tools/perf/ui/gtk/annotate.c
@@ -4,7 +4,6 @@
 #include "util/evsel.h"
 #include "ui/helpline.h"
 
-
 enum {
ANN_COL__PERCENT,
ANN_COL__OFFSET,
@@ -162,11 +161,14 @@ static int symbol__gtk_annotate(struct symbol *sym, 
struct map *map,
GtkWidget *notebook;
GtkWidget *scrolled_window;
GtkWidget *tab_label;
+   char *target_arch = NULL;
 
if (map->dso->annotate_warned)
return -1;
 
-   if (symbol__annotate(sym, map, 0) < 0) {
+   target_arch = perf_evsel__env_arch(evsel);
+
+   if (symbol__annotate(sym, map, 0, target_arch) < 0) {
ui__error("%s", ui_helpline__current);
return -1;
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b2c7ae4..e0dc7b2 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -20,6 +20,8 @@
 #include 
 #include 
 #include 
+#include 
+#include "../arch/common.h"
 
 const char *disassembler_style;
 const char *objdump_path;
@@ -28,6 +30,13 @@ static regex_tfile_lineno;
 static struct ins *ins__find(const char *name);
 static int disasm_line__parse(char *line, char **namep, char **rawp);
 
+static struct arch_instructions {
+   const char *arch;
+   intnmemb;
+   struct ins *instructions;
+   struct ins *(*ins__find)(const char *);
+} arch_ins;
+
 static void ins__delete(struct ins_operands *ops)
 {
if (ops == NULL)
@@ -183,7 +192,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 = arch_ins.ins__find(name);
free(name);
 
if (ops->locked.ins == NULL)
@@ -354,26 +363,12 @@ static struct ins_ops nop_ops = {
.scnprintf = nop__scnprintf,
 };
 
-static struct ins instructions[] = {
+static struct ins instructions_x86[] = {
{ .name = "add",   .ops  = &mov_ops, },
{ .name = "addl",  .ops  = &mov_ops, },
{ .name = "addq",  .ops  = &mov_ops, },