Re: [PATCH] perf/annotate: Fix branch instruction with multiple operands

[Arnaldo Carvalho de Melo]

Em Fri, May 26, 2017 at 06:23:10PM -0500, Kim Phillips escreveu:
> From: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxx>
> Reported-by: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> ---
> 
> 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@xxxxxxxxxxxxxxxxxx>

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