diff --git a/CMakeLists.txt b/CMakeLists.txt index 39800c3e9..0a52493b2 100644 --- 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 index 87443651e..cdae24845 100644 --- 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 vptr_deleter_armed_flag_ptr; std::shared_ptr 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 diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 892a5101c..c263013ea 100644 --- 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 #include @@ -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(); 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(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(); - // 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); - - // Critical transfer-of-ownership section. This must stay together. - holder().release_ownership(); - auto result = std::unique_ptr(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"); } - 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); + + // 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. + if (self_life_support != nullptr) { + holder().disown(); + } else { + holder().release_ownership(); + } + 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; + } 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); + } + // 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 new file mode 100644 index 000000000..524bfcf54 --- /dev/null +++ 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 + +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().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 ::value, int> = 0> +virtual_overrider_self_life_support * +dynamic_cast_virtual_overrider_self_life_support_ptr(T * /*raw_type_ptr*/) { + return nullptr; +} + +template ::value, int> = 0> +virtual_overrider_self_life_support * +dynamic_cast_virtual_overrider_self_life_support_ptr(T *raw_type_ptr) { + return dynamic_cast(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 index 12d14e45a..cfa8d9ca4 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -48,6 +48,7 @@ detail_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", } cmake_files = { diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index 970899a05..63c22e11b 100644 --- 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 new_owner(hld.as_raw_ptr_unowned()); + hld.disown(); + REQUIRE(hld.as_lvalue_ref() == 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 new_owner(hld.as_raw_ptr_unowned()); + 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 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 index e62f8e763..37372f116 100644 --- 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 // 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 +struct AbaseAlias : Abase { + using Abase::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, /* 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 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 AddInCppRawPtr(const Abase *obj, int other_val) { + return obj->Add(other_val) * 10 + 7; +} + +template +int AddInCppSharedPtr(std::shared_ptr> obj, int other_val) { return obj->Add(other_val) * 100 + 11; } -int AddInCppUniquePtr(std::unique_ptr obj, int other_val) { +template +int AddInCppUniquePtr(std::unique_ptr> obj, int other_val) { return obj->Add(other_val) * 100 + 13; } +template +void wrap(py::module_ m, const char *py_class_name) { + py::classh, AbaseAlias>(m, py_class_name) + .def(py::init(), 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")); +} + } // 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(m, "Abase") - .def(py::init(), 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 index 4918b9338..efd6af7a4 100644 --- a/tests/test_class_sh_with_alias.py +++ b/tests/test_class_sh_with_alias.py @@ -4,34 +4,54 @@ import pytest from pybind11_tests import class_sh_with_alias as m -class PyDrvd(m.Abase): +class PyDrvd0(m.Abase0): def __init__(self, val): - super(PyDrvd, self).__init__(val) + super(PyDrvd0, self).__init__(val) def Add(self, other_val): # noqa: N802 return self.Get() * 100 + other_val -def test_drvd_add(): - drvd = PyDrvd(74) +class PyDrvd1(m.Abase1): + def __init__(self, val): + super(PyDrvd1, self).__init__(val) + + def Add(self, other_val): # noqa: N802 + return self.Get() * 200 + other_val + + +def test_drvd0_add(): + drvd = PyDrvd0(74) assert drvd.Add(38) == (74 * 10 + 3) * 100 + 38 -def test_add_in_cpp_raw_ptr(): - drvd = PyDrvd(52) +def test_drvd0_add_in_cpp_raw_ptr(): + drvd = PyDrvd0(52) assert m.AddInCppRawPtr(drvd, 27) == ((52 * 10 + 3) * 100 + 27) * 10 + 7 -def test_add_in_cpp_shared_ptr(): - drvd = PyDrvd(36) - assert m.AddInCppSharedPtr(drvd, 56) == ((36 * 10 + 3) * 100 + 56) * 100 + 11 +def test_drvd0_add_in_cpp_shared_ptr(): + while True: + drvd = PyDrvd0(36) + assert m.AddInCppSharedPtr(drvd, 56) == ((36 * 10 + 3) * 100 + 56) * 100 + 11 + return # Comment out for manual leak checking (use `top` command). -def test_add_in_cpp_unique_ptr(): - drvd = PyDrvd(0) - with pytest.raises(ValueError) as exc_info: - m.AddInCppUniquePtr(drvd, 0) - assert ( - str(exc_info.value) - == "Ownership of instance with virtual overrides in Python cannot be transferred to C++." - ) +def test_drvd0_add_in_cpp_unique_ptr(): + while True: + drvd = PyDrvd0(0) + with pytest.raises(ValueError) as exc_info: + m.AddInCppUniquePtr(drvd, 0) + assert ( + str(exc_info.value) + == "Ownership of instance with virtual overrides in Python" + " cannot be transferred to C++." + ) + return # Comment out for manual leak checking (use `top` command). + + +def test_drvd1_add_in_cpp_unique_ptr(): + while True: + drvd = PyDrvd1(25) + assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13 + return # Comment out for manual leak checking (use `top` command).