From e09ea59f07e597b0f504502d69c2b98ae449c1a3 Mon Sep 17 00:00:00 2001 From: StarQTius Date: Sat, 12 Mar 2022 19:44:01 +0100 Subject: [PATCH 1/2] Make non-embedded module clear their locals before deallocation --- include/pybind11/detail/internals.h | 8 +++++++ include/pybind11/embed.h | 5 ++-- include/pybind11/pybind11.h | 34 +++++++++++++++++++++++++++- tests/test_embed/external_module.cpp | 3 +++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 3f83113bd7..5337e5eeb9 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -510,11 +510,19 @@ struct local_internals { }; /// Works like `get_internals`, but for things which are locally registered. +/// There is one variable storing the local internals for each module, but every embedded module +/// shares the same locals. inline local_internals &get_local_internals() { static local_internals locals; return locals; } +/// Clears the locally registered types and exception translators. +inline void clear_local_internals() { + detail::get_local_internals().registered_types_cpp.clear(); + detail::get_local_internals().registered_exception_translators.clear(); +} + /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its /// `c_str()`. Such strings objects have a long storage duration -- the internal strings are only /// cleared when the program exits or after interpreter shutdown (when embedding), and so are diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 98cc3e4664..204c402893 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -42,7 +42,7 @@ static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name); \ static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ - auto m = ::pybind11::module_::create_extension_module( \ + auto m = ::pybind11::module_::create_embedded_extension_module( \ PYBIND11_TOSTRING(name), nullptr, &PYBIND11_CONCAT(pybind11_module_def_, name)); \ try { \ PYBIND11_CONCAT(pybind11_init_, name)(m); \ @@ -200,8 +200,7 @@ inline void finalize_interpreter() { } // Local internals contains data managed by the current interpreter, so we must clear them to // avoid undefined behaviors when initializing another interpreter - detail::get_local_internals().registered_types_cpp.clear(); - detail::get_local_internals().registered_exception_translators.clear(); + detail::clear_local_internals(); Py_Finalize(); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 422d534c12..4181be4183 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1232,7 +1232,7 @@ class module_ : public object { /* m_slots */ nullptr, /* m_traverse */ nullptr, /* m_clear */ nullptr, - /* m_free */ nullptr}; + /* m_free */ [](void *) { detail::clear_local_internals(); }}; auto *m = PyModule_Create(def); if (m == nullptr) { if (PyErr_Occurred()) { @@ -1245,6 +1245,38 @@ class module_ : public object { // For Python 2, reinterpret_borrow was correct. return reinterpret_borrow(m); } + + /** \rst + Create a new top-level embedded module. + + ``def`` should point to a statically allocated module_def. + \endrst */ + static module_ + create_embedded_extension_module(const char *name, const char *doc, module_def *def) { + // 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 */ -1, + /* m_methods */ nullptr, + /* m_slots */ nullptr, + /* m_traverse */ nullptr, + /* m_clear */ nullptr, + /* m_free */ nullptr}; + auto *m = PyModule_Create(def); + if (m == nullptr) { + if (PyErr_Occurred()) { + throw error_already_set(); + } + pybind11_fail("Internal error in module_::create_embedded_extension_module()"); + } + // TODO: Should be reinterpret_steal for Python 3, but Python also steals it again when + // returned from PyInit_... + // For Python 2, reinterpret_borrow was correct. + return reinterpret_borrow(m); + } }; // When inside a namespace (or anywhere as long as it's not the first item on a line), diff --git a/tests/test_embed/external_module.cpp b/tests/test_embed/external_module.cpp index 5c482fe061..c423bc877b 100644 --- a/tests/test_embed/external_module.cpp +++ b/tests/test_embed/external_module.cpp @@ -13,7 +13,10 @@ PYBIND11_MODULE(external_module, m) { int v; }; + class B {}; + py::class_(m, "A").def(py::init()).def_readwrite("value", &A::v); + py::class_(m, "B", py::module_local()); m.def("internals_at", []() { return reinterpret_cast(&py::detail::get_internals()); }); From 85a89fe5fb8a10f77855a164d039a3c0cebac9e5 Mon Sep 17 00:00:00 2001 From: StarQTius Date: Thu, 17 Mar 2022 11:42:54 +0100 Subject: [PATCH 2/2] Add create_extension_module_impl() & clarify testing purpose --- include/pybind11/embed.h | 7 +- include/pybind11/pybind11.h | 93 ++++++++++++--------------- tests/test_embed/external_module.cpp | 4 ++ tests/test_embed/test_interpreter.cpp | 1 + 4 files changed, 51 insertions(+), 54 deletions(-) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 204c402893..73371b5c0f 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -42,8 +42,11 @@ static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name); \ static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ - auto m = ::pybind11::module_::create_embedded_extension_module( \ - PYBIND11_TOSTRING(name), nullptr, &PYBIND11_CONCAT(pybind11_module_def_, name)); \ + auto m = ::pybind11::detail::create_extension_module_impl( \ + PYBIND11_TOSTRING(name), \ + nullptr, \ + &PYBIND11_CONCAT(pybind11_module_def_, name), \ + true); \ try { \ PYBIND11_CONCAT(pybind11_init_, name)(m); \ return m.ptr(); \ diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4181be4183..b0241597fc 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1127,6 +1127,14 @@ class cpp_function : public function { } }; +class module_; + +PYBIND11_NAMESPACE_BEGIN(detail) + +module_ create_extension_module_impl(const char *, const char *, PyModuleDef *, bool); + +PYBIND11_NAMESPACE_END(detail) + /// Wrapper for Python extension modules class module_ : public object { public: @@ -1221,63 +1229,44 @@ class module_ : public object { ``def`` should point to a statically allocated module_def. \endrst */ static module_ create_extension_module(const char *name, const char *doc, module_def *def) { - // 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 */ -1, - /* m_methods */ nullptr, - /* m_slots */ nullptr, - /* m_traverse */ nullptr, - /* m_clear */ nullptr, - /* m_free */ [](void *) { detail::clear_local_internals(); }}; - auto *m = PyModule_Create(def); - if (m == nullptr) { - if (PyErr_Occurred()) { - throw error_already_set(); - } - pybind11_fail("Internal error in module_::create_extension_module()"); - } - // TODO: Should be reinterpret_steal for Python 3, but Python also steals it again when - // returned from PyInit_... - // For Python 2, reinterpret_borrow was correct. - return reinterpret_borrow(m); + return detail::create_extension_module_impl(name, doc, def, false); } +}; - /** \rst - Create a new top-level embedded module. +PYBIND11_NAMESPACE_BEGIN(detail) - ``def`` should point to a statically allocated module_def. - \endrst */ - static module_ - create_embedded_extension_module(const char *name, const char *doc, module_def *def) { - // 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 */ -1, - /* m_methods */ nullptr, - /* m_slots */ nullptr, - /* m_traverse */ nullptr, - /* m_clear */ nullptr, - /* m_free */ nullptr}; - auto *m = PyModule_Create(def); - if (m == nullptr) { - if (PyErr_Occurred()) { - throw error_already_set(); - } - pybind11_fail("Internal error in module_::create_embedded_extension_module()"); +// Initializes a 'PyModuleDef' instance and create an extension module out of it. +inline module_ create_extension_module_impl(const char *name, + const char *doc, + PyModuleDef *def, + bool is_embedded) { + // 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 */ -1, + /* m_methods */ nullptr, + /* m_slots */ nullptr, + /* m_traverse */ nullptr, + /* m_clear */ nullptr, + // Non-embedded modules must clean their own locals since they can only be accessed by them + /* m_free */ is_embedded ? nullptr : +[](void *) { detail::clear_local_internals(); }}; + auto *m = PyModule_Create(def); + if (m == nullptr) { + if (PyErr_Occurred()) { + throw error_already_set(); } - // TODO: Should be reinterpret_steal for Python 3, but Python also steals it again when - // returned from PyInit_... - // For Python 2, reinterpret_borrow was correct. - return reinterpret_borrow(m); + pybind11_fail("Internal error in module_::create_extension_module_impl()"); } -}; + // TODO: Should be reinterpret_steal for Python 3, but Python also steals it again when + // returned from PyInit_... + // For Python 2, reinterpret_borrow was correct. + return reinterpret_borrow(m); +} + +PYBIND11_NAMESPACE_END(detail) // When inside a namespace (or anywhere as long as it's not the first item on a line), // C++20 allows "module" to be used. This is provided for backward compatibility, and for diff --git a/tests/test_embed/external_module.cpp b/tests/test_embed/external_module.cpp index c423bc877b..78b20c6a07 100644 --- a/tests/test_embed/external_module.cpp +++ b/tests/test_embed/external_module.cpp @@ -16,6 +16,10 @@ PYBIND11_MODULE(external_module, m) { class B {}; py::class_(m, "A").def(py::init()).def_readwrite("value", &A::v); + + // PR #3798 : Local internals must be cleared on finalization so they can be registered again. + // This class is not explicitly used in the test, but it is still registered / unregistered + // when the interpreter stops and restarts. py::class_(m, "B", py::module_local()); m.def("internals_at", diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 1c45457a05..232c984ac7 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -184,6 +184,7 @@ TEST_CASE("Restart the interpreter") { == py::module_::import("external_module").attr("internals_at")().cast()); // Restart the interpreter. + // PR #3798 : Local internals must be cleared on finalization so they can be registered again. py::finalize_interpreter(); REQUIRE(Py_IsInitialized() == 0);