Merge pull request #1060 from sebpuetz/memoffset

Calculate offsets for weakreflist and dict in PyCell.

CHANGELOG.md

@@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Fixed
 - Conversion from types with an `__index__` method to Rust BigInts. [#1027](https://github.com/PyO3/pyo3/pull/1027)
 - Fix segfault with #[pyclass(dict, unsendable)] [#1058](https://github.com/PyO3/pyo3/pull/1058)
+- Don't rely on the order of structmembers to compute offsets in PyCell. Related to
+  [#1058](https://github.com/PyO3/pyo3/pull/1058). [#1059](https://github.com/PyO3/pyo3/pull/1059)
 
 ## [0.11.1] - 2020-06-30
 ### Added

src/pycell.rs

@@ -8,9 +8,9 @@ use crate::type_object::{PyBorrowFlagLayout, PyLayout, PySizedLayout, PyTypeInfo
 use crate::types::PyAny;
 use crate::{ffi, FromPy, PyErr, PyNativeType, PyObject, PyResult, Python};
 use std::cell::{Cell, UnsafeCell};
-use std::fmt;
 use std::mem::ManuallyDrop;
 use std::ops::{Deref, DerefMut};
+use std::{fmt, mem};
 
 /// Base layout of PyCell.
 /// This is necessary for sharing BorrowFlag between parents and children.
@@ -162,10 +162,32 @@ impl<T: PyClass> PyCellInner<T> {
 pub struct PyCell<T: PyClass> {
     inner: PyCellInner<T>,
     thread_checker: T::ThreadChecker,
+    // DO NOT CHANGE THE ORDER OF THESE FIELDS WITHOUT CHANGING PyCell::dict_offset()
+    // AND PyCell::weakref_offset()
     dict: T::Dict,
     weakref: T::WeakRef,
 }
 
+impl<T: PyClass> PyCell<T> {
+    /// Get the offset of the dictionary from the start of the struct in bytes.
+    pub(crate) fn dict_offset() -> Option<usize> {
+        if T::Dict::IS_DUMMY {
+            None
+        } else {
+            Some(mem::size_of::<Self>() - mem::size_of::<T::Dict>() - mem::size_of::<T::WeakRef>())
+        }
+    }
+
+    /// Get the offset of the weakref list from the start of the struct in bytes.
+    pub(crate) fn weakref_offset() -> Option<usize> {
+        if T::WeakRef::IS_DUMMY {
+            None
+        } else {
+            Some(mem::size_of::<Self>() - mem::size_of::<T::WeakRef>())
+        }
+    }
+}
+
 unsafe impl<T: PyClass> PyNativeType for PyCell<T> {}
 
 impl<T: PyClass> PyCell<T> {

src/pyclass.rs

@@ -137,18 +137,14 @@ where
     // type size
     type_object.tp_basicsize = std::mem::size_of::<T::Layout>() as ffi::Py_ssize_t;
 
-    let mut offset = type_object.tp_basicsize;
-
     // __dict__ support
-    if let Some(dict_offset) = T::Dict::OFFSET {
-        offset += dict_offset as ffi::Py_ssize_t;
-        type_object.tp_dictoffset = offset;
+    if let Some(dict_offset) = PyCell::<T>::dict_offset() {
+        type_object.tp_dictoffset = dict_offset as ffi::Py_ssize_t;
     }
 
     // weakref support
-    if let Some(weakref_offset) = T::WeakRef::OFFSET {
-        offset += weakref_offset as ffi::Py_ssize_t;
-        type_object.tp_weaklistoffset = offset;
+    if let Some(weakref_offset) = PyCell::<T>::weakref_offset() {
+        type_object.tp_weaklistoffset = weakref_offset as ffi::Py_ssize_t;
     }
 
     // GC support
@@ -206,7 +202,7 @@ where
     // properties
     let mut props = py_class_properties::<T>();
 
-    if T::Dict::OFFSET.is_some() {
+    if !T::Dict::IS_DUMMY {
         props.push(ffi::PyGetSetDef_DICT);
     }
     if !props.is_empty() {

src/pyclass_slots.rs

@@ -2,19 +2,17 @@
 //! Mainly used by our proc-macro codes.
 use crate::{ffi, Python};
 
-const POINTER_SIZE: isize = std::mem::size_of::<*mut ffi::PyObject>() as _;
-
 /// Represents `__dict__` field for `#[pyclass]`.
 pub trait PyClassDict {
-    const OFFSET: Option<isize> = None;
+    const IS_DUMMY: bool = true;
     fn new() -> Self;
     unsafe fn clear_dict(&mut self, _py: Python) {}
     private_decl! {}
 }
 
 /// Represents `__weakref__` field for `#[pyclass]`.
 pub trait PyClassWeakRef {
-    const OFFSET: Option<isize> = None;
+    const IS_DUMMY: bool = true;
     fn new() -> Self;
     unsafe fn clear_weakrefs(&mut self, _obj: *mut ffi::PyObject, _py: Python) {}
     private_decl! {}
@@ -45,7 +43,7 @@ pub struct PyClassDictSlot(*mut ffi::PyObject);
 
 impl PyClassDict for PyClassDictSlot {
     private_impl! {}
-    const OFFSET: Option<isize> = Some(-POINTER_SIZE);
+    const IS_DUMMY: bool = false;
     fn new() -> Self {
         Self(std::ptr::null_mut())
     }
@@ -64,7 +62,7 @@ pub struct PyClassWeakRefSlot(*mut ffi::PyObject);
 
 impl PyClassWeakRef for PyClassWeakRefSlot {
     private_impl! {}
-    const OFFSET: Option<isize> = Some(-POINTER_SIZE);
+    const IS_DUMMY: bool = false;
     fn new() -> Self {
         Self(std::ptr::null_mut())
     }

tests/test_unsendable_dict.rs

@@ -19,3 +19,27 @@ fn test_unsendable_dict() {
     let inst = Py::new(py, UnsendableDictClass {}).unwrap();
     py_run!(py, inst, "assert inst.__dict__ == {}");
 }
+
+#[pyclass(dict, unsendable, weakref)]
+struct UnsendableDictClassWithWeakRef {}
+
+#[pymethods]
+impl UnsendableDictClassWithWeakRef {
+    #[new]
+    fn new() -> Self {
+        Self {}
+    }
+}
+
+#[test]
+fn test_unsendable_dict_with_weakref() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let inst = Py::new(py, UnsendableDictClassWithWeakRef {}).unwrap();
+    py_run!(py, inst, "assert inst.__dict__ == {}");
+    py_run!(
+        py,
+        inst,
+        "import weakref; assert weakref.ref(inst)() is inst; inst.a = 1; assert inst.a == 1"
+    );
+}