Porting/adapting Dustin's PR #2839 to smart_holder branch (#2886)

rwgk · web-flow · commit 97a7fb722a98 · 2021-03-03T17:58:42.000-08:00
* WIP: test setup complete, AddInCppUniquePtr failing (reproduces PyCLIF smart_ptrs_test failure).

* Fully tested locally.

* Adding new tests to cmake file.
diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h
@@ -102,6 +102,7 @@ struct smart_holder {
     bool vptr_is_using_builtin_delete : 1;
     bool vptr_is_external_shared_ptr : 1;
     bool is_populated : 1;
+    bool pointee_depends_on_holder_owner : 1; // SMART_HOLDER_WIP: See PR #2839.
 
     // Design choice: smart_holder is movable but not copyable.
     smart_holder(smart_holder &&)      = default;
@@ -111,13 +112,14 @@ struct smart_holder {
 
     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} {}
+          vptr_is_using_builtin_delete{false}, vptr_is_external_shared_ptr{false},
+          is_populated{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_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
-          vptr_is_external_shared_ptr{false}, is_populated{false} {}
+          vptr_is_external_shared_ptr{false}, is_populated{false}, pointee_depends_on_holder_owner{
+                                                                       false} {}
 
     bool has_pointee() const { return vptr.get() != nullptr; }
 
diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h
@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include "../gil.h"
 #include "../pytypes.h"
 #include "common.h"
 #include "descr.h"
@@ -262,7 +263,9 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
     }
 
     template <typename T>
-    static void init_instance_for_type(detail::instance *inst, const void *holder_const_void_ptr) {
+    static void init_instance_for_type(detail::instance *inst,
+                                       const void *holder_const_void_ptr,
+                                       bool has_alias) {
         // Need for const_cast is a consequence of the type_info::init_instance type:
         // void (*init_instance)(instance *, const void *);
         auto holder_void_ptr = const_cast<void *>(holder_const_void_ptr);
@@ -284,6 +287,7 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
             new (std::addressof(v_h.holder<holder_type>()))
                 holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr<T>()));
         }
+        v_h.holder<holder_type>().pointee_depends_on_holder_owner = has_alias;
         v_h.set_holder_constructed();
     }
 
@@ -331,15 +335,34 @@ struct smart_holder_type_caster_load {
         return *raw_ptr;
     }
 
+    struct shared_ptr_dec_ref_deleter {
+        // Note: deleter destructor fails on MSVC 2015 and GCC 4.8, so we manually call dec_ref
+        // here instead.
+        handle ref;
+        void operator()(void *) {
+            gil_scoped_acquire gil;
+            ref.dec_ref();
+        }
+    };
+
     std::shared_ptr<T> loaded_as_shared_ptr() const {
         if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr)
             throw cast_error("Unowned pointer from direct conversion cannot be converted to a"
                              " std::shared_ptr.");
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
-        std::shared_ptr<void> void_ptr = holder().template as_shared_ptr<void>();
-        return std::shared_ptr<T>(void_ptr, convert_type(void_ptr.get()));
+        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) {
+            // Tie lifetime of trampoline Python part to C++ part (PR #2839).
+            return std::shared_ptr<T>(
+                type_raw_ptr,
+                shared_ptr_dec_ref_deleter{
+                    handle((PyObject *) load_impl.loaded_v_h.inst).inc_ref()});
+        }
+        std::shared_ptr<void> void_shd_ptr = holder().template as_shared_ptr<void>();
+        return std::shared_ptr<T>(void_shd_ptr, type_raw_ptr);
     }
 
     template <typename D>
@@ -352,6 +375,10 @@ struct smart_holder_type_caster_load {
         throw_if_uninitialized_or_disowned_holder();
         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>();
         // SMART_HOLDER_WIP: MISSING: Safety checks for type conversions
         // (T must be polymorphic or meet certain other conditions).
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -1616,7 +1616,7 @@ class class_ : public detail::generic_type {
     template <typename T = type,
               detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value, int> = 0>
     static void init_instance(detail::instance *inst, const void *holder_ptr) {
-        detail::type_caster<T>::template init_instance_for_type<type>(inst, holder_ptr);
+        detail::type_caster<T>::template init_instance_for_type<type>(inst, holder_ptr, has_alias);
     }
     // clang-format off
 
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -104,7 +104,9 @@ set(PYBIND11_TEST_FILES
     test_class_sh_basic.cpp
     test_class_sh_factory_constructors.cpp
     test_class_sh_inheritance.cpp
+    test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
     test_class_sh_unique_ptr_member.cpp
+    test_class_sh_with_alias.cpp
     test_constants_and_functions.cpp
     test_copy_move.cpp
     test_custom_type_casters.cpp
diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
@@ -0,0 +1,85 @@
+// 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.
+
+#include "pybind11_tests.h"
+#include "pybind11/smart_holder.h"
+
+namespace {
+
+// For testing whether a python subclass of a C++ object dies when the
+// last python reference is lost
+struct SpBase {
+    // returns true if the base virtual function is called
+    virtual bool is_base_used() { return true; }
+
+    // returns true if there's an associated python instance
+    bool has_python_instance() {
+        auto tinfo = py::detail::get_type_info(typeid(SpBase));
+        return (bool)py::detail::get_object_handle(this, tinfo);
+    }
+
+    SpBase()               = default;
+    SpBase(const SpBase &) = delete;
+    virtual ~SpBase()      = default;
+};
+
+struct PySpBase : SpBase {
+    bool is_base_used() override { PYBIND11_OVERRIDE(bool, SpBase, is_base_used); }
+};
+
+struct SpBaseTester {
+    std::shared_ptr<SpBase> get_object() { return m_obj; }
+    void set_object(std::shared_ptr<SpBase> obj) { m_obj = obj; }
+    bool is_base_used() { return m_obj->is_base_used(); }
+    bool has_instance() { return (bool)m_obj; }
+    bool has_python_instance() { return m_obj && m_obj->has_python_instance(); }
+    void set_nonpython_instance() {
+        m_obj = std::make_shared<SpBase>();
+    }
+    std::shared_ptr<SpBase> m_obj;
+};
+
+// For testing that a C++ class without an alias does not retain the python
+// portion of the object
+struct SpGoAway {};
+
+struct SpGoAwayTester {
+    std::shared_ptr<SpGoAway> m_obj;
+};
+
+} // namespace
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(SpBase)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(SpBaseTester)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(SpGoAway)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(SpGoAwayTester)
+
+TEST_SUBMODULE(class_sh_trampoline_shared_ptr_cpp_arg, m) {
+    // For testing whether a python subclass of a C++ object dies when the
+    // last python reference is lost
+
+    py::classh<SpBase, PySpBase>(m, "SpBase")
+        .def(py::init<>())
+        .def("is_base_used", &SpBase::is_base_used)
+        .def("has_python_instance", &SpBase::has_python_instance);
+
+    py::classh<SpBaseTester>(m, "SpBaseTester")
+        .def(py::init<>())
+        .def("get_object", &SpBaseTester::get_object)
+        .def("set_object", &SpBaseTester::set_object)
+        .def("is_base_used", &SpBaseTester::is_base_used)
+        .def("has_instance", &SpBaseTester::has_instance)
+        .def("has_python_instance", &SpBaseTester::has_python_instance)
+        .def("set_nonpython_instance", &SpBaseTester::set_nonpython_instance)
+        .def_readwrite("obj", &SpBaseTester::m_obj);
+
+    // For testing that a C++ class without an alias does not retain the python
+    // portion of the object
+
+    py::classh<SpGoAway>(m, "SpGoAway").def(py::init<>());
+
+    py::classh<SpGoAwayTester>(m, "SpGoAwayTester")
+        .def(py::init<>())
+        .def_readwrite("obj", &SpGoAwayTester::m_obj);
+}
diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py
@@ -0,0 +1,139 @@
+# -*- coding: utf-8 -*-
+import pytest
+
+import pybind11_tests.class_sh_trampoline_shared_ptr_cpp_arg as m
+
+
+def test_shared_ptr_cpp_arg():
+    import weakref
+
+    class PyChild(m.SpBase):
+        def is_base_used(self):
+            return False
+
+    tester = m.SpBaseTester()
+
+    obj = PyChild()
+    objref = weakref.ref(obj)
+
+    # Pass the last python reference to the C++ function
+    tester.set_object(obj)
+    del obj
+    pytest.gc_collect()
+
+    # python reference is still around since C++ has it now
+    assert objref() is not None
+    assert tester.is_base_used() is False
+    assert tester.obj.is_base_used() is False
+    assert tester.get_object() is objref()
+
+
+def test_shared_ptr_cpp_prop():
+    class PyChild(m.SpBase):
+        def is_base_used(self):
+            return False
+
+    tester = m.SpBaseTester()
+
+    # Set the last python reference as a property of the C++ object
+    tester.obj = PyChild()
+    pytest.gc_collect()
+
+    # python reference is still around since C++ has it now
+    assert tester.is_base_used() is False
+    assert tester.has_python_instance() is True
+    assert tester.obj.is_base_used() is False
+    assert tester.obj.has_python_instance() is True
+
+
+def test_shared_ptr_arg_identity():
+    import weakref
+
+    tester = m.SpBaseTester()
+
+    obj = m.SpBase()
+    objref = weakref.ref(obj)
+
+    tester.set_object(obj)
+    del obj
+    pytest.gc_collect()
+
+    # python reference is still around since C++ has it
+    assert objref() is not None
+    assert tester.get_object() is objref()
+    assert tester.has_python_instance() is True
+
+    # python reference disappears once the C++ object releases it
+    tester.set_object(None)
+    pytest.gc_collect()
+    assert objref() is None
+
+
+def test_shared_ptr_alias_nonpython():
+    tester = m.SpBaseTester()
+
+    # C++ creates the object, a python instance shouldn't exist
+    tester.set_nonpython_instance()
+    assert tester.is_base_used() is True
+    assert tester.has_instance() is True
+    assert tester.has_python_instance() is False
+
+    # Now a python instance exists
+    cobj = tester.get_object()
+    assert cobj.has_python_instance()
+    assert tester.has_instance() is True
+    assert tester.has_python_instance() is True
+
+    # Now it's gone
+    del cobj
+    pytest.gc_collect()
+    assert tester.has_instance() is True
+    assert tester.has_python_instance() is False
+
+    # SMART_HOLDER_WIP: the behavior below is DIFFERENT from PR #2839
+    # When we pass it as an arg to a new tester the python instance should
+    # disappear because it wasn't created with an alias
+    new_tester = m.SpBaseTester()
+
+    cobj = tester.get_object()
+    assert cobj.has_python_instance()
+
+    new_tester.set_object(cobj)
+    assert tester.has_python_instance() is True
+    assert new_tester.has_python_instance() is True
+
+    del cobj
+    pytest.gc_collect()
+
+    # Gone!
+    assert tester.has_instance() is True
+    assert tester.has_python_instance() is True  # False in PR #2839
+    assert new_tester.has_instance() is True
+    assert new_tester.has_python_instance() is True  # False in PR #2839
+
+
+def test_shared_ptr_goaway():
+    import weakref
+
+    tester = m.SpGoAwayTester()
+
+    obj = m.SpGoAway()
+    objref = weakref.ref(obj)
+
+    assert tester.obj is None
+
+    tester.obj = obj
+    del obj
+    pytest.gc_collect()
+
+    # python reference is no longer around
+    assert objref() is None
+    # C++ reference is still around
+    assert tester.obj is not None
+
+
+def test_infinite():
+    tester = m.SpBaseTester()
+    while True:
+        tester.set_object(m.SpBase())
+        return  # Comment out for manual leak checking (use `top` command).
diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp
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
`@@ -1616,7 +1616,7 @@ class class_ : public detail::generic_type {`
`1616`	`1616`	`template <typename T = type,`
`1617`	`1617`	`detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value, int> = 0>`
`1618`	`1618`	`static void init_instance(detail::instance inst, const void holder_ptr) {`
`1619`		`- detail::type_caster<T>::template init_instance_for_type<type>(inst, holder_ptr);`
	`1619`	`+ detail::type_caster<T>::template init_instance_for_type<type>(inst, holder_ptr, has_alias);`
`1620`	`1620`	`}`
`1621`	`1621`	`// clang-format off`
`1622`	`1622`