diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index a93eeff0373..ad411437fd4 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -5190,14 +5190,40 @@ fn bhimpl_jit_leave_portal_frame() {} /// blackhole.py:1547-1548 `bhimpl_hint_force_virtualizable(r): pass`. fn bhimpl_hint_force_virtualizable(_r: i64) {} +/// Called at every `-live-` marker, i.e. once per source-level instruction the +/// blackhole replays. Arguments are the interpreter and the marker's own +/// bytecode position. +/// +/// RPython's `bhimpl_live` is a plain no-op, and it can be: the frame fields +/// its interpreter writes per instruction (`dispatch_bytecode`'s +/// `self.last_instr = intmask(next_instr)`) are ordinary source-level stores, +/// so they are compiled into the jitcode and the blackhole replays them like +/// any other operation. A consumer whose jitcode cannot carry such a store — +/// because the value is a distinct compile-time constant per instruction and +/// `check_result`'s 256-entry per-kind cap rejects one pool entry per +/// instruction — registers this hook and writes the field itself. +pub type LiveMarkerHook = fn(&BlackholeInterpreter, usize); + +static LIVE_MARKER_HOOK: std::sync::OnceLock = std::sync::OnceLock::new(); + +/// Install the [`LiveMarkerHook`]. First registration wins; later calls are +/// ignored, so a consumer may call it from every driver install path. +pub fn register_live_marker_hook(hook: LiveMarkerHook) { + let _ = LIVE_MARKER_HOOK.set(hook); +} + /// Handler for `live/` — liveness marker. Argcodes: empty, but the assembler /// emits a 2-byte offset after the opcode. Skip those 2 bytes. /// RPython blackhole.py:146-158 (inside _get_method for `-live-` ops). fn handler_live( - _bh: &mut BlackholeInterpreter, + bh: &mut BlackholeInterpreter, _code: &[u8], position: usize, ) -> Result { + if let Some(hook) = LIVE_MARKER_HOOK.get() { + // `position` is past the opcode byte; the marker op starts one earlier. + hook(bh, position - 1); + } // Skip the 2-byte liveness offset (RPython: OFFSET_SIZE = 2). Ok(position + 2) } diff --git a/majit/majit-metainterp/src/optimizeopt/mod.rs b/majit/majit-metainterp/src/optimizeopt/mod.rs index 1b121c2f0bb..1ab9f8f34ea 100644 --- a/majit/majit-metainterp/src/optimizeopt/mod.rs +++ b/majit/majit-metainterp/src/optimizeopt/mod.rs @@ -581,11 +581,6 @@ pub struct OptContext { /// 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, - /// virtualize.py:84-90 postprocess_FINISH queues the stashed - /// GUARD_NOT_FORCED_2 here so the outer optimizer can insert it at - /// `len(_newoperations) - 1` with full `store_final_boxes_in_guard` - /// semantics. - pub(crate) pending_finish_guard_postprocess: Option, // ptr_info merged into forwarded (Forwarded::Info variant) // // RPython parity: per-OpRef IntBound storage lives ENTIRELY on @@ -1664,7 +1659,6 @@ 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(), @@ -2270,7 +2264,6 @@ 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/virtualize.rs b/majit/majit-metainterp/src/optimizeopt/virtualize.rs index 11ff75de4ce..c80fd8d995d 100644 --- a/majit/majit-metainterp/src/optimizeopt/virtualize.rs +++ b/majit/majit-metainterp/src/optimizeopt/virtualize.rs @@ -2084,7 +2084,7 @@ impl Optimization for OptVirtualize { OptimizationResult::PassOn } - // virtualize.py:80-90 optimize_FINISH / postprocess_FINISH + // virtualize.py optimize_FINISH / postprocess_FINISH // // def optimize_FINISH(self, op): // self._finish_guard_op = self._last_guard_not_forced_2 @@ -2099,25 +2099,44 @@ impl Optimization for OptVirtualize { // assert i >= 0 // self.optimizer._newoperations.insert(i, guard_op) // - // majit ordering: emit_extra queues the stashed guard for the - // passes after virtualize, and `drain_extra_operations_from` - // (called by propagate_from_pass_range right after this method - // returns) flushes those queued ops through the pipeline before - // the FINISH replacement is propagated. The guard therefore lands - // in `new_operations` first, the FINISH lands second — matching - // RPython's "insert at len-1" final layout. The guard's resume - // data is finalized when `emit_guard_operation` calls - // `store_final_boxes_in_guard` during its emission. + // 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. // - // RPython parity: optimize_FINISH does NOT call the generic - // escaping-op force path here. Forcing the FINISH args in the - // virtualize pass would happen before the stashed - // GUARD_NOT_FORCED_2 is reinserted, and store_final_boxes_in_guard - // would then see the already-forced return box in vable_array. - // The actual arg forcing belongs later in Optimizer._emit_operation, - // after the queued guard has been flushed ahead of FINISH. + // 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. 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 } @@ -2272,23 +2291,6 @@ impl Optimization for OptVirtualize { self.finish_guard_op = None; } - 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 new file mode 100644 index 00000000000..f2aaa50f453 --- /dev/null +++ b/pyre/bench/frame_lineno_mid_replay_regression.py @@ -0,0 +1,139 @@ +# Self-checking regression guard for the coordinate a frame reports WHILE it is +# still running (registered via check.py run_selfcheck, NOT the synthetic suite). +# +# `dispatch_bytecode` (pyopcode.py) stamps `last_instr` before every opcode, so +# a running frame answers `f_lineno`, `f_lasti` and any traceback taken off it +# for the instruction it is on. Compiled code does not run that store, and the +# blackhole replaying a frame syncs only `valuestackdepth`, so the coordinate +# reaches the frame only if the replay publishes it at the `-live-` marker. +# Without that publish a frame the function-entry portal compiled still carries +# the `-1` initialization sentinel, which `offset2lineno` answers with the code +# object's first line -- the `def` line, i.e. offset 0. +# +# Splitting each survey across several calls is what makes it a test: the loop +# compiles part-way through, so a set over the rounds holds the interpreted +# 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. +import sys + +N = 4000 + + +def caller_offset(): + """The caller's coordinate, read from a callee while the caller is still + running. Nothing has left the caller's frame yet, so neither exit publish + has fired and the answer can only come from the frame being kept current.""" + frame = sys._getframe(1) + return frame.f_lineno - frame.f_code.co_firstlineno + + +def raises_out(i): + raise KeyError(i) + + +def plain(n): # +0 + k = 0 # +1 + while k < n: # +2 + k += 1 # +3 + return caller_offset() # +4 + + +def mid_replay_getframe(n): # +0 + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + return caller_offset() # +9 + + +def mid_replay_handler(n): # +0 + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + try: + raises_out(k) # +10 + except KeyError as e: + t = e.__traceback__ + base = t.tb_frame.f_code.co_firstlineno + return (t.tb_lineno - base, t.tb_frame.f_lineno - base) # +14 + + +def recursive_mid_replay(n, depth): + """Direct recursion, every level with its own hot loop, so every level is + replayed and every level shares ONE code object with its caller -- the shape + where a per-level frame mix-up survives a code-object check. A level + answering for another one shows up as a shifted offset.""" + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + if depth > 0: + inner = recursive_mid_replay(n, depth - 1) + else: + inner = () + return ((caller_offset(), caller_offset()),) + inner # +17 + + +def main(): + rounds = 8 + each = N // rounds + failures = [] + + def check(label, got, want): + if got != want: + failures.append(f"{label}: got {got!r}, want {want!r}") + + check("plain", sorted({plain(each) for _ in range(rounds)}), [4]) + check( + "getframe", + sorted({mid_replay_getframe(each) for _ in range(rounds)}), + [9], + ) + check( + "handler", + sorted({mid_replay_handler(each) for _ in range(rounds)}), + [(10, 14)], + ) + check( + "recursive", + recursive_mid_replay(N // 2, 3), + ((17, 17), (17, 17), (17, 17), (17, 17)), + ) + + if failures: + for f in failures: + print("FAIL", f) + return 1 + print("PASS mid-replay coordinates") + return 0 + + +sys.exit(main()) diff --git a/pyre/bench/synth/exception_traceback_frame_lineno.py b/pyre/bench/synth/exception_traceback_frame_lineno.py new file mode 100644 index 00000000000..720a31ca63b --- /dev/null +++ b/pyre/bench/synth/exception_traceback_frame_lineno.py @@ -0,0 +1,198 @@ +# A traceback that outlives its frame keeps the frame answering for the line it +# stopped on, not the line it was defined at. +# +# `f_lineno` resolves through `offset2lineno(pycode, last_instr)`, and compiled +# code does not run the interpreter's per-opcode `last_instr` store, so the +# value only reaches the frame if the trace publishes it. A frame the +# function-entry portal compiled starts from the `-1` initialization sentinel, +# which `offset2lineno` answers with the code object's first line, so a missing +# publish reports the `def` line. +# +# Both frame exits have to publish, and the return exit is the one that is easy +# to miss: `catches_here` leaves by RETURN, not by the raise, because it keeps +# running after it catches. The two coordinates a traceback carries answer +# different questions and only one of them moves: `tb_lineno` is frozen at the +# raise site when the node is built, while `f_lineno` is read off the frame on +# every access, so the `return` is the line the FRAME has to report while its +# node keeps reporting the raise. +# +# The driver decides which route compiles the callee - a `while` loop reaches it +# as a function-entry portal, a `for` loop inlines it into the loop trace - so +# the two are surveyed separately and must agree. A publish wired into only one +# route shows up as a disagreement between the two lines without the oracle +# having to say anything. The `loop_owner_*` group covers the third route: a +# frame whose own `while` IS the compiled loop leaves through a guard failure, +# and the replay that finishes it has to reach the same `return` coordinate. +# +# Surveying every iteration rather than sampling the last one is what catches a +# frame that is only sometimes right: the pre-compile iterations are correct, so +# a miss appears as a SECOND tuple in the shape set. +# +# Both frame exits publishing is not enough on its own: a frame can also be +# READ while it is still running. That half lives in +# `pyre/bench/frame_lineno_mid_replay_regression.py` instead of here, because +# the wasm backend does not satisfy it yet and the synthetic suite has no +# per-backend scoping; the guard carries the measurement. +# +# Offsets run from `co_firstlineno` so edits above these functions do not move +# the expected values. +import sys + +N = 4000 + + +def chain(traceback): + out = [] + while traceback is not None: + frame = traceback.tb_frame + base = frame.f_code.co_firstlineno + out.append( + ( + frame.f_code.co_name, + traceback.tb_lineno - base, + frame.f_lineno - base, + ) + ) + traceback = traceback.tb_next + return tuple(out) + + +def catches_here(i): + try: + raise ValueError(i) + except ValueError as e: + return e.__traceback__ + + +def raises_out(i): + raise KeyError(i) + + +def catches_callee(i): + try: + raises_out(i) + except KeyError as e: + return e.__traceback__ + + +def while_same(): + seen = set() + k = 0 + while k < N: + seen.add(chain(catches_here(k))) + k += 1 + return sorted(seen) + + +def for_same(): + seen = set() + for k in range(N): + seen.add(chain(catches_here(k))) + return sorted(seen) + + +def while_callee(): + seen = set() + k = 0 + while k < N: + seen.add(chain(catches_callee(k))) + k += 1 + return sorted(seen) + + +def for_callee(): + seen = set() + for k in range(N): + seen.add(chain(catches_callee(k))) + return sorted(seen) + + +def loop_owner_return(n): + """The frame that raises OWNS the compiled loop, so it never goes through + the function-entry portal: the loop guard fails and the rest of the frame + is replayed from the guard's resume image. Each arm puts a different + amount of work between the last iteration and the `return`, so the reported + offset says which coordinate the frame is stuck on — the raise inside the + body, the loop exit, or the `return` it actually reached.""" + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + return tb + + +def loop_owner_stmt(n): + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + j = k + return tb + + +def loop_owner_call(n): + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + catches_here(k) + return tb + + +def loop_owner_second_loop(n): + tb = None + k = 0 + while k < n: + try: + raise ValueError(k) + except ValueError as e: + tb = e.__traceback__ + k += 1 + m = 0 + while m < 3: + m += 1 + return tb + + +def loop_owners(): + return [chain(fn(N)) for fn in ( + loop_owner_return, + loop_owner_stmt, + loop_owner_call, + loop_owner_second_loop, + )] + + +def kept_alive(): + """Many tracebacks alive at once: a shared or recycled frame collapses.""" + kept = [] + k = 0 + while k < N: + kept.append(catches_here(k)) + k += 1 + shapes = sorted({chain(t) for t in kept}) + distinct = len({id(t.tb_frame) for t in kept}) == len(kept) + return shapes, distinct + + +print("while/same ", while_same()) +print("for/same ", for_same()) +print("while/callee", while_callee()) +print("for/callee ", for_callee()) +for shape in loop_owners(): + print("loop_owner ", shape) +kept_shapes, kept_distinct = kept_alive() +print("kept ", kept_shapes) +print("kept_distinct", kept_distinct) diff --git a/pyre/check.py b/pyre/check.py index 73db23a8400..492e17fdb0f 100644 --- a/pyre/check.py +++ b/pyre/check.py @@ -1803,6 +1803,20 @@ def main(): f"{B}/getframe_escape_flush_writethrough_regression.py", 15, ) + # 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). + 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 # `# pyre-check: max-pypy-ratio`; a decline that keeps every crossing 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 3d1c0401943..69fec905983 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -157,6 +157,25 @@ thread_local! { static FBW_FINISH_CONCRETE: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// `(frame, last_instr_before)` for the concrete half of + /// [`fbw_publish_exit_last_instr`], so a walk that does not commit can put + /// the field back. + /// + /// The publish fires when the walk reaches the exit; whether that exit is + /// kept is decided afterwards, in the walk-end epilogue. A declined walk + /// returns to a replay that resumes the frame from the pre-walk state, and + /// `last_instr` is exactly what that resume reads + /// (`PyFrame::next_instr` = `last_instr + 1`), so an exit coordinate left + /// behind would restart the frame PAST its own return or raise. The + /// recorded `setfield_vable_i` half needs no undo — it only reaches a frame + /// on a compiled run. + /// + /// Only the first publish of a walk is recorded, so the restore targets the + /// value the frame carried when the walk began rather than an intermediate + /// one. Cleared with the store journal at the start of every walk. + static FBW_EXIT_LAST_INSTR_UNDO: std::cell::Cell> = + const { std::cell::Cell::new(None) }; + /// Armed by the bridge tracer (`call_jit::trace_and_compile_from_bridge`) /// before a single-frame, direct-return-capable guard-failure walk. When /// set, the `run_perfn_walk` epilogue lets a bridge `Terminate` walk keep @@ -359,6 +378,29 @@ pub(crate) fn fbw_store_journal_reset() { // unrelated POP_EXCEPT in this walk. FBW_EXC_PREV.with(|s| s.borrow_mut().clear()); FBW_EXC_PENDING_PUSH_SET.with(|c| c.set(false)); + FBW_EXIT_LAST_INSTR_UNDO.with(|c| c.set(None)); +} + +/// Put `last_instr` back for a walk that did not commit its end state, so the +/// replay resumes where the frame stood before the walk. Runs beside +/// [`fbw_store_journal_rollback`] on every non-committed exit; the commit side +/// just drops the undo ([`fbw_exit_last_instr_commit`]). +pub(crate) fn fbw_exit_last_instr_rollback() { + let Some((frame, before)) = FBW_EXIT_LAST_INSTR_UNDO.with(|c| c.take()) else { + return; + }; + // SAFETY: the frame the publish wrote is the walk's live recording frame, + // which outlives the walk, and `frame_layout` pins `last_instr` to this + // offset with a compile-time assertion against the interpreter's constant. + unsafe { + *((frame + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET) as *mut isize) = before; + } +} + +/// Drop the undo: the walk's end state is kept, so the published exit +/// coordinate is the one the frame should carry. +pub(crate) fn fbw_exit_last_instr_commit() { + FBW_EXIT_LAST_INSTR_UNDO.with(|c| c.set(None)); } /// Record the element a walked eager list store displaces, for rollback @@ -1301,8 +1343,10 @@ pub(crate) fn fbw_store_token_in_vable( /// Shared top-level finish path for the three value-returning arms /// (`ref_return` / `int_return` / `float_return`). Re-boxes `result` to -/// `Type::Ref`, records the vable store-back + `GUARD_NOT_FORCED_2`, and -/// stashes the finish payload for `full_body_walk_trace`. Deliberately +/// `Type::Ref`, publishes the return coordinate into `last_instr`, stores the +/// virtualizable back into the frame, and stashes the finish payload for +/// `full_body_walk_trace`. The store-back is what leaves `vable_token` clear, +/// so the `GUARD_NOT_FORCED_2` arming below it declines. Deliberately /// does NOT record the `FINISH` op: under the gate the compile consumer /// (`finish_and_compile` -> `recorder.finish`, mod.rs) records it from /// `finish_args`, so recording it here too would double it. @@ -1312,15 +1356,53 @@ pub(crate) fn fbw_terminate_with_finish( op_pc: usize, ) -> Result<(), DispatchError> { let finish_value = fbw_ensure_boxed_for_ca(ctx, op_pc, result)?; + fbw_publish_exit_last_instr(ctx, op_pc); + fbw_force_virtualizable_before_return(ctx); fbw_store_token_in_vable(ctx, op_pc)?; FBW_FINISH_PAYLOAD.with(|c| c.set(Some((finish_value, Type::Ref)))); Ok(()) } +/// `jit.hint(frame, force_virtualizable=True)` on the way out of the portal +/// (`opimpl_hint_force_virtualizable` → `gen_store_back_in_vable`, pyjitpl.py). +/// +/// `doc/jit/virtualizable.rst` names this as the remedy for exactly this shape: +/// "If you have something equivalent of a Python generator, where the +/// virtualizable survives for longer, you want to force it before returning. +/// It's better to do it that way than by an external call some time later." +/// +/// Upstream applies it to ONE exit — `interp_jit.py` `PyFrame.dispatch` reads +/// `except Yield: … jit.hint(self, force_virtualizable=True)` against a bare +/// `except Return: return self.popvalue()`. A generator frame is the only one +/// that outlives its dispatch there; every other frame is answered lazily, +/// through the marker `store_token_in_vable` leaves behind and the deadframe it +/// names. So this fires on an exit upstream leaves alone, and the ordinary +/// return gives up the `FORCE_TOKEN`/`GUARD_NOT_FORCED_2` protocol for an +/// unconditional store-back. +/// +/// 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. +/// +/// Storing back here is what makes the token store unnecessary rather than +/// merely redundant: `gen_store_back_in_vable` sets `forced_virtualizable`, and +/// `store_token_in_vable` returns early on that (pyjitpl.py) — the two are +/// alternatives, not a sequence — and its final store zeroes the token slot. +fn fbw_force_virtualizable_before_return(ctx: &mut WalkContext<'_, '_, Sym>) { + let Some(vbox) = ctx.trace_ctx.standard_virtualizable_box() else { + return; + }; + ctx.trace_ctx.gen_store_back_in_vable(vbox); +} + /// Void variant of [`fbw_terminate_with_finish`] for the top-level /// `void_return/` portal exit (`compile_done_with_this_frame`'s VOID -/// branch, pyjitpl.py). Records the vable store-back + -/// `GUARD_NOT_FORCED_2`, then stashes a `Type::Void`-marked payload so +/// branch, pyjitpl.py). Publishes the return coordinate and stores the +/// virtualizable back the same way, then stashes a `Type::Void`-marked payload so /// [`crate::trace::full_body_walk_trace`] builds a `TraceAction::Finish` /// with no args (`done_with_this_frame_descr_from_types(&[])` resolves the /// void descr). Like the value path it does NOT record the `FINISH` op — @@ -1329,40 +1411,49 @@ pub(crate) fn fbw_terminate_void_with_finish( ctx: &mut WalkContext<'_, '_, Sym>, op_pc: usize, ) -> Result<(), DispatchError> { + fbw_publish_exit_last_instr(ctx, op_pc); + fbw_force_virtualizable_before_return(ctx); fbw_store_token_in_vable(ctx, op_pc)?; FBW_FINISH_PAYLOAD.with(|c| c.set(Some((OpRef::NONE, Type::Void)))); Ok(()) } -/// Publish the raising instruction's Python coordinate into the standard -/// virtualizable's `last_instr` slot before the top-level frame exits with -/// an exception. +/// Publish the exiting instruction's Python coordinate into the standard +/// virtualizable's `last_instr` slot before the top-level frame exits. /// -/// `handle_exception` (pyre-interpreter) stamps this frame's traceback node -/// with `frame.last_instr` and keys the exception-table lookup on the same -/// field, so the value has to be the coordinate the raise actually happened -/// at. Compiled code never runs the per-opcode interpreter store: a frame +/// Compiled code never runs the per-opcode interpreter store, so a frame /// entered through the function-entry portal still carries the `-1` -/// initialization sentinel, which `offset2lineno` answers with the code -/// object's first line — the `def` line — and a loop-entry frame carries the -/// loop header. In both shapes the node comes out stamped with a line the -/// frame was not executing. +/// initialization sentinel and a loop-entry frame carries the loop header. +/// `offset2lineno` answers `-1` with the code object's first line — the `def` +/// line — so whatever reads the field afterwards reports a line the frame was +/// not executing. /// -/// Upstream never faces this: `handle_operation_error` runs INSIDE the traced -/// portal, so the node is recorded from the vable's own `last_instr` box -/// (`pyopcode.py:147-148`), which every opcode writes. pyre records the -/// top-level frame's node from the interpreter instead — the exception -/// surfaces there as `exit_frame_with_exception` — so the field it reads has -/// to be published before the trace finishes. +/// Both frame exits publish, because both leave a reader behind. On the +/// uncaught raise, `handle_exception` (pyre-interpreter) stamps the traceback +/// node from `frame.last_instr` and keys the exception-table lookup on the +/// same field. On the normal return, a traceback the frame handed out +/// outlives it, and `tb_frame.f_lineno` resolves through `offset2lineno` on +/// this field — the return is the last coordinate the frame ever reached. +/// +/// Upstream never faces either shape: the portal is entered only from a +/// backward jump (`can_enter_jit`, `interp_jit.py`), so a loop-free function +/// is never compiled as one, and the frame's `dispatch` loop runs inside the +/// traced portal where every opcode writes `last_instr` (`pyopcode.py`). +/// pyre's function-entry portal reaches the field from the interpreter after +/// the trace has finished, so the coordinate has to be published before it +/// does. /// /// The store is the static-field shape `gen_store_back_in_vable` emits for /// this slot, so it reaches the frame on a compiled run; the shadow mirror /// keeps the walker's own virtualizable view in step with it. -pub(crate) fn fbw_publish_raise_last_instr( +pub(crate) fn fbw_publish_exit_last_instr( ctx: &mut WalkContext<'_, '_, Sym>, opcode_position: usize, ) { - let jitcode_index = ctx.session.borrow().recording_jitcode_index; + let (recording_frame_ptr, jitcode_index) = { + let session = ctx.session.borrow(); + (session.recording_frame_ptr, session.recording_jitcode_index) + }; let Some(py_pc) = crate::state::python_pc_for_jitcode_pc_public(jitcode_index, opcode_position as i32) else { @@ -1386,6 +1477,33 @@ pub(crate) fn fbw_publish_raise_last_instr( value, Value::Int(i64::from(py_pc)), ); + // The recorded store has to have a concrete counterpart. Upstream's + // tracing IS the interpreter, so its per-opcode `last_instr` write + // (`pyopcode.py`) lands in the real frame on the very iteration the trace + // is recorded from; the walker only records ops, so without this the + // recording iteration is the one iteration that still reports the stale + // sentinel — a single wrong answer in the middle of a survey. + // `recording_frame_ptr` is the LIVE frame, not `virtualizable_heap_ptr`'s + // trace-stepping snapshot: the snapshot's storage is released when tracing + // ends, so a store there reaches nothing the interpreter goes on to read. + // Unlike the recorded store, this one lands whether or not the walk goes on + // to commit, so it is journaled: a declined walk resumes the frame from its + // pre-walk state and reads this very field to find the next instruction. + if recording_frame_ptr != 0 { + // SAFETY: the recording frame is the live `PyFrame` this walk steps, + // and `frame_layout` pins `last_instr` to this offset with a + // compile-time assertion against the interpreter's own constant. + let slot = + (recording_frame_ptr + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET) as *mut isize; + FBW_EXIT_LAST_INSTR_UNDO.with(|c| { + if c.get().is_none() { + c.set(Some((recording_frame_ptr, unsafe { *slot }))); + } + }); + unsafe { + *slot = py_pc as isize; + } + } } /// Exception variant of [`fbw_terminate_with_finish`] for the top-level diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 4d48cae943c..f43da787a67 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -2355,7 +2355,7 @@ pub fn walk( // when the trace hands it the exception, and it reads the // raise coordinate out of `frame.last_instr`. Compiled // code never wrote that field, so publish it here. - fbw_publish_raise_last_instr(ctx, recording_opcode_position); + fbw_publish_exit_last_instr(ctx, recording_opcode_position); // RPython parity: framestack exhausted with no handler // match → `compile_exit_frame_with_exception(last_exc_box)`. // Stash the exception the same way the value-return arms diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 7fae37ac916..131d5459d0c 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -820,6 +820,89 @@ pub fn python_pc_for_jitcode_pc_public(jitcode_index: i32, offset: i32) -> Optio ) } +/// `majit_metainterp::blackhole::LiveMarkerHook` implementation: stamp the +/// instruction the blackhole is about to replay into the frame's `last_instr`. +/// +/// `dispatch_bytecode` (pyopcode.py) writes `self.last_instr = intmask( +/// next_instr)` before every opcode, and the interpreter's own loop +/// (`eval.rs::eval_loop`) mirrors it, so anything that reads the frame while it +/// is running — `f_lineno`, `f_lasti`, a traceback node, the exception-table +/// lookup — sees the instruction actually executing. Upstream gets the same +/// invariant during blackhole replay for free, because that write is a +/// source-level store its codewriter lowers into the jitcode. pyre's codewriter +/// cannot: it unrolls the bytecode per PC, so the store would need one distinct +/// int pool constant per instruction against `check_result`'s 256-entry cap. +/// Publishing here costs no jitcode, no recorded operation and no pool entry, +/// and only the replay pays for it. +/// +/// The frame is the one THIS level reads and writes — the portal red the +/// codewriter threads through every `getarrayitem_vable_r`, taken from this +/// level's own register bank. `virtualizable_ptr` is deliberately not a +/// fallback: a nested level that carries no frame of its own would resolve it +/// to the level ABOVE, and direct recursion would then get the callee's +/// coordinate written into the caller's frame, past the code-object check. +/// That check stays as the second half of the same argument — the frame must +/// also be running the function this JitCode was built for. +/// +/// This runs once per replayed instruction, so it resolves everything under a +/// single store borrow and takes no reference count: the `Arc`-cloning +/// accessors (`pyjitcode_for_jitcode_index`, `pyjitcode_for_code`) each re-run +/// `ensure_finish_setup`, whose opname-map clone alone costs more than the +/// replayed instruction, and `compiled_jitcode_lookup` is a linear scan. +pub fn publish_last_instr_at_live_marker( + bh: &majit_metainterp::blackhole::BlackholeInterpreter, + marker_pc: usize, +) { + // A JitCode only exists once `finish_setup` has run, so the store is read + // directly here rather than through `ensure_finish_setup`. A borrow already + // held (a reentrant walker path) skips the publish rather than panicking. + METAINTERP_SD.with(|r| { + let Ok(sd) = r.try_borrow() else { + return; + }; + let Some(jitcode) = sd.jitcodes.get(bh.jitcode.index()) else { + return; + }; + let metadata = &jitcode.payload.metadata; + let frame = match bh + .registers_r + .get(metadata.portal_frame_reg as usize) + .copied() + { + Some(value) if value > 0 => value as usize, + _ => return, + }; + // SAFETY: the portal red holds the concrete `PyFrame` the blackhole + // runs against, and `frame_layout` pins `pycode` to this offset with a + // compile-time assertion against the interpreter's own constant. + let w_code = + unsafe { *((frame + crate::frame_layout::PYFRAME_PYCODE_OFFSET) as *const *const ()) }; + // A non-standard virtualizable frame from a bridge sub-walk carries the + // `GcRef(usize::MAX)` sentinel (or null) here instead of a real + // `PyCode`. `w_code_get_ptr` requires a valid code object, so the null + // and sentinel tests run first, and `is_code` before the deref for the + // same reason its other callers order them that way — `py_type_check` + // would itself dereference a raw sentinel. + if w_code.is_null() || w_code as usize == usize::MAX { + return; + } + if !unsafe { pyre_interpreter::pycode::is_code(w_code as PyObjectRef) } { + return; + } + let raw_code = unsafe { pyre_interpreter::w_code_get_ptr(w_code as PyObjectRef) }; + if raw_code as *const CodeObject != jitcode.payload.code_ptr { + return; + } + let py_pc = crate::jitcode_dispatch::python_pc_for_jitcode_pc(metadata, marker_pc); + // SAFETY: same frame, and `last_instr` carries the same compile-time + // offset assertion. + unsafe { + *((frame + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET) as *mut isize) = + py_pc as isize; + } + }); +} + /// Whether a JitCode exception exit came from the Python bare-reraise /// instruction path. `RAISE_VARARGS 0` and `RERAISE` both use /// RaiseWithExplicitTraceback and skip record_application_traceback. @@ -4689,6 +4772,58 @@ pub(crate) fn can_flush_walk_end_state_after_outer_call( !arr_ptr.is_null() && unsafe { &*arr_ptr }.as_slice().len() >= nlocals + below.len() + 1 } +/// Take a copy of `frame`'s locals so a caller that publishes over them can put +/// the frame back exactly as it found it. Returned as raw words: boxing a slot +/// during the publish can collect, so the copy has to be registered as resume +/// roots (`push_resume_ref_roots`) for the duration, like the register image +/// `apply_blackhole_crn` holds. +/// +/// Upstream needs no counterpart. `resume.py`'s virtualizable write-back +/// (`VirtualizableInfo.write_from_resume_data`) runs on a per-call `MIFrame` +/// whose values RPython's GC sees through ordinary object references, so the +/// question of publishing over a live frame and taking it back never arises; +/// carrying the old slots as raw words, and rooting them by hand, is what +/// replaces that ownership. +pub(crate) fn capture_frame_locals(frame: usize) -> Option> { + if frame == 0 { + return None; + } + let nlocals = concrete_nlocals(frame)?; + let arr_ptr = unsafe { + *((frame as *const u8).add(PYFRAME_LOCALS_CELLS_STACK_OFFSET) + as *const *mut pyre_object::FixedObjectArray) + }; + if arr_ptr.is_null() { + return None; + } + let slots = unsafe { &*arr_ptr }.as_slice(); + if slots.len() < nlocals { + return None; + } + Some(slots[..nlocals].iter().map(|&o| o as i64).collect()) +} + +/// Put back what [`capture_frame_locals`] took. +pub(crate) fn restore_frame_locals(frame: usize, saved: &[i64]) { + if frame == 0 { + return; + } + let arr_ptr = unsafe { + *((frame as *const u8).add(PYFRAME_LOCALS_CELLS_STACK_OFFSET) + as *const *mut pyre_object::FixedObjectArray) + }; + if arr_ptr.is_null() { + return; + } + let len = unsafe { &*arr_ptr }.as_slice().len(); + for (abs, &value) in saved.iter().enumerate().take(len) { + unsafe { + (*arr_ptr).as_mut_slice()[abs] = value as usize as PyObjectRef; + } + } + frame_array_write_barrier(frame as *mut u8, arr_ptr); +} + /// 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. @@ -4711,6 +4846,13 @@ pub(crate) fn write_back_outer_locals(ctx: &TraceCtx, frame: usize) -> bool { return false; } let base = info.num_static_extra_boxes; + // Every slot has to resolve before the first store, the way the merge-point + // flush validates ahead of its commit loop: bailing partway through leaves + // the frame carrying a mix of walk-current and pre-walk locals, which is + // neither of the two states a caller can recover from. + if (0..nlocals).any(|abs| ctx.virtualizable_entry_at(base + abs).is_none()) { + return false; + } // Boxing an Int/Float slot allocates; the detached frame array is // forwarded only while it is in the remembered set, and each minor // consumes that entry, so re-arm the barrier after every store. diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index 0eda8de7154..01c2c5ad840 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -1968,7 +1968,11 @@ fn apply_blackhole_crn( true } -fn try_adopt_single_frame_blackhole(ctx: &mut TraceCtx, cf_addr: usize) -> bool { +fn try_adopt_single_frame_blackhole( + ctx: &mut TraceCtx, + cf_addr: usize, + live_root_addr: usize, +) -> bool { let Some(mut latched) = crate::jitcode_dispatch::take_single_frame_blackhole() else { return false; }; @@ -1986,6 +1990,61 @@ fn try_adopt_single_frame_blackhole(ctx: &mut TraceCtx, cf_addr: usize) -> bool let Some(stack_base) = crate::state::concrete_nlocals(cf_addr) else { return false; }; + // The escape flush that ran ahead of the forcing residual is + // all-or-nothing, and its decline is what this latch is gated on + // (`committed_frame_escape_pc().is_none()`). What it declines on is the + // operand-stack half — the shadow's stack region reads NULL away from a + // merge point — and the register image below carries that half anyway. + // The LOCALS half is not optional: every LOAD_FAST lowers to + // `getarrayitem_vable_r` on the frame the register image names, so without + // it the replay reads whatever that frame held before the walk began, and + // a local the walk assigned comes back null. Publish that half here and + // withdraw it if it cannot complete, so a decline still leaves the legacy + // replay pristine pre-walk state. + // Which frame gets it is an identity question, and the walked frame has two + // representations: `cf_addr` is the `snapshot_for_tracing` copy the walk + // steps concretely, `live_root_addr` the frame the compiled loop runs on. + // The register is recovered the same way `try_adopt_multi_frame_blackhole` + // recovers `per_frame[0]`, and `seed_virtualizable_boxes` bakes that root + // vable identity against the live address whenever there is one, so the + // comparison uses the same address under the same fallback. Code-object + // equality would not do: two invocations of one function share a code + // object, and the shadow belongs to exactly one of them. + let root_addr = if live_root_addr != 0 { + live_root_addr + } else { + cf_addr + }; + let (frame_reg, _) = crate::state::portal_red_regs_at(jitcode_index); + let vable_frame = latched + .miframe + .ref_values + .get(frame_reg as usize) + .copied() + .flatten() + .unwrap_or(0) as usize; + if vable_frame == 0 || vable_frame != root_addr { + return false; + } + let Some(mut locals_undo) = crate::state::capture_frame_locals(vable_frame) else { + return false; + }; + 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()); + } + if !crate::state::write_back_outer_locals(ctx, vable_frame) { + crate::state::restore_frame_locals(vable_frame, &locals_undo); + majit_gc::shadow_stack::pop_resume_ref_roots_to(root_depth); + if crate::jitcode_dispatch::fbw_debug_abort_enabled() { + eprintln!("[fbw-blackhole] single-frame locals publish declined — legacy replay kept"); + } + return false; + } + // The undo image stays rooted across the drive. The publish overwrote the + // slots it was taken from, so these words are the only remaining reference + // to the pre-walk locals, and a collection inside the drive would both free + // them and leave the withdrawal below writing back pre-move addresses. let mut terminal = majit_metainterp::drive_single_frame_blackhole( &mut latched.miframe, majit_metainterp::blackhole::StateFieldLayout::default(), @@ -2000,26 +2059,27 @@ fn try_adopt_single_frame_blackhole(ctx: &mut TraceCtx, cf_addr: usize) -> bool let adopted = match terminal.outcome { majit_metainterp::jitexc::JitException::ContinueRunningNormally { ref green_int, .. - } => { - let Some(&resume_py_pc) = green_int.first() else { - return false; - }; - let resume_py_pc = resume_py_pc as usize; - if !apply_blackhole_crn( - cf_addr, - jitcode_index, - terminal.position, - terminal.last_opcode_position, - &terminal.registers_i, - terminal.registers_r.as_mut_slice(), - &terminal.registers_f, - resume_py_pc, - ) { - return false; + } => match green_int.first() { + Some(&resume_py_pc) => { + let resume_py_pc = resume_py_pc as usize; + if apply_blackhole_crn( + cf_addr, + jitcode_index, + terminal.position, + terminal.last_opcode_position, + &terminal.registers_i, + terminal.registers_r.as_mut_slice(), + &terminal.registers_f, + resume_py_pc, + ) { + WALK_END_RESTART_PC.with(|slot| slot.set(Some(resume_py_pc))); + true + } else { + false + } } - WALK_END_RESTART_PC.with(|slot| slot.set(Some(resume_py_pc))); - true - } + None => false, + }, majit_metainterp::jitexc::JitException::DoneWithThisFrameVoid => { crate::jitcode_dispatch::fbw_finish_concrete_set(crate::state::ConcreteValue::Null); true @@ -2063,7 +2123,15 @@ fn try_adopt_single_frame_blackhole(ctx: &mut TraceCtx, cf_addr: usize) -> bool jitcode_index, terminal.position, terminal.last_opcode_position, ); } + } else { + // The terminal was not adoptable, so this returns to the legacy + // 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. + crate::state::restore_frame_locals(vable_frame, &locals_undo); } + majit_gc::shadow_stack::pop_resume_ref_roots_to(root_depth); adopted } @@ -2211,6 +2279,25 @@ fn try_adopt_multi_frame_blackhole( // the snapshot. The snapshot is freed at the end of this walk, so a link // to it would survive as a dangling `f_back` for any later // `sys._getframe().f_back` or traceback walk. + // + // The links are recorded as they are overwritten, because they only hold + // for a chain that is actually driven. An adopt keeps them; every decline + // below returns to legacy escape/replay, which never entered these levels, + // so leaving a synthetic `f_back` behind would show the abandoned chain to + // anything that still reaches one of these frames — a `sys._getframe().f_back` + // walk or a traceback the walk handed out. + let relink_barrier = |callee: *mut pyre_interpreter::PyFrame| { + // `enter` stores into a frame whose allocation barrier is still in + // effect; these frames were built many collections ago, so each + // store needs its own remembered-set entry. + if pyre_object::gc_hook::try_gc_owns_object(callee as *mut u8) { + pyre_object::gc_hook::try_gc_write_barrier(callee as *mut u8); + } + }; + let mut saved_links: Vec<( + *mut pyre_interpreter::PyFrame, + *mut pyre_interpreter::PyFrame, + )> = Vec::with_capacity(per_frame.len()); unsafe { for i in 0..per_frame.len() { let callee = per_frame[i].0 as *mut pyre_interpreter::PyFrame; @@ -2222,15 +2309,59 @@ fn try_adopt_multi_frame_blackhole( if std::ptr::eq(callee, f_back) { continue; } + saved_links.push((callee, (*callee).f_backref)); (*callee).f_backref = f_back; - // `enter` stores into a frame whose allocation barrier is still in - // effect; these frames were built many collections ago, so each - // store needs its own remembered-set entry. - if pyre_object::gc_hook::try_gc_owns_object(callee as *mut u8) { - pyre_object::gc_hook::try_gc_write_barrier(callee as *mut u8); + relink_barrier(callee); + } + } + let restore_links = |saved: &[( + *mut pyre_interpreter::PyFrame, + *mut pyre_interpreter::PyFrame, + )]| { + for &(callee, f_back) in saved { + unsafe { + (*callee).f_backref = f_back; } + relink_barrier(callee); } + }; + // Frame 0 is the walked frame, and the escape flush that ran ahead of the + // forcing residual declined — that decline is what the latch is gated on. + // Its LOCALS half is not optional for frame 0's level either: every + // LOAD_FAST lowers to `getarrayitem_vable_r` on `per_frame[0]`, so without + // it the level reads what the live frame held before the walk began. Same + // publish and same withdrawal as the single-frame arm. + // + // The INNER levels get no counterpart, and that is a second thing standing + // between this path and its flip. The walk's virtualizable shadow covers + // the walked frame only, so an inlined callee's frame array keeps its + // pre-sub-walk contents while the values the sub-walk assigned sit in that + // level's register image; a local assigned inside the callee and read back + // after the escape therefore reads null. Measured on + // `scratchpad/exc_virt/s21_sigsegv.py` with the gate on: an inlined callee + // that stores `e.__traceback__` and then reads an attribute off it faults + // in `object_getattr_miss`, where the same shape through the single-frame + // arm is correct. Publishing them needs a per-level slot→value map — what + // `apply_blackhole_crn` builds from `pcdep_trivia_at` — run before the + // drive rather than after it. + let Some(mut locals_undo) = crate::state::capture_frame_locals(root_addr) else { + mfdbg!("frame 0: {root_addr:#x} locals not capturable"); + restore_links(&saved_links); + return false; + }; + 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()); } + if !crate::state::write_back_outer_locals(ctx, root_addr) { + crate::state::restore_frame_locals(root_addr, &locals_undo); + majit_gc::shadow_stack::pop_resume_ref_roots_to(undo_depth); + restore_links(&saved_links); + mfdbg!("frame 0: {root_addr:#x} locals publish declined"); + return false; + } + // Rooted across the drive for the same reason as the single-frame arm: the + // publish overwrote the slots these words came from. let ec = unsafe { (*(cf_addr as *mut pyre_interpreter::PyFrame)).execution_context as *mut pyre_interpreter::PyExecutionContext @@ -2293,11 +2424,12 @@ fn try_adopt_multi_frame_blackhole( } let adopted = match outcome { majit_metainterp::jitexc::JitException::ContinueRunningNormally { - ref green_int, .. - } => { + ref green_int, + .. + } => 'crn: { let Some(&resume_py_pc) = green_int.first() else { mfdbg!("ContinueRunningNormally with no green int"); - return false; + break 'crn false; }; let resume_py_pc = resume_py_pc as usize; // Only the frame address is checked. `resume_py_pc` is a @@ -2309,7 +2441,7 @@ fn try_adopt_multi_frame_blackhole( // replay that re-executes what the chain already committed. if cf_addr == 0 { mfdbg!("cf_addr {cf_addr:#x} is zero"); - return false; + break 'crn false; } // The terminal frame's own `setfield_vable` operations committed // the frame fields they wrote, but the aborted mid-expression @@ -2318,14 +2450,14 @@ fn try_adopt_multi_frame_blackhole( // performs. let Some(terminal) = mf_terminal.as_mut() else { mfdbg!("no terminal image for the ContinueRunningNormally handoff"); - return false; + break 'crn false; }; let Ok(terminal_jitcode_index) = i32::try_from(terminal.jitcode_index) else { mfdbg!( "terminal jitcode index {} out of range", terminal.jitcode_index ); - return false; + break 'crn false; }; // Snapshot, not `root_addr`: with the fold above the match the // snapshot is the committed image the epilogue propagates, @@ -2341,7 +2473,7 @@ fn try_adopt_multi_frame_blackhole( resume_py_pc, ) { mfdbg!("apply_blackhole_crn rejected the terminal image"); - return false; + break 'crn false; } WALK_END_RESTART_PC.with(|slot| slot.set(Some(resume_py_pc))); true @@ -2384,13 +2516,20 @@ fn try_adopt_multi_frame_blackhole( if crate::jitcode_dispatch::fbw_debug_abort_enabled() { eprintln!("[fbw-blackhole] adopted multi-frame terminal depth={depth}"); } + } 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. + crate::state::restore_frame_locals(root_addr, &locals_undo); + restore_links(&saved_links); } + majit_gc::shadow_stack::pop_resume_ref_roots_to(undo_depth); adopted } fn try_adopt_force_blackhole(ctx: &mut TraceCtx, cf_addr: usize, live_root_addr: usize) -> bool { try_adopt_multi_frame_blackhole(ctx, cf_addr, live_root_addr) - || try_adopt_single_frame_blackhole(ctx, cf_addr) + || try_adopt_single_frame_blackhole(ctx, cf_addr, live_root_addr) } fn run_perfn_walk( @@ -3659,11 +3798,17 @@ fn run_perfn_walk( let journal = crate::jitcode_dispatch::fbw_store_journal_len(); if committed { crate::jitcode_dispatch::fbw_store_journal_commit(); + crate::jitcode_dispatch::fbw_exit_last_instr_commit(); // A committed bridge recording keeps its advanced iterator cursor (the // compiled bridge / adopted end state owns the iteration count). crate::jitcode_dispatch::fbw_bridge_iter_journal_clear(); } else { crate::jitcode_dispatch::fbw_store_journal_rollback(); + // The exit coordinate the walk published goes back too: this replay + // resumes the frame from its pre-walk state and derives the next + // instruction from that field, so a kept exit coordinate would restart + // it past its own return or raise. + crate::jitcode_dispatch::fbw_exit_last_instr_rollback(); // A bridge/retrace recording that does not commit restores the // iterator cursor it eagerly advanced, so the interpreter resume // re-consumes the in-flight item exactly once (no drop). diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index 19560562740..94840dcc0be 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -3653,6 +3653,16 @@ fn build_jit_driver_pair() -> JitDriverPair { d.set_vtable_offset(Some(pyre_object::pyobject::OB_TYPE_OFFSET)); // resume.py:1367 — BlackholeAllocator for virtual materialization. d.register_blackhole_allocator(PyreBlackholeAllocator); + // `dispatch_bytecode` (pyopcode.py) stamps `last_instr` before each + // opcode, so a running frame always answers `f_lineno` — and every + // traceback taken off it — for the instruction it is on. That store is a + // source-level one upstream and rides in the jitcode; this codewriter + // unrolls the bytecode, so the same store would need one int pool + // constant per instruction. The blackhole publishes it at the `-live-` + // marker instead. + majit_metainterp::blackhole::register_live_marker_hook( + pyre_jit_trace::state::publish_last_instr_at_live_marker, + ); // warmspot.py:1039 handle_jitexception_from_blackhole parity: // portal_runner is called when ContinueRunningNormally is raised // at a recursive portal level during blackhole execution. diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index a513e35a3d5..52548f53e37 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -7824,7 +7824,7 @@ impl CodeWriter { let (instruction, op_arg) = arg_state.get(code_unit); let mut exception_edge_handled = false; - // pyframe.py:379-417 pushvalue/popvalue_maybe_none parity: + // pyframe.py pushvalue/popvalue_maybe_none parity: // RPython's push/pop each write `self.valuestackdepth = depth +/- 1`. // On the JIT, these map to per-push `setfield_vable_i`. pyre's // codewriter stores stack values in typed registers rather than @@ -7832,20 +7832,28 @@ impl CodeWriter { // setitem for each push. As the coarsest RPython-compatible // approximation we flush `valuestackdepth` once at opcode entry, // reflecting the pre-opcode stack depth — which is what the - // interpreter (eval.rs:92 `target_depth = frame.nlocals() + - // frame.ncells() + entry.depth`) uses when an exception handler - // unwinds the frame. + // interpreter's `target_depth` (`eval.rs`) uses when an exception + // handler unwinds the frame. // - // RPython interp_jit.py keeps `next_instr` as a green portal - // argument and updates `last_instr` in the interpreter loop; it - // does not lower a per-bytecode virtualizable write here. pyre's - // portal entry / guard-resume paths already restore - // `frame.next_instr`, and the interpreter updates `last_instr` - // once execution returns there. Emitting `py_pc + 1` here only - // grows the int constant pool linearly with function size and - // trips assembler.py's 256-entry cap. - // pyframe.py:379-417: valuestackdepth is written per-push/per-pop - // via setfield_vable_i (jtransform.py:923-928), NOT once at opcode + // `dispatch_bytecode` (pyopcode.py) DOES write `last_instr` once + // per opcode, and that write is part of the traced portal, so + // upstream's jitcode carries it and the blackhole replays it. + // pyre cannot mirror it here: upstream's `next_instr` is a live + // RPython variable, while this codewriter unrolls the bytecode + // per PC, so the same store needs one distinct int pool constant + // per PC and `assembler.py check_result`'s 256-entry + // `num_regs_i + constants_i` cap rejects any function past a few + // 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. + // 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. // (The old single-entry flush is removed.) @@ -8406,6 +8414,52 @@ impl CodeWriter { Instruction::ReturnValue => { let retval_reg = emit_popvalue_ref!(current_depth, py_pc); let retval = pop_ref_or_fresh(&mut current_state, &mut graph); + // `dispatch_bytecode` (pyopcode.py) stamps + // `self.last_instr = intmask(next_instr)` before running + // each opcode, so a frame that has returned answers + // `offset2lineno` — `f_lineno`, and every traceback that + // outlives the frame — for its `return`. The blackhole + // replays this jitcode instead of that loop and syncs only + // `valuestackdepth` (`emit_vsd!`), so a frame finished by a + // guard-failure resume would otherwise keep whichever + // coordinate the trace last published: for a loop whose body + // raises and catches, the raise. Store `py_pc`, not the + // `py_pc - 1` of the resume-at sites (`emit_abort_permanent!`) + // — this opcode is dispatched here, not resumed at. + // + // Portal jitcode only. In a non-portal callee + // `frame_var` aliases the OUTERMOST frame rather + // than naming that callee's own (the same aliasing + // the `LoadGlobal` register-form namespace declines + // for below), so the store would stamp the callee's + // coordinate into its caller and the caller would + // then resolve it against its own line table. A + // frame observed after an inlined callee returned — + // through `sys._getframe(1)` or a retained traceback + // — would report an unrelated line, which is the + // very failure this store exists to remove. + // Declining leaves the caller's own coordinate + // intact. The inlined callee's own frame stays + // unpublished, the same inner-level gap the + // `-live-` marker hook declines on + // (`publish_last_instr_at_live_marker` resolves the + // frame from the replaying level's own portal red + // and requires a code-object match). + if is_true_portal { + let v_li: super::flow::FlowValue = + super::flow::Constant::signed(py_pc as i64).into(); + record_graph_op( + ¤t_block.block(), + "setfield_vable_i", + vable_setfield_int_graph_args( + frame_var.into(), + v_li.into(), + VABLE_LAST_INSTR_FIELD_IDX, + ), + None, + py_pc as i64, + ); + } // ref_return reads from the stack slot // directly — the obj_tmp0 staging was redundant since // this is the terminating op of the block.