Re: [PATCH v2 38/58] perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

sashiko-bot
2026-04-23T04:12:46Z

Sashiko AI review found 4 potential issue(s):
- [High] Calling `self.session.process(pid)` can raise an unhandled `TypeError` if the thread is not found, terminating event processing prematurely.
- [Medium] The refactored script fails to group output by syscall, causing the `syscall: <name>` header to be printed redundantly for every distinct error code.
- [Medium] The script no longer sorts the error counts by occurrence frequency.
- [Low] The `self.unhandled` dictionary tracks unhandled events but is never used or printed.
--

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

perf failed-syscalls-by-pid: Port failed-syscalls-by-pid to use python module

This commit ports the failed-syscalls-by-pid script from the legacy Util
library to the perf Python module API. It uses perf.syscall_name() for
resolution and standard dictionaries for aggregation.

> diff --git a/tools/perf/python/failed-syscalls-by-pid.py b/tools/perf/python/failed-syscalls-by-pid.py
> new file mode 100755
> index 0000000000000..1a8c9bb7e90c4
> --- /dev/null
> +++ b/tools/perf/python/failed-syscalls-by-pid.py
[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process a single sample event."""
> + event_name = str(sample.evsel)
> + if "sys_exit" not in event_name:
> + return
> +
> + pid = sample.sample_pid
> + if hasattr(self, 'session') and self.session:
> + comm = self.session.process(pid).comm()

Can self.session.process(pid) raise a TypeError here if the thread is not
found?

If a thread is untracked or short-lived, the C extension in
tools/perf/util/python.c:pyrf_session__process() raises a TypeError. Since
there is no try/except block here, this exception will cause the C extension
to return -1 and immediately abort the underlying
perf_session__process_events() loop.

> + else:
> + comm = "Unknown"
[ ... ]
> + ret = getattr(sample, "ret", 0)
> + if ret < 0:
> + syscall_id = getattr(sample, "id", -1)
> + if syscall_id == -1:
> + syscall_id = getattr(sample, "sys_id", -1)
> +
> + if syscall_id != -1:
> + self.syscalls[(comm, pid, syscall_id, ret)] += 1
> + else:
> + self.unhandled[event_name] += 1

This isn't a bug, but the unhandled dictionary is populated here and never
used or printed in the print_summary() method.

> + def print_summary(self) -> None:
> + """Print aggregated statistics."""
[ ... ]
> + print(f"{'comm [pid]':<30} {'count':>10}")
> + print(f"{'-' * 30:<30} {'-' * 10:>10}")
> +
> + sorted_keys = sorted(self.syscalls.keys(), key=lambda k: (k[0], k[1], k[2]))

Does this sort the output incorrectly compared to the original script?

The original script sorted the error counts for a given syscall in descending
order by frequency. This sorts only by the dictionary keys, ignoring the
actual error counts stored in the values.

> + current_comm_pid = None
> + for comm, pid, syscall_id, ret in sorted_keys:
> + if current_comm_pid != (comm, pid):
> + print(f"\n{comm} [{pid}]")
> + current_comm_pid = (comm, pid)
> + try:
> + name = perf.syscall_name(syscall_id) or str(syscall_id)
> + except AttributeError:
> + name = str(syscall_id)
> + print(f" syscall: {name:<16}")
> + err_str = strerror(ret)
> + count = self.syscalls[(comm, pid, syscall_id, ret)]
> + print(f" err = {err_str:<20} {count:10d}")

Will this print the syscall name header redundantly for every distinct error
code?

The loop tracks current_comm_pid to avoid redundant process headers, but it
doesn't track the current syscall_id. If a single syscall returns multiple
different error codes, the syscall name header will be printed for each one.

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