Flatten Python eval loop with trampoline for reduced call overhead - #8431
Flatten Python eval loop with trampoline for reduced call overhead#8431youknowone wants to merge 10 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe 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. ChangesDatastack tail-call execution
Environment hook removal Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
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::TailCalland arun_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_iframeintoenter_iframe/exit_iframe(withIframeEntryState) 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
iframepassed intorun_frame_fast_trampoline, leaving the outer cleanup path with a dangling reference. Skip datastack release whencaller_iframe_ptris 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
iframepassed intorun_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
iframeforrun_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
iframepassed intorun_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.
| unsafe { | ||
| if let Some(base) = caller_iframe.release_datastack_frame() { | ||
| self.datastack_pop(base); | ||
| } | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
crates/vm/src/vm/mod.rs (3)
1524-1524: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
_caller_refsbecause 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_refsand keep the explicitdropcalls, which correctly document the release ordering relative toexit_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 winDocument which side owns releasing the initial
iframe.
run_frame_fastnever releases the datastack allocation foriframe. On theReturnandErrarms it only callsexit_iframe. The callers incrates/vm/src/builtins/function.rs(Lines 645-649 and 822-826) perform the release. On theTailCallarm, however,run_frame_fast_trampolinepushesiframeontoframe_stackand 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 winThe
pending_tailcall_refsside channel has no single access point and no invariant check. The field ispub(crate)and every producer and consumer opens its ownunsafe { &mut *...get() }block, so the "one prepare, then one drain" contract is stated nowhere and checked nowhere. The sibling fieldpending_tailcall_framealready uses private storage withset_pending_tailcallandtake_pending_tailcallaccessors.
crates/vm/src/vm/mod.rs#L113-L117: make the field private and addpush_pending_tailcall_refandtake_pending_tailcall_refsmethods that hold the singleunsafeblock, then use them at the four drain sites on Lines 1465, 1484, 1532, and 1587.crates/vm/src/frame.rs#L10665-L10671: callvm.push_pending_tailcall_ref(callable)and adddebug_assert!that the channel was empty on entry; apply the same change totailcall_prepare_bound_method_frameat 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 winExtract the shared callee-frame construction from the two tail-call helpers.
tailcall_prepare_frame(Lines 10630-10646) andtailcall_prepare_bound_method_frame(Lines 10692-10708) contain identical logic: theNEWLOCALScheck that buildsFrameLocals, and theInterpreterFrame::new_on_datastackcall 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
funcand 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 valueAssert that the pending-refs channel is empty before pushing.
The trampoline drains
pending_tailcall_refsinto theSuspendedFramethat 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
SuspendedFrameand are released later than intended. Add a debug assertion so the violation is visible in test builds. Apply the same assertion intailcall_prepare_bound_method_frameat 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
📒 Files selected for processing (6)
.claude/settings.jsoncrates/vm/src/builtins/function.rscrates/vm/src/coroutine.rscrates/vm/src/frame.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/thread.rs
💤 Files with no reviewable changes (1)
- .claude/settings.json
There was a problem hiding this comment.
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_ptris the rootiframepassed intorun_frame_fast_trampoline, that frame is still owned by the outer caller (e.g.PyFunction::invoke_with_locals), which will callrelease_datastack_frame()afterrun_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
iframepassed torun_frame_fast_trampoline, the outer Rust caller will later callrelease_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_popthe initialiframethat the Rust caller will clean up afterrun_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_pophere can free the rootiframeearly (before returning fromrun_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
iframeargument.
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_iframehere can free the root frame owned by the Rust caller, which is then accessed again afterrun_frame_fast()returns (UAF/double-pop). The trampoline should only pop frames that are fully owned by the trampoline (i.e., not the initialiframeargument).
unsafe {
if let Some(base) = caller_iframe.release_datastack_frame() {
self.datastack_pop(base);
}
}
| pub(crate) fn new_on_datastack<'a>( | ||
| code: &Py<PyCode>, | ||
| globals: &Py<PyDict>, | ||
| builtins: &PyObject, | ||
| func_obj: Option<&PyObject>, |
f6ff843 to
29e9b20
Compare
There was a problem hiding this comment.
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 intorun_frame_fast_trampoline, that frame is still owned by the outer caller (e.g.PyFunction::invoke), which also callsiframe.release_datastack_frame()afterrun_frame_fastreturns. 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_datastackreturns&'a mut Selfwith a lifetime parameter'athat is not tied to any input (includingvm). Because the reference is created from a raw pointer, this lets callers within the crate accidentally (or indirectly via inference) treat the returned&mut InterpreterFrameas 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 iflasti()is 0, producing a hugeidxand causing the traceback callsite lookup to be skipped (and potentially hiding trace context). Other codepaths in this file usesaturating_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::invokealways callsiframe.release_datastack_frame()aftervm.run_frame_fast(iframe). With the new trampoline,run_frame_fast_trampolinemay 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);
}
29e9b20 to
a1fa320
Compare
There was a problem hiding this comment.
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_exceptioncomputes the call-site index asexec.lasti() - 1, butlastiis advanced to the next instruction and may have skipped over one or more inlineCACHEentries. This can underflow whenlasti==0and can also attach traceback entries to aCACHEop 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_refsvia.drain(..).collect(), which allocates a freshVecon 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_refsis anUnsafeCell<Vec<PyObjectRef>>but is exposed aspub(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 (likepending_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 usescore::ptr::fn_addr_eqto avoid platform/linker edge cases (e.g. crates/vm/src/vm/vm_ops.rs:353). Usingfn_addr_eqhere would avoid relying on an allowed lint.
#[allow(unpredictable_function_pointer_comparisons)]
if getattro != PyBaseObject::getattro {
| let state = self.enter_iframe(iframe)?; | ||
| let result = f(iframe); | ||
| self.exit_iframe(state); | ||
| result |
There was a problem hiding this comment.
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
📒 Files selected for processing (5)
crates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/frame.rscrates/vm/src/types/slot.rscrates/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
| 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 | ||
| } { |
There was a problem hiding this comment.
🎯 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
fiRepository: 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 || trueRepository: 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:
- 1: https://doc.rust-lang.org/stable/std/ptr/fn.fn_addr_eq.html
- 2: https://doc.rust-lang.org/stable/std/primitive.fn.html
- 3: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/types/static.UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS.html
- 4:
fn_addr_eqdocs suggest incorrect reasoning rust-lang/rust#160202 - 5: Add lint against function pointer comparisons rust-lang/rust#118833
- 6: Add
ptr::fn_addr_eqto compare functions pointers. rust-lang/libs-team#323 - 7: https://doc.rust-lang.org/nightly/std/ptr/fn.fn_addr_eq.html
- 8: Tracking Issue for
ptr::fn_addr_eqrust-lang/rust#129322 - 9: Inconsistent behavior when comparing function pointers in release rust-lang/rust#54685
- 10: https://doc.rust-lang.org/std/ptr/fn.fn_addr_eq.html
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`.
a1fa320 to
48b4283
Compare
There was a problem hiding this comment.
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_datastackreturns&'a mut Selfwith an unconstrained lifetime parameter'a. Because'ais 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 theclippy::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 theclippy::namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Useclippy::...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 theclippy::namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Useclippy::...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 theclippy::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 theclippy::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; withoutclippy::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; withoutclippy::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 whenlasti == 0(panic in debug, wrap in release). Other traceback-attachment sites in this file usesaturating_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; withoutclippy::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; withoutclippy::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; withoutclippy::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 theclippy::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 theclippy::namespace it won't take effect under Clippy and may trigger an unknown-lint warning. Useclippy::...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 theclippy::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; withoutclippy::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; withoutclippy::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; withoutclippy::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; withoutclippy::it won't silence Clippy and may warn as unknown lint. Use the namespaced form.
#[allow(unpredictable_function_pointer_comparisons)]
48b4283 to
87e0b78
Compare
There was a problem hiding this comment.
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
iframepassed torun_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
iframepassed torun_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_iframeeven when it may be the rootiframestill owned by the outer caller ofrun_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
iframepassed torun_frame_fast_trampoline, this will cause the outer call site to access a droppedInterpreterFrameduring 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 rootiframethat the outer call site still expects to clean up afterrun_frame_fastreturns.
unsafe {
if let Some(base) = caller_iframe.release_datastack_frame() {
self.datastack_pop(base);
}
}
crates/vm/src/frame.rs:980
InterpreterFrame::new_on_datastackis a safe function but returns&'a mut Selfwhere'ais 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_refsispub(crate)and exposes anUnsafeCell<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 spreadunsafe { &mut *...get() }usages. Consider making this field private and providing small helper methods onVirtualMachine(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 iflasti()is 0, producing a huge index and potentially skipping traceback attachment. Even if current invariants makelasti()>0here, 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_codeused to be fallible (returningNoneon size arithmetic overflow). After this change it always returnsSome(...)anddatastack_iframe_total_bytesusesexpect(...)internally, so a craftedPyCodewith hugelocalspluskinds.len()/max_stackdepthcould 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,
))
| unsafe { | ||
| if let Some(base) = caller_iframe.release_datastack_frame() { | ||
| self.datastack_pop(base); | ||
| } | ||
| } |
There was a problem hiding this comment.
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_refsis documented as a thread-local TailCall side channel, but it ispub(crate)and exposesUnsafeCell<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_exceptioncomputesidxwithexec.lasti() as usize - 1, which will underflow iflastiis 0, producing a huge index and potentially skipping traceback attachment / causing unexpected behavior. Other traceback sites in this file usesaturating_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;
9d95398 to
6be566b
Compare
There was a problem hiding this comment.
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_codenow wrapsdatastack_iframe_total_bytes(...)inSome(...), butdatastack_iframe_total_bytesusesexpect(...)onchecked_add/checked_muland 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 returningNoneinstead 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_refsintoowned_refsvia.drain(..).collect().collect()allocates a newVeceach 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 freshVecper 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 {
There was a problem hiding this comment.
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 liftReplace raw function-address comparisons with explicit slot-origin metadata.
crate::types::fn_addrconverts 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 thenew_wrapperorigin is unknown.crates/vm/src/builtins/type.rs#L3070-L3071: preserve thecall_slot_newsafety rejection unless the slot origins are known to match.crates/vm/src/stdlib/_thread.rs#L1000-L1001: distinguishobject.__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
📒 Files selected for processing (11)
.claude/settings.jsoncrates/vm/src/builtins/frame.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/type.rscrates/vm/src/coroutine.rscrates/vm/src/frame.rscrates/vm/src/object/ext.rscrates/vm/src/protocol/callable.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/vm/mod.rscrates/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
There was a problem hiding this comment.
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 Selfreturns 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) ifrelease_datastack_frame+datastack_pophave 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_exceptioncomputes the traceback instruction index aslasti - 1, butlastiin 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 incorrecttb_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.
| /// 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")); |
There was a problem hiding this comment.
isn't this equals to .expect(...)?
There was a problem hiding this comment.
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
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
a210f1d to
ca7ebef
Compare
There was a problem hiding this comment.
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_datastackis a safe function that returns&'a mut Selfwith 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 beunsafe(caller must uphold the LIFO lifetime) and/or return a raw pointer / RAII guard type instead of an arbitrary-lifetime&mutreference.
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_trampolinemanually callenter_iframe/exit_iframewithout a guard. Ifrun_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. Previouslywith_iframeprovided panic-safety viascopeguard; 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_tailcalloverwritespending_tailcall_frameunconditionally. 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 adebug_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
ca7ebef to
a2e9a49
Compare
There was a problem hiding this comment.
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 callsenter_iframe()/exit_iframe()but doesn’t guardexit_iframe()against panics fromrun_iframe()(or theYield in non-generator framepanic). 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 Selfreturns a reference with an unconstrained lifetime parameter. This is unsound: callers can infer/choose'ato outlive the datastack allocation, creating a safe-looking&mut InterpreterFramethat can be used afterdatastack_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 aslasti - 1, butlastiis advanced past inline cache entries by the main run loop. For CALL_*_EXACT_ARGS this can point at anInstruction::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_refsispub(crate)even though its safety comment says it should be accessed only through the tailcall side-channel logic. Exposing anUnsafeCell<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>>,
Summary
Flatten the Python-to-Python call path so that
CallPyExactArgsandCallBoundMethodExactArgsno longer recurse through new Rust stack frames. Instead, the bytecode loop returns aTailCallsignal and a trampoline swaps frames in a single loop — matching CPython 3.12+'s approach.close youknowone#40
Changes
Phase 0: Factor
with_iframeenter_iframe/exit_iframefromwith_iframewithIframeEntryStatestructPhase 1: Datastack-allocated InterpreterFrame
InterpreterFrame::new_on_datastack()bump-allocates both the InterpreterFrame and its LocalsPlus in a single datastack pushrelease_datastack_frame()drops values and returns the base pointer fordatastack_popPhase 2–4: Trampoline loop + TailCall + exception propagation
ExecutionResult::TailCallvariantpending_tailcall_frameside channel onVirtualMachinerun_frame_fast_trampoline()withVec<SuspendedFrame>stacktailcall_prepare_frame()builds callee frame on datastack and stores pointertrampoline_handle_exception()— unwinds through suspended callers, attaches traceback entriesPhase 5: Optimizations
enter_iframe_uncheckedskips recursion/C-stack checks in trampoline (already verified byspecialization_call_recursion_guard)temporary_refsmutex to trampoline-localSuspendedFrame.owned_refsvia VM side channel — eliminates mutex lock + Vec allocation per callCallBoundMethodExactArgsmaterializedpointer once inexit_iframeSafety
PendingFramewrapper for the tailcall side channel is fully private — the struct, its innerNonNull, and theCellfield are all non-pub. External code accesses only throughset_pending_tailcall/take_pending_tailcall, preventing reuse of the unsafeSend+Syncimpl elsewhere.Performance
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, andtry/exceptacross call boundaries all verified.Refs: youknowone#40
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability