diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index de92f4156f..e169f2f26b 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -301,7 +301,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - bool void_cast_raw_ptr = false) { + 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); @@ -311,8 +311,8 @@ struct smart_holder { } else { gd = make_guarded_custom_deleter(true); } - if (void_cast_raw_ptr) { - hld.vptr.reset(static_cast(unq_ptr.get()), std::move(gd)); + if (void_ptr != nullptr) { + hld.vptr.reset(void_ptr, std::move(gd)); } else { hld.vptr.reset(unq_ptr.get(), std::move(gd)); } diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 9f95e8c72b..b3289773a7 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -386,8 +386,8 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag template static smart_holder smart_holder_from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr) { - return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), - void_cast_raw_ptr); + void *void_ptr = void_cast_raw_ptr ? static_cast(unq_ptr.get()) : nullptr; + return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), void_ptr); } template @@ -836,7 +836,8 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; - auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src); + auto smhldr = pybindit::memory::smart_holder::from_shared_ptr( + std::shared_ptr(src, const_cast(st.first))); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) { @@ -890,17 +891,16 @@ struct smart_holder_type_caster> : smart_holder_type_caste return none().release(); } - auto src_raw_ptr = src.get(); - auto st = type_caster_base::src_and_type(src_raw_ptr); + auto st = type_caster_base::src_and_type(src.get()); if (st.second == nullptr) { return handle(); // no type info: error will be set already } - void *src_raw_void_ptr = static_cast(src_raw_ptr); + void *src_raw_void_ptr = const_cast(st.first); const detail::type_info *tinfo = st.second; if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { auto *self_life_support - = dynamic_raw_ptr_cast_if_possible(src_raw_ptr); + = dynamic_raw_ptr_cast_if_possible(src.get()); if (self_life_support != nullptr) { value_and_holder &v_h = self_life_support->v_h; if (v_h.inst != nullptr && v_h.vh != nullptr) { @@ -926,8 +926,14 @@ struct smart_holder_type_caster> : smart_holder_type_caste void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; - auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src), - /*void_cast_raw_ptr*/ false); + if (static_cast(src.get()) == src_raw_void_ptr) { + // This is a multiple-inheritance situation that is incompatible with the current + // shared_from_this handling (see PR #3023). + // SMART_HOLDER_WIP: IMPROVABLE: Is there a better solution? + src_raw_void_ptr = nullptr; + } + auto smhldr + = pybindit::memory::smart_holder::from_unique_ptr(std::move(src), src_raw_void_ptr); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d958a8c5ca..e6970739ff 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -127,6 +127,7 @@ set(PYBIND11_TEST_FILES test_class_sh_disowning_mi test_class_sh_factory_constructors test_class_sh_inheritance + test_class_sh_mi_thunks test_class_sh_module_local.py test_class_sh_property test_class_sh_shared_ptr_copy_move diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp new file mode 100644 index 0000000000..c4f430e864 --- /dev/null +++ b/tests/test_class_sh_mi_thunks.cpp @@ -0,0 +1,100 @@ +#include +#include + +#include "pybind11_tests.h" + +#include +#include +#include + +namespace test_class_sh_mi_thunks { + +// For general background: https://shaharmike.com/cpp/vtable-part2/ +// C++ vtables - Part 2 - Multiple Inheritance +// ... the compiler creates a 'thunk' method that corrects `this` ... + +struct Base0 { + virtual ~Base0() = default; + Base0() = default; + Base0(const Base0 &) = delete; +}; + +struct Base1 { + virtual ~Base1() = default; + // Using `vector` here because it is known to make this test very sensitive to bugs. + std::vector vec = {1, 2, 3, 4, 5}; + Base1() = default; + Base1(const Base1 &) = delete; +}; + +struct Derived : Base1, Base0 { + ~Derived() override = default; + Derived() = default; + Derived(const Derived &) = delete; +}; + +} // namespace test_class_sh_mi_thunks + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Base0) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Base1) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Derived) + +TEST_SUBMODULE(class_sh_mi_thunks, m) { + using namespace test_class_sh_mi_thunks; + + m.def("ptrdiff_drvd_base0", []() { + auto drvd = std::unique_ptr(new Derived); + auto *base0 = dynamic_cast(drvd.get()); + return std::ptrdiff_t(reinterpret_cast(drvd.get()) + - reinterpret_cast(base0)); + }); + + py::classh(m, "Base0"); + py::classh(m, "Base1"); + py::classh(m, "Derived"); + + m.def( + "get_drvd_as_base0_raw_ptr", + []() { + auto *drvd = new Derived; + auto *base0 = dynamic_cast(drvd); + return base0; + }, + py::return_value_policy::take_ownership); + + m.def("get_drvd_as_base0_shared_ptr", []() { + auto drvd = std::make_shared(); + auto base0 = std::dynamic_pointer_cast(drvd); + return base0; + }); + + m.def("get_drvd_as_base0_unique_ptr", []() { + auto drvd = std::unique_ptr(new Derived); + auto base0 = std::unique_ptr(std::move(drvd)); + return base0; + }); + + m.def("vec_size_base0_raw_ptr", [](const Base0 *obj) { + const auto *obj_der = dynamic_cast(obj); + if (obj_der == nullptr) { + return std::size_t(0); + } + return obj_der->vec.size(); + }); + + m.def("vec_size_base0_shared_ptr", [](const std::shared_ptr &obj) -> std::size_t { + const auto obj_der = std::dynamic_pointer_cast(obj); + if (!obj_der) { + return std::size_t(0); + } + return obj_der->vec.size(); + }); + + m.def("vec_size_base0_unique_ptr", [](std::unique_ptr obj) -> std::size_t { + const auto *obj_der = dynamic_cast(obj.get()); + if (obj_der == nullptr) { + return std::size_t(0); + } + return obj_der->vec.size(); + }); +} diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py new file mode 100644 index 0000000000..7d6e3849a9 --- /dev/null +++ b/tests/test_class_sh_mi_thunks.py @@ -0,0 +1,56 @@ +import pytest + +from pybind11_tests import class_sh_mi_thunks as m + + +def test_ptrdiff_drvd_base0(): + ptrdiff = m.ptrdiff_drvd_base0() + # A failure here does not (necessarily) mean that there is a bug, but that + # test_class_sh_mi_thunks is not exercising what it is supposed to. + # If this ever fails on some platforms: use pytest.skip() + # If this ever fails on all platforms: don't know, seems extremely unlikely. + assert ptrdiff != 0 + + +@pytest.mark.parametrize( + "vec_size_fn", + [ + m.vec_size_base0_raw_ptr, + m.vec_size_base0_shared_ptr, + ], +) +@pytest.mark.parametrize( + "get_fn", + [ + m.get_drvd_as_base0_raw_ptr, + m.get_drvd_as_base0_shared_ptr, + m.get_drvd_as_base0_unique_ptr, + ], +) +def test_get_vec_size_raw_shared(get_fn, vec_size_fn): + obj = get_fn() + assert vec_size_fn(obj) == 5 + + +@pytest.mark.parametrize( + "get_fn", [m.get_drvd_as_base0_raw_ptr, m.get_drvd_as_base0_unique_ptr] +) +def test_get_vec_size_unique(get_fn): + obj = get_fn() + assert m.vec_size_base0_unique_ptr(obj) == 5 + with pytest.raises(ValueError) as exc_info: + m.vec_size_base0_unique_ptr(obj) + assert ( + str(exc_info.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) + + +def test_get_shared_vec_size_unique(): + obj = m.get_drvd_as_base0_shared_ptr() + with pytest.raises(ValueError) as exc_info: + m.vec_size_base0_unique_ptr(obj) + assert ( + str(exc_info.value) + == "Cannot disown external shared_ptr (loaded_as_unique_ptr)." + )