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

sashiko-bot
2026-04-25T23:13:13Z

Sashiko AI review found 4 potential issue(s):
- [High] Renaming methods in `libxed.py` breaks `exported-sql-viewer.py`.
- [High] Invalid format specifiers in f-strings cause immediate runtime crashes.
- [High] Unhandled `struct.error` exceptions due to missing `raw_buf` bounds checks.
- [High] Context switch feature regression and dead code.
--

commit 287f3427909d144b37c8690735624ae6bca74489
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
removes Python 2 compatibility checks. It also renames methods in libxed.py
to snake_case to comply with pylint.

> - Renamed methods in libxed.py to snake_case (Instruction ->
> instruction, SetMode -> set_mode, DisassembleOne -> disassemble_one)
> to comply with pylint.

Does this renaming break tools/perf/python/exported-sql-viewer.py?

It appears that script also imports LibXED and might still be calling
the old CamelCase methods (Instruction, SetMode, DisassembleOne), which
could result in an AttributeError when disassembly is enabled.

[ ... ]

> 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
> @@ -0,0 +1,435 @@

[ ... ]

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

If raw_buf is empty or undersized, will struct.unpack_from raise a
struct.error here?

Since the callbacks are called in do_process_event() where a missing raw_buf
is explicitly handled by setting raw_buf = b"", the exceptions wouldn't be
caught and might propagate up, aborting the trace analysis.

[ ... ]

> + def print_cbr(self, raw_buf: bytes) -> None:
> + """Print CBR data."""
> + if len(raw_buf) < 12:
> + return
> + data = struct.unpack_from("<BBBBII", raw_buf)
> + cbr = data[0]
> + f = (data[4] + 500) // 1000
> + if data[2] == 0:
> + return
> + p = ((cbr * 1000 // data[2]) + 5) // 10
> + print(f"{cbr:3u} freq: {f:4u} MHz ({p:3u}%)", end=' ')

Are these format codes valid in Python 3 f-strings?

It looks like the 'u' format code is not supported for integers. Evaluating
these might raise a ValueError and cause a runtime crash.

[ ... ]

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

This also appears to use the unsupported 'u' format code for integers
in {pid:5u}, {tid:<5u}, and {cpu:03u}.

[ ... ]

> + def print_common_ip(self, sample: perf.sample_event, symbol: str, dso: str) -> 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 for string alignment in f-strings?

It looks like the standard f-string format specification doesn't support
the '-' sign for string alignment, which might raise a ValueError when
evaluated.

[ ... ]

> + 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]

Could this be a regression of the context switch feature?

It looks like self.switch_str is initialized and consumed here, but never
populated. Additionally, the underlying perf.session API doesn't seem to
expose a context_switch event callback to Python, leaving this as
unreachable code.

[ ... ]

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