Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
On Fri, Sep 8, 2017 at 12:05 AM, Milian Wolff wrote:
> On Thursday, September 7, 2017 4:58:53 PM CEST Namhyung Kim wrote:
>> Hi Milian,
>
> Hey Namhyung!
>
>> > > > > @@ -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 for the review, I'll try to look into these issues once I have
>> > > more
>> > > time again.
>> >
>> > OK, so I just dug into this part of the patch again. I don't think it's
>> > actually a problem after all:
>> >
>> > When an inline node reuses the real symbol, that symbol won't have its
>> > `inlined` member set to true. Thus these symbols will never get deleted by
>> > inline_node__delete.
>>
>> But ilist->symbol is a dangling pointer so accessing ->inlined would
>> be a problem, no?
>
> Sorry, but I can't follow. Why would it be a dangling pointer? Note, again,
> that I've tested this with both valgrind and ASAN and neither reports any
> issues about this code.
IIUC, ilist->symbol can point an existing symbol. And all existing
symbols are freed before calling inline_node__delete(). I don't know
why valgrind or asan didn't catch anything.. maybe I'm missing
something.
--
Thanks,
Namhyung
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
On Thursday, September 7, 2017 4:58:53 PM CEST Namhyung Kim wrote:
> Hi Milian,
Hey Namhyung!
> > > > > @@ -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 for the review, I'll try to look into these issues once I have
> > > more
> > > time again.
> >
> > OK, so I just dug into this part of the patch again. I don't think it's
> > actually a problem after all:
> >
> > When an inline node reuses the real symbol, that symbol won't have its
> > `inlined` member set to true. Thus these symbols will never get deleted by
> > inline_node__delete.
>
> But ilist->symbol is a dangling pointer so accessing ->inlined would
> be a problem, no?
Sorry, but I can't follow. Why would it be a dangling pointer? Note, again,
that I've tested this with both valgrind and ASAN and neither reports any
issues about this code.
> > If you have suggestions on how to make this clearer, I'm
> > all ears. For now, I'll add a comment to where we alias/reuse the symbol.
> >
> > I'll try to split up the patch now to make it somehow easier to review.
>
> Thanks for doing this, but I'm afraid I don't have time to review
> before going to OSS-NA.
No problem, enjoy!
--
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
Hi Milian,
On Wed, Sep 06, 2017 at 03:13:35PM +0200, Milian Wolff wrote:
> On Sonntag, 20. August 2017 22:57:17 CEST Milian Wolff wrote:
> > On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:
> > > 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.
> >
> > OK, I'll rephrase this to make it more explicit. But what I call "map" is
> > what you call "tree", no?
> >
> >
> >
> > > > @@ -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.
> >
> > OK, I'll try to see how this goes. It would simply add the boiler plate code
> > to pass the srcline around, and then set it from within the callchain code,
> > right?
> >
> > > > 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 {
> > > >
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
On Sonntag, 20. August 2017 22:57:17 CEST Milian Wolff wrote:
> On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:
> > 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.
>
> OK, I'll rephrase this to make it more explicit. But what I call "map" is
> what you call "tree", no?
>
>
>
> > > @@ -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.
>
> OK, I'll try to see how this goes. It would simply add the boiler plate code
> to pass the srcline around, and then set it from within the callchain code,
> right?
>
> > > 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)
> >
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
Hi Milian,
Sorry for late reply, I was on vacation.
On Mon, Aug 21, 2017 at 5:57 AM, Milian Wolff wrote:
> On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:
>> 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.
>
> OK, I'll rephrase this to make it more explicit. But what I call "map" is what
> you call "tree", no?
Right, but as we have "map" in perf code base, I think it's better to
use "tree".
>
>
>
>> > @@ -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.
>
> OK, I'll try to see how this goes. It would simply add the boiler plate code
> to pass the srcline around, and then set it from within the callchain code,
> right?
Yes. It'd help reviewers to concentrate on the logic IMHO.
Thanks,
Namhyung
>
>> > 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 cons
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
On Mittwoch, 16. August 2017 09:53:13 CEST Namhyung Kim wrote:
> 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.
OK, I'll rephrase this to make it more explicit. But what I call "map" is what
you call "tree", no?
> > @@ -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.
OK, I'll try to see how this goes. It would simply add the boiler plate code
to pass the srcline around, and then set it from within the callchain code,
right?
> > 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;
> > +
> > + i
Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
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
> Cc: David Ahern
> Cc: Namhyung Kim
> Cc: Peter Zijlstra
> Cc: Yao Jin
> Signed-off-by: Milian Wolff
> ---
> 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",
>
[PATCH v2 03/14] perf report: create real callchain entries for inlined frames
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
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
Cc: David Ahern
Cc: Namhyung Kim
Cc: Peter Zijlstra
Cc: Yao Jin
Signed-off-by: Milian Wolff
---
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->branc
