From 7947485890c6d02227bbb902735f989a1955860e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 21:15:14 -0700 Subject: [PATCH 1/6] Using new smart_holder::reclaim_disowned in smart_holder_type_caster for unique_ptr. --- include/pybind11/detail/smart_holder_poc.h | 6 +++++ .../detail/smart_holder_type_casters.h | 25 ++++++++++++++++--- .../virtual_overrider_self_life_support.h | 25 +++++++++++++------ tests/pure_cpp/smart_holder_poc_test.cpp | 14 +++++++++++ tests/test_class_sh_trampoline_unique_ptr.cpp | 14 ++++++++++- tests/test_class_sh_trampoline_unique_ptr.py | 24 ++++++++++++++---- 6 files changed, 92 insertions(+), 16 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index cdae24845f..6500d9f895 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -240,6 +240,12 @@ struct smart_holder { was_disowned = true; } + // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). + void reclaim_disowned() { + *vptr_deleter_armed_flag_ptr = true; + was_disowned = false; + } + // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). void release_disowned() { vptr.reset(); diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 14b3ea9979..25476f4123 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -406,8 +406,7 @@ struct smart_holder_type_caster_load { } auto result = std::unique_ptr(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,28 @@ struct smart_holder_type_caster> : smart_holder_type_caste void *src_raw_void_ptr = static_cast(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( + 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(); + if (!holder.was_disowned) { + pybind11_fail("smart_holder_type_casters: unexpected " + "smart_holder.was_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(make_new_instance(tinfo->type)); auto *inst_raw_ptr = reinterpret_cast(inst.ptr()); diff --git a/include/pybind11/virtual_overrider_self_life_support.h b/include/pybind11/virtual_overrider_self_life_support.h index bf82f69a67..8e0eaed3f5 100644 --- a/include/pybind11/virtual_overrider_self_life_support.h +++ b/include/pybind11/virtual_overrider_self_life_support.h @@ -20,16 +20,27 @@ PYBIND11_NAMESPACE_END(detail) // 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 { - detail::value_and_holder loaded_v_h; + detail::value_and_holder v_h; + + void activate_life_support(const detail::value_and_holder &v_h) { + Py_INCREF((PyObject *) v_h.inst); + this->v_h = v_h; + } + + void deactivate_life_support() { + Py_DECREF((PyObject *) v_h.inst); + v_h = detail::value_and_holder(); + } + ~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 (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(); - loaded_v_h.value_ptr() = nullptr; - loaded_v_h.holder().release_disowned(); - detail::deregister_instance(loaded_v_h.inst, value_void_ptr, loaded_v_h.type); - Py_DECREF((PyObject *) loaded_v_h.inst); // Must be after deregister. + v_h.value_ptr() = nullptr; + v_h.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); } } diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index 63c22e11b5..d183fed045 100644 --- a/tests/pure_cpp/smart_holder_poc_test.cpp +++ b/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 new_owner(hld.as_raw_ptr_unowned()); + hld.disown(); + REQUIRE(hld.as_lvalue_ref() == 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() == 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 new_owner(hld.as_raw_ptr_unowned()); diff --git a/tests/test_class_sh_trampoline_unique_ptr.cpp b/tests/test_class_sh_trampoline_unique_ptr.cpp index 375f146575..2b3087ba87 100644 --- a/tests/test_class_sh_trampoline_unique_ptr.cpp +++ b/tests/test_class_sh_trampoline_unique_ptr.cpp @@ -6,11 +6,17 @@ #include "pybind11/virtual_overrider_self_life_support.h" #include "pybind11_tests.h" +#include + namespace { class Class { public: - virtual ~Class() = default; + virtual ~Class() = default; + + void setVal(std::uint64_t val) { val_ = val; } + std::uint64_t getVal() const { return val_; } + virtual std::unique_ptr clone() const = 0; virtual int foo() const = 0; @@ -19,6 +25,9 @@ class Class { // Some compilers complain about implicitly defined versions of some of the following: Class(const Class &) = default; + +private: + std::uint64_t val_ = 0; }; } // namespace @@ -41,8 +50,11 @@ class PyClass : public Class, public py::virtual_overrider_self_life_support { TEST_SUBMODULE(class_sh_trampoline_unique_ptr, m) { py::classh(m, "Class") .def(py::init<>()) + .def("set_val", &Class::setVal) + .def("get_val", &Class::getVal) .def("clone", &Class::clone) .def("foo", &Class::foo); + m.def("clone", [](const Class &obj) { return obj.clone(); }); m.def("clone_and_foo", [](const Class &obj) { return obj.clone()->foo(); }); } diff --git a/tests/test_class_sh_trampoline_unique_ptr.py b/tests/test_class_sh_trampoline_unique_ptr.py index d43cfeb4fd..932db659ef 100644 --- a/tests/test_class_sh_trampoline_unique_ptr.py +++ b/tests/test_class_sh_trampoline_unique_ptr.py @@ -5,13 +5,27 @@ class MyClass(m.Class): def foo(self): - return 10 + return 10 + self.get_val() def clone(self): - return MyClass() + cloned = MyClass() + cloned.set_val(self.get_val() + 3) + return cloned -def test_py_clone_and_foo(): +def test_m_clone(): obj = MyClass() - assert obj.foo() == 10 - assert m.clone_and_foo(obj) == 10 + while True: + obj.set_val(5) + obj = m.clone(obj) + assert obj.get_val() == 5 + 3 + assert obj.foo() == 10 + 5 + 3 + return # Comment out for manual leak checking (use `top` command). + + +def test_m_clone_and_foo(): + obj = MyClass() + obj.set_val(7) + while True: + assert m.clone_and_foo(obj) == 10 + 7 + 3 + return # Comment out for manual leak checking (use `top` command). From 0d8dcc9e7f56eece074c21392b6e3cb1596733c8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 21:24:54 -0700 Subject: [PATCH 2/6] Systematically renaming was_disowned to is_disowned (because disowning is now reversible: reclaim_disowned). --- include/pybind11/detail/smart_holder_poc.h | 16 +++++----- .../detail/smart_holder_type_casters.h | 8 ++--- tests/pure_cpp/smart_holder_poc_test.cpp | 6 ++-- tests/test_class_sh_disowning.py | 8 ++--- tests/test_class_sh_disowning_mi.py | 30 +++++++++---------- 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 6500d9f895..2e03bc5e1b 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/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,13 +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; - was_disowned = false; + is_disowned = false; } // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). @@ -254,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); } diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 25476f4123..10a9e00155 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.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(); 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(context); holder().ensure_use_count_1(context); auto raw_void_ptr = holder().template as_raw_ptr_unowned(); @@ -707,9 +707,9 @@ struct smart_holder_type_caster> : smart_holder_type_caste value_and_holder &v_h = self_life_support->v_h; if (v_h.inst != nullptr && v_h.vh != nullptr) { auto &holder = v_h.holder(); - if (!holder.was_disowned) { + if (!holder.is_disowned) { pybind11_fail("smart_holder_type_casters: unexpected " - "smart_holder.was_disowned failure."); + "smart_holder.is_disowned failure."); } // Critical transfer-of-ownership section. This must stay together. self_life_support->deactivate_life_support(); diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index d183fed045..18c57fa197 100644 --- a/tests/pure_cpp/smart_holder_poc_test.cpp +++ b/tests/pure_cpp/smart_holder_poc_test.cpp @@ -153,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 new_owner(hld.as_raw_ptr_unowned()); 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)."); } diff --git a/tests/test_class_sh_disowning.py b/tests/test_class_sh_disowning.py index 0c3de77c6f..f98157138b 100644 --- a/tests/test_class_sh_disowning.py +++ b/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). diff --git a/tests/test_class_sh_disowning_mi.py b/tests/test_class_sh_disowning_mi.py index a236588cae..61cb9c6a25 100644 --- a/tests/test_class_sh_disowning_mi.py +++ b/tests/test_class_sh_disowning_mi.py @@ -18,7 +18,7 @@ def test_diamond_inheritance(): assert d is d.c0().c1().b().c0().b() -def was_disowned(callable_method): +def is_disowned(callable_method): try: callable_method() except ValueError as e: @@ -34,7 +34,7 @@ def test_disown_b(): b = m.B() assert b.get() == 10 m.disown_b(b) - assert was_disowned(b.get) + assert is_disowned(b.get) @pytest.mark.parametrize("var_to_disown", ["c0", "b"]) @@ -43,8 +43,8 @@ def test_disown_c0(var_to_disown): assert c0.get() == 1020 b = c0.b() m.disown_b(locals()[var_to_disown]) - assert was_disowned(c0.get) - assert was_disowned(b.get) + assert is_disowned(c0.get) + assert is_disowned(b.get) @pytest.mark.parametrize("var_to_disown", ["c1", "b"]) @@ -53,8 +53,8 @@ def test_disown_c1(var_to_disown): assert c1.get() == 1021 b = c1.b() m.disown_b(locals()[var_to_disown]) - assert was_disowned(c1.get) - assert was_disowned(b.get) + assert is_disowned(c1.get) + assert is_disowned(b.get) @pytest.mark.parametrize("var_to_disown", ["d", "c1", "c0", "b"]) @@ -65,10 +65,10 @@ def test_disown_d(var_to_disown): c0 = d.c0() c1 = d.c1() m.disown_b(locals()[var_to_disown]) - assert was_disowned(d.get) - assert was_disowned(c1.get) - assert was_disowned(c0.get) - assert was_disowned(b.get) + assert is_disowned(d.get) + assert is_disowned(c1.get) + assert is_disowned(c0.get) + assert is_disowned(b.get) # Based on test_multiple_inheritance.py:test_multiple_inheritance_python. @@ -211,10 +211,10 @@ def test_disown_base1_first(cls, i, j, v): obj = cls(i, j) assert obj.foo() == i assert m.disown_base1(obj) == 2000 * i + 1 - assert was_disowned(obj.foo) + assert is_disowned(obj.foo) assert obj.bar() == j assert m.disown_base2(obj) == 2000 * j + 2 - assert was_disowned(obj.bar) + assert is_disowned(obj.bar) if v is not None: assert obj.v() == v @@ -226,10 +226,10 @@ def test_disown_base2_first(cls, i, j, v): obj = cls(i, j) assert obj.bar() == j assert m.disown_base2(obj) == 2000 * j + 2 - assert was_disowned(obj.bar) + assert is_disowned(obj.bar) assert obj.foo() == i assert m.disown_base1(obj) == 2000 * i + 1 - assert was_disowned(obj.foo) + assert is_disowned(obj.foo) if v is not None: assert obj.v() == v @@ -249,5 +249,5 @@ def test_disown_base2(cls, j, v): obj = cls(j) assert obj.bar() == j assert m.disown_base2(obj) == 2000 * j + 2 - assert was_disowned(obj.bar) + assert is_disowned(obj.bar) assert obj.v() == v From 462abfe75e81838dea7b178b84977edf50673d69 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 21:36:04 -0700 Subject: [PATCH 3/6] Systematically renaming virtual_overrider_self_life_support to trampoline_self_life_support (to reuse existing terminology instead of introducing new one). --- CMakeLists.txt | 2 +- .../pybind11/detail/smart_holder_type_casters.h | 11 +++++------ ..._support.h => trampoline_self_life_support.h} | 16 +++++++--------- tests/extra_python_package/test_files.py | 2 +- tests/test_class_sh_trampoline_unique_ptr.cpp | 4 ++-- tests/test_class_sh_virtual_py_cpp_mix.cpp | 4 ++-- tests/test_class_sh_with_alias.cpp | 2 +- tests/test_class_sh_with_alias.py | 2 +- 8 files changed, 20 insertions(+), 23 deletions(-) rename include/pybind11/{virtual_overrider_self_life_support.h => trampoline_self_life_support.h} (75%) diff --git a/CMakeLists.txt b/CMakeLists.txt index b698b016da..de5ed3b7e1 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 10a9e00155..97791ceb12 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/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" @@ -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(raw_type_ptr); + = dynamic_raw_ptr_cast_if_possible(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. @@ -701,8 +701,7 @@ struct smart_holder_type_caster> : smart_holder_type_caste const detail::type_info *tinfo = st.second; if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { auto *self_life_support - = dynamic_raw_ptr_cast_if_possible( - src_raw_ptr); + = dynamic_raw_ptr_cast_if_possible(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) { diff --git a/include/pybind11/virtual_overrider_self_life_support.h b/include/pybind11/trampoline_self_life_support.h similarity index 75% rename from include/pybind11/virtual_overrider_self_life_support.h rename to include/pybind11/trampoline_self_life_support.h index 8e0eaed3f5..4980aae519 100644 --- a/include/pybind11/virtual_overrider_self_life_support.h +++ b/include/pybind11/trampoline_self_life_support.h @@ -19,7 +19,7 @@ PYBIND11_NAMESPACE_END(detail) // 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 { +struct trampoline_self_life_support { detail::value_and_holder v_h; void activate_life_support(const detail::value_and_holder &v_h) { @@ -32,7 +32,7 @@ struct virtual_overrider_self_life_support { v_h = detail::value_and_holder(); } - ~virtual_overrider_self_life_support() { + ~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) { @@ -47,13 +47,11 @@ struct virtual_overrider_self_life_support { } // 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; + 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) diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 3c79ac6382..e912a48fa9 100644 --- a/tests/extra_python_package/test_files.py +++ b/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 = { diff --git a/tests/test_class_sh_trampoline_unique_ptr.cpp b/tests/test_class_sh_trampoline_unique_ptr.cpp index 2b3087ba87..8a54da8f86 100644 --- a/tests/test_class_sh_trampoline_unique_ptr.cpp +++ b/tests/test_class_sh_trampoline_unique_ptr.cpp @@ -3,7 +3,7 @@ // BSD-style license that can be found in the LICENSE file. #include "pybind11/smart_holder.h" -#include "pybind11/virtual_overrider_self_life_support.h" +#include "pybind11/trampoline_self_life_support.h" #include "pybind11_tests.h" #include @@ -36,7 +36,7 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(Class) namespace { -class PyClass : public Class, public py::virtual_overrider_self_life_support { +class PyClass : public Class, public py::trampoline_self_life_support { public: std::unique_ptr clone() const override { PYBIND11_OVERRIDE_PURE(std::unique_ptr, Class, clone); diff --git a/tests/test_class_sh_virtual_py_cpp_mix.cpp b/tests/test_class_sh_virtual_py_cpp_mix.cpp index 02850fedfa..696368ad45 100644 --- a/tests/test_class_sh_virtual_py_cpp_mix.cpp +++ b/tests/test_class_sh_virtual_py_cpp_mix.cpp @@ -31,13 +31,13 @@ int get_from_cpp_plainc_ptr(const Base *b) { return b->get() + 4000; } int get_from_cpp_unique_ptr(std::unique_ptr b) { return b->get() + 5000; } -struct BaseVirtualOverrider : Base, py::virtual_overrider_self_life_support { +struct BaseVirtualOverrider : Base, py::trampoline_self_life_support { using Base::Base; int get() const override { PYBIND11_OVERRIDE(int, Base, get); } }; -struct CppDerivedVirtualOverrider : CppDerived, py::virtual_overrider_self_life_support { +struct CppDerivedVirtualOverrider : CppDerived, py::trampoline_self_life_support { using CppDerived::CppDerived; int get() const override { PYBIND11_OVERRIDE(int, CppDerived, get); } diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp index 649eb61f0a..8ff0d1afc8 100644 --- a/tests/test_class_sh_with_alias.cpp +++ b/tests/test_class_sh_with_alias.cpp @@ -35,7 +35,7 @@ struct AbaseAlias : Abase { }; template <> -struct AbaseAlias<1> : Abase<1>, py::virtual_overrider_self_life_support { +struct AbaseAlias<1> : Abase<1>, py::trampoline_self_life_support { using Abase<1>::Abase; int Add(int other_val) const override { diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py index 4650eaa5e1..f397cc80d3 100644 --- a/tests/test_class_sh_with_alias.py +++ b/tests/test_class_sh_with_alias.py @@ -45,7 +45,7 @@ def test_drvd0_add_in_cpp_unique_ptr(): assert ( str(exc_info.value) == "Alias class (also known as trampoline) does not inherit from" - " py::virtual_overrider_self_life_support, therefore the ownership of this" + " py::trampoline_self_life_support, therefore the ownership of this" " instance cannot safely be transferred to C++." ) return # Comment out for manual leak checking (use `top` command). From e54ccf49f788839e43bba068892d16847cbbb9a9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 21:45:28 -0700 Subject: [PATCH 4/6] Systematically renaming test_class_sh_with_alias to test_class_sh_trampoline_basic. --- tests/CMakeLists.txt | 2 +- ..._alias.cpp => test_class_sh_trampoline_basic.cpp} | 12 ++++++------ ...th_alias.py => test_class_sh_trampoline_basic.py} | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) rename tests/{test_class_sh_with_alias.cpp => test_class_sh_trampoline_basic.cpp} (87%) rename tests/{test_class_sh_with_alias.py => test_class_sh_trampoline_basic.py} (96%) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index bf77050510..887503aef9 100644 --- a/tests/CMakeLists.txt +++ b/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 diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_trampoline_basic.cpp similarity index 87% rename from tests/test_class_sh_with_alias.cpp rename to tests/test_class_sh_trampoline_basic.cpp index 8ff0d1afc8..935dafd940 100644 --- a/tests/test_class_sh_with_alias.cpp +++ b/tests/test_class_sh_trampoline_basic.cpp @@ -5,7 +5,7 @@ #include namespace pybind11_tests { -namespace class_sh_with_alias { +namespace class_sh_trampoline_basic { template // Using int as a trick to easily generate a series of types. struct Abase { @@ -73,14 +73,14 @@ void wrap(py::module_ m, const char *py_class_name) { m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val")); } -} // namespace class_sh_with_alias +} // namespace class_sh_trampoline_basic } // namespace pybind11_tests -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<0>) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<1>) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_trampoline_basic::Abase<0>) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_trampoline_basic::Abase<1>) -TEST_SUBMODULE(class_sh_with_alias, m) { - using namespace pybind11_tests::class_sh_with_alias; +TEST_SUBMODULE(class_sh_trampoline_basic, m) { + using namespace pybind11_tests::class_sh_trampoline_basic; wrap<0>(m, "Abase0"); wrap<1>(m, "Abase1"); } diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_trampoline_basic.py similarity index 96% rename from tests/test_class_sh_with_alias.py rename to tests/test_class_sh_trampoline_basic.py index f397cc80d3..8c82decf8f 100644 --- a/tests/test_class_sh_with_alias.py +++ b/tests/test_class_sh_trampoline_basic.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- import pytest -from pybind11_tests import class_sh_with_alias as m +from pybind11_tests import class_sh_trampoline_basic as m class PyDrvd0(m.Abase0): From 5cfd2c5befbfbc15afc9be2fcd0d575ce56c717a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 22:11:45 -0700 Subject: [PATCH 5/6] Adding a Trampolines and std::unique_ptr section to README_smart_holder.rst. --- README_smart_holder.rst | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/README_smart_holder.rst b/README_smart_holder.rst index 3054dfbf67..baf7b8ff07 100644 --- a/README_smart_holder.rst +++ b/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" +`_ +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 ----------------------- From 01bf7a4c4d7f9ce1231bffb0cf07005c1e45c753 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Apr 2021 22:27:54 -0700 Subject: [PATCH 6/6] MSVC compatibility. --- include/pybind11/trampoline_self_life_support.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/trampoline_self_life_support.h b/include/pybind11/trampoline_self_life_support.h index 4980aae519..fe2e4899f9 100644 --- a/include/pybind11/trampoline_self_life_support.h +++ b/include/pybind11/trampoline_self_life_support.h @@ -22,9 +22,9 @@ PYBIND11_NAMESPACE_END(detail) 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); - this->v_h = 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() {