On Montag, 23. Oktober 2017 21:04:53 CEST Arnaldo Carvalho de Melo wrote: > Em Fri, Oct 20, 2017 at 10:21:03PM +0200, Milian Wolff escreveu: > > On Freitag, 20. Oktober 2017 18:15:40 CEST Arnaldo Carvalho de Melo wrote: > > > Em Thu, Oct 19, 2017 at 01:38:31PM +0200, Milian Wolff escreveu: > > > > This series of patches completely reworks the way inline frames are > > > > handled. Instead of querying for the inline nodes on-demand in the > > > > individual tools, we now create proper callchain nodes for inlined > > > > frames. The advantages this approach brings are numerous: > > > > > > > > - less duplicated code in the individual browser > > > > - aggregated cost for inlined frames for the --children top-down list > > > > - various bug fixes that arose from querying for a srcline/symbol > > > > based on > > > > > > > > the IP of a sample, which will always point to the last inlined > > > > frame > > > > instead of the corresponding non-inlined frame > > > > > > > > - overall much better support for visualizing cost for heavily-inlined > > > > C++ > > > > > > > > code, which simply was confusing and unreliably before > > > > > > > > - srcline honors the global setting as to whether full paths or > > > > basenames > > > > > > > > should be shown > > > > > > > > - caches for inlined frames and srcline information, which allow us to > > > > > > > > enable inline frame handling by default > > > > > > > > For comparison, below lists the output before and after for `perf > > > > script` > > > > > > > and `perf report`. The example file I used to generate the perf data is: > > > So, please check my tmp.perf/core branch, it has this patchset + the fix > > > I proposed for the match_chain() to always use absolute addresses. > > > > OK, so I've looked at it. I think there are some style issues with the > > indentation in match_chain_addresses. Also, the unmap_ip lines are too > > long > > for checkpatch.pl > > > > Additionally, we can now still run into the CCKEY_ADDRESS code path (when > > match_chain_strings for inlined symbols returns MATCH_ERROR, or when > > either > > cnode->ms.sym or node->sym is invalid), but won't unmap the IP properly > > then. > > so you're saying that cnode->ip and node->ip may be relative or > absolute? I thought they were always absolute, but I'll double check.
See below. > > Can we maybe instead use something like this on top of your patch? > > > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c > > index 01fc95fdd1e0..92bca95be202 100644 > > --- a/tools/perf/util/callchain.c > > +++ b/tools/perf/util/callchain.c > > @@ -669,11 +669,16 @@ static enum match_result match_chain_strings(const > > char *left, > > > > static enum match_result match_chain_addresses(u64 left_ip, u64 right_ip) > > { > > > > if (left_ip == right_ip) > > > > - return MATCH_EQ; > > - else if (left_ip < right_ip) > > - return MATCH_LT; > > - else > > - return MATCH_GT; > > + return MATCH_EQ; > > + else if (left_ip < right_ip) > > + return MATCH_LT; > > + else > > + return MATCH_GT; > > +} > > Applied the space fixes above, but the following I don't think makes > things clearer, it is not "unmap_ip()" it is at its best > try_to_unmap_ip_but_do_not_unmap_if_not_possible() which is confusing > 8-) > > So we better fix it in the users and continue using the existing > map->unmap_ip(map, rel_ip) idiom. > > > +static u64 unmap_ip(struct map *map, u64 ip) > > +{ > > + return map ? map->unmap_ip(map, ip) : ip; > > > > } > > > > static enum match_result match_chain(struct callchain_cursor_node *node, > > > > @@ -702,9 +707,10 @@ static enum match_result match_chain(struct > > callchain_cursor_node *node, > > > > if (match != MATCH_ERROR) > > > > break; > > > > } else { > > > > - u64 left = > > cnode->ms.map->unmap_ip(cnode->ms.map, cnode- > > > > >ms.sym->start), > > > > - right = node->map->unmap_ip(node->map, > > node->sym- >start); > > - > > + u64 left = unmap_ip(cnode->ms.map, > > + cnode->ms.sym->start); > > + u64 right = unmap_ip(node->map, > > + node->sym->start); > > So, in the above, you say that cnode->ms.map or node->map may be NULL, > right? But then both are asking for a sym->start (which is a relative > address, it came from a symtab), and furthermore, for cnode->ms.sym to > be not NULL means that cnode->ms.map is not NULL, after all > cnode->ms.sym came from a dso__find_symbol(cnode->ms.map->dso). Ugh sorry yes, now I see where my confusion comes from... I clearly did not understand Ravi's patch in its entirety - sorry for that. So trying to bring back some clarity, let's summarize: - sym->start is always relative - *->ip is absolute if no map could be found - *->ip is relative otherwise if there is a map - we need to always use relative addresses as we want to aggregate from different address spaces (see also Namhyung's latest mail in the thread on v6 of this patch series) - we need to prevent merging of equal relative addresses from different DSOs So to fix this all, I guess the suggested approach by Namhyung would be best, i.e. fixup my initial match_addresses to take the map, and then if the map is valid also take the dso into account when comparing the addresses: if (left_dso != right_dso) return left_dso < right_dso ? MATCH_LT : MATCH_GT; else if (left_ip != right_ip) return left_ip < right_ip ? MATCH_LT : MATCH_GT; else return MATCH_EQ; > Ditto for node->sym/node->map. > > > match = match_chain_addresses(left, right); > > break; > > > > } > > > > @@ -713,7 +719,9 @@ static enum match_result match_chain(struct > > callchain_cursor_node *node, > > > > __fallthrough; > > > > case CCKEY_ADDRESS: > > > > default: > > - match = match_chain_addresses(cnode->ip, node->ip); > > + match = match_chain_addresses(unmap_ip(cnode->ms.map, > > + cnode->ip), > > + unmap_ip(node->map, node->ip)); > > Here I need to look further, to see what kind of address cnode->ip is, > my expectation is that it is a absolute address, so no need for > unmapping, will check. Please double check this and also the other points in my list above. It is all a bit confusing... Do you want me to supply another patch, or will you take care of this? -- 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