From: Namhyung Kim <namhyung@kernel.org>
To: Milian Wolff <milian.wolff@kdab.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Jin Yao <yao.jin@linux.intel.com>,
"linux-kernel@vger.kernel.org" <Linux-kernel@vger.kernel.org>,
linux-perf-users <linux-perf-users@vger.kernel.org>,
Arnaldo Carvalho de Melo <acme@redhat.com>,
David Ahern <dsahern@gmail.com>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
kernel-team@lge.com
Subject: Re: [PATCH v2 03/14] perf report: create real callchain entries for inlined frames
Date: Mon, 28 Aug 2017 21:18:45 +0900 [thread overview]
Message-ID: <CAM9d7cgSFtRq1_ULV=7N9dHKeXtieF06aj4eyRvJRzsE+iSiXA@mail.gmail.com> (raw)
In-Reply-To: <15340121.XrrsYTu5UN@agathebauer>
Hi Milian,
Sorry for late reply, I was on vacation.
On Mon, Aug 21, 2017 at 5:57 AM, Milian Wolff <milian.wolff@kdab.com> 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".
>
> <snip>
>
>> > @@ -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 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.
>
> Yes, that does indeed seem to be an issue. I wonder why I'm not seeing any
> crashes or valgrind/ASAN reports though. I'll have to dig into this.
>
>> > + } 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 for the review, I'll try to look into these issues once I have more
> time again.
>
> Cheers
>
> --
> Milian Wolff | milian.wolff@kdab.com | Senior Software Engineer
> KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
> Tel: +49-30-521325470
> KDAB - The Qt Experts
--
Thanks,
Namhyung
next prev parent reply other threads:[~2017-08-28 12:19 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-06 21:24 [PATCH v2 00/14] generate full callchain cursor entries for inlined frames Milian Wolff
2017-08-06 21:24 ` [PATCH v2 01/14] perf report: remove code to handle inline frames from browsers Milian Wolff
2017-08-07 15:07 ` Arnaldo Carvalho de Melo
2017-08-07 19:22 ` Milian Wolff
2017-08-07 20:16 ` Arnaldo Carvalho de Melo
2017-08-06 21:24 ` [PATCH v2 02/14] perf util: take elf_name as const string in dso__demangle_sym Milian Wolff
2017-08-07 15:10 ` Arnaldo Carvalho de Melo
2017-08-14 17:48 ` [tip:perf/core] perf util: Take " tip-bot for Milian Wolff
2017-08-06 21:24 ` [PATCH v2 03/14] perf report: create real callchain entries for inlined frames Milian Wolff
2017-08-16 7:53 ` Namhyung Kim
2017-08-20 20:57 ` Milian Wolff
2017-08-28 12:18 ` Namhyung Kim [this message]
2017-09-06 13:13 ` Milian Wolff
2017-09-07 14:58 ` Namhyung Kim
2017-09-07 15:05 ` Milian Wolff
2017-09-07 15:16 ` Namhyung Kim
2017-08-06 21:24 ` [PATCH v2 04/14] perf report: fall-back to function name comparison for -g srcline Milian Wolff
2017-08-06 21:24 ` [PATCH v2 05/14] perf report: mark inlined frames in output by " (inlined)" suffix Milian Wolff
2017-08-06 21:24 ` [PATCH v2 06/14] perf script: mark inlined frames and do not print DSO for them Milian Wolff
2017-08-06 21:24 ` [PATCH v2 07/14] perf report: compare symbol name for inlined frames when matching Milian Wolff
2017-08-06 21:24 ` [PATCH v2 08/14] perf report: compare symbol name for inlined frames when sorting Milian Wolff
2017-08-06 21:24 ` [PATCH v2 09/14] perf report: properly handle branch count in match_chain Milian Wolff
2017-08-06 21:24 ` [PATCH v2 10/14] perf report: cache failed lookups of inlined frames Milian Wolff
2017-08-06 21:24 ` [PATCH v2 11/14] perf report: cache srclines for callchain nodes Milian Wolff
2017-08-10 2:13 ` Namhyung Kim
2017-08-10 11:51 ` Milian Wolff
2017-08-10 14:56 ` Namhyung Kim
2017-08-10 17:58 ` Arnaldo Carvalho de Melo
2017-08-11 11:28 ` Milian Wolff
2017-08-06 21:24 ` [PATCH v2 12/14] perf report: use srcline from callchain for hist entries Milian Wolff
2017-08-06 21:24 ` [PATCH v2 13/14] perf util: do not consider empty files as valid srclines Milian Wolff
2017-08-07 15:21 ` Arnaldo Carvalho de Melo
2017-08-14 17:48 ` [tip:perf/core] perf srcline: Do " tip-bot for Milian Wolff
2017-08-06 21:24 ` [PATCH v2 14/14] perf util: enable handling of inlined frames by default Milian Wolff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM9d7cgSFtRq1_ULV=7N9dHKeXtieF06aj4eyRvJRzsE+iSiXA@mail.gmail.com' \
--to=namhyung@kernel.org \
--cc=Linux-kernel@vger.kernel.org \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@kernel.org \
--cc=acme@redhat.com \
--cc=dsahern@gmail.com \
--cc=kernel-team@lge.com \
--cc=linux-perf-users@vger.kernel.org \
--cc=milian.wolff@kdab.com \
--cc=yao.jin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).