jit: root the forced-virtual caches, store them inside handle_async_forcing, and zero PyFrame.vable_token on JIT-allocated frames - #902
Conversation
WalkthroughForced virtual forcing now stores cache data internally with its virtualizable key. New driver and metainterpreter APIs support GC walking and pruning, while per-mutator GC hooks track cached references and remove entries owned by dead frames. Resume tests cover ChangesForced virtual cache lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant force_pyframe
participant JitDriver
participant MetaInterp
participant ResumeData
participant GC
force_pyframe->>JitDriver: force_virtualizable_token(token)
JitDriver->>MetaInterp: force_virtualizable_token_with_allocator(token)
MetaInterp->>ResumeData: force_from_resumedata(...)
ResumeData-->>MetaInterp: virtual caches and virtualizable pointer
MetaInterp->>MetaInterp: save_forced_virtuals(...)
GC->>JitDriver: walk_forced_virtuals_refs(visitor)
GC->>JitDriver: prune_forced_virtuals(classify)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 248e9a7). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5374de06b6
ℹ️ 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".
| /// Unmaterialized `0` cache slots pass through unchanged, as they do in | ||
| /// `shadow_stack::walk_resume_ref_roots`. | ||
| pub fn walk_forced_virtuals_refs(&mut self, mut visitor: impl FnMut(&mut GcRef)) { | ||
| for (_owner, ptrs, _ints) in self.forced_virtuals.iter_mut() { |
There was a problem hiding this comment.
If a minor collection occurs after asynchronous forcing but before GUARD_NOT_FORCED consumes the cache, the live PyFrame owner can be forwarded through its normal roots while this walker updates only ptrs and leaves _owner at its from-space address. take_forced_virtuals(frame as u64) then misses the cache under the frame's new address and may reconstruct duplicate virtuals; an unconsumed young-owner entry also cannot later be classified as dead old-gen state and can retain its values indefinitely. Re-key the weak owner from the collector's forwarding information during the minor walk.
Useful? React with 👍 / 👎.
| // before `force_all_virtuals` allocates: unlike `blackhole_from_resumedata`, | ||
| // the bare `prepare` above opened no resume-root scope, so there is nothing | ||
| // to forward the reader's slot in place. | ||
| let virtualizable_ptr = resumereader.virtualizable_ptr; |
There was a problem hiding this comment.
When the forced virtualizable is nursery-resident and materializing a later virtual triggers a minor collection, copying virtualizable_ptr before force_all_virtuals() does not keep that raw integer synchronized with the frame's forwarded address. The returned cache is consequently saved under a stale from-space key, so the following forced-guard resume looks it up using the live frame address and misses, potentially rebuilding objects that were already materialized and exposed. Keep the reader's virtualizable slot in a resume-root scope through materialization and read it afterward.
Useful? React with 👍 / 👎.
| JIT_DRIVER.with(|cell| { | ||
| let data = cell as *const _ as *const (); | ||
| // SAFETY: same re-derivation and same aliasing caveat as the root | ||
| // walkers above (`jit_driver_pair_from_root_area`). The pruner runs on | ||
| // the collecting thread, so it reaches only that thread's driver. |
There was a problem hiding this comment.
During a cross-thread major collection, the root phase walks forced caches from every registered mutator via walk_all_extra_areas, but this pruner consults JIT_DRIVER.with and therefore examines only the collecting thread's TLS. A live idle worker's cache whose owner has died is never removed when majors keep running on other threads, so its materialized objects remain pinned and its stale frame-address key can later collide with a reused address on that worker. Pruning must iterate the same registered driver areas as the root walk rather than caller TLS.
AGENTS.md reference: AGENTS.md:L148-L162
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-metainterp/src/pyjitpl.rs (1)
11662-11789: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSite where the (potentially stale)
virtualizable_ptrbecomes the permanent cache key.
save_forced_virtuals(virtualizable_ptr as u64, ...)stores the pointer returned byforce_from_resumedataverbatim as theforced_virtualsowner key. See the consolidated comment (anchored atresume.rs#L7654-L7662) for the shared concern about this value's validity across a GC boundary.Independent of that: no unit test in this PR exercises
save_forced_virtuals/take_forced_virtuals/prune_forced_virtuals/walk_forced_virtuals_refsdirectly (the new tests inresume.rsonly cover the resume-decoding side). Given these are new, GC-sensitive code paths, a couple of focused unit tests (hit/miss ontake_forced_virtuals, retain/drop onprune_forced_virtuals) would materially increase confidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 11662 - 11789, Ensure the owner key passed to save_forced_virtuals in handle_async_forcing remains valid across GC and is not derived from a stale virtualizable_ptr returned by force_from_resumedata. Add focused unit tests for take_forced_virtuals hit/miss behavior and prune_forced_virtuals retaining live entries while dropping obsolete ones, covering the new forced-virtual cache paths directly.majit/majit-metainterp/src/resume.rs (1)
7621-7662: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep
virtualizable_ptrforward-safe before caching it as cache identity.force_from_resumedata()is deliberately not root-scoped, butforce_all_virtuals()can runBlackholeAllocatorassignments and trigger a minor collection that moves the named frame. This leaves thevirtualizable_ptrused bysave_forced_virtuals(),take_forced_virtuals(), andprune_forced_virtuals()as a possibly-stale from-space address. Use theblackhole_from_resumedatapattern of rooting the reader slot over the force-window and reading it back afterward, or otherwise force/promote/pin the virtualizable before creating theforced_virtualskey.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-metainterp/src/resume.rs` around lines 7621 - 7662, Keep the virtualizable cache key forwarding-safe across the force window: in resume.rs:7621-7662, update force_from_resumedata to root the reader’s virtualizable slot using the established blackhole_from_resumedata pattern before force_all_virtuals, then read the forwarded pointer afterward; apply the corresponding handling in pyjitpl.rs:1869-1910 and pyjitpl.rs:11662-11789 where the pointer is passed to save_forced_virtuals, take_forced_virtuals, or prune_forced_virtuals, ensuring those operations receive the post-collection address.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 11662-11789: Ensure the owner key passed to save_forced_virtuals
in handle_async_forcing remains valid across GC and is not derived from a stale
virtualizable_ptr returned by force_from_resumedata. Add focused unit tests for
take_forced_virtuals hit/miss behavior and prune_forced_virtuals retaining live
entries while dropping obsolete ones, covering the new forced-virtual cache
paths directly.
In `@majit/majit-metainterp/src/resume.rs`:
- Around line 7621-7662: Keep the virtualizable cache key forwarding-safe across
the force window: in resume.rs:7621-7662, update force_from_resumedata to root
the reader’s virtualizable slot using the established blackhole_from_resumedata
pattern before force_all_virtuals, then read the forwarded pointer afterward;
apply the corresponding handling in pyjitpl.rs:1869-1910 and
pyjitpl.rs:11662-11789 where the pointer is passed to save_forced_virtuals,
take_forced_virtuals, or prune_forced_virtuals, ensuring those operations
receive the post-collection address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a6993405-a856-4fa6-8135-96cd1a5da7f6
📒 Files selected for processing (4)
majit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/resume.rspyre/pyre-jit/src/eval.rs
|
Adjudicating the three Codex findings. Two refuted on a shared premise, one correct and fixed in P1 "Re-key the forced-cache owner during minor collections" — refutedThe premise is that the The frames a trace builds do go through a movable path — Rather than leave that as an argument, it is now a checked invariant — The second half of the finding — that an unconsumed young-owner entry can never be classified dead — is the documented contract of this mechanism, not new: the classifier keeps every owner outside old-gen, because "An owner outside old-gen is either immortal ( P1 "Root the virtualizable key while forcing virtuals" — refutedSame premise, same refutation: the virtualizable is move-stable, so copying Reading it before the materialization is also deliberate, not incidental. The sibling P2 "Prune caches from every mutator's driver" — correct, fixedRight, and citing the in-repo contract makes it sharper: Fixed by giving the mechanism the missing half: One correction to the suggested shape, found by asserting rather than assuming: pruning cannot unconditionally iterate all mutators. The pre-sweep point does not own collector-side STW — the unconditional form tripped The mapdict tables stay on the global registration — they are a process-global Noted, not touched
Verification after the fix
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78e30de90d
ℹ️ 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".
| // resume.py:1351: return resumereader.force_all_virtuals() | ||
| let (ptrs, ints) = resumereader.force_all_virtuals(); | ||
| (ptrs.to_vec(), ints.to_vec()) | ||
| (ptrs.to_vec(), ints.to_vec(), virtualizable_ptr) |
There was a problem hiding this comment.
In free-threaded execution, when forcing materializes multiple virtuals and another mutator starts a major collection, a later backend allocation enters gc_sync::gc_op and can quiesce this thread after an earlier virtual has been placed only in resumereader.virtuals_cache. The new permanent walker cannot see that cache until this function returns and save_forced_virtuals runs, while the plain prepare above did not register a ResumeRefRootsScope; consequently the major can sweep the earlier old-generation virtual and this return hands a dangling address to the saved cache. Keep prepare_resume_heap_with_roots's scope alive through force_all_virtuals, as the blackhole construction path does.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/pyjitpl.rs (1)
1869-1910: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNew GC walker/pruner for
forced_virtualslook correct but have no direct unit coverage in this file.
walk_forced_virtuals_refscorrectly walks only the ptr half via the same reinterpret-cast pattern used bywalk_resume_ref_roots, andprune_forced_virtualscorrectly retains only entries whose owner still classifies to the same address. Both are new publicMetaInterpsurface consumed by GC/driver code not in this review batch. Consider adding a focused unit test in this file that populatesforced_virtuals, exerciseswalk_forced_virtuals_refs(verifying the visitor sees exactly the ptr slots) andprune_forced_virtuals(verifying dead-owner entries are dropped and live ones survive), mirroring the existingtest_handle_async_forcing_*style tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 1869 - 1910, Add focused unit coverage near the existing test_handle_async_forcing_* tests for MetaInterp::walk_forced_virtuals_refs and MetaInterp::prune_forced_virtuals. Populate forced_virtuals with distinct pointer and integer slots, verify the walker visits exactly the pointer slots, then classify owners so dead entries are removed while live entries remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 1869-1910: Add focused unit coverage near the existing
test_handle_async_forcing_* tests for MetaInterp::walk_forced_virtuals_refs and
MetaInterp::prune_forced_virtuals. Populate forced_virtuals with distinct
pointer and integer slots, verify the walker visits exactly the pointer slots,
then classify owners so dead entries are removed while live entries remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7e2076c-e5d6-46f2-807e-84e9f543ce3a
📒 Files selected for processing (4)
majit/majit-gc/src/collector.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-metainterp/src/pyjitpl.rspyre/pyre-jit/src/eval.rs
…c_forcing Two review findings on `MetaInterp::forced_virtuals`, the stand-in for the `AllVirtuals` upstream hides in the deadframe's `jf_savedata` word. Rooting. The ptr half holds what `force_all_virtuals` (resume.py:969-981) materialized until the following GUARD_NOT_FORCED consumes it, and it was in no root set: a major collection inside that window frees any old-generation object without `flags::VISITED` (`OldGen::sweep_arenas_step`), and a virtual named only by a resume frame's ref registers has this Vec as its only referent. Upstream gets the edge from `jf_savedata` being traced as a GCREF field (`majit-backend/src/jitframe.rs:354`; the previous citation, :278, is `JitFrame::init`). Enroll it as the fifth `register_mutator_extra_area` member alongside rd_consts / partial_trace / active_trace / compile_snapshot, walking only the ptr half — the int half is unboxed field values. Register an ephemeron pruner, like `mapdict::prune_dead_owner_entries`, so an entry the guard never consumes goes when its owner frame is swept instead of pinning its objects and leaving a key a recycled PyFrame address could match. `EphemeronPrunerFn` carries no data pointer, so the pruner reaches only the collecting thread's `JIT_DRIVER`. Store placement. `compile.py:996-1000` calls `set_savedata_ref` inside `handle_async_forcing`; pyre did it at the `force_pyframe` hook, so `force_pyframe_vref` — the materializing arm of `virtualref.py:134 force_virtual_if_necessary`, which calls `force_virtualizable_token` as a statement — dropped the cache. `force_from_resumedata` now also returns the virtualizable its vable section named (resume.py:1404) and `handle_async_forcing` keys the store on it, so both entry points are covered and the key comes from the resume data rather than the caller. `force_virtualizable_token` returns nothing, like `force_now`. Adds a `[jit][take_forced_virtuals] hit/miss` counterpart to the existing `handle_async_forcing` log, and two tests for the `all_virtuals = Some(..)` resume: `consume_vref_and_vable` jumps the vable and vref sections (resume.py:1433-1435), and `_prepare_virtuals` zeroes a preloaded cache (resume.py:990-991), which is why the caller passes rd_virtuals as None. Assisted-by: Claude
`MetaInterp::forced_virtuals` was pruned through
`register_ephemeron_pruner`, which hands the classifier no way to name a
thread, so the pyre side had to read `JIT_DRIVER` from caller TLS and saw
only the collecting thread's driver. That contradicts the contract stated
on `MutatorExtraWalkFn` -- "must derive every thread-specific address from
`data`, never from caller TLS" -- and left another mutator's dead-owner
entries pinned.
Add `register_mutator_pruner` / `prune_all_mutator_areas` /
`prune_my_mutator_areas` next to the extra-area equivalents, stored in the
same `MutatorEntry`. The collector calls them from the pre-sweep point it
already prunes at, with the same classifier, and picks between all-mutator
and own-mutator on `gc_sync::mutators_quiesced()` -- the same predicate
`do_collect_nursery` and `enumerate_root_walker_values` use, so a
collection's prune reach always equals its own root-walk reach. That
branch is load-bearing: the unconditional form tripped the quiescence
assertion, because the pre-sweep point does not own STW.
pyre registers the pruner on the same `jit_driver` data as
`forced_virtuals_root_walker_area`. The mapdict tables stay on the global
registration -- they are a process-global `Mutex` map, reachable from any
thread.
Also assert in `save_forced_virtuals` that the owner is not
nursery-resident. The entry is keyed by a bare address, which is only
sound because the virtualizable comes from `FrameBox::new` ->
`try_gc_alloc_stable_raw` ("stable across minor and major collections"),
never from the frames a trace builds virtually. A debug run of the
getframe force fixture completes 11 majors and 44 minors with five forces
and five cache hits, and the assertion does not fire.
Assisted-by: Claude
`force_pyframe` and `force_pyframe_vref` reach the same `handle_async_forcing`, and nothing downstream distinguished them, so a census of async forcing could not say which hook produced an event. That is how the vref hook silently kept the store the frame hook had. Three lines, all behind `majit_log_enabled`: hook entry for the vref (distinct from the token arm, since a vref built during tracing carries `forced` already set and `virtual_token = TOKEN_NONE` and returns without running the closure -- counting only the closure conflates "never reached" with "reached and short-circuited"), the token arm itself, and the frame hook. Census over the 330 runnable synth fixtures with these: 5 async-force events, all from getframe_caller_locals_nested_compiled_callee, all consumed by their GUARD_NOT_FORCED; 4 of the 5 materialize an empty cache. The vref hook is entered 195 times across 6 fixtures and takes the token arm 0 times. Assisted-by: Claude
`rvirtualizable.py:29` declares `('vable_token', llmemory.GCREF)`, so
upstream's `gc_fielddescrs` names it and `rewrite.py:498-504
clear_gc_fields` zeroes the slot behind every `new`. pyre types the field
`usize` and `PYFRAME_DESCR_GROUP`'s positional census did not list it, so
a JIT-inlined `NewWithVtable(pyframe_size_descr())` left it holding
recycled nursery bytes — `Nursery::reset` does not zero-fill and the
`CallMallocNursery` fast path clears only the header word.
`emit_force_virtualizable` (`pyjitpl.py:1148-1158`) then read those bytes
with `GETFIELD_GC_R`; the backend spilled the result to a jitframe
ref-root slot and marked it live in the call site's gcmap, so the next
minor collection dereferenced them and aborted with
`GC BUG: invalid type_id` (`synth/mutate_then_raise_caught`, cranelift).
Carry the edge through a new `build_object_descr_group_with_extra_gc_edges`
lane, the one the inherited `PyObject.w_class` edge already uses, which
adds to `gc_fielddescrs` without disturbing the positional
`all_fielddescrs` that `field_descr_from_group` indexes.
Assisted-by: Claude
`emit_empty_list_inline` and `w_list_size_descr`'s doc both stated that `NewWithVtable` leaves the untouched slots zero-filled. It does not: the nursery is not zero-filled (`incminimark.py:211 malloc_zero_filled = False`) and the only zeroes an inline allocation gets are the pending stores `clear_gc_fields` derives from `gc_fielddescrs`. `items`, `int_items.block` and `float_items.block` are GC-pointer fields of the descr and so are covered; `length` is a plain int and was not. Store `length = 0` explicitly, as `rlist.py ll_newlist` does, and restate both comments in terms of the mechanism that actually clears the slots. Assisted-by: Claude
#907 added a second `#[cfg(not(feature = "sandbox"))] fn interp_return_log_enabled` with the same body next to the existing one, so `pyre-interpreter` fails to compile with E0428 on main. Keep the first definition and its doc comment; delete the duplicate. Assisted-by: Claude
78e30de to
248e9a7
Compare
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 248e9a7cac
ℹ️ 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".
| for (_owner, ptrs, _ints) in self.forced_virtuals.iter_mut() { | ||
| for slot in ptrs.iter_mut() { |
There was a problem hiding this comment.
When a forced-cache entry is left unconsumed and its frame becomes unreachable before a major collection, this unconditional root walk marks every cached virtual before finish_incremental_marking invokes the owner pruner (collector.rs:2878-2882). Removing the entry at that point cannot clear the objects' VISITED bits, so the entire materialized graph survives the sweep and remains retained until another major collection; a large abandoned cache can therefore cause substantial memory retention or OOM. Treat the table as an ephemeron by marking values only for owners proven live, or attach the cache to the deadframe so it naturally shares the owner's lifetime.
AGENTS.md reference: AGENTS.md:L186-L192
Useful? React with 👍 / 👎.
1 participant
Follow-up to #899, acting on its review. Two findings on
MetaInterp::forced_virtuals— pyre's stand-in for theAllVirtualsupstream hides in the deadframe'sjf_savedataword — plus the unit test coderabbit asked for.Sections 4-6 were added later: chasing this PR's failing
pyre/check.py (macos-latest)job led to a GC-corrupting descr-census gap that was already red onmain, plus one adjacent hardening and themainbuild fix the branch needs to be gated at all.1. The cache was in no root set
The ptr half holds what
force_all_virtuals(resume.py:969-981) materialized, and it lives from the force until the followingGUARD_NOT_FORCEDconsumes it. Nothing rooted those slots.The exposure is reclamation, not relocation: virtuals are allocated non-moving (
dynasm_alloc_oldgen_typed), so there is nothing to forward — butOldGen::sweep_arenas_stepfrees any old-generation object that lacksflags::VISITED, andfinish_alloc_in_oldgensets that flag only when a marking cycle is already in progress. A virtual named only by a resume frame's ref registers is written nowhere else at force time, so thisVecis its only referent. Upstream is immune becausejf_savedatais traced as a real GCREF field (majit-backend/src/jitframe.rs:354— the previous citation,:278, isJitFrame::init, and is corrected here).Fix: enroll it as the fifth
register_mutator_extra_areamember, alongsiderd_consts/partial_trace/active_trace/compile_snapshot. Only the ptr half is walked — the int half isvirtuals_int_cache, unboxed integer field values, and handing those to the visitor would test integers as heap addresses.prepare_resume_heap_with_rootsroots the same one half for the same reason.push_resume_ref_rootscannot serve here: it snapshots(ptr, len)and releases by depth truncation, and this window spans a return to compiled code — every interveningDropwould truncate past the force-time push.residual_call.rs:589-594already documents that and solves it with a permanent extra area.Rooting alone would leak, so an ephemeron pruner is registered too, modelled on
mapdict::prune_dead_owner_entries: an entry the guard never consumes is dropped when its owner frame is swept, which is the lifetimejf_savedatagets for free by living on the deadframe. Without it an unconsumed entry pins its objects for the process lifetime and leaves a stalePyFramekey that a later frame at the same address could fish.The pruner is registered per mutator, not through the global
register_ephemeron_pruner. That registration hands the classifier no way to name a thread, so it would have forced the pyre side to readJIT_DRIVERfrom caller TLS and see only the collecting thread's driver — contradicting the contract stated onMutatorExtraWalkFn("must derive every thread-specific address fromdata, never from caller TLS") and leaving another mutator's dead-owner entries pinned. Soregister_mutator_pruner/prune_all_mutator_areas/prune_my_mutator_areasjoin the extra-area equivalents in the sameMutatorEntry, and the collector picks between all-mutator and own-mutator ongc_sync::mutators_quiesced()— the same predicatedo_collect_nurseryandenumerate_root_walker_valuesuse, so a collection's prune reach always equals its own root-walk reach. That branch is load-bearing: the unconditional form tripped the quiescence assertion, because the pre-sweep point does not own STW. The mapdict tables stay on the global registration; they are a process-globalMutexmap, reachable from any thread.The entry is keyed by a bare address, which is sound only because the virtualizable is move-stable: it comes from
FrameBox::new→try_gc_alloc_stable_raw, whose registered contract is "stable across minor and major collections (MiniMark mark-sweep does not move old-gen objects)", never from the frames a trace builds virtually (emit_new_pyframe_inline_with_params→NewWithVtable, which can land in the nursery).save_forced_virtualsnow asserts that, so the invariant is checked rather than argued.2.
force_pyframe_vrefdropped the cachecompile.py:996-1000callsset_savedata_refinsidehandle_async_forcing. #899 did it at theforce_pyframehook instead, soforce_pyframe_vref— the materializing arm ofvirtualref.py:134 force_virtual_if_necessary, which callsforce_virtualizable_tokenas a statement — discarded what the force produced, and itsGUARD_NOT_FORCEDrebuilt the virtuals fromrd_virtuals.Fix: move the store inside, as upstream has it.
force_from_resumedatanow also returns the virtualizable its vable section named (resume.py:1404), andhandle_async_forcingkeys the store on that — so the key comes from the guard's own resume data rather than from a caller-side guess, and both force entry points are covered by construction.force_virtualizable_tokenreturns nothing, likeforce_now.3. Tests and a diagnostic
Two tests for the
all_virtuals = Some(..)resume:consume_vref_and_vablejumps the vable and vref sections (resume.py:1433-1435) — asserted by decoding the same resume data twice and checking that the ordinary path surfaces the virtualizable while theGUARD_NOT_FORCEDpath surfaces none and the frame section behind them still decodes (which is what proves the jump lengths line up)._prepare_virtualszeroes a preloaded cache (resume.py:990-991), which is whyblackhole_from_resumedatamust passrd_virtuals/rd_guard_pendingfieldsasNoneon that path.Plus a
[jit][take_forced_virtuals] owner=.. hit/misscounterpart to the existinghandle_async_forcingline. Only aGUARD_NOT_FORCEDreaches it (is_guard_forced()gates the callers), so hit/miss is the force→resume handoff itself.Two review findings adjudicated as false positives
jitdriver.rs:3766/6209passall_virtuals = None. Those are the majit state-field macro JIT's resume paths. That JIT has no virtualizable force — its virtualizable is a host-stack&state,force_virtualizable_tokenis never called on it, andhandle_async_forcingis unreachable there — so there is never a cache to hand over.Noneis correct, not a dropped edge.ragnf = 2and an empty cache. It resumes withNoneinstead, i.e. it rebuilds. That is strictly the safer of the two:ragnf = 2with an empty cache would make anyTAGVIRTUALreference index an emptyvirtuals_cache, whereas rebuilding produces a correct (if redundant) object set. This mirrorscompile.py:959-960, which also tolerates an absent cache.4.
PyFrame.vable_tokenwas never zero-initialized on a JIT-allocated frameFound while chasing the
synth/mutate_then_raise_caughtabort that was alreadyred on
main(pyre/check.pycranelift 344/345, and themacos-latestjob onthis PR):
GC BUG: invalid type_id=… site=minor_custom_trace_target, reachedfrom
gc_alloc_nursery_shim.The nursery is deliberately not zero-filled —
Nursery::resetmirrorsincminimark.py:211 malloc_zero_filled = False— and the backend'sCallMallocNurseryfast path clears only the 8-byte header word. So the wholezero-initialization of a JIT-inline-allocated object is the pending stores that
rewrite.py:498-504 clear_gc_fieldsderives fromdescr.gc_fielddescrs().rvirtualizable.py:29appends('vable_token', llmemory.GCREF)to thevirtualizable's own fields, so upstream's
gc_fielddescrsnames it and the slotis cleared on every
new. pyre types the fieldusizeandPYFRAME_DESCR_GROUP's positional census never listed it, soemit_new_pyframe_inline_with_params'NewWithVtable(pyframe_size_descr())left it holding bytes from the previous nursery cycle.
emit_force_virtualizable(pyjitpl.py:1148-1158) then read them withGETFIELD_GC_R; the backend spilled that Ref into a jitframe ref-root slot andmarked it live in the call site's gcmap, so the next minor collection
dereferenced them.
Fix: carry the edge through a new
build_object_descr_group_with_extra_gc_edgeslane — the one the inherited
PyObject.w_classedge already uses — which adds togc_fielddescrswithout disturbing the positionalall_fielddescrsthatfield_descr_from_groupindexes. Adding it positionally instead would risk adescr split.
How it was pinned down
The panic's neighbourhood dump is what settled it. Reading each word around
obj_addras a header and stepping by that type's size landed exactly on thenext candidate header (
0 → 8 + 24 = 32), which provesobj_addrwas a headeraddress and the real object was
obj_addr + 8— so the slot held a bad value,not a stale pointer. From there: the holder's
jf_gcmapbits gave the slot(
bit = max_output_slots + slot,holder_offset = 64 + 8*bit), the backend'sref_root_slotsnamed the producing var, and its defining op wasGcLoadR(v651, #80)off aCallMallocNursery(240)whose offset 80 had no store.The
GcStore(p, ofs, #0)ops already in the trace are theclear_gc_fieldspending zeros, so "which offsets got a zero" reads the census straight off the
trace.
It looked nondeterministic because it reproduces 5/5 under non-tty stdio
(
capture_output=True, stdin=DEVNULL) and 0/100 standalone — stdio setup shiftsthe allocation history, hence what the recycled bytes happen to hold.
Audit of the same class
Every hand-written descr group that JIT code can allocate was checked against its
Rust struct's GC pointers.
PyFramewas the only gap.PyFramevable_tokenW_ListObjectitems,int_items.block,float_items.blockType::RefPyTracebackw_class,frame,w_next,w_codeW_FloatObjectw_dict,w_slots(forwarded unconditionally)Type::RefW_LongObjectvalueType::RefW_IntObject,W_TupleObject,Method,W_SliceObject, specialised tuplesW_DictObject(dstrategy,dstorage) andW_ObjectObject(storage) haveincomplete censuses, but neither has a
*_size_descr()accessor at all, so noallocation op can carry them — unreachable today, not fixed here.
5. The inline empty list assumed a memzero that does not happen
Same false premise, one level down:
emit_empty_list_inlineandw_list_size_descr's doc both stated thatNewWithVtableleaves untouched slotszero-filled.
items/int_items.block/float_items.blockare GC-pointerfields of the descr and so are covered by
clear_gc_fields;lengthis a plainint and was not. It is unreachable today —
set_object_items_from_vecresetslengthon the Empty→Object transition and the typed strategies read*_items.leninstead — so this is hardening, not a live bug. Stored explicitly,as
rlist.py ll_newlistdoes, and both comments restated in terms of themechanism that actually clears the slots.
6.
maindid not compile#907added a second#[cfg(not(feature = "sandbox"))] fn interp_return_log_enabledwith an identical body next to the existing one, sopyre-interpreterfails E0428. The branch cannot be built or gated withoutremoving it, so the duplicate is dropped here.
Verification
Witness for the handoff, on the #899 fixture
getframe_caller_locals_nested_compiled_callee:Identical to the pre-change measurement, which is what confirms the new key (the decoded virtualizable) resolves to the same frame the consumer looks up.
The
mutate_then_raise_caughtabort of section 4 reproduces 5/5 onorigin/mainwithout any commit from this branch, and 0/30 with the fix.Gate, rebased onto
origin/main25b2442c4e:check.pydynasm 345/345, cranelift 345/345, wasm 341/341. Before section 4 the same tree was cranelift 344/345 — that one failure is this bug. Earlier in the branch: differential JIT-vs-PYRE_NO_JITcorpus 329/329 on both backends ·cargo test -p majit-metainterp --lib1418 passed ·cargo fmt --checkclean · type-checks on dynasm, cranelift and wasm32.Honest scope note: the current corpus produces
forces == hits— every entry is consumed, so the pruner never fires and no collection lands inside the rooting window. Both are structural fixes; the paths that leave an entry unconsumed are the escaped-virtualizable raise andhandle_fail's bridge-compiled arm.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
GUARD_NOT_FORCEDresumption when virtualizable values are present, ensuring decoding stays aligned.Improvements