Skip to content

jit: register _collections deque iterators as GC roots - #684

Merged
youknowone merged 1 commit into
mainfrom
deque-iterator-gcroot
Jul 20, 2026
Merged

youknowone merged 1 commit into
mainfrom
deque-iterator-gcroot

Conversation

@youknowone

@youknowone youknowone commented Jul 20, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

_collections._deque_iterator (W_DequeIter) and _collections._deque_reverse_iterator
(W_DequeRevIter) are #[pyre_class] wrappers that each hold a traced deque: PyObjectRef
managed child, but they were never wired into build_gc. On the native backends the GC-root
completeness oracle (eval.rs, #[cfg(not(target_arch = "wasm32"))], unconditional) therefore
aborts at GC init:

GC-root registration gap: #[pyre_class] type(s) with managed children were never wired into
build_gc ... Register each in build_gc:
["_collections._deque_iterator", "_collections._deque_reverse_iterator"]

This fires before any user code runs, so every native JIT program panics — even a trivial
while loop. main (014bc0fae6b) is affected.

Fix

Both iterators are .allocate()-immortal: allocate lowers to malloc_typed, and the GC
marker skips malloc_typed objects. So they must be registered on the immortal-root path,
register_pyre_class_offsets, alongside W_Enumerate / W_UnpackIter — the immortal-root walker
force-forwards the deque child on every collection. Re-export the two types from _collections
(they live in private submodules) so build_gc can name them.

Correction (force-pushed): the first revision of this branch used register_pyre_class (the
marker-tid path, like W_Deque). That satisfies the completeness oracle's tid check, but the
marker never scans a malloc_typed object, so the deque child would still be swept while the
iterator is live — a latent use-after-free that only surfaces under a major collection. The
current revision uses register_pyre_class_offsets.

Validation

  • Distinguishing test: a module-global deque reachable only through a live iterator, then a
    gc.collect() (major collection). With register_pyre_class_offsets the sum matches CPython;
    the earlier register_pyre_class dropped the deque (swept), so this is the test that separates
    the two paths — a light/minor-GC or churn workload passes either way.
  • Full check.py: dynasm 241/241, cranelift 241/241 green. One unrelated pre-existing
    wasm-only failure (selfrec_tail_exception_unwind, tracked separately) passes on both natives
    and is not touched by this change.

Summary by CodeRabbit

  • Bug Fixes
    • Improved garbage collection handling for forward and reverse collections.deque iterators.
    • Prevented active deque iterators and their underlying data from being incorrectly removed during garbage collection.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change re-exports GC-managed deque iterator wrappers and registers both wrapper types in the JIT garbage-collector type registry.

Changes

Deque iterator GC registration

Layer / File(s) Summary
Expose and register deque iterators
pyre/pyre-interpreter/src/module/_collections/mod.rs, pyre/pyre-jit/src/eval.rs
Re-exports W_DequeIter and W_DequeRevIter, then registers both wrapper types in build_gc() at the end of the existing type-ID chain.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: lifthrasiir

Poem

I’m a rabbit who hops through the heap,
Guarding deque edges while iterators sleep.
Two wrappers now shine in the GC’s bright light,
Their type IDs stay steady and right.
Hop, hop—no deque gets swept out of sight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: registering _collections deque iterators as GC roots in the JIT.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch deque-iterator-gcroot

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.

@github-actions

github-actions Bot commented Jul 20, 2026 •

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 4bbc502).
Updated: 2026-07-20T10:37:54.400Z

Files in the reviewed diff
pyre/pyre-interpreter/src/module/_collections/mod.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/_collections/mod.rs:24 ↔ pypy/module/_collections/interp_deque.py:48 — Pyre represents deque contents as one backing list; PyPy uses linked fixed-size Block objects. This pre-dates the patch and diverges from the upstream storage shape.

  • pyre/pyre-interpreter/src/module/_collections/mod.rs:59 ↔ pypy/module/_collections/interp_deque.py:107 — Pyre uses a wrapping i64 mutation generation, while PyPy invalidates an identity-bearing Lock. After counter wraparound, a stale Pyre iterator can theoretically become valid again; the volatile accesses also do not provide PyPy-equivalent synchronization under free-threading. This pre-dates the patch.

4. Structural adaptations

  • pyre/pyre-jit/src/eval.rs:1858 ↔ pypy/module/_collections/interp_deque.py:629 and pyre/pyre-jit/src/eval.rs:1860 ↔ pypy/module/_collections/interp_deque.py:697 — PyPy’s GC traces each iterator’s deque field through its normal W_Root object graph. Because Pyre’s W_DequeIter and W_DequeRevIter are Rust-allocated immortal wrappers, registering their generated pointer offsets is the necessary Rust GC adaptation. It preserves the same reachability relationship.

W_DequeIter and W_DequeRevIter are #[pyre_class] wrappers holding a traced
`deque` edge but were never wired into build_gc, so the native GC-root
completeness oracle aborts at init. They are `.allocate()`-immortal
(malloc_typed), which the marker skips, so register both in the immortal
`register_pyre_class_offsets` block alongside W_Enumerate/W_UnpackIter, and
re-export the two types from the _collections module so build_gc can name them.

Assisted-by: Claude
@youknowone
youknowone force-pushed the deque-iterator-gcroot branch from 37824cd to 4bbc502 Compare July 20, 2026 10:18
@youknowone

Copy link
Copy Markdown
Collaborator Author

Force-pushed a corrected revision (37824cdc73b → 4bbc5027b64).

The first revision registered the two deque iterators via register_pyre_class (marker-tid path). That clears the completeness oracle, but W_DequeIter/W_DequeRevIter are .allocate()-immortal (malloc_typed), and the marker skips malloc_typed objects — so the deque child was never actually forwarded and would be swept out from under a live iterator on a major collection (latent UAF). The oracle only checks that a tid is registered, not that the marker reaches the object, so it passed anyway.

Corrected to the immortal-root path register_pyre_class_offsets, next to W_Enumerate/W_UnpackIter, whose walker force-forwards the child on every collection. Verified with an explicit gc.collect() after building a global iterator over a deque (offsets path preserves it; the marker-tid path dropped it). dynasm 241/241, cranelift 241/241.

Note: PR #643 registers the same deque iterators via register_pyre_class and has the same latent gap; it should move to the offsets path too.

— commented by Claude

@youknowone
youknowone merged commit 508a5f3 into main Jul 20, 2026
30 of 31 checks passed
@youknowone
youknowone deleted the deque-iterator-gcroot branch July 20, 2026 15:29
youknowone added a commit that referenced this pull request Jul 20, 2026
#684 (register deque iterators as GC roots) and #685 (native _warnings,
which bundled its own deque-iterator registration) both landed on main and
each added a `_collections` re-export plus a build_gc registration for
W_DequeIter / W_DequeRevIter. Merged together they collide:

- `_collections/mod.rs` re-exports each type twice -> E0252, main fails to
  compile.
- `eval.rs` registers each iterator twice: #684 via the immortal
  `register_pyre_class_offsets` list and #685 via the managed
  `register_pyre_class` marker path.

W_DequeIter / W_DequeRevIter are `.allocate()`-immortal (malloc_typed),
which the marker skips, so the immortal `register_pyre_class_offsets`
registration (#684) is the effective one. Drop #685's managed registration
and the duplicate re-export.

Assisted-by: Claude
youknowone added a commit that referenced this pull request Jul 20, 2026
#685 and #684 each added `pub use deque_iter::W_DequeIter` /
`pub use deque_rev_iter::W_DequeRevIter` to _collections/mod.rs at different
offsets; both landed on main, so the names are now re-exported twice — E0252
`defined multiple times`, which breaks the Charon LLBC extraction and any
fresh pyre-interpreter compile. Keep the documented #684 block and remove the
bare duplicate.

Assisted-by: Claude
youknowone added a commit that referenced this pull request Jul 20, 2026
…tration

#684 (register deque iterators as GC roots) and #685 (native _warnings +
_tokenize) both landed on main. #684 added the deque iterators to the immortal
`register_pyre_class_offsets` list plus a `_collections` re-export; #685 added a
second `_collections` re-export and managed `register_pyre_class` registrations
for W_DequeIter, W_DequeRevIter, and W_TokenizerIter. Merged onto this branch:

- `_collections/mod.rs` re-exports W_DequeIter / W_DequeRevIter twice.
- `eval.rs` registers the deque iterators twice, once via the immortal offsets
  list and once via the managed marker path.

W_DequeIter, W_DequeRevIter, and W_TokenizerIter are all `allocate`-immortal
(malloc_typed), which the marker skips, so the managed `register_pyre_class`
registrations neither root their inline children nor belong in `pytype_to_tid`;
the extra tids 117-119 break the subclass-range alias and hierarchy asserts at
startup.

Drop the duplicate re-export and the three managed registrations, and add
W_TokenizerIter to `register_pyre_class_offsets` so its `readline` field is
rooted. The deque iterators were already in that list via #684.

Assisted-by: Claude
youknowone added a commit that referenced this pull request Jul 20, 2026
#685 and #684 each added `pub use deque_iter::W_DequeIter` /
`pub use deque_rev_iter::W_DequeRevIter` to _collections/mod.rs at different
offsets; both landed on main, so the names are now re-exported twice — E0252
`defined multiple times`, which breaks the Charon LLBC extraction and any
fresh pyre-interpreter compile. Keep the documented #684 block and remove the
bare duplicate.

Assisted-by: Claude
youknowone added a commit that referenced this pull request Jul 20, 2026
…ubclass ranges (#205 C2) (#646)

* pyre-jit-trace: make the trace walker generic over a WalkSym symbolic state

Route the FBW walker's symbolic-state access through a WalkSym trait and
thread a `Sym: WalkSym` type parameter through the walk/step entry points,
WalkContext, FbwWalkMode, and the dispatch helpers. The conversion is
distributed across the jitcode_dispatch directory-module submodules (arith,
branch, bridge_subwalk, diag, fbw_state, heapcache_ops, inline_call, mod,
residual_call, resume_snapshot, specialize, tests, vable_ops, vstack_mirror).

PyreSym field access becomes trait accessor calls; concrete PyreSym
parameters become the generic Sym. jd0 (PyreSym) monomorphizes to the prior
concrete walker.

Assisted-by: Claude

* majit: resolve build-time jitcode descrs via a shared global pool

Add a process-global RuntimeBhDescr pool (set_global_build_descr_pool +
GlobalDescrPool newtype) and JitCode::descr_at, which reads the per-jitcode
exec.descrs first and falls back to the global pool. Route the JitCodeMachine
descr reads (switch, untyped/typed inline_call, residual-call calldescrs,
runtime_bh_descr fielddescrs, call_target/call_assembler_target) through
descr_at, so build-time LLBC-extracted jitcodes with an empty per-jitcode
pool resolve through the shared pool.

Decode the typed inline_call_{r,ir,irf}_* family in JitCodeMachine dispatch.

Assisted-by: Claude

* jit: dormant jd1 unpackiterable driver groundwork and subclass-range GC numbering

Add the dormant jd1 (unpackiterable_driver) scaffolding: UnpackSym /
UnpackJitState (unpack_state.rs), register it as an inert second jitdriver,
wire the no-op merge-point hook (baseobjspace.rs, call.rs, eval.rs), and
resolve the portal jitcode by canonical key (jitcode_runtime.rs).

Install the build-time descr pool from ALL_DESCRS
(install_global_build_descr_pool) and add the resolution test.

Match interpreter subclass ranges to GC numbering (pyobject.rs, typedef.rs,
pyre-interpreter/lib.rs, eval.rs build_gc census). Add census entries for
W_Accumulate / W_ZipLongest (tids 111-112), which shift the coroutine /
coroutine-wrapper / dict-view-iterator tail to tids 113-115.

check.py: dynasm 225/225, cranelift 225/225, wasm 224/224.

Assisted-by: Claude

* jit: thread jd1 reds (w_iterator, items) through the unpackiterable merge-point hook

baseobjspace.py:31 unpackiterable_driver uses reds='auto', auto-collecting the
loop locals w_iterator and items. The pyre meta-tracer cannot auto-collect Rust
locals, so pass them explicitly from the marker (baseobjspace.rs) through
unpack_merge_point (call.rs) to the hook (eval.rs unpack_merge_point_jit). The
hook body remains a no-op; behavior unchanged. cargo check -p pyre-jit clean.

Assisted-by: Claude

* gc: restore W_Deque (tid 116) in the subclass-range census

build_gc registers `collections.deque` W_Deque via register_pyre_class at the
tail of the type-id chain, after the coroutine and dict-view-iterator slots, so
it takes the current max GC type id 116. The subclass-range census enumerated
the object hierarchy only through the dict-view iterators (tid 115), so the
startup assertion in build_gc — actual GC vtable registrations against the
shared alias census — failed on the extra tid-116 registration, panicking at
every process start.

Add (116, Some(0)) to SUBCLASS_RANGE_HIERARCHY and
subclass_range_alias(116, W_Deque) to
pyre_interpreter::all_subclass_range_aliases.

Assisted-by: Claude

* jit: WIP jd1 unpackiterable driver live-path drive (gated PYRE_JD1, walk SIGBUSes)

Replace the no-op unpack_merge_point_jit hook with a real drive of jd1's
extracted _unpackiterable_unknown_length loop, gated behind PYRE_JD1 (default
off; PYRE_JD1_THRESHOLD overrides the warmup count, PYRE_JD1_DEBUG logs each
stage).

On the hot iterator-type green key the hook:
  - resolves jd1's portal jitcode via portal_jitcode_for_key
    ("baseobjspace::_unpackiterable_unknown_length") + installs the global
    build-time descr pool,
  - force_start_tracing with a descriptor carrying index=Some(2) so
    elect_active_jitdriver_sd elects the novable jd1 over jd0, seeding
    w_iterator/items as the two Ref reds (live_values),
  - drives trace_jitcode_with_args_and_runtime through the production
    with_trace_ctx_and_token_resolver runtime with the reds as InputArg
    argboxes,
  - abort_trace(false) to discard the recorded trace (residual side effects
    already landed on the shared reds).

Verified so far (PYRE_JD1_THRESHOLD=1): the hook fires, the counter fires, and
force_start_tracing returns StartedTracing — jd1 is elected and the trace
session starts. The subsequent JitCodeMachine walk of the extracted body
SIGBUSes inside trace_jitcode_with_args_and_runtime (before it returns), so
residual execution is not yet working; the crash is independent of the runtime
(bare ClosureRuntime and the resolver runtime both SIGBUS).

Also found: _unpackiterable_unknown_length is rarely reached from Python —
list()/tuple()/set()/sorted()/[*x]/{*x}/starred-assign all drain via direct
next-loops that bypass this marker; only ~3 startup unpacks hit it.

Assisted-by: Claude

* jit: enter the jd1 trace at the merge point instead of the jitcode entry

`trace_jitcode_with_args_and_runtime` resets the frame pc to 0 in
`setup_call`, so driving the jd1 portal replayed
`_unpackiterable_unknown_length`'s prologue (`length_hint` / `newlist` /
`iterator_greenkey`) with `items` seeded where the function's
`w_iterable` argument belongs, faulting inside `length_hint`.

Add `trace_jitcode_from_merge_point`: it decodes the `jit_merge_point`
op's green/red register lists at the header pc, seeds the green ref as a
Const and each red ref as its InputArg into those registers, and starts
the walk at the header pc (code_cursor) with the prologue skipped.

Wire `drive_unpack_iterable_trace` to it, threading the raw greenkey
pointer through the merge-point hook for the green seed.

Add gated diagnostics: `MAJIT_OPTRACE` per-op cursor/pc/opcode trace in
`run_to_end`, and `PYRE_JD1_DUMP` op-layout dump of the extracted body.

The walk now executes the merge point and reaches the first residual
call; the residual funcptr constant for `next` is unpatched
(`jit_trace_fnaddrs` omits the unpack-loop callees), so the call still
faults — fn-address resolution is a follow-up.

Assisted-by: Claude

* jit: bind jd1 unpack-loop residual callees (next, w_list_append)

The `unpackiterable_driver` body residual-calls `next(w_iterator)` and
`w_list_append(items, w_item)` directly in source, so the codewriter records
the bare source paths. Without a runtime binding their funcptr constants fall
back to a `symbolic_fnaddr_for_path` hash the residual handler cannot resolve,
faulting when the merge-point walk executes the call.

Add `bh_next`, a Ref-returning `#[jit_may_force]` bridge that publishes the
raised exception (StopIteration included, unlike `jit_next`, whose null-for-
StopIteration convention serves FOR_ITER's GuardNonnull). Register it under the
`next` source paths and bind `w_list_append` (a `-> ()` residual) directly in
`jit_trace_fnaddrs`.

Assisted-by: Claude

* interpreter: dedup and correct the #684/#685 deque/tokenizer GC registration

#684 (register deque iterators as GC roots) and #685 (native _warnings +
_tokenize) both landed on main. #684 added the deque iterators to the immortal
`register_pyre_class_offsets` list plus a `_collections` re-export; #685 added a
second `_collections` re-export and managed `register_pyre_class` registrations
for W_DequeIter, W_DequeRevIter, and W_TokenizerIter. Merged onto this branch:

- `_collections/mod.rs` re-exports W_DequeIter / W_DequeRevIter twice.
- `eval.rs` registers the deque iterators twice, once via the immortal offsets
  list and once via the managed marker path.

W_DequeIter, W_DequeRevIter, and W_TokenizerIter are all `allocate`-immortal
(malloc_typed), which the marker skips, so the managed `register_pyre_class`
registrations neither root their inline children nor belong in `pytype_to_tid`;
the extra tids 117-119 break the subclass-range alias and hierarchy asserts at
startup.

Drop the duplicate re-export and the three managed registrations, and add
W_TokenizerIter to `register_pyre_class_offsets` so its `readline` field is
rooted. The deque iterators were already in that list via #684.

Assisted-by: Claude
youknowone added a commit that referenced this pull request Jul 20, 2026
#684 re-exported W_DequeIter / W_DequeRevIter at the top of the module while
#685 had already re-exported the same two names below the `mod deque_rev_iter`
block, so the merged tree carries two `pub use` of each and fails to compile
(E0252). Remove the lower pair; the top pair keeps its build_gc-root comment.

Assisted-by: Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant