Skip to content
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

Introduce get_python_state_dict() for Python 3.12 compatibility. #4570

Merged
merged 6 commits into from
Mar 28, 2023
Merged

Introduce get_python_state_dict() for Python 3.12 compatibility. #4570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12ef776
Introduce `get_python_state_dict()`
Mar 14, 2023
1c5bc85
Conditional version bump for Python 3.12+
Mar 26, 2023
e8b34b4
Shuffle subexpressions to make the condition easier to understand (no…
Mar 26, 2023
e23ddd8
Make pybind11 ABI version 5 the minimum for Python 3.12+ (as suggeste…
Mar 27, 2023
5d4716a
Add back condition for PYPY_VERSION, but keep it open for future PyPy…
Mar 27, 2023
7729612
Fall back to simple `|| defined(PYPY_VERSION)`. `PY_VERSION_HEX` does…
Mar 27, 2023
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
57 changes: 49 additions & 8 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,18 @@
/// further ABI-incompatible changes may be made before the ABI is officially
/// changed to the new version.
#ifndef PYBIND11_INTERNALS_VERSION
# define PYBIND11_INTERNALS_VERSION 4
# if PY_VERSION_HEX >= 0x030C0000
// Version bump for Python 3.12+, before first 3.12 beta release.
# define PYBIND11_INTERNALS_VERSION 5
# else
# define PYBIND11_INTERNALS_VERSION 4
# endif
#endif

// This requirement is mainly to reduce the support burden (see PR #4570).
static_assert(PY_VERSION_HEX < 0x030C0000 || PYBIND11_INTERNALS_VERSION >= 5,
"pybind11 ABI version 5 is the minimum for Python 3.12+");

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

using ExceptionTranslator = void (*)(std::exception_ptr);
Expand Down Expand Up @@ -421,6 +430,38 @@ inline void translate_local_exception(std::exception_ptr p) {
}
#endif

inline object get_python_state_dict() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I wonder if we can do some handling of if this is called when the python interpreter is closed to improve/fix #4505

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think (but could be wrong) that

are different & independent for the most part.

(It bugs me a lot that we don't have a solid tear-down solution in finalize_interpreter(), but unfortunately that's not something I can devote my time to anytime soon.)

object state_dict;
#if PYBIND11_INTERNALS_VERSION <= 4 || PY_VERSION_HEX < 0x03080000 || defined(PYPY_VERSION)
state_dict = reinterpret_borrow<object>(PyEval_GetBuiltins());
#else
# if PY_VERSION_HEX < 0x03090000
PyInterpreterState *istate = _PyInterpreterState_Get();
# else
PyInterpreterState *istate = PyInterpreterState_Get();
# endif
if (istate) {
state_dict = reinterpret_borrow<object>(PyInterpreterState_GetDict(istate));
}
#endif
if (!state_dict) {
raise_from(PyExc_SystemError, "pybind11::detail::get_python_state_dict() FAILED");
}
return state_dict;
}

inline object get_internals_obj_from_state_dict(handle state_dict) {
return reinterpret_borrow<object>(dict_getitemstring(state_dict.ptr(), PYBIND11_INTERNALS_ID));
}

inline internals **get_internals_pp_from_capsule(handle obj) {
void *raw_ptr = PyCapsule_GetPointer(obj.ptr(), /*name=*/nullptr);
if (raw_ptr == nullptr) {
raise_from(PyExc_SystemError, "pybind11::detail::get_internals_pp_from_capsule() FAILED");
}
return static_cast<internals **>(raw_ptr);
}

/// Return a reference to the current `internals` data
PYBIND11_NOINLINE internals &get_internals() {
auto **&internals_pp = get_internals_pp();
Expand All @@ -445,12 +486,12 @@ PYBIND11_NOINLINE internals &get_internals() {
#endif
error_scope err_scope;

PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID);
auto builtins = handle(PyEval_GetBuiltins());
if (builtins.contains(id) && isinstance<capsule>(builtins[id])) {
internals_pp = static_cast<internals **>(capsule(builtins[id]));

// We loaded builtins through python's builtins, which means that our `error_already_set`
dict state_dict = get_python_state_dict();
if (object internals_obj = get_internals_obj_from_state_dict(state_dict)) {
internals_pp = get_internals_pp_from_capsule(internals_obj);
}
if (internals_pp && *internals_pp) {
// We loaded the internals through `state_dict`, 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.
//
Expand Down Expand Up @@ -484,7 +525,7 @@ PYBIND11_NOINLINE internals &get_internals() {
# endif
internals_ptr->istate = tstate->interp;
#endif
builtins[id] = capsule(internals_pp);
state_dict[PYBIND11_INTERNALS_ID] = capsule(internals_pp);
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();
Expand Down
10 changes: 4 additions & 6 deletions include/pybind11/embed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,16 +243,14 @@ inline void initialize_interpreter(bool init_signal_handlers = true,

\endrst */
inline void finalize_interpreter() {
handle builtins(PyEval_GetBuiltins());
const char *id = PYBIND11_INTERNALS_ID;

// Get the internals pointer (without creating it if it doesn't exist). It's possible for the
// internals to be created during Py_Finalize() (e.g. if a py::capsule calls `get_internals()`
// during destruction), so we get the pointer-pointer here and check it after Py_Finalize().
detail::internals **internals_ptr_ptr = detail::get_internals_pp();
// It could also be stashed in builtins, so look there too:
if (builtins.contains(id) && isinstance<capsule>(builtins[id])) {
internals_ptr_ptr = capsule(builtins[id]);
// It could also be stashed in state_dict, so look there too:
if (object internals_obj
= get_internals_obj_from_state_dict(detail::get_python_state_dict())) {
internals_ptr_ptr = detail::get_internals_pp_from_capsule(internals_obj);
}
// Local internals contains data managed by the current interpreter, so we must clear them to
// avoid undefined behaviors when initializing another interpreter
Expand Down
22 changes: 11 additions & 11 deletions tests/test_embed/test_interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,10 @@ TEST_CASE("Add program dir to path using PyConfig") {
}
#endif

bool has_pybind11_internals_builtin() {
auto builtins = py::handle(PyEval_GetBuiltins());
return builtins.contains(PYBIND11_INTERNALS_ID);
};
bool has_state_dict_internals_obj() {
return bool(
py::detail::get_internals_obj_from_state_dict(py::detail::get_python_state_dict()));
}

bool has_pybind11_internals_static() {
auto **&ipp = py::detail::get_internals_pp();
Expand All @@ -268,7 +268,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<int>() == 3);
REQUIRE(has_pybind11_internals_builtin());
REQUIRE(has_state_dict_internals_obj());
REQUIRE(has_pybind11_internals_static());
REQUIRE(py::module_::import("external_module").attr("A")(123).attr("value").cast<int>()
== 123);
Expand All @@ -285,10 +285,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_state_dict_internals_obj());
REQUIRE_FALSE(has_pybind11_internals_static());
pybind11::detail::get_internals();
REQUIRE(has_pybind11_internals_builtin());
REQUIRE(has_state_dict_internals_obj());
REQUIRE(has_pybind11_internals_static());
REQUIRE(reinterpret_cast<uintptr_t>(*py::detail::get_internals_pp())
== py::module_::import("external_module").attr("internals_at")().cast<uintptr_t>());
Expand All @@ -303,13 +303,13 @@ TEST_CASE("Restart the interpreter") {
py::detail::get_internals();
*static_cast<bool *>(ran) = true;
});
REQUIRE_FALSE(has_pybind11_internals_builtin());
REQUIRE_FALSE(has_state_dict_internals_obj());
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_state_dict_internals_obj());
REQUIRE_FALSE(has_pybind11_internals_static());

// C++ modules can be reloaded.
Expand All @@ -331,7 +331,7 @@ TEST_CASE("Subinterpreter") {

REQUIRE(m.attr("add")(1, 2).cast<int>() == 3);
}
REQUIRE(has_pybind11_internals_builtin());
REQUIRE(has_state_dict_internals_obj());
REQUIRE(has_pybind11_internals_static());

/// Create and switch to a subinterpreter.
Expand All @@ -341,7 +341,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_state_dict_internals_obj());
REQUIRE(has_pybind11_internals_static());

// Modules tags should be gone.
Expand Down