Re: [PATCH v1 45/58] perf stackcollapse: Port stackcollapse to use python module

sashiko-bot
2026-04-20T00:41:11Z

Sashiko AI review found 2 potential issue(s):
- [Medium] Script crashes with a TypeError on samples without callchains due to a flawed hasattr() check.
- [Medium] All samples are incorrectly grouped under an 'Unknown' process name because perf.sample_event lacks a 'comm' attribute.
--

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

perf stackcollapse: Port stackcollapse to use python module

Modernize the legacy stackcollapse.py trace script by refactoring it
into a class-based architecture. The script uses perf.session for event
processing and aggregates call stacks to produce output suitable for
flame graphs.

> diff --git a/tools/perf/python/stackcollapse.py b/tools/perf/python/stackcollapse.py
> new file mode 100755
> index 0000000000000..22caf97c9cac0
> --- /dev/null
> +++ b/tools/perf/python/stackcollapse.py
> @@ -0,0 +1,120 @@

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None:
> + """Collect call stack for each sample."""
> + stack = []
> + if hasattr(sample, "callchain"):
> + for node in sample.callchain:

Does this code crash when a sample lacks a callchain?

The perf.sample_event object exposes callchain as a C API property descriptor.
If a sample lacks a callchain, pyrf_sample_event__get_callchain explicitly
returns Py_None.

Because returning None does not raise an AttributeError,
hasattr(sample, "callchain") evaluates to True. The script then attempts
to iterate over None in the for loop, which raises a TypeError and
terminates the script.

> + stack.append(self.tidy_function_name(node.symbol, node.dso))
> + else:
> + # Fallback if no callchain
> + sym = getattr(sample, "symbol", "[unknown]")
> + dso = getattr(sample, "dso", "[unknown]")
> + stack.append(self.tidy_function_name(sym, dso))
> +
> + if self.args.include_comm:
> + comm = getattr(sample, "comm", "Unknown").replace(" ", "_")

Are all samples grouped under an "Unknown" process name?

The C extension's perf.sample_event type does not expose a comm attribute,
unlike the legacy perf script dictionary. Consequently, getattr will silently
and perpetually default to "Unknown", preventing proper process separation in
the collapsed output.

> + sep = "-"
> + if self.args.include_pid:
> + comm = f"{comm}{sep}{getattr(sample, 'sample_pid', 0)}"
> + sep = "/"

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