From 262a60940d70edae8c989519cdee70ed3b6262ec Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 13 Sep 2025 23:23:33 -0700 Subject: [PATCH 01/19] ChatGPT-generated diamond virtual-inheritance test case. --- tests/test_class_sh_mi_thunks.cpp | 67 +++++++++++++++++++++++++++++++ tests/test_class_sh_mi_thunks.py | 16 ++++++++ 2 files changed, 83 insertions(+) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index d8548ec5c6..574925b83d 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -30,6 +30,54 @@ struct Derived : Base1, Base0 { Derived(const Derived &) = delete; }; +// ChatGPT-generated Diamond + +struct VBase { + virtual ~VBase() = default; + virtual int ping() const { return 1; } + int vbase_tag = 42; // ensure it's not empty +}; + +// Left/right add some weight to steer layout differences across compilers +struct Left : virtual VBase { + char pad_l[7]; + virtual ~Left() = default; +}; +struct Right : virtual VBase { + long long pad_r; + virtual ~Right() = default; +}; + +struct Diamond : Left, Right { + Diamond() = default; + Diamond(const Diamond &) = default; + ~Diamond() override = default; + int ping() const override { return 7; } + int self_tag = 99; +}; + +// Factory that returns the *virtual base* type; this is the seam +std::shared_ptr make_diamond_as_vbase() { + auto sp = std::make_shared(); + return sp; // upcast to VBase shared_ptr (virtual base) +} + +// For diagnostics / skip decisions in test +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()))}; +} + } // namespace test_class_sh_mi_thunks TEST_SUBMODULE(class_sh_mi_thunks, m) { @@ -90,4 +138,23 @@ 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", &make_diamond_as_vbase); + + 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); } diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 32bf47554b..7111986d13 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -51,3 +51,19 @@ def test_get_shared_vec_size_unique(): assert ( str(exc_info.value) == "Cannot disown external shared_ptr (load_as_unique_ptr)." ) + + +def _delta_nonzero(): + a = m.diamond_addrs() + return (a.as_vbase - a.as_self) != 0 + + +@pytest.mark.skipif( + not _delta_nonzero(), reason="virtual base at offset 0 on this compiler/layout" +) +def test_shared_ptr_return_to_virtual_base_triggers_vi_path(): + # This exercised the broken seam pre-fix. + vb = m.make_diamond_as_vbase() + # If registration used the wrong subobject, this would crash pre-fix on MSVC + # and often on Linux with diamond MI (or under ASan/UBSan). + assert vb.ping() == 7 From 5f77da3d1bd2466e62847eaefce129b5bffcd152 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 13 Sep 2025 23:42:22 -0700 Subject: [PATCH 02/19] Report "virtual base at offset 0" but don't skip test. --- tests/test_class_sh_mi_thunks.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 7111986d13..cd3e4c74b4 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -53,14 +53,12 @@ def test_get_shared_vec_size_unique(): ) -def _delta_nonzero(): - a = m.diamond_addrs() - return (a.as_vbase - a.as_self) != 0 +def test_virtual_base_at_offset_0(): + addrs = m.diamond_addrs() + if addrs.as_vbase - addrs.as_self == 0: + pytest.skip("virtual base at offset 0 on this compiler/layout") -@pytest.mark.skipif( - not _delta_nonzero(), reason="virtual base at offset 0 on this compiler/layout" -) def test_shared_ptr_return_to_virtual_base_triggers_vi_path(): # This exercised the broken seam pre-fix. vb = m.make_diamond_as_vbase() From a50ff8d88317ac05a97bd02895b6362af58810e4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 00:27:11 -0700 Subject: [PATCH 03/19] Remove Left/Right virtual default dtors, to resolve clang-tidy errors: ``` /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:44:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors] 44 | virtual ~Left() = default; | ~~~~~~~ ^ | override /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:48:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors] 48 | virtual ~Right() = default; | ~~~~~~~ ^ | override ``` --- tests/test_class_sh_mi_thunks.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 574925b83d..0c55ae30a6 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -41,11 +41,9 @@ struct VBase { // Left/right add some weight to steer layout differences across compilers struct Left : virtual VBase { char pad_l[7]; - virtual ~Left() = default; }; struct Right : virtual VBase { long long pad_r; - virtual ~Right() = default; }; struct Diamond : Left, Right { From 72c40ab1ccb2c3ca073057d71eac48c098656801 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 13 Sep 2025 14:05:28 -0700 Subject: [PATCH 04/19] Add assert(ptr) in register_instance_impl, deregister_instance_impl --- include/pybind11/detail/class.h | 2 ++ 1 file changed, 2 insertions(+) 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) { From 598d4ec033226582c6b28d584a411769074022ea Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 13 Sep 2025 14:38:19 -0700 Subject: [PATCH 05/19] Proper bug fix --- include/pybind11/detail/type_caster_base.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 23d9407350..a95cb1ab64 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -657,9 +657,8 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr &src, return none().release(); } - auto src_raw_ptr = src.get(); + 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 +672,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) { From 3620ceb02442817063b2aa26ee623d1f65adba50 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 08:16:42 -0700 Subject: [PATCH 06/19] Also exercise smart_holder_from_unique_ptr --- tests/test_class_sh_mi_thunks.cpp | 19 +++++++++++++------ tests/test_class_sh_mi_thunks.py | 15 ++++++++++----- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 0c55ae30a6..f72a6c15bc 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -30,7 +30,7 @@ struct Derived : Base1, Base0 { Derived(const Derived &) = delete; }; -// ChatGPT-generated Diamond +// ChatGPT-generated Diamond. See PR #5836 for background. struct VBase { virtual ~VBase() = default; @@ -54,10 +54,16 @@ struct Diamond : Left, Right { int self_tag = 99; }; -// Factory that returns the *virtual base* type; this is the seam -std::shared_ptr make_diamond_as_vbase() { - auto sp = std::make_shared(); - return sp; // upcast to VBase shared_ptr (virtual base) +// Factory that returns the *virtual base* type (shared_ptr) +std::shared_ptr make_diamond_as_vbase_shared_ptr() { + auto shptr = std::make_shared(); + return shptr; // upcast to VBase shared_ptr (virtual base) +} + +// Factory that returns the *virtual base* type (unique_ptr) +std::unique_ptr make_diamond_as_vbase_unique_ptr() { + auto uqptr = std::unique_ptr(new Diamond); + return uqptr; // upcast to VBase unique_ptr (virtual base) } // For diagnostics / skip decisions in test @@ -146,7 +152,8 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { .def(py::init<>()) .def("ping", &Diamond::ping); - m.def("make_diamond_as_vbase", &make_diamond_as_vbase); + 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) diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index cd3e4c74b4..efb180fe2c 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -59,9 +59,14 @@ def test_virtual_base_at_offset_0(): pytest.skip("virtual base at offset 0 on this compiler/layout") -def test_shared_ptr_return_to_virtual_base_triggers_vi_path(): - # This exercised the broken seam pre-fix. - vb = m.make_diamond_as_vbase() - # If registration used the wrong subobject, this would crash pre-fix on MSVC - # and often on Linux with diamond MI (or under ASan/UBSan). +@pytest.mark.parametrize( + "make_fn", + [ + 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_shared_ptr_return_to_virtual_base_triggers_vi_path(make_fn): + # See PR #5836 for background + vb = make_fn() assert vb.ping() == 7 From b3cc7924814f0331a6a48c0847eeedcf683e331e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 11:01:04 -0700 Subject: [PATCH 07/19] [skip ci] ChatGPT-generated bug fix: smart_holder::from_unique_ptr() --- include/pybind11/detail/struct_smart_holder.h | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 5b65b4a9b2..1633d6d73b 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -339,22 +339,26 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *void_ptr = nullptr) { + void *alias_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); + T *owned_raw = unq_ptr.release(); // pointer we intend to delete + std::shared_ptr owner(owned_raw, std::move(gd)); + + // Publish either the subobject alias (for identity/VI) or the full object. + if (alias_ptr) { + hld.vptr = std::shared_ptr(owner, alias_ptr); // aliasing shared_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; } From ecf0252785e0a4ce2c212875b4a2c6ff1f5eaff2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 11:27:20 -0700 Subject: [PATCH 08/19] Exception-safe ownership transfer from unique_ptr to shared_ptr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT: * shared_ptr’s ctor can throw (control-block alloc). Using get() keeps unique_ptr owning the memory if that happens, so no leak. * Only after the shared_ptr is successfully constructed do you release(), transferring ownership exactly once. --- include/pybind11/detail/struct_smart_holder.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index 1633d6d73b..e776c175e2 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -349,8 +349,10 @@ struct smart_holder { = hld.vptr_is_using_std_default_delete ? make_guarded_std_default_delete(true) : make_guarded_custom_deleter(std::move(unq_ptr.get_deleter()), true); - T *owned_raw = unq_ptr.release(); // pointer we intend to delete - std::shared_ptr owner(owned_raw, std::move(gd)); + // 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 subobject alias (for identity/VI) or the full object. if (alias_ptr) { From 7d047e8d94f829695696369a737a5f89ef85d3ea Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 11:38:21 -0700 Subject: [PATCH 09/19] [skip ci] Rename alias_ptr to mi_subobject_ptr to distinguish from trampoline code (which often uses the term "alias", too) --- include/pybind11/detail/struct_smart_holder.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index e776c175e2..a858ad10ec 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -339,7 +339,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - void *alias_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(); @@ -355,8 +355,8 @@ struct smart_holder { (void) unq_ptr.release(); // Publish either the subobject alias (for identity/VI) or the full object. - if (alias_ptr) { - hld.vptr = std::shared_ptr(owner, alias_ptr); // aliasing shared_ptr + if (mi_subobject_ptr) { + hld.vptr = std::shared_ptr(owner, mi_subobject_ptr); } else { hld.vptr = std::static_pointer_cast(owner); } From 00257be5b6f3d61a7f78e262275f025b4bafa516 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 11:54:11 -0700 Subject: [PATCH 10/19] [skip ci] Also exercise smart_holder::from_raw_ptr_take_ownership --- tests/test_class_sh_mi_thunks.cpp | 13 +++++++++++-- tests/test_class_sh_mi_thunks.py | 3 ++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index f72a6c15bc..6d4c855559 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -54,16 +54,22 @@ struct Diamond : Left, Right { int self_tag = 99; }; +// Factory that returns the *virtual base* type (raw pointer) +VBase *make_diamond_as_vbase_raw_ptr() { + auto ptr = new Diamond; + return ptr; // upcast +} + // Factory that returns the *virtual base* type (shared_ptr) std::shared_ptr make_diamond_as_vbase_shared_ptr() { auto shptr = std::make_shared(); - return shptr; // upcast to VBase shared_ptr (virtual base) + return shptr; // upcast } // Factory that returns the *virtual base* type (unique_ptr) std::unique_ptr make_diamond_as_vbase_unique_ptr() { auto uqptr = std::unique_ptr(new Diamond); - return uqptr; // upcast to VBase unique_ptr (virtual base) + return uqptr; // upcast } // For diagnostics / skip decisions in test @@ -152,6 +158,9 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { .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); diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index efb180fe2c..b34d049670 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -62,11 +62,12 @@ def test_virtual_base_at_offset_0(): @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_shared_ptr_return_to_virtual_base_triggers_vi_path(make_fn): +def test_make_diamond_as_vbase(make_fn): # See PR #5836 for background vb = make_fn() assert vb.ping() == 7 From 8f87bc921030abd68819445dc7252a90f20669b4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 13:59:02 -0700 Subject: [PATCH 11/19] [skip ci] Add st.first comments (generated by ChatGPT) --- include/pybind11/detail/type_caster_base.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a95cb1ab64..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,6 +659,8 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr &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; From 4efd7e71e7015ebddc73da2512d4d46a37c97fbb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 14:25:11 -0700 Subject: [PATCH 12/19] [skip ci] Copy and extend (raw_ptr, unique_ptr) reproducer from PR #5796 --- tests/test_class_sh_mi_thunks.cpp | 47 +++++++++++++++++++++++++++++++ tests/test_class_sh_mi_thunks.py | 14 +++++++++ 2 files changed, 61 insertions(+) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 6d4c855559..7dd257e1de 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -88,6 +88,45 @@ DiamondAddrs diamond_addrs() { reinterpret_cast(static_cast(sp.get()))}; } +// Animal-Cat-Tiger reproducer copied from PR #5796. +// clone_raw_ptr, clone_unique_ptr added in 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) { @@ -171,4 +210,12 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { .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 b34d049670..338adad45b 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -71,3 +71,17 @@ def test_make_diamond_as_vbase(make_fn): # See PR #5836 for background 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): + tiger = m.Tiger() + cloned = clone_fn(tiger) + assert isinstance(cloned, m.Tiger) From 77be20f05aa379f1ad8e5c0716425010dbab587e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 14:57:09 -0700 Subject: [PATCH 13/19] Some polishing: comments, add back Left/Right dtors for consistency within test_class_sh_mi_thunks.cpp --- tests/test_class_sh_mi_thunks.cpp | 15 ++++++++------- tests/test_class_sh_mi_thunks.py | 4 +++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 7dd257e1de..2979df7ac5 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,7 +32,7 @@ struct Derived : Base1, Base0 { Derived(const Derived &) = delete; }; -// ChatGPT-generated Diamond. See PR #5836 for background. +// ChatGPT-generated Diamond added under PR #5836 struct VBase { virtual ~VBase() = default; @@ -41,9 +43,11 @@ struct VBase { // Left/right add some weight to steer layout differences across compilers struct Left : virtual VBase { char pad_l[7]; + ~Left() override = default; }; struct Right : virtual VBase { long long pad_r; + ~Right() override = default; }; struct Diamond : Left, Right { @@ -54,25 +58,22 @@ struct Diamond : Left, Right { int self_tag = 99; }; -// Factory that returns the *virtual base* type (raw pointer) VBase *make_diamond_as_vbase_raw_ptr() { auto ptr = new Diamond; return ptr; // upcast } -// Factory that returns the *virtual base* type (shared_ptr) std::shared_ptr make_diamond_as_vbase_shared_ptr() { auto shptr = std::make_shared(); return shptr; // upcast } -// Factory that returns the *virtual base* type (unique_ptr) std::unique_ptr make_diamond_as_vbase_unique_ptr() { auto uqptr = std::unique_ptr(new Diamond); return uqptr; // upcast } -// For diagnostics / skip decisions in test +// For diagnostics struct DiamondAddrs { uintptr_t as_self; uintptr_t as_vbase; @@ -88,8 +89,8 @@ DiamondAddrs diamond_addrs() { reinterpret_cast(static_cast(sp.get()))}; } -// Animal-Cat-Tiger reproducer copied from PR #5796. -// clone_raw_ptr, clone_unique_ptr added in PR #5836. +// Animal-Cat-Tiger reproducer copied from PR #5796 +// clone_raw_ptr, clone_unique_ptr added under PR #5836 class Animal { public: diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 338adad45b..a3341829eb 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -56,6 +56,7 @@ def test_get_shared_vec_size_unique(): def test_virtual_base_at_offset_0(): addrs = m.diamond_addrs() if addrs.as_vbase - addrs.as_self == 0: + # Not an actual skip, just a trick generate a message in the pytest summary pytest.skip("virtual base at offset 0 on this compiler/layout") @@ -68,7 +69,7 @@ def test_virtual_base_at_offset_0(): ], ) def test_make_diamond_as_vbase(make_fn): - # See PR #5836 for background + # Added under PR #5836 vb = make_fn() assert vb.ping() == 7 @@ -82,6 +83,7 @@ def test_make_diamond_as_vbase(make_fn): ], ) 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) From cd9458bfd75d048fc0dc8a91de2deefcaa68eb47 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 15:09:04 -0700 Subject: [PATCH 14/19] explicitly default copy/move for VBase to silence -Wdeprecated-copy-with-dtor --- tests/test_class_sh_mi_thunks.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 2979df7ac5..d82681e757 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -35,6 +35,11 @@ struct Derived : Base1, Base0 { // 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 From 5e86d879da47f8dc75b8ec1cbe90b63d1cb595df Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 14 Sep 2025 17:48:25 -0700 Subject: [PATCH 15/19] Resolve clang-tidy error: ``` /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:67:5: error: 'auto ptr' can be declared as 'auto *ptr' [readability-qualified-auto,-warnings-as-errors] 67 | auto ptr = new Diamond; | ^~~~ | auto * ``` --- tests/test_class_sh_mi_thunks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index d82681e757..29e2966d7f 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -64,7 +64,7 @@ struct Diamond : Left, Right { }; VBase *make_diamond_as_vbase_raw_ptr() { - auto ptr = new Diamond; + auto *ptr = new Diamond; return ptr; // upcast } From 31886ecfdccf88f7116f10f96b5bd32674ff11ea Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 28 Sep 2025 14:04:04 -0700 Subject: [PATCH 16/19] Expand comment in `smart_holder::from_unique_ptr()` --- include/pybind11/detail/struct_smart_holder.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index a858ad10ec..5d7c31cdaf 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -354,7 +354,21 @@ struct smart_holder { // Relinquish ownership only after successful construction of owner (void) unq_ptr.release(); - // Publish either the subobject alias (for identity/VI) or the full object. + // 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 { From ba89e1dd65e5b21fe098e08db1e7cecf7a0a7264 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 28 Sep 2025 14:34:15 -0700 Subject: [PATCH 17/19] Better Left/Right padding to make it more likely that we avoid "all at offset 0". Clarify comment. --- tests/test_class_sh_mi_thunks.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 29e2966d7f..8d7262a9ab 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -45,13 +45,16 @@ struct VBase { int vbase_tag = 42; // ensure it's not empty }; -// Left/right add some weight to steer layout differences across compilers +// Make the virtual bases non-empty and (likely) differently sized/aligned. +// 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[7]; + char pad_l[4]; // small, typically 4 + padding ~Left() override = default; }; struct Right : virtual VBase { - long long pad_r; + alignas(16) char pad_r[16]; // larger + alignment nudge to differ from Left ~Right() override = default; }; From 0df90e66cc2e8f79a462a7a7efa21f9ba539046c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 28 Sep 2025 15:30:57 -0700 Subject: [PATCH 18/19] Give up on `alignas(16)` to resolve MSVC warning: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` "D:\a\pybind11\pybind11\build\ALL_BUILD.vcxproj" (default target) (1) -> "D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj" (default target) (13) -> (ClCompile target) -> D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(70,17): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(80,43): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: 'std::_Ref_count_obj2<_Ty>': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: with [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: [ [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: _Ty=test_class_sh_mi_thunks::Diamond [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: ] [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\include\pybind11\detail\init.h(77,21): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] ``` The warning came from alignas(16) making Diamond over-aligned, while regular new/make_shared aren’t guaranteed to return 16-byte aligned memory on MSVC (hence C4316). I’ve removed the explicit alignment and switched to asymmetric payload sizes (char[4] vs char[24]), which still nudges MI layout without relying on over-alignment. This keeps the test goal and eliminates the warning across all MSVC builds. If we ever want to stress over-alignment explicitly, we can add aligned operator new/delete under __cpp_aligned_new, but that’s more than we need here. --- tests/test_class_sh_mi_thunks.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 8d7262a9ab..0d1672cfb6 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -45,7 +45,7 @@ struct VBase { int vbase_tag = 42; // ensure it's not empty }; -// Make the virtual bases non-empty and (likely) differently sized/aligned. +// 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. @@ -54,7 +54,7 @@ struct Left : virtual VBase { ~Left() override = default; }; struct Right : virtual VBase { - alignas(16) char pad_r[16]; // larger + alignment nudge to differ from Left + char pad_r[24]; // larger, to differ from Left ~Right() override = default; }; From 9dbd9f153c35599e1472d99b2a610fd1292862ab Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 28 Sep 2025 22:05:43 -0700 Subject: [PATCH 19/19] =?UTF-8?q?Rename=20test=5Fvirtual=5Fbase=5Fat=5Foff?= =?UTF-8?q?set=5F0()=20=E2=86=92=20test=5Fvirtual=5Fbase=5Fnot=5Fat=5Foffs?= =?UTF-8?q?et=5F0()=20and=20replace=20pytest.skip()=20with=20assert.=20Add?= =?UTF-8?q?=20helpful=20comment=20for=20future=20maintainers.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/test_class_sh_mi_thunks.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index a3341829eb..7fa164d48f 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -53,11 +53,26 @@ def test_get_shared_vec_size_unique(): ) -def test_virtual_base_at_offset_0(): +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() - if addrs.as_vbase - addrs.as_self == 0: - # Not an actual skip, just a trick generate a message in the pytest summary - pytest.skip("virtual base at offset 0 on this compiler/layout") + 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(