Messages in this thread | | | Date | Wed, 14 Jan 2015 10:45:06 +0900 | From | Namhyung Kim <> | Subject | Re: [PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols |
| |
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
| |