Received-SPF: pass (google.com: domain of linux-kernel+bounces-28507-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240116113437.1507537-1-changbin.du@huawei.com>
 <87il3tl6zf.fsf@linux.intel.com> <20240117024546.2npqrprazaobyogn@M910t>
In-Reply-To: <20240117024546.2npqrprazaobyogn@M910t>
From: Namhyung Kim <namhyung@kernel.org>
Date: Tue, 16 Jan 2024 19:48:33 -0800
Message-ID: <CAM9d7cghAPQ+xg=DA1E_YD3SR8afzt+fS15cU1FUA7YLLYDuLQ@mail.gmail.com>
Subject: Re: [PATCH 0/3] perf: script: Intro capstone disasm engine to show
 instruction trace
To: Changbin Du <changbin.du@huawei.com>
Cc: Andi Kleen <ak@linux.intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com>, 
	linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello,

On Tue, Jan 16, 2024 at 6:46=E2=80=AFPM Changbin Du <changbin.du@huawei.com=
> wrote:
>
> On Tue, Jan 16, 2024 at 05:53:40AM -0800, Andi Kleen wrote:
> > Changbin Du <changbin.du@huawei.com> writes:
> >
> > > This series introduces capstone disassembler engine to print instruct=
ions of
> > > Intel PT trace, which was printed via the XED tool.
> >
> > FWIW at least on x86 in my experience capstone isn't that great an
> > disassembler. I used it in another project and ran into many decoding b=
ugs.
> > They're mostly in obscure corners, but can be fairly annoying.
> >
> If so, I will preserve the old --xed option as an alternate.
>
> > My other concern with your patchkit is that you change the default
> > output formats. Since perf script is often used with scripts
> > (as the name implies) there is a certain expectation that the output
> > remains stable and parse-able. There are actually use cases where
> > the raw bytes "insn" output is needed.
> >
> > I would rather define new perf script output types for the new decoded =
output,
> > but keep the old alone.
> Agreed. What about these?
>   - Add a new field 'insn_disam' to display decoded output.
>   - Add params to '--insn-trace' option, that is '--insn-trace=3D[raw|dis=
am]'.
>     'raw' is the default value.

Sounds good.  FWIW capstone has been working fine in my
experience. :)

Thanks,
Namhyung