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

sashiko-bot
2026-04-24T17:13:16Z

Sashiko AI review found 3 potential issue(s):
- [Medium] Invalid formatting specifiers in f-strings cause unconditional runtime crashes.
- [Medium] Missing buffer length validations before struct unpacking cause script crashes on truncated events.
- [Medium] Context switch reporting functionality is silently broken due to missing API support.
--

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

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

This commit refactors intel-pt-events.py to use a class structure and the
perf python module. It eliminates global state, removes Python 2
compatibility checks, and updates libxed.py methods to comply with pylint.

> diff --git a/tools/perf/python/intel-pt-events.py b/tools/perf/python/intel-pt-events.py
> new file mode 100755
> index 00000000000000..682bf80becfeef
> --- /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)

Can raw_buf be too small or empty here?

The exception handler in do_process_event() defaults raw_buf to b""
if the sample lacks raw data. Calling struct.unpack_from() on an empty
buffer might raise a struct.error and immediately abort the trace session.

[ ... ]

> + def print_pwre(self, raw_buf: bytes) -> None:
> + """Print PWRE data."""
> + data = struct.unpack_from("<IQ", raw_buf)

Are length checks needed here as well? Unlike print_cbr() which explicitly
checks if len(raw_buf) is sufficient, functions like print_pwre(),
print_exstop(), and print_evt() appear to unpack without validating the
length of raw_buf.

[ ... ]

> + 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
> + machine_pid = getattr(sample, "machine_pid", 0)
> + if machine_pid:
> + vcpu = getattr(sample, "vcpu", -1)
> + return (f"VM:{machine_pid:5d} VCPU:{vcpu:03d} {comm:>16s} {pid:5u}/{tid:<5u} "
> + f"[{cpu:03u}] {ts // 1000000000:9u}.{ts % 1000000000:09u} ")
> + return (f"{comm:>16s} {pid:5u}/{tid:<5u} [{cpu:03u}] "
> + f"{ts // 1000000000:9u}.{ts % 1000000000:09u} ")

Will this crash with a ValueError? Python f-strings do not support the 'u'
format specifier for integers. Because common_start_str() is called for
most events, this might crash unconditionally on the very first event
processed.

[ ... ]

> + def print_common_ip(self, sample: perf.sample_event, symbol: str, dso: str) -> None:

[ ... ]

> + if self.insn and self.disassembler is not None:
> + try:
> + insn = sample.insn()
> + except AttributeError:
> + insn = None
> + if insn:
> + cnt, text = self.disassem(insn, ip)
> + byte_str = (f"{ip:x}").rjust(16)
> + for k in range(cnt):
> + byte_str += f" {insn[k]:02x}"
> + print(f"{byte_str:-40s} {text:-30s}", end=' ')

Does Python allow the '-' sign modifier in string format specifiers? It
looks like this might raise a ValueError, and perhaps '<' should be used
for left alignment instead.

[ ... ]

> + def do_process_event(self, sample: perf.sample_event) -> None:
> + """Process event and print info."""
> + comm = "Unknown"

[ ... ]

> + cpu = sample.sample_cpu
> + if cpu in self.switch_str:
> + print(self.switch_str[cpu])
> + del self.switch_str[cpu]

Is the --all-switch-events feature still functional after porting to the
standalone perf.session API?

It looks like self.switch_str is never populated because the new session
only registers a sample callback, losing the legacy context_switch callback
that previously recorded switch events.

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