From 4c5aee92d53385a3bc623b59f79d8cc9f57c15b7 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 31 Jul 2021 13:53:23 +0100 Subject: [PATCH] pycell: fix calculation of dictoffset on 32-bit Windows --- CHANGELOG.md | 1 + build.rs | 9 ++++++- src/pycell.rs | 70 ++++++++++++++++++++++++++++++++++++++++++-------- src/pyclass.rs | 8 +++--- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5966ec39..79a04564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Fix regression in 0.14.0 rejecting usage of `#[doc(hidden)]` on structs and functions annotated with PyO3 macros. [#1722](https://github.com/PyO3/pyo3/pull/1722) - Fix regression in 0.14.0 leading to incorrect code coverage being computed for `#[pyfunction]`s. [#1726](https://github.com/PyO3/pyo3/pull/1726) - Fix incorrect FFI definition of `Py_Buffer` on PyPy. [#1737](https://github.com/PyO3/pyo3/pull/1737) +- Fix incorrect calculation of `dictoffset` on 32-bit Windows. [#1475](https://github.com/PyO3/pyo3/pull/1475) ## [0.14.1] - 2021-07-04 diff --git a/build.rs b/build.rs index 6f06ad0d..282949cf 100644 --- a/build.rs +++ b/build.rs @@ -192,11 +192,18 @@ fn configure_pyo3() -> Result<()> { )?; interpreter_config.emit_pyo3_cfgs(); + let rustc_minor_version = rustc_minor_version().unwrap_or(0); + // Enable use of const generics on Rust 1.51 and greater - if rustc_minor_version().unwrap_or(0) >= 51 { + if rustc_minor_version >= 51 { println!("cargo:rustc-cfg=min_const_generics"); } + // Enable use of std::ptr::addr_of! on Rust 1.51 and greater + if rustc_minor_version >= 51 { + println!("cargo:rustc-cfg=addr_of"); + } + Ok(()) } diff --git a/src/pycell.rs b/src/pycell.rs index 6eea718a..a7f8c267 100644 --- a/src/pycell.rs +++ b/src/pycell.rs @@ -104,8 +104,6 @@ pub struct PyCell { pub(crate) struct PyCellContents { pub(crate) value: ManuallyDrop>, pub(crate) thread_checker: T::ThreadChecker, - // DO NOT CHANGE THE ORDER OF THESE FIELDS WITHOUT CHANGING PyCell::dict_offset() - // AND PyCell::weakref_offset() pub(crate) dict: T::Dict, pub(crate) weakref: T::WeakRef, } @@ -113,25 +111,77 @@ pub(crate) struct PyCellContents { impl PyCell { /// Get the offset of the dictionary from the start of the struct in bytes. #[cfg(not(all(Py_LIMITED_API, not(Py_3_9))))] - pub(crate) fn dict_offset() -> Option { + pub(crate) fn dict_offset() -> Option { + use std::convert::TryInto; if T::Dict::IS_DUMMY { None } else { - Some( - std::mem::size_of::() - - std::mem::size_of::() - - std::mem::size_of::(), - ) + #[cfg(addr_of)] + let offset = { + // With std::ptr::addr_of - can measure offset using uninit memory without UB. + let cell = std::mem::MaybeUninit::::uninit(); + let base_ptr = cell.as_ptr(); + let dict_ptr = unsafe { std::ptr::addr_of!((*base_ptr).contents.dict) }; + unsafe { (dict_ptr as *const u8).offset_from(base_ptr as *const u8) } + }; + #[cfg(not(addr_of))] + let offset = { + // No std::ptr::addr_of - need to take references to PyCell to measure offsets; + // make a zero-initialised "fake" one so that referencing it is not UB. + let mut cell = std::mem::MaybeUninit::::uninit(); + unsafe { + std::ptr::write_bytes(cell.as_mut_ptr(), 0, 1); + } + let cell = unsafe { cell.assume_init() }; + let dict_ptr = &cell.contents.dict; + // offset_from wasn't stabilised until 1.47, so we also have to work around + // that... + let offset = (dict_ptr as *const _ as usize) - (&cell as *const _ as usize); + // This isn't a valid cell, so ensure no Drop code runs etc. + std::mem::forget(cell); + offset + }; + // Py_ssize_t may not be equal to isize on all platforms + #[allow(clippy::useless_conversion)] + Some(offset.try_into().expect("offset should fit in Py_ssize_t")) } } /// Get the offset of the weakref list from the start of the struct in bytes. #[cfg(not(all(Py_LIMITED_API, not(Py_3_9))))] - pub(crate) fn weakref_offset() -> Option { + pub(crate) fn weakref_offset() -> Option { + use std::convert::TryInto; if T::WeakRef::IS_DUMMY { None } else { - Some(std::mem::size_of::() - std::mem::size_of::()) + #[cfg(addr_of)] + let offset = { + // With std::ptr::addr_of - can measure offset using uninit memory without UB. + let cell = std::mem::MaybeUninit::::uninit(); + let base_ptr = cell.as_ptr(); + let weaklist_ptr = unsafe { std::ptr::addr_of!((*base_ptr).contents.weakref) }; + unsafe { (weaklist_ptr as *const u8).offset_from(base_ptr as *const u8) } + }; + #[cfg(not(addr_of))] + let offset = { + // No std::ptr::addr_of - need to take references to PyCell to measure offsets; + // make a zero-initialised "fake" one so that referencing it is not UB. + let mut cell = std::mem::MaybeUninit::::uninit(); + unsafe { + std::ptr::write_bytes(cell.as_mut_ptr(), 0, 1); + } + let cell = unsafe { cell.assume_init() }; + let weaklist_ptr = &cell.contents.weakref; + // offset_from wasn't stabilised until 1.47, so we also have to work around + // that... + let offset = (weaklist_ptr as *const _ as usize) - (&cell as *const _ as usize); + // This isn't a valid cell, so ensure no Drop code runs etc. + std::mem::forget(cell); + offset + }; + // Py_ssize_t may not be equal to isize on all platforms + #[allow(clippy::useless_conversion)] + Some(offset.try_into().expect("offset should fit in Py_ssize_t")) } } } diff --git a/src/pyclass.rs b/src/pyclass.rs index 987b6050..83a1d963 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -174,13 +174,13 @@ fn tp_init_additional(type_object: *mut ffi::PyTypeObject) { // __dict__ support if let Some(dict_offset) = PyCell::::dict_offset() { unsafe { - (*type_object).tp_dictoffset = dict_offset as ffi::Py_ssize_t; + (*type_object).tp_dictoffset = dict_offset; } } // weakref support if let Some(weakref_offset) = PyCell::::weakref_offset() { unsafe { - (*type_object).tp_weaklistoffset = weakref_offset as ffi::Py_ssize_t; + (*type_object).tp_weaklistoffset = weakref_offset; } } } @@ -233,11 +233,11 @@ fn py_class_method_defs( #[cfg(Py_3_9)] fn py_class_members() -> Vec { #[inline(always)] - fn offset_def(name: &'static str, offset: usize) -> ffi::structmember::PyMemberDef { + fn offset_def(name: &'static str, offset: ffi::Py_ssize_t) -> ffi::structmember::PyMemberDef { ffi::structmember::PyMemberDef { name: name.as_ptr() as _, type_code: ffi::structmember::T_PYSSIZET, - offset: offset as _, + offset, flags: ffi::structmember::READONLY, doc: std::ptr::null_mut(), }