diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index 37476802548..455deaaf9a8 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -6056,10 +6056,15 @@ fn read_descr<'a>(bh: &'a BlackholeInterpreter, code: &[u8], pos: usize) -> (&'a /// RPython: fielddescr carries byte offset directly; pyre VableField.index /// needs vinfo.static_fields[index].offset resolution. /// -/// Vable scalar word-size invariant: `field_size: 8`, `field_type: Ref`, -/// `field_flag: Pointer`, `is_field_signed: false`. Every vable scalar -/// field in pyre is laid out as a single machine word, so the synthesized -/// BhDescr can hard-code these. The dynasm / cranelift `bh_getfield_gc_*` +/// Vable scalar word-size invariant: `field_size: size_of::()`, +/// `field_type: Ref`, `field_flag: Pointer`, `is_field_signed: false`. +/// Every vable scalar field in pyre is laid out as a single machine word, +/// so the synthesized BhDescr can derive these. The width has to be the +/// target's word, not a literal 8: the size-dispatching +/// `Backend::bh_setfield_gc_i` picks its store width from it, and on a +/// 32-bit target an 8-byte store at `valuestackdepth` runs past the field +/// and clears the `last_instr` that follows it. The dynasm / cranelift +/// `bh_getfield_gc_*` /// overrides on this BhDescr therefore read i64 / GcRef / f64 at the /// resolved offset without consulting size/sign — equivalent to the /// llmodel.py:705 `read_int_at_mem(struct, ofs, 8, False)` call. @@ -6098,7 +6103,7 @@ fn read_descr_vable_field(bh: &BlackholeInterpreter, code: &[u8], pos: usize) -> BhDescr::Field { offset, // Vable scalar word-size invariant — see fn doc-block. - field_size: 8, + field_size: std::mem::size_of::(), field_type: majit_ir::value::Type::Ref, field_flag: majit_ir::descr::ArrayFlag::Pointer, is_field_signed: false, @@ -6144,7 +6149,11 @@ fn read_descr_vable_array(bh: &BlackholeInterpreter, code: &[u8], pos: usize) -> ( BhDescr::Field { offset, - field_size: 8, + // The array field is a pointer, so its width is the target word + // like the scalars above. Latent rather than live: the `_gc_r` + // accessors this descr reaches take `as_offset()` and store at + // pointer width without consulting the size. + field_size: std::mem::size_of::(), field_type: majit_ir::value::Type::Ref, field_flag: majit_ir::descr::ArrayFlag::Pointer, is_field_signed: false, diff --git a/majit/majit-metainterp/src/optimizeopt/mod.rs b/majit/majit-metainterp/src/optimizeopt/mod.rs index 1ab9f8f34ea..5da4338b034 100644 --- a/majit/majit-metainterp/src/optimizeopt/mod.rs +++ b/majit/majit-metainterp/src/optimizeopt/mod.rs @@ -578,6 +578,12 @@ pub struct OptContext { /// Set by rewrite pass, executed by emit_operation after the guard /// is added to new_operations (matching RPython's callback pattern). pub(crate) pending_guard_class_postprocess: Option, + /// virtualize.py:84-90 postprocess_FINISH queues the stashed + /// GUARD_NOT_FORCED_2 here so the outer optimizer can insert it at + /// `new_operations.len() - 1` with full `store_final_boxes_in_guard` + /// semantics — the pass that stashes it holds no Optimizer, and both the + /// finalization and the knowledge collection are Optimizer-side. + pub(crate) pending_finish_guard_postprocess: Option, /// rewrite.py:282: postprocess_GUARD_NONNULL → mark_last_guard. /// Deferred until emit adds the guard to new_operations. pub(crate) pending_mark_last_guard: Option, @@ -1659,6 +1665,7 @@ impl OptContext { extra_operations_after: VecDeque::new(), pending_guard_class_postprocess: None, pending_mark_last_guard: None, + pending_finish_guard_postprocess: None, imported_short_pure_ops: Vec::new(), imported_virtual_args: None, imported_loop_invariant_results: Vec::new(), @@ -2264,6 +2271,7 @@ impl OptContext { extra_operations_after: VecDeque::new(), pending_guard_class_postprocess: None, pending_mark_last_guard: None, + pending_finish_guard_postprocess: None, imported_short_pure_ops: Vec::new(), imported_virtual_args: None, imported_loop_invariant_results: Vec::new(), diff --git a/majit/majit-metainterp/src/optimizeopt/optimizer.rs b/majit/majit-metainterp/src/optimizeopt/optimizer.rs index 24bacbccec7..94c622abbbd 100644 --- a/majit/majit-metainterp/src/optimizeopt/optimizer.rs +++ b/majit/majit-metainterp/src/optimizeopt/optimizer.rs @@ -4493,6 +4493,7 @@ impl Optimizer { for &pp_idx in postprocess_passes.iter().rev() { self.passes[pp_idx].propagate_postprocess(&op, ctx); } + self.drain_pending_finish_guard_postprocess(ctx); return Ok(()); } OptimizationResult::Replace(op) => { @@ -4551,6 +4552,7 @@ impl Optimizer { for &pp_idx in postprocess_passes.iter().rev() { self.passes[pp_idx].propagate_postprocess(¤t_op, ctx); } + self.drain_pending_finish_guard_postprocess(ctx); return Ok(()); } OptimizationResult::Remove => { @@ -4588,6 +4590,10 @@ impl Optimizer { for &pp_idx in postprocess_passes.iter().rev() { self.passes[pp_idx].propagate_postprocess(¤t_op, ctx); } + // The FINISH reaches its emit down this path — `optimize_FINISH` + // returns PassOn — so this is the drain that postprocess_FINISH + // actually goes through. + self.drain_pending_finish_guard_postprocess(ctx); Ok(()) } @@ -5205,6 +5211,45 @@ impl Optimizer { new_idx } + /// virtualize.py:84-90 postprocess_FINISH, Optimizer half. + /// + /// `OptVirtualize::propagate_postprocess` stashed the GUARD_NOT_FORCED_2 it + /// took off the FINISH; the finalization needs `store_final_boxes_in_guard` + /// and the knowledge `collect_optimizer_knowledge_for_resume` gathers, both + /// of which live here. Running after the FINISH's own emit is the point: + /// that emit force_box'd the FINISH args, so a return box that was virtual + /// is materialized by the time it is numbered. + /// + /// The guard goes straight into `new_operations` rather than back through + /// the pass chain, matching `_newoperations.insert` — it has already been + /// through every pass once, on its way to being stashed. + fn drain_pending_finish_guard_postprocess(&mut self, ctx: &mut OptContext) { + let Some(guard_op) = ctx.pending_finish_guard_postprocess.take() else { + return; + }; + // virtualize.py:87 store_final_boxes_in_guard(guard_op, []) — the + // pendingfields argument is the empty list. + let knowledge_for_resume = self.collect_optimizer_knowledge_for_resume(ctx); + let knowledge = if knowledge_for_resume.is_empty() { + None + } else { + Some(knowledge_for_resume) + }; + let guard_op = Self::store_final_boxes_in_guard(guard_op, ctx, knowledge, Vec::new()); + // virtualize.py:88-90 `i = len(_newoperations) - 1; assert i >= 0; + // insert(i, guard_op)` — the FINISH this postprocess belongs to is the + // last element, so the guard lands immediately in front of it. + let Some(i) = ctx.new_operations.len().checked_sub(1) else { + debug_assert!(false, "virtualize.py:89 assert i >= 0"); + return; + }; + ctx.new_operations.insert(i, std::rc::Rc::new(guard_op)); + // `new_operations_index` maps position -> op with last-occurrence-wins + // semantics, which an insert in the middle cannot maintain + // incrementally. + ctx.rebuild_new_operations_index(); + } + fn collect_optimizer_knowledge_for_resume( &mut self, ctx: &mut OptContext, diff --git a/majit/majit-metainterp/src/optimizeopt/virtualize.rs b/majit/majit-metainterp/src/optimizeopt/virtualize.rs index b2a0052be57..e046cd1ed9b 100644 --- a/majit/majit-metainterp/src/optimizeopt/virtualize.rs +++ b/majit/majit-metainterp/src/optimizeopt/virtualize.rs @@ -2099,44 +2099,23 @@ impl Optimization for OptVirtualize { // assert i >= 0 // self.optimizer._newoperations.insert(i, guard_op) // - // majit ordering: upstream INSERTS because its postprocess runs - // after the FINISH is already appended. This pass runs BEFORE the - // FINISH reaches the terminal emit, so `emit_extra` queues the - // stashed guard for the passes after virtualize and - // `drain_extra_operations_from` (called right after this method - // returns) flushes it through the pipeline first. The guard lands - // in `new_operations` first, the FINISH second — the same final - // op order. + // The stash here is only half the port: the guard has to be + // finalized and inserted AFTER the FINISH is emitted, because + // `emit(op)` force_box's the FINISH args and + // `store_final_boxes_in_guard` has to see a return box that was + // virtual as already materialized. Finalizing it on the way + // through the pipeline instead would encode that same box as still + // virtual — a consistent image, but not upstream's image. // - // The RESUME DATA is where the two diverge. Upstream finalizes the - // guard in `postprocess_FINISH`, i.e. after `emit(op)` forced the - // FINISH args, so `store_final_boxes_in_guard` sees a return box - // that was virtual as already materialized. Here the guard is - // finalized on the way through the pipeline, before that forcing, - // and encodes the same box as still virtual. Both are consistent - // images, but they are not the same image. - // - // BLOCKER for the faithful order. `propagate_postprocess` (the - // port of optimizer.py's postprocess dispatch) is a method on a - // PASS, and the finalization a guard needs is - // `Optimizer::store_final_boxes_in_guard` with the knowledge - // `collect_optimizer_knowledge_for_resume(&self)` gathers — which - // needs the Optimizer, not a pass. Running it from here with no - // knowledge would drop the bridgeopt sections that - // `serialize_optimizer_knowledge` puts in every other guard, buying - // one ordering divergence with a worse one. Reaching upstream's - // shape needs an Optimizer-side FINISH postprocess that can insert - // at `new_operations.len() - 1` after its own emit. - // - // Nothing arms the token today — the portal-return - // `gen_store_back_in_vable` sets `forced_virtualizable`, so - // `store_token_in_vable` early-returns and no `GUARD_NOT_FORCED_2` - // reaches a FINISH — so neither image is currently observable. + // `propagate_postprocess` below runs at the right moment but is a + // method on a PASS, and both the finalization and the + // `collect_optimizer_knowledge_for_resume` that feeds it are + // Optimizer-side. So it hands the guard to the Optimizer through + // `ctx.pending_finish_guard_postprocess`, the same shape + // `pending_guard_class_postprocess` uses, and + // `drain_pending_finish_guard_postprocess` does the insert. OpCode::Finish => { self.finish_guard_op = self.last_guard_not_forced_2.take(); - if let Some(guard_op) = self.finish_guard_op.clone() { - ctx.emit_extra(ctx.current_pass_idx, guard_op); - } OptimizationResult::PassOn } @@ -2291,6 +2270,36 @@ impl Optimization for OptVirtualize { self.finish_guard_op = None; } + // virtualize.py:84-90 postprocess_FINISH + // + // def postprocess_FINISH(self, op): + // guard_op = self._finish_guard_op + // if guard_op is not None: + // guard_op = self.optimizer.store_final_boxes_in_guard(guard_op, []) + // i = len(self.optimizer._newoperations) - 1 + // assert i >= 0 + // self.optimizer._newoperations.insert(i, guard_op) + // + // The two Optimizer-side halves are in + // `Optimizer::drain_pending_finish_guard_postprocess`, which this hands + // the guard to. + fn have_postprocess_op(&self, opcode: OpCode) -> bool { + matches!(opcode, OpCode::Finish) + } + + fn propagate_postprocess(&mut self, op: &Op, ctx: &mut OptContext) { + if op.opcode != OpCode::Finish { + return; + } + if let Some(guard_op) = self.finish_guard_op.take() { + debug_assert!( + ctx.pending_finish_guard_postprocess.is_none(), + "postprocess_FINISH queued multiple guards" + ); + ctx.pending_finish_guard_postprocess = Some(guard_op); + } + } + fn name(&self) -> &'static str { "virtualize" } diff --git a/pyre/bench/frame_lineno_mid_replay_regression.py b/pyre/bench/frame_lineno_mid_replay_regression.py index f2aaa50f453..7ce48c8b9aa 100644 --- a/pyre/bench/frame_lineno_mid_replay_regression.py +++ b/pyre/bench/frame_lineno_mid_replay_regression.py @@ -15,20 +15,19 @@ # answer and the replayed one together and a divergence appears as a SECOND # element rather than a shifted single value. # -# Why this is not a synthetic bench: the wasm backend does not satisfy the -# invariant today, and check.py's synthetic suite has no per-backend scoping. -# Measured on this HEAD -- `plain` below reports `[0, 4]` on wasm against `[4]` -# on pypy3, CPython, dynasm and cranelift, with the divergence appearing from -# the first COMPILED call onward and persisting. Instrumenting both ends showed -# the marker hook writing the right coordinate into the right frame at the right -# offset and reading it back intact, and the interpreter then reading 0 from -# that same address -- so a wasm-side writer clears it between the publish and -# the residual `sys._getframe`. Neither `restore_resume_state_from` nor -# `set_last_instr_from_next_instr` is that writer (probed: neither ever targets -# the caller frame), and a blackhole `setfield_vable_i` cannot be, because the -# wasm blackhole builder carries no cpu and that handler would panic. The -# invariant is asserted here for the native backends while that is open; the -# post-return coordinate, which wasm does satisfy, stays in the synthetic bench. +# It asserts rather than diffing against pypy3 so the recursive case can pin an +# exact per-level tuple, which an output diff cannot express. +# +# It is also the guard for a store-width defect, which is why it is worth +# keeping separate: `PyFrame.valuestackdepth` and `PyFrame.last_instr` are +# adjacent machine words, so a store that takes its width from a descr +# declaring a fixed 8 bytes runs past the first field and deposits the zero +# high half onto the second. On a 64-bit target the two are the same number +# and nothing happens; on wasm32 the words are 4 bytes and `plain` below +# reported `[0, 4]` against `[4]` everywhere else, from the first compiled call +# onward. The publish itself was never at fault -- it stores through +# `*mut isize` and read back intact; the clobber came afterwards, from the +# blackhole replaying a `setfield_vable_i` at the neighbouring offset. import sys N = 4000 diff --git a/pyre/check.py b/pyre/check.py index 492e17fdb0f..63e93736e9d 100644 --- a/pyre/check.py +++ b/pyre/check.py @@ -1806,16 +1806,13 @@ def main(): # The coordinate a frame reports WHILE it is still running: compiled code # runs no per-opcode `last_instr` store, so a replayed frame answers for # the instruction it is on only if the blackhole publishes at the - # `-live-` marker. Skipped on wasm, which does not satisfy it today -- - # the guard's own header carries the measurement (the publish lands and - # reads back, and a wasm-side writer clears it before the residual - # `sys._getframe`). The post-return coordinate, which wasm does satisfy, - # stays in the synthetic bench (exception_traceback_frame_lineno). + # `-live-` marker. Runs on every backend -- it is also the guard that + # catches a store whose width overruns `valuestackdepth` onto the + # `last_instr` next to it, which is a 32-bit-only failure. chk.run_selfcheck( "frame_lineno_mid_replay", f"{B}/frame_lineno_mid_replay_regression.py", 20, - skip_backends=("wasm",), ) # The branchy-inlined-callee guard (gh#343) lives in the synthetic parity # suite as bridge_branchy_callee.py, gated against pypy by diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index 2b729cb5636..3e47645e564 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -1360,7 +1360,13 @@ static PYFRAME_DESCR_GROUP: LazyLock = LazyLock::new(|| { ( "valuestackdepth", crate::frame_layout::PYFRAME_VALUESTACKDEPTH_OFFSET, - 8, + // `usize`, not a fixed 64-bit int. The other `Type::Int` + // fields in this table are all `i64`, so 8 is right for + // them; this one and `last_instr` below are the two that + // are a machine word wide. A literal 8 makes the store a + // byte pair too wide on a 32-bit target, and the overrun + // lands on `last_instr`, which sits immediately after it. + std::mem::size_of::(), Type::Int, true, false, @@ -1369,7 +1375,8 @@ static PYFRAME_DESCR_GROUP: LazyLock = LazyLock::new(|| { ( "last_instr", crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET, - 8, + // `isize` — see the width note on `valuestackdepth` above. + std::mem::size_of::(), Type::Int, true, false, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs index cb785c158a4..3be52ce1aca 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -1382,11 +1382,15 @@ pub(crate) fn fbw_terminate_with_finish( /// /// A frame the function-entry portal compiled can outlive its trace the same /// way a generator's does — a traceback it hands out keeps it alive — and the -/// lazy route is not available to narrow this back down: the backend frees the -/// jitframe chain before `execute_token` returns, so that marker would name -/// freed memory rather than a retained deadframe. Narrowing the force to the -/// frames that actually escape needs that retention first; the escape is a -/// runtime property, which is exactly what the token protocol exists to answer. +/// lazy route is not available to narrow this back down. Two things have to +/// land before it is: the backend frees the jitframe chain before +/// `execute_token` returns, so the marker would name freed memory rather than +/// a retained deadframe; and no backend arms `jf_force_descr` for a standalone +/// trailing `GUARD_NOT_FORCED_2`, which upstream does from +/// `consider_guard_not_forced_2` (x86/regalloc.py), so the armed-token test +/// would answer false for a portal exit even once the chain is retained. +/// Narrowing the force to the frames that actually escape needs both; the +/// escape is a runtime property, which is what the token protocol answers. /// /// Storing back here is what makes the token store unnecessary rather than /// merely redundant: `gen_store_back_in_vable` sets `forced_virtualizable`, and diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 0318efc9ca7..759810b9eb6 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -4854,6 +4854,43 @@ pub(crate) fn restore_frame_locals(frame: usize, saved: &[i64]) { frame_array_write_barrier(frame as *mut u8, arr_ptr); } +/// The scalar half of the frame image [`capture_frame_locals`] covers. +#[derive(Clone, Copy)] +pub(crate) struct FrameScalars { + last_instr: isize, + valuestackdepth: usize, +} + +/// Take the two frame words a blackhole drive moves as it replays. It reaches +/// them through `setfield_vable_i` against the frame register, which is the +/// live frame, so a caller that may still hand that frame back to the +/// interpreter has to take these beside the slots: the interpreter derives its +/// next opcode from `last_instr + 1`, and reads the operand stack at +/// `valuestackdepth`. Restoring only the locals leaves the two disagreeing. +pub(crate) fn capture_frame_scalars(frame: usize) -> Option { + if frame == 0 { + return None; + } + Some(FrameScalars { + last_instr: unsafe { + *((frame + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET) as *const isize) + }, + valuestackdepth: concrete_stack_depth(frame)?, + }) +} + +/// Put back what [`capture_frame_scalars`] took. +pub(crate) fn restore_frame_scalars(frame: usize, saved: FrameScalars) { + if frame == 0 { + return; + } + unsafe { + *((frame + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET) as *mut isize) = + saved.last_instr; + } + set_concrete_stack_depth(frame, saved.valuestackdepth); +} + /// Materialize the outer frame's locals from the virtualizable shadow. /// Callers must run their complete preflight before executing a rebuilt /// callee; after that point a failure would make replay unsafe. diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index 01c2c5ad840..537bfc02162 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -2029,6 +2029,7 @@ fn try_adopt_single_frame_blackhole( let Some(mut locals_undo) = crate::state::capture_frame_locals(vable_frame) else { return false; }; + let scalars_undo = crate::state::capture_frame_scalars(vable_frame); let root_depth = majit_gc::shadow_stack::resume_ref_roots_depth(); unsafe { majit_gc::shadow_stack::push_resume_ref_roots(locals_undo.as_mut_slice()); @@ -2128,8 +2129,15 @@ fn try_adopt_single_frame_blackhole( // escape/replay path, which resumes the frame from its pre-walk state — // the state `restore_escape_flush_undo` puts back for the flush half. // The publish above and the replay's own vable stores both landed here, - // so both have to come off. + // so both have to come off. The drive reached the frame's scalars + // through `setfield_vable_i`, for which no undo image exists anywhere + // else: the store journal covers heap effects and + // `fbw_exit_last_instr_rollback` only arms on a return or void-return + // exit, which an unadoptable terminal is not. crate::state::restore_frame_locals(vable_frame, &locals_undo); + if let Some(scalars) = scalars_undo { + crate::state::restore_frame_scalars(vable_frame, scalars); + } } majit_gc::shadow_stack::pop_resume_ref_roots_to(root_depth); adopted @@ -2349,6 +2357,7 @@ fn try_adopt_multi_frame_blackhole( restore_links(&saved_links); return false; }; + let scalars_undo = crate::state::capture_frame_scalars(root_addr); let undo_depth = majit_gc::shadow_stack::resume_ref_roots_depth(); unsafe { majit_gc::shadow_stack::push_resume_ref_roots(locals_undo.as_mut_slice()); @@ -2519,8 +2528,12 @@ fn try_adopt_multi_frame_blackhole( } else { // Same withdrawal as the single-frame arm: an unadoptable terminal // returns to legacy escape/replay, which resumes the walked frame from - // its pre-walk state. The chain the drive was given comes down with it. + // its pre-walk state. The chain the drive was given comes down with it, + // and so do the frame scalars the drive moved. crate::state::restore_frame_locals(root_addr, &locals_undo); + if let Some(scalars) = scalars_undo { + crate::state::restore_frame_scalars(root_addr, scalars); + } restore_links(&saved_links); } majit_gc::shadow_stack::pop_resume_ref_roots_to(undo_depth); diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index d8c5360a315..60c5648a830 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -7846,12 +7846,14 @@ impl CodeWriter { // hundred instructions. Convergence path: a value operand that // encodes the immediate inline instead of through the per-kind // pool, after which this becomes an unconditional per-PC store. - // Until then the coordinate is published only where the frame - // stops being replayed — the frame exits (`ReturnValue`, - // `emit_abort_permanent!`) and the raises that resume in the - // interpreter — so a frame observed MID-replay (via a callee's - // `sys._getframe` or traceback) still reports the last published - // coordinate. + // Until then the jitcode carries the store only at the frame + // exits (`ReturnValue`, `emit_abort_permanent!`) and the raises + // that resume in the interpreter. A frame observed MID-replay + // — through a callee's `sys._getframe` or a traceback — is + // answered instead by the blackhole, which publishes the + // coordinate at each `-live-` marker it passes; the levels that + // still go unpublished are the inlined non-portal callees, + // whose `frame_var` aliases the outermost frame. // pyframe.py: valuestackdepth is written per-push/per-pop // via setfield_vable_i (jtransform.py), NOT once at opcode // entry. The per-push/per-pop emit_vsd! calls below mirror that.