From 760585a83db4c3d7e59b3d20513a3b1b6c4f2a8b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 23 Aug 2024 12:13:40 -0700 Subject: [PATCH 01/14] Replace `unique_ptr_cref_roundtrip()` with `pass_unique_ptr_cref()`, `rtrn_unique_ptr_cref()` to make the current behavior obvious. --- include/pybind11/cast.h | 2 +- tests/test_class_sh_basic.cpp | 13 +++++++++++-- tests/test_class_sh_basic.py | 28 +++++++++++++++------------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 0262296aa9..2fafaf9ef8 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1040,7 +1040,7 @@ struct move_only_holder_caster< policy = return_value_policy::reference_internal; } if (policy != return_value_policy::reference_internal) { - throw cast_error("Invalid return_value_policy for unique_ptr&"); + throw cast_error("Invalid return_value_policy for const unique_ptr&"); } return type_caster_base::cast(src.get(), policy, parent); } diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 9602387b34..2f7c34d1f0 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -120,7 +120,14 @@ std::string get_mtxt(atyp const &obj) { return obj.mtxt; } std::ptrdiff_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } std::unique_ptr unique_ptr_roundtrip(std::unique_ptr obj) { return obj; } -const std::unique_ptr &unique_ptr_cref_roundtrip(const std::unique_ptr &obj) { + +std::string pass_unique_ptr_cref(const std::unique_ptr &obj) { return obj->mtxt; } + +const std::unique_ptr &rtrn_unique_ptr_cref(const std::string &mtxt) { + static std::unique_ptr obj{new atyp{"static_ctor_arg"}}; + if (!mtxt.empty()) { + obj->mtxt = mtxt; + } return obj; } @@ -217,7 +224,9 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("get_ptr", get_ptr); // pass_cref m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp - m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); + + m.def("pass_unique_ptr_cref", pass_unique_ptr_cref); + m.def("rtrn_unique_ptr_cref", rtrn_unique_ptr_cref); py::classh(m, "SharedPtrStash") .def(py::init<>()) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 87f1f8f09f..9aabc1a1c3 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -151,19 +151,21 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): id_orig = id_rtrn -# This currently fails, because a unique_ptr is always loaded by value -# due to pybind11/detail/smart_holder_type_casters.h:689 -# I think, we need to provide more cast operators. -@pytest.mark.skip() -def test_unique_ptr_cref_roundtrip(): - orig = m.atyp("passenger") - id_orig = id(orig) - mtxt_orig = m.get_mtxt(orig) - - recycled = m.unique_ptr_cref_roundtrip(orig) - assert m.get_mtxt(orig) == mtxt_orig - assert m.get_mtxt(recycled) == mtxt_orig - assert id(recycled) == id_orig +def test_pass_unique_ptr_cref(): + obj = m.atyp("ctor_arg") + assert m.get_mtxt(obj) == "ctor_arg_MvCtor" + assert m.pass_unique_ptr_cref(obj) == "ctor_arg_MvCtor" + with pytest.raises(ValueError, match="Python instance was disowned"): + m.get_mtxt(obj) + + +def test_rtrn_unique_ptr_cref(): + obj0 = m.rtrn_unique_ptr_cref("") + assert m.get_mtxt(obj0) == "static_ctor_arg" + obj1 = m.rtrn_unique_ptr_cref("passed_mtxt_1") + assert m.get_mtxt(obj1) == "passed_mtxt_1" + assert m.get_mtxt(obj0) == "passed_mtxt_1" + assert obj0 is obj1 @pytest.mark.parametrize( From b969e9c99de8df3da760e58f75957831d8e801c9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 23 Aug 2024 21:54:54 -0700 Subject: [PATCH 02/14] add in unique_ptr_storage, unique_ptr_storage_deleter --- include/pybind11/cast.h | 44 ++++++++++++++++++- include/pybind11/detail/struct_smart_holder.h | 22 ++++++---- tests/test_class_sh_basic.cpp | 5 +++ tests/test_class_sh_basic.py | 14 +++++- 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 2fafaf9ef8..6993ae90e8 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1067,8 +1067,14 @@ struct move_only_holder_caster< + clean_type_id(typeinfo->cpptype->name()) + ")"); } - template - using cast_op_type = std::unique_ptr; + template + using cast_op_type + = conditional_t::type, + const std::unique_ptr &>::value + || std::is_same::type, + const std::unique_ptr &>::value, + const std::unique_ptr &, + std::unique_ptr>; explicit operator std::unique_ptr() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { @@ -1077,6 +1083,39 @@ struct move_only_holder_caster< pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } + explicit operator const std::unique_ptr &() { + if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + unique_ptr_storage = std::shared_ptr>( + new std::unique_ptr{ + sh_load_helper.template load_as_unique_ptr(value)}, + unique_ptr_storage_deleter(&sh_load_helper.holder())); + return *unique_ptr_storage; + } + pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); + } + + struct unique_ptr_storage_deleter { + unique_ptr_storage_deleter(smart_holder *hld) : hld{hld} {} + + void operator()(std::unique_ptr *ptr) { + if (*ptr) { + if (hld->is_disowned) { + hld->reclaim_disowned(); + ptr->release(); + } else if (hld->is_populated) { + hld->populate_from_unique_ptr(std::move(*ptr)); + ptr->release(); + } else { + pybind11_fail("Expected to be UNREACHABLE: " __FILE__ + ":" PYBIND11_TOSTRING(__LINE__)); + } + } + delete ptr; + } + + smart_holder *hld; + }; + bool try_implicit_casts(handle src, bool convert) { for (auto &cast : typeinfo->implicit_casts) { move_only_holder_caster sub_caster(*cast.first); @@ -1096,6 +1135,7 @@ struct move_only_holder_caster< static bool try_direct_conversions(handle) { return false; } + std::shared_ptr> unique_ptr_storage; smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl }; diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index ccd289716b..24e7ccb47a 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -287,24 +287,30 @@ struct smart_holder { release_disowned(); } + // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). template - static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *void_ptr = nullptr) { - smart_holder hld; - hld.rtti_uqp_del = &typeid(D); - hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); + void populate_from_unique_ptr(std::unique_ptr &&unq_ptr, void *void_ptr = nullptr) { + rtti_uqp_del = &typeid(D); + vptr_is_using_builtin_delete = is_std_default_delete(*rtti_uqp_del); guarded_delete gd{nullptr, false}; - if (hld.vptr_is_using_builtin_delete) { + if (vptr_is_using_builtin_delete) { gd = make_guarded_builtin_delete(true); } else { gd = make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); } if (void_ptr != nullptr) { - hld.vptr.reset(void_ptr, std::move(gd)); + vptr.reset(void_ptr, std::move(gd)); } else { - hld.vptr.reset(unq_ptr.get(), std::move(gd)); + vptr.reset(unq_ptr.get(), std::move(gd)); } (void) unq_ptr.release(); + } + + template + static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, + void *void_ptr = nullptr) { + smart_holder hld; + hld.populate_from_unique_ptr(std::move(unq_ptr), void_ptr); hld.is_populated = true; return hld; } diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 2f7c34d1f0..ac19a750bf 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -131,6 +131,10 @@ const std::unique_ptr &rtrn_unique_ptr_cref(const std::string &mtxt) { return obj; } +const std::unique_ptr &unique_ptr_cref_roundtrip(const std::unique_ptr &obj) { + return obj; +} + struct SharedPtrStash { std::vector> stash; void Add(const std::shared_ptr &obj) { stash.push_back(obj); } @@ -227,6 +231,7 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_unique_ptr_cref", pass_unique_ptr_cref); m.def("rtrn_unique_ptr_cref", rtrn_unique_ptr_cref); + m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); py::classh(m, "SharedPtrStash") .def(py::init<>()) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 9aabc1a1c3..4e936803e6 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -155,8 +155,7 @@ def test_pass_unique_ptr_cref(): obj = m.atyp("ctor_arg") assert m.get_mtxt(obj) == "ctor_arg_MvCtor" assert m.pass_unique_ptr_cref(obj) == "ctor_arg_MvCtor" - with pytest.raises(ValueError, match="Python instance was disowned"): - m.get_mtxt(obj) + assert m.get_mtxt(obj) == "ctor_arg_MvCtor" def test_rtrn_unique_ptr_cref(): @@ -168,6 +167,17 @@ def test_rtrn_unique_ptr_cref(): assert obj0 is obj1 +def test_unique_ptr_cref_roundtrip(num_round_trips=1000): + # Multiple roundtrips to stress-test implementation. + orig = m.atyp("passenger") + mtxt_orig = m.get_mtxt(orig) + recycled = orig + for _ in range(num_round_trips): + recycled = m.unique_ptr_cref_roundtrip(recycled) + assert recycled is orig + assert m.get_mtxt(recycled) == mtxt_orig + + @pytest.mark.parametrize( ("pass_f", "rtrn_f", "moved_out", "moved_in"), [ From 351a877a4c930126c1e3feaf5e590933fb95f8e5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 12:52:21 -0700 Subject: [PATCH 03/14] Add shared_ptr_storage (with that disowning fails as expected). --- include/pybind11/cast.h | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 6993ae90e8..37fa8ff94e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1085,35 +1085,27 @@ struct move_only_holder_caster< explicit operator const std::unique_ptr &() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); unique_ptr_storage = std::shared_ptr>( new std::unique_ptr{ sh_load_helper.template load_as_unique_ptr(value)}, - unique_ptr_storage_deleter(&sh_load_helper.holder())); + unique_ptr_storage_deleter()); return *unique_ptr_storage; } pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } struct unique_ptr_storage_deleter { - unique_ptr_storage_deleter(smart_holder *hld) : hld{hld} {} + unique_ptr_storage_deleter() {} void operator()(std::unique_ptr *ptr) { - if (*ptr) { - if (hld->is_disowned) { - hld->reclaim_disowned(); - ptr->release(); - } else if (hld->is_populated) { - hld->populate_from_unique_ptr(std::move(*ptr)); - ptr->release(); - } else { - pybind11_fail("Expected to be UNREACHABLE: " __FILE__ - ":" PYBIND11_TOSTRING(__LINE__)); - } + if (!ptr) { + pybind11_fail("Expected to be UNREACHABLE: " __FILE__ + ":" PYBIND11_TOSTRING(__LINE__)); } + ptr->release(); delete ptr; } - - smart_holder *hld; }; bool try_implicit_casts(handle src, bool convert) { @@ -1136,6 +1128,7 @@ struct move_only_holder_caster< static bool try_direct_conversions(handle) { return false; } std::shared_ptr> unique_ptr_storage; + std::shared_ptr shared_ptr_storage; smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl }; From fb37c10203e0878a5fe3fe6250c33dcc749e35d7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 13:14:56 -0700 Subject: [PATCH 04/14] Add load_as_const_unique_ptr() --- include/pybind11/cast.h | 3 +- include/pybind11/detail/type_caster_base.h | 28 +++++++++++++++++++ ...t_class_sh_trampoline_shared_from_this.cpp | 10 +++++-- ...st_class_sh_trampoline_shared_from_this.py | 5 +++- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 37fa8ff94e..05bd437b5d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1088,7 +1088,8 @@ struct move_only_holder_caster< shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); unique_ptr_storage = std::shared_ptr>( new std::unique_ptr{ - sh_load_helper.template load_as_unique_ptr(value)}, + sh_load_helper.template load_as_const_unique_ptr( + shared_ptr_storage.get())}, unique_ptr_storage_deleter()); return *unique_ptr_storage; } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 9edfe20228..fb0f513c0a 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -829,6 +829,34 @@ struct load_helper : value_and_holder_helper { return result; } + + // This assumes load_as_shared_ptr succeeded, and the returned shared_ptr is still alive. + // The returned unique_ptr is meant to never expire. + template + std::unique_ptr load_as_const_unique_ptr(T *raw_type_ptr) { + if (!have_holder()) { + return unique_with_deleter(nullptr, std::unique_ptr()); + } + + // Temporary variable to store the extracted deleter in. + std::unique_ptr extracted_deleter; + + auto *gd = std::get_deleter(holder().vptr); + if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del() call above. + // In struct_smart_holder, a custom deleter is always stored in a guarded delete. + // The guarded delete's std::function actually points at the + // custom_deleter type, so we can verify it is of the custom deleter type and + // finally extract its deleter. + using custom_deleter_D = pybindit::memory::custom_deleter; + const auto &custom_deleter_ptr = gd->del_fun.template target(); + assert(custom_deleter_ptr != nullptr); + // Now that we have confirmed the type of the deleter matches the desired return + // value we can extract the function. + extracted_deleter = std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); + } + + return unique_with_deleter(raw_type_ptr, std::move(extracted_deleter)); + } }; PYBIND11_NAMESPACE_END(smart_holder_type_caster_support) diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index e5d688af66..f2a71a1bfd 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -87,7 +87,12 @@ long pass_shared_ptr(const std::shared_ptr &obj) { return sft.use_count(); } -void pass_unique_ptr(const std::unique_ptr &) {} +std::string pass_unique_ptr_cref(const std::unique_ptr &obj) { + return obj ? obj->history : ""; +} +std::string pass_unique_ptr_rref(std::unique_ptr &&) { + throw std::runtime_error("Expected to not be reached."); +} Sft *make_pure_cpp_sft_raw_ptr(const std::string &history_seed) { return new Sft{history_seed}; } @@ -135,7 +140,8 @@ TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) { m.def("use_count", use_count); m.def("pass_shared_ptr", pass_shared_ptr); - m.def("pass_unique_ptr", pass_unique_ptr); + m.def("pass_unique_ptr_cref", pass_unique_ptr_cref); + m.def("pass_unique_ptr_rref", pass_unique_ptr_rref); m.def("make_pure_cpp_sft_raw_ptr", make_pure_cpp_sft_raw_ptr); m.def("make_pure_cpp_sft_unq_ptr", make_pure_cpp_sft_unq_ptr); m.def("make_pure_cpp_sft_shd_ptr", make_pure_cpp_sft_shd_ptr); diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index a074b20a17..7c5ee0e8ef 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -137,11 +137,14 @@ def test_pass_released_shared_ptr_as_unique_ptr(): obj = PySft("PySft") stash1 = m.SftSharedPtrStash(1) stash1.Add(obj) # Releases shared_ptr to C++. + assert m.pass_unique_ptr_cref(obj) == "PySft_Stash1Add" + assert obj.history == "PySft_Stash1Add" with pytest.raises(ValueError) as exc_info: - m.pass_unique_ptr(obj) + m.pass_unique_ptr_rref(obj) assert str(exc_info.value) == ( "Python instance is currently owned by a std::shared_ptr." ) + assert obj.history == "PySft_Stash1Add" @pytest.mark.parametrize( From d1e18d9ac27eb9c117f78fcc13c5dd93df047c11 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 14:13:52 -0700 Subject: [PATCH 05/14] Restore original struct_smart_holder.h --- include/pybind11/detail/struct_smart_holder.h | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 24e7ccb47a..ccd289716b 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -287,30 +287,24 @@ struct smart_holder { release_disowned(); } - // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details). template - void populate_from_unique_ptr(std::unique_ptr &&unq_ptr, void *void_ptr = nullptr) { - rtti_uqp_del = &typeid(D); - vptr_is_using_builtin_delete = is_std_default_delete(*rtti_uqp_del); + static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, + void *void_ptr = nullptr) { + smart_holder hld; + hld.rtti_uqp_del = &typeid(D); + hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); guarded_delete gd{nullptr, false}; - if (vptr_is_using_builtin_delete) { + if (hld.vptr_is_using_builtin_delete) { gd = make_guarded_builtin_delete(true); } else { gd = make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); } if (void_ptr != nullptr) { - vptr.reset(void_ptr, std::move(gd)); + hld.vptr.reset(void_ptr, std::move(gd)); } else { - vptr.reset(unq_ptr.get(), std::move(gd)); + hld.vptr.reset(unq_ptr.get(), std::move(gd)); } (void) unq_ptr.release(); - } - - template - static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *void_ptr = nullptr) { - smart_holder hld; - hld.populate_from_unique_ptr(std::move(unq_ptr), void_ptr); hld.is_populated = true; return hld; } From d4a96bb00aec89cf6f5b9ae96ff2b3ebbd18988b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 14:45:16 -0700 Subject: [PATCH 06/14] factor out `smart_holder::extract_deleter()` --- include/pybind11/cast.h | 2 +- include/pybind11/detail/struct_smart_holder.h | 16 +++++++ include/pybind11/detail/type_caster_base.h | 46 ++++--------------- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 05bd437b5d..9cd3b9269a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1104,7 +1104,7 @@ struct move_only_holder_caster< pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } - ptr->release(); + (void) ptr->release(); delete ptr; } }; diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index ccd289716b..401a6fd1b2 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -231,6 +231,22 @@ struct smart_holder { vptr_del_ptr->armed_flag = armed_flag; } + // Caller is responsible for precondition: ensure_compatible_rtti_uqp_del() must succeed. + template + std::unique_ptr extract_deleter(const char *context) const { + auto *gd = std::get_deleter(vptr); + if (gd && gd->use_del_fun) { + const auto &custom_deleter_ptr = gd->del_fun.template target>(); + if (custom_deleter_ptr == nullptr) { + throw std::runtime_error( + std::string("smart_holder::extract_deleter() precondition failure (") + context + + ")."); + } + return std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); + } + return nullptr; + } + static smart_holder from_raw_ptr_unowned(void *raw_ptr) { smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index fb0f513c0a..02de8f618a 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -794,22 +794,7 @@ struct load_helper : value_and_holder_helper { "instance cannot safely be transferred to C++."); } - // Temporary variable to store the extracted deleter in. - std::unique_ptr extracted_deleter; - - auto *gd = std::get_deleter(holder().vptr); - if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del() call above. - // In struct_smart_holder, a custom deleter is always stored in a guarded delete. - // The guarded delete's std::function actually points at the - // custom_deleter type, so we can verify it is of the custom deleter type and - // finally extract its deleter. - using custom_deleter_D = pybindit::memory::custom_deleter; - const auto &custom_deleter_ptr = gd->del_fun.template target(); - assert(custom_deleter_ptr != nullptr); - // Now that we have confirmed the type of the deleter matches the desired return - // value we can extract the function. - extracted_deleter = std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); - } + std::unique_ptr extracted_deleter = holder().extract_deleter(context); // Critical transfer-of-ownership section. This must stay together. if (self_life_support != nullptr) { @@ -830,32 +815,17 @@ struct load_helper : value_and_holder_helper { return result; } - // This assumes load_as_shared_ptr succeeded, and the returned shared_ptr is still alive. - // The returned unique_ptr is meant to never expire. + // This assumes load_as_shared_ptr succeeded(), and the returned shared_ptr is still alive. + // The returned unique_ptr is meant to never expire (the behavior is undefined otherwise). template - std::unique_ptr load_as_const_unique_ptr(T *raw_type_ptr) { + std::unique_ptr + load_as_const_unique_ptr(T *raw_type_ptr, const char *context = "load_as_const_unique_ptr") { if (!have_holder()) { return unique_with_deleter(nullptr, std::unique_ptr()); } - - // Temporary variable to store the extracted deleter in. - std::unique_ptr extracted_deleter; - - auto *gd = std::get_deleter(holder().vptr); - if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del() call above. - // In struct_smart_holder, a custom deleter is always stored in a guarded delete. - // The guarded delete's std::function actually points at the - // custom_deleter type, so we can verify it is of the custom deleter type and - // finally extract its deleter. - using custom_deleter_D = pybindit::memory::custom_deleter; - const auto &custom_deleter_ptr = gd->del_fun.template target(); - assert(custom_deleter_ptr != nullptr); - // Now that we have confirmed the type of the deleter matches the desired return - // value we can extract the function. - extracted_deleter = std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); - } - - return unique_with_deleter(raw_type_ptr, std::move(extracted_deleter)); + holder().template ensure_compatible_rtti_uqp_del(context); + return unique_with_deleter(raw_type_ptr, + std::move(holder().extract_deleter(context))); } }; From 0523d42907cd1f880f2cd8ef296b92b5366b6546 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 14:50:11 -0700 Subject: [PATCH 07/14] Better error message. --- include/pybind11/cast.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 9cd3b9269a..43aed733c1 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1101,8 +1101,8 @@ struct move_only_holder_caster< void operator()(std::unique_ptr *ptr) { if (!ptr) { - pybind11_fail("Expected to be UNREACHABLE: " __FILE__ - ":" PYBIND11_TOSTRING(__LINE__)); + pybind11_fail("FATAL: `const std::unique_ptr &` was disowned (EXPECT " + "UNDEFINED BEHAVIOR)."); } (void) ptr->release(); delete ptr; From a300855ca0419b76c97d85bd50f435d251c60723 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 15:01:28 -0700 Subject: [PATCH 08/14] Misc cleanup/tidying. --- include/pybind11/cast.h | 40 +++++++++------------- include/pybind11/detail/type_caster_base.h | 6 ++-- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 43aed733c1..4ea7ae4aa6 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -871,14 +871,14 @@ struct copyable_holder_caster< pybind11_fail("Passing `std::shared_ptr *` from Python to C++ is not supported " "(inherently unsafe)."); } - return std::addressof(shared_ptr_holder); + return std::addressof(shared_ptr_storage); } explicit operator std::shared_ptr &() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { - shared_ptr_holder = sh_load_helper.load_as_shared_ptr(value); + shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); } - return shared_ptr_holder; + return shared_ptr_storage; } static handle @@ -924,7 +924,7 @@ struct copyable_holder_caster< } if (v_h.holder_constructed()) { value = v_h.value_ptr(); - shared_ptr_holder = v_h.template holder>(); + shared_ptr_storage = v_h.template holder>(); return; } throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " @@ -953,8 +953,8 @@ struct copyable_holder_caster< if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = sub_caster.sh_load_helper.loaded_v_h; } else { - shared_ptr_holder - = std::shared_ptr(sub_caster.shared_ptr_holder, (type *) value); + shared_ptr_storage + = std::shared_ptr(sub_caster.shared_ptr_storage, (type *) value); } return true; } @@ -964,8 +964,8 @@ struct copyable_holder_caster< static bool try_direct_conversions(handle) { return false; } - std::shared_ptr shared_ptr_holder; smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl + std::shared_ptr shared_ptr_storage; }; #endif // PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT @@ -1090,25 +1090,19 @@ struct move_only_holder_caster< new std::unique_ptr{ sh_load_helper.template load_as_const_unique_ptr( shared_ptr_storage.get())}, - unique_ptr_storage_deleter()); + [](std::unique_ptr *ptr) { + if (!ptr) { + pybind11_fail("FATAL: `const std::unique_ptr &` was disowned " + "(EXPECT UNDEFINED BEHAVIOR)."); + } + (void) ptr->release(); + delete ptr; + }); return *unique_ptr_storage; } pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } - struct unique_ptr_storage_deleter { - unique_ptr_storage_deleter() {} - - void operator()(std::unique_ptr *ptr) { - if (!ptr) { - pybind11_fail("FATAL: `const std::unique_ptr &` was disowned (EXPECT " - "UNDEFINED BEHAVIOR)."); - } - (void) ptr->release(); - delete ptr; - } - }; - bool try_implicit_casts(handle src, bool convert) { for (auto &cast : typeinfo->implicit_casts) { move_only_holder_caster sub_caster(*cast.first); @@ -1128,9 +1122,9 @@ struct move_only_holder_caster< static bool try_direct_conversions(handle) { return false; } - std::shared_ptr> unique_ptr_storage; - std::shared_ptr shared_ptr_storage; smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl + std::shared_ptr shared_ptr_storage; + std::shared_ptr> unique_ptr_storage; }; #endif // PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 02de8f618a..f1a5d0d8dc 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -794,7 +794,7 @@ struct load_helper : value_and_holder_helper { "instance cannot safely be transferred to C++."); } - std::unique_ptr extracted_deleter = holder().extract_deleter(context); + std::unique_ptr extracted_deleter = holder().template extract_deleter(context); // Critical transfer-of-ownership section. This must stay together. if (self_life_support != nullptr) { @@ -824,8 +824,8 @@ struct load_helper : value_and_holder_helper { return unique_with_deleter(nullptr, std::unique_ptr()); } holder().template ensure_compatible_rtti_uqp_del(context); - return unique_with_deleter(raw_type_ptr, - std::move(holder().extract_deleter(context))); + return unique_with_deleter( + raw_type_ptr, std::move(holder().template extract_deleter(context))); } }; From 48b1bf62611007a2921cc7278990c2a818cd7719 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 24 Aug 2024 16:11:57 -0700 Subject: [PATCH 09/14] Use `re.match("ctor_arg(_MvCtor)*_MvCtor", ...)` for compatibility with MSVC, NVHPC, ICC --- tests/test_class_sh_basic.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 4e936803e6..7db7d31b7a 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -153,9 +153,9 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): def test_pass_unique_ptr_cref(): obj = m.atyp("ctor_arg") - assert m.get_mtxt(obj) == "ctor_arg_MvCtor" - assert m.pass_unique_ptr_cref(obj) == "ctor_arg_MvCtor" - assert m.get_mtxt(obj) == "ctor_arg_MvCtor" + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.get_mtxt(obj)) + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.pass_unique_ptr_cref(obj)) + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.get_mtxt(obj)) def test_rtrn_unique_ptr_cref(): From e47e43c039640c9ed1b014b19b6b9aa79878a82d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 25 Aug 2024 09:05:18 -0700 Subject: [PATCH 10/14] Add small comments. --- include/pybind11/cast.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 4ea7ae4aa6..8ecc20f25c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1085,7 +1085,9 @@ struct move_only_holder_caster< explicit operator const std::unique_ptr &() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + // Get shared_ptr to ensure that the Python object is not disowned elsewhere. shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); + // Build a temporary unique_ptr that is meant to never expire. unique_ptr_storage = std::shared_ptr>( new std::unique_ptr{ sh_load_helper.template load_as_const_unique_ptr( @@ -1123,7 +1125,7 @@ struct move_only_holder_caster< static bool try_direct_conversions(handle) { return false; } smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl - std::shared_ptr shared_ptr_storage; + std::shared_ptr shared_ptr_storage; // Serves as a pseudo lock. std::shared_ptr> unique_ptr_storage; }; From 028f18178aea0bc1b35d9db347be5b129fcb1a22 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 25 Aug 2024 09:09:06 -0700 Subject: [PATCH 11/14] Fix small, inconsequential oversight in test code. --- tests/test_class_sh_trampoline_shared_from_this.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index f2a71a1bfd..9c2e4ec762 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -90,7 +90,7 @@ long pass_shared_ptr(const std::shared_ptr &obj) { std::string pass_unique_ptr_cref(const std::unique_ptr &obj) { return obj ? obj->history : ""; } -std::string pass_unique_ptr_rref(std::unique_ptr &&) { +void pass_unique_ptr_rref(std::unique_ptr &&) { throw std::runtime_error("Expected to not be reached."); } From a0be89d203994d51181de0f5f96c3e9093241027 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 25 Aug 2024 09:14:35 -0700 Subject: [PATCH 12/14] Apply suggestion by @iwanders under PR #5334 --- include/pybind11/detail/struct_smart_holder.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 401a6fd1b2..75bae7e564 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -234,7 +234,7 @@ struct smart_holder { // Caller is responsible for precondition: ensure_compatible_rtti_uqp_del() must succeed. template std::unique_ptr extract_deleter(const char *context) const { - auto *gd = std::get_deleter(vptr); + const auto *gd = std::get_deleter(vptr); if (gd && gd->use_del_fun) { const auto &custom_deleter_ptr = gd->del_fun.template target>(); if (custom_deleter_ptr == nullptr) { From 9adbb61d36eb1e69a30950b39051f59f30f4eb47 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 25 Aug 2024 09:55:53 -0700 Subject: [PATCH 13/14] Remove `std::move()` in `smart_holder::extract_deleter()` --- include/pybind11/detail/struct_smart_holder.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 75bae7e564..a6faf06e5e 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -242,7 +242,7 @@ struct smart_holder { std::string("smart_holder::extract_deleter() precondition failure (") + context + ")."); } - return std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); + return std::unique_ptr(new D(custom_deleter_ptr->deleter)); // D must be copyable. } return nullptr; } From be3bafe9df489c90534391207903249d8ca621e5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 25 Aug 2024 10:02:41 -0700 Subject: [PATCH 14/14] Add `static_assert()` following a suggestion by @iwanders under PR #5334 --- include/pybind11/detail/struct_smart_holder.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index a6faf06e5e..b1e24d7bb3 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -242,7 +242,9 @@ struct smart_holder { std::string("smart_holder::extract_deleter() precondition failure (") + context + ")."); } - return std::unique_ptr(new D(custom_deleter_ptr->deleter)); // D must be copyable. + static_assert(std::is_copy_constructible::value, + "Required for compatibility with smart_holder functionality."); + return std::unique_ptr(new D(custom_deleter_ptr->deleter)); } return nullptr; }