[PATCH v3 1/3] perf tools: Add from argument to dso__find_symbol_by_name()

[PATCH v3 1/3] perf tools: Add from 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 'from' argument to dso__find_symbol_by_name() to remember
current symbol position and returns next symbol if it provided.  For the
first call the from should be NULL and the returned symbol should be
used as from 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/util/map.c    | 13 ++++++++++---
 tools/perf/util/map.h    |  3 +++
 tools/perf/util/symbol.c | 41 ++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/symbol.h |  2 +-
 4 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 62ca9f2607d5..01e2696ca8b5 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -301,8 +301,9 @@ struct symbol *map__find_symbol(struct map *map, u64 addr,
 	return dso__find_symbol(map->dso, map->type, addr);
 }
 
-struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
-					symbol_filter_t filter)
+struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
+					     symbol_filter_t filter,
+					     struct symbol *from)
 {
 	if (map__load(map, filter) < 0)
 		return NULL;
@@ -310,7 +311,13 @@ 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, from);
+}
+
+struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
+					symbol_filter_t filter)
+{
+	return map__find_symbol_by_name_from(map, name, filter, NULL);
 }
 
 struct map *map__clone(struct map *map)
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 6951a9d42339..69acc58e6f9a 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -138,6 +138,9 @@ 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);
+struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
+					     symbol_filter_t filter,
+					     struct symbol *from);
 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..e5800497ca61 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 @from is NULL, returns first symbol that matched with @name.
+ * Otherwise, returns next symbol after @from 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 *from)
 {
-	return symbols__find_by_name(&dso->symbol_names[type], name);
+	struct rb_node *n;
+	struct symbol_name_rb_node *s;
+
+	if (from == NULL)
+		return symbols__find_by_name(&dso->symbol_names[type], name);
+
+	s = container_of(from, 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..cd4f51d95bd0 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 *from);
 
 struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
 struct symbol *dso__next_symbol(struct symbol *sym);
-- 
2.2.1

[PATCH v3 2/3] 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_from() 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 | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 94a717bf007d..3cfad9c74faf 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2193,18 +2193,17 @@ 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 = NULL;
+	int found = 0;
+
+	while ((sym = map__find_symbol_by_name_from(map, name, NULL, sym))) {
+		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
+			found++;
 	}
-	return 1;
+
+	return found;
 }
 
 #define strdup_or_goto(str, label)	\
@@ -2221,11 +2220,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 = 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 +2241,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 +2272,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_from(map, pp->function, NULL, sym))) {
 		tev = (*tevs) + ret;
 		tp = &tev->point;
 		if (ret == num_matched_functions) {
-- 
2.2.1

[PATCH v3 3/3] 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>
Tested-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 3cfad9c74faf..8fcb19b0e74f 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) {
-- 
2.2.1

Re: [PATCH v3 1/3] perf tools: Add from argument to dso__find_symbol_by_name()

[Masami Hiramatsu]

(2015/01/15 8:48), Namhyung Kim wrote:
> 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 'from' argument to dso__find_symbol_by_name() to remember
> current symbol position and returns next symbol if it provided.  For the
> first call the from should be NULL and the returned symbol should be
> used as from to the next call.  It will return NULL if there's no symbol
> that has given name anymore.
> 

Looks good to me

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

Thank you!

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/map.c    | 13 ++++++++++---
>  tools/perf/util/map.h    |  3 +++
>  tools/perf/util/symbol.c | 41 ++++++++++++++++++++++++++++++++++++-----
>  tools/perf/util/symbol.h |  2 +-
>  4 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 62ca9f2607d5..01e2696ca8b5 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -301,8 +301,9 @@ struct symbol *map__find_symbol(struct map *map, u64 addr,
>  	return dso__find_symbol(map->dso, map->type, addr);
>  }
>  
> -struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> -					symbol_filter_t filter)
> +struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
> +					     symbol_filter_t filter,
> +					     struct symbol *from)
>  {
>  	if (map__load(map, filter) < 0)
>  		return NULL;
> @@ -310,7 +311,13 @@ 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, from);
> +}
> +
> +struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> +					symbol_filter_t filter)
> +{
> +	return map__find_symbol_by_name_from(map, name, filter, NULL);
>  }
>  
>  struct map *map__clone(struct map *map)
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index 6951a9d42339..69acc58e6f9a 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -138,6 +138,9 @@ 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);
> +struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
> +					     symbol_filter_t filter,
> +					     struct symbol *from);
>  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..e5800497ca61 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 @from is NULL, returns first symbol that matched with @name.
> + * Otherwise, returns next symbol after @from 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 *from)
>  {
> -	return symbols__find_by_name(&dso->symbol_names[type], name);
> +	struct rb_node *n;
> +	struct symbol_name_rb_node *s;
> +
> +	if (from == NULL)
> +		return symbols__find_by_name(&dso->symbol_names[type], name);
> +
> +	s = container_of(from, 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..cd4f51d95bd0 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 *from);
>  
>  struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
>  struct symbol *dso__next_symbol(struct symbol *sym);
> 


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

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

[Masami Hiramatsu]

(2015/01/15 8:48), Namhyung Kim wrote:
> 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_from() 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.
> 

Looks good to me :)

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

Thank you!

> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/probe-event.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 94a717bf007d..3cfad9c74faf 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2193,18 +2193,17 @@ 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 = NULL;
> +	int found = 0;
> +
> +	while ((sym = map__find_symbol_by_name_from(map, name, NULL, sym))) {
> +		if (sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL)
> +			found++;
>  	}
> -	return 1;
> +
> +	return found;
>  }
>  
>  #define strdup_or_goto(str, label)	\
> @@ -2221,11 +2220,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 = 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 +2241,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 +2272,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_from(map, pp->function, NULL, sym))) {
>  		tev = (*tevs) + ret;
>  		tp = &tev->point;
>  		if (ret == num_matched_functions) {
> 


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

Re: [PATCH v3 3/3] perf probe: Fix probing kretprobes

[Masami Hiramatsu]

(2015/01/15 8:48), 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.

Oops, I missed the 3rd version... Of course,

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

Thank you!

> Reported-by: Jiri Olsa <jolsa@redhat.com>
> Tested-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 3cfad9c74faf..8fcb19b0e74f 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) {
> 


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

Re: [PATCH v3 1/3] perf tools: Add from argument to dso__find_symbol_by_name()

[Arnaldo Carvalho de Melo]

Em Thu, Jan 15, 2015 at 08:48:20AM +0900, Namhyung Kim escreveu:
> 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 'from' argument to dso__find_symbol_by_name() to remember
> current symbol position and returns next symbol if it provided.  For the
> first call the from should be NULL and the returned symbol should be
> used as from to the next call.  It will return NULL if there's no symbol
> that has given name anymore.

I had some issues with this patch that made me to split it into multiple
ones and use a slightly different approach, I put those patches in a
tmp.perf/urgent branch at my tree:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/urgent

I tested it with a ret probe for do_fork and it work well, thanks for
fixing this up!

See below for the issues:
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/map.c    | 13 ++++++++++---
>  tools/perf/util/map.h    |  3 +++
>  tools/perf/util/symbol.c | 41 ++++++++++++++++++++++++++++++++++++-----
>  tools/perf/util/symbol.h |  2 +-
>  4 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 62ca9f2607d5..01e2696ca8b5 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -301,8 +301,9 @@ struct symbol *map__find_symbol(struct map *map, u64 addr,
>  	return dso__find_symbol(map->dso, map->type, addr);
>  }
>  
> -struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> -					symbol_filter_t filter)
> +struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
> +					     symbol_filter_t filter,
> +					     struct symbol *from)
>  {
>  	if (map__load(map, filter) < 0)
>  		return NULL;
> @@ -310,7 +311,13 @@ 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, from);
> +}
> +
> +struct symbol *map__find_symbol_by_name(struct map *map, const char *name,
> +					symbol_filter_t filter)
> +{
> +	return map__find_symbol_by_name_from(map, name, filter, NULL);
>  }
>  
>  struct map *map__clone(struct map *map)
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index 6951a9d42339..69acc58e6f9a 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -138,6 +138,9 @@ 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);
> +struct symbol *map__find_symbol_by_name_from(struct map *map, const char *name,
> +					     symbol_filter_t filter,
> +					     struct symbol *from);
>  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..e5800497ca61 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;
>  }

Up to here we have a standalone patch, one that fixes
symbols__find_by_name to return the first entry with the given symbol
name, so I split it as-is and kept your authorship credit.
>  
>  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 @from is NULL, returns first symbol that matched with @name.
> + * Otherwise, returns next symbol after @from 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 *from)


Here I think this method should've been renamed
dso__find_symbol_by_name_from(), to keep the semantic coherent, matching
the map__find_symbol_by_name_from() counterpart.

But then, since when from is !NULL, it is not about finding anything,
instead it is about asking for the next entry in the sorted-by-name
rbtree, so instead I introduced a symbol__next_by_name() that does just
that.

The callers then will see if there is a next entry (sorted by name) and
if that entry has the same name.

I.e. keeping the name "find_symbol_by_name" when there is no "find"
operation being made is confusing.

Then, after introducing it, I further introduced a
map__for_each_symbol_with_name(map, sym_name, sym) that wraps it all up
and provides all the symbols with a given name for the loop block in
front of it.

Also:

>  {
> -	return symbols__find_by_name(&dso->symbol_names[type], name);
> +	struct rb_node *n;
> +	struct symbol_name_rb_node *s;
> +
> +	if (from == NULL)
> +		return symbols__find_by_name(&dso->symbol_names[type], name);
> +
> +	s = container_of(from, struct symbol_name_rb_node, sym);
> +	n = rb_prev(&s->rb_node);

Why are you going backwards here? Since you above stated that
"Otherwise, returns next symbol after @from".

And since you, in symbols__find_by_name as well go on backwards
(rb_prev), rightly, to find the first entry with a given name?

> +	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..cd4f51d95bd0 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 *from);
>  
>  struct symbol *dso__first_symbol(struct dso *dso, enum map_type type);
>  struct symbol *dso__next_symbol(struct symbol *sym);
> -- 
> 2.2.1