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

sashiko-bot
2026-04-24T17:38:13Z

Sashiko AI review found 3 potential issue(s):
- [High] Unbreakable reference cycle between `pyrf_event` and `pyrf_branch_stack` causes a memory leak on every sample.
- [High] Unhandled Python exception from `PyObject_New` propagates into `PyObject_CallFunction`, violating the C-API contract.
- [Medium] The `branch_entry` Python type fails to expose critical fields from `struct branch_flags`.
--

commit d2aa87cc1645801ae5e1749160678a984b4fb28a
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 c4f0e01b64f364..b446b1e9cfb646 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
[ ... ]
> +static PyGetSetDef pyrf_branch_entry__getset[] = {
> + { .name = "from_ip", .get = (getter)pyrf_branch_entry__get_from, },
> + { .name = "to_ip", .get = (getter)pyrf_branch_entry__get_to, },
> + { .name = "mispred", .get = (getter)pyrf_branch_entry__get_mispred, },
> + { .name = "predicted", .get = (getter)pyrf_branch_entry__get_predicted, },
> + { .name = "in_tx", .get = (getter)pyrf_branch_entry__get_in_tx, },
> + { .name = "abort", .get = (getter)pyrf_branch_entry__get_abort, },
> + { .name = "cycles", .get = (getter)pyrf_branch_entry__get_cycles, },
> + { .name = "type", .get = (getter)pyrf_branch_entry__get_type, },
> + { .name = NULL, },
> +};

This isn't a bug, but struct branch_flags contains additional fields like
spec, new_type, priv, and not_taken. Are these intentionally omitted, or
should they be exposed to match the data available to native perf scripts?

[ ... ]
> @@ -3160,6 +3315,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 PyObject_New() fails and returns NULL, the Python MemoryError exception
indicator is set. Since the code skips the block below and falls through to
PyObject_CallFunction() with this exception still pending, could this violate
the C-API contract and trigger a fatal assertion failure in the Python
runtime?

> + if (pstack) {
> + Py_INCREF(pevent);
> + pstack->pevent = pevent;

Will this create an unbreakable reference cycle?

pstack takes a reference to pevent here, and then pevent takes ownership of
pstack below via pevent->brstack = (PyObject *)pstack. Because neither
pyrf_sample_event__type nor pyrf_branch_stack__type implement the Python
cyclic garbage collection protocol (Py_TPFLAGS_HAVE_GC), the cycle is hidden
from the garbage collector.

Would this cause both objects, along with the dynamically allocated
pstack->entries, to leak on every sample that has a branch stack?

Also, since the branch entries are eagerly copied into pstack->entries below,
does pstack actually need to keep a reference to 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;
> + } else {
> + Py_DECREF(pstack);
> + }
> + }
> + }
> +
> ret = PyObject_CallFunction(psession->sample, "O", pyevent);

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