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

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Jan 12, 2015 at 09:31:30PM +0900, Masami Hiramatsu wrote:
> (2015/01/10 19:33), 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() 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.  Now
> > resulting symbol references are saved in the funcs list.
> > 
> > Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 87 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index 7f9b8632e433..e5af16988791 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
> >  	return ret;
> >  }
> >  
> > -static char *looking_function_name;
> > -static int num_matched_functions;
> > +struct symbol_entry {
> > +	struct list_head node;
> > +	struct symbol *sym;
> > +};
> >  
> > -static int probe_function_filter(struct map *map __maybe_unused,
> > -				      struct symbol *sym)
> > +/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
> > +static int add_symbol_entry(struct symbol *sym, struct list_head *head)
> >  {
> > -	if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
> > -	    strcmp(looking_function_name, sym->name) == 0) {
> > -		num_matched_functions++;
> > +	struct symbol_entry *ent;
> > +
> > +	if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
> >  		return 0;
> > -	}
> > +
> > +	ent = malloc(sizeof(*ent));
> > +	if (ent == NULL)
> > +		return -1;
> 
> return -ENOMEM; ?

Okay, will change.


> 
> > +
> > +	ent->sym = sym;
> > +	list_add(&ent->node, head);
> >  	return 1;
> >  }
> >  
> > +static int find_probe_functions(struct map *map, char *name, struct list_head *head)
> > +{
> > +	struct symbol *sym, *orig_sym;
> > +	struct symbol_entry *ent;
> > +	struct rb_node *node;
> > +	int found = 0;
> > +	int ret;
> > +
> > +	sym = map__find_symbol_by_name(map, name, NULL);
> > +	if (sym == NULL)
> > +		return 0;
> > +
> > +	ret = add_symbol_entry(sym, head);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	found += ret;
> 
> If ret always be 1 in successful case, we'd better do "found++" here.
> And it also means we can do it shorter as below.
> 
> if (add_symbol_entry(sym, head) < 0)
> 	goto err;
> found++;

But it can return 0 in successful case like STB_WEAK..  I'm not sure
how we can handle the weak functions properly, but anyway the original
code already ignored the weak functions.


> 
> > +
> > +	/* search back and forth to find symbols that have same name */
> 
> Hmm, I see. but this code looks no-good sign... Can we have any generic
> synonym handling routine?

Like what?  I guess we can change map__find_symbol_by_name() to return
a list of symbols or add a new function to do it.  Is it okay to you?


> 
> Other parts looks good to me:)

Thanks for your review!
Namhyung