Fix smart_holder multiple/virtual inheritance bugs in shared_ptr and unique_ptr to-Python conversions (#5836)

* ChatGPT-generated diamond virtual-inheritance test case.

* Report "virtual base at offset 0" but don't skip test.

* 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
```

* Add assert(ptr) in register_instance_impl, deregister_instance_impl

* Proper bug fix

* Also exercise smart_holder_from_unique_ptr

* [skip ci] ChatGPT-generated bug fix: smart_holder::from_unique_ptr()

* Exception-safe ownership transfer from unique_ptr to shared_ptr

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.

* [skip ci] Rename alias_ptr to mi_subobject_ptr to distinguish from trampoline code (which often uses the term "alias", too)

* [skip ci] Also exercise smart_holder::from_raw_ptr_take_ownership

* [skip ci] Add st.first comments (generated by ChatGPT)

* [skip ci] Copy and extend (raw_ptr, unique_ptr) reproducer from PR #5796

* Some polishing: comments, add back Left/Right dtors for consistency within test_class_sh_mi_thunks.cpp

* explicitly default copy/move for VBase to silence -Wdeprecated-copy-with-dtor

* 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 *
```

* Expand comment in `smart_holder::from_unique_ptr()`

* Better Left/Right padding to make it more likely that we avoid "all at offset 0". Clarify comment.

* Give up on `alignas(16)` to resolve MSVC warning:

```
       "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.

* Rename test_virtual_base_at_offset_0() → test_virtual_base_not_at_offset_0() and replace pytest.skip() with assert. Add helpful comment for future maintainers.

Author: rwgk

include/pybind11/detail/class.h

@@ -334,13 +334,15 @@ 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<PyObject *>(self));
 #endif
     with_instance_map(ptr, [&](instance_map &instances) { instances.emplace(ptr, 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) {

include/pybind11/detail/struct_smart_holder.h

@@ -339,22 +339,42 @@ struct smart_holder {
 
     template <typename T, typename D>
     static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&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<T, D>();
-        guarded_delete gd{nullptr, false};
-        if (hld.vptr_is_using_std_default_delete) {
-            gd = make_guarded_std_default_delete<T>(true);
-        } else {
-            gd = make_guarded_custom_deleter<T, D>(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<T>(true)
+                  : make_guarded_custom_deleter<T, D>(std::move(unq_ptr.get_deleter()), true);
+        // Critical: construct owner with pointer we intend to delete
+        std::shared_ptr<T> 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<void>:
+        //   - `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<void>(owner, mi_subobject_ptr);
         } else {
-            hld.vptr.reset(unq_ptr.get(), std::move(gd));
+            hld.vptr = std::static_pointer_cast<void>(owner);
         }
-        (void) unq_ptr.release();
+
         hld.is_populated = true;
         return hld;
     }

include/pybind11/detail/type_caster_base.h

@@ -576,6 +576,8 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&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<void *>(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<T> &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<void *>(st.first);
     assert(st.second != nullptr);
-    void *src_raw_void_ptr = static_cast<void *>(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<T> &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<void>(src, const_cast<void *>(st.first)));
+    auto smhldr = smart_holder::from_shared_ptr(std::shared_ptr<void>(src, src_raw_void_ptr));
     tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));
 
     if (policy == return_value_policy::reference_internal) {

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<VBase> make_diamond_as_vbase_shared_ptr() {
+    auto shptr = std::make_shared<Diamond>();
+    return shptr; // upcast
+}
+
+std::unique_ptr<VBase> make_diamond_as_vbase_unique_ptr() {
+    auto uqptr = std::unique_ptr<Diamond>(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<Diamond>();
+    return DiamondAddrs{reinterpret_cast<uintptr_t>(sp.get()),
+                        reinterpret_cast<uintptr_t>(static_cast<VBase *>(sp.get())),
+                        reinterpret_cast<uintptr_t>(static_cast<Left *>(sp.get())),
+                        reinterpret_cast<uintptr_t>(static_cast<Right *>(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<Animal> clone_shared_ptr() const = 0;
+    virtual std::unique_ptr<Animal> 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<Animal> clone_shared_ptr() const override {
+        return std::make_shared<Tiger>(*this); // upcast
+    }
+    std::unique_ptr<Animal> clone_unique_ptr() const override {
+        return std::unique_ptr<Tiger>(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_<VBase, py::smart_holder>(m, "VBase").def("ping", &VBase::ping);
+
+    py::class_<Left, VBase, py::smart_holder>(m, "Left");
+    py::class_<Right, VBase, py::smart_holder>(m, "Right");
+
+    py::class_<Diamond, Left, Right, py::smart_holder>(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_<DiamondAddrs, py::smart_holder>(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<Animal>(m, "Animal");
+    py::classh<Cat, Animal>(m, "Cat");
+    py::classh<Tiger, Cat>(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);
 }

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<VBase*>(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)