Fix exception state model and finally handler cleanup

youknowone · youknowone · commit a15185eb8329 · 2026-04-05T14:05:40.000+09:00
- ExceptionStack: init with base slot instead of empty vec
- with_frame_impl: save/restore exc_info value instead of
  push/pop slot, so callees see caller's handled exception
- Remove unused with_frame_exc
- resume_gen_frame: use push_exception/pop_exception methods
- codegen: move RERAISE inside cleanup handler's exception
  table range in finally blocks (both try-finally and
  try-except-finally), so POP_EXCEPT runs before re-raising
diff --git a/crates/codegen/src/compile.rs b/crates/codegen/src/compile.rs
@@ -3166,16 +3166,18 @@ impl Compiler {
                 }
                 self.compile_statements(finalbody)?;
 
-                // Pop FinallyEnd fblock BEFORE emitting RERAISE
-                // This ensures RERAISE routes to outer exception handler, not cleanup block
-                // Cleanup block is only for new exceptions raised during finally body execution
+                // RERAISE must be inside the cleanup handler's exception table
+                // range. When RERAISE re-raises the exception, the cleanup
+                // handler (COPY 3, POP_EXCEPT, RERAISE 1) runs POP_EXCEPT to
+                // restore exc_info before the exception reaches the outer handler.
+                emit!(self, Instruction::Reraise { depth: 0 });
+
+                // PopBlock after RERAISE (dead code, but marks the exception
+                // table range end so the cleanup covers RERAISE).
                 if finally_cleanup_block.is_some() {
                     emit!(self, PseudoInstruction::PopBlock);
                     self.pop_fblock(FBlockType::FinallyEnd);
                 }
-
-                // CPython re-raises first and lets the cleanup block restore prev_exc.
-                emit!(self, Instruction::Reraise { depth: 0 });
             }
 
             if let Some(cleanup) = finally_cleanup_block {
@@ -3432,16 +3434,18 @@ impl Compiler {
             // Run finally body
             self.compile_statements(finalbody)?;
 
-            // Pop FinallyEnd fblock BEFORE emitting RERAISE
-            // This ensures RERAISE routes to outer exception handler, not cleanup block
-            // Cleanup block is only for new exceptions raised during finally body execution
+            // RERAISE must be inside the cleanup handler's exception table
+            // range. The cleanup handler (COPY 3, POP_EXCEPT, RERAISE 1)
+            // runs POP_EXCEPT to restore exc_info before re-raising to
+            // the outer handler.
+            emit!(self, Instruction::Reraise { depth: 0 });
+
+            // PopBlock after RERAISE (dead code, but marks the exception
+            // table range end so the cleanup covers RERAISE).
             if finally_cleanup_block.is_some() {
                 emit!(self, PseudoInstruction::PopBlock);
                 self.pop_fblock(FBlockType::FinallyEnd);
             }
-
-            // CPython re-raises first and lets the cleanup block restore prev_exc.
-            emit!(self, Instruction::Reraise { depth: 0 });
         }
 
         // finally cleanup block
@@ -3473,11 +3477,7 @@ impl Compiler {
         let handler_block = self.new_block();
         let cleanup_block = self.new_block();
         let end_block = self.new_block();
-        let orelse_block = if orelse.is_empty() {
-            end_block
-        } else {
-            self.new_block()
-        };
+        let orelse_block = self.new_block();
 
         emit!(self, Instruction::Nop);
         emit!(
@@ -3624,16 +3624,16 @@ impl Compiler {
         emit!(self, Instruction::Reraise { depth: 1 });
         self.set_no_location();
 
+        self.switch_to_block(orelse_block);
+        self.set_no_location();
         if !orelse.is_empty() {
-            self.switch_to_block(orelse_block);
-            self.set_no_location();
             self.compile_statements(orelse)?;
-            emit!(
-                self,
-                PseudoInstruction::JumpNoInterrupt { delta: end_block }
-            );
-            self.set_no_location();
         }
+        emit!(
+            self,
+            PseudoInstruction::JumpNoInterrupt { delta: end_block }
+        );
+        self.set_no_location();
 
         self.switch_to_block(end_block);
         Ok(())
diff --git a/crates/vm/src/frame.rs b/crates/vm/src/frame.rs
@@ -6769,7 +6769,6 @@ impl ExecutingFrame<'_> {
             }
             bytecode::RaiseKind::BareRaise => {
                 // RAISE_VARARGS 0: bare `raise` gets exception from VM state
-                // This is the current exception set by PUSH_EXC_INFO
                 vm.topmost_exception()
                     .ok_or_else(|| vm.new_runtime_error("No active exception to reraise"))?
             }
diff --git a/crates/vm/src/vm/mod.rs b/crates/vm/src/vm/mod.rs
@@ -108,7 +108,7 @@ pub struct VirtualMachine {
 }
 
 /// Non-owning frame pointer for the frames stack.
-/// The pointed-to frame is kept alive by the caller of with_frame_exc/resume_gen_frame.
+/// The pointed-to frame is kept alive by the caller of with_frame/resume_gen_frame.
 #[derive(Copy, Clone)]
 pub struct FramePtr(NonNull<Py<Frame>>);
 
@@ -124,11 +124,23 @@ impl FramePtr {
 // FrameRef is alive on the call stack. The Vec is always empty when the VM moves between threads.
 unsafe impl Send for FramePtr {}
 
-#[derive(Debug, Default)]
+#[derive(Debug)]
 struct ExceptionStack {
+    /// Linked list of handled-exception slots (`_PyErr_StackItem` chain).
+    /// Bottom element is the thread's base slot; generator/coroutine resume
+    /// pushes an additional slot.  Normal frame calls do **not** push/pop.
     stack: Vec<Option<PyBaseExceptionRef>>,
 }
 
+impl Default for ExceptionStack {
+    fn default() -> Self {
+        // Thread's base `_PyErr_StackItem` – always present.
+        Self {
+            stack: vec![None],
+        }
+    }
+}
+
 /// Stop-the-world state for fork safety. Before `fork()`, the requester
 /// stops all other Python threads so they are not holding internal locks.
 #[cfg(all(unix, feature = "threading"))]
@@ -1554,31 +1566,20 @@ impl VirtualMachine {
         frame: FrameRef,
         f: F,
     ) -> PyResult<R> {
-        self.with_frame_impl(frame, None, true, f)
-    }
-
-    /// Like `with_frame` but allows specifying the initial exception state.
-    pub fn with_frame_exc<R, F: FnOnce(FrameRef) -> PyResult<R>>(
-        &self,
-        frame: FrameRef,
-        exc: Option<PyBaseExceptionRef>,
-        f: F,
-    ) -> PyResult<R> {
-        self.with_frame_impl(frame, exc, true, f)
+        self.with_frame_impl(frame, true, f)
     }
 
     pub(crate) fn with_frame_untraced<R, F: FnOnce(FrameRef) -> PyResult<R>>(
         &self,
         frame: FrameRef,
         f: F,
     ) -> PyResult<R> {
-        self.with_frame_impl(frame, None, false, f)
+        self.with_frame_impl(frame, false, f)
     }
 
     fn with_frame_impl<R, F: FnOnce(FrameRef) -> PyResult<R>>(
         &self,
         frame: FrameRef,
-        exc: Option<PyBaseExceptionRef>,
         traced: bool,
         f: F,
     ) -> PyResult<R> {
@@ -1597,19 +1598,22 @@ impl VirtualMachine {
                 old_frame as *mut Frame,
                 core::sync::atomic::Ordering::Relaxed,
             );
-            // Push exception context for frame isolation.
-            // For normal calls: None (clean slate).
-            // For generators: the saved exception from last yield.
-            self.push_exception(exc);
+            // Normal frame calls share the caller's exc_info slot so that
+            // callees can see the caller's handled exception via sys.exc_info().
+            // Save the current value to restore on exit — this prevents
+            // exc_info pollution from frames with unbalanced
+            // PUSH_EXC_INFO/POP_EXCEPT (e.g., exception escaping an except block
+            // whose cleanup entry is missing from the exception table).
+            let saved_exc = self.current_exception();
             let old_owner = frame.owner.swap(
                 crate::frame::FrameOwner::Thread as i8,
                 core::sync::atomic::Ordering::AcqRel,
             );
 
-            // Ensure cleanup on panic: restore owner, pop exception, frame chain, and frames Vec.
+            // Ensure cleanup on panic: restore owner, exc_info, frame chain, and frames Vec.
             scopeguard::defer! {
                 frame.owner.store(old_owner, core::sync::atomic::Ordering::Release);
-                self.pop_exception();
+                self.set_exception(saved_exc);
                 crate::vm::thread::set_current_frame(old_frame);
                 self.frames.borrow_mut().pop();
                 #[cfg(feature = "threading")]
@@ -1624,9 +1628,9 @@ impl VirtualMachine {
         })
     }
 
-    /// Lightweight frame execution for generator/coroutine resume.
-    /// Pushes to the thread frame stack and fires trace/profile events,
-    /// but skips the thread exception update for performance.
+    /// Frame execution for generator/coroutine resume.
+    /// Pushes a new exc_info slot (gi_exc_state) onto the chain,
+    /// linking the generator's saved handled-exception.
     pub fn resume_gen_frame<R, F: FnOnce(&Py<Frame>) -> PyResult<R>>(
         &self,
         frame: &FrameRef,
@@ -1649,20 +1653,20 @@ impl VirtualMachine {
             old_frame as *mut Frame,
             core::sync::atomic::Ordering::Relaxed,
         );
-        // Inline exception push without thread exception update
-        self.exceptions.borrow_mut().stack.push(exc);
+        // Push generator's exc_info slot onto the chain
+        // (gi_exc_state.previous_item = tstate->exc_info;
+        //  tstate->exc_info = &gi_exc_state;)
+        self.push_exception(exc);
         let old_owner = frame.owner.swap(
             crate::frame::FrameOwner::Thread as i8,
             core::sync::atomic::Ordering::AcqRel,
         );
 
-        // Ensure cleanup on panic: restore owner, pop exception, frame chain, frames Vec,
-        // and recursion depth.
+        // Ensure cleanup on panic: restore owner, pop exc_info slot, frame chain,
+        // frames Vec, and recursion depth.
         scopeguard::defer! {
             frame.owner.store(old_owner, core::sync::atomic::Ordering::Release);
-            self.exceptions.borrow_mut().stack
-                .pop()
-                .expect("pop_exception() without nested exc stack");
+            self.pop_exception();
             crate::vm::thread::set_current_frame(old_frame);
             self.frames.borrow_mut().pop();
             #[cfg(feature = "threading")]
@@ -2037,12 +2041,14 @@ impl VirtualMachine {
         }
     }
 
+    /// Push a new exc_info slot (for generator/coroutine resume).
     pub(crate) fn push_exception(&self, exc: Option<PyBaseExceptionRef>) {
         self.exceptions.borrow_mut().stack.push(exc);
         #[cfg(feature = "threading")]
         thread::update_thread_exception(self.topmost_exception());
     }
 
+    /// Pop the topmost exc_info slot (generator/coroutine yield/return).
     pub(crate) fn pop_exception(&self) -> Option<PyBaseExceptionRef> {
         let exc = self
             .exceptions
@@ -2059,6 +2065,7 @@ impl VirtualMachine {
         self.exceptions.borrow().stack.last().cloned().flatten()
     }
 
+    /// Set the current exc_info slot value (PUSH_EXC_INFO / POP_EXCEPT).
     pub(crate) fn set_exception(&self, exc: Option<PyBaseExceptionRef>) {
         // don't be holding the RefCell guard while __del__ is called
         let mut excs = self.exceptions.borrow_mut();

Original file line number	Diff line number	Diff line change
`@@ -6769,7 +6769,6 @@ impl ExecutingFrame<'_> {`
`6769`	`6769`	`}`
`6770`	`6770`	`bytecode::RaiseKind::BareRaise => {`
`6771`	`6771`	// RAISE_VARARGS 0: bare `raise` gets exception from VM state
`6772`		`- // This is the current exception set by PUSH_EXC_INFO`
`6773`	`6772`	`vm.topmost_exception()`
`6774`	`6773`	`.ok_or_else(\|\| vm.new_runtime_error("No active exception to reraise"))?`
`6775`	`6774`	`}`