[smart_holder] test_class_sh_mi_thunks (started from PR #4374) (#4380)

rwgk · web-flow · commit 4766065e5c7c · 2022-12-12T19:55:16.000-08:00
* Content of PR #4374 applied on top of smart_holder branch. * More tests, with USE_SH switch. [ci skip] * Use `std::dynamic_pointer_cast<Base0>` [ci skip] * All tests pass when using `m.make_derived_as_base0_raw_ptr()`, with `USE_SH` defined or not defined. [ci skip] * WIP * Debug LOOOK & one-line bug fix: ```diff - auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src); + auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(std::shared_ptr<void>(src, const_cast<void *>(st.first))); ``` * Remove all print LOOOK and clang-format the fix. * Resolve clang-tidy errors. * Systematic test matrix. * Bug fix in `smart_holder_type_caster<std::unique_ptr<T, D>>::cast()` * Rename: test_mi_debug -> test_class_sh_mi_thunks * Add `test_ptrdiff_derived_base0()` * Miscellaneous polishing (naming, comments). No functional changes. * Improve test_class_sh_mi_thunks.py implementation. No change in test coverage. * Resolve clang-tidy error.
diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h
@@ -301,7 +301,7 @@ struct smart_holder {
 
     template <typename T, typename D>
     static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&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<T>(*hld.rtti_uqp_del);
@@ -311,8 +311,8 @@ struct smart_holder {
         } else {
             gd = make_guarded_custom_deleter<T, D>(true);
         }
-        if (void_cast_raw_ptr) {
-            hld.vptr.reset(static_cast<void *>(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
@@ -386,8 +386,8 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
     template <typename T, typename D>
     static smart_holder smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&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<void *>(unq_ptr.get()) : nullptr;
+        return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), void_ptr);
     }
 
     template <typename T>
@@ -836,7 +836,8 @@ struct smart_holder_type_caster<std::shared_ptr<T>> : 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<void>(src, const_cast<void *>(st.first)));
         tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));
 
         if (policy == return_value_policy::reference_internal) {
@@ -890,17 +891,16 @@ struct smart_holder_type_caster<std::unique_ptr<T, D>> : smart_holder_type_caste
             return none().release();
         }
 
-        auto src_raw_ptr = src.get();
-        auto st = type_caster_base<T>::src_and_type(src_raw_ptr);
+        auto st = type_caster_base<T>::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<void *>(src_raw_ptr);
+        void *src_raw_void_ptr = const_cast<void *>(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<trampoline_self_life_support>(src_raw_ptr);
+                = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(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<std::unique_ptr<T, D>> : 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<void *>(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<const void *>(&smhldr));
 
         if (policy == return_value_policy::reference_internal) {
diff --git 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
@@ -0,0 +1,100 @@
+#include <pybind11/pybind11.h>
+#include <pybind11/smart_holder.h>
+
+#include "pybind11_tests.h"
+
+#include <cstddef>
+#include <memory>
+#include <vector>
+
+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<int> 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<Derived>(new Derived);
+        auto *base0 = dynamic_cast<Base0 *>(drvd.get());
+        return std::ptrdiff_t(reinterpret_cast<char *>(drvd.get())
+                              - reinterpret_cast<char *>(base0));
+    });
+
+    py::classh<Base0>(m, "Base0");
+    py::classh<Base1>(m, "Base1");
+    py::classh<Derived, Base1, Base0>(m, "Derived");
+
+    m.def(
+        "get_drvd_as_base0_raw_ptr",
+        []() {
+            auto *drvd = new Derived;
+            auto *base0 = dynamic_cast<Base0 *>(drvd);
+            return base0;
+        },
+        py::return_value_policy::take_ownership);
+
+    m.def("get_drvd_as_base0_shared_ptr", []() {
+        auto drvd = std::make_shared<Derived>();
+        auto base0 = std::dynamic_pointer_cast<Base0>(drvd);
+        return base0;
+    });
+
+    m.def("get_drvd_as_base0_unique_ptr", []() {
+        auto drvd = std::unique_ptr<Derived>(new Derived);
+        auto base0 = std::unique_ptr<Base0>(std::move(drvd));
+        return base0;
+    });
+
+    m.def("vec_size_base0_raw_ptr", [](const Base0 *obj) {
+        const auto *obj_der = dynamic_cast<const Derived *>(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<Base0> &obj) -> std::size_t {
+        const auto obj_der = std::dynamic_pointer_cast<Derived>(obj);
+        if (!obj_der) {
+            return std::size_t(0);
+        }
+        return obj_der->vec.size();
+    });
+
+    m.def("vec_size_base0_unique_ptr", [](std::unique_ptr<Base0> obj) -> std::size_t {
+        const auto *obj_der = dynamic_cast<const Derived *>(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
@@ -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)."
+    )
