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

[Namhyung Kim]

Hi Milian,

On Sun, May 14, 2017 at 08:10:50PM +0200, Milian Wolff wrote:
> 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?

I think you'd be better adding (fake) dso and sym to keep the inline
information.  The fake dso can be paired with the original dso and
maintain a tree of (inlined) symbols.  You may need a fake map to
point the fake dso then.  It seems a bit compilcated but that way the
code will be more consistent and easier to handle (e.g. for caching
and/or deletion) IMHO.

Also, for the children mode, callchain nodes should have enough
information to create hist entries (but I'm not sure how to apply
self periods for those inlined entries).

Thanks,
Namhyung