Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751234AbdFANKD (ORCPT ); Thu, 1 Jun 2017 09:10:03 -0400 Received: from mail.kernel.org ([198.145.29.99]:60644 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751108AbdFANKA (ORCPT ); Thu, 1 Jun 2017 09:10:00 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E6C5239DF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=acme@kernel.org Date: Thu, 1 Jun 2017 10:09:56 -0300 From: Arnaldo Carvalho de Melo To: Kim Phillips Cc: Ravi Bangoria , anton@samba.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, treeze.taeung@gmail.com, borntraeger@de.ibm.com, linux-kernel@vger.kernel.org, Robin Murphy , Mark Rutland Subject: Re: [PATCH] perf/annotate: Fix branch instruction with multiple operands Message-ID: <20170601130956.GC2899@kernel.org> References: <20170525055957.7779-1-ravi.bangoria@linux.vnet.ibm.com> <20170526182310.6e253240645df02342fe24c3@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170526182310.6e253240645df02342fe24c3@arm.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5954 Lines: 169 Em Fri, May 26, 2017 at 06:23:10PM -0500, Kim Phillips escreveu: > From: Ravi Bangoria > > Perf annotate is dropping the cr* fields from branch instructions. > Fix it by adding support to display branch instructions having > multiple operands. > > Objdump of int_sqrt: > > 20.36 | c0000000004d2694: subf r10,r10,r3 > | c0000000004d2698: v bgt cr6,c0000000004d26a0 > 1.82 | c0000000004d269c: mr r3,r10 > 29.18 | c0000000004d26a0: mr r10,r8 > | c0000000004d26a4: v bgt cr7,c0000000004d26ac > | c0000000004d26a8: mr r10,r7 > > Before Patch: > > 20.36 | subf r10,r10,r3 > | v bgt 40 > 1.82 | mr r3,r10 > 29.18 | 40: mr r10,r8 > | v bgt 4c > | mr r10,r7 > > After patch: > > 20.36 | subf r10,r10,r3 > | v bgt cr6,40 > 1.82 | mr r3,r10 > 29.18 | 40: mr r10,r8 > | v bgt cr7,4c > | mr r10,r7 > > Reported-by: Anton Blanchard > Signed-off-by: Ravi Bangoria > > Reduced to keep only one scnprintf and supplemented for AArch64 > conditional branch instructions: > > Non-simplified (raw objdump) view: > > │ffff0000083cd11c: ↑ cbz w0, ffff0000083cd100 ... > 4.44 │ffff000│083cd134: ↓ tbnz w0, #26, ffff0000083cd190 ... > 1.37 │ffff000│083cd144: ↓ tbnz w22, #5, ffff0000083cd1a4 │ffff000│083cd148: mov w19, #0x20000 //▒ > 1.02 │ffff000│083cd14c: ↓ tbz w22, #2, ffff0000083cd1ac ... > 0.68 │ffff000└──3cd16c: ↑ cbnz w0, ffff0000083cd120 > Simplified, before this patch: > > │ ↑ cbz 40 ▒ > ... > 4.44 │ │↓ tbnz w0, #26, ffff0000083cd190 ... > 1.37 │ │↓ tbnz w22, #5, ffff0000083cd1a4 │ │ mov w19, #0x20000 // #131072 ▒ > 1.02 │ │↓ tbz w22, #2, ffff0000083cd1ac ... > 0.68 │ └──cbnz 60 ▒ > > the cbz operand is missing, and the tbz doesn't get simplified processing > at all because the address-get function failed to match an address. > > Simplified, After this patch applied: > > │ ↑ cbz w0, 40 ▒ > ... > 4.44 │ │↓ tbnz w0, #26, d0 ▒ > ... > 1.37 │ │↓ tbnz w22, #5, e4 ▒ > │ │ mov w19, #0x20000 // #131072 ▒ > 1.02 │ │↓ tbz w22, #2, ec ▒ > ... > 0.68 │ └──cbnz w0, 60 ▒ > > Signed-off-by: Kim Phillips > Reported-by: Robin Murphy > Cc: Mark Rutland > --- > > Sorry if any confusion: I thought it easier to merge the changes into > one patch and resubmit it. The only patch to apply is this one; I > tested on powerpc and x86_64 also, and they still work as with Ravi's > original patch (this one just adds the ARM fixes, and slightly > optimizes Ravi's original patch). Humm, authorship info really gests confusing, can't you just have one commit log, combining the original one with what you did, and attribute the patch to you and have a: [acme@jouet linux]$ git log | grep -i originally-by: | wc -l 58 [acme@jouet linux]$ Originally-by: Ravi Bangoria Ravi? I'm trying to catch up on my patch queue, so haven't read this thoroughly to have an idea if this is fair or OK, can you guys comment on it? - Arnaldo > tools/perf/util/annotate.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 683f8340460c..3174930e7cea 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -239,10 +239,20 @@ static int jump__parse(struct arch *arch __maybe_unused, struct ins_operands *op > const char *s = strchr(ops->raw, '+'); > const char *c = strchr(ops->raw, ','); > > - if (c++ != NULL) > + /* > + * skip over possible up to 2 operands to get to address, e.g.: > + * tbnz w0, #26, ffff0000083cd190 > + */ > + if (c++ != NULL) { > ops->target.addr = strtoull(c, NULL, 16); > - else > + if (!ops->target.addr) { > + c = strchr(c, ','); > + if (c++ != NULL) > + ops->target.addr = strtoull(c, NULL, 16); > + } > + } else { > ops->target.addr = strtoull(ops->raw, NULL, 16); > + } > > if (s++ != NULL) { > ops->target.offset = strtoull(s, NULL, 16); > @@ -257,10 +267,27 @@ static int jump__parse(struct arch *arch __maybe_unused, struct ins_operands *op > static int jump__scnprintf(struct ins *ins, char *bf, size_t size, > struct ins_operands *ops) > { > + const char *c = strchr(ops->raw, ','); > + > if (!ops->target.addr || ops->target.offset < 0) > return ins__raw_scnprintf(ins, bf, size, ops); > > - return scnprintf(bf, size, "%-6.6s %" PRIx64, ins->name, ops->target.offset); > + if (c != NULL) { > + const char *c2 = strchr(c + 1, ','); > + > + /* check for 3-op insn */ > + if (c2 != NULL) > + c = c2; > + c++; > + > + /* mirror arch objdump's space-after-comma style */ > + if (*c == ' ') > + c++; > + } > + > + return scnprintf(bf, size, "%-6.6s %.*s%" PRIx64, > + ins->name, c ? c - ops->raw : 0, ops->raw, > + ops->target.offset); > } > > static struct ins_ops jump_ops = { > -- > 2.11.0