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 
> >> 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 
> >> Tested-by: Christian Borntraeger 
> >> Reviewed-by: Hendrik Brueckner 
> >> ---
> >>  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, , 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, , 16);
> >> +  else
> >> +  ops->target.addr = 0;
> >> +  } else
> >> +  ops->target.addr = strtoull(ops->raw, , 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 majord...@vger.kernel.org
> > 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

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 
> >> 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 
> >> Tested-by: Christian Borntraeger 
> >> Reviewed-by: Hendrik Brueckner 
> >> ---
> >>  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, , 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, , 16);
> >> +  else
> >> +  ops->target.addr = 0;
> >> +  } else
> >> +  ops->target.addr = strtoull(ops->raw, , 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 majord...@vger.kernel.org
> > 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

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

[Thomas-Mich Richter]

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 
>> 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 
>> Tested-by: Christian Borntraeger 
>> Reviewed-by: Hendrik Brueckner 
>> ---
>>  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, , 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, , 16);
>> +else
>> +ops->target.addr = 0;
>> +} else
>> +ops->target.addr = strtoull(ops->raw, , 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.

>> -- 
>> 2.14.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majord...@vger.kernel.org
> 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

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

[Thomas-Mich Richter]

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 
>> 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 
>> Tested-by: Christian Borntraeger 
>> Reviewed-by: Hendrik Brueckner 
>> ---
>>  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, , 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, , 16);
>> +else
>> +ops->target.addr = 0;
>> +} else
>> +ops->target.addr = strtoull(ops->raw, , 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.

>> -- 
>> 2.14.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majord...@vger.kernel.org
> 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

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

[Arnaldo Carvalho de Melo]

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 
> 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 
> Tested-by: Christian Borntraeger 
> Reviewed-by: Hendrik Brueckner 
> ---
>  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, , 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, , 16);
> + else
> + ops->target.addr = 0;
> + } else
> + ops->target.addr = strtoull(ops->raw, , 16);

Do we have to do this here? Can't we have a s390_call__parse() and set
that in the s/390 instructions?

We should avoid arch specific stuff in the common code, and we could
even have a call_reg__parse() perhaps? One that knows that the first
operand is a reg and it should _always_ skip te first one, be it on
s/390 or elsewhere?

- Arnaldo
  
>   name = strchr(endptr, '<');
>   if (name == NULL)
> -- 
> 2.14.3

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

[Arnaldo Carvalho de Melo]

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 
> 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 
> Tested-by: Christian Borntraeger 
> Reviewed-by: Hendrik Brueckner 
> ---
>  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, , 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, , 16);
> + else
> + ops->target.addr = 0;
> + } else
> + ops->target.addr = strtoull(ops->raw, , 16);

Do we have to do this here? Can't we have a s390_call__parse() and set
that in the s/390 instructions?

We should avoid arch specific stuff in the common code, and we could
even have a call_reg__parse() perhaps? One that knows that the first
operand is a reg and it should _always_ skip te first one, be it on
s/390 or elsewhere?

- Arnaldo
  
>   name = strchr(endptr, '<');
>   if (name == NULL)
> -- 
> 2.14.3