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

From: Arnaldo Carvalho de Melo
Date: Wed Mar 07 2018 - 09:43:35 EST


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@xxxxxxxxxxxxxxxxxx>
> ---
> 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