diff --git a/majit/majit-metainterp/src/history.rs b/majit/majit-metainterp/src/history.rs index d9e5620d8a5..7fdfbae1003 100644 --- a/majit/majit-metainterp/src/history.rs +++ b/majit/majit-metainterp/src/history.rs @@ -2711,6 +2711,14 @@ impl TraceCtx { self.recorder.set_last_guard_op_resume_position(snapshot_id); } + /// Set rd_resume_position on the guard op `from_end` guards back from the + /// most recent one (see + /// [`crate::recorder::Trace::set_guard_op_resume_position_from_end`]). + pub fn set_guard_op_resume_position_from_end(&mut self, from_end: usize, snapshot_id: i32) { + self.recorder + .set_guard_op_resume_position_from_end(from_end, snapshot_id); + } + /// TODO: low-level / single-frame snapshot helper /// used by callers that record guards without a populated framestack /// to walk. diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 877517e0e1b..fcedc21ffdc 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -171,6 +171,7 @@ pub use trace_ctx::GreenBox; pub use trace_ctx::MergePoint; pub use trace_ctx::ReconstructRecipe; pub use trace_ctx::TraceCtx; +pub use trace_ctx::VableArrayStore; /// Compute green key from code pointer and PC. /// Must use the same hash as the front-end's make_green_key — the full diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 6a011152500..1d41073a1e5 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -5086,7 +5086,7 @@ impl MetaInterp { fdescr: DescrRef, adescr: DescrRef, ) { - let ok = self + let stored = self .tracing .as_mut() .expect("opimpl_setarrayitem_vable_int requires active tracing") @@ -5102,7 +5102,7 @@ impl MetaInterp { false, ); assert!( - ok, + matches!(stored, crate::trace_ctx::VableArrayStore::Stored(_)), "opimpl_setarrayitem_vable_int: virtualizable array slot missing" ); } @@ -5119,7 +5119,7 @@ impl MetaInterp { fdescr: DescrRef, adescr: DescrRef, ) { - let ok = self + let stored = self .tracing .as_mut() .expect("opimpl_setarrayitem_vable_ref requires active tracing") @@ -5135,7 +5135,7 @@ impl MetaInterp { false, ); assert!( - ok, + matches!(stored, crate::trace_ctx::VableArrayStore::Stored(_)), "opimpl_setarrayitem_vable_ref: virtualizable array slot missing" ); } @@ -5152,7 +5152,7 @@ impl MetaInterp { fdescr: DescrRef, adescr: DescrRef, ) { - let ok = self + let stored = self .tracing .as_mut() .expect("opimpl_setarrayitem_vable_float requires active tracing") @@ -5168,7 +5168,7 @@ impl MetaInterp { false, ); assert!( - ok, + matches!(stored, crate::trace_ctx::VableArrayStore::Stored(_)), "opimpl_setarrayitem_vable_float: virtualizable array slot missing" ); } diff --git a/majit/majit-metainterp/src/pyjitpl/dispatch.rs b/majit/majit-metainterp/src/pyjitpl/dispatch.rs index 5819f6f9755..d45a6ba8e11 100644 --- a/majit/majit-metainterp/src/pyjitpl/dispatch.rs +++ b/majit/majit-metainterp/src/pyjitpl/dispatch.rs @@ -13,8 +13,21 @@ use majit_ir::{OpCode, OpRef, Type, Value}; use super::{MIFrame, MIFrameStack}; use crate::jitcode::insns::MAX_HOST_CALL_ARITY; use crate::jitcode::{self, JitArgKind, JitCallArg, JitCallTarget, JitCode, JitCodeRuntimeExt}; +use crate::trace_ctx::{VableArrayStore, VableEntryWrite}; use crate::{TraceAction, TraceCtx}; +/// Which recorded op [`JitCodeMachine::publish_last_guard_resume_snapshot`] +/// points at the snapshot it just captured. +#[derive(Clone, Copy)] +enum GuardStampTarget { + /// The last recorded op — correct when the guard IS the last op, which is + /// the case for every guard the dispatcher records itself. + LastOp, + /// A guard op counted back from the most recent one (`0` == the most + /// recent), for guards a `TraceCtx` helper emitted before returning. + GuardFromEnd(usize), +} + /// Decode a virtualizable shadow Value (RPython Box concrete) back into the /// raw int/ref/float bit pattern that pyre stores in register shadows /// (`frame.int_values`, `frame.ref_values`, `frame.float_values`). @@ -1443,7 +1456,7 @@ where sym, resume_pc, after_residual_call, - false, + GuardStampTarget::LastOp, Some((opcode, fail_args.len())), ); guard_op @@ -1471,6 +1484,23 @@ where /// the promotes are conditional (an already-constant index, or the /// standard virtualizable short-circuiting at step 1) and emit nothing in /// the common case, so an unchanged count means there is nothing to stamp. + /// One call can emit TWO: a vable array access whose symbolic frame box + /// differs from the standard box but shares its pointer promotes the + /// `isstandard` PTR_EQ and then the index, so every guard the call added is + /// stamped, not just the last. In pyre the two snapshots currently come + /// out identical because `TraceCtx::replace_box` updates the side tables + /// but not the live `MIFrame`s; this is a pre-existing gap, not parity: + /// upstream `MetaInterp.replace_box` walks the framestack via + /// `frame.replace_active_box_in_frame`, so its second capture sees the + /// standard box where the first saw the old one. The loop runs in emission + /// order because each capture leaves the root frame's in-flight result slot + /// cleared. + /// + /// `write` is the shadow slot a `vable_set*` overwrote. Upstream reaches + /// `virtualizable_boxes[index] = valuebox` only after both promotes have + /// captured their resume data, so the slot is put back for the duration of + /// the capture — otherwise the guard's resume data carries the very write + /// its resume pc re-executes (see [`VableEntryWrite`]). /// /// `opcode_pc` is the vable op's own JitCode position. The lowering emits /// a `-live-` marker in front of every vable op (`lower_vable.rs`, mirroring @@ -1482,8 +1512,10 @@ where sym: &mut S, opcode_pc: usize, guards_before: usize, + write: Option, ) { - if ctx.num_guards() <= guards_before { + let minted = ctx.num_guards().saturating_sub(guards_before); + if minted == 0 { return; } if sym.fail_args_with_ctx(ctx).is_none() { @@ -1492,7 +1524,23 @@ where // guard without one. return; } - self.publish_last_guard_resume_snapshot(ctx, sym, opcode_pc, false, true, None); + let restored = write.and_then(|w| { + ctx.swap_virtualizable_entry(w.index, w.prev_box, w.prev_value) + .map(|current| (w.index, current)) + }); + for from_end in (0..minted).rev() { + self.publish_last_guard_resume_snapshot( + ctx, + sym, + opcode_pc, + false, + GuardStampTarget::GuardFromEnd(from_end), + None, + ); + } + if let Some((index, (op, value))) = restored { + ctx.swap_virtualizable_entry(index, op, value); + } } /// Attach a resume snapshot built from the live framestack to the guard @@ -1506,11 +1554,12 @@ where /// guard's resumepc independent of the dispatcher's post-decode /// `frame.pc`. /// - /// `stamp_last_guard_op` selects which recorded op receives the position: - /// the last op, or the last *guard* op. They differ whenever the guard is - /// not the last thing recorded — `emit_force_virtualizable` records - /// GETFIELD_GC / PTR_NE / COND_CALL on top of the vable promote, so that - /// caller needs the guard-op form or the position lands on the COND_CALL. + /// `target` selects which recorded op receives the position: the last op, + /// or a *guard* op counted back from the most recent one. They differ + /// whenever the guard is not the last thing recorded — + /// `emit_force_virtualizable` records GETFIELD_GC / PTR_NE / COND_CALL on + /// top of the vable promote, so that caller needs the guard form or the + /// position lands on the COND_CALL. /// /// `diag` carries the guard opcode and its fail-arg count for the /// `callee_rca` trace only; callers re-stamping a guard `TraceCtx` @@ -1521,7 +1570,7 @@ where sym: &mut S, resume_pc: usize, after_residual_call: bool, - stamp_last_guard_op: bool, + target: GuardStampTarget, diag: Option<(OpCode, usize)>, ) { let top_idx = self @@ -1655,10 +1704,11 @@ where } self.frames.frames[top_idx].pc = saved_top_pc; let snapshot_id = ctx.capture_resumedata(snapshot); - if stamp_last_guard_op { - ctx.set_last_guard_op_resume_position(snapshot_id); - } else { - ctx.set_last_guard_resume_position(snapshot_id); + match target { + GuardStampTarget::LastOp => ctx.set_last_guard_resume_position(snapshot_id), + GuardStampTarget::GuardFromEnd(from_end) => { + ctx.set_guard_op_resume_position_from_end(from_end, snapshot_id) + } } } @@ -3331,7 +3381,7 @@ where let guards_before = ctx.num_guards(); let (opref, value) = ctx.vable_getfield_int(opcode_pc, vable_opref, vable_struct_ptr, fielddescr); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_int_reg(dest, Some(opref), value.map(value_as_int_bits)); } jitcode::insns::BC_GETFIELD_VABLE_R => { @@ -3350,7 +3400,7 @@ where let guards_before = ctx.num_guards(); let (opref, value) = ctx.vable_getfield_ref(opcode_pc, vable_opref, vable_struct_ptr, fielddescr); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_ref_reg(dest, Some(opref), value.map(value_as_ref_bits)); } jitcode::insns::BC_GETFIELD_VABLE_F => { @@ -3369,7 +3419,7 @@ where let guards_before = ctx.num_guards(); let (opref, value) = ctx.vable_getfield_float(opcode_pc, vable_opref, vable_struct_ptr, fielddescr); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_float_reg(dest, Some(opref), value.map(value_as_float_bits)); } jitcode::insns::BC_NEW | jitcode::insns::BC_NEW_WITH_VTABLE => { @@ -3788,14 +3838,14 @@ where }; let (value, concrete) = self.read_int_reg(src); let guards_before = ctx.num_guards(); - ctx.vable_setfield( + let write = ctx.vable_setfield( opcode_pc, vable_opref, fielddescr, value, Some(Value::Int(concrete)), ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } jitcode::insns::BC_SETFIELD_VABLE_R => { let (opcode_pc, vable_reg, field_idx, src) = { @@ -3811,14 +3861,14 @@ where }; let (value, concrete) = self.read_ref_reg(src); let guards_before = ctx.num_guards(); - ctx.vable_setfield( + let write = ctx.vable_setfield( opcode_pc, vable_opref, fielddescr, value, Some(Value::Ref(majit_ir::GcRef(concrete as usize))), ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } jitcode::insns::BC_SETFIELD_VABLE_F => { let (opcode_pc, vable_reg, field_idx, src) = { @@ -3834,14 +3884,14 @@ where }; let (value, concrete) = self.read_float_reg(src); let guards_before = ctx.num_guards(); - ctx.vable_setfield( + let write = ctx.vable_setfield( opcode_pc, vable_opref, fielddescr, value, Some(Value::Float(f64::from_bits(concrete as u64))), ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } // ── BC_ARRAYLEN_GC ── // @@ -4247,7 +4297,7 @@ where fdescr, adescr, ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_int_reg(dest, Some(opref), value.map(value_as_int_bits)); } jitcode::insns::BC_GETARRAYITEM_VABLE_R => { @@ -4272,7 +4322,7 @@ where fdescr, adescr, ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_ref_reg(dest, Some(opref), value.map(value_as_ref_bits)); } jitcode::insns::BC_GETARRAYITEM_VABLE_F => { @@ -4297,7 +4347,7 @@ where fdescr, adescr, ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); self.set_float_reg(dest, Some(opref), value.map(value_as_float_bits)); } jitcode::insns::BC_SETARRAYITEM_VABLE_I => { @@ -4315,7 +4365,7 @@ where let (index, index_value) = self.read_int_reg(index_reg); let (value, concrete) = self.read_int_reg(src); let guards_before = ctx.num_guards(); - if !ctx.vable_setarrayitem_indexed( + let write = match ctx.vable_setarrayitem_indexed( opcode_pc, vable_opref, index, @@ -4329,9 +4379,10 @@ where // Promoted index falls outside the standard virtualizable // array (e.g. a transient out-of-bounds state-field index); // this slot cannot be virtualized, so abort the trace. - return TraceAction::Abort; - } - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + VableArrayStore::OutOfVable => return TraceAction::Abort, + VableArrayStore::Stored(write) => write, + }; + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } jitcode::insns::BC_SETARRAYITEM_VABLE_R => { let (opcode_pc, vable_reg, array_idx, index_reg, src) = { @@ -4348,7 +4399,7 @@ where let (index, index_value) = self.read_int_reg(index_reg); let (value, concrete) = self.read_ref_reg(src); let guards_before = ctx.num_guards(); - if !ctx.vable_setarrayitem_indexed( + let write = match ctx.vable_setarrayitem_indexed( opcode_pc, vable_opref, index, @@ -4359,9 +4410,10 @@ where Value::Ref(majit_ir::GcRef(concrete as usize)), false, ) { - return TraceAction::Abort; - } - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + VableArrayStore::OutOfVable => return TraceAction::Abort, + VableArrayStore::Stored(write) => write, + }; + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } jitcode::insns::BC_SETARRAYITEM_VABLE_F => { let (opcode_pc, vable_reg, array_idx, index_reg, src) = { @@ -4378,7 +4430,7 @@ where let (index, index_value) = self.read_int_reg(index_reg); let (value, concrete) = self.read_float_reg(src); let guards_before = ctx.num_guards(); - if !ctx.vable_setarrayitem_indexed( + let write = match ctx.vable_setarrayitem_indexed( opcode_pc, vable_opref, index, @@ -4389,9 +4441,10 @@ where Value::Float(f64::from_bits(concrete as u64)), false, ) { - return TraceAction::Abort; - } - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + VableArrayStore::OutOfVable => return TraceAction::Abort, + VableArrayStore::Stored(write) => write, + }; + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, write); } jitcode::insns::BC_ARRAYLEN_VABLE => { let (opcode_pc, vable_reg, array_idx, dest) = { @@ -4414,7 +4467,7 @@ where fdescr, adescr, ); - self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before); + self.capture_vable_promote_guard(ctx, sym, opcode_pc, guards_before, None); // pyjitpl.py:1262-1263 `result = // vinfo.get_array_length(virtualizable, arrayindex); // return ConstInt(result)`. RPython reads from the live @@ -10777,6 +10830,259 @@ mod tests { assert_eq!(recorder.num_ops(), 0); } + /// A `JitCodeSym` that names a live set, so guard capture is reachable. + /// `DummySym` answers `None` and skips it entirely. + struct FailArgsSym { + fail_args: Vec, + } + + impl JitCodeSym for FailArgsSym { + fn total_slots(&self) -> usize { + self.fail_args.len() + } + + fn loop_header_pc(&self) -> usize { + 0 + } + + fn fail_args(&self) -> Option> { + Some(self.fail_args.clone()) + } + } + + #[test] + fn vable_setarrayitem_promote_guard_snapshot_predates_the_store() { + // `_opimpl_setarrayitem_vable` (pyjitpl.py:1236-1247) promotes the + // index in `_get_arrayitem_vable_index` (:1201-1216) and only THEN + // reaches `virtualizable_boxes[index] = valuebox`. The promote's + // `implement_guard_value` captures resume data at the promote, so the + // guard's snapshot names the slot's OLD Box. + // + // Were it captured after the store, a failing index promote would + // restore the traced write into the promoted slot and then re-execute + // the setter at the real index — one spurious write per failure. + // The snapshot's liveness decode reads the `-live-` marker in front of + // the vable op (`lower_vable.rs`, jtransform.py:764/798/814/845/926). + let mut asm = majit_translate::codewriter::assembler::Assembler::new(); + let mut builder = JitCodeBuilder::new(); + builder.live(&mut asm, &[0, 1], &[0], &[]); + builder.vable_setarrayitem_int_with_base(0, 0, 0, 1); + let jitcode = builder.finish(); + + let mut staticdata = crate::MetaInterpStaticData::new(); + staticdata.op_live = crate::jitcode::insns::BC_LIVE as i32; + staticdata.liveness_info = asm.all_liveness().to_vec(); + let mut recorder = crate::recorder::Trace::new(); + recorder.record_input_arg(majit_ir::Type::Int); // index + recorder.record_input_arg(majit_ir::Type::Int); // value + let mut ctx = TraceCtx::new(recorder, 0, std::sync::Arc::new(staticdata)); + let info = make_test_vable_info(); + let field_box = ctx.const_int(111); + let array_box = ctx.const_int(222); + let vable_ref = ctx.const_ref(999); + ctx.init_virtualizable_boxes( + &info, + vable_ref, + Value::Ref(majit_ir::GcRef(999)), + &[field_box, array_box], + &[Value::Int(111), Value::Int(222)], + &[1], + ); + // Flat layout is [static field 0, array slot 0, identity]; the write + // below lands on slot 1, which currently holds `array_box`. + assert_eq!( + ctx.virtualizable_entry_at(1).map(|(op, _)| op), + Some(array_box) + ); + + let mut sym = FailArgsSym { + fail_args: vec![OpRef::input_arg_int(0), OpRef::input_arg_int(1)], + }; + let action = trace_jitcode_with_args( + &mut ctx, + &mut sym, + &jitcode, + 0, + |_pc| 0, + &[ + (JitArgKind::Ref, vable_ref, 999), + // A non-constant index is what makes the promote emit a guard; + // a Const index short-circuits `implement_guard_value`. + (JitArgKind::Int, OpRef::input_arg_int(0), 0), + (JitArgKind::Int, OpRef::input_arg_int(1), 777), + ], + ); + assert!(matches!(action, TraceAction::Continue)); + + let value_box = ctx + .virtualizable_entry_at(1) + .map(|(op, _)| op) + .expect("the store must have landed on slot 1"); + assert_ne!( + value_box, array_box, + "the setter must have replaced the slot's Box", + ); + + let snapshots = ctx.snapshots().to_vec(); + let recorder = ctx.into_recorder(); + let guard = recorder + .ops() + .iter() + .rev() + .find(|op| op.opcode == OpCode::GuardValue) + .expect("a non-constant vable array index must promote"); + let resume = guard.rd_resume_position.get(); + assert!( + resume >= 0, + "the promote guard must carry a resume position", + ); + // `build_vable_snapshot_boxes` moves the identity Box to the front, so + // assert on membership rather than on the shadow's flat index. + let boxes = &snapshots[resume as usize].vable_boxes; + assert!( + boxes.contains(&crate::recorder::SnapshotTagged::Box( + array_box, + majit_ir::Type::Int + )), + "the promote guard's snapshot must hold the PRE-store Box; got {boxes:?}", + ); + assert!( + !boxes.contains(&crate::recorder::SnapshotTagged::Box( + value_box, + majit_ir::Type::Int + )), + "the snapshot must not carry the write its resume pc re-executes; got {boxes:?}", + ); + } + + #[test] + fn both_promote_guards_of_one_vable_opcode_are_stamped() { + // When the vable Box is not the standard Box but carries its pointer, + // `_nonstandard_virtualizable` cannot short-circuit on Box identity + // (pyjitpl.py:1131) and promotes the `isstandard` PTR_EQ (:1135-1138); + // `_get_arrayitem_vable_index` then promotes the index (:1201-1216). + // One opcode, two guards, and upstream captures resume data at each + // `implement_guard_value` — so neither may be left unstamped. + let mut asm = majit_translate::codewriter::assembler::Assembler::new(); + let mut builder = JitCodeBuilder::new(); + builder.live(&mut asm, &[0, 1], &[0], &[]); + builder.vable_setarrayitem_int_with_base(0, 0, 0, 1); + let jitcode = builder.finish(); + + let mut staticdata = crate::MetaInterpStaticData::new(); + staticdata.op_live = crate::jitcode::insns::BC_LIVE as i32; + staticdata.liveness_info = asm.all_liveness().to_vec(); + let mut recorder = crate::recorder::Trace::new(); + let vable_arg = recorder.record_input_arg(majit_ir::Type::Ref); + recorder.record_input_arg(majit_ir::Type::Int); // index + recorder.record_input_arg(majit_ir::Type::Int); // value + let mut ctx = TraceCtx::new(recorder, 0, std::sync::Arc::new(staticdata)); + let info = make_test_vable_info(); + let field_box = ctx.const_int(111); + let array_box = ctx.const_int(222); + let standard_box = ctx.const_ref(999); + ctx.init_virtualizable_boxes( + &info, + standard_box, + Value::Ref(majit_ir::GcRef(999)), + &[field_box, array_box], + &[Value::Int(111), Value::Int(222)], + &[1], + ); + // Same pointer, different Box: `concrete_ptrs_eq` answers isstandard=1, + // which is the leg that emits the PTR_EQ guard and then continues into + // the standard path instead of falling back to the heap. + ctx.set_opref_concrete(vable_arg, Value::Ref(majit_ir::GcRef(999))); + + let mut sym = FailArgsSym { + fail_args: vec![OpRef::input_arg_int(0), OpRef::input_arg_int(1)], + }; + let action = trace_jitcode_with_args( + &mut ctx, + &mut sym, + &jitcode, + 0, + |_pc| 0, + &[ + (JitArgKind::Ref, vable_arg, 999), + (JitArgKind::Int, OpRef::input_arg_int(0), 0), + (JitArgKind::Int, OpRef::input_arg_int(1), 777), + ], + ); + assert!(matches!(action, TraceAction::Continue)); + + let snapshots = ctx.snapshots().to_vec(); + let recorder = ctx.into_recorder(); + let guards: Vec<_> = recorder + .ops() + .iter() + .filter(|op| op.opcode.is_guard()) + .collect(); + assert_eq!( + guards.len(), + 2, + "one setarrayitem_vable over a non-identical standard Box promotes \ + twice; got {:?}", + guards.iter().map(|op| op.opcode).collect::>(), + ); + // A guard `TraceCtx` minted internally already points at the one-frame + // placeholder `record_guard_with_snapshot` publishes, so a resume + // position alone proves nothing: the frame it names carries + // `UNSTAMPED_JITCODE_INDEX` until the dispatch layer re-stamps it, and + // resume decoding sizes the frame from that coordinate. + for (i, guard) in guards.iter().enumerate() { + let resume = guard.rd_resume_position.get(); + assert!( + resume >= 0, + "guard {i} ({:?}) was left without a resume position", + guard.opcode, + ); + let frames = &snapshots[resume as usize].frames; + assert!( + frames + .iter() + .all(|f| f.jitcode_index != crate::recorder::UNSTAMPED_JITCODE_INDEX), + "guard {i} ({:?}) still points at an unstamped frame", + guard.opcode, + ); + } + } + + #[test] + fn every_guard_one_vable_opcode_emits_gets_a_resume_position() { + // `set_guard_op_resume_position_from_end` is what lets a caller stamp + // more than the last guard: one vable array access can emit the + // `isstandard` PTR_EQ promote (`_nonstandard_virtualizable`, + // pyjitpl.py:1135-1138) and then the index promote (:1201-1216). + // Walking back over them leaves none holding the + // `UNSTAMPED_JITCODE_INDEX` frame the recorder mints. + let mut recorder = crate::recorder::Trace::new(); + let a = recorder.record_input_arg(majit_ir::Type::Int); + recorder.record_guard(OpCode::GuardValue, &[a], None); + recorder.record_op(OpCode::IntAdd, &[a, a]); + recorder.record_guard(OpCode::GuardValue, &[a], None); + + recorder.set_guard_op_resume_position_from_end(0, 7); + recorder.set_guard_op_resume_position_from_end(1, 5); + + let guards: Vec = recorder + .ops() + .iter() + .filter(|op| op.opcode.is_guard()) + .map(|op| op.rd_resume_position.get()) + .collect(); + assert_eq!(guards, vec![5, 7]); + // Past the end is a no-op, not a panic or a mis-stamp. + recorder.set_guard_op_resume_position_from_end(2, 9); + let guards: Vec = recorder + .ops() + .iter() + .filter(|op| op.opcode.is_guard()) + .map(|op| op.rd_resume_position.get()) + .collect(); + assert_eq!(guards, vec![5, 7]); + } + #[test] fn jitcode_new_struct_records_new_with_size_descr_and_ref_result() { // blackhole.py:1301 bhimpl_new: the tracer records `New` carrying a diff --git a/majit/majit-metainterp/src/recorder.rs b/majit/majit-metainterp/src/recorder.rs index a2b31a96512..80947a94aef 100644 --- a/majit/majit-metainterp/src/recorder.rs +++ b/majit/majit-metainterp/src/recorder.rs @@ -427,7 +427,27 @@ impl Trace { /// COND_CALL) is not the last op when the caller captures, so the /// resume position must target the guard by its guard-ness. pub fn set_last_guard_op_resume_position(&mut self, snapshot_id: i32) { - if let Some(op) = self.ops.iter().rev().find(|op| op.opcode.is_guard()) { + self.set_guard_op_resume_position_from_end(0, snapshot_id); + } + + /// Set rd_resume_position on the guard op `from_end` guards back from the + /// most recently recorded one (`0` == the most recent). + /// + /// One helper call can emit more than one guard: a vable array access + /// promotes the `isstandard` PTR_EQ in `_nonstandard_virtualizable` + /// (`pyjitpl.py:1135-1138`) and then the index in + /// `_get_arrayitem_vable_index` (`:1201-1216`). Upstream captures resume + /// data inside each `implement_guard_value`, so both are stamped; a caller + /// that only reaches the guards after the helper returns walks back over + /// them with this. + pub fn set_guard_op_resume_position_from_end(&mut self, from_end: usize, snapshot_id: i32) { + if let Some(op) = self + .ops + .iter() + .rev() + .filter(|op| op.opcode.is_guard()) + .nth(from_end) + { op.rd_resume_position.set(snapshot_id); } } diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index 6915fe4bd08..4643acb3725 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -705,6 +705,51 @@ pub struct BridgeInlineCarrier { pub recipes: Vec, } +/// The virtualizable shadow slot a `vable_set*` standard leg overwrote, and +/// the Box it held before. +/// +/// `_opimpl_setfield_vable` / `_opimpl_setarrayitem_vable` (pyjitpl.py:1188, +/// :1236) reach `virtualizable_boxes[index] = valuebox` only AFTER +/// `_nonstandard_virtualizable` / `_get_arrayitem_vable_index` have promoted, +/// and each promote captures its guard's resume data inside +/// `implement_guard_value` — that is, against the shadow as it stood before the +/// write. Pyre fuses promote and store into one `TraceCtx` call and the +/// dispatcher builds the snapshot afterwards, so the caller puts this slot back +/// for the duration of the capture ([`TraceCtx::swap_virtualizable_entry`]). +/// Without it the guard's resume data carries the very write its resume pc will +/// re-execute: a failing index promote would restore the value into the +/// promoted slot and then write it again at the real index. +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct VableEntryWrite { + /// Flat index into `virtualizable_boxes` / `virtualizable_values`. + pub index: usize, + pub prev_box: OpRef, + pub prev_value: Value, +} + +impl VableEntryWrite { + /// Read the slot `index` currently holds, or `None` when no shadow is + /// active — the caller then has nothing to restore. + fn of(ctx: &TraceCtx, index: usize) -> Option { + let (prev_box, prev_value) = ctx.virtualizable_entry_at(index)?; + Some(Self { + index, + prev_box, + prev_value, + }) + } +} + +/// Outcome of `vable_setarrayitem_indexed`. +pub enum VableArrayStore { + /// The promoted index falls outside the standard virtualizable array, so + /// the slot cannot be virtualized and the caller aborts the trace. + OutOfVable, + /// Stored. `Some` names the shadow slot the standard leg overwrote; + /// `None` is the nonstandard leg, which records a heap `SetarrayitemGc`. + Stored(Option), +} + /// rlib/jit.py:592 default `trace_limit` — mirrored here so standalone /// TraceCtx construction (unit tests, `setup_tracing` before a warmstate /// override) matches the RPython baseline. @@ -2845,6 +2890,30 @@ impl TraceCtx { } } + /// Put `opref`/`value` into flat slot `index` and hand back what it held. + /// + /// This is the save/restore half of [`VableEntryWrite`], not a store: it + /// deliberately leaves `virtualizable_live_null_slots` alone, where + /// [`Self::set_virtualizable_entry_at`] clears it and + /// `vable_setarrayitem_indexed`'s `live_null_push` arm sets it right after. + /// Restoring through the store would drop that flag. + /// + /// `None` when no shadow is active or `index` is out of range — the same + /// condition under which [`Self::virtualizable_entry_at`] reads `None`. + pub fn swap_virtualizable_entry( + &mut self, + index: usize, + opref: OpRef, + value: Value, + ) -> Option<(OpRef, Value)> { + let prev = self.virtualizable_entry_at(index)?; + let boxes = self.virtualizable_boxes.as_mut()?; + *boxes.get_mut(index)? = opref; + let values = self.virtualizable_values.as_mut()?; + *values.get_mut(index)? = value; + Some(prev) + } + /// Whether the last executed store into flat slot `index` wrote a live NULL Ref. pub fn virtualizable_slot_stored_live_null(&self, index: usize) -> bool { self.virtualizable_live_null_slots @@ -4056,6 +4125,12 @@ impl TraceCtx { /// self.metainterp.synchronize_virtualizable() /// # XXX only the index'th field needs to be synchronized, really /// ``` + /// + /// Returns the shadow slot the standard leg overwrote, so a caller that + /// still has to build a resume snapshot for a guard the promote above + /// emitted can put the old Box back first — see [`VableEntryWrite`]. + /// `None` on the nonstandard leg, which records a heap `SetfieldGc` and + /// leaves the shadow untouched. pub fn vable_setfield( &mut self, pc: usize, @@ -4063,7 +4138,7 @@ impl TraceCtx { fielddescr: DescrRef, value: OpRef, concrete: Option, - ) { + ) -> Option { let vable_concrete = self.concrete_of_opref(vable_opref); if self.is_nonstandard_virtualizable(pc, vable_opref, &fielddescr, vable_concrete) { // self._opimpl_setfield_gc_any(box, valuebox, fielddescr) @@ -4091,7 +4166,7 @@ impl TraceCtx { // (Box identity, not value equality). self.profiler() .count_ops(OpCode::SetfieldGc, crate::pyjitpl::counters::HEAPCACHED_OPS); - return; + return None; } } // pyjitpl.py:1173-1199 nonstandard vable miss delegates to @@ -4109,7 +4184,7 @@ impl TraceCtx { // it through `box_value(cached)`. let _ = concrete; self.heapcache_setfield_cached(vable_opref, field_index, value); - return; + return None; } // index = self._get_virtualizable_field_index(fielddescr) // self.metainterp.virtualizable_boxes[index] = valuebox @@ -4125,6 +4200,7 @@ impl TraceCtx { // role-2 shadow store keeps a `Value::Ref(GcRef::NO_CONCRETE)` // until that store is itself moved to `Vec>`. let stored = concrete.unwrap_or(Value::Ref(majit_ir::GcRef::NO_CONCRETE)); + let overwritten = VableEntryWrite::of(self, index); self.set_virtualizable_entry_at(index, value, stored); // Keep the heapcache consistent: if a prior nonstandard getfield // cached a value for this field (e.g., before a replace_box made @@ -4136,6 +4212,7 @@ impl TraceCtx { // pyjitpl.py:3446 write_boxes parity: mirror the updated // shadow slot back into the live virtualizable. self.synchronize_virtualizable(); + overwritten } /// Record a virtualizable field write with an explicit field descriptor. @@ -4699,7 +4776,7 @@ impl TraceCtx { value: OpRef, concrete: Value, live_null_push: bool, - ) -> bool { + ) -> VableArrayStore { let vable_concrete = self.concrete_of_opref(vable_opref); if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, vable_concrete) { let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr); @@ -4708,7 +4785,7 @@ impl TraceCtx { self.profiler() .count_ops(OpCode::SetarrayitemGc, crate::counters::RECORDED_OPS); self.execute_setarrayitem_gc(array_opref, index, value, adescr); - return true; + return VableArrayStore::Stored(None); } // index = self._get_arrayitem_vable_index(pc, fdescr, indexbox) // self.metainterp.virtualizable_boxes[index] = valuebox @@ -4716,8 +4793,9 @@ impl TraceCtx { let Some(flat_idx) = self.get_arrayitem_vable_index(pc, index, index_runtime_value, &fdescr) else { - return false; + return VableArrayStore::OutOfVable; }; + let overwritten = VableEntryWrite::of(self, flat_idx); self.set_virtualizable_entry_at(flat_idx, value, concrete); if live_null_push && matches!(concrete, Value::Ref(r) if r.is_null()) { if let Some(live_null_slots) = self.virtualizable_live_null_slots.as_mut() { @@ -4725,7 +4803,7 @@ impl TraceCtx { } } self.synchronize_virtualizable(); - true + VableArrayStore::Stored(overwritten) } /// pyjitpl.py:754-763 `opimpl_arraylen_gc(arraybox, arraydescr)`. 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 683ab8eea5a..72e1349c274 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -2786,10 +2786,17 @@ pub(crate) fn try_execute_residual_call_via_executor( // `W_UnicodeObject.descr_iter` into a fresh sequence iterator. Pyre's // tagged `GetIter` call has the same no-user-dispatch property for an // exact string; subclasses retain the conservative decline. - // The wasm optimizer currently rejects the resulting longer trace as - // `InvalidLoop` (three compile aborts versus the conservative path's one - // trace-time decline). Keep its prior admission boundary until that - // backend can consume this shape; interpreter semantics are identical. + // The wasm optimizer still rejects the resulting longer trace: dropping + // this gate and rebuilding the guest reads `abrt_bad_loop=1` on + // `str_search_index_bounds`, with `bridges_compiled` 7 -> 5 and + // `guard_failures` 2096 -> 7098 against 1897 for the native backends. + // Keep its prior admission boundary until that backend can consume this + // shape; interpreter semantics are identical. Its own cost on that bench + // is one trace-time decline — the hazardous-callee arm of + // `fbw_abort_nested_unjournaled_residual`, reached because the `GetIter` + // sits inside the loop-bearing `fold` this comment's `str()` sibling names + // — and the 199 guard failures its unbridged guard re-fires one + // `trace_eagerness` cycle later. let native_exact_str_replay = !cfg!(target_arch = "wasm32"); let observed_exact_str_iter = native_exact_str_replay && helper == majit_ir::PyreHelperKind::GetIter diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs index afbef1ee384..a45b9177757 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::jitcode_runtime::{insns_opname_to_byte, named_jitcode}; use majit_ir::Type; -use majit_metainterp::make_fail_descr; +use majit_metainterp::{VableArrayStore, make_fail_descr}; #[test] fn propagated_subwalk_abort_cannot_rebind_its_pc_to_a_caller_frame() { @@ -570,16 +570,19 @@ fn vable_store_tracks_live_null_without_changing_the_recorded_trace() { let index0 = tc.const_int(0); let const_null = tc.const_null(); let ops_before = tc.num_ops(); - assert!(tc.vable_setarrayitem_indexed( - 0, - vable, - index0, - 0, - fdescr.clone(), - adescr.clone(), - const_null, - null, - true, + assert!(matches!( + tc.vable_setarrayitem_indexed( + 0, + vable, + index0, + 0, + fdescr.clone(), + adescr.clone(), + const_null, + null, + true, + ), + VableArrayStore::Stored(_) )); assert!(tc.virtualizable_slot_stored_live_null(flat_base)); assert!( @@ -598,37 +601,44 @@ fn vable_store_tracks_live_null_without_changing_the_recorded_trace() { "the side-table marker records no op" ); - assert!(tc.vable_setarrayitem_indexed( - 0, - vable, - index0, - 0, - fdescr.clone(), - adescr.clone(), - const_null, - null, - false, + assert!(matches!( + tc.vable_setarrayitem_indexed( + 0, + vable, + index0, + 0, + fdescr.clone(), + adescr.clone(), + const_null, + null, + false, + ), + VableArrayStore::Stored(_) )); assert!(!tc.virtualizable_slot_stored_live_null(flat_base)); let index1 = tc.const_int(1); let non_null = tc.const_ref(2); - assert!(tc.vable_setarrayitem_indexed( - 0, - vable, - index1, - 1, - fdescr.clone(), - adescr.clone(), - non_null, - Value::Ref(majit_ir::GcRef(2)), - true, + assert!(matches!( + tc.vable_setarrayitem_indexed( + 0, + vable, + index1, + 1, + fdescr.clone(), + adescr.clone(), + non_null, + Value::Ref(majit_ir::GcRef(2)), + true, + ), + VableArrayStore::Stored(_) )); assert!(!tc.virtualizable_slot_stored_live_null(flat_base + 1)); - assert!( - tc.vable_setarrayitem_indexed(0, vable, index0, 0, fdescr, adescr, const_null, null, true,) - ); + assert!(matches!( + tc.vable_setarrayitem_indexed(0, vable, index0, 0, fdescr, adescr, const_null, null, true,), + VableArrayStore::Stored(_) + )); assert!(tc.virtualizable_slot_stored_live_null(flat_base)); tc.set_virtualizable_entry_at(flat_base, const_null, null); assert!(!tc.virtualizable_slot_stored_live_null(flat_base));