Re: [PATCH v2] perf report: distinguish between inliners in the same function

[Milian Wolff]

On Freitag, 12. Mai 2017 15:01:29 CEST Namhyung Kim wrote:
> On Fri, May 12, 2017 at 12:37:01PM +0200, Milian Wolff wrote:
> > On Mittwoch, 10. Mai 2017 07:53:52 CEST Namhyung Kim wrote:
> > > Hi,
> > 
> > > On Wed, May 03, 2017 at 11:35:36PM +0200, Milian Wolff wrote:
> > <snip>
> > 
> > > > +static enum match_result match_chain_srcline(struct
> > > > callchain_cursor_node
> > > > *node, +					     struct callchain_list *cnode)
> > > > +{
> > > > +	char *left = get_srcline(cnode->ms.map->dso,
> > > > +				 map__rip_2objdump(cnode->ms.map, cnode->ip),
> > > > +				 cnode->ms.sym, true, false);
> > > > +	char *right = get_srcline(node->map->dso,
> > > > +				  map__rip_2objdump(node->map, node->ip),
> > > > +				  node->sym, true, false);
> > > > +	enum match_result ret = match_chain_strings(left, right);
> > > 
> > > I think we need to check inlined srcline as well.  There might be a
> > > case that two samples have different addresses (and from different
> > > callchains) but happens to be mapped to a same srcline IMHO.
> > 
> > I think I'm missing something, but isn't this what this function provides?
> > The function above is now being used by the match_chain_inliner function
> > below.
> > 
> > Ah, or do you mean for code such as this:
> > 
> > ~~~~~
> > inline_func_1(); inline_func_2();
> > ~~~~~
> > 
> > Here, both branches could be inlined into the same line and the same issue
> > would occur, i.e. different branches get collapsed into the first match
> > for
> > the given srcline?
> > 
> > Hm yes, this should be fixed too.
> 
> OK.
> 
> > But, quite frankly, I think it just shows more and more that the current
> > inliner support is really fragile and leads to lots of issues throughout
> > the code base as the inlined frames are different from non-inlined
> > frames, but should most of the same be handled just like them.
> > 
> > So, maybe it's time to once more think about going back to my initial
> > approach: Make inlined frames code-wise equal to non-inlined frames, i.e.
> > instead of requesting the inlined frames within match_chain, do it outside
> > and create callchain_node/callchain_cursor instances (not sure which one
> > right now) for the inlined frames too.
> > 
> > This way, we should be able to centrally add support for inlined frames
> > and
> > all areas will benefit from it:
> > 
> > - aggregation by srcline/function will magically work
> > - all browsers will automatically display them, i.e. no longer need to
> > duplicate the code for inliner support in perf script, perf report tui/
> > stdio/...
> > - we can easily support --inline in other tools, like `perf trace --call-
> > graph`
> > 
> > So before I invest more time trying to massage match_chain to behave well
> > for inline nodes, can I get some feedback on the above?
> 
> Fair enough.  I agree that it'd be better adding them as separate
> callchain nodes when resolving callchains.

Can you, or anyone else more involved with the current callchain code, guide 
me a bit?

My previous attempt at doing this can be seen here:
https://github.com/milianw/linux/commit/
71d031c9d679bfb4a4044226e8903dd80ea601b3

There are some issues with that. Most of it boils down to the question of how 
to feed an inlined frame into a callchain_cursor_node. The latter contains a 
symbol* e.g. and users of that class currently rely on using the IP to find 
e.g. the corresponding srcline.

>From what I can see, we either have to hack inline nodes in there, cf. my 
original approach in the github repo above. Or, better, we would have to 
(heavily?) refactor the callchain_cursor_node struct and the code depending on 
it. What I have in mind would be something like adding two members to this 
struct:

const char* funcname;
const char* srcline;

For non-inlined frames, the funcname aliases the `symbol->name` value, and the 
srcline is queried as-needed. For inlined frames the values from the inlined 
node struct are used. The inlined frames for a given code location would all 
share the same symbol and ip.

Would that be OK as a path forward?

Cheers

-- 
Milian Wolff | milian.wolff@xxxxxxxx | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Attachment: smime.p7s
Description: S/MIME cryptographic signature