From 38317c31398b21de9c782f30da233612e0583767 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 8 Oct 2023 22:22:37 -0700 Subject: [PATCH 01/31] LazyInitializeAtLeastOnceDestroyNever v1 --- include/pybind11/numpy.h | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 23c38660e9..5a60f18b2a 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -14,12 +14,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -42,6 +44,30 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) +// Main author of this class: jbms@ +template +class LazyInitializeAtLeastOnceDestroyNever { +public: + template + T &Get(Initialize &&initialize) { + if (!initialized_) { + assert(PyGILState_Check()); + // Multiple threads may run this concurrently, but that is fine. + auto value = initialize(); // May release and re-acquire the GIL. + if (!initialized_) { // This runs with the GIL held, + new // therefore this is reached only once. + (reinterpret_cast(value_storage_)) T(std::move(value)); + initialized_ = true; + } + } + return *reinterpret_cast(value_storage_); + } + +private: + alignas(T) char value_storage_[sizeof(T)]; + bool initialized_ = false; +}; + template <> struct handle_type_name { static constexpr auto name = const_name("numpy.ndarray"); @@ -206,8 +232,8 @@ struct npy_api { }; static npy_api &get() { - static npy_api api = lookup(); - return api; + static LazyInitializeAtLeastOnceDestroyNever api_init; + return api_init.Get(lookup); } bool PyArray_Check_(PyObject *obj) const { @@ -643,10 +669,11 @@ class dtype : public object { char flags() const { return detail::array_descriptor_proxy(m_ptr)->flags; } private: - static object _dtype_from_pep3118() { - module_ m = detail::import_numpy_core_submodule("_internal"); - static PyObject *obj = m.attr("_dtype_from_pep3118").cast().release().ptr(); - return reinterpret_borrow(obj); + static object &_dtype_from_pep3118() { + static detail::LazyInitializeAtLeastOnceDestroyNever imported_obj; + return imported_obj.Get([]() { + return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); + }); } dtype strip_padding(ssize_t itemsize) { From e7b8c4f0fcd72191e88d1c17abf5da08fe3a9c6f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 8 Oct 2023 22:38:19 -0700 Subject: [PATCH 02/31] Go back to using `union` as originally suggested by jbms@. The trick (also suggested by jbms@) is to add empty ctor + dtor. --- include/pybind11/numpy.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 5a60f18b2a..0a00120c90 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include @@ -55,16 +54,21 @@ class LazyInitializeAtLeastOnceDestroyNever { // Multiple threads may run this concurrently, but that is fine. auto value = initialize(); // May release and re-acquire the GIL. if (!initialized_) { // This runs with the GIL held, - new // therefore this is reached only once. - (reinterpret_cast(value_storage_)) T(std::move(value)); + new (&value_) // therefore this is reached only once. + T(std::move(value)); initialized_ = true; } } - return *reinterpret_cast(value_storage_); + return value_; } + LazyInitializeAtLeastOnceDestroyNever() {} + ~LazyInitializeAtLeastOnceDestroyNever() {} + private: - alignas(T) char value_storage_[sizeof(T)]; + union { + T value_; + }; bool initialized_ = false; }; From 74ac0d99fb5c70aa66248fed5d7536089da3e253 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 05:08:24 -0700 Subject: [PATCH 03/31] Revert "Go back to using `union` as originally suggested by jbms@. The trick (also suggested by jbms@) is to add empty ctor + dtor." This reverts commit e7b8c4f0fcd72191e88d1c17abf5da08fe3a9c6f. --- include/pybind11/numpy.h | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 0a00120c90..5a60f18b2a 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -54,21 +55,16 @@ class LazyInitializeAtLeastOnceDestroyNever { // Multiple threads may run this concurrently, but that is fine. auto value = initialize(); // May release and re-acquire the GIL. if (!initialized_) { // This runs with the GIL held, - new (&value_) // therefore this is reached only once. - T(std::move(value)); + new // therefore this is reached only once. + (reinterpret_cast(value_storage_)) T(std::move(value)); initialized_ = true; } } - return value_; + return *reinterpret_cast(value_storage_); } - LazyInitializeAtLeastOnceDestroyNever() {} - ~LazyInitializeAtLeastOnceDestroyNever() {} - private: - union { - T value_; - }; + alignas(T) char value_storage_[sizeof(T)]; bool initialized_ = false; }; From 109a1659c881d4c569430add6d3e170b44599eb8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 06:47:45 -0700 Subject: [PATCH 04/31] Remove `#include ` * `include\pybind11/numpy.h(24,10): fatal error C1083: Cannot open include file: 'stdalign.h': No such file or directory` * @tkoeppe wrote: this is a C interop header (and we're not writing C) --- include/pybind11/numpy.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 5a60f18b2a..60034ae989 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include From 88cec1152ab5576db19bab95c484672f06f5989a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 07:00:47 -0700 Subject: [PATCH 05/31] Suppress gcc 4.8.5 (CentOS 7) warning. ``` include/pybind11/eigen/../numpy.h:63:53: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] return *reinterpret_cast(value_storage_); ^ ``` --- include/pybind11/numpy.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 60034ae989..397e92769a 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -59,7 +59,13 @@ class LazyInitializeAtLeastOnceDestroyNever { initialized_ = true; } } + PYBIND11_WARNING_PUSH +#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 + // Needed for gcc 4.8.5 + PYBIND11_WARNING_DISABLE_CLANG("-Wstrict-aliasing") +#endif return *reinterpret_cast(value_storage_); + PYBIND11_WARNING_POP } private: From 1ce27154d655dd768ddd855394f30969c476b9fb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 07:24:17 -0700 Subject: [PATCH 06/31] Replace comments: Document PRECONDITION. Adopt comment suggested by @tkoeppe: https://github.com/pybind/pybind11/pull/4877#discussion_r1350356093 --- include/pybind11/numpy.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 397e92769a..cb0e8a4e26 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -47,15 +47,18 @@ PYBIND11_NAMESPACE_BEGIN(detail) template class LazyInitializeAtLeastOnceDestroyNever { public: + // PRECONDITION: The GIL must be held when `Get()` is called. + // It is possible that multiple threads execute `Get()` with `initialized_` + // still being false, and thus proceed to execute `initialize()`. This can + // happen if `initialize()` releases and reacquires the GIL internally. + // We accept this, and expect the operation to be both idempotent and cheap. template T &Get(Initialize &&initialize) { if (!initialized_) { assert(PyGILState_Check()); - // Multiple threads may run this concurrently, but that is fine. - auto value = initialize(); // May release and re-acquire the GIL. - if (!initialized_) { // This runs with the GIL held, - new // therefore this is reached only once. - (reinterpret_cast(value_storage_)) T(std::move(value)); + auto value = initialize(); + if (!initialized_) { + new (reinterpret_cast(value_storage_)) T(std::move(value)); initialized_ = true; } } From e7be9c2c2ea99382101764f3ccc183c61b40c68d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 07:35:33 -0700 Subject: [PATCH 07/31] Adopt suggestion by @tkoeppe: * https://github.com/pybind/pybind11/pull/4877#issuecomment-1752969127 * https://godbolt.org/z/Wa79nKz6e --- include/pybind11/numpy.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index cb0e8a4e26..62768adcef 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -71,6 +71,13 @@ class LazyInitializeAtLeastOnceDestroyNever { PYBIND11_WARNING_POP } + constexpr LazyInitializeAtLeastOnceDestroyNever() = default; +#if __cplusplus >= 202002L + constexpr +#endif // C++20 + ~LazyInitializeAtLeastOnceDestroyNever() + = default; + private: alignas(T) char value_storage_[sizeof(T)]; bool initialized_ = false; From f07b28bda9f91fb723aa898a21c81b6dd6857072 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 07:50:16 -0700 Subject: [PATCH 08/31] Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` g++ -o pybind11/tests/test_numpy_array.os -c -std=c++20 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.11 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp ``` ``` In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp:10: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h: In static member function ‘static pybind11::detail::npy_api& pybind11::detail::npy_api::get()’: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h:258:82: error: ‘constinit’ variable ‘api_init’ does not have a constant initializer 258 | PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever api_init; | ^~~~~~~~ ``` ``` In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_numpy_array.cpp:10: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h: In static member function ‘static pybind11::object& pybind11::dtype::_dtype_from_pep3118()’: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/numpy.h:697:13: error: ‘constinit’ variable ‘imported_obj’ does not have a constant initializer 697 | imported_obj; | ^~~~~~~~~~~~ ``` --- include/pybind11/numpy.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 62768adcef..b1365ec1e3 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -43,6 +43,14 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) +#ifndef PYBIND11_CONSTINIT +# if __cplusplus >= 202002L +# define PYBIND11_CONSTINIT constinit +# else +# define PYBIND11_CONSTINIT +# endif +#endif + // Main author of this class: jbms@ template class LazyInitializeAtLeastOnceDestroyNever { @@ -247,7 +255,7 @@ struct npy_api { }; static npy_api &get() { - static LazyInitializeAtLeastOnceDestroyNever api_init; + PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever api_init; return api_init.Get(lookup); } @@ -685,7 +693,8 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - static detail::LazyInitializeAtLeastOnceDestroyNever imported_obj; + PYBIND11_CONSTINIT static detail::LazyInitializeAtLeastOnceDestroyNever + imported_obj; return imported_obj.Get([]() { return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); }); From 36be645758aa82b576d24003808386bec6e55bf9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 07:56:40 -0700 Subject: [PATCH 09/31] Revert "Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases:" This reverts commit f07b28bda9f91fb723aa898a21c81b6dd6857072. --- include/pybind11/numpy.h | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index b1365ec1e3..62768adcef 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -43,14 +43,6 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) -#ifndef PYBIND11_CONSTINIT -# if __cplusplus >= 202002L -# define PYBIND11_CONSTINIT constinit -# else -# define PYBIND11_CONSTINIT -# endif -#endif - // Main author of this class: jbms@ template class LazyInitializeAtLeastOnceDestroyNever { @@ -255,7 +247,7 @@ struct npy_api { }; static npy_api &get() { - PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever api_init; + static LazyInitializeAtLeastOnceDestroyNever api_init; return api_init.Get(lookup); } @@ -693,8 +685,7 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - PYBIND11_CONSTINIT static detail::LazyInitializeAtLeastOnceDestroyNever - imported_obj; + static detail::LazyInitializeAtLeastOnceDestroyNever imported_obj; return imported_obj.Get([]() { return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); }); From 7bc16a67fc4972282abf1849090374670ae939a5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 08:20:54 -0700 Subject: [PATCH 10/31] Reapply "Add `PYBIND11_CONSTINIT`, but it does not work for the current use cases:" This reverts commit 36be645758aa82b576d24003808386bec6e55bf9. --- include/pybind11/numpy.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 62768adcef..b1365ec1e3 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -43,6 +43,14 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) +#ifndef PYBIND11_CONSTINIT +# if __cplusplus >= 202002L +# define PYBIND11_CONSTINIT constinit +# else +# define PYBIND11_CONSTINIT +# endif +#endif + // Main author of this class: jbms@ template class LazyInitializeAtLeastOnceDestroyNever { @@ -247,7 +255,7 @@ struct npy_api { }; static npy_api &get() { - static LazyInitializeAtLeastOnceDestroyNever api_init; + PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever api_init; return api_init.Get(lookup); } @@ -685,7 +693,8 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - static detail::LazyInitializeAtLeastOnceDestroyNever imported_obj; + PYBIND11_CONSTINIT static detail::LazyInitializeAtLeastOnceDestroyNever + imported_obj; return imported_obj.Get([]() { return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); }); From 78f4e930ac960a127fca72e866583d8094219a3a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 08:33:11 -0700 Subject: [PATCH 11/31] Add Default Member Initializer on `value_storage_` as suggested by @tkoeppe: https://github.com/pybind/pybind11/pull/4877#issuecomment-1753201342 This fixes the errors reported under commit f07b28bda9f91fb723aa898a21c81b6dd6857072. --- include/pybind11/numpy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index b1365ec1e3..cb26878053 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -87,7 +87,7 @@ class LazyInitializeAtLeastOnceDestroyNever { = default; private: - alignas(T) char value_storage_[sizeof(T)]; + alignas(T) char value_storage_[sizeof(T)] = {}; bool initialized_ = false; }; From 6d9441dbf4f43f35ead6b7480b05dc74415dd33f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 10:23:41 -0700 Subject: [PATCH 12/31] Fix copy-paste-missed-a-change mishap in commit 88cec1152ab5576db19bab95c484672f06f5989a. --- include/pybind11/numpy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index cb26878053..47050a36d1 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -73,7 +73,7 @@ class LazyInitializeAtLeastOnceDestroyNever { PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 // Needed for gcc 4.8.5 - PYBIND11_WARNING_DISABLE_CLANG("-Wstrict-aliasing") + PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing") #endif return *reinterpret_cast(value_storage_); PYBIND11_WARNING_POP From a864f2123cc83ecfc0daf755e9a70767b07de448 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 10:36:16 -0700 Subject: [PATCH 13/31] Semi-paranoid placement new (based on https://github.com/pybind/pybind11/pull/4877#discussion_r1350573114). --- include/pybind11/numpy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 47050a36d1..1c76177f95 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -66,7 +66,7 @@ class LazyInitializeAtLeastOnceDestroyNever { assert(PyGILState_Check()); auto value = initialize(); if (!initialized_) { - new (reinterpret_cast(value_storage_)) T(std::move(value)); + ::new (value_storage_) T(std::move(value)); initialized_ = true; } } From 6689b06449c3d7b716ee71a834b9bb0d035d1dac Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 11:05:07 -0700 Subject: [PATCH 14/31] Move PYBIND11_CONSTINIT to detail/common.h --- include/pybind11/detail/common.h | 8 ++++++++ include/pybind11/numpy.h | 8 -------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index e79f7693d0..9adf73acad 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -118,6 +118,14 @@ # endif #endif +#ifndef PYBIND11_CONSTINIT +# ifdef PYBIND11_CPP20 +# define PYBIND11_CONSTINIT constinit +# else +# define PYBIND11_CONSTINIT +# endif +#endif + // Compiler version assertions #if defined(__INTEL_COMPILER) # if __INTEL_COMPILER < 1800 diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 1c76177f95..7fe6096ab4 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -43,14 +43,6 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) -#ifndef PYBIND11_CONSTINIT -# if __cplusplus >= 202002L -# define PYBIND11_CONSTINIT constinit -# else -# define PYBIND11_CONSTINIT -# endif -#endif - // Main author of this class: jbms@ template class LazyInitializeAtLeastOnceDestroyNever { From d965f29db6c5bced2e23a608ccf0f1ad953dd39c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 12:28:01 -0700 Subject: [PATCH 15/31] Move code to the right places, rename new class and some variables. --- CMakeLists.txt | 1 + include/pybind11/detail/common.h | 12 ++-- .../pybind11/gil_save_static_initialization.h | 66 +++++++++++++++++++ include/pybind11/numpy.h | 51 ++------------ 4 files changed, 78 insertions(+), 52 deletions(-) create mode 100644 include/pybind11/gil_save_static_initialization.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 65ad0c22fd..f896244e14 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -142,6 +142,7 @@ set(PYBIND11_HEADERS include/pybind11/embed.h include/pybind11/eval.h include/pybind11/gil.h + include/pybind11/gil_save_static_initialization.h include/pybind11/iostream.h include/pybind11/functional.h include/pybind11/numpy.h diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 9adf73acad..83800e960b 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -118,12 +118,12 @@ # endif #endif -#ifndef PYBIND11_CONSTINIT -# ifdef PYBIND11_CPP20 -# define PYBIND11_CONSTINIT constinit -# else -# define PYBIND11_CONSTINIT -# endif +#if defined(PYBIND11_CPP20) +# define PYBIND11_CONSTINIT constinit +# define PYBIND11_DTOR_CONSTEXPR constexpr +#else +# define PYBIND11_CONSTINIT +# define PYBIND11_DTOR_CONSTEXPR #endif // Compiler version assertions diff --git a/include/pybind11/gil_save_static_initialization.h b/include/pybind11/gil_save_static_initialization.h new file mode 100644 index 0000000000..a0523265f3 --- /dev/null +++ b/include/pybind11/gil_save_static_initialization.h @@ -0,0 +1,66 @@ +// Copyright (c) 2023 The pybind Community. + +#pragma once + +#include "detail/common.h" + +#include +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) + +// Use the `gil_save_static_initialization` class below instead of the naive +// +// static auto imported_obj = py::module_::import("module_name"); // BAD, DO NOT USE! +// +// which has two serious issues: +// +// 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and +// 2. deadlocks in multi-threaded processes. +// +// This alternative will avoid both problems: +// +// PYBIND11_CONSTINIT static py::gil_save_static_initialization obj_importer; +// auto imported_obj = obj_importer.get([]() { return py::module_::import("module_name"); }); +// +// The `get()` argument is meant to be a callable that makes Python C API calls. +// +// `T` can be any C++ type, it does not have to be a Python type. +// +// Main author of this class: jbms@ (original name: LazyInitializeAtLeastOnceDestroyNever) +template +class gil_save_static_initialization { +public: + // PRECONDITION: The GIL must be held when `get()` is called. + // It is possible that multiple threads execute `get()` with `initialized_` + // still being false, and thus proceed to execute `initialize()`. This can + // happen if `initialize()` releases and reacquires the GIL internally. + // We accept this, and expect the operation to be both idempotent and cheap. + template + T &get(Initialize &&initialize) { + if (!initialized_) { + assert(PyGILState_Check()); + auto value = initialize(); + if (!initialized_) { + ::new (value_storage_) T(std::move(value)); + initialized_ = true; + } + } + PYBIND11_WARNING_PUSH +#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 + // Needed for gcc 4.8.5 + PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing") +#endif + return *reinterpret_cast(value_storage_); + PYBIND11_WARNING_POP + } + + constexpr gil_save_static_initialization() = default; + PYBIND11_DTOR_CONSTEXPR ~gil_save_static_initialization() = default; + +private: + alignas(T) char value_storage_[sizeof(T)] = {}; + bool initialized_ = false; +}; + +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 7fe6096ab4..f2a7e5a189 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -11,10 +11,10 @@ #include "pybind11.h" #include "complex.h" +#include "gil_save_static_initialization.h" #include #include -#include #include #include #include @@ -43,46 +43,6 @@ class array; // Forward declaration PYBIND11_NAMESPACE_BEGIN(detail) -// Main author of this class: jbms@ -template -class LazyInitializeAtLeastOnceDestroyNever { -public: - // PRECONDITION: The GIL must be held when `Get()` is called. - // It is possible that multiple threads execute `Get()` with `initialized_` - // still being false, and thus proceed to execute `initialize()`. This can - // happen if `initialize()` releases and reacquires the GIL internally. - // We accept this, and expect the operation to be both idempotent and cheap. - template - T &Get(Initialize &&initialize) { - if (!initialized_) { - assert(PyGILState_Check()); - auto value = initialize(); - if (!initialized_) { - ::new (value_storage_) T(std::move(value)); - initialized_ = true; - } - } - PYBIND11_WARNING_PUSH -#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 - // Needed for gcc 4.8.5 - PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing") -#endif - return *reinterpret_cast(value_storage_); - PYBIND11_WARNING_POP - } - - constexpr LazyInitializeAtLeastOnceDestroyNever() = default; -#if __cplusplus >= 202002L - constexpr -#endif // C++20 - ~LazyInitializeAtLeastOnceDestroyNever() - = default; - -private: - alignas(T) char value_storage_[sizeof(T)] = {}; - bool initialized_ = false; -}; - template <> struct handle_type_name { static constexpr auto name = const_name("numpy.ndarray"); @@ -247,8 +207,8 @@ struct npy_api { }; static npy_api &get() { - PYBIND11_CONSTINIT static LazyInitializeAtLeastOnceDestroyNever api_init; - return api_init.Get(lookup); + PYBIND11_CONSTINIT static gil_save_static_initialization imported_api; + return imported_api.get(lookup); } bool PyArray_Check_(PyObject *obj) const { @@ -685,9 +645,8 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - PYBIND11_CONSTINIT static detail::LazyInitializeAtLeastOnceDestroyNever - imported_obj; - return imported_obj.Get([]() { + PYBIND11_CONSTINIT static gil_save_static_initialization imported_obj; + return imported_obj.get([]() { return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); }); } From 398a42c0e8decdd4fdaee56fe6c08c46876814d7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 12:35:23 -0700 Subject: [PATCH 16/31] Fix oversight: update tests/extra_python_package/test_files.py --- tests/extra_python_package/test_files.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index e3d881c0b3..b020198fd0 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -35,6 +35,7 @@ "include/pybind11/eval.h", "include/pybind11/functional.h", "include/pybind11/gil.h", + "include/pybind11/gil_save_static_initialization.h", "include/pybind11/iostream.h", "include/pybind11/numpy.h", "include/pybind11/operators.h", From ab2cf8e318c816e347740ea33b9cbc86d2b76d3e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 13:52:00 -0700 Subject: [PATCH 17/31] Get the name right first. --- .../{gil_save_static_initialization.h => gil_safe_call_once.h} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename include/pybind11/{gil_save_static_initialization.h => gil_safe_call_once.h} (100%) diff --git a/include/pybind11/gil_save_static_initialization.h b/include/pybind11/gil_safe_call_once.h similarity index 100% rename from include/pybind11/gil_save_static_initialization.h rename to include/pybind11/gil_safe_call_once.h From 6d5bdd8b0f9ad26a321a3e643eca4faef09f902f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 14:24:16 -0700 Subject: [PATCH 18/31] Use `std::call_once`, `std::atomic`, following a pattern developed by @tkoeppe --- CMakeLists.txt | 2 +- include/pybind11/gil.h | 2 + include/pybind11/gil_safe_call_once.h | 55 ++++++++++++------------ include/pybind11/numpy.h | 10 ++--- tests/extra_python_package/test_files.py | 2 +- 5 files changed, 36 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f896244e14..d2c44dc40d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -142,7 +142,7 @@ set(PYBIND11_HEADERS include/pybind11/embed.h include/pybind11/eval.h include/pybind11/gil.h - include/pybind11/gil_save_static_initialization.h + include/pybind11/gil_safe_call_once.h include/pybind11/iostream.h include/pybind11/functional.h include/pybind11/numpy.h diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 570a5581d5..b7392bbb1a 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -137,6 +137,7 @@ class gil_scoped_acquire { class gil_scoped_release { public: + // PRECONDITION: The GIL must be held when this constructor is called. explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) { // `get_internals()` must be called here unconditionally in order to initialize // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an @@ -201,6 +202,7 @@ class gil_scoped_release { PyThreadState *state; public: + // PRECONDITION: The GIL must be held when this constructor is called. gil_scoped_release() : state{PyEval_SaveThread()} {} gil_scoped_release(const gil_scoped_release &) = delete; gil_scoped_release &operator=(const gil_scoped_release &) = delete; diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index a0523265f3..25cf59511f 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -3,64 +3,63 @@ #pragma once #include "detail/common.h" +#include "gil.h" +#include #include +#include #include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) -// Use the `gil_save_static_initialization` class below instead of the naive +// Use the `gil_safe_call_once_and_store` class below instead of the naive // // static auto imported_obj = py::module_::import("module_name"); // BAD, DO NOT USE! // // which has two serious issues: // // 1. Py_DECREF() calls potentially after the Python interpreter was finalized already, and -// 2. deadlocks in multi-threaded processes. +// 2. deadlocks in multi-threaded processes (because of missing lock ordering). // -// This alternative will avoid both problems: +// The following alternative avoids both problems: // -// PYBIND11_CONSTINIT static py::gil_save_static_initialization obj_importer; -// auto imported_obj = obj_importer.get([]() { return py::module_::import("module_name"); }); +// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store obj_importer; +// auto imported_obj = obj_importer.get_stored([]() { return py::module_::import("module_name"); +// }); // -// The `get()` argument is meant to be a callable that makes Python C API calls. +// The `get_stored()` argument is meant to be a callable that makes Python C API calls. // // `T` can be any C++ type, it does not have to be a Python type. -// -// Main author of this class: jbms@ (original name: LazyInitializeAtLeastOnceDestroyNever) template -class gil_save_static_initialization { +class gil_safe_call_once_and_store { public: - // PRECONDITION: The GIL must be held when `get()` is called. - // It is possible that multiple threads execute `get()` with `initialized_` - // still being false, and thus proceed to execute `initialize()`. This can - // happen if `initialize()` releases and reacquires the GIL internally. - // We accept this, and expect the operation to be both idempotent and cheap. - template - T &get(Initialize &&initialize) { - if (!initialized_) { - assert(PyGILState_Check()); - auto value = initialize(); - if (!initialized_) { - ::new (value_storage_) T(std::move(value)); - initialized_ = true; - } + // PRECONDITION: The GIL must be held when `get_stored()` is called. + template + T &get_stored(Callable &&fn) { + if (!is_initialized_.load(std::memory_order_acquire)) { + gil_scoped_release gil_rel; + std::call_once(once_flag_, [&] { + gil_scoped_acquire gil_acq; + ::new (storage_) T(fn()); + is_initialized_.store(true, std::memory_order_release); + }); } PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 // Needed for gcc 4.8.5 PYBIND11_WARNING_DISABLE_GCC("-Wstrict-aliasing") #endif - return *reinterpret_cast(value_storage_); + return *reinterpret_cast(storage_); PYBIND11_WARNING_POP } - constexpr gil_save_static_initialization() = default; - PYBIND11_DTOR_CONSTEXPR ~gil_save_static_initialization() = default; + constexpr gil_safe_call_once_and_store() = default; + PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default; private: - alignas(T) char value_storage_[sizeof(T)] = {}; - bool initialized_ = false; + alignas(T) char storage_[sizeof(T)] = {}; + std::once_flag once_flag_ = {}; + std::atomic is_initialized_ = {}; }; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index f2a7e5a189..c5322cb67b 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -11,7 +11,7 @@ #include "pybind11.h" #include "complex.h" -#include "gil_save_static_initialization.h" +#include "gil_safe_call_once.h" #include #include @@ -207,8 +207,8 @@ struct npy_api { }; static npy_api &get() { - PYBIND11_CONSTINIT static gil_save_static_initialization imported_api; - return imported_api.get(lookup); + PYBIND11_CONSTINIT static gil_safe_call_once_and_store imported_api; + return imported_api.get_stored(lookup); } bool PyArray_Check_(PyObject *obj) const { @@ -645,8 +645,8 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - PYBIND11_CONSTINIT static gil_save_static_initialization imported_obj; - return imported_obj.get([]() { + PYBIND11_CONSTINIT static gil_safe_call_once_and_store imported_obj; + return imported_obj.get_stored([]() { return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); }); } diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index b020198fd0..344e70d5db 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -35,7 +35,7 @@ "include/pybind11/eval.h", "include/pybind11/functional.h", "include/pybind11/gil.h", - "include/pybind11/gil_save_static_initialization.h", + "include/pybind11/gil_safe_call_once.h", "include/pybind11/iostream.h", "include/pybind11/numpy.h", "include/pybind11/operators.h", From 4c5dd1bb57e46360f63e029c1f7c00a1593fa427 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 15:45:17 -0700 Subject: [PATCH 19/31] Make the API more self-documenting (and possibly more easily reusable). --- include/pybind11/gil_safe_call_once.h | 22 ++++++++++++++++------ include/pybind11/numpy.h | 15 +++++++++------ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 25cf59511f..c4ca98adbb 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -23,11 +23,15 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) // // The following alternative avoids both problems: // -// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store obj_importer; -// auto imported_obj = obj_importer.get_stored([]() { return py::module_::import("module_name"); -// }); +// PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store storage; +// auto &imported_obj = storage // Do NOT make this `static`! +// .call_once_and_store_result([]() { +// return py::module_::import("module_name"); +// }) +// .get_stored(); // -// The `get_stored()` argument is meant to be a callable that makes Python C API calls. +// The `call_once_and_store_result()` argument is meant to be a callable that +// makes Python C API calls. // // `T` can be any C++ type, it does not have to be a Python type. template @@ -35,7 +39,7 @@ class gil_safe_call_once_and_store { public: // PRECONDITION: The GIL must be held when `get_stored()` is called. template - T &get_stored(Callable &&fn) { + gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) { if (!is_initialized_.load(std::memory_order_acquire)) { gil_scoped_release gil_rel; std::call_once(once_flag_, [&] { @@ -44,6 +48,12 @@ class gil_safe_call_once_and_store { is_initialized_.store(true, std::memory_order_release); }); } + return *this; + } + + // This must only be called after `call_once_and_store()` was called. + T &get_stored() const { + assert(is_initialized_.load(std::memory_order_relaxed)); PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 // Needed for gcc 4.8.5 @@ -57,7 +67,7 @@ class gil_safe_call_once_and_store { PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default; private: - alignas(T) char storage_[sizeof(T)] = {}; + alignas(T) mutable char storage_[sizeof(T)] = {}; std::once_flag once_flag_ = {}; std::atomic is_initialized_ = {}; }; diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index c5322cb67b..40f988fdff 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -207,8 +207,8 @@ struct npy_api { }; static npy_api &get() { - PYBIND11_CONSTINIT static gil_safe_call_once_and_store imported_api; - return imported_api.get_stored(lookup); + PYBIND11_CONSTINIT static gil_safe_call_once_and_store storage; + return storage.call_once_and_store_result(lookup).get_stored(); } bool PyArray_Check_(PyObject *obj) const { @@ -645,10 +645,13 @@ class dtype : public object { private: static object &_dtype_from_pep3118() { - PYBIND11_CONSTINIT static gil_safe_call_once_and_store imported_obj; - return imported_obj.get_stored([]() { - return detail::import_numpy_core_submodule("_internal").attr("_dtype_from_pep3118"); - }); + PYBIND11_CONSTINIT static gil_safe_call_once_and_store storage; + return storage + .call_once_and_store_result([]() { + return detail::import_numpy_core_submodule("_internal") + .attr("_dtype_from_pep3118"); + }) + .get_stored(); } dtype strip_padding(ssize_t itemsize) { From 8633c5be462c802573f45c9f7593341811a371f6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 15:56:58 -0700 Subject: [PATCH 20/31] google-clang-tidy IWYU fixes --- include/pybind11/gil_safe_call_once.h | 3 +-- include/pybind11/numpy.h | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index c4ca98adbb..5f688c8556 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -8,7 +8,6 @@ #include #include #include -#include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -37,7 +36,7 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) template class gil_safe_call_once_and_store { public: - // PRECONDITION: The GIL must be held when `get_stored()` is called. + // PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called. template gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) { if (!is_initialized_.load(std::memory_order_acquire)) { diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 40f988fdff..8551aa2648 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -10,8 +10,10 @@ #pragma once #include "pybind11.h" +#include "detail/common.h" #include "complex.h" #include "gil_safe_call_once.h" +#include "pytypes.h" #include #include From 82f3efc3ee2d1c6bc9984e7f1636f37298f66a70 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 21:37:16 -0700 Subject: [PATCH 21/31] Rewrite comment as suggested by @tkoeppe --- include/pybind11/gil_safe_call_once.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 5f688c8556..1946421d62 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -29,10 +29,10 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) // }) // .get_stored(); // -// The `call_once_and_store_result()` argument is meant to be a callable that -// makes Python C API calls. +// The parameter of `call_once_and_store_result()` must be callable. It can make +// CPython API calls, and in particular, it can temporarily release the GIL. // -// `T` can be any C++ type, it does not have to be a Python type. +// `T` can be any C++ type, it does not have to involve CPython API types. template class gil_safe_call_once_and_store { public: From 3ebd1392f39fe6b9c7e2bd8401ffd5a803e92cc6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 9 Oct 2023 21:54:44 -0700 Subject: [PATCH 22/31] Update test_exceptions.cpp and exceptions.rst --- docs/advanced/exceptions.rst | 9 ++++----- tests/test_exceptions.cpp | 8 ++++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 616e2d7726..1d1cc1351a 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -141,15 +141,14 @@ standard python RuntimeError: .. code-block:: cpp - // This is a static object, so we must leak the Python reference: - // It is undefined when the destructor will run, possibly only after the - // Python interpreter is finalized already. - static py::handle exc = py::exception(m, "MyCustomError").release(); + static py::gil_safe_call_once_and_store exc_storage; + exc_storage.call_once_and_store_result( + [&]() { return py::exception(m, "MyCustomError"); }); py::register_exception_translator([](std::exception_ptr p) { try { if (p) std::rethrow_exception(p); } catch (const MyCustomException &e) { - py::set_error(exc, e.what()); + py::set_error(exc_storage.get_stored(), e.what()); } catch (const OtherException &e) { py::set_error(PyExc_RuntimeError, e.what()); } diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index f6d9c215c9..21c6bd3e07 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -6,6 +6,8 @@ All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. */ +#include + #include "test_exceptions.h" #include "local_bindings.h" @@ -114,7 +116,9 @@ TEST_SUBMODULE(exceptions, m) { []() { throw std::runtime_error("This exception was intentionally thrown."); }); // PLEASE KEEP IN SYNC with docs/advanced/exceptions.rst - static py::handle ex = py::exception(m, "MyException").release(); + static py::gil_safe_call_once_and_store ex_storage; + ex_storage.call_once_and_store_result( + [&]() { return py::exception(m, "MyException"); }); py::register_exception_translator([](std::exception_ptr p) { try { if (p) { @@ -122,7 +126,7 @@ TEST_SUBMODULE(exceptions, m) { } } catch (const MyException &e) { // Set MyException as the active python error - py::set_error(ex, e.what()); + py::set_error(ex_storage.get_stored(), e.what()); } }); From c33712d2696d4adaa962196b21f78c28338817e8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 10:00:52 -0700 Subject: [PATCH 23/31] Fix oversight in previous commit: add `PYBIND11_CONSTINIT` --- docs/advanced/exceptions.rst | 2 +- tests/test_exceptions.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 1d1cc1351a..e20f42b5fe 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -141,7 +141,7 @@ standard python RuntimeError: .. code-block:: cpp - static py::gil_safe_call_once_and_store exc_storage; + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store exc_storage; exc_storage.call_once_and_store_result( [&]() { return py::exception(m, "MyCustomError"); }); py::register_exception_translator([](std::exception_ptr p) { diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 21c6bd3e07..d3af7ab728 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -116,7 +116,7 @@ TEST_SUBMODULE(exceptions, m) { []() { throw std::runtime_error("This exception was intentionally thrown."); }); // PLEASE KEEP IN SYNC with docs/advanced/exceptions.rst - static py::gil_safe_call_once_and_store ex_storage; + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store ex_storage; ex_storage.call_once_and_store_result( [&]() { return py::exception(m, "MyException"); }); py::register_exception_translator([](std::exception_ptr p) { From dcf2b9221379a303eb7752aa5ec8e19ee6f7b130 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 10:45:14 -0700 Subject: [PATCH 24/31] Make `get_stored()` non-const for simplicity. As suggested by @tkoeppe: not seeing any reasonable use in which `get_stored` has to be const. --- include/pybind11/gil_safe_call_once.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 1946421d62..8487ac0e0a 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -51,7 +51,8 @@ class gil_safe_call_once_and_store { } // This must only be called after `call_once_and_store()` was called. - T &get_stored() const { + // Not const for simplicity. (Could be made const if there is an unforeseen need.) + T &get_stored() { assert(is_initialized_.load(std::memory_order_relaxed)); PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 @@ -66,7 +67,7 @@ class gil_safe_call_once_and_store { PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default; private: - alignas(T) mutable char storage_[sizeof(T)] = {}; + alignas(T) char storage_[sizeof(T)] = {}; std::once_flag once_flag_ = {}; std::atomic is_initialized_ = {}; }; From 4557dce0baec8a1a3eaad77a223f7cf864eb4f36 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 11:27:53 -0700 Subject: [PATCH 25/31] Add comment regarding `KeyboardInterrupt` behavior, based heavily on information provided by @jbms. --- include/pybind11/gil_safe_call_once.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 8487ac0e0a..504c63bc70 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -33,6 +33,16 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) // CPython API calls, and in particular, it can temporarily release the GIL. // // `T` can be any C++ type, it does not have to involve CPython API types. +// +// The behavior with regard to signals, e.g. `SIGINT` (`KeyboardInterrupt`), +// is not ideal. If the main thread is the one to actually run the `Callable`, +// then a `KeyboardInterrupt` will interrupt it if it is running normal Python +// code. The situation is different if a non-main thread runs the +// `Callable`, and then the main thread starts waiting for it to complete: +// a `KeyboardInterrupt` will not interrupt the non-main thread, but it will +// get processed only when it is the main thread's turn again and it is running +// normal Python code. However, this will be unnoticeable for quick call-once +// functions, which is usually the case. template class gil_safe_call_once_and_store { public: From 704fe13595bd4e97735633890d20c7118dda7d93 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 13:29:12 -0700 Subject: [PATCH 26/31] Add `assert(PyGILState_Check())` in `gil_scoped_release` ctor (simple & non-simple implementation) as suggested by @EthanSteinberg. --- include/pybind11/gil.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index b7392bbb1a..1cd7b7575f 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -139,6 +139,7 @@ class gil_scoped_release { public: // PRECONDITION: The GIL must be held when this constructor is called. explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) { + assert(PyGILState_Check()); // `get_internals()` must be called here unconditionally in order to initialize // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an // initialization race could occur as multiple threads try `gil_scoped_acquire`. @@ -203,7 +204,10 @@ class gil_scoped_release { public: // PRECONDITION: The GIL must be held when this constructor is called. - gil_scoped_release() : state{PyEval_SaveThread()} {} + gil_scoped_release() { + assert(PyGILState_Check()); + state = PyEval_SaveThread(); + } gil_scoped_release(const gil_scoped_release &) = delete; gil_scoped_release &operator=(const gil_scoped_release &) = delete; ~gil_scoped_release() { PyEval_RestoreThread(state); } From b2f87a8c133a6ea5fb2f02e051d0291476499d92 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 15:40:24 -0700 Subject: [PATCH 27/31] Fix oversight in previous commit (missing include cassert). --- include/pybind11/gil.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 1cd7b7575f..da22f48d7e 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -11,6 +11,8 @@ #include "detail/common.h" +#include + #if defined(WITH_THREAD) && !defined(PYBIND11_SIMPLE_GIL_MANAGEMENT) # include "detail/internals.h" #endif From fad1017a08c2fd0f2df3901b3e2e3b08c8568990 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 15:41:53 -0700 Subject: [PATCH 28/31] Remove use of std::atomic, leaving comments with rationale, why it is not needed. --- include/pybind11/gil_safe_call_once.h | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 504c63bc70..e3a78d4482 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -5,7 +5,6 @@ #include "detail/common.h" #include "gil.h" -#include #include #include @@ -49,21 +48,25 @@ class gil_safe_call_once_and_store { // PRECONDITION: The GIL must be held when `call_once_and_store_result()` is called. template gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) { - if (!is_initialized_.load(std::memory_order_acquire)) { - gil_scoped_release gil_rel; + if (!is_initialized_) { // This read is guarded by the GIL. + // Multiple threads may enter here, because CPython API calls in the + // `fn()` call below may release and reacquire the GIL. + gil_scoped_release gil_rel; // Needed to establish lock ordering. std::call_once(once_flag_, [&] { + // Only one thread will ever enter here. gil_scoped_acquire gil_acq; ::new (storage_) T(fn()); - is_initialized_.store(true, std::memory_order_release); + is_initialized_ = true; // This write is guarded by the GIL. }); } + // Intentionally not returning `T &` to ensure the calling code is self-documenting. return *this; } - // This must only be called after `call_once_and_store()` was called. + // This must only be called after `call_once_and_store_result()` was called. // Not const for simplicity. (Could be made const if there is an unforeseen need.) T &get_stored() { - assert(is_initialized_.load(std::memory_order_relaxed)); + assert(is_initialized_); PYBIND11_WARNING_PUSH #if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 // Needed for gcc 4.8.5 @@ -77,9 +80,11 @@ class gil_safe_call_once_and_store { PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default; private: + // `is_initialized_` below and `storage_` here can be replaced with `std::optional` + // when pybind11 drops C++11 support. alignas(T) char storage_[sizeof(T)] = {}; std::once_flag once_flag_ = {}; - std::atomic is_initialized_ = {}; + bool is_initialized_ = false; }; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) From 845330253710e043041b9785bc0ae84fb257c1da Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 10 Oct 2023 16:00:13 -0700 Subject: [PATCH 29/31] Rewrite comment re `std:optional` based on deeper reflection (aka 2nd thoughts). --- include/pybind11/gil_safe_call_once.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index e3a78d4482..5b824de59f 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -80,11 +80,12 @@ class gil_safe_call_once_and_store { PYBIND11_DTOR_CONSTEXPR ~gil_safe_call_once_and_store() = default; private: - // `is_initialized_` below and `storage_` here can be replaced with `std::optional` - // when pybind11 drops C++11 support. alignas(T) char storage_[sizeof(T)] = {}; std::once_flag once_flag_ = {}; bool is_initialized_ = false; + // The `is_initialized_`-`storage_` pair is very similar to `std::optional`, + // but the latter does not have the triviality properties of former, + // therefore `std::optional` is not a viable alternative here. }; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) From 66bbc67323d21ea666eb01bba37079762646055c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 11 Oct 2023 03:55:56 -0700 Subject: [PATCH 30/31] Additional comment with the conclusion of a discussion under PR #4877. * https://github.com/pybind/pybind11/pull/4877#issuecomment-1757363179 --- include/pybind11/gil_safe_call_once.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index 5b824de59f..d574508e41 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -58,6 +58,8 @@ class gil_safe_call_once_and_store { ::new (storage_) T(fn()); is_initialized_ = true; // This write is guarded by the GIL. }); + // The C++ standard guarantees that all threads entering above will observe + // `is_initialized_` as true here. } // Intentionally not returning `T &` to ensure the calling code is self-documenting. return *this; From 7cd939026115ac56dfd2d60a09a09bf3883b8025 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 11 Oct 2023 16:06:20 -0700 Subject: [PATCH 31/31] Small comment changes suggested by @tkoeppe. --- include/pybind11/gil_safe_call_once.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index d574508e41..eaf84d16e8 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -49,24 +49,22 @@ class gil_safe_call_once_and_store { template gil_safe_call_once_and_store &call_once_and_store_result(Callable &&fn) { if (!is_initialized_) { // This read is guarded by the GIL. - // Multiple threads may enter here, because CPython API calls in the - // `fn()` call below may release and reacquire the GIL. + // Multiple threads may enter here, because the GIL is released in the next line and + // CPython API calls in the `fn()` call below may release and reacquire the GIL. gil_scoped_release gil_rel; // Needed to establish lock ordering. std::call_once(once_flag_, [&] { // Only one thread will ever enter here. gil_scoped_acquire gil_acq; - ::new (storage_) T(fn()); - is_initialized_ = true; // This write is guarded by the GIL. + ::new (storage_) T(fn()); // fn may release, but will reacquire, the GIL. + is_initialized_ = true; // This write is guarded by the GIL. }); - // The C++ standard guarantees that all threads entering above will observe - // `is_initialized_` as true here. + // All threads will observe `is_initialized_` as true here. } // Intentionally not returning `T &` to ensure the calling code is self-documenting. return *this; } // This must only be called after `call_once_and_store_result()` was called. - // Not const for simplicity. (Could be made const if there is an unforeseen need.) T &get_stored() { assert(is_initialized_); PYBIND11_WARNING_PUSH