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

sashiko-bot
2026-04-25T18:15:51Z

Sashiko AI review found 4 potential issue(s):
- [High] Fatal logic error in event filtering causes all trace samples to be silently skipped.
- [High] Unhandled `None` return value from `sample.srccode()` causes fatal `TypeError` crashes.
- [Medium] Test suite breakage due to missing PYTHONPATH configuration for standalone scripts.
- [Medium] Crash in subprocess execution when `annotate.objdump` contains command-line arguments.
--

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

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

This commit adds a python port of the arm-cs-trace-disasm script that directly
uses the perf python module. It aims to speed up execution by avoiding
intermediate dictionary creation and updates the test suite to use the 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 000000000000..92c97cca6f66
> --- /dev/null
> +++ b/tools/perf/python/arm-cs-trace-disasm.py
[ ... ]
> +def read_disam(dso_fname: str, dso_start: int, start_addr: int,
> + stop_addr: int, objdump: str) -> List[str]:
[ ... ]
> + if addr_range in DISASM_CACHE:
> + disasm_output = DISASM_CACHE[addr_range]
> + else:
> + start_addr = start_addr - dso_start
> + stop_addr = stop_addr - dso_start
> + disasm = [objdump, "-d", "-z",
> + f"--start-address={start_addr:#x}",
> + f"--stop-address={stop_addr:#x}"]
> + disasm += [dso_fname]
> + disasm_output = check_output(disasm).decode('utf-8').split('\n')

What happens if objdump contains command-line arguments (for example, if a
user configured annotate.objdump to "llvm-objdump --x86-asm-syntax=intel")?

Because check_output() is called with a list, won't it treat the entire
string as the executable name and raise a FileNotFoundError instead of
parsing the arguments?

[ ... ]
> +def print_srccode(comm: str, sample: perf.sample_event, symbol: str, dso: str) -> None:
[ ... ]
> + global GLB_SOURCE_FILE_NAME, GLB_LINE_NUMBER, GLB_DSO
> +
> + source_file_name, line_number, source_line = sample.srccode()

Is it possible for sample.srccode() to return None?

Looking at pyrf_sample_event__srccode in the C extension, it returns
Py_RETURN_NONE if pyrf_sample_event__resolve_al fails to resolve the sample
address.

If trace data contains unresolvable samples, will this unpack operation fail
with a TypeError?

[ ... ]
> +class TraceDisasm:
[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
[ ... ]
> + ev_name = str(sample.evsel)
[ ... ]
> + if ev_name.startswith("instructions"):
> + print_srccode(comm, sample, symbol, dso)
> + return
> +
> + if not ev_name.startswith("branches"):
> + return

Are these startswith() checks working as intended?

The pyrf_evsel__str function in tools/perf/util/python.c formats the string
representation as evsel(<name>) (e.g., "evsel(instructions:u)").

Does this mean ev_name.startswith("instructions") will always evaluate to
False, causing the script to skip all trace samples?

[ ... ]
> diff --git a/tools/perf/tests/shell/test_arm_coresight_disasm.sh b/tools/perf/tests/shell/test_arm_coresight_disasm.sh
> index 0dfb4fadf531..c15cd60e1c24 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}

Does this direct python invocation break the test suite in out-of-tree builds?

Unlike perf script which internally resolves the module, calling the python
interpreter directly without configuring PYTHONPATH or injecting the module
directory into sys.path might cause python to fall back to system paths and
fail to import the local perf module.

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