From c38a39f9df6ffc5635f3f929d3d2991e573b2eb1 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 02:32:02 +0900 Subject: [PATCH 01/18] jit(fbw): emit ExecutionContext.enter/leave at the inlined-call push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire the walker's multi-frame inline push to the ported `jit.virtual_ref` machinery, which had no production caller: `opimpl_virtual_ref` / `opimpl_virtual_ref_finish`, `bhimpl_virtual_ref{,_finish}`, the optimizer's `VIRTUAL_REF` transform, the snapshot encode and the resume-side `continue_tracing` were all in place with `virtualref_boxes` permanently empty. `walker_ec_enter` / `walker_ec_leave` port `executioncontext.py:85-107` at the seeded inline level: read `ec.topframeref` into the callee's `f_backref`, record `VIRTUAL_REF` on the virtual callee frame, store the vref back into `ec.topframeref`, and unwind the same three at the return. Both the recorded ops and the concrete stores run, because the walk is also the interpreter executing the iteration — a residual `sys._getframe()` in the callee body reads the live `ec` and would otherwise see the caller. `leave` runs in the original's `finally` position: the sub-walk block is an expression that always completes, so a normal return, a raised exception and a declined callee all reach it. Move `opimpl_virtual_ref{,_finish}` from `MetaInterp` onto `TraceCtx` and retire the duplicate `PyreSym.virtualref_boxes`, leaving one store as upstream has one `MetaInterp.virtualref_boxes`. The duplicate was load-bearing once a producer exists: the bridge resume decode restored pairs into the `PyreSym` copy while every full-body-walker snapshot reads the `TraceCtx` one, so a parent's open virtual_ref scope was dropped at bridge entry. Bracket the walker's residual calls with the vref halves (`pyjitpl.py:2017` / `:2049`), alongside the virtualizable halves already there: `vrefs_before_residual_call` stamps TOKEN_TRACING_RESCALL, `vrefs_after_residual_call` turns a vref the callee forced into a `VIRTUAL_REF_FINISH` plus ConstPtr(NULL) before the CALL is recorded. Add `EC_TOPFRAMEREF_OFFSET` and `ec_topframeref_descr`, hoisting the ExecutionContext field group so `sys_exc_value` and `topframeref` share one struct identity. `leave`'s `if frame.escaped or got_exception` branch is emitted only concretely, not into the IR; `alloc_virtual_ref` remains a leaked Box rather than a GC object. Both are documented at their sites. Assisted-by: Claude --- majit/majit-metainterp/src/pyjitpl.rs | 72 +------ majit/majit-metainterp/src/trace_ctx.rs | 107 +++++++++++ majit/majit-metainterp/src/virtualref.rs | 17 ++ pyre/pyre-interpreter/src/executioncontext.rs | 9 +- pyre/pyre-jit-trace/src/descr.rs | 75 +++++--- .../src/jitcode_dispatch/inline_call.rs | 179 ++++++++++++++++++ .../src/jitcode_dispatch/mod.rs | 20 +- .../src/jitcode_dispatch/residual_call.rs | 32 +++- pyre/pyre-jit-trace/src/state.rs | 83 ++------ pyre/pyre-jit-trace/src/trace_opcode.rs | 35 ++-- 10 files changed, 419 insertions(+), 210 deletions(-) diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 89ca338eee5..411fe219ab8 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -4923,25 +4923,9 @@ impl MetaInterp { let Some(ctx) = self.tracing.as_mut() else { return OpRef::NONE; }; - // pyjitpl.py:1804: virtual_ref_during_tracing(virtual_obj) - // `vrefinfo = self.staticdata.virtualref_info` (pyjitpl.py:1314). - let vref_ptr = self - .staticdata - .virtualref_info - .virtual_ref_during_tracing(virtual_obj_ptr as *mut u8); - // pyjitpl.py:1805: cindex = ConstInt(len(virtualref_boxes) // 2) - let cindex = ctx.const_int((ctx.virtualref_boxes.len() / 2) as i64); - // pyjitpl.py:1806-1807: - // resbox = metainterp.history.record2(rop.VIRTUAL_REF, box, cindex, vref) - // self.metainterp.heapcache.new(resbox) - // `TraceCtx::virtual_ref` bundles both so the heapcache `new` - // is not skipped (the inline `ctx.record_op(VirtualRefR, ...)` - // form bypassed `heap_cache.new_object` — pyjitpl.py:1807 parity). - let vref = ctx.virtual_ref(virtual_obj, cindex); - // pyjitpl.py:1814: virtualref_boxes += [virtualbox, vrefbox] - ctx.virtualref_boxes.push((virtual_obj, virtual_obj_ptr)); - ctx.virtualref_boxes.push((vref, vref_ptr as usize)); - vref + // `@arguments("box", returns="box")` — the jitcode opimpl yields only + // the box; the concrete vref has no register to land in. + ctx.opimpl_virtual_ref(virtual_obj, virtual_obj_ptr).0 } /// pyjitpl.py:1819-1832 `opimpl_virtual_ref_finish(box)` parity — @@ -4952,54 +4936,10 @@ impl MetaInterp { let Some(ctx) = self.tracing.as_mut() else { return; }; - // `pyjitpl.py:1820-1822`: - // vrefbox = metainterp.virtualref_boxes.pop() - // lastbox = metainterp.virtualref_boxes.pop() - let (vrefbox, vref_ptr) = ctx - .virtualref_boxes - .pop() - .expect("opimpl_virtual_ref_finish: missing vrefbox"); - let (lastbox, lastbox_ptr) = ctx - .virtualref_boxes - .pop() - .expect("opimpl_virtual_ref_finish: missing virtualbox"); - // `pyjitpl.py:1823 assert box.getref_base() == lastbox.getref_base()` - // — compare the concrete ref base, not the SSA OpRef. PyPy permits - // alias boxes that share `getref_base()` but differ in box identity; - // an `OpRef`-identity assert would reject those. Read - // `virtual_obj`'s ref value off its variant tag when it is a - // ConstPtr, falling back to the pre-pop side-table pointer that the - // matching `opimpl_virtual_ref(virtual_obj, virtual_obj_ptr)` - // recorded as `lastbox_ptr`. - let virtual_obj_ptr = match virtual_obj.inline_const_to_value() { - Some(Value::Ref(r)) => r.as_usize(), - _ => lastbox_ptr, - }; - // pyjitpl.py:1825 `assert box.getref_base() == lastbox.getref_base()` - // — RPython's plain `assert` fires in both untranslated and - // translated builds (the latter via the same fail-fast on - // invariant break); the Rust port mirrors that with `assert_eq!` - // so release builds also fail at the divergence point rather - // than silently corrupting the vref stack. - assert_eq!( - virtual_obj_ptr, lastbox_ptr, - "opimpl_virtual_ref_finish: leaving frame ref != top virtualref ref \ - (virtual_obj={:?}, lastbox={:?})", - virtual_obj, lastbox + assert!( + ctx.opimpl_virtual_ref_finish(virtual_obj), + "opimpl_virtual_ref_finish: missing vrefbox" ); - // pyjitpl.py:1826-1832 `vrefinfo = ...; vref = vrefbox.getref_base(); - // if vrefinfo.is_virtual_ref(vref): record VIRTUAL_REF_FINISH`. - let is_vref = vref_ptr != 0 - && unsafe { - self.staticdata - .virtualref_info - .is_virtual_ref(vref_ptr as *const u8) - }; - if is_vref { - // pyjitpl.py:1831-1832 `VIRTUAL_REF_FINISH(vrefbox, nullbox)`. - let null = ctx.const_ref(0); - let _ = ctx.record_op(OpCode::VirtualRefFinish, &[vrefbox, null]); - } } /// Whether the engine is currently tracing. diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index 21eadd7d192..ac34d7c0aa3 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -1045,6 +1045,113 @@ impl TraceCtx { result } + /// Live `[virtualbox, vrefbox]` entry count — twice the number of open + /// `virtual_ref` scopes. `pyjitpl.py:2995` asserts this is zero when a + /// loop header is reached ("missing virtual_ref_finish()?"). + pub fn virtualref_boxes_len(&self) -> usize { + self.virtualref_boxes.len() + } + + /// `pyjitpl.py:3433 rebuild_state_after_failure`'s + /// `self.virtualref_boxes = virtualref_boxes`. A bridge resumes into its + /// parent's still-open `virtual_ref` scopes, so the pairs the parent guard + /// encoded are re-tracked before the bridge trace records anything — + /// otherwise its own `virtual_ref_finish` would pop an empty stack. + pub fn restore_virtualref_boxes(&mut self, boxes: Vec<(OpRef, usize)>) { + self.virtualref_boxes = boxes; + } + + /// `pyjitpl.py:1789-1814 opimpl_virtual_ref` — `ExecutionContext.enter`'s + /// `jit.virtual_ref(frame)` (`executioncontext.py:89`) as the tracer sees + /// it. Creates the concrete vref, records `VIRTUAL_REF(box, cindex)`, and + /// pushes the `[virtualbox, vrefbox]` pair. + /// + /// Lives here rather than on `MetaInterp` because `virtualref_boxes` does: + /// upstream has exactly one `MetaInterp.virtualref_boxes`, and both the + /// MIFrame leg and the full-body walker record into this one trace. + /// + /// Returns the recorded box and the concrete vref. Upstream returns only + /// the box because the `jit.virtual_ref` call it lowers hands its runtime + /// result straight back to the interpreter's `enter`; pyre's walker has to + /// perform that store itself, so it needs both. + pub fn opimpl_virtual_ref( + &mut self, + virtual_obj: OpRef, + virtual_obj_ptr: usize, + ) -> (OpRef, *mut u8) { + // pyjitpl.py:1804 `vref = vrefinfo.virtual_ref_during_tracing(box)`. + let vref_ptr = self + .metainterp_sd + .virtualref_info + .virtual_ref_during_tracing(virtual_obj_ptr as *mut u8); + // pyjitpl.py:1805 `cindex = ConstInt(len(virtualref_boxes) // 2)`. + let cindex = self.const_int((self.virtualref_boxes.len() / 2) as i64); + // pyjitpl.py:1806-1807 `record2(VIRTUAL_REF, box, cindex)` + + // `heapcache.new(resbox)`, bundled by `virtual_ref`. + let vref = self.virtual_ref(virtual_obj, cindex); + // pyjitpl.py:1814 `virtualref_boxes += [virtualbox, vrefbox]`. + self.virtualref_boxes.push((virtual_obj, virtual_obj_ptr)); + self.virtualref_boxes.push((vref, vref_ptr as usize)); + (vref, vref_ptr) + } + + /// `pyjitpl.py:1819-1832 opimpl_virtual_ref_finish(box)` — + /// `ExecutionContext.leave`'s `jit.virtual_ref_finish` + /// (`executioncontext.py:107`). The vrefbox is reconstituted by popping, + /// not passed in, so the stack discipline is checked rather than assumed. + /// + /// Returns whether a pair was popped: the walker brackets a callee level + /// whose `enter` may have been skipped, and an unbalanced pop would eat + /// an enclosing level's pair. + pub fn opimpl_virtual_ref_finish(&mut self, virtual_obj: OpRef) -> bool { + // pyjitpl.py:1820-1822 `vrefbox = pop(); lastbox = pop()`. + let Some((vrefbox, vref_ptr)) = self.virtualref_boxes.pop() else { + return false; + }; + let (lastbox, lastbox_ptr) = self + .virtualref_boxes + .pop() + .expect("opimpl_virtual_ref_finish: vrefbox without its virtualbox"); + // pyjitpl.py:1823 `assert box.getref_base() == lastbox.getref_base()` + // — compare the concrete ref base, not the SSA OpRef. PyPy permits + // alias boxes that share `getref_base()` but differ in box identity; + // an `OpRef`-identity assert would reject those. Read `virtual_obj`'s + // ref value off its variant tag when it is a ConstPtr, falling back to + // the pre-pop side-table pointer the matching `opimpl_virtual_ref` + // recorded as `lastbox_ptr`. + let virtual_obj_ptr = match virtual_obj.inline_const_to_value() { + Some(Value::Ref(r)) => r.as_usize(), + _ => lastbox_ptr, + }; + // RPython's plain `assert` fires in both untranslated and translated + // builds, so this is an `assert_eq!`: a release build must fail at the + // divergence rather than silently corrupt the vref stack. + assert_eq!( + virtual_obj_ptr, lastbox_ptr, + "opimpl_virtual_ref_finish: leaving frame ref != top virtualref ref \ + (virtual_obj={virtual_obj:?}, lastbox={lastbox:?})" + ); + // pyjitpl.py:1826-1832 `if vrefinfo.is_virtual_ref(vref): record + // VIRTUAL_REF_FINISH`. False once `stop_tracking_virtualref` has + // replaced the box with ConstPtr(NULL) — the finish already ran. + let is_vref = vref_ptr != 0 + && unsafe { + self.metainterp_sd + .virtualref_info + .is_virtual_ref(vref_ptr as *const u8) + }; + if is_vref { + // pyjitpl.py:1831-1832 `VIRTUAL_REF_FINISH(vrefbox, nullbox)`. + let null = self.const_ref(0); + let _ = Self::do_record_op( + &mut self.recorder, + OpCode::VirtualRefFinish, + &[vrefbox, null], + ); + } + true + } + /// `pyjitpl.py:3317-3324 MetaInterp.vable_and_vrefs_before_residual_call` /// — the vrefs half (the virtualizable-info half lives on /// `JitCodeMachine::prepare_standard_virtualizable_before_residual_call`). diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index 180a9a4ac55..bd10cbfef4d 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -150,6 +150,23 @@ pub use crate::jit::InvalidVirtualRef; /// `virtualref.py:85-91 virtual_ref_during_tracing(real_object)`. /// Initializes virtual_token = TOKEN_NONE, forced = real_object. /// Returns raw pointer; caller owns the allocation. +/// +/// The allocation is a leaked `Box`, not a GC object as +/// `lltype.malloc(self.JIT_VIRTUAL_REF)` is: the vref outlives every scope +/// that could free it — the trace stores its address in `virtualref_boxes` +/// and in resume data, the interpreter stores it in `ec.topframeref` / +/// `f_backref`, and after `virtual_ref_finish` a forced frame may still be +/// reached through it — so ownership genuinely belongs to the collector. Two +/// consequences follow, and both are why this is safe rather than merely +/// tolerated: the address never moves, which is what lets `virtualref_boxes` +/// hold it as a raw pointer across a collection, and the GC skips it as +/// unowned when it visits the root slot it sits in +/// (`majit-gc gc_current_object_address` early-out). What is lost is +/// reclamation: one 24-byte vref per inlined call, per trace recording, is +/// never freed. That is bounded by compilation events rather than by +/// iterations, and converges the same way the sibling divergence noted on +/// `set_vref_gc_type_id`'s registration does — by moving this allocation, the +/// `_dummy`, and the JITFRAME under the GC together. fn alloc_virtual_ref(real_object: *mut u8) -> *mut u8 { let vref = Box::new(JitVirtualRef { super_: ObjectHeader { diff --git a/pyre/pyre-interpreter/src/executioncontext.rs b/pyre/pyre-interpreter/src/executioncontext.rs index e94a6c54806..481ed175137 100644 --- a/pyre/pyre-interpreter/src/executioncontext.rs +++ b/pyre/pyre-interpreter/src/executioncontext.rs @@ -78,7 +78,7 @@ pub fn vref_referent(ptr: *mut PyFrame) -> *mut PyFrame { /// jit_virtual_ref_vtable: return inst` (the pointer already *is* the frame) /// else materialize via `force_virtual`. #[inline] -pub(crate) fn force_vref(ptr: *mut PyFrame) -> *mut PyFrame { +pub fn force_vref(ptr: *mut PyFrame) -> *mut PyFrame { if unsafe { majit_metainterp::virtualref::ptr_is_virtual_ref(ptr as *const u8) } { // Only the tracer stores a `JitVirtualRef` here, and it registers the // hook when the driver comes up, so an unset hook means the slot was @@ -306,6 +306,13 @@ pub fn execution_context_builtin_cache_get(ec: &ExecutionContext) -> PyObjectRef /// GETFIELD_GC/SETFIELD_GC lowering of PUSH_EXC_INFO / POP_EXCEPT. pub const EC_SYS_EXC_VALUE_OFFSET: usize = std::mem::offset_of!(ExecutionContext, sys_exc_value); +/// Byte offset of `topframeref` within `ExecutionContext`, for the JIT's +/// GETFIELD_GC/SETFIELD_GC lowering of [`ExecutionContext::enter`] / +/// [`ExecutionContext::leave`] at an inlined call. The traced sequence is +/// `executioncontext.py:88-89` — read the slot into the callee's `f_backref`, +/// then store the callee's `jit.virtual_ref` back into it. +pub const EC_TOPFRAMEREF_OFFSET: usize = std::mem::offset_of!(ExecutionContext, topframeref); + /// Size of `ExecutionContext`, for the JIT's StructPtrInfo SizeDescr /// describing the (non-GC) EC struct. The EC is never JIT-allocated; /// this size only backs the field-tracking SizeDescr. diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index d02334dcde5..5e4601af8e6 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -2641,36 +2641,57 @@ pub fn w_exception_slot_descr( /// dead-store-eliminates a balanced save/restore (and the stored /// exception, if it never otherwise escapes, stays virtual and DCEs). pub fn ec_sys_exc_value_descr() -> DescrRef { - static EC_DESCR_GROUP: LazyLock = LazyLock::new(|| { - use majit_ir::descr::{ArrayFlag, SimpleFieldDescrSpec}; - // type_id 0 + vtable 0 → SimpleSizeDescr::is_object() == false, so - // the optimizer builds a StructPtrInfo for the (non-GC) EC pointer. - // The single field is Ref-typed (ref value tracking) but - // Unsigned-flagged (is_pointer_field() == false → no write barrier; - // the slot is a forwarded GC root, see eval::walk_pyframe_roots). - majit_ir::descr::make_simple_descr_group( - u32::MAX, - pyre_interpreter::EC_SIZE, - 0, - 0, - &[SimpleFieldDescrSpec { - index: 0, - field_key: "sys_exc_value".to_string(), - name: "ExecutionContext.sys_exc_value".to_string(), - offset: pyre_interpreter::EC_SYS_EXC_VALUE_OFFSET, - field_size: std::mem::size_of::(), - field_type: Type::Ref, - is_immutable: false, - is_quasi_immutable: false, - flag: ArrayFlag::Unsigned, - virtualizable: false, - index_in_parent: 0, - }], - ) - }); EC_DESCR_GROUP.field_descrs[0].clone() as DescrRef } +/// Field descr for `ExecutionContext::topframeref`, used by the JIT lowering +/// of `executioncontext.py:88-89 enter` / `:96-97 leave` at an inlined call: +/// `frame.f_backref = self.topframeref` reads it and +/// `self.topframeref = jit.virtual_ref(frame)` writes it back. +/// +/// Same group as [`ec_sys_exc_value_descr`] — one struct, one identity, so the +/// heap optimizer can forward an `enter` store to the matching `leave` read +/// and dead-store-eliminate a balanced pair whose frame never escaped. +pub fn ec_topframeref_descr() -> DescrRef { + EC_DESCR_GROUP.field_descrs[1].clone() as DescrRef +} + +/// The `ExecutionContext` field group. `type_id 0 + vtable 0` → +/// `SimpleSizeDescr::is_object() == false`, so the optimizer builds a +/// StructPtrInfo for the (non-GC) EC pointer. Both fields are Ref-typed +/// (ref value tracking) but Unsigned-flagged (`is_pointer_field()` is false → +/// no write barrier), which is correct because each slot is forwarded +/// directly as a GC root every collection (`eval::walk_pyframe_roots`), so the +/// generational remembered-set barrier is unnecessary. +static EC_DESCR_GROUP: LazyLock = LazyLock::new(|| { + use majit_ir::descr::{ArrayFlag, SimpleFieldDescrSpec}; + let field = |index: u32, field_key: &str, offset: usize| SimpleFieldDescrSpec { + index, + // descr.py:220-233 cache key is the bare fieldname; `name` is the + // qualified display form. + field_key: field_key.to_string(), + name: format!("ExecutionContext.{field_key}"), + offset, + field_size: std::mem::size_of::(), + field_type: Type::Ref, + is_immutable: false, + is_quasi_immutable: false, + flag: ArrayFlag::Unsigned, + virtualizable: false, + index_in_parent: 0, + }; + majit_ir::descr::make_simple_descr_group( + u32::MAX, + pyre_interpreter::EC_SIZE, + 0, + 0, + &[ + field(0, "sys_exc_value", pyre_interpreter::EC_SYS_EXC_VALUE_OFFSET), + field(1, "topframeref", pyre_interpreter::EC_TOPFRAMEREF_OFFSET), + ], + ) +}); + /// Size descriptor for W_SliceObject allocation via NewWithVtable. /// vtable = &SLICE_TYPE (ob_type for virtual materialization). /// Mirrors `pypy/objspace/std/objspace.py:385` `space.newslice` → diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index bee922fdb55..01bb7899bc0 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1607,6 +1607,142 @@ pub(crate) fn try_walker_inline_user_call( ) } +/// `executioncontext.py:85-89 ExecutionContext.enter`, at an inlined call. +/// +/// ```python +/// def enter(self, frame): +/// frame.f_backref = self.topframeref +/// self.topframeref = jit.virtual_ref(frame) +/// ``` +/// +/// Both halves run. The recorded ops are what a guard resumes into; the +/// concrete stores are what the callee body observes while this walk records +/// it, because the walk IS the interpreter running — a `sys._getframe()` in +/// the body executes as a residual against the live `ec`, and without the +/// concrete store it would read the CALLER and commit the wrong frame. +/// +/// The concrete slot holds the `JitVirtualRef`, not the frame: +/// `executioncontext::force_vref` resolves it for every reader, and a vref +/// built by `virtual_ref_during_tracing` already carries `forced = frame` with +/// `virtual_token = TOKEN_NONE` (`virtualref.py:85-92`), so the resolution is +/// exact and cannot fail. Storing the vref rather than the frame is what lets +/// the optimizer keep the frame virtual: nothing reads the frame itself unless +/// something forces it. +/// +/// Returns the vref's OpRef for the matching [`walker_ec_leave`]. +fn walker_ec_enter( + ctx: &mut TraceCtx, + callee_frame: OpRef, + callee_ec: OpRef, + concrete_frame: *mut pyre_interpreter::PyFrame, + concrete_ec: *mut pyre_interpreter::PyExecutionContext, +) -> OpRef { + // `frame.f_backref = self.topframeref` — the caller's vref moves into the + // callee, unforced. `emit_new_pyframe_inline_with_params` leaves the slot + // at its constructor default, so this is the store that links the chain. + let concrete_caller_topframeref = unsafe { (*concrete_ec).topframeref }; + let caller_topframeref = ctx.record_op_with_descr( + OpCode::GetfieldGcR, + &[callee_ec], + crate::descr::ec_topframeref_descr(), + ); + ctx.set_opref_concrete( + caller_topframeref, + majit_ir::Value::Ref(majit_ir::GcRef(concrete_caller_topframeref as usize)), + ); + ctx.record_op_with_descr( + OpCode::SetfieldGc, + &[callee_frame, caller_topframeref], + crate::descr::pyframe_f_backref_descr(), + ); + // `self.topframeref = jit.virtual_ref(frame)`. + let (vref, concrete_vref) = ctx.opimpl_virtual_ref(callee_frame, concrete_frame as usize); + ctx.set_opref_concrete( + vref, + majit_ir::Value::Ref(majit_ir::GcRef(concrete_vref as usize)), + ); + ctx.record_op_with_descr( + OpCode::SetfieldGc, + &[callee_ec, vref], + crate::descr::ec_topframeref_descr(), + ); + unsafe { + (*concrete_frame).f_backref = concrete_caller_topframeref; + (*concrete_ec).topframeref = concrete_vref as *mut pyre_interpreter::PyFrame; + } + vref +} + +/// `executioncontext.py:91-107 ExecutionContext.leave`'s frame-chain half, at +/// the return from an inlined call. +/// +/// ```python +/// frame_vref = self.topframeref +/// self.topframeref = frame.f_backref +/// if frame.escaped or got_exception: +/// f_back = frame.f_backref() +/// if f_back: +/// f_back.mark_as_escaped() +/// frame_vref() +/// jit.virtual_ref_finish(frame_vref, frame) +/// ``` +/// +/// The profile-hook half stays with the interpreter's own +/// [`pyre_interpreter::PyExecutionContext::leave`]; a traced call never runs a +/// profile hook, which is why upstream can inline this frame at all. +/// +/// Upstream runs this from a `finally`, so the caller must too: every path out +/// of the callee level — normal return, raised exception, or a declined +/// sub-walk — has to reach it, or `virtualref_boxes` is left unbalanced and the +/// loop header trips `assert len(self.virtualref_boxes) == 0`. +fn walker_ec_leave( + ctx: &mut TraceCtx, + callee_frame: OpRef, + callee_ec: OpRef, + concrete_frame: *mut pyre_interpreter::PyFrame, + concrete_ec: *mut pyre_interpreter::PyExecutionContext, + got_exception: bool, +) { + // `self.topframeref = frame.f_backref` — no parens: the caller's vref + // moves back unforced, so a caller frame that stayed virtual stays virtual. + let concrete_f_backref = unsafe { (*concrete_frame).f_backref }; + let f_backref = ctx.record_op_with_descr( + OpCode::GetfieldGcR, + &[callee_frame], + crate::descr::pyframe_f_backref_descr(), + ); + ctx.set_opref_concrete( + f_backref, + majit_ir::Value::Ref(majit_ir::GcRef(concrete_f_backref as usize)), + ); + ctx.record_op_with_descr( + OpCode::SetfieldGc, + &[callee_ec, f_backref], + crate::descr::ec_topframeref_descr(), + ); + unsafe { + let frame_vref = (*concrete_ec).topframeref; + (*concrete_ec).topframeref = concrete_f_backref; + if (*concrete_frame).escaped() || got_exception { + // A frame that reached app level must keep its caller reachable + // too, or the next `_getframe().f_back` walks into a frame the JIT + // was still free to keep virtual. `get_f_back` forces, which is + // `f_back = frame.f_backref()` with the parens. + let f_back = (*concrete_frame).get_f_back(); + if !f_back.is_null() { + (*f_back).mark_as_escaped(); + } + // `frame_vref()` — force the leaving frame's own vref so it + // outlives the JIT frame. A no-op at recording time (the vref + // already carries `forced`), kept because it is the operation + // upstream performs and the optimizer reads the trace, not this. + let _ = pyre_interpreter::executioncontext::force_vref(frame_vref); + } + } + // `jit.virtual_ref_finish(frame_vref, frame)`. + ctx.opimpl_virtual_ref_finish(callee_frame); +} + /// Shared post-resolution half of the FBW inline lever. Ordinary Python calls /// resolve their callee from the CALL operand; builtin-dispatch specializers /// resolve an app-level descriptor first and enter here with that function as @@ -2172,6 +2308,9 @@ pub(crate) fn try_walker_inline_resolved_user_call( let mut ca_callee_frame = OpRef::NONE; let mut ca_callee_ec = OpRef::NONE; let mut ca_nlocals = 0usize; + // 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::(); // A strict straight-line callee at the top inline level is seeded the same // way, so its in-callee guards route through the multi-frame snapshot. A // deeper strict callee (`inline_depth >= fbw_max_multiframe_depth()`) keeps the @@ -2385,6 +2524,7 @@ pub(crate) fn try_walker_inline_resolved_user_call( ca_callee_frame = callee_frame; ca_callee_ec = callee_ec; ca_nlocals = nlocals; + ca_concrete_frame = concrete_frame_ptr; callee_frame_seeded = true; } } @@ -2613,6 +2753,28 @@ pub(crate) fn try_walker_inline_resolved_user_call( ctx.outer_resume_marker_jit_pc, ) }; + // `executioncontext.py:88 enter` — emitted here, past every decline gate, + // so a callee that never runs leaves no half-entered chain behind. A + // seeded level is the only one with a frame object to enter with; an + // 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() && { + let concrete_ec = unsafe { (*ca_concrete_frame).execution_context } + as *mut pyre_interpreter::PyExecutionContext; + if concrete_ec.is_null() { + false + } else { + walker_ec_enter( + ctx.trace_ctx, + ca_callee_frame, + ca_callee_ec, + ca_concrete_frame, + concrete_ec, + ); + true + } + }; let (callee_outcome, callee_class_of_last_exc_is_const) = { let mut sub_wc = WalkContext { callee_shadow: Some(Default::default()), @@ -2937,6 +3099,23 @@ pub(crate) fn try_walker_inline_resolved_user_call( let class_of_last_exc_is_const = sub_wc.fbw_mode.class_of_last_exc_is_const; (result, class_of_last_exc_is_const) }; + // `executioncontext.py:91-107 leave`, in the original's `finally` + // position: the sub-walk block above is an expression that always + // completes, so every callee exit — return, exception, or decline — + // arrives here before any of the early returns below. + if entered_ec { + let concrete_ec = unsafe { (*ca_concrete_frame).execution_context } + as *mut pyre_interpreter::PyExecutionContext; + let got_exception = !matches!(callee_outcome, Ok((DispatchOutcome::SubReturn { .. }, _))); + walker_ec_leave( + ctx.trace_ctx, + ca_callee_frame, + ca_callee_ec, + ca_concrete_frame, + concrete_ec, + got_exception, + ); + } // RPython has one MetaInterp shared by every MIFrame. The sub-walk uses // a copied FbwWalkMode only to satisfy Rust's nested borrow, so write the // MetaInterp-owned exception state back across the frame boundary. diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index a8e47185ddb..a8d4146536f 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -101,18 +101,14 @@ //! token before the call and probe-and-clears it after, surfacing //! [`DispatchError::VableEscapedDuringResidualCall`] on a force //! (`pyjitpl.py` ABORT_ESCAPE parity). The vref halves -//! (`vrefs_before_residual_call` / `vrefs_after_residual_call` / -//! `stop_tracking_virtualref`) ARE ported — on `TraceCtx`, and -//! wired on the metainterp leg — but the walker never calls -//! them, so the gap is the call, not the port. Two -//! `virtualref_boxes` also exist: `TraceCtx`'s, which the -//! bracket and the guard snapshots read, and `PyreSym`'s, which -//! only the caller-less `opimpl_virtual_ref` / -//! `opimpl_virtual_ref_finish` and the resume-side decode -//! touch. A producer must push into `TraceCtx`'s or nothing -//! downstream sees it. Unreachable today — the codewriter -//! emits no `jit.virtual_ref` producers (`jit/call.rs`), -//! leaving both empty so every loop iterates zero times. +//! (`vrefs_before_residual_call` / `vrefs_after_residual_call` → +//! `stop_tracking_virtualref`) are bracketed at the same two +//! points, over the vrefs an inlined call's `enter` pushed +//! (`inline_call.rs::walker_ec_enter`). They still iterate zero +//! times for a callee the walker inlines without seeding a frame: +//! such a level has no frame object to take a `jit.virtual_ref` +//! of, where upstream's `perform_call` builds one for every +//! inlined call (`pyjitpl.py:2445-2476, 1862-1874`). //! b. **Codewriter-side**: `direct_assembler_call` + KEEPALIVE on //! vablebox (`pyjitpl.py:3589-3609 + 2080-2081`). Walker's //! residual_call dispatchers never receive `assembler_call=True` diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index c6ccf4a666b..6a88699159f 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -1886,6 +1886,14 @@ pub(crate) fn try_execute_residual_call_via_executor( // `vable_and_vrefs_before_residual_call` (pyjitpl.py) runs only past // the OS_NOT_IN_TRACE / force-virtual short-circuits. RPython mirror: // `pyjitpl.py`. + // `vrefinfo.tracing_before_residual_call(vref)` for every live vref + // (`pyjitpl.py:3341-3348`), which upstream runs first inside the same + // `vable_and_vrefs_before_residual_call` the vable half below mirrors. + // Stamps TOKEN_TRACING_RESCALL so the post-call check can tell "forced by + // this callee" from "untouched". Armed here, past every decline gate, for + // the same reason the vable half is: a declined residual must not strand a + // token. + ctx.trace_ctx.vrefs_before_residual_call(); let live_frame = if ctx.fbw_mode.snapshot_sym.is_null() { 0 } else { @@ -2089,6 +2097,14 @@ pub(crate) fn try_execute_residual_call_via_executor( .unwrap_or(std::ptr::null()); ctx.trace_ctx .set_virtualizable_heap_ptr(restored_vable_heap_ptr); + // `pyjitpl.py:2049` step 3, "after this call, check the vrefs. If any + // have been forced by the call, then we record in the trace a + // VIRTUAL_REF_FINISH---before we record any CALL". Runs before the + // virtualizable check below, matching the upstream order, and before the + // CALL op is recorded further down. A vref the callee handed to Python + // stops being tracked and its box becomes ConstPtr(NULL), so the resume + // snapshot no longer claims the frame is still virtual. + ctx.trace_ctx.vrefs_after_residual_call(); // `vinfo.tracing_after_residual_call(virtualizable)` // heap half: a cleared token means the callee forced the virtualizable — // the frame escaped, the trace must abort (pyjitpl.py @@ -2659,15 +2675,13 @@ pub(crate) fn do_not_in_trace_call_result( /// `*token_ptr == 0` assertion in `tracing_before_residual_call` /// intact. /// -/// Despite the name it records no vref half. `vrefs_before_residual_call` / -/// `vrefs_after_residual_call` / `stop_tracking_virtualref` are ported on -/// `TraceCtx` and wired on the metainterp leg; what is missing is the walker -/// calling them. Note the two `virtualref_boxes`: the bracket and the guard -/// snapshots read `TraceCtx`'s, while `PyreSym`'s is touched only by the -/// caller-less `opimpl_virtual_ref` and the resume-side decode. Unreachable -/// today — the codewriter emits no `jit.virtual_ref` producers -/// (`jit/call.rs`), leaving both empty so every upstream loop iterates zero -/// times. +/// The vref halves are bracketed by that same executor: +/// `TraceCtx::vrefs_before_residual_call` stamps every live vref before the +/// call and `vrefs_after_residual_call` turns any the callee forced into a +/// `VIRTUAL_REF_FINISH` + ConstPtr(NULL) box before the CALL op is recorded. +/// They iterate over the vrefs an inlined call's `enter` pushed +/// (`inline_call.rs::walker_ec_enter`), so a residual that hands an inlined +/// callee's frame to Python stops the trace from claiming it is still virtual. pub(crate) fn walker_vable_and_vrefs_before_residual_call(ctx: &mut TraceCtx) { // pyjitpl.py: vinfo = self.jitdriver_sd.virtualizable_info; // if vinfo is not None: diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index f3ad6e85821..faa8213dc34 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -2455,11 +2455,6 @@ pub struct PyreSym { /// Used by PUSH_EXC_INFO / POP_EXCEPT to preserve nested handler state. pub(crate) current_exc_value: pyre_object::PyObjectRef, pub(crate) current_exc_box: OpRef, - /// pyjitpl.py:2597 virtualref_boxes: pairs of (jit_virtual, real_vref). - /// Each pair: (symbolic OpRef, concrete pointer). - /// resume.py:1093 restores virtual references on guard failure. - /// Pairs stored flat: [virt_sym, virt_ptr, real_sym, real_ptr, ...]. - pub(crate) virtualref_boxes: Vec<(OpRef, usize)>, // ── RPython MIFrame.registers_{i,r,f} port (pyjitpl.py:74-90) ── // // RPython reference (target shape): @@ -5260,7 +5255,6 @@ impl PyreSym { trace_built_exc: indexmap::IndexMap::new(), current_exc_value: pyre_interpreter::eval::get_current_exception(), current_exc_box: OpRef::NONE, - virtualref_boxes: Vec::new(), // RPython pyjitpl.py:74-78 init: registers_X[i] = CONST_NULL for // i in num_regs. Sized lazily here — `setup_kind_register_banks` // resizes `registers_i` / `registers_f` once the owning JitCode is @@ -5816,66 +5810,6 @@ impl PyreSym { } } -/// pyjitpl.py:1789-1814 opimpl_virtual_ref parity. -/// Creates a concrete JitVirtualRef via virtual_ref_during_tracing(), -/// records VIRTUAL_REF(box, cindex), and pushes -/// [virtualbox, vrefbox] onto virtualref_boxes. -/// -/// Upstream's caller is `executioncontext.py:89 enter`, which the tracer -/// reaches by tracing through the interpreter's own frame-entry code. The -/// pyre walker builds its inline levels itself and never traces `enter`, so -/// this has no caller yet and `virtualref_boxes` stays empty in every live -/// trace. Wiring it is a prerequisite for the multi-frame blackhole adopt -/// (`try_adopt_multi_frame_blackhole`, `trace.rs`). -pub(crate) fn opimpl_virtual_ref( - ctx: &mut TraceCtx, - sym: &mut PyreSym, - virtual_obj: OpRef, - virtual_obj_ptr: usize, -) -> OpRef { - // pyjitpl.py:1804: virtual_ref_during_tracing(virtual_obj) - let vref_info = majit_metainterp::virtualref::VirtualRefInfo::new(); - let vref_ptr = vref_info.virtual_ref_during_tracing(virtual_obj_ptr as *mut u8); - // pyjitpl.py:1805: cindex = ConstInt(len(virtualref_boxes) // 2) - let cindex = ctx.const_int((sym.virtualref_boxes.len() / 2) as i64); - // pyjitpl.py:1806: record VIRTUAL_REF(box, cindex) - let vref = ctx.record_op(OpCode::VirtualRefR, &[virtual_obj, cindex]); - // pyjitpl.py:1807: heapcache.new(resbox) - ctx.heap_cache_mut().new_box(vref); - // pyjitpl.py:1814: virtualref_boxes += [virtualbox, vrefbox] - sym.virtualref_boxes.push((virtual_obj, virtual_obj_ptr)); - sym.virtualref_boxes.push((vref, vref_ptr as usize)); - vref -} - -/// pyjitpl.py:1819-1831 opimpl_virtual_ref_finish parity. -/// Pops vrefbox and lastbox from virtualref_boxes (LIFO), -/// asserts `box == lastbox`, records VIRTUAL_REF_FINISH if still virtual. -/// -/// Called from metainterp finishframe_inline/exception (executioncontext.leave parity). -pub(crate) fn opimpl_virtual_ref_finish(ctx: &mut TraceCtx, sym: &mut PyreSym, virtual_obj: OpRef) { - if sym.virtualref_boxes.len() < 2 { - return; - } - // pyjitpl.py:1821: vrefbox = virtualref_boxes.pop() - let (vref_opref, vref_ptr) = sym.virtualref_boxes.pop().unwrap(); - // pyjitpl.py:1822: lastbox = virtualref_boxes.pop() - let (lastbox_opref, _lastbox_ptr) = sym.virtualref_boxes.pop().unwrap(); - // pyjitpl.py:1823: assert box.getref_base() == lastbox.getref_base() - debug_assert_eq!( - virtual_obj, lastbox_opref, - "opimpl_virtual_ref_finish: leaving frame box != top virtualref box" - ); - // pyjitpl.py:1831: if is_virtual_ref(vref) → record VIRTUAL_REF_FINISH - let vref_info = majit_metainterp::virtualref::VirtualRefInfo::new(); - let is_vref = vref_ptr != 0 && unsafe { vref_info.is_virtual_ref(vref_ptr as *const u8) }; - if is_vref { - // pyjitpl.py:1832: VIRTUAL_REF_FINISH(vrefbox, nullbox) - let null = ctx.const_ref(0); - let _ = ctx.record_op(OpCode::VirtualRefFinish, &[vref_opref, null]); - } -} - impl PyreJitState { /// Canonical PyPy portal driver layout from `interp_jit.py:67-74`. /// @@ -9374,10 +9308,11 @@ impl JitState for PyreJitState { // (majit/majit-ir/src/resumedata.rs:402-416) decoded it into // `resume_data.virtualref_values`. Materialize the OpRef + concrete // pointer for each pair through the same `resolve` / `decode_concrete` - // callbacks used for `frames[0].values`, then push the pairs into - // `sym.virtualref_boxes` so `opimpl_virtual_ref` / - // `opimpl_virtual_ref_finish` handlers (state.rs:3475-3500) observe - // the parent's still-open virtualref scope at bridge entry. + // callbacks used for `frames[0].values`, then hand the pairs to the + // trace's own `virtualref_boxes` — the single store, matching + // upstream's one `MetaInterp.virtualref_boxes` — so an inlined call's + // `virtual_ref_finish` and the residual-call bracket both observe the + // parent's still-open virtualref scope at bridge entry. // // For traces with no active `virtual_ref`, `virtualref_values` is // empty and this loop is a no-op. @@ -9398,6 +9333,8 @@ impl JitState for PyreJitState { "virtualref_values must contain an even number of entries (got {})", vref_values.len(), ); + let mut restored_virtualref_boxes: Vec<(OpRef, usize)> = + Vec::with_capacity(vref_values.len()); for pair in vref_values.chunks_exact(2) { let (virt_opref, virt_val) = bridge_decode_box( ctx, @@ -9423,8 +9360,8 @@ impl JitState for PyreJitState { ); let virt_ptr = value_to_usize(&virt_val); let vref_ptr = value_to_usize(&vref_val); - sym.virtualref_boxes.push((virt_opref, virt_ptr)); - sym.virtualref_boxes.push((vref_opref, vref_ptr)); + restored_virtualref_boxes.push((virt_opref, virt_ptr)); + restored_virtualref_boxes.push((vref_opref, vref_ptr)); // pyjitpl.py:3438 / resume.py:1397: continue_tracing is called // unconditionally for every (vref, real_object) pair. The // is_virtual_ref(vref) guard (virtualref.py:123) and the @@ -9436,6 +9373,8 @@ impl JitState for PyreJitState { vrefinfo.continue_tracing(vref_ptr as *mut u8, virt_ptr as *mut u8); } } + // `pyjitpl.py:3433 self.virtualref_boxes = virtualref_boxes`. + ctx.restore_virtualref_boxes(restored_virtualref_boxes); // `sync_virtualizable_after_guard_failure` runs before bridge setup, // but on the multi-frame inlined-callee path its resume-decoded array diff --git a/pyre/pyre-jit-trace/src/trace_opcode.rs b/pyre/pyre-jit-trace/src/trace_opcode.rs index 3140680cae3..e727d5553d8 100644 --- a/pyre/pyre-jit-trace/src/trace_opcode.rs +++ b/pyre/pyre-jit-trace/src/trace_opcode.rs @@ -2586,16 +2586,15 @@ impl MIFrame { self.orgpc = pc; } self.generate_guard(ctx, majit_ir::OpCode::GuardFutureCondition, &[]); - // pyjitpl.py:2971 assert len(self.virtualref_boxes) == 0, + // pyjitpl.py:2995 assert len(self.virtualref_boxes) == 0, // "missing virtual_ref_finish()?" - // Reached loop header must not have dangling virtualrefs — they - // should have been finished by prior vrefs_after_residual_call / - // stop_tracking_virtualref. pyre's equivalent is sym.virtualref_boxes. - debug_assert!( - self.sym().virtualref_boxes.is_empty(), - "missing virtual_ref_finish()? close_loop_args_at reached with \ - virtualref_boxes={:?}", - self.sym().virtualref_boxes.len() + // Reached loop header must not have dangling virtualrefs — every + // `enter` must have been matched by a `leave`, or one closed by + // vrefs_after_residual_call / stop_tracking_virtualref. + debug_assert_eq!( + ctx.virtualref_boxes_len(), + 0, + "missing virtual_ref_finish()? close_loop_args_at reached with open virtualrefs" ); // Verify `live_args_shape_at` formula matches actual output. // If this fires, the helper's shape derivation is stale relative @@ -2982,7 +2981,10 @@ impl MIFrame { // multi-frame parent chain was the retired trait-interpret leg. let frames = vec![top_frame]; 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(); // PHASE 1.4 candidate D probe: detect snapshot-time divergence // between vable_boxes (heap mirror) and registers_r (machine // register source). Both should be populated by store_local_value's @@ -3348,19 +3350,6 @@ impl MIFrame { boxes } - /// pyjitpl.py:2597 virtualref_boxes parity. - /// pyjitpl.py:2597 virtualref_boxes parity. - /// Returns pairs of (jit_virtual, real_vref) as SnapshotTagged. - fn build_virtualref_boxes( - sym: &PyreSym, - ctx: &majit_metainterp::TraceCtx, - ) -> Vec { - sym.virtualref_boxes - .iter() - .map(|&(opref, _concrete)| Self::opref_to_snapshot_tagged(opref, ctx)) - .collect() - } - /// RPython pyjitpl.py:177 get_list_of_active_boxes parity: #[allow(dead_code)] fn fail_args_to_snapshot_boxes( From e02a744a2a754e67743dcf8a254f22f1754e2ce9 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 02:53:38 +0900 Subject: [PATCH 02/18] jit(fbw): Codex parity review fixes for the inlined-call enter/leave `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 --- .../src/jitcode_dispatch/inline_call.rs | 56 +++++++++++++++++-- .../src/jitcode_dispatch/residual_call.rs | 18 +++++- pyre/pyre-jit/src/eval.rs | 16 ++++-- 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 01bb7899bc0..5c517ec4012 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1691,6 +1691,14 @@ fn walker_ec_enter( /// [`pyre_interpreter::PyExecutionContext::leave`]; a traced call never runs a /// profile hook, which is why upstream can inline this frame at all. /// +/// The escape branch runs in both worlds. Concretely it marks the caller and +/// forces the leaving vref; in the trace it records the force as +/// `VIRTUAL_REF_FINISH(vrefbox, virtualbox)` — upstream's "already forced +/// during tracing" form — rather than the NULL form, so `vref.forced` ends up +/// pointing at the virtual instead of staying NULL. That is what keeps a +/// later read through a deeper escaped frame's `f_backref` from hitting +/// `InvalidVirtualRef`. +/// /// Upstream runs this from a `finally`, so the caller must too: every path out /// of the callee level — normal return, raised exception, or a declined /// sub-walk — has to reach it, or `virtualref_boxes` is left unbalanced and the @@ -1720,10 +1728,11 @@ fn walker_ec_leave( &[callee_ec, f_backref], crate::descr::ec_topframeref_descr(), ); - unsafe { + let escaped = unsafe { let frame_vref = (*concrete_ec).topframeref; (*concrete_ec).topframeref = concrete_f_backref; - if (*concrete_frame).escaped() || got_exception { + let escaped = (*concrete_frame).escaped() || got_exception; + if escaped { // A frame that reached app level must keep its caller reachable // too, or the next `_getframe().f_back` walks into a frame the JIT // was still free to keep virtual. `get_f_back` forces, which is @@ -1733,11 +1742,40 @@ fn walker_ec_leave( (*f_back).mark_as_escaped(); } // `frame_vref()` — force the leaving frame's own vref so it - // outlives the JIT frame. A no-op at recording time (the vref - // already carries `forced`), kept because it is the operation - // upstream performs and the optimizer reads the trace, not this. + // outlives the JIT frame. let _ = pyre_interpreter::executioncontext::force_vref(frame_vref); } + escaped + }; + if escaped { + // The concrete force above is only half of `frame_vref()`: the + // optimizer reads the trace, not the heap. Record the force as + // `VIRTUAL_REF_FINISH(vrefbox, virtualbox)` — the non-null second + // operand is upstream's "this vref was forced during tracing already" + // encoding, which `optimize_VIRTUAL_REF_FINISH` lowers to storing the + // virtual into `vref.forced` (`virtualize.py:141-151`). + // + // Without it the finish below would emit the NULL form, leaving + // `forced` NULL and `virtual_token` cleared, and a later read through + // a deeper escaped frame's `f_backref` would hit `InvalidVirtualRef`. + // `stop_tracking_virtualref` also replaces the vrefbox with + // ConstPtr(NULL), so the finish that follows sees a non-vref and + // records nothing — one finish, not two. + // + // Upstream reaches the same end state by a different route: it records + // `frame_vref()` as a force and then the ordinary NULL finish, so + // `forced` is written at runtime by `force_now` rather than by the + // optimizer. That route needs the `jit_force_virtual` lowering, which + // is the one piece of this protocol pyre has not wired + // (`jitcode_dispatch/mod.rs` item c — `_do_jit_force_virtual` is + // tests-only, production reach 0). Both forms are already understood + // by `optimize_VIRTUAL_REF_FINISH`, so this uses the one that is + // reachable; converge on the upstream spelling when the force lowering + // lands. + 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); @@ -3106,7 +3144,13 @@ pub(crate) fn try_walker_inline_resolved_user_call( if entered_ec { let concrete_ec = unsafe { (*ca_concrete_frame).execution_context } as *mut pyre_interpreter::PyExecutionContext; - let got_exception = !matches!(callee_outcome, Ok((DispatchOutcome::SubReturn { .. }, _))); + // `leave(frame, w_exitvalue, got_exception)` — the caller passes true + // only when the frame is unwinding an exception, which for an inlined + // callee is `SubRaise` and nothing else. A tracing decline (`Err`) or + // a loop transition is not an exception exit: treating it as one would + // permanently `mark_as_escaped` the caller and force a vref that never + // needed forcing. + let got_exception = matches!(callee_outcome, Ok((DispatchOutcome::SubRaise { .. }, _))); walker_ec_leave( ctx.trace_ctx, ca_callee_frame, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index 6a88699159f..3e68c60b196 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -1893,7 +1893,16 @@ pub(crate) fn try_execute_residual_call_via_executor( // this callee" from "untouched". Armed here, past every decline gate, for // the same reason the vable half is: a declined residual must not strand a // token. - ctx.trace_ctx.vrefs_before_residual_call(); + // + // Gated on `is_may_force` — the walker's `check_forces_virtual_or_ + // virtualizable()` — because `do_residual_call` runs the whole preparation + // block only for `assembler_call or effectinfo.check_forces_...` + // (`pyjitpl.py:2007`). A call that cannot force needs no stamp, and + // stamping one would leave `tracing_after_residual_call` reading a token + // nobody will clear. + if is_may_force { + ctx.trace_ctx.vrefs_before_residual_call(); + } let live_frame = if ctx.fbw_mode.snapshot_sym.is_null() { 0 } else { @@ -2103,8 +2112,11 @@ pub(crate) fn try_execute_residual_call_via_executor( // virtualizable check below, matching the upstream order, and before the // CALL op is recorded further down. A vref the callee handed to Python // stops being tracked and its box becomes ConstPtr(NULL), so the resume - // snapshot no longer claims the frame is still virtual. - ctx.trace_ctx.vrefs_after_residual_call(); + // snapshot no longer claims the frame is still virtual. Paired with the + // pre-call stamp, so it carries the same `is_may_force` gate. + if is_may_force { + ctx.trace_ctx.vrefs_after_residual_call(); + } // `vinfo.tracing_after_residual_call(virtualizable)` // heap half: a cleared token means the callee forced the virtualizable — // the frame escaped, the trace must abort (pyjitpl.py diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index c79fdccc566..ae617298b93 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3834,10 +3834,18 @@ unsafe extern "C" fn force_pyframe_vref( // returns into RPython; this hook is a `extern "C"` pointer read from the // frame chain with no exception channel, so the unreachable state is // asserted instead. Unreachable holds because a vref reaches the chain - // only between `virtual_ref` and `virtual_ref_finish`, and `finish` writes - // `forced` before the frame is dropped. Whoever teaches the tracer to - // emit `VIRTUAL_REF` owns re-checking that: a trace aborted between the - // two leaves a vref naming a JIT frame that is already gone. + // only between `virtual_ref` and `virtual_ref_finish`, and every path that + // can outlive the finish writes `forced` first. + // + // Re-checked now that the walker emits `VIRTUAL_REF` at the inlined-call + // push (`inline_call.rs::walker_ec_enter`). A leaving frame that escaped + // takes `ExecutionContext.leave`'s escape branch, which records + // `VIRTUAL_REF_FINISH(vrefbox, virtualbox)` — the form that stores the + // virtual into `forced` — and marks the caller escaped so its own leave + // does the same. A frame that did not escape has no reader left once + // `leave` restores `topframeref`. The state that would reach here is a + // vref finished with the NULL form and still read afterwards, which + // requires that propagation to have been skipped. forced.expect("InvalidVirtualRef: frame-chain vref forced after its frame died") as *mut pyre_interpreter::PyFrame } From d0307d25eb58433240d8d442bb50496fcfb3199c Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 03:13:27 +0900 Subject: [PATCH 03/18] majit, jit(fbw): second Codex parity review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- majit/majit-metainterp/src/trace_ctx.rs | 38 +++++++++++-------- .../src/jitcode_dispatch/inline_call.rs | 11 ++++-- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index ac34d7c0aa3..f6dfd8691d3 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -1115,22 +1115,28 @@ impl TraceCtx { // pyjitpl.py:1823 `assert box.getref_base() == lastbox.getref_base()` // — compare the concrete ref base, not the SSA OpRef. PyPy permits // alias boxes that share `getref_base()` but differ in box identity; - // an `OpRef`-identity assert would reject those. Read `virtual_obj`'s - // ref value off its variant tag when it is a ConstPtr, falling back to - // the pre-pop side-table pointer the matching `opimpl_virtual_ref` - // recorded as `lastbox_ptr`. - let virtual_obj_ptr = match virtual_obj.inline_const_to_value() { - Some(Value::Ref(r)) => r.as_usize(), - _ => lastbox_ptr, - }; - // RPython's plain `assert` fires in both untranslated and translated - // builds, so this is an `assert_eq!`: a release build must fail at the - // divergence rather than silently corrupt the vref stack. - assert_eq!( - virtual_obj_ptr, lastbox_ptr, - "opimpl_virtual_ref_finish: leaving frame ref != top virtualref ref \ - (virtual_obj={virtual_obj:?}, lastbox={lastbox:?})" - ); + // an `OpRef`-identity assert would reject those. + // + // `concrete_of_opref` is pyre's `getref_base()`: it reads a ConstPtr's + // value inline and otherwise resolves the `opref_concrete` stamp that + // every recording site writes. Sourcing it that way is what gives the + // assert teeth — deriving it from `lastbox_ptr` on a non-const box, as + // an earlier spelling did, compared the popped pointer against itself + // and could never fire, which is exactly the mismatched-nesting bug + // upstream is asserting against. A box with no stamp at all is not a + // mismatch, only an unknown, so it is skipped rather than failed. + if let Some(Value::Ref(r)) = self.concrete_of_opref(virtual_obj) { + // RPython's plain `assert` fires in both untranslated and + // translated builds, so this is an `assert_eq!`: a release build + // must fail at the divergence rather than silently corrupt the + // vref stack. + assert_eq!( + r.as_usize(), + lastbox_ptr, + "opimpl_virtual_ref_finish: leaving frame ref != top virtualref ref \ + (virtual_obj={virtual_obj:?}, lastbox={lastbox:?})" + ); + } // pyjitpl.py:1826-1832 `if vrefinfo.is_virtual_ref(vref): record // VIRTUAL_REF_FINISH`. False once `stop_tracking_virtualref` has // replaced the box with ConstPtr(NULL) — the finish already ran. diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 5c517ec4012..3881e77c045 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1687,9 +1687,14 @@ fn walker_ec_enter( /// jit.virtual_ref_finish(frame_vref, frame) /// ``` /// -/// The profile-hook half stays with the interpreter's own -/// [`pyre_interpreter::PyExecutionContext::leave`]; a traced call never runs a -/// profile hook, which is why upstream can inline this frame at all. +/// The profile-hook half (`if self.profilefunc: self._trace(frame, +/// 'leaveframe', w_exitvalue)`) stays with the interpreter's own +/// [`pyre_interpreter::PyExecutionContext::leave`]. Omitting it here does not +/// lose a leave event, because `is_being_profiled` is a portal-driver GREEN +/// (`interp_jit.py:68 greens = ['next_instr', 'is_being_profiled', 'pycode']`): +/// a trace is keyed on it, so one recorded with profiling off is only ever +/// entered with profiling off, and turning profiling on selects a different +/// green key rather than reusing this trace. /// /// The escape branch runs in both worlds. Concretely it marks the caller and /// forces the leaving vref; in the trace it records the force as From efaf811b78628a8ae1203dd8f3d1fb9d8bc9f7bb Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 03:30:35 +0900 Subject: [PATCH 04/18] jit: seed w_class from SizeDescr::w_class_obj in the resume materializers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- pyre/pyre-jit-trace/src/state.rs | 53 ++++++++++++++++---------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index faa8213dc34..39c0659cb4d 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -7578,14 +7578,18 @@ fn materialize_concrete_virtual_ptr( } // Pyre adaptation: bh_new_with_vtable writes vtable at // vtable_offset but PyObject.w_class needs separate init - // (pyobject.rs:51). Matches materialize_virtual_object at - // state.rs:7220. - if vtable != 0 { + // (pyobject.rs:51). Matches materialize_virtual_object. + // + // `w_class_obj()` — not `get_instantiate(vtable)` — is the source: + // a vtable word is only a `PyType` pointer for a pyre object + // descr. `JitVirtualRef` carries the `jit_virtual_ref_vtable` + // type-id constant there (`virtualref.py:21-23`) and its offset-8 + // slot is `virtual_token`, not `w_class`, so it returns None and + // this seeding is skipped. + if let Some(w_class) = size_descr.w_class_obj() { unsafe { let pyobj = ptr as *mut pyre_object::PyObject; - (*pyobj).w_class = pyre_object::pyobject::get_instantiate( - &*(vtable as *const pyre_object::pyobject::PyType), - ); + (*pyobj).w_class = w_class as pyre_object::pyobject::PyObjectRef; } } let gcref = majit_ir::GcRef(ptr as usize); @@ -9975,9 +9979,7 @@ fn materialize_virtual_object( fields: &[(u32, majit_metainterp::resume::MaterializedValue)], materialized_refs: &[Option], ) -> Option { - use pyre_object::pyobject::{ - OB_TYPE_OFFSET, PyObject, PyType, W_CLASS_OFFSET, get_instantiate, - }; + use pyre_object::pyobject::{OB_TYPE_OFFSET, PyObject, PyType, W_CLASS_OFFSET}; let size_descr = descr.as_size_descr()?; let vtable = size_descr.vtable(); @@ -9996,24 +9998,21 @@ fn materialize_virtual_object( return None; } - if vtable as u64 == majit_metainterp::virtualref::JIT_VIRTUAL_REF_VTABLE { - // `JitVirtualRef` is a `GcStruct` whose `('super', rclass.OBJECT)` slot - // holds the type-id constant itself, not a `PyType *`. It has no - // `w_class`, and running the arm below would dereference the - // `JIT_VIRTUAL_REF_VTABLE` magic as a type object. The field replay - // then fills `virtual_token` and `forced` from the traced values, the - // same two the optimizer seeds when it lowers `VIRTUAL_REF`. - unsafe { (raw as *mut u64).write(vtable as u64) }; - } else { - unsafe { - let ptr = raw as *mut PyObject; - (*ptr).ob_type = vtable as *const PyType; - // rclass.py:739-743 set `w_class` from the cached instantiate - // pointer on the PyType. Tracing may later overwrite this via - // an explicit `SetfieldGc(w_class)`; the field replay below - // takes precedence for that case (heaptracker.py:66-style - // "typeptr" filter does NOT apply to w_class in pyre). - (*ptr).w_class = get_instantiate(&*(vtable as *const PyType)); + unsafe { + let ptr = raw as *mut PyObject; + (*ptr).ob_type = vtable as *const PyType; + // rclass.py:739-743 set `w_class` from the cached instantiate + // pointer on the PyType. Tracing may later overwrite this via + // an explicit `SetfieldGc(w_class)`; the field replay below + // takes precedence for that case (heaptracker.py:66-style + // "typeptr" filter does NOT apply to w_class in pyre). + // + // `w_class_obj()` is None when the vtable word is not a `PyType` + // pointer — `JitVirtualRef` stores the `jit_virtual_ref_vtable` + // type-id constant at offset 0 (`virtualref.py:21-23`) and keeps + // `virtual_token` where `w_class` would sit. + if let Some(w_class) = size_descr.w_class_obj() { + (*ptr).w_class = w_class as *mut PyObject; } } From 24fe83338aefcf7dfd7fd91da42179208f2e80cb Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 07:45:54 +0900 Subject: [PATCH 05/18] jit(fbw): force the vref in gettopframe_raw; balance ec.topframeref across an assembler run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- majit/majit-metainterp/src/virtualref.rs | 50 ++++++++++++------- pyre/pyre-interpreter/src/executioncontext.rs | 9 +++- pyre/pyre-jit/src/eval.rs | 21 ++++++++ 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index bd10cbfef4d..97b6cc9421c 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -151,31 +151,43 @@ pub use crate::jit::InvalidVirtualRef; /// Initializes virtual_token = TOKEN_NONE, forced = real_object. /// Returns raw pointer; caller owns the allocation. /// -/// The allocation is a leaked `Box`, not a GC object as -/// `lltype.malloc(self.JIT_VIRTUAL_REF)` is: the vref outlives every scope -/// that could free it — the trace stores its address in `virtualref_boxes` -/// and in resume data, the interpreter stores it in `ec.topframeref` / -/// `f_backref`, and after `virtual_ref_finish` a forced frame may still be -/// reached through it — so ownership genuinely belongs to the collector. Two -/// consequences follow, and both are why this is safe rather than merely -/// tolerated: the address never moves, which is what lets `virtualref_boxes` -/// hold it as a raw pointer across a collection, and the GC skips it as -/// unowned when it visits the root slot it sits in -/// (`majit-gc gc_current_object_address` early-out). What is lost is -/// reclamation: one 24-byte vref per inlined call, per trace recording, is -/// never freed. That is bounded by compilation events rather than by -/// iterations, and converges the same way the sibling divergence noted on -/// `set_vref_gc_type_id`'s registration does — by moving this allocation, the -/// `_dummy`, and the JITFRAME under the GC together. +/// `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 a backend has installed its +/// allocator hook (`vref_gc_type_id() == 0`, or 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 != 0 { + let gcref = majit_gc::alloc_oldgen_typed(type_id, std::mem::size_of::()); + 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 } /// Token value indicating no JIT frame is active. diff --git a/pyre/pyre-interpreter/src/executioncontext.rs b/pyre/pyre-interpreter/src/executioncontext.rs index 481ed175137..e9ba891d1da 100644 --- a/pyre/pyre-interpreter/src/executioncontext.rs +++ b/pyre/pyre-interpreter/src/executioncontext.rs @@ -429,9 +429,16 @@ impl ExecutionContext { frame } + /// `self.topframeref()` without the virtualizable force that + /// [`Self::gettopframe`] adds — "raw" is about `force_frame`, not about the + /// vref. The vref force is not optional: `executioncontext.py:68/72/446/451` + /// all read the slot *with* the parens, and the only unforced reads upstream + /// are `:88`/`:96`, which move the vref along rather than dereference it. + /// A `JitVirtualRef` handed out here would be dereferenced at `PyFrame` + /// field offsets by every caller. #[inline] pub fn gettopframe_raw(&self) -> *mut PyFrame { - self.topframeref + force_vref(self.topframeref) } /// `executioncontext.py gettopframe_nohidden` — follow `f_backref` past every diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index ae617298b93..645a208e9d2 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -7515,6 +7515,24 @@ fn execute_assembler( } } + // `executioncontext.py:91-107 leave` runs from a `finally`, so upstream's + // `topframeref` is balanced no matter how a frame is left: a guard failure + // inside an inlined callee resumes into that callee's own `MIFrame` level + // and its `leave` still executes. Pyre's compiled trace carries `enter` / + // `leave` as walker-recorded field ops rather than as jitcode, so the ops + // after a failing guard never run and every `enter` this run performed + // without its matching `leave` would leave a `JitVirtualRef` published in + // the live slot — read as a `PyFrame` by the interpreter that resumes, and + // still carrying an active FORCE_TOKEN naming the JIT frame that has just + // died. Bracket the assembler run the way `install_current_frame` / + // `CurrentFrameGuard` bracket a frame: a balanced run restores the same + // value it saved, an unbalanced exit restores the caller. + 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 } + }; // warmstate.py:395 func_execute_token(loop_token, *args) → deadframe let outcome = { let _frame_locals_root = FrameLocalsRoot::new(frame_root.frame()); @@ -7526,6 +7544,9 @@ fn execute_assembler( || {}, ) }; + if !ec_for_topframeref.is_null() { + unsafe { (*ec_for_topframeref).topframeref = saved_topframeref }; + } // rstack.stack_check_slowpath → _StackOverflow parity: drain the // JIT-overflow flag the backend probe records when it trips. The From 3a70fe0121c360df910df895acee12d47fd29da1 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 09:40:29 +0900 Subject: [PATCH 06/18] jit(fbw): replay ExecutionContext.leave for bridge-resumed frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- majit/majit-metainterp/src/trace_ctx.rs | 9 ++++ .../src/jitcode_dispatch/bridge_subwalk.rs | 50 +++++++++++++++++++ .../src/jitcode_dispatch/inline_call.rs | 2 +- pyre/pyre-jit-trace/src/state.rs | 36 +++++++++++++ pyre/pyre-jit-trace/src/trace.rs | 8 +++ 5 files changed, 104 insertions(+), 1 deletion(-) diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index f6dfd8691d3..f5e6ae83155 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -1052,6 +1052,15 @@ impl TraceCtx { self.virtualref_boxes.len() } + /// The innermost still-open scope's `virtualbox` — `virtualref_boxes[-2]`, + /// the operand `opimpl_virtual_ref_finish` pops next. A bridge resumes + /// into scopes its parent guard opened, so the frame box that closes one is + /// the one the parent encoded, not a box this trace built. + pub fn innermost_virtualref_virtual(&self) -> Option<(OpRef, usize)> { + let len = self.virtualref_boxes.len(); + (len >= 2).then(|| self.virtualref_boxes[len - 2]) + } + /// `pyjitpl.py:3433 rebuild_state_after_failure`'s /// `self.virtualref_boxes = virtualref_boxes`. A bridge resumes into its /// parent's still-open `virtual_ref` scopes, so the pairs the parent guard diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs index 0a6bfac00df..c1cab2237f9 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs @@ -9,6 +9,56 @@ use super::*; +/// `executioncontext.py:91-107 leave` for a frame the bridge resumed into +/// rather than entered. +/// +/// 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`). +/// 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 close is emitted here as each carrier frame returns. +/// +/// Without it the bridge finishes with `ec.topframeref` still naming the +/// resumed frame's vref — one that, in compiled code, carries a live +/// FORCE_TOKEN and a null `forced` (`virtualize.py optimize_VIRTUAL_REF`). Any +/// later `gettopframe` in the same trace then forces a vref whose scope no +/// guard still encodes, and the force yields null. +/// +/// The frame box comes from the restored pair rather than from anything this +/// trace built: it is the box the parent's `enter` used, which is what +/// `opimpl_virtual_ref_finish`'s identity assert compares against. +pub(crate) fn carrier_ec_leave( + ctx: &mut TraceCtx, + root_sym: &Sym, + got_exception: bool, +) { + let Some((callee_frame, concrete_frame)) = ctx.innermost_virtualref_virtual() else { + return; + }; + let concrete_ec = + root_sym.concrete_execution_context() as *mut pyre_interpreter::PyExecutionContext; + if concrete_frame == 0 || concrete_ec.is_null() { + return; + } + // `frame.execution_context`, the same read `walker_ec_enter`'s counterpart + // performs at the inlined-call push. + let callee_ec = ctx.record_op_with_descr( + OpCode::GetfieldGcR, + &[root_sym.frame()], + crate::descr::pyframe_execution_context_descr(), + ); + super::inline_call::walker_ec_leave( + ctx, + callee_frame, + callee_ec, + concrete_frame as *mut pyre_interpreter::PyFrame, + concrete_ec, + got_exception, + ); +} + #[allow(clippy::too_many_arguments)] pub fn dispatch_via_miframe( trace_ctx: &mut TraceCtx, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 3881e77c045..6816e2b2453 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1708,7 +1708,7 @@ fn walker_ec_enter( /// of the callee level — normal return, raised exception, or a declined /// sub-walk — has to reach it, or `virtualref_boxes` is left unbalanced and the /// loop header trips `assert len(self.virtualref_boxes) == 0`. -fn walker_ec_leave( +pub(crate) fn walker_ec_leave( ctx: &mut TraceCtx, callee_frame: OpRef, callee_ec: OpRef, diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 39c0659cb4d..48bf27c1fc8 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -9377,6 +9377,42 @@ impl JitState for PyreJitState { vrefinfo.continue_tracing(vref_ptr as *mut u8, virt_ptr as *mut u8); } } + // `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 trace that + // abandoned an inlined callee level at this guard left that level's + // vref published — and a compiled-trace vref carries a live FORCE_TOKEN + // with a null `forced` (`virtualize.py optimize_VIRTUAL_REF`), naming a + // JIT frame that has now exited. Forcing it reaches `cpu.force` with + // no armed `jf_force_descr`. + // + // The resume data says what the slot should hold: the innermost scope + // the guard still had open, whose vref the `continue_tracing` loop + // above has just repaired, or — with no scope open — the resumed frame + // itself. Rewrite only when the slot holds some *other* vref; an + // already-correct slot (every non-abandoning exit) is left alone. + let restored_top = restored_virtualref_boxes + .last() + .map(|&(_, vref_ptr)| vref_ptr) + .unwrap_or(sym.concrete_vable_ptr as usize); + let live_ec = if sym.concrete_vable_ptr.is_null() { + std::ptr::null_mut() + } else { + let frame = sym.concrete_vable_ptr as *const pyre_interpreter::PyFrame; + unsafe { (*frame).execution_context as *mut pyre_interpreter::PyExecutionContext } + }; + if !live_ec.is_null() && restored_top != 0 { + let published = unsafe { (*live_ec).topframeref }; + if published as usize != restored_top + && unsafe { + majit_metainterp::virtualref::ptr_is_virtual_ref(published as *const u8) + } + { + unsafe { + (*live_ec).topframeref = restored_top as *mut pyre_interpreter::PyFrame; + } + } + } // `pyjitpl.py:3433 self.virtualref_boxes = virtualref_boxes`. ctx.restore_virtualref_boxes(restored_virtualref_boxes); diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index ebddf237575..0093e5a401e 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -1646,6 +1646,9 @@ fn drive_bridge_carrier_walk( let want_compile = n >= 1 && n <= crate::jitcode_dispatch::fbw_max_multiframe_depth(); let mut middles_ok = true; if want_compile { + // The deepest carrier frame has returned; close the `virtual_ref` + // scope its `enter` opened in the parent trace. + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); for i in (0..n.saturating_sub(1)).rev() { // recipes[i]'s paused parents are the shallower frames // recipes[..i] (the root sits above them all). @@ -1665,6 +1668,7 @@ fn drive_bridge_carrier_walk( break; } } + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); } } if want_compile && middles_ok { @@ -1707,6 +1711,10 @@ fn drive_bridge_carrier_walk( if let Some((exc, exc_concrete)) = subwalk_raise { if carrier.recipes.len() == 1 { if let Some(catch_target) = carrier_root_catch_target(sym, root_pc) { + // The carrier frame is unwinding, so this is `leave`'s + // `got_exception=True` arm: the caller is marked escaped and + // the leaving frame's own vref is forced. + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, true); crate::jitcode_dispatch::set_carrier_raise_seed( crate::jitcode_dispatch::CarrierRaiseSeed { exc, From 393cd2538d91ea667d4344e01b82aa9ea73832da Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Sun, 26 Jul 2026 09:58:02 +0900 Subject: [PATCH 07/18] majit(virtualref): guard alloc_virtual_ref on the real unset sentinel `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 --- majit/majit-metainterp/src/virtualref.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index 97b6cc9421c..ab435e639c3 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -15,10 +15,15 @@ use std::sync::atomic::{AtomicU32, Ordering}; +/// The value [`VREF_GC_TYPE_ID`] holds before `set_vref_gc_type_id` runs. Zero +/// is a legitimate id, so the sentinel has to be a value the registry never +/// hands out. +const VREF_GC_TYPE_ID_UNSET: u32 = u32::MAX; + /// GC type id for JitVirtualRef, set by `set_vref_gc_type_id()` at startup. /// RPython registers JIT_VIRTUAL_REF as a real GC type; pyre does the same /// via `gc.register_type(TypeInfo::with_gc_ptrs(...))` in eval.rs. -static VREF_GC_TYPE_ID: AtomicU32 = AtomicU32::new(u32::MAX); +static VREF_GC_TYPE_ID: AtomicU32 = AtomicU32::new(VREF_GC_TYPE_ID_UNSET); /// Set the GC type id for JitVirtualRef. Called once at startup after /// `gc.register_type()` returns the assigned id. @@ -166,8 +171,8 @@ pub use crate::jit::InvalidVirtualRef; /// 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 a backend has installed its -/// allocator hook (`vref_gc_type_id() == 0`, or an old-gen allocation failure); +/// 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 = JitVirtualRef { @@ -178,7 +183,7 @@ fn alloc_virtual_ref(real_object: *mut u8) -> *mut u8 { forced: real_object, }; let type_id = vref_gc_type_id(); - if type_id != 0 { + if type_id != VREF_GC_TYPE_ID_UNSET { let gcref = majit_gc::alloc_oldgen_typed(type_id, std::mem::size_of::()); if gcref.0 != 0 { unsafe { std::ptr::write(gcref.0 as *mut JitVirtualRef, vref) }; From 5eebf365390b9a3d00890a90b7bd80b862ff10de Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Mon, 27 Jul 2026 12:52:32 +0900 Subject: [PATCH 08/18] jit, majit: write-barrier the blackhole resume ref stores and the vref/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 --- majit/majit-metainterp/src/virtualref.rs | 4 ++++ pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index ab435e639c3..1eeefebbc9a 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -495,6 +495,10 @@ impl VirtualRefInfo { // `virtualref.py:127 assert vref.virtual_token != TOKEN_TRACING_RESCALL` debug_assert_ne!(vref.virtual_token, token_tracing_rescall()); vref.virtual_token = TOKEN_NONE; + // The vref is an old-gen allocation and `real_object` can be young, + // so the `forced` store needs the generational barrier its creation + // counterpart in `alloc_virtual_ref` already carries. + majit_gc::gc_write_barrier(majit_ir::GcRef(vref_ptr as usize)); vref.forced = real_object; } } diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 6816e2b2453..96eca90aa10 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1666,6 +1666,11 @@ fn walker_ec_enter( &[callee_ec, vref], crate::descr::ec_topframeref_descr(), ); + // The recording-time shadow of the `SetfieldGc` above: `PyFrame.f_backref` + // is a `Type::Ref` field, so the emitted store carries the generational + // barrier and the concrete store has to carry it too. This frame is an + // old-gen `FrameBox` and the caller's vref can be young. + pyre_object::gc_hook::try_gc_write_barrier(concrete_frame as *mut u8); unsafe { (*concrete_frame).f_backref = concrete_caller_topframeref; (*concrete_ec).topframeref = concrete_vref as *mut pyre_interpreter::PyFrame; From 783eea4bbee03d3679e018db5f493849f0f671e5 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Mon, 27 Jul 2026 18:11:31 +0900 Subject: [PATCH 09/18] jit, majit: close every bridge-carrier vref scope; force vrefs through 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 --- majit/majit-metainterp/src/jitdriver.rs | 14 ++++++++ majit/majit-metainterp/src/pyjitpl.rs | 36 +++++++++++++++++-- pyre/pyre-jit-trace/src/trace.rs | 48 ++++++++++++++++++++----- pyre/pyre-jit/src/eval.rs | 2 +- 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/majit/majit-metainterp/src/jitdriver.rs b/majit/majit-metainterp/src/jitdriver.rs index c134aa5147e..96218c0b65a 100644 --- a/majit/majit-metainterp/src/jitdriver.rs +++ b/majit/majit-metainterp/src/jitdriver.rs @@ -4535,6 +4535,20 @@ impl JitDriver { &mut self.meta } + /// `compile.py:966-1000 ResumeGuardForcedDescr.force_now` materializes + /// virtuals through the same resume allocator used by ordinary guard + /// failure. In particular, a `jit.virtual_ref` frame must not be decoded + /// through `NullAllocator`, or its `forced` writeback remains null. + pub fn force_virtualizable_token(&mut self, token: u64) { + let fallback_alloc = crate::resume::NullAllocator; + let allocator: &dyn crate::resume::BlackholeAllocator = self + .blackhole_allocator + .as_deref() + .unwrap_or(&fallback_alloc); + self.meta + .force_virtualizable_token_with_allocator(token, allocator); + } + fn prepare_exit_resume_heap_with_blackhole_allocator( &self, exit_layout: &CompiledExitLayout, diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 411fe219ab8..f1cad0d6b31 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -11564,6 +11564,23 @@ impl MetaInterp { trace_id: u64, fail_index: u32, fail_values: &[i64], + ) -> Option<(Vec, Vec)> { + self.handle_async_forcing_with_allocator( + green_key, + trace_id, + fail_index, + fail_values, + &crate::resume::NullAllocator, + ) + } + + fn handle_async_forcing_with_allocator( + &mut self, + green_key: u64, + trace_id: u64, + fail_index: u32, + fail_values: &[i64], + allocator: &dyn crate::resume::BlackholeAllocator, ) -> Option<(Vec, Vec)> { if crate::majit_log_enabled() { eprintln!( @@ -11626,7 +11643,6 @@ impl MetaInterp { let deadframe_types = self.get_recovery_slot_types(green_key, norm_tid, fail_index); // compile.py:990-991: vinfo = self.jitdriver_sd.virtualizable_info let vinfo = self.virtualizable_info(); - let allocator = crate::resume::NullAllocator; let all_liveness = self.staticdata.liveness_info.as_slice(); let (all_virtuals_ptr, all_virtuals_int) = crate::resume::force_from_resumedata( rd_numb, @@ -11639,7 +11655,7 @@ impl MetaInterp { Some(&self.staticdata.virtualref_info as &dyn crate::resume::VRefInfo), vinfo.map(|v| v.as_ref() as &dyn crate::resume::VirtualizableInfo), None, // ginfo — pyre has no greenfield mechanism - &allocator, + allocator, ); drop(_cc_guard); // compile.py:999-1000: obj = AllVirtuals(all_virtuals) @@ -11657,6 +11673,14 @@ impl MetaInterp { /// Force a running virtualizable identified by its backend force token. pub fn force_virtualizable_token(&mut self, token: u64) { + self.force_virtualizable_token_with_allocator(token, &crate::resume::NullAllocator); + } + + pub fn force_virtualizable_token_with_allocator( + &mut self, + token: u64, + allocator: &dyn crate::resume::BlackholeAllocator, + ) { let deadframe = self .backend .force(GcRef(token as usize)) @@ -11681,7 +11705,13 @@ impl MetaInterp { Type::Void => 0, }) .collect::>(); - let _ = self.handle_async_forcing(green_key, trace_id, fail_index, &fail_values); + let _ = self.handle_async_forcing_with_allocator( + green_key, + trace_id, + fail_index, + &fail_values, + allocator, + ); } pub fn is_force_token_armed(&self, token: u64) -> bool { diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index 0093e5a401e..fa89ded57bf 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -1531,6 +1531,30 @@ fn drive_bridge_carrier_walk( root_pc: usize, cf_addr: usize, carrier: &majit_metainterp::BridgeInlineCarrier, +) -> TraceAction { + let action = drive_bridge_carrier_walk_inner(ctx, sym, w_code, root_pc, cf_addr, carrier); + // `pyframe.py:316-358 execute_frame` closes every resumed frame from its + // `finally: executioncontext.leave(...)`, including when tracing the + // continuation declines. Successful carrier drives close frames as each + // return is threaded below; this finally-shaped epilogue closes any + // restored scopes left by setup failures, walk errors, or abort paths. + while ctx.virtualref_boxes_len() >= 2 { + let before = ctx.virtualref_boxes_len(); + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); + if ctx.virtualref_boxes_len() == before { + break; + } + } + action +} + +fn drive_bridge_carrier_walk_inner( + ctx: &mut TraceCtx, + sym: &mut Sym, + w_code: *const (), + root_pc: usize, + cf_addr: usize, + carrier: &majit_metainterp::BridgeInlineCarrier, ) -> TraceAction { let session = std::cell::RefCell::new(crate::jitcode_dispatch::WalkSession::default()); crate::jitcode_dispatch::bool_box_truth_reset(); @@ -1623,6 +1647,14 @@ fn drive_bridge_carrier_walk( &[] }, ); + let deepest_got_exception = matches!( + &walk, + Some(Ok(( + crate::jitcode_dispatch::DispatchOutcome::SubRaise { .. }, + _ + ))) + ); + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, deepest_got_exception); // 2b-ii: on a clean single-recipe `SubReturn`, thread the callee result // into the root's operand-stack result slot and walk the ROOT top-level to // compile the bridge (the recorded callee continuation + the root @@ -1646,9 +1678,6 @@ fn drive_bridge_carrier_walk( let want_compile = n >= 1 && n <= crate::jitcode_dispatch::fbw_max_multiframe_depth(); let mut middles_ok = true; if want_compile { - // The deepest carrier frame has returned; close the `virtual_ref` - // scope its `enter` opened in the parent trace. - crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); for i in (0..n.saturating_sub(1)).rev() { // recipes[i]'s paused parents are the shallower frames // recipes[..i] (the root sits above them all). @@ -1668,7 +1697,6 @@ fn drive_bridge_carrier_walk( break; } } - crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); } } if want_compile && middles_ok { @@ -1711,10 +1739,6 @@ fn drive_bridge_carrier_walk( if let Some((exc, exc_concrete)) = subwalk_raise { if carrier.recipes.len() == 1 { if let Some(catch_target) = carrier_root_catch_target(sym, root_pc) { - // The carrier frame is unwinding, so this is `leave`'s - // `got_exception=True` arm: the caller is marked escaped and - // the leaving frame's own vref is forced. - crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, true); crate::jitcode_dispatch::set_carrier_raise_seed( crate::jitcode_dispatch::CarrierRaiseSeed { exc, @@ -1841,6 +1865,14 @@ fn drive_middle_frame_and_thread( paused_parents, child_result, ); + let got_exception = matches!( + &middle_walk, + Some(Ok(( + crate::jitcode_dispatch::DispatchOutcome::SubRaise { .. }, + _ + ))) + ); + crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, got_exception); match middle_walk { Some(Ok(( crate::jitcode_dispatch::DispatchOutcome::SubReturn { diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 645a208e9d2..4b710a46170 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3825,7 +3825,7 @@ unsafe extern "C" fn force_pyframe_vref( // the vref names, then run the guard's async forcing, which is // what writes `virtual_token = TOKEN_NONE` and `forced` back. let token = (*v).virtual_token as usize as u64; - driver.meta_interp_mut().force_virtualizable_token(token); + driver.force_virtualizable_token(token); }) }; // `virtualref.py:174-176` — `token == TOKEN_NONE` with no `forced` means From 646c78c7da4bf0ea9011d63a62c4e2811940906f Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Mon, 27 Jul 2026 23:07:48 +0900 Subject: [PATCH 10/18] =?UTF-8?q?jit,=20majit:=20PR=20#796=20review=20roun?= =?UTF-8?q?d=20=E2=80=94=20vref=20finish,=20carrier=20drain=20scope,=20unw?= =?UTF-8?q?ind-safe=20topframeref?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `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 --- majit/majit-metainterp/src/trace_ctx.rs | 13 ++++++ majit/majit-metainterp/src/virtualref.rs | 9 +++- .../src/jitcode_dispatch/bridge_subwalk.rs | 8 ++++ .../src/jitcode_dispatch/inline_call.rs | 5 ++- pyre/pyre-jit-trace/src/state.rs | 16 +++++-- pyre/pyre-jit-trace/src/trace.rs | 43 +++++++++--------- pyre/pyre-jit/src/eval.rs | 44 +++++++++++++++---- 7 files changed, 103 insertions(+), 35 deletions(-) diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index f5e6ae83155..492ea9b3305 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -1052,6 +1052,13 @@ impl TraceCtx { self.virtualref_boxes.len() } + /// Snapshot the open virtual-ref scopes before a non-committal sub-walk. + /// If that walk is cut from the recorder, its stack mutations must be + /// rolled back with the operations. + pub fn snapshot_virtualref_boxes(&self) -> Vec<(OpRef, usize)> { + self.virtualref_boxes.clone() + } + /// The innermost still-open scope's `virtualbox` — `virtualref_boxes[-2]`, /// the operand `opimpl_virtual_ref_finish` pops next. A bridge resumes /// into scopes its parent guard opened, so the frame box that closes one is @@ -1061,6 +1068,12 @@ impl TraceCtx { (len >= 2).then(|| self.virtualref_boxes[len - 2]) } + /// The innermost still-open scope's `vrefbox` — + /// `virtualref_boxes[-1]`. + pub fn innermost_virtualref_vref(&self) -> Option<(OpRef, usize)> { + self.virtualref_boxes.last().copied() + } + /// `pyjitpl.py:3433 rebuild_state_after_failure`'s /// `self.virtualref_boxes = virtualref_boxes`. A bridge resumes into its /// parent's still-open `virtual_ref` scopes, so the pairs the parent guard diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index 1eeefebbc9a..425dd38bbbc 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -497,8 +497,13 @@ impl VirtualRefInfo { vref.virtual_token = TOKEN_NONE; // The vref is an old-gen allocation and `real_object` can be young, // so the `forced` store needs the generational barrier its creation - // counterpart in `alloc_virtual_ref` already carries. - majit_gc::gc_write_barrier(majit_ir::GcRef(vref_ptr as usize)); + // counterpart in `alloc_virtual_ref` already carries. That + // counterpart barriers only inside its GC-allocated arm, so skip + // an address the collector does not own (the `Box` arm taken + // before the vref type id is registered). + if majit_gc::gc_owns_object(vref_ptr as usize) { + majit_gc::gc_write_barrier(majit_ir::GcRef(vref_ptr as usize)); + } vref.forced = real_object; } } diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs index c1cab2237f9..5eeeb48bdb1 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs @@ -49,6 +49,14 @@ pub(crate) fn carrier_ec_leave( &[root_sym.frame()], crate::descr::pyframe_execution_context_descr(), ); + // Every `GetfieldGcR` the enter/leave pair records carries its concrete + // value (`history.py:803 *FrontendOp(pos, value)`); without it + // `concrete_of_opref` reports this result symbolic on the residual-call + // and snapshot paths. The value is the same EC the leave below acts on. + ctx.set_opref_concrete( + callee_ec, + majit_ir::Value::Ref(majit_ir::GcRef(concrete_ec as usize)), + ); super::inline_call::walker_ec_leave( ctx, callee_frame, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 96eca90aa10..2c55d26f5fc 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -1783,7 +1783,10 @@ pub(crate) fn walker_ec_leave( // reachable; converge on the upstream spelling when the force lowering // lands. let live = ctx.virtualref_boxes_len(); - if live >= 2 { + let vref_is_live = ctx + .innermost_virtualref_vref() + .is_some_and(|(vrefbox, _)| vrefbox.as_const_ptr().is_none_or(|vref| vref.0 != 0)); + if live >= 2 && vref_is_live { ctx.stop_tracking_virtualref(live - 2); } } diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 48bf27c1fc8..4a279d499e2 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -9403,11 +9403,19 @@ impl JitState for PyreJitState { }; if !live_ec.is_null() && restored_top != 0 { let published = unsafe { (*live_ec).topframeref }; - if published as usize != restored_top - && unsafe { + // A scope the guard still had open must be republished whatever the + // slot currently holds: `execute_assembler` restores `topframeref` + // to the frame it saved before the run, so after an inlined-callee + // guard the slot names the portal frame — not a vref — and leaving + // it there would expose the portal to a residual `sys._getframe()` + // and close the wrong concrete chain on leave. With no scope open + // `restored_top` is the resumed frame itself, and only a stale vref + // in the slot is worth overwriting. + let stale_scope = !restored_virtualref_boxes.is_empty() + || unsafe { majit_metainterp::virtualref::ptr_is_virtual_ref(published as *const u8) - } - { + }; + if published as usize != restored_top && stale_scope { unsafe { (*live_ec).topframeref = restored_top as *mut pyre_interpreter::PyFrame; } diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index fa89ded57bf..130eacf1560 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -1524,31 +1524,31 @@ fn carrier_root_catch_target(sym: &Sym, root_pc: usize) -> Option< candidate.filter(|&t| crate::jitcode_dispatch::exc_handler_rejoins_loop(code, t)) } -fn drive_bridge_carrier_walk( +fn discard_bridge_carrier_walk( ctx: &mut TraceCtx, sym: &mut Sym, - w_code: *const (), - root_pc: usize, - cf_addr: usize, - carrier: &majit_metainterp::BridgeInlineCarrier, -) -> TraceAction { - let action = drive_bridge_carrier_walk_inner(ctx, sym, w_code, root_pc, cf_addr, carrier); - // `pyframe.py:316-358 execute_frame` closes every resumed frame from its - // `finally: executioncontext.leave(...)`, including when tracing the - // continuation declines. Successful carrier drives close frames as each - // return is threaded below; this finally-shaped epilogue closes any - // restored scopes left by setup failures, walk errors, or abort paths. - while ctx.virtualref_boxes_len() >= 2 { + entry_depth: usize, + pre_pos: majit_metainterp::recorder::TracePosition, + pre_virtualref_boxes: &[(majit_ir::OpRef, usize)], +) { + // `pyframe.py:316-358 execute_frame` closes exactly the frame entered by + // that invocation in its `finally: executioncontext.leave(...)`. Close + // only scopes this carrier walk opened, while their recorder positions + // are still live. If the walk already closed a parent scope, preserve + // only the snapshot prefix that still survives instead of reopening it. + let restore_depth = ctx.virtualref_boxes_len().min(entry_depth); + while ctx.virtualref_boxes_len() > entry_depth { let before = ctx.virtualref_boxes_len(); crate::jitcode_dispatch::carrier_ec_leave(ctx, sym, false); if ctx.virtualref_boxes_len() == before { break; } } - action + ctx.cut_trace(pre_pos); + ctx.restore_virtualref_boxes(pre_virtualref_boxes[..restore_depth].to_vec()); } -fn drive_bridge_carrier_walk_inner( +fn drive_bridge_carrier_walk( ctx: &mut TraceCtx, sym: &mut Sym, w_code: *const (), @@ -1556,6 +1556,9 @@ fn drive_bridge_carrier_walk_inner( cf_addr: usize, carrier: &majit_metainterp::BridgeInlineCarrier, ) -> TraceAction { + let entry_depth = ctx.virtualref_boxes_len(); + let pre_virtualref_boxes = ctx.snapshot_virtualref_boxes(); + let pre_pos = ctx.get_trace_position(); let session = std::cell::RefCell::new(crate::jitcode_dispatch::WalkSession::default()); crate::jitcode_dispatch::bool_box_truth_reset(); crate::jitcode_dispatch::fbw_finish_payload_reset(); @@ -1582,10 +1585,10 @@ fn drive_bridge_carrier_walk_inner( // p2_local_result_bridge.py (loops_aborted 6 -> 505), so keep only // this measured P2 class permanently declined. fbw_bridge_decline(ctx); + discard_bridge_carrier_walk(ctx, sym, entry_depth, pre_pos, &pre_virtualref_boxes); return p2_drain_abort(); }; - let pre_pos = ctx.get_trace_position(); // `setup_reconstructed_callee_frame` emits the callee frame vable into the // trace and returns `argboxes_r` seeding the portal reds + in-flight // operand-stack temps; the `_pending` callee sym is unused on the sub-walk @@ -1594,12 +1597,12 @@ fn drive_bridge_carrier_walk_inner( let Some((_pending, argboxes_r)) = crate::state::setup_reconstructed_callee_frame(ctx, recipe, root_ec, Vec::new()) else { - ctx.cut_trace(pre_pos); + discard_bridge_carrier_walk(ctx, sym, entry_depth, pre_pos, &pre_virtualref_boxes); crate::jitcode_dispatch::census_record("P2Drain::SetupFailed"); return p2_drain_abort(); }; let Some(callee_pjc) = crate::state::pyjitcode_for_code(recipe.code_ptr) else { - ctx.cut_trace(pre_pos); + discard_bridge_carrier_walk(ctx, sym, entry_depth, pre_pos, &pre_virtualref_boxes); crate::jitcode_dispatch::census_record("P2Drain::NoCalleePjc"); return p2_drain_abort(); }; @@ -1609,7 +1612,7 @@ fn drive_bridge_carrier_walk_inner( recipe.jitcode_pc, ); let Some(entry) = entry else { - ctx.cut_trace(pre_pos); + discard_bridge_carrier_walk(ctx, sym, entry_depth, pre_pos, &pre_virtualref_boxes); crate::jitcode_dispatch::census_record("P2Drain::NoCalleeEntry"); return p2_drain_abort(); }; @@ -1794,7 +1797,7 @@ fn drive_bridge_carrier_walk_inner( } } - ctx.cut_trace(pre_pos); + discard_bridge_carrier_walk(ctx, sym, entry_depth, pre_pos, &pre_virtualref_boxes); crate::jitcode_dispatch::bool_box_truth_reset(); crate::jitcode_dispatch::fbw_finish_payload_reset(); // Non-commit epilogue: the sub-walk concrete-executed the reconstructed diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 4b710a46170..731155a9636 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -248,6 +248,41 @@ impl Drop for FrameLocalsRoot { } } +/// Restores `ExecutionContext.topframeref` after compiled execution, including +/// panic unwinds. The saved pointer lives on the shadow stack so a moving +/// collection can forward it in place, matching `CurrentFrameGuard`. +struct TopFrameRefGuard { + ec: *mut PyExecutionContext, + saved_root: Option, +} + +impl TopFrameRefGuard { + fn new(ec: *mut PyExecutionContext) -> Self { + let saved_root = if ec.is_null() { + None + } else { + let saved = unsafe { (*ec).topframeref }; + Some(majit_gc::shadow_stack::push(majit_ir::GcRef( + saved as usize, + ))) + }; + Self { ec, saved_root } + } +} + +impl Drop for TopFrameRefGuard { + fn drop(&mut self) { + let Some(saved_root) = self.saved_root else { + return; + }; + let saved = majit_gc::shadow_stack::get(saved_root); + majit_gc::shadow_stack::pop_to(saved_root); + unsafe { + (*self.ec).topframeref = saved.0 as *mut PyFrame; + } + } +} + /// Bridge pyre-object's `is_managed_heap_object` query to /// `majit_gc::gc_owns_object`. Used by host-side allocators /// (`pyre_object::dealloc_items_block`) to discriminate @@ -7528,13 +7563,9 @@ fn execute_assembler( // `CurrentFrameGuard` bracket a frame: a balanced run restores the same // value it saved, an unbalanced exit restores the caller. 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 } - }; // warmstate.py:395 func_execute_token(loop_token, *args) → deadframe let outcome = { + let _topframeref_guard = TopFrameRefGuard::new(ec_for_topframeref); let _frame_locals_root = FrameLocalsRoot::new(frame_root.frame()); driver.run_compiled_detailed_with_bridge_keyed( green_key, @@ -7544,9 +7575,6 @@ fn execute_assembler( || {}, ) }; - if !ec_for_topframeref.is_null() { - unsafe { (*ec_for_topframeref).topframeref = saved_topframeref }; - } // rstack.stack_check_slowpath → _StackOverflow parity: drain the // JIT-overflow flag the backend probe records when it trips. The From 369c2f89922caf7ad28a83b64d298a3b5caa0441 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Tue, 28 Jul 2026 11:43:47 +0900 Subject: [PATCH 11/18] majit: memclear bh_new_array's payload; barrier the vable array ref store - 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 --- majit/majit-backend-cranelift/src/compiler.rs | 24 +++++++++++++++-- majit/majit-metainterp/src/virtualizable.rs | 27 ++++++++++++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/majit/majit-backend-cranelift/src/compiler.rs b/majit/majit-backend-cranelift/src/compiler.rs index 2a70d3bd954..86bedfa951e 100644 --- a/majit/majit-backend-cranelift/src/compiler.rs +++ b/majit/majit-backend-cranelift/src/compiler.rs @@ -16667,9 +16667,29 @@ impl majit_backend::Backend for CraneliftBackend { type_id != 0, "bh_new_array requires ArrayDescr.tid (descr.py:340) — got 0" ); - active_runtime_alloc_varsize_typed_and_set_len( + let obj = active_runtime_alloc_varsize_typed_and_set_len( type_id, base_size, itemsize, len_offset, length, - ) as i64 + ); + // The nursery is not zero-filled (`incminimark.py:211 + // malloc_zero_filled = False`), so the fresh block still holds the + // recycled bytes of whatever lived there before. `framework.py:1058-1079 + // gct_do_malloc_varsize_clear` compensates by memclearing the fixed and + // the variable part and only then storing the length, which is what a + // GC-traced array needs: the tracer visits every item slot, including + // the ones past `valuestackdepth` that nobody has written yet. + // + // This belongs here rather than in the shared allocation helper: the + // inline allocators in compiled code get their clearing from the + // rewriter's ZERO_ARRAY (`rewrite.py:499`, `:521`) and must stay + // memclear-free on the fast path. + if obj != 0 { + unsafe { + let p = obj as *mut u8; + std::ptr::write_bytes(p, 0, base_size + itemsize * length); + *(p.add(len_offset) as *mut usize) = length; + } + } + obj as i64 } /// llmodel.py:790 bh_new_array_clear = bh_new_array. diff --git a/majit/majit-metainterp/src/virtualizable.rs b/majit/majit-metainterp/src/virtualizable.rs index c32d69a68b4..129f0a12351 100644 --- a/majit/majit-metainterp/src/virtualizable.rs +++ b/majit/majit-metainterp/src/virtualizable.rs @@ -2928,21 +2928,40 @@ pub(crate) unsafe fn vable_write_array_item( // `size_of::()` (4 bytes on wasm32) while an `i64` payload // array is a fixed 8, regardless of word width. let item_size = array.item_size; - let data_ptr = match array.storage { + // `owner_ptr` is the block base the GC would know, i.e. before the + // items offset — the barrier argument. `data_ptr` is items-adjusted + // and is not a valid object address. + let (data_ptr, owner_ptr) = match array.storage { VableArrayStorage::EmbeddedArray { ptr_offset } => { let container = *(vable_ptr.add(array.field_offset) as *const *mut u8); - *(container.add(ptr_offset) as *const *mut u8) + let data = *(container.add(ptr_offset) as *const *mut u8); + (data, data) } VableArrayStorage::DirectPointer => { let arr_ptr = *(vable_ptr.add(array.field_offset) as *const *mut u8); - arr_ptr.add(array.items_offset) + (arr_ptr.add(array.items_offset), arr_ptr) + } + VableArrayStorage::RustVec { data_ptr_fn, .. } => { + (data_ptr_fn(vable_ptr) as *mut u8, std::ptr::null_mut()) } - VableArrayStorage::RustVec { data_ptr_fn, .. } => data_ptr_fn(vable_ptr) as *mut u8, }; if !data_ptr.is_null() { let dest = data_ptr.add(index * item_size); if array.item_type == Type::Ref { std::ptr::write(dest as *mut usize, value as usize); + // `llmodel.py:495-497 write_ref_at_mem` — "the write barrier is + // implied above" — is what every blackhole ref store funnels + // through upstream. The stored ref can be nursery-young while + // the array and its owning frame are old-gen, so arm whichever + // side the collector owns, exactly as the sibling + // `VirtualizableInfo::write_array_item` already does. + if value != 0 { + if majit_gc::gc_owns_object(owner_ptr as usize) { + majit_gc::gc_write_barrier(majit_ir::GcRef(owner_ptr as usize)); + } else if majit_gc::gc_owns_object(vable_ptr as usize) { + majit_gc::gc_write_barrier(majit_ir::GcRef(vable_ptr as usize)); + } + } } else { std::ptr::write(dest as *mut i64, value); } From baffb18ceeed55d88519ab758ee3f5a0ec694499 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 01:16:09 +0900 Subject: [PATCH 12/18] jit: rustfmt the hoisted EC descr group field list Assisted-by: Claude --- pyre/pyre-jit-trace/src/descr.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 5e4601af8e6..e8e00def57d 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -2686,7 +2686,11 @@ static EC_DESCR_GROUP: LazyLock = LazyLock::n 0, 0, &[ - field(0, "sys_exc_value", pyre_interpreter::EC_SYS_EXC_VALUE_OFFSET), + field( + 0, + "sys_exc_value", + pyre_interpreter::EC_SYS_EXC_VALUE_OFFSET, + ), field(1, "topframeref", pyre_interpreter::EC_TOPFRAMEREF_OFFSET), ], ) From bb664726b8e1feac22f4ea7a1a9a008deebbe9bb Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 05:18:54 +0900 Subject: [PATCH 13/18] jit, majit: restore the inlined-callee topframeref publish; make the 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 --- majit/majit-gc/src/rewrite.rs | 47 ++++++++++++---- .../majit-metainterp/src/optimizeopt/heap.rs | 25 +++++---- .../src/optimizeopt/virtualize.rs | 29 +++++----- majit/majit-metainterp/src/pyjitpl.rs | 7 +++ majit/majit-metainterp/src/virtualref.rs | 21 ++++---- .../src/jitcode_dispatch/inline_call.rs | 53 ++++++++++++------- .../src/jitcode_dispatch/residual_call.rs | 41 ++++++++------ pyre/pyre-jit/src/eval.rs | 22 +++++--- 8 files changed, 158 insertions(+), 87 deletions(-) diff --git a/majit/majit-gc/src/rewrite.rs b/majit/majit-gc/src/rewrite.rs index 13ac100cd02..6099c9b4d24 100644 --- a/majit/majit-gc/src/rewrite.rs +++ b/majit/majit-gc/src/rewrite.rs @@ -2954,17 +2954,44 @@ impl GcRewriter for GcRewriterImpl { let ops = Self::remove_bridge_exception(ops); // Result positions are consumed only by result-producing ops; a - // Void-result op never occupies a position slot. Skip Void ops here - // for the same reason `emit` assigns no position id when - // `rt == Type::Void` — otherwise a Void op carrying the `VoidOp( - // u32::MAX)` sentinel (op_typed(NONE.raw(), Void)) would saturate - // the max and overflow the first `next_pos += 1` in `emit`. - let next_pos = ops + // Void-result op never occupies a position slot. Skip sentinels and + // constants below for the same reason `emit` assigns no position id + // when `rt == Type::Void`. + // + // 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. + let max_result_pos = ops .iter() - .filter(|op| op.result_type() != Type::Void) - .filter_map(|op| (!op.pos.get().is_none()).then_some(op.pos.get().raw())) - .max() - .map_or(0, |max_pos| max_pos.saturating_add(1)); + .filter_map(|op| { + let pos = op.pos.get(); + (!pos.is_none() && !pos.is_constant()).then_some(pos.raw()) + }) + .max(); + 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_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 next_pos = max_raw_pos.map_or(0, |max_pos| max_pos.saturating_add(1)); let mut st = RewriteState::with_constants(ops.len(), next_pos, constants.clone()); for (i, orig_op) in ops.iter().enumerate() { // rewrite.py:366-367 — if `remove_tested_failarg` rewrote this diff --git a/majit/majit-metainterp/src/optimizeopt/heap.rs b/majit/majit-metainterp/src/optimizeopt/heap.rs index 9752019f209..e9fc6c2edb5 100644 --- a/majit/majit-metainterp/src/optimizeopt/heap.rs +++ b/majit/majit-metainterp/src/optimizeopt/heap.rs @@ -1157,19 +1157,22 @@ impl OptHeap { if ctx.is_virtualizable(&target) { return true; } - // Indirect: SETARRAYITEM_GC on the frame's array-pointer field. The - // array operand is produced by reading that field off the frame. + // `virtualizable.py:81-84` permits indirection only through an array + // field: SETARRAYITEM_GC on an array read from the standard frame. + // A SETFIELD_GC target merely read from the frame is a different + // object and must never inherit virtualizable ownership. match ctx.get_producing_op(&target) { Some(producer) - if matches!( - producer.opcode, - OpCode::GetfieldGcI - | OpCode::GetfieldGcR - | OpCode::GetfieldGcF - | OpCode::GetfieldRawI - | OpCode::GetfieldRawR - | OpCode::GetfieldRawF - ) => + if op.opcode == OpCode::SetarrayitemGc + && matches!( + producer.opcode, + OpCode::GetfieldGcI + | OpCode::GetfieldGcR + | OpCode::GetfieldGcF + | OpCode::GetfieldRawI + | OpCode::GetfieldRawR + | OpCode::GetfieldRawF + ) => { ctx.resolve_operand_operand_opt(&producer.arg(0)) .map_or(false, |frame| ctx.is_virtualizable(&frame)) diff --git a/majit/majit-metainterp/src/optimizeopt/virtualize.rs b/majit/majit-metainterp/src/optimizeopt/virtualize.rs index e046cd1ed9b..d62427a032c 100644 --- a/majit/majit-metainterp/src/optimizeopt/virtualize.rs +++ b/majit/majit-metainterp/src/optimizeopt/virtualize.rs @@ -2370,7 +2370,9 @@ impl FieldDescr for VRefFieldDescr { } fn field_size(&self) -> usize { - 8 + // `virtualref.py:17-20` declares both fields as pointer types, so + // source translation gives them the target pointer width. + std::mem::size_of::<*mut u8>() } fn field_type(&self) -> majit_ir::Type { @@ -2445,18 +2447,17 @@ fn build_vref_field_descr(index: u32) -> Arc { // and `optimize_jit_force_virtual`'s constant-null read agree // on the value tag. // - // TODO (GC trace divergence). The optimizer - // descriptor is `Type::Ref` for parity with the rtyper's - // setfield_gc_r emit, but the actual GC tracer at - // `pyre/pyre-jit/src/eval.rs:241-247` registers JIT_VIRTUAL_REF - // with `gc_ptr_offsets = [16]` (forced only). See - // `JitVirtualRef` doc-comment in `majit-metainterp/src/virtualref.rs` - // for why `virtual_token` is intentionally outside the GC's - // view: every value it holds at runtime (TOKEN_NONE, - // `token_tracing_rescall()` static address, libc::calloc'd - // JITFRAME address) lives outside any GC heap. - VREF_VIRTUAL_TOKEN_FIELD_INDEX => (8, Type::Ref), - VREF_FORCED_FIELD_INDEX => (16, Type::Ref), + // `virtualref.py:17-20` makes these fields part of the translated + // JitVirtualRef structure; use that structure's target layout rather + // than assuming native 64-bit pointer offsets. + VREF_VIRTUAL_TOKEN_FIELD_INDEX => ( + std::mem::offset_of!(crate::virtualref::JitVirtualRef, virtual_token), + Type::Ref, + ), + VREF_FORCED_FIELD_INDEX => ( + std::mem::offset_of!(crate::virtualref::JitVirtualRef, forced), + Type::Ref, + ), _ => panic!("invalid JitVirtualRef field slot {index}"), }; Arc::new(VRefFieldDescr { @@ -2466,7 +2467,7 @@ fn build_vref_field_descr(index: u32) -> Arc { }) } -/// Size descriptor for JitVirtualRef (24 bytes = super_.typeptr + virtual_token + forced). +/// `virtualref.py:17-20` target-layout size descriptor for JitVirtualRef. #[derive(Debug)] struct VRefSizeDescr; diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index f1cad0d6b31..304030199bf 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -3027,6 +3027,13 @@ impl MetaInterp { } } + /// `pyjitpl.py:3326-3334` keeps exactly one standard virtualizable + /// identity in `virtualizable_boxes[-1]`; `vable_ptr` is its concrete + /// heap counterpart. + pub fn standard_virtualizable_heap_ptr(&self) -> *const u8 { + self.vable_ptr + } + /// Cache fallback virtualizable array lengths for trace-entry box setup. pub(crate) fn set_vable_array_lengths(&mut self, lengths: Vec) { self.vable_array_lengths = lengths; diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index 425dd38bbbc..f0aa0df16d1 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -39,16 +39,14 @@ pub(crate) fn vref_gc_type_id() -> u32 { /// `rpython/rtyper/rclass.py:OBJECT` — RPython's GC /// object header. Every `rclass.OBJECT` subclass starts with a /// `typeptr` field (a vtable pointer) used for runtime type identity -/// checks (`inst.typeptr == some_vtable`). Pyre's analogue: a u64 -/// `typeptr` slot at offset 0 carrying the type-id constant for -/// each registered GC type. +/// checks (`inst.typeptr == some_vtable`). Pyre's analogue is the +/// pointer-sized identity word at offset 0. #[repr(C)] pub struct ObjectHeader { /// `rclass.OBJECT.typeptr` — runtime type identity. RPython /// stores a pointer to the per-class `OBJECT_VTABLE` instance; - /// pyre stores the type-id constant directly (e.g. - /// `JIT_VIRTUAL_REF_VTABLE` for `JitVirtualRef`). - pub typeptr: u64, + /// pyre stores a pointer-sized identity constant directly. + pub typeptr: usize, } /// `rpython/rlib/jit.py JitVirtualRef`: heap-allocated virtual @@ -105,7 +103,9 @@ pub struct JitVirtualRef { /// real OBJECT_VTABLE pointer; the comparison `header.typeptr == /// JIT_VIRTUAL_REF_VTABLE` is the structural equivalent of /// upstream's `inst.typeptr == self.jit_virtual_ref_vtable`. -pub const JIT_VIRTUAL_REF_VTABLE: u64 = 0x4A49_5456_5245_4621; // "JITVREF!" +// `virtualref.py:21-23` allocates an OBJECT_VTABLE pointer, so its translated +// identity occupies one target word rather than an unconditional u64. +pub const JIT_VIRTUAL_REF_VTABLE: usize = 0x4A56_5221; // "JVR!" /// Free-function form of [`VirtualRefInfo::is_virtual_ref`] for callers that /// don't hold a `VirtualRefInfo` — e.g. the interpreter's frame-chain force @@ -203,10 +203,9 @@ pub const TOKEN_NONE: *mut u8 = std::ptr::null_mut(); /// ```python /// _DUMMY = lltype.GcStruct('JITFRAME_DUMMY') /// ``` -/// Pyre stores the corresponding type-id as a u64 magic constant — -/// the typeptr written into the `super_.typeptr` slot of the -/// allocated `_dummy` instance below. -pub const JITFRAME_DUMMY_VTABLE: u64 = 0x4A46_4D44_554D_4D59; // "JFMDUMMY" +/// `virtualizable.py:326-330` uses a real pointer to the `_dummy` object, so +/// the translated identity word is pointer-sized too. +pub const JITFRAME_DUMMY_VTABLE: usize = 0x4A46_444D; // "JFDM" /// Lazy initialisation of the `_dummy` address. `OnceLock` /// (instead of `OnceLock<*mut u8>`) so the cell is `Sync` — diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index 2c55d26f5fc..1c885fed468 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -852,15 +852,6 @@ pub(crate) fn try_walker_call_assembler_self_recursive( // SETFIELD_GC(vable_token) before the assembler call. maybe_walker_vable_and_vrefs_before_residual_call(ctx); - let ca_result = ctx.trace_ctx.call_assembler_red_only_ref_arc( - token, - &[callee_frame, ec], - &[Type::Ref, Type::Ref], - ); - // pyjitpl.py: KEEPALIVE on the callee virtualizable so it - // survives until the result is consumed. - ctx.trace_ctx.record_op(OpCode::Keepalive, &[callee_frame]); - // pyjitpl.py `execute_and_record_varargs(CALL_MAY_FORCE_R)`: // the forces branch EXECUTES the call during tracing — // `direct_assembler_call` (pyjitpl.py) only rewrites the @@ -886,11 +877,26 @@ pub(crate) fn try_walker_call_assembler_self_recursive( OpCode::CallMayForceR, &allboxes, call_descr, - ca_result, + OpRef::NONE, op.pc, None, )? }; + // `pyjitpl.py:2049-2079` checks vrefs after concrete execution, records + // CALL_ASSEMBLER, then emits GUARD_NOT_FORCED. In particular, + // VIRTUAL_REF_FINISH must precede the call so the call and guard remain + // adjacent and the backend can arm the JIT frame's force descriptor. + let ca_result = ctx.trace_ctx.call_assembler_red_only_ref_arc( + token, + &[callee_frame, ec], + &[Type::Ref, Type::Ref], + ); + if let ResidualExecOutcome::Executed(Ok(result)) = exec { + ctx.trace_ctx.set_opref_concrete( + ca_result, + majit_ir::Value::Ref(majit_ir::GcRef(result as usize)), + ); + } // A decline leaves the CALL_ASSEMBLER recorded symbolically WITHOUT // running it — a side effect only the legacy replay applies, so the // walk-end no-replay commit must stay off for this trace (see @@ -1006,6 +1012,9 @@ pub(crate) fn try_walker_call_assembler_self_recursive( // the call (`capture_resumedata(after_residual_call=True)`). ctx.trace_ctx.record_guard(OpCode::GuardNotForced, &[], 0); walker_capture_snapshot_for_last_guard(ctx, op.pc)?; + // `pyjitpl.py:2080-2081` keeps the assembler virtualizable alive after + // the force guard has captured its resume data. + ctx.trace_ctx.record_op(OpCode::Keepalive, &[callee_frame]); // pyjitpl.py `handle_possible_exception`. if exec_raised { // Raising branch (pyjitpl.py): `GUARD_EXCEPTION` with @@ -1098,13 +1107,6 @@ pub(crate) fn emit_walker_loop_callee_call_assembler( // is why forcing the still-virtual frame here — allocation plus one // SETARRAYITEM_GC per known element — is the upstream op sequence rather // than a decline. - let ca_result = ctx.trace_ctx.call_assembler_red_only_ref_arc( - token, - &[callee_frame, callee_ec], - &[Type::Ref, Type::Ref], - ); - ctx.trace_ctx.record_op(OpCode::Keepalive, &[callee_frame]); - // Run the call concretely to stamp `ca_result` (same rationale as the // self-recursive arm: the downstream consumer needs the real concrete to // take its int specialization). ⚠️ The inlined prologue already ran the @@ -1133,10 +1135,23 @@ pub(crate) fn emit_walker_loop_callee_call_assembler( OpCode::CallMayForceR, &allboxes, call_descr, - ca_result, + OpRef::NONE, op.pc, None, )?; + // `pyjitpl.py:2049-2079` records a forced VIRTUAL_REF_FINISH before the + // selected CALL_ASSEMBLER, followed immediately by GUARD_NOT_FORCED. + let ca_result = ctx.trace_ctx.call_assembler_red_only_ref_arc( + token, + &[callee_frame, callee_ec], + &[Type::Ref, Type::Ref], + ); + if let ResidualExecOutcome::Executed(Ok(result)) = exec { + ctx.trace_ctx.set_opref_concrete( + ca_result, + majit_ir::Value::Ref(majit_ir::GcRef(result as usize)), + ); + } let exec_raised = match exec { ResidualExecOutcome::Executed(result) => result.is_err(), ResidualExecOutcome::Declined(cause) => { @@ -1152,6 +1167,8 @@ pub(crate) fn emit_walker_loop_callee_call_assembler( ctx.trace_ctx.record_guard(OpCode::GuardNotForced, &[], 0); walker_capture_snapshot_for_last_guard(ctx, op.pc)?; + // `pyjitpl.py:2080-2081` places KEEPALIVE after GUARD_NOT_FORCED. + ctx.trace_ctx.record_op(OpCode::Keepalive, &[callee_frame]); if exec_raised { walker_record_guard_exception(ctx, op.pc); let exc = ctx diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index 3e68c60b196..ffdcb63d30e 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -2422,24 +2422,31 @@ pub(crate) fn try_execute_residual_call_via_executor( // the recorded OpRef with the executed concrete so downstream // `concrete_of_opref` / `box_value` consumers see the folded // value. An executed void helper has nothing to stamp. - match call_descr.result_type() { - majit_ir::Type::Int => { - ctx.trace_ctx - .set_opref_concrete(recorded, majit_ir::Value::Int(result_i64)); - } - majit_ir::Type::Ref => { - ctx.trace_ctx.set_opref_concrete( - recorded, - majit_ir::Value::Ref(majit_ir::GcRef(result_i64 as usize)), - ); - } - majit_ir::Type::Float => { - ctx.trace_ctx.set_opref_concrete( - recorded, - majit_ir::Value::Float(f64::from_bits(result_i64 as u64)), - ); + // `pyjitpl.py:2049-2068` checks forced virtual refs before + // recording the selected CALL operation. The assembler-call + // walker uses `OpRef::NONE` while concrete-executing so + // `vrefs_after_residual_call` can emit VIRTUAL_REF_FINISH before + // CALL_ASSEMBLER, then stamps the newly recorded call itself. + if !recorded.is_none() { + match call_descr.result_type() { + majit_ir::Type::Int => { + ctx.trace_ctx + .set_opref_concrete(recorded, majit_ir::Value::Int(result_i64)); + } + majit_ir::Type::Ref => { + ctx.trace_ctx.set_opref_concrete( + recorded, + majit_ir::Value::Ref(majit_ir::GcRef(result_i64 as usize)), + ); + } + majit_ir::Type::Float => { + ctx.trace_ctx.set_opref_concrete( + recorded, + majit_ir::Value::Float(f64::from_bits(result_i64 as u64)), + ); + } + majit_ir::Type::Void => {} } - majit_ir::Type::Void => {} } // #57 Option C (capture): this residual is the FOR_ITER advance // (`for_iter_next`) — it just advanced the real shared heap diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 731155a9636..d76d1532da1 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3903,12 +3903,22 @@ unsafe extern "C" fn force_pyframe(frame: *mut pyre_interpreter::PyFrame) { majit_metainterp::virtualizable::VableToken::TracingRescall ) }); - let live_frame_armed = match info.read_token(frame.cast()) { - majit_metainterp::virtualizable::VableToken::Active(token) => { - driver.meta_interp_mut().is_force_token_armed(token) - } - _ => false, - }; + // `pyjitpl.py:3326-3334` reads the token only from + // `virtualizable_boxes[-1]`. An inlined callee materialized through + // a virtual reference is an ordinary frame, not that one standard + // virtualizable, and its token slot is not part of its resume image. + let standard_frame = driver + .meta_interp() + .standard_virtualizable_heap_ptr() + .cast_mut() + .cast::(); + let live_frame_armed = std::ptr::eq(frame, standard_frame) + && match info.read_token(frame.cast()) { + majit_metainterp::virtualizable::VableToken::Active(token) => { + driver.meta_interp_mut().is_force_token_armed(token) + } + _ => false, + }; let mut force = |ptr: *mut u8| { info.force_virtualizable_if_necessary(ptr, |token| { driver.meta_interp_mut().force_virtualizable_token(token); From 5b9a3b141a20f5913e81889f85936e8f1dd7c01c Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 09:50:41 +0900 Subject: [PATCH 14/18] majit(gc): restrict rewriter position reservation to typed body variants 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 --- majit/majit-gc/src/rewrite.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/majit/majit-gc/src/rewrite.rs b/majit/majit-gc/src/rewrite.rs index 6099c9b4d24..33d2705958e 100644 --- a/majit/majit-gc/src/rewrite.rs +++ b/majit/majit-gc/src/rewrite.rs @@ -2969,13 +2969,24 @@ impl GcRewriter for GcRewriterImpl { .iter() .filter_map(|op| { let pos = op.pos.get(); - (!pos.is_none() && !pos.is_constant()).then_some(pos.raw()) + (!pos.is_constant() + && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref))) + .then(|| pos.raw()) }) .max(); 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_none() && !pos.is_constant() && pos.raw() > result_high_water { + // Only the typed body variants `emit` draws from `next_pos` + // count. `ty()` is None for `None`/`TempVar` and `Void` for the + // `VoidOp(u32::MAX)` sentinel — reserving that sentinel would + // pin `next_pos` at `u32::MAX` and overflow the first `+= 1`, + // which is why the result scan above skips Void as well. The + // constant test comes first: `raw()` panics on an inline Const. + 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()))); } From 77445c0918b120db4827f242eff65fc45045f75c Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 10:28:14 +0900 Subject: [PATCH 15/18] jit, majit: correct the JitVirtualRef doc comments to the current layout 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 --- majit/majit-gc/src/rewrite.rs | 9 ++++--- majit/majit-metainterp/src/virtualref.rs | 31 ++++++++++++------------ pyre/pyre-jit/src/eval.rs | 9 ++++--- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/majit/majit-gc/src/rewrite.rs b/majit/majit-gc/src/rewrite.rs index 33d2705958e..6e9f8f99e99 100644 --- a/majit/majit-gc/src/rewrite.rs +++ b/majit/majit-gc/src/rewrite.rs @@ -2980,9 +2980,12 @@ impl GcRewriter for GcRewriterImpl { // Only the typed body variants `emit` draws from `next_pos` // count. `ty()` is None for `None`/`TempVar` and `Void` for the // `VoidOp(u32::MAX)` sentinel — reserving that sentinel would - // pin `next_pos` at `u32::MAX` and overflow the first `+= 1`, - // which is why the result scan above skips Void as well. The - // constant test comes first: `raw()` panics on an inline Const. + // pin `next_pos` at `u32::MAX`, which the first `+= 1` in + // `emit` overflows: a panic where overflow checks are on, and a + // silent wrap to 0 in release, handing every rewritten op a + // position that aliases a live operand. That is why the result + // scan above skips Void as well. The constant test comes first: + // `raw()` panics on an inline Const. if !pos.is_constant() && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref)) && pos.raw() > result_high_water diff --git a/majit/majit-metainterp/src/virtualref.rs b/majit/majit-metainterp/src/virtualref.rs index f0aa0df16d1..1ad6c686fcb 100644 --- a/majit/majit-metainterp/src/virtualref.rs +++ b/majit/majit-metainterp/src/virtualref.rs @@ -69,17 +69,18 @@ pub struct ObjectHeader { /// `optimizeopt/virtualize.rs`) agree on the slot type. /// /// TODO (GC trace). Upstream traces both fields as -/// real GC pointers; pyre traces only `forced`. `eval.rs:241-247` -/// registers JIT_VIRTUAL_REF with `gc_ptr_offsets = [16]` (forced -/// only). The reason is that every value `virtual_token` ever holds -/// at runtime falls outside the GC heap: +/// real GC pointers; pyre traces only `forced` — the type registration +/// in `eval.rs` derives that one entry from `offset_of!(JitVirtualRef, +/// forced)`, so it follows the target's pointer width. The reason +/// `virtual_token` is left out is that every value it ever holds at +/// runtime falls outside the GC heap: /// - `TOKEN_NONE` — null, safe to walk. /// - `token_tracing_rescall()` — program-lifetime leaked /// `Box` (see `allocate_tracing_rescall_dummy` / /// `TRACING_RESCALL_DUMMY_PTR` below), host-heap allocated and /// never freed; not a GC-allocated `_dummy` GcStruct. /// - an active JITFRAME address — `libc::calloc`'d on a host-side -/// pool (eval.rs:232-240), not nursery/oldgen. +/// pool, not nursery/oldgen. /// Routing it through `trace_and_update_object` would either be a /// no-op or trip a poison-address check. The optimizer-side /// `Type::Ref` is intentionally retained so that @@ -114,8 +115,8 @@ pub const JIT_VIRTUAL_REF_VTABLE: usize = 0x4A56_5221; // "JVR!" /// `virtualref.py:94-98 is_virtual_ref(gcref)`. /// /// # Safety -/// `ptr` must be null or point to a valid object whose first 8 bytes are the -/// `('super', rclass.OBJECT)` typeptr word. +/// `ptr` must be null or point to a valid object whose leading +/// pointer-sized word is the `('super', rclass.OBJECT)` typeptr. #[inline] pub unsafe fn ptr_is_virtual_ref(ptr: *const u8) -> bool { unsafe { @@ -157,8 +158,8 @@ pub use crate::jit::InvalidVirtualRef; /// Returns raw pointer; caller owns the allocation. /// /// `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 +/// one here too: `forced` is the sole traced slot of the type 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 @@ -427,7 +428,7 @@ impl VirtualRefInfo { /// /// # Safety /// `vref_ptr` must be null or point to a valid GCREF object - /// whose first 8 bytes are the type-tag word. + /// whose leading pointer-sized word is the type-tag. pub unsafe fn tracing_before_residual_call(&self, vref_ptr: *mut u8) { unsafe { if !self.is_virtual_ref(vref_ptr) { @@ -448,7 +449,7 @@ impl VirtualRefInfo { /// /// # Safety /// `vref_ptr` must be null or point to a valid GCREF object - /// whose first 8 bytes are the type-tag word. + /// whose leading pointer-sized word is the type-tag. pub unsafe fn tracing_after_residual_call(&self, vref_ptr: *mut u8) -> bool { unsafe { if !self.is_virtual_ref(vref_ptr) { @@ -481,7 +482,7 @@ impl VirtualRefInfo { /// /// # Safety /// `vref_ptr` must be null or point to a valid GCREF object - /// whose first 8 bytes are the type-tag word. + /// whose leading pointer-sized word is the type-tag. pub unsafe fn continue_tracing(&self, vref_ptr: *mut u8, real_object: *mut u8) { unsafe { if !self.is_virtual_ref(vref_ptr) { @@ -537,8 +538,8 @@ impl VirtualRefInfo { /// `JIT_VIRTUAL_REF_VTABLE`. /// /// # Safety - /// `ptr` must point to a valid GCREF object whose first 8 bytes - /// are the typeptr word, or be null. + /// `ptr` must point to a valid GCREF object whose leading + /// pointer-sized word is the typeptr, or be null. pub unsafe fn is_virtual_ref(&self, ptr: *const u8) -> bool { unsafe { ptr_is_virtual_ref(ptr) } } @@ -671,7 +672,7 @@ mod tests { /// `virtualref.py:101-102`: `tracing_before_residual_call` /// returns silently when the gcref is not a JitVirtualRef. /// pyre's `is_virtual_ref` guard mirrors the shape — a - /// non-vref pointer (here a `u64` whose first 8 bytes are NOT + /// non-vref pointer (here a word whose leading bytes are NOT /// `JIT_VIRTUAL_REF_VTABLE`) must be left untouched. #[test] fn tracing_before_residual_call_skips_non_vref() { diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index d76d1532da1..be33ca8bda1 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -1268,19 +1268,20 @@ fn build_gc() -> Box { // collections triggered by CallMallocNursery slow paths. majit_gc::shadow_stack::register_libc_jitframe_tracer(pyre_libc_jitframe_tracer); // virtualref.py — JIT_VIRTUAL_REF as a proper GC type. - // Layout: super_.typeptr(u64, offset 0) | virtual_token(*mut u8, offset 8) | forced(*mut u8, offset 16) + // Layout: three pointer-sized words — super_.typeptr | virtual_token | + // forced — so the size and the traced offset below are both derived from + // the struct rather than spelled out for one word width. // // Note (GC trace divergence). Upstream // `virtualref.py:17-20` declares both `virtual_token` and // `forced` as GC slots (`llmemory.GCREF` / `OBJECTPTR`); pyre - // registers only `forced` (offset 16) in `gc_ptr_offsets`. + // registers only `forced` in `gc_ptr_offsets`. // The `virtual_token` slot is intentionally outside the GC's // view because every runtime value it can hold lives outside // any GC heap: TOKEN_NONE (null), `token_tracing_rescall()` // (program-lifetime leaked `Box` dummy lazily // allocated by `allocate_tracing_rescall_dummy` and cached in - // `TRACING_RESCALL_DUMMY_PTR`, see `majit-metainterp/src/ - // virtualref.rs:140-180`), and active JITFRAME addresses + // `TRACING_RESCALL_DUMMY_PTR`), and active JITFRAME addresses // (libc::calloc'd, see `register_libc_jitframe_tracer` above). // The optimizer-side descriptor at // `majit-metainterp/src/optimizeopt/virtualize.rs:make_vref_field_descr` From 16d00803e5de0a54478571d47aaa45a9edca3830 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 12:39:34 +0900 Subject: [PATCH 16/18] majit(gc): keep every non-Void payload in the rewriter high-water mark 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 --- majit/majit-gc/src/rewrite.rs | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/majit/majit-gc/src/rewrite.rs b/majit/majit-gc/src/rewrite.rs index 6e9f8f99e99..7be4b2b7498 100644 --- a/majit/majit-gc/src/rewrite.rs +++ b/majit/majit-gc/src/rewrite.rs @@ -2965,31 +2965,28 @@ impl GcRewriter for GcRewriterImpl { // 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. + // The `VoidOp(u32::MAX)` sentinel is the one raw payload that must not + // enter the high-water mark: it would pin `next_pos` at `u32::MAX`, + // which the first `+= 1` in `emit` overflows — a panic where overflow + // checks are on, and a silent wrap to 0 in release, handing every + // rewritten op a position that aliases a live operand. Every other + // non-constant payload still counts, `TempVar` included: its sentinel + // range sits above the body positions, and lowering the mark past it + // lets a new box collide with a position that reaches the backend + // through resume data rather than through `pos`/args/failargs. + let counts_toward_high_water = + |pos: OpRef| !pos.is_none() && !pos.is_constant() && pos.ty() != Some(Type::Void); let max_result_pos = ops .iter() .filter_map(|op| { let pos = op.pos.get(); - (!pos.is_constant() - && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref))) - .then(|| pos.raw()) + counts_toward_high_water(pos).then(|| pos.raw()) }) .max(); let mut max_raw_pos = max_result_pos; if let Some(result_high_water) = max_result_pos { let mut reserve_later_box = |pos: OpRef| { - // Only the typed body variants `emit` draws from `next_pos` - // count. `ty()` is None for `None`/`TempVar` and `Void` for the - // `VoidOp(u32::MAX)` sentinel — reserving that sentinel would - // pin `next_pos` at `u32::MAX`, which the first `+= 1` in - // `emit` overflows: a panic where overflow checks are on, and a - // silent wrap to 0 in release, handing every rewritten op a - // position that aliases a live operand. That is why the result - // scan above skips Void as well. The constant test comes first: - // `raw()` panics on an inline Const. - if !pos.is_constant() - && matches!(pos.ty(), Some(Type::Int | Type::Float | Type::Ref)) - && pos.raw() > result_high_water - { + if counts_toward_high_water(pos) && pos.raw() > result_high_water { max_raw_pos = Some(max_raw_pos.map_or(pos.raw(), |old: u32| old.max(pos.raw()))); } From 21d053b5196dcb437494b90266982adc12e777f0 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 19:15:41 +0900 Subject: [PATCH 17/18] majit(dynasm): stop the write-barrier emitters from assembling nothing `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 --- .../src/aarch64/assembler.rs | 85 +++++++++++++------ .../majit-backend-dynasm/src/x86/assembler.rs | 25 ++++-- 2 files changed, 80 insertions(+), 30 deletions(-) diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index 3d33ae563ad..0cca6d2ad72 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -6067,15 +6067,24 @@ impl<'a> AssemblerARM64<'a> { /// aarch64/opassembler.py:912 _write_barrier_fastpath parity. fn emit_write_barrier_fastpath(&mut self, op: &Op, arglocs: &[Loc]) { + // opassembler.py:934 `mc.LDRB_ri(r.ip0.value, loc_base.value, ...)` + // indexes the base as a core register; upstream guarantees that in + // `ARMRegisterManager.return_constant` (aarch64/regalloc.py:70), which + // materializes every Const into a scratch register. The shared + // `RegisterManager::return_constant` follows the llsupport spelling + // (llsupport/regalloc.py:625) and can hand back a bare `Loc::Immed`, + // so state the contract instead of emitting nothing — a barrier that + // assembles to zero bytes stays invisible until it corrupts memory. let loc_base = match arglocs.first() { Some(Loc::Reg(r)) => *r, - _ => return, + other => { + panic!("write barrier base loc must be Loc::Reg (regalloc contract), got {other:?}") + } }; let is_array = op.opcode == majit_ir::OpCode::CondCallGcWbArray; - let loc_index = match arglocs.get(1) { - Some(Loc::Reg(r)) => Some(*r), - _ => None, - }; + // opassembler.py:996 `loc_index = arglocs[1]` — the location kind is + // discriminated at the card-marking block, not here. + let loc_index = arglocs.get(1).copied(); self.emit_write_barrier_fastpath_for_base(loc_base, is_array, loc_index); } @@ -6083,12 +6092,14 @@ impl<'a> AssemblerARM64<'a> { &mut self, loc_base: crate::regloc::RegLoc, is_array: bool, - loc_index: Option, + loc_index: Option, ) { - let wb = match crate::runner::dynasm_write_barrier_descr() { - Some(wb) => wb, - None => return, - }; + // opassembler.py:917-919 asserts the descriptor is the collector's + // write-barrier class. `COND_CALL_GC_WB` only exists because the GC + // rewriter emitted it, so a missing descriptor here means the two + // disagree; returning would drop the barrier without a trace. + let wb = crate::runner::dynasm_write_barrier_descr() + .expect("COND_CALL_GC_WB emitted without a write barrier descriptor"); let card_marking = is_array && wb.jit_wb_cards_set != 0; // opassembler.py:922-929: build mask @@ -6137,20 +6148,46 @@ impl<'a> AssemblerARM64<'a> { // opassembler.py:996-1015: card marking inline dynasm!(self.mc ; .arch aarch64 ; =>card_mark); - if let Some(loc_index) = loc_index { - let shift = 3 + wb.jit_wb_card_page_shift; - dynasm!(self.mc ; .arch aarch64 - ; lsr x16, X(loc_index.value), shift - ; mvn x30, x16 - ; lsr x16, X(loc_index.value), wb.jit_wb_card_page_shift - ; and x17, x16, 7 - ; mov x16, 1 - ; lsl x17, x16, x17 - ; sub x30, x30, majit_gc::header::GcHeader::SIZE as u32 - ; ldrb w16, [X(loc_base.value), x30] - ; orr w16, w16, w17 - ; strb w16, [X(loc_base.value), x30] - ); + match loc_index { + // opassembler.py:997 `assert loc_index.is_core_reg()` + Some(Loc::Reg(loc_index)) => { + let shift = 3 + wb.jit_wb_card_page_shift; + dynasm!(self.mc ; .arch aarch64 + ; lsr x16, X(loc_index.value), shift + ; mvn x30, x16 + ; lsr x16, X(loc_index.value), wb.jit_wb_card_page_shift + ; and x17, x16, 7 + ; mov x16, 1 + ; lsl x17, x16, x17 + ; sub x30, x30, majit_gc::header::GcHeader::SIZE as u32 + ; ldrb w16, [X(loc_base.value), x30] + ; orr w16, w16, w17 + ; strb w16, [X(loc_base.value), x30] + ); + } + // x86/assembler.py:2382-2386 `elif isinstance(loc_index, ImmedLoc)`: + // byte offset and bit mask are both assembly-time constants, so + // the sequence collapses to one load/or/store at a fixed + // displacement. A64 has no or-to-memory form, so the OR8 there + // becomes ldrb/orr/strb here. `byte_ofs` carries the same + // `- GcHeader::SIZE` bias the register form applies with + // `sub x30, x30, ...`: the base addresses the payload while the + // card bytes sit before the header. + Some(Loc::Immed(loc_index)) => { + let byte_index = loc_index.value >> wb.jit_wb_card_page_shift; + let byte_ofs = !(byte_index >> 3) - majit_gc::header::GcHeader::SIZE as i64; + let byte_val = (1_i64 << (byte_index & 7)) as u32; + self.emit_mov_imm64(30, byte_ofs); + dynasm!(self.mc ; .arch aarch64 + ; mov w17, byte_val + ; ldrb w16, [X(loc_base.value), x30] + ; orr w16, w16, w17 + ; strb w16, [X(loc_base.value), x30] + ); + } + // x86/assembler.py:2387-2388 + // `raise AssertionError("index is neither RegLoc nor ImmedLoc")` + _ => panic!("index is neither RegLoc nor ImmedLoc"), } } else { // opassembler.py:968-976: non-array slow path diff --git a/majit/majit-backend-dynasm/src/x86/assembler.rs b/majit/majit-backend-dynasm/src/x86/assembler.rs index bb03de41989..af575494a72 100644 --- a/majit/majit-backend-dynasm/src/x86/assembler.rs +++ b/majit/majit-backend-dynasm/src/x86/assembler.rs @@ -7617,13 +7617,24 @@ impl<'a> Assembler386<'a> { } fn emit_write_barrier_fastpath_kind(&mut self, arglocs: &[Loc], is_array: bool) { - let wb = match crate::runner::dynasm_write_barrier_descr() { - Some(wb) => wb, - None => return, - }; + // x86/assembler.py:2399-2401 asserts the descriptor is the collector's + // write-barrier class. `COND_CALL_GC_WB` only exists because the GC + // rewriter emitted it, so a missing descriptor here means the two + // disagree; returning would drop the barrier without a trace. + let wb = crate::runner::dynasm_write_barrier_descr() + .expect("COND_CALL_GC_WB emitted without a write barrier descriptor"); + // x86/assembler.py:2415-2420 feeds `loc_base = arglocs[0]` into + // `addr_add_const`, and `AddressLoc` (x86/regloc.py:213) accepts an + // immediate base, so upstream needs no assertion here. This backend + // addresses the flag byte only through a core register, and the paired + // lowered `GcStore` already contracts for one, so state the contract + // instead of emitting nothing — a barrier that assembles to zero bytes + // stays invisible until it corrupts memory. let loc_base = match arglocs.first() { Some(Loc::Reg(r)) => *r, - _ => return, + other => { + panic!("write barrier base loc must be Loc::Reg (regalloc contract), got {other:?}") + } }; let card_marking = is_array && wb.jit_wb_cards_set != 0; let mut mask = wb.jit_wb_if_flag_singlebyte as i64; @@ -7719,7 +7730,9 @@ impl<'a> Assembler386<'a> { ; or BYTE [Rq(loc_base.value as u8) + byte_ofs as i32], byte_val as i8 ); } - _ => {} + // x86/assembler.py:2387-2388 + // `raise AssertionError("index is neither RegLoc nor ImmedLoc")` + _ => panic!("index is neither RegLoc nor ImmedLoc"), } } else { // Non-array: generic barrier From f06af59666ec46d2d4c90cb0af6efcf8b512b966 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Wed, 29 Jul 2026 20:04:13 +0900 Subject: [PATCH 18/18] majit: cover the aarch64 immediate card arm and the unmanaged-frame barrier decline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/aarch64/assembler.rs | 167 +++++++++++++++++- majit/majit-gc/src/collector.rs | 74 ++++++++ 2 files changed, 240 insertions(+), 1 deletion(-) diff --git a/majit/majit-backend-dynasm/src/aarch64/assembler.rs b/majit/majit-backend-dynasm/src/aarch64/assembler.rs index 0cca6d2ad72..58f7f692539 100644 --- a/majit/majit-backend-dynasm/src/aarch64/assembler.rs +++ b/majit/majit-backend-dynasm/src/aarch64/assembler.rs @@ -7531,8 +7531,12 @@ mod tests { use std::time::Duration; use majit_backend::{Backend, JitCellToken}; + use majit_ir::forwarding::bound_operand_from_opref; use majit_ir::operand::Operand; - use majit_ir::{Op, OpCode, OpRef, Type, make_array_descr_signed, make_loop_target_descr}; + use majit_ir::{ + GcRef, InputArg, Op, OpCode, OpRef, Type, Value, make_array_descr_signed, + make_loop_target_descr, + }; use crate::runner::DynasmBackend; @@ -7756,4 +7760,165 @@ mod tests { .expect("the compiled-loop worker must resume cleanly"); worker.join().unwrap(); } + + // ── COND_CALL_GC_WB_ARRAY inline card marking ────────────────────── + + /// Array length large enough that indices land in more than one card + /// byte at the default `card_page_indices = 128` (incminimark.py:275): + /// eight cards per byte means index 1024 is the first index in card + /// byte 1. + const CARD_ARRAY_LENGTH: usize = 2048; + + /// incminimark.py:1017-1030 `external_malloc` with card bits: an + /// old-gen varsize array whose items are GC pointers gets GCFLAG_HAS_CARDS + /// and a run of zeroed card bytes in front of the header. + fn alloc_old_card_array(gc: &mut majit_gc::collector::MiniMarkGC, type_id: u32) -> GcRef { + let item_size = std::mem::size_of::(); + let total_size = majit_gc::header::GcHeader::SIZE + 8 + item_size * CARD_ARRAY_LENGTH; + let obj = gc.alloc_in_oldgen_with_cards(type_id, total_size, CARD_ARRAY_LENGTH, true); + // `dirty_cards` reads the length out of the array's own length field. + unsafe { *(obj.0 as *mut usize) = CARD_ARRAY_LENGTH }; + obj + } + + /// Compile and run a one-operation trace holding a single + /// `COND_CALL_GC_WB_ARRAY` against `obj`. + /// + /// `index_in_register` selects which argloc kind the emitter sees: + /// a non-constant `InputArg` is forced into a core register, while a + /// `ConstInt` reaches `RegisterManager::return_constant` + /// (llsupport/regalloc.py:625) with no selected register and comes back + /// as a bare `Loc::Immed`. + fn run_cond_call_gc_wb_array(trace_id: u64, obj: GcRef, index: i64, index_in_register: bool) { + let mut backend = DynasmBackend::new(); + backend.attach_default_test_descrs(); + + let mut inputargs = vec![InputArg::new_ref(0)]; + let mut values = vec![Value::Ref(obj)]; + let index_operand = if index_in_register { + inputargs.push(InputArg::new_int(1)); + values.push(Value::Int(index)); + bound_operand_from_opref(OpRef::input_arg_int(1)) + } else { + bound_operand_from_opref(OpRef::const_int(index)) + }; + + let barrier = Op::new( + OpCode::CondCallGcWbArray, + &[ + bound_operand_from_opref(OpRef::input_arg_ref(0)), + index_operand, + ], + ); + barrier.pos.set(OpRef::void_op(2)); + + let finish = Op::new(OpCode::Finish, &[]); + finish.pos.set(OpRef::void_op(3)); + finish.set_fail_arg_types(vec![]); + finish.setfailargs(vec![].into()); + + let mut token = JitCellToken::new(trace_id); + backend + .compile_loop(&inputargs, &[Rc::new(barrier), Rc::new(finish)], &mut token) + .expect("compile COND_CALL_GC_WB_ARRAY trace"); + let frame = backend.execute_token(&token, &values); + assert!( + backend.get_latest_descr(&frame).is_finish(), + "the barrier trace must run to its FINISH" + ); + } + + /// opassembler.py:996-1015 inline card marking, immediate-index arm. + /// + /// The register arm shifts the index at runtime; the immediate arm folds + /// the same two quantities — card byte displacement and card bit — at + /// assembly time (x86/assembler.py:2382-2386). Both must dirty exactly + /// the card `mark_card` (incminimark.py:1574-1598) would dirty. + /// + /// Regression cover: while the whole card sequence sat under a match that + /// only admitted `Loc::Reg`, an immediate index assembled to zero bytes + /// and the array kept a clean card across a barrier that was supposed to + /// dirty one. + #[test] + fn cond_call_gc_wb_array_immed_index_marks_same_card_as_reg_index() { + // gc.py:273 JIT_WB_CARDS_SET — zero means the backend emits no card + // sequence at all, which would leave this test asserting nothing. + let wb = crate::runner::dynasm_write_barrier_descr() + .expect("a write barrier descriptor must be resolvable"); + assert_ne!( + wb.jit_wb_cards_set, 0, + "card marking must be enabled for this test to exercise the card arms" + ); + let card_page_shift = wb.jit_wb_card_page_shift; + + let mut gc = majit_gc::collector::MiniMarkGC::new(); + let item_size = std::mem::size_of::(); + let type_id = gc.register_type(majit_gc::TypeInfo::varsize( + 8, + item_size, + 0, + true, + Vec::new(), + )); + let obj_immed = alloc_old_card_array(&mut gc, type_id); + let obj_reg = alloc_old_card_array(&mut gc, type_id); + let obj_interp = alloc_old_card_array(&mut gc, type_id); + + // opassembler.py:943-949 branches straight to the inline card block + // when GCFLAG_CARDS_SET is already set, so the compiled arms never + // reach the `jit_remember_young_pointer_from_array` helper. + // `mark_card` sets the same flag on the interpreter's object itself. + for obj in [obj_immed, obj_reg] { + unsafe { + (*majit_gc::header::header_of(obj.0)).set_flag(majit_gc::flags::CARDS_SET); + } + } + for obj in [obj_immed, obj_reg, obj_interp] { + assert!( + gc.dirty_cards(obj).is_empty(), + "a freshly allocated card array starts with every card clean" + ); + } + + // Indices chosen to span two card bytes and several bits within them. + const INDICES: [i64; 5] = [0, 5, 200, 1152, 2047]; + for (n, &index) in INDICES.iter().enumerate() { + let trace_id = 9100 + 2 * n as u64; + run_cond_call_gc_wb_array(trace_id, obj_immed, index, false); + run_cond_call_gc_wb_array(trace_id + 1, obj_reg, index, true); + gc.do_write_barrier_card(obj_interp, index as usize, card_page_shift); + // Compare after every index, not only at the end: an aggregate + // comparison would accept two arms that dirty the same set of + // cards while pairing them with different indices. + assert_eq!( + gc.dirty_cards(obj_immed), + gc.dirty_cards(obj_reg), + "index {index} must dirty the same cards through both arms" + ); + } + + let mut expected: Vec = INDICES + .iter() + .map(|&index| (index as usize) >> card_page_shift) + .collect(); + expected.sort_unstable(); + expected.dedup(); + + let immed_cards = gc.dirty_cards(obj_immed); + let reg_cards = gc.dirty_cards(obj_reg); + let interp_cards = gc.dirty_cards(obj_interp); + + assert_eq!( + immed_cards, reg_cards, + "an immediate index must dirty the same cards as the register arm" + ); + assert_eq!( + immed_cards, interp_cards, + "the compiled card bits must match remember_young_pointer_from_array2" + ); + assert_eq!( + immed_cards, expected, + "each index must dirty exactly its own card, and nothing else" + ); + } } diff --git a/majit/majit-gc/src/collector.rs b/majit/majit-gc/src/collector.rs index 0c26e7e5f0a..2b8f8c78e0b 100644 --- a/majit/majit-gc/src/collector.rs +++ b/majit/majit-gc/src/collector.rs @@ -3243,6 +3243,24 @@ impl MiniMarkGC { /// the barrier must ignore any address the GC does not own rather than read /// its non-header word. This centralizes the `try_gc_owns_object` guard that /// `object_array.rs` / `list_write_barrier` already apply per call site. + /// + /// The guard is equally load-bearing on the JIT path, which is why this + /// entry point — not `jit_remember_young_pointer` — is what the backends' + /// non-array COND_CALL_GC_WB helper calls. `_reload_frame_if_necessary` + /// (aarch64/assembler.py:967-980) re-applies the non-array barrier fast + /// path to the *current jitframe* after every collecting helper call. + /// Most jitframes are nursery-allocated and carry a GcHeader, but not + /// all: the JITFRAME slow path falls back to `libc::calloc` when no + /// allocator is active, and CALL_ASSEMBLER builds some callee frames the + /// same way — the class `shadow_stack::register_libc_jitframe` exists to + /// track. Those have no GcHeader, so the inline flag test reads + /// `jit_wb_if_flag_byteofs` (negative, where a header would be) out of + /// bytes whose contents are unspecified. When they happen to carry + /// TRACK_YOUNG_PTRS' bit the helper is entered in earnest, and only + /// `is_managed_heap_object` keeps the unmanaged block out of + /// `remembered_set`, where the next minor would decode a type id from it. + /// `write_barrier_ignores_unmanaged_jitframe_with_flag_byte_set` pins + /// that case. pub fn do_write_barrier(&mut self, obj: GcRef) { // incminimark's write_barrier receives a typed, non-null struct pointer. // pyre's GcRef is nullable (GcRef::NULL is the sentinel) and reaches the @@ -5117,6 +5135,62 @@ mod tests { assert_eq!(gc.remembered_set.len(), 0); } + #[test] + fn write_barrier_ignores_unmanaged_jitframe_with_flag_byte_set() { + // `_reload_frame_if_necessary` (aarch64/assembler.py:967-980, + // x86/assembler.py:1369) re-applies the NON-array write-barrier fast + // path to the current jitframe after a collecting helper call, so a + // plain COND_CALL_GC_WB on the frame reaches the generic barrier at + // runtime. + // + // A jitframe that came from the `libc::calloc` fallback rather than + // the nursery has no GcHeader, so the byte the inline test reads + // (`jit_wb_if_flag_byteofs`, negative — where a header would be) is + // not a header and its contents are unspecified. Whether it carries + // TRACK_YOUNG_PTRS' bit is a property of the host allocator and the + // heap's history, so the bit is set by hand here rather than waited + // for. Without the `is_managed_heap_object` guard in + // `do_write_barrier`, that block would enter `remembered_set` and the + // next minor would decode a type id from those bytes. + let mut gc = test_gc(1024); + + // Same shape the JITFRAME slow path calloc's: the fixed `JitFrame` + // words plus the trailing slot array, zero-filled. One extra leading + // word keeps the negative flag-byte read inside this allocation, + // standing in for whatever precedes a real malloc'd frame. + let slots = 32usize; + let frame_size = std::mem::size_of::() * (7 + 1 + slots); + let layout = + std::alloc::Layout::from_size_align(GcHeader::SIZE + frame_size, GcHeader::SIZE) + .unwrap(); + let base = unsafe { std::alloc::alloc_zeroed(layout) }; + assert!(!base.is_null()); + let frame = unsafe { base.add(GcHeader::SIZE) }; + let obj = frame as usize; + + // The frame is a fresh host allocation, so it cannot overlap either + // managed generation — that is precisely what the guard detects. + assert!(!gc.nursery.contains(obj)); + assert!(!gc.oldgen.contains(obj)); + + // Set the bit the inline COND_CALL_GC_WB test reads, so the helper is + // entered exactly as it is when the bytes before a real malloc'd + // frame happen to carry TRACK_YOUNG_PTRS. + let descr = crate::WriteBarrierDescr::for_current_gc(); + let flag_byte = unsafe { frame.offset(descr.jit_wb_if_flag_byteofs as isize) }; + unsafe { *flag_byte |= descr.jit_wb_if_flag_singlebyte }; + let flag_byte_before = unsafe { *flag_byte }; + + gc.do_write_barrier(GcRef(obj)); + + assert_eq!(gc.remembered_set.len(), 0); + // `remember_young_pointer` clears TRACK_YOUNG_PTRS, so an unchanged + // byte also proves the barrier never wrote through the fake header. + assert_eq!(unsafe { *flag_byte }, flag_byte_before); + + unsafe { std::alloc::dealloc(base, layout) }; + } + #[test] fn test_nursery_collection_with_pointers() { // Object layout: one GcRef field at offset 0 (payload = 8 bytes).