Re: [PATCH v5 3/5] perf: script: add field 'disasm' to display mnemonic instructions

[Changbin Du <changbin.du@huawei.com>]

On Mon, Feb 05, 2024 at 03:10:58PM +0200, Adrian Hunter wrote:
> On 5/02/24 14:19, Changbin Du wrote:
> > On Mon, Feb 05, 2024 at 11:23:21AM +0200, Adrian Hunter wrote:
> >>>  struct perf_script {
> >>> @@ -190,6 +191,7 @@ struct output_option {
> >>>  	{.str = "bpf-output",   .field = PERF_OUTPUT_BPF_OUTPUT},
> >>>  	{.str = "callindent", .field = PERF_OUTPUT_CALLINDENT},
> >>>  	{.str = "insn", .field = PERF_OUTPUT_INSN},
> >>> +	{.str = "disasm", .field = PERF_OUTPUT_DISASM},
> >>>  	{.str = "insnlen", .field = PERF_OUTPUT_INSNLEN},
> >>>  	{.str = "brstackinsn", .field = PERF_OUTPUT_BRSTACKINSN},
> >>>  	{.str = "brstackoff", .field = PERF_OUTPUT_BRSTACKOFF},
> >>> @@ -1515,6 +1517,10 @@ static int perf_sample__fprintf_insn(struct perf_sample *sample,
> >>>  		printed += fprintf(fp, " insn: ");
> >>>  		printed += sample__fprintf_insn_raw(sample, fp);
> >>>  	}
> >>> +	if (PRINT_FIELD(DISASM) && sample->insn_len) {
> >>> +		printed += fprintf(fp, "\t\t");
> >>
> >> This is good, except if both 'insn' and 'disasm' are used together.
> >> It either:
> >>  a) without libcapstone, prints insn bytes twice
> >>
> >> 	Probably simpler to make 'disasm' without libcapstone
> >> 	a fatal error explaining that perf needs to be built
> >> 	with libcapstone support for 'disasm' to work.
> >>
> > Instead of fatal error, I print a warning message for this. Because
> > perf_sample__fprintf_insn() cannot return negtive error number.
> 
> It could be validated in advance, perhaps in parse_output_fields().
>
I did as below:

--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3112,6 +3112,13 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
                        rc = -EINVAL;
                        goto out;
                }
+#ifndef HAVE_LIBCAPSTONE_SUPPORT
+               if (change != REMOVE && strcmp(tok, "disasm") == 0) {
+                       fprintf(stderr, "Field \"disasm\" requires perf to be built with libcapstone support.\n");
+                       rc = -EINVAL;
+                       goto out;
+               }
+#endif

Then,
$ sudo perf script -F +disasm
Field "disasm" requires perf to be built with libcapstone support.

 Usage: perf script [<options>]
 ...

> > 
> >>  b) with libcapstone, disassembly does not line up nicely
> >>
> >  
> > 
> 

-- 
Cheers,
Changbin Du