Skip to content

Split InterpreterFrame from FrameObject for stack-allocated execution - #8354

Merged
youknowone merged 58 commits into
RustPython:mainfrom
youknowone:light-frame-call-overhead
Jul 31, 2026
Merged

Split InterpreterFrame from FrameObject for stack-allocated execution#8354
youknowone merged 58 commits into
RustPython:mainfrom
youknowone:light-frame-call-overhead

Conversation

@youknowone

@youknowone youknowone commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

Split InterpreterFrame (execution state) from FrameObject (Python-visible frame object) so that normal function calls stack-allocate only an InterpreterFrame on the Rust stack. A full FrameObject is created lazily via materialize() only when Python code observes the frame (e.g. sys._getframe(), traceback creation, sys.settrace()).

This mirrors CPython 3.11+ where _PyInterpreterFrame lives on the C stack and PyFrameObject is allocated on demand.

Architecture

  • InterpreterFrame#[repr(C)] struct on the Rust stack inside with_iframe. Contains code, globals, builtins, localsplus, lasti, trace state, and a previous pointer forming the TLS frame chain.
  • FrameObjectPyObject wrapper created by materialize() when needed. Holds an owned copy of InterpreterFrame and is linked via materialized / find_live_source_iframe() for bidirectional access.
  • with_iframe — new fast path for regular function calls. No heap allocation, no refcount, no freelist.
  • with_frame — existing path for generators, coroutines, exec(), eval() that require a durable FrameObject.

Key design decisions

current_code() and free-threading safety: current_code() reads the topmost InterpreterFrame from thread-local CURRENT_FRAME and borrows its code pointer. This is safe because: (1) CURRENT_FRAME is per-thread TLS — no cross-thread access; (2) the code pointer borrows from the PyFunction on the caller's stack, which is alive while the frame executes; (3) .to_owned() increments the refcount before returning, producing an independent PyRef<PyCode>.

Cross-thread frame access: f_back, sys._current_frames(), and sys._current_exceptions() use stop-the-world (STW) to safely materialize cross-thread iframe chains. STW is entered before dereferencing any cross-thread pointer to prevent use-after-free races. The non-unix path now uses STW identically to unix, replacing the previous frames Mutex fallback.

set_f_lineno (debugger jump): Writes lasti, pending_stack_pops, and pending_unwind_from_stack to the live source iframe via find_live_source_iframe(), not the materialized copy, so pdb jump commands take effect on stack-allocated frames.

GC tracking timing: Materialized FrameObjects are tracked in GC only at with_iframe cleanup after set_current_frame restores the old chain. This prevents premature collection while the frame is still executing.

retained_back: Only captures already-materialized callers to avoid adding refcounts on local variables (which would delay __del__ / ResourceWarning). For non-materialized callers, f_back resolves via the TLS chain while executing, or returns None after return.

Performance improvements

  • Zero heap allocation for normal function calls (no FrameObject, no freelist, no refcount)

  • Amortized C stack overflow check (every 8th recursion depth)

  • Panic-safe recursion depth via scopeguard in with_frame

Benchmark results (Apple M-series, release build)

Metric Before After Improvement
1M call overhead ~150 ms ~88 ms ~41%
fib(28) ~175 ms ~117 ms ~33%

Test fixes included

  • test_sys (Windows): full frame chain materialization with retained_back in non-unix get_all_current_frames
  • test_current_exceptions (Windows): STW-based cross-thread f_back on all platforms
  • test_frame: frame.clear() rejects live frames, f_locals proxy writes to live iframe
  • test_traceback: deferred GC tracking for materialized frames
  • test_generators: removed @expectedFailure for frame/GC cycle tests
  • test_pdb: f_trace propagation to live source iframe
  • test_faulthandler: use top_iframe for all frame chain walking

Addresses youknowone#40

Summary by CodeRabbit

  • Performance

    • Improved execution efficiency with a lightweight fast-frame path for eligible code.
    • Reduced overhead from recursion and stack checks through periodic native stack probing.
  • Bug Fixes

    • Improved traceback formatting, frame navigation, and depth reporting across threads and lightweight frames.
    • Improved tracing, profiling, warnings, imports, built-in introspection, and sys._getframe behavior.
    • Tightened text encoding validation for consistent NUL detection.
    • Improved frame and locals visibility for generators, coroutines, and asynchronous generators.

Copilot AI review requested due to automatic review settings July 23, 2026 15:30
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

RustPython migrates frame execution and introspection from Frame to InterpreterFrame and FrameObject. It adds stack-backed execution, updates frame-chain publication, and adapts tracing, traceback, C API, warning, generator, monitoring, and signal-safe inspection paths.

Changes

Frame runtime migration

Layer / File(s) Summary
InterpreterFrame and FrameObject model
crates/vm/src/frame.rs, crates/vm/src/builtins/frame.rs, crates/vm/src/object/*
Frame storage, materialization, locals synchronization, ownership, navigation, tracing state, and cleanup use InterpreterFrame and FrameObject.
Light-frame execution and VM chaining
crates/vm/src/builtins/function.rs, crates/vm/src/vm/mod.rs, crates/vm/src/coroutine.rs, crates/vm/src/builtins/*generator.rs
Function calls use stack-backed interpreter frames when possible. Heap frames remain available for generators, coroutines, and tracing.
Frame-facing APIs
crates/capi/src/*, crates/vm/src/stdlib/sys.rs, crates/vm/src/builtins/traceback.rs, crates/vm/src/builtins/frame_locals_proxy.rs
Public frame handles, frame accessors, traceback objects, coroutine frame properties, and locals proxies use FrameObjectRef.
Thread and signal-safe inspection
crates/vm/src/vm/thread.rs, crates/stdlib/src/faulthandler.rs, crates/vm/src/stdlib/_thread.rs, crates/vm/src/gc_state.rs
Thread slots publish interpreter-frame pointers. Fault-handler dumps, cross-thread frame inspection, and GC checks traverse those pointers.
Runtime consumers
crates/vm/src/warn.rs, crates/vm/src/protocol/callable.rs, crates/vm/src/stdlib/sys/monitoring.rs, crates/vm/src/stdlib/*, crates/vm/src/exceptions.rs, crates/vm/src/import.rs, crates/vm/src/suggestion.rs
Runtime consumers obtain code, globals, builtins, locals, tracing state, and traceback data through iframe-backed accessors.
Workflow and repository support
.github/workflows/*, .github/zizmor.yml, .gitignore, .cspell.dict/rust-more.txt
Workflow permissions and SHA handling are updated. Repository ignore and spelling configuration are also updated.

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
Loading

Possibly related PRs

Suggested reviewers: copilot, shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main refactor: separating InterpreterFrame from FrameObject to support stack-allocated execution.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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:

  • Add a stack-allocated LightFrame header (with lazy materialization to Frame) plus unified heavy/light stack walking APIs for _getframe-style introspection.
  • Route specialized exact-args call sites through PyFunction::invoke_light_slots() to run bytecode using a light frame on the DataStack.
  • Optimize VirtualMachine::with_frame by inlining recursion checks, amortizing C-stack checks, and skipping traced-frame dispatch when tracing is off.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
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.
Comments suppressed due to low confidence (1)

crates/vm/src/vm/mod.rs:1759

  • After adding an early 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.

Comment thread crates/vm/src/frame.rs Outdated
Comment on lines +211 to +223
// 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 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 == H0
  • L2.previous_light == L1
  • L2.saved_current_frame == H1

The 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:

  • link to previous_light only when it has the same saved_current_frame;
  • otherwise link to this light frame’s 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).


🧬 Code Graph Analysis Results

Relevant snippets

crates/vm/src/exceptions.rs

  • Lines 27-34 (GC traversal for exceptions)
unsafe 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);
    }
}
  • Lines 420-423 (Exception constructor enum)
pub enum ExceptionCtor {
    Class(PyTypeRef),
    Instance(PyBaseExceptionRef),
}
  • Lines 425-443 (converting an object into 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()
                ))
            })
    }
}
  • Lines 445-480 (instantiating exception instances/values)
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)
            }
        }
    }
}
  • Lines 1645-1667 (exception payload fields)
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()
    }
}
  • Lines 3043-3106 (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()))
}
  • Lines 3110-3175 (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.rs

  • Lines 780-939 (thread datastack allocation used by frame localsplus)
impl 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)
    }
}

Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +925 to +930
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);
}
Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +899 to +903
// 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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai can this be confirmed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 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:

  • with threading: AtomicU32
  • without threading: 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.

Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +863 to +867
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai can this be confirmed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 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).

Comment thread crates/vm/src/vm/mod.rs
Comment on lines +1727 to +1728
self.recursion_depth.update(|d| d + 1);

Copilot AI review requested due to automatic review settings July 23, 2026 16:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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.

Comment thread crates/vm/src/builtins/function.rs Outdated
Comment on lines +803 to +811
// 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);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 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.

Comment thread crates/vm/src/vm/mod.rs Outdated
Comment on lines +1732 to +1737
// 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,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai can you confirm this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 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.

@youknowone

Copy link
Copy Markdown
Member Author

@fanninpm Thank you so much, it is unexpectedly very helpful 😂

Copilot AI review requested due to automatic review settings July 23, 2026 17:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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);

Comment thread crates/vm/src/frame.rs Outdated
Comment on lines +301 to +302
/// The current thread's topmost frame object, if any.
/// If light frames are active, they are on top of the heavy chain.
Comment thread crates/vm/src/frame.rs Outdated
Comment on lines +278 to +282
/// 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 {
@youknowone

Copy link
Copy Markdown
Member Author

the implementation direction is wrong. reworking

Copilot AI review requested due to automatic review settings July 23, 2026 23:34
@youknowone
youknowone force-pushed the light-frame-call-overhead branch from 2ed7ca5 to e73a11c Compare July 23, 2026 23:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 31 changed files in this pull request and generated 4 comments.

Comment thread crates/vm/src/frame.rs Outdated
Comment on lines +2204 to +2207
*frame.retained_back.lock() = frame
.previous_frame()
.as_heavy()
.and_then(|h| unsafe { owned_chain_frame(h) });
Comment thread rustpython-unicode-isolation-issue.md Outdated
Comment on lines +1 to +3
# Complete `rustpython-unicode` isolation: case mapping, casing predicates, and sre case/space parity

Follow-up to #7560, continuing from #8211 (merged).
Comment thread .claude/ccmetr/bin/ccmetr-client.py Outdated
Comment on lines +4 to +8
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
Comment thread .claude/ccmetr/.install-manifest.json Outdated
Comment on lines +2 to +4
"mode": "merge",
"target": "/Users/youknowone/Projects/RustPython-11/.claude",
"timestamp": "2026-07-22T12:47:24Z",
Copilot AI review requested due to automatic review settings July 24, 2026 00:16
@youknowone
youknowone force-pushed the light-frame-call-overhead branch from e73a11c to f6bae97 Compare July 24, 2026 00:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Comment thread crates/stdlib/src/faulthandler.rs Outdated
Comment on lines 283 to 290
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() };
}
Comment thread crates/vm/src/builtins/frame.rs Outdated
Comment on lines 732 to 737
// 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);
}
Copilot AI review requested due to automatic review settings July 24, 2026 01:49
@youknowone
youknowone force-pushed the light-frame-call-overhead branch from f6bae97 to f1395e4 Compare July 24, 2026 01:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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();

Comment thread crates/vm/src/vm/thread.rs Outdated
Comment on lines +679 to +684
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() {
Comment thread crates/vm/src/vm/mod.rs Outdated
Comment on lines +1794 to +1803
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);
}
Copilot AI review requested due to automatic review settings July 24, 2026 02:34
@youknowone
youknowone force-pushed the light-frame-call-overhead branch from f1395e4 to bc4425b Compare July 24, 2026 02:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

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);

Comment thread crates/vm/src/vm/thread.rs Outdated
Comment on lines +802 to +805
#[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
),
Copilot AI review requested due to automatic review settings July 24, 2026 04:13
@youknowone
youknowone force-pushed the light-frame-call-overhead branch from bc4425b to 9c7476d Compare July 24, 2026 04:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for 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.

⚠️ Outside diff range comments (2)
crates/vm/src/builtins/frame.rs (2)

500-513: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the live instruction position for the initial f_lineno check.

Line 501 reads self.lasti() before find_live_source_iframe(). A materialized FrameObject can retain lasti == 0 while its live InterpreterFrame has advanced. The getter then returns the first line and skips the live prev_line path.

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 win

Add lifetime synchronization guards to find_live_source_iframe().

find_live_source_iframe() returns a raw InterpreterFrame from the current thread’s chain, and callers access prev_line and localsplus through 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.
🤖 Prompt for all review comments with 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.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: b7c36c19-18b8-431c-a737-d1cf8b609678

📥 Commits

Reviewing files that changed from the base of the PR and between aeb53c7 and a01403b.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/frame.rs
  • crates/vm/src/frame.rs
  • crates/vm/src/vm/mod.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/frame.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/vm/mod.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for 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.

⚠️ Outside diff range comments (1)
crates/vm/src/builtins/frame.rs (1)

448-527: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the live lasti for the initial f_lineno check.

f_lineno returns the first line when self.lasti() == 0 at Lines [500]-[502]. For a materialized live frame, execution updates the live iframe, while the materialized lasti is synchronized after execution. Therefore, the live read added at Lines [578]-[583] can be skipped while the cached value remains zero.

Compute current_lasti from find_live_source_iframe() before the early return and use it for both checks. Add a regression test that reads frame.f_lineno during 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.
🤖 Prompt for all review comments with 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.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: ab0b5b55-ac35-477b-b7c1-6e8e5e34c15c

📥 Commits

Reviewing files that changed from the base of the PR and between a01403b and e6efa79.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/frame.rs
  • crates/vm/src/frame.rs
  • crates/vm/src/vm/mod.rs
💤 Files with no reviewable changes (1)
  • crates/vm/src/frame.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/vm/mod.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

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.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ebf7152-5c9a-457a-8849-c7c42c6bfd97

📥 Commits

Reviewing files that changed from the base of the PR and between a01403b and 2d9876a.

📒 Files selected for processing (5)
  • .gitignore
  • crates/vm/src/builtins/frame.rs
  • crates/vm/src/frame.rs
  • crates/vm/src/stdlib/sys/monitoring.rs
  • crates/vm/src/vm/mod.rs
💤 Files with no reviewable changes (2)
  • .gitignore
  • crates/vm/src/frame.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/builtins/frame.rs

Comment thread crates/vm/src/vm/mod.rs
Comment on lines +1764 to +1776
// 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());
}

Copy link
Copy Markdown
Contributor

Choose a reason for 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.

Proposed fix
-                    if let Some(fo) = prev_iframe.frame_obj() {
-                        *guard = Some(fo.to_owned());
-                    }
+                    *guard = Some(prev_iframe.materialize_chain(self));
📝 Committable suggestion

‼️ 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.

Suggested change
// 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));
🤖 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/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.

@fanninpm

Copy link
Copy Markdown
Contributor

@ShaharNaveh would it make sense to use the allow-by-default undocumented_unsafe_blocks Clippy restriction lint? The AI tools removed a lot of the // SAFETY: comments from a bunch of unsafe function calls.

@ShaharNaveh

Copy link
Copy Markdown
Contributor

@ShaharNaveh would it make sense to use the allow-by-default undocumented_unsafe_blocks Clippy restriction lint? The AI tools removed a lot of the // SAFETY: comments from a bunch of unsafe function calls.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants