call PyObject_GC_Untrack before deallocating

PyO3 · Sep 8, 2023 · 8514a01 · 8514a01
1 parent d3d2530
commit 8514a01
Show file tree

Hide file tree

Showing 10 changed files with 62 additions and 66 deletions.
diff --git a/guide/src/class.md b/guide/src/class.md
@@ -1136,7 +1136,6 @@ impl pyo3::IntoPy<PyObject> for MyClass {
 impl pyo3::impl_::pyclass::PyClassImpl for MyClass {
     const IS_BASETYPE: bool = false;
     const IS_SUBCLASS: bool = false;
-    type Layout = PyCell<MyClass>;
     type BaseType = PyAny;
     type ThreadChecker = pyo3::impl_::pyclass::SendablePyClass<MyClass>;
     type PyClassMutability = <<pyo3::PyAny as pyo3::impl_::pyclass::PyClassBaseType>::PyClassMutability as pyo3::impl_::pycell::PyClassMutability>::MutableChild;

diff --git a/newsfragments/3404.fixed.md b/newsfragments/3404.fixed.md
@@ -0,0 +1 @@
+Fix `ResourceWarning` and crashes related to GC when running with debug builds of CPython.
diff --git a/noxfile.py b/noxfile.py
@@ -475,14 +475,14 @@ def set_minimal_package_versions(session: nox.Session):
     min_pkg_versions = {
         "rust_decimal": "1.26.1",
         "csv": "1.1.6",
+        "indexmap": "1.9.3",
         "hashbrown": "0.12.3",
         "log": "0.4.17",
         "once_cell": "1.17.2",
         "rayon": "1.6.1",
         "rayon-core": "1.10.2",
         "regex": "1.7.3",
         "proptest": "1.0.0",
-        "indexmap": "1.9.3",
         "chrono": "0.4.25",
     }
 

diff --git a/pyo3-ffi/src/objimpl.rs b/pyo3-ffi/src/objimpl.rs
@@ -7,6 +7,7 @@ use crate::pyport::Py_ssize_t;
 extern "C" {
     #[cfg_attr(PyPy, link_name = "PyPyObject_Malloc")]
     pub fn PyObject_Malloc(size: size_t) -> *mut c_void;
+    #[cfg_attr(PyPy, link_name = "PyPyObject_Calloc")]
     pub fn PyObject_Calloc(nelem: size_t, elsize: size_t) -> *mut c_void;
     #[cfg_attr(PyPy, link_name = "PyPyObject_Realloc")]
     pub fn PyObject_Realloc(ptr: *mut c_void, new_size: size_t) -> *mut c_void;
@@ -72,7 +73,9 @@ extern "C" {
     pub fn _PyObject_GC_New(arg1: *mut PyTypeObject) -> *mut PyObject;
     #[cfg_attr(PyPy, link_name = "_PyPyObject_GC_NewVar")]
     pub fn _PyObject_GC_NewVar(arg1: *mut PyTypeObject, arg2: Py_ssize_t) -> *mut PyVarObject;
+    #[cfg(not(PyPy))]
     pub fn PyObject_GC_Track(arg1: *mut c_void);
+    #[cfg(not(PyPy))]
     pub fn PyObject_GC_UnTrack(arg1: *mut c_void);
     #[cfg_attr(PyPy, link_name = "PyPyObject_GC_Del")]
     pub fn PyObject_GC_Del(arg1: *mut c_void);

diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs
@@ -1029,7 +1029,6 @@ impl<'a> PyClassImplsBuilder<'a> {
                 const IS_MAPPING: bool = #is_mapping;
                 const IS_SEQUENCE: bool = #is_sequence;
 
-                type Layout = _pyo3::PyCell<Self>;
                 type BaseType = #base;
                 type ThreadChecker = #thread_checker;
                 #inventory

diff --git a/src/impl_/freelist.rs b/src/impl_/freelist.rs
@@ -55,7 +55,7 @@ impl<T> FreeList<T> {
         }
     }
 
-    /// Inserts a value into the list. Returns `None` if the `FreeList` is full.
+    /// Inserts a value into the list. Returns `Some(val)` if the `FreeList` is full.
     pub fn insert(&mut self, val: T) -> Option<T> {
         let next = self.split + 1;
         if next < self.capacity {

diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs
@@ -6,7 +6,6 @@ use crate::{
     internal_tricks::extract_c_string,
     pycell::PyCellLayout,
     pyclass_init::PyObjectInit,
-    type_object::PyLayout,
     Py, PyAny, PyCell, PyClass, PyErr, PyMethodDefType, PyNativeType, PyResult, PyTypeInfo, Python,
 };
 use std::{
@@ -153,9 +152,6 @@ pub trait PyClassImpl: Sized + 'static {
     /// #[pyclass(sequence)]
     const IS_SEQUENCE: bool = false;
 
-    /// Layout
-    type Layout: PyLayout<Self>;
-
     /// Base class
     type BaseType: PyTypeInfo + PyClassBaseType;
 
@@ -1098,9 +1094,18 @@ impl<T: PyClass> PyClassBaseType for T {
     type PyClassMutability = T::PyClassMutability;
 }
 
-/// Implementation of tp_dealloc for all pyclasses
+/// Implementation of tp_dealloc for pyclasses without gc
 pub(crate) unsafe extern "C" fn tp_dealloc<T: PyClass>(obj: *mut ffi::PyObject) {
-    crate::impl_::trampoline::dealloc(obj, T::Layout::tp_dealloc)
+    crate::impl_::trampoline::dealloc(obj, PyCell::<T>::tp_dealloc)
+}
+
+/// Implementation of tp_dealloc for pyclasses with gc
+pub(crate) unsafe extern "C" fn tp_dealloc_with_gc<T: PyClass>(obj: *mut ffi::PyObject) {
+    #[cfg(not(PyPy))]
+    {
+        ffi::PyObject_GC_UnTrack(obj.cast());
+    }
+    crate::impl_::trampoline::dealloc(obj, PyCell::<T>::tp_dealloc)
 }
 
 pub(crate) unsafe extern "C" fn get_sequence_item_from_mapping(

diff --git a/src/pycell/impl_.rs b/src/pycell/impl_.rs
@@ -174,8 +174,7 @@ impl<T: PyClassImpl<PyClassMutability = Self>> GetBorrowChecker<T> for Immutable
 impl<T: PyClassImpl<PyClassMutability = Self>, M: PyClassMutability> GetBorrowChecker<T>
     for ExtendsMutableAncestor<M>
 where
-    T::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>
-        + PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
+    T::BaseType: PyClassImpl + PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
     <T::BaseType as PyClassImpl>::PyClassMutability: PyClassMutability<Checker = BorrowChecker>,
 {
     fn borrow_checker(cell: &PyCell<T>) -> &BorrowChecker {
@@ -188,7 +187,6 @@ where
 mod tests {
     use super::*;
 
-    use crate::impl_::pyclass::{PyClassBaseType, PyClassImpl};
     use crate::prelude::*;
     use crate::pyclass::boolean_struct::{False, True};
     use crate::PyClass;
@@ -239,25 +237,11 @@ mod tests {
     fn assert_immutable<T: PyClass<Frozen = True, PyClassMutability = ImmutableClass>>() {}
     fn assert_mutable_with_mutable_ancestor<
         T: PyClass<Frozen = False, PyClassMutability = ExtendsMutableAncestor<MutableClass>>,
-    >()
-    // These horrible bounds are necessary for Rust 1.48 but not newer versions
-    where
-        <T as PyClassImpl>::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>,
-        <<T as PyClassImpl>::BaseType as PyClassImpl>::PyClassMutability:
-            PyClassMutability<Checker = BorrowChecker>,
-        <T as PyClassImpl>::BaseType: PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
-    {
+    >() {
     }
     fn assert_immutable_with_mutable_ancestor<
         T: PyClass<Frozen = True, PyClassMutability = ExtendsMutableAncestor<ImmutableClass>>,
-    >()
-    // These horrible bounds are necessary for Rust 1.48 but not newer versions
-    where
-        <T as PyClassImpl>::BaseType: PyClassImpl<Layout = PyCell<T::BaseType>>,
-        <<T as PyClassImpl>::BaseType as PyClassImpl>::PyClassMutability:
-            PyClassMutability<Checker = BorrowChecker>,
-        <T as PyClassImpl>::BaseType: PyClassBaseType<LayoutAsBase = PyCell<T::BaseType>>,
-    {
+    >() {
     }
 
     #[test]

diff --git a/src/pyclass.rs b/src/pyclass.rs
@@ -15,9 +15,7 @@ pub use self::gc::{PyTraverseError, PyVisit};
 ///
 /// The `#[pyclass]` attribute implements this trait for your Rust struct -
 /// you shouldn't implement this trait directly.
-pub trait PyClass:
-    PyTypeInfo<AsRefTarget = PyCell<Self>> + PyClassImpl<Layout = PyCell<Self>>
-{
+pub trait PyClass: PyTypeInfo<AsRefTarget = PyCell<Self>> + PyClassImpl {
     /// Whether the pyclass is frozen.
     ///
     /// This can be enabled via `#[pyclass(frozen)]`.

diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs
@@ -1,16 +1,18 @@
+use pyo3_ffi::PyType_IS_GC;
+
 use crate::{
     exceptions::PyTypeError,
     ffi,
     impl_::pyclass::{
         assign_sequence_item_from_mapping, get_sequence_item_from_mapping, tp_dealloc,
-        PyClassItemsIter,
+        tp_dealloc_with_gc, PyClassItemsIter,
     },
     impl_::{
         pymethods::{get_doc, get_name, Getter, Setter},
         trampoline::trampoline,
     },
     types::PyType,
-    Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
+    Py, PyCell, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
 };
 use std::{
     borrow::Cow,
@@ -32,22 +34,37 @@ where
     T: PyClass,
 {
     unsafe {
-        PyTypeBuilder::default()
-            .type_doc(T::doc(py)?)
-            .offsets(T::dict_offset(), T::weaklist_offset())
-            .slot(ffi::Py_tp_base, T::BaseType::type_object_raw(py))
-            .slot(ffi::Py_tp_dealloc, tp_dealloc::<T> as *mut c_void)
-            .set_is_basetype(T::IS_BASETYPE)
-            .set_is_mapping(T::IS_MAPPING)
-            .set_is_sequence(T::IS_SEQUENCE)
-            .class_items(T::items_iter())
-            .build(py, T::NAME, T::MODULE, std::mem::size_of::<T::Layout>())
+        PyTypeBuilder {
+            slots: Vec::new(),
+            method_defs: Vec::new(),
+            getset_builders: HashMap::new(),
+            cleanup: Vec::new(),
+            tp_base: T::BaseType::type_object_raw(py),
+            tp_dealloc: tp_dealloc::<T>,
+            tp_dealloc_with_gc: tp_dealloc_with_gc::<T>,
+            is_mapping: T::IS_MAPPING,
+            is_sequence: T::IS_SEQUENCE,
+            has_new: false,
+            has_dealloc: false,
+            has_getitem: false,
+            has_setitem: false,
+            has_traverse: false,
+            has_clear: false,
+            has_dict: false,
+            class_flags: 0,
+            #[cfg(all(not(Py_3_9), not(Py_LIMITED_API)))]
+            buffer_procs: Default::default(),
+        }
+        .type_doc(T::doc(py)?)
+        .offsets(T::dict_offset(), T::weaklist_offset())
+        .set_is_basetype(T::IS_BASETYPE)
+        .class_items(T::items_iter())
+        .build(py, T::NAME, T::MODULE, std::mem::size_of::<PyCell<T>>())
     }
 }
 
 type PyTypeBuilderCleanup = Box<dyn Fn(&PyTypeBuilder, *mut ffi::PyTypeObject)>;
 
-#[derive(Default)]
 struct PyTypeBuilder {
     slots: Vec<ffi::PyType_Slot>,
     method_defs: Vec<ffi::PyMethodDef>,
@@ -56,6 +73,9 @@ struct PyTypeBuilder {
     /// PyType_FromSpec API for... there's no reason this should work,
     /// except for that it does and we have tests.
     cleanup: Vec<PyTypeBuilderCleanup>,
+    tp_base: *mut ffi::PyTypeObject,
+    tp_dealloc: ffi::destructor,
+    tp_dealloc_with_gc: ffi::destructor,
     is_mapping: bool,
     is_sequence: bool,
     has_new: bool,
@@ -114,13 +134,6 @@ impl PyTypeBuilder {
         }
     }
 
-    /// # Safety
-    /// The given pointer must be of the correct type for the given slot
-    unsafe fn slot<T>(mut self, slot: c_int, pfunc: *mut T) -> Self {
-        self.push_slot(slot, pfunc);
-        self
-    }
-
     fn pymethod_def(&mut self, def: &PyMethodDefType) {
         match def {
             PyMethodDefType::Getter(getter) => {
@@ -220,16 +233,6 @@ impl PyTypeBuilder {
         self
     }
 
-    fn set_is_mapping(mut self, is_mapping: bool) -> Self {
-        self.is_mapping = is_mapping;
-        self
-    }
-
-    fn set_is_sequence(mut self, is_sequence: bool) -> Self {
-        self.is_sequence = is_sequence;
-        self
-    }
-
     /// # Safety
     /// All slots in the PyClassItemsIter should be correct
     unsafe fn class_items(mut self, iter: PyClassItemsIter) -> Self {
@@ -342,15 +345,19 @@ impl PyTypeBuilder {
 
         let getset_destructors = self.finalize_methods_and_properties()?;
 
+        unsafe { self.push_slot(ffi::Py_tp_base, self.tp_base) }
+
         if !self.has_new {
             // Safety: This is the correct slot type for Py_tp_new
             unsafe { self.push_slot(ffi::Py_tp_new, no_constructor_defined as *mut c_void) }
         }
 
-        assert!(
-            self.has_dealloc,
-            "PyTypeBuilder requires you to specify slot ffi::Py_tp_dealloc"
-        );
+        let tp_dealloc = if self.has_traverse || unsafe { PyType_IS_GC(self.tp_base) == 1 } {
+            self.tp_dealloc_with_gc
+        } else {
+            self.tp_dealloc
+        };
+        unsafe { self.push_slot(ffi::Py_tp_dealloc, tp_dealloc as *mut c_void) }
 
         if self.has_clear && !self.has_traverse {
             return Err(PyTypeError::new_err(format!(
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Fix `ResourceWarning` and crashes related to GC when running with debug builds of CPython.