Re: [PATCH v6 39/59] perf intel-pt-events: Port intel-pt-events/libxed to use python module

sashiko-bot
2026-04-25T18:10:06Z

Sashiko AI review found 5 potential issue(s):
- [Critical] Immediate crash due to AttributeError when accessing thread ID.
- [High] Unhandled TypeError crashing the parser when symbol resolution fails.
- [High] Silently dropped IPC metrics and missing fields due to API mismatch.
- [High] Missing bounds check leading to struct.error crashes on undersized buffers.
- [Medium] Context switch trace is dead code and non-functional.
--

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

perf intel-pt-events: Port intel-pt-events/libxed to use python module

This commit ports the intel-pt-events.py script to use the newer perf Python
module, refactoring it into a class-based structure. It eliminates global
state, drops Python 2 compatibility checks, and updates libxed.py methods to
snake_case.

diff --git a/tools/perf/python/intel-pt-events.py b/tools/perf/python/intel-pt-events.py
new file mode 100755
index 000000000000..19a0faec8f5f
--- /dev/null
+++ b/tools/perf/python/intel-pt-events.py
[ ... ]
> + def print_ptwrite(self, raw_buf: bytes) -> None:
> + """Print PTWRITE data."""
> + data = struct.unpack_from("<IQ", raw_buf)

Will this code raise an unhandled struct.error exception and abort the
processing loop if a malformed perf.data trace provides a raw_buf smaller than
12 bytes, or an empty byte string? This pattern also appears in print_mwait,
print_pwrx, and other similar functions.

[ ... ]
> + def common_start_str(self, comm: str, sample: perf.sample_event) -> str:
> + """Return common start string for display."""
> + ts = sample.sample_time
> + cpu = sample.sample_cpu
> + pid = sample.sample_pid
> + tid = sample.tid

Does the perf.sample_event C extension expose this property as sample_tid
rather than tid? If so, will accessing sample.tid raise an AttributeError on
the first event and crash the script?

[ ... ]
> + def print_common_ip(self, sample: perf.sample_event, symbol: str, dso: str) -> None:
> + """Print IP and symbol info."""
> + ip = sample.sample_ip
> + offs = f"+{sample.symoff:#x}" if hasattr(sample, "symoff") else ""
> + cyc_cnt = getattr(sample, "cyc_cnt", 0)
> + if cyc_cnt:
> + insn_cnt = getattr(sample, "insn_cnt", 0)
> + ipc_str = f" IPC: {insn_cnt / cyc_cnt:#.2f} ({insn_cnt}/{cyc_cnt})"

Have these legacy metric fields been renamed in the new perf Python module API?
If the module exports them as sample_cyc_count and sample_insn_count instead
of cyc_cnt and insn_cnt, getattr will always return 0 and silently bypass the
IPC string generation. Other legacy fields like symoff and flags_disp might
also be missing from the new API.

[ ... ]
> + def print_srccode(self, comm: str, sample: perf.sample_event,
> + symbol: str, dso: str, with_insn: bool) -> None:
> + """Print source code info."""
> + ip = sample.sample_ip
> + if symbol == "[unknown]":
> + start_str = self.common_start_str(comm, sample) + (f"{ip:x}").rjust(16).ljust(40)
> + else:
> + offs = f"+{sample.symoff:#x}" if hasattr(sample, "symoff") else ""
> + start_str = self.common_start_str(comm, sample) + (symbol + offs).ljust(40)

If symbol resolution fails in the C extension and
pyrf_sample_event__get_symbol returns None instead of "[unknown]", will
the comparison symbol == "[unknown]" evaluate to False? This would cause the
else branch to execute, attempting to concatenate None with a string (offs),
which would raise a TypeError.

[ ... ]
> +if __name__ == "__main__":
> + ap = argparse.ArgumentParser()
> + ap.add_argument("-i", "--input", default="perf.data", help="Input file name")
> + ap.add_argument("--insn-trace", action='store_true')
> + ap.add_argument("--src-trace", action='store_true')
> + ap.add_argument("--all-switch-events", action='store_true')
> + ap.add_argument("--interleave", type=int, nargs='?', const=4, default=0)
> + args = ap.parse_args()
> +
> + analyzer = IntelPTAnalyzer(args)
> +
> + try:
> + session = perf.session(perf.data(args.input), sample=analyzer.process_event)
> + analyzer.session = session
> + session.process_events()

Are context switch events functional with this configuration? The script accepts
the --all-switch-events argument and checks the self.switch_str dictionary when
processing samples, but the perf.session setup only registers the sample
callback. Is there a missing callback registration needed to populate the
dictionary with context switch events?

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