Skip to content

Conversation

@ngoldbaum
Copy link
Contributor

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_Create definition to where it lives now in the upstream headers.

I also fixed a small issue I noticed in the definition of PyModuleDef_Base. m_init shouldn't be using an Option because empty options aren't NULL, and m_init might be NULL in some situations. Better to just model it exactly as upstream defines it: *mut PyObject.

@ngoldbaum
Copy link
Contributor Author

I'm going to add uses of these new APIs in the two FFI examples, which should fix the coverage failure.

@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Jan 19, 2026

The ABI check just caught me failing to clear stale build artifacts in string_sum on my local copy. That'll definitely be useful for me given Cargo's inability to detect a Python symlink changing out from under it and my heavy use of pyenv for my work.

abiinfo_minor_version: 0,
flags: PyABIInfo_DEFAULT_FLAGS,
build_version: 0,
abi_version: 0,
Copy link
Contributor Author

@ngoldbaum ngoldbaum Jan 22, 2026

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me 👍

Copy link
Member

@davidhewitt davidhewitt left a 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me 👍

Copy link
Member

@davidhewitt davidhewitt left a 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?

Comment on lines 58 to 69
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,
Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

@ngoldbaum ngoldbaum requested a review from davidhewitt January 23, 2026 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants