Hi Milian, On Sun, Aug 06, 2017 at 11:24:35PM +0200, Milian Wolff wrote: > The inlined frames use a fake symbol that is tracked in a special > map inside the dso, which is always sorted by name. All other
It seems the above is not true. Fake symbols are maintained by inline_node which in turn maintained by dso->inlines tree. > entries of the symbol beside the function name are unused for > inline frames. The advantage of this approach is that all existing > users of the callchain API can now transparently display inlined > frames without having to patch their code. > > Cc: Arnaldo Carvalho de Melo <[email protected]> > Cc: David Ahern <[email protected]> > Cc: Namhyung Kim <[email protected]> > Cc: Peter Zijlstra <[email protected]> > Cc: Yao Jin <[email protected]> > Signed-off-by: Milian Wolff <[email protected]> > --- > tools/perf/util/callchain.c | 31 +++----- > tools/perf/util/callchain.h | 6 +- > tools/perf/util/dso.c | 2 + > tools/perf/util/dso.h | 1 + > tools/perf/util/machine.c | 56 +++++++++++++- > tools/perf/util/srcline.c | 183 > ++++++++++++++++++++++++++++++++++---------- > tools/perf/util/srcline.h | 19 ++++- > tools/perf/util/symbol.h | 1 + > 8 files changed, 230 insertions(+), 69 deletions(-) > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c > index f320b0777e0d..9854adb06ac1 100644 > --- a/tools/perf/util/callchain.c > +++ b/tools/perf/util/callchain.c > @@ -559,6 +559,7 @@ fill_node(struct callchain_node *node, struct > callchain_cursor *cursor) > call->ip = cursor_node->ip; > call->ms.sym = cursor_node->sym; > call->ms.map = map__get(cursor_node->map); > + call->srcline = cursor_node->srcline; > > if (cursor_node->branch) { > call->branch_count = 1; > @@ -640,20 +641,11 @@ enum match_result { > static enum match_result match_chain_srcline(struct callchain_cursor_node > *node, > struct callchain_list *cnode) > { > - char *left = NULL; > - char *right = NULL; > + const char *left = cnode->srcline; > + const char *right = node->srcline; > enum match_result ret = MATCH_EQ; > int cmp; > > - if (cnode->ms.map) > - left = get_srcline(cnode->ms.map->dso, > - map__rip_2objdump(cnode->ms.map, cnode->ip), > - cnode->ms.sym, true, false); > - if (node->map) > - right = get_srcline(node->map->dso, > - map__rip_2objdump(node->map, node->ip), > - node->sym, true, false); > - > if (left && right) > cmp = strcmp(left, right); > else if (!left && right) > @@ -668,8 +660,6 @@ static enum match_result match_chain_srcline(struct > callchain_cursor_node *node, > if (cmp != 0) > ret = cmp < 0 ? MATCH_LT : MATCH_GT; > > - free_srcline(left); > - free_srcline(right); > return ret; > } > > @@ -958,7 +948,7 @@ merge_chain_branch(struct callchain_cursor *cursor, > list_for_each_entry_safe(list, next_list, &src->val, list) { > callchain_cursor_append(cursor, list->ip, > list->ms.map, list->ms.sym, > - false, NULL, 0, 0, 0); > + false, NULL, 0, 0, 0, list->srcline); > list_del(&list->list); > map__zput(list->ms.map); > free(list); > @@ -998,7 +988,8 @@ int callchain_merge(struct callchain_cursor *cursor, > int callchain_cursor_append(struct callchain_cursor *cursor, > u64 ip, struct map *map, struct symbol *sym, > bool branch, struct branch_flags *flags, > - int nr_loop_iter, int samples, u64 branch_from) > + int nr_loop_iter, int samples, u64 branch_from, > + const char *srcline) > { > struct callchain_cursor_node *node = *cursor->last; > > @@ -1017,6 +1008,7 @@ int callchain_cursor_append(struct callchain_cursor > *cursor, > node->branch = branch; > node->nr_loop_iter = nr_loop_iter; > node->samples = samples; > + node->srcline = srcline; > > if (flags) > memcpy(&node->branch_flags, flags, > @@ -1104,12 +1096,7 @@ char *callchain_list__sym_name(struct callchain_list > *cl, > int printed; > > if (cl->ms.sym) { > - if (show_srcline && cl->ms.map && !cl->srcline) > - cl->srcline = get_srcline(cl->ms.map->dso, > - map__rip_2objdump(cl->ms.map, > - cl->ip), > - cl->ms.sym, false, show_addr); > - if (cl->srcline) > + if (show_srcline && cl->srcline) > printed = scnprintf(bf, bfsize, "%s %s", > cl->ms.sym->name, cl->srcline); > else > @@ -1524,7 +1511,7 @@ int callchain_cursor__copy(struct callchain_cursor *dst, > rc = callchain_cursor_append(dst, node->ip, node->map, > node->sym, > node->branch, &node->branch_flags, > node->nr_loop_iter, node->samples, > - node->branch_from); > + node->branch_from, node->srcline); > if (rc) > break; > > diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h > index 97738201464a..bf81b56f34c3 100644 > --- a/tools/perf/util/callchain.h > +++ b/tools/perf/util/callchain.h > @@ -121,7 +121,7 @@ struct callchain_list { > u64 iter_count; > u64 samples_count; > struct branch_type_stat brtype_stat; > - char *srcline; > + const char *srcline; > struct list_head list; > }; > > @@ -135,6 +135,7 @@ struct callchain_cursor_node { > u64 ip; > struct map *map; > struct symbol *sym; > + const char *srcline; > bool branch; > struct branch_flags branch_flags; > u64 branch_from; > @@ -201,7 +202,8 @@ static inline void callchain_cursor_reset(struct > callchain_cursor *cursor) > int callchain_cursor_append(struct callchain_cursor *cursor, u64 ip, > struct map *map, struct symbol *sym, > bool branch, struct branch_flags *flags, > - int nr_loop_iter, int samples, u64 branch_from); > + int nr_loop_iter, int samples, u64 branch_from, > + const char *srcline); > > /* Close a cursor writing session. Initialize for the reader */ > static inline void callchain_cursor_commit(struct callchain_cursor *cursor) I think it'd be better splitting srcline change into a separate commit. > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c > index b9e087fb8247..72e6e390fd26 100644 > --- a/tools/perf/util/dso.c > +++ b/tools/perf/util/dso.c > @@ -9,6 +9,7 @@ > #include "compress.h" > #include "path.h" > #include "symbol.h" > +#include "srcline.h" > #include "dso.h" > #include "machine.h" > #include "auxtrace.h" > @@ -1233,6 +1234,7 @@ void dso__delete(struct dso *dso) > dso->long_name); > for (i = 0; i < MAP__NR_TYPES; ++i) > symbols__delete(&dso->symbols[i]); > + inlines__tree_delete(&dso->inlined_nodes); Hmm.. inline_node is released after symbol but it seems to have a problem. Please see below. > > if (dso->short_name_allocated) { > zfree((char **)&dso->short_name); > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h > index f886141678eb..7d1e2b3c1f10 100644 > --- a/tools/perf/util/dso.h > +++ b/tools/perf/util/dso.h > @@ -141,6 +141,7 @@ struct dso { > struct rb_root *root; /* root of rbtree that rb_node is in */ > struct rb_root symbols[MAP__NR_TYPES]; > struct rb_root symbol_names[MAP__NR_TYPES]; > + struct rb_root inlined_nodes; > struct { > u64 addr; > struct symbol *symbol; [SNIP] > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index d4df353051af..a7f8499c8756 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c > index ebc88a74e67b..a1fdf035d1dd 100644 > --- a/tools/perf/util/srcline.c > +++ b/tools/perf/util/srcline.c > @@ -33,28 +33,17 @@ static const char *dso__name(struct dso *dso) > return dso_name; > } > > -static int inline_list__append(char *filename, char *funcname, int line_nr, > - struct inline_node *node, struct dso *dso) > +static int inline_list__append(struct symbol *symbol, char *srcline, > + struct inline_node *node) > { > struct inline_list *ilist; > - char *demangled; > > ilist = zalloc(sizeof(*ilist)); > if (ilist == NULL) > return -1; > > - ilist->filename = filename; > - ilist->line_nr = line_nr; > - > - if (dso != NULL) { > - demangled = dso__demangle_sym(dso, 0, funcname); > - if (demangled == NULL) { > - ilist->funcname = funcname; > - } else { > - ilist->funcname = demangled; > - free(funcname); > - } > - } > + ilist->symbol = symbol; > + ilist->srcline = srcline; > > if (callchain_param.order == ORDER_CALLEE) > list_add_tail(&ilist->list, &node->val); > @@ -64,6 +53,30 @@ static int inline_list__append(char *filename, char > *funcname, int line_nr, > return 0; > } > > +// basename version that takes a const input string > +static const char *gnu_basename(const char *path) > +{ > + const char *base = strrchr(path, '/'); > + > + return base ? base + 1 : path; > +} > + > +static char *srcline_from_fileline(const char *file, unsigned int line) > +{ > + char *srcline; > + > + if (!file) > + return NULL; > + > + if (!srcline_full_filename) > + file = gnu_basename(file); > + > + if (asprintf(&srcline, "%s:%u", file, line) < 0) > + return NULL; > + > + return srcline; > +} > + > #ifdef HAVE_LIBBFD_SUPPORT > > /* > @@ -203,19 +216,55 @@ static void addr2line_cleanup(struct a2l_data *a2l) > > #define MAX_INLINE_NEST 1024 > > +static struct symbol *new_inline_sym(struct dso *dso, > + struct symbol *base_sym, > + const char *funcname) > +{ > + struct symbol *inline_sym; > + char *demangled = NULL; > + > + if (dso) { > + demangled = dso__demangle_sym(dso, 0, funcname); > + if (demangled) > + funcname = demangled; > + } > + > + if (strcmp(funcname, base_sym->name) == 0) { > + // reuse the real, existing symbol > + inline_sym = base_sym; So inline_node could refer the existing symbol. > + } else { > + // create a fake symbol for the inline frame > + inline_sym = symbol__new(base_sym ? base_sym->start : 0, > + base_sym ? base_sym->end : 0, > + base_sym ? base_sym->binding : 0, > + funcname); > + if (inline_sym) > + inline_sym->inlined = 1; > + } > + > + free(demangled); > + > + return inline_sym; > +} > + > static int inline_list__append_dso_a2l(struct dso *dso, > - struct inline_node *node) > + struct inline_node *node, > + struct symbol *sym) > { > struct a2l_data *a2l = dso->a2l; > - char *funcname = a2l->funcname ? strdup(a2l->funcname) : NULL; > - char *filename = a2l->filename ? strdup(a2l->filename) : NULL; > + struct symbol *inline_sym = new_inline_sym(dso, sym, a2l->funcname); > + char *srcline = NULL; > + > + if (a2l->filename) > + srcline = srcline_from_fileline(a2l->filename, a2l->line); > > - return inline_list__append(filename, funcname, a2l->line, node, dso); > + return inline_list__append(inline_sym, srcline, node); > } > [SNIP] > @@ -511,10 +563,63 @@ void inline_node__delete(struct inline_node *node) > > list_for_each_entry_safe(ilist, tmp, &node->val, list) { > list_del_init(&ilist->list); > - zfree(&ilist->filename); > - zfree(&ilist->funcname); > + zfree(&ilist->srcline); > + // only the inlined symbols are owned by the list > + if (ilist->symbol && ilist->symbol->inlined) > + symbol__delete(ilist->symbol); Existing symbols are released at this moment. Thanks, Namhyung > free(ilist); > } > > free(node); > } > + > +void inlines__tree_insert(struct rb_root *tree, struct inline_node *inlines) > +{ > + struct rb_node **p = &tree->rb_node; > + struct rb_node *parent = NULL; > + const u64 addr = inlines->addr; > + struct inline_node *i; > + > + while (*p != NULL) { > + parent = *p; > + i = rb_entry(parent, struct inline_node, rb_node); > + if (addr < i->addr) > + p = &(*p)->rb_left; > + else > + p = &(*p)->rb_right; > + } > + rb_link_node(&inlines->rb_node, parent, p); > + rb_insert_color(&inlines->rb_node, tree); > +} > + > +struct inline_node *inlines__tree_find(struct rb_root *tree, u64 addr) > +{ > + struct rb_node *n = tree->rb_node; > + > + while (n) { > + struct inline_node *i = rb_entry(n, struct inline_node, > + rb_node); > + > + if (addr < i->addr) > + n = n->rb_left; > + else if (addr > i->addr) > + n = n->rb_right; > + else > + return i; > + } > + > + return NULL; > +} > + > +void inlines__tree_delete(struct rb_root *tree) > +{ > + struct inline_node *pos; > + struct rb_node *next = rb_first(tree); > + > + while (next) { > + pos = rb_entry(next, struct inline_node, rb_node); > + next = rb_next(&pos->rb_node); > + rb_erase(&pos->rb_node, tree); > + inline_node__delete(pos); > + } > +} > diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h > index 7b52ba88676e..0d2aca92e8c7 100644 > --- a/tools/perf/util/srcline.h > +++ b/tools/perf/util/srcline.h > @@ -2,6 +2,7 @@ > #define PERF_SRCLINE_H > > #include <linux/list.h> > +#include <linux/rbtree.h> > #include <linux/types.h> > > struct dso; > @@ -17,18 +18,28 @@ void free_srcline(char *srcline); > #define SRCLINE_UNKNOWN ((char *) "??:0") > > struct inline_list { > - char *filename; > - char *funcname; > - unsigned int line_nr; > + struct symbol *symbol; > + char *srcline; > struct list_head list; > }; > > struct inline_node { > u64 addr; > struct list_head val; > + struct rb_node rb_node; > }; > > -struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr); > +// parse inlined frames for the given address > +struct inline_node *dso__parse_addr_inlines(struct dso *dso, u64 addr, > + struct symbol *sym); > +// free resources associated to the inline node list > void inline_node__delete(struct inline_node *node); > > +// insert the inline node list into the DSO, which will take ownership > +void inlines__tree_insert(struct rb_root *tree, struct inline_node *inlines); > +// find previously inserted inline node list > +struct inline_node *inlines__tree_find(struct rb_root *tree, u64 addr); > +// delete all nodes within the tree of inline_node s > +void inlines__tree_delete(struct rb_root *tree); > + > #endif /* PERF_SRCLINE_H */ > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h > index f0b08810d7fa..b358570ce615 100644 > --- a/tools/perf/util/symbol.h > +++ b/tools/perf/util/symbol.h > @@ -59,6 +59,7 @@ struct symbol { > u8 binding; > u8 idle:1; > u8 ignore:1; > + u8 inlined:1; > u8 arch_sym; > char name[0]; > }; > -- > 2.13.3 >
