| 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
| ||
|
Message-ID: <20170601130956.GC2899@kernel.org>
Date: Thu, 1 Jun 2017 10:09:56 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Kim Phillips <kim.phillips@....com>
Cc: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>, anton@...ba.org,
peterz@...radead.org, mingo@...hat.com,
alexander.shishkin@...ux.intel.com, treeze.taeung@...il.com,
borntraeger@...ibm.com, linux-kernel@...r.kernel.org,
Robin Murphy <robin.murphy@....com>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] perf/annotate: Fix branch instruction with multiple
operands
Em Fri, May 26, 2017 at 06:23:10PM -0500, Kim Phillips escreveu:
> From: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
>
> 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 <int_sqrt+0x40>
> 1.82 | c0000000004d269c: mr r3,r10
> 29.18 | c0000000004d26a0: mr r10,r8
> | c0000000004d26a4: v bgt cr7,c0000000004d26ac <int_sqrt+0x4c>
> | 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 <anton@...ba.org>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
>
> Reduced to keep only one scnprintf and supplemented for AArch64
> conditional branch instructions:
>
> Non-simplified (raw objdump) view:
>
> │ffff0000083cd11c: ↑ cbz w0, ffff0000083cd100 <security_fil▒
> ...
> 4.44 │ffff000│083cd134: ↓ tbnz w0, #26, ffff0000083cd190 <securit▒
> ...
> 1.37 │ffff000│083cd144: ↓ tbnz w22, #5, ffff0000083cd1a4 <securit▒
> │ffff000│083cd148: mov w19, #0x20000 //▒
> 1.02 │ffff000│083cd14c: ↓ tbz w22, #2, ffff0000083cd1ac <securit▒
> ...
> 0.68 │ffff000└──3cd16c: ↑ cbnz w0, ffff0000083cd120 <security_fil▒
>
> Simplified, before this patch:
>
> │ ↑ cbz 40 ▒
> ...
> 4.44 │ │↓ tbnz w0, #26, ffff0000083cd190 <security_file_permiss▒
> ...
> 1.37 │ │↓ tbnz w22, #5, ffff0000083cd1a4 <security_file_permiss▒
> │ │ mov w19, #0x20000 // #131072 ▒
> 1.02 │ │↓ tbz w22, #2, ffff0000083cd1ac <security_file_permiss▒
> ...
> 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 <kim.phillips@....com>
> Reported-by: Robin Murphy <robin.murphy@....com>
> Cc: Mark Rutland <mark.rutland@....com>
> ---
>
> 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@...et linux]$ git log | grep -i originally-by: | wc -l
58
[acme@...et linux]$
Originally-by: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
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 <security_file_permission+0xd0>
> + */
> + 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
Powered by blists - more mailing lists