First fully successful attempt to make shared_from_this and trampolines play nicely.

Now also passes the open_spiel iterated_prisoners_dilemma_test ASAN clean, in addition to all pybind11 and PyCLIF unit tests.

The problem was that calling `std::shared_ptr<void>::reset()` with a `void` pointer cannot possibly update the `shared_from_this` `weak_ptr`.

The solution is to store a `shd_ptr_reset` function pointer in `guarded_deleter` (similar in idea to the stored function pointer for calling `delete`).

This commit still includes all debugging code, i.e. is "dirty". The code will be cleaned up after the GitHub CI is fully successful.

include/pybind11/detail/smart_holder_poc.h

@@ -51,28 +51,47 @@ Details:
 #include <type_traits>
 #include <typeinfo>
 
+//#include <iostream>
+inline void to_cout(std::string /*msg*/) { /*std::cout << msg << std::endl;*/ }
+
 // pybindit = Python Bindings Innovation Track.
 // Currently not in pybind11 namespace to signal that this POC does not depend
 // on any existing pybind11 functionality.
 namespace pybindit {
 namespace memory {
 
+inline int shared_from_this_status(...) { return 0; }
+
+template <typename AnyBaseOfT>
+inline int shared_from_this_status(const std::enable_shared_from_this<AnyBaseOfT> *ptr) {
+  if (ptr->weak_from_this().lock()) return 1;
+  return -1;
+}
+
 struct smart_holder;
 
 struct guarded_delete {
     smart_holder *hld            = nullptr;
     void (*callback_ptr)(void *) = nullptr;
     void *callback_arg           = nullptr;
-    void (*del_ptr)(void *);
+    void (*shd_ptr_reset_fptr)(std::shared_ptr<void>&, void *, guarded_delete &&);
+    void (*del_fptr)(void *);
     bool armed_flag;
-    guarded_delete(void (*del_ptr)(void *), bool armed_flag)
-        : del_ptr{del_ptr}, armed_flag{armed_flag} {}
+    guarded_delete(void (*shd_ptr_reset_fptr)(std::shared_ptr<void>&, void *, guarded_delete &&), void (*del_fptr)(void *), bool armed_flag)
+        : shd_ptr_reset_fptr{shd_ptr_reset_fptr}, del_fptr{del_fptr}, armed_flag{armed_flag} {
+to_cout("LOOOK guarded_delete ctor " + std::to_string(__LINE__) + " " + __FILE__);
+        }
     void operator()(void *raw_ptr) const;
 };
 
+template <typename T>
+inline void shd_ptr_reset(std::shared_ptr<void>& shd_ptr, void *raw_ptr, guarded_delete &&gdel) {
+    shd_ptr.reset(reinterpret_cast<T *>(raw_ptr), gdel);
+}
+
 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 reinterpret_cast<T *>(raw_ptr);
 }
 
 template <typename T, typename std::enable_if<!std::is_destructible<T>::value, int>::type = 0>
@@ -85,17 +104,17 @@ inline void builtin_delete_if_destructible(void *) {
 
 template <typename T>
 guarded_delete make_guarded_builtin_delete(bool armed_flag) {
-    return guarded_delete(builtin_delete_if_destructible<T>, armed_flag);
+    return guarded_delete(shd_ptr_reset<T>, builtin_delete_if_destructible<T>, armed_flag);
 }
 
 template <typename T, typename D>
 inline void custom_delete(void *raw_ptr) {
-    D()((T *) raw_ptr);
+    D()(reinterpret_cast<T *>(raw_ptr));
 }
 
 template <typename T, typename D>
 guarded_delete make_guarded_custom_deleter(bool armed_flag) {
-    return guarded_delete(custom_delete<T, D>, armed_flag);
+    return guarded_delete(shd_ptr_reset<T>, custom_delete<T, D>, armed_flag);
 };
 
 template <typename T>
@@ -199,16 +218,18 @@ struct smart_holder {
     }
 
     void reset_vptr_deleter_armed_flag(bool armed_flag) const {
-        auto vptr_del_ptr = std::get_deleter<guarded_delete>(vptr);
-        if (vptr_del_ptr == nullptr) {
+to_cout("LOOOK smart_holder reset_vptr_deleter_armed_flag " + std::to_string(__LINE__) + " " + __FILE__);
+        auto vptr_del_fptr = std::get_deleter<guarded_delete>(vptr);
+        if (vptr_del_fptr == nullptr) {
             throw std::runtime_error(
                 "smart_holder::reset_vptr_deleter_armed_flag() called in an invalid context.");
         }
-        vptr_del_ptr->armed_flag = armed_flag;
+        vptr_del_fptr->armed_flag = armed_flag;
     }
 
     template <typename T>
     static smart_holder from_raw_ptr_unowned(T *raw_ptr) {
+to_cout("LOOOK smart_holder from_raw_ptr_unowned " + std::to_string(__LINE__) + " " + __FILE__);
         smart_holder hld;
         hld.vptr.reset(raw_ptr, [](void *) {});
         hld.vptr_is_using_noop_deleter = true;
@@ -218,6 +239,7 @@ struct smart_holder {
 
     template <typename T>
     T *as_raw_ptr_unowned() const {
+to_cout("LOOOK smart_holder as_raw_ptr_unowned PTR" + std::to_string((unsigned long) vptr.get()) + " " + std::to_string(__LINE__) + " " + __FILE__);
         return static_cast<T *>(vptr.get());
     }
 
@@ -239,6 +261,8 @@ struct smart_holder {
 
     template <typename T>
     static smart_holder from_raw_ptr_take_ownership(T *raw_ptr) {
+to_cout("");
+to_cout("LOOOK smart_holder from_raw_ptr_take_ownership " + std::to_string(__LINE__) + " " + __FILE__);
         ensure_pointee_is_destructible<T>("from_raw_ptr_take_ownership");
         smart_holder hld;
         hld.vptr.reset(raw_ptr, make_guarded_builtin_delete<T>(true));
@@ -277,6 +301,7 @@ struct smart_holder {
 
     template <typename T>
     T *as_raw_ptr_release_ownership(const char *context = "as_raw_ptr_release_ownership") {
+to_cout("LOOOK smart_holder as_raw_ptr_release_ownership " + std::to_string(__LINE__) + " " + __FILE__);
         ensure_can_release_ownership(context);
         T *raw_ptr = as_raw_ptr_unowned<T>();
         release_ownership();
@@ -285,6 +310,7 @@ struct smart_holder {
 
     template <typename T, typename D>
     static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr) {
+to_cout("LOOOK smart_holder from_unique_ptr " + std::to_string(__LINE__) + " " + __FILE__);
         smart_holder hld;
         hld.rtti_uqp_del                 = &typeid(D);
         hld.vptr_is_using_builtin_delete = is_std_default_delete<T>(*hld.rtti_uqp_del);
@@ -300,6 +326,7 @@ struct smart_holder {
 
     template <typename T, typename D = std::default_delete<T>>
     std::unique_ptr<T, D> as_unique_ptr() {
+to_cout("LOOOK smart_holder as_unique_ptr " + std::to_string(__LINE__) + " " + __FILE__);
         static const char *context = "as_unique_ptr";
         ensure_compatible_rtti_uqp_del<T, D>(context);
         ensure_use_count_1(context);
@@ -310,6 +337,7 @@ struct smart_holder {
 
     template <typename T>
     static smart_holder from_shared_ptr(std::shared_ptr<T> shd_ptr) {
+to_cout("LOOOK smart_holder from_shared_ptr " + std::to_string(__LINE__) + " " + __FILE__);
         smart_holder hld;
         hld.vptr                        = std::static_pointer_cast<void>(shd_ptr);
         hld.vptr_is_external_shared_ptr = true;
@@ -319,18 +347,25 @@ struct smart_holder {
 
     template <typename T>
     std::shared_ptr<T> as_shared_ptr() const {
+to_cout("LOOOK smart_holder as_shared_ptr " + std::to_string(__LINE__) + " " + __FILE__);
         return std::static_pointer_cast<T>(vptr);
     }
 };
 
 inline void guarded_delete::operator()(void *raw_ptr) const {
     if (hld) {
+to_cout("LOOOK guarded_delete call hld " + std::to_string(__LINE__) + " " + __FILE__);
         assert(armed_flag);
-        hld->vptr.reset(hld->vptr.get(), guarded_delete{del_ptr, true});
+        assert(hld->vptr.get() == raw_ptr);
+        assert(hld->vptr_is_released);
+        (*shd_ptr_reset_fptr)(hld->vptr, hld->vptr.get(), guarded_delete{shd_ptr_reset_fptr, del_fptr, true});
         hld->vptr_is_released = false;
         (*callback_ptr)(callback_arg); // Py_DECREF.
     } else if (armed_flag) {
-        (*del_ptr)(raw_ptr);
+to_cout("LOOOK guarded_delete call del " + std::to_string(__LINE__) + " " + __FILE__);
+        (*del_fptr)(raw_ptr);
+    } else {
+to_cout("LOOOK guarded_delete call noop " + std::to_string(__LINE__) + " " + __FILE__);
     }
 }
 

include/pybind11/detail/smart_holder_type_casters.h

@@ -371,6 +371,7 @@ struct smart_holder_type_caster_load {
     }
 
     static void dec_ref_void(void *ptr) {
+to_cout("LOOOK dec_ref_void Py_REFCNT(" + std::to_string(Py_REFCNT(reinterpret_cast<PyObject *>(ptr))) + ") " + std::to_string(__LINE__) + " " + __FILE__);
         gil_scoped_acquire gil;
         Py_DECREF(reinterpret_cast<PyObject *>(ptr));
     }
@@ -378,6 +379,7 @@ struct smart_holder_type_caster_load {
     struct shared_ptr_dec_ref_deleter {
         PyObject *self;
         void operator()(void *) {
+to_cout("LOOOK shared_ptr_dec_ref_deleter call " + std::to_string(__LINE__) + " " + __FILE__);
             gil_scoped_acquire gil;
             Py_DECREF(self);
         }
@@ -399,25 +401,32 @@ struct smart_holder_type_caster_load {
             auto vptr_del_ptr = std::get_deleter<pybindit::memory::guarded_delete>(hld.vptr);
             if (vptr_del_ptr != nullptr) {
                 assert(!hld.vptr_is_released);
-                std::shared_ptr<void> released(hld.vptr.get(), [](void *) {});
+                std::shared_ptr<T> to_be_returned(hld.vptr, type_raw_ptr);
+int sftstat = pybindit::memory::shared_from_this_status(type_raw_ptr);
+to_cout("LOOOK loaded_as_shared_ptr return released SFT=" + std::to_string(sftstat) + " #1 " + std::to_string(__LINE__) + " " + __FILE__);
+                std::shared_ptr<void> non_owning(hld.vptr.get(), [](void *) {});
                 // Critical transfer-of-ownership section. This must stay together.
                 vptr_del_ptr->hld          = &hld;
                 vptr_del_ptr->callback_ptr = dec_ref_void;
                 vptr_del_ptr->callback_arg = self;
-                hld.vptr.swap(released);
+                hld.vptr = non_owning;
                 hld.vptr_is_released = true;
                 Py_INCREF(self);
                 // Critical section end.
-                return std::shared_ptr<T>(released, type_raw_ptr);
+sftstat = pybindit::memory::shared_from_this_status(type_raw_ptr);
+to_cout("LOOOK loaded_as_shared_ptr return released SFT=" + std::to_string(sftstat) + " #2 " + std::to_string(__LINE__) + " " + __FILE__);
+                return to_be_returned;
             }
             // XXX XXX XXX Ensure not shared_from_this.
             Py_INCREF(self);
+to_cout("LOOOK loaded_as_shared_ptr return shared_ptr_dec_ref_deleter " + std::to_string(__LINE__) + " " + __FILE__);
             return std::shared_ptr<T>(type_raw_ptr, shared_ptr_dec_ref_deleter{self});
         }
         if (hld.vptr_is_using_noop_deleter) {
             throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr).");
         }
         std::shared_ptr<void> void_shd_ptr = hld.template as_shared_ptr<void>();
+to_cout("LOOOK loaded_as_shared_ptr return hld vptr " + std::to_string(__LINE__) + " " + __FILE__);
         return std::shared_ptr<T>(void_shd_ptr, type_raw_ptr);
     }
 
@@ -701,10 +710,12 @@ struct smart_holder_type_caster<std::shared_ptr<T>> : smart_holder_type_caster_l
 
         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))
+        if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) {
             // SMART_HOLDER_WIP: MISSING: Enforcement of consistency with existing smart_holder.
             // SMART_HOLDER_WIP: MISSING: keep_alive.
+to_cout("LOOOK shtc sh return existing_inst " + std::to_string(__LINE__) + " " + __FILE__);
             return existing_inst;
+        }
 
         auto inst           = reinterpret_steal<object>(make_new_instance(tinfo->type));
         auto *inst_raw_ptr  = reinterpret_cast<instance *>(inst.ptr());
@@ -718,6 +729,7 @@ struct smart_holder_type_caster<std::shared_ptr<T>> : smart_holder_type_caster_l
         if (policy == return_value_policy::reference_internal)
             keep_alive_impl(inst, parent);
 
+to_cout("LOOOK shtc sh return new inst " + std::to_string(__LINE__) + " " + __FILE__);
         return inst.release();
     }
 

tests/CMakeLists.txt

@@ -109,6 +109,7 @@ set(PYBIND11_TEST_FILES
     test_class_sh_shared_from_this.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

tests/test_class_sh_trampoline_shared_from_this.cpp

@@ -0,0 +1,36 @@
+// Copyright (c) 2021 The Pybind Development Team.
+// All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#include "pybind11/smart_holder.h"
+#include "pybind11_tests.h"
+
+#include <memory>
+#include <string>
+
+namespace {
+
+struct WithSft : std::enable_shared_from_this<WithSft> {
+    virtual ~WithSft() = default;
+};
+
+struct WithSftTrampoline : WithSft {
+    using WithSft::WithSft;
+};
+
+void pass_shared_ptr(const std::shared_ptr<WithSft> &obj) {
+    to_cout("LOOOK pass_shared_ptr entry");
+    to_cout("LOOOK obj->shared_from_this();");
+    obj->shared_from_this();
+    to_cout("LOOOK pass_shared_ptr return");
+}
+
+} // namespace
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(WithSft)
+
+TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) {
+    py::classh<WithSft, WithSftTrampoline>(m, "WithSft").def(py::init<>());
+    m.def("pass_shared_ptr", pass_shared_ptr);
+    m.def("to_cout", to_cout);
+}

tests/test_class_sh_trampoline_shared_from_this.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+
+import pybind11_tests.class_sh_trampoline_shared_from_this as m
+
+
+class PyWithSft(m.WithSft):
+    pass
+
+
+def test_pass_shared_ptr():
+    m.to_cout("")
+    m.to_cout(">>> obj = PyWithSft()")
+    obj = PyWithSft()
+    m.to_cout(">>> m.pass_shared_ptr(obj) #1")
+    m.pass_shared_ptr(obj)
+    m.to_cout(">>> m.pass_shared_ptr(obj) #2")
+    m.pass_shared_ptr(obj)
+    m.to_cout(">>> del obj")
+    del obj