Update FFI wrappers for PyModExport and PyABIInfo APIs#5746
Update FFI wrappers for PyModExport and PyABIInfo APIs#5746davidhewitt merged 13 commits intoPyO3:mainfrom
Conversation
|
I'm going to add uses of these new APIs in the two FFI examples, which should fix the coverage failure. |
|
The ABI check just caught me failing to clear stale build artifacts in |
| abiinfo_minor_version: 0, | ||
| flags: PyABIInfo_DEFAULT_FLAGS, | ||
| build_version: 0, | ||
| abi_version: 0, |
There was a problem hiding this comment.
Technically it could be possible for us to expose these. However, for expediency I'm just setting them to zero. Both flags support this.
For build_version we could probably expose something like Py_VERSION_HEX that follows our Py_3_X conditional compilation flags. Technically that's not supported but @encukou suggested to update the spec to allow this: python/cpython#143884 (comment)
For abi_version I think we can already do that, at least for limited API builds? But it's a bit tricky and I'd like to make progress elsewhere rather than focusing on this.
I can create a followup issue to describe the situation.
| abiinfo_minor_version: 0, | ||
| flags: PyABIInfo_DEFAULT_FLAGS, | ||
| build_version: 0, | ||
| abi_version: 0, |
davidhewitt
left a comment
There was a problem hiding this comment.
Actually, I think the PyModuleDef_Base change might need reverting?
| pub m_init: Option<extern "C" fn() -> *mut PyObject>, | ||
| pub m_init: *mut PyObject, | ||
| pub m_index: Py_ssize_t, | ||
| pub m_copy: *mut PyObject, | ||
| } | ||
|
|
||
| #[allow( | ||
| clippy::declare_interior_mutable_const, | ||
| reason = "contains atomic refcount on free-threaded builds" | ||
| )] | ||
| pub const PyModuleDef_HEAD_INIT: PyModuleDef_Base = PyModuleDef_Base { | ||
| ob_base: PyObject_HEAD_INIT, | ||
| m_init: None, |
There was a problem hiding this comment.
I think this change is not correct. The C definition of m_init is
PyObject* (*m_init)(void);which I believe is a function pointer taking no arguments and returning *mut PyObject. So extern "C" fn() -> *mut PyObject is the right function pointer description, however Rust function pointers are non-null so they need wrapping in the Option to handle the null case.
There was a problem hiding this comment.
Maybe some code to convince you of this:
use std::mem::transmute;
fn main() {
dbg!(std::mem::size_of::<
extern "C" fn() -> *mut pyo3::ffi::PyObject,
>());
dbg!(std::mem::size_of::<
Option<extern "C" fn() -> *mut pyo3::ffi::PyObject>,
>());
let _null_fn_ptr: Option<extern "C" fn() -> *mut pyo3::ffi::PyObject> = None;
dbg!(unsafe { transmute::<_, *mut std::ffi::c_void>(_null_fn_ptr) });
}results in
[src/main.rs:4:5] std::mem::size_of::< extern "C" fn() -> *mut pyo3::ffi::PyObject, >() = 8
[src/main.rs:7:5] std::mem::size_of::< Option<extern "C" fn() -> *mut pyo3::ffi::PyObject>, >() = 8
[src/main.rs:13:5] unsafe { transmute::<_, *mut std::ffi::c_void>(_null_fn_ptr) } = 0x0000000000000000
There was a problem hiding this comment.
Thanks for the explanation and for catching this! For what it's worth I originally intended to call this out for special attention but it slipped my mind.
I'll add a comment explaining so future readers don't make the same mistake I did.
There was a problem hiding this comment.
I completely misread and misunderstood the signature of m_init in moduleobject.h. Thanks again for catching this!
baef384 to
2358c16
Compare
clin1234
left a comment
There was a problem hiding this comment.
After these changes, LGTM
Towards fixing #5644.
See https://docs.python.org/3.15/c-api/stable.html#abi-checking and https://docs.python.org/3.15/c-api/extension-modules.html#extension-export-hook for upstream docs for the newly-wrapped APIs.
I moved the
PyModule_Createdefinition to where it lives now in the upstream headers.I also fixed a small issue I noticed in the definition of
PyModuleDef_Base.m_initshouldn't be using anOptionbecause empty options aren't NULL, andm_initmight be NULL in some situations. Better to just model it exactly as upstream defines it:*mut PyObject.