* [PATCH] perf/annotate/powerpc: Fix branch instruction with multiple operands @ 2017-05-25 5:59 Ravi Bangoria 2017-05-26 23:23 ` [PATCH] perf/annotate: " Kim Phillips 0 siblings, 1 reply; 9+ messages in thread From: Ravi Bangoria @ 2017-05-25 5:59 UTC (permalink / raw) To: acme Cc: anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, kim.phillips, 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 <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@samba.org> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> --- tools/perf/util/annotate.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 683f834..a031c4d 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -257,10 +257,18 @@ 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) + return scnprintf(bf, size, "%-6.6s %.*s%" PRIx64, + ins->name, c - ops->raw, ops->raw, + ops->target.offset); + else + return scnprintf(bf, size, "%-6.6s %" PRIx64, + ins->name, ops->target.offset); } static struct ins_ops jump_ops = { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] perf/annotate: Fix branch instruction with multiple operands 2017-05-25 5:59 [PATCH] perf/annotate/powerpc: Fix branch instruction with multiple operands Ravi Bangoria @ 2017-05-26 23:23 ` Kim Phillips 2017-06-01 13:09 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 9+ messages in thread From: Kim Phillips @ 2017-05-26 23:23 UTC (permalink / raw) To: Ravi Bangoria, acme Cc: anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland From: Ravi Bangoria <ravi.bangoria@linux.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@samba.org> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.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@arm.com> Reported-by: Robin Murphy <robin.murphy@arm.com> Cc: Mark Rutland <mark.rutland@arm.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). 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] perf/annotate: Fix branch instruction with multiple operands 2017-05-26 23:23 ` [PATCH] perf/annotate: " Kim Phillips @ 2017-06-01 13:09 ` Arnaldo Carvalho de Melo 2017-06-01 13:47 ` Ravi Bangoria 2017-06-01 14:29 ` [PATCH v2] " Kim Phillips 0 siblings, 2 replies; 9+ messages in thread From: Arnaldo Carvalho de Melo @ 2017-06-01 13:09 UTC (permalink / raw) To: Kim Phillips Cc: Ravi Bangoria, anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland Em Fri, May 26, 2017 at 06:23:10PM -0500, Kim Phillips escreveu: > From: Ravi Bangoria <ravi.bangoria@linux.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@samba.org> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.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@arm.com> > Reported-by: Robin Murphy <robin.murphy@arm.com> > Cc: Mark Rutland <mark.rutland@arm.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@jouet linux]$ git log | grep -i originally-by: | wc -l 58 [acme@jouet linux]$ Originally-by: Ravi Bangoria <ravi.bangoria@linux.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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf/annotate: Fix branch instruction with multiple operands 2017-06-01 13:09 ` Arnaldo Carvalho de Melo @ 2017-06-01 13:47 ` Ravi Bangoria 2017-06-01 14:29 ` [PATCH v2] " Kim Phillips 1 sibling, 0 replies; 9+ messages in thread From: Ravi Bangoria @ 2017-06-01 13:47 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Kim Phillips, anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland, Ravi Bangoria Thanks Arnaldo, On Thursday 01 June 2017 06:39 PM, Arnaldo Carvalho de Melo wrote: > 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.bangoria@linux.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? Yes, I've tested Kim's patch on powerpc and it works for me. -Ravi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] perf/annotate: Fix branch instruction with multiple operands 2017-06-01 13:09 ` Arnaldo Carvalho de Melo 2017-06-01 13:47 ` Ravi Bangoria @ 2017-06-01 14:29 ` Kim Phillips 2017-06-01 15:03 ` Ravi Bangoria 2017-06-07 15:57 ` [tip:perf/urgent] perf annotate: " tip-bot for Kim Phillips 1 sibling, 2 replies; 9+ messages in thread From: Kim Phillips @ 2017-06-01 14:29 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ravi Bangoria, anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland Perf annotate is dropping the cr* fields from branch instructions. Fix it by adding support to display branch instructions having multiple operands. Power Arch 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 Power Arch 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 Power Arch 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 Also support AArch64 conditional branch instructions, which can have up to three operands: Aarch64 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▒ Aarch64 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 parsing function failed to match an address. Aarch64 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 Reported-by: Anton Blanchard <anton@samba.org> Reported-by: Robin Murphy <robin.murphy@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Originally-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> Signed-off-by: Kim Phillips <kim.phillips@arm.com> --- v2: addressing Arnaldo's clean-this-up comment: merged commit texts between Ravi's original, author changed in lieu of adding something I hadn't known nor thought about possibly doing: Originally-by: Ravi. Ravi, if that's not OK, provide an alternative means of keeping things clean for Arnaldo, thanks. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] perf/annotate: Fix branch instruction with multiple operands 2017-06-01 14:29 ` [PATCH v2] " Kim Phillips @ 2017-06-01 15:03 ` Ravi Bangoria 2017-06-01 16:42 ` Arnaldo Carvalho de Melo 2017-06-01 17:50 ` Arnaldo Carvalho de Melo 2017-06-07 15:57 ` [tip:perf/urgent] perf annotate: " tip-bot for Kim Phillips 1 sibling, 2 replies; 9+ messages in thread From: Ravi Bangoria @ 2017-06-01 15:03 UTC (permalink / raw) To: Kim Phillips, Arnaldo Carvalho de Melo Cc: anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland, Ravi Bangoria Hi Kim, Arnaldo, On Thursday 01 June 2017 07:59 PM, Kim Phillips wrote: > v2: addressing Arnaldo's clean-this-up comment: merged commit texts > between Ravi's original, author changed in lieu of adding something I > hadn't known nor thought about possibly doing: Originally-by: Ravi. > Ravi, if that's not OK, provide an alternative means of keeping things > clean for Arnaldo, thanks. I'm fine :) No issues. Thanks, Ravi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] perf/annotate: Fix branch instruction with multiple operands 2017-06-01 15:03 ` Ravi Bangoria @ 2017-06-01 16:42 ` Arnaldo Carvalho de Melo 2017-06-01 17:50 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 9+ messages in thread From: Arnaldo Carvalho de Melo @ 2017-06-01 16:42 UTC (permalink / raw) To: Ravi Bangoria Cc: Kim Phillips, anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland Em Thu, Jun 01, 2017 at 08:33:43PM +0530, Ravi Bangoria escreveu: > Hi Kim, Arnaldo, > > On Thursday 01 June 2017 07:59 PM, Kim Phillips wrote: > > v2: addressing Arnaldo's clean-this-up comment: merged commit texts > > between Ravi's original, author changed in lieu of adding something I > > hadn't known nor thought about possibly doing: Originally-by: Ravi. > > Ravi, if that's not OK, provide an alternative means of keeping things > > clean for Arnaldo, thanks. > > I'm fine :) No issues. Thanks for checking. - Arnaldo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] perf/annotate: Fix branch instruction with multiple operands 2017-06-01 15:03 ` Ravi Bangoria 2017-06-01 16:42 ` Arnaldo Carvalho de Melo @ 2017-06-01 17:50 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 9+ messages in thread From: Arnaldo Carvalho de Melo @ 2017-06-01 17:50 UTC (permalink / raw) To: Ravi Bangoria Cc: Kim Phillips, anton, peterz, mingo, alexander.shishkin, treeze.taeung, borntraeger, linux-kernel, Robin Murphy, Mark Rutland Em Thu, Jun 01, 2017 at 08:33:43PM +0530, Ravi Bangoria escreveu: > Hi Kim, Arnaldo, > > On Thursday 01 June 2017 07:59 PM, Kim Phillips wrote: > > v2: addressing Arnaldo's clean-this-up comment: merged commit texts > > between Ravi's original, author changed in lieu of adding something I > > hadn't known nor thought about possibly doing: Originally-by: Ravi. > > Ravi, if that's not OK, provide an alternative means of keeping things > > clean for Arnaldo, thanks. > > I'm fine :) No issues. Thanks, applied to perf/urgent. - Arnaldo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:perf/urgent] perf annotate: Fix branch instruction with multiple operands 2017-06-01 14:29 ` [PATCH v2] " Kim Phillips 2017-06-01 15:03 ` Ravi Bangoria @ 2017-06-07 15:57 ` tip-bot for Kim Phillips 1 sibling, 0 replies; 9+ messages in thread From: tip-bot for Kim Phillips @ 2017-06-07 15:57 UTC (permalink / raw) To: linux-tip-commits Cc: alexander.shishkin, ravi.bangoria, hpa, acme, mingo, anton, treeze.taeung, peterz, borntraeger, kim.phillips, mark.rutland, robin.murphy, linux-kernel, tglx Commit-ID: b13bbeee5ee606cfb57ddcf47e66802f9aa7273e Gitweb: http://git.kernel.org/tip/b13bbeee5ee606cfb57ddcf47e66802f9aa7273e Author: Kim Phillips <kim.phillips@arm.com> AuthorDate: Thu, 1 Jun 2017 09:29:59 -0500 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 1 Jun 2017 14:48:36 -0300 perf annotate: Fix branch instruction with multiple operands 'perf annotate' is dropping the cr* fields from branch instructions. Fix it by adding support to display branch instructions having multiple operands. Power Arch 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 Power Arch 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 Power Arch 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 Also support AArch64 conditional branch instructions, which can have up to three operands: Aarch64 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▒ Aarch64 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 parsing function failed to match an address. Aarch64 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 Originally-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> Tested-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> Reported-by: Anton Blanchard <anton@samba.org> Reported-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Kim Phillips <kim.phillips@arm.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Taeung Song <treeze.taeung@gmail.com> Link: http://lkml.kernel.org/r/20170601092959.f60d98912e8a1b66fd1e4c0e@arm.com Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- 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 07d5608..1367d7e 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 = { ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-07 16:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-25 5:59 [PATCH] perf/annotate/powerpc: Fix branch instruction with multiple operands Ravi Bangoria 2017-05-26 23:23 ` [PATCH] perf/annotate: " Kim Phillips 2017-06-01 13:09 ` Arnaldo Carvalho de Melo 2017-06-01 13:47 ` Ravi Bangoria 2017-06-01 14:29 ` [PATCH v2] " Kim Phillips 2017-06-01 15:03 ` Ravi Bangoria 2017-06-01 16:42 ` Arnaldo Carvalho de Melo 2017-06-01 17:50 ` Arnaldo Carvalho de Melo 2017-06-07 15:57 ` [tip:perf/urgent] perf annotate: " tip-bot for Kim Phillips
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.