Pull request overview

This PR flattens the Python-to-Python call path in RustPython’s fast frame execution by introducing a TailCall signal + trampoline loop, so specialized calls (CallPyExactArgs, CallBoundMethodExactArgs) can switch frames without building additional Rust stack frames—similar to CPython’s post-3.12 approach.

Changes:

• Introduces ExecutionResult::TailCall and a run_frame_fast_trampoline() loop that iteratively enters/swaps frames and propagates exceptions without mutual recursion.
• Adds datastack co-allocation for InterpreterFrame + LocalsPlus (new_on_datastack / release_datastack_frame) to reduce allocation overhead and improve locality.
• Refactors with_iframe into enter_iframe / exit_iframe (with IframeEntryState) and adds VM “side channels” for pending tailcall frames and owned refs.

Reviewed changes

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

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

• Same issue as above: freeing the suspended caller frame here can free the root iframe passed into run_frame_fast_trampoline, leaving the outer cleanup path with a dangling reference. Skip datastack release when caller_iframe_ptr is the root frame pointer.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• This unwind path also releases the suspended caller’s datastack allocation. If this caller is the root iframe passed into run_frame_fast_trampoline, the outer call site will later touch freed memory during its cleanup. Add the same root-frame guard here.

                                    unsafe {
                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }

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

• This branch releases the suspended caller frame even when it is the root iframe for run_frame_fast_trampoline, which can make the subsequent outer cleanup use-after-free. Guard against releasing the root frame pointer here as well.

                                    unsafe {
                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }

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

• This release block can also free the root iframe passed into run_frame_fast_trampoline, leaving the outer call site with a dangling reference for its cleanup. Apply the same root-frame guard here.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• This error path releases the suspended caller frame unconditionally; when the caller is the root iframe, this can cause use-after-free in the outer cleanup path. Add the same root-frame guard here.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Actionable comments posted: 5

crates/vm/src/vm/mod.rs (3)

1524-1524: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename _caller_refs because the binding is used.

The leading underscore marks a binding as intentionally unused. This binding is destructured at Lines 1524, 1574, and then explicitly consumed by drop(_caller_refs) at Lines 1535, 1544, 1555, 1590, 1599, 1613, 1626, and 1639. The name contradicts the usage and hides that the drop point is deliberate.

Rename it to caller_refs and keep the explicit drop calls, which correctly document the release ordering relative to exit_iframe.

🤖 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` at line 1524, Rename the destructured owned_refs
binding from _caller_refs to caller_refs in the relevant VM code paths,
including both destructuring sites, and update every explicit drop(_caller_refs)
call to drop(caller_refs). Preserve all drop calls and their existing ordering
relative to exit_iframe.

---

1419-1437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document which side owns releasing the initial iframe.

run_frame_fast never releases the datastack allocation for iframe. On the Return and Err arms it only calls exit_iframe. The callers in crates/vm/src/builtins/function.rs (Lines 645-649 and 822-826) perform the release. On the TailCall arm, however, run_frame_fast_trampoline pushes iframe onto frame_stack and later releases it itself.

The two paths therefore assign release ownership differently. Add a doc line stating that the caller owns the release for the non-tail-call paths, and see the separate comment on the trampoline for the resulting double-release on the tail-call path.

🤖 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 1419 - 1437, Document the
release-ownership contract in run_frame_fast: callers own releasing the initial
iframe for the Return and Err paths, while the tail-call path transfers
ownership to run_frame_fast_trampoline. Add this as a concise doc line without
changing the existing control flow.

---

113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The pending_tailcall_refs side channel has no single access point and no invariant check. The field is pub(crate) and every producer and consumer opens its own unsafe { &mut *...get() } block, so the "one prepare, then one drain" contract is stated nowhere and checked nowhere. The sibling field pending_tailcall_frame already uses private storage with set_pending_tailcall and take_pending_tailcall accessors.

• crates/vm/src/vm/mod.rs#L113-L117: make the field private and add push_pending_tailcall_ref and take_pending_tailcall_refs methods that hold the single unsafe block, then use them at the four drain sites on Lines 1465, 1484, 1532, and 1587.
• crates/vm/src/frame.rs#L10665-L10671: call vm.push_pending_tailcall_ref(callable) and add debug_assert! that the channel was empty on entry; apply the same change to tailcall_prepare_bound_method_frame at Line 10723.

🤖 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 113 - 117, Encapsulate pending
tail-call reference access in VM methods: make pending_tailcall_refs private,
add push_pending_tailcall_ref and take_pending_tailcall_refs with the sole
UnsafeCell access, and replace the four drain-site direct accesses in
crates/vm/src/vm/mod.rs (lines 1465, 1484, 1532, and 1587). In
crates/vm/src/frame.rs lines 10665-10671, update tailcall_prepare_frame to call
vm.push_pending_tailcall_ref(callable) and assert the channel is empty on entry;
apply the same change to tailcall_prepare_bound_method_frame at line 10723.

crates/vm/src/frame.rs (2)

10630-10646: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared callee-frame construction from the two tail-call helpers.

tailcall_prepare_frame (Lines 10630-10646) and tailcall_prepare_bound_method_frame (Lines 10692-10708) contain identical logic: the NEWLOCALS check that builds FrameLocals, and the InterpreterFrame::new_on_datastack call with the same seven arguments. Only the argument-placement offset and the reference-transfer step differ.

Extract the common part into one helper that takes func and returns the callee frame. This keeps the two frame layouts in sync if the construction contract changes.

♻️ Proposed shared constructor

+    /// Build a callee `InterpreterFrame` for `func` on the datastack.
+    fn tailcall_new_callee_frame<'a>(
+        func: &Py<PyFunction>,
+        vm: &VirtualMachine,
+    ) -> &'a mut InterpreterFrame {
+        let code: &Py<PyCode> = &func.code;
+        let locals = if code.flags.contains(bytecode::CodeFlags::NEWLOCALS) {
+            FrameLocals::lazy()
+        } else {
+            FrameLocals::with_locals(crate::function::ArgMapping::from_dict_exact(
+                func.globals.clone(),
+            ))
+        };
+        InterpreterFrame::new_on_datastack(
+            code,
+            &func.globals,
+            &func.builtins,
+            Some(func.as_object()),
+            locals,
+            func.closure.as_ref().map_or(&[], |c| c.as_slice()),
+            vm,
+        )
+    }

Then both helpers reduce to let callee_iframe = Self::tailcall_new_callee_frame(func, vm);.

🤖 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/frame.rs` around lines 10630 - 10646, Extract the duplicated
NEWLOCALS-based FrameLocals setup and InterpreterFrame::new_on_datastack call
from tailcall_prepare_frame and tailcall_prepare_bound_method_frame into a
shared Self::tailcall_new_callee_frame(func, vm) helper returning the callee
frame. Replace both existing construction blocks with calls to this helper,
leaving each method’s distinct argument-placement offset and reference-transfer
logic unchanged.

---

10665-10671: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert that the pending-refs channel is empty before pushing.

The trampoline drains pending_tailcall_refs into the SuspendedFrame that owns the calling frame. That mapping is correct only if each prepare call starts from an empty vector. Nothing enforces the invariant here.

If a future change lets a second prepare run before the trampoline drains, the refs of two callees merge into one SuspendedFrame and are released later than intended. Add a debug assertion so the violation is visible in test builds. Apply the same assertion in tailcall_prepare_bound_method_frame at Line 10723.

🛡️ Proposed assertion

         let callable = self.pop_value();
-        unsafe { &mut *vm.pending_tailcall_refs.get() }.push(callable);
+        let refs = unsafe { &mut *vm.pending_tailcall_refs.get() };
+        debug_assert!(
+            refs.is_empty(),
+            "pending_tailcall_refs not drained by the trampoline"
+        );
+        refs.push(callable);

🤖 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/frame.rs` around lines 10665 - 10671, In the tail-call
preparation flow, assert that vm.pending_tailcall_refs is empty immediately
before pushing the callable in the visible prepare logic. Add the same debug
assertion to tailcall_prepare_bound_method_frame before its corresponding push,
preserving the existing ownership-transfer behavior.

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/frame.rs`:
- Around line 2354-2363: Update the traceback index calculation in the
exception-handling flow to use a saturating subtraction when deriving idx from
exec.lasti(), matching the defensive behavior in gen_throw and unwind_blocks.
Preserve the existing call-site lookup and traceback construction.
- Around line 2292-2308: Extract the InterpreterFrame padding/alignment
calculation from datastack_iframe_total_bytes into a shared helper, then use
that helper for localsplus_offset_aligned in new_on_datastack as well. Ensure
both allocation sizing and the localsplus write offset derive from the same
alignment logic.

In `@crates/vm/src/vm/mod.rs`:
- Around line 1463-1472: The trampoline must distinguish the caller-owned entry
frame from frames it allocates. In crates/vm/src/vm/mod.rs lines 1463-1472, mark
the initial SuspendedFrame as the entry frame; in lines 1543-1551, the Err arm
at line 1558, and Action::Unwind release sites at lines 1602 and 1629, skip
release_datastack_frame and datastack_pop for that frame. Document on
run_frame_fast at lines 1419-1437 that the caller owns entry-frame datastack
release on every arm, including TailCall.
- Around line 1475-1650: Extract a shared helper for the duplicated `match
result` dispatch logic in the surrounding VM execution loop, accepting the
`PyResult<ExecutionResult>` and frame context needed to perform tail-call
suspension, return cleanup, and exception unwinding. Replace all three `match
result` blocks in `Action::EnterCallee`, `Action::ReturnValue`, and handled
`Action::Unwind` with calls to this helper, preserving the existing ownership
and unsafe teardown behavior at every release site.
- Around line 2218-2222: Update exit_iframe to clear the iframe’s previous field
before unlinking the frame chain, matching the teardown behavior in with_frame
and resume_gen_frame. Perform this store before set_current_frame(old_chain) so
heap-backed iframes cannot retain a dangling caller pointer.

---

Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 10630-10646: Extract the duplicated NEWLOCALS-based FrameLocals
setup and InterpreterFrame::new_on_datastack call from tailcall_prepare_frame
and tailcall_prepare_bound_method_frame into a shared
Self::tailcall_new_callee_frame(func, vm) helper returning the callee frame.
Replace both existing construction blocks with calls to this helper, leaving
each method’s distinct argument-placement offset and reference-transfer logic
unchanged.
- Around line 10665-10671: In the tail-call preparation flow, assert that
vm.pending_tailcall_refs is empty immediately before pushing the callable in the
visible prepare logic. Add the same debug assertion to
tailcall_prepare_bound_method_frame before its corresponding push, preserving
the existing ownership-transfer behavior.

In `@crates/vm/src/vm/mod.rs`:
- Line 1524: Rename the destructured owned_refs binding from _caller_refs to
caller_refs in the relevant VM code paths, including both destructuring sites,
and update every explicit drop(_caller_refs) call to drop(caller_refs). Preserve
all drop calls and their existing ordering relative to exit_iframe.
- Around line 1419-1437: Document the release-ownership contract in
run_frame_fast: callers own releasing the initial iframe for the Return and Err
paths, while the tail-call path transfers ownership to
run_frame_fast_trampoline. Add this as a concise doc line without changing the
existing control flow.
- Around line 113-117: Encapsulate pending tail-call reference access in VM
methods: make pending_tailcall_refs private, add push_pending_tailcall_ref and
take_pending_tailcall_refs with the sole UnsafeCell access, and replace the four
drain-site direct accesses in crates/vm/src/vm/mod.rs (lines 1465, 1484, 1532,
and 1587). In crates/vm/src/frame.rs lines 10665-10671, update
tailcall_prepare_frame to call vm.push_pending_tailcall_ref(callable) and assert
the channel is empty on entry; apply the same change to
tailcall_prepare_bound_method_frame at line 10723.

Fix all unresolved CodeRabbit comments on this PR:

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

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: d520647d-6506-4647-ac87-2bba33de3d53

Reviewing files that changed from the base of the PR and between aa4f98a and 30634d6.

• .claude/settings.json
• crates/vm/src/builtins/function.rs
• crates/vm/src/coroutine.rs
• crates/vm/src/frame.rs
• crates/vm/src/vm/mod.rs
• crates/vm/src/vm/thread.rs

• .claude/settings.json

Pull request overview

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

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

• This releases and pops a suspended caller frame unconditionally. When caller_iframe_ptr is the root iframe passed into run_frame_fast_trampoline, that frame is still owned by the outer caller (e.g. PyFunction::invoke_with_locals), which will call release_datastack_frame() after run_frame_fast() returns. Popping it here makes that later access a use-after-free/double-pop.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• This unwind/handler path also unconditionally pops the suspended caller frame. If this is the root iframe passed to run_frame_fast_trampoline, the outer Rust caller will later call release_datastack_frame() on freed memory. Guard the pop so the root frame is not freed here.

                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }
                                    }

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

• Same root-frame ownership issue on the error-unwind path: the trampoline must not datastack_pop the initial iframe that the Rust caller will clean up after run_frame_fast() returns.

                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }
                                    }

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

• Unconditional release_datastack_frame/datastack_pop here can free the root iframe early (before returning from run_frame_fast), leading to a later UAF when the caller performs its own datastack cleanup. Add a root-frame guard.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• Same as other branches: this can pop the root frame that is still owned by the outer Rust caller. Only pop datastack frames that are trampoline-owned, not the initial iframe argument.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• Same issue as the return path: popping caller_iframe here can free the root frame owned by the Rust caller, which is then accessed again after run_frame_fast() returns (UAF/double-pop). The trampoline should only pop frames that are fully owned by the trampoline (i.e., not the initial iframe argument).

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

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

• The trampoline calls caller_iframe.release_datastack_frame() + datastack_pop() when a suspended caller returns/errors. For the root frame passed into run_frame_fast_trampoline, that frame is still owned by the outer caller (e.g. PyFunction::invoke), which also calls iframe.release_datastack_frame() after run_frame_fast returns. This makes TailCall execution vulnerable to double-drop / use-after-free / double-pop of the datastack allocation.

                            drop(_caller_refs);
                            self.exit_iframe(caller_entry);
                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

crates/vm/src/frame.rs:984

• InterpreterFrame::new_on_datastack returns &'a mut Self with a lifetime parameter 'a that is not tied to any input (including vm). Because the reference is created from a raw pointer, this lets callers within the crate accidentally (or indirectly via inference) treat the returned &mut InterpreterFrame as having an arbitrary lifetime, which is unsound for a safe function and can lead to use-after-pop UB.

    pub(crate) fn new_on_datastack<'a>(
        code: &Py<PyCode>,
        globals: &Py<PyDict>,
        builtins: &PyObject,
        func_obj: Option<&PyObject>,
        locals: FrameLocals,
        closure: &[PyCellRef],
        vm: &VirtualMachine,
    ) -> &'a mut Self {

crates/vm/src/frame.rs:2357

• let idx = exec.lasti() as usize - 1; can underflow if lasti() is 0, producing a huge idx and causing the traceback callsite lookup to be skipped (and potentially hiding trace context). Other codepaths in this file use saturating_sub(1) for the same pattern.

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = exec.lasti() as usize - 1;

crates/vm/src/builtins/function.rs:648

• PyFunction::invoke always calls iframe.release_datastack_frame() after vm.run_frame_fast(iframe). With the new trampoline, run_frame_fast_trampoline may already have released/popped the root frame's datastack allocation during TailCall unwinding/return, which would make this cleanup a use-after-free/double-pop. Frame allocation ownership between the caller and the trampoline needs to be made consistent.

        let result = self
            .fill_locals_from_args_iframe(iframe, func_args, vm)
            .and_then(|()| vm.run_frame_fast(iframe));
        // Release data stack memory — must happen on both success and error.
        unsafe {
            if let Some(base) = iframe.release_datastack_frame() {
                vm.datastack_pop(base);
            }

Pull request overview

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

crates/vm/src/frame.rs:2357

• trampoline_handle_exception computes the call-site index as exec.lasti() - 1, but lasti is advanced to the next instruction and may have skipped over one or more inline CACHE entries. This can underflow when lasti==0 and can also attach traceback entries to a CACHE op rather than the actual CALL instruction.

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = exec.lasti() as usize - 1;

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

• The trampoline drains pending_tailcall_refs via .drain(..).collect(), which allocates a fresh Vec on every TailCall. Since TailCall is intended to be the hot path, this per-call heap allocation likely eats into the intended call-overhead reduction. Consider an allocation-free transfer (e.g., moving/swapping a preallocated buffer, or using a small inline container for the common 1–2 refs).

        let initial_ptr = self.take_pending_tailcall();
        // Drain the refs that keep the initial callee's raw pointers alive.
        let initial_refs = unsafe { &mut *self.pending_tailcall_refs.get() }
            .drain(..)
            .collect();

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

• pending_tailcall_refs is an UnsafeCell<Vec<PyObjectRef>> but is exposed as pub(crate), which makes it easy for unrelated code to accidentally access it without upholding the single-thread / single-borrow invariants. For safety, it would be better to keep this field private (like pending_tailcall_frame) and expose only narrowly-scoped helper methods (e.g., push/drain/swap) that enforce the intended usage.

    /// Side channel for TailCall: the bytecode loop stores the new frame
    /// pointer here before returning `ExecutionResult::TailCall`.
    /// Access only via `set_pending_tailcall` / `take_pending_tailcall`.
    pending_tailcall_frame: Cell<Option<PendingFrame>>,
    /// Owned references that keep callee raw pointers valid during TailCall.
    /// Set by `tailcall_prepare_frame`, drained by the trampoline into
    /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
    /// per-thread and this field is only accessed on the owning thread.
    pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,

crates/vm/src/vm/method.rs:26

• This compares function pointers directly and silences unpredictable_function_pointer_comparisons. Elsewhere in the codebase, function-pointer equality uses core::ptr::fn_addr_eq to avoid platform/linker edge cases (e.g. crates/vm/src/vm/vm_ops.rs:353). Using fn_addr_eq here would avoid relying on an allowed lint.

        #[allow(unpredictable_function_pointer_comparisons)]
        if getattro != PyBaseObject::getattro {

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/builtins/type.rs`:
- Around line 2830-2834: Remove the function-pointer equality check from the
constructor fast path around `slot_new` and `is_wrapper`. Track whether the
native slot originated from `crate::types::new_wrapper` separately, or disable
the fast path when that origin is unknown, so wrapper constructors always reach
the later `__init__` call; do not replace the comparison with `fn_addr_eq`.

Fix all unresolved CodeRabbit comments on this PR:

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

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 19a4f9bd-bc83-4d98-b536-dc08e8a132bd

Reviewing files that changed from the base of the PR and between 29e9b20 and a1fa320.

• crates/vm/src/builtins/type.rs
• crates/vm/src/class.rs
• crates/vm/src/frame.rs
• crates/vm/src/types/slot.rs
• crates/vm/src/vm/method.rs

• crates/vm/src/class.rs
• crates/vm/src/vm/method.rs
• crates/vm/src/frame.rs

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

crates/vm/src/frame.rs:984

• InterpreterFrame::new_on_datastack returns &'a mut Self with an unconstrained lifetime parameter 'a. Because 'a is not tied to any input, callers can effectively choose any lifetime (including 'static), which makes this safe function unsound and can lead to use-after-free of datastack memory.

        func_obj: Option<&PyObject>,
        locals: FrameLocals,
        closure: &[PyCellRef],
        vm: &VirtualMachine,
    ) -> &'a mut Self {

crates/vm/src/vm/vm_ops.rs:287

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint, but without the clippy:: namespace it will be treated as an unknown lint by rustc and won't silence Clippy in CI. Use the tool-lint name instead.

    #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/types/slot.rs:957

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Use clippy::... here.

                        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/types/slot.rs:989

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Use clippy::... here.

                        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/frame.rs:10175

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

                        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/frame.rs:10961

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/class.rs:210

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

                    #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/builtins/type.rs:3070

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

    #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/frame.rs:2357

• let idx = exec.lasti() as usize - 1; can underflow when lasti == 0 (panic in debug, wrap in release). Other traceback-attachment sites in this file use saturating_sub(1) for the same reason.

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = exec.lasti() as usize - 1;

crates/vm/src/vm/method.rs:25

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as an unknown lint. Prefer the namespaced form.

        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/types/slot_defs.rs:612

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown. Use the namespaced form.

                        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/stdlib/_thread.rs:994

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown. Use the namespaced form.

            #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/vm/vm_ops.rs:177

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint, but without the clippy:: namespace it will be treated as an unknown lint by rustc and won't silence Clippy in CI. Use the tool-lint name instead.

This issue also appears on line 287 of the same file.

    #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/types/slot.rs:880

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Use clippy::... here.

This issue also appears in the following locations of the same file:

• line 957
• line 989

                            #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/frame.rs:9152

• This #[allow(unpredictable_function_pointer_comparisons)] attribute appears to be a Clippy lint; without the clippy:: namespace it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

This issue also appears in the following locations of the same file:

• line 10175
• line 10961

        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/class.rs:26

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

This issue also appears on line 210 of the same file.

        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/builtins/type.rs:2831

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

This issue also appears on line 3070 of the same file.

                #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/builtins/set.rs:962

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

        #[allow(unpredictable_function_pointer_comparisons)]

crates/vm/src/builtins/object.rs:128

• This #[allow(unpredictable_function_pointer_comparisons)] attribute looks like a Clippy lint; without clippy:: it won't silence Clippy and may warn as unknown lint. Use the namespaced form.

        #[allow(unpredictable_function_pointer_comparisons)]

Pull request overview

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

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

• In the unwind path, the trampoline also drops the caller frame unconditionally. When the caller is the root iframe passed to run_frame_fast_trampoline, this will conflict with the outer cleanup that also drops/pops the datastack frame.

                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }
                                    }

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

• Same issue as the previous block: the trampoline is dropping the caller frame on the error path. If this is the root iframe passed to run_frame_fast, it will be dropped here and then accessed again by the outer call site cleanup (iframe.release_datastack_frame()), which is undefined behavior.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• Same root-frame double-drop issue on the unwind+error path: the trampoline drops caller_iframe even when it may be the root iframe still owned by the outer caller of run_frame_fast.

                                        if let Some(base) = caller_iframe.release_datastack_frame()
                                        {
                                            self.datastack_pop(base);
                                        }
                                    }

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

• The trampoline drops/pops the caller frame here as well; if this frame is the original iframe passed to run_frame_fast_trampoline, this will cause the outer call site to access a dropped InterpreterFrame during its own datastack cleanup.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

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

• Same as above: this unconditional release_datastack_frame() in the trampoline can drop the root iframe that the outer call site still expects to clean up after run_frame_fast returns.

                            unsafe {
                                if let Some(base) = caller_iframe.release_datastack_frame() {
                                    self.datastack_pop(base);
                                }
                            }

crates/vm/src/frame.rs:980

• InterpreterFrame::new_on_datastack is a safe function but returns &'a mut Self where 'a is unconstrained by any input lifetimes. That makes the API unsound (callers can choose an arbitrary lifetime like 'static), allowing use of a datastack-backed frame reference after the datastack allocation has been popped.

    pub(crate) fn new_on_datastack<'a>(
        code: &Py<PyCode>,
        globals: &Py<PyDict>,
        builtins: &PyObject,
        func_obj: Option<&PyObject>,

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

• pending_tailcall_refs is pub(crate) and exposes an UnsafeCell<Vec<PyObjectRef>> across the crate, which makes it easy for new call sites to bypass the intended invariants ("set by tailcall_prepare_frame, drained by trampoline") and spread unsafe { &mut *...get() } usages. Consider making this field private and providing small helper methods on VirtualMachine (e.g., push_pending_tailcall_ref, drain_pending_tailcall_refs_into(&mut Vec<_>)) so the unsafety and invariants are centralized.

    /// Owned references that keep callee raw pointers valid during TailCall.
    /// Set by `tailcall_prepare_frame`, drained by the trampoline into
    /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
    /// per-thread and this field is only accessed on the owning thread.
    pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,

crates/vm/src/frame.rs:2357

• let idx = exec.lasti() as usize - 1; will underflow if lasti() is 0, producing a huge index and potentially skipping traceback attachment. Even if current invariants make lasti()>0 here, it’s safer to encode that assumption explicitly to avoid silent wraparound if this helper is reused or called from another opcode.

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = exec.lasti() as usize - 1;

crates/vm/src/builtins/function.rs:882

• datastack_frame_size_bytes_for_code used to be fallible (returning None on size arithmetic overflow). After this change it always returns Some(...) and datastack_iframe_total_bytes uses expect(...) internally, so a crafted PyCode with huge localspluskinds.len() / max_stackdepth could panic the VM (DoS) instead of cleanly failing the fast-path.

    let nlocalsplus = code.localspluskinds.len();
    Some(crate::frame::datastack_iframe_total_bytes(
        nlocalsplus,
        code.max_stackdepth as usize,
    ))

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

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

• pending_tailcall_refs is documented as a thread-local TailCall side channel, but it is pub(crate) and exposes UnsafeCell<Vec<PyObjectRef>> to the entire crate. This makes it easy for other code paths (including future changes) to mutate it without the intended single-thread / set+drain protocol, weakening the safety invariant around raw frame pointers.

    /// Owned references that keep callee raw pointers valid during TailCall.
    /// Set by `tailcall_prepare_frame`, drained by the trampoline into
    /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
    /// per-thread and this field is only accessed on the owning thread.
    pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,

crates/vm/src/frame.rs:2356

• trampoline_handle_exception computes idx with exec.lasti() as usize - 1, which will underflow if lasti is 0, producing a huge index and potentially skipping traceback attachment / causing unexpected behavior. Other traceback sites in this file use saturating_sub(1) to avoid this.

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = exec.lasti() as usize - 1;

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

crates/vm/src/frame.rs:977

• InterpreterFrame::new_on_datastack<'a>(...) -> &'a mut Self returns a mutable reference with a lifetime parameter that is not tied to any input. This makes it possible (within the crate) to extend the returned reference beyond the datastack allocation’s validity in otherwise-safe code, which is unsound (use-after-free / UB) if release_datastack_frame+datastack_pop have occurred.

    #[allow(clippy::too_many_arguments)]
    pub(crate) fn new_on_datastack<'a>(
        code: &Py<PyCode>,
        globals: &Py<PyDict>,
        builtins: &PyObject,

crates/vm/src/frame.rs:2362

• trampoline_handle_exception computes the traceback instruction index as lasti - 1, but lasti in this VM points past the instruction and its inline CACHE entries (the run loop skips caches). That means this may attach the traceback entry to a CACHE slot rather than the actual call-site opcode, producing an incorrect tb_lasti / location (unlike the normal exception path, which uses the real instruction index).

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = (exec.lasti() as usize).saturating_sub(1);

    // Add traceback entry at the call site.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

crates/vm/src/frame.rs:983

• InterpreterFrame::new_on_datastack is a safe function that returns &'a mut Self with an unconstrained generic lifetime 'a. This lets callers create a reference that (in safe Rust) can outlive the datastack allocation, which is unsound and can lead to use-after-free/UB. This API should be unsafe (caller must uphold the LIFO lifetime) and/or return a raw pointer / RAII guard type instead of an arbitrary-lifetime &mut reference.

    pub(crate) fn new_on_datastack<'a>(
        code: &Py<PyCode>,
        globals: &Py<PyDict>,
        builtins: &PyObject,
        func_obj: Option<&PyObject>,
        locals: FrameLocals,
        closure: &[PyCellRef],
        vm: &VirtualMachine,
    ) -> &'a mut Self {
        let nlocalsplus = code.localspluskinds.len();

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

• run_frame_fast/run_frame_fast_trampoline manually call enter_iframe/exit_iframe without a guard. If run_iframe (or any code in the trampoline) panics, the frame chain/TLS (CURRENT_FRAME), recursion depth, and exception state may not be restored, and datastack-allocated callee frames may not be released. Previously with_iframe provided panic-safety via scopeguard; the trampoline path should provide equivalent unwind cleanup (e.g., scopeguard for the entry frame plus a Drop-based cleanup that exits/unlinks any entered frames and pops datastack allocations).

    pub fn run_frame_fast(&self, iframe: &mut crate::frame::InterpreterFrame) -> PyResult {
        use crate::frame::ExecutionResult;

        let entry_state = self.enter_iframe(iframe)?;
        let result = crate::frame::run_iframe(iframe, self);

        match result {
            Ok(ExecutionResult::Return(value)) => {
                self.exit_iframe(entry_state);
                Ok(value)
            }
            Ok(ExecutionResult::TailCall) => self.run_frame_fast_trampoline(iframe, entry_state),
            Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"),
            Err(exc) => {
                self.exit_iframe(entry_state);
                Err(exc)

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

• set_pending_tailcall overwrites pending_tailcall_frame unconditionally. If a logic bug ever causes a second TailCall to be prepared before the trampoline consumes the first, this would silently drop the earlier pending frame pointer (and its associated refs), making debugging much harder and potentially leaking datastack allocations. Adding a debug_assert! here helps catch invariant violations early.

    pub(crate) fn set_pending_tailcall(&self, iframe: &mut crate::frame::InterpreterFrame) {
        self.pending_tailcall_frame
            .set(Some(PendingFrame(core::ptr::NonNull::from(iframe))));
    }

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

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

• run_frame_fast() manually calls enter_iframe() / exit_iframe() but doesn’t guard exit_iframe() against panics from run_iframe() (or the Yield in non-generator frame panic). That can leave TLS frame chain + recursion_depth inconsistent after an unwind.

        let entry_state = self.enter_iframe(iframe)?;
        let result = crate::frame::run_iframe(iframe, self);

        match result {
            Ok(ExecutionResult::Return(value)) => {

crates/vm/src/frame.rs:978

• InterpreterFrame::new_on_datastack<'a>(...) -> &'a mut Self returns a reference with an unconstrained lifetime parameter. This is unsound: callers can infer/choose 'a to outlive the datastack allocation, creating a safe-looking &mut InterpreterFrame that can be used after datastack_pop() (UB).

    pub(crate) fn new_on_datastack<'a>(
        code: &Py<PyCode>,
        globals: &Py<PyDict>,
        builtins: &PyObject,
        func_obj: Option<&PyObject>,

crates/vm/src/frame.rs:2369

• trampoline_handle_exception() computes the call-site index as lasti - 1, but lasti is advanced past inline cache entries by the main run loop. For CALL_*_EXACT_ARGS this can point at an Instruction::Cache, producing incorrect traceback locations/offsets. The run loop already handles this case elsewhere by scanning backwards past CACHE entries (e.g. InstrumentedNotTaken).

    // lasti points past the CallPyExactArgs instruction (+ cache entries).
    // The exception occurred at the previous instruction (the call site).
    let idx = (exec.lasti() as usize).saturating_sub(1);

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

• pending_tailcall_refs is pub(crate) even though its safety comment says it should be accessed only through the tailcall side-channel logic. Exposing an UnsafeCell<Vec<PyObjectRef>> across the crate makes it easy to accidentally introduce aliased mutable access or cross-thread access later, undermining the safety invariant.

    /// Owned references that keep callee raw pointers valid during TailCall.
    /// Set by `tailcall_prepare_frame`, drained by the trampoline into
    /// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
    /// per-thread and this field is only accessed on the owning thread.
    pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,