jit(fbw): emit ExecutionContext.enter/leave at the inlined-call push - #796
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR centralizes virtual-reference tracing in ChangesVirtual-reference tracing and execution-context flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InlineWalker
participant TraceCtx
participant ExecutionContext
participant CompiledExecutor
InlineWalker->>TraceCtx: record virtual-reference enter
TraceCtx->>ExecutionContext: update topframeref and frame-chain link
InlineWalker->>TraceCtx: finalize virtual-reference leave
CompiledExecutor->>ExecutionContext: save topframeref
CompiledExecutor->>ExecutionContext: restore topframeref
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
![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
https://github.com/youknowone/pyre/blob/feefdcd2e446b3498025dac2b1f53830c4dce5b3/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs#L1653
When a nested seeded callee frame escapes (for example, it is retained via sys._getframe()), this branch marks and forces frames only in the recording-time objects; the emitted IR proceeds directly to VIRTUAL_REF_FINISH with a null object. The optimizer therefore leaves the caller vref's forced field null on the compiled path, so a later escaped_frame.f_back reaches force_pyframe_vref and panics with InvalidVirtualRef. Emit the escaped/exception check, caller escape propagation, and frame-vref force before finishing the vref.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ 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".
| let vref_ptr = self | ||
| .metainterp_sd | ||
| .virtualref_info | ||
| .virtual_ref_during_tracing(virtual_obj_ptr as *mut u8); |
There was a problem hiding this comment.
In a long-lived process that continues compiling traces or bridges, every seeded inline call through this newly reachable producer allocates a 24-byte JitVirtualRef using Box::into_raw, and no trace retirement or other owner ever reclaims it. Compilation events are not a finite bound for dynamic workloads, so JIT memory grows permanently with the number of recorded inline calls; use the collector-owned allocation used by the upstream lltype.malloc(self.JIT_VIRTUAL_REF) implementation.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit f06af59). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecbc636f73
ℹ️ 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".
| let live = ctx.virtualref_boxes_len(); | ||
| if live >= 2 { | ||
| ctx.stop_tracking_virtualref(live - 2); |
There was a problem hiding this comment.
When a may-force residual exposes this callee frame, vrefs_after_residual_call has already called stop_tracking_virtualref and replaced the vref box with ConstPtr(NULL). Since the same exposure marks the frame escaped, this block calls stop_tracking_virtualref again; the resulting VIRTUAL_REF_FINISH(NULL, callee_frame) is lowered by optimize_virtual_ref_finish to field stores through a null vref pointer, so the compiled trace can crash on leave. Check whether the top pair still contains a live vref before emitting the forced finish.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| let escaped = unsafe { | ||
| let frame_vref = (*concrete_ec).topframeref; | ||
| (*concrete_ec).topframeref = concrete_f_backref; | ||
| let escaped = (*concrete_frame).escaped() || got_exception; |
There was a problem hiding this comment.
For seeded inline frames whose escaped state differs on a later compiled execution, this recording-time-only test permanently selects either the forced or NULL VIRTUAL_REF_FINISH form. In particular, when the recording run sees escaped == false but a compiled run lets a nested frame retain this frame through f_backref, the emitted NULL finish clears the token without preserving forced; a later force_pyframe_vref then reaches InvalidVirtualRef and panics. Emit the frame.escaped || got_exception decision and caller propagation into the trace rather than specializing it from the concrete recording frame.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| index, | ||
| name: name.to_string(), | ||
| offset, | ||
| field_size: 8, |
There was a problem hiding this comment.
On wasm32, every new SetfieldGc of ExecutionContext.topframeref uses this descriptor's 8-byte width. The wasm backend only substitutes the 4-byte pointer width when is_pointer_field() is true, but this descriptor is deliberately Unsigned, so it emits an i64 store and overwrites the adjacent w_tracefunc field after topframeref. Use the target pointer width or otherwise provide the backend with a pointer-width store while retaining the required barrier behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ccff44d6
ℹ️ 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".
| if published as usize != restored_top | ||
| && unsafe { | ||
| majit_metainterp::virtualref::ptr_is_virtual_ref(published as *const u8) | ||
| } |
There was a problem hiding this comment.
When a guard fails inside an inlined callee, execute_assembler has already restored EC.topframeref to the saved portal frame at pyre-jit/src/eval.rs:7459-7460. That value is not a virtual ref, so this predicate refuses to install restored_top even though restored_virtualref_boxes contains the active callee scopes. The concrete bridge walk consequently exposes the portal frame to residual sys._getframe() calls and later closes the wrong concrete frame chain; republish the innermost restored vref whenever such a scope exists rather than requiring the currently published value to be another vref.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
| return gcref.0 as *mut u8; | ||
| } | ||
| } | ||
| Box::into_raw(Box::new(vref)) as *mut u8 |
There was a problem hiding this comment.
Although the normal allocation is now GC-owned, the new allocation-failure fallback still creates a Box<JitVirtualRef>. In production, if alloc_oldgen_typed returns null under memory pressure, the collector treats this address as unowned and therefore never traces its forced field; once EC.topframeref holds it, the wrapped seeded frame has no other GC-visible edge and can be reclaimed during a subsequent collection, leaving force_vref to return a dangling frame pointer. Propagate the allocation failure or use a collector-visible rooted allocation instead of this fallback.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/trace.rs (1)
1264-1292: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSave and restore
virtualref_boxesaround the non-committal carrier walk.
carrier_ec_leavepopsctx.virtualref_boxes; mid-loopbreakonly leaves the prior pops undone, and the discard path later callsctx.cut_trace(pre_pos), which only rewinds the recorder. A failed multi-frame carrier walk where the sub-walk recorded vref scope closures but was discarded leavesvirtualref_boxesshorter than the restored trace; save it before the sub-walk and restore it before returningp2_drain_abort().🤖 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 `@pyre/pyre-jit-trace/src/trace.rs` around lines 1264 - 1292, In the carrier walk around drive_middle_frame_and_thread, save the current ctx.virtualref_boxes state before any carrier_ec_leave calls. On the failed/discard path, restore that saved state after discarding the sub-walk and before returning p2_drain_abort(), ensuring the context matches the restored trace even when the loop exits early.pyre/pyre-jit/src/eval.rs (1)
7442-7461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe
topframerefbracket is not unwind-safe, unlike theCurrentFrameGuardit cites.The comment models this on
install_current_frame/CurrentFrameGuard, but those are RAII; this is a straight-line save/restore.run_compiled_detailed_with_bridge_keyedcan panic (strict-moderesume_unwind, backend/blackhole paths), and on that unwind Line 7459 never runs — leaving exactly the poisoned state this change exists to prevent: aJitVirtualRefpublished in the live slot, read as aPyFrameby whatever resumes, carrying a FORCE_TOKEN for a dead JIT frame. ADrop-based guard closes the hole for free.🛡️ Proposed fix
- let ec_for_topframeref = frame_root.frame().execution_context as *mut PyExecutionContext; - let saved_topframeref = if ec_for_topframeref.is_null() { - std::ptr::null_mut() - } else { - unsafe { (*ec_for_topframeref).topframeref } - }; + struct TopFrameRefGuard { + ec: *mut PyExecutionContext, + saved: *mut PyFrame, + } + impl Drop for TopFrameRefGuard { + fn drop(&mut self) { + if !self.ec.is_null() { + unsafe { (*self.ec).topframeref = self.saved }; + } + } + } + let ec_for_topframeref = frame_root.frame().execution_context as *mut PyExecutionContext; + let _topframeref_guard = TopFrameRefGuard { + ec: ec_for_topframeref, + saved: if ec_for_topframeref.is_null() { + std::ptr::null_mut() + } else { + unsafe { (*ec_for_topframeref).topframeref } + }, + };and drop the manual restore at Lines 7459-7461 (note the guard must outlive only the
outcomecomputation, so scope it accordingly).
🤖 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.
Inline comments:
In `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 1053-1085: Update TraceCtx’s active-trace root-walking path to
include every raw pointer stored in virtualref_boxes, forwarding and rewriting
virtual_obj_ptr and vref_ptr during collection. Ensure
vrefs_before_residual_call, vrefs_after_residual_call, and
opimpl_virtual_ref_finish read the updated pointers rather than stale cached
values, while preserving the existing virtualref_boxes pairing and indexing.
In `@majit/majit-metainterp/src/virtualref.rs`:
- Around line 498-501: Guard the gc_write_barrier call in continue_tracing with
majit_gc::gc_owns_object(vref_ptr), invoking the barrier only when the vref
address belongs to the GC. Preserve the existing tracing flow and avoid applying
the backend barrier to Box::into_raw fallback allocations.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Line 2700: Update the field_size expression for topframeref in the surrounding
descriptor definition to derive its size from the actual *mut PyFrame field type
rather than pyre_object::PyObjectRef, preserving the existing field(...)
configuration.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs`:
- Around line 47-51: Mark the `callee_ec` result from `record_op_with_descr` as
concrete before passing it to `walker_ec_leave`, using the pointer returned by
`root_sym.concrete_execution_context()`. Preserve the existing `GetfieldGcR`
recording and ensure residual-call and snapshot paths can resolve the stamped
execution context.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 2249-2251: Remove the redundant ca_concrete_frame declaration and
its seed-block assignment, then use concrete_callee_frame at the entered_ec and
leave sites. Preserve concrete_callee_frame as the single value carrying
concrete_frame_ptr outside the seed block.
- Around line 1712-1718: Capture the boolean returned by
TraceCtx::opimpl_virtual_ref_finish when processing callee_frame and assert it
succeeds without removing the call in release builds; use a let binding followed
by debug_assert!. Apply the same result-checking pattern to the live >= 2
stop_tracking_virtualref call so unbalanced virtual-reference operations are
detected at their source.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 1264-1292: In the carrier walk around
drive_middle_frame_and_thread, save the current ctx.virtualref_boxes state
before any carrier_ec_leave calls. On the failed/discard path, restore that
saved state after discarding the sub-walk and before returning p2_drain_abort(),
ensuring the context matches the restored trace even when the loop exits early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dba4c575-efee-4d3d-967d-88d7c1612c68
📒 Files selected for processing (13)
majit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rs
| index, | ||
| name: name.to_string(), | ||
| offset, | ||
| field_size: std::mem::size_of::<pyre_object::PyObjectRef>(), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
field_size derived from the wrong type for topframeref.
topframeref is *mut PyFrame, not pyre_object::PyObjectRef. The two happen to be the same width today, so this is inert, but the shared field(...) helper hides the mismatch — a future fat-pointer/tagged representation on either side would silently desync the descr from the struct.
🤖 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 `@pyre/pyre-jit-trace/src/descr.rs` at line 2700, Update the field_size
expression for topframeref in the surrounding descriptor definition to derive
its size from the actual *mut PyFrame field type rather than
pyre_object::PyObjectRef, preserving the existing field(...) configuration.
| let live = ctx.virtualref_boxes_len(); | ||
| if live >= 2 { | ||
| ctx.stop_tracking_virtualref(live - 2); | ||
| } | ||
| } | ||
| // `jit.virtual_ref_finish(frame_vref, frame)`. | ||
| ctx.opimpl_virtual_ref_finish(callee_frame); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Discarding the opimpl_virtual_ref_finish result hides an unbalanced vref stack until the loop header.
TraceCtx::opimpl_virtual_ref_finish returns false when there is no vrefbox to pop; MetaInterp::opimpl_virtual_ref_finish asserts on exactly that. Here the bool is dropped, so an enter/leave imbalance surfaces later as the virtualref_boxes must be empty assert at the loop header (or not at all if the trace aborts first), far from the offending frame. The live >= 2 guard above has the same shape.
🛡️ Proposed fix
// `jit.virtual_ref_finish(frame_vref, frame)`.
- ctx.opimpl_virtual_ref_finish(callee_frame);
+ debug_assert!(
+ ctx.opimpl_virtual_ref_finish(callee_frame),
+ "walker_ec_leave: no vrefbox for this frame — enter/leave unbalanced"
+ );Note debug_assert! would drop the call in release builds; use a let ok = ...; debug_assert!(ok, ...) binding instead.
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 1712 -
1718, Capture the boolean returned by TraceCtx::opimpl_virtual_ref_finish when
processing callee_frame and assert it succeeds without removing the call in
release builds; use a let binding followed by debug_assert!. Apply the same
result-checking pattern to the live >= 2 stop_tracking_virtualref call so
unbalanced virtual-reference operations are detected at their source.
| // The seeded callee frame's runtime object, for the `enter`/`leave` | ||
| // bracket below — the OpRef alone cannot carry it out of the seed block. | ||
| let mut ca_concrete_frame = std::ptr::null_mut::<pyre_interpreter::PyFrame>(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ca_concrete_frame duplicates the existing concrete_callee_frame.
Both are declared in the same outer scope and both are assigned concrete_frame_ptr inside the seed block (Line 2406 and Line 2423), so they are always identical. The doc comment's rationale ("the OpRef alone cannot carry it out of the seed block") is already satisfied by concrete_callee_frame. Two names for one value invites them diverging later.
♻️ Proposed cleanup
- // The seeded callee frame's runtime object, for the `enter`/`leave`
- // bracket below — the OpRef alone cannot carry it out of the seed block.
- let mut ca_concrete_frame = std::ptr::null_mut::<pyre_interpreter::PyFrame>();and drop the assignment at Line 2423, using concrete_callee_frame at the entered_ec / leave sites.
Also applies to: 2423-2423
🤖 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 `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 2249 -
2251, Remove the redundant ca_concrete_frame declaration and its seed-block
assignment, then use concrete_callee_frame at the entered_ec and leave sites.
Preserve concrete_callee_frame as the single value carrying concrete_frame_ptr
outside the seed block.
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit/src/eval.rs (1)
3858-3892: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
force_pyframe's forcing path was not migrated to the new allocator-awareforce_virtualizable_token.This function was updated to call
driver.force_virtualizable_token(token)so virtualizable materialization uses the registeredblackhole_allocatorinstead of silently falling back toNullAllocator(per the new doc comment: ajit.virtual_refframe decoded throughNullAllocatorleavesforcednull). The siblingforce_pyframe(same file, lines ~3894-3940) still callsdriver.meta_interp_mut().force_virtualizable_token(token)directly at itsforceclosure, bypassing the fix. If a frame forced via that path can also contain nested virtual refs, it is exposed to the same null-forcedbug this PR just fixed elsewhere.🐛 Proposed fix
let mut force = |ptr: *mut u8| { info.force_virtualizable_if_necessary(ptr, |token| { - driver.meta_interp_mut().force_virtualizable_token(token); + driver.force_virtualizable_token(token); }); };🤖 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 `@pyre/pyre-jit/src/eval.rs` around lines 3858 - 3892, Update the sibling force_pyframe function’s force closure to call driver.force_virtualizable_token(token) instead of driver.meta_interp_mut().force_virtualizable_token(token). Keep the token extraction and surrounding virtual-reference forcing logic unchanged so this path also uses the registered blackhole_allocator.majit/majit-metainterp/src/pyjitpl.rs (1)
2957-2982: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize concurrent
take_back_all_descrswrites.
all_descrs()now returns the same process-wideMutex<Vec<DescrRef>>, so differentMetaInterpinstances snap the same base list, append different newDescrRefs, and then race throughif all_descrs.len() >= slot.len() { *slot = all_descrs; }. The longer/equal write replaces the shared slot and can discard another in-flight compilation’s unique descriptors, breakingdescr_indexintegrity. Use the existing publish-stylePUBLISHserialization for these write-backs, or an append-only merge instead of length comparison.🤖 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 2957 - 2982, Serialize take_back_all_descrs write-backs using the existing PUBLISH mechanism so concurrent MetaInterp instances cannot replace the shared descriptor list with stale snapshots. Update take_back_all_descrs to acquire the same publish-style serialization around the length check and assignment, preserving the monotonic write-back behavior.
🤖 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.
Inline comments:
In `@majit/majit-metainterp/src/virtualref.rs`:
- Around line 158-195: Remove the Box::into_raw fallback from alloc_virtual_ref
and make a registered GC type plus successful old-generation allocation
mandatory. Propagate allocation or registration failure through the callers
instead of returning an untracked JitVirtualRef, while preserving the existing
initialization and creation write barrier for successful allocations.
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 2957-2982: Serialize take_back_all_descrs write-backs using the
existing PUBLISH mechanism so concurrent MetaInterp instances cannot replace the
shared descriptor list with stale snapshots. Update take_back_all_descrs to
acquire the same publish-style serialization around the length check and
assignment, preserving the monotonic write-back behavior.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 3858-3892: Update the sibling force_pyframe function’s force
closure to call driver.force_virtualizable_token(token) instead of
driver.meta_interp_mut().force_virtualizable_token(token). Keep the token
extraction and surrounding virtual-reference forcing logic unchanged so this
path also uses the registered blackhole_allocator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5523b2f8-6317-44db-bcf1-72ca1a58e988
📒 Files selected for processing (16)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/virtualizable.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rs
| /// | ||
| /// `lltype.malloc(self.JIT_VIRTUAL_REF)` is a GC allocation, and it has to be | ||
| /// one here too: `forced` is a traced slot (`gc_ptr_offsets = [16]`, registered | ||
| /// with [`set_vref_gc_type_id`]), so once `ExecutionContext.topframeref` holds | ||
| /// the vref instead of the frame, this object is the only edge keeping the | ||
| /// frame it wraps reachable. A host-heap allocation is invisible to the | ||
| /// collector — the root walker's `gc_current_object_address` early-out returns | ||
| /// an unowned address unchanged — which drops that edge and lets a live frame | ||
| /// be collected out from under the walk. | ||
| /// | ||
| /// Old-gen, not nursery: `virtualref_boxes`, the recorded ops' concrete stamps | ||
| /// and the interpreter's `f_backref` chain all hold this address as a raw | ||
| /// pointer that no root walker forwards, and MiniMark's old-gen is mark-sweep, | ||
| /// so the address is stable across a minor collection. The `forced` frame may | ||
| /// be young, hence the creation write barrier. | ||
| /// | ||
| /// The `Box` fallback covers the window before `set_vref_gc_type_id` has run | ||
| /// (the id still reads its unset sentinel) and an old-gen allocation failure; | ||
| /// it is leaked, and reclamation is what it gives up. | ||
| fn alloc_virtual_ref(real_object: *mut u8) -> *mut u8 { | ||
| let vref = Box::new(JitVirtualRef { | ||
| let vref = JitVirtualRef { | ||
| super_: ObjectHeader { | ||
| typeptr: JIT_VIRTUAL_REF_VTABLE, | ||
| }, | ||
| virtual_token: TOKEN_NONE, | ||
| forced: real_object, | ||
| }); | ||
| Box::into_raw(vref) as *mut u8 | ||
| }; | ||
| let type_id = vref_gc_type_id(); | ||
| if type_id != VREF_GC_TYPE_ID_UNSET { | ||
| let gcref = majit_gc::alloc_oldgen_typed(type_id, std::mem::size_of::<JitVirtualRef>()); | ||
| if gcref.0 != 0 { | ||
| unsafe { std::ptr::write(gcref.0 as *mut JitVirtualRef, vref) }; | ||
| // Creation write barrier: an old-gen vref may point at a young frame. | ||
| majit_gc::gc_write_barrier(gcref); | ||
| return gcref.0 as *mut u8; | ||
| } | ||
| } | ||
| Box::into_raw(Box::new(vref)) as *mut u8 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not silently fall back to a host-heap vref.
Box::into_raw is still used when old-generation allocation fails. That object is recognized as a JitVirtualRef, but the collector cannot trace its forced field, so the materialized frame may be collected while topframeref still holds the vref. The fallback also leaks every allocation. Make GC registration/allocation an invariant or propagate allocation failure; the ownership guard below does not restore the missing GC edge.
As per coding guidelines, the generated JIT must preserve interpreter semantics and upstream storage ownership.
🤖 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/virtualref.rs` around lines 158 - 195, Remove the
Box::into_raw fallback from alloc_virtual_ref and make a registered GC type plus
successful old-generation allocation mandatory. Propagate allocation or
registration failure through the callers instead of returning an untracked
JitVirtualRef, while preserving the existing initialization and creation write
barrier for successful allocations.
Source: Coding guidelines
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87103f0c38
ℹ️ 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".
| // unseeded (register-resident) inline has none, which is the remaining gap | ||
| // between this chain and upstream's, where `perform_call` builds a frame | ||
| // for every inlined call (`pyjitpl.py:2445-2476, 1862-1874`). | ||
| let entered_ec = callee_frame_seeded && !ca_concrete_frame.is_null() && { |
There was a problem hiding this comment.
When a strict inline is deeper than fbw_max_multiframe_depth() or fails a seeding precondition but remains inlined via the single-frame fallback, this predicate skips both ExecutionContext.enter and the matching leave. That callee therefore has no independent red frame: frame inspection observes the caller, and guard-failure resume collapses the callee's globals/locals onto the caller boundary. Build and thread a frame for every inline level, or decline the inline when that is impossible.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
| }) | ||
| .max(); | ||
| let mut max_raw_pos = max_result_pos; | ||
| if let Some(result_high_water) = max_result_pos { |
There was a problem hiding this comment.
When a trace or bridge contains only void operations but still references input or fail boxes—for example LABEL(input0); SETFIELD_GC(input0, ConstPtr); JUMP—max_result_pos is None, so this conditional skips scanning those operands and initializes next_pos to 0. Replacing the ConstPtr then emits LOAD_FROM_GC_TABLE at raw position 0, colliding with input0 in backend SSA and potentially compiling the store against the wrong value. Include arguments and fail arguments in the high-water calculation even when no result position exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
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/trace_ctx.rs (1)
2093-2102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFall back to the box's own type, not
Type::Int.When
virtualizable_slot_type(i)returnsNone(novirtualizable_info, or an index outside the layout derived from a stalevirtualizable_array_lengths), aRefslot — including the identity slot at[-1]— gets declaredInt. That declaration reachesremove_consts_and_duplicates'sdebug_assert!(tp == declared)and, on the registration side, the typed LABEL inputargs.🐛 Proposed fix
- .map(|(i, &opref)| (opref, self.virtualizable_slot_type(i).unwrap_or(Type::Int))) + .map(|(i, &opref)| { + let tp = self + .virtualizable_slot_type(i) + .or_else(|| opref.ty()) + .unwrap_or(Type::Int); + (opref, tp) + })🤖 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/trace_ctx.rs` around lines 2093 - 2102, Update collect_virtualizable_typed_boxes to use each box’s own type when virtualizable_slot_type(i) returns None, rather than defaulting to Type::Int. Preserve the existing slot type when available and ensure fallback typing covers Ref and identity slots consistently for downstream declarations.
🤖 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.
Inline comments:
In `@majit/majit-gc/src/rewrite.rs`:
- Around line 2976-2993: Make the operand/failarg reservation scan unconditional
rather than guarding it with max_result_pos. Update the logic around
reserve_later_box in the trace rewrite flow to compute max_raw_pos from every
valid non-constant operand and failarg, including Void-only traces where
max_result_pos is None, while preserving the existing high-water comparison when
result positions exist.
---
Outside diff comments:
In `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 2093-2102: Update collect_virtualizable_typed_boxes to use each
box’s own type when virtualizable_slot_type(i) returns None, rather than
defaulting to Type::Int. Preserve the existing slot type when available and
ensure fallback typing covers Ref and identity slots consistently for downstream
declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca2dc2a8-b35e-4633-b669-81212f57307c
📒 Files selected for processing (19)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-gc/src/rewrite.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/virtualizable.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rs
| if let Some(result_high_water) = max_result_pos { | ||
| let mut reserve_later_box = |pos: OpRef| { | ||
| if !pos.is_none() && !pos.is_constant() && pos.raw() > result_high_water { | ||
| max_raw_pos = | ||
| Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw()))); | ||
| } | ||
| }; | ||
| for op in &ops { | ||
| for arg in op.getarglist() { | ||
| reserve_later_box(arg.to_opref()); | ||
| } | ||
| if let Some(fail_args) = op.getfailargs() { | ||
| for arg in fail_args { | ||
| reserve_later_box(arg.to_opref()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reserve operand positions even when there are no result positions.
For a Void-only trace with a non-constant operand/failarg, max_result_pos is None, so Line 2976 skips the reservation scan and next_pos becomes 0. A generated LoadFromGcTable can then reuse an input box’s SSA position, aliasing backend values. Compute the high-water mark across valid operands/failargs unconditionally.
Proposed fix
- if let Some(result_high_water) = max_result_pos {
- let mut reserve_later_box = |pos: OpRef| {
- if !pos.is_none() && !pos.is_constant() && pos.raw() > result_high_water {
- max_raw_pos =
- Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw())));
- }
- };
- for op in &ops {
- for arg in op.getarglist() {
- reserve_later_box(arg.to_opref());
- }
- if let Some(fail_args) = op.getfailargs() {
- for arg in fail_args {
- reserve_later_box(arg.to_opref());
- }
- }
+ let mut reserve_box = |pos: OpRef| {
+ if !pos.is_none() && !pos.is_constant() {
+ max_raw_pos = Some(max_raw_pos.map_or(pos.raw(), |old| old.max(pos.raw())));
+ }
+ };
+ for op in &ops {
+ for arg in op.getarglist() {
+ reserve_box(arg.to_opref());
+ }
+ if let Some(fail_args) = op.getfailargs() {
+ for arg in fail_args {
+ reserve_box(arg.to_opref());
+ }
}
}As per coding guidelines, the generated JIT must preserve interpreter semantics.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(result_high_water) = max_result_pos { | |
| let mut reserve_later_box = |pos: OpRef| { | |
| if !pos.is_none() && !pos.is_constant() && pos.raw() > result_high_water { | |
| max_raw_pos = | |
| Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw()))); | |
| } | |
| }; | |
| for op in &ops { | |
| for arg in op.getarglist() { | |
| reserve_later_box(arg.to_opref()); | |
| } | |
| if let Some(fail_args) = op.getfailargs() { | |
| for arg in fail_args { | |
| reserve_later_box(arg.to_opref()); | |
| } | |
| } | |
| } | |
| } | |
| let mut reserve_box = |pos: OpRef| { | |
| if !pos.is_none() && !pos.is_constant() { | |
| max_raw_pos = Some(max_raw_pos.map_or(pos.raw(), |old| old.max(pos.raw()))); | |
| } | |
| }; | |
| for op in &ops { | |
| for arg in op.getarglist() { | |
| reserve_box(arg.to_opref()); | |
| } | |
| if let Some(fail_args) = op.getfailargs() { | |
| for arg in fail_args { | |
| reserve_box(arg.to_opref()); | |
| } | |
| } | |
| } |
🤖 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-gc/src/rewrite.rs` around lines 2976 - 2993, Make the
operand/failarg reservation scan unconditional rather than guarding it with
max_result_pos. Update the logic around reserve_later_box in the trace rewrite
flow to compute max_raw_pos from every valid non-constant operand and failarg,
including Void-only traces where max_result_pos is None, while preserving the
existing high-water comparison when result positions exist.
Source: Coding guidelines
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d3f4da58d
ℹ️ 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".
| is_quasi_immutable: false, | ||
| flag: ArrayFlag::Unsigned, | ||
| virtualizable: false, | ||
| index_in_parent: 0, |
There was a problem hiding this comment.
When a trace both manipulates sys_exc_value and enters or leaves a seeded inline frame, both descriptors currently use index_in_parent = 0. The optimizer uses that value as the shared StructPtrInfo field slot for parent-bound descriptors, so a value cached for sys_exc_value can be returned for topframeref (or conversely), publishing an exception object as the frame chain or corrupting the active exception state. Set the parent index from index so these two physical fields occupy separate slots.
Useful? React with 👍 / 👎.
| let restored_top = restored_virtualref_boxes | ||
| .last() | ||
| .map(|&(_, vref_ptr)| vref_ptr) | ||
| .unwrap_or(sym.concrete_vable_ptr as usize); |
There was a problem hiding this comment.
When an inline frame was forced by a residual call during recording, stop_tracking_virtualref preserves its pair but replaces the vref box and concrete pointer with NULL; a later guard therefore restores a nonempty restored_virtualref_boxes whose final vref_ptr is 0. This calculation makes restored_top zero and the following condition skips republishing anything, leaving the assembler guard's saved portal frame in EC.topframeref while the bridge resumes inside the callee, so sys._getframe() observes the portal and carrier leave closes the wrong chain. Fresh evidence beyond the prior live-vref republishing comment is the stopped-pair representation at TraceCtx::stop_tracking_virtualref; select the paired materialized virtual frame when the restored vref pointer is null.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
majit/majit-metainterp/src/virtualref.rs (1)
158-196: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUntracked
Boxfallback inalloc_virtual_refremains unresolved.This reproduces a previously-flagged major issue on this same function that has not been marked addressed: when the GC type id is unset or old-gen allocation fails, the code still falls back to a leaked, GC-invisible
Box::into_rawallocation forJitVirtualRef. The struct's own doc block acknowledges this: "TheBoxfallback covers the window beforeset_vref_gc_type_idhas run (the id still reads its unset sentinel) and an old-gen allocation failure; it is leaked, and reclamation is what it gives up." Sinceforcedis the only traced edge keeping a live frame reachable oncetopframerefholds the vref, a host-heapBoxvref can let the collector reclaim a still-referenced frame out from under the walk, and the allocation is leaked either way. As per coding guidelines, this deviation should be fixed as a translator/generation defect (mandatory GC registration + propagated allocation failure) rather than left as a silent fallback.🤖 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/virtualref.rs` around lines 158 - 196, The fallback in alloc_virtual_ref must be removed: require set_vref_gc_type_id to register a valid GC type before allocation, use alloc_oldgen_typed for every JitVirtualRef, and propagate registration or allocation failure instead of returning a leaked Box::into_raw object. Preserve the existing write barrier for successfully allocated old-generation references.Source: Coding guidelines
majit/majit-gc/src/rewrite.rs (1)
2957-3005: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOperand/failarg reservation scan is still skipped when
max_result_posisNone.This reproduces the previously-flagged, still-open major issue on this exact block: the reservation loop over
op.getarglist()/op.getfailargs()only runsif let Some(result_high_water) = max_result_pos, so a Void-only trace with a non-constant operand/failarg leavesmax_raw_posatNoneandnext_posbecomes0, entirely skipping the high-water reservation the comment describes: "rewrite.py:106-116 replaces a ConstPtr argument with a fresh LOAD_FROM_GC_TABLE box. RPython boxes retain distinct identity when their producer was optimized away, so pyre's numeric namespace must reserve argument and guard-fail positions too. Forced virtuals can carry producerless constant boxes as allocation lengths; reusing such a raw position would alias an Int box with the new Ref box in backend SSA." A freshly-generatedLoadFromGcTableop can then reuse position0(or any low position), aliasing an existing input box in backend SSA. The fix from the earlier review — computingmax_raw_posfrom operands/failargs unconditionally rather than only whenmax_result_pos.is_some()— has not been applied.🐛 Proposed fix (unconditional reservation scan)
- let mut max_raw_pos = max_result_pos; - if let Some(result_high_water) = max_result_pos { - let mut reserve_later_box = |pos: OpRef| { - if !pos.is_constant() - && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref)) - && pos.raw() > result_high_water - { - max_raw_pos = - Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw()))); - } - }; - for op in &ops { - for arg in op.getarglist() { - reserve_later_box(arg.to_opref()); - } - if let Some(fail_args) = op.getfailargs() { - for arg in fail_args { - reserve_later_box(arg.to_opref()); - } - } - } - } + let mut max_raw_pos = max_result_pos; + let mut reserve_box = |pos: OpRef| { + if !pos.is_constant() + && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref)) + { + max_raw_pos = Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw()))); + } + }; + for op in &ops { + for arg in op.getarglist() { + reserve_box(arg.to_opref()); + } + if let Some(fail_args) = op.getfailargs() { + for arg in fail_args { + reserve_box(arg.to_opref()); + } + } + }🤖 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-gc/src/rewrite.rs` around lines 2957 - 3005, Make the operand and failarg reservation scan in the next_pos initialization unconditional: do not gate the loop over op.getarglist() and op.getfailargs() on max_result_pos being Some. Initialize the high-water state from max_result_pos when present, scan all typed non-constant operands and failargs regardless, and preserve the existing exclusions for constants, untyped values, and Void sentinels before computing next_pos.Source: Coding guidelines
pyre/pyre-jit-trace/src/descr.rs (1)
2666-2697: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
index_in_parentis hardcoded to0for both fields — likely identity bug fortopframeref.The
fieldclosure takes anindex: u32parameter that's threaded intoSimpleFieldDescrSpec.index, butindex_in_parentis hardcoded to0for every call site, so bothsys_exc_value(index: 0) andtopframeref(index: 1) report the sameindex_in_parent. This was harmless when the group had only one field (where0was trivially correct); adding a second field without updatingindex_in_parentlooks like an oversight. If any consumer (heap-cache field identity, GC rewrite per-field bitstrings, etc.) usesindex_in_parentto distinguish sibling fields on the same struct, this collision could aliastopframerefreads/writes withsys_exc_value's.🐛 Proposed fix
let field = |index: u32, field_key: &str, offset: usize| SimpleFieldDescrSpec { index, field_key: field_key.to_string(), name: format!("ExecutionContext.{field_key}"), offset, field_size: std::mem::size_of::<pyre_object::PyObjectRef>(), field_type: Type::Ref, is_immutable: false, is_quasi_immutable: false, flag: ArrayFlag::Unsigned, virtualizable: false, - index_in_parent: 0, + index_in_parent: index, };🔎 Verification
#!/bin/bash # Confirm how index_in_parent is consumed for SimpleFieldDescr, and whether # other multi-field groups vary it per field. rg -n "index_in_parent" -C3 majit/majit-ir/src/descr.rs rg -n "struct SimpleFieldDescrSpec" -A20 majit/majit-ir/src/descr.rs rg -n "make_simple_descr_group\(" -A15 majit/majit-ir/src -g '!descr.rs' 2>/dev/nullAlso note: this same closure sets
field_size: std::mem::size_of::<pyre_object::PyObjectRef>()fortopframeref, which is actually*mut PyFrame. Sizes are identical today so this stays inert, but it's the same class of issue flagged in a previous review on this exact line for the (then single-field) group.🤖 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 `@pyre/pyre-jit-trace/src/descr.rs` around lines 2666 - 2697, Update the EC_DESCR_GROUP field closure so index_in_parent is derived from the field’s index rather than hardcoded to zero, ensuring sys_exc_value and topframeref have distinct sibling identities. Also set field_size from the actual field type used by each ExecutionContext field, including the *mut PyFrame type for topframeref, instead of assuming PyObjectRef for both.
🤖 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.
Inline comments:
In `@majit/majit-metainterp/src/trace_ctx.rs`:
- Around line 1099-1118: Update the minor-collection GC root walker to visit
every entry in TraceCtx::virtualref_boxes, including both the stored OpRef and
its associated raw pointer, so cached virtual-reference pointers remain valid
before consumers such as is_virtual_ref, tracing_before_residual_call, and
sanity comparison use them.
In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 2975-2979: Update build_framestack_snapshot to retain the
vable_boxes returned by list_of_boxes_virtualizable(ctx) and obtain vref_boxes
through the available vref-only helper, removing the call to
ctx.build_snapshot_vable_vref_boxes() so the vable snapshot is not rebuilt or
discarded.
---
Duplicate comments:
In `@majit/majit-gc/src/rewrite.rs`:
- Around line 2957-3005: Make the operand and failarg reservation scan in the
next_pos initialization unconditional: do not gate the loop over op.getarglist()
and op.getfailargs() on max_result_pos being Some. Initialize the high-water
state from max_result_pos when present, scan all typed non-constant operands and
failargs regardless, and preserve the existing exclusions for constants, untyped
values, and Void sentinels before computing next_pos.
In `@majit/majit-metainterp/src/virtualref.rs`:
- Around line 158-196: The fallback in alloc_virtual_ref must be removed:
require set_vref_gc_type_id to register a valid GC type before allocation, use
alloc_oldgen_typed for every JitVirtualRef, and propagate registration or
allocation failure instead of returning a leaked Box::into_raw object. Preserve
the existing write barrier for successfully allocated old-generation references.
In `@pyre/pyre-jit-trace/src/descr.rs`:
- Around line 2666-2697: Update the EC_DESCR_GROUP field closure so
index_in_parent is derived from the field’s index rather than hardcoded to zero,
ensuring sys_exc_value and topframeref have distinct sibling identities. Also
set field_size from the actual field type used by each ExecutionContext field,
including the *mut PyFrame type for topframeref, instead of assuming PyObjectRef
for both.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c3f7ae6-ebd0-4a3f-9ac1-73ef558a77b7
📒 Files selected for processing (19)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-gc/src/rewrite.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/virtualizable.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rs
| let vable_boxes = self.list_of_boxes_virtualizable(ctx); | ||
| let vref_boxes = Self::build_virtualref_boxes(self.sym(), ctx); | ||
| // `pyjitpl.py:2623` passes `self.virtualref_boxes` to the snapshot | ||
| // alongside the virtualizable boxes; the vable half here comes from | ||
| // the trait leg's own mirror, so take only the vref half. | ||
| let (_, vref_boxes) = ctx.build_snapshot_vable_vref_boxes(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect TraceCtx::build_snapshot_vable_vref_boxes to see if it duplicates
# the vable-box construction that list_of_boxes_virtualizable also performs.
rg -n "fn build_snapshot_vable_vref_boxes" -A 40 majit/majit-metainterp/src/trace_ctx.rsRepository: youknowone/pyre
Length of output: 2451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the vable-box helper definitions and call sites.
rg -n "list_of_boxes_virtualizable|build_snapshot_vable_vref_boxes|build_vable_snapshot_boxes|build_vref_snapshot_boxes" -S .
printf '\n--- trace_opcode.rs 2950-3005 ---\n'
sed -n '2950,3005p' pyre/pyre-jit-trace/src/trace_opcode.rs
printf '\n--- pyjitpl.rs relevant helpers ---\n'
rg -n "pub fn build_vable_snapshot_boxes|pub fn build_vref_snapshot_boxes|fn build_vable_snapshot_boxes|fn build_vref_snapshot_boxes" -A 80 pyre/pyre-jit-trace/src/pyjitpl.rsRepository: youknowone/pyre
Length of output: 10080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- trace_opcode.rs 3107-3124 ---'
sed -n '3107,3124p' pyre/pyre-jit-trace/src/trace_opcode.rs
printf '%s\n' '--- majit-metainterp/pyjitpl dispatch helpers ---'
sed -n '8755,8810p' majit/majit-metainterp/src/pyjitpl/dispatch.rs
printf '%s\n' '--- compare builders in trace_opcode.rs ---'
sed -n '3270,3340p' pyre/pyre-jit-trace/src/trace_opcode.rsRepository: youknowone/pyre
Length of output: 7522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- trace_opcode.rs build_framestack_snapshot call sites ---'
rg -n "build_framestack_snapshot|build_snapshot_vable_vref_boxes" pyre/pyre-jit-trace/src -S
printf '%s\n' '--- trace_opcode.rs 2920-2990 ---'
sed -n '2920,2990p' pyre/pyre-jit-trace/src/trace_opcode.rs
printf '%s\n' '--- exact list_of_boxes_virtualizable implementation ---'
sed -n '3107,3215p' pyre/pyre-jit-trace/src/trace_opcode.rs
printf '%s\n' '--- pyjitpl dispatcher full builder references ---'
sed -n '8725,8755p' majit/majit-metainterp/src/pyjitpl/dispatch.rsRepository: youknowone/pyre
Length of output: 12829
Avoid rebuilding the vable snapshot boxes in build_framestack_snapshot.
list_of_boxes_virtualizable(ctx) reads the canonically mirrored ctx.virtualizable_boxes and produces the trace-local vable snapshot list. Calling ctx.build_snapshot_vable_vref_boxes() afterwards reconstructs the same vable snapshot from ctx.virtualizable_boxes and discards it, wasting vable snapshot allocation on every guard capture. Keep using the returned vable_boxes and call a vref-only helper for vref_boxes.
🤖 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 `@pyre/pyre-jit-trace/src/trace_opcode.rs` around lines 2975 - 2979, Update
build_framestack_snapshot to retain the vable_boxes returned by
list_of_boxes_virtualizable(ctx) and obtain vref_boxes through the available
vref-only helper, removing the call to ctx.build_snapshot_vable_vref_boxes() so
the vable snapshot is not rebuilt or discarded.
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/ca4a6d35eabf590854874d79c3b7fd7d390d70f4/pyre-jit-trace/src/trace.rs#L1663
When the reconstructed callee walk returns Err, None, or a non-return outcome—or when a deeper return is followed by a middle/root reconstruction failure—this call closes the restored carrier scope before the code decides whether the bridge will be committed. The abort epilogue explicitly replays execution from the original guard, but discard_bridge_carrier_walk retains the shortened vref prefix and does not undo walker_ec_leave's concrete topframeref/escape mutations, so replay resumes inside the callee with its active frame scope already removed; frame inspection then observes the caller and the eventual leave can close the wrong chain. Defer these carrier leaves until commit, or restore the complete pre-walk vref and concrete EC state on every abort.
AGENTS.md reference: AGENTS.md:L32-L42
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 3045-3067: Refresh or root the concrete frame pointer across the
inline callee sub-walk before the leave-time use in inline_call.rs:3045-3067,
using the callee OpRef shadow or equivalent shadow-stack pinning so
walker_ec_leave receives a valid frame after residual calls or GC. Apply the
corresponding pointer-safety update at residual_call.rs:459-484 wherever
INLINE_CONCRETE_FRAME is read, preserving the existing residual-call behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 559893bd-d954-4300-bf7d-42e51bf937fe
📒 Files selected for processing (20)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-gc/src/rewrite.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/optimizeopt/heap.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rsmajit/majit-metainterp/src/virtualizable.rsmajit/majit-metainterp/src/virtualref.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/listobject.rs
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
`got_exception` was set for every callee outcome other than `SubReturn`, including a tracing decline and a loop transition. `leave`'s caller passes it only for an exception exit, so a decline permanently marked the caller escaped and forced a vref that never needed forcing. Now `SubRaise` only. Gate both vref halves of the residual-call bracket on `is_may_force`, the walker's `check_forces_virtual_or_virtualizable()`. `do_residual_call` runs the whole preparation block only for `assembler_call or effectinfo.check_forces_...` (pyjitpl.py:2007); stamping a call that cannot force left a token nothing would clear. Record the escape branch's force instead of performing it only concretely: a leaving frame that escaped emits `VIRTUAL_REF_FINISH(vrefbox, virtualbox)`, the non-null form `optimize_VIRTUAL_REF_FINISH` lowers to storing the virtual into `vref.forced` (virtualize.py:141-151), by way of `stop_tracking_virtualref`. Previously the NULL form was emitted, leaving `forced` NULL with `virtual_token` cleared, so a later read through a deeper escaped frame's `f_backref` reached `InvalidVirtualRef`. Upstream writes `forced` at runtime through `frame_vref()`'s force instead; that route needs the `jit_force_virtual` lowering, which is unwired, and both forms are already understood by the optimizer. Assisted-by: Claude
`opimpl_virtual_ref_finish`'s nesting assert was vacuous. For a non-constant `virtual_obj` it derived the compared pointer from `lastbox_ptr`, the value it then compared against, so `assert_eq!(virtual_obj_ptr, lastbox_ptr)` could never fire — disabling exactly the mismatched-nesting check `pyjitpl.py:1823` exists to make. The spelling was inherited from the `MetaInterp` copy, where the method had no callers; the inlined-call enter/leave puts it on a live path. Source the pointer from `concrete_of_opref` instead — pyre's `getref_base()` — so both sides are derived independently. An unstamped box is skipped as unknown rather than failed. Replace the inline `leave` profile-hook comment's assertion with the reason it holds: `is_being_profiled` is a portal-driver green (`interp_jit.py:68 greens = ['next_instr', 'is_being_profiled', 'pycode']`), so a trace recorded with profiling off is only entered with profiling off, and enabling profiling selects a different green key rather than reusing the trace. Assisted-by: Claude
…zers `materialize_concrete_virtual_ptr` and `materialize_virtual_object` computed the fresh object's `w_class` as `get_instantiate(descr.vtable() as *PyType)`. `VRefSizeDescr::vtable()` returns the `jit_virtual_ref_vtable` type-id constant, not a `PyType` pointer, so materializing a virtual `JitVirtualRef` on a guard failure dereferenced the constant; the offset-8 write would also have clobbered `JitVirtualRef.virtual_token`. Both sites now read `SizeDescr::w_class_obj()`, which returns `None` for a descr whose vtable word is not a `PyType` — the same accessor the dynasm, wasm and GC-rewrite allocation paths already use. Assisted-by: Claude
…cross an assembler run `ExecutionContext::gettopframe_raw` returned `self.topframeref` unforced. `executioncontext.py:68/72/446/451` all read that slot as `self.topframeref()` — with the parens — and the only unforced reads upstream are `:88` and `:96`, which move the vref along rather than dereference it. "raw" here is about skipping `gettopframe`'s virtualizable force, not the vref force; without it a `JitVirtualRef` reaches `bh_call_fn_impl`, which dereferences it at `PyFrame` field offsets. `leave` runs from a `finally` upstream, and a guard failure inside an inlined callee resumes into that callee's own `MIFrame` level, so `topframeref` is balanced however a frame is left. Pyre carries `enter`/`leave` as walker-recorded field ops rather than jitcode, so the ops after a failing guard never run. Save and restore the slot around `run_compiled_detailed_with_bridge_keyed`, the way `install_current_frame` / `CurrentFrameGuard` bracket a frame: a balanced run restores what it saved, an unbalanced exit restores the caller. `alloc_virtual_ref` now allocates through the GC, matching `lltype.malloc(self.JIT_VIRTUAL_REF)`. `forced` is a traced slot, so once `topframeref` holds the vref this object is the only edge keeping the frame reachable; a host-heap allocation is invisible to the collector. Old-gen (mark-sweep, non-moving) keeps the raw addresses in `virtualref_boxes` and the recorded ops' concrete stamps valid, and the creation write barrier covers the old-to-young `forced` edge. Assisted-by: Claude
A carrier frame's `enter` — and the `virtual_ref` scope it opened — belongs to the parent trace. `rebuild_state_after_failure` restores the still-open pairs (`pyjitpl.py:3433 self.virtualref_boxes = virtualref_boxes`), and upstream's resume continues inside that frame's `execute_frame`, so its `finally: ec.leave(...)` still runs and closes the scope. Pyre's carrier sub-walk enters the callee body directly with nothing standing in for that `finally`, so the bridge finished with `ec.topframeref` still naming the resumed frame's vref. In compiled code that vref carries a live FORCE_TOKEN and a null `forced` (`virtualize.py optimize_VIRTUAL_REF`), so a later `gettopframe` forced a vref whose scope no guard still encodes and the force yielded null. `carrier_ec_leave` closes one scope through the existing `walker_ec_leave`, sourcing the frame box from the restored pair rather than from anything the bridge built — that box is what `opimpl_virtual_ref_finish`'s identity assert compares against. The carrier drain calls it as the deepest sub-walk returns and again after each middle frame, innermost first; the `SubRaise` route passes `got_exception=true`. `ExecutionContext.topframeref` is a live heap field, so unlike the RPython locals `rebuild_state_after_failure` reconstructs, whatever the dead trace last stored there is still in it. A CALL_ASSEMBLER callee trace reaches its guard without passing through `execute_assembler`, so an abandoned callee level's vref survives into the bridge walk and its JIT frame is already gone — `cpu.force` then asserts on a null `jf_force_descr`. The resume data names what the slot should hold: the innermost still-open scope, whose vref the `continue_tracing` loop has just repaired, or the resumed frame when no scope is open. Rewrite only when the slot holds some other vref. 327 bench fixtures and `check.py` dynasm 322/322 pass; cranelift and wasm still fail on GC type ids. Assisted-by: Claude
`VREF_GC_TYPE_ID` starts at `u32::MAX`, not 0, so the `type_id != 0` guard let `alloc_virtual_ref` reach `alloc_oldgen_typed` with the unset sentinel whenever a vref was built before `set_vref_gc_type_id` ran. Zero is a legitimate id, so name the sentinel and compare against it. Assisted-by: Claude
…f/f_backref concrete stores `PyreBlackholeAllocator::bh_setfield_gc_r` and `bh_setinteriorfield_gc_r` performed the ref store without the generational barrier that `llmodel.py:723 bh_setfield_gc_r` carries through `:495 write_ref_at_mem`. The cranelift backend's implementation of the same trait methods already called it. Blackhole resume materializes virtuals into the old generation (`ResumeDataDirectReader` -> `VirtualInfo::allocate` -> `allocate_with_vtable` -> `bh_new_with_vtable`) and fills their ref fields through these setters, so a nursery value stored there did not join `old_objects_pointing_to_young` and the slot was not forwarded by the next minor collection. `VirtualRefInfo::continue_tracing` writes `vref.forced` on an old-gen vref without the barrier its `alloc_virtual_ref` counterpart already carries. `walker_ec_enter`'s concrete `f_backref` store is the recording-time shadow of a `SetfieldGc` on a `Type::Ref` field, whose emitted form is barriered. Assisted-by: Claude
…h the resume allocator `drive_bridge_carrier_walk` emitted `carrier_ec_leave` only on the compile-bound success paths, so setup failures, walk errors and abort paths left restored vref scopes open and the `f_backref` chain grew without bound. Each carrier frame now leaves immediately after its walk, with a drain epilogue, mirroring `pyframe.py execute_frame`'s `finally: ec.leave(...)`. `ResidualFrameChainGuard::enter` replaced an already-published tracing vref with the raw frame; a tracing vref carries `forced = frame` (`virtualref.py:85-92`), so the guard now declines that case. `MetaInterp::force_virtualizable_token` decoded through `NullAllocator`, leaving `forced` null after the writeback. `compile.py:966-1000 ResumeGuardForcedDescr.force_now` materializes through the same resume allocator ordinary guard failure uses; the registered blackhole allocator is now threaded through. Assisted-by: Claude
…unwind-safe topframeref - `walker_ec_leave` skips the forced `VIRTUAL_REF_FINISH` when the tracked vref box is already `ConstPtr(NULL)`, which `vrefs_after_residual_call` installs when a may-force residual exposed the frame. - The non-committal bridge carrier walk closes only the scopes it opened: the drain moved inside the walk, bounded by the entry depth and run before `cut_trace`, and `virtualref_boxes` is restored to the surviving snapshot prefix. The unbounded post-walk wrapper is gone. - `execute_assembler`'s `ec.topframeref` save/restore became a `Drop` guard so a panic unwind cannot leave a `JitVirtualRef` published in the live slot. - `continue_tracing` barriers `vref.forced` only when the collector owns the vref, matching the GC-allocated arm of `alloc_virtual_ref`. - `carrier_ec_leave` stamps the concrete value on the `execution_context` `GetfieldGcR` it records. - Guard-failure resume republishes the innermost restored vref whenever a scope is still open, not only when the published value is itself a vref. Assisted-by: Claude
…tore - The cranelift `bh_new_array` cleared nothing: the nursery is not zero-filled, so a resumed frame's `locals_cells_stack_w` came back holding the recycled bytes of the previous tenant, and the GC's per-item walk read them as children. Clear the fixed and the variable part and re-store the length, as `gct_do_malloc_varsize_clear` does. The inline allocators keep their clearing from the rewriter's ZERO_ARRAY, so the memclear stays out of the shared allocation helper. The dynasm path already allocates old-gen (zero-filled) and the wasm nursery zero-fills on reset. - `vable_write_array_item`'s `Type::Ref` arm stored without the write barrier that `llmodel.py:495-497 write_ref_at_mem` implies for every blackhole ref store. Arm whichever side the collector owns, as the sibling `VirtualizableInfo::write_array_item` already does; the barrier argument is the array block base, before the items offset. Assisted-by: Claude
Assisted-by: Claude
…vref layout target-correct
`sys._getframe(N)` inside an inlined callee resolved one level too far up,
returning the caller. Two fixtures caught it: a wrong count in
`getframe_while_escaping_read_frame_identity` and `KeyError: 'base'` (exit 1) in
`getframe_inline_subwalk_multiframe`.
- `OptHeap::writes_into_virtualizable` reported true for a SETFIELD_GC whose
target was merely read off the virtualizable frame, so `emit_lazy_setfield`
dropped `ec.topframeref = callee_vref`. The preamble lost the publish while
the peeled body kept it, which is why every other iteration was wrong.
`virtualizable.py:81-84` permits indirection only through an array field, so
the indirect branch is now restricted to SETARRAYITEM_GC.
- With the publish restored the vref is forced where it never was before, which
exposed the rest:
* `VIRTUAL_REF_FINISH` now runs before CALL_ASSEMBLER so the call stays
adjacent to GUARD_NOT_FORCED and the backends install `jf_force_descr`.
* `force_pyframe` reads `vable_token` only from the standard virtualizable
(`pyjitpl.py:3326-3334`); an inlined callee materialized through a virtual
reference is an ordinary frame whose token slot is not in its resume image.
* `JitVirtualRef` field offsets come from `offset_of!` and its identity word
is pointer-sized, per `virtualref.py:17-23`, instead of hardcoded 8/16 and
u64. The old layout was wrong on wasm32, where a pointer is four bytes.
* The GC rewriter reserves argument and guard-fail box positions, so a
producerless constant box position is no longer reused for a fresh
reference (`rewrite.py:106-116`).
pyre/check.py: dynasm 3 -> 1, cranelift 3 -> 1, wasm 18 -> 2 failures. The
remainder are the `ast_compile_roundtrip` cpython/pypy oracle mismatch and one
wasm GC panic in `inline_chain_depth_typeflip`.
Assisted-by: Claude
The argument/failarg scan added alongside the result scan admitted the `VoidOp(u32::MAX)` sentinel, which pinned `next_pos` at `u32::MAX` and overflowed the first `next_pos += 1` in `emit`. Both scans now select on `ty()` being Int/Float/Ref, and test `is_constant()` before `raw()`, which panics on the inline Const variants. Assisted-by: Claude
The struct's identity word and both reference slots are pointer-sized, and the type registration derives its size and traced offset from the struct, but the surrounding comments still described the earlier fixed 8/16-byte spelling. They also carried line numbers of files in this repo, which the comment convention excludes. Also record what the rewriter's `next_pos` sentinel guard prevents in a release build, where the overflow wraps to 0 instead of panicking. Assisted-by: Claude
The earlier narrowing to Int/Float/Ref also dropped `TempVar`, which the original scan counted. Excluding it lowers the mark past the sentinel range and lets a rewriter-introduced box take a position that reaches the backend through resume data rather than through `pos`/args/failargs. Only the `VoidOp(u32::MAX)` sentinel needs excluding, so both scans now share one predicate that does exactly that. Assisted-by: Claude
`COND_CALL_GC_WB` had three paths that emitted zero bytes and returned: - A missing write-barrier descriptor returned early on both arches. Descriptor *resolution* already falls back to the current MiniMark layout, but the emitter's own `None` arm was untouched. Now `expect`, matching the descriptor assert at opassembler.py:917-919 and x86/assembler.py:2399-2401. Every dynasm `set_gc_allocator` call site installs `MiniMarkGC`, whose `get_write_barrier_descr` returns `Some`. - A base argloc that was not `Loc::Reg` returned early on both arches. Upstream aarch64 gets a register from `ARMRegisterManager.return_constant` (aarch64/regalloc.py:70); the shared `RegisterManager::return_constant` follows the llsupport spelling (llsupport/regalloc.py:625) and can return a bare `Loc::Immed`. Now panics, like the paired lowered `GcStore`. - aarch64 card marking ran the whole sequence under `if let Some(loc_index)` where `loc_index` was narrowed to `Loc::Reg`, so an immediate index produced an empty `card_mark` body. `nbody` emits 46 `COND_CALL_GC_WB_ARRAY` ops and every one carries an `Immed` index, so that body was always empty on aarch64 while x86 emitted the card bits. Ports the immediate arm from x86/assembler.py:2382-2386; A64 has no or-to-memory form, so the `OR8` becomes ldrb/orr/strb. Both arches now end the index match with x86/assembler.py:2387-2388's `AssertionError` instead of falling through. `pyre/check.py --backend dynasm`: 333 passed. `nested_loop` (2.1x, a bench that sits on the 2x gate; it emits no write barriers at all) and `synth/ast_compile_roundtrip` (baseline cpython/pypy output mismatch, pyre not run) are unrelated. Assisted-by: Claude
…arrier decline Neither path is reachable from `pyre/check.py`, so both get a unit test. `cond_call_gc_wb_array_immed_index_marks_same_card_as_reg_index` allocates three old-gen card arrays, sets CARDS_SET by hand so the emitted `b.ne =>card_mark` is taken on the first check and the array helper is never entered, then drives each index through the compiled immediate arm, the compiled register arm, and `do_write_barrier_card`, comparing `dirty_cards` after every index. A `ConstInt` index reaches the emitter as `Loc::Immed` because both `CondCallGcWb*` prepare sites call `make_sure_var_in_reg` with no selected register (llsupport/regalloc.py:625). Negative control: with the immediate arm disabled the compiled cards read `[]` against `[0]`. `write_barrier_ignores_unmanaged_jitframe_with_flag_byte_set` pins the `is_managed_heap_object` guard in `do_write_barrier`. A jitframe from the `libc::calloc` fallback has no GcHeader, so the inline flag test reads `jit_wb_if_flag_byteofs` out of bytes that are not a header; the test sets that bit explicitly rather than depending on the host allocator, and asserts the block neither enters `remembered_set` nor has the byte cleared. Negative control: removing the guard fails the `remembered_set` assertion. Also extends `do_write_barrier`'s doc comment to name the jitframe case — it previously justified the guard only by interpreter `Box::into_raw` call sites, which made the JIT path look like dead weight. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/f06af59666ec46d2d4c90cb0af6efcf8b512b966/pyre-jit-trace/src/trace.rs#L1660
When drive_bridge_carrier_subwalk returns None or Err because bridge tracing cannot continue, the resumed callee has not returned or raised—it will continue through the bridge-abort recovery path. This unconditional leave nevertheless pops its restored vref scope and concretely rewrites EC.topframeref to the caller; the later discard restores only virtualref_boxes, not that concrete frame chain. Consequently, resumed interpreter/blackhole code inside the callee can expose the caller through sys._getframe() and eventually leave the wrong frame. Call carrier_ec_leave only for terminal SubReturn/SubRaise outcomes, while preserving the active callee on tracing aborts.
AGENTS.md reference: AGENTS.md:L24-L30
ℹ️ 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".
1 participant
Wires the walker's multi-frame inline push to the
jit.virtual_refmachinery. That machinery was already ported in full and had zero production callers —opimpl_virtual_ref/opimpl_virtual_ref_finishon both legs,bhimpl_virtual_ref{,_finish}(wired), the optimizer'sVIRTUAL_REF→ virtual-struct transform (optimizeopt/virtualize.rs:2029), the snapshot encode, and the resume-sideconsume_virtualref_boxes+continue_tracingall existed withvirtualref_boxespermanently empty. The missing piece was the producer.What this does
walker_ec_enter/walker_ec_leaveportexecutioncontext.py:85-107at the seeded inline level:Both the recorded ops and the concrete stores run. The walk is also the interpreter executing this iteration, so a residual
sys._getframe()inside the callee body reads the liveec; recording the ops alone would leave it seeing the CALLER. The concrete slot holds theJitVirtualRef, whichforce_vref(#785) resolves — a vref fromvirtual_ref_during_tracingcarriesforced = framewithvirtual_token = TOKEN_NONE(virtualref.py:85-92), so that read is exact and cannot fail.leavesits in the original'sfinallyposition. The sub-walk block is an expression that always completes, so a normal return, a raised exception, and a declined callee all reach it; everyreturn Errdecline gate sits before theenter.Single
virtualref_boxesopimpl_virtual_ref{,_finish}move fromMetaInterpontoTraceCtx, and the duplicatePyreSym.virtualref_boxesis retired — upstream has oneMetaInterp.virtualref_boxes.This was not cosmetic. Once a producer exists, the split is a correctness bug:
state.rs's bridge resume restored pairs into thePyreSymcopy, while every full-body-walker snapshot reads theTraceCtxone (build_snapshot_vable_vref_boxes). A parent's still-open virtual_ref scope was therefore dropped at bridge entry.Residual-call bracket
The walker's residual calls now carry the vref halves (
pyjitpl.py:2017/:2049) next to the virtualizable halves already there.vrefs_before_residual_callstampsTOKEN_TRACING_RESCALL;vrefs_after_residual_callturns a vref the callee forced intoVIRTUAL_REF_FINISH+ ConstPtr(NULL) before the CALL op is recorded. This is the safety half and cannot be deferred past the producer — an unbracketed residual that hands the frame to Python would otherwise go undetected.Known gaps, stated rather than hidden
leave's escape branch is concrete-only.if frame.escaped or got_exception: f_back.mark_as_escaped(); frame_vref()runs at recording time but is not emitted into the IR.optimize_VIRTUAL_REF_FINISHleavesforcedNULL on the normal path, so with two nested seeded inline levels A→B where B's frame escapes, a later compiled-code read ofB.f_backrefreaches a finished vref andforce_pyframe_vref'sexpectpanics. imp, virtualref: ImportRLock port; frame-chain vref reads through the vref #785's comment named this as the condition theVIRTUAL_REFemit owns re-checking; this is that re-check, and the answer is that the state stops being unreachable.escapedis a bit inPyFrame.flags, so the emit costs a getfield + guard per inline leave — the cost upstream pays for the same code.alloc_virtual_refis a leakedBox, not a GC object. Ownership genuinely belongs to the collector (trace, resume data and the frame chain all hold the address), so freeing it is not correct. It costs 24 bytes per inlined call per trace recording — bounded by compilation events, not iterations — and converges with the sibling_dummy/JITFRAME divergence already documented atset_vref_gc_type_id.Unseeded (register-resident) inline levels still take no vref: they have no frame object to take one of, where upstream's
perform_callbuilds a frame for every inlined call. That is the next slice, and it is whattrace.rs:1694's decline-to-legacy-replay is waiting on.Verification
cargo check -p majit-metainterp -p pyre-interpreter -p pyre-jit-trace -p pyre-jit --features dynasmis clean (the one warning is pre-existing, in an untouched file).check.pyhas not been run locally on this commit — the machine has been at load 26-59 all session from sibling worktrees, where the perf gates are meaningless and builds get SIGTERM'd. CI's quiet runners are the judge here, and the two gaps above are exactly what I want the parity review to weigh in on.Summary by CodeRabbit
Bug Fixes
ExecutionContextenter/leave bracketing (including exception/escape paths) and added safety to prevent staletopframeref.New Features
standard_virtualizable_heap_ptr.ExecutionContexttopframeref field offset/descriptor support for JIT lowering.Documentation