[PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[Namhyung Kim]

Currently the symbol structure is allocated with symbol_conf.priv_size
to carry sideband information like annotation, map browser on TUI and
sort-by-name tree node.  So retrieving these information from symbol
needs to care about the details of such placement.

However the annotation code just assumes that the symbol is placed after
the struct annotation.  But actually there's other info between them.
So accessing those struct will lead to an undefined behavior (usually a
crash) after they write their info to the same location.

To reproduce the problem, please follow the steps below:

  1. run perf report (TUI of course) with -v option
  2. open map browser (by pressing right arrow key for any entry)
  3. search any function (by pressing '/' key and input whatever..)
  4. return to the hist browser (by pressing 'q' or left arrow key)
  5. open annotation window for the same entry (by pressing 'a' key)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 0784a9420528..cadbdc90a5cb 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -116,11 +116,6 @@ struct annotation {
 	struct annotated_source *src;
 };
 
-struct sannotation {
-	struct annotation annotation;
-	struct symbol	  symbol;
-};
-
 static inline struct sym_hist *annotation__histogram(struct annotation *notes, int idx)
 {
 	return (((void *)&notes->src->histograms) +
@@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
 
 static inline struct annotation *symbol__annotation(struct symbol *sym)
 {
-	struct sannotation *a = container_of(sym, struct sannotation, symbol);
-	return &a->annotation;
+	return (void *)sym - symbol_conf.priv_size;
 }
 
 int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx);
-- 
2.2.1

[PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()

[Namhyung Kim]

When a dso contains multiple symbols which have same name, current
dso__find_symbol_by_name() only finds an one of them and there's no way
to get the all symbols without going through the rbtree.

So add the new link argument to dso__find_symbol_by_name() to remember
current symbol position and returns next symbol if it provided.  For the
first call the link should be NULL and the returned symbol should be
used as link to the next call.  It will return NULL if there's no symbol
that has given name anymore.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/map.c |  2 +-
 tools/perf/util/map.c        |  7 ++++---
 tools/perf/util/map.h        |  3 ++-
 tools/perf/util/symbol.c     | 41 ++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/symbol.h     |  2 +-
 5 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/map.c b/tools/perf/ui/browsers/map.c
index b11639f33682..577c1c249abc 100644
--- a/tools/perf/ui/browsers/map.c
+++ b/tools/perf/ui/browsers/map.c
@@ -55,7 +55,7 @@ static int map_browser__search(struct map_browser *browser)
 		u64 addr = strtoull(target, NULL, 16);
 		sym = map__find_symbol(browser->map, addr, NULL);
 	} else
-		sym = map__find_symbol_by_name(browser->map, target, NULL);
+		sym = map__find_symbol_by_name(browser->map, target, NULL, NULL);
 
 	if (sym != NULL) {
 		u32 *idx = symbol__browser_index(sym);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 62ca9f2607d5..8c49aa69f6de 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -302,7 +302,8 @@ struct symbol *map__find_symbol(struct map *map, u64 addr,
 }
 
 struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
-					symbol_filter_t filter)
+					symbol_filter_t filter,
+					struct symbol *link)
 {
 	if (map__load(map, filter) < 0)
 		return NULL;
@@ -310,7 +311,7 @@ struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
 	if (!dso__sorted_by_name(map->dso, map->type))
 		dso__sort_by_name(map->dso, map->type);
 
-	return dso__find_symbol_by_name(map->dso, map->type, name);
+	return dso__find_symbol_by_name(map->dso, map->type, name, link);
 }
 
 struct map *map__clone(struct map *map)
@@ -542,7 +543,7 @@ struct symbol *map_groups__find_symbol_by_name(struct map_groups *mg,
 
 	for (nd = rb_first(&mg->maps[type]); nd; nd = rb_next(nd)) {
 		struct map *pos = rb_entry(nd, struct map, rb_node);
-		struct symbol *sym = map__find_symbol_by_name(pos, name, filter);
+		struct symbol *sym = map__find_symbol_by_name(pos, name, filter, NULL);
 
 		if (sym == NULL)
 			continue;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 6951a9d42339..1e8e5a4dd080 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -137,7 +137,8 @@ int map__load(struct map *map, symbol_filter_t filter);
 struct symbol *map__find_symbol(struct map *map,
 				u64 addr, symbol_filter_t filter);
 struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
-					symbol_filter_t filter);
+					symbol_filter_t filter,
+					struct symbol *link);
 void map__fixup_start(struct map *map);
 void map__fixup_end(struct map *map);
 
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index c24c5b83156c..2d546f7ce84e 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -396,6 +396,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
+	struct symbol_name_rb_node *s;
 
 	if (symbols == NULL)
 		return NULL;
@@ -403,7 +404,6 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 	n = symbols->rb_node;
 
 	while (n) {
-		struct symbol_name_rb_node *s;
 		int cmp;
 
 		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
@@ -414,10 +414,24 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 		else if (cmp > 0)
 			n = n->rb_right;
 		else
-			return &s->sym;
+			break;
 	}
 
-	return NULL;
+	if (n == NULL)
+		return NULL;
+
+	/* return first symbol that has same name (if any) */
+	for (n = rb_prev(n); n; n = rb_prev(n)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
+		if (strcmp(tmp->sym.name, s->sym.name))
+			break;
+
+		s = tmp;
+	}
+
+	return &s->sym;
 }
 
 struct symbol *dso__find_symbol(struct dso *dso,
@@ -436,10 +450,27 @@ struct symbol *dso__next_symbol(struct symbol *sym)
 	return symbols__next(sym);
 }
 
+/*
+ * When @link is NULL, returns first symbol that matched with @name.
+ * Otherwise, returns next symbol after @link in case some (local) symbols
+ * have same name, or NULL.
+ */
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
-					const char *name)
+					const char *name, struct symbol *link)
 {
-	return symbols__find_by_name(&dso->symbol_names[type], name);
+	struct rb_node *n;
+	struct symbol_name_rb_node *s;
+
+	if (link == NULL)
+		return symbols__find_by_name(&dso->symbol_names[type], name);
+
+	s = container_of(link, struct symbol_name_rb_node, sym);
+	n = rb_prev(&s->rb_node);
+	if (n == NULL)
+		return NULL;
+
+	s = rb_entry(n, struct symbol_name_rb_node, rb_node);
+	return strcmp(s->sym.name, name) ? NULL : &s->sym;
 }
 
 void dso__sort_by_name(struct dso *dso, enum map_type type)
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 9d602e9c6f59..895de3587b61 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -230,7 +230,7 @@ int dso__load_kallsyms(struct dso *dso, const char *filename, struct map *map,
 struct symbol *dso__find_symbol(struct dso *dso, enum map_type type,
 				u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
-					const char *name);
+					const char *name, struct symbol *link);
 
 struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
 struct symbol *dso__next_symbol(struct symbol *sym);
-- 
2.2.1

Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()

[David Ahern]

On 1/14/15 4:18 AM, Namhyung Kim wrote:
> @@ -414,10 +414,24 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>   		else if (cmp > 0)
>   			n = n->rb_right;
>   		else
> -			return &s->sym;
> +			break;
>   	}
>
> -	return NULL;
> +	if (n == NULL)
> +		return NULL;
> +
> +	/* return first symbol that has same name (if any) */
> +	for (n = rb_prev(n); n; n = rb_prev(n)) {
> +		struct symbol_name_rb_node *tmp;
> +
> +		tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> +		if (strcmp(tmp->sym.name, s->sym.name))

strcmp() == 0?

> +			break;
> +
> +		s = tmp;
> +	}
> +
> +	return &s->sym;
>   }
>
>   struct symbol *dso__find_symbol(struct dso *dso,

Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()

[Namhyung Kim]

On Wed, Jan 14, 2015 at 09:36:34AM -0700, David Ahern wrote:
> On 1/14/15 4:18 AM, Namhyung Kim wrote:
> >@@ -414,10 +414,24 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> >  		else if (cmp > 0)
> >  			n = n->rb_right;
> >  		else
> >-			return &s->sym;
> >+			break;
> >  	}
> >
> >-	return NULL;
> >+	if (n == NULL)
> >+		return NULL;
> >+
> >+	/* return first symbol that has same name (if any) */
> >+	for (n = rb_prev(n); n; n = rb_prev(n)) {
> >+		struct symbol_name_rb_node *tmp;
> >+
> >+		tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> >+		if (strcmp(tmp->sym.name, s->sym.name))
> 
> strcmp() == 0?

No, at this point the 's' points to the first symbol that has same
name.  And if it finds another symbol (tmp) that has same name, it
updates the s to point to the tmp and continues.  Otherwise it returns
with the existing symbol (s).

Thanks,
Namhyung


> 
> >+			break;
> >+
> >+		s = tmp;
> >+	}
> >+
> >+	return &s->sym;
> >  }
> >
> >  struct symbol *dso__find_symbol(struct dso *dso,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()

[David Ahern]

On 1/14/15 5:23 PM, Namhyung Kim wrote:
> On Wed, Jan 14, 2015 at 09:36:34AM -0700, David Ahern wrote:
>>> +	/* return first symbol that has same name (if any) */
>>> +	for (n = rb_prev(n); n; n = rb_prev(n)) {
>>> +		struct symbol_name_rb_node *tmp;
>>> +
>>> +		tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
>>> +		if (strcmp(tmp->sym.name, s->sym.name))
>>
>> strcmp() == 0?
>
> No, at this point the 's' points to the first symbol that has same
> name.  And if it finds another symbol (tmp) that has same name, it
> updates the s to point to the tmp and continues.  Otherwise it returns
> with the existing symbol (s).

I squinted a bit harder at your v3 and realized that. Was not obvious 
based on the first reading of the loop.

David

[tip:perf/core] perf symbols: Return the first entry with a given name in find_by_name method

[tip-bot for Namhyung Kim]

Commit-ID:  de4809999dac44c51e1f9ad892deb0336296dc4e
Gitweb:     http://git.kernel.org/tip/de4809999dac44c51e1f9ad892deb0336296dc4e
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Fri, 16 Jan 2015 15:31:28 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 21 Jan 2015 10:05:44 -0300

perf symbols: Return the first entry with a given name in find_by_name method

When a dso contains multiple symbols which have same name, current
dso__find_symbol_by_name() only finds an one of them and there's no way
to get the all symbols without going through the rbtree.

So make symbols__find_by_name() return the first entry with the given
name and the next patch in this series will provide a way to iterate
from there, by the name ordered rb_tree, till a suitable symbol is
found.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Link: http://lkml.kernel.org/r/1421234288-22758-2-git-send-email-namhyung@kernel.org
[ Yanked this independent hunk, without changes, from a larger patch  ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index c24c5b8..3cb928e 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -396,6 +396,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
+	struct symbol_name_rb_node *s;
 
 	if (symbols == NULL)
 		return NULL;
@@ -403,7 +404,6 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 	n = symbols->rb_node;
 
 	while (n) {
-		struct symbol_name_rb_node *s;
 		int cmp;
 
 		s = rb_entry(n, struct symbol_name_rb_node, rb_node);
@@ -414,10 +414,24 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 		else if (cmp > 0)
 			n = n->rb_right;
 		else
-			return &s->sym;
+			break;
 	}
 
-	return NULL;
+	if (n == NULL)
+		return NULL;
+
+	/* return first symbol that has same name (if any) */
+	for (n = rb_prev(n); n; n = rb_prev(n)) {
+		struct symbol_name_rb_node *tmp;
+
+		tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
+		if (strcmp(tmp->sym.name, s->sym.name))
+			break;
+
+		s = tmp;
+	}
+
+	return &s->sym;
 }
 
 struct symbol *dso__find_symbol(struct dso *dso,

[PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols

[Namhyung Kim]

The find_probe_trace_events_from_map() searches matching symbol from a
map (so from a backing dso).  For uprobes, it'll create a new map (and
dso) and loads it using a filter.  It's a little bit inefficient in that
it'll read out the symbol table everytime but works well anyway.

For kprobes however, it'll reuse existing kernel map which might be
loaded before.  In this case map__load() just returns with no result.
It makes kprobes always failed to find symbol even if it exists in the
map (dso).

To fix it, use map__find_symbol_by_name() instead.  It'll load a map
with full symbols and sorts them by name.  It needs to search sibing
nodes since there can be multiple (local) symbols with same name.

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 94a717bf007d..b1406d79b271 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2193,18 +2193,18 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	return ret;
 }
 
-static char *looking_function_name;
-static int num_matched_functions;
-
-static int probe_function_filter(struct map *map __maybe_unused,
-				      struct symbol *sym)
+static int find_probe_functions(struct map *map, char *name)
 {
-	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
-	    strcmp(looking_function_name, sym->name) == 0) {
-		num_matched_functions++;
-		return 0;
+	struct symbol *sym, *link = NULL;
+	int found = 0;
+
+	while ((sym = map__find_symbol_by_name(map, name, NULL, link)) != NULL) {
+		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
+			found++;
+		link = sym;
 	}
-	return 1;
+
+	return found;
 }
 
 #define strdup_or_goto(str, label)	\
@@ -2221,11 +2221,11 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct map *map = NULL;
 	struct kmap *kmap = NULL;
 	struct ref_reloc_sym *reloc_sym = NULL;
-	struct symbol *sym;
-	struct rb_node *nd;
+	struct symbol *sym, *link = NULL;
 	struct probe_trace_event *tev;
 	struct perf_probe_point *pp = &pev->point;
 	struct probe_trace_point *tp;
+	int num_matched_functions;
 	int ret, i;
 
 	/* Init maps of given executable or kernel */
@@ -2242,10 +2242,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	 * Load matched symbols: Since the different local symbols may have
 	 * same name but different addresses, this lists all the symbols.
 	 */
-	num_matched_functions = 0;
-	looking_function_name = pp->function;
-	ret = map__load(map, probe_function_filter);
-	if (ret || num_matched_functions == 0) {
+	num_matched_functions = find_probe_functions(map, pp->function);
+	if (num_matched_functions == 0) {
 		pr_err("Failed to find symbol %s in %s\n", pp->function,
 			target ? : "kernel");
 		ret = -ENOENT;
@@ -2275,7 +2273,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 	ret = 0;
-	map__for_each_symbol(map, sym, nd) {
+	while ((sym = map__find_symbol_by_name(map, pp->function, NULL, link))) {
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
-- 
2.2.1

Re: [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols

[Arnaldo Carvalho de Melo]

Em Wed, Jan 14, 2015 at 08:18:07PM +0900, Namhyung Kim escreveu:
> The find_probe_trace_events_from_map() searches matching symbol from a
> map (so from a backing dso).  For uprobes, it'll create a new map (and
> dso) and loads it using a filter.  It's a little bit inefficient in that
> it'll read out the symbol table everytime but works well anyway.
> 
> For kprobes however, it'll reuse existing kernel map which might be
> loaded before.  In this case map__load() just returns with no result.
> It makes kprobes always failed to find symbol even if it exists in the
> map (dso).
> 
> To fix it, use map__find_symbol_by_name() instead.  It'll load a map
> with full symbols and sorts them by name.  It needs to search sibing
> nodes since there can be multiple (local) symbols with same name.
> 
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 94a717bf007d..b1406d79b271 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2193,18 +2193,18 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>  	return ret;
>  }
>  
> -static char *looking_function_name;
> -static int num_matched_functions;
> -
> -static int probe_function_filter(struct map *map __maybe_unused,
> -				      struct symbol *sym)
> +static int find_probe_functions(struct map *map, char *name)
>  {
> -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
> -	    strcmp(looking_function_name, sym->name) == 0) {
> -		num_matched_functions++;
> -		return 0;
> +	struct symbol *sym, *link = NULL;
> +	int found = 0;
> +
> +	while ((sym = map__find_symbol_by_name(map, name, NULL, link)) != NULL) {
> +		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
> +			found++;
> +		link = sym;
>  	}
> -	return 1;
> +
> +	return found;
>  }

How about using the "from" (that you called "link") to follow the list.h
style and provide: 

	map__find_symbol_by_name_from(map, sym, filter, from);

And use it to implement map__find_symbol_by_name():

	map__find_symbol_by_name(map, sym, filter)
	{
		return map__find_symbol_by_name_from(map, sym, filter, NULL);
	}

Your loop would then become:

	link = NULL;

	while ((sym = map__find_symbol_by_name_from(map, name, NULL, link)) != NULL) {
		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
			found++;
		link = sym;
	}

And no map__find_symbol_by_name() callsites would have to be touched.

- Arnaldo
  
>  #define strdup_or_goto(str, label)	\
> @@ -2221,11 +2221,11 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	struct map *map = NULL;
>  	struct kmap *kmap = NULL;
>  	struct ref_reloc_sym *reloc_sym = NULL;
> -	struct symbol *sym;
> -	struct rb_node *nd;
> +	struct symbol *sym, *link = NULL;
>  	struct probe_trace_event *tev;
>  	struct perf_probe_point *pp = &pev->point;
>  	struct probe_trace_point *tp;
> +	int num_matched_functions;
>  	int ret, i;
>  
>  	/* Init maps of given executable or kernel */
> @@ -2242,10 +2242,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	 * Load matched symbols: Since the different local symbols may have
>  	 * same name but different addresses, this lists all the symbols.
>  	 */
> -	num_matched_functions = 0;
> -	looking_function_name = pp->function;
> -	ret = map__load(map, probe_function_filter);
> -	if (ret || num_matched_functions == 0) {
> +	num_matched_functions = find_probe_functions(map, pp->function);
> +	if (num_matched_functions == 0) {
>  		pr_err("Failed to find symbol %s in %s\n", pp->function,
>  			target ? : "kernel");
>  		ret = -ENOENT;
> @@ -2275,7 +2273,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	}
>  
>  	ret = 0;
> -	map__for_each_symbol(map, sym, nd) {
> +	while ((sym = map__find_symbol_by_name(map, pp->function, NULL, link))) {
>  		tev = (*tevs) + ret;
>  		tp = &tev->point;
>  		if (ret == num_matched_functions) {
> -- 
> 2.2.1

Re: Re: [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols

[Masami Hiramatsu]

(2015/01/14 22:54), Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 14, 2015 at 08:18:07PM +0900, Namhyung Kim escreveu:
>> The find_probe_trace_events_from_map() searches matching symbol from a
>> map (so from a backing dso).  For uprobes, it'll create a new map (and
>> dso) and loads it using a filter.  It's a little bit inefficient in that
>> it'll read out the symbol table everytime but works well anyway.
>>
>> For kprobes however, it'll reuse existing kernel map which might be
>> loaded before.  In this case map__load() just returns with no result.
>> It makes kprobes always failed to find symbol even if it exists in the
>> map (dso).
>>
>> To fix it, use map__find_symbol_by_name() instead.  It'll load a map
>> with full symbols and sorts them by name.  It needs to search sibing
>> nodes since there can be multiple (local) symbols with same name.
>>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/util/probe-event.c | 32 +++++++++++++++-----------------
>>  1 file changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 94a717bf007d..b1406d79b271 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -2193,18 +2193,18 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>  	return ret;
>>  }
>>  
>> -static char *looking_function_name;
>> -static int num_matched_functions;
>> -
>> -static int probe_function_filter(struct map *map __maybe_unused,
>> -				      struct symbol *sym)
>> +static int find_probe_functions(struct map *map, char *name)
>>  {
>> -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
>> -	    strcmp(looking_function_name, sym->name) == 0) {
>> -		num_matched_functions++;
>> -		return 0;
>> +	struct symbol *sym, *link = NULL;
>> +	int found = 0;
>> +
>> +	while ((sym = map__find_symbol_by_name(map, name, NULL, link)) != NULL) {
>> +		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
>> +			found++;
>> +		link = sym;
>>  	}
>> -	return 1;
>> +
>> +	return found;
>>  }
> 
> How about using the "from" (that you called "link") to follow the list.h
> style and provide: 
> 
> 	map__find_symbol_by_name_from(map, sym, filter, from);
> 
> And use it to implement map__find_symbol_by_name():
> 
> 	map__find_symbol_by_name(map, sym, filter)
> 	{
> 		return map__find_symbol_by_name_from(map, sym, filter, NULL);
> 	}
> 
> Your loop would then become:
> 
> 	link = NULL;
> 
> 	while ((sym = map__find_symbol_by_name_from(map, name, NULL, link)) != NULL) {
> 		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
> 			found++;
> 		link = sym;
> 	}

+1 ;)

Would it be nicer to rename "link" local variable to "from" ?

> 
> And no map__find_symbol_by_name() callsites would have to be touched.

BTW, such other callsites has no similar issues?

Thank you,

> 
> - Arnaldo
>   
>>  #define strdup_or_goto(str, label)	\
>> @@ -2221,11 +2221,11 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  	struct map *map = NULL;
>>  	struct kmap *kmap = NULL;
>>  	struct ref_reloc_sym *reloc_sym = NULL;
>> -	struct symbol *sym;
>> -	struct rb_node *nd;
>> +	struct symbol *sym, *link = NULL;
>>  	struct probe_trace_event *tev;
>>  	struct perf_probe_point *pp = &pev->point;
>>  	struct probe_trace_point *tp;
>> +	int num_matched_functions;
>>  	int ret, i;
>>  
>>  	/* Init maps of given executable or kernel */
>> @@ -2242,10 +2242,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  	 * Load matched symbols: Since the different local symbols may have
>>  	 * same name but different addresses, this lists all the symbols.
>>  	 */
>> -	num_matched_functions = 0;
>> -	looking_function_name = pp->function;
>> -	ret = map__load(map, probe_function_filter);
>> -	if (ret || num_matched_functions == 0) {
>> +	num_matched_functions = find_probe_functions(map, pp->function);
>> +	if (num_matched_functions == 0) {
>>  		pr_err("Failed to find symbol %s in %s\n", pp->function,
>>  			target ? : "kernel");
>>  		ret = -ENOENT;
>> @@ -2275,7 +2273,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>>  	}
>>  
>>  	ret = 0;
>> -	map__for_each_symbol(map, sym, nd) {
>> +	while ((sym = map__find_symbol_by_name(map, pp->function, NULL, link))) {
>>  		tev = (*tevs) + ret;
>>  		tp = &tev->point;
>>  		if (ret == num_matched_functions) {
>> -- 
>> 2.2.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[tip:perf/core] perf probe: Do not rely on map__load() filter to find symbols

[tip-bot for Namhyung Kim]

Commit-ID:  564c62a4d7f9a4b60920ee0b02cfe890471d87f4
Gitweb:     http://git.kernel.org/tip/564c62a4d7f9a4b60920ee0b02cfe890471d87f4
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 14 Jan 2015 20:18:07 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 21 Jan 2015 10:06:02 -0300

perf probe: Do not rely on map__load() filter to find symbols

The find_probe_trace_events_from_map() searches matching symbol from a
map (so from a backing dso).  For uprobes, it'll create a new map (and
dso) and loads it using a filter.  It's a little bit inefficient in that
it'll read out the symbol table everytime but works well anyway.

For kprobes however, it'll reuse existing kernel map which might be
loaded before.  In this case map__load() just returns with no result.
It makes kprobes always failed to find symbol even if it exists in the
map (dso).

To fix it, use map__find_symbol_by_name() instead.  It'll load a map
with full symbols and sorts them by name.  It needs to search sibing
nodes since there can be multiple (local) symbols with same name.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Link: http://lkml.kernel.org/r/1421234288-22758-3-git-send-email-namhyung@kernel.org
[ Use symbol__next_by_name ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 94a717b..b24482e 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2193,18 +2193,20 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 	return ret;
 }
 
-static char *looking_function_name;
-static int num_matched_functions;
-
-static int probe_function_filter(struct map *map __maybe_unused,
-				      struct symbol *sym)
+static int find_probe_functions(struct map *map, char *name)
 {
-	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
-	    strcmp(looking_function_name, sym->name) == 0) {
-		num_matched_functions++;
-		return 0;
+	int found = 0;
+	struct symbol *sym = map__find_symbol_by_name(map, name, NULL);
+
+	while (sym != NULL) {
+		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
+			found++;
+		sym = symbol__next_by_name(sym);
+		if (sym == NULL || strcmp(sym->name, name))
+			break;
 	}
-	return 1;
+
+	return found;
 }
 
 #define strdup_or_goto(str, label)	\
@@ -2222,10 +2224,10 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct kmap *kmap = NULL;
 	struct ref_reloc_sym *reloc_sym = NULL;
 	struct symbol *sym;
-	struct rb_node *nd;
 	struct probe_trace_event *tev;
 	struct perf_probe_point *pp = &pev->point;
 	struct probe_trace_point *tp;
+	int num_matched_functions;
 	int ret, i;
 
 	/* Init maps of given executable or kernel */
@@ -2242,10 +2244,8 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	 * Load matched symbols: Since the different local symbols may have
 	 * same name but different addresses, this lists all the symbols.
 	 */
-	num_matched_functions = 0;
-	looking_function_name = pp->function;
-	ret = map__load(map, probe_function_filter);
-	if (ret || num_matched_functions == 0) {
+	num_matched_functions = find_probe_functions(map, pp->function);
+	if (num_matched_functions == 0) {
 		pr_err("Failed to find symbol %s in %s\n", pp->function,
 			target ? : "kernel");
 		ret = -ENOENT;
@@ -2275,7 +2275,9 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	}
 
 	ret = 0;
-	map__for_each_symbol(map, sym, nd) {
+	sym = map__find_symbol_by_name(map, pp->function, NULL);
+
+	while (sym != NULL) {
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
@@ -2323,6 +2325,10 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 					strdup_or_goto(pev->args[i].type,
 							nomem_out);
 		}
+
+		sym = symbol__next_by_name(sym);
+		if (sym == NULL || strcmp(sym->name, pp->function))
+			break;
 	}
 
 out:

[PATCH v2 4/4] perf probe: Fix probing kretprobes

[Namhyung Kim]

The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
instead of the symbol name") converts kprobes to use ref_reloc_sym
(i.e. _stext) and offset instead of using symbol's name directly.  So
on my system, adding do_fork ends up with like below:

  $ sudo perf probe -v --add do_fork%return
  probe-definition(0): do_fork%return
  symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (7 entries long)
  Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
  Could not open debuginfo. Try to use symbols.
  Opening /sys/kernel/debug/tracing/kprobe_events write=1
  Added new event:
  Writing event: r:probe/do_fork _stext+456136
  Failed to write event: Invalid argument
  Error: Failed to add events. Reason: Operation not permitted (Code: -1)

As you can see, the do_fork was translated to _stext+456136.  This was
because to support (local) symbols that have same name.  But the
problem is that kretprobe requires to be inserted at function start
point so it simply checks whether it's called with offset 0.  And if
not, it'll return with -EINVAL.  You can see it with dmesg.

  $ dmesg | tail -1
    [125621.764103] Return probe must be used without offset.

So we need to use the symbol name instead of ref_reloc_sym in case of
return probes.

Reported-by: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/probe-event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b1406d79b271..7c94acc014e9 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -446,7 +446,7 @@ static int post_process_probe_trace_events(struct probe_trace_event *tevs,
 	}
 
 	for (i = 0; i < ntevs; i++) {
-		if (tevs[i].point.address) {
+		if (tevs[i].point.address && !tevs[i].point.retprobe) {
 			tmp = strdup(reloc_sym->name);
 			if (!tmp)
 				return -ENOMEM;
@@ -2255,7 +2255,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		goto out;
 	}
 
-	if (!pev->uprobes) {
+	if (!pev->uprobes && !pp->retprobe) {
 		kmap = map__kmap(map);
 		reloc_sym = kmap->ref_reloc_sym;
 		if (!reloc_sym) {
-- 
2.2.1

Re: [PATCH v2 4/4] perf probe: Fix probing kretprobes

[Jiri Olsa]

On Wed, Jan 14, 2015 at 08:18:08PM +0900, Namhyung Kim wrote:
> The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
> instead of the symbol name") converts kprobes to use ref_reloc_sym
> (i.e. _stext) and offset instead of using symbol's name directly.  So
> on my system, adding do_fork ends up with like below:
> 
>   $ sudo perf probe -v --add do_fork%return
>   probe-definition(0): do_fork%return
>   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (7 entries long)
>   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
>   Could not open debuginfo. Try to use symbols.
>   Opening /sys/kernel/debug/tracing/kprobe_events write=1
>   Added new event:
>   Writing event: r:probe/do_fork _stext+456136
>   Failed to write event: Invalid argument
>   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
> 
> As you can see, the do_fork was translated to _stext+456136.  This was
> because to support (local) symbols that have same name.  But the
> problem is that kretprobe requires to be inserted at function start
> point so it simply checks whether it's called with offset 0.  And if
> not, it'll return with -EINVAL.  You can see it with dmesg.
> 
>   $ dmesg | tail -1
>     [125621.764103] Return probe must be used without offset.
> 
> So we need to use the symbol name instead of ref_reloc_sym in case of
> return probes.
> 
> Reported-by: Jiri Olsa <jolsa@redhat.com>

'perf probe --add do_fork%return' inserts the probe correctly now

Tested-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: [PATCH v2 4/4] perf probe: Fix probing kretprobes

[Masami Hiramatsu]

(2015/01/14 20:18), Namhyung Kim wrote:
> The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
> instead of the symbol name") converts kprobes to use ref_reloc_sym
> (i.e. _stext) and offset instead of using symbol's name directly.  So
> on my system, adding do_fork ends up with like below:
> 
>   $ sudo perf probe -v --add do_fork%return
>   probe-definition(0): do_fork%return
>   symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
>   0 arguments
>   Looking at the vmlinux_path (7 entries long)
>   Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
>   Could not open debuginfo. Try to use symbols.
>   Opening /sys/kernel/debug/tracing/kprobe_events write=1
>   Added new event:
>   Writing event: r:probe/do_fork _stext+456136
>   Failed to write event: Invalid argument
>   Error: Failed to add events. Reason: Operation not permitted (Code: -1)
> 
> As you can see, the do_fork was translated to _stext+456136.  This was
> because to support (local) symbols that have same name.  But the
> problem is that kretprobe requires to be inserted at function start
> point so it simply checks whether it's called with offset 0.  And if
> not, it'll return with -EINVAL.  You can see it with dmesg.
> 
>   $ dmesg | tail -1
>     [125621.764103] Return probe must be used without offset.
> 
> So we need to use the symbol name instead of ref_reloc_sym in case of
> return probes.
> 
> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Looks good to me!

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you,

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index b1406d79b271..7c94acc014e9 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -446,7 +446,7 @@ static int post_process_probe_trace_events(struct probe_trace_event *tevs,
>  	}
>  
>  	for (i = 0; i < ntevs; i++) {
> -		if (tevs[i].point.address) {
> +		if (tevs[i].point.address && !tevs[i].point.retprobe) {
>  			tmp = strdup(reloc_sym->name);
>  			if (!tmp)
>  				return -ENOMEM;
> @@ -2255,7 +2255,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  		goto out;
>  	}
>  
> -	if (!pev->uprobes) {
> +	if (!pev->uprobes && !pp->retprobe) {
>  		kmap = map__kmap(map);
>  		reloc_sym = kmap->ref_reloc_sym;
>  		if (!reloc_sym) {
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[tip:perf/core] perf probe: Fix probing kretprobes

[tip-bot for Namhyung Kim]

Commit-ID:  25dd9171f51c482eb7c4dc8618766ae733756e2d
Gitweb:     http://git.kernel.org/tip/25dd9171f51c482eb7c4dc8618766ae733756e2d
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 14 Jan 2015 20:18:08 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 21 Jan 2015 10:06:24 -0300

perf probe: Fix probing kretprobes

The commit dfef99cd0b2c ("perf probe: Use ref_reloc_sym based address
instead of the symbol name") converts kprobes to use ref_reloc_sym (i.e.
_stext) and offset instead of using symbol's name directly.  So on my
system, adding do_fork ends up with like below:

  $ sudo perf probe -v --add do_fork%return
  probe-definition(0): do_fork%return
  symbol:do_fork file:(null) line:0 offset:0 return:1 lazy:(null)
  0 arguments
  Looking at the vmlinux_path (7 entries long)
  Using /lib/modules/3.17.6-1-ARCH/build/vmlinux for symbols
  Could not open debuginfo. Try to use symbols.
  Opening /sys/kernel/debug/tracing/kprobe_events write=1
  Added new event:
  Writing event: r:probe/do_fork _stext+456136
  Failed to write event: Invalid argument
  Error: Failed to add events. Reason: Operation not permitted (Code: -1)

As you can see, the do_fork was translated to _stext+456136.  This was
because to support (local) symbols that have same name.  But the problem
is that kretprobe requires to be inserted at function start point so it
simply checks whether it's called with offset 0.  And if not, it'll
return with -EINVAL.  You can see it with dmesg.

  $ dmesg | tail -1
    [125621.764103] Return probe must be used without offset.

So we need to use the symbol name instead of ref_reloc_sym in case of
return probes.

Reported-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1421234288-22758-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 7cc89b1..919937e 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -446,7 +446,7 @@ static int post_process_probe_trace_events(struct probe_trace_event *tevs,
 	}
 
 	for (i = 0; i < ntevs; i++) {
-		if (tevs[i].point.address) {
+		if (tevs[i].point.address && !tevs[i].point.retprobe) {
 			tmp = strdup(reloc_sym->name);
 			if (!tmp)
 				return -ENOMEM;
@@ -2254,7 +2254,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		goto out;
 	}
 
-	if (!pev->uprobes) {
+	if (!pev->uprobes && !pp->retprobe) {
 		kmap = map__kmap(map);
 		reloc_sym = kmap->ref_reloc_sym;
 		if (!reloc_sym) {

Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[Arnaldo Carvalho de Melo]

Em Wed, Jan 14, 2015 at 08:18:05PM +0900, Namhyung Kim escreveu:
> Currently the symbol structure is allocated with symbol_conf.priv_size
> to carry sideband information like annotation, map browser on TUI and
> sort-by-name tree node.  So retrieving these information from symbol
> needs to care about the details of such placement.
> 
> However the annotation code just assumes that the symbol is placed after
> the struct annotation.  But actually there's other info between them.
> So accessing those struct will lead to an undefined behavior (usually a
> crash) after they write their info to the same location.
> 
> To reproduce the problem, please follow the steps below:
> 
>   1. run perf report (TUI of course) with -v option
>   2. open map browser (by pressing right arrow key for any entry)
>   3. search any function (by pressing '/' key and input whatever..)
>   4. return to the hist browser (by pressing 'q' or left arrow key)
>   5. open annotation window for the same entry (by pressing 'a' key)

Thanks, nice fix, description and reproduction steps, applied to perf/urgent.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate.h | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 0784a9420528..cadbdc90a5cb 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -116,11 +116,6 @@ struct annotation {
>  	struct annotated_source *src;
>  };
>  
> -struct sannotation {
> -	struct annotation annotation;
> -	struct symbol	  symbol;
> -};
> -
>  static inline struct sym_hist *annotation__histogram(struct annotation *notes, int idx)
>  {
>  	return (((void *)&notes->src->histograms) +
> @@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
>  
>  static inline struct annotation *symbol__annotation(struct symbol *sym)
>  {
> -	struct sannotation *a = container_of(sym, struct sannotation, symbol);
> -	return &a->annotation;
> +	return (void *)sym - symbol_conf.priv_size;
>  }
>  
>  int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx);
> -- 
> 2.2.1

Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[David Ahern]

On 1/14/15 4:18 AM, Namhyung Kim wrote:

> @@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
>
>   static inline struct annotation *symbol__annotation(struct symbol *sym)
>   {
> -	struct sannotation *a = container_of(sym, struct sannotation, symbol);
> -	return &a->annotation;
> +	return (void *)sym - symbol_conf.priv_size;

symbol__priv(sym);

David

Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[Arnaldo Carvalho de Melo]

Em Wed, Jan 14, 2015 at 07:57:49AM -0700, David Ahern escreveu:
> On 1/14/15 4:18 AM, Namhyung Kim wrote:
> 
> >@@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
> >
> >  static inline struct annotation *symbol__annotation(struct symbol *sym)
> >  {
> >-	struct sannotation *a = container_of(sym, struct sannotation, symbol);
> >-	return &a->annotation;
> >+	return (void *)sym - symbol_conf.priv_size;
> 
> symbol__priv(sym);

Amended, thanks.

- Arnaldo

Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[Namhyung Kim]

Hi Arnaldo and David,

On Wed, Jan 14, 2015 at 06:08:52PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 14, 2015 at 07:57:49AM -0700, David Ahern escreveu:
> > On 1/14/15 4:18 AM, Namhyung Kim wrote:
> > 
> > >@@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
> > >
> > >  static inline struct annotation *symbol__annotation(struct symbol *sym)
> > >  {
> > >-	struct sannotation *a = container_of(sym, struct sannotation, symbol);
> > >-	return &a->annotation;
> > >+	return (void *)sym - symbol_conf.priv_size;
> > 
> > symbol__priv(sym);
> 
> Amended, thanks.

Oh, I missed that.  Thanks to both of you!
Namhyung

Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI

[Arnaldo Carvalho de Melo]

Em Thu, Jan 15, 2015 at 09:20:21AM +0900, Namhyung Kim escreveu:
> On Wed, Jan 14, 2015 at 06:08:52PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 14, 2015 at 07:57:49AM -0700, David Ahern escreveu:
> > > On 1/14/15 4:18 AM, Namhyung Kim wrote:
> > > >  static inline struct annotation *symbol__annotation(struct symbol *sym)
> > > >  {
> > > >-	struct sannotation *a = container_of(sym, struct sannotation, symbol);
> > > >-	return &a->annotation;
> > > >+	return (void *)sym - symbol_conf.priv_size;
 
> > > symbol__priv(sym);

> > Amended, thanks.
 
> Oh, I missed that.  Thanks to both of you!

I thought I had, probably did, but in just one of my devel machines,
ended up going as Namhyung did, no biggie, I'll fix it up later, on the
next perf/core devel cycle.

- Arnaldo

[tip:perf/urgent] perf tools: Fix segfault for symbol annotation on TUI

[tip-bot for Namhyung Kim]

Commit-ID:  813ccd15452ed34e97aa526ffc70d6d8e6c466c5
Gitweb:     http://git.kernel.org/tip/813ccd15452ed34e97aa526ffc70d6d8e6c466c5
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Wed, 14 Jan 2015 20:18:05 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 16 Jan 2015 17:49:29 -0300

perf tools: Fix segfault for symbol annotation on TUI

Currently the symbol structure is allocated with symbol_conf.priv_size
to carry sideband information like annotation, map browser on TUI and
sort-by-name tree node.  So retrieving these information from symbol
needs to care about the details of such placement.

However the annotation code just assumes that the symbol is placed after
the struct annotation.  But actually there's other info between them.
So accessing those struct will lead to an undefined behavior (usually a
crash) after they write their info to the same location.

To reproduce the problem, please follow the steps below:

  1. run perf report (TUI of course) with -v option
  2. open map browser (by pressing right arrow key for any entry)
  3. search any function (by pressing '/' key and input whatever..)
  4. return to the hist browser (by pressing 'q' or left arrow key)
  5. open annotation window for the same entry (by pressing 'a' key)

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1421234288-22758-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/annotate.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 0784a94..cadbdc9 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -116,11 +116,6 @@ struct annotation {
 	struct annotated_source *src;
 };
 
-struct sannotation {
-	struct annotation annotation;
-	struct symbol	  symbol;
-};
-
 static inline struct sym_hist *annotation__histogram(struct annotation *notes, int idx)
 {
 	return (((void *)&notes->src->histograms) +
@@ -129,8 +124,7 @@ static inline struct sym_hist *annotation__histogram(struct annotation *notes, i
 
 static inline struct annotation *symbol__annotation(struct symbol *sym)
 {
-	struct sannotation *a = container_of(sym, struct sannotation, symbol);
-	return &a->annotation;
+	return (void *)sym - symbol_conf.priv_size;
 }
 
 int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx);