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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Mar 07, 2018 at 02:43:25PM +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.
> 
> Introduce a s390 specific call parsing function which skips
> the first operand on s390.
> 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> ---
>  tools/perf/arch/s390/annotate/instructions.c | 53 +++++++++++++++++++++++++++-
>  tools/perf/util/annotate.c                   |  2 +-
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/s390/annotate/instructions.c b/tools/perf/arch/s390/annotate/instructions.c
> index 8c72b44444cb..0a3e39b5143d 100644
> --- a/tools/perf/arch/s390/annotate/instructions.c
> +++ b/tools/perf/arch/s390/annotate/instructions.c
> @@ -1,6 +1,57 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/compiler.h>
>  
> +static int s390_call__parse(struct arch *arch, struct ins_operands *ops,
> +			    struct map *map)
> +{
> +	char *endptr, *tok, *name;
> +	struct addr_map_symbol target = {
> +		.map = map,
> +	};
> +
> +	tok = strchr(ops->raw, ',');
> +	if (!tok)
> +		return -1;
> +
> +	ops->target.addr = strtoull(tok + 1, &endptr, 16);
> +
> +	name = strchr(endptr, '<');
> +	if (name == NULL)
> +		return -1;
> +
> +	name++;
> +
> +	if (arch->objdump.skip_functions_char &&
> +	    strchr(name, arch->objdump.skip_functions_char))
> +		return -1;
> +
> +	tok = strchr(name, '>');
> +	if (tok == NULL)
> +		return -1;
> +
> +	*tok = '\0';
> +	ops->target.name = strdup(name);
> +	*tok = '>';
> +
> +	if (ops->target.name == NULL)
> +		return -1;
> +	target.addr = map__objdump_2mem(map, ops->target.addr);
> +
> +	if (map_groups__find_ams(&target) == 0 &&
> +	    map__rip_2objdump(target.map, map->map_ip(target.map, target.addr)) == ops->target.addr)
> +		ops->target.sym = target.sym;
> +
> +	return 0;
> +}
> +
> +static int call__scnprintf(struct ins *ins, char *bf, size_t size,
> +			   struct ins_operands *ops);
> +
> +static struct ins_ops s390_call_ops = {
> +	.parse	   = s390_call__parse,
> +	.scnprintf = call__scnprintf,
> +};
> +
>  static struct ins_ops *s390__associate_ins_ops(struct arch *arch, const char *name)
>  {
>  	struct ins_ops *ops = NULL;
> @@ -14,7 +65,7 @@ static struct ins_ops *s390__associate_ins_ops(struct arch *arch, const char *na
>  	if (!strcmp(name, "bras") ||
>  	    !strcmp(name, "brasl") ||
>  	    !strcmp(name, "basr"))
> -		ops = &call_ops;
> +		ops = &s390_call_ops;
>  	if (!strcmp(name, "br"))
>  		ops = &ret_ops;
>  
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 49ff825f745c..bc3302da702b 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -248,7 +248,7 @@ static struct ins_ops call_ops = {
>  
>  bool ins__is_call(const struct ins *ins)
>  {
> -	return ins->ops == &call_ops;
> +	return ins->ops == &call_ops || ins->ops == &s390_call_ops;

This is good enough so far, but I think we may end up just making this
be:

bool ins__is_call(const struct ins *ins)
{
	return ins->ops->is_call;
}

This way we remove this arch specific stuff from the core annotate.c
code, applying this patch, this can be done on top, later.

- Arnaldo