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

sashiko-bot
2026-04-24T17:36:23Z

Sashiko AI review found 3 potential issue(s):
- [High] Event names are incorrectly matched because `str(sample.evsel)` includes the `evsel(...)` wrapper.
- [Medium] Incorrect mathematical calculation of the symbol offset `symoff` due to mixing absolute and map-relative addresses.
- [Medium] The updated shell test silently fails on out-of-tree builds because it lacks the necessary environment setup to locate the `perf` Python module.
--

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

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

Add a port of the arm-cs-trace-disasm script that uses the perf python
module directly. This approach is significantly faster than using perf
script callbacks as it avoids creating intermediate dictionaries for
all event fields. Update the testing 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 00000000000000..d1227e809adf68
> --- /dev/null
> +++ b/tools/perf/python/arm-cs-trace-disasm.py
[ ... ]
> +def print_srccode(comm: str, sample: perf.sample_event, symbol: str, dso: str) -> None:
> + """Print source code and symbols for a sample."""
> + ip = sample.sample_ip
> + if symbol == "[unknown]":
> + start_str = common_start_str(comm, sample) + f"{ip:x}".rjust(16).ljust(40)
> + else:
> + symoff = 0
> + sym_start = sample.sym_start
> + if sym_start is not None:
> + symoff = ip - sym_start

Does this calculate the correct offset for dynamically loaded libraries?
If sample.sample_ip is an absolute instruction pointer, and sample.sym_start
is map-relative, won't subtracting them yield an incorrect offset?

The original C-side code avoided this by using the pre-computed offset
via al->addr - sym->start.

[ ... ]
> + 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
[ ... ]
> + ev_name = str(sample.evsel)
[ ... ]
> + if ev_name.startswith("instructions"):
> + print_srccode(comm, sample, symbol, dso)
> + return
> +
> + if not ev_name.startswith("branches"):
> + return

Will this correctly match the event names?
The __str__ representation of pyrf_evsel in tools/perf/util/python.c returns
the event name wrapped as "evsel(%s)".

Does this cause ev_name to be evaluated as strings like
"evsel(instructions:u)", which would fail both startswith() checks and
result in the script silently dropping all events?

> diff --git a/tools/perf/tests/shell/test_arm_coresight_disasm.sh b/tools/perf/tests/shell/test_arm_coresight_disasm.sh
> index 0dfb4fadf53177..c15cd60e1c2464 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}

Could this fail silently on out-of-tree builds?
If the perf python module is not installed globally, the script will fail to
import perf with a ModuleNotFoundError.

Because the PYTHONPATH is not exported and sys.path is not modified to point
to the built python extension directory, and errors are swallowed by
2> /dev/null, the output file ends up empty.

Would it be better to ensure PYTHONPATH includes the built extension directory
similar to how it is handled in python-use.sh?

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