GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Flatten Python eval loop with trampoline for reduced call overhead by youknowone · Pull Request #8431 · RustPython/RustPython Conversation
|
| 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).
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
Performance
Reliability
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This PR 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:
ExecutionResult::TailCall and a run_frame_fast_trampoline() loop that iteratively enters/swaps frames and propagates exceptions without mutual recursion.InterpreterFrame + LocalsPlus (new_on_datastack / release_datastack_frame) to reduce allocation overhead and improve locality.with_iframe into enter_iframe / exit_iframe (with IframeEntryState) and adds VM “side channels” for pending tailcall frames and owned refs.Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
| 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. |
crates/vm/src/vm/mod.rs:1561
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
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
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
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
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| unsafe { | ||
| if let Some(base) = caller_iframe.release_datastack_frame() { | ||
| self.datastack_pop(base); | ||
| } | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed? If so, can the fix be applied to all the mentioned locations?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline crates/vm/src/vm/mod.rs --items all --type function --match 'run_frame_fast|run_frame_fast_trampoline' ast-grep outline crates/vm/src/builtins/function.rs --items all --type function --match 'invoke|exact' printf '\n--- trampoline cleanup sites ---\n' rg -n -C 6 'release_datastack_frame|is_entry|run_frame_fast_trampoline|run_frame_fast\s*\(' crates/vm/src/vm/mod.rs printf '\n--- PyFunction frame ownership sites ---\n' rg -n -C 8 'run_frame_fast\s*\(|release_datastack_frame' crates/vm/src/builtins/function.rs
Length of output: 644
@fanninpm, verification is inconclusive because repository access failed during clone.
The supplied file content appears to contain an is_entry guard that prevents the trampoline from releasing the entry frame. However, I cannot confirm that this guard exists in the revision for the review comment.
Please retry the repository access. Then I can verify all listed cleanup locations and provide a complete fix plan if the issue is present.
You are interacting with an AI system.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 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.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 2354-2363: Update the traceback index calculation in the
exception-handling flow to use a saturating subtraction when deriving idx from
exec.lasti(), matching the defensive behavior in gen_throw and unwind_blocks.
Preserve the existing call-site lookup and traceback construction.
- Around line 2292-2308: Extract the InterpreterFrame padding/alignment
calculation from datastack_iframe_total_bytes into a shared helper, then use
that helper for localsplus_offset_aligned in new_on_datastack as well. Ensure
both allocation sizing and the localsplus write offset derive from the same
alignment logic.
In `@crates/vm/src/vm/mod.rs`:
- Around line 1463-1472: The trampoline must distinguish the caller-owned entry
frame from frames it allocates. In crates/vm/src/vm/mod.rs lines 1463-1472, mark
the initial SuspendedFrame as the entry frame; in lines 1543-1551, the Err arm
at line 1558, and Action::Unwind release sites at lines 1602 and 1629, skip
release_datastack_frame and datastack_pop for that frame. Document on
run_frame_fast at lines 1419-1437 that the caller owns entry-frame datastack
release on every arm, including TailCall.
- Around line 1475-1650: Extract a shared helper for the duplicated `match
result` dispatch logic in the surrounding VM execution loop, accepting the
`PyResult<ExecutionResult>` and frame context needed to perform tail-call
suspension, return cleanup, and exception unwinding. Replace all three `match
result` blocks in `Action::EnterCallee`, `Action::ReturnValue`, and handled
`Action::Unwind` with calls to this helper, preserving the existing ownership
and unsafe teardown behavior at every release site.
- Around line 2218-2222: Update exit_iframe to clear the iframe’s previous field
before unlinking the frame chain, matching the teardown behavior in with_frame
and resume_gen_frame. Perform this store before set_current_frame(old_chain) so
heap-backed iframes cannot retain a dangling caller pointer.
---
Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 10630-10646: Extract the duplicated NEWLOCALS-based FrameLocals
setup and InterpreterFrame::new_on_datastack call from tailcall_prepare_frame
and tailcall_prepare_bound_method_frame into a shared
Self::tailcall_new_callee_frame(func, vm) helper returning the callee frame.
Replace both existing construction blocks with calls to this helper, leaving
each method’s distinct argument-placement offset and reference-transfer logic
unchanged.
- Around line 10665-10671: In the tail-call preparation flow, assert that
vm.pending_tailcall_refs is empty immediately before pushing the callable in the
visible prepare logic. Add the same debug assertion to
tailcall_prepare_bound_method_frame before its corresponding push, preserving
the existing ownership-transfer behavior.
In `@crates/vm/src/vm/mod.rs`:
- Line 1524: Rename the destructured owned_refs binding from _caller_refs to
caller_refs in the relevant VM code paths, including both destructuring sites,
and update every explicit drop(_caller_refs) call to drop(caller_refs). Preserve
all drop calls and their existing ordering relative to exit_iframe.
- Around line 1419-1437: Document the release-ownership contract in
run_frame_fast: callers own releasing the initial iframe for the Return and Err
paths, while the tail-call path transfers ownership to
run_frame_fast_trampoline. Add this as a concise doc line without changing the
existing control flow.
- Around line 113-117: Encapsulate pending tail-call reference access in VM
methods: make pending_tailcall_refs private, add push_pending_tailcall_ref and
take_pending_tailcall_refs with the sole UnsafeCell access, and replace the four
drain-site direct accesses in crates/vm/src/vm/mod.rs (lines 1465, 1484, 1532,
and 1587). In crates/vm/src/frame.rs lines 10665-10671, update
tailcall_prepare_frame to call vm.push_pending_tailcall_ref(callable) and assert
the channel is empty on entry; apply the same change to
tailcall_prepare_bound_method_frame at line 10723.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: d520647d-6506-4647-ac87-2bba33de3d53
.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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
crates/vm/src/vm/mod.rs:1550
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
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
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
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
iframe argument. unsafe {
if let Some(base) = caller_iframe.release_datastack_frame() {
self.datastack_pop(base);
}
}
crates/vm/src/vm/mod.rs:1561
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);
}
}
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| pub(crate) fn new_on_datastack<'a>( | ||
| code: &Py<PyCode>, | ||
| globals: &Py<PyDict>, | ||
| builtins: &PyObject, | ||
| func_obj: Option<&PyObject>, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline crates/vm/src/frame.rs --match 'new_on_datastack|release_datastack_frame|datastack_pop' --view expanded ast-grep outline crates/vm/src/vm/mod.rs --match 'datastack_push|datastack_pop|run_frame_fast' --view expanded printf '\n=== new_on_datastack uses ===\n' rg -n -C 5 '\bnew_on_datastack\s*\(' crates/vm --glob '*.rs' printf '\n=== datastack_pop definition and uses ===\n' rg -n -C 6 '\bdatastack_pop\s*\(' crates/vm --glob '*.rs' printf '\n=== direct lifetime selections ===\n' rg -n -C 3 'new_on_datastack::<|InterpreterFrame::new_on_datastack' crates --glob '*.rs'
Length of output: 28023
@fanninpm, confirmed. The finding is valid.
InterpreterFrame::new_on_datastack at Line 974 returns &'a mut Self, but no input uses 'a. A caller can therefore select 'static for the returned reference.
This is unsafe even without datastack_pop. The frame stores borrowed raw pointers to code, globals, builtins, and func_obj. Safe caller code can retain the returned frame after those input owners are dropped. Any later access through the frame can then dereference invalid pointers.
The current call sites release the frame correctly. They do not make the safe API sound.
The fix should either:
code: &'a Py<PyCode>, globals: &'a Py<PyDict>, builtins: &'a PyObject, and vm: &'a VirtualMachine; ornew_on_datastack as unsafe and document the lifetime, input-liveness, and LIFO release requirements.The first option gives the compiler the required lifetime checks.
You are interacting with an AI system.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
f6ff843 to
29e9b20
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
crates/vm/src/vm/mod.rs:1550
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);
}
Sorry, something went wrong.
There was an error while loading. Please reload this page.
29e9b20 to
a1fa320
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
crates/vm/src/frame.rs:2357
trampoline_handle_exception computes the call-site index as exec.lasti() - 1, but lasti is advanced to the next instruction and may have skipped over one or more inline CACHE entries. This can underflow when lasti==0 and can also attach traceback entries to a CACHE op rather than the actual CALL instruction. // lasti points past the CallPyExactArgs instruction (+ cache entries).
// The exception occurred at the previous instruction (the call site).
let idx = exec.lasti() as usize - 1;
crates/vm/src/vm/mod.rs:1467
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
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 {
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let state = self.enter_iframe(iframe)?; | ||
| let result = f(iframe); | ||
| self.exit_iframe(state); | ||
| result |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed? If so, can the suggested changeset still be applied?
Commented from CodeRabbit Change Stack
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- checked-out revision ---' git rev-parse HEAD git status --short printf '%s\n' '--- with_iframe at checked-out revision ---' ast-grep outline crates/vm/src/vm/mod.rs --match with_iframe --view expanded rg -n -A18 -B4 'pub fn with_iframe' crates/vm/src/vm/mod.rs printf '%s\n' '--- with_iframe at Change Stack snapshot ---' git show 48b4283a1f8453d435fda49a8fe8e1459671d3a1:crates/vm/src/vm/mod.rs \ | rg -n -A18 -B4 'pub fn with_iframe' || true
Length of output: 2617
@fanninpm, confirmed.
At snapshot commit 48b4283a1f8453d435fda49a8fe8e1459671d3a1, VirtualMachine::with_iframe was not panic-safe. A panic from f(iframe) bypassed self.exit_iframe(state).
The checked-out PR revision a2e9a49e70380a79172b2f63c02ecde096f87f28 already applies the required changeset. Lines 2301-2306 use scopeguard to call exit_iframe during unwinding, then disarm the guard before the normal cleanup call. This provides cleanup on both paths and prevents double cleanup.
No further change is required for this finding.
You are interacting with an AI system.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 2830-2834: Remove the function-pointer equality check from the
constructor fast path around `slot_new` and `is_wrapper`. Track whether the
native slot originated from `crate::types::new_wrapper` separately, or disable
the fast path when that origin is unknown, so wrapper constructors always reach
the later `__init__` call; do not replace the comparison with `fn_addr_eq`.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 19a4f9bd-bc83-4d98-b536-dc08e8a132bd
crates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/frame.rscrates/vm/src/types/slot.rscrates/vm/src/vm/method.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🏁 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:
fn_addr_eq docs suggest incorrect reasoning rust-lang/rust#160202ptr::fn_addr_eq to compare functions pointers. rust-lang/libs-team#323ptr::fn_addr_eq rust-lang/rust#129322Do 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.
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`.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
a1fa320 to
48b4283
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
crates/vm/src/frame.rs:984
InterpreterFrame::new_on_datastack returns &'a mut Self with an unconstrained lifetime parameter 'a. Because 'a is not tied to any input, callers can effectively choose any lifetime (including 'static), which makes this safe function unsound and can lead to use-after-free of datastack memory. func_obj: Option<&PyObject>,
locals: FrameLocals,
closure: &[PyCellRef],
vm: &VirtualMachine,
) -> &'a mut Self {
crates/vm/src/vm/vm_ops.rs:287
#[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
#[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
#[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
#[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
#[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
#[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
#[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
#[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
#[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
#[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
#[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
#[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:
#[allow(unpredictable_function_pointer_comparisons)]
crates/vm/src/frame.rs:9152
#[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:
#[allow(unpredictable_function_pointer_comparisons)]
crates/vm/src/class.rs:26
#[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
#[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
#[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
#[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)]
Sorry, something went wrong.
There was an error while loading. Please reload this page.
48b4283 to
87e0b78
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
crates/vm/src/vm/mod.rs:1606
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
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
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
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
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,
))
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| unsafe { | ||
| if let Some(base) = caller_iframe.release_datastack_frame() { | ||
| self.datastack_pop(base); | ||
| } | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai can this be confirmed? If so, can the fix be applied to all the mentioned locations?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- current diff: vm/mod.rs ---' git diff -- crates/vm/src/vm/mod.rs printf '%s\n' '--- trampoline outline ---' ast-grep outline crates/vm/src/vm/mod.rs --items all --type function --match 'run_frame_fast|run_frame_fast_trampoline|enter_iframe|exit_iframe' printf '%s\n' '--- trampoline source ---' rg -n -A260 -B30 'fn run_frame_fast_trampoline\b|pub fn run_frame_fast\b' crates/vm/src/vm/mod.rs printf '%s\n' '--- run_frame_fast callers and datastack releases ---' rg -n -C5 '\brun_frame_fast\s*\(|\brelease_datastack_frame\s*\(' crates/vm/src/builtins/function.rs crates/vm/src/frame.rs crates/vm/src/vm/mod.rs
Length of output: 32735
@fanninpm, confirmed. The original finding was valid for the earlier diff.
The current branch already applies the required fix. run_frame_fast_trampoline marks the initial caller as is_entry: true at Line 1477. Each caller return and exception path releases the datastack frame only when !caller_is_entry.
This preserves ownership of the entry frame for PyFunction::invoke and invoke_prepared_exact_args, which release it after run_frame_fast returns. The trampoline still releases only callee frames that it allocated.
The fix covers the locations previously listed. No additional code change is required.
You are interacting with an AI system.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
crates/vm/src/vm/mod.rs:117
pending_tailcall_refs is documented as a thread-local TailCall side channel, but it is pub(crate) and exposes UnsafeCell<Vec<PyObjectRef>> to the entire crate. This makes it easy for other code paths (including future changes) to mutate it without the intended single-thread / set+drain protocol, weakening the safety invariant around raw frame pointers. /// Owned references that keep callee raw pointers valid during TailCall.
/// Set by `tailcall_prepare_frame`, drained by the trampoline into
/// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
/// per-thread and this field is only accessed on the owning thread.
pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,
crates/vm/src/frame.rs:2356
trampoline_handle_exception computes idx with exec.lasti() as usize - 1, which will underflow if lasti is 0, producing a huge index and potentially skipping traceback attachment / causing unexpected behavior. Other traceback sites in this file use saturating_sub(1) to avoid this. // lasti points past the CallPyExactArgs instruction (+ cache entries).
// The exception occurred at the previous instruction (the call site).
let idx = exec.lasti() as usize - 1;
Sorry, something went wrong.
There was an error while loading. Please reload this page.
9d95398 to
6be566b
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
crates/vm/src/frame.rs:977
InterpreterFrame::new_on_datastack<'a>(...) -> &'a mut Self returns a mutable reference with a lifetime parameter that is not tied to any input. This makes it possible (within the crate) to extend the returned reference beyond the datastack allocation’s validity in otherwise-safe code, which is unsound (use-after-free / UB) if release_datastack_frame+datastack_pop have occurred. #[allow(clippy::too_many_arguments)]
pub(crate) fn new_on_datastack<'a>(
code: &Py<PyCode>,
globals: &Py<PyDict>,
builtins: &PyObject,
crates/vm/src/frame.rs:2362
trampoline_handle_exception computes the traceback instruction index as lasti - 1, but lasti in this VM points past the instruction and its inline CACHE entries (the run loop skips caches). That means this may attach the traceback entry to a CACHE slot rather than the actual call-site opcode, producing an incorrect tb_lasti / location (unlike the normal exception path, which uses the real instruction index). // lasti points past the CallPyExactArgs instruction (+ cache entries).
// The exception occurred at the previous instruction (the call site).
let idx = (exec.lasti() as usize).saturating_sub(1);
// Add traceback entry at the call site.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /// 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.
The reason will be displayed to describe this comment to others. Learn more.
isn't this equals to .expect(...)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
omg claude lied in comment. fixed it
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I felt like there should be a Clippy lint for this, but there isn't. See rust-lang/rust-clippy#11271
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
crates/vm/src/frame.rs:983
InterpreterFrame::new_on_datastack is a safe function that returns &'a mut Self with an unconstrained generic lifetime 'a. This lets callers create a reference that (in safe Rust) can outlive the datastack allocation, which is unsound and can lead to use-after-free/UB. This API should be unsafe (caller must uphold the LIFO lifetime) and/or return a raw pointer / RAII guard type instead of an arbitrary-lifetime &mut reference. pub(crate) fn new_on_datastack<'a>(
code: &Py<PyCode>,
globals: &Py<PyDict>,
builtins: &PyObject,
func_obj: Option<&PyObject>,
locals: FrameLocals,
closure: &[PyCellRef],
vm: &VirtualMachine,
) -> &'a mut Self {
let nlocalsplus = code.localspluskinds.len();
crates/vm/src/vm/mod.rs:1439
run_frame_fast/run_frame_fast_trampoline manually call enter_iframe/exit_iframe without a guard. If run_iframe (or any code in the trampoline) panics, the frame chain/TLS (CURRENT_FRAME), recursion depth, and exception state may not be restored, and datastack-allocated callee frames may not be released. Previously with_iframe provided panic-safety via scopeguard; the trampoline path should provide equivalent unwind cleanup (e.g., scopeguard for the entry frame plus a Drop-based cleanup that exits/unlinks any entered frames and pops datastack allocations). pub fn run_frame_fast(&self, iframe: &mut crate::frame::InterpreterFrame) -> PyResult {
use crate::frame::ExecutionResult;
let entry_state = self.enter_iframe(iframe)?;
let result = crate::frame::run_iframe(iframe, self);
match result {
Ok(ExecutionResult::Return(value)) => {
self.exit_iframe(entry_state);
Ok(value)
}
Ok(ExecutionResult::TailCall) => self.run_frame_fast_trampoline(iframe, entry_state),
Ok(ExecutionResult::Yield(_)) => panic!("Yield in non-generator frame"),
Err(exc) => {
self.exit_iframe(entry_state);
Err(exc)
crates/vm/src/vm/mod.rs:1407
set_pending_tailcall overwrites pending_tailcall_frame unconditionally. If a logic bug ever causes a second TailCall to be prepared before the trampoline consumes the first, this would silently drop the earlier pending frame pointer (and its associated refs), making debugging much harder and potentially leaking datastack allocations. Adding a debug_assert! here helps catch invariant violations early. pub(crate) fn set_pending_tailcall(&self, iframe: &mut crate::frame::InterpreterFrame) {
self.pending_tailcall_frame
.set(Some(PendingFrame(core::ptr::NonNull::from(iframe))));
}
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- 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.
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
crates/vm/src/vm/mod.rs:1431
run_frame_fast() manually calls enter_iframe() / exit_iframe() but doesn’t guard exit_iframe() against panics from run_iframe() (or the Yield in non-generator frame panic). That can leave TLS frame chain + recursion_depth inconsistent after an unwind. let entry_state = self.enter_iframe(iframe)?;
let result = crate::frame::run_iframe(iframe, self);
match result {
Ok(ExecutionResult::Return(value)) => {
crates/vm/src/frame.rs:978
InterpreterFrame::new_on_datastack<'a>(...) -> &'a mut Self returns a reference with an unconstrained lifetime parameter. This is unsound: callers can infer/choose 'a to outlive the datastack allocation, creating a safe-looking &mut InterpreterFrame that can be used after datastack_pop() (UB). pub(crate) fn new_on_datastack<'a>(
code: &Py<PyCode>,
globals: &Py<PyDict>,
builtins: &PyObject,
func_obj: Option<&PyObject>,
crates/vm/src/frame.rs:2369
trampoline_handle_exception() computes the call-site index as lasti - 1, but lasti is advanced past inline cache entries by the main run loop. For CALL_*_EXACT_ARGS this can point at an Instruction::Cache, producing incorrect traceback locations/offsets. The run loop already handles this case elsewhere by scanning backwards past CACHE entries (e.g. InstrumentedNotTaken). // lasti points past the CallPyExactArgs instruction (+ cache entries).
// The exception occurred at the previous instruction (the call site).
let idx = (exec.lasti() as usize).saturating_sub(1);
crates/vm/src/vm/mod.rs:117
pending_tailcall_refs is pub(crate) even though its safety comment says it should be accessed only through the tailcall side-channel logic. Exposing an UnsafeCell<Vec<PyObjectRef>> across the crate makes it easy to accidentally introduce aliased mutable access or cross-thread access later, undermining the safety invariant. /// Owned references that keep callee raw pointers valid during TailCall.
/// Set by `tailcall_prepare_frame`, drained by the trampoline into
/// its local `owned_refs` Vec. Uses UnsafeCell because the VM is
/// per-thread and this field is only accessed on the owning thread.
pub(crate) pending_tailcall_refs: core::cell::UnsafeCell<Vec<PyObjectRef>>,
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…8431) * Factor with_iframe into enter_iframe/exit_iframe helpers 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 * Allocate InterpreterFrame and LocalsPlus together on the datastack 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 * Implement trampoline loop for Python-to-Python calls 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 * Optimize trampoline: skip recursion check, avoid temporary_refs mutex, 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 * Use UnsafeCell for pending_tailcall_refs The VM is per-thread so RefCell's runtime borrow checking is unnecessary overhead. Replace with UnsafeCell for direct access. Assisted-by: Claude * Remove .claude/settings.json from tracking Assisted-by: Claude * Replace SendPtr with Option<SendNonNull> for type-safe pending tailcall 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 * Restrict PendingFrame to private, expose only set/take methods 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 * Restore C-stack overflow check in trampoline enter_iframe_unchecked 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 * Address code review: entry frame double-free, panic safety, cleanup - 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
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Perf: Python→Python call overhead is ~15x CPython (eager frame lifecycle + recursive eval loop)
There was an error while loading. Please reload this page.