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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240206020056.tpy2c6ycok4podjd@M910t>
Date: Tue, 6 Feb 2024 10:00:56 +0800
From: Changbin Du <changbin.du@...wei.com>
To: Adrian Hunter <adrian.hunter@...el.com>
CC: Changbin Du <changbin.du@...wei.com>, Peter Zijlstra
	<peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de
 Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, Alexander
 Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
	<linux-kernel@...r.kernel.org>, <linux-perf-users@...r.kernel.org>, Andi
 Kleen <ak@...ux.intel.com>, Thomas Richter <tmricht@...ux.ibm.com>,
	<changbin.du@...il.com>
Subject: Re: [PATCH v5 3/5] perf: script: add field 'disasm' to display
 mnemonic instructions

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