Re: [PATCH Resent] perf annotate: Fix s390 target function disassembly

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 03:52:35PM +0100, Thomas-Mich Richter escreveu:
> On 03/06/2018 03:04 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 06, 2018 at 01:39:55PM +0100, Thomas Richter escreveu:
> >> Perf annotate displays function call assembler instructions
> >> with a right arrow. Hitting enter on this line/instruction
> >> causes the browser to disassemble this target function and
> >> show it on the screen.  On s390 this results in an error
> >> message 'The called function was not found.'
> >>
> >> The function call assembly line parsing does not handle
> >> the s390 bras and brasl instructions. Function call__parse
> >> expects the target as first operand:
> >> 	callq	e9140 <__fxstat>
> >> S390 has a register number as first operand:
> >> 	brasl	%r14,41d60 <abort>
> >> Therefore the target addresses on s390 are always zero
> >> which is an invalid address.
> >>
> >> Fix this by skipping the first operand on s390.
> >>
> >> Signed-off-by: Thomas Richter <tmricht@xxxxxxxxxxxxxxxxxx>
> >> Tested-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> >> Reviewed-by: Hendrik Brueckner <brueckner@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>  tools/perf/util/annotate.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> >> index 49ff825f745c..feb6006b676d 100644
> >> --- a/tools/perf/util/annotate.c
> >> +++ b/tools/perf/util/annotate.c
> >> @@ -192,6 +192,14 @@ static int call__parse(struct arch *arch, struct ins_operands *ops, struct map *
> >>  	};
> >>  
> >>  	ops->target.addr = strtoull(ops->raw, &endptr, 16);
> >> +	if (!strcmp(arch->name, "s390")) {
> >> +		/* s390 function call 1st operand is register */
> >> +		tok = strchr(ops->raw, ',');
> >> +		if (tok)
> >> +			ops->target.addr = strtoull(tok + 1, &endptr, 16);
> >> +		else
> >> +			ops->target.addr = 0;
> >> +	} else
> >> +		ops->target.addr = strtoull(ops->raw, &endptr, 16);
> > 
> > Do we have to do this here? Can't we have a s390_call__parse() and set
> > that in the s/390 instructions?
> 
> 
> Originally I wanted to add an architecture specific weak function to handle s390
> This did not work because file util/annotate.c includes the architecture specific versions
> around line 100:
> 
> #include "arch/arm/annotate/instructions.c"
> #include "arch/arm64/annotate/instructions.c"
> #include "arch/x86/annotate/instructions.c"
> #include "arch/powerpc/annotate/instructions.c"
> #include "arch/s390/annotate/instructions.c"
> 
> This includes the C file for s390 so we end up having function call__parse defined twice,
> one with a weak definition and one without which results in a compiler error.
> 
> I will use  a s390 specific call__parse function and sent another patch.

So, in tools/perf/arch/s390/annotate/instructions.c you have that
function s390__associate_ins_ops() where it sets the handler for the
"brasl" asm instruction to be 'call__ops', what I am proposing is that
instead you provide a brasl__ops and use it, this ops knows that it is a
'call', but the target is not where call__ops expects it to be, thus we
have a separate ops for that, got it?

- Arnaldo
 
> >> -- 
> >> 2.14.3
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> -- 
> Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
> --
> Vorsitzende des Aufsichtsrats: Martina Koederitz 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294