Skip to content

feat: change PYBIND11_MODULE to use multi-phase init (PEP 489) #5574

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 21, 2025
+104 −17
Merged

feat: change PYBIND11_MODULE to use multi-phase init (PEP 489) #5574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
335d8d4
Change PYBIND11_MODULE to use multi-phase init
b-pass Mar 20, 2025
1dc9900
Avoid dynamic allocation and non-trivial destruction
b-pass Mar 20, 2025
422b08f
Oops, stray cut and paste character
b-pass Mar 20, 2025
554e1b2
Remove assignment from placement new, change size fo slots array
b-pass Mar 21, 2025
850c587
style: pre-commit fixes
pre-commit-ci[bot] Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,11 +372,9 @@
#define PYBIND11_CATCH_INIT_EXCEPTIONS \
catch (pybind11::error_already_set & e) { \
pybind11::raise_from(e, PyExc_ImportError, "initialization failed"); \
return nullptr; \
} \
catch (const std::exception &e) { \
::pybind11::set_error(PyExc_ImportError, e.what()); \
return nullptr; \
}

/** \rst
Expand Down Expand Up @@ -404,6 +402,7 @@
return pybind11_init(); \
} \
PYBIND11_CATCH_INIT_EXCEPTIONS \
return nullptr; \
} \
PyObject *pybind11_init()

Expand Down Expand Up @@ -447,23 +446,33 @@
PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_CLANG("-Wgnu-zero-variadic-macro-arguments")
#define PYBIND11_MODULE(name, variable, ...) \
static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name) \
PYBIND11_MAYBE_UNUSED; \
PYBIND11_MAYBE_UNUSED \
static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name); \
static std::vector<PyModuleDef_Slot> PYBIND11_CONCAT(pybind11_module_slots_, name); \
Copy link
Collaborator

@rwgk rwgk Mar 20, 2025

Choose a reason for hiding this comment

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

Slight worries: Could we get into trouble during process teardown?

Concretely, could there be a situation in which slots.data() is referenced after this static variable was deleted already?

Probably not ... but maybe?

I'd want to err on the side of "abundance of caution".

I think we should use the standard trick of leaking a pointer. It's really simple, all doubts removed. Untested:

    static auto *PYBIND11_CONCAT(pybind11_module_slots_, name) = new std::vector<PyModuleDef_Slot>();

Copy link
Collaborator

@henryiii henryiii Mar 20, 2025

Choose a reason for hiding this comment

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

I'd prefer not to leak a pointer unless we are sure it is needed. Isn't this already the lifetime of the module, though?

Copy link
Contributor Author

@b-pass b-pass Mar 20, 2025

Choose a reason for hiding this comment

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

Yes, this is a static global for the lifetime of the module... it ends up referenced inside the initialized PyModuleDef, which is defined right above it. So I don't think there's any way that the PyModuleDef could outlive it.

Copy link
Collaborator

@rwgk rwgk Mar 20, 2025

Choose a reason for hiding this comment

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

It's standard best practice for non-POD static objects.

There is no good reason for taking any risks (running destructors during process teardown; or freeing in the wrong order). The memory will be freed by the kernel for sure.

Copy link
Contributor Author

@b-pass b-pass Mar 20, 2025

Choose a reason for hiding this comment

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

Ok, I switched it to an std::array of slots, which is POD (since PyModuleDef_Slot is POD), so it has no destructor.... it's more along the lines of the original suggestion of using a named constant instead of a magic number for the size.... the size is part of the typedef (with a comment, ofc).

static int PYBIND11_CONCAT(pybind11_exec_, name)(PyObject *); \
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
PYBIND11_PLUGIN_IMPL(name) { \
PYBIND11_CHECK_PYTHON_VERSION \
PYBIND11_ENSURE_INTERNALS_READY \
auto m = ::pybind11::module_::create_extension_module( \
auto &slots = PYBIND11_CONCAT(pybind11_module_slots_, name); \
slots.clear(); \
slots.emplace_back(PyModuleDef_Slot{ \
Py_mod_exec, reinterpret_cast<void *>(&PYBIND11_CONCAT(pybind11_exec_, name))}); \
auto m = ::pybind11::module_::initialize_multiphase_module_def( \
PYBIND11_TOSTRING(name), \
nullptr, \
&PYBIND11_CONCAT(pybind11_module_def_, name), \
slots, \
##__VA_ARGS__); \
return m.ptr(); \
} \
int PYBIND11_CONCAT(pybind11_exec_, name)(PyObject * pm) { \
try { \
auto m = pybind11::reinterpret_borrow<::pybind11::module_>(pm); \
PYBIND11_CONCAT(pybind11_init_, name)(m); \
return m.ptr(); \
return 0; \
} \
PYBIND11_CATCH_INIT_EXCEPTIONS \
return -1; \
} \
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ & (variable))
PYBIND11_WARNING_POP
Expand Down
1 change: 1 addition & 0 deletions include/pybind11/embed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
return m.ptr(); \
} \
PYBIND11_CATCH_INIT_EXCEPTIONS \
return nullptr; \
} \
PYBIND11_EMBEDDED_MODULE_IMPL(name) \
::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name)( \
Expand Down
66 changes: 66 additions & 0 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,6 +1255,22 @@ class mod_gil_not_used {
bool flag_;
};

PYBIND11_NAMESPACE_BEGIN(detail)

inline bool gil_not_used_option() { return false; }
template <typename F, typename... O>
bool gil_not_used_option(F &&, O &&...o);
template <typename... O>
inline bool gil_not_used_option(mod_gil_not_used f, O &&...o) {
return f.flag() || gil_not_used_option(o...);
}
template <typename F, typename... O>
inline bool gil_not_used_option(F &&, O &&...o) {
return gil_not_used_option(o...);
}

PYBIND11_NAMESPACE_END(detail)

/// Wrapper for Python extension modules
class module_ : public object {
public:
Expand Down Expand Up @@ -1389,6 +1405,56 @@ class module_ : public object {
// For Python 2, reinterpret_borrow was correct.
return reinterpret_borrow<module_>(m);
}

/** \rst
Initialized a module def for use with multi-phase module initialization.

``def`` should point to a statically allocated module_def.
``slots`` must already contain a Py_mod_exec or Py_mod_create slot and will be filled with
additional slots (and the empty terminator slot) from the supplied options.
\endrst */
template <typename... Options>
static object initialize_multiphase_module_def(const char *name,
const char *doc,
module_def *def,
std::vector<PyModuleDef_Slot> &slots,
Options &&...options) {
// remove zero end sentinel, if present
while (!slots.empty() && slots.back().slot == 0) {
slots.pop_back();
}

bool nogil PYBIND11_MAYBE_UNUSED = detail::gil_not_used_option(options...);
if (nogil) {
#if defined(Py_mod_gil) && defined(Py_GIL_DISABLED)
slots.emplace_back(PyModuleDef_Slot{Py_mod_gil, Py_MOD_GIL_NOT_USED});
#endif
}

// must have a zero end sentinel
slots.emplace_back(PyModuleDef_Slot{0, nullptr});

// module_def is PyModuleDef
// Placement new (not an allocation).
def = new (def)
PyModuleDef{/* m_base */ PyModuleDef_HEAD_INIT,
/* m_name */ name,
/* m_doc */ options::show_user_defined_docstrings() ? doc : nullptr,
/* m_size */ 0,
/* m_methods */ nullptr,
/* m_slots */ slots.data(),
/* m_traverse */ nullptr,
/* m_clear */ nullptr,
/* m_free */ nullptr};
auto *m = PyModuleDef_Init(def);
if (m == nullptr) {
if (PyErr_Occurred()) {
throw error_already_set();
}
pybind11_fail("Internal error in module_::initialize_multiphase_module_def()");
}
return reinterpret_borrow<object>(m);
}
};

PYBIND11_NAMESPACE_BEGIN(detail)
Expand Down
Loading