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

543-557: Reraise classification looks right; please run required build/lint steps.

Treating EndAsyncFor/CleanupThrow as reraise preserves traceback semantics. Since bytecode-instruction handling changed here, please run the clean build plus rustfmt/clippy steps before merge:

rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r
cargo fmt
cargo clippy

As per coding guidelines: “When modifying bytecode instructions, perform a clean build by running: rm -r target/debug/build/rustpython-* && find . | grep -E ".pyc$" | xargs rm -r”, “Follow the default rustfmt code style by running cargo fmt to format Rust code”, and “Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes.”

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

1088-1142: resume_gen_frame intentionally skips thread frame updates — verify cross-thread visibility.

This lightweight path skips push_thread_frame/pop_thread_frame and update_thread_exception, meaning generator/coroutine frames won't appear in sys._current_frames() or sys._current_exceptions() from other threads. These frames do appear in vm.frames (so sys._getframe() works correctly from the owning thread).

If this omission is intentional for performance, consider documenting the trade-off in the doc comment. If generators should be visible in _current_frames(), the thread frame push/pop needs to be added back.

Also, a minor note: Line 1113 pushes directly to self.exceptions.borrow_mut().stack rather than calling self.push_exception(exc), which also means the thread exception slot doesn't update. When the generator yields or returns, set_exception is typically called to save/restore the exception state, so this is likely fine — but it means a concurrent sys._current_exceptions() call won't see the generator's in-flight exception.

---

1121-1128: Misleading panic message in scopeguard cleanup.

Line 1125 panics with "pop_exception() without nested exc stack", but this is a direct Vec::pop(), not a call to pop_exception(). Consider updating the message for clarity.

✏️ Suggested wording

             self.exceptions.borrow_mut().stack
                 .pop()
-                .expect("pop_exception() without nested exc stack");
+                .expect("resume_gen_frame: exception stack underflow");

---

1454-1467: set_exception pushes when the stack is empty — is this intentional?

When excs.stack is empty (no frame has been entered), set_exception pushes a new entry rather than panicking. This is a defensive fallback that silently papers over a potential caller bug. If it's intentional (e.g., for generator throw before first next), a brief comment would clarify the rationale.

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

1097-1129: Panic-safety gap between state mutations and scopeguard setup.

Lines 1101–1117 mutate recursion_depth, frames, exceptions, and frame.owner before the scopeguard::defer! on line 1121. If any operation in that window panics (e.g., a borrow_mut on an already-borrowed RefCell), the state won't be rolled back.

In practice these are simple operations unlikely to panic, but you could tighten this by incrementing recursion_depth inside the defer setup or using an early-established guard pattern. Low priority given the hot-path nature of this code.

---

1454-1467: Defensive fallback in set_exception when stack is empty.

Lines 1461–1463 push a new entry when the stack is empty. This avoids a panic, but it silently grows the exception stack without a corresponding frame push. A later pop_exception may then pop an entry that wasn't pushed by push_exception/with_frame_exc, potentially causing misalignment.

Consider adding a debug_assert!(!excs.stack.is_empty()) to catch unexpected empty-stack calls during development, while keeping the push fallback for release builds.

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

4-25: Consider encapsulating ThreadSlot fields to protect invariants (esp. exception swap/drop rules).

ThreadSlot is pub and both frames and exception are pub, but exception usage has a strict “owning-thread” drop contract (to avoid dropping a Py*Ref outside a VM context). Even though misuse would require unsafe for swap, narrowing the surface (e.g., pub(crate) fields + small accessor methods) would make it harder to accidentally violate the intended model.

Also: please ensure this lands cargo fmt/cargo clippy cleanly since it touches core VM threading primitives.

As per coding guidelines: “Follow the default rustfmt code style by running cargo fmt to format Rust code” and “Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes”.

Also applies to: 31-33

---

120-142: get_all_current_exceptions() holds the registry lock longer than the docstring implies.

The comment says “acquires … briefly, then reads each slot’s exception atomically”, but the current implementation keeps the lock while reading each slot. This is probably fine today, but it’s easy to make it match the intent and reduce contention by snapshotting (tid, Arc<ThreadSlot>) first, dropping the lock, then reading atomics.

Proposed refactor (drop registry lock before per-slot reads)

 pub fn get_all_current_exceptions(vm: &VirtualMachine) -> Vec<(u64, Option<PyBaseExceptionRef>)> {
-    let registry = vm.state.thread_frames.lock();
-    registry
-        .iter()
-        .map(|(id, slot)| (*id, slot.exception.to_owned()))
-        .collect()
+    let slots: Vec<(u64, CurrentFrameSlot)> = {
+        let registry = vm.state.thread_frames.lock();
+        registry
+            .iter()
+            .map(|(&id, slot)| (id, Arc::clone(slot)))
+            .collect()
+    };
+    slots
+        .into_iter()
+        .map(|(id, slot)| (id, slot.exception.to_owned()))
+        .collect()
 }

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

659-712: Traceback injection in gen_throw error paths is good; consider factoring the repeated “append traceback entry” block.

You’re doing the same (idx calc → bounds check → PyTraceback::new → set_traceback_typed) in multiple places now. A tiny helper (even a local closure inside gen_throw) would reduce duplication and make it less likely that one path drifts (e.g., missing bounds checks or using a slightly different idx policy).

crates/stdlib/src/faulthandler.rs (2)

328-345: dump_frame_from_ref should avoid relying on current_location() indexing (prefer a bounds-checked lineno derivation).

Even though this isn’t the signal-handler path, faulthandler should be best-effort and not panic if it ever sees an unexpected lasti/locations state. You already do a defensive bounds check in dump_frame_from_raw; it’d be good to mirror that here too.

Proposed defensive lineno computation

 fn dump_frame_from_ref(fd: i32, frame: &crate::vm::Py<Frame>) {
     let funcname = frame.code.obj_name.as_str();
     let filename = frame.code.source_path().as_str();
-    let lineno = if frame.lasti() == 0 {
-        frame.code.first_line_number.map(|n| n.get()).unwrap_or(1) as u32
-    } else {
-        frame.current_location().line.get() as u32
-    };
+    let lasti = frame.lasti();
+    let lineno = if lasti == 0 {
+        frame.code.first_line_number.map(|n| n.get()).unwrap_or(1) as u32
+    } else {
+        let idx = (lasti as usize).saturating_sub(1);
+        frame
+            .code
+            .locations
+            .get(idx)
+            .map(|(loc, _)| loc.line.get() as u32)
+            .unwrap_or_else(|| frame.code.first_line_number.map(|n| n.get()).unwrap_or(1) as u32)
+    };

---

401-428: Avoid holding vm.state.thread_frames lock while doing blocking I/O in dump_all_threads / watchdog dumps.

dump_all_threads() currently holds the registry lock across per-thread printing. If the fd blocks (pipe, slow terminal), that can unnecessarily stall thread cleanup/registration. You already snapshot slots for dump_traceback_later; doing the same here would reduce contention and matches the intended “debug dump shouldn’t freeze unrelated VM operations” behavior.

Sketch refactor: snapshot registry entries before writing

 #[cfg(feature = "threading")]
 {
     let current_tid = rustpython_vm::stdlib::thread::get_ident();
-    let registry = vm.state.thread_frames.lock();
+    let slots: Vec<(u64, ThreadFrameSlot)> = {
+        let registry = vm.state.thread_frames.lock();
+        registry.iter().map(|(&tid, slot)| (tid, Arc::clone(slot))).collect()
+    };

     // First dump non-current threads, then current thread last
-    for (&tid, slot) in registry.iter() {
+    for (tid, slot) in &slots {
         if tid == current_tid {
             continue;
         }
         let frames_guard = slot.frames.lock();
-        dump_traceback_thread_frames(fd, tid, false, &frames_guard);
+        dump_traceback_thread_frames(fd, *tid, false, &frames_guard);
         puts(fd, "\n");
     }

Also applies to: 871-876

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

90-116: Make running flag cleanup panic-safe (scopeguard) and consider avoiding the raw pointer capture.

If func(f) (or anything inside resume_gen_frame) ever panics, self.running stays true, permanently bricking the coroutine (“already executing”). A small scopeguard::defer! would make this robust at near-zero cost.

Also, if borrowck allows, capturing &self.exception directly is clearer than a *const PyAtomicRef<_> (and keeps the unsafe surface smaller).

Possible adjustment

     fn run_with_context<F>(
         &self,
         jen: &PyObject,
         vm: &VirtualMachine,
         func: F,
     ) -> PyResult<ExecutionResult>
     where
         F: FnOnce(&Py<Frame>) -> PyResult<ExecutionResult>,
     {
         if self.running.compare_exchange(false, true).is_err() {
             return Err(vm.new_value_error(format!("{} already executing", gen_name(jen, vm))));
         }
+        scopeguard::defer! {
+            self.running.store(false);
+        }

         // SAFETY: running.compare_exchange guarantees exclusive access
         let gen_exc = unsafe { self.exception.swap(None) };
-        let exception_ptr = &self.exception as *const PyAtomicRef<Option<PyBaseException>>;
+        let exception_ref = &self.exception;

         let result = vm.resume_gen_frame(&self.frame, gen_exc, |f| {
             let result = func(f);
             // SAFETY: exclusive access guaranteed by running flag
-            let _old = unsafe { (*exception_ptr).swap(vm.current_exception()) };
+            let _old = unsafe { exception_ref.swap(vm.current_exception()) };
             result
         });

-        self.running.store(false);
         result
     }

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

1088-1142: Consider extracting shared frame-setup/teardown logic to reduce duplication.

resume_gen_frame and with_frame_exc share ~30 lines of nearly identical boilerplate (push FramePtr, set current frame, store previous, swap owner, scopeguard cleanup, trace events). Differences are: (1) thread-frame push/pop, (2) per-frame trace setup, (3) thread exception update, (4) with_recursion vs manual check.

A shared helper parameterized on those flags (or a builder/options struct) would consolidate the invariants in one place and reduce the risk of them drifting apart. Understandably this may be deferred if the perf-critical path needs to stay fully inlined.

As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."

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

17-25: ThreadSlot: consider restricting field visibility.

Both frames and exception are pub, exposing the raw Mutex<Vec<FramePtr>> and PyAtomicRef to any module. Currently, external reads go through accessor functions (get_all_current_exceptions, get_all_current_frames), but the public fields allow bypassing those. If the intent is to keep access centralized, pub(crate) would be tighter.

---

90-99: Silent no-op when CURRENT_THREAD_SLOT is None.

push_thread_frame and pop_thread_frame silently skip if the slot is uninitialized. While this is a safe defensive choice (the slot should always be Some after enter_vm), a debug_assert! would help catch unexpected call-order bugs during development.

Optional: add debug assertion

 pub fn push_thread_frame(fp: FramePtr) {
     CURRENT_THREAD_SLOT.with(|slot| {
-        if let Some(s) = slot.borrow().as_ref() {
-            s.frames.lock().push(fp);
-        }
+        let borrowed = slot.borrow();
+        debug_assert!(borrowed.is_some(), "push_thread_frame called before enter_vm");
+        if let Some(s) = borrowed.as_ref() {
+            s.frames.lock().push(fp);
+        }
     });
 }

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

103-118: FramePtr safety: the Send impl relies on a non-local invariant.

The unsafe impl Send for FramePtr comment states the Vec is "always empty when the VM moves between threads." This is critical — if violated, a FramePtr could dangle on the new thread. The new_thread() method (line 306) creates a fresh frames: RefCell::new(vec![]), which upholds the invariant for the threading path. Good.

However, there's no compile-time enforcement. If someone later pushes to frames and then moves the VM (e.g., via ThreadedVirtualMachine), this would be UB. Consider adding a debug_assert!(self.frames.borrow().is_empty()) in new_thread() or in the ThreadedVirtualMachine construction.

---

1087-1131: resume_gen_frame: intentionally skips update_thread_exception — document the trade-off.

Line 1113 pushes to the exception stack directly, bypassing thread::update_thread_exception. This means sys._current_exceptions() won't reflect the exception state during generator execution. If this is a deliberate optimization (avoiding TLS + atomic overhead on every generator resume), a brief comment explaining why would help future maintainers, beyond "inline exception push."

Also, the expect on line 1125 in the scopeguard is safe (it pops the entry pushed at line 1113), but if resume_gen_frame is ever called without the matching push being reached (e.g., early return added between 1100 and 1113), the cleanup would panic. The current code is correct, but it's fragile against refactoring.

Suggested comment improvement

-        // Inline exception push without thread exception update
-        self.exceptions.borrow_mut().stack.push(exc);
+        // Inline exception push: skip thread::update_thread_exception() to avoid
+        // TLS + atomic overhead on every generator resume.  sys._current_exceptions()
+        // will not reflect the generator's transient exception state — acceptable
+        // because the frame is short-lived and the snapshot is best-effort.
+        self.exceptions.borrow_mut().stack.push(exc);

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

1095-1100: Recursion guard logic is duplicated from with_recursion.

Lines 1096–1100 replicate the check_recursive_call + check_c_stack_overflow + depth-increment sequence from with_recursion (lines 1000–1011). Consider extracting the guard into a reusable helper (e.g., returning a depth guard) so both call sites share the same logic.

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

93-110: push_thread_frame / pop_thread_frame silently no-op when slot is None.

If CURRENT_THREAD_SLOT is None (i.e. enter_vm was never called), both functions silently skip the operation. This is safe defensively, but a debug_assert! on the None path would help catch initialization ordering bugs during development.

Optional: add debug assertion

 pub fn push_thread_frame(fp: FramePtr) {
     CURRENT_THREAD_SLOT.with(|slot| {
         if let Some(s) = slot.borrow().as_ref() {
             s.frames.lock().push(fp);
+        } else {
+            debug_assert!(false, "push_thread_frame called without initialized thread slot");
         }
     });
 }