Adding virtual_overrider_self_life_support. (#2902)

rwgk · web-flow · commit 469792032a77 · 2021-03-16T06:31:24.000-07:00
* Initial version of virtual_overrider_self_life_support (enables safely passing unique_ptr to C++).

* Clang 3.6, 3.7 compatibility.

* Adding missing default constructor.

* Restoring test for exception for the case that virtual_overrider_self_life_support is not used.

* Fixing oversight: Adding missing holder().ensure_was_not_disowned().

* Adding unit tests for new `struct smart_holder` member functions.

* Moving virtual_overrider_self_life_support to separate include file, with iwyu cleanup.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -110,6 +110,7 @@ set(PYBIND11_HEADERS
     include/pybind11/detail/smart_holder_type_casters.h
     include/pybind11/detail/type_caster_base.h
     include/pybind11/detail/typeid.h
+    include/pybind11/detail/virtual_overrider_self_life_support.h
     include/pybind11/attr.h
     include/pybind11/buffer_info.h
     include/pybind11/cast.h
diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h
@@ -95,13 +95,14 @@ inline bool is_std_default_delete(const std::type_info &rtti_deleter) {
 }
 
 struct smart_holder {
-    const std::type_info *rtti_uqp_del;
+    const std::type_info *rtti_uqp_del = nullptr;
     std::shared_ptr<bool> vptr_deleter_armed_flag_ptr;
     std::shared_ptr<void> vptr;
     bool vptr_is_using_noop_deleter : 1;
     bool vptr_is_using_builtin_delete : 1;
     bool vptr_is_external_shared_ptr : 1;
     bool is_populated : 1;
+    bool was_disowned : 1;
     bool pointee_depends_on_holder_owner : 1; // SMART_HOLDER_WIP: See PR #2839.
 
     // Design choice: smart_holder is movable but not copyable.
@@ -111,15 +112,15 @@ struct smart_holder {
     smart_holder &operator=(const smart_holder &) = delete;
 
     smart_holder()
-        : rtti_uqp_del{nullptr}, vptr_is_using_noop_deleter{false},
-          vptr_is_using_builtin_delete{false}, vptr_is_external_shared_ptr{false},
-          is_populated{false}, pointee_depends_on_holder_owner{false} {}
+        : vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
+          vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
+          pointee_depends_on_holder_owner{false} {}
 
     explicit smart_holder(bool vptr_deleter_armed_flag)
-        : rtti_uqp_del{nullptr}, vptr_deleter_armed_flag_ptr{new 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}, pointee_depends_on_holder_owner{
-                                                                       false} {}
+          vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
+          pointee_depends_on_holder_owner{false} {}
 
     bool has_pointee() const { return vptr.get() != nullptr; }
 
@@ -135,6 +136,12 @@ struct smart_holder {
             throw std::runtime_error(std::string("Unpopulated holder (") + context + ").");
         }
     }
+    void ensure_was_not_disowned(const char *context) const {
+        if (was_disowned) {
+            throw std::runtime_error(std::string("Holder was disowned already (") + context
+                                     + ").");
+        }
+    }
 
     void ensure_vptr_is_using_builtin_delete(const char *context) const {
         if (vptr_is_external_shared_ptr) {
@@ -227,16 +234,29 @@ struct smart_holder {
         return hld;
     }
 
+    // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
+    void disown() {
+        *vptr_deleter_armed_flag_ptr = false;
+        was_disowned                 = true;
+    }
+
+    // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
+    void release_disowned() {
+        vptr.reset();
+        vptr_deleter_armed_flag_ptr.reset();
+    }
+
+    // SMART_HOLDER_WIP: review this function.
     void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") {
+        ensure_was_not_disowned(context);
         ensure_vptr_is_using_builtin_delete(context);
         ensure_use_count_1(context);
     }
 
-    // Caller is responsible for calling ensure_can_release_ownership().
+    // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
     void release_ownership() {
         *vptr_deleter_armed_flag_ptr = false;
-        vptr.reset();
-        vptr_deleter_armed_flag_ptr.reset();
+        release_disowned();
     }
 
     template <typename T>
diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h
@@ -13,6 +13,7 @@
 #include "smart_holder_sfinae_hooks_only.h"
 #include "type_caster_base.h"
 #include "typeid.h"
+#include "virtual_overrider_self_life_support.h"
 
 #include <cstddef>
 #include <memory>
@@ -352,6 +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");
         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) {
@@ -373,28 +375,44 @@ struct smart_holder_type_caster_load {
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
+        holder().ensure_was_not_disowned(context);
         holder().template ensure_compatible_rtti_uqp_del<T, D>(context);
         holder().ensure_use_count_1(context);
-        if (holder().pointee_depends_on_holder_owner) {
-            throw value_error("Ownership of instance with virtual overrides in Python cannot be "
-                              "transferred to C++.");
-        }
         auto raw_void_ptr = holder().template as_raw_ptr_unowned<void>();
+
+        void *value_void_ptr = load_impl.loaded_v_h.value_ptr();
+        if (value_void_ptr != raw_void_ptr) {
+            pybind11_fail("smart_holder_type_casters: loaded_as_unique_ptr failure:"
+                          " value_void_ptr != raw_void_ptr");
+        }
+
         // SMART_HOLDER_WIP: MISSING: Safety checks for type conversions
         // (T must be polymorphic or meet certain other conditions).
         T *raw_type_ptr = convert_type(raw_void_ptr);
 
+        auto *self_life_support
+            = dynamic_cast_virtual_overrider_self_life_support_ptr(raw_type_ptr);
+        if (self_life_support == nullptr && holder().pointee_depends_on_holder_owner) {
+            throw value_error("Ownership of instance with virtual overrides in Python cannot be "
+                              "transferred to C++.");
+        }
+
         // Critical transfer-of-ownership section. This must stay together.
-        holder().release_ownership();
+        if (self_life_support != nullptr) {
+            holder().disown();
+        } else {
+            holder().release_ownership();
+        }
         auto result = std::unique_ptr<T, D>(raw_type_ptr);
-
-        void *value_void_ptr = load_impl.loaded_v_h.value_ptr();
-        if (value_void_ptr != raw_void_ptr) {
-            pybind11_fail("smart_holder_type_casters: loaded_as_unique_ptr failure:"
-                          " value_void_ptr != raw_void_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;
+        } else {
+            load_impl.loaded_v_h.value_ptr() = nullptr;
+            deregister_instance(
+                load_impl.loaded_v_h.inst, value_void_ptr, load_impl.loaded_v_h.type);
         }
-        load_impl.loaded_v_h.value_ptr() = nullptr;
-        deregister_instance(load_impl.loaded_v_h.inst, value_void_ptr, load_impl.loaded_v_h.type);
+        // Critical section end.
 
         return result;
     }
diff --git a/include/pybind11/detail/virtual_overrider_self_life_support.h b/include/pybind11/detail/virtual_overrider_self_life_support.h
@@ -0,0 +1,62 @@
+// 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 "common.h"
+#include "smart_holder_poc.h"
+#include "type_caster_base.h"
+
+#include <type_traits>
+
+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);
+
+// 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 virtual_overrider_self_life_support {
+    value_and_holder loaded_v_h;
+    ~virtual_overrider_self_life_support() {
+        if (loaded_v_h.inst != nullptr && loaded_v_h.vh != nullptr) {
+            void *value_void_ptr = loaded_v_h.value_ptr();
+            if (value_void_ptr != nullptr) {
+                PyGILState_STATE threadstate = PyGILState_Ensure();
+                Py_DECREF((PyObject *) loaded_v_h.inst);
+                loaded_v_h.value_ptr() = nullptr;
+                loaded_v_h.holder<pybindit::memory::smart_holder>().release_disowned();
+                deregister_instance(loaded_v_h.inst, value_void_ptr, loaded_v_h.type);
+                PyGILState_Release(threadstate);
+            }
+        }
+    }
+
+    // Some compilers complain about implicitly defined versions of some of the following:
+    virtual_overrider_self_life_support()                                            = default;
+    virtual_overrider_self_life_support(const virtual_overrider_self_life_support &) = default;
+    virtual_overrider_self_life_support(virtual_overrider_self_life_support &&)      = default;
+    virtual_overrider_self_life_support &operator=(const virtual_overrider_self_life_support &)
+        = default;
+    virtual_overrider_self_life_support &operator=(virtual_overrider_self_life_support &&)
+        = default;
+};
+
+template <typename T, detail::enable_if_t<!std::is_polymorphic<T>::value, int> = 0>
+virtual_overrider_self_life_support *
+dynamic_cast_virtual_overrider_self_life_support_ptr(T * /*raw_type_ptr*/) {
+    return nullptr;
+}
+
+template <typename T, detail::enable_if_t<std::is_polymorphic<T>::value, int> = 0>
+virtual_overrider_self_life_support *
+dynamic_cast_virtual_overrider_self_life_support_ptr(T *raw_type_ptr) {
+    return dynamic_cast<virtual_overrider_self_life_support *>(raw_type_ptr);
+}
+
+PYBIND11_NAMESPACE_END(detail)
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py
@@ -48,6 +48,7 @@
     "include/pybind11/detail/smart_holder_type_casters.h",
     "include/pybind11/detail/type_caster_base.h",
     "include/pybind11/detail/typeid.h",
+    "include/pybind11/detail/virtual_overrider_self_life_support.h",
 }
 
 cmake_files = {
diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp
@@ -129,6 +129,26 @@ TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
     REQUIRE(*new_owner == 19);
 }
 
+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>());
+    hld.disown();
+    REQUIRE(hld.as_lvalue_ref<int>() == 19);
+    REQUIRE(*new_owner == 19);
+    hld.release_disowned();
+    REQUIRE(!hld.has_pointee());
+}
+
+TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_was_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.
+    std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
+    hld.disown();
+    REQUIRE_THROWS_WITH(hld.ensure_was_not_disowned(context),
+                        "Holder was disowned already (test_case).");
+}
+
 TEST_CASE("from_unique_ptr+as_lvalue_ref", "[S]") {
     std::unique_ptr<int> orig_owner(new int(19));
     auto hld = smart_holder::from_unique_ptr(std::move(orig_owner));
diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp
@@ -7,6 +7,7 @@
 namespace pybind11_tests {
 namespace test_class_sh_with_alias {
 
+template <int SerNo> // Using int as a trick to easily generate a series of types.
 struct Abase {
     int val          = 0;
     virtual ~Abase() = default;
@@ -21,41 +22,65 @@ struct Abase {
     Abase &operator=(Abase &&) = default;
 };
 
-struct AbaseAlias : Abase {
-    using Abase::Abase;
+template <int SerNo>
+struct AbaseAlias : Abase<SerNo> {
+    using Abase<SerNo>::Abase;
 
     int Add(int other_val) const override {
-        PYBIND11_OVERRIDE_PURE(int,   /* Return type */
-                               Abase, /* Parent class */
-                               Add,   /* Name of function in C++ (must match Python name) */
+        PYBIND11_OVERRIDE_PURE(int,          /* Return type */
+                               Abase<SerNo>, /* Parent class */
+                               Add,          /* Name of function in C++ (must match Python name) */
                                other_val);
     }
 };
 
-int AddInCppRawPtr(const Abase *obj, int other_val) { return obj->Add(other_val) * 10 + 7; }
+template <>
+struct AbaseAlias<1> : Abase<1>, py::detail::virtual_overrider_self_life_support {
+    using Abase<1>::Abase;
 
-int AddInCppSharedPtr(std::shared_ptr<Abase> obj, int other_val) {
+    int Add(int other_val) const override {
+        PYBIND11_OVERRIDE_PURE(int,      /* Return type */
+                               Abase<1>, /* Parent class */
+                               Add,      /* Name of function in C++ (must match Python name) */
+                               other_val);
+    }
+};
+
+template <int SerNo>
+int AddInCppRawPtr(const Abase<SerNo> *obj, int other_val) {
+    return obj->Add(other_val) * 10 + 7;
+}
+
+template <int SerNo>
+int AddInCppSharedPtr(std::shared_ptr<Abase<SerNo>> obj, int other_val) {
     return obj->Add(other_val) * 100 + 11;
 }
 
-int AddInCppUniquePtr(std::unique_ptr<Abase> obj, int other_val) {
+template <int SerNo>
+int AddInCppUniquePtr(std::unique_ptr<Abase<SerNo>> obj, int other_val) {
     return obj->Add(other_val) * 100 + 13;
 }
 
+template <int SerNo>
+void wrap(py::module_ m, const char *py_class_name) {
+    py::classh<Abase<SerNo>, AbaseAlias<SerNo>>(m, py_class_name)
+        .def(py::init<int>(), py::arg("val"))
+        .def("Get", &Abase<SerNo>::Get)
+        .def("Add", &Abase<SerNo>::Add, py::arg("other_val"));
+
+    m.def("AddInCppRawPtr", AddInCppRawPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
+    m.def("AddInCppSharedPtr", AddInCppSharedPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
+    m.def("AddInCppUniquePtr", AddInCppUniquePtr<SerNo>, py::arg("obj"), py::arg("other_val"));
+}
+
 } // namespace test_class_sh_with_alias
 } // namespace pybind11_tests
 
-PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<0>)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<1>)
 
 TEST_SUBMODULE(class_sh_with_alias, m) {
     using namespace pybind11_tests::test_class_sh_with_alias;
-
-    py::classh<Abase, AbaseAlias>(m, "Abase")
-        .def(py::init<int>(), py::arg("val"))
-        .def("Get", &Abase::Get)
-        .def("Add", &Abase::Add, py::arg("other_val"));
-
-    m.def("AddInCppRawPtr", AddInCppRawPtr, py::arg("obj"), py::arg("other_val"));
-    m.def("AddInCppSharedPtr", AddInCppSharedPtr, py::arg("obj"), py::arg("other_val"));
-    m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val"));
+    wrap<0>(m, "Abase0");
+    wrap<1>(m, "Abase1");
 }
diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py

Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,7 @@`
`48`	`48`	`"include/pybind11/detail/smart_holder_type_casters.h",`
`49`	`49`	`"include/pybind11/detail/type_caster_base.h",`
`50`	`50`	`"include/pybind11/detail/typeid.h",`
	`51`	`+ "include/pybind11/detail/virtual_overrider_self_life_support.h",`
`51`	`52`	`}`
`52`	`53`
`53`	`54`	`cmake_files = {`