From 97a7fb722a9842cae4d91be82897dd863f6b607d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 3 Mar 2021 17:58:42 -0800 Subject: [PATCH] Porting/adapting Dustin's PR #2839 to smart_holder branch (#2886) * WIP: test setup complete, AddInCppUniquePtr failing (reproduces PyCLIF smart_ptrs_test failure). * Fully tested locally. * Adding new tests to cmake file. --- include/pybind11/detail/smart_holder_poc.h | 8 +- .../detail/smart_holder_type_casters.h | 33 ++++- include/pybind11/pybind11.h | 2 +- tests/CMakeLists.txt | 2 + ...class_sh_trampoline_shared_ptr_cpp_arg.cpp | 85 +++++++++++ ..._class_sh_trampoline_shared_ptr_cpp_arg.py | 139 ++++++++++++++++++ tests/test_class_sh_with_alias.cpp | 61 ++++++++ tests/test_class_sh_with_alias.py | 37 +++++ 8 files changed, 360 insertions(+), 7 deletions(-) create mode 100644 tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp create mode 100644 tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py create mode 100644 tests/test_class_sh_with_alias.cpp create mode 100644 tests/test_class_sh_with_alias.py diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index a70ffa62d..87443651e 100644 --- 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 index d0179e9e3..892a5101c 100644 --- 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 - 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(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::from_raw_ptr_unowned(v_h.value_ptr())); } + v_h.holder().pointee_depends_on_holder_owner = has_alias; v_h.set_holder_constructed(); } @@ -331,6 +335,16 @@ 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 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" @@ -338,8 +352,17 @@ struct smart_holder_type_caster_load { if (!have_holder()) return nullptr; throw_if_uninitialized_or_disowned_holder(); - std::shared_ptr void_ptr = holder().template as_shared_ptr(); - return std::shared_ptr(void_ptr, convert_type(void_ptr.get())); + 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) { + // Tie lifetime of trampoline Python part to C++ part (PR #2839). + return std::shared_ptr( + type_raw_ptr, + shared_ptr_dec_ref_deleter{ + handle((PyObject *) load_impl.loaded_v_h.inst).inc_ref()}); + } + std::shared_ptr void_shd_ptr = holder().template as_shared_ptr(); + return std::shared_ptr(void_shd_ptr, type_raw_ptr); } template @@ -352,6 +375,10 @@ struct smart_holder_type_caster_load { throw_if_uninitialized_or_disowned_holder(); 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). diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e3dd4c822..2818c8363 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1616,7 +1616,7 @@ private: template ::value, int> = 0> static void init_instance(detail::instance *inst, const void *holder_ptr) { - detail::type_caster::template init_instance_for_type(inst, holder_ptr); + detail::type_caster::template init_instance_for_type(inst, holder_ptr, has_alias); } // clang-format off diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ef6fc28f1..efc548c41 100644 --- 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 new file mode 100644 index 000000000..dfdbea18a --- /dev/null +++ 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 get_object() { return m_obj; } + void set_object(std::shared_ptr 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(); + } + std::shared_ptr 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 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(m, "SpBase") + .def(py::init<>()) + .def("is_base_used", &SpBase::is_base_used) + .def("has_python_instance", &SpBase::has_python_instance); + + py::classh(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(m, "SpGoAway").def(py::init<>()); + + py::classh(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 new file mode 100644 index 000000000..9304aa0b7 --- /dev/null +++ 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 new file mode 100644 index 000000000..e62f8e763 --- /dev/null +++ b/tests/test_class_sh_with_alias.cpp @@ -0,0 +1,61 @@ +#include "pybind11_tests.h" + +#include + +#include + +namespace pybind11_tests { +namespace test_class_sh_with_alias { + +struct Abase { + int val = 0; + virtual ~Abase() = default; + Abase(int val_) : val{val_} {} + int Get() const { return val * 10 + 3; } + virtual int Add(int other_val) const = 0; + + // Some compilers complain about implicitly defined versions of some of the following: + Abase(const Abase &) = default; + Abase(Abase &&) = default; + Abase &operator=(const Abase &) = default; + Abase &operator=(Abase &&) = default; +}; + +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) */ + other_val); + } +}; + +int AddInCppRawPtr(const Abase *obj, int other_val) { return obj->Add(other_val) * 10 + 7; } + +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) { + return obj->Add(other_val) * 100 + 13; +} + +} // namespace test_class_sh_with_alias +} // namespace pybind11_tests + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase) + +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")); +} diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py new file mode 100644 index 000000000..4918b9338 --- /dev/null +++ b/tests/test_class_sh_with_alias.py @@ -0,0 +1,37 @@ +# -*- coding: utf-8 -*- +import pytest + +from pybind11_tests import class_sh_with_alias as m + + +class PyDrvd(m.Abase): + def __init__(self, val): + super(PyDrvd, self).__init__(val) + + def Add(self, other_val): # noqa: N802 + return self.Get() * 100 + other_val + + +def test_drvd_add(): + drvd = PyDrvd(74) + assert drvd.Add(38) == (74 * 10 + 3) * 100 + 38 + + +def test_add_in_cpp_raw_ptr(): + drvd = PyDrvd(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_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++." + )