From cdf210d29185ed0105f06cf882478d7e657fc7d9 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 14 May 2025 04:55:10 -0400 Subject: [PATCH 01/11] convert module initialization to use multi-phase-init --- pyo3-macros-backend/src/module.rs | 61 ++++--- src/impl_/pymodule.rs | 275 +++++++++++++++++++++++------- src/impl_/trampoline.rs | 17 +- 3 files changed, 261 insertions(+), 92 deletions(-) diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 831bd677b92..3578eae7473 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -389,23 +389,28 @@ pub fn pymodule_module_impl( #[cfg(not(feature = "experimental-inspect"))] let introspection_id = quote! {}; + let gil_used = options.gil_used.map_or(true, |op| op.value.value); + let num_slots: usize = 2; // initializer + gil_used + let module_def = quote! {{ use #pyo3_path::impl_::pymodule as impl_; - const INITIALIZER: impl_::ModuleInitializer = impl_::ModuleInitializer(__pyo3_pymodule); - unsafe { - impl_::ModuleDef::new( - __PYO3_NAME, - #doc, - INITIALIZER - ) + + unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { + #pyo3_path::impl_::trampoline::module_exec(module, |m| __pyo3_pymodule(m)) } + + static SLOTS: impl_::PyModuleSlots<{ #num_slots + 1 }> = impl_::PyModuleSlotsBuilder::new() + .with_mod_exec(__pyo3_module_exec) + .with_gil_used(#gil_used) + .build(); + impl_::ModuleDef::new(__PYO3_NAME, #doc, &SLOTS) }}; let initialization = module_initialization( &name, ctx, module_def, options.submodule.is_some(), - options.gil_used.is_none_or(|op| op.value.value), + true, // FIXME remove this ); let module_consts_names = module_consts.iter().map(|i| i.unraw().to_string()); @@ -456,13 +461,10 @@ pub fn pymodule_function_impl( let vis = &function.vis; let doc = get_doc(&function.attrs, None, ctx)?; - let initialization = module_initialization( - &name, - ctx, - quote! { MakeDef::make_def() }, - false, - options.gil_used.is_none_or(|op| op.value.value), - ); + let gil_used = options.gil_used.map_or(true, |op| op.value.value); + + let initialization = + module_initialization(&name, ctx, quote! { MakeDef::make_def() }, false, gil_used); #[cfg(feature = "experimental-inspect")] let introspection = @@ -497,18 +499,22 @@ pub fn pymodule_function_impl( #[allow(unknown_lints, non_local_definitions)] impl #ident::MakeDef { const fn make_def() -> #pyo3_path::impl_::pymodule::ModuleDef { - fn __pyo3_pymodule(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { - #ident(#(#module_args),*) - } + use #pyo3_path::impl_::pymodule as impl_; - const INITIALIZER: #pyo3_path::impl_::pymodule::ModuleInitializer = #pyo3_path::impl_::pymodule::ModuleInitializer(__pyo3_pymodule); - unsafe { - #pyo3_path::impl_::pymodule::ModuleDef::new( - #ident::__PYO3_NAME, - #doc, - INITIALIZER - ) + unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { + #pyo3_path::impl_::trampoline::module_exec(module, |module| #ident(#(#module_args),*)) } + + static SLOTS: impl_::PyModuleSlots<4> = impl_::PyModuleSlotsBuilder::new() + .with_mod_exec(__pyo3_module_exec) + .with_gil_used(#gil_used) + .build(); + + impl_::ModuleDef::new( + #ident::__PYO3_NAME, + #doc, + &SLOTS + ) } } }) @@ -544,7 +550,10 @@ fn module_initialization( #[doc(hidden)] #[export_name = #pyinit_symbol] pub unsafe extern "C" fn __pyo3_init() -> *mut #pyo3_path::ffi::PyObject { - unsafe { #pyo3_path::impl_::trampoline::module_init(|py| _PYO3_DEF.make_module(py, #gil_used)) } + _PYO3_DEF.init_multi_phase( + unsafe { #pyo3_path::Python::assume_attached() }, + #gil_used + ) } }); } diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 7151ad3bad3..5532b2483d9 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,6 +1,13 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -use std::{cell::UnsafeCell, ffi::CStr, marker::PhantomData}; +#[cfg(Py_3_13)] +use std::sync::Once; +use std::{ + cell::UnsafeCell, + ffi::CStr, + marker::PhantomData, + os::raw::{c_int, c_void}, +}; #[cfg(all( not(any(PyPy, GraalPy)), @@ -16,13 +23,11 @@ use portable_atomic::AtomicI64; target_has_atomic = "64", ))] use std::sync::atomic::AtomicI64; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::Ordering; #[cfg(not(any(PyPy, GraalPy)))] use crate::exceptions::PyImportError; use crate::prelude::PyTypeMethods; -#[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))] -use crate::PyErr; use crate::{ ffi, impl_::pyfunction::PyFunctionDef, @@ -30,12 +35,12 @@ use crate::{ types::{PyModule, PyModuleMethods}, Bound, Py, PyClass, PyResult, PyTypeInfo, Python, }; +use crate::{ffi_ptr_ext::FfiPtrExt, PyErr}; /// `Sync` wrapper of `ffi::PyModuleDef`. pub struct ModuleDef { // wrapped in UnsafeCell so that Rust compiler treats this as interior mutability ffi_def: UnsafeCell, - initializer: ModuleInitializer, /// Interpreter ID where module was initialized (not applicable on PyPy). #[cfg(all( not(any(PyPy, GraalPy)), @@ -46,20 +51,24 @@ pub struct ModuleDef { /// Initialized module object, cached to avoid reinitialization. module: PyOnceLock>, /// Whether or not the module supports running without the GIL - gil_used: AtomicBool, + gil_used: bool, + /// Guard to allow updating the `Py_mod_gil` slot within `ffi_def` + /// before first use. + #[cfg(Py_3_13)] + gil_used_once: Once, } -/// Wrapper to enable initializer to be used in const fns. -pub struct ModuleInitializer(pub for<'py> fn(&Bound<'py, PyModule>) -> PyResult<()>); - unsafe impl Sync for ModuleDef {} impl ModuleDef { /// Make new module definition with given module name. - pub const unsafe fn new( + pub const fn new( name: &'static CStr, doc: &'static CStr, - initializer: ModuleInitializer, + // TODO: it might be nice to make this unsized and not need the + // const N generic parameter, however that might need unsized return values + // or other messy hacks. + slots: &'static PyModuleSlots, ) -> Self { #[allow(clippy::declare_interior_mutable_const)] const INIT: ffi::PyModuleDef = ffi::PyModuleDef { @@ -77,12 +86,12 @@ impl ModuleDef { let ffi_def = UnsafeCell::new(ffi::PyModuleDef { m_name: name.as_ptr(), m_doc: doc.as_ptr(), + m_slots: unsafe { (*slots.0.get()).as_mut_ptr() }, ..INIT }); ModuleDef { ffi_def, - initializer, // -1 is never expected to be a valid interpreter ID #[cfg(all( not(any(PyPy, GraalPy)), @@ -91,16 +100,29 @@ impl ModuleDef { ))] interpreter: AtomicI64::new(-1), module: PyOnceLock::new(), - gil_used: AtomicBool::new(true), + gil_used: true, + #[cfg(Py_3_13)] + gil_used_once: Once::new(), } } - /// Builds a module using user given initializer. Used for [`#[pymodule]`][crate::pymodule]. + + pub fn init_multi_phase(&'static self, _py: Python<'_>, _gil_used: bool) -> *mut ffi::PyObject { + // Once the ffi_def has been used, we can no longer modify, so we set the once. + #[cfg(Py_3_13)] + self.gil_used_once.call_once(|| {}); + unsafe { ffi::PyModuleDef_Init(self.ffi_def.get()) } + } + + /// Builds a module object directly. Used for [`#[pymodule]`][crate::pymodule] submodules. #[cfg_attr(any(Py_LIMITED_API, not(Py_GIL_DISABLED)), allow(unused_variables))] pub fn make_module(&'static self, py: Python<'_>, gil_used: bool) -> PyResult> { // Check the interpreter ID has not changed, since we currently have no way to guarantee // that static data is not reused across interpreters. // // PyPy does not have subinterpreters, so no need to check interpreter ID. + // + // TODO: it should be possible to use the Py_mod_multiple_interpreters slot on sufficiently + // new Python versions to remove the need for this custom logic #[cfg(not(any(PyPy, GraalPy)))] { // PyInterpreterState_Get is only available on 3.9 and later, but is missing @@ -134,34 +156,147 @@ impl ModuleDef { } } } - self.module - .get_or_try_init(py, || { - let module = unsafe { - Py::::from_owned_ptr_or_err( - py, - ffi::PyModule_Create(self.ffi_def.get()), - )? - }; - #[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))] - { - let gil_used_ptr = { - if gil_used { + + // Make a dummy spec, needs a `name` attribute and that seems to be sufficient + // for the loader system + // + // TODO there's probably a cleaner way to handle this case + + #[crate::pyclass(crate = "crate")] + struct ModuleName { + #[pyo3(get)] + name: String, + } + + let ffi_def = self.ffi_def.get(); + + let name = unsafe { CStr::from_ptr((*ffi_def).m_name).to_str()? }.to_string(); + let spec = Bound::new(py, ModuleName { name })?; + + // If the first time writing this ffi def, we can inherit `gil_used` from parent + // modules. + // + // TODO: remove this once we default to `gil_used = false` (i.e. assume free-threading + // support in extensions). This is purely a convenience so that users only need to + // annotate the top-level module. + // + // SAFETY: + // - ffi_def is known to be non-null + // - we check slots is non-null + // - it is valid for a slot to be zeroed + // - we are writing to the slot under the protection of a `Once`. + #[cfg(Py_3_13)] + self.gil_used_once.call_once(|| unsafe { + let slots = (*ffi_def).m_slots; + if !slots.is_null() { + let mut slot = slots; + while slot != std::mem::zeroed() { + if (*slot).slot == ffi::Py_mod_gil { + (*slot).value = if gil_used { ffi::Py_MOD_GIL_USED } else { ffi::Py_MOD_GIL_NOT_USED - } - }; - if unsafe { ffi::PyUnstable_Module_SetGIL(module.as_ptr(), gil_used_ptr) } < 0 { - return Err(PyErr::fetch(py)); + }; + break; } + slot = slot.add(1); } - self.initializer.0(module.bind(py))?; - Ok(module) + } + }); + + self.module + .get_or_try_init(py, || { + let def = self.ffi_def.get(); + let module = unsafe { + ffi::PyModule_FromDefAndSpec(def, spec.as_ptr()).assume_owned_or_err(py)? + } + .cast_into()?; + if unsafe { ffi::PyModule_ExecDef(module.as_ptr(), def) } != 0 { + return Err(PyErr::fetch(py)); + } + Ok(module.unbind()) }) .map(|py_module| py_module.clone_ref(py)) } } +/// Type of the exec slot used to initialise module contents +pub type ModuleExecSlot = unsafe extern "C" fn(*mut ffi::PyObject) -> c_int; + +/// Builder to create `PyModuleSlots`. The size of the number of slots desired must +/// be known up front, and N needs to be at least one greater than the number of +/// actual slots pushed due to the need to have a zeroed element on the end. +pub struct PyModuleSlotsBuilder { + // values (initially all zeroed) + values: [ffi::PyModuleDef_Slot; N], + // current length + len: usize, +} + +impl PyModuleSlotsBuilder { + #[allow(clippy::new_without_default)] + pub const fn new() -> Self { + Self { + values: [unsafe { std::mem::zeroed() }; N], + len: 0, + } + } + + pub const fn with_mod_exec(self, exec: ModuleExecSlot) -> Self { + self.push(ffi::Py_mod_exec, exec as *mut c_void) + } + + pub const fn with_gil_used(self, gil_used: bool) -> Self { + #[cfg(Py_3_13)] + { + self.push( + ffi::Py_mod_gil, + if gil_used { + ffi::Py_MOD_GIL_USED + } else { + ffi::Py_MOD_GIL_NOT_USED + }, + ) + } + + #[cfg(not(Py_3_13))] + { + // Py_mod_gil didn't exist before 3.13, can just make + // this function a noop. + // + // By handling it here we can avoid conditional + // compilation within the macros; they can always emit + // a `.with_gil_used()` call. + self + } + } + + pub const fn build(self) -> PyModuleSlots { + // Required to guarantee there's still a zeroed element + // at the end + assert!( + self.len < N, + "N must be greater than the number of slots pushed" + ); + PyModuleSlots(UnsafeCell::new(self.values)) + } + + const fn push(mut self, slot: c_int, value: *mut c_void) -> Self { + self.values[self.len] = ffi::PyModuleDef_Slot { slot, value }; + self.len += 1; + self + } +} + +/// Wrapper to safely store module slots, to be used in a `ModuleDef`. +pub struct PyModuleSlots(UnsafeCell<[ffi::PyModuleDef_Slot; N]>); + +// It might be possible to avoid this with SyncUnsafeCell in the future +// +// SAFETY: the inner values are only accessed within a `ModuleDef`, +// which only uses them to build the `ffi::ModuleDef`. +unsafe impl Sync for PyModuleSlots {} + /// Trait to add an element (class, function...) to a module. /// /// Currently only implemented for classes. @@ -214,7 +349,7 @@ impl PyAddToModule for PyFunctionDef { impl PyAddToModule for ModuleDef { fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_submodule( - self.make_module(module.py(), self.gil_used.load(Ordering::Relaxed))? + self.make_module(module.py(), self.gil_used)? .bind(module.py()), ) } @@ -222,31 +357,36 @@ impl PyAddToModule for ModuleDef { #[cfg(test)] mod tests { - use std::{ - borrow::Cow, - ffi::CStr, - sync::atomic::{AtomicBool, Ordering}, - }; + use std::{borrow::Cow, ffi::CStr, os::raw::c_int}; use crate::{ - types::{any::PyAnyMethods, module::PyModuleMethods, PyModule}, - Bound, PyResult, Python, + ffi, + impl_::{ + pymodule::{PyModuleSlots, PyModuleSlotsBuilder}, + trampoline, + }, + types::{any::PyAnyMethods, module::PyModuleMethods}, + Python, }; - use super::{ModuleDef, ModuleInitializer}; + use super::ModuleDef; #[test] fn module_init() { - static MODULE_DEF: ModuleDef = unsafe { - ModuleDef::new( - c"test_module", - c"some doc", - ModuleInitializer(|m| { + unsafe extern "C" fn module_exec(module: *mut ffi::PyObject) -> c_int { + unsafe { + trampoline::module_exec(module, |m| { m.add("SOME_CONSTANT", 42)?; Ok(()) - }), - ) - }; + }) + } + } + + static SLOTS: PyModuleSlots<2> = PyModuleSlotsBuilder::new() + .with_mod_exec(module_exec) + .build(); + static MODULE_DEF: ModuleDef = ModuleDef::new(c"test_module", c"some doc", &SLOTS); + Python::attach(|py| { let module = MODULE_DEF.make_module(py, false).unwrap().into_bound(py); assert_eq!( @@ -283,23 +423,36 @@ mod tests { static NAME: &CStr = c"test_module"; static DOC: &CStr = c"some doc"; - static INIT_CALLED: AtomicBool = AtomicBool::new(false); - - #[allow(clippy::unnecessary_wraps)] - fn init(_: &Bound<'_, PyModule>) -> PyResult<()> { - INIT_CALLED.store(true, Ordering::SeqCst); - Ok(()) - } + static SLOTS: PyModuleSlots<2> = PyModuleSlotsBuilder::new().build(); unsafe { - let module_def: ModuleDef = ModuleDef::new(NAME, DOC, ModuleInitializer(init)); + let module_def: ModuleDef = ModuleDef::new(NAME, DOC, &SLOTS); assert_eq!((*module_def.ffi_def.get()).m_name, NAME.as_ptr() as _); assert_eq!((*module_def.ffi_def.get()).m_doc, DOC.as_ptr() as _); + assert_eq!((*module_def.ffi_def.get()).m_slots, SLOTS.0.get().cast()); + } + } - Python::attach(|py| { - module_def.initializer.0(&py.import("builtins").unwrap()).unwrap(); - assert!(INIT_CALLED.load(Ordering::SeqCst)); - }) + #[test] + #[should_panic] + fn test_module_slots_builder_overflow() { + unsafe extern "C" fn module_exec(_module: *mut ffi::PyObject) -> c_int { + 0 + } + + PyModuleSlotsBuilder::<0>::new().with_mod_exec(module_exec); + } + + #[test] + #[should_panic] + fn test_module_slots_builder_overflow_2() { + unsafe extern "C" fn module_exec(_module: *mut ffi::PyObject) -> c_int { + 0 } + + PyModuleSlotsBuilder::<2>::new() + .with_mod_exec(module_exec) + .with_mod_exec(module_exec) + .build(); } } diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs index 6bcf71c6ca7..91ef17eac12 100644 --- a/src/impl_/trampoline.rs +++ b/src/impl_/trampoline.rs @@ -12,14 +12,21 @@ use std::{ use crate::internal::state::AttachGuard; use crate::{ ffi, ffi_ptr_ext::FfiPtrExt, impl_::callback::PyCallbackOutput, impl_::panic::PanicTrap, - impl_::pymethods::IPowModulo, panic::PanicException, types::PyModule, Py, PyResult, Python, + impl_::pymethods::IPowModulo, panic::PanicException, types::PyModule, Bound, PyResult, Python, }; #[inline] -pub unsafe fn module_init( - f: for<'py> unsafe fn(Python<'py>) -> PyResult>, -) -> *mut ffi::PyObject { - unsafe { trampoline(|py| f(py).map(|module| module.into_ptr())) } +pub unsafe fn module_exec( + module: *mut ffi::PyObject, + f: for<'a, 'py> fn(&'a Bound<'py, PyModule>) -> PyResult<()>, +) -> c_int { + unsafe { + trampoline(|py| { + let module = module.assume_borrowed_or_err(py)?.cast::()?; + f(&module)?; + Ok(0) + }) + } } #[inline] From 7ec94aa7f6812d95f2da7efcd9fc874fbe947464 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 19 May 2025 15:06:28 -0400 Subject: [PATCH 02/11] attempt to fix CI issues --- src/impl_.rs | 1 + src/impl_/pymodule.rs | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/impl_.rs b/src/impl_.rs index d72a084e48f..32202794dd4 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -22,6 +22,7 @@ pub mod pyclass; pub mod pyclass_init; pub mod pyfunction; pub mod pymethods; +#[cfg(feature = "macros")] // depends on #[pyclass] pub mod pymodule; #[doc(hidden)] pub mod trampoline; diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 5532b2483d9..ee1d5f76c0d 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -22,8 +22,7 @@ use portable_atomic::AtomicI64; not(all(windows, Py_LIMITED_API, not(Py_3_10))), target_has_atomic = "64", ))] -use std::sync::atomic::AtomicI64; -use std::sync::atomic::Ordering; +use std::sync::atomic::{AtomicI64, Ordering}; #[cfg(not(any(PyPy, GraalPy)))] use crate::exceptions::PyImportError; @@ -86,7 +85,9 @@ impl ModuleDef { let ffi_def = UnsafeCell::new(ffi::PyModuleDef { m_name: name.as_ptr(), m_doc: doc.as_ptr(), - m_slots: unsafe { (*slots.0.get()).as_mut_ptr() }, + // TODO: would be slightly nicer to use `[T]::as_mut_ptr()` here, + // but that requires mut ptr deref on MSRV. + m_slots: slots.0.get() as _, ..INIT }); @@ -236,8 +237,15 @@ pub struct PyModuleSlotsBuilder { impl PyModuleSlotsBuilder { #[allow(clippy::new_without_default)] pub const fn new() -> Self { + // Because the array is c-style, the empty elements should be zeroed. + // `std::mem::zeroed()` requires msrv 1.75 for const + const ZEROED_SLOT: ffi::PyModuleDef_Slot = ffi::PyModuleDef_Slot { + slot: 0, + value: std::ptr::null_mut(), + }; + Self { - values: [unsafe { std::mem::zeroed() }; N], + values: [ZEROED_SLOT; N], len: 0, } } @@ -261,6 +269,9 @@ impl PyModuleSlotsBuilder { #[cfg(not(Py_3_13))] { + // Silence unused variable warning + let _ = gil_used; + // Py_mod_gil didn't exist before 3.13, can just make // this function a noop. // From 21abced542f2428c078c1211b0056f435e9ac974 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Tue, 20 May 2025 22:32:40 +0100 Subject: [PATCH 03/11] alternative spec implementation, fix oob read --- src/impl_.rs | 1 - src/impl_/pymodule.rs | 50 ++++++++++++++++++++++--------------------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/impl_.rs b/src/impl_.rs index 32202794dd4..d72a084e48f 100644 --- a/src/impl_.rs +++ b/src/impl_.rs @@ -22,7 +22,6 @@ pub mod pyclass; pub mod pyclass_init; pub mod pyfunction; pub mod pymethods; -#[cfg(feature = "macros")] // depends on #[pyclass] pub mod pymodule; #[doc(hidden)] pub mod trampoline; diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index ee1d5f76c0d..46fb4749131 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -31,8 +31,8 @@ use crate::{ ffi, impl_::pyfunction::PyFunctionDef, sync::PyOnceLock, - types::{PyModule, PyModuleMethods}, - Bound, Py, PyClass, PyResult, PyTypeInfo, Python, + types::{any::PyAnyMethods, dict::PyDictMethods, PyDict, PyModule, PyModuleMethods}, + Bound, Py, PyAny, PyClass, PyResult, PyTypeInfo, Python, }; use crate::{ffi_ptr_ext::FfiPtrExt, PyErr}; @@ -160,19 +160,16 @@ impl ModuleDef { // Make a dummy spec, needs a `name` attribute and that seems to be sufficient // for the loader system - // - // TODO there's probably a cleaner way to handle this case - #[crate::pyclass(crate = "crate")] - struct ModuleName { - #[pyo3(get)] - name: String, - } + static SIMPLE_NAMESPACE: PyOnceLock> = PyOnceLock::new(); + let simple_ns = SIMPLE_NAMESPACE.import(py, "types", "SimpleNamespace")?; let ffi_def = self.ffi_def.get(); let name = unsafe { CStr::from_ptr((*ffi_def).m_name).to_str()? }.to_string(); - let spec = Bound::new(py, ModuleName { name })?; + let kwargs = PyDict::new(py); + kwargs.set_item("name", name)?; + let spec = simple_ns.call((), Some(&kwargs))?; // If the first time writing this ffi def, we can inherit `gil_used` from parent // modules. @@ -187,20 +184,25 @@ impl ModuleDef { // - it is valid for a slot to be zeroed // - we are writing to the slot under the protection of a `Once`. #[cfg(Py_3_13)] - self.gil_used_once.call_once(|| unsafe { - let slots = (*ffi_def).m_slots; + self.gil_used_once.call_once(|| { + // SAFETY: ffi_def is non-null + let slots = unsafe { (*ffi_def).m_slots }; if !slots.is_null() { - let mut slot = slots; - while slot != std::mem::zeroed() { - if (*slot).slot == ffi::Py_mod_gil { - (*slot).value = if gil_used { + let mut slot_ptr = slots; + // SAFETY: non-null slots pointer + while unsafe { *slot_ptr != ZEROED_SLOT } { + // SAFETY: no other accessors due to call_once guard + let slot = unsafe { &mut *slot_ptr }; + if slot.slot == ffi::Py_mod_gil { + slot.value = if gil_used { ffi::Py_MOD_GIL_USED } else { ffi::Py_MOD_GIL_NOT_USED }; break; } - slot = slot.add(1); + // SAFETY: we have guaranteed there is a trailer zeroed slot + slot_ptr = unsafe { slot_ptr.add(1) }; } } }); @@ -237,14 +239,9 @@ pub struct PyModuleSlotsBuilder { impl PyModuleSlotsBuilder { #[allow(clippy::new_without_default)] pub const fn new() -> Self { - // Because the array is c-style, the empty elements should be zeroed. - // `std::mem::zeroed()` requires msrv 1.75 for const - const ZEROED_SLOT: ffi::PyModuleDef_Slot = ffi::PyModuleDef_Slot { - slot: 0, - value: std::ptr::null_mut(), - }; - Self { + // Because the array is c-style, the empty elements should be zeroed. + // `std::mem::zeroed()` requires msrv 1.75 for const values: [ZEROED_SLOT; N], len: 0, } @@ -308,6 +305,11 @@ pub struct PyModuleSlots(UnsafeCell<[ffi::PyModuleDef_Slot; N]>) // which only uses them to build the `ffi::ModuleDef`. unsafe impl Sync for PyModuleSlots {} +const ZEROED_SLOT: ffi::PyModuleDef_Slot = ffi::PyModuleDef_Slot { + slot: 0, + value: std::ptr::null_mut(), +}; + /// Trait to add an element (class, function...) to a module. /// /// Currently only implemented for classes. From 3cdadf76358e26ff89621388ee79734a41714c45 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 30 May 2025 16:23:54 +0100 Subject: [PATCH 04/11] fix MSRV --- pyo3-macros-backend/src/module.rs | 77 +++++++++++++++---------------- 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 3578eae7473..cdc7d626275 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -15,7 +15,7 @@ use crate::{ get_doc, pyclass::PyClassPyO3Option, pyfunction::{impl_wrap_pyfunction, PyFunctionOptions}, - utils::{has_attribute, has_attribute_with_namespace, Ctx, IdentOrStr}, + utils::{has_attribute, has_attribute_with_namespace, Ctx, IdentOrStr, PythonDoc}, }; use proc_macro2::{Span, TokenStream}; use quote::quote; @@ -390,27 +390,14 @@ pub fn pymodule_module_impl( let introspection_id = quote! {}; let gil_used = options.gil_used.map_or(true, |op| op.value.value); - let num_slots: usize = 2; // initializer + gil_used - let module_def = quote! {{ - use #pyo3_path::impl_::pymodule as impl_; - - unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { - #pyo3_path::impl_::trampoline::module_exec(module, |m| __pyo3_pymodule(m)) - } - - static SLOTS: impl_::PyModuleSlots<{ #num_slots + 1 }> = impl_::PyModuleSlotsBuilder::new() - .with_mod_exec(__pyo3_module_exec) - .with_gil_used(#gil_used) - .build(); - impl_::ModuleDef::new(__PYO3_NAME, #doc, &SLOTS) - }}; let initialization = module_initialization( &name, ctx, - module_def, + quote! { __pyo3_pymodule }, options.submodule.is_some(), - true, // FIXME remove this + gil_used, + doc, ); let module_consts_names = module_consts.iter().map(|i| i.unraw().to_string()); @@ -463,8 +450,14 @@ pub fn pymodule_function_impl( let gil_used = options.gil_used.map_or(true, |op| op.value.value); - let initialization = - module_initialization(&name, ctx, quote! { MakeDef::make_def() }, false, gil_used); + let initialization = module_initialization( + &name, + ctx, + quote! { ModuleExec::__pyo3_module_exec }, + false, + gil_used, + doc, + ); #[cfg(feature = "experimental-inspect")] let introspection = @@ -497,24 +490,9 @@ pub fn pymodule_function_impl( // (and `super` doesn't always refer to the outer scope, e.g. if the `#[pymodule] is // inside a function body) #[allow(unknown_lints, non_local_definitions)] - impl #ident::MakeDef { - const fn make_def() -> #pyo3_path::impl_::pymodule::ModuleDef { - use #pyo3_path::impl_::pymodule as impl_; - - unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { - #pyo3_path::impl_::trampoline::module_exec(module, |module| #ident(#(#module_args),*)) - } - - static SLOTS: impl_::PyModuleSlots<4> = impl_::PyModuleSlotsBuilder::new() - .with_mod_exec(__pyo3_module_exec) - .with_gil_used(#gil_used) - .build(); - - impl_::ModuleDef::new( - #ident::__PYO3_NAME, - #doc, - &SLOTS - ) + impl #ident::ModuleExec { + fn __pyo3_module_exec(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { + #ident(#(#module_args),*) } } }) @@ -523,9 +501,10 @@ pub fn pymodule_function_impl( fn module_initialization( name: &syn::Ident, ctx: &Ctx, - module_def: TokenStream, + module_exec: TokenStream, is_submodule: bool, gil_used: bool, + doc: PythonDoc, ) -> TokenStream { let Ctx { pyo3_path, .. } = ctx; let pyinit_symbol = format!("PyInit_{name}"); @@ -536,9 +515,27 @@ fn module_initialization( #[doc(hidden)] pub const __PYO3_NAME: &'static ::std::ffi::CStr = #pyo3_name; - pub(super) struct MakeDef; + // This structure exists for `fn` modules declared within `fn` bodies, where due to the hidden + // module (used for importing) the `fn` to initialize the module cannot be seen from the #module_def + // declaration just below. #[doc(hidden)] - pub static _PYO3_DEF: #pyo3_path::impl_::pymodule::ModuleDef = #module_def; + pub(super) struct ModuleExec; + + #[doc(hidden)] + pub static _PYO3_DEF: #pyo3_path::impl_::pymodule::ModuleDef = { + use #pyo3_path::impl_::pymodule as impl_; + + unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { + #pyo3_path::impl_::trampoline::module_exec(module, #module_exec) + } + + static SLOTS: impl_::PyModuleSlots<4> = impl_::PyModuleSlotsBuilder::new() + .with_mod_exec(__pyo3_module_exec) + .with_gil_used(#gil_used) + .build(); + + impl_::ModuleDef::new(__PYO3_NAME, #doc, &SLOTS) + }; #[doc(hidden)] // so wrapped submodules can see what gil_used is pub static __PYO3_GIL_USED: bool = #gil_used; From d1adbfd9c522e29c0323ec7e0fe0be396328c2ba Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 30 May 2025 17:06:38 +0100 Subject: [PATCH 05/11] need to exec multi-phase-init module --- pytests/tests/test_misc.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index dd7f8007e81..117af8bf419 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -24,6 +24,7 @@ def test_multiple_imports_same_interpreter_ok(): spec = importlib.util.find_spec("pyo3_pytests.pyo3_pytests") module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) assert dir(module) == dir(pyo3_pytests.pyo3_pytests) From 311b972226dc6251e383add3cb9e3d48bb3b0684 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Thu, 16 Oct 2025 14:57:41 +0000 Subject: [PATCH 06/11] tests/ui: rere invalid_result_conversion --- tests/ui/invalid_result_conversion.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index f782e00828f..ef31f073689 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -6,12 +6,12 @@ error[E0277]: the trait bound `PyErr: From` is not satisfied | = help: the following other types implement trait `From`: `PyErr` implements `From` + `PyErr` implements `From>` + `PyErr` implements `From>` `PyErr` implements `From` `PyErr` implements `From>` `PyErr` implements `From>` `PyErr` implements `From` `PyErr` implements `From` - `PyErr` implements `From` - `PyErr` implements `From>` and $N others = note: required for `MyError` to implement `Into` From 549019a37c5c04bfa18f3e0a20a535133ed3ec57 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Thu, 16 Oct 2025 16:37:38 +0000 Subject: [PATCH 07/11] newsfragments: add PR fragment --- newsfragments/5525.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/5525.added.md diff --git a/newsfragments/5525.added.md b/newsfragments/5525.added.md new file mode 100644 index 00000000000..3262953febc --- /dev/null +++ b/newsfragments/5525.added.md @@ -0,0 +1 @@ +added PEP-489 multi-phase initialization for pymodules From 36a94c64a85d14f7075ca1343c76ee2ffc9a97e6 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Tue, 21 Oct 2025 11:16:56 +0000 Subject: [PATCH 08/11] src/impl_: drop gil_used_once special casing This code was for user convenience when gil_used defaulted to true, but since we're changing that to false, it is no longer needed. --- src/impl_/pymodule.rs | 49 +------------------------------------------ 1 file changed, 1 insertion(+), 48 deletions(-) diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 46fb4749131..4fd429ea000 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,7 +1,5 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -#[cfg(Py_3_13)] -use std::sync::Once; use std::{ cell::UnsafeCell, ffi::CStr, @@ -51,10 +49,6 @@ pub struct ModuleDef { module: PyOnceLock>, /// Whether or not the module supports running without the GIL gil_used: bool, - /// Guard to allow updating the `Py_mod_gil` slot within `ffi_def` - /// before first use. - #[cfg(Py_3_13)] - gil_used_once: Once, } unsafe impl Sync for ModuleDef {} @@ -102,21 +96,16 @@ impl ModuleDef { interpreter: AtomicI64::new(-1), module: PyOnceLock::new(), gil_used: true, - #[cfg(Py_3_13)] - gil_used_once: Once::new(), } } pub fn init_multi_phase(&'static self, _py: Python<'_>, _gil_used: bool) -> *mut ffi::PyObject { - // Once the ffi_def has been used, we can no longer modify, so we set the once. - #[cfg(Py_3_13)] - self.gil_used_once.call_once(|| {}); unsafe { ffi::PyModuleDef_Init(self.ffi_def.get()) } } /// Builds a module object directly. Used for [`#[pymodule]`][crate::pymodule] submodules. #[cfg_attr(any(Py_LIMITED_API, not(Py_GIL_DISABLED)), allow(unused_variables))] - pub fn make_module(&'static self, py: Python<'_>, gil_used: bool) -> PyResult> { + pub fn make_module(&'static self, py: Python<'_>, _gil_used: bool) -> PyResult> { // Check the interpreter ID has not changed, since we currently have no way to guarantee // that static data is not reused across interpreters. // @@ -171,42 +160,6 @@ impl ModuleDef { kwargs.set_item("name", name)?; let spec = simple_ns.call((), Some(&kwargs))?; - // If the first time writing this ffi def, we can inherit `gil_used` from parent - // modules. - // - // TODO: remove this once we default to `gil_used = false` (i.e. assume free-threading - // support in extensions). This is purely a convenience so that users only need to - // annotate the top-level module. - // - // SAFETY: - // - ffi_def is known to be non-null - // - we check slots is non-null - // - it is valid for a slot to be zeroed - // - we are writing to the slot under the protection of a `Once`. - #[cfg(Py_3_13)] - self.gil_used_once.call_once(|| { - // SAFETY: ffi_def is non-null - let slots = unsafe { (*ffi_def).m_slots }; - if !slots.is_null() { - let mut slot_ptr = slots; - // SAFETY: non-null slots pointer - while unsafe { *slot_ptr != ZEROED_SLOT } { - // SAFETY: no other accessors due to call_once guard - let slot = unsafe { &mut *slot_ptr }; - if slot.slot == ffi::Py_mod_gil { - slot.value = if gil_used { - ffi::Py_MOD_GIL_USED - } else { - ffi::Py_MOD_GIL_NOT_USED - }; - break; - } - // SAFETY: we have guaranteed there is a trailer zeroed slot - slot_ptr = unsafe { slot_ptr.add(1) }; - } - } - }); - self.module .get_or_try_init(py, || { let def = self.ffi_def.get(); From 512bf596c7e98611eee3c83a74c0f1061378ede2 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Tue, 21 Oct 2025 11:24:54 +0000 Subject: [PATCH 09/11] pyo3-macros-backend: switch default gil_used to false --- pyo3-macros-backend/src/module.rs | 4 ++-- src/impl_/pymodule.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index cdc7d626275..8eef9d57876 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -389,7 +389,7 @@ pub fn pymodule_module_impl( #[cfg(not(feature = "experimental-inspect"))] let introspection_id = quote! {}; - let gil_used = options.gil_used.map_or(true, |op| op.value.value); + let gil_used = options.gil_used.is_some_and(|op| op.value.value); let initialization = module_initialization( &name, @@ -448,7 +448,7 @@ pub fn pymodule_function_impl( let vis = &function.vis; let doc = get_doc(&function.attrs, None, ctx)?; - let gil_used = options.gil_used.map_or(true, |op| op.value.value); + let gil_used = options.gil_used.is_some_and(|op| op.value.value); let initialization = module_initialization( &name, diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 4fd429ea000..1d52b0063d1 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -95,7 +95,7 @@ impl ModuleDef { ))] interpreter: AtomicI64::new(-1), module: PyOnceLock::new(), - gil_used: true, + gil_used: false, } } From 87029795bd43176fa4234248d2cbdd26f18967f3 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Thu, 23 Oct 2025 11:15:37 +0000 Subject: [PATCH 10/11] tests/ui: rere invalid_property_args --- tests/ui/invalid_property_args.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index 047e6709c21..dc52fc31159 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -65,11 +65,11 @@ error[E0277]: `PhantomData` cannot be converted to a Python object &'a (T0, T1, T2, T3, T4) and $N others = note: required for `PhantomData` to implement `for<'py> PyO3GetField<'py>` -note: required by a bound in `PyClassGetterGenerator::::generate` +note: required by a bound in `PyClassGetterGenerator::::generate` --> src/impl_/pyclass.rs | | pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType | -------- required by a bound in this associated function ... | for<'py> FieldT: PyO3GetField<'py>, - | ^^^^^^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate` + | ^^^^^^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate` From 01b44edb4b9b47843c6c2e8c3e3832e34249ab03 Mon Sep 17 00:00:00 2001 From: Bazaah Date: Thu, 23 Oct 2025 11:16:47 +0000 Subject: [PATCH 11/11] src/impl_: use mem::zeroed over ZEROED_SLOT --- src/impl_/pymodule.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 1d52b0063d1..1ad5a4f499f 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -193,9 +193,7 @@ impl PyModuleSlotsBuilder { #[allow(clippy::new_without_default)] pub const fn new() -> Self { Self { - // Because the array is c-style, the empty elements should be zeroed. - // `std::mem::zeroed()` requires msrv 1.75 for const - values: [ZEROED_SLOT; N], + values: [unsafe { std::mem::zeroed() }; N], len: 0, } } @@ -258,11 +256,6 @@ pub struct PyModuleSlots(UnsafeCell<[ffi::PyModuleDef_Slot; N]>) // which only uses them to build the `ffi::ModuleDef`. unsafe impl Sync for PyModuleSlots {} -const ZEROED_SLOT: ffi::PyModuleDef_Slot = ffi::PyModuleDef_Slot { - slot: 0, - value: std::ptr::null_mut(), -}; - /// Trait to add an element (class, function...) to a module. /// /// Currently only implemented for classes.