Actionable comments posted: 2

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 177-179: Update the stale lifecycle comment on
Frame::new_on_datastack (and the similar comment at the other location) to
reference the current API: instruct callers to call materialize_localsplus() to
migrate localsplus before invoking datastack_pop(), instead of mentioning the
old release_datastack(); keep the intent (drop values and return base pointer)
but replace the function name and order of operations to read something like
"The caller must call materialize_localsplus() when the frame finishes to
migrate localsplus before calling datastack_pop() to drop values and return the
base pointer."
- Around line 288-352: The stack helpers currently use debug_assert! which is
disabled in release and can lead to UB; make their bounds checks run in all
builds by replacing debug_assert! with runtime checks that panic on violation
(e.g., assert! or explicit if-check+panic) in stack_push, stack_pop,
stack_index, and stack_index_mut; keep stack_try_push as the non-panicking
alternative, but ensure stack_push checks idx < self.capacity(), stack_pop
checks self.stack_top > 0, and stack_index/stack_index_mut check idx <
self.stack_top as usize before performing the raw-pointer arithmetic so
out-of-bounds errors produce a controlled panic instead of undefined behavior.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between bc108df and ec12e3d.

• .cspell.dict/cpython.txt
• crates/vm/src/builtins/frame.rs
• crates/vm/src/builtins/function.rs
• crates/vm/src/builtins/super.rs
• crates/vm/src/datastack.rs
• crates/vm/src/frame.rs
• crates/vm/src/lib.rs
• crates/vm/src/vm/mod.rs
• crates/vm/src/vm/thread.rs

Actionable comments posted: 1

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

177-179: ⚠️ Potential issue | 🟡 Minor

Refresh datastack lifecycle comments to the current API.

These comments still instruct callers to use release_datastack(), but the migration path is now materialize_localsplus() before datastack_pop(). Please update both spots so the cleanup contract is accurate.
As per coding guidelines, "Do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code."

Also applies to: 470-472

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 177 - 179, Update the datastack
lifecycle comments in the docs for the frame constructor functions (e.g.,
new_on_datastack and the other commented spot near the Frame methods) to reflect
the current API: instruct callers to call materialize_localsplus() to
materialize localsplus and then call datastack_pop() (or the
DataStack::datastack_pop equivalent) instead of the outdated
release_datastack(); keep the rest of the comment intact but replace the
release_datastack() reference with the new sequence (materialize_localsplus()
before datastack_pop()) so the cleanup contract matches the migrated API.

---

290-351: ⚠️ Potential issue | 🔴 Critical

Harden LocalsPlus stack bounds checks for release builds.

stack_push, stack_pop, stack_index, and stack_index_mut rely on debug_assert! before unsafe/raw-pointer access. In release builds, invariant breaks can become out-of-bounds UB instead of controlled failure.

Suggested hardening

@@
     fn stack_push(&mut self, val: Option<PyStackRef>) {
         let idx = self.nlocalsplus as usize + self.stack_top as usize;
-        debug_assert!(
+        assert!(
             idx < self.capacity(),
             "stack overflow: stack_top={}, capacity={}",
             self.stack_top,
             self.stack_capacity()
         );
@@
     fn stack_pop(&mut self) -> Option<PyStackRef> {
-        debug_assert!(self.stack_top > 0, "stack underflow");
+        assert!(self.stack_top > 0, "stack underflow");
         self.stack_top -= 1;
@@
     fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-        debug_assert!(idx < self.stack_top as usize);
+        assert!(idx < self.stack_top as usize, "stack index out of bounds");
         let data = self.data_as_slice();
         let raw_idx = self.nlocalsplus as usize + idx;
         unsafe { &*(data.as_ptr().add(raw_idx) as *const Option<PyStackRef>) }
     }
@@
     fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-        debug_assert!(idx < self.stack_top as usize);
+        assert!(idx < self.stack_top as usize, "stack index out of bounds");
         let raw_idx = self.nlocalsplus as usize + idx;
         let data = self.data_as_mut_slice();
         unsafe { &mut *(data.as_mut_ptr().add(raw_idx) as *mut Option<PyStackRef>) }
     }

As per coding guidelines, "Follow Rust best practices for error handling and memory management."

#!/bin/bash
set -euo pipefail

echo "=== stack helper implementations ==="
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "=== debug_assert usage in stack helpers ==="
rg -n 'fn stack_(push|pop|index|index_mut)\(|debug_assert!\(' crates/vm/src/frame.rs -n -C2

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 290 - 351, Replace reliance on
debug_assert! with explicit runtime checks that panic on violation so release
builds don't hit UB: in stack_push (function stack_push) check idx <
self.capacity() and panic! with the same "stack overflow..." message instead of
debug_assert; in stack_pop (function stack_pop) check self.stack_top > 0 and
panic! with "stack underflow"; in stack_index and stack_index_mut (functions
stack_index and stack_index_mut) check idx < self.stack_top as usize and panic!
with the same invariant message before doing the raw pointer casts; keep
stack_try_push as-is (it already returns Err) and preserve existing messages.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 69-70: Update the lifecycle comment above FrameState to reference
the current field names: replace the stale "Frame.stack" mention with
"Frame.localsplus" (keep the other references to Frame.cells_frees and
Frame.prev_line unchanged) so the comment accurately describes that FrameState
fields are stored directly on Frame as UnsafeCell via Frame.localsplus,
Frame.cells_frees, and Frame.prev_line; ensure wording remains consistent with
safety reasoning and existing style.

---

Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 177-179: Update the datastack lifecycle comments in the docs for
the frame constructor functions (e.g., new_on_datastack and the other commented
spot near the Frame methods) to reflect the current API: instruct callers to
call materialize_localsplus() to materialize localsplus and then call
datastack_pop() (or the DataStack::datastack_pop equivalent) instead of the
outdated release_datastack(); keep the rest of the comment intact but replace
the release_datastack() reference with the new sequence
(materialize_localsplus() before datastack_pop()) so the cleanup contract
matches the migrated API.
- Around line 290-351: Replace reliance on debug_assert! with explicit runtime
checks that panic on violation so release builds don't hit UB: in stack_push
(function stack_push) check idx < self.capacity() and panic! with the same
"stack overflow..." message instead of debug_assert; in stack_pop (function
stack_pop) check self.stack_top > 0 and panic! with "stack underflow"; in
stack_index and stack_index_mut (functions stack_index and stack_index_mut)
check idx < self.stack_top as usize and panic! with the same invariant message
before doing the raw pointer casts; keep stack_try_push as-is (it already
returns Err) and preserve existing messages.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between ec12e3d and e94f2de.

• crates/vm/src/frame.rs

Actionable comments posted: 2

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

288-352: ⚠️ Potential issue | 🔴 Critical

Harden stack helper bounds checks for release builds.

stack_push, stack_pop, stack_index, and stack_index_mut gate unsafe pointer access with debug_assert!. In release, these checks disappear, so any invariant break can become UB instead of a controlled panic.

Proposed hardening

 fn stack_push(&mut self, val: Option<PyStackRef>) {
     let idx = self.nlocalsplus as usize + self.stack_top as usize;
-    debug_assert!(
+    assert!(
         idx < self.capacity(),
         "stack overflow: stack_top={}, capacity={}",
         self.stack_top,
         self.stack_capacity()
     );
@@
 fn stack_pop(&mut self) -> Option<PyStackRef> {
-    debug_assert!(self.stack_top > 0, "stack underflow");
+    assert!(self.stack_top > 0, "stack underflow");
     self.stack_top -= 1;
@@
 fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
     let data = self.data_as_slice();
     let raw_idx = self.nlocalsplus as usize + idx;
     unsafe { &*(data.as_ptr().add(raw_idx) as *const Option<PyStackRef>) }
 }
@@
 fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
     let raw_idx = self.nlocalsplus as usize + idx;
     let data = self.data_as_mut_slice();
     unsafe { &mut *(data.as_mut_ptr().add(raw_idx) as *mut Option<PyStackRef>) }
 }

#!/bin/bash
set -euo pipefail

echo "=== Stack helpers using debug-only checks ==="
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "=== Nearby subtraction-based stack indexing callsites ==="
rg -n 'stack_len\s*-\s*|stack_index\(' crates/vm/src/frame.rs | sed -n '1,80p'

Expected result: Line 288-352 still shows debug_assert! in helper methods guarding unsafe accesses, confirming release-mode check elision risk.

As per coding guidelines, "Follow Rust best practices for error handling and memory management".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 288 - 352, The helper methods
(stack_push, stack_try_push already checks, stack_pop, stack_index,
stack_index_mut) use debug_assert! which is compiled out in release causing
potential UB; change those debug-only guards into release-time checks (use
assert! or explicit if .. { panic!(...) }) before performing unsafe pointer
accesses and ensure stack_push mirrors stack_try_push’s capacity check (panic on
overflow) so all unsafe transmute/slice/from_raw_parts calls are protected in
release builds.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/vm/Cargo.toml`:
- Line 13: The default features list in crates/vm/Cargo.toml now enables
"threading" (the line default = ["compiler", "wasmbind", "gc", "host_env",
"stdio", "threading"]), which forces threading-related code and synchronization
overhead on all consumers; either remove "threading" from that default array so
it becomes opt-in, or add clear documentation and a commit message explaining
why threading must be default and the tradeoffs (including which modules like
stdlib.thread, core VM frame tracking, and Arc vs Rc changes are affected).
After changing the default, run cargo build/test with and without defaults to
confirm no accidental breakage and update READMEs or the crate-level
documentation to describe the threading feature and recommended usage for
single-threaded targets.

In `@crates/vm/src/datastack.rs`:
- Around line 77-79: The arithmetic for computing aligned sizes and growing
chunk_size in datastack's allocator (functions has_space(), push(), push_slow())
must be changed to checked arithmetic to avoid overflow before any pointer
.add() or subtraction: replace (size + ALIGN - 1) with a checked sequence using
size.checked_add(ALIGN - 1).and_then(|v| v.checked_mul(1)) or more simply
size.checked_add(ALIGN - 1).and_then(|v| v.checked_sub(v % ALIGN)) to compute
aligned_size (or use checked_add then checked_sub of remainder), bail/panic with
a clear message on None; in push() and has_space() use that checked aligned_size
and return/panic on overflow rather than letting wrap occur; in push_slow()
compute aligned_size and the total allocation request via checked_add for
MINIMUM_OVERHEAD and other offsets, and replace chunk_size *= 2 with chunk_size
= chunk_size.checked_mul(2).expect("overflow while growing chunk_size") so any
overflow panics before pointer arithmetic occurs. Ensure all uses of
aligned_size and chunk_size later in pointer.add()/offset math use the checked
results.

---

Outside diff comments:
In `@crates/vm/src/builtins/function.rs`:
- Around line 567-610: The frame's datastack can be leaked if
fill_locals_from_args returns Err because early returns skip the explicit
vm.datastack_pop; to fix, create a small RAII guard (e.g., DatastackUnwindGuard)
that captures the Frame and Vm reference after Frame::new and calls
vm.datastack_pop(base) from Drop when materialize_localsplus() yields
Some(base), and give it a dismiss() method to cancel the pop; instantiate this
guard immediately after creating the frame and before calling
fill_locals_from_args(&frame,...), call guard.dismiss() in each successful
branch where you already manage the datastack manually (the generator/coroutine
branches and after vm.run_frame in the (false,false) branch once you pop), and
leave the guard to automatically pop on error paths so no datastack-backed
storage is leaked (refer to functions/values: Frame::new, fill_locals_from_args,
frame.materialize_localsplus, vm.datastack_pop).

---

Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 288-352: The helper methods (stack_push, stack_try_push already
checks, stack_pop, stack_index, stack_index_mut) use debug_assert! which is
compiled out in release causing potential UB; change those debug-only guards
into release-time checks (use assert! or explicit if .. { panic!(...) }) before
performing unsafe pointer accesses and ensure stack_push mirrors
stack_try_push’s capacity check (panic on overflow) so all unsafe
transmute/slice/from_raw_parts calls are protected in release builds.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 56f1f9fe-c928-4835-94bd-a1f6f5d80c40

Reviewing files that changed from the base of the PR and between e94f2de and 42a2a37.

• .cspell.dict/cpython.txt
• crates/vm/Cargo.toml
• crates/vm/src/builtins/frame.rs
• crates/vm/src/builtins/function.rs
• crates/vm/src/builtins/super.rs
• crates/vm/src/datastack.rs
• crates/vm/src/frame.rs
• crates/vm/src/lib.rs
• crates/vm/src/vm/mod.rs
• crates/vm/src/vm/thread.rs

• .cspell.dict/cpython.txt
• crates/vm/src/builtins/super.rs
• crates/vm/src/vm/thread.rs
• crates/vm/src/lib.rs

Actionable comments posted: 2

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

290-352: ⚠️ Potential issue | 🔴 Critical

Harden stack helper bounds checks in release builds.

LocalsPlus::stack_push, stack_pop, stack_index, and stack_index_mut still rely on debug_assert! before unsafe pointer-based access. In release builds, invariant violations can become UB instead of a controlled failure.

Suggested hardening

 fn stack_push(&mut self, val: Option<PyStackRef>) {
     let idx = self.nlocalsplus as usize + self.stack_top as usize;
-    debug_assert!(
+    assert!(
         idx < self.capacity(),
         "stack overflow: stack_top={}, capacity={}",
         self.stack_top,
         self.stack_capacity()
     );
@@
 fn stack_pop(&mut self) -> Option<PyStackRef> {
-    debug_assert!(self.stack_top > 0, "stack underflow");
+    assert!(self.stack_top > 0, "stack underflow");
@@
 fn stack_index(&self, idx: usize) -> &Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");
@@
 fn stack_index_mut(&mut self, idx: usize) -> &mut Option<PyStackRef> {
-    debug_assert!(idx < self.stack_top as usize);
+    assert!(idx < self.stack_top as usize, "stack index out of bounds");

#!/bin/bash
set -euo pipefail

echo "Inspect stack helper implementations:"
nl -ba crates/vm/src/frame.rs | sed -n '288,352p'

echo
echo "Spot stack_len - N arithmetic call-sites that depend on these checks:"
rg -nP --type=rust 'stack_len\s*-\s*|stack_index\(' crates/vm/src/frame.rs | head -n 80

As per coding guidelines, "Follow Rust best practices for error handling and memory management."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 290 - 352, The stack helpers use
debug_assert! which is compiled out in release and can lead to UB; replace those
debug-only asserts with explicit runtime checks that panic with clear messages
before performing any unsafe pointer or transmute operations. Concretely: in
stack_push check idx < self.capacity() and panic on overflow (stack_push still
panics; stack_try_push remains as-is), in stack_pop check self.stack_top > 0 and
panic on underflow before decrementing/reading, and in stack_index and
stack_index_mut check idx < self.stack_top and that raw_idx < self.capacity()
(or otherwise ensure the computed pointer is valid) and panic with a descriptive
message before casting the slice pointer returned by
data_as_slice()/data_as_mut_slice(); perform these checks immediately prior to
any unsafe use so no unsafe transmute/pointer arithmetic runs when invariants
are violated.

crates/vm/src/gc_state.rs (1)

831-841: Consider adding a module-level documentation note about thread-local behavior in non-threading mode.

The thread-local GcState in non-threading mode is correctly implemented: static_cell! expands to thread_local! when the threading feature is disabled (confirmed at crates/common/src/static_cell.rs:113-125). This means each thread gets its own independent GcState, which is appropriate for satisfying !Sync constraints.

This behavior is already documented inline at line 139: "In non-threading mode, gc_state() is thread-local so Sync is not needed." However, adding a brief note to the module-level docs (lines 1-4) would help maintainers understand this design decision upfront. The reinit_after_fork() method is safely gated to threading-only mode, and callers do not make cross-thread assumptions about GC state visibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/gc_state.rs` around lines 831 - 841, Add a short module-level
documentation note at the top of the gc_state.rs module explaining that
gc_state() returns a true global When the `threading` feature is enabled but
expands to a thread-local `GcState` when `threading` is disabled (because
`rustpython_common::static_cell!` becomes `thread_local!`), and that this design
satisfies `!Sync` constraints; reference the `gc_state()` function, the
`GcState` type, the `static_cell!` macro, and mention that `reinit_after_fork()`
is gated to threading-only behavior so callers must not assume cross-thread
visibility of GC state.

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

195-207: Mark materialization result as #[must_use] to prevent lifecycle mistakes.

Ignoring the returned base pointer makes it easy to skip datastack_pop, which can leak datastack chunks.

Suggested hardening

+#[must_use = "call VirtualMachine::datastack_pop() with the returned base pointer"]
 fn materialize_to_heap(&mut self) -> Option<*mut u8> {
@@
+#[must_use = "call VirtualMachine::datastack_pop() with the returned base pointer"]
 pub(crate) unsafe fn materialize_localsplus(&self) -> Option<*mut u8> {

Also applies to: 736-738

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/frame.rs` around lines 195 - 207, Mark the pointer-returning
materialization function with #[must_use] so callers cannot ignore the returned
base pointer; specifically add #[must_use] to the fn materialize_to_heap(&mut
self) -> Option<*mut u8> declaration and likewise annotate any other methods
that return a raw pointer from LocalsPlusData::DataStack (the same pattern
flagged around the other occurrence) to ensure callers handle
datastack_pop/lifecycle cleanup.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/common/src/lock.rs`:
- Around line 31-33: Clarify and reinforce the unsafe Sync impl for LazyLock by
updating the doc comment near the LazyLock<T, F> declaration and the unsafe
impl<T, F> Sync for LazyLock<T, F> to state explicitly that this impl is only
safe when both the "std" and "threading" features are absent (i.e., truly
single-threaded embedded/WASM-only targets), and either add a compile-time
assertion or a marker trait check to prevent this impl being used when either
feature is enabled; mention the exact symbols LazyLock and the unsafe impl Sync
so reviewers can find and update the documentation and/or add the compile-time
guard.

In `@crates/vm/src/frame.rs`:
- Around line 165-188: The constructors for LocalsPlus (fn new and fn
new_on_datastack) perform unchecked arithmetic and casts: replace nlocalsplus +
stacksize with checked_add(...).expect(...) to detect overflow, replace capacity
* size_of::<usize>() with checked_mul(...).expect(...), and replace the usize ->
u32 casts for nlocalsplus with u32::try_from(...).expect(...) to avoid silent
truncation; update both new and new_on_datastack paths and include clear expect
messages mentioning the function/operation (e.g., "LocalsPlus::new: capacity
overflow") to aid debugging while preserving the existing behaviors on success.

---

Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 290-352: The stack helpers use debug_assert! which is compiled out
in release and can lead to UB; replace those debug-only asserts with explicit
runtime checks that panic with clear messages before performing any unsafe
pointer or transmute operations. Concretely: in stack_push check idx <
self.capacity() and panic on overflow (stack_push still panics; stack_try_push
remains as-is), in stack_pop check self.stack_top > 0 and panic on underflow
before decrementing/reading, and in stack_index and stack_index_mut check idx <
self.stack_top and that raw_idx < self.capacity() (or otherwise ensure the
computed pointer is valid) and panic with a descriptive message before casting
the slice pointer returned by data_as_slice()/data_as_mut_slice(); perform these
checks immediately prior to any unsafe use so no unsafe transmute/pointer
arithmetic runs when invariants are violated.

---

Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 195-207: Mark the pointer-returning materialization function with
#[must_use] so callers cannot ignore the returned base pointer; specifically add
#[must_use] to the fn materialize_to_heap(&mut self) -> Option<*mut u8>
declaration and likewise annotate any other methods that return a raw pointer
from LocalsPlusData::DataStack (the same pattern flagged around the other
occurrence) to ensure callers handle datastack_pop/lifecycle cleanup.

In `@crates/vm/src/gc_state.rs`:
- Around line 831-841: Add a short module-level documentation note at the top of
the gc_state.rs module explaining that gc_state() returns a true global When the
`threading` feature is enabled but expands to a thread-local `GcState` when
`threading` is disabled (because `rustpython_common::static_cell!` becomes
`thread_local!`), and that this design satisfies `!Sync` constraints; reference
the `gc_state()` function, the `GcState` type, the `static_cell!` macro, and
mention that `reinit_after_fork()` is gated to threading-only behavior so
callers must not assume cross-thread visibility of GC state.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4f9a0b14-3c1d-49d6-ad14-a98933e00e4a

Reviewing files that changed from the base of the PR and between 42a2a37 and 859b6f7.

• crates/common/src/lock.rs
• crates/vm/src/frame.rs
• crates/vm/src/gc_state.rs

Actionable comments posted: 4

crates/vm/src/datastack.rs (1)

77-90: ⚠️ Potential issue | 🔴 Critical

Use checked alignment math and avoid pre-check .add() UB in the fast path.

Line 77 and Line 88 still use unchecked (size + ALIGN - 1), and Line 90 performs self.top.add(aligned_size) in the condition. A large size can wrap aligned_size, and .add() must only be used with a proven in-bounds offset.

Suggested fix

 impl DataStack {
+    #[inline(always)]
+    fn align_size(size: usize) -> usize {
+        size.checked_add(ALIGN - 1)
+            .map(|v| v & !(ALIGN - 1))
+            .expect("DataStack allocation size overflow")
+    }
+
     /// Check if the current chunk has at least `size` bytes available.
     #[inline(always)]
     pub fn has_space(&self, size: usize) -> bool {
-        let aligned_size = (size + ALIGN - 1) & !(ALIGN - 1);
+        let aligned_size = Self::align_size(size);
         (self.limit as usize).saturating_sub(self.top as usize) >= aligned_size
     }
@@
     #[inline(always)]
     pub fn push(&mut self, size: usize) -> *mut u8 {
-        let aligned_size = (size + ALIGN - 1) & !(ALIGN - 1);
+        let aligned_size = Self::align_size(size);
         unsafe {
-            if self.top.add(aligned_size) <= self.limit {
+            let available = (self.limit as usize).saturating_sub(self.top as usize);
+            if available >= aligned_size {
                 let ptr = self.top;
                 self.top = self.top.add(aligned_size);
                 ptr
             } else {
                 self.push_slow(aligned_size)
             }
         }
     }

As per coding guidelines: "Follow Rust best practices for error handling and memory management".

#!/bin/bash
set -euo pipefail

# Verify unchecked alignment arithmetic and pointer add in fast path.
rg -nP '\(size \+ ALIGN - 1\)|\.add\(aligned_size\)' crates/vm/src/datastack.rs

# Show the exact fast-path blocks for manual inspection.
sed -n '72,98p' crates/vm/src/datastack.rs

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/datastack.rs` around lines 77 - 90, The push method currently
uses unchecked alignment math `(size + ALIGN - 1)` and calls
`self.top.add(aligned_size)` in the conditional, which can overflow/wrap for
large `size` and invokes .add() without proven in-bounds offsets; fix push in
datastack.rs by (1) computing aligned_size with checked arithmetic (use
checked_add/checked_sub/checked_mul or a helper that returns None on overflow)
and return a null or handle allocation failure on overflow, and (2) replace the
conditional that uses `self.top.add(aligned_size) <= self.limit` with a
bounds-safe check (compute a checked pointer offset or compare sizes using
`(self.limit as usize).saturating_sub(self.top as usize) >= aligned_size` or use
`offset`/`wrapping_offset` only after verifying aligned_size fits) so .add() is
only called when the offset is proven in-bounds; update push to return early on
overflow and ensure pop still receives a valid pointer.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/common/src/lock.rs`:
- Around line 27-30: The build breaks when the "threading" feature is enabled
without "std" because lock.rs unconditionally re-exports std::sync::OnceLock and
std::sync::LazyLock; either make the feature graph include std by changing the
Cargo.toml entry threading = ["parking_lot"] to threading = ["parking_lot",
"std"], or add a compile-time guard in lock.rs that detects cfg(all(feature =
"threading", not(feature = "std"))) and emits compile_error! with a clear
message; locate the uses of OnceLock and LazyLock in lock.rs and apply one of
these fixes so the crate cannot enable threading without std.

In `@crates/vm/src/frame.rs`:
- Line 675: In Frame::new replace the unchecked addition that computes
nlocalsplus with checked arithmetic: compute nlocalsplus by using .checked_add
(e.g., first nlocals.checked_add(num_cells) and then .checked_add(nfrees)) and
handle the None case by returning an appropriate error (or propagating a Result)
instead of allowing wrapping; reference the variables nlocals, num_cells, nfrees
and the function Frame::new so you locate the calculation and ensure subsequent
uses of nlocalsplus assume a valid non-wrapped value.

In `@crates/vm/src/gc_state.rs`:
- Around line 831-840: The gc_state() helper currently uses
rustpython_common::static_cell! which becomes thread-local when the "threading"
feature is disabled, causing per-thread GcState sets and breaking global
allocation tracking; change gc_state() to initialize a single process-wide
static (e.g., a OnceLock or equivalent global static) so it always returns the
same &'static GcState across threads regardless of the "threading" feature,
using GcState::new for initialization; update the static declaration in
gc_state() (replace static_cell! usage) so allocation/GC logic (calls to
gc_state() from object/core.rs) see one shared GcState.
- Around line 137-142: The unconditional unsafe impl Send for GcState is unsound
in non-threading builds; change the declaration so Send is only implemented when
the threading feature is enabled (mirror the existing #[cfg(feature =
"threading")] used for Sync). Update the unsafe impl Send for GcState to be
gated with #[cfg(feature = "threading")] so that both Send and Sync are only
provided in threading builds, keeping the safety comment and behavior of GcState
unchanged.

---

Duplicate comments:
In `@crates/vm/src/datastack.rs`:
- Around line 77-90: The push method currently uses unchecked alignment math
`(size + ALIGN - 1)` and calls `self.top.add(aligned_size)` in the conditional,
which can overflow/wrap for large `size` and invokes .add() without proven
in-bounds offsets; fix push in datastack.rs by (1) computing aligned_size with
checked arithmetic (use checked_add/checked_sub/checked_mul or a helper that
returns None on overflow) and return a null or handle allocation failure on
overflow, and (2) replace the conditional that uses `self.top.add(aligned_size)
<= self.limit` with a bounds-safe check (compute a checked pointer offset or
compare sizes using `(self.limit as usize).saturating_sub(self.top as usize) >=
aligned_size` or use `offset`/`wrapping_offset` only after verifying
aligned_size fits) so .add() is only called when the offset is proven in-bounds;
update push to return early on overflow and ensure pop still receives a valid
pointer.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d2e83ddf-e6e7-4b27-b9dd-ea4f192513a6

Reviewing files that changed from the base of the PR and between 859b6f7 and 09a5a5e.

• .cspell.dict/cpython.txt
• crates/common/src/lock.rs
• crates/vm/src/builtins/frame.rs
• crates/vm/src/builtins/function.rs
• crates/vm/src/builtins/super.rs
• crates/vm/src/datastack.rs
• crates/vm/src/frame.rs
• crates/vm/src/gc_state.rs
• crates/vm/src/lib.rs
• crates/vm/src/vm/mod.rs
• crates/vm/src/vm/thread.rs

• crates/vm/src/vm/thread.rs
• .cspell.dict/cpython.txt
• crates/vm/src/builtins/frame.rs
• crates/vm/src/lib.rs

Addressing review comments