From 2fc29ced2f9a391bc973f7745b8c33fff4bd6787 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 09:39:34 +0900 Subject: [PATCH 01/10] Factor with_iframe into enter_iframe/exit_iframe helpers Extract the frame entry (recursion check, TLS link, exception save) and exit (materialization sync, TLS restore, GC tracking) logic from with_iframe into standalone enter_iframe/exit_iframe methods. with_iframe now calls them, with no behavioral change. This prepares for the trampoline loop where enter/exit are called individually rather than wrapped around a closure. Assisted-by: Claude --- crates/vm/src/vm/mod.rs | 79 ++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index a48ce56c14a..0d419134342 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -777,6 +777,15 @@ pub fn process_hash_secret_seed() -> u32 { *SEED.get_or_init(|| u32::from_ne_bytes(rustpython_common::rand::os_random())) } +/// Saved state from `enter_iframe`, needed by `exit_iframe` to restore +/// the previous frame chain and exception state. +pub(crate) struct IframeEntryState { + pub(crate) iframe_ptr: *const crate::frame::InterpreterFrame, + pub(crate) old_chain: *const crate::frame::InterpreterFrame, + pub(crate) saved_exc: Option, + pub(crate) save_exc: bool, +} + impl VirtualMachine { fn init_callable_cache(&mut self) -> PyResult<()> { self.callable_cache.len = Some(self.builtins.get_attr("len", self)?); @@ -1805,15 +1814,14 @@ impl VirtualMachine { result } - /// Execute a stack-allocated InterpreterFrame without heap-allocating - /// a FrameObject. This is the fast path for regular function calls. - /// The frame is pushed onto the chain as a `*const InterpreterFrame`. - #[inline(always)] - pub fn with_iframe( + /// Push `iframe` onto the frame chain: recursion/C-stack check, TLS + /// link, exception save. Returns the saved state needed by + /// `exit_iframe`. + #[inline] + pub(crate) fn enter_iframe( &self, iframe: &mut crate::frame::InterpreterFrame, - f: impl FnOnce(&mut crate::frame::InterpreterFrame) -> PyResult, - ) -> PyResult { + ) -> PyResult { self.check_recursive_call("")?; let depth = self.recursion_depth.get(); @@ -1839,29 +1847,35 @@ impl VirtualMachine { None }; - let result = f(iframe); + Ok(IframeEntryState { + iframe_ptr, + old_chain, + saved_exc, + save_exc, + }) + } + + /// Pop `iframe` from the frame chain: sync materialized state, restore + /// exception, TLS unlink, GC tracking. + pub(crate) fn exit_iframe(&self, state: IframeEntryState) { + let IframeEntryState { + iframe_ptr, + old_chain, + saved_exc, + save_exc, + } = state; // If this iframe was materialized, capture f_back so that code - // holding a reference to the FrameObject (e.g. sys._getframe() - // return value, traceback frames) can walk the chain after return. - // - // Read materialized through the raw TLS pointer instead of the - // &mut iframe reference. During f(iframe), bytecode can - // materialize the frame via the TLS chain (a raw pointer alias); - // the &mut borrow lets LLVM assume no aliased writes, which can - // cause the store to be invisible through `iframe.materialized`. + // holding a reference to the FrameObject can walk the chain after + // return. Read materialized through read_volatile to bypass + // LLVM's noalias on the &mut iframe borrow. { - // Use read_volatile through the original raw pointer to bypass - // LLVM's noalias assumptions on the &mut iframe borrow. let mat_ptr = unsafe { let field_ptr = core::ptr::addr_of!((*iframe_ptr).materialized); core::ptr::read_volatile(field_ptr as *const usize) }; if mat_ptr != 0 { let fo = unsafe { &*(mat_ptr as *const crate::Py) }; - // Sync localsplus, prev_line, lasti from the live iframe to - // the materialized FrameObject so f_locals, f_lineno, f_lasti - // reflect the final state after execution. unsafe { let live_iframe = &*iframe_ptr; fo.iframe_mut() @@ -1879,15 +1893,9 @@ impl VirtualMachine { } if !old_chain.is_null() { let prev_iframe = unsafe { &*old_chain }; - // Use materialize_chain to avoid cloning localsplus, which - // would create extra refcounts on local variables. The - // lightweight frame has empty localsplus; live values are - // read through find_live_source_iframe when needed. let back_fo = prev_iframe.materialize_chain(self); *fo.iframe().cold().retained_back.lock() = Some(back_fo); } - // Set owner to FrameObject since this frame is no longer - // executing on a thread. fo.iframe().owner.store( crate::frame::FrameOwner::FrameObject as i8, core::sync::atomic::Ordering::Release, @@ -1898,15 +1906,11 @@ impl VirtualMachine { if save_exc { self.restore_exception(saved_exc); } - // Restore the frame chain BEFORE clearing temporary_refs, so - // top_frame no longer points at the materialized FrameObject - // when its last strong reference is released. let _ = crate::vm::thread::set_current_frame(old_chain); self.recursion_depth.update(|d| d - 1); - // Now that the frame is off the chain, track the materialized - // FrameObject in the GC and release temporary_refs so cycle - // collection can detect and reclaim reference cycles. + // Track the materialized FrameObject in GC and release + // temporary_refs after the frame is off the chain. { let mat_ptr = unsafe { let field_ptr = core::ptr::addr_of!((*iframe_ptr).materialized); @@ -1922,7 +1926,16 @@ impl VirtualMachine { } } } + } + pub fn with_iframe( + &self, + iframe: &mut crate::frame::InterpreterFrame, + f: impl FnOnce(&mut crate::frame::InterpreterFrame) -> PyResult, + ) -> PyResult { + let state = self.enter_iframe(iframe)?; + let result = f(iframe); + self.exit_iframe(state); result } From b581c9007515358f1b10f4946e6bba33ac2dde1f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 11:00:46 +0900 Subject: [PATCH 02/10] Allocate InterpreterFrame and LocalsPlus together on the datastack Add InterpreterFrame::new_on_datastack() that bump-allocates both the InterpreterFrame struct and its LocalsPlus data array in a single datastack push, eliminating one allocation per function call. Update datastack_frame_size_bytes_for_code() to include InterpreterFrame size. Convert invoke_prepared_exact_args() and the invoke() fast path to use the combined allocation. Add release_datastack_frame() method on InterpreterFrame that drops all localsplus values, runs field destructors (trace, temporary_refs, retained_back, etc.), and returns the datastack base pointer for pop. Assisted-by: Claude --- crates/vm/src/builtins/function.rs | 35 +++----- crates/vm/src/frame.rs | 124 +++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 22 deletions(-) diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index 39195202af7..3cd8a767a11 100644 --- a/crates/vm/src/builtins/function.rs +++ b/crates/vm/src/builtins/function.rs @@ -617,11 +617,6 @@ impl Py { // Fast path: stack-allocated InterpreterFrame, no FrameObject. // No refcount inc for code — it's alive via self.code for the call duration. - let nlocalsplus = code.localspluskinds.len(); - let max_stackdepth = code.max_stackdepth as usize; - let localsplus = - crate::frame::LocalsPlus::new_on_datastack(nlocalsplus, max_stackdepth, vm); - let locals = if code.flags.contains(bytecode::CodeFlags::NEWLOCALS) { crate::frame::FrameLocals::lazy() } else if let Some(locals) = locals { @@ -634,22 +629,21 @@ impl Py { // Use self.as_object() as raw pointer — no refcount inc/dec. // The function is alive on the caller's stack for the call duration. - let mut iframe = crate::frame::InterpreterFrame::new( + let iframe = crate::frame::InterpreterFrame::new_on_datastack( &self.code, &self.globals, &self.builtins, Some(self.as_object()), - localsplus, locals, self.closure.as_ref().map_or(&[], |c| c.as_slice()), - crate::frame::FrameOwner::Thread, + vm, ); let result = self - .fill_locals_from_args_iframe(&mut iframe, func_args, vm) - .and_then(|()| vm.run_frame_fast(&mut iframe)); + .fill_locals_from_args_iframe(iframe, func_args, vm) + .and_then(|()| vm.run_frame_fast(iframe)); // Release data stack memory — must happen on both success and error. unsafe { - if let Some(base) = iframe.localsplus.release_datastack() { + if let Some(base) = iframe.release_datastack_frame() { vm.datastack_pop(base); } } @@ -797,10 +791,6 @@ impl Py { vm: &VirtualMachine, ) -> PyResult { let code = &*self.code; - let nlocalsplus = code.localspluskinds.len(); - let max_stackdepth = code.max_stackdepth as usize; - let localsplus = - crate::frame::LocalsPlus::new_on_datastack(nlocalsplus, max_stackdepth, vm); let locals = if code.flags.contains(bytecode::CodeFlags::NEWLOCALS) { crate::frame::FrameLocals::lazy() @@ -810,15 +800,14 @@ impl Py { )) }; - let mut iframe = crate::frame::InterpreterFrame::new( + let iframe = crate::frame::InterpreterFrame::new_on_datastack( code, &self.globals, &self.builtins, Some(self.as_object()), - localsplus, locals, self.closure.as_ref().map_or(&[], |c| c.as_slice()), - crate::frame::FrameOwner::Thread, + vm, ); // Fill arguments directly into fastlocals @@ -829,9 +818,9 @@ impl Py { } } - let result = vm.run_frame_fast(&mut iframe); + let result = vm.run_frame_fast(iframe); unsafe { - if let Some(base) = iframe.localsplus.release_datastack() { + if let Some(base) = iframe.release_datastack_frame() { vm.datastack_pop(base); } } @@ -887,8 +876,10 @@ pub(crate) fn datastack_frame_size_bytes_for_code(code: &Py) -> Option()) + Some(crate::frame::datastack_iframe_total_bytes( + nlocalsplus, + code.max_stackdepth as usize, + )) } impl PyPayload for PyFunction { diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index bf276ec45e0..44075c38348 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -861,6 +861,9 @@ pub struct InterpreterFrame { /// Used by `frame.clear()` to reject clearing an executing frame, /// even when called from a different thread. pub(crate) owner: atomic::AtomicI8, + /// Base pointer of the datastack allocation when this frame and its + /// localsplus are bump-allocated together. Null for heap-backed frames. + pub(crate) datastack_base: *mut u8, /// Pointer to the owning `Py`, or null for stack-allocated /// frames that have not been materialized yet. /// Stored as `usize` for `PyAtomic` compatibility. @@ -944,11 +947,112 @@ impl InterpreterFrame { generator: PyAtomicBorrow::new(), previous: Radium::new(0), owner: atomic::AtomicI8::new(owner as i8), + datastack_base: core::ptr::null_mut(), materialized: Radium::new(0), cold: OnceCell::new(), } } + /// Allocate an InterpreterFrame and its LocalsPlus data together on the + /// thread data stack in a single bump allocation. + /// + /// Layout: `[InterpreterFrame | localsplus usize×capacity]` + /// + /// Returns a mutable reference whose lifetime is bounded by the data + /// stack's LIFO discipline. The caller must call + /// `release_datastack_frame()` (unsafe) when done, then + /// `vm.datastack_pop(base)`. The reference must not be used after + /// `release_datastack_frame` returns. + #[allow(clippy::too_many_arguments)] + pub(crate) fn new_on_datastack<'a>( + code: &Py, + globals: &Py, + builtins: &PyObject, + func_obj: Option<&PyObject>, + locals: FrameLocals, + closure: &[PyCellRef], + vm: &VirtualMachine, + ) -> &'a mut Self { + let nlocalsplus = code.localspluskinds.len(); + let stacksize = code.max_stackdepth as usize; + let capacity = nlocalsplus + .checked_add(stacksize) + .expect("LocalsPlus capacity overflow"); + + let total_bytes = datastack_iframe_total_bytes(nlocalsplus, stacksize); + let base = vm.datastack_push(total_bytes); + + // InterpreterFrame lives at the start of the allocation. + let iframe_ptr = base as *mut Self; + // LocalsPlus data follows the InterpreterFrame, aligned to usize. + let localsplus_offset = core::mem::size_of::(); + let localsplus_offset_aligned = (localsplus_offset + core::mem::align_of::() - 1) + & !(core::mem::align_of::() - 1); + let localsplus_data_ptr = unsafe { base.add(localsplus_offset_aligned) } as *mut usize; + + // Zero-initialize localsplus data. + unsafe { core::ptr::write_bytes(localsplus_data_ptr, 0, capacity) }; + + let nlocalsplus_u32 = u32::try_from(nlocalsplus).expect("nlocalsplus exceeds u32"); + let localsplus = LocalsPlus { + data: LocalsPlusData::DataStack { + ptr: localsplus_data_ptr, + capacity, + }, + nlocalsplus: nlocalsplus_u32, + stack_top: 0, + }; + + let mut iframe = Self::new( + code, + globals, + builtins, + func_obj, + localsplus, + locals, + closure, + FrameOwner::Thread, + ); + iframe.datastack_base = base; + + // Write the fully initialized InterpreterFrame into the datastack. + unsafe { + core::ptr::write(iframe_ptr, iframe); + &mut *iframe_ptr + } + } + + /// Release this datastack-allocated frame's resources and return the + /// base pointer for `vm.datastack_pop()`. + /// + /// Drops all localsplus values, runs destructors for all frame fields + /// (trace, temporary_refs, retained_back, etc.), and detaches the + /// backing store. + /// Returns `None` if this frame is not datastack-allocated. + /// + /// After this call, the InterpreterFrame at `self` is logically dead — + /// the caller must not use `self` again except to pass the returned + /// base to `vm.datastack_pop()`. + pub(crate) unsafe fn release_datastack_frame(&mut self) -> Option<*mut u8> { + let base = self.datastack_base; + if base.is_null() { + return None; + } + self.datastack_base = core::ptr::null_mut(); + // Drop all localsplus values while the backing store is still valid. + self.localsplus.drop_values(); + // Detach from the data stack so further accesses see an empty frame. + self.localsplus.data = LocalsPlusData::Heap(Box::default()); + self.localsplus.nlocalsplus = 0; + // Drop remaining frame fields (trace, temporary_refs, retained_back, + // etc.) by running destructors in place. The localsplus is already + // empty/heap-backed, so this only drops non-localsplus fields. + // SAFETY: `self` points to valid, initialized memory on the data + // stack. After this call the memory is logically dead. + unsafe { core::ptr::drop_in_place(self) }; + Some(base) + } + /// Get the last instruction index. #[inline(always)] pub fn get_lasti(&self) -> u32 { @@ -1038,6 +1142,7 @@ impl InterpreterFrame { // If we copied Thread from the source iframe, frame.clear() would // reject the frame with "cannot clear an executing frame". owner: atomic::AtomicI8::new(FrameOwner::FrameObject as i8), + datastack_base: core::ptr::null_mut(), materialized: Radium::new(0), cold: OnceCell::from(Box::new(FrameColdData { escaped: atomic::AtomicBool::new(true), @@ -1117,6 +1222,7 @@ impl InterpreterFrame { generator: PyAtomicBorrow::new(), previous: Radium::new(0), owner: atomic::AtomicI8::new(FrameOwner::FrameObject as i8), + datastack_base: core::ptr::null_mut(), materialized: Radium::new(0), cold: OnceCell::from(Box::new(FrameColdData { escaped: atomic::AtomicBool::new(true), @@ -2179,6 +2285,24 @@ impl Py { } } +/// Total bytes needed to co-allocate an InterpreterFrame and its LocalsPlus +/// data on the thread data stack. +pub(crate) fn datastack_iframe_total_bytes(nlocalsplus: usize, stacksize: usize) -> usize { + let iframe_size = core::mem::size_of::(); + // Align the localsplus data to usize alignment after the InterpreterFrame. + let iframe_padded = + (iframe_size + core::mem::align_of::() - 1) & !(core::mem::align_of::() - 1); + let capacity = nlocalsplus + .checked_add(stacksize) + .expect("LocalsPlus capacity overflow"); + let data_bytes = capacity + .checked_mul(core::mem::size_of::()) + .expect("LocalsPlus byte size overflow"); + iframe_padded + .checked_add(data_bytes) + .expect("datastack iframe total size overflow") +} + /// Execute an InterpreterFrame's bytecode directly, without a FrameObject. /// /// # Safety From b089987cfabe8ec96492f2717888bee880ea797e Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 11:47:16 +0900 Subject: [PATCH 03/10] Implement trampoline loop for Python-to-Python calls Add ExecutionResult::TailCall variant and a trampoline in run_frame_fast that flattens Python-to-Python calls into a single Rust stack frame instead of recursing through the eval loop. CallPyExactArgs now prepares the callee frame on the datastack and returns TailCall when tailcall_enabled is set (run_iframe path only). The trampoline dispatches via a state machine (EnterCallee / ReturnValue / Unwind) in a single loop, avoiding mutual recursion between helper functions that would exhaust the C stack. Exception propagation through suspended frames uses trampoline_handle_exception which adds traceback entries and calls unwind_blocks on each caller. Assisted-by: Claude --- crates/vm/src/builtins/function.rs | 6 +- crates/vm/src/coroutine.rs | 2 + crates/vm/src/frame.rs | 157 +++++++++++++++++- crates/vm/src/vm/mod.rs | 258 ++++++++++++++++++++++++++++- crates/vm/src/vm/thread.rs | 1 + 5 files changed, 416 insertions(+), 8 deletions(-) diff --git a/crates/vm/src/builtins/function.rs b/crates/vm/src/builtins/function.rs index 3cd8a767a11..0fc4119b805 100644 --- a/crates/vm/src/builtins/function.rs +++ b/crates/vm/src/builtins/function.rs @@ -64,9 +64,9 @@ fn format_missing_args( #[pyclass(module = false, name = "function", traverse = "manual")] #[derive(Debug)] pub struct PyFunction { - code: PyAtomicRef, - globals: PyDictRef, - builtins: PyObjectRef, + pub(crate) code: PyAtomicRef, + pub(crate) globals: PyDictRef, + pub(crate) builtins: PyObjectRef, pub(crate) closure: Option>>, defaults_and_kwdefaults: PyMutex<(Option, Option)>, name: PyMutex, diff --git a/crates/vm/src/coroutine.rs b/crates/vm/src/coroutine.rs index c7252c66d12..43a28320e00 100644 --- a/crates/vm/src/coroutine.rs +++ b/crates/vm/src/coroutine.rs @@ -23,6 +23,7 @@ impl ExecutionResult { }; PyIterReturn::StopIteration(arg) } + Self::TailCall => unreachable!("TailCall in generator/coroutine"), } } } @@ -104,6 +105,7 @@ impl Coro { self.clear_frame_locals_on_close(); } Ok(ExecutionResult::Yield(_)) => {} + Ok(ExecutionResult::TailCall) => unreachable!("TailCall in generator/coroutine"), } } diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index 44075c38348..67b0235752e 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -544,6 +544,13 @@ impl LocalsPlus { Ok(()) } + /// Push a PyObjectRef onto the evaluation stack. + /// Panics on overflow. + pub(crate) fn push_stack(&mut self, value: PyObjectRef) { + self.stack_try_push(Some(PyStackRef::new_owned(value))) + .unwrap_or_else(|_| panic!("stack overflow in push_stack")); + } + /// Pop a value from the evaluation stack. #[inline(always)] fn stack_pop(&mut self) -> Option { @@ -1456,6 +1463,10 @@ unsafe impl Traverse for FrameObject { pub enum ExecutionResult { Return(PyObjectRef), Yield(PyObjectRef), + /// The bytecode loop wants to tail-call into a new frame that has + /// already been prepared on the datastack. The trampoline reads the + /// pending frame pointer from `vm.pending_tailcall_frame`. + TailCall, } /// A valid execution result, or an exception @@ -2200,6 +2211,7 @@ impl Py { func_obj, prev_line: &iframe.prev_line, monitoring_mask: 0, + tailcall_enabled: false, }; f(exec) } @@ -2262,6 +2274,7 @@ impl Py { func_obj, prev_line: &iframe.prev_line, monitoring_mask: 0, + tailcall_enabled: false, }; exec.yield_from_target().map(PyObject::to_owned) } @@ -2303,6 +2316,69 @@ pub(crate) fn datastack_iframe_total_bytes(nlocalsplus: usize, stacksize: usize) .expect("datastack iframe total size overflow") } +/// Handle an exception propagating into a suspended caller frame in the +/// trampoline. Adds a traceback entry at the caller's call site, then +/// tries the caller's exception table via `unwind_blocks`. +/// +/// Returns: +/// - `Ok(None)` — handler found, the caller's `run_iframe` can be re-entered +/// - `Ok(Some(result))` — handler returned a result (break from the run loop) +/// - `Err(exc)` — no handler, exception propagates to the next caller +pub(crate) fn trampoline_handle_exception( + iframe: &mut InterpreterFrame, + exception: &PyBaseExceptionRef, + vm: &VirtualMachine, +) -> FrameResult { + let code: &Py = unsafe { &*iframe.code }; + let globals: &Py = unsafe { &*iframe.globals }; + let builtins: &PyObject = unsafe { &*iframe.builtins }; + let func_obj: Option<&PyObject> = if iframe.func_obj.is_null() { + None + } else { + Some(unsafe { &*iframe.func_obj }) + }; + let builtins_dict = if globals.class().is(vm.ctx.types.dict_type) { + builtins + .downcast_ref_if_exact::(vm) + .map(|d| unsafe { PyExact::ref_unchecked(d) }) + } else { + None + }; + let iframe_ptr = iframe as *const InterpreterFrame; + let mut exec = ExecutingFrame { + code, + localsplus: &mut iframe.localsplus, + locals: &iframe.locals, + globals, + builtins, + builtins_dict, + lasti: &iframe.lasti, + iframe: iframe_ptr, + func_obj, + prev_line: &mut iframe.prev_line, + monitoring_mask: 0, + tailcall_enabled: false, + }; + + // lasti points past the CallPyExactArgs instruction (+ cache entries). + // The exception occurred at the previous instruction (the call site). + let idx = exec.lasti() as usize - 1; + + // Add traceback entry at the call site. + if let Some((loc, _end_loc)) = exec.code.locations.get(idx) { + let next = exception.__traceback__(); + let new_traceback = PyTraceback::new(next, exec.frame_object(vm), idx as u32 * 2, loc.line); + exception.set_traceback_typed(Some(new_traceback.into_ref(&vm.ctx))); + } + + exec.unwind_blocks( + vm, + UnwindReason::Raising { + exception: exception.clone(), + }, + ) +} + /// Execute an InterpreterFrame's bytecode directly, without a FrameObject. /// /// # Safety @@ -2341,6 +2417,7 @@ pub(crate) fn run_iframe( func_obj, prev_line: &mut iframe.prev_line, monitoring_mask: 0, + tailcall_enabled: true, }; exec.run(vm) } @@ -2370,6 +2447,9 @@ pub(crate) struct ExecutingFrame<'a> { prev_line: &'a core::cell::Cell, /// Cached monitoring events mask. Reloaded at Resume instruction only, monitoring_mask: u32, + /// Whether TailCall is allowed. True when running under the trampoline + /// (`run_frame_fast`), false for FrameObject-based execution. + tailcall_enabled: bool, } #[inline] @@ -5771,7 +5851,11 @@ impl ExecutingFrame<'_> { if self.specialization_call_recursion_guard(vm) { return self.execute_call_vectorcall(nargs, vm); } - // Stage args without a per-call Vec: [self?, arg1, ..., argN] + if self.tailcall_enabled && !func.is_generator_like() { + self.tailcall_prepare_frame(nargs, self_or_null_is_some, vm); + return Ok(Some(ExecutionResult::TailCall)); + } + // Recursive path: pop args and call. let base = usize::from(self_or_null_is_some); let mut arg_buf = CallArgBuffer::new(nargs as usize + base); let args = arg_buf.slots(); @@ -10527,6 +10611,77 @@ impl ExecutingFrame<'_> { >= vm.recursion_limit.get() } + /// Prepare a callee frame on the datastack for a TailCall. + /// Pops args, self_or_null, and callable from the caller's stack, + /// builds the callee InterpreterFrame, and stores its pointer in + /// `vm.pending_tailcall_frame`. + /// + /// The callable must be at stack position `nargs + 1` (already validated). + fn tailcall_prepare_frame( + &mut self, + nargs: u32, + self_or_null_is_some: bool, + vm: &VirtualMachine, + ) { + // Pop args first, then self_or_null, then callable. + let base = usize::from(self_or_null_is_some); + let effective_nargs = nargs as usize + base; + + // Collect args into a small buffer. + let mut arg_buf = CallArgBuffer::new(effective_nargs); + let arg_slots = arg_buf.slots(); + for (slot, arg) in arg_slots[base..] + .iter_mut() + .zip(self.pop_multiple(nargs as usize)) + { + *slot = Some(arg); + } + let self_or_null = self.pop_value_opt(); + debug_assert_eq!(self_or_null.is_some(), self_or_null_is_some); + if self_or_null.is_some() { + arg_slots[0] = self_or_null; + } + let callable = self.pop_value(); + let func = callable.downcast_ref_if_exact::(vm).unwrap(); + + let code: &Py = &func.code; + + let locals = if code.flags.contains(bytecode::CodeFlags::NEWLOCALS) { + FrameLocals::lazy() + } else { + FrameLocals::with_locals(crate::function::ArgMapping::from_dict_exact( + func.globals.clone(), + )) + }; + + let callee_iframe = InterpreterFrame::new_on_datastack( + code, + &func.globals, + &func.builtins, + Some(func.as_object()), + locals, + func.closure.as_ref().map_or(&[], |c| c.as_slice()), + vm, + ); + + // Fill arguments from the pre-popped arg buffer into fastlocals. + { + let fastlocals = callee_iframe.localsplus.fastlocals_mut(); + for (dst, src) in fastlocals[..effective_nargs] + .iter_mut() + .zip(arg_slots.iter_mut()) + { + *dst = src.take(); + } + } + // Keep the callable alive — the callee iframe borrows code/globals/ + // builtins from the PyFunction via raw pointers. + callee_iframe.temporary_refs.lock().push(callable); + + vm.pending_tailcall_frame + .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); + } + #[inline] fn for_iter_has_end_for_shape(&self, instr_idx: usize, jump_delta: u32) -> bool { let target_idx = instr_idx diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 0d419134342..48fb1ee1547 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -106,6 +106,11 @@ pub struct VirtualMachine { pub asyncio_running_task: RefCell>, pub(crate) callable_cache: CallableCache, pub(crate) audit_hooks: RefCell>, + /// Side channel for TailCall: the bytecode loop stores the new frame + /// pointer here before returning `ExecutionResult::TailCall`. + /// Wrapped in `SendPtr` for `Send` safety — the pointer is only + /// ever read on the same thread that wrote it. + pub(crate) pending_tailcall_frame: Cell>, } /// Non-owning frame pointer for the non-unix threading frames stack. @@ -777,6 +782,33 @@ pub fn process_hash_secret_seed() -> u32 { *SEED.get_or_init(|| u32::from_ne_bytes(rustpython_common::rand::os_random())) } +/// A `*mut T` wrapper that implements `Send`. +/// Only valid when the pointer is exclusively used by one thread at a time. +#[repr(transparent)] +pub(crate) struct SendPtr(pub(crate) *mut T); + +impl Copy for SendPtr {} +impl Clone for SendPtr { + fn clone(&self) -> Self { + *self + } +} + +// SAFETY: `pending_tailcall_frame` is per-thread (set by the bytecode loop, +// read by the trampoline on the same thread). The VirtualMachine is per-thread. +unsafe impl Send for SendPtr {} +// SAFETY: The pointer is never accessed from multiple threads concurrently. +unsafe impl Sync for SendPtr {} + +impl SendPtr { + const fn null() -> Self { + Self(core::ptr::null_mut()) + } + fn get(self) -> *mut T { + self.0 + } +} + /// Saved state from `enter_iframe`, needed by `exit_iframe` to restore /// the previous frame chain and exception state. pub(crate) struct IframeEntryState { @@ -786,6 +818,12 @@ pub(crate) struct IframeEntryState { pub(crate) save_exc: bool, } +/// Caller frame suspended by a TailCall in the trampoline. +struct SuspendedFrame { + iframe: *mut crate::frame::InterpreterFrame, + entry_state: IframeEntryState, +} + impl VirtualMachine { fn init_callable_cache(&mut self) -> PyResult<()> { self.callable_cache.len = Some(self.builtins.get_attr("len", self)?); @@ -900,6 +938,7 @@ impl VirtualMachine { asyncio_running_task: RefCell::new(None), callable_cache: CallableCache::default(), audit_hooks: RefCell::new(vec![]), + pending_tailcall_frame: Cell::new(SendPtr::null()), }; if vm.state.hash_secret.hash_str("") @@ -1342,11 +1381,222 @@ impl VirtualMachine { #[inline(always)] /// Run a stack-allocated InterpreterFrame without heap allocation. - /// This is the fast path for regular (non-generator) function calls. + /// Uses a trampoline loop to flatten Python-to-Python calls: when the + /// bytecode loop returns `TailCall`, the trampoline swaps to the new + /// frame without adding a Rust stack frame. pub fn run_frame_fast(&self, iframe: &mut crate::frame::InterpreterFrame) -> PyResult { - match self.with_iframe(iframe, |iframe| crate::frame::run_iframe(iframe, self))? { - ExecutionResult::Return(value) => Ok(value), - _ => panic!("Got unexpected result from function"), + use crate::frame::ExecutionResult; + + let entry_state = self.enter_iframe(iframe)?; + let result = crate::frame::run_iframe(iframe, self); + + match result { + Ok(ExecutionResult::Return(value)) => { + self.exit_iframe(entry_state); + Ok(value) + } + Ok(ExecutionResult::TailCall) => self.run_frame_fast_trampoline(iframe, entry_state), + Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"), + Err(exc) => { + self.exit_iframe(entry_state); + Err(exc) + } + } + } + + /// Cold path: at least one TailCall was issued. Run the trampoline. + /// All frame dispatch happens in this single loop — no mutual recursion + /// between helper functions, so C stack depth is bounded. + #[cold] + #[inline(never)] + fn run_frame_fast_trampoline( + &self, + iframe: &mut crate::frame::InterpreterFrame, + entry_state: IframeEntryState, + ) -> PyResult { + use crate::frame::ExecutionResult; + + let mut frame_stack: Vec = Vec::with_capacity(8); + frame_stack.push(SuspendedFrame { + iframe: iframe as *mut crate::frame::InterpreterFrame, + entry_state, + }); + + // What we need to do next. + enum Action { + /// Enter and run a new callee frame (pointer from pending_tailcall_frame). + EnterCallee(*mut crate::frame::InterpreterFrame), + /// Push a return value onto the next caller and re-enter it. + ReturnValue(PyObjectRef), + /// Propagate an exception through suspended callers. + Unwind(PyBaseExceptionRef), + } + + let initial_ptr = self.pending_tailcall_frame.get().get(); + debug_assert!(!initial_ptr.is_null()); + self.pending_tailcall_frame.set(SendPtr::null()); + let mut action = Action::EnterCallee(initial_ptr); + + loop { + match action { + Action::EnterCallee(callee_ptr) => { + let callee = unsafe { &mut *callee_ptr }; + let callee_entry = match self.enter_iframe(callee) { + Ok(state) => state, + Err(exc) => { + unsafe { + if let Some(base) = callee.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::Unwind(exc); + continue; + } + }; + + let result = crate::frame::run_iframe(callee, self); + match result { + Ok(ExecutionResult::TailCall) => { + frame_stack.push(SuspendedFrame { + iframe: callee_ptr, + entry_state: callee_entry, + }); + let next = self.pending_tailcall_frame.get().get(); + debug_assert!(!next.is_null()); + self.pending_tailcall_frame.set(SendPtr::null()); + action = Action::EnterCallee(next); + } + Ok(ExecutionResult::Return(value)) => { + self.exit_iframe(callee_entry); + unsafe { + if let Some(base) = callee.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::ReturnValue(value); + } + Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"), + Err(exc) => { + self.exit_iframe(callee_entry); + unsafe { + if let Some(base) = callee.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::Unwind(exc); + } + } + } + + Action::ReturnValue(value) => { + let Some(caller) = frame_stack.pop() else { + // All frames consumed — this is the final return. + return Ok(value); + }; + let caller_iframe = unsafe { &mut *caller.iframe }; + caller_iframe.localsplus.push_stack(value); + + let result = crate::frame::run_iframe(caller_iframe, self); + match result { + Ok(ExecutionResult::TailCall) => { + frame_stack.push(caller); + let next = self.pending_tailcall_frame.get().get(); + debug_assert!(!next.is_null()); + self.pending_tailcall_frame.set(SendPtr::null()); + action = Action::EnterCallee(next); + } + Ok(ExecutionResult::Return(value)) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::ReturnValue(value); + } + Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"), + Err(exc) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::Unwind(exc); + } + } + } + + Action::Unwind(exc) => { + let Some(caller) = frame_stack.pop() else { + return Err(exc); + }; + let caller_iframe = unsafe { &mut *caller.iframe }; + + let handled = + crate::frame::trampoline_handle_exception(caller_iframe, &exc, self); + + match handled { + Ok(None) => { + // Handler found — resume the caller's dispatch loop. + let result = crate::frame::run_iframe(caller_iframe, self); + match result { + Ok(ExecutionResult::TailCall) => { + frame_stack.push(caller); + let next = self.pending_tailcall_frame.get().get(); + debug_assert!(!next.is_null()); + self.pending_tailcall_frame.set(SendPtr::null()); + action = Action::EnterCallee(next); + } + Ok(ExecutionResult::Return(value)) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() + { + self.datastack_pop(base); + } + } + action = Action::ReturnValue(value); + } + Ok(ExecutionResult::Yield(_)) => { + panic!("Yield in non-generator frame") + } + Err(new_exc) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() + { + self.datastack_pop(base); + } + } + action = Action::Unwind(new_exc); + } + } + } + Ok(Some(ExecutionResult::Return(value))) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::ReturnValue(value); + } + Ok(Some(_)) => { + panic!("Unexpected execution result in trampoline unwind") + } + Err(new_exc) => { + self.exit_iframe(caller.entry_state); + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::Unwind(new_exc); + } + } + } + } } } diff --git a/crates/vm/src/vm/thread.rs b/crates/vm/src/vm/thread.rs index 1520b2cd883..4ec35d74a84 100644 --- a/crates/vm/src/vm/thread.rs +++ b/crates/vm/src/vm/thread.rs @@ -1022,6 +1022,7 @@ impl VirtualMachine { asyncio_running_task: RefCell::new(None), callable_cache: self.callable_cache.clone(), audit_hooks: RefCell::new(vec![]), + pending_tailcall_frame: Cell::new(super::SendPtr::null()), }; ThreadedVirtualMachine { vm } } From cdc0d32ed9d6b739b2a311ddc5fbe384c7608cea Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 18:35:04 +0900 Subject: [PATCH 04/10] Optimize trampoline: skip recursion check, avoid temporary_refs mutex, add bound method TailCall - Add enter_iframe_unchecked for trampoline callee entry (recursion already checked by specialization_call_recursion_guard) - Move callable ownership from per-frame temporary_refs mutex to trampoline-local SuspendedFrame.owned_refs via VM side channel - Add TailCall support for CallBoundMethodExactArgs - Move args directly from caller stack to callee fastlocals - Read materialized pointer once in exit_iframe Incremental call overhead: ~55 ns -> ~35 ns Assisted-by: Claude --- crates/vm/src/frame.rs | 111 +++++++++++++++++++++++++++++-------- crates/vm/src/vm/mod.rs | 109 ++++++++++++++++++++++++++---------- crates/vm/src/vm/thread.rs | 1 + 3 files changed, 168 insertions(+), 53 deletions(-) diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index 67b0235752e..26d85be3087 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -5912,7 +5912,16 @@ impl ExecutingFrame<'_> { if self.specialization_call_recursion_guard(vm) { return self.execute_call_vectorcall(nargs, vm); } - // Stage args without a per-call Vec: + if self.tailcall_enabled && !func.is_generator_like() { + self.tailcall_prepare_bound_method_frame( + nargs, + bound_function, + bound_self, + vm, + ); + return Ok(Some(ExecutionResult::TailCall)); + } + // Recursive path: stage args without a per-call Vec. // [bound_self, arg1, ..., argN] let mut arg_buf = CallArgBuffer::new(nargs as usize + 1); let args = arg_buf.slots(); @@ -10623,25 +10632,13 @@ impl ExecutingFrame<'_> { self_or_null_is_some: bool, vm: &VirtualMachine, ) { - // Pop args first, then self_or_null, then callable. let base = usize::from(self_or_null_is_some); let effective_nargs = nargs as usize + base; - // Collect args into a small buffer. - let mut arg_buf = CallArgBuffer::new(effective_nargs); - let arg_slots = arg_buf.slots(); - for (slot, arg) in arg_slots[base..] - .iter_mut() - .zip(self.pop_multiple(nargs as usize)) - { - *slot = Some(arg); - } - let self_or_null = self.pop_value_opt(); - debug_assert_eq!(self_or_null.is_some(), self_or_null_is_some); - if self_or_null.is_some() { - arg_slots[0] = self_or_null; - } - let callable = self.pop_value(); + // Peek at the callable (still on the stack) to build the callee + // frame. The callable stays on the caller's stack until we're done + // constructing the callee. + let callable = self.nth_value(nargs + 1); let func = callable.downcast_ref_if_exact::(vm).unwrap(); let code: &Py = &func.code; @@ -10664,19 +10661,85 @@ impl ExecutingFrame<'_> { vm, ); - // Fill arguments from the pre-popped arg buffer into fastlocals. + // Move args directly from the caller's stack into callee fastlocals, + // avoiding an intermediate buffer. { let fastlocals = callee_iframe.localsplus.fastlocals_mut(); - for (dst, src) in fastlocals[..effective_nargs] + for (dst, arg) in fastlocals[base..effective_nargs] .iter_mut() - .zip(arg_slots.iter_mut()) + .zip(self.pop_multiple(nargs as usize)) { - *dst = src.take(); + *dst = Some(arg); + } + let self_or_null = self.pop_value_opt(); + debug_assert_eq!(self_or_null.is_some(), self_or_null_is_some); + if self_or_null.is_some() { + fastlocals[0] = self_or_null; } } - // Keep the callable alive — the callee iframe borrows code/globals/ - // builtins from the PyFunction via raw pointers. - callee_iframe.temporary_refs.lock().push(callable); + + // Pop the callable and transfer ownership to the trampoline via + // the VM side channel, avoiding a per-frame mutex lock on + // temporary_refs. + let callable = self.pop_value(); + vm.pending_tailcall_refs.borrow_mut().push(callable); + + vm.pending_tailcall_frame + .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); + } + + /// Prepare a callee frame for a bound method TailCall. + /// Pops args, self_or_null (null), and callable from the caller's stack, + /// builds the callee InterpreterFrame with bound_self prepended, and + /// stores its pointer in `vm.pending_tailcall_frame`. + fn tailcall_prepare_bound_method_frame( + &mut self, + nargs: u32, + bound_function: PyObjectRef, + bound_self: PyObjectRef, + vm: &VirtualMachine, + ) { + let effective_nargs = nargs as usize + 1; // +1 for bound_self + + let func = bound_function + .downcast_ref_if_exact::(vm) + .unwrap(); + let code: &Py = &func.code; + + let locals = if code.flags.contains(bytecode::CodeFlags::NEWLOCALS) { + FrameLocals::lazy() + } else { + FrameLocals::with_locals(crate::function::ArgMapping::from_dict_exact( + func.globals.clone(), + )) + }; + + let callee_iframe = InterpreterFrame::new_on_datastack( + code, + &func.globals, + &func.builtins, + Some(func.as_object()), + locals, + func.closure.as_ref().map_or(&[], |c| c.as_slice()), + vm, + ); + + // Move args directly from the caller's stack into callee fastlocals. + let fastlocals = callee_iframe.localsplus.fastlocals_mut(); + for (dst, arg) in fastlocals[1..effective_nargs] + .iter_mut() + .zip(self.pop_multiple(nargs as usize)) + { + *dst = Some(arg); + } + self.pop_value_opt(); // null (self_or_null) + let callable = self.pop_value(); // callable (bound method) + fastlocals[0] = Some(bound_self); + + // Transfer ownership to the trampoline via the VM side channel. + let mut refs = vm.pending_tailcall_refs.borrow_mut(); + refs.push(bound_function); + refs.push(callable); vm.pending_tailcall_frame .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 48fb1ee1547..f7a127678c0 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -111,6 +111,11 @@ pub struct VirtualMachine { /// Wrapped in `SendPtr` for `Send` safety — the pointer is only /// ever read on the same thread that wrote it. pub(crate) pending_tailcall_frame: Cell>, + /// Owned references that keep callee raw pointers valid during TailCall. + /// Set by `tailcall_prepare_frame`, drained by the trampoline into + /// its local `owned_refs` Vec. This avoids per-frame mutex lock on + /// `temporary_refs`. + pub(crate) pending_tailcall_refs: RefCell>, } /// Non-owning frame pointer for the non-unix threading frames stack. @@ -822,6 +827,11 @@ pub(crate) struct IframeEntryState { struct SuspendedFrame { iframe: *mut crate::frame::InterpreterFrame, entry_state: IframeEntryState, + /// Owned references that keep callee's raw pointers (code, globals, + /// builtins borrowed from PyFunction) valid. Drained from + /// `vm.pending_tailcall_refs` when the callee's TailCall is consumed. + /// Dropped when this SuspendedFrame is popped (after callee returns/errors). + owned_refs: Vec, } impl VirtualMachine { @@ -939,6 +949,7 @@ impl VirtualMachine { callable_cache: CallableCache::default(), audit_hooks: RefCell::new(vec![]), pending_tailcall_frame: Cell::new(SendPtr::null()), + pending_tailcall_refs: RefCell::new(Vec::with_capacity(2)), }; if vm.state.hash_secret.hash_str("") @@ -1417,10 +1428,6 @@ impl VirtualMachine { use crate::frame::ExecutionResult; let mut frame_stack: Vec = Vec::with_capacity(8); - frame_stack.push(SuspendedFrame { - iframe: iframe as *mut crate::frame::InterpreterFrame, - entry_state, - }); // What we need to do next. enum Action { @@ -1435,31 +1442,29 @@ impl VirtualMachine { let initial_ptr = self.pending_tailcall_frame.get().get(); debug_assert!(!initial_ptr.is_null()); self.pending_tailcall_frame.set(SendPtr::null()); + // Drain the refs that keep the initial callee's raw pointers alive. + let initial_refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + frame_stack.push(SuspendedFrame { + iframe: iframe as *mut crate::frame::InterpreterFrame, + entry_state, + owned_refs: initial_refs, + }); let mut action = Action::EnterCallee(initial_ptr); loop { match action { Action::EnterCallee(callee_ptr) => { let callee = unsafe { &mut *callee_ptr }; - let callee_entry = match self.enter_iframe(callee) { - Ok(state) => state, - Err(exc) => { - unsafe { - if let Some(base) = callee.release_datastack_frame() { - self.datastack_pop(base); - } - } - action = Action::Unwind(exc); - continue; - } - }; + let callee_entry = self.enter_iframe_unchecked(callee); let result = crate::frame::run_iframe(callee, self); match result { Ok(ExecutionResult::TailCall) => { + let refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); frame_stack.push(SuspendedFrame { iframe: callee_ptr, entry_state: callee_entry, + owned_refs: refs, }); let next = self.pending_tailcall_frame.get().get(); debug_assert!(!next.is_null()); @@ -1493,20 +1498,32 @@ impl VirtualMachine { // All frames consumed — this is the final return. return Ok(value); }; - let caller_iframe = unsafe { &mut *caller.iframe }; + let SuspendedFrame { + iframe: caller_iframe_ptr, + entry_state: caller_entry, + owned_refs: _caller_refs, + } = caller; + let caller_iframe = unsafe { &mut *caller_iframe_ptr }; caller_iframe.localsplus.push_stack(value); let result = crate::frame::run_iframe(caller_iframe, self); match result { Ok(ExecutionResult::TailCall) => { - frame_stack.push(caller); + let refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + drop(_caller_refs); + frame_stack.push(SuspendedFrame { + iframe: caller_iframe_ptr, + entry_state: caller_entry, + owned_refs: refs, + }); let next = self.pending_tailcall_frame.get().get(); debug_assert!(!next.is_null()); self.pending_tailcall_frame.set(SendPtr::null()); action = Action::EnterCallee(next); } Ok(ExecutionResult::Return(value)) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { self.datastack_pop(base); @@ -1516,7 +1533,8 @@ impl VirtualMachine { } Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"), Err(exc) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { self.datastack_pop(base); @@ -1531,7 +1549,12 @@ impl VirtualMachine { let Some(caller) = frame_stack.pop() else { return Err(exc); }; - let caller_iframe = unsafe { &mut *caller.iframe }; + let SuspendedFrame { + iframe: caller_iframe_ptr, + entry_state: caller_entry, + owned_refs: _caller_refs, + } = caller; + let caller_iframe = unsafe { &mut *caller_iframe_ptr }; let handled = crate::frame::trampoline_handle_exception(caller_iframe, &exc, self); @@ -1542,14 +1565,22 @@ impl VirtualMachine { let result = crate::frame::run_iframe(caller_iframe, self); match result { Ok(ExecutionResult::TailCall) => { - frame_stack.push(caller); + let refs = + self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + drop(_caller_refs); + frame_stack.push(SuspendedFrame { + iframe: caller_iframe_ptr, + entry_state: caller_entry, + owned_refs: refs, + }); let next = self.pending_tailcall_frame.get().get(); debug_assert!(!next.is_null()); self.pending_tailcall_frame.set(SendPtr::null()); action = Action::EnterCallee(next); } Ok(ExecutionResult::Return(value)) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { @@ -1562,7 +1593,8 @@ impl VirtualMachine { panic!("Yield in non-generator frame") } Err(new_exc) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { @@ -1574,7 +1606,8 @@ impl VirtualMachine { } } Ok(Some(ExecutionResult::Return(value))) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { self.datastack_pop(base); @@ -1586,7 +1619,8 @@ impl VirtualMachine { panic!("Unexpected execution result in trampoline unwind") } Err(new_exc) => { - self.exit_iframe(caller.entry_state); + drop(_caller_refs); + self.exit_iframe(caller_entry); unsafe { if let Some(base) = caller_iframe.release_datastack_frame() { self.datastack_pop(base); @@ -2079,6 +2113,17 @@ impl VirtualMachine { return Err(self.new_recursion_error(String::new())); } + Ok(self.enter_iframe_unchecked(iframe)) + } + + /// Like `enter_iframe` but skips recursion and C-stack checks. + /// Used by the trampoline where the caller has already verified + /// recursion depth and all execution stays in one Rust stack frame. + #[inline(always)] + pub(crate) fn enter_iframe_unchecked( + &self, + iframe: &mut crate::frame::InterpreterFrame, + ) -> IframeEntryState { self.recursion_depth.update(|d| d + 1); let iframe_ptr = iframe as *const crate::frame::InterpreterFrame; @@ -2097,12 +2142,12 @@ impl VirtualMachine { None }; - Ok(IframeEntryState { + IframeEntryState { iframe_ptr, old_chain, saved_exc, save_exc, - }) + } } /// Pop `iframe` from the frame chain: sync materialized state, restore @@ -2115,6 +2160,13 @@ impl VirtualMachine { save_exc, } = state; + // Read the materialized pointer once via read_volatile (bypasses + // LLVM's noalias on the &mut iframe borrow). + let mat_ptr = unsafe { + let field_ptr = core::ptr::addr_of!((*iframe_ptr).materialized); + core::ptr::read_volatile(field_ptr as *const usize) + }; + // If this iframe was materialized, capture f_back so that code // holding a reference to the FrameObject can walk the chain after // return. Read materialized through read_volatile to bypass @@ -2151,7 +2203,6 @@ impl VirtualMachine { core::sync::atomic::Ordering::Release, ); } - } if save_exc { self.restore_exception(saved_exc); diff --git a/crates/vm/src/vm/thread.rs b/crates/vm/src/vm/thread.rs index 4ec35d74a84..7073acd3aa1 100644 --- a/crates/vm/src/vm/thread.rs +++ b/crates/vm/src/vm/thread.rs @@ -1023,6 +1023,7 @@ impl VirtualMachine { callable_cache: self.callable_cache.clone(), audit_hooks: RefCell::new(vec![]), pending_tailcall_frame: Cell::new(super::SendPtr::null()), + pending_tailcall_refs: RefCell::new(Vec::with_capacity(2)), }; ThreadedVirtualMachine { vm } } From d00c2506f08e54764589fac844a716ac829fbeeb Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 19:40:03 +0900 Subject: [PATCH 05/10] Use UnsafeCell for pending_tailcall_refs The VM is per-thread so RefCell's runtime borrow checking is unnecessary overhead. Replace with UnsafeCell for direct access. Assisted-by: Claude --- crates/vm/src/frame.rs | 4 ++-- crates/vm/src/vm/mod.rs | 25 ++++++++++++++++--------- crates/vm/src/vm/thread.rs | 2 +- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index 26d85be3087..6c1a83b7286 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -10682,7 +10682,7 @@ impl ExecutingFrame<'_> { // the VM side channel, avoiding a per-frame mutex lock on // temporary_refs. let callable = self.pop_value(); - vm.pending_tailcall_refs.borrow_mut().push(callable); + unsafe { &mut *vm.pending_tailcall_refs.get() }.push(callable); vm.pending_tailcall_frame .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); @@ -10737,7 +10737,7 @@ impl ExecutingFrame<'_> { fastlocals[0] = Some(bound_self); // Transfer ownership to the trampoline via the VM side channel. - let mut refs = vm.pending_tailcall_refs.borrow_mut(); + let refs = unsafe { &mut *vm.pending_tailcall_refs.get() }; refs.push(bound_function); refs.push(callable); diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index f7a127678c0..8f9a7c16f4b 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -113,9 +113,9 @@ pub struct VirtualMachine { pub(crate) pending_tailcall_frame: Cell>, /// Owned references that keep callee raw pointers valid during TailCall. /// Set by `tailcall_prepare_frame`, drained by the trampoline into - /// its local `owned_refs` Vec. This avoids per-frame mutex lock on - /// `temporary_refs`. - pub(crate) pending_tailcall_refs: RefCell>, + /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is + /// per-thread and this field is only accessed on the owning thread. + pub(crate) pending_tailcall_refs: core::cell::UnsafeCell>, } /// Non-owning frame pointer for the non-unix threading frames stack. @@ -949,7 +949,7 @@ impl VirtualMachine { callable_cache: CallableCache::default(), audit_hooks: RefCell::new(vec![]), pending_tailcall_frame: Cell::new(SendPtr::null()), - pending_tailcall_refs: RefCell::new(Vec::with_capacity(2)), + pending_tailcall_refs: core::cell::UnsafeCell::new(Vec::with_capacity(2)), }; if vm.state.hash_secret.hash_str("") @@ -1443,7 +1443,9 @@ impl VirtualMachine { debug_assert!(!initial_ptr.is_null()); self.pending_tailcall_frame.set(SendPtr::null()); // Drain the refs that keep the initial callee's raw pointers alive. - let initial_refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + let initial_refs = unsafe { &mut *self.pending_tailcall_refs.get() } + .drain(..) + .collect(); frame_stack.push(SuspendedFrame { iframe: iframe as *mut crate::frame::InterpreterFrame, entry_state, @@ -1460,7 +1462,9 @@ impl VirtualMachine { let result = crate::frame::run_iframe(callee, self); match result { Ok(ExecutionResult::TailCall) => { - let refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + let refs = unsafe { &mut *self.pending_tailcall_refs.get() } + .drain(..) + .collect(); frame_stack.push(SuspendedFrame { iframe: callee_ptr, entry_state: callee_entry, @@ -1509,7 +1513,9 @@ impl VirtualMachine { let result = crate::frame::run_iframe(caller_iframe, self); match result { Ok(ExecutionResult::TailCall) => { - let refs = self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + let refs = unsafe { &mut *self.pending_tailcall_refs.get() } + .drain(..) + .collect(); drop(_caller_refs); frame_stack.push(SuspendedFrame { iframe: caller_iframe_ptr, @@ -1565,8 +1571,9 @@ impl VirtualMachine { let result = crate::frame::run_iframe(caller_iframe, self); match result { Ok(ExecutionResult::TailCall) => { - let refs = - self.pending_tailcall_refs.borrow_mut().drain(..).collect(); + let refs = unsafe { &mut *self.pending_tailcall_refs.get() } + .drain(..) + .collect(); drop(_caller_refs); frame_stack.push(SuspendedFrame { iframe: caller_iframe_ptr, diff --git a/crates/vm/src/vm/thread.rs b/crates/vm/src/vm/thread.rs index 7073acd3aa1..aaf4a3918ba 100644 --- a/crates/vm/src/vm/thread.rs +++ b/crates/vm/src/vm/thread.rs @@ -1023,7 +1023,7 @@ impl VirtualMachine { callable_cache: self.callable_cache.clone(), audit_hooks: RefCell::new(vec![]), pending_tailcall_frame: Cell::new(super::SendPtr::null()), - pending_tailcall_refs: RefCell::new(Vec::with_capacity(2)), + pending_tailcall_refs: core::cell::UnsafeCell::new(Vec::with_capacity(2)), }; ThreadedVirtualMachine { vm } } From e0fea234d2b07aeed470eafd39d8dce8beec4326 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sat, 1 Aug 2026 19:42:44 +0900 Subject: [PATCH 06/10] Remove .claude/settings.json from tracking Assisted-by: Claude --- .claude/settings.json | 15 --------------- 1 file changed, 15 deletions(-) delete mode 100644 .claude/settings.json diff --git a/.claude/settings.json b/.claude/settings.json deleted file mode 100644 index 22f0a9a8a01..00000000000 --- a/.claude/settings.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "hooks": { - "SessionStart": [ - { - "matcher": "", - "hooks": [ - { - "type": "command", - "command": "bash .claude/scripts/setup-env.sh" - } - ] - } - ] - } -} From 80028a80a68c74c4a359240d21cb75d8750e52ae Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sun, 2 Aug 2026 14:27:58 +0900 Subject: [PATCH 07/10] Replace SendPtr with Option for type-safe pending tailcall Use NonNull + Option instead of raw *mut T with manual null checks. The compiler enforces non-null via the type system, and Option has the same size as a raw pointer thanks to niche optimization. Also extract take_pending_tailcall helper to deduplicate the pattern. Assisted-by: Claude --- crates/vm/src/frame.rs | 8 +++-- crates/vm/src/vm/mod.rs | 64 ++++++++++++++++---------------------- crates/vm/src/vm/thread.rs | 2 +- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index 6c1a83b7286..d8410ec67aa 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -10685,7 +10685,9 @@ impl ExecutingFrame<'_> { unsafe { &mut *vm.pending_tailcall_refs.get() }.push(callable); vm.pending_tailcall_frame - .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); + .set(Some(crate::vm::SendNonNull(core::ptr::NonNull::from( + &mut *callee_iframe, + )))); } /// Prepare a callee frame for a bound method TailCall. @@ -10742,7 +10744,9 @@ impl ExecutingFrame<'_> { refs.push(callable); vm.pending_tailcall_frame - .set(crate::vm::SendPtr(callee_iframe as *mut InterpreterFrame)); + .set(Some(crate::vm::SendNonNull(core::ptr::NonNull::from( + &mut *callee_iframe, + )))); } #[inline] diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 8f9a7c16f4b..8472fafbaf9 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -108,9 +108,7 @@ pub struct VirtualMachine { pub(crate) audit_hooks: RefCell>, /// Side channel for TailCall: the bytecode loop stores the new frame /// pointer here before returning `ExecutionResult::TailCall`. - /// Wrapped in `SendPtr` for `Send` safety — the pointer is only - /// ever read on the same thread that wrote it. - pub(crate) pending_tailcall_frame: Cell>, + pub(crate) pending_tailcall_frame: Cell>>, /// Owned references that keep callee raw pointers valid during TailCall. /// Set by `tailcall_prepare_frame`, drained by the trampoline into /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is @@ -787,32 +785,23 @@ pub fn process_hash_secret_seed() -> u32 { *SEED.get_or_init(|| u32::from_ne_bytes(rustpython_common::rand::os_random())) } -/// A `*mut T` wrapper that implements `Send`. -/// Only valid when the pointer is exclusively used by one thread at a time. +/// A `NonNull` wrapper that implements `Send + Sync`. +/// Only valid when the pointer is exclusively used by one thread at a time +/// (the VirtualMachine is per-thread). #[repr(transparent)] -pub(crate) struct SendPtr(pub(crate) *mut T); +pub(crate) struct SendNonNull(pub(crate) core::ptr::NonNull); -impl Copy for SendPtr {} -impl Clone for SendPtr { +impl Copy for SendNonNull {} +impl Clone for SendNonNull { fn clone(&self) -> Self { *self } } -// SAFETY: `pending_tailcall_frame` is per-thread (set by the bytecode loop, -// read by the trampoline on the same thread). The VirtualMachine is per-thread. -unsafe impl Send for SendPtr {} -// SAFETY: The pointer is never accessed from multiple threads concurrently. -unsafe impl Sync for SendPtr {} - -impl SendPtr { - const fn null() -> Self { - Self(core::ptr::null_mut()) - } - fn get(self) -> *mut T { - self.0 - } -} +// SAFETY: The VirtualMachine is per-thread; the pointer is only ever +// accessed on the thread that wrote it. +unsafe impl Send for SendNonNull {} +unsafe impl Sync for SendNonNull {} /// Saved state from `enter_iframe`, needed by `exit_iframe` to restore /// the previous frame chain and exception state. @@ -948,7 +937,7 @@ impl VirtualMachine { asyncio_running_task: RefCell::new(None), callable_cache: CallableCache::default(), audit_hooks: RefCell::new(vec![]), - pending_tailcall_frame: Cell::new(SendPtr::null()), + pending_tailcall_frame: Cell::new(None), pending_tailcall_refs: core::cell::UnsafeCell::new(Vec::with_capacity(2)), }; @@ -1390,6 +1379,16 @@ impl VirtualMachine { } } + /// Take the pending tailcall frame pointer, resetting the side channel. + #[inline(always)] + fn take_pending_tailcall(&self) -> *mut crate::frame::InterpreterFrame { + self.pending_tailcall_frame + .take() + .expect("TailCall without pending frame") + .0 + .as_ptr() + } + #[inline(always)] /// Run a stack-allocated InterpreterFrame without heap allocation. /// Uses a trampoline loop to flatten Python-to-Python calls: when the @@ -1439,9 +1438,7 @@ impl VirtualMachine { Unwind(PyBaseExceptionRef), } - let initial_ptr = self.pending_tailcall_frame.get().get(); - debug_assert!(!initial_ptr.is_null()); - self.pending_tailcall_frame.set(SendPtr::null()); + let initial_ptr = self.take_pending_tailcall(); // Drain the refs that keep the initial callee's raw pointers alive. let initial_refs = unsafe { &mut *self.pending_tailcall_refs.get() } .drain(..) @@ -1470,10 +1467,7 @@ impl VirtualMachine { entry_state: callee_entry, owned_refs: refs, }); - let next = self.pending_tailcall_frame.get().get(); - debug_assert!(!next.is_null()); - self.pending_tailcall_frame.set(SendPtr::null()); - action = Action::EnterCallee(next); + action = Action::EnterCallee(self.take_pending_tailcall()); } Ok(ExecutionResult::Return(value)) => { self.exit_iframe(callee_entry); @@ -1522,10 +1516,7 @@ impl VirtualMachine { entry_state: caller_entry, owned_refs: refs, }); - let next = self.pending_tailcall_frame.get().get(); - debug_assert!(!next.is_null()); - self.pending_tailcall_frame.set(SendPtr::null()); - action = Action::EnterCallee(next); + action = Action::EnterCallee(self.take_pending_tailcall()); } Ok(ExecutionResult::Return(value)) => { drop(_caller_refs); @@ -1580,10 +1571,7 @@ impl VirtualMachine { entry_state: caller_entry, owned_refs: refs, }); - let next = self.pending_tailcall_frame.get().get(); - debug_assert!(!next.is_null()); - self.pending_tailcall_frame.set(SendPtr::null()); - action = Action::EnterCallee(next); + action = Action::EnterCallee(self.take_pending_tailcall()); } Ok(ExecutionResult::Return(value)) => { drop(_caller_refs); diff --git a/crates/vm/src/vm/thread.rs b/crates/vm/src/vm/thread.rs index aaf4a3918ba..1bab539a0a6 100644 --- a/crates/vm/src/vm/thread.rs +++ b/crates/vm/src/vm/thread.rs @@ -1022,7 +1022,7 @@ impl VirtualMachine { asyncio_running_task: RefCell::new(None), callable_cache: self.callable_cache.clone(), audit_hooks: RefCell::new(vec![]), - pending_tailcall_frame: Cell::new(super::SendPtr::null()), + pending_tailcall_frame: Cell::new(None), pending_tailcall_refs: core::cell::UnsafeCell::new(Vec::with_capacity(2)), }; ThreadedVirtualMachine { vm } From 0bb5668d30ae58b3a6eca770745ad2a852fb2a17 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sun, 2 Aug 2026 14:37:16 +0900 Subject: [PATCH 08/10] Restrict PendingFrame to private, expose only set/take methods Rename SendNonNull to PendingFrame and make it fully private: the struct, its field, and the pending_tailcall_frame Cell are all non-pub. External code accesses the side channel only through set_pending_tailcall (pub(crate)) and take_pending_tailcall (private). This ensures the unsafe Send+Sync impl cannot be reused elsewhere without justifying a new safety argument. Assisted-by: Claude --- crates/vm/src/frame.rs | 10 ++-------- crates/vm/src/vm/mod.rs | 42 +++++++++++++++++++++++++++++++---------- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index d8410ec67aa..46efe784390 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -10684,10 +10684,7 @@ impl ExecutingFrame<'_> { let callable = self.pop_value(); unsafe { &mut *vm.pending_tailcall_refs.get() }.push(callable); - vm.pending_tailcall_frame - .set(Some(crate::vm::SendNonNull(core::ptr::NonNull::from( - &mut *callee_iframe, - )))); + vm.set_pending_tailcall(callee_iframe); } /// Prepare a callee frame for a bound method TailCall. @@ -10743,10 +10740,7 @@ impl ExecutingFrame<'_> { refs.push(bound_function); refs.push(callable); - vm.pending_tailcall_frame - .set(Some(crate::vm::SendNonNull(core::ptr::NonNull::from( - &mut *callee_iframe, - )))); + vm.set_pending_tailcall(callee_iframe); } #[inline] diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 8472fafbaf9..4a900a035f3 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -108,7 +108,8 @@ pub struct VirtualMachine { pub(crate) audit_hooks: RefCell>, /// Side channel for TailCall: the bytecode loop stores the new frame /// pointer here before returning `ExecutionResult::TailCall`. - pub(crate) pending_tailcall_frame: Cell>>, + /// Access only via `set_pending_tailcall` / `take_pending_tailcall`. + pending_tailcall_frame: Cell>, /// Owned references that keep callee raw pointers valid during TailCall. /// Set by `tailcall_prepare_frame`, drained by the trampoline into /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is @@ -786,22 +787,33 @@ pub fn process_hash_secret_seed() -> u32 { } /// A `NonNull` wrapper that implements `Send + Sync`. -/// Only valid when the pointer is exclusively used by one thread at a time -/// (the VirtualMachine is per-thread). +/// +/// # Safety contract +/// +/// This type bypasses Rust's `Send`/`Sync` bounds on `NonNull`. It is +/// sound **only** when the pointer is exclusively accessed by one thread +/// at a time. In this codebase, that invariant is upheld because +/// `VirtualMachine` is per-thread. +/// +/// **Do not use this type outside `pending_tailcall_frame`.** It exists +/// solely to let a `Cell>` field on the per-thread +/// VM satisfy `Send + Sync`. If you need a `Send`-able pointer +/// elsewhere, justify and document the safety invariant at that site. #[repr(transparent)] -pub(crate) struct SendNonNull(pub(crate) core::ptr::NonNull); +struct PendingFrame(core::ptr::NonNull); -impl Copy for SendNonNull {} -impl Clone for SendNonNull { +impl Copy for PendingFrame {} +impl Clone for PendingFrame { fn clone(&self) -> Self { *self } } -// SAFETY: The VirtualMachine is per-thread; the pointer is only ever -// accessed on the thread that wrote it. -unsafe impl Send for SendNonNull {} -unsafe impl Sync for SendNonNull {} +// SAFETY: VirtualMachine is per-thread; the pointer is only ever +// accessed on the thread that wrote it. The pointed-to InterpreterFrame +// lives on that thread's datastack and is valid from set to take. +unsafe impl Send for PendingFrame {} +unsafe impl Sync for PendingFrame {} /// Saved state from `enter_iframe`, needed by `exit_iframe` to restore /// the previous frame chain and exception state. @@ -1379,6 +1391,16 @@ impl VirtualMachine { } } + /// Store a callee frame pointer for the trampoline to pick up after + /// `TailCall` is returned. The pointed-to InterpreterFrame must live + /// on the current thread's datastack and remain valid until the + /// trampoline calls `take_pending_tailcall`. + #[inline(always)] + pub(crate) fn set_pending_tailcall(&self, iframe: &mut crate::frame::InterpreterFrame) { + self.pending_tailcall_frame + .set(Some(PendingFrame(core::ptr::NonNull::from(iframe)))); + } + /// Take the pending tailcall frame pointer, resetting the side channel. #[inline(always)] fn take_pending_tailcall(&self) -> *mut crate::frame::InterpreterFrame { From a561f48367935ad2ae01c7f4019ee3c402107440 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Sun, 2 Aug 2026 21:40:13 +0900 Subject: [PATCH 09/10] Restore C-stack overflow check in trampoline enter_iframe_unchecked enter_iframe_unchecked was skipping C-stack checks under the assumption that the trampoline stays in one Rust stack frame. But each run_iframe call still consumes Rust stack, so deep Python recursion through the trampoline can exhaust the C stack (observed as STATUS_STACK_OVERFLOW on Windows CI). Keep the C-stack check (every 8th call) while still skipping the Python recursion depth check (already done by specialization_call_recursion_guard). Assisted-by: Claude --- crates/vm/src/vm/mod.rs | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 4a900a035f3..23535ad02b6 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -1476,7 +1476,18 @@ impl VirtualMachine { match action { Action::EnterCallee(callee_ptr) => { let callee = unsafe { &mut *callee_ptr }; - let callee_entry = self.enter_iframe_unchecked(callee); + let callee_entry = match self.enter_iframe_unchecked(callee) { + Ok(state) => state, + Err(exc) => { + unsafe { + if let Some(base) = callee.release_datastack_frame() { + self.datastack_pop(base); + } + } + action = Action::Unwind(exc); + continue; + } + }; let result = crate::frame::run_iframe(callee, self); match result { @@ -2130,17 +2141,23 @@ impl VirtualMachine { return Err(self.new_recursion_error(String::new())); } - Ok(self.enter_iframe_unchecked(iframe)) + self.enter_iframe_unchecked(iframe) } - /// Like `enter_iframe` but skips recursion and C-stack checks. - /// Used by the trampoline where the caller has already verified - /// recursion depth and all execution stays in one Rust stack frame. + /// Like `enter_iframe` but skips the Python recursion depth check + /// (already verified by `specialization_call_recursion_guard`). + /// Still checks C-stack overflow since each `run_iframe` call + /// consumes Rust stack space. #[inline(always)] pub(crate) fn enter_iframe_unchecked( &self, iframe: &mut crate::frame::InterpreterFrame, - ) -> IframeEntryState { + ) -> PyResult { + let depth = self.recursion_depth.get(); + if depth & 7 == 0 && self.check_c_stack_overflow() { + return Err(self.new_recursion_error(String::new())); + } + self.recursion_depth.update(|d| d + 1); let iframe_ptr = iframe as *const crate::frame::InterpreterFrame; @@ -2159,12 +2176,12 @@ impl VirtualMachine { None }; - IframeEntryState { + Ok(IframeEntryState { iframe_ptr, old_chain, saved_exc, save_exc, - } + }) } /// Pop `iframe` from the frame chain: sync materialized state, restore From a2e9a49e70380a79172b2f63c02ecde096f87f28 Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Mon, 3 Aug 2026 00:23:11 +0900 Subject: [PATCH 10/10] Address code review: entry frame double-free, panic safety, cleanup - Fix double-free: mark entry frame with is_entry flag in SuspendedFrame so the trampoline skips its datastack release (the caller owns that cleanup) - Clear iframe.previous in exit_iframe before unlinking the chain, matching with_frame and resume_gen_frame behavior - Add scopeguard in with_iframe for panic safety - Use saturating_sub(1) for lasti in trampoline_handle_exception - Extract datastack_iframe_localsplus_offset helper to avoid duplicated alignment computation Assisted-by: Claude --- crates/vm/src/frame.rs | 23 ++++++----- crates/vm/src/vm/mod.rs | 87 ++++++++++++++++++++++++++++------------- 2 files changed, 73 insertions(+), 37 deletions(-) diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs index 46efe784390..e16007ad0e1 100644 --- a/crates/vm/src/frame.rs +++ b/crates/vm/src/frame.rs @@ -548,7 +548,7 @@ impl LocalsPlus { /// Panics on overflow. pub(crate) fn push_stack(&mut self, value: PyObjectRef) { self.stack_try_push(Some(PyStackRef::new_owned(value))) - .unwrap_or_else(|_| panic!("stack overflow in push_stack")); + .expect("stack overflow in push_stack"); } /// Pop a value from the evaluation stack. @@ -992,10 +992,8 @@ impl InterpreterFrame { // InterpreterFrame lives at the start of the allocation. let iframe_ptr = base as *mut Self; // LocalsPlus data follows the InterpreterFrame, aligned to usize. - let localsplus_offset = core::mem::size_of::(); - let localsplus_offset_aligned = (localsplus_offset + core::mem::align_of::() - 1) - & !(core::mem::align_of::() - 1); - let localsplus_data_ptr = unsafe { base.add(localsplus_offset_aligned) } as *mut usize; + let localsplus_data_ptr = + unsafe { base.add(datastack_iframe_localsplus_offset()) } as *mut usize; // Zero-initialize localsplus data. unsafe { core::ptr::write_bytes(localsplus_data_ptr, 0, capacity) }; @@ -2298,13 +2296,18 @@ impl Py { } } +/// Byte offset from the start of a datastack allocation to the LocalsPlus data, +/// accounting for alignment padding after the InterpreterFrame header. +#[inline] +fn datastack_iframe_localsplus_offset() -> usize { + let iframe_size = core::mem::size_of::(); + (iframe_size + core::mem::align_of::() - 1) & !(core::mem::align_of::() - 1) +} + /// Total bytes needed to co-allocate an InterpreterFrame and its LocalsPlus /// data on the thread data stack. pub(crate) fn datastack_iframe_total_bytes(nlocalsplus: usize, stacksize: usize) -> usize { - let iframe_size = core::mem::size_of::(); - // Align the localsplus data to usize alignment after the InterpreterFrame. - let iframe_padded = - (iframe_size + core::mem::align_of::() - 1) & !(core::mem::align_of::() - 1); + let iframe_padded = datastack_iframe_localsplus_offset(); let capacity = nlocalsplus .checked_add(stacksize) .expect("LocalsPlus capacity overflow"); @@ -2362,7 +2365,7 @@ pub(crate) fn trampoline_handle_exception( // lasti points past the CallPyExactArgs instruction (+ cache entries). // The exception occurred at the previous instruction (the call site). - let idx = exec.lasti() as usize - 1; + let idx = (exec.lasti() as usize).saturating_sub(1); // Add traceback entry at the call site. if let Some((loc, _end_loc)) = exec.code.locations.get(idx) { diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs index 23535ad02b6..e0a086c10db 100644 --- a/crates/vm/src/vm/mod.rs +++ b/crates/vm/src/vm/mod.rs @@ -833,6 +833,11 @@ struct SuspendedFrame { /// `vm.pending_tailcall_refs` when the callee's TailCall is consumed. /// Dropped when this SuspendedFrame is popped (after callee returns/errors). owned_refs: Vec, + /// True for the initial frame passed into the trampoline by the caller. + /// The caller owns the datastack allocation for the entry frame, so the + /// trampoline must NOT release it — only callee-allocated frames are + /// released here. + is_entry: bool, } impl VirtualMachine { @@ -1469,6 +1474,7 @@ impl VirtualMachine { iframe: iframe as *mut crate::frame::InterpreterFrame, entry_state, owned_refs: initial_refs, + is_entry: true, }); let mut action = Action::EnterCallee(initial_ptr); @@ -1499,6 +1505,7 @@ impl VirtualMachine { iframe: callee_ptr, entry_state: callee_entry, owned_refs: refs, + is_entry: false, }); action = Action::EnterCallee(self.take_pending_tailcall()); } @@ -1533,6 +1540,7 @@ impl VirtualMachine { iframe: caller_iframe_ptr, entry_state: caller_entry, owned_refs: _caller_refs, + is_entry: caller_is_entry, } = caller; let caller_iframe = unsafe { &mut *caller_iframe_ptr }; caller_iframe.localsplus.push_stack(value); @@ -1548,15 +1556,18 @@ impl VirtualMachine { iframe: caller_iframe_ptr, entry_state: caller_entry, owned_refs: refs, + is_entry: caller_is_entry, }); action = Action::EnterCallee(self.take_pending_tailcall()); } Ok(ExecutionResult::Return(value)) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } } } action = Action::ReturnValue(value); @@ -1565,9 +1576,11 @@ impl VirtualMachine { Err(exc) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } } } action = Action::Unwind(exc); @@ -1583,6 +1596,7 @@ impl VirtualMachine { iframe: caller_iframe_ptr, entry_state: caller_entry, owned_refs: _caller_refs, + is_entry: caller_is_entry, } = caller; let caller_iframe = unsafe { &mut *caller_iframe_ptr }; @@ -1603,16 +1617,20 @@ impl VirtualMachine { iframe: caller_iframe_ptr, entry_state: caller_entry, owned_refs: refs, + is_entry: caller_is_entry, }); action = Action::EnterCallee(self.take_pending_tailcall()); } Ok(ExecutionResult::Return(value)) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() - { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = + caller_iframe.release_datastack_frame() + { + self.datastack_pop(base); + } } } action = Action::ReturnValue(value); @@ -1623,10 +1641,13 @@ impl VirtualMachine { Err(new_exc) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() - { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = + caller_iframe.release_datastack_frame() + { + self.datastack_pop(base); + } } } action = Action::Unwind(new_exc); @@ -1636,9 +1657,11 @@ impl VirtualMachine { Ok(Some(ExecutionResult::Return(value))) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } } } action = Action::ReturnValue(value); @@ -1649,9 +1672,11 @@ impl VirtualMachine { Err(new_exc) => { drop(_caller_refs); self.exit_iframe(caller_entry); - unsafe { - if let Some(base) = caller_iframe.release_datastack_frame() { - self.datastack_pop(base); + if !caller_is_entry { + unsafe { + if let Some(base) = caller_iframe.release_datastack_frame() { + self.datastack_pop(base); + } } } action = Action::Unwind(new_exc); @@ -2194,13 +2219,6 @@ impl VirtualMachine { save_exc, } = state; - // Read the materialized pointer once via read_volatile (bypasses - // LLVM's noalias on the &mut iframe borrow). - let mat_ptr = unsafe { - let field_ptr = core::ptr::addr_of!((*iframe_ptr).materialized); - core::ptr::read_volatile(field_ptr as *const usize) - }; - // If this iframe was materialized, capture f_back so that code // holding a reference to the FrameObject can walk the chain after // return. Read materialized through read_volatile to bypass @@ -2237,10 +2255,22 @@ impl VirtualMachine { core::sync::atomic::Ordering::Release, ); } + } if save_exc { self.restore_exception(saved_exc); } + // Clear previous before popping — it may point to a stack-allocated + // iframe that will be freed when the caller's with_iframe exits. + { + #[allow(unused_imports)] + use rustpython_common::atomic::Radium; + unsafe { + (*iframe_ptr) + .previous + .store(0, core::sync::atomic::Ordering::Relaxed); + } + } let _ = crate::vm::thread::set_current_frame(old_chain); self.recursion_depth.update(|d| d - 1); @@ -2269,7 +2299,10 @@ impl VirtualMachine { f: impl FnOnce(&mut crate::frame::InterpreterFrame) -> PyResult, ) -> PyResult { let state = self.enter_iframe(iframe)?; + // Ensure exit_iframe runs even if f(iframe) panics. + let guard = scopeguard::guard(state, |s| self.exit_iframe(s)); let result = f(iframe); + let state = scopeguard::ScopeGuard::into_inner(guard); self.exit_iframe(state); result }