Re: [PATCH v2 21/58] perf python: Expose brstack in sample event

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

Sashiko AI review found 2 potential issue(s):
- [High] A circular reference between `pyrf_event` and `pyrf_branch_stack` causes a permanent memory leak on every sample containing a branch stack.
- [Medium] The `brstack` property returns a single-use iterator instead of an iterable collection, leading to unexpected API behavior.
--

commit 7706b9a1f76a3982766c802596501351980d123a
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 b7f877189f8cf..5ace996267151 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> +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;
> +
> + pentry = PyObject_New(struct pyrf_branch_entry, &pyrf_branch_entry__type);
> + if (!pentry)
> + return NULL;
> +
> + pentry->from = pstack->entries[pstack->pos].from;
> + pentry->to = pstack->entries[pstack->pos].to;
> + pentry->flags = pstack->entries[pstack->pos].flags;
> +
> + pstack->pos++;
> + return (PyObject *)pentry;
> +}

Since the property returns the same object and advances pstack->pos during
iteration without resetting it, does this mean the iterator gets permanently
exhausted after one use?

Would it be better for the property to return a new iterator object or
implement sequence semantics?

[ ... ]

> @@ -3157,6 +3312,28 @@ static int pyrf_session_tool__sample(const struct perf_tool *tool,
> }
> }
>
> + 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 this create a strong reference cycle where pstack->pevent holds a
reference to pevent, and pevent->brstack holds a reference to pstack?

Since neither pyrf_event__type nor pyrf_branch_stack__type implements
Python's Py_TPFLAGS_HAVE_GC and tp_traverse or tp_clear protocols, can the
Python cyclic garbage collector detect or break this cycle?

When the Python user script discards the event, wouldn't pevent's reference
count remain at 1, preventing pyrf_event__delete from executing and
leaking pevent, pstack, and pstack->entries for every event containing a
branch stack?

Since pstack->entries eagerly deep-copies the branch stack data, is it
necessary for pstack to hold a reference to pevent to remain valid?

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