Re: [PATCH v6 21/59] perf python: Expose brstack in sample event

sashiko-bot
2026-04-25T18:11:40Z

Sashiko AI review found 2 potential issue(s):
- [High] Unbreakable reference cycle between `pyrf_event` and `pyrf_branch_stack` causes a severe memory leak.
- [Medium] The `brstack` property returns a stateful, single-use iterator, causing unexpected exhaustion on subsequent accesses.
--

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

perf python: Expose brstack in sample event

Implement pyrf_branch_entry and pyrf_branch_stack for lazy
iteration over branch stack entries.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 37764a287a71..228c1c628bf4 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -870,6 +874,144 @@ static PyObject *pyrf_sample_event__get_callchain(PyObject *self, void *closure
> return pevent->callchain;
> }
>

[ ... ]

> +struct pyrf_branch_stack {
> + PyObject_HEAD
> + struct pyrf_event *pevent;
> + struct branch_entry *entries;
> + u64 nr;
> + u64 pos;
> +};
> +

[ ... ]

> +static PyObject *pyrf_branch_stack__next(struct pyrf_branch_stack *pstack)
> +{
> + struct pyrf_branch_entry *pentry;
> +
> + if (pstack->pos >= pstack->nr)
> + return NULL;

[ ... ]

> + pstack->pos++;
> + return (PyObject *)pentry;
> +}
> +
> +static PyTypeObject pyrf_branch_stack__type = {
> + PyVarObject_HEAD_INIT(NULL, 0)
> + .tp_name = "perf.branch_stack",
> + .tp_basicsize = sizeof(struct pyrf_branch_stack),
> + .tp_dealloc = (destructor)pyrf_branch_stack__delete,
> + .tp_flags = Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
> + .tp_doc = "perf branch stack object.",
> + .tp_iter = PyObject_SelfIter,
> + .tp_iternext = (iternextfunc)pyrf_branch_stack__next,
> +};
> +
> +static PyObject *pyrf_sample_event__get_brstack(PyObject *self, void *closure __maybe_unused)
> +{
> + struct pyrf_event *pevent = (void *)self;
> +
> + if (!pevent->brstack)
> + Py_RETURN_NONE;
> +
> + Py_INCREF(pevent->brstack);
> + return pevent->brstack;
> +}

This property returns the same pyrf_branch_stack object on every access. Since
pyrf_branch_stack is a stateful iterator that modifies pstack->pos, will
accessing event.brstack a second time return an exhausted iterator yielding
zero elements?

[ ... ]

> @@ -1180,6 +1335,27 @@ static PyObject *pyrf_event__new(const union perf_event *event, struct evsel *ev
> addr_location__exit(&al);
> }
> }
> + if (sample->branch_stack) {
> + struct branch_stack *bs = sample->branch_stack;
> + struct branch_entry *entries = perf_sample__branch_entries(sample);
> + struct pyrf_branch_stack *pstack;
> +
> + pstack = PyObject_New(struct pyrf_branch_stack, &pyrf_branch_stack__type);
> + if (pstack) {
> + Py_INCREF(pevent);
> + pstack->pevent = pevent;
> + pstack->pos = 0;
> + pstack->nr = bs->nr;
> + pstack->entries = calloc(bs->nr, sizeof(struct branch_entry));
> + if (pstack->entries) {
> + memcpy(pstack->entries, entries,
> + bs->nr * sizeof(struct branch_entry));
> + pevent->brstack = (PyObject *)pstack;

Does setting pstack->pevent = pevent and pevent->brstack = pstack create an
unbreakable reference cycle?

Since neither pyrf_event nor pyrf_branch_stack implement Python's cyclic
garbage collection interface, Python's GC won't be able to detect or break
this cycle.

When the external references to the event are dropped, won't this permanently
leak the event, the branch stack, and the pstack->entries array for every
processed sample?

Is the pstack->pevent reference needed, given that pstack->entries is an
independent copy and pstack->pevent appears unused during iteration?

> + } else {
> + Py_DECREF(pstack);
> + }
> + }
> + }
> return (PyObject *)pevent;
> }

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