Re: [PATCH v7 18/59] perf python: Add callchain support

sashiko-bot
2026-04-25T23:16:15Z

Sashiko AI review found 4 potential issue(s):
- [High] A strong reference cycle between `pyrf_event` and `pyrf_callchain` causes a massive memory leak for every parsed event with a callchain.
- [High] Missing `addr_location__exit(&al)` cleanup when `machine__resolve()` fails, leading to reference leaks.
- [Medium] Callchain frames are unconditionally resolved eagerly, contradicting the commit message's claim of lazy iteration and introducing significant performance overhead.
- [Medium] The `callchain` property returns an exhaustible iterator rather than a reusable iterable, violating expected property semantics and causing silent data loss on subsequent accesses.
--

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

perf python: Add callchain support

This commit adds python bindings for callchain iteration in perf scripts.
It introduces pyrf_callchain_node and pyrf_callchain types and adds a
callchain property to sample events.

> Implement pyrf_callchain_node and pyrf_callchain types for lazy
> iteration over callchain frames. Add callchain property to
> sample_event.

Does this actually provide lazy iteration?

The pyrf_event__new() implementation unconditionally performs the
machine__resolve() and thread__resolve_callchain() operations, allocating
all frames upfront whenever an event has callchain data.

This eager execution appears to make the lazy evaluation fallback logic in
pyrf_callchain__next() unreachable.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 824cf58645e0..2953c4c8e142 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -635,6 +813,12 @@ pyrf_sample_event__getattro(struct pyrf_event *pevent, PyObject *attr_name)
> }
>
> static PyGetSetDef pyrf_sample_event__getset[] = {
> + {
> + .name = "callchain",
> + .get = pyrf_sample_event__get_callchain,
> + .set = NULL,
> + .doc = "event callchain.",
> + },

Will this property return an exhaustible iterator rather than a reusable
iterable?

When a script accesses the callchain property, it receives the exact same
pyrf_callchain iterator instance. As the script iterates over it,
pyrf_callchain__next() increments pchain->pos.

Once consumed, pchain->pos is not rewound, so any subsequent iteration
attempts on the same event will silently yield an empty sequence.

[ ... ]
> @@ -896,6 +1089,49 @@ static PyObject *pyrf_event__new(const union perf_event *event, struct evsel *ev
> return PyErr_Format(PyExc_OSError,
> "perf: can't parse sample, err=%d", err);
> }
> + sample = &pevent->sample;
> + if (machine && sample->callchain) {
> + struct addr_location al;
> + struct callchain_cursor *cursor;
> + u64 i;
> + struct pyrf_callchain *pchain;
> +
> + addr_location__init(&al);
> + if (machine__resolve(machine, &al, sample) >= 0) {

If machine__resolve() partially populates the addr_location structure but
ultimately fails and returns a negative value, is it possible to leak the
thread or map references?

Since addr_location__exit() is only called inside the success block at the
end of this function, the failure path skips the cleanup entirely.

> + cursor = get_tls_callchain_cursor();
> + if (thread__resolve_callchain(al.thread, cursor, evsel, sample,
> + NULL, NULL, PERF_MAX_STACK_DEPTH) == 0) {
> + callchain_cursor_commit(cursor);
> +
> + pchain = PyObject_New(struct pyrf_callchain, &pyrf_callchain__type);
> + if (pchain) {
> + pchain->pevent = pevent;
> + Py_INCREF(pevent);

Could this create an uncollectable strong reference cycle between the
pyrf_event and pyrf_callchain objects?

The newly created pchain takes a strong reference to pevent here, and later
pevent takes ownership of the callchain:

pevent->callchain = (PyObject *)pchain;

Because neither of these C extension types implement Python cycle garbage
collector hooks, it looks like this cyclic reference will never be broken,
leaking both objects and their dynamically allocated structures on every
parsed sample.

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