Skip to content

Flatten Python eval loop with trampoline for reduced call overhead - #8431

Open
youknowone wants to merge 10 commits into
RustPython:mainfrom
youknowone:flatten-eval-loop
Open

Flatten Python eval loop with trampoline for reduced call overhead#8431
youknowone wants to merge 10 commits into
RustPython:mainfrom
youknowone:flatten-eval-loop

Conversation

@youknowone

@youknowone youknowone commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Flatten the Python-to-Python call path so that CallPyExactArgs and CallBoundMethodExactArgs no longer recurse through new Rust stack frames. Instead, the bytecode loop returns a TailCall signal and a trampoline swaps frames in a single loop — matching CPython 3.12+'s approach.

close youknowone#40

Changes

Phase 0: Factor with_iframe

  • Extract enter_iframe/exit_iframe from with_iframe with IframeEntryState struct
  • No behavioral change — prepares composable pieces for the trampoline

Phase 1: Datastack-allocated InterpreterFrame

  • InterpreterFrame::new_on_datastack() bump-allocates both the InterpreterFrame and its LocalsPlus in a single datastack push
  • release_datastack_frame() drops values and returns the base pointer for datastack_pop

Phase 2–4: Trampoline loop + TailCall + exception propagation

  • Add ExecutionResult::TailCall variant
  • Add pending_tailcall_frame side channel on VirtualMachine
  • Implement run_frame_fast_trampoline() with Vec<SuspendedFrame> stack
  • tailcall_prepare_frame() builds callee frame on datastack and stores pointer
  • Exception propagation via trampoline_handle_exception() — unwinds through suspended callers, attaches traceback entries
  • Generators/coroutines and tracing fall back to recursive path

Phase 5: Optimizations

  • enter_iframe_unchecked skips recursion/C-stack checks in trampoline (already verified by specialization_call_recursion_guard)
  • Move callable ownership from per-frame temporary_refs mutex to trampoline-local SuspendedFrame.owned_refs via VM side channel — eliminates mutex lock + Vec allocation per call
  • Add TailCall support for CallBoundMethodExactArgs
  • Move args directly from caller stack to callee fastlocals (no intermediate buffer)
  • Read materialized pointer once in exit_iframe

Safety

  • PendingFrame wrapper for the tailcall side channel is fully private — the struct, its inner NonNull, and the Cell field are all non-pub. External code accesses only through set_pending_tailcall / take_pending_tailcall, preventing reuse of the unsafe Send+Sync impl elsewhere.

Performance

Metric Before (PR #8354) After Change
Incremental call overhead ~55 ns ~35 ns -36%
fib(28) ~178 ms ~137 ms -23%

Measured on Apple Silicon. The remaining gap to the ≤30 ns target is addressable by InterpreterFrame hot/cold field splitting (separate work).

Test coverage

All existing tests pass: test_frame, test_traceback, test_generators, test_sys, test_pdb, test_exceptions, test_call, test_funcattrs, test_descr, test_faulthandler.

Deep recursion (RecursionError), exception propagation through trampoline, sys._getframe() chain, and try/except across call boundaries all verified.

Refs: youknowone#40

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Performance

    • Improved execution efficiency for Python functions and bound methods.
    • Reduced overhead from function calls and execution-frame management.
    • Improved handling of deep call chains for more stack-safe execution.
  • Reliability

    • Strengthened exception handling during complex execution flows.
    • Improved coroutine and generator execution behavior.
    • Enhanced cleanup and garbage collection during frame transitions.

Copilot AI review requested due to automatic review settings August 2, 2026 06:17
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9dee1254-e610-4552-b502-1df548b44562

📥 Commits

Reviewing files that changed from the base of the PR and between ca7ebef and a2e9a49.

📒 Files selected for processing (2)
  • crates/vm/src/frame.rs
  • crates/vm/src/vm/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/frame.rs

📝 Walkthrough

Walkthrough

The VM now allocates interpreter frames with co-located datastack storage and executes eligible Python calls through a non-recursive tail-call trampoline. Frame lifecycle cleanup and exception handling are centralized. The Claude session-start configuration was removed.

Changes

Datastack tail-call execution

Layer / File(s) Summary
Frame storage and execution contracts
crates/vm/src/frame.rs
InterpreterFrame now supports datastack allocation, materialization, cleanup, sizing, GC traversal, ExecutionResult::TailCall, and trampoline exception handling.
Datastack callee preparation
crates/vm/src/builtins/function.rs, crates/vm/src/frame.rs
Function and bound-method calls prepare eligible datastack frames, transfer arguments, and release frames through the centralized lifecycle.
VM trampoline and frame lifecycle
crates/vm/src/vm/mod.rs, crates/vm/src/vm/thread.rs, crates/vm/src/coroutine.rs
VirtualMachine iteratively executes pending tail calls, restores iframe state, propagates exceptions, initializes tail-call storage, and marks unsupported coroutine tail calls unreachable.

Environment hook removal
.claude/settings.json|The Claude SessionStart hook and its environment setup configuration were removed.|

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested reviewers: moreal, fanninpm, copilot

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements issue #40's trampoline and datastack changes, but reported performance remains above its 30 ns and supporting targets. Optimize the remaining call-path overhead until issue #40's primary and supporting performance criteria are met, or update the issue scope.
Out of Scope Changes check ⚠️ Warning Removing .claude/settings.json is unrelated to the Python evaluation-loop performance objectives in issue #40. Restore the unrelated .claude/settings.json deletion or move it to a separate pull request.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main trampoline-based evaluation-loop change and its call-overhead goal.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 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 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.

Show a summary per file
File Description
crates/vm/src/vm/thread.rs Initializes new per-thread VM fields used by the tailcall trampoline side channels.
crates/vm/src/vm/mod.rs Implements TailCall side channel plumbing, trampoline loop, and factors with_iframe into enter_iframe/exit_iframe.
crates/vm/src/frame.rs Adds TailCall result type, datastack frame allocation/release, trampoline exception handling, and tailcall preparation from the bytecode loop.
crates/vm/src/coroutine.rs Marks TailCall as unreachable for generator/coroutine execution paths.
crates/vm/src/builtins/function.rs Switches PyFunction fast-path execution to build frames via InterpreterFrame::new_on_datastack() and uses the updated execution API.
.claude/settings.json Removes Claude session hook configuration file.
Suppressed comments (5)

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.

Comment thread crates/vm/src/vm/mod.rs Outdated
Comment on lines +1546 to +1550
unsafe {
if let Some(base) = caller_iframe.release_datastack_frame() {
self.datastack_pop(base);
}
}

@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: 5

🧹 Nitpick comments (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.
🤖 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/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.
🪄 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: d520647d-6506-4647-ac87-2bba33de3d53

📥 Commits

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

📒 Files selected for processing (6)
  • .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
💤 Files with no reviewable changes (1)
  • .claude/settings.json

Comment thread crates/vm/src/frame.rs
Comment thread crates/vm/src/frame.rs
Comment thread crates/vm/src/vm/mod.rs
Comment thread crates/vm/src/vm/mod.rs
Comment thread crates/vm/src/vm/mod.rs
Copilot AI review requested due to automatic review settings August 2, 2026 06:45

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 8 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

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

Comment thread crates/vm/src/frame.rs
Comment on lines +976 to +980
pub(crate) fn new_on_datastack<'a>(
code: &Py<PyCode>,
globals: &Py<PyDict>,
builtins: &PyObject,
func_obj: Option<&PyObject>,
Copilot AI review requested due to automatic review settings August 2, 2026 07:05

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 8 changed files in this pull request and generated no new comments.

Suppressed comments (4)

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

Copilot AI review requested due to automatic review settings August 2, 2026 07: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 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

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 {

Comment thread crates/vm/src/vm/mod.rs
Comment on lines +2241 to 2244
let state = self.enter_iframe(iframe)?;
let result = f(iframe);
self.exit_iframe(state);
result

@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/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`.
🪄 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: 19a4f9bd-bc83-4d98-b536-dc08e8a132bd

📥 Commits

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

📒 Files selected for processing (5)
  • 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
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/vm/src/class.rs
  • crates/vm/src/vm/method.rs
  • crates/vm/src/frame.rs

Comment thread crates/vm/src/builtins/type.rs Outdated
Comment on lines +2830 to +2834
if zelf.slots.init.load().is_none() && !zelf.is(vm.ctx.types.type_type) && {
#[allow(unpredictable_function_pointer_comparisons)]
let is_wrapper = slot_new == (crate::types::new_wrapper as crate::types::NewFunc);
!is_wrapper
} {

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 | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

if rg -n 'slot_new\s*==\s*\(crate::types::new_wrapper|new_wrapper.*==.*slot_new' \
    crates/vm/src/builtins/type.rs; then
  echo "The constructor fast path still depends on function-pointer equality." >&2
  exit 1
fi

Repository: RustPython/RustPython

Length of output: 333


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== candidate file outline =="
ast-grep outline crates/vm/src/builtins/type.rs --match 'slot_new' --view expanded || true

echo "== relevant lines =="
sed -n '2800,2855p' crates/vm/src/builtins/type.rs | nl -ba -v2800

echo "== new_wrapper definition/usages =="
rg -n "fn new_wrapper|new_wrapper|type_new|NewFunc|fn_addr_eq|slot_new ==" crates/vm/src -g '*.rs'

Repository: RustPython/RustPython

Length of output: 315


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== relevant lines =="
sed -n '2800,2855p' crates/vm/src/builtins/type.rs

echo "== new_wrapper definitions/usages =="
rg -n "fn new_wrapper|new_wrapper|type_new|NewFunc|fn_addr_eq|slot_new ==" crates/vm/src -g '*.rs'

Repository: RustPython/RustPython

Length of output: 4454


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "== types/slot.rs relevant sections =="
sed -n '300,320p' crates/vm/src/types/slot.rs
sed -n '620,665p' crates/vm/src/types/slot.rs
sed -n '855,890p' crates/vm/src/types/slot.rs

echo "== vm/vm_ops.rs address-equality usage =="
sed -n '340,365p' crates/vm/src/vm/vm_ops.rs

echo "== build/dependency evidence for Rust edition and unsafe-function ptr behavior docs, if unavailable, source snippets are enough =="
rg -n 'edition\s*=' Cargo.toml crates/vm/Cargo.toml
rg -n 'r#"fn_ptr_expr|fn ptr comparison|unsafe-function pointer comparisons' crates packages Cargo.lock 2>/dev/null || true

Repository: RustPython/RustPython

Length of output: 6009


🌐 Web query:

Rust UnsafeCode Guide function pointer comparison undefined behavior fn_addr_eq

💡 Result:

Comparing function pointers in Rust—whether using the == operator or the std::ptr::fn_addr_eq function—is inherently unpredictable and does not provide a reliable check for functional equality [1][2][3]. While this comparison is not typically classified as Undefined Behavior (UB) itself, the underlying reasons for its unreliability are tied to how the compiler and linker manage function code, which can lead to unexpected runtime outcomes [1][4][5]. Key details regarding function pointer comparison: - Lack of Identity: Functions in Rust do not have an intrinsic "identity" [1][2]. The compiler may duplicate identical functions across different code generation units, or conversely, it may merge distinct functions that result in identical machine code (deduplication) [1][6][5]. - Unpredictable Results: Because of these optimizations, comparing two function pointers can return false even for the same function, or true for distinct functions [1][7]. - Official Guidance: Rust issues a warning, unpredictable_function_pointer_comparisons, whenever standard function pointer comparisons are detected [3][5]. The function std::ptr::fn_addr_eq was introduced as a clearer, explicit alternative for developers who specifically need to compare the raw memory addresses of function pointers, making it evident that the developer is aware of these unreliable, platform-dependent semantics [1][6][8]. - Not UB, but Risky: The act of comparing the pointers is generally a defined operation (returning a boolean), but relying on the result for program logic is discouraged because the result is unstable and subject to compiler optimization settings and linkage behavior [1][4][9]. In summary, std::ptr::fn_addr_eq is the recommended way to perform address-based comparison if it is required, but it does not guarantee that the functions are logically equivalent or that the comparison results will be consistent across different builds or optimization levels [1][7][10].

Citations:


Do not rely on function-pointer equality for the constructor fast path.

Rust function-pointer comparisons can return false for the same function, and fn_addr_eq has the same semantics. If slot_new is new_wrapper but the comparison is false, !is_wrapper becomes true and slot_new returns before the later __init__ call runs. Track the wrapper/native slot origin separately, or disable this fast path unless the slot source is known; avoid using fn_addr_eq as the fix.

🤖 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/type.rs` around lines 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`.

Copilot AI review requested due to automatic review settings August 2, 2026 08:04

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 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (19)

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

Copilot AI review requested due to automatic review settings August 2, 2026 08:56

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 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (9)

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

Comment thread crates/vm/src/vm/mod.rs Outdated
Comment on lines +1546 to +1550
unsafe {
if let Some(base) = caller_iframe.release_datastack_frame() {
self.datastack_pop(base);
}
}
Copilot AI review requested due to automatic review settings August 2, 2026 12:40

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 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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;

Copilot AI review requested due to automatic review settings August 2, 2026 13:53

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 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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

  • datastack_frame_size_bytes_for_code now wraps datastack_iframe_total_bytes(...) in Some(...), but datastack_iframe_total_bytes uses expect(...) on checked_add/checked_mul and can panic on overflow. This changes the function’s semantics from “return None on overflow” to “abort the interpreter”, which is reachable via extremely large code objects (DoS). Consider keeping this helper overflow-safe and returning None instead of panicking.
pub(crate) fn datastack_frame_size_bytes_for_code(code: &Py<PyCode>) -> Option<usize> {
    if code.flags.intersects(
        bytecode::CodeFlags::GENERATOR
            | bytecode::CodeFlags::COROUTINE
            | bytecode::CodeFlags::ASYNC_GENERATOR,
    ) {
        return None;
    }
    let nlocalsplus = code.localspluskinds.len();
    Some(crate::frame::datastack_iframe_total_bytes(
        nlocalsplus,
        code.max_stackdepth as usize,
    ))
}

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

  • The trampoline drains pending_tailcall_refs into owned_refs via .drain(..).collect(). collect() allocates a new Vec each time a TailCall is handled (even though this staging Vec is sized to 1–2 elements), which undercuts the stated goal of removing per-call allocations in the fast call path. Consider storing owned refs in a fixed-size container (since this path currently pushes 1–2 refs) or otherwise moving/swap’ing buffers so the existing allocation can be reused without creating a fresh Vec per 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();
        frame_stack.push(SuspendedFrame {

@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

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/type.rs (1)

2832-2833: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Replace raw function-address comparisons with explicit slot-origin metadata.

crate::types::fn_addr converts function pointers to integer addresses. Do not use that value as semantic function identity.

  • crates/vm/src/builtins/type.rs#L2832-L2833: preserve argument cloning whenever the new_wrapper origin is unknown.
  • crates/vm/src/builtins/type.rs#L3070-L3071: preserve the call_slot_new safety rejection unless the slot origins are known to match.
  • crates/vm/src/stdlib/_thread.rs#L1000-L1001: distinguish object.__init__ from custom initializers without address equality.

The constructor fast-path finding was already reported in the previous review and remains unresolved.

#!/usr/bin/env bash
set -euo pipefail
rg -n -A6 -B6 \
  'fn_addr\(|new_wrapper|call_slot_new|custom_init' \
  crates/vm/src/types \
  crates/vm/src/builtins/type.rs \
  crates/vm/src/stdlib/_thread.rs
🤖 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/type.rs` around lines 2832 - 2833, Replace raw
fn_addr-based identity checks at crates/vm/src/builtins/type.rs:2832-2833,
crates/vm/src/builtins/type.rs:3070-3071, and
crates/vm/src/stdlib/_thread.rs:1000-1001 with explicit slot-origin metadata. In
the argument-cloning path, clone whenever the new_wrapper origin is unknown; in
the call_slot_new safety check, reject unless origins are known to match; and in
the custom_init handling, distinguish object.__init__ from custom initializers
using origin metadata rather than address equality. Leave the previously
reported constructor fast-path unchanged.
🤖 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/object/ext.rs`:
- Line 336: Use the live InterpreterFrame’s temporary_refs for PyAtomicRef
cleanup instead of the detached materialized frame returned by
vm.current_frame(). Update the temporary-ref pushes at
crates/vm/src/object/ext.rs:336, 412, 455, and 502, and
crates/vm/src/builtins/type.rs:1516, ensuring exit_iframe() clears the same live
iframe storage.

---

Outside diff comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 2832-2833: Replace raw fn_addr-based identity checks at
crates/vm/src/builtins/type.rs:2832-2833,
crates/vm/src/builtins/type.rs:3070-3071, and
crates/vm/src/stdlib/_thread.rs:1000-1001 with explicit slot-origin metadata. In
the argument-cloning path, clone whenever the new_wrapper origin is unknown; in
the call_slot_new safety check, reject unless origins are known to match; and in
the custom_init handling, distinguish object.__init__ from custom initializers
using origin metadata rather than address equality. Leave the previously
reported constructor fast-path unchanged.
🪄 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: db2504e0-0b70-4e3d-80b7-be8ba9eb79c8

📥 Commits

Reviewing files that changed from the base of the PR and between a1fa320 and 212d9c1.

📒 Files selected for processing (11)
  • .claude/settings.json
  • crates/vm/src/builtins/frame.rs
  • crates/vm/src/builtins/function.rs
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/coroutine.rs
  • crates/vm/src/frame.rs
  • crates/vm/src/object/ext.rs
  • crates/vm/src/protocol/callable.rs
  • crates/vm/src/stdlib/_thread.rs
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/vm/thread.rs
💤 Files with no reviewable changes (1)
  • .claude/settings.json
🚧 Files skipped from review as they are similar to previous changes (5)
  • crates/vm/src/vm/thread.rs
  • crates/vm/src/coroutine.rs
  • crates/vm/src/builtins/function.rs
  • crates/vm/src/vm/mod.rs
  • crates/vm/src/frame.rs

Comment thread crates/vm/src/object/ext.rs
Copilot AI review requested due to automatic review settings August 3, 2026 01:25

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 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

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.

Comment thread crates/vm/src/frame.rs Outdated
/// Panics on overflow.
pub(crate) fn push_stack(&mut self, value: PyObjectRef) {
self.stack_try_push(Some(PyStackRef::new_owned(value)))
.unwrap_or_else(|_| panic!("stack overflow in push_stack"));

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.

isn't this equals to .expect(...)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

omg claude lied in comment. fixed it

Extract the frame entry (recursion check, TLS link, exception save)
and exit (materialization sync, TLS restore, GC tracking) logic
from with_iframe into standalone enter_iframe/exit_iframe methods.
with_iframe now calls them, with no behavioral change.

This prepares for the trampoline loop where enter/exit are called
individually rather than wrapped around a closure.

Assisted-by: Claude
Add InterpreterFrame::new_on_datastack() that bump-allocates both the
InterpreterFrame struct and its LocalsPlus data array in a single
datastack push, eliminating one allocation per function call.

Update datastack_frame_size_bytes_for_code() to include InterpreterFrame
size. Convert invoke_prepared_exact_args() and the invoke() fast path to
use the combined allocation.

Add release_datastack_frame() method on InterpreterFrame that drops all
localsplus values, runs field destructors (trace, temporary_refs,
retained_back, etc.), and returns the datastack base pointer for pop.

Assisted-by: Claude
Add ExecutionResult::TailCall variant and a trampoline in
run_frame_fast that flattens Python-to-Python calls into a single
Rust stack frame instead of recursing through the eval loop.

CallPyExactArgs now prepares the callee frame on the datastack and
returns TailCall when tailcall_enabled is set (run_iframe path only).
The trampoline dispatches via a state machine (EnterCallee / ReturnValue
/ Unwind) in a single loop, avoiding mutual recursion between helper
functions that would exhaust the C stack.

Exception propagation through suspended frames uses
trampoline_handle_exception which adds traceback entries and calls
unwind_blocks on each caller.

Assisted-by: Claude
…, add bound method TailCall

- Add enter_iframe_unchecked for trampoline callee entry (recursion
  already checked by specialization_call_recursion_guard)
- Move callable ownership from per-frame temporary_refs mutex to
  trampoline-local SuspendedFrame.owned_refs via VM side channel
- Add TailCall support for CallBoundMethodExactArgs
- Move args directly from caller stack to callee fastlocals
- Read materialized pointer once in exit_iframe

Incremental call overhead: ~55 ns -> ~35 ns

Assisted-by: Claude
The VM is per-thread so RefCell's runtime borrow checking is
unnecessary overhead. Replace with UnsafeCell for direct access.

Assisted-by: Claude
Use NonNull + Option instead of raw *mut T with manual null checks.
The compiler enforces non-null via the type system, and Option<NonNull>
has the same size as a raw pointer thanks to niche optimization.
Also extract take_pending_tailcall helper to deduplicate the pattern.

Assisted-by: Claude
Rename SendNonNull to PendingFrame and make it fully private: the
struct, its field, and the pending_tailcall_frame Cell are all
non-pub. External code accesses the side channel only through
set_pending_tailcall (pub(crate)) and take_pending_tailcall (private).

This ensures the unsafe Send+Sync impl cannot be reused elsewhere
without justifying a new safety argument.

Assisted-by: Claude
enter_iframe_unchecked was skipping C-stack checks under the
assumption that the trampoline stays in one Rust stack frame. But
each run_iframe call still consumes Rust stack, so deep Python
recursion through the trampoline can exhaust the C stack (observed
as STATUS_STACK_OVERFLOW on Windows CI).

Keep the C-stack check (every 8th call) while still skipping the
Python recursion depth check (already done by
specialization_call_recursion_guard).

Assisted-by: Claude
Copilot AI review requested due to automatic review settings August 3, 2026 07:07

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 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

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

- Fix double-free: mark entry frame with is_entry flag in
  SuspendedFrame so the trampoline skips its datastack release
  (the caller owns that cleanup)
- Clear iframe.previous in exit_iframe before unlinking the chain,
  matching with_frame and resume_gen_frame behavior
- Add scopeguard in with_iframe for panic safety
- Use saturating_sub(1) for lasti in trampoline_handle_exception
- Extract datastack_iframe_localsplus_offset helper to avoid
  duplicated alignment computation

Assisted-by: Claude
Copilot AI review requested due to automatic review settings August 3, 2026 07:53

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 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

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>>,

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.

Perf: Python→Python call overhead is ~15x CPython (eager frame lifecycle + recursive eval loop)

3 participants