diff --git a/majit/majit-metainterp/src/blackhole.rs b/majit/majit-metainterp/src/blackhole.rs index 4e88b53e38d..faf2c155263 100644 --- a/majit/majit-metainterp/src/blackhole.rs +++ b/majit/majit-metainterp/src/blackhole.rs @@ -1482,6 +1482,26 @@ impl BlackholeInterpreter { unsafe { let mut current = Some(&mut *self); while let Some(frame) = current { + // `virtualizable_ptr` is a bare copy of the frame red this + // level was bound to, and the banks rooted just above do not + // cover it: a collection forwards the register the copy came + // from, never the copy. A young virtualizable makes that + // difference observable — a compiled trace allocates an + // inlined callee's `PyFrame` with its own `NewWithVtable`, + // which the GC rewriter lowers to a nursery allocation, so the + // first minor collection `run_inner` triggers moves it and + // leaves this field naming the vacated block. Every later + // reader takes that address: the vable opcodes below, and the + // traceback recorded for each frame an exception propagates + // through, which stores it into `PyTraceback.frame` — a slot + // the collector does trace, so the stale pointer surfaces as + // an invalid type id rather than as a wrong answer. Root the + // SLOT so the walker rewrites it, the same shape + // `blackhole_from_resumedata` already applies to the resume + // reader's own `virtualizable_ptr` for the chain-build window. + majit_gc::shadow_stack::push_resume_ref_roots(std::slice::from_mut( + &mut frame.virtualizable_ptr, + )); if !frame.virtualizable_info.is_null() { let vinfo = &*frame.virtualizable_info; vinfo.push_resume_ref_roots_for_registers(&frame.registers_r); diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 3d4bcde4d56..b23b11c756d 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -154,10 +154,12 @@ pub use pyjitpl::{ MetaInterpStaticData, RawCompileResult, StandaloneFrameStack, build_state_field_snapshot, call_int_function, call_ref_function, call_void_function, counters, record_application_traceback_for_recording, record_application_traceback_hook_address, + record_discarded_level_traceback_for_recording, record_discarded_level_traceback_hook_address, record_inline_application_traceback_for_recording, record_inline_application_traceback_hook_address, set_record_application_traceback_hook, - set_record_inline_application_traceback_hook, struct_fields_write_effect_info, trace_jitcode, - trace_jitcode_from_merge_point, trace_jitcode_with_args, trace_jitcode_with_args_and_runtime, + set_record_discarded_level_traceback_hook, set_record_inline_application_traceback_hook, + struct_fields_write_effect_info, trace_jitcode, trace_jitcode_from_merge_point, + trace_jitcode_with_args, trace_jitcode_with_args_and_runtime, }; pub use resume_box_reader::{ BridgeVirtualCache, decode_fieldnum, default_bridge_array_descr, emit_pending_field_op, diff --git a/majit/majit-metainterp/src/pyjitpl.rs b/majit/majit-metainterp/src/pyjitpl.rs index 4b27deed517..8d5604b0396 100644 --- a/majit/majit-metainterp/src/pyjitpl.rs +++ b/majit/majit-metainterp/src/pyjitpl.rs @@ -1571,11 +1571,22 @@ pub struct JitHooks { /// majit keeps only this crate-neutral ABI hook. pub type RecordApplicationTraceback = extern "C" fn(i64, i64, i64, i64); pub type RecordInlineApplicationTraceback = extern "C" fn(i64, i64, i64, i64, i64); +/// Runtime callback for a frame the resume crossed and then DISCARDED. +/// +/// `pyopcode.py:148` attaches a node on every frame an unwind passes through, +/// which upstream gets for free because the unwind is traced code running on a +/// rebuilt MIFrame stack. A resume that re-points the live frame straight at +/// its own handler has no such stack, and the only description left of the +/// frames it skipped is the `(w_code, python pc)` pair in the resume data — +/// so this hook takes a PYTHON pc where the two above take a jitcode one. +pub type RecordDiscardedLevelTraceback = extern "C" fn(i64, i64, i64); static RECORD_APPLICATION_TRACEBACK: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); static RECORD_INLINE_APPLICATION_TRACEBACK: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); +static RECORD_DISCARDED_LEVEL_TRACEBACK: std::sync::atomic::AtomicUsize = + std::sync::atomic::AtomicUsize::new(0); pub fn set_record_application_traceback_hook(hook: Option) { RECORD_APPLICATION_TRACEBACK.store( @@ -1593,6 +1604,13 @@ pub fn set_record_inline_application_traceback_hook( ); } +pub fn set_record_discarded_level_traceback_hook(hook: Option) { + RECORD_DISCARDED_LEVEL_TRACEBACK.store( + hook.map_or(0, |callback| callback as usize), + std::sync::atomic::Ordering::Release, + ); +} + pub fn record_application_traceback_hook_address() -> *const () { RECORD_APPLICATION_TRACEBACK.load(std::sync::atomic::Ordering::Acquire) as *const () } @@ -1601,6 +1619,10 @@ pub fn record_inline_application_traceback_hook_address() -> *const () { RECORD_INLINE_APPLICATION_TRACEBACK.load(std::sync::atomic::Ordering::Acquire) as *const () } +pub fn record_discarded_level_traceback_hook_address() -> *const () { + RECORD_DISCARDED_LEVEL_TRACEBACK.load(std::sync::atomic::Ordering::Acquire) as *const () +} + fn record_application_traceback(exc_value: i64, frame_ptr: *const u8, frame: &MIFrame) { if exc_value == 0 || frame_ptr.is_null() || frame.inline_frame { return; @@ -1667,6 +1689,28 @@ pub fn record_inline_application_traceback_for_recording( ); } +/// Invoke the host callback for a frame the unwind crossed but the resume +/// discarded, named by its `(w_code, python pc)` coordinate alone. +/// +/// The two callbacks above both address their frame through a jitcode: one has +/// the concrete frame pointer, the other the promoted metadata of a level the +/// walk is still standing in. A discarded level is neither — the resume data +/// is the last thing that knows it existed, and what it carries is a Python +/// coordinate. Hence the third arity. +pub fn record_discarded_level_traceback_for_recording(exc_value: i64, w_code: i64, py_pc: i64) { + if exc_value == 0 || w_code == 0 { + return; + } + let callback = RECORD_DISCARDED_LEVEL_TRACEBACK.load(std::sync::atomic::Ordering::Acquire); + if callback == 0 { + return; + } + // Safety: the only stored values come from a RecordDiscardedLevelTraceback + // function pointer in set_record_discarded_level_traceback_hook. + let callback: RecordDiscardedLevelTraceback = unsafe { std::mem::transmute(callback) }; + callback(exc_value, w_code, py_pc); +} + /// framework.py `root_walker.walk_roots` per-op helper: visit every /// inline `ConstPtr.value` slot stored in `op.args` and `op.fail_args`. /// history.py:314 `ConstPtr.value` is inline on the Box object; pyre diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs index e23ceb6af66..8e3e648bf8a 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs @@ -492,6 +492,14 @@ pub fn dispatch_via_miframe( wc.last_exc_value = Some(value_op); wc.last_exc_value_concrete = ConcreteValue::Ref(exc_edge_concrete); wc.fbw_mode.class_of_last_exc_is_const = true; + // The inlined callees this route unwound clear out of, innermost + // first, BEFORE the catching frame's own node: both recorders + // prepend, so emission order is the chain read outermost-first. + record_exc_edge_discarded_tracebacks( + &mut wc, + value_op, + ConcreteValue::Ref(exc_edge_concrete), + ); record_bridge_handler_entry_traceback( &mut wc, value_op, diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 5184099e952..70743b6b120 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -562,6 +562,61 @@ fn record_top_level_application_traceback( } } +/// One node per inlined level the exception-edge bridge resumed PAST, for the +/// frames `set_exc_edge_discarded_levels` parked before the walk began. +/// +/// `pyopcode.py:148 pytraceback.record_application_traceback` runs before the +/// `:152` exception-table lookup, so a frame the unwind only PASSES THROUGH +/// contributes a node just like the one that catches it. Upstream never has to +/// say so: it resumes onto a rebuilt MIFrame stack and the unwind is traced code +/// that runs each level's own recorder. The exc-edge route exists precisely +/// because pyre discards those levels instead, which leaves this the only place +/// their nodes can come from. +/// +/// Innermost-first, because `record_application_traceback` PREPENDS: the deepest +/// discarded frame must go on while the node the residual raise already attached +/// is still the head. The caller then records the catching frame last, so the +/// chain reads outermost-first exactly as the interpreter builds it. +/// +/// Emitted as IR, not merely executed: `trace_and_compile_from_bridge` runs once, +/// and every later guard failure of this class enters the compiled bridge +/// directly. +fn record_exc_edge_discarded_tracebacks( + ctx: &mut WalkContext<'_, '_, Sym>, + exc: OpRef, + exc_concrete: ConcreteValue, +) { + let levels = take_exc_edge_discarded_levels(); + if levels.is_empty() { + return; + } + let ConcreteValue::Ref(exc_ptr) = exc_concrete else { + return; + }; + if exc_ptr.is_null() { + return; + } + let hook = majit_metainterp::record_discarded_level_traceback_hook_address(); + for &(w_code, py_pc) in levels.iter().rev() { + majit_metainterp::record_discarded_level_traceback_for_recording( + exc_ptr as usize as i64, + w_code as i64, + py_pc as i64, + ); + if hook.is_null() || exc.is_none() { + continue; + } + let w_code_op = ctx.trace_ctx.const_ref(w_code as i64); + let py_pc_op = ctx.trace_ctx.const_int(py_pc as i64); + ctx.trace_ctx.call_void_typed_with_effect( + hook, + &[exc, w_code_op, py_pc_op], + &[Type::Ref, Type::Ref, Type::Int], + default_effect_info(), + ); + } +} + fn record_inline_application_traceback( ctx: &mut WalkContext<'_, '_, Sym>, exc: OpRef, @@ -2306,6 +2361,35 @@ pub(crate) fn take_carrier_raise_seed() -> Option { FBW_CARRIER_RAISE_SEED.with(|c| c.take()) } +thread_local! { + /// The inlined levels an exception-edge bridge resumes PAST, outermost-first + /// and excluding the live frame, as `(w_code, python pc)`. + /// + /// `call_jit.rs trace_and_compile_from_bridge` routes the exc edge only for + /// a raise that unwinds clear out of every inlined callee, and answers that + /// question from the per-level `resume_coords` it decoded. Those coordinates + /// are the last description of the discarded levels anywhere in the process — + /// the walk that follows starts flat — so they are parked here for the walk's + /// handler entry to turn into traceback nodes. + static FBW_EXC_EDGE_DISCARDED_LEVELS: std::cell::RefCell> = + const { std::cell::RefCell::new(Vec::new()) }; +} + +/// Publish the levels an exc-edge route is about to discard. Always called on +/// the routing path, with an empty slice for a single-frame resume, so a +/// previous bridge's levels can never leak into this one. +pub fn set_exc_edge_discarded_levels(levels: &[(usize, usize)]) { + FBW_EXC_EDGE_DISCARDED_LEVELS.with(|c| { + let mut slot = c.borrow_mut(); + slot.clear(); + slot.extend_from_slice(levels); + }); +} + +pub(crate) fn take_exc_edge_discarded_levels() -> Vec<(usize, usize)> { + FBW_EXC_EDGE_DISCARDED_LEVELS.with(|c| std::mem::take(&mut *c.borrow_mut())) +} + /// Walk one opcode at `pc` and return the dispatch outcome plus the /// next pc. Side effects reach `ctx.trace_ctx` only for opnames whose /// handler explicitly records (e.g. `ref_return/r` calls diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 3d356801299..b693ea494fc 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -4449,6 +4449,22 @@ pub(crate) fn record_quasiimmut_field(ctx: &mut TraceCtx, obj: OpRef, descr: Des ); return; } + // quasiimmut.py:124 `self.qmut = get_current_qmut_instance(cpu, struct, + // mutatefielddescr)` — the half that makes the value captured below + // answerable later. The hidden `mutate_` field is null until + // something installs it, and while it is null a write to the + // quasi-immutable field takes the no-watchers early return: the value moves + // and nothing is forced. Installing at the record makes every write from + // here on run the invalidation, which is what both the tracer's own force + // check and the optimizer's revalidation read. + // + // Ordered before the value read for the reason upstream orders `__init__` + // that way: a write landing between the two must be one the watcher sees. + // Reading first leaves a window where the field moves with no watcher + // installed, so nothing invalidates and nothing bumps the force counter, + // and the trace keeps a value that is already stale. Without a GIL that + // window is a real interleaving, not a theoretical one. + install_quasiimmut_field(ctx, obj, &descr); // quasiimmut.py:125 `self.constantfieldbox = // self.get_current_constant_fieldvalue()` — the field's value at the moment // the trace baked it. `heap.py:803 is_still_valid_for` compares it against @@ -4456,7 +4472,6 @@ pub(crate) fn record_quasiimmut_field(ctx: &mut TraceCtx, obj: OpRef, descr: Des // captured here; by the time the optimizer runs, the change it is looking // for has already happened. let constantfieldbox = current_quasiimmut_field_value(ctx, obj, &descr); - install_quasiimmut_field(ctx, obj, &descr); ctx.heap_cache_mut().quasi_immut_now_known(field_index, obj); // Upstream carries the captured value on the per-op `QuasiImmutDescr` // (quasiimmut.py:113-159), a descr minted fresh for every recorded diff --git a/pyre/pyre-jit-trace/src/trace.rs b/pyre/pyre-jit-trace/src/trace.rs index 1f5b2f32df1..c9e536b69cc 100644 --- a/pyre/pyre-jit-trace/src/trace.rs +++ b/pyre/pyre-jit-trace/src/trace.rs @@ -3976,7 +3976,24 @@ fn run_perfn_walk( }) = &walk_result { let abort_jit_pc = *pc; - if !crate::jitcode_dispatch::fbw_executed_nonpure_residual() { + // A walk commits at most ONE leg. The carrier block above may + // already have taken `CalleeRebuild`, which resumes INSIDE the + // rebuilt callee, past what it applied; rewinding the caller to its + // CALL on top of that runs the whole callee a second time. Nothing + // else keeps the two apart — this leg's own gates are inclusion + // tests on the residual odometer, and `walk_end_resume_provable` + // cannot see effects applied by the plain interpretation the + // rebuild resumed into (the `MidBodyDecline::AfterRun` argument, + // which the carrier block applies only to its own fallback). + if WALK_END_FLUSH_COMMITTED.with(|c| c.get()) { + crate::jitcode_dispatch::fbw_abort_outer_resume_reset(); + if crate::jitcode_dispatch::fbw_debug_abort_enabled() { + eprintln!( + "[fbw-abort-flush] declined at abort_jit_pc={abort_jit_pc} \ + (a carrier leg already committed this walk)" + ); + } + } else if !crate::jitcode_dispatch::fbw_executed_nonpure_residual() { if crate::jitcode_dispatch::fbw_debug_abort_enabled() { eprintln!( "[fbw-abort-flush] declined at abort_jit_pc={abort_jit_pc} \ diff --git a/pyre/pyre-jit/src/call_jit.rs b/pyre/pyre-jit/src/call_jit.rs index badbb278497..2b2f64938f2 100644 --- a/pyre/pyre-jit/src/call_jit.rs +++ b/pyre/pyre-jit/src/call_jit.rs @@ -807,6 +807,84 @@ pub(crate) extern "C" fn record_inline_traceback_for_recording( } } +/// `pytraceback.py:104-109 record_application_traceback` for a frame the resume +/// crossed and discarded — an inlined callee the exception-edge bridge unwound +/// clear out of on its way to the live frame's own handler. +/// +/// `pyopcode.py:148` runs BEFORE the `:152` exception-table lookup, so a frame +/// that only propagates contributes a node exactly like one that catches. The +/// levels named here never reach a recorder that could speak for them: the +/// route exists because the unwind discards them, and by the time the walk +/// starts they are gone. +/// +/// The node's frame is fabricated from the code object, as +/// [`record_inline_traceback_for_recording`] does and for the same reason — +/// the level's own `PyFrame` stayed virtual in the compiled trace and the +/// resume never materialized it, so there is no other object to name. Unlike +/// that case there is also no seeded frame it could be confused with: nothing +/// else in this process can reach the discarded level either. +/// +/// `py_pc` is the level's resume coordinate, which is `next_instr`-style — the +/// same coordinate `exc_table_offset` converts with `saturating_sub(1)` and +/// `set_last_instr_from_next_instr` converts for the live frame. Everything +/// below wants the instruction that RAN: the opcode decode that answers the +/// bare-reraise question, `frame.last_instr`, and `record_application_traceback`'s +/// `tb_lasti`. Convert once, up front. +pub(crate) extern "C" fn record_discarded_level_traceback( + exc_value: i64, + w_code_value: i64, + py_pc: i64, +) { + if exc_value == 0 || w_code_value == 0 || py_pc < 0 { + return; + } + let last_instruction = py_pc.saturating_sub(1); + let w_code = w_code_value as PyObjectRef; + let raw_code = + unsafe { pyre_interpreter::w_code_get_ptr(w_code) as *const pyre_interpreter::CodeObject }; + if raw_code.is_null() { + return; + } + // Same RaiseWithExplicitTraceback rule the other two recorders follow: a + // bare reraise preserves the traceback the original raise attached. + let bare_reraise = match unsafe { + pyre_interpreter::decode_instruction_at(&*raw_code, last_instruction as usize) + } { + Some((pyre_interpreter::Instruction::RaiseVarargs { .. }, op_arg)) => { + u32::from(op_arg) == 0 + } + Some((pyre_interpreter::Instruction::Reraise { .. }, _)) => true, + _ => false, + }; + if bare_reraise { + return; + } + let w_globals = unsafe { pyre_interpreter::w_code_get_w_globals(w_code) }; + let w_exc = exc_value as PyObjectRef; + let _roots = pyre_object::gc_roots::push_roots(); + pyre_object::gc_roots::pin_root(w_exc); + pyre_object::gc_roots::pin_root(w_code); + pyre_object::gc_roots::pin_root(w_globals); + let Ok(mut frame) = pyre_interpreter::createframe_obj( + w_code as *const (), + w_globals, + pyre_interpreter::call::getexecutioncontext(), + None, + ) else { + return; + }; + frame.last_instr = last_instruction as isize; + let frame_ptr = frame.into_raw(); + pyre_object::gc_roots::pin_root(frame_ptr as PyObjectRef); + unsafe { + pyre_interpreter::pytraceback::record_application_traceback( + w_exc, + frame_ptr, + last_instruction, + ); + } +} + #[majit_macros::jit_may_force] pub extern "C" fn jit_force_callee_frame(frame_ptr: i64) -> i64 { #[cfg(feature = "cranelift")] @@ -3328,6 +3406,15 @@ pub fn trace_and_compile_from_bridge( let route_exc_edge = caught_in_frame && (!is_multiframe_resume || unwind_to_live_frame) && pyre_jit_trace::jitcode_dispatch::exc_edge_bridge_enabled(); + // The levels this route is about to throw away: `resume_coords` minus the + // live frame, which keeps its own recorder at the handler entry. Published + // unconditionally on the routing path — an empty slice for the single-frame + // shape — so a previous bridge's levels cannot survive into this walk. + pyre_jit_trace::jitcode_dispatch::set_exc_edge_discarded_levels(if route_exc_edge { + resume_coords.get(1..).unwrap_or(&[]) + } else { + &[] + }); if route_exc_edge && guard_exc != 0 { // Publish the grabbed exception (`cpu.grab_exc_value` result) so the // walker's `seed_standing_exception_for_walk` threads it into diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index d0d4c49fa7e..8aaff4dfb31 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -4064,6 +4064,9 @@ fn build_jit_driver_pair() -> JitDriverPair { majit_metainterp::set_record_inline_application_traceback_hook(Some( crate::call_jit::record_inline_traceback_for_recording, )); + majit_metainterp::set_record_discarded_level_traceback_hook(Some( + crate::call_jit::record_discarded_level_traceback, + )); let info = build_pyframe_virtualizable_info(); let mut d = JitDriver::new(JIT_THRESHOLD); d.set_virtualizable_info(info.clone()); @@ -7472,14 +7475,23 @@ fn eval_loop_jit(frame: &mut PyFrame) -> LoopResult { let step_result = execute_opcode_step(unsafe { &mut *f }, code, instruction, op_arg, next_instr); match step_result { - Ok(StepResult::Continue) => { - // pyjitpl.py:2843 blackhole_if_trace_too_long — check after - // every traced step to prevent infinite trace recording. - driver.blackhole_if_trace_too_long(); - } + // `blackhole_if_trace_too_long` belongs to the tracer's own + // stepping loop (`pyjitpl.py:2861-2867 _interpret`, which drives + // `framestack[-1].run_one_step()`), not to the concrete bytecode + // dispatch loop — `pyframe.py dispatch_bytecode` has no such call. + // Pyre's tracer is the walker, and its walk loop already runs the + // check (`jitcode_dispatch/mod.rs`). Calling it from here tore + // down `MetaInterp.tracing` from a *re-entrant* interpreter run: + // a residual call executed inside an inline sub-walk runs real + // Python through `eval_loop_jit`, whose per-step check saw the + // OUTER trace over its limit and moved that `TraceCtx` out of the + // shared slot, leaving the in-flight walk recording through a + // dangling `&mut TraceCtx`. RPython cannot reach this: a nested + // JIT entry builds its own `MetaInterp` (`warmstate.py:437-441 + // bound_reached`), so the outer attempt's history is untouchable. + Ok(StepResult::Continue) => {} Ok(StepResult::CloseLoop { loop_header_pc, .. }) => { if !cached_loop_header_pcs(code).contains(&loop_header_pc) { - driver.blackhole_if_trace_too_long(); continue; } // execute_opcode_step (above) is a collection point and this arm diff --git a/pyre/pyre-jit/src/jit/codewriter.rs b/pyre/pyre-jit/src/jit/codewriter.rs index 9f6768f1098..7bc5d117f64 100644 --- a/pyre/pyre-jit/src/jit/codewriter.rs +++ b/pyre/pyre-jit/src/jit/codewriter.rs @@ -4311,6 +4311,21 @@ fn register_helper_fn_pointers( } } +/// Collect `(py_pc, insn_idx)` pairs into one entry per `insn_idx`, preserving +/// first-seen order. Both `-live-` marker families are grouped this way, so a +/// marker several Python PCs share accumulates the union of their narrowed sets. +fn group_py_pcs_by_insn(pairs: impl Iterator) -> Vec<(usize, Vec)> { + let mut groups: Vec<(usize, Vec)> = Vec::new(); + for (py_pc, insn_idx) in pairs { + if let Some(entry) = groups.iter_mut().find(|(idx, _)| *idx == insn_idx) { + entry.1.push(py_pc); + } else { + groups.push((insn_idx, vec![py_pc])); + } + } + groups +} + /// RPython: `liveness.py:19-80` `compute_liveness(ssarepr)` — /// backward dataflow over the populated `SSARepr` that fills each /// `-live-` marker with the set of registers alive across it. @@ -4440,37 +4455,59 @@ fn filter_liveness_in_place( let nlocals = code.varnames.len(); let live_markers_out = live_markers.clone(); + // Group `(py_pc, insn_idx)` pairs by `insn_idx` so a shared + // marker accumulates the UNION of per-PC narrowed sets (resume + // from any sharing PC reads a conservative superset, which is + // safe — preserving more registers than strictly needed never + // causes incorrect resume). + let groups = group_py_pcs_by_insn( + live_markers + .iter() + .enumerate() + .map(|(py_pc, &i)| (py_pc, i)), + ); + + // The after-residual-call `-live-` is a resume coordinate of exactly the + // same class as the per-PC marker: `get_list_of_active_boxes` reads the + // marker AFTER the call whenever `in_a_call` or `after_residual_call` + // holds and the one before the op otherwise (`pyjitpl.py:194-198`), and + // `compute_liveness` (`liveness.py:19-23`) is one uniform pass over every + // `-live-` in the stream. The LV∩SSA narrowing below is pyre's adaptation + // for a tracer that materializes Python-frame slots only; restricting it + // to the per-PC markers left the after-call markers naming SSA-live + // scratch Ref colors no trace-time writer populates, which read back + // `OpRef::NONE` and force the resume snapshot to decline. Narrow both. + let per_pc_markers: std::collections::BTreeSet = + groups.iter().map(|&(insn_idx, _)| insn_idx).collect(); + let after_call_groups = + group_py_pcs_by_insn(after_call_post_merge.iter().enumerate().filter_map( + |(py_pc, anchor)| { + // Folded onto a per-PC marker — already narrowed by that group. + anchor + .filter(|idx| !per_pc_markers.contains(idx)) + .map(|idx| (py_pc, idx)) + }, + )); + // Snapshot original marker contents BEFORE any mutation so that // when multiple Python PCs share a single post-merge `-live-` // marker (possible once `remove_repeated_live` folds adjacent // markers without protection), each PC's narrowing pass reads // the SSA union — not a previously-narrowed set. - let original_markers: Vec> = live_markers + let original_markers: std::collections::BTreeMap> = groups .iter() - .map(|&idx| { - ssarepr + .chain(after_call_groups.iter()) + .map(|&(insn_idx, _)| { + let args = ssarepr .insns - .get(idx) + .get(insn_idx) .and_then(|i| i.live_args()) .map(|args| args.to_vec()) - .unwrap_or_default() + .unwrap_or_default(); + (insn_idx, args) }) .collect(); - // Group `(py_pc, insn_idx)` pairs by `insn_idx` so a shared - // marker accumulates the UNION of per-PC narrowed sets (resume - // from any sharing PC reads a conservative superset, which is - // safe — preserving more registers than strictly needed never - // causes incorrect resume). - let mut groups: Vec<(usize, Vec)> = Vec::new(); - for (py_pc, &insn_idx) in live_markers.iter().enumerate() { - if let Some(entry) = groups.iter_mut().find(|(idx, _)| *idx == insn_idx) { - entry.1.push(py_pc); - } else { - groups.push((insn_idx, vec![py_pc])); - } - } - // #348 Part (2): marker-consistent per-PC color→slot map. A folded marker // carries the UNION of its group's Ref colors (`union_r` below); a single // PC's `pcdep_color_slots` entry covers only its own colors, so a runtime @@ -4481,10 +4518,18 @@ fn filter_liveness_in_place( // semantics a per-program-point coloring guarantees. let mut marker_pcdep: Vec> = vec![Vec::new(); walker_tracked.len()]; - for (insn_idx, py_pcs) in groups { + let all_groups = groups + .into_iter() + .map(|(insn_idx, py_pcs)| (insn_idx, py_pcs, false)) + .chain( + after_call_groups + .into_iter() + .map(|(insn_idx, py_pcs)| (insn_idx, py_pcs, true)), + ); + for (insn_idx, py_pcs, after_residual_call) in all_groups { // Original snapshot is the same for every PC in the group // (they all point at the same marker). - let original = &original_markers[py_pcs[0]]; + let original = &original_markers[&insn_idx]; let non_register: Vec = original .iter() .filter(|op| !matches!(op, SsaOperand::Register(_))) @@ -4584,6 +4629,29 @@ fn filter_liveness_in_place( if portal_ec_reg != u16::MAX { s.insert(portal_ec_reg); } + // The call's own result register is defined by the call this + // marker follows and holds no Python-frame slot at the marker, + // so the frame-slot retain would drop it — yet it is precisely + // the register the resume must carry: `get_list_of_active_boxes` + // names it explicitly (`ord(self.bytecode[self.pc - 1])`, + // `pyjitpl.py:186`) to clear the not-yet-defined box in a paused + // caller, and the resumed frame reads it back as the call result. + if after_residual_call { + for prev in ssarepr.insns[..insn_idx].iter().rev() { + if prev.is_live() { + continue; + } + if let super::flatten::Insn::Op { + result: Some(result), + .. + } = prev + && result.kind == SsaKind::Ref + { + s.insert(result.index); + } + break; + } + } s }; // Re-add the per-PC frame-live colors backward liveness @@ -4741,7 +4809,10 @@ fn filter_liveness_in_place( // out of `registers_r` range or decode to NONE under the int-typed // trace, so the inversion is a no-op the overlay then fills — keeps the // map non-empty and the runtime on the correct (overlay) path. - { + // Only the per-PC marker owns a Python PC's published map: both markers + // of one PC share the single `marker_pcdep[py_pc]` entry, and the + // runtime inverts a resume color through the PC's own coloring. + if !after_residual_call { let pcdep = pcdep_color_slots; // Publish each member PC's OWN per-PC color→slot entry, NOT // the cross-PC union. The folded `-live-` marker's `union_r` makes