Adding reclaim_disowned logic & miscellaneous naming and documentation improvements.

CMakeLists.txt

@@ -131,7 +131,7 @@ set(PYBIND11_HEADERS
     include/pybind11/smart_holder.h
     include/pybind11/stl.h
     include/pybind11/stl_bind.h
-    include/pybind11/virtual_overrider_self_life_support.h)
+    include/pybind11/trampoline_self_life_support.h)
 
 # Compare with grep and warn if mismatched
 if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)

README_smart_holder.rst

@@ -164,6 +164,26 @@ of interest have made the switch, because then the code will continue to
 work in either mode.
 
 
+Trampolines and std::unique_ptr
+-------------------------------
+
+A pybind11 `"trampoline"
+<https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python>`_
+is a C++ helper class with virtual function overrides that transparently
+call back from C++ into Python. To enable safely passing a ``std::unique_ptr``
+to a trampoline object between Python and C++, the trampoline class must
+inherit from ``py::trampoline_self_life_support``, for example:
+
+.. code-block:: cpp
+
+   class PyAnimal : public Animal, public py::trampoline_self_life_support {
+       ...
+   };
+
+This is the only difference compared to traditional pybind11. A fairly
+minimal but complete example is tests/test_class_sh_trampoline_unique_ptr.cpp.
+
+
 Ideas for the long-term
 -----------------------
 

include/pybind11/detail/smart_holder_poc.h

@@ -102,7 +102,7 @@ struct smart_holder {
     bool vptr_is_using_builtin_delete : 1;
     bool vptr_is_external_shared_ptr : 1;
     bool is_populated : 1;
-    bool was_disowned : 1;
+    bool is_disowned : 1;
     bool pointee_depends_on_holder_owner : 1; // SMART_HOLDER_WIP: See PR #2839.
 
     // Design choice: smart_holder is movable but not copyable.
@@ -113,13 +113,13 @@ struct smart_holder {
 
     smart_holder()
         : vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
-          vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
+          vptr_is_external_shared_ptr{false}, is_populated{false}, is_disowned{false},
           pointee_depends_on_holder_owner{false} {}
 
     explicit smart_holder(bool vptr_deleter_armed_flag)
         : vptr_deleter_armed_flag_ptr{new bool{vptr_deleter_armed_flag}},
           vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
-          vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
+          vptr_is_external_shared_ptr{false}, is_populated{false}, is_disowned{false},
           pointee_depends_on_holder_owner{false} {}
 
     bool has_pointee() const { return vptr.get() != nullptr; }
@@ -136,8 +136,8 @@ struct smart_holder {
             throw std::runtime_error(std::string("Unpopulated holder (") + context + ").");
         }
     }
-    void ensure_was_not_disowned(const char *context) const {
-        if (was_disowned) {
+    void ensure_is_not_disowned(const char *context) const {
+        if (is_disowned) {
             throw std::runtime_error(std::string("Holder was disowned already (") + context
                                      + ").");
         }
@@ -237,7 +237,13 @@ struct smart_holder {
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
     void disown() {
         *vptr_deleter_armed_flag_ptr = false;
-        was_disowned                 = true;
+        is_disowned                  = true;
+    }
+
+    // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
+    void reclaim_disowned() {
+        *vptr_deleter_armed_flag_ptr = true;
+        is_disowned                  = false;
     }
 
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
@@ -248,7 +254,7 @@ struct smart_holder {
 
     // SMART_HOLDER_WIP: review this function.
     void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") {
-        ensure_was_not_disowned(context);
+        ensure_is_not_disowned(context);
         ensure_vptr_is_using_builtin_delete(context);
         ensure_use_count_1(context);
     }

include/pybind11/detail/smart_holder_type_casters.h

@@ -6,7 +6,7 @@
 
 #include "../gil.h"
 #include "../pytypes.h"
-#include "../virtual_overrider_self_life_support.h"
+#include "../trampoline_self_life_support.h"
 #include "common.h"
 #include "descr.h"
 #include "dynamic_raw_ptr_cast_if_possible.h"
@@ -353,7 +353,7 @@ struct smart_holder_type_caster_load {
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
-        holder().ensure_was_not_disowned("loaded_as_shared_ptr");
+        holder().ensure_is_not_disowned("loaded_as_shared_ptr");
         auto void_raw_ptr = holder().template as_raw_ptr_unowned<void>();
         auto type_raw_ptr = convert_type(void_raw_ptr);
         if (holder().pointee_depends_on_holder_owner) {
@@ -375,7 +375,7 @@ struct smart_holder_type_caster_load {
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
-        holder().ensure_was_not_disowned(context);
+        holder().ensure_is_not_disowned(context);
         holder().template ensure_compatible_rtti_uqp_del<T, D>(context);
         holder().ensure_use_count_1(context);
         auto raw_void_ptr = holder().template as_raw_ptr_unowned<void>();
@@ -391,11 +391,11 @@ struct smart_holder_type_caster_load {
         T *raw_type_ptr = convert_type(raw_void_ptr);
 
         auto *self_life_support
-            = dynamic_raw_ptr_cast_if_possible<virtual_overrider_self_life_support>(raw_type_ptr);
+            = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(raw_type_ptr);
         if (self_life_support == nullptr && holder().pointee_depends_on_holder_owner) {
             throw value_error("Alias class (also known as trampoline) does not inherit from "
-                              "py::virtual_overrider_self_life_support, therefore the "
-                              "ownership of this instance cannot safely be transferred to C++.");
+                              "py::trampoline_self_life_support, therefore the ownership of this "
+                              "instance cannot safely be transferred to C++.");
         }
 
         // Critical transfer-of-ownership section. This must stay together.
@@ -406,8 +406,7 @@ struct smart_holder_type_caster_load {
         }
         auto result = std::unique_ptr<T, D>(raw_type_ptr);
         if (self_life_support != nullptr) {
-            Py_INCREF((PyObject *) load_impl.loaded_v_h.inst);
-            self_life_support->loaded_v_h = load_impl.loaded_v_h;
+            self_life_support->activate_life_support(load_impl.loaded_v_h);
         } else {
             load_impl.loaded_v_h.value_ptr() = nullptr;
             deregister_instance(
@@ -700,8 +699,27 @@ struct smart_holder_type_caster<std::unique_ptr<T, D>> : smart_holder_type_caste
 
         void *src_raw_void_ptr         = static_cast<void *>(src_raw_ptr);
         const detail::type_info *tinfo = st.second;
-        if (find_registered_python_instance(src_raw_void_ptr, tinfo))
+        if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) {
+            auto *self_life_support
+                = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(src_raw_ptr);
+            if (self_life_support != nullptr) {
+                value_and_holder &v_h = self_life_support->v_h;
+                if (v_h.inst != nullptr && v_h.vh != nullptr) {
+                    auto &holder = v_h.holder<pybindit::memory::smart_holder>();
+                    if (!holder.is_disowned) {
+                        pybind11_fail("smart_holder_type_casters: unexpected "
+                                      "smart_holder.is_disowned failure.");
+                    }
+                    // Critical transfer-of-ownership section. This must stay together.
+                    self_life_support->deactivate_life_support();
+                    holder.reclaim_disowned();
+                    src.release();
+                    // Critical section end.
+                    return existing_inst;
+                }
+            }
             throw cast_error("Invalid unique_ptr: another instance owns this pointer already.");
+        }
 
         auto inst           = reinterpret_steal<object>(make_new_instance(tinfo->type));
         auto *inst_raw_ptr  = reinterpret_cast<instance *>(inst.ptr());

include/pybind11/trampoline_self_life_support.h

@@ -0,0 +1,57 @@
+// Copyright (c) 2021 The Pybind Development Team.
+// All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#pragma once
+
+#include "detail/common.h"
+#include "detail/smart_holder_poc.h"
+#include "detail/type_caster_base.h"
+
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
+
+PYBIND11_NAMESPACE_BEGIN(detail)
+// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code.
+inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
+PYBIND11_NAMESPACE_END(detail)
+
+// The original core idea for this struct goes back to PyCLIF:
+// https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37
+// URL provided here mainly to give proper credit. To fully explain the `HoldPyObj` feature, more
+// context is needed (SMART_HOLDER_WIP).
+struct trampoline_self_life_support {
+    detail::value_and_holder v_h;
+
+    void activate_life_support(const detail::value_and_holder &v_h_) {
+        Py_INCREF((PyObject *) v_h_.inst);
+        v_h = v_h_;
+    }
+
+    void deactivate_life_support() {
+        Py_DECREF((PyObject *) v_h.inst);
+        v_h = detail::value_and_holder();
+    }
+
+    ~trampoline_self_life_support() {
+        if (v_h.inst != nullptr && v_h.vh != nullptr) {
+            void *value_void_ptr = v_h.value_ptr();
+            if (value_void_ptr != nullptr) {
+                PyGILState_STATE threadstate = PyGILState_Ensure();
+                v_h.value_ptr()              = nullptr;
+                v_h.holder<pybindit::memory::smart_holder>().release_disowned();
+                detail::deregister_instance(v_h.inst, value_void_ptr, v_h.type);
+                Py_DECREF((PyObject *) v_h.inst); // Must be after deregister.
+                PyGILState_Release(threadstate);
+            }
+        }
+    }
+
+    // Some compilers complain about implicitly defined versions of some of the following:
+    trampoline_self_life_support()                                     = default;
+    trampoline_self_life_support(const trampoline_self_life_support &) = default;
+    trampoline_self_life_support(trampoline_self_life_support &&)      = default;
+    trampoline_self_life_support &operator=(const trampoline_self_life_support &) = default;
+    trampoline_self_life_support &operator=(trampoline_self_life_support &&) = default;
+};
+
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

tests/CMakeLists.txt

@@ -106,11 +106,11 @@ set(PYBIND11_TEST_FILES
     test_class_sh_disowning_mi.cpp
     test_class_sh_factory_constructors.cpp
     test_class_sh_inheritance.cpp
+    test_class_sh_trampoline_basic.cpp
     test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
     test_class_sh_trampoline_unique_ptr.cpp
     test_class_sh_unique_ptr_member.cpp
     test_class_sh_virtual_py_cpp_mix.cpp
-    test_class_sh_with_alias.cpp
     test_constants_and_functions.cpp
     test_copy_move.cpp
     test_custom_type_casters.cpp

tests/extra_python_package/test_files.py

@@ -35,7 +35,7 @@
     "include/pybind11/smart_holder.h",
     "include/pybind11/stl.h",
     "include/pybind11/stl_bind.h",
-    "include/pybind11/virtual_overrider_self_life_support.h",
+    "include/pybind11/trampoline_self_life_support.h",
 }
 
 detail_headers = {

tests/pure_cpp/smart_holder_poc_test.cpp

@@ -129,6 +129,20 @@ TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
     REQUIRE(*new_owner == 19);
 }
 
+TEST_CASE("from_raw_ptr_take_ownership+disown+reclaim_disowned", "[S]") {
+    auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
+    std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
+    hld.disown();
+    REQUIRE(hld.as_lvalue_ref<int>() == 19);
+    REQUIRE(*new_owner == 19);
+    hld.reclaim_disowned(); // Manually veriified: without this, clang++ -fsanitize=address reports
+                            // "detected memory leaks".
+    new_owner.release();    // Manually verified: without this, clang++ -fsanitize=address reports
+                            // "attempting double-free".
+    REQUIRE(hld.as_lvalue_ref<int>() == 19);
+    REQUIRE(new_owner.get() == nullptr);
+}
+
 TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
     auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
     std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
@@ -139,13 +153,13 @@ TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
     REQUIRE(!hld.has_pointee());
 }
 
-TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_was_not_disowned", "[E]") {
+TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_is_not_disowned", "[E]") {
     const char *context = "test_case";
     auto hld            = smart_holder::from_raw_ptr_take_ownership(new int(19));
-    hld.ensure_was_not_disowned(context); // Does not throw.
+    hld.ensure_is_not_disowned(context); // Does not throw.
     std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
     hld.disown();
-    REQUIRE_THROWS_WITH(hld.ensure_was_not_disowned(context),
+    REQUIRE_THROWS_WITH(hld.ensure_is_not_disowned(context),
                         "Holder was disowned already (test_case).");
 }
 

tests/test_class_sh_disowning.py

@@ -41,7 +41,7 @@ def test_mixed():
             # the already disowned obj1a fails as expected.
             m.mixed(obj1a, obj2b)
 
-        def was_disowned(obj):
+        def is_disowned(obj):
             try:
                 obj.get()
             except ValueError:
@@ -50,13 +50,13 @@ def was_disowned(obj):
 
         # Either obj1b or obj2b was disowned in the expected failed m.mixed() calls above, but not
         # both.
-        was_disowned_results = (was_disowned(obj1b), was_disowned(obj2b))
-        assert was_disowned_results.count(True) == 1
+        is_disowned_results = (is_disowned(obj1b), is_disowned(obj2b))
+        assert is_disowned_results.count(True) == 1
         if first_pass:
             first_pass = False
             print(
                 "\nC++ function argument %d is evaluated first."
-                % (was_disowned_results.index(True) + 1)
+                % (is_disowned_results.index(True) + 1)
             )
 
         return  # Comment out for manual leak checking (use `top` command).