Merge branch 'main' into acquire

CHANGELOG.md

@@ -55,9 +55,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Only allow each `#[pymodule]` to be initialized once. [#2523](https://github.com/PyO3/pyo3/pull/2523)
 - `pyo3_build_config::add_extension_module_link_args()` now also emits linker arguments for `wasm32-unknown-emscripten`. [#2538](https://github.com/PyO3/pyo3/pull/2538)
 - Downcasting (`PyTryFrom`) behavior has changed for `PySequence` and `PyMapping`: classes are now required to inherit from (or register with) the corresponding Python standard library abstract base class. See the [migration guide](https://pyo3.rs/latest/migration.html) for information on fixing broken downcasts. [#2477](https://github.com/PyO3/pyo3/pull/2477)
-- Deprecated `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). This change was made for several reasons:
-  - GIL drop order tracking has turned out to be [error prone](https://github.com/PyO3/pyo3/issues/1683). With a scoped API like `Python::with_gil`, these are always dropped in the correct order.
-  - It promotes passing and keeping the GILGuard around, which is almost always not what you actually want.
+- Disable `PyFunction` on `Py_LIMITED_API` and PyPy. [#2542](https://github.com/PyO3/pyo3/pull/2542)
+- Panics during drop of panic payload caught by PyO3 will now abort. [#2544](https://github.com/PyO3/pyo3/pull/2544)
+- Deprecate `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549).
 
 ### Removed
 

benches/bench_gil.rs

@@ -12,9 +12,7 @@ fn bench_clean_gilpool_new(b: &mut Bencher<'_>) {
 
 fn bench_clean_acquire_gil(b: &mut Bencher<'_>) {
     // Acquiring first GIL will also create a "clean" GILPool, so this measures the Python overhead.
-    b.iter(|| {
-        let _ = Python::with_gil(|_| {});
-    });
+    b.iter(|| Python::with_gil(|_| {}));
 }
 
 fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) {
@@ -24,9 +22,7 @@ fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) {
             // Clone and drop an object so that the GILPool has work to do.
             let _ = obj.clone();
         },
-        |_| {
-            let _ = Python::with_gil(|_| {});
-        },
+        |_| Python::with_gil(|_| {}),
         BatchSize::NumBatches(1),
     );
 }

pyo3-ffi/src/funcobject.rs → pyo3-ffi/src/cpython/funcobject.rs

@@ -1,8 +1,8 @@
 use std::os::raw::c_int;
 
-use crate::object::{PyObject, PyTypeObject, Py_TYPE};
+use crate::PyObject;
 
-#[cfg(all(not(PyPy), not(Py_LIMITED_API), not(Py_3_10)))]
+#[cfg(all(not(PyPy), not(Py_3_10)))]
 #[repr(C)]
 pub struct PyFunctionObject {
     pub ob_base: PyObject,
@@ -22,7 +22,7 @@ pub struct PyFunctionObject {
     pub vectorcall: Option<crate::vectorcallfunc>,
 }
 
-#[cfg(all(not(PyPy), not(Py_LIMITED_API), Py_3_10))]
+#[cfg(all(not(PyPy), Py_3_10))]
 #[repr(C)]
 pub struct PyFunctionObject {
     pub ob_base: PyObject,
@@ -44,25 +44,24 @@ pub struct PyFunctionObject {
     pub func_version: u32,
 }
 
-#[cfg(all(PyPy, not(Py_LIMITED_API)))]
+#[cfg(PyPy)]
 #[repr(C)]
 pub struct PyFunctionObject {
     pub ob_base: PyObject,
     pub func_name: *mut PyObject,
 }
 
-#[cfg(all(not(PyPy), Py_LIMITED_API))]
-opaque_struct!(PyFunctionObject);
-
 #[cfg_attr(windows, link(name = "pythonXY"))]
 extern "C" {
+    #[cfg(not(PyPy))] // broken, see https://foss.heptapod.net/pypy/pypy/-/issues/3776
     #[cfg_attr(PyPy, link_name = "PyPyFunction_Type")]
-    pub static mut PyFunction_Type: PyTypeObject;
+    pub static mut PyFunction_Type: crate::PyTypeObject;
 }
 
+#[cfg(not(PyPy))]
 #[inline]
 pub unsafe fn PyFunction_Check(op: *mut PyObject) -> c_int {
-    (Py_TYPE(op) == addr_of_mut_shim!(PyFunction_Type)) as c_int
+    (crate::Py_TYPE(op) == addr_of_mut_shim!(PyFunction_Type)) as c_int
 }
 
 extern "C" {

pyo3-ffi/src/cpython/genobject.rs

@@ -2,13 +2,16 @@ use crate::object::*;
 use crate::PyFrameObject;
 #[cfg(not(PyPy))]
 use crate::_PyErr_StackItem;
+#[cfg(Py_3_11)]
+use std::os::raw::c_char;
 use std::os::raw::c_int;
 
 #[cfg(not(PyPy))]
 #[repr(C)]
 #[derive(Copy, Clone)]
 pub struct PyGenObject {
     pub ob_base: PyObject,
+    #[cfg(not(Py_3_11))]
     pub gi_frame: *mut PyFrameObject,
     #[cfg(not(Py_3_10))]
     pub gi_running: c_int,
@@ -17,6 +20,18 @@ pub struct PyGenObject {
     pub gi_name: *mut PyObject,
     pub gi_qualname: *mut PyObject,
     pub gi_exc_state: _PyErr_StackItem,
+    #[cfg(Py_3_11)]
+    pub gi_origin_or_finalizer: *mut PyObject,
+    #[cfg(Py_3_11)]
+    pub gi_hooks_inited: c_char,
+    #[cfg(Py_3_11)]
+    pub gi_closed: c_char,
+    #[cfg(Py_3_11)]
+    pub gi_running_async: c_char,
+    #[cfg(Py_3_11)]
+    pub gi_frame_state: i8,
+    #[cfg(Py_3_11)]
+    pub gi_iframe: [*mut PyObject; 1],
 }
 
 #[cfg_attr(windows, link(name = "pythonXY"))]

pyo3-ffi/src/cpython/mod.rs

@@ -12,6 +12,7 @@ pub(crate) mod dictobject;
 // skipped fileobject.h
 // skipped fileutils.h
 pub(crate) mod frameobject;
+pub(crate) mod funcobject;
 pub(crate) mod genobject;
 pub(crate) mod import;
 #[cfg(all(Py_3_8, not(PyPy)))]
@@ -44,6 +45,7 @@ pub use self::descrobject::*;
 #[cfg(not(PyPy))]
 pub use self::dictobject::*;
 pub use self::frameobject::*;
+pub use self::funcobject::*;
 pub use self::genobject::*;
 pub use self::import::*;
 #[cfg(all(Py_3_8, not(PyPy)))]

pyo3-ffi/src/cpython/pystate.rs

@@ -31,8 +31,10 @@ pub const PyTrace_OPCODE: c_int = 7;
 #[repr(C)]
 #[derive(Clone, Copy)]
 pub struct _PyErr_StackItem {
+    #[cfg(not(Py_3_11))]
     pub exc_type: *mut PyObject,
     pub exc_value: *mut PyObject,
+    #[cfg(not(Py_3_11))]
     pub exc_traceback: *mut PyObject,
     pub previous_item: *mut _PyErr_StackItem,
 }

pyo3-ffi/src/lib.rs

@@ -299,8 +299,6 @@ pub use self::enumobject::*;
 pub use self::fileobject::*;
 pub use self::fileutils::*;
 pub use self::floatobject::*;
-#[cfg(not(Py_LIMITED_API))]
-pub use self::funcobject::*;
 pub use self::import::*;
 pub use self::intrcheck::*;
 pub use self::iterobject::*;
@@ -368,8 +366,6 @@ mod fileobject;
 mod fileutils;
 mod floatobject;
 // skipped empty frameobject.h
-#[cfg(not(Py_LIMITED_API))]
-pub(crate) mod funcobject;
 // skipped genericaliasobject.h
 mod import;
 // skipped interpreteridobject.h

pyo3-macros-backend/src/pymethod.rs

@@ -339,19 +339,20 @@ fn impl_traverse_slot(cls: &syn::Type, spec: FnSpec<'_>) -> MethodAndSlotDef {
             arg: *mut ::std::os::raw::c_void,
         ) -> ::std::os::raw::c_int
         {
+            let trap = _pyo3::impl_::panic::PanicTrap::new("uncaught panic inside __traverse__ handler");
             let pool = _pyo3::GILPool::new();
             let py = pool.python();
-            _pyo3::callback::abort_on_traverse_panic(::std::panic::catch_unwind(move || {
-                let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
-
-                let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
-                let borrow = slf.try_borrow();
-                if let ::std::result::Result::Ok(borrow) = borrow {
-                    _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit))
-                } else {
-                    0
-                }
-            }))
+            let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
+
+            let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
+            let borrow = slf.try_borrow();
+            let retval = if let ::std::result::Result::Ok(borrow) = borrow {
+                _pyo3::impl_::pymethods::unwrap_traverse_result(borrow.#ident(visit))
+            } else {
+                0
+            };
+            trap.disarm();
+            retval
         }
     };
     let slot_def = quote! {

src/callback.rs

@@ -5,6 +5,7 @@
 use crate::err::{PyErr, PyResult};
 use crate::exceptions::PyOverflowError;
 use crate::ffi::{self, Py_hash_t};
+use crate::impl_::panic::PanicTrap;
 use crate::panic::PanicException;
 use crate::{GILPool, IntoPyPointer};
 use crate::{IntoPy, PyObject, Python};
@@ -243,9 +244,15 @@ where
     F: for<'py> FnOnce(Python<'py>) -> PyResult<R> + UnwindSafe,
     R: PyCallbackOutput,
 {
+    let trap = PanicTrap::new("uncaught panic at ffi boundary");
     let pool = GILPool::new();
     let py = pool.python();
-    panic_result_into_callback_output(py, panic::catch_unwind(move || -> PyResult<_> { body(py) }))
+    let out = panic_result_into_callback_output(
+        py,
+        panic::catch_unwind(move || -> PyResult<_> { body(py) }),
+    );
+    trap.disarm();
+    out
 }
 
 /// Converts the output of std::panic::catch_unwind into a Python function output, either by raising a Python
@@ -259,28 +266,11 @@ pub fn panic_result_into_callback_output<R>(
 where
     R: PyCallbackOutput,
 {
-    let py_result = match panic_result {
-        Ok(py_result) => py_result,
-        Err(payload) => Err(PanicException::from_panic_payload(payload)),
+    let py_err = match panic_result {
+        Ok(Ok(value)) => return value,
+        Ok(Err(py_err)) => py_err,
+        Err(payload) => PanicException::from_panic_payload(payload),
     };
-
-    py_result.unwrap_or_else(|py_err| {
-        py_err.restore(py);
-        R::ERR_VALUE
-    })
-}
-
-/// Aborts if panic has occurred. Used inside `__traverse__` implementations, where panicking is not possible.
-#[doc(hidden)]
-#[inline]
-pub fn abort_on_traverse_panic(
-    panic_result: Result<c_int, Box<dyn Any + Send + 'static>>,
-) -> c_int {
-    match panic_result {
-        Ok(traverse_result) => traverse_result,
-        Err(_payload) => {
-            eprintln!("FATAL: panic inside __traverse__ handler; aborting.");
-            ::std::process::abort()
-        }
-    }
+    py_err.restore(py);
+    R::ERR_VALUE
 }

src/err/mod.rs

@@ -463,11 +463,14 @@ impl PyErr {
     /// This is the opposite of `PyErr::fetch()`.
     #[inline]
     pub fn restore(self, py: Python<'_>) {
-        let (ptype, pvalue, ptraceback) = self
-            .state
-            .into_inner()
-            .expect("Cannot restore a PyErr while normalizing it")
-            .into_ffi_tuple(py);
+        let state = match self.state.into_inner() {
+            Some(state) => state,
+            // Safety: restore takes `self` by value so nothing else is accessing this err
+            // and the invariant is that state is always defined except during make_normalized
+            None => unsafe { std::hint::unreachable_unchecked() },
+        };
+
+        let (ptype, pvalue, ptraceback) = state.into_ffi_tuple(py);
         unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) }
     }
 
@@ -527,6 +530,11 @@ impl PyErr {
         }
     }
 
+    pub(crate) fn write_unraisable(self, py: Python<'_>, context: PyObject) {
+        self.restore(py);
+        unsafe { ffi::PyErr_WriteUnraisable(context.as_ptr()) };
+    }
+
     #[inline]
     fn from_state(state: PyErrState) -> PyErr {
         PyErr {

src/gil.rs

@@ -756,7 +756,7 @@ mod tests {
             let obj: Arc<Py<PyAny>> = Arc::new(get_object(py));
             let thread_obj = Arc::clone(&obj);
 
-            let count = (&*obj).get_refcnt(py);
+            let count = obj.get_refcnt(py);
             println!(
                 "1: The object has been created and its reference count is {}",
                 count

src/impl_.rs

@@ -11,6 +11,7 @@ pub mod freelist;
 pub mod frompyobject;
 pub mod ghost;
 pub(crate) mod not_send;
+pub mod panic;
 #[doc(hidden)]
 pub mod pyclass;
 #[doc(hidden)]

src/impl_/panic.rs

@@ -0,0 +1,27 @@
+/// Type which will panic if dropped.
+///
+/// If this is dropped during a panic, this will cause an abort.
+///
+/// Use this to avoid letting unwinds cross through the FFI boundary, which is UB.
+pub struct PanicTrap {
+    msg: &'static str,
+}
+
+impl PanicTrap {
+    #[inline]
+    pub const fn new(msg: &'static str) -> Self {
+        Self { msg }
+    }
+
+    #[inline]
+    pub const fn disarm(self) {
+        std::mem::forget(self)
+    }
+}
+
+impl Drop for PanicTrap {
+    fn drop(&mut self) {
+        // Panic here will abort the process, assuming in an unwind.
+        panic!("{}", self.msg)
+    }
+}

src/panic.rs

@@ -16,7 +16,9 @@ Python interpreter to exit.
 );
 
 impl PanicException {
-    // Try to format the error in the same way panic does
+    /// Creates a new PanicException from a panic payload.
+    ///
+    /// Attempts to format the error in the same way panic does.
     #[cold]
     pub(crate) fn from_panic_payload(payload: Box<dyn Any + Send + 'static>) -> PyErr {
         if let Some(string) = payload.downcast_ref::<String>() {