Re: [PATCH v2 18/58] perf python: Add callchain support

sashiko-bot
2026-04-23T04:19:38Z

Sashiko AI review found 3 potential issue(s):
- [High] Unbreakable reference cycle between pyrf_event and pyrf_callchain causes a severe memory leak.
- [Medium] Leftover deferred resolution block in pyrf_callchain__next() is unreachable dead code.
- [Medium] Implementation-defined behavior when resolving a callchain with 0 frames.
--

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

perf python: Add callchain support

This commit implements pyrf_callchain_node and pyrf_callchain types for lazy
iteration over callchain frames. It also adds a callchain property to
sample_event to expose this information to Python scripts.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 3e0534744f0a2..ebbb10c0efeb8 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -621,6 +624,181 @@ static PyObject *pyrf_sample_event__insn(PyObject *self, PyObject *args __maybe_
> pevent->sample.insn_len);
> }
>

[ ... ]

> +static PyObject *pyrf_callchain__next(struct pyrf_callchain *pchain)
> +{
> + struct pyrf_callchain_node *pnode;
> +
> + if (!pchain->resolved) {

Is this deferred resolution block reachable?

It appears that pchain->resolved is unconditionally set to true when the
callchain is eagerly resolved in pyrf_session_tool__sample(). Since the
pyrf_callchain__type does not define a tp_new function to be instantiated
directly from Python, it seems this condition will never be met, making this
dead code.

> + struct evsel *evsel = pchain->pevent->sample.evsel;
> + struct evlist *evlist = evsel->evlist;
> + struct perf_session *session = evlist ? evlist__session(evlist) : NULL;
> + struct addr_location al;
> + struct callchain_cursor *cursor;
> + struct callchain_cursor_node *node;
> + u64 i;
> +
> + if (!session || !pchain->pevent->sample.callchain)
> + return NULL;
> +
> + addr_location__init(&al);
> + if (machine__resolve(&session->machines.host, &al, &pchain->pevent->sample) < 0) {
> + addr_location__exit(&al);
> + return NULL;
> + }
> +
> + cursor = get_tls_callchain_cursor();
> + if (thread__resolve_callchain(al.thread, cursor, evsel,
> + &pchain->pevent->sample, NULL, NULL,
> + PERF_MAX_STACK_DEPTH) != 0) {
> + addr_location__exit(&al);
> + return NULL;
> + }
> + callchain_cursor_commit(cursor);
> +
> + pchain->nr_frames = cursor->nr;
> + if (pchain->nr_frames > 0) {
> + pchain->frames = calloc(pchain->nr_frames, sizeof(*pchain->frames));
> + if (!pchain->frames) {
> + addr_location__exit(&al);
> + return PyErr_NoMemory();
> + }
> +
> + 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);
> + }
> + }
> + pchain->resolved = true;
> + addr_location__exit(&al);
> + }

[ ... ]

> @@ -2807,6 +2998,49 @@ static int pyrf_session_tool__sample(const struct perf_tool *tool,
> if (pevent->sample.merged_callchain)
> pevent->sample.callchain = NULL;
>
> + if (sample->callchain) {
> + struct addr_location al;
> + struct callchain_cursor *cursor;
> + u64 i;
> + struct pyrf_callchain *pchain;
> +
> + addr_location__init(&al);
> + if (machine__resolve(&psession->session->machines.host, &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);
> +
> + pchain = PyObject_New(struct pyrf_callchain, &pyrf_callchain__type);
> + if (pchain) {
> + pchain->pevent = pevent;
> + Py_INCREF(pevent);

Does this create an unbreakable reference cycle?

pchain takes a strong reference to pevent here, and shortly below,
pevent takes ownership of pchain via pevent->callchain = (PyObject *)pchain.

Since neither pyrf_event__type nor pyrf_callchain__type implements the
Py_TPFLAGS_HAVE_GC flag and the required tp_traverse/tp_clear callbacks,
the Python garbage collector won't be able to break this circular reference.
Will this cause both objects, the frames buffer, and the map references to
leak for every sample event with a callchain?

> + pchain->nr_frames = cursor->nr;
> + pchain->pos = 0;
> + pchain->resolved = true;
> + pchain->frames = calloc(pchain->nr_frames,
> + sizeof(*pchain->frames));

What happens when resolving a callchain with 0 frames?

If cursor->nr is 0, we pass 0 to calloc(), which is implementation-defined.
If libc returns NULL (like musl), the subsequent if (pchain->frames) check
will fail, discarding pchain and leaving event.callchain as None.
If libc returns a valid pointer (like glibc), the check passes and
event.callchain becomes an empty iterator.

Could we skip the allocation or handle the 0-frame case explicitly to avoid
inconsistent API behavior across different libc implementations?

> + 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;
> + } else {
> + Py_DECREF(pchain);
> + }
> + }
> + }
> + addr_location__exit(&al);
> + }
> + }

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