linux-kernel - Re: [PATCH v7 0/5] generate full callchain cursor entries for inlined frames

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Oct 2017 22:21:03 +0200
From:   Milian Wolff <milian.wolff@...b.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     jolsa@...nel.org, namhyung@...nel.org,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Andi Kleen <andi@...stfloor.org>,
        Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH v7 0/5] generate full callchain cursor entries for inlined frames

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.

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;
+}
+
+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);
 				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));
 		break;
 	}
 
Cheers
-- 
Milian Wolff | milian.wolff@...b.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts