GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Split InterpreterFrame from FrameObject for stack-allocated execution by youknowone · Pull Request #8354 · RustPython/RustPython Conversation
|
| Metric | Before | After | Improvement |
|---|---|---|---|
| 1M call overhead | ~150 ms | ~88 ms | ~41% |
| fib(28) | ~175 ms | ~117 ms | ~33% |
test_sys (Windows): full frame chain materialization with retained_back in non-unix get_all_current_framestest_current_exceptions (Windows): STW-based cross-thread f_back on all platformstest_frame: frame.clear() rejects live frames, f_locals proxy writes to live iframetest_traceback: deferred GC tracking for materialized framestest_generators: removed @expectedFailure for frame/GC cycle teststest_pdb: f_trace propagation to live source iframetest_faulthandler: use top_iframe for all frame chain walkingAddresses youknowone#40
Performance
Bug Fixes
sys._getframe behavior.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRustPython migrates frame execution and introspection from ChangesFrame runtime migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PyFunction
participant VirtualMachine
participant InterpreterFrame
participant ThreadSlot
participant FrameObject
PyFunction->>VirtualMachine: invoke function
VirtualMachine->>InterpreterFrame: execute stack-backed frame
InterpreterFrame->>ThreadSlot: publish current iframe
InterpreterFrame->>FrameObject: materialize when required
FrameObject-->>VirtualMachine: provide frame state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces a “LightFrame” fast-path to reduce Python→Python call overhead by avoiding allocation/materialization of full Frame PyObjects for specialized exact-args call paths, and streamlines the remaining heavy with_frame path to reduce tracing/recursion-check overhead. It also updates sys._getframe and VM frame-walk helpers to account for (and lazily materialize) light frames when the call stack is observed.
Changes:
LightFrame header (with lazy materialization to Frame) plus unified heavy/light stack walking APIs for _getframe-style introspection.PyFunction::invoke_light_slots() to run bytecode using a light frame on the DataStack.VirtualMachine::with_frame by inlining recursion checks, amortizing C-stack checks, and skipping traced-frame dispatch when tracing is off.Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/vm/src/vm/thread.rs | Adds TLS pointer for the current light-frame chain (CURRENT_LIGHT_FRAME) plus get/set helpers. |
| crates/vm/src/vm/mod.rs | Streamlines with_frame recursion/C-stack checks and avoids traced dispatch when tracing is disabled; switches current_frame() to unified heavy/light lookup. |
| crates/vm/src/stdlib/sys.rs | Updates sys._getframe/related lookups to use VM-aware frame walking that can materialize light frames. |
| crates/vm/src/frame.rs | Introduces LightFrame, lazy materialization, and unified heavy/light stack walking helpers; extends ExecutingFrame to abstract heavy vs light sources. |
| crates/vm/src/builtins/function.rs | Adds invoke_light_slots fast path that allocates and runs a LightFrame on the DataStack for unobserved calls. |
crates/vm/src/vm/mod.rs:1759
recursion_depth guard, this later decrement will double-decrement the depth on normal return/panic. Remove it (or replace with a comment) and let the earlier guard handle balancing. self.recursion_depth.update(|d| d - 1);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Set up the `previous` frame pointer for f_back: | ||
| // If the previous light frame exists, materialize it and point there. | ||
| // Otherwise, point to the top heavy frame. | ||
| let prev_light = (*light).previous_light; | ||
| let prev_frame_ptr = if !prev_light.is_null() { | ||
| let prev_materialized = materialize_light_frame(prev_light as *mut _, vm); | ||
| let ptr = &*prev_materialized as *const Py<Frame> as *mut Frame; | ||
| core::mem::forget(prev_materialized); | ||
| ptr | ||
| } else { | ||
| crate::vm::thread::get_current_frame() as *mut Frame | ||
| }; | ||
| iframe.previous.store(prev_frame_ptr, atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can you confirm this?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '== LightFrame and thread-frame symbol references ==' rg -n -C 4 \ -e '\bLightFrame\b' \ -e '\bprevious_light\b' \ -e '\bsaved_current_frame\b' \ -e '\bget_current_light_frame\s*\(' \ -e '\bset_current_light_frame\s*\(' \ -e '\bget_current_frame\s*\(' \ -e '\bset_current_frame\s*\(' \ crates printf '%s\n' '== Candidate source files ==' fd -t f -e rs . crates | sort
Length of output: 54707
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '== Light-frame creation, execution, and cleanup ==' sed -n '780,955p' crates/vm/src/builtins/function.rs printf '%s\n' '== Heavy-frame push/pop scopes ==' sed -n '1700,1825p' crates/vm/src/vm/mod.rs printf '%s\n' '== f_back implementation and materialization call sites ==' rg -n -C 5 \ -e '\bfn f_back\b' \ -e '\.f_back\s*\(' \ -e '\bmaterialize_light_frame_pub\s*\(' \ -e '\bmaterialize_light_frame\s*\(' \ crates/vm/src -g '*.rs'
Length of output: 20624
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '700,775p' crates/vm/src/builtins/frame.rs
Length of output: 3354
@fanninpm, confirmed — the finding is valid.
A concrete failing stack is:
H0 → L1 → H1 → L2
where L1 is a light frame, H1 is a heavy Python frame entered through a native callback, and L2 is a subsequent light call.
L1.saved_current_frame == H0L2.previous_light == L1L2.saved_current_frame == H1The current code materializes L2 with f_back = L1, skipping its immediate caller H1. Also, for a light frame without a previous_light, using get_current_frame() at materialization time can select a heavy frame that was pushed after that light frame began, rather than its caller.
f_back follows the stored previous pointer directly, so it cannot repair this later.
The materialization logic should use the entry-time boundary:
previous_light only when it has the same saved_current_frame;saved_current_frame, rather than the current heavy-frame TLS value.One caveat: this fixes the immediate incorrect predecessor. Preserving the entire H1 → L1 relationship for a full f_back walk across native callbacks would need additional chain handling, because heavy-frame push currently links H1.previous only to the prior heavy frame (H0).
crates/vm/src/exceptions.rsunsafe impl Traverse for PyBaseException { fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { self.traceback.traverse(tracer_fn); self.cause.traverse(tracer_fn); self.context.traverse(tracer_fn); self.args.traverse(tracer_fn); } }
pub enum ExceptionCtor { Class(PyTypeRef), Instance(PyBaseExceptionRef), }
ExceptionCtor)impl TryFromObject for ExceptionCtor { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> { obj.downcast::<PyType>() .and_then(|cls| { if cls.fast_issubclass(vm.ctx.exceptions.base_exception_type) { Ok(Self::Class(cls)) } else { Err(cls.into()) } }) .or_else(|obj| obj.downcast::<PyBaseException>().map(Self::Instance)) .map_err(|obj| { vm.new_type_error(format!( "exceptions must be classes or instances deriving from BaseException, not {}", obj.class().name() )) }) } }
impl ExceptionCtor { pub fn instantiate(self, vm: &VirtualMachine) -> PyResult<PyBaseExceptionRef> { match self { Self::Class(cls) => vm.invoke_exception(&cls, vec![]), Self::Instance(exc) => Ok(exc), } } pub fn instantiate_value( self, value: PyObjectRef, vm: &VirtualMachine, ) -> PyResult<PyBaseExceptionRef> { let exc_inst = value.clone().downcast::<PyBaseException>().ok(); match (self, exc_inst) { // both are instances; which would we choose? (Self::Instance(_exc_a), Some(_exc_b)) => { Err(vm.new_type_error("instance exception may not have a separate value")) } // if the "type" is an instance and the value isn't, use the "type" (Self::Instance(exc), None) => Ok(exc), // if the value is an instance of the type, use the instance value (Self::Class(cls), Some(exc)) if exc.fast_isinstance(&cls) => Ok(exc), // otherwise; construct an exception of the type using the value as args (Self::Class(cls), _) => { let args = match_class!(match value { PyNone => vec![], tup @ PyTuple => tup.to_vec(), exc @ PyBaseException => exc.args().to_vec(), obj => vec![obj], }); vm.invoke_exception(&cls, args) } } } }
pub struct PyBaseException { pub(super) traceback: PyRwLock<Option<PyTracebackRef>>, pub(super) cause: PyRwLock<Option<PyRef<Self>>>, pub(super) context: PyRwLock<Option<PyRef<Self>>>, pub(super) suppress_context: AtomicCell<bool>, pub(super) args: PyRwLock<PyTupleRef>, } impl PyBaseException { pub fn get_arg(&self, idx: usize) -> Option<PyObjectRef> { self.args.read().get(idx).cloned() } }
ExceptionGroup matching used by bytecode CHECK_EG_MATCH)pub(crate) fn exception_group_match( exc_value: &PyObjectRef, match_type: &PyObjectRef, vm: &VirtualMachine, ) -> PyResult<(PyObjectRef, PyObjectRef)> { // Implements _PyEval_ExceptionGroupMatch // If exc_value is None, return (None, None) if vm.is_none(exc_value) { return Ok((vm.ctx.none(), vm.ctx.none())); } // Validate match_type and reject ExceptionGroup/BaseExceptionGroup check_except_star_type_valid(match_type, vm)?; // Check if exc_value matches match_type if exc_value.is_instance(match_type, vm)? { // Full match of exc itself let is_eg = exc_value.fast_isinstance(vm.ctx.exceptions.base_exception_group); let matched = if is_eg { exc_value.clone() } else { // Naked exception - wrap it in ExceptionGroup let excs = vm.ctx.new_tuple(vec![exc_value.clone()]); let eg_type: PyObjectRef = crate::exception_group::exception_group().to_owned().into(); let wrapped = eg_type.call((vm.ctx.new_str(""), excs), vm)?; // Copy traceback from original exception if let Ok(exc) = exc_value.clone().downcast::<types::PyBaseException>() && let Some(tb) = exc.__traceback__() && let Ok(wrapped_exc) = wrapped.clone().downcast::<types::PyBaseException>() { let _ = wrapped_exc.set___traceback__(tb.into(), vm); } wrapped }; return Ok((vm.ctx.none(), matched)); } // Check for partial match if it's an exception group if exc_value.fast_isinstance(vm.ctx.exceptions.base_exception_group) { let pair = vm.call_method(exc_value, "split", (match_type.clone(),))?; if !pair.class().is(vm.ctx.types.tuple_type) { return Err(vm.new_type_error(format!( "{}.split must return a tuple, not {}", exc_value.class().name(), pair.class().name() ))); } let pair_tuple: PyTupleRef = pair.try_into_value(vm)?; if pair_tuple.len() < 2 { return Err(vm.new_type_error(format!( "{}.split must return a 2-tuple, got tuple of size {}", exc_value.class().name(), pair_tuple.len() ))); } let matched = pair_tuple[0].clone(); let rest = pair_tuple[1].clone(); return Ok((rest, matched)); } // No match Ok((exc_value.clone(), vm.ctx.none())) }
prep_reraise_star used by PREP_RERAISE_STAR)pub fn prep_reraise_star(orig: PyObjectRef, excs: PyObjectRef, vm: &VirtualMachine) -> PyResult { use crate::builtins::PyList; let excs_list = excs .downcast::<PyList>() .map_err(|_| vm.new_type_error("expected list for prep_reraise_star"))?; let excs_vec: Vec<PyObjectRef> = excs_list.borrow_vec().to_vec(); // If no exceptions to process, return None if excs_vec.is_empty() { return Ok(vm.ctx.none()); } // Special case: naked exception (not an ExceptionGroup) // Only one except* clause could have executed, so there's at most one exception to raise if !orig.fast_isinstance(vm.ctx.exceptions.base_exception_group) { // Find first non-None exception let first = excs_vec.into_iter().find(|e| !vm.is_none(e)); return Ok(first.unwrap_or_else(|| vm.ctx.none())); } // Split excs into raised (new) and reraised (from original) by comparing metadata let mut raised: Vec<PyObjectRef> = Vec::new(); let mut reraised: Vec<PyObjectRef> = Vec::new(); for exc in excs_vec { if vm.is_none(&exc) { continue; } // Check if this exception came from the original group if is_exception_from_orig(&exc, &orig, vm) { reraised.push(exc); } else { raised.push(exc); } } // If no exceptions to reraise, return None if raised.is_empty() && reraised.is_empty() { return Ok(vm.ctx.none()); } // Project reraised exceptions onto original structure to preserve nesting let reraised_eg = exception_group_projection(&orig, &reraised, vm)?; // If no new raised exceptions, just return the reraised projection if raised.is_empty() { return Ok(reraised_eg); } // Combine raised with reraised_eg if !vm.is_none(&reraised_eg) { raised.push(reraised_eg); } // If only one exception, return it directly if raised.len() == 1 { return Ok(raised.into_iter().next().unwrap()); } // Create new ExceptionGroup for multiple exceptions let excs_tuple = vm.ctx.new_tuple(raised); let eg_type: PyObjectRef = crate::exception_group::exception_group().to_owned().into(); eg_type.call((vm.ctx.new_str(""), excs_tuple), vm) }
crates/vm/src/vm/mod.rsimpl VirtualMachine { /// Bump-allocate `size` bytes from the thread data stack. /// /// # Safety /// The returned pointer must be freed by calling `datastack_pop` in LIFO order. #[inline(always)] pub(crate) fn datastack_push(&self, size: usize) -> *mut u8 { unsafe { (*self.datastack.get()).push(size) } } /// Check whether the thread data stack currently has room for `size` bytes. #[inline(always)] pub(crate) fn datastack_has_space(&self, size: usize) -> bool { unsafe { (*self.datastack.get()).has_space(size) } } /// Pop a previous data stack allocation. /// /// # Safety /// `base` must be a pointer returned by `datastack_push` on this VM, /// and all allocations made after it must already have been popped. #[inline(always)] pub(crate) unsafe fn datastack_pop(&self, base: *mut u8) { unsafe { (*self.datastack.get()).pop(base) } } /// Temporarily detach the current thread (ATTACHED → DETACHED) while /// running `f`, then re-attach afterwards. Allows `stop_the_world` to /// park this thread during blocking syscalls. #[inline] pub fn allow_threads<R>(&self, f: impl FnOnce() -> R) -> R { thread::allow_threads(self, f) } }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if !materialized_ptr.is_null() { | ||
| // Frame was materialized (observed by traceback/sys._getframe). | ||
| // Drop the leaked ref from materialization. | ||
| let materialized_ref: FrameRef = (&*materialized_ptr).to_owned(); | ||
| drop(materialized_ref); | ||
| } |
| // Run the bytecode | ||
| let lasti_ref: &rustpython_common::atomic::PyAtomic<u32> = | ||
| &*(&(*light).lasti as *const core::sync::atomic::AtomicU32 | ||
| as *const rustpython_common::atomic::PyAtomic<u32>); | ||
| let code_ref = (*(*light).code).to_owned(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- PyAtomic definitions and cfg gates ---' rg -n -C 8 'PyAtomic|type .*Atomic|struct .*Atomic' crates --glob '*.rs' | head -n 280 printf '%s\n' '--- LightFrame declaration and lasti uses ---' rg -n -C 12 '\bstruct\s+LightFrame\b|\blasti\b' crates/vm/src --glob '*.rs' | head -n 360 printf '%s\n' '--- workspace threading feature declarations ---' rg -n -C 4 '^\s*threading\s*=|feature.*threading|\bthreading\b' Cargo.toml crates --glob 'Cargo.toml' | head -n 240
Length of output: 46644
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate atomic and frame files ---' fd -t f -e rs 'atomic|frame' crates/common crates/vm | sort printf '%s\n' '--- exact PyAtomic definition ---' rg -n -C 24 'pub\s+(type|use)\s+PyAtomic\b|PyAtomic\s*[=<]' crates/common --glob '*.rs' printf '%s\n' '--- exact LightFrame definition and runner signature ---' rg -n -C 28 '\bstruct\s+LightFrame\b|fn\s+run_light_frame\b|impl\s+LightFrame\b' crates/vm/src/frame.rs printf '%s\n' '--- package feature wiring relevant to wasm and common ---' sed -n '1,80p' crates/wasm/Cargo.toml sed -n '1,55p' crates/common/Cargo.toml sed -n '1,45p' crates/vm/Cargo.toml
Length of output: 20133
@fanninpm, confirmed.
PyAtomic<u32> is feature-dependent in crates/common/src/atomic.rs:
threading: AtomicU32threading: core::cell::Cell<u32>But LightFrame::lasti is unconditionally declared as core::sync::atomic::AtomicU32 (in crates/vm/src/frame.rs, Line 72), then cast to &PyAtomic<u32> in the new call path. The wasm package explicitly builds the VM without the threading feature, so that cast becomes effectively:
&AtomicU32 -> &Cell<u32>
That is unsound. A matching size/alignment today would not make it valid: the types have no guaranteed layout equivalence, and Cell operations perform interior mutation through a reference that does not actually point to a Cell.
The appropriate fix is to make LightFrame::lasti a PyAtomic<u32>, initialize it with Radium::new(0), and pass &(*light).lasti directly—removing the cast entirely.
One wording refinement: the unsafe raw-pointer cast likely bypasses a normal compile-time type mismatch, so I can confirm the non-threading unsoundness rather than specifically a guaranteed build failure.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Recursion depth check | ||
| if vm.current_recursion_depth() >= vm.recursion_limit.get() { | ||
| // Clean up the light frame TLS before erroring | ||
| crate::vm::thread::set_current_light_frame(prev_light); | ||
| // Drop values we moved into localsplus |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '=== Native-stack guard call sites and recursion helpers ===' rg -n -C 5 '\bcheck_c_stack_overflow\b|\bwith_recursion\b|\bwith_frame\b' crates/vm --glob '*.rs' printf '%s\n' '=== Light-frame runner definitions/call sites ===' rg -n -C 5 '\brun_light_frame\s*\(' crates/vm --glob '*.rs' printf '%s\n' '=== Light-frame invocation and recursion-depth operations ===' rg -n -C 4 '\binvoke_light_slots\s*\(|\brecursion_depth_(increment|decrement)\s*\(' crates/vm --glob '*.rs'
Length of output: 34152
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '=== All tracked definitions/references related to light frames ===' git grep -n -E 'run_light_frame|LightFrame|check_c_stack_overflow' -- '*.rs' || true printf '%s\n' '=== Relevant VirtualMachine frame-helper implementation ===' sed -n '1690,1775p' crates/vm/src/vm/mod.rs printf '%s\n' '=== Complete light-frame recursion-check and execution region ===' sed -n '850,930p' crates/vm/src/builtins/function.rs printf '%s\n' '=== Source-file locations that may supply included/generated frame code ===' rg -n -C 2 'include!|mod .*frame|pub.*frame' crates/vm/src --glob '*.rs'
Length of output: 32072
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '=== run_light_frame implementation ===' sed -n '2325,2415p' crates/vm/src/frame.rs printf '%s\n' '=== Calls made by run_light_frame (for recursion/frame-helper delegation) ===' sed -n '2334,2415p' crates/vm/src/frame.rs | rg -n -C 2 '\b(with_frame|with_recursion|check_recursive_call|run_frame|run|execute)\b' || true
Length of output: 3377
@fanninpm, confirmed.
PyFunction::invoke_light_slots checks only recursion_depth >= recursion_limit before incrementing the depth. It then calls run_light_frame, which directly executes ExecutingFrame::run and does not delegate to with_frame, with_recursion, or any other C-stack guard.
In contrast, VirtualMachine::with_frame performs the amortized native-stack probe:
let depth = self.recursion_depth.get(); if depth & 63 == 0 && self.check_c_stack_overflow() { return Err(self.new_recursion_error(String::new())); }
Therefore the light-frame fast path bypasses the native-stack protection. With an increased sys.setrecursionlimit(), sufficiently deep recursive light-frame calls may exhaust the native stack rather than reliably raising RecursionError.
The appropriate fix is to add the same amortized check to invoke_light_slots before recursion_depth_increment() (exposing a crate-visible VM helper if needed).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| self.recursion_depth.update(|d| d + 1); | ||
|
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
crates/vm/src/vm/mod.rs:1732
self.recursion_depth.update(|d| d + 1) happens before any scopeguard::defer! is installed. On some cfgs, the intervening operations (notably push_thread_frame(...).lock().push(...) on non-unix threading builds) can panic (e.g. allocation failure), which would leak the incremented recursion depth and potentially break subsequent recursion checks. To keep invariants panic-safe, consider either (a) installing a recursion-depth-only defer immediately after increment, or (b) moving the increment down so it occurs after the fallible setup and immediately before the existing scopeguard::defer! block. self.recursion_depth.update(|d| d + 1);
// Publish the frame for sys._current_frames() and faulthandler.
#[cfg(all(not(unix), feature = "threading"))]
crate::vm::thread::push_thread_frame(FramePtr(NonNull::from(&*frame)));
// Link frame into the signal-safe frame chain.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Generator/coroutine code and tracing must use the heavy path | ||
| if code.flags.intersects( | ||
| bytecode::CodeFlags::GENERATOR | ||
| | bytecode::CodeFlags::COROUTINE | ||
| | bytecode::CodeFlags::ASYNC_GENERATOR, | ||
| ) || vm.use_tracing.get() | ||
| { | ||
| return self.invoke_exact_args_slots(args, vm); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can you confirm this?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '=== Relevant symbols ===' rg -n -C 5 \ -e 'struct FrameLocals\b' \ -e 'impl FrameLocals\b' \ -e 'fn lazy\b' \ -e 'pub.*fn new\(' \ -e '\binvoke_light_slots\s*\(' \ crates/vm/src printf '%s\n' '=== Frame module outline ===' ast-grep outline crates/vm/src/frame.rs --items all --type class,function,method --match 'Frame|FrameLocals|LightFrame' || true printf '%s\n' '=== Function module call contexts ===' rg -n -C 12 '\binvoke_light_slots\s*\(' crates/vm/src/builtins/function.rs crates/vm/src
Length of output: 70868
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '=== Frame locals and Frame::new ===' sed -n '897,970p' crates/vm/src/frame.rs sed -n '1220,1305p' crates/vm/src/frame.rs printf '%s\n' '=== Exact-call specialization dispatch and guard setup ===' sed -n '5340,5490p' crates/vm/src/frame.rs rg -n -C 8 \ -e 'CallPythonExactArgs' \ -e 'CallBoundMethodExactArgs' \ -e 'can_specialize_call\s*\(' \ -e 'is_optimized_for_call_specialization\s*\(' \ crates/vm/src/frame.rs printf '%s\n' '=== Code flag mutation/validation surfaces ===' rg -n -C 8 \ -e 'CodeFlags' \ -e 'co_flags' \ -e 'replace\(' \ -e 'PyCodeNew' \ crates/vm/src/builtins/code.rs crates/vm/src printf '%s\n' '=== Tests using code.replace or FunctionType ===' rg -n -C 5 \ -e '\.replace\s*\(' \ -e 'FunctionType\s*\(' \ -e 'types\.CodeType' \ -e 'co_flags' \ Lib test crates \ -g '*.py' -g '*.rs' || true
Length of output: 192102
@fanninpm, confirmed — the finding is valid.
FrameLocals::lazy() is explicitly the CO_NEWLOCALS representation. In contrast, the normal Frame::new path uses the function’s globals as the locals mapping when CO_NEWLOCALS is absent:
None if code.flags.contains(CodeFlags::NEWLOCALS) => FrameLocals::lazy(), None => FrameLocals::with_locals(ArgMapping::from_dict_exact(scope.globals.clone())),
invoke_light_slots always selects FrameLocals::lazy(), while its current fallback only covers generator/coroutine code and tracing. The call-specialization eligibility also requires CO_OPTIMIZED but not CO_NEWLOCALS.
This state is reachable because RustPython’s code.replace(co_flags=...) and code-object constructor accept supplied flags without enforcing CO_OPTIMIZED => CO_NEWLOCALS. Consequently, a function with CO_OPTIMIZED but without CO_NEWLOCALS can be specialized and then observe an empty lazy locals dictionary instead of its globals mapping.
The fix should require CO_NEWLOCALS for the light-frame path and fall back to invoke_exact_args_slots otherwise. Adding that condition to can_specialize_call avoids creating the specialization; keeping a defensive check in invoke_light_slots would also protect current and future callers.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Link frame into the signal-safe frame chain. | ||
| let old_frame = crate::vm::thread::set_current_frame((&**frame) as *const Frame); | ||
| frame.previous.store( | ||
| old_frame as *mut Frame, | ||
| core::sync::atomic::Ordering::Relaxed, | ||
| ); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can you confirm this?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail fd -t f 'frame\.rs|mod\.rs|thread\.rs' crates/vm/src | sort ast-grep outline crates/vm/src/builtins/frame.rs --items all --view expanded ast-grep outline crates/vm/src/frame.rs --items all --view expanded ast-grep outline crates/vm/src/vm/thread.rs --items all --view expanded rg -n -C 5 -P '\b(frame_at_offset_vm|find_owned_chain_frame|retained_back|f_back|with_frame|resume_gen_frame|Light)\b' \ crates/vm/src/builtins/frame.rs crates/vm/src/frame.rs crates/vm/src/vm crates/vm/src \ || true
Length of output: 79142
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- crates/vm/src/frame.rs: materialization and unified walk ---' sed -n '145,405p' crates/vm/src/frame.rs echo '--- crates/vm/src/builtins/frame.rs: f_back ---' sed -n '720,755p' crates/vm/src/builtins/frame.rs echo '--- crates/vm/src/builtins/function.rs: light-frame invocation ---' sed -n '840,965p' crates/vm/src/builtins/function.rs echo '--- all light-frame state access and heavy-frame entry call sites ---' rg -n -C 4 -P '\b(get_current_light_frame|set_current_light_frame|saved_current_frame|run_frame\s*\(|with_frame\s*\(|ensure_heavy\s*\()' \ crates/vm/src --glob '*.rs'
Length of output: 34501
@fanninpm, confirmed — the concern is valid.
frame_at_offset_vm explicitly models this case: it walks “heavy frames pushed by native callbacks from within a light frame” before returning the active light frame. So the unified stack can be:
heavy callback frame → light caller → earlier heavy frame
But VirtualMachine::with_frame sets the callback frame’s previous pointer from set_current_frame, which is only the prior heavy frame. Py<Frame>::f_back follows only that pointer through find_owned_chain_frame / retained_back; neither path consults or materializes CURRENT_LIGHT_FRAME.
Therefore, when a heavy frame is entered while a light frame is active, sys._getframe() can report the light frame as its logical caller, while heavy_frame.f_back skips it. That breaks the expected adjacent-frame invariant.
The fix needs to materialize/retain the active light frame as the heavy callback frame’s back-link at the heavy-frame entry boundary, or the affected CPython test needs an explicit temporary expected-failure marker if this remains an acknowledged limitation.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@fanninpm Thank you so much, it is unexpectedly very helpful 😂 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
crates/vm/src/vm/mod.rs:1727
with_frame() increments recursion_depth before installing the scopeguard::defer! that decrements it. A panic between the increment and the defer installation (e.g., during materialize_light_frame_pub / Frame::new allocations) would leak recursion_depth, potentially causing spurious RecursionError later in the thread. self.recursion_depth.update(|d| d + 1);
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// The current thread's topmost frame object, if any. | ||
| /// If light frames are active, they are on top of the heavy chain. |
| /// Public wrapper for `materialize_light_frame`. | ||
| /// | ||
| /// # Safety | ||
| /// `light` must point to a valid LightFrame whose borrowed pointers are still alive. | ||
| pub unsafe fn materialize_light_frame_pub(light: *mut LightFrame, vm: &VirtualMachine) -> FrameRef { |
|
the implementation direction is wrong. reworking |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
2ed7ca5 to
e73a11c
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 24 out of 31 changed files in this pull request and generated 4 comments.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| *frame.retained_back.lock() = frame | ||
| .previous_frame() | ||
| .as_heavy() | ||
| .and_then(|h| unsafe { owned_chain_frame(h) }); |
| # Complete `rustpython-unicode` isolation: case mapping, casing predicates, and sre case/space parity | ||
|
|
||
| Follow-up to #7560, continuing from #8211 (merged). |
| Forwards Claude Code hook/statusline events to the gateway (ANTHROPIC_BASE_URL) | ||
| and acts on the JSON it returns: injects the gateway's `context` as non-blocking | ||
| additional context (no yellow "blocked by hook" UI); on a closed task uploads the | ||
| named transcripts and bundles them; on an auth failure (re)creates | ||
| settings.local.json and blocks with fix-it steps instead of a raw 401. All wording |
| "mode": "merge", | ||
| "target": "/Users/youknowone/Projects/RustPython-11/.claude", | ||
| "timestamp": "2026-07-22T12:47:24Z", |
e73a11c to
f6bae97
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| while !cur.is_null() && depth < MAX_FRAME_DEPTH { | ||
| if let Some(heavy) = cur.as_heavy() { | ||
| let frame = unsafe { &*heavy }; | ||
| dump_frame_from_raw(fd, frame); | ||
| depth += 1; | ||
| } | ||
| cur = unsafe { cur.next() }; | ||
| } |
| // Light frame predecessor: materialize it | ||
| if let Some(light) = chain.as_light() { | ||
| let frame = unsafe { crate::frame::materialize_light_frame_pub(light, vm) }; | ||
| frame.mark_escaped(); | ||
| return Some(frame); | ||
| } |
f6bae97 to
f1395e4
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
crates/vm/src/frame.rs:407
current_thread_frame() now explicitly skips light frames. This function is used by VirtualMachine::current_frame() (and many call sites such as builtins that rely on the actual current Python frame). With light frames enabled, those APIs will observe the wrong caller frame whenever the top of stack is a LightFrame.To preserve existing semantics, current_thread_frame() should materialize the light frame when the current VM is available (via TLS), and only fall back to the heavy-only behavior when no VM is attached.
/// The current thread's topmost heavy frame object, if any.
/// Skips light frames — use `current_thread_frame_vm` to materialize them.
#[must_use]
pub fn current_thread_frame() -> Option<FrameRef> {
let mut cur = crate::vm::thread::get_current_frame();
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| pub fn set_current_frame(chain: FrameChainPtr) -> FrameChainPtr { | ||
| // Publish the top heavy frame for cross-thread readers (signal safety). | ||
| // Only heavy frames are published — light frames cannot be safely | ||
| // dereferenced from a signal handler on another thread. | ||
| #[cfg(all(unix, feature = "threading"))] | ||
| { | ||
| if let Some(heavy) = chain.as_heavy() { |
| let payload: *const Frame = &***frame; | ||
| let old_chain = | ||
| crate::vm::thread::set_current_frame(crate::frame::FrameChainPtr::from_heavy(payload)); | ||
| { | ||
| #[allow(unused_imports)] | ||
| use rustpython_common::atomic::Radium; | ||
| frame | ||
| .previous | ||
| .store(old_chain.raw(), core::sync::atomic::Ordering::Relaxed); | ||
| } |
f1395e4 to
bc4425b
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
crates/vm/src/vm/thread.rs:684
set_current_frame updates ThreadSlot::top_frame only when chain is heavy. When a heavy frame returns to a light frame (or to null), top_frame can remain pointing at the just-popped heavy frame, so sys._current_frames() / GC assertions may read a dangling pointer even under stop-the-world. Publish the nearest heavy frame reachable from chain (or null) instead of leaving top_frame unchanged when chain is light. // Publish the top heavy frame for cross-thread readers (signal safety).
// Only heavy frames are published — light frames cannot be safely
// dereferenced from a signal handler on another thread.
#[cfg(all(unix, feature = "threading"))]
if let Some(heavy) = chain.as_heavy() {
crates/vm/src/builtins/frame.rs:736
f_back materializes a light predecessor immediately. If the Frame object is accessed from another thread (e.g. via sys._current_frames()), this can race with the owning thread mutating the light frame’s DataStack-backed locals/state, causing a data race/UB. Only materialize directly when the light entry is on the current thread’s chain; otherwise, on unix+threading, materialize under stop-the-world (or return None on platforms without stop-the-world). // Light frame predecessor: materialize it
if let Some(light) = chain.as_light() {
let frame = unsafe { crate::frame::materialize_light_frame_pub(light, vm) };
frame.mark_escaped();
return Some(frame);
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #[cfg(unix)] | ||
| top_frame: AtomicPtr::new(get_current_frame() as *mut Frame), | ||
| top_frame: AtomicPtr::new( | ||
| get_current_frame().as_heavy().unwrap_or(core::ptr::null()) as *mut Frame | ||
| ), |
bc4425b to
9c7476d
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
crates/vm/src/builtins/frame.rs (2)
500-513: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the live instruction position for the initial
f_linenocheck.Line 501 reads
self.lasti()beforefind_live_source_iframe(). A materializedFrameObjectcan retainlasti == 0while its liveInterpreterFramehas advanced. The getter then returns the first line and skips the liveprev_linepath.Resolve the live position before the zero check and reuse it.
Proposed fix
- // If lasti is 0, execution hasn't started yet - use first line number - if self.lasti() == 0 { + let live = self.find_live_source_iframe(); + let current_lasti = if !live.is_null() { + unsafe { (*live).lasti.load(Relaxed) } + } else { + self.lasti() + }; + // If lasti is 0, execution hasn't started yet - use first line number + if current_lasti == 0 { return self .iframe() .code() @@ - let live = self.find_live_source_iframe(); if !live.is_null() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/builtins/frame.rs` around lines 500 - 513, Update the f_lineno logic in the frame getter to call find_live_source_iframe() before checking whether the instruction position is zero, and use that live frame position for the initial check. Reuse the resolved live frame for the existing prev_line path so a materialized FrameObject cannot incorrectly return first_line_number when execution has advanced.
448-467: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd lifetime synchronization guards to
find_live_source_iframe().
find_live_source_iframe()returns a rawInterpreterFramefrom the current thread’s chain, and callers accessprev_lineandlocalsplusthrough that pointer without stop-the-world or synchronized-field protection. Keep this API scoped to callers that already guarantee frame lifetime plus execution barriers, or add explicit synchronization for each read/write site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/builtins/frame.rs` around lines 448 - 467, Restrict find_live_source_iframe to callers that already guarantee the InterpreterFrame remains live and execution is synchronized, or add the required stop-the-world and synchronized-field guards at every site that dereferences its returned pointer, including prev_line and localsplus accesses. Ensure no raw frame pointer is read or written without these lifetime and execution barriers.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/vm/src/builtins/frame.rs`:
- Around line 500-513: Update the f_lineno logic in the frame getter to call
find_live_source_iframe() before checking whether the instruction position is
zero, and use that live frame position for the initial check. Reuse the resolved
live frame for the existing prev_line path so a materialized FrameObject cannot
incorrectly return first_line_number when execution has advanced.
- Around line 448-467: Restrict find_live_source_iframe to callers that already
guarantee the InterpreterFrame remains live and execution is synchronized, or
add the required stop-the-world and synchronized-field guards at every site that
dereferences its returned pointer, including prev_line and localsplus accesses.
Ensure no raw frame pointer is read or written without these lifetime and
execution barriers.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7c36c19-18b8-431c-a737-d1cf8b609678
crates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/vm/src/vm/mod.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
crates/vm/src/builtins/frame.rs (1)
448-527: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the live
lastifor the initialf_linenocheck.
f_linenoreturns the first line whenself.lasti() == 0at Lines [500]-[502]. For a materialized live frame, execution updates the live iframe, while the materializedlastiis synchronized after execution. Therefore, the live read added at Lines [578]-[583] can be skipped while the cached value remains zero.Compute
current_lastifromfind_live_source_iframe()before the early return and use it for both checks. Add a regression test that readsframe.f_linenoduring execution.Also applies to: 571-583
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/builtins/frame.rs` around lines 448 - 527, Update FrameObject::f_lineno to resolve the live source iframe and derive current_lasti before the initial zero check, using the live InterpreterFrame::lasti when available and the materialized value otherwise. Use current_lasti for the early first-line decision and preserve the existing live prev_line and returned-frame location behavior. Add a regression test that reads frame.f_lineno while the frame is executing.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/vm/src/builtins/frame.rs`:
- Around line 448-527: Update FrameObject::f_lineno to resolve the live source
iframe and derive current_lasti before the initial zero check, using the live
InterpreterFrame::lasti when available and the materialized value otherwise. Use
current_lasti for the early first-line decision and preserve the existing live
prev_line and returned-frame location behavior. Add a regression test that reads
frame.f_lineno while the frame is executing.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab0b5b55-ac35-477b-b7c1-6e8e5e34c15c
crates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/vm/src/vm/mod.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Assisted-by: Claude
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Wow, that cut the cpython tests time from 22m± -> 18m±
amazing
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Assisted-by: Claude
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
fire_stop_iteration was passing the raw iterator return value to callbacks, but the STOP_ITERATION event callback signature expects a StopIteration exception instance. Wrap non-StopIteration values in a new StopIteration(value), matching PyMonitoring_FireStopIterationEvent. This fixes test_pdb_await_support where bdb's exception_callback received None instead of a StopIteration instance. Assisted-by: Claude
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/vm/mod.rs`:
- Around line 1764-1776: When an escaped frame (strong > 1) is detected, ensure
the predecessor InterpreterFrame referenced by old_chain is materialized before
attempting to set retained_back. The current logic only sets retained_back if
prev_iframe.frame_obj() returns Some, but for unmaterialized light
InterpreterFrames this method returns None, causing f_back to be lost after the
caller returns. Materialize prev_iframe when strong > 1 and the predecessor is
not yet materialized, so that retained_back captures the predecessor correctly
and preserves f_back beyond the caller's execution lifetime, consistent with how
with_iframe handles this scenario.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ebf7152-5c9a-457a-8849-c7c42c6bfd97
.gitignorecrates/vm/src/builtins/frame.rscrates/vm/src/frame.rscrates/vm/src/stdlib/sys/monitoring.rscrates/vm/src/vm/mod.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Only set retained_back if someone else holds a reference (escaped) | ||
| // AND the caller already has a FrameObject. Materializing the caller | ||
| // here would add refcounts on its local variables, preventing timely | ||
| // __del__ / ResourceWarning on dealloc. If the caller hasn't been | ||
| // materialized, f_back will resolve via the TLS chain while the | ||
| // caller is still executing, or return None after it returns. | ||
| if strong > 1 { | ||
| let mut guard = frame.iframe().retained_back.lock(); | ||
| if guard.is_none() { | ||
| let prev_iframe = unsafe { &*old_chain }; | ||
| if let Some(fo) = prev_iframe.frame_obj() { | ||
| *guard = Some(fo.to_owned()); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve f_back after the caller returns.
Line 1774 skips retention when old_chain is an unmaterialized light InterpreterFrame. If an exec() or eval() frame escapes, its f_back works only while its caller executes and becomes None after the caller returns. Materialize and retain the predecessor for escaped frames, as with_iframe already does.
- if let Some(fo) = prev_iframe.frame_obj() { - *guard = Some(fo.to_owned()); - } + *guard = Some(prev_iframe.materialize_chain(self));
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only set retained_back if someone else holds a reference (escaped) | |
| // AND the caller already has a FrameObject. Materializing the caller | |
| // here would add refcounts on its local variables, preventing timely | |
| // __del__ / ResourceWarning on dealloc. If the caller hasn't been | |
| // materialized, f_back will resolve via the TLS chain while the | |
| // caller is still executing, or return None after it returns. | |
| if strong > 1 { | |
| let mut guard = frame.iframe().retained_back.lock(); | |
| if guard.is_none() { | |
| let prev_iframe = unsafe { &*old_chain }; | |
| if let Some(fo) = prev_iframe.frame_obj() { | |
| *guard = Some(fo.to_owned()); | |
| } | |
| // Only set retained_back if someone else holds a reference (escaped) | |
| // AND the caller already has a FrameObject. Materializing the caller | |
| // here would add refcounts on its local variables, preventing timely | |
| // __del__ / ResourceWarning on dealloc. If the caller hasn't been | |
| // materialized, f_back will resolve via the TLS chain while the | |
| // caller is still executing, or return None after it returns. | |
| if strong > 1 { | |
| let mut guard = frame.iframe().retained_back.lock(); | |
| if guard.is_none() { | |
| let prev_iframe = unsafe { &*old_chain }; | |
| *guard = Some(prev_iframe.materialize_chain(self)); |
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/vm/mod.rs` around lines 1764 - 1776, When an escaped frame
(strong > 1) is detected, ensure the predecessor InterpreterFrame referenced by
old_chain is materialized before attempting to set retained_back. The current
logic only sets retained_back if prev_iframe.frame_obj() returns Some, but for
unmaterialized light InterpreterFrames this method returns None, causing f_back
to be lost after the caller returns. Materialize prev_iframe when strong > 1 and
the predecessor is not yet materialized, so that retained_back captures the
predecessor correctly and preserves f_back beyond the caller's execution
lifetime, consistent with how with_iframe handles this scenario.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@ShaharNaveh would it make sense to use the allow-by-default |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I would more than like to enable it! the only reason why I haven't enabled it already is because we have too many places that violates it, but using an AI tool to document it all/parts of it would be great |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.