Message-Id: <918E5168-1EA0-4A5F-81BB-0A7321E18274@linux.vnet.ibm.com>
Date: Fri, 26 Jul 2024 18:05:37 +0530
From: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
        Ian Rogers <irogers@...gle.com>, Namhyung Kim <namhyung@...nel.org>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>, akanksha@...ux.ibm.com,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Hari Bathini <hbathini@...ux.ibm.com>,
        Disha Goel <disgoel@...ux.vnet.ibm.com>
Subject: Re: [PATCH V8 14/15] tools/perf: Add support to use libcapstone in
 powerpc



> On 25 Jul 2024, at 2:30 AM, Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> 
> 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?
> 

Hi Arnaldo

Yes, I did compile with and without capstone devel support also.
But didn’t hit the compilation issue. 

...                             libcapstone: [ OFF ]
  LINK    perf


...                             libcapstone: [ on  ]
  LINK    perf


> 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.

Ok Arnaldo, Sure, I will ensure to follow this every time

Thanks,
Athira
> 
> 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