smart_holder trampoline shared_from_this support

include/pybind11/detail/init.h

@@ -182,8 +182,13 @@ void construct(value_and_holder &v_h, std::unique_ptr<Cpp<Class>, D> &&unq_ptr,
     if (Class::has_alias && need_alias && !is_alias<Class>(ptr))
         throw type_error("pybind11::init(): construction failed: returned std::unique_ptr pointee "
                          "is not an alias instance");
-    auto smhldr
-        = type_caster<Cpp<Class>>::template smart_holder_from_unique_ptr(std::move(unq_ptr));
+    // Here and below: if the new object is a trampoline, the shared_from_this mechanism needs
+    // to be prevented from accessing the smart_holder vptr, because it does not keep the
+    // trampoline Python object alive. For types that don't inherit from enable_shared_from_this
+    // it does not matter if void_cast_raw_ptr is true or false, therefore it's not necessary
+    // to also inspect the type.
+    auto smhldr = type_caster<Cpp<Class>>::template smart_holder_from_unique_ptr(
+        std::move(unq_ptr), /*void_cast_raw_ptr*/ Class::has_alias && is_alias<Class>(ptr));
     v_h.value_ptr() = ptr;
     v_h.type->init_instance(v_h.inst, &smhldr);
 }
@@ -197,8 +202,8 @@ void construct(value_and_holder &v_h,
                bool /*need_alias*/) {
     auto *ptr = unq_ptr.get();
     no_nullptr(ptr);
-    auto smhldr
-        = type_caster<Alias<Class>>::template smart_holder_from_unique_ptr(std::move(unq_ptr));
+    auto smhldr = type_caster<Alias<Class>>::template smart_holder_from_unique_ptr(
+        std::move(unq_ptr), /*void_cast_raw_ptr*/ true);
     v_h.value_ptr() = ptr;
     v_h.type->init_instance(v_h.inst, &smhldr);
 }

include/pybind11/detail/smart_holder_poc.h

@@ -40,6 +40,10 @@ High-level aspects:
 
 * By choice, the smart_holder is movable but not copyable, to keep the design
   simple, and to guard against accidental copying overhead.
+
+* The `void_cast_raw_ptr` option is needed to make the `smart_holder` `vptr`
+  member invisible to the `shared_from_this` mechanism, in case the lifetime
+  of a `PyObject` is tied to the pointee.
 */
 
 #pragma once
@@ -56,7 +60,15 @@ High-level aspects:
 namespace pybindit {
 namespace memory {
 
+static constexpr bool type_has_shared_from_this(...) { return false; }
+
+template <typename T>
+static constexpr bool type_has_shared_from_this(const std::enable_shared_from_this<T> *) {
+    return true;
+}
+
 struct guarded_delete {
+    std::weak_ptr<void> released_ptr; // Trick to keep the smart_holder memory footprint small.
     void (*del_ptr)(void *);
     bool armed_flag;
     guarded_delete(void (*del_ptr)(void *), bool armed_flag)
@@ -69,7 +81,7 @@ struct guarded_delete {
 
 template <typename T, typename std::enable_if<std::is_destructible<T>::value, int>::type = 0>
 inline void builtin_delete_if_destructible(void *raw_ptr) {
-    delete (T *) raw_ptr;
+    delete static_cast<T *>(raw_ptr);
 }
 
 template <typename T, typename std::enable_if<!std::is_destructible<T>::value, int>::type = 0>
@@ -87,7 +99,7 @@ guarded_delete make_guarded_builtin_delete(bool armed_flag) {
 
 template <typename T, typename D>
 inline void custom_delete(void *raw_ptr) {
-    D()((T *) raw_ptr);
+    D()(static_cast<T *>(raw_ptr));
 }
 
 template <typename T, typename D>
@@ -203,8 +215,7 @@ struct smart_holder {
         vptr_del_ptr->armed_flag = armed_flag;
     }
 
-    template <typename T>
-    static smart_holder from_raw_ptr_unowned(T *raw_ptr) {
+    static smart_holder from_raw_ptr_unowned(void *raw_ptr) {
         smart_holder hld;
         hld.vptr.reset(raw_ptr, [](void *) {});
         hld.vptr_is_using_noop_deleter = true;
@@ -234,10 +245,14 @@ struct smart_holder {
     }
 
     template <typename T>
-    static smart_holder from_raw_ptr_take_ownership(T *raw_ptr) {
+    static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) {
         ensure_pointee_is_destructible<T>("from_raw_ptr_take_ownership");
         smart_holder hld;
-        hld.vptr.reset(raw_ptr, make_guarded_builtin_delete<T>(true));
+        auto gd = make_guarded_builtin_delete<T>(true);
+        if (void_cast_raw_ptr)
+            hld.vptr.reset(static_cast<void *>(raw_ptr), std::move(gd));
+        else
+            hld.vptr.reset(raw_ptr, std::move(gd));
         hld.vptr_is_using_builtin_delete = true;
         hld.is_populated                 = true;
         return hld;
@@ -280,15 +295,20 @@ struct smart_holder {
     }
 
     template <typename T, typename D>
-    static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr) {
+    static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr,
+                                        bool void_cast_raw_ptr = false) {
         smart_holder hld;
         hld.rtti_uqp_del                 = &typeid(D);
         hld.vptr_is_using_builtin_delete = is_std_default_delete<T>(*hld.rtti_uqp_del);
-        if (hld.vptr_is_using_builtin_delete) {
-            hld.vptr.reset(unq_ptr.get(), make_guarded_builtin_delete<T>(true));
-        } else {
-            hld.vptr.reset(unq_ptr.get(), make_guarded_custom_deleter<T, D>(true));
-        }
+        guarded_delete gd{nullptr, false};
+        if (hld.vptr_is_using_builtin_delete)
+            gd = make_guarded_builtin_delete<T>(true);
+        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));
+        else
+            hld.vptr.reset(unq_ptr.get(), std::move(gd));
         unq_ptr.release();
         hld.is_populated = true;
         return hld;

include/pybind11/detail/smart_holder_type_casters.h

@@ -270,6 +270,32 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
         return &modified_type_caster_generic_load_impl::local_load;
     }
 
+    using holder_type = pybindit::memory::smart_holder;
+
+    template <typename WrappedType>
+    static bool try_initialization_using_shared_from_this(holder_type *, WrappedType *, ...) {
+        return false;
+    }
+
+    // Adopting existing approach used by type_caster_base, although it leads to somewhat fuzzy
+    // ownership semantics: if we deteced via shared_from_this that a shared_ptr exists already, it
+    // is reused, irrespective of the return_value_policy in effect.
+    // "SomeBaseOfWrappedType" is needed because std::enable_shared_from_this is not necessarily a
+    // direct base of WrappedType.
+    template <typename WrappedType, typename SomeBaseOfWrappedType>
+    static bool try_initialization_using_shared_from_this(
+        holder_type *uninitialized_location,
+        WrappedType *value_ptr_w_t,
+        const std::enable_shared_from_this<SomeBaseOfWrappedType> *) {
+        auto shd_ptr = std::dynamic_pointer_cast<WrappedType>(
+            detail::try_get_shared_from_this(value_ptr_w_t));
+        if (!shd_ptr)
+            return false;
+        // Note: inst->owned ignored.
+        new (uninitialized_location) holder_type(holder_type::from_shared_ptr(shd_ptr));
+        return true;
+    }
+
     template <typename WrappedType, typename AliasType>
     static void init_instance_for_type(detail::instance *inst, const void *holder_const_void_ptr) {
         // Need for const_cast is a consequence of the type_info::init_instance type:
@@ -281,26 +307,34 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
             register_instance(inst, v_h.value_ptr(), v_h.type);
             v_h.set_instance_registered();
         }
-        using holder_type = pybindit::memory::smart_holder;
+        auto uninitialized_location = std::addressof(v_h.holder<holder_type>());
+        auto value_ptr_w_t          = v_h.value_ptr<WrappedType>();
+        bool pointee_depends_on_holder_owner
+            = dynamic_raw_ptr_cast_if_possible<AliasType>(value_ptr_w_t) != nullptr;
         if (holder_void_ptr) {
             // Note: inst->owned ignored.
             auto holder_ptr = static_cast<holder_type *>(holder_void_ptr);
-            new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(*holder_ptr));
-        } else if (inst->owned) {
-            new (std::addressof(v_h.holder<holder_type>())) holder_type(
-                holder_type::from_raw_ptr_take_ownership(v_h.value_ptr<WrappedType>()));
-        } else {
-            new (std::addressof(v_h.holder<holder_type>()))
-                holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr<WrappedType>()));
+            new (uninitialized_location) holder_type(std::move(*holder_ptr));
+        } else if (!try_initialization_using_shared_from_this(
+                       uninitialized_location, value_ptr_w_t, value_ptr_w_t)) {
+            if (inst->owned) {
+                new (uninitialized_location) holder_type(holder_type::from_raw_ptr_take_ownership(
+                    value_ptr_w_t, /*void_cast_raw_ptr*/ pointee_depends_on_holder_owner));
+            } else {
+                new (uninitialized_location)
+                    holder_type(holder_type::from_raw_ptr_unowned(value_ptr_w_t));
+            }
         }
         v_h.holder<holder_type>().pointee_depends_on_holder_owner
-            = dynamic_raw_ptr_cast_if_possible<AliasType>(v_h.value_ptr<WrappedType>()) != nullptr;
+            = pointee_depends_on_holder_owner;
         v_h.set_holder_constructed();
     }
 
     template <typename T, typename D>
-    static smart_holder smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr) {
-        return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr));
+    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);
     }
 
     template <typename T>
@@ -309,6 +343,18 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
     }
 };
 
+struct shared_ptr_trampoline_self_life_support {
+    PyObject *self;
+    explicit shared_ptr_trampoline_self_life_support(instance *inst)
+        : self{reinterpret_cast<PyObject *>(inst)} {
+        Py_INCREF(self);
+    }
+    void operator()(void *) {
+        gil_scoped_acquire gil;
+        Py_DECREF(self);
+    }
+};
+
 template <typename T>
 struct smart_holder_type_caster_load {
     using holder_type = pybindit::memory::smart_holder;
@@ -342,34 +388,56 @@ struct smart_holder_type_caster_load {
         return *raw_ptr;
     }
 
-    struct shared_ptr_dec_ref_deleter {
-        // Note: deleter destructor fails on MSVC 2015 and GCC 4.8, so we manually call dec_ref
-        // here instead.
-        handle ref;
-        void operator()(void *) {
-            gil_scoped_acquire gil;
-            ref.dec_ref();
-        }
-    };
-
     std::shared_ptr<T> loaded_as_shared_ptr() const {
         if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr)
             throw cast_error("Unowned pointer from direct conversion cannot be converted to a"
                              " std::shared_ptr.");
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
-        holder().ensure_is_not_disowned("loaded_as_shared_ptr");
-        auto void_raw_ptr = holder().template as_raw_ptr_unowned<void>();
+        holder_type &hld = holder();
+        hld.ensure_is_not_disowned("loaded_as_shared_ptr");
+        if (hld.vptr_is_using_noop_deleter) {
+            throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr).");
+        }
+        auto void_raw_ptr = hld.template as_raw_ptr_unowned<void>();
         auto type_raw_ptr = convert_type(void_raw_ptr);
-        if (holder().pointee_depends_on_holder_owner) {
-            // Tie lifetime of trampoline Python part to C++ part (PR #2839).
-            return std::shared_ptr<T>(
-                type_raw_ptr,
-                shared_ptr_dec_ref_deleter{
-                    handle((PyObject *) load_impl.loaded_v_h.inst).inc_ref()});
+        if (hld.pointee_depends_on_holder_owner) {
+            auto vptr_gd_ptr = std::get_deleter<pybindit::memory::guarded_delete>(hld.vptr);
+            if (vptr_gd_ptr != nullptr) {
+                std::shared_ptr<void> released_ptr = vptr_gd_ptr->released_ptr.lock();
+                if (released_ptr)
+                    return std::shared_ptr<T>(released_ptr, type_raw_ptr);
+                std::shared_ptr<T> to_be_released(
+                    type_raw_ptr,
+                    shared_ptr_trampoline_self_life_support(load_impl.loaded_v_h.inst));
+                vptr_gd_ptr->released_ptr = to_be_released;
+                return to_be_released;
+            }
+            auto sptsls_ptr = std::get_deleter<shared_ptr_trampoline_self_life_support>(hld.vptr);
+            if (sptsls_ptr != nullptr) {
+                // This code is reachable only if there are multiple registered_instances for the
+                // same pointee.
+                if (reinterpret_cast<PyObject *>(load_impl.loaded_v_h.inst) == sptsls_ptr->self) {
+                    pybind11_fail("smart_holder_type_casters loaded_as_shared_ptr failure: "
+                                  "load_impl.loaded_v_h.inst == sptsls_ptr->self");
+                }
+            }
+            if (sptsls_ptr != nullptr
+                || !pybindit::memory::type_has_shared_from_this(type_raw_ptr)) {
+                return std::shared_ptr<T>(
+                    type_raw_ptr,
+                    shared_ptr_trampoline_self_life_support(load_impl.loaded_v_h.inst));
+            }
+            if (hld.vptr_is_external_shared_ptr) {
+                pybind11_fail("smart_holder_type_casters loaded_as_shared_ptr failure: not "
+                              "implemented: trampoline-self-life-support for external shared_ptr "
+                              "to type inheriting from std::enable_shared_from_this.");
+            }
+            pybind11_fail("smart_holder_type_casters: loaded_as_shared_ptr failure: internal "
+                          "inconsistency.");
         }
-        std::shared_ptr<void> void_shd_ptr = holder().template as_shared_ptr<void>();
+        std::shared_ptr<void> void_shd_ptr = hld.template as_shared_ptr<void>();
         return std::shared_ptr<T>(void_shd_ptr, type_raw_ptr);
     }
 
@@ -381,6 +449,7 @@ struct smart_holder_type_caster_load {
         if (!have_holder())
             return nullptr;
         throw_if_uninitialized_or_disowned_holder();
+        throw_if_instance_is_currently_owned_by_shared_ptr();
         holder().ensure_is_not_disowned(context);
         holder().template ensure_compatible_rtti_uqp_del<T, D>(context);
         holder().ensure_use_count_1(context);
@@ -444,6 +513,14 @@ struct smart_holder_type_caster_load {
         }
     }
 
+    // have_holder() must be true or this function will fail.
+    void throw_if_instance_is_currently_owned_by_shared_ptr() const {
+        auto vptr_gd_ptr = std::get_deleter<pybindit::memory::guarded_delete>(holder().vptr);
+        if (vptr_gd_ptr != nullptr && !vptr_gd_ptr->released_ptr.expired()) {
+            throw value_error("Python instance is currently owned by a std::shared_ptr.");
+        }
+    }
+
     T *convert_type(void *void_ptr) const {
         if (void_ptr != nullptr && load_impl.loaded_v_h_cpptype != nullptr
             && !load_impl.reinterpret_cast_deemed_ok && load_impl.implicit_cast != nullptr) {
@@ -748,7 +825,8 @@ 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));
+        auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src),
+                                                                      /*void_cast_raw_ptr*/ false);
         tinfo->init_instance(inst_raw_ptr, static_cast<const void *>(&smhldr));
 
         if (policy == return_value_policy::reference_internal)

tests/CMakeLists.txt

@@ -109,6 +109,7 @@ set(PYBIND11_TEST_FILES
     test_class_sh_shared_ptr_copy_move.cpp
     test_class_sh_trampoline_basic.cpp
     test_class_sh_trampoline_self_life_support.cpp
+    test_class_sh_trampoline_shared_from_this.cpp
     test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
     test_class_sh_trampoline_unique_ptr.cpp
     test_class_sh_unique_ptr_member.cpp