From 3b1472174e8b8e7756259126cfd9c65f73d577d9 Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Fri, 28 Oct 2022 11:46:52 +0200 Subject: [PATCH 01/10] stash pybind11 data structures in interpreter state dictionary --- include/pybind11/detail/internals.h | 35 +++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index d47084e263..37ed711d5a 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -277,6 +277,12 @@ struct type_info { # endif #endif +#if PY_VERSION_HEX < 0x03090000 +# define PYBIND11_INTERPRETER_STATE_GET _PyInterpreterState_Get +#else +# define PYBIND11_INTERPRETER_STATE_GET PyInterpreterState_Get +#endif + #define PYBIND11_INTERNALS_ID \ "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ @@ -403,7 +409,7 @@ inline void translate_local_exception(std::exception_ptr p) { /// Return a reference to the current `internals` data PYBIND11_NOINLINE internals &get_internals() { - auto **&internals_pp = get_internals_pp(); + internals **&internals_pp = get_internals_pp(); if (internals_pp && *internals_pp) { return **internals_pp; } @@ -419,11 +425,24 @@ PYBIND11_NOINLINE internals &get_internals() { } gil; error_scope err_scope; - PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID); - auto builtins = handle(PyEval_GetBuiltins()); - if (builtins.contains(id) && isinstance(builtins[id])) { - internals_pp = static_cast(capsule(builtins[id])); + const char *id_cstr = PYBIND11_INTERNALS_ID; + PYBIND11_STR_TYPE id(id_cstr); + + dict state_dict + = reinterpret_borrow(PyInterpreterState_GetDict(PYBIND11_INTERPRETER_STATE_GET())); + if (!state_dict) + pybind11_fail("get_internals(): PyInterpreterState_GetDict() failed!"); + + if (state_dict.contains(id_cstr)) { + object o = state_dict[id]; + // May fail if 'capsule_obj' is not a capsule, or if it has a different + // name. We clear the error status below in that case + internals_pp = static_cast(PyCapsule_GetPointer(o.ptr(), id_cstr)); + if (!internals_pp) + PyErr_Clear(); + } + if (internals_pp) { // We loaded builtins through python's builtins, which means that our `error_already_set` // and `builtin_exception` may be different local classes than the ones set up in the // initial exception translator, below, so add another for our local exception classes. @@ -435,9 +454,7 @@ PYBIND11_NOINLINE internals &get_internals() { (*internals_pp)->registered_exception_translators.push_front(&translate_local_exception); #endif } else { - if (!internals_pp) { - internals_pp = new internals *(); - } + internals_pp = new internals *(); auto *&internals_ptr = *internals_pp; internals_ptr = new internals(); #if defined(WITH_THREAD) @@ -459,7 +476,7 @@ PYBIND11_NOINLINE internals &get_internals() { # endif internals_ptr->istate = tstate->interp; #endif - builtins[id] = capsule(internals_pp); + state_dict[id] = capsule(internals_pp, id_cstr); internals_ptr->registered_exception_translators.push_front(&translate_exception); internals_ptr->static_property_type = make_static_property_type(); internals_ptr->default_metaclass = make_default_metaclass(); From 7a944063e1c4ea4dd35a89ccfc951c2c89553c1a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Oct 2022 09:52:23 +0000 Subject: [PATCH 02/10] style: pre-commit fixes --- include/pybind11/detail/internals.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 37ed711d5a..645109ed77 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -278,9 +278,9 @@ struct type_info { #endif #if PY_VERSION_HEX < 0x03090000 -# define PYBIND11_INTERPRETER_STATE_GET _PyInterpreterState_Get +# define PYBIND11_INTERPRETER_STATE_GET _PyInterpreterState_Get #else -# define PYBIND11_INTERPRETER_STATE_GET PyInterpreterState_Get +# define PYBIND11_INTERPRETER_STATE_GET PyInterpreterState_Get #endif #define PYBIND11_INTERNALS_ID \ From 3a89f671078831d7388cc89285a28e1def8c8432 Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Mon, 31 Oct 2022 21:27:48 +0100 Subject: [PATCH 03/10] Make changes specific to Python 3.8+, incorporate feedback --- include/pybind11/detail/internals.h | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 645109ed77..4929a68cee 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -277,12 +277,6 @@ struct type_info { # endif #endif -#if PY_VERSION_HEX < 0x03090000 -# define PYBIND11_INTERPRETER_STATE_GET _PyInterpreterState_Get -#else -# define PYBIND11_INTERPRETER_STATE_GET PyInterpreterState_Get -#endif - #define PYBIND11_INTERNALS_ID \ "__pybind11_internals_v" PYBIND11_TOSTRING(PYBIND11_INTERNALS_VERSION) \ PYBIND11_INTERNALS_KIND PYBIND11_COMPILER_TYPE PYBIND11_STDLIB PYBIND11_BUILD_ABI \ @@ -425,21 +419,30 @@ PYBIND11_NOINLINE internals &get_internals() { } gil; error_scope err_scope; - const char *id_cstr = PYBIND11_INTERNALS_ID; - PYBIND11_STR_TYPE id(id_cstr); + constexpr const char *id_cstr = PYBIND11_INTERNALS_ID; + str id(id_cstr); + + dict state_dict; +#if PY_VERSION_HEX < 0x03080000 + state_dict = reinterpret_borrow(PyEval_GetBuiltins()); +#elif PY_VERSION_HEX < 0x03090000 + state_dict = reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); +#else + state_dict = reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); +#endif - dict state_dict - = reinterpret_borrow(PyInterpreterState_GetDict(PYBIND11_INTERPRETER_STATE_GET())); - if (!state_dict) - pybind11_fail("get_internals(): PyInterpreterState_GetDict() failed!"); + if (!state_dict) { + pybind11_fail("get_internals(): could not acquire state dictionary!"); + } if (state_dict.contains(id_cstr)) { object o = state_dict[id]; // May fail if 'capsule_obj' is not a capsule, or if it has a different // name. We clear the error status below in that case internals_pp = static_cast(PyCapsule_GetPointer(o.ptr(), id_cstr)); - if (!internals_pp) + if (!internals_pp) { PyErr_Clear(); + } } if (internals_pp) { From daf768aeb7e1c0708922204e4d6e11923fbee3be Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Mon, 31 Oct 2022 22:06:56 +0100 Subject: [PATCH 04/10] fix segfault --- include/pybind11/detail/internals.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 4929a68cee..b98b2bc8e4 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -445,7 +445,7 @@ PYBIND11_NOINLINE internals &get_internals() { } } - if (internals_pp) { + if (internals_pp && *internals_pp) { // We loaded builtins through python's builtins, which means that our `error_already_set` // and `builtin_exception` may be different local classes than the ones set up in the // initial exception translator, below, so add another for our local exception classes. @@ -457,7 +457,9 @@ PYBIND11_NOINLINE internals &get_internals() { (*internals_pp)->registered_exception_translators.push_front(&translate_local_exception); #endif } else { - internals_pp = new internals *(); + if (!internals_pp) { + internals_pp = new internals *(); + } auto *&internals_ptr = *internals_pp; internals_ptr = new internals(); #if defined(WITH_THREAD) From 8df2e819d271790778419f731567852e92a9b771 Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Wed, 2 Nov 2022 15:28:00 +0100 Subject: [PATCH 05/10] fix C++ test --- tests/test_embed/test_interpreter.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 6299293b91..85e11ba627 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -169,8 +169,15 @@ TEST_CASE("There can be only one interpreter") { } bool has_pybind11_internals_builtin() { - auto builtins = py::handle(PyEval_GetBuiltins()); - return builtins.contains(PYBIND11_INTERNALS_ID); + py::dict state_dict; +#if PY_VERSION_HEX < 0x03080000 + state_dict = py::reinterpret_borrow(PyEval_GetBuiltins()); +#elif PY_VERSION_HEX < 0x03090000 + state_dict = py::reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); +#else + state_dict = py::reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); +#endif + return state_dict.contains(PYBIND11_INTERNALS_ID); }; bool has_pybind11_internals_static() { From f4e6c7833e816fd5a43daccdc133cc6e548d88ce Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 2 Nov 2022 14:29:06 +0000 Subject: [PATCH 06/10] style: pre-commit fixes --- tests/test_embed/test_interpreter.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 85e11ba627..7b911b2b8c 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -173,9 +173,11 @@ bool has_pybind11_internals_builtin() { #if PY_VERSION_HEX < 0x03080000 state_dict = py::reinterpret_borrow(PyEval_GetBuiltins()); #elif PY_VERSION_HEX < 0x03090000 - state_dict = py::reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); + state_dict + = py::reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); #else - state_dict = py::reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); + state_dict + = py::reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); #endif return state_dict.contains(PYBIND11_INTERNALS_ID); }; From cc03a5651239acd9db0f952aa64860d31e48ddeb Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Wed, 2 Nov 2022 15:44:19 +0100 Subject: [PATCH 07/10] continue to use PyEval_GetBuiltins for PyPy --- include/pybind11/detail/internals.h | 2 +- tests/test_embed/test_interpreter.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index b98b2bc8e4..6049791390 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -423,7 +423,7 @@ PYBIND11_NOINLINE internals &get_internals() { str id(id_cstr); dict state_dict; -#if PY_VERSION_HEX < 0x03080000 +#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) state_dict = reinterpret_borrow(PyEval_GetBuiltins()); #elif PY_VERSION_HEX < 0x03090000 state_dict = reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index 7b911b2b8c..afcb4fc958 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -170,7 +170,7 @@ TEST_CASE("There can be only one interpreter") { bool has_pybind11_internals_builtin() { py::dict state_dict; -#if PY_VERSION_HEX < 0x03080000 +#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) state_dict = py::reinterpret_borrow(PyEval_GetBuiltins()); #elif PY_VERSION_HEX < 0x03090000 state_dict From 780c6ff9c89efe0465d003857923e3c340de3ca7 Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Wed, 2 Nov 2022 22:36:17 +0100 Subject: [PATCH 08/10] incorporated feedback --- include/pybind11/detail/internals.h | 33 +++++++++++++++++---------- tests/test_embed/test_interpreter.cpp | 28 ++++++++--------------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 6049791390..93cf246ceb 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -401,6 +401,26 @@ inline void translate_local_exception(std::exception_ptr p) { } #endif +inline object get_internals_state_dict() { + object state_dict; +#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) + state_dict = reinterpret_borrow(PyEval_GetBuiltins()); +#else +# if PY_VERSION_HEX < 0x03090000 + PyInterpreterState *istate = _PyInterpreterState_Get(); +#else + PyInterpreterState *istate = PyInterpreterState_Get(); +#endif + if (istate) + state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); +#endif + if (!state_dict) { + raise_from(PyExc_SystemError, "get_internals(): could not acquire state dictionary!"); + } + + return state_dict; +} + /// Return a reference to the current `internals` data PYBIND11_NOINLINE internals &get_internals() { internals **&internals_pp = get_internals_pp(); @@ -422,18 +442,7 @@ PYBIND11_NOINLINE internals &get_internals() { constexpr const char *id_cstr = PYBIND11_INTERNALS_ID; str id(id_cstr); - dict state_dict; -#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) - state_dict = reinterpret_borrow(PyEval_GetBuiltins()); -#elif PY_VERSION_HEX < 0x03090000 - state_dict = reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); -#else - state_dict = reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); -#endif - - if (!state_dict) { - pybind11_fail("get_internals(): could not acquire state dictionary!"); - } + dict state_dict = get_internals_state_dict(); if (state_dict.contains(id_cstr)) { object o = state_dict[id]; diff --git a/tests/test_embed/test_interpreter.cpp b/tests/test_embed/test_interpreter.cpp index afcb4fc958..a363bcbeeb 100644 --- a/tests/test_embed/test_interpreter.cpp +++ b/tests/test_embed/test_interpreter.cpp @@ -168,18 +168,8 @@ TEST_CASE("There can be only one interpreter") { py::initialize_interpreter(); } -bool has_pybind11_internals_builtin() { - py::dict state_dict; -#if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) - state_dict = py::reinterpret_borrow(PyEval_GetBuiltins()); -#elif PY_VERSION_HEX < 0x03090000 - state_dict - = py::reinterpret_borrow(PyInterpreterState_GetDict(_PyInterpreterState_Get())); -#else - state_dict - = py::reinterpret_borrow(PyInterpreterState_GetDict(PyInterpreterState_Get())); -#endif - return state_dict.contains(PYBIND11_INTERNALS_ID); +bool has_pybind11_internals_state_dict() { + return py::detail::get_internals_state_dict().contains(PYBIND11_INTERNALS_ID); }; bool has_pybind11_internals_static() { @@ -190,7 +180,7 @@ bool has_pybind11_internals_static() { TEST_CASE("Restart the interpreter") { // Verify pre-restart state. REQUIRE(py::module_::import("widget_module").attr("add")(1, 2).cast() == 3); - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_state_dict()); REQUIRE(has_pybind11_internals_static()); REQUIRE(py::module_::import("external_module").attr("A")(123).attr("value").cast() == 123); @@ -207,10 +197,10 @@ TEST_CASE("Restart the interpreter") { REQUIRE(Py_IsInitialized() == 1); // Internals are deleted after a restart. - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_state_dict()); REQUIRE_FALSE(has_pybind11_internals_static()); pybind11::detail::get_internals(); - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_state_dict()); REQUIRE(has_pybind11_internals_static()); REQUIRE(reinterpret_cast(*py::detail::get_internals_pp()) == py::module_::import("external_module").attr("internals_at")().cast()); @@ -225,13 +215,13 @@ TEST_CASE("Restart the interpreter") { py::detail::get_internals(); *static_cast(ran) = true; }); - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_state_dict()); REQUIRE_FALSE(has_pybind11_internals_static()); REQUIRE_FALSE(ran); py::finalize_interpreter(); REQUIRE(ran); py::initialize_interpreter(); - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_state_dict()); REQUIRE_FALSE(has_pybind11_internals_static()); // C++ modules can be reloaded. @@ -253,7 +243,7 @@ TEST_CASE("Subinterpreter") { REQUIRE(m.attr("add")(1, 2).cast() == 3); } - REQUIRE(has_pybind11_internals_builtin()); + REQUIRE(has_pybind11_internals_state_dict()); REQUIRE(has_pybind11_internals_static()); /// Create and switch to a subinterpreter. @@ -263,7 +253,7 @@ TEST_CASE("Subinterpreter") { // Subinterpreters get their own copy of builtins. detail::get_internals() still // works by returning from the static variable, i.e. all interpreters share a single // global pybind11::internals; - REQUIRE_FALSE(has_pybind11_internals_builtin()); + REQUIRE_FALSE(has_pybind11_internals_state_dict()); REQUIRE(has_pybind11_internals_static()); // Modules tags should be gone. From 39c21a36e4aaa4c87d59509b1b4c5303bdc0bb7f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 2 Nov 2022 21:36:59 +0000 Subject: [PATCH 09/10] style: pre-commit fixes --- include/pybind11/detail/internals.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 93cf246ceb..8abecb8c5c 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -406,11 +406,11 @@ inline object get_internals_state_dict() { #if PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION) state_dict = reinterpret_borrow(PyEval_GetBuiltins()); #else -# if PY_VERSION_HEX < 0x03090000 +# if PY_VERSION_HEX < 0x03090000 PyInterpreterState *istate = _PyInterpreterState_Get(); -#else +# else PyInterpreterState *istate = PyInterpreterState_Get(); -#endif +# endif if (istate) state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); #endif From 78e074b8bbda1073e60273620e084aac28b0740c Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Wed, 2 Nov 2022 23:17:46 +0100 Subject: [PATCH 10/10] clang-tidy fix --- include/pybind11/detail/internals.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 8abecb8c5c..03c105063d 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -411,8 +411,9 @@ inline object get_internals_state_dict() { # else PyInterpreterState *istate = PyInterpreterState_Get(); # endif - if (istate) + if (istate) { state_dict = reinterpret_borrow(PyInterpreterState_GetDict(istate)); + } #endif if (!state_dict) { raise_from(PyExc_SystemError, "get_internals(): could not acquire state dictionary!");