From bfd4c4893eb38159316b61e81091f021c8576bcc Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 21 Jun 2021 18:09:14 -0700 Subject: [PATCH] Leveraging new `noop_deleter_acting_as_weak_ptr_owner` to retrieve released `vptr`. The placeholder `vptr` is never exposed anymore, therefore the externally visible `use_count`s are more intuitive. --- include/pybind11/detail/smart_holder_poc.h | 6 ++++ .../detail/smart_holder_type_casters.h | 36 +++++++++++-------- ...t_class_sh_trampoline_shared_from_this.cpp | 6 ++-- ...st_class_sh_trampoline_shared_from_this.py | 26 ++++++++------ 4 files changed, 47 insertions(+), 27 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 11386044d..e0e7623ce 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -123,6 +123,12 @@ guarded_delete make_guarded_custom_deleter(bool armed_flag) { return guarded_delete(shd_ptr_reset, custom_delete, armed_flag); }; +// Trick to keep the smart_holder memory footprint small. +struct noop_deleter_acting_as_weak_ptr_owner { + std::weak_ptr passenger; + void operator()(void *) {}; +}; + template inline bool is_std_default_delete(const std::type_info &rtti_deleter) { return rtti_deleter == typeid(std::default_delete) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index d136b5187..1e5cc152a 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -398,35 +398,41 @@ to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + " auto type_raw_ptr = convert_type(void_raw_ptr); if (hld.pointee_depends_on_holder_owner) { auto self = reinterpret_cast(load_impl.loaded_v_h.inst); - auto vptr_del_ptr = std::get_deleter(hld.vptr); - if (vptr_del_ptr != nullptr) { + auto vptr_gd_ptr = std::get_deleter(hld.vptr); + if (vptr_gd_ptr != nullptr) { assert(!hld.vptr_is_released); 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 *) {}); + std::shared_ptr non_owning( + hld.vptr.get(), + pybindit::memory::noop_deleter_acting_as_weak_ptr_owner{hld.vptr}); // 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; + vptr_gd_ptr->hld = &hld; + vptr_gd_ptr->callback_ptr = dec_ref_void; + vptr_gd_ptr->callback_arg = self; hld.vptr = non_owning; hld.vptr_is_released = true; Py_INCREF(self); // Critical section end. -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}); + auto vptr_ndaawp_ptr = std::get_deleter< + pybindit::memory::noop_deleter_acting_as_weak_ptr_owner>(hld.vptr); + if (vptr_ndaawp_ptr != nullptr) { + assert(hld.vptr_is_released); + auto released_vptr = vptr_ndaawp_ptr->passenger.lock(); + if (released_vptr != nullptr) { + return std::shared_ptr(released_vptr, type_raw_ptr); + } + } + pybind11_fail( + "smart_holder_type_casters: loaded_as_shared_ptr failure:" + " fatal internal inconsistency."); } if (hld.vptr_is_using_noop_deleter) { throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr)."); } + assert(!hld.vptr_is_released); 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); } diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index 096f48cee..40afaf095 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -74,8 +74,10 @@ struct SftTrampoline : Sft, py::trampoline_self_life_support { using Sft::Sft; }; -void pass_shared_ptr(const std::shared_ptr &obj) { - obj->shared_from_this()->history += "_PassSharedPtr"; +long pass_shared_ptr(const std::shared_ptr &obj) { + auto sft = obj->shared_from_this(); + sft->history += "_PassSharedPtr"; + return sft.use_count(); } void pass_unique_ptr(const std::unique_ptr &) {} diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index 6068230f9..cab007973 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -20,7 +20,8 @@ def test_pass_shared_ptr(): m.pass_shared_ptr(obj) assert obj.history == "PySft_PassSharedPtr" assert obj.use_count() in [2, -1] - m.pass_shared_ptr(obj) + uc = m.pass_shared_ptr(obj) + assert uc == 2 # +1 for passed argument, +1 for shared_from_this. assert obj.history == "PySft_PassSharedPtr_PassSharedPtr" assert obj.use_count() in [2, -1] @@ -32,28 +33,33 @@ def test_pass_shared_ptr_while_stashed(): stash1.Add(obj) assert obj.history == "PySft_Stash1Add" assert obj.use_count() in [2, -1] - m.pass_shared_ptr(obj) + assert stash1.history(0) == "PySft_Stash1Add" + assert stash1.use_count(0) == 1 # obj does NOT own the shared_ptr anymore. + uc = m.pass_shared_ptr(obj) + assert uc == 3 # +1 for passed argument, +1 for shared_from_this. assert obj.history == "PySft_Stash1Add_PassSharedPtr" assert obj.use_count() in [2, -1] + assert stash1.history(0) == "PySft_Stash1Add_PassSharedPtr" + assert stash1.use_count(0) == 1 stash2 = m.SftSharedPtrStash(2) stash2.Add(obj) assert obj.history == "PySft_Stash1Add_PassSharedPtr_Stash2Add" - assert obj.use_count() in [2, -1] + assert obj.use_count() in [3, -1] assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add" - assert stash2.use_count(0) == 1 # TODO: this is not great. + assert stash2.use_count(0) == 2 stash2.Add(obj) assert obj.history == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" - assert obj.use_count() in [2, -1] - assert stash1.use_count(0) == 1 + assert obj.use_count() in [4, -1] + assert stash1.use_count(0) == 3 assert stash1.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" - assert stash2.use_count(0) == 1 + assert stash2.use_count(0) == 3 assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" - assert stash2.use_count(1) == 1 + assert stash2.use_count(1) == 3 assert stash2.history(1) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" del obj - assert stash2.use_count(0) == 1 + assert stash2.use_count(0) == 3 assert stash2.history(0) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" - assert stash2.use_count(1) == 1 + assert stash2.use_count(1) == 3 assert stash2.history(1) == "PySft_Stash1Add_PassSharedPtr_Stash2Add_Stash2Add" del stash2 gc.collect()