Re: [PATCH v1 32/58] perf arm-cs-trace-disasm: Port arm-cs-trace-disasm to use python module

sashiko-bot
2026-04-20T00:28:40Z

Sashiko AI review found 4 potential issue(s):
- [High] Missing `import perf` causes immediate runtime crash.
- [High] Runtime crash due to unpacking mismatch on `sample.srccode()`.
- [Medium] Broken test integration due to stale script path.
- [Medium] Missing early termination causes severe performance regression.
--

commit 5bdc3caf81b1523e22044cb273de9d509ecc0c13
Author: Ian Rogers <irogers@google.com>

perf arm-cs-trace-disasm: Port arm-cs-trace-disasm to use python module

This commit adds a port of the arm-cs-trace-disasm script that uses the
perf python module directly to improve performance. It also updates the
testing to use the newly ported script.

> diff --git a/tools/perf/python/arm-cs-trace-disasm.py b/tools/perf/python/arm-cs-trace-disasm.py
> new file mode 100755
> index 0000000000000..3557a02b03fbc
> --- /dev/null
> +++ b/tools/perf/python/arm-cs-trace-disasm.py
> @@ -0,0 +1,333 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +"""
> +arm-cs-trace-disasm.py: ARM CoreSight Trace Dump With Disassember using perf python module
> +"""
> +
> +import os
> +from os import path
> +import re
> +from subprocess import check_output
> +import argparse
> +import platform
> +import sys
> +from typing import Dict, List, Optional
> +

Does this script need an import for the perf module? The code uses it
directly in several places like perf.config_get() and perf.sample_event,
but it does not appear to be imported.

[ ... ]

> +def print_srccode(comm: str, sample: perf.sample_event, symbol: str, dso: str) -> None:
> + """Print source code and symbols for a sample."""

[ ... ]

> + global GLB_SOURCE_FILE_NAME, GLB_LINE_NUMBER, GLB_DSO
> +
> + source_file_name, line_number, source_line, _ = sample.srccode()

Can this cause a runtime error when unpacking? The underlying
pyrf_sample_event__srccode in python.c constructs a 3-element tuple using
Py_BuildValue("(sIs#)", ...), but this code attempts to unpack 4 elements.

[ ... ]

> +class TraceDisasm:
> + """Class to handle trace disassembly."""
> + def __init__(self, cli_options: argparse.Namespace):
> + self.options = cli_options
> + self.sample_idx = -1
> + self.session: Optional[perf.session] = None
> +
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process a single perf event."""
> + self.sample_idx += 1
> + global SAMPLE_IDX
> + SAMPLE_IDX = self.sample_idx
> +
> + if self.options.start_time and sample.sample_time < self.options.start_time:
> + return
> + if self.options.stop_time and sample.sample_time > self.options.stop_time:
> + return
> + if self.options.start_sample and self.sample_idx < self.options.start_sample:
> + return
> + if self.options.stop_sample and self.sample_idx > self.options.stop_sample:
> + return

Does returning here correctly stop the analysis? Since
self.session.process_events() loops through all records, returning only skips
the current callback instead of terminating the session like the original
script did.

[ ... ]

> diff --git a/tools/perf/tests/shell/test_arm_coresight_disasm.sh b/tools/perf/tests/shell/test_arm_coresight_disasm.sh
> index 0dfb4fadf5317..9749501cbf783 100755
> --- a/tools/perf/tests/shell/test_arm_coresight_disasm.sh
> +++ b/tools/perf/tests/shell/test_arm_coresight_disasm.sh

[ ... ]

> @@ -45,8 +45,9 @@ branch_search="\sbl${sep}b${sep}b.ne${sep}b.eq${sep}cbz\s"
> if [ -e /proc/kcore ]; then
> echo "Testing kernel disassembly"
> perf record -o ${perfdata} -e cs_etm//k --kcore -- touch $file > /dev/null 2>&1
> - perf script -i ${perfdata} -s python:${script_path} -- \
> - -d --stop-sample=30 2> /dev/null > ${file}
> + # shellcheck source=lib/setup_python.sh
> + . "$(dirname "$0")"/lib/setup_python.sh
> + $PYTHON ${script_path} -i ${perfdata} -d --stop-sample=30 2> /dev/null > ${file}

Will this test fail because script_path still points to the old legacy
script? The new python module was added to tools/perf/python/ instead of
tools/perf/scripts/python/.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260419235911.2186050-1-irogers@google.com?part=32