Message-ID: <ZqFrhCguUd_8n-xa@x1>
Date: Wed, 24 Jul 2024 18:00:52 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Cc: jolsa@...nel.org, adrian.hunter@...el.com, irogers@...gle.com,
	namhyung@...nel.org, segher@...nel.crashing.org,
	christophe.leroy@...roup.eu, linux-kernel@...r.kernel.org,
	linux-perf-users@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
	akanksha@...ux.ibm.com, maddy@...ux.ibm.com, kjain@...ux.ibm.com,
	hbathini@...ux.ibm.com, disgoel@...ux.vnet.ibm.com
Subject: Re: [PATCH V8 14/15] tools/perf: Add support to use libcapstone in
 powerpc

On Thu, Jul 18, 2024 at 02:13:57PM +0530, Athira Rajeev wrote:
> Now perf uses the capstone library to disassemble the instructions in
> x86. capstone is used (if available) for perf annotate to speed up.
> Currently it only supports x86 architecture. Patch includes changes to
> enable this in powerpc. For now, only for data type sort keys, this
> method is used and only binary code (raw instruction) is read. This is
> because powerpc approach to understand instructions and reg fields uses
> raw instruction. The "cs_disasm" is currently not enabled. While
> attempting to do cs_disasm, observation is that some of the instructions
> were not identified (ex: extswsli, maddld) and it had to fallback to use
> objdump. Hence enabling "cs_disasm" is added in comment section as a
> TODO for powerpc.
> 
> Reviewed-and-tested-by: Kajol Jain <kjain@...ux.ibm.com>
> Reviewed-by: Namhyung Kim <namhyung@...nel.org>
> Signed-off-by: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>

Is this building on ppc?

Here, when building on a x86-64 machine with:

ake -C tools/perf EXTRA_CFLAGS=-DREFCNT_CHECKING=1 O=/tmp/build/perf-tools-next/

That I got to by the usual:

make -C tools/perf build-test

I get:

util/disasm.c: In function ‘symbol__disassemble_capstone_powerpc’:
util/disasm.c:1618:34: error: ‘struct dso’ has no member named ‘nsinfo’
 1618 |         nsinfo__mountns_enter(dso->nsinfo, &nsc);
      |                                  ^~
make[4]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: /tmp/build/perf-tools-next/util/disasm.o] Error 1
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:158: util] Error 2
make[2]: *** [Makefile.perf:762: /tmp/build/perf-tools-next/perf-util-in.o] Error 2
make[1]: *** [Makefile.perf:265: sub-make] Error 2
make: *** [Makefile:70: all] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
⬢[acme@...lbox perf-tools-next]$

And then when looking at this changeset I noticed that you added a
function and then only called it if:

 static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
                                        struct annotate_args *args)
 {
@@ -1945,6 +2083,11 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
                        err = symbol__disassemble_raw(symfs_filename, sym, args);
                        if (err == 0)
                                goto out_remove_tmp;
+#ifdef HAVE_LIBCAPSTONE_SUPPORT
+                       err = symbol__disassemble_capstone_powerpc(symfs_filename, sym, args);
+                       if (err == 0)
+                               goto out_remove_tmp;
+#endif
                }
        }

but the symbol__disassemble_capstone_powerpc() is being unconditionally
built, i.e. it is not surrounded by '#ifdef HAVE_LIBCAPSTONE_SUPPORT'

And then there:

dso->nsinfo

It should have been:

⬢[acme@...lbox perf-tools-next]$ git diff
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 2a1c657d6aa66cc2..99081f37c5daba60 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1615,7 +1615,7 @@ static int symbol__disassemble_capstone_powerpc(char *filename, struct symbol *s
        if (args->options->objdump_path)
                return -1;
 
-       nsinfo__mountns_enter(dso->nsinfo, &nsc);
+       nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
        fd = open(filename, O_RDONLY);
        nsinfo__mountns_exit(&nsc);
        if (fd < 0)
⬢[acme@...lbox perf-tools-next]$

So it is critical that you try using:

make -C tools/perf build-test

Before asking for review.

Please do that next time.

I'm fixing this up this time so that we can make progress.

Best regards,

- Arnaldo


> ---
>  tools/perf/util/disasm.c | 143 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 insertions(+)
> 
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index a848e6f5f05a..63681df6482b 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1585,6 +1585,144 @@ static void print_capstone_detail(cs_insn *insn, char *buf, size_t len,
>  	}
>  }
>  
> +static int symbol__disassemble_capstone_powerpc(char *filename, struct symbol *sym,
> +					struct annotate_args *args)
> +{
> +	struct annotation *notes = symbol__annotation(sym);
> +	struct map *map = args->ms.map;
> +	struct dso *dso = map__dso(map);
> +	struct nscookie nsc;
> +	u64 start = map__rip_2objdump(map, sym->start);
> +	u64 end = map__rip_2objdump(map, sym->end);
> +	u64 len = end - start;
> +	u64 offset;
> +	int i, fd, count;
> +	bool is_64bit = false;
> +	bool needs_cs_close = false;
> +	u8 *buf = NULL;
> +	struct find_file_offset_data data = {
> +		.ip = start,
> +	};
> +	csh handle;
> +	char disasm_buf[512];
> +	struct disasm_line *dl;
> +	u32 *line;
> +	bool disassembler_style = false;
> +
> +	if (args->options->objdump_path)
> +		return -1;
> +
> +	nsinfo__mountns_enter(dso->nsinfo, &nsc);
> +	fd = open(filename, O_RDONLY);
> +	nsinfo__mountns_exit(&nsc);
> +	if (fd < 0)
> +		return -1;
> +
> +	if (file__read_maps(fd, /*exe=*/true, find_file_offset, &data,
> +			    &is_64bit) == 0)
> +		goto err;
> +
> +	if (!args->options->disassembler_style ||
> +			!strcmp(args->options->disassembler_style, "att"))
> +		disassembler_style = true;
> +
> +	if (capstone_init(maps__machine(args->ms.maps), &handle, is_64bit, disassembler_style) < 0)
> +		goto err;
> +
> +	needs_cs_close = true;
> +
> +	buf = malloc(len);
> +	if (buf == NULL)
> +		goto err;
> +
> +	count = pread(fd, buf, len, data.offset);
> +	close(fd);
> +	fd = -1;
> +
> +	if ((u64)count != len)
> +		goto err;
> +
> +	line = (u32 *)buf;
> +
> +	/* add the function address and name */
> +	scnprintf(disasm_buf, sizeof(disasm_buf), "%#"PRIx64" <%s>:",
> +		  start, sym->name);
> +
> +	args->offset = -1;
> +	args->line = disasm_buf;
> +	args->line_nr = 0;
> +	args->fileloc = NULL;
> +	args->ms.sym = sym;
> +
> +	dl = disasm_line__new(args);
> +	if (dl == NULL)
> +		goto err;
> +
> +	annotation_line__add(&dl->al, &notes->src->source);
> +
> +	/*
> +	 * TODO: enable disassm for powerpc
> +	 * count = cs_disasm(handle, buf, len, start, len, &insn);
> +	 *
> +	 * For now, only binary code is saved in disassembled line
> +	 * to be used in "type" and "typeoff" sort keys. Each raw code
> +	 * is 32 bit instruction. So use "len/4" to get the number of
> +	 * entries.
> +	 */
> +	count = len/4;
> +
> +	for (i = 0, offset = 0; i < count; i++) {
> +		args->offset = offset;
> +		sprintf(args->line, "%x", line[i]);
> +
> +		dl = disasm_line__new(args);
> +		if (dl == NULL)
> +			goto err;
> +
> +		annotation_line__add(&dl->al, &notes->src->source);
> +
> +		offset += 4;
> +	}
> +
> +	/* It failed in the middle */
> +	if (offset != len) {
> +		struct list_head *list = &notes->src->source;
> +
> +		/* Discard all lines and fallback to objdump */
> +		while (!list_empty(list)) {
> +			dl = list_first_entry(list, struct disasm_line, al.node);
> +
> +			list_del_init(&dl->al.node);
> +			disasm_line__free(dl);
> +		}
> +		count = -1;
> +	}
> +
> +out:
> +	if (needs_cs_close)
> +		cs_close(&handle);
> +	free(buf);
> +	return count < 0 ? count : 0;
> +
> +err:
> +	if (fd >= 0)
> +		close(fd);
> +	if (needs_cs_close) {
> +		struct disasm_line *tmp;
> +
> +		/*
> +		 * It probably failed in the middle of the above loop.
> +		 * Release any resources it might add.
> +		 */
> +		list_for_each_entry_safe(dl, tmp, &notes->src->source, al.node) {
> +			list_del(&dl->al.node);
> +			free(dl);
> +		}
> +	}
> +	count = -1;
> +	goto out;
> +}
> +
>  static int symbol__disassemble_capstone(char *filename, struct symbol *sym,
>  					struct annotate_args *args)
>  {
> @@ -1942,6 +2080,11 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>  			err = symbol__disassemble_raw(symfs_filename, sym, args);
>  			if (err == 0)
>  				goto out_remove_tmp;
> +#ifdef HAVE_LIBCAPSTONE_SUPPORT
> +			err = symbol__disassemble_capstone_powerpc(symfs_filename, sym, args);
> +			if (err == 0)
> +				goto out_remove_tmp;
> +#endif
>  		}
>  	}
>  
> -- 
> 2.43.0

Powered by blists - more mailing lists