Eliminate duplicate TLS keys for loader_life_support stack (#3275)

* Eliminate duplicate TLS keys for loader_life_support stack

This revises the existing fix for
https://github.com/pybind/pybind11/issues/2765 in
https://github.com/pybind/pybind11/pull/3237 to reduce the amount of
TLS storage used.

The shared TLS key is stored in two different ways, depending on
`PYBIND11_INTERNALS_VERSION`.  If `PYBIND11_INTERNALS_VERSION ==
4` (as is currently set), the TLS key is stored in the
`internal::shared_data` map to avoid breaking ABI compatibility.  If
`PYBIND11_INTERNALS_VERSION > 4`, the TLS key is stored directly in
the `internals` struct.

* Fix test_pytypes.py::test_issue2361 failure on PyPy3.7

* Add github actions tests for unstable ABI

.github/workflows/ci.yml

@@ -144,6 +144,24 @@ jobs:
       if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))"
       if: "!(runner.os == 'Windows' && (matrix.python == 3.8 || matrix.python == 3.9 || matrix.python == '3.10-dev'))"
       run: cmake --build build2 --target cpptest
       run: cmake --build build2 --target cpptest
 
 
+    # Third build - C++17 mode with unstable ABI
+    - name: Configure (unstable ABI)
+      run: >
+        cmake -S . -B build3
+        -DPYBIND11_WERROR=ON
+        -DDOWNLOAD_CATCH=ON
+        -DDOWNLOAD_EIGEN=ON
+        -DCMAKE_CXX_STANDARD=17
+        -DPYBIND11_INTERNALS_VERSION=10000000
+        "-DPYBIND11_TEST_OVERRIDE=test_call_policies.cpp;test_gil_scoped.cpp;test_thread.cpp"
+        ${{ matrix.args }}
+
+    - name: Build (unstable ABI)
+      run: cmake --build build3 -j 2
+
+    - name: Python tests (unstable ABI)
+      run: cmake --build build3 --target pytest
+
     - name: Interface test
     - name: Interface test
       run: cmake --build build2 --target test_cmake_build
       run: cmake --build build2 --target test_cmake_build
 
 

CMakeLists.txt

@@ -89,6 +89,9 @@ endif()
 option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT})
 option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT})
 option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT})
 option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT})
 option(PYBIND11_NOPYTHON "Disable search for Python" OFF)
 option(PYBIND11_NOPYTHON "Disable search for Python" OFF)
+set(PYBIND11_INTERNALS_VERSION
+    ""
+    CACHE STRING "Override the ABI version, may be used to enable the unstable ABI.")
 
 
 cmake_dependent_option(
 cmake_dependent_option(
   USE_PYTHON_INCLUDE_DIR
   USE_PYTHON_INCLUDE_DIR
@@ -183,6 +186,10 @@ if(NOT TARGET pybind11_headers)
 
 
   target_compile_features(pybind11_headers INTERFACE cxx_inheriting_constructors cxx_user_literals
   target_compile_features(pybind11_headers INTERFACE cxx_inheriting_constructors cxx_user_literals
                                                      cxx_right_angle_brackets)
                                                      cxx_right_angle_brackets)
+  if(NOT "${PYBIND11_INTERNALS_VERSION}" STREQUAL "")
+    target_compile_definitions(
+      pybind11_headers INTERFACE "PYBIND11_INTERNALS_VERSION=${PYBIND11_INTERNALS_VERSION}")
+  endif()
 else()
 else()
   # It is invalid to install a target twice, too.
   # It is invalid to install a target twice, too.
   set(PYBIND11_INSTALL OFF)
   set(PYBIND11_INSTALL OFF)

include/pybind11/detail/type_caster_base.h

@@ -35,30 +35,43 @@ private:
     loader_life_support* parent = nullptr;
     loader_life_support* parent = nullptr;
     std::unordered_set<PyObject *> keep_alive;
     std::unordered_set<PyObject *> keep_alive;
 
 
-    static loader_life_support** get_stack_pp() {
 #if defined(WITH_THREAD)
 #if defined(WITH_THREAD)
-        thread_local static loader_life_support* per_thread_stack = nullptr;
-        return &per_thread_stack;
+    // Store stack pointer in thread-local storage.
+    static PYBIND11_TLS_KEY_REF get_stack_tls_key() {
+#    if PYBIND11_INTERNALS_VERSION == 4
+        return get_local_internals().loader_life_support_tls_key;
+#    else
+        return get_internals().loader_life_support_tls_key;
+#    endif
+    }
+    static loader_life_support *get_stack_top() {
+        return static_cast<loader_life_support *>(PYBIND11_TLS_GET_VALUE(get_stack_tls_key()));
+    }
+    static void set_stack_top(loader_life_support *value) {
+        PYBIND11_TLS_REPLACE_VALUE(get_stack_tls_key(), value);
+    }
 #else
 #else
-        static loader_life_support* global_stack = nullptr;
-        return &global_stack;
-#endif
+    // Use single global variable for stack.
+    static loader_life_support **get_stack_pp() {
+        static loader_life_support *global_stack = nullptr;
+        return global_stack;
     }
     }
+    static loader_life_support *get_stack_top() { return *get_stack_pp(); }
+    static void set_stack_top(loader_life_support *value) { *get_stack_pp() = value; }
+#endif
 
 
 public:
 public:
     /// A new patient frame is created when a function is entered
     /// A new patient frame is created when a function is entered
     loader_life_support() {
     loader_life_support() {
-        loader_life_support** stack = get_stack_pp();
-        parent = *stack;
-        *stack = this;
+        parent = get_stack_top();
+        set_stack_top(this);
     }
     }
 
 
     /// ... and destroyed after it returns
     /// ... and destroyed after it returns
     ~loader_life_support() {
     ~loader_life_support() {
-        loader_life_support** stack = get_stack_pp();
-        if (*stack != this)
+        if (get_stack_top() != this)
             pybind11_fail("loader_life_support: internal error");
             pybind11_fail("loader_life_support: internal error");
-        *stack = parent;
+        set_stack_top(parent);
         for (auto* item : keep_alive)
         for (auto* item : keep_alive)
             Py_DECREF(item);
             Py_DECREF(item);
     }
     }
@@ -66,7 +79,7 @@ public:
     /// This can only be used inside a pybind11-bound function, either by `argument_loader`
     /// This can only be used inside a pybind11-bound function, either by `argument_loader`
     /// at argument preparation time or by `py::cast()` at execution time.
     /// at argument preparation time or by `py::cast()` at execution time.
     PYBIND11_NOINLINE static void add_patient(handle h) {
     PYBIND11_NOINLINE static void add_patient(handle h) {
-        loader_life_support* frame =  *get_stack_pp();
+        loader_life_support *frame = get_stack_top();
         if (!frame) {
         if (!frame) {
             // NOTE: It would be nice to include the stack frames here, as this indicates
             // NOTE: It would be nice to include the stack frames here, as this indicates
             // use of pybind11::cast<> outside the normal call framework, finding such
             // use of pybind11::cast<> outside the normal call framework, finding such

include/pybind11/gil.h

@@ -50,7 +50,7 @@ PYBIND11_NAMESPACE_END(detail)
 class gil_scoped_acquire {
 class gil_scoped_acquire {
 public:
 public:
     PYBIND11_NOINLINE gil_scoped_acquire() {
     PYBIND11_NOINLINE gil_scoped_acquire() {
-        auto const &internals = detail::get_internals();
+        auto &internals = detail::get_internals();
         tstate = (PyThreadState *) PYBIND11_TLS_GET_VALUE(internals.tstate);
         tstate = (PyThreadState *) PYBIND11_TLS_GET_VALUE(internals.tstate);
 
 
         if (!tstate) {
         if (!tstate) {
@@ -132,7 +132,7 @@ public:
         // `get_internals()` must be called here unconditionally in order to initialize
         // `get_internals()` must be called here unconditionally in order to initialize
         // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
         // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
         // initialization race could occur as multiple threads try `gil_scoped_acquire`.
         // initialization race could occur as multiple threads try `gil_scoped_acquire`.
-        const auto &internals = detail::get_internals();
+        auto &internals = detail::get_internals();
         tstate = PyEval_SaveThread();
         tstate = PyEval_SaveThread();
         if (disassoc) {
         if (disassoc) {
             auto key = internals.tstate;
             auto key = internals.tstate;

tests/test_pytypes.py

@@ -467,7 +467,8 @@ def test_issue2361():
     assert m.issue2361_str_implicit_copy_none() == "None"
     assert m.issue2361_str_implicit_copy_none() == "None"
     with pytest.raises(TypeError) as excinfo:
     with pytest.raises(TypeError) as excinfo:
         assert m.issue2361_dict_implicit_copy_none()
         assert m.issue2361_dict_implicit_copy_none()
-    assert "'NoneType' object is not iterable" in str(excinfo.value)
+    assert "NoneType" in str(excinfo.value)
+    assert "iterable" in str(excinfo.value)
 
 
 
 
 @pytest.mark.parametrize(
 @pytest.mark.parametrize(