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

sashiko-bot
2026-04-25T18:15:36Z

Sashiko AI review found 3 potential issue(s):
- [High] Uncollectable reference cycle between pyrf_event and pyrf_callchain causes a massive memory leak.
- [High] API Contract Violation: Returning a valid object while a Python exception is pending causes a SystemError.
- [Medium] Dead code and massive performance overhead due to unconditional eager callchain resolution.
--

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

perf python: Add callchain support

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

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -822,10 +1012,12 @@ static PyTypeObject *pyrf_event__type[] = {
> };
>
> static PyObject *pyrf_event__new(const union perf_event *event, struct evsel *evsel,
> - struct perf_session *session __maybe_unused)
> + struct perf_session *session)
> {

[ ... ]

> + 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) {
> + 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);

The commit message mentions lazy iteration over callchain frames, but this
appears to eagerly call machine__resolve() and thread__resolve_callchain()
for every sample event with a callchain.

Does this make the lazy evaluation logic in pyrf_callchain__next() dead
code, since pchain->resolved is always set to true here?

Also, could this impose a large performance overhead for scripts that
do not access the callchain property?

> +
> + pchain = PyObject_New(struct pyrf_callchain, &pyrf_callchain__type);
> + if (pchain) {

If PyObject_New() fails and returns NULL, the Python C API automatically
sets a MemoryError exception.

Since the code falls through the if block and returns the valid pevent
pointer at the end of the function, does returning a valid object pointer
while an exception indicator is set cause the interpreter to crash with a
SystemError?

Should PyErr_Clear() be called if the intent is to gracefully degrade without
a callchain on allocation failure?

> + pchain->pevent = pevent;
> + Py_INCREF(pevent);
> + pchain->nr_frames = cursor->nr;
> + pchain->pos = 0;
> + pchain->resolved = true;
> + pchain->frames = calloc(pchain->nr_frames,
> + sizeof(*pchain->frames));
> + if (pchain->frames) {
> + struct callchain_cursor_node *node;
> +
> + for (i = 0; i < pchain->nr_frames; i++) {
> + node = callchain_cursor_current(cursor);
> + pchain->frames[i].ip = node->ip;
> + pchain->frames[i].map =
> + map__get(node->ms.map);
> + pchain->frames[i].sym = node->ms.sym;
> + callchain_cursor_advance(cursor);
> + }
> + pevent->callchain = (PyObject *)pchain;

Since pchain holds a strong reference to pevent, and pevent holds a strong
reference to pchain, does this create an uncollectable reference cycle?

Neither of these Python extension types appears to implement Python's
cycle-breaking garbage collector (via Py_TPFLAGS_HAVE_GC, tp_traverse,
and tp_clear).

Will this permanently leak the event, the callchain, the frames array,
and the associated map objects when the Python script discards the event?

> + } else {
> + Py_DECREF(pchain);
> + }
> + }
> + }
> + addr_location__exit(&al);
> + }
> + }
> return (PyObject *)pevent;
> }

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