diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index cd7e87f845..64e371db4c 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -334,6 +334,7 @@ inline void enable_try_inc_ref(PyObject *obj) { #endif inline bool register_instance_impl(void *ptr, instance *self) { + assert(ptr); #ifdef Py_GIL_DISABLED enable_try_inc_ref(reinterpret_cast(self)); #endif @@ -341,6 +342,7 @@ inline bool register_instance_impl(void *ptr, instance *self) { return true; // unused, but gives the same signature as the deregister func } inline bool deregister_instance_impl(void *ptr, instance *self) { + assert(ptr); return with_instance_map(ptr, [&](instance_map &instances) { auto range = instances.equal_range(ptr); for (auto it = range.first; it != range.second; ++it) { diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 5b65b4a9b2..5d7c31cdaf 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -339,22 +339,42 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *void_ptr = nullptr) { + void *mi_subobject_ptr = nullptr) { smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_std_default_delete = uqp_del_is_std_default_delete(); - guarded_delete gd{nullptr, false}; - if (hld.vptr_is_using_std_default_delete) { - gd = make_guarded_std_default_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)); + + // Build the owning control block on the *real object start* (T*). + guarded_delete gd + = hld.vptr_is_using_std_default_delete + ? make_guarded_std_default_delete(true) + : make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); + // Critical: construct owner with pointer we intend to delete + std::shared_ptr owner(unq_ptr.get(), std::move(gd)); + // Relinquish ownership only after successful construction of owner + (void) unq_ptr.release(); + + // Publish either the MI/VI subobject pointer (if provided) or the full object. + // Why this is needed: + // * The `owner` shared_ptr must always manage the true object start (T*). + // That ensures the deleter is invoked on a valid object header, so the + // virtual destructor can dispatch safely (critical on MSVC with virtual + // inheritance, where base subobjects are not at offset 0). + // * However, pybind11 needs to *register* and expose the subobject pointer + // appropriate for the type being bound. + // This pointer may differ from the T* object start under multiple/virtual + // inheritance. + // This is achieved by using an aliasing shared_ptr: + // - `owner` retains lifetime of the actual T* object start for deletion. + // - `vptr` points at the adjusted subobject (mi_subobject_ptr), giving + // Python the correct identity/registration address. + // If no subobject pointer is passed, we simply publish the full object. + if (mi_subobject_ptr) { + hld.vptr = std::shared_ptr(owner, mi_subobject_ptr); } else { - hld.vptr.reset(unq_ptr.get(), std::move(gd)); + hld.vptr = std::static_pointer_cast(owner); } - (void) unq_ptr.release(); + hld.is_populated = true; return hld; } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 23d9407350..79346cf6a9 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -576,6 +576,8 @@ handle smart_holder_from_unique_ptr(std::unique_ptr &&src, if (!src) { return none().release(); } + // st.first is the subobject pointer appropriate for tinfo (may differ from src.get() + // under MI/VI). Use this for Python identity/registration, but keep ownership on T*. void *src_raw_void_ptr = const_cast(st.first); assert(st.second != nullptr); const detail::type_info *tinfo = st.second; @@ -657,9 +659,10 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr &src, return none().release(); } - auto src_raw_ptr = src.get(); + // st.first is the subobject pointer appropriate for tinfo (may differ from src.get() + // under MI/VI). Use this for Python identity/registration, but keep ownership on T*. + void *src_raw_void_ptr = const_cast(st.first); assert(st.second != nullptr); - 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)) { // PYBIND11:REMINDER: MISSING: Enforcement of consistency with existing smart_holder. @@ -673,8 +676,7 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr &src, void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; - auto smhldr - = smart_holder::from_shared_ptr(std::shared_ptr(src, const_cast(st.first))); + auto smhldr = smart_holder::from_shared_ptr(std::shared_ptr(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/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index d8548ec5c6..0d1672cfb6 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -10,6 +10,8 @@ namespace test_class_sh_mi_thunks { // C++ vtables - Part 2 - Multiple Inheritance // ... the compiler creates a 'thunk' method that corrects `this` ... +// This test was added under PR #4380 + struct Base0 { virtual ~Base0() = default; Base0() = default; @@ -30,6 +32,110 @@ struct Derived : Base1, Base0 { Derived(const Derived &) = delete; }; +// ChatGPT-generated Diamond added under PR #5836 + +struct VBase { + VBase() = default; + VBase(const VBase &) = default; // silence -Wdeprecated-copy-with-dtor + VBase &operator=(const VBase &) = default; + VBase(VBase &&) = default; + VBase &operator=(VBase &&) = default; + virtual ~VBase() = default; + virtual int ping() const { return 1; } + int vbase_tag = 42; // ensure it's not empty +}; + +// Make the virtual bases non-empty and (likely) differently sized. +// The test does *not* require different sizes; we only want to avoid "all at offset 0". +// If a compiler/ABI still places the virtual base at offset 0, our test logs that via +// test_virtual_base_at_offset_0() and continues. +struct Left : virtual VBase { + char pad_l[4]; // small, typically 4 + padding + ~Left() override = default; +}; +struct Right : virtual VBase { + char pad_r[24]; // larger, to differ from Left + ~Right() override = default; +}; + +struct Diamond : Left, Right { + Diamond() = default; + Diamond(const Diamond &) = default; + ~Diamond() override = default; + int ping() const override { return 7; } + int self_tag = 99; +}; + +VBase *make_diamond_as_vbase_raw_ptr() { + auto *ptr = new Diamond; + return ptr; // upcast +} + +std::shared_ptr make_diamond_as_vbase_shared_ptr() { + auto shptr = std::make_shared(); + return shptr; // upcast +} + +std::unique_ptr make_diamond_as_vbase_unique_ptr() { + auto uqptr = std::unique_ptr(new Diamond); + return uqptr; // upcast +} + +// For diagnostics +struct DiamondAddrs { + uintptr_t as_self; + uintptr_t as_vbase; + uintptr_t as_left; + uintptr_t as_right; +}; + +DiamondAddrs diamond_addrs() { + auto sp = std::make_shared(); + return DiamondAddrs{reinterpret_cast(sp.get()), + reinterpret_cast(static_cast(sp.get())), + reinterpret_cast(static_cast(sp.get())), + reinterpret_cast(static_cast(sp.get()))}; +} + +// Animal-Cat-Tiger reproducer copied from PR #5796 +// clone_raw_ptr, clone_unique_ptr added under PR #5836 + +class Animal { +public: + Animal() = default; + Animal(const Animal &) = default; + Animal &operator=(const Animal &) = default; + virtual Animal *clone_raw_ptr() const = 0; + virtual std::shared_ptr clone_shared_ptr() const = 0; + virtual std::unique_ptr clone_unique_ptr() const = 0; + virtual ~Animal() = default; +}; + +class Cat : virtual public Animal { +public: + Cat() = default; + Cat(const Cat &) = default; + Cat &operator=(const Cat &) = default; + ~Cat() override = default; +}; + +class Tiger : virtual public Cat { +public: + Tiger() = default; + Tiger(const Tiger &) = default; + Tiger &operator=(const Tiger &) = default; + ~Tiger() override = default; + Animal *clone_raw_ptr() const override { + return new Tiger(*this); // upcast + } + std::shared_ptr clone_shared_ptr() const override { + return std::make_shared(*this); // upcast + } + std::unique_ptr clone_unique_ptr() const override { + return std::unique_ptr(new Tiger(*this)); // upcast + } +}; + } // namespace test_class_sh_mi_thunks TEST_SUBMODULE(class_sh_mi_thunks, m) { @@ -90,4 +196,35 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { } return obj_der->vec.size(); }); + + py::class_(m, "VBase").def("ping", &VBase::ping); + + py::class_(m, "Left"); + py::class_(m, "Right"); + + py::class_(m, "Diamond", py::multiple_inheritance()) + .def(py::init<>()) + .def("ping", &Diamond::ping); + + m.def("make_diamond_as_vbase_raw_ptr", + &make_diamond_as_vbase_raw_ptr, + py::return_value_policy::take_ownership); + m.def("make_diamond_as_vbase_shared_ptr", &make_diamond_as_vbase_shared_ptr); + m.def("make_diamond_as_vbase_unique_ptr", &make_diamond_as_vbase_unique_ptr); + + py::class_(m, "DiamondAddrs") + .def_readonly("as_self", &DiamondAddrs::as_self) + .def_readonly("as_vbase", &DiamondAddrs::as_vbase) + .def_readonly("as_left", &DiamondAddrs::as_left) + .def_readonly("as_right", &DiamondAddrs::as_right); + + m.def("diamond_addrs", &diamond_addrs); + + py::classh(m, "Animal"); + py::classh(m, "Cat"); + py::classh(m, "Tiger", py::multiple_inheritance()) + .def(py::init<>()) + .def("clone_raw_ptr", &Tiger::clone_raw_ptr) + .def("clone_shared_ptr", &Tiger::clone_shared_ptr) + .def("clone_unique_ptr", &Tiger::clone_unique_ptr); } diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 32bf47554b..7fa164d48f 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -51,3 +51,54 @@ def test_get_shared_vec_size_unique(): assert ( str(exc_info.value) == "Cannot disown external shared_ptr (load_as_unique_ptr)." ) + + +def test_virtual_base_not_at_offset_0(): + # This test ensures that the Diamond fixture actually exercises a non-zero + # virtual-base subobject offset on our supported platforms/ABIs. + # + # If this assert ever fails on some platform/toolchain, please adjust the + # C++ fixture so the virtual base is *not* at offset 0: + # - Keep VBase non-empty. + # - Make Left and Right non-empty and asymmetrically sized and, if + # needed, nudge with a modest alignment. + # - The goal is to achieve a non-zero address delta between `Diamond*` + # and `static_cast(Diamond*)`. + # + # Rationale: certain smart_holder features are exercised only when the + # registered subobject address differs from the most-derived object start, + # so this check guards test efficacy across compilers. + addrs = m.diamond_addrs() + assert addrs.as_vbase - addrs.as_self != 0, ( + "Diamond VBase at offset 0 on this platform; to ensure test efficacy, " + "tweak fixtures (VBase/Left/Right) to ensure non-zero subobject offset." + ) + + +@pytest.mark.parametrize( + "make_fn", + [ + m.make_diamond_as_vbase_raw_ptr, # exercises smart_holder::from_raw_ptr_take_ownership + m.make_diamond_as_vbase_shared_ptr, # exercises smart_holder_from_shared_ptr + m.make_diamond_as_vbase_unique_ptr, # exercises smart_holder_from_unique_ptr + ], +) +def test_make_diamond_as_vbase(make_fn): + # Added under PR #5836 + vb = make_fn() + assert vb.ping() == 7 + + +@pytest.mark.parametrize( + "clone_fn", + [ + m.Tiger.clone_raw_ptr, + m.Tiger.clone_shared_ptr, + m.Tiger.clone_unique_ptr, + ], +) +def test_animal_cat_tiger(clone_fn): + # Based on Animal-Cat-Tiger reproducer under PR #5796 + tiger = m.Tiger() + cloned = clone_fn(tiger) + assert isinstance(cloned, m.Tiger)