call, display, ast, codecs: root twenty-three object walks across the Python they run by youknowone · Pull Request #1277 · youknowone/pyre

youknowone · 2026-08-16T15:32:08Z

Twenty-three sites where a PyObjectRef was held in a Rust local, a native Vec, or an off-heap struct across code that allocates or runs Python, and then used without being read back from the shadow stack. W_ListObject and W_DictObject are the two kinds the collector moves, so every case below is reachable with a list or a dict in the exposed slot.

Rebased onto affdab0e7e7. The FrameLocalsProxy::update/pop fixes this branch carried were dropped in the rebase: #1263 landed equivalent rooting for both (and for setdefault, which this branch did not cover), so pyframe.rs is now byte-identical to main.

Repr and pattern-matching walks (`5a957f4f3f3`)

display.rs dict_repr and the dict-view repr arm iterated a native snapshot Vec while each key's and value's __repr__ ran Python. list_repr/tuple_repr were converted earlier; these were the remaining siblings.
The exception-args arm walked the tuple w_exception_get_args mints on every call. That header is old-gen and does not move, but nothing rooted it, so the collector never traced it and its element slots kept the pre-move addresses of anything an item's __repr__ relocated.
type_methods.rs dict_view_snapshot's Items arm called w_tuple_new once per pair with the remaining pairs — and the tuples already built — unrooted.
typedef.rs dict_view_all_contained_in walked its snapshot across contains.
opcode_ops.rs match_class_value accumulated extracted attributes in a native Vec across the getattr calls that produce them. case list(items) reaches it with the subject itself as the extracted value.
_pypy_generic_alias.rs repr_items_list re-fetched by index from a list local held across each element's repr.

Call ABI, frame proxy, converter, registries (`30666cbe952`)

call.rs — the metaclass / staticmethod / classmethod __call__ override arms in all three dispatchers forwarded the argument view built before the override probe, which binds through baseobjspace::get and so runs Python for a property or a user __get__. call_function_impl_result's user_call_slot arm and call_with_kwargs_in_ctx_impl's builtin ABI arms already rebuilt the view from the roots and say why in a comment; the override arms now do the same. call_non_function_callable_with_mode had no roots at all.
baseobjspace.rs call_args_and_c_profile_args had no push_roots while running the profile hook, the callee, and the return hook. arguments is now &mut so its vectors are refreshed between the hooks, and the callee's result is published across the return hook.
builtins.rs import_error_setstate held the state dict across three dict.pop calls; the converted twin base_exception_setstate is 190 lines above.
opcode_ops.rs dict_update_value walked a snapshot across w_dict_store on the is_dict fast path, while the general mapping path below it was already rooted.
typedef.rs dict_view_isdisjoint / setlike_descr_isdisjoint, _pypy_generic_alias.rs unpack_args / subs_tvars, and the three _collections deque extend arms each walked a collect_iterable Vec across user __eq__/__hash__ or block allocation.
_ast/convert.rs Converter::pin pinned its argument and returned the pre-pin local, so the pin was a pure no-op for every list in every AST.
_codecs' registry list and two dicts were forwarded only behind the prebuilt-roots gate, which a minor collection skips once the dirty bit is clear — and CodecState::new sets no dirty bit. The walk moves up beside _pickle and the audit hooks, which were hoisted out of that gate for exactly this reason.
display.rs' mid-repr cycle set keyed on raw addresses, so a container that moved mid-walk stopped matching itself and its cycle recursed unbounded instead of emitting the [...] placeholder. It now holds the objects and is registered as a per-mutator root area.

Verification

Two repro scripts (60 assertions total) covering every site, each using a list or a dict as the value that must survive. Both pass on CPython 3.14.6 and on this branch, at the default nursery and under PYPY_GC_NURSERY=1 (collect on every malloc).

The stress run is what found three defects the review had missed: the two dispatcher twins of the override arms, and the unrooted call result across the return hook. Before those three fixes, PYPY_GC_NURSERY=1 returned the last churn list in place of the real argument (('called', ([399, [399]],))) and PYPY_GC_NURSERY=4096 segfaulted; after them both settings are clean.

cargo test --all --features dynasm — 138 suites, rc=0
pyre/check.py — dynasm 436/436, cranelift 436/436

Left open

wasm synth/short_circuit_value_kept_stack 6.1x > 4x. The fixture is a pure-arithmetic 11.18M-iteration while loop — none of the twenty-three sites runs inside it, and this branch touches no bench, wasm, or check.py code. It has measured 5.6x / 5.5x / 5.2x on the three previous bases on this machine, and the new base (check.py: raise the wasm/dynasm ratio ceiling to 4x #1272) both raised the global ceiling to 4x and removed this fixture's own stated ceiling. The ceiling has been refit four times (check.py: derive the pypy ratio floor from the ceiling, and arm it only on measurable baselines #1080, check.py: derive the pypy floor from the ceiling, and refit the ceilings to measured ratios #1086, check: stabilise the two flaky jitstats fixtures and give wasm-heavy benches a stated ratio ceiling #1231, check.py: raise the wasm/dynasm ratio ceiling to 4x #1272). I did not build the base alone on this machine, so this is an attribution from the fixture's content and the measurement history rather than a same-machine A/B.
_ast/convert.rs still has a window at the 77 node call sites: a field array's earlier element goes stale while its siblings allocate. Only lists are movable there (nodes are instances; the rest are str/int/None), so closing it means having the list-producing helpers return shadow slots and node take them — a typed change across all 77 sites, not a rooting patch. It reproduces only under PYPY_GC_NURSERY=1, where ast.unparse emits invalid source; the default nursery is unaffected. Documented at Converter::pin.

🤖 Generated with Claude Code

coderabbitai · 2026-08-16T15:32:22Z

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Limit details: You’ve used all 2 included reviews currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b542b016-a24b-46a3-a26c-c1f8dfa628f4

📥 Commits

Reviewing files that changed from the base of the PR and between c291606 and 3610c99.

📒 Files selected for processing (12)

pyre/pyre-interpreter/src/_pypy_generic_alias.rs
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/call.rs
pyre/pyre-interpreter/src/display.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/module/_ast/convert.rs
pyre/pyre-interpreter/src/module/_collections/mod.rs
pyre/pyre-interpreter/src/opcode_ops.rs
pyre/pyre-interpreter/src/type_methods.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-jit/src/eval.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

https://github.com/youknowone/pyre/blob/30666cbe9528300e53dbfafcb7f365051d862943/pyre-interpreter/src/display.rs#L272
Keep indexed repr guard TLS access behind residual helpers

After re-extracting the interpreter LLBC, every JIT-translated repr path using ReprGuard now expands REPR_ACTIVE.with(...) directly here and again through repr_leave_at during Drop. Unlike the previous repr_enter/repr_leave pair, these accesses are neither dont_look_inside nor registered in jit_fnaddr.rs; the translator cannot lower the thread_local!/RefCell machinery, so the change can poison annotation or prevent these repr paths from producing JitCode. Return the index and remove it through opaque, registered residual helpers instead.

AGENTS.md reference: AGENTS.md:L5-L12

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

github-actions · 2026-08-16T16:22:08Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 3610c99).
Updated: 2026-08-16T22:43:45.660Z

Files in the reviewed diff

pyre/pyre-interpreter/src/_pypy_generic_alias.rs
pyre/pyre-interpreter/src/baseobjspace.rs
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/call.rs
pyre/pyre-interpreter/src/display.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/module/_ast/convert.rs
pyre/pyre-interpreter/src/module/_collections/mod.rs
pyre/pyre-interpreter/src/opcode_ops.rs
pyre/pyre-interpreter/src/type_methods.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-jit/src/eval.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

pyre/pyre-interpreter/src/module/_ast/convert.rs:1825 ↔ pypy/interpreter/astcompiler/ast.py:210 — AST field values are still passed in a Rust &[(&str, PyObjectRef)] before Converter::node roots them. getattr_str at line 1827 can collect first, leaving previously evaluated movable list values stale; PyPy’s translated to_object locals are GC-rooted across its space.* calls. This was already present on upstream/main (the former pin returned the unrefreshed local); the patch fixes selected instances but explicitly leaves the general case open. Under a nursery collection it can produce an invalid AST / invalid ast.unparse result.
pyre/pyre-interpreter/src/module/_collections/mod.rs:536 ↔ pypy/module/_collections/interp_deque.py:194 — deque.init, extend, and extendleft fully materialize the iterable before mutating the deque. PyPy advances the iterator and appends each item in the same loop (:194-209), so if an iterator yields values and then raises, PyPy retains the yielded prefix while pyre leaves the deque unchanged. upstream/main used the same collect_iterable approach at these call sites; this patch preserves it while adding GC roots.

4. Structural adaptations

pyre/pyre-interpreter/src/baseobjspace.rs:12757 ↔ rpython/memory/gctransform/shadowstack.py:31 — explicit Rust shadow-stack pinning and reloads in the changed call, generic-alias, container, opcode, display, and collection paths replace RPython’s translator-emitted gc_push_roots. This is a fundamental Rust/GC integration adaptation; it preserves PyPy’s observable operation ordering.
pyre/pyre-interpreter/src/display.rs:185 ↔ pypy/objspace/std/objspace.py:126 — pyre stores the active-repr identity set in thread-local state and registers it as a per-mutator GC root, whereas PyPy stores it on the execution context (ec._py_repr). This is the required free-threading adaptation of PyPy’s execution-context-local recursion guard; forwarding the entries preserves recursive repr behavior across moving collections.
pyre/pyre-jit/src/eval.rs:4454 ↔ rpython/memory/gctransform/shadowstack.py:44 — registering the repr active-set walker is the Rust equivalent of having the translated root walker visit live shadow-stack slots during collection.

…hey run `dict_repr` and the dict-view repr arm iterated a native snapshot `Vec`, whose entries the collector does not walk, while each key's and value's `__repr__` ran Python. The exception-args arm walked the tuple `w_exception_get_args` mints on every call. That header is old-gen and does not move, but nothing rooted it, so the collector never traced it and its element slots kept the pre-move addresses of any argument an item's `__repr__` relocated. `dict_view_snapshot`'s `Items` arm called `w_tuple_new` once per pair with the remaining pairs, and the tuples already built, unrooted. `dict_view_all_contained_in` walked its snapshot across `contains`, which runs a user `__contains__`/`__eq__`. `match_class_value` accumulated extracted attributes in a native `Vec` across the `getattr` calls that produce them, and re-read `subject` from a copy taken before `isinstance`. `repr_items_list` re-fetched by index from a `list` local held across each element's repr, and a `W_ListObject` header moves. Each site now pins its values and reads them back from the shadow stack at the point of use. Assisted-by: Claude

…cross the Python they run Sites where a `PyObjectRef` was held in a Rust local, a native `Vec`, or an off-heap struct across code that allocates or runs Python, and then used without being read back from the shadow stack. `call.rs`: the metaclass / staticmethod / classmethod `__call__` override arms in all three dispatchers forwarded the argument view built before the override probe, which binds through `baseobjspace::get` and so runs Python for a property or a user `__get__`. `call_function_impl_result`'s `user_call_slot` arm and `call_with_kwargs_in_ctx_impl`'s builtin ABI arms already rebuilt the view from the roots; the override arms now do the same, and `call_non_function_callable_with_mode` gained the roots it had none of. `baseobjspace.rs call_args_and_c_profile_args` had no `push_roots` at all while running the profile hook before the call, the callee, and the return hook. `arguments` is now `&mut` so its vectors can be refreshed between the hooks, and the callee's result is published across the return hook. `builtins.rs import_error_setstate` held the state dict across three `dict.pop` calls — the converted twin `base_exception_setstate` sits 190 lines above. `opcode_ops.rs dict_update_value` walked a snapshot across `w_dict_store` on the `is_dict` fast path. `typedef.rs dict_view_isdisjoint` and `setlike_descr_isdisjoint`, `_pypy_generic_alias.rs unpack_args` and `subs_tvars`, and the three `_collections` deque extend arms each walked a `collect_iterable` Vec across user `__eq__`/`__hash__` or block allocation. `_ast/convert.rs Converter::pin` pinned its argument and returned the pre-pin local, so the pin was a no-op; `node` and `module_to_object` now read their values back. The remaining window at the 77 `node` call sites is documented at `pin`. `_codecs`' registry list and two dicts were forwarded only behind the prebuilt-roots gate, which a minor collection skips once the dirty bit is clear, and `CodecState::new` sets no dirty bit; the walk moves up beside `_pickle` and the audit hooks, which were hoisted for that same reason. `display.rs`' mid-repr cycle set keyed on raw addresses, so a container that moved mid-walk stopped matching itself and its cycle recursed instead of emitting the placeholder. It now holds the objects and is registered as a per-mutator root area. Assisted-by: Claude

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3610c99ce8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-16T22:36:31Z

+    let _override_roots = pyre_object::gc_roots::push_roots();
+    let override_base = pyre_object::gc_roots::shadow_stack_len();
+    pyre_object::gc_roots::pin_roots(args);


Reload every branch from the newly pinned argument slots

When another mutator has existed, pin_roots(args) performs forwarding queries and can update these slots during a collection, but the ordinary type-call and exact-staticmethod branches below still forward the original args slice. A list or dict argument can therefore be passed through its pre-move address precisely because of this new rooting call; those branches must also rebuild their arguments from override_base (and the callable should be included in the live root set).

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T22:36:31Z

+    refresh(arguments);
+    // The return hook runs Python too, so the callee's result is published
+    // before it and read back afterwards.
+    let result_slot = pyre_object::gc_roots::shadow_stack_len();
+    pyre_object::gc_roots::pin_root(w_res);


Refresh profiler arguments after pinning the result

With an active C profiler and a foreign mutator, pin_root(w_res) can synchronize with a collection after refresh(arguments) has copied the current pointers back into the native vectors. The shadow-stack slots are forwarded, but arguments.arguments_w and keywords_w immediately become stale again, so c_return_trace can read a pre-move list or dict as its firstarg() while rebinding a fixed-code builtin. Pin the result before refreshing, or refresh again afterward.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T22:36:31Z

+            let _roots = pyre_object::gc_roots::push_roots();
+            let dict_slot = pyre_object::gc_roots::shadow_stack_len();
+            pyre_object::gc_roots::pin_root(dict);
+            let entries = pyre_object::w_dict_items(source);


Pin the source before querying the GC

For DICT_UPDATE with distinct dict operands, pin_root(dict) may query the collector once another mutator has existed, but source is still only a raw local at that point. If a collection moves the source, the following w_dict_items(source) dereferences its stale address. Publish both dicts together and take the snapshot from the forwarded source slot.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T22:36:31Z

    fn pin(&self, value: PyObjectRef) -> PyObjectRef {
+        let slot = self.pin_slot(value);
+        pyre_object::gc_roots::shadow_stack_get(slot)


Carry rooted slots through all AST field construction

Returning a PyObjectRef from pin still loses forwarding when a list-valued field is built before an allocating sibling: by the time node receives the field array, it pins the stale copied address rather than reading the original slot. As the added comment notes, PYPY_GC_NURSERY=1 can consequently make ast.unparse emit invalid source; the list-producing helpers and all node sites need to carry slot identities through sibling construction rather than landing this partial rooting workaround.

AGENTS.md reference: AGENTS.md:L288-L290

Useful? React with 👍 / 👎.

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

youknowone force-pushed the gc-decouple branch from 30666cb to 1eb5b31 Compare August 16, 2026 16:03

youknowone changed the title ~~call, display, pyframe, ast, codecs: root twenty-four object walks across the Python they run~~ call, display, ast, codecs: root twenty-three object walks across the Python they run Aug 16, 2026

youknowone added 2 commits August 17, 2026 07:30

youknowone force-pushed the gc-decouple branch from 1eb5b31 to 3610c99 Compare August 16, 2026 22:30

youknowone merged commit cdd6b6e into main Aug 16, 2026
8 of 9 checks passed

youknowone deleted the gc-decouple branch August 16, 2026 22:31

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

youknowone mentioned this pull request Aug 17, 2026

_ast: address the converted tree's values by shadow slot #1290

Merged

Navigation Menu

call, display, ast, codecs: root twenty-three object walks across the Python they run - #1277

call, display, ast, codecs: root twenty-three object walks across the Python they run#1277
youknowone merged 2 commits into
mainfrom
gc-decouple

youknowone commented Aug 16, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 16, 2026 •

edited

Loading

Review limit reached

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

github-actions Bot commented Aug 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repr and pattern-matching walks (5a957f4f3f3)

Call ABI, frame proxy, converter, registries (30666cbe952)

Verification

Left open

Uh oh!

coderabbitai Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

github-actions Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Codex parity review

1. Regressions to PyPy parity introduced by this patch

2. Other mismatches introduced by this patch

3. Pre-existing mismatches (already present before this patch)

4. Structural adaptations

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Aug 16, 2026 •

edited

Loading

Repr and pattern-matching walks (`5a957f4f3f3`)

Call ABI, frame proxy, converter, registries (`30666cbe952`)

coderabbitai Bot commented Aug 16, 2026 •

edited

Loading

github-actions Bot commented Aug 16, 2026 •

edited

Loading