LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
@ 2015-01-14 11:18 Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
                   ` (5 more replies)
  0 siblings, 6 replies; 20+ messages in thread
From: Namhyung Kim @ 2015-01-14 11:18 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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


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

* [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
@ 2015-01-14 11:18 ` Namhyung Kim
  2015-01-14 16:36   ` David Ahern
  2015-01-28 15:01   ` [tip:perf/core] perf symbols: Return the first entry with a given name in find_by_name method tip-bot for Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols Namhyung Kim
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 20+ messages in thread
From: Namhyung Kim @ 2015-01-14 11:18 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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


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

* [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
@ 2015-01-14 11:18 ` Namhyung Kim
  2015-01-14 13:54   ` Arnaldo Carvalho de Melo
  2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 20+ messages in thread
From: Namhyung Kim @ 2015-01-14 11:18 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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


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

* [PATCH v2 4/4] perf probe: Fix probing kretprobes
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
  2015-01-14 11:18 ` [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols Namhyung Kim
@ 2015-01-14 11:18 ` Namhyung Kim
  2015-01-14 16:26   ` Jiri Olsa
                     ` (2 more replies)
  2015-01-14 12:57 ` [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Arnaldo Carvalho de Melo
                   ` (2 subsequent siblings)
  5 siblings, 3 replies; 20+ messages in thread
From: Namhyung Kim @ 2015-01-14 11:18 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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


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

* Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
                   ` (2 preceding siblings ...)
  2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
@ 2015-01-14 12:57 ` Arnaldo Carvalho de Melo
  2015-01-14 14:57 ` David Ahern
  2015-01-17 10:13 ` [tip:perf/urgent] " tip-bot for Namhyung Kim
  5 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-01-14 12:57 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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

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

* Re: [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols
  2015-01-14 11:18 ` [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols Namhyung Kim
@ 2015-01-14 13:54   ` Arnaldo Carvalho de Melo
  2015-01-15 12:09     ` Masami Hiramatsu
  2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-01-14 13:54 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu,
	David Ahern

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

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

* Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
                   ` (3 preceding siblings ...)
  2015-01-14 12:57 ` [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Arnaldo Carvalho de Melo
@ 2015-01-14 14:57 ` David Ahern
  2015-01-14 21:08   ` Arnaldo Carvalho de Melo
  2015-01-17 10:13 ` [tip:perf/urgent] " tip-bot for Namhyung Kim
  5 siblings, 1 reply; 20+ messages in thread
From: David Ahern @ 2015-01-14 14:57 UTC (permalink / raw)
  To: Namhyung Kim, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu

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

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

* Re: [PATCH v2 4/4] perf probe: Fix probing kretprobes
  2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
@ 2015-01-14 16:26   ` Jiri Olsa
  2015-01-15 12:11   ` Masami Hiramatsu
  2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2 siblings, 0 replies; 20+ messages in thread
From: Jiri Olsa @ 2015-01-14 16:26 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, LKML,
	Masami Hiramatsu, David Ahern

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

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

* Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()
  2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
@ 2015-01-14 16:36   ` David Ahern
  2015-01-15  0:23     ` Namhyung Kim
  2015-01-28 15:01   ` [tip:perf/core] perf symbols: Return the first entry with a given name in find_by_name method tip-bot for Namhyung Kim
  1 sibling, 1 reply; 20+ messages in thread
From: David Ahern @ 2015-01-14 16:36 UTC (permalink / raw)
  To: Namhyung Kim, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, Masami Hiramatsu

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,


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

* Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-14 14:57 ` David Ahern
@ 2015-01-14 21:08   ` Arnaldo Carvalho de Melo
  2015-01-15  0:20     ` Namhyung Kim
  0 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-01-14 21:08 UTC (permalink / raw)
  To: David Ahern
  Cc: Namhyung Kim, Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML,
	Masami Hiramatsu

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

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

* Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-14 21:08   ` Arnaldo Carvalho de Melo
@ 2015-01-15  0:20     ` Namhyung Kim
  2015-01-17 12:38       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 20+ messages in thread
From: Namhyung Kim @ 2015-01-15  0:20 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: David Ahern, Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML,
	Masami Hiramatsu

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

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

* Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()
  2015-01-14 16:36   ` David Ahern
@ 2015-01-15  0:23     ` Namhyung Kim
  2015-01-15  0:42       ` David Ahern
  0 siblings, 1 reply; 20+ messages in thread
From: Namhyung Kim @ 2015-01-15  0:23 UTC (permalink / raw)
  To: David Ahern
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, Masami Hiramatsu

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/

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

* Re: [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name()
  2015-01-15  0:23     ` Namhyung Kim
@ 2015-01-15  0:42       ` David Ahern
  0 siblings, 0 replies; 20+ messages in thread
From: David Ahern @ 2015-01-15  0:42 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, Masami Hiramatsu

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

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

* Re: Re: [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols
  2015-01-14 13:54   ` Arnaldo Carvalho de Melo
@ 2015-01-15 12:09     ` Masami Hiramatsu
  0 siblings, 0 replies; 20+ messages in thread
From: Masami Hiramatsu @ 2015-01-15 12:09 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern

(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



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

* Re: [PATCH v2 4/4] perf probe: Fix probing kretprobes
  2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
  2015-01-14 16:26   ` Jiri Olsa
@ 2015-01-15 12:11   ` Masami Hiramatsu
  2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2 siblings, 0 replies; 20+ messages in thread
From: Masami Hiramatsu @ 2015-01-15 12:11 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern

(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



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

* [tip:perf/urgent] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
                   ` (4 preceding siblings ...)
  2015-01-14 14:57 ` David Ahern
@ 2015-01-17 10:13 ` tip-bot for Namhyung Kim
  5 siblings, 0 replies; 20+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-01-17 10:13 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, jolsa, dsahern, mingo, namhyung, acme, hpa,
	masami.hiramatsu.pt, a.p.zijlstra, linux-kernel

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);

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

* Re: [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI
  2015-01-15  0:20     ` Namhyung Kim
@ 2015-01-17 12:38       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-01-17 12:38 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: David Ahern, Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML,
	Masami Hiramatsu

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

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

* [tip:perf/core] perf symbols: Return the first entry with a given name in find_by_name method
  2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
  2015-01-14 16:36   ` David Ahern
@ 2015-01-28 15:01   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 20+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-01-28 15:01 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: namhyung, mingo, dsahern, hpa, jolsa, tglx, acme, linux-kernel,
	masami.hiramatsu.pt

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,

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

* [tip:perf/core] perf probe: Do not rely on map__load() filter to find symbols
  2015-01-14 11:18 ` [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols Namhyung Kim
  2015-01-14 13:54   ` Arnaldo Carvalho de Melo
@ 2015-01-28 15:02   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 20+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-01-28 15:02 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, jolsa, dsahern, namhyung, hpa, mingo, masami.hiramatsu.pt,
	linux-kernel, tglx

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:

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

* [tip:perf/core] perf probe: Fix probing kretprobes
  2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
  2015-01-14 16:26   ` Jiri Olsa
  2015-01-15 12:11   ` Masami Hiramatsu
@ 2015-01-28 15:02   ` tip-bot for Namhyung Kim
  2 siblings, 0 replies; 20+ messages in thread
From: tip-bot for Namhyung Kim @ 2015-01-28 15:02 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, mingo, linux-kernel, dsahern, masami.hiramatsu.pt, tglx,
	namhyung, hpa, jolsa

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) {

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

end of thread, other threads:[~2015-01-28 21:24 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-14 11:18 [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Namhyung Kim
2015-01-14 11:18 ` [PATCH v2 2/4] perf tools: Add link argument to dso__find_symbol_by_name() Namhyung Kim
2015-01-14 16:36   ` David Ahern
2015-01-15  0:23     ` Namhyung Kim
2015-01-15  0:42       ` David Ahern
2015-01-28 15:01   ` [tip:perf/core] perf symbols: Return the first entry with a given name in find_by_name method tip-bot for Namhyung Kim
2015-01-14 11:18 ` [PATCH v2 3/4] perf probe: Do not rely on map__load() filter to find symbols Namhyung Kim
2015-01-14 13:54   ` Arnaldo Carvalho de Melo
2015-01-15 12:09     ` Masami Hiramatsu
2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-01-14 11:18 ` [PATCH v2 4/4] perf probe: Fix probing kretprobes Namhyung Kim
2015-01-14 16:26   ` Jiri Olsa
2015-01-15 12:11   ` Masami Hiramatsu
2015-01-28 15:02   ` [tip:perf/core] " tip-bot for Namhyung Kim
2015-01-14 12:57 ` [PATCH v2 1/4] perf tools: Fix segfault for symbol annotation on TUI Arnaldo Carvalho de Melo
2015-01-14 14:57 ` David Ahern
2015-01-14 21:08   ` Arnaldo Carvalho de Melo
2015-01-15  0:20     ` Namhyung Kim
2015-01-17 12:38       ` Arnaldo Carvalho de Melo
2015-01-17 10:13 ` [tip:perf/urgent] " tip-bot for Namhyung Kim

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).