-
Notifications
You must be signed in to change notification settings - Fork 927
Update FFI wrappers for PyModExport and PyABIInfo APIs #5746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me 👍
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
| abiinfo_minor_version: 0, | ||
| flags: PyABIInfo_DEFAULT_FLAGS, | ||
| build_version: 0, | ||
| abi_version: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me 👍
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely misread and misunderstood the signature of m_init in moduleobject.h. Thanks again for catching this!
baef384 to
2358c16
Compare
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.