From bf8d6a2900b518b0d5ac47705f48e22f718a6ed5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Jun 2021 18:13:27 -0700 Subject: [PATCH] First fully successful attempt to make `shared_from_this` and trampolines play nicely. Now also passes the open_spiel iterated_prisoners_dilemma_test ASAN clean, in addition to all pybind11 and PyCLIF unit tests. The problem was that calling `std::shared_ptr::reset()` with a `void` pointer cannot possibly update the `shared_from_this` `weak_ptr`. The solution is to store a `shd_ptr_reset` function pointer in `guarded_deleter` (similar in idea to the stored function pointer for calling `delete`). This commit still includes all debugging code, i.e. is "dirty". The code will be cleaned up after the GitHub CI is fully successful. --- include/pybind11/detail/smart_holder_poc.h | 59 +++++++++++++++---- .../detail/smart_holder_type_casters.h | 20 +++++-- tests/CMakeLists.txt | 1 + ...t_class_sh_trampoline_shared_from_this.cpp | 36 +++++++++++ ...st_class_sh_trampoline_shared_from_this.py | 19 ++++++ 5 files changed, 119 insertions(+), 16 deletions(-) create mode 100644 tests/test_class_sh_trampoline_shared_from_this.cpp create mode 100644 tests/test_class_sh_trampoline_shared_from_this.py diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 5b25558a9..abb87827f 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -51,28 +51,47 @@ Details: #include #include +//#include +inline void to_cout(std::string /*msg*/) { /*std::cout << msg << std::endl;*/ } + // pybindit = Python Bindings Innovation Track. // Currently not in pybind11 namespace to signal that this POC does not depend // on any existing pybind11 functionality. namespace pybindit { namespace memory { +inline int shared_from_this_status(...) { return 0; } + +template +inline int shared_from_this_status(const std::enable_shared_from_this *ptr) { + if (ptr->weak_from_this().lock()) return 1; + return -1; +} + struct smart_holder; struct guarded_delete { smart_holder *hld = nullptr; void (*callback_ptr)(void *) = nullptr; void *callback_arg = nullptr; - void (*del_ptr)(void *); + void (*shd_ptr_reset_fptr)(std::shared_ptr&, void *, guarded_delete &&); + void (*del_fptr)(void *); bool armed_flag; - guarded_delete(void (*del_ptr)(void *), bool armed_flag) - : del_ptr{del_ptr}, armed_flag{armed_flag} {} + guarded_delete(void (*shd_ptr_reset_fptr)(std::shared_ptr&, void *, guarded_delete &&), void (*del_fptr)(void *), bool armed_flag) + : shd_ptr_reset_fptr{shd_ptr_reset_fptr}, del_fptr{del_fptr}, armed_flag{armed_flag} { +to_cout("LOOOK guarded_delete ctor " + std::to_string(__LINE__) + " " + __FILE__); + } void operator()(void *raw_ptr) const; }; +template +inline void shd_ptr_reset(std::shared_ptr& shd_ptr, void *raw_ptr, guarded_delete &&gdel) { + shd_ptr.reset(reinterpret_cast(raw_ptr), gdel); +} + template ::value, int>::type = 0> inline void builtin_delete_if_destructible(void *raw_ptr) { - delete (T *) raw_ptr; + delete reinterpret_cast(raw_ptr); } template ::value, int>::type = 0> @@ -85,17 +104,17 @@ inline void builtin_delete_if_destructible(void *) { template guarded_delete make_guarded_builtin_delete(bool armed_flag) { - return guarded_delete(builtin_delete_if_destructible, armed_flag); + return guarded_delete(shd_ptr_reset, builtin_delete_if_destructible, armed_flag); } template inline void custom_delete(void *raw_ptr) { - D()((T *) raw_ptr); + D()(reinterpret_cast(raw_ptr)); } template guarded_delete make_guarded_custom_deleter(bool armed_flag) { - return guarded_delete(custom_delete, armed_flag); + return guarded_delete(shd_ptr_reset, custom_delete, armed_flag); }; template @@ -199,16 +218,18 @@ struct smart_holder { } void reset_vptr_deleter_armed_flag(bool armed_flag) const { - auto vptr_del_ptr = std::get_deleter(vptr); - if (vptr_del_ptr == nullptr) { +to_cout("LOOOK smart_holder reset_vptr_deleter_armed_flag " + std::to_string(__LINE__) + " " + __FILE__); + auto vptr_del_fptr = std::get_deleter(vptr); + if (vptr_del_fptr == nullptr) { throw std::runtime_error( "smart_holder::reset_vptr_deleter_armed_flag() called in an invalid context."); } - vptr_del_ptr->armed_flag = armed_flag; + vptr_del_fptr->armed_flag = armed_flag; } template static smart_holder from_raw_ptr_unowned(T *raw_ptr) { +to_cout("LOOOK smart_holder from_raw_ptr_unowned " + std::to_string(__LINE__) + " " + __FILE__); smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); hld.vptr_is_using_noop_deleter = true; @@ -218,6 +239,7 @@ struct smart_holder { template T *as_raw_ptr_unowned() const { +to_cout("LOOOK smart_holder as_raw_ptr_unowned PTR" + std::to_string((unsigned long) vptr.get()) + " " + std::to_string(__LINE__) + " " + __FILE__); return static_cast(vptr.get()); } @@ -239,6 +261,8 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr) { +to_cout(""); +to_cout("LOOOK smart_holder from_raw_ptr_take_ownership " + std::to_string(__LINE__) + " " + __FILE__); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; hld.vptr.reset(raw_ptr, make_guarded_builtin_delete(true)); @@ -277,6 +301,7 @@ struct smart_holder { template T *as_raw_ptr_release_ownership(const char *context = "as_raw_ptr_release_ownership") { +to_cout("LOOOK smart_holder as_raw_ptr_release_ownership " + std::to_string(__LINE__) + " " + __FILE__); ensure_can_release_ownership(context); T *raw_ptr = as_raw_ptr_unowned(); release_ownership(); @@ -285,6 +310,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr) { +to_cout("LOOOK smart_holder from_unique_ptr " + std::to_string(__LINE__) + " " + __FILE__); smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -300,6 +326,7 @@ struct smart_holder { template > std::unique_ptr as_unique_ptr() { +to_cout("LOOOK smart_holder as_unique_ptr " + std::to_string(__LINE__) + " " + __FILE__); static const char *context = "as_unique_ptr"; ensure_compatible_rtti_uqp_del(context); ensure_use_count_1(context); @@ -310,6 +337,7 @@ struct smart_holder { template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { +to_cout("LOOOK smart_holder from_shared_ptr " + std::to_string(__LINE__) + " " + __FILE__); smart_holder hld; hld.vptr = std::static_pointer_cast(shd_ptr); hld.vptr_is_external_shared_ptr = true; @@ -319,18 +347,25 @@ struct smart_holder { template std::shared_ptr as_shared_ptr() const { +to_cout("LOOOK smart_holder as_shared_ptr " + std::to_string(__LINE__) + " " + __FILE__); return std::static_pointer_cast(vptr); } }; inline void guarded_delete::operator()(void *raw_ptr) const { if (hld) { +to_cout("LOOOK guarded_delete call hld " + std::to_string(__LINE__) + " " + __FILE__); assert(armed_flag); - hld->vptr.reset(hld->vptr.get(), guarded_delete{del_ptr, true}); + assert(hld->vptr.get() == raw_ptr); + assert(hld->vptr_is_released); + (*shd_ptr_reset_fptr)(hld->vptr, hld->vptr.get(), guarded_delete{shd_ptr_reset_fptr, del_fptr, true}); hld->vptr_is_released = false; (*callback_ptr)(callback_arg); // Py_DECREF. } else if (armed_flag) { - (*del_ptr)(raw_ptr); +to_cout("LOOOK guarded_delete call del " + std::to_string(__LINE__) + " " + __FILE__); + (*del_fptr)(raw_ptr); + } else { +to_cout("LOOOK guarded_delete call noop " + std::to_string(__LINE__) + " " + __FILE__); } } diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 2e3f215b0..107daad85 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -371,6 +371,7 @@ struct smart_holder_type_caster_load { } static void dec_ref_void(void *ptr) { +to_cout("LOOOK dec_ref_void Py_REFCNT(" + std::to_string(Py_REFCNT(reinterpret_cast(ptr))) + ") " + std::to_string(__LINE__) + " " + __FILE__); gil_scoped_acquire gil; Py_DECREF(reinterpret_cast(ptr)); } @@ -378,6 +379,7 @@ struct smart_holder_type_caster_load { struct shared_ptr_dec_ref_deleter { PyObject *self; void operator()(void *) { +to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + " " + __FILE__); gil_scoped_acquire gil; Py_DECREF(self); } @@ -399,25 +401,32 @@ struct smart_holder_type_caster_load { auto vptr_del_ptr = std::get_deleter(hld.vptr); if (vptr_del_ptr != nullptr) { assert(!hld.vptr_is_released); - std::shared_ptr released(hld.vptr.get(), [](void *) {}); + std::shared_ptr to_be_returned(hld.vptr, type_raw_ptr); +int sftstat = pybindit::memory::shared_from_this_status(type_raw_ptr); +to_cout("LOOOK loaded_as_shared_ptr return released SFT=" + std::to_string(sftstat) + " #1 " + std::to_string(__LINE__) + " " + __FILE__); + std::shared_ptr non_owning(hld.vptr.get(), [](void *) {}); // Critical transfer-of-ownership section. This must stay together. vptr_del_ptr->hld = &hld; vptr_del_ptr->callback_ptr = dec_ref_void; vptr_del_ptr->callback_arg = self; - hld.vptr.swap(released); + hld.vptr = non_owning; hld.vptr_is_released = true; Py_INCREF(self); // Critical section end. - return std::shared_ptr(released, type_raw_ptr); +sftstat = pybindit::memory::shared_from_this_status(type_raw_ptr); +to_cout("LOOOK loaded_as_shared_ptr return released SFT=" + std::to_string(sftstat) + " #2 " + std::to_string(__LINE__) + " " + __FILE__); + return to_be_returned; } // XXX XXX XXX Ensure not shared_from_this. Py_INCREF(self); +to_cout("LOOOK loaded_as_shared_ptr return shared_ptr_dec_ref_deleter " + std::to_string(__LINE__) + " " + __FILE__); return std::shared_ptr(type_raw_ptr, shared_ptr_dec_ref_deleter{self}); } if (hld.vptr_is_using_noop_deleter) { throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr)."); } std::shared_ptr void_shd_ptr = hld.template as_shared_ptr(); +to_cout("LOOOK loaded_as_shared_ptr return hld vptr " + std::to_string(__LINE__) + " " + __FILE__); return std::shared_ptr(void_shd_ptr, type_raw_ptr); } @@ -701,10 +710,12 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l void *src_raw_void_ptr = static_cast(src_raw_ptr); const detail::type_info *tinfo = st.second; - if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) + if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { // SMART_HOLDER_WIP: MISSING: Enforcement of consistency with existing smart_holder. // SMART_HOLDER_WIP: MISSING: keep_alive. +to_cout("LOOOK shtc sh return existing_inst " + std::to_string(__LINE__) + " " + __FILE__); return existing_inst; + } auto inst = reinterpret_steal(make_new_instance(tinfo->type)); auto *inst_raw_ptr = reinterpret_cast(inst.ptr()); @@ -718,6 +729,7 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l if (policy == return_value_policy::reference_internal) keep_alive_impl(inst, parent); +to_cout("LOOOK shtc sh return new inst " + std::to_string(__LINE__) + " " + __FILE__); return inst.release(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7a865b3c6..563bc8daf 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -109,6 +109,7 @@ set(PYBIND11_TEST_FILES test_class_sh_shared_from_this.cpp test_class_sh_trampoline_basic.cpp test_class_sh_trampoline_self_life_support.cpp + test_class_sh_trampoline_shared_from_this.cpp test_class_sh_trampoline_shared_ptr_cpp_arg.cpp test_class_sh_trampoline_unique_ptr.cpp test_class_sh_unique_ptr_member.cpp diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp new file mode 100644 index 000000000..f6329eefa --- /dev/null +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -0,0 +1,36 @@ +// 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/smart_holder.h" +#include "pybind11_tests.h" + +#include +#include + +namespace { + +struct WithSft : std::enable_shared_from_this { + virtual ~WithSft() = default; +}; + +struct WithSftTrampoline : WithSft { + using WithSft::WithSft; +}; + +void pass_shared_ptr(const std::shared_ptr &obj) { + to_cout("LOOOK pass_shared_ptr entry"); + to_cout("LOOOK obj->shared_from_this();"); + obj->shared_from_this(); + to_cout("LOOOK pass_shared_ptr return"); +} + +} // namespace + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(WithSft) + +TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) { + py::classh(m, "WithSft").def(py::init<>()); + m.def("pass_shared_ptr", pass_shared_ptr); + m.def("to_cout", to_cout); +} diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py new file mode 100644 index 000000000..f7840a382 --- /dev/null +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- + +import pybind11_tests.class_sh_trampoline_shared_from_this as m + + +class PyWithSft(m.WithSft): + pass + + +def test_pass_shared_ptr(): + m.to_cout("") + m.to_cout(">>> obj = PyWithSft()") + obj = PyWithSft() + m.to_cout(">>> m.pass_shared_ptr(obj) #1") + m.pass_shared_ptr(obj) + m.to_cout(">>> m.pass_shared_ptr(obj) #2") + m.pass_shared_ptr(obj) + m.to_cout(">>> del obj") + del obj