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

sashiko-bot
2026-04-23T04:11:20Z

Sashiko AI review found 4 potential issue(s):
- [High] Invalid Python f-string integer format code (`u`) will cause a ValueError.
- [High] Invalid Python f-string string alignment specifier (`-`) will cause a ValueError.
- [Medium] Unhandled `struct.error` due to lack of `raw_buf` length validation.
- [Medium] Context switch event tracking has been dropped, resulting in dead code.
--

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

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

Ported from tools/perf/scripts/python/.
- Refactored intel-pt-events.py to use a class structure to eliminate
global state and improve maintainability.
- Removed Python 2 compatibility checks.
- Renamed methods in libxed.py to snake_case (Instruction ->
instruction, SetMode -> set_mode, DisassembleOne -> disassemble_one)
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 0000000000000..682bf80becfee
> --- /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)

If raw_buf is empty or smaller than the format size (which can happen if
sample.raw_buf falls back to an empty byte string later in the code), won't
this raise an unhandled struct.error and crash the script?

[ ... ]
> + 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} ")

Does Python 3 support the "u" format specifier for integers in f-strings?

It appears that "u" is a valid format code in C-style formatting, but Python
f-strings use "d" for integers. This might raise a ValueError on the first
sample.

[ ... ]
> + def print_common_ip(self, sample: perf.sample_event, symbol: str, dso: str) -> None:
> + """Print IP and symbol info."""
> + ip = sample.sample_ip
[ ... ]
> + 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=' ')

Is the "-" character allowed as a string alignment specifier in Python
f-strings?

Applying it to a string format type might raise a ValueError. Would using "<"
for left-alignment work 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]

Are switch events being tracked correctly here?

It looks like self.switch_str is never populated anywhere in the code. Along
with the --all-switch-events argparse flag seemingly not being used, was the
context switch event logic dropped during the refactoring?

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