From 9e7b022baf8b6bc060c3f4655de4ae9ab2757d8d Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Mon, 16 Oct 2017 00:26:39 -0400 Subject: [PATCH] WIP: Squashed commit. --- include/pybind11/attr.h | 3 + include/pybind11/cast.h | 402 +++++++++++++++++++++++---- include/pybind11/detail/common.h | 99 +++++++ include/pybind11/detail/internals.h | 2 + include/pybind11/pybind11.h | 337 +++++++++++++++++++++- include/pybind11/pytypes.h | 102 +++++++ tests/CMakeLists.txt | 1 + tests/test_methods_and_attributes.py | 12 +- tests/test_ownership_transfer.cpp | 123 ++++++++ tests/test_ownership_transfer.py | 86 ++++++ tests/test_smart_ptr.cpp | 5 +- 11 files changed, 1109 insertions(+), 63 deletions(-) create mode 100644 tests/test_ownership_transfer.cpp create mode 100644 tests/test_ownership_transfer.py diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index dce875a6b9..182e99111d 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -226,6 +226,9 @@ struct type_record { /// Function pointer to class_<..>::dealloc void (*dealloc)(detail::value_and_holder &) = nullptr; + /// See `type_info::has_cpp_release`. + instance::type_release_info_t release_info; + /// List of base classes of the newly created type list bases; diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index eab904bee0..075e6a5999 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -156,12 +156,16 @@ inline const std::vector &all_type_info(PyTypeObject *type) * ancestors are pybind11-registered. Throws an exception if there are multiple bases--use * `all_type_info` instead if you want to support multiple bases. */ -PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) { +PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type, bool do_throw = true) { auto &bases = all_type_info(type); if (bases.size() == 0) return nullptr; - if (bases.size() > 1) - pybind11_fail("pybind11::detail::get_type_info: type has multiple pybind11-registered bases"); + if (bases.size() > 1) { + if (do_throw) + pybind11_fail("pybind11::detail::get_type_info: type has multiple pybind11-registered bases"); + else + return nullptr; + } return bases.front(); } @@ -229,6 +233,7 @@ struct value_and_holder { template H &holder() const { return reinterpret_cast(vh[1]); } + void* holder_ptr() const { return &vh[1]; } bool holder_constructed() const { return inst->simple_layout ? inst->simple_holder_constructed @@ -479,6 +484,75 @@ inline PyThreadState *get_thread_state_unchecked() { inline void keep_alive_impl(handle nurse, handle patient); inline PyObject *make_new_instance(PyTypeObject *type); +enum class LoadType { + PureCpp, + DerivedCppSinglePySingle, + DerivedCppSinglePyMulti, + DerivedCppMulti, + /// Polymorphic casting or copy-based casting may be necessary. + ConversionNeeded, +}; + +typedef type_info* base_ptr_t; +typedef const std::vector bases_t; + +inline LoadType determine_load_type(handle src, const type_info* typeinfo, + const bases_t** out_bases = nullptr, + base_ptr_t* out_base = nullptr) { + // Null out inputs. + if (out_bases) + *out_bases = nullptr; + if (out_base) + *out_base = nullptr; + PyTypeObject *srctype = Py_TYPE(src.ptr()); + // See `type_caster_generic::load_impl` below for more detail on comments. + + // Case 1: If src is an exact type match for the target type then we can reinterpret_cast + // the instance's value pointer to the target type: + if (srctype == typeinfo->type) { + // TODO(eric.cousineau): Determine if the type is upcast from a type, which is + // still a pure C++ object? + return LoadType::PureCpp; + } + // Case 2: We have a derived class + else if (PyType_IsSubtype(srctype, typeinfo->type)) { + const bases_t& bases = all_type_info(srctype); + if (out_bases) + *out_bases = &bases; // Copy to output for caching. + const bool no_cpp_mi = typeinfo->simple_type; + // Case 2a: the python type is a Python-inherited derived class that inherits from just + // one simple (no MI) pybind11 class, or is an exact match, so the C++ instance is of + // the right type and we can use reinterpret_cast. + // (This is essentially the same as case 2b, but because not using multiple inheritance + // is extremely common, we handle it specially to avoid the loop iterator and type + // pointer lookup overhead) + // TODO(eric.cousineau): This seems to also capture C++-registered classes as well, not just Python-derived + // classes. + if (bases.size() == 1 && (no_cpp_mi || bases.front()->type == typeinfo->type)) { + return LoadType::DerivedCppSinglePySingle; + } + // Case 2b: the python type inherits from multiple C++ bases. Check the bases to see if + // we can find an exact match (or, for a simple C++ type, an inherited match); if so, we + // can safely reinterpret_cast to the relevant pointer. + else if (bases.size() > 1) { + for (auto base : bases) { + if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { + if (out_base) { + *out_base = base; + } + return LoadType::DerivedCppSinglePyMulti; + } + } + } + // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type match + // in the registered bases, above, so try implicit casting (needed for proper C++ casting + // when MI is involved). + return LoadType::DerivedCppMulti; + } else { + return LoadType::ConversionNeeded; + } +} + class type_caster_generic { public: PYBIND11_NOINLINE type_caster_generic(const std::type_info &type_info) @@ -495,7 +569,7 @@ class type_caster_generic { const detail::type_info *tinfo, void *(*copy_constructor)(const void *), void *(*move_constructor)(const void *), - const void *existing_holder = nullptr) { + holder_erased existing_holder = {}) { if (!tinfo) // no type info: error will be set already return handle(); @@ -503,11 +577,64 @@ class type_caster_generic { if (src == nullptr) return none().release(); + const bool take_ownership = policy == return_value_policy::automatic || policy == return_value_policy::take_ownership; + // We only come across `!existing_holder` if we are coming from `cast` and not `cast_holder`. + const bool is_bare_ptr = !existing_holder.ptr() && existing_holder.type_id() == HolderTypeId::Unknown; + auto it_instances = get_internals().registered_instances.equal_range(src); for (auto it_i = it_instances.first; it_i != it_instances.second; ++it_i) { for (auto instance_type : detail::all_type_info(Py_TYPE(it_i->second))) { - if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) - return handle((PyObject *) it_i->second).inc_ref(); + if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) { + instance* const inst = it_i->second; + + bool try_to_reclaim = false; + if (!is_bare_ptr) { + switch (instance_type->release_info.holder_type_id) { + case detail::HolderTypeId::UniquePtr: { + try_to_reclaim = take_ownership; + break; + } + case detail::HolderTypeId::SharedPtr: { + if (take_ownership) { + // Only try to reclaim the object if (a) it is not owned and (b) has no holder. + if (!inst->simple_holder_constructed) { + if (inst->owned) + throw std::runtime_error("Internal error?"); + try_to_reclaim = true; + } + } + break; + } + default: { + // Otherwise, do not try any reclaiming. + break; + } + } + } + if (try_to_reclaim) { + // If this object has already been registered, but we wish to take ownership of it, + // then use the `has_cpp_release` mechanisms to reclaim ownership. + // @note This should be the sole occurrence of this registered object when releasing back. + // @note This code path should not be invoked for pure C++ + + // TODO(eric.cousineau): This may be still be desirable if this is a raw pointer... + // Need to think of a desirable workflow - and if there is possible interop. + if (!existing_holder) { + throw std::runtime_error("Internal error: Should have non-null holder."); + } + // TODO(eric.cousineau): This field may not be necessary if the lowest-level type is valid. + // See `move_only_holder_caster::load_value`. + if (!inst->reclaim_from_cpp) { + throw std::runtime_error("Instance is registered but does not have a registered reclaim method. Internal error?"); + } + return inst->reclaim_from_cpp(inst, existing_holder).release(); + } else { + // TODO(eric.cousineau): Should really check that ownership is consistent. + // e.g. if we say to take ownership of a pointer that is passed, does not have a holder... + // In the end, pybind11 would let ownership slip, and leak memory, possibly violating RAII (if someone is using that...) + return handle((PyObject *) it_i->second).inc_ref(); + } + } } } @@ -559,13 +686,14 @@ class type_caster_generic { throw cast_error("unhandled return_value_policy: should not happen!"); } - tinfo->init_instance(wrapper, existing_holder); + // TODO(eric.cousineau): Propagate `holder_erased` through this chain. + tinfo->init_instance(wrapper, existing_holder.ptr()); return inst.release(); } // Base methods for generic caster; there are overridden in copyable_holder_caster - void load_value(value_and_holder &&v_h) { + void load_value(value_and_holder &&v_h, LoadType) { auto *&vptr = v_h.value_ptr(); // Lazy allocation for unallocated values: if (vptr == nullptr) { @@ -638,49 +766,35 @@ class type_caster_generic { auto &this_ = static_cast(*this); this_.check_holder_compat(); - PyTypeObject *srctype = Py_TYPE(src.ptr()); - - // Case 1: If src is an exact type match for the target type then we can reinterpret_cast - // the instance's value pointer to the target type: - if (srctype == typeinfo->type) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder()); - return true; - } - // Case 2: We have a derived class - else if (PyType_IsSubtype(srctype, typeinfo->type)) { - auto &bases = all_type_info(srctype); - bool no_cpp_mi = typeinfo->simple_type; - - // Case 2a: the python type is a Python-inherited derived class that inherits from just - // one simple (no MI) pybind11 class, or is an exact match, so the C++ instance is of - // the right type and we can use reinterpret_cast. - // (This is essentially the same as case 2b, but because not using multiple inheritance - // is extremely common, we handle it specially to avoid the loop iterator and type - // pointer lookup overhead) - if (bases.size() == 1 && (no_cpp_mi || bases.front()->type == typeinfo->type)) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder()); + const bases_t* bases = nullptr; + base_ptr_t base_py_multi = nullptr; + LoadType load_type = determine_load_type(src, typeinfo, &bases, &base_py_multi); + switch (load_type) { + case LoadType::PureCpp: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(), + load_type); return true; } - // Case 2b: the python type inherits from multiple C++ bases. Check the bases to see if - // we can find an exact match (or, for a simple C++ type, an inherited match); if so, we - // can safely reinterpret_cast to the relevant pointer. - else if (bases.size() > 1) { - for (auto base : bases) { - if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(base)); - return true; - } - } + case LoadType::DerivedCppSinglePySingle: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(), + load_type); + return true; } - - // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type match - // in the registered bases, above, so try implicit casting (needed for proper C++ casting - // when MI is involved). - if (this_.try_implicit_casts(src, convert)) + case LoadType::DerivedCppSinglePyMulti: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(base_py_multi), + load_type); return true; + } + case LoadType::DerivedCppMulti: { + if (this_.try_implicit_casts(src, convert)) + return true; + } + case LoadType::ConversionNeeded: { + break; + } } - // Perform an implicit conversion + // If nothing else succeeds, perform an implicit conversion if (convert) { for (auto &converter : typeinfo->implicit_conversions) { auto temp = reinterpret_steal(converter(src.ptr(), typeinfo->type)); @@ -828,8 +942,11 @@ template class type_caster_base : public type_caster_generic { make_copy_constructor(src), make_move_constructor(src)); } - static handle cast_holder(const itype *src, const void *holder) { + static handle cast_holder(const itype *src, holder_erased holder) { auto st = src_and_type(src); + if (!holder) { + throw std::runtime_error("Internal error: Should not have null holder"); + } return type_caster_generic::cast( st.first, return_value_policy::take_ownership, {}, st.second, nullptr, nullptr, holder); @@ -1371,6 +1488,12 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +inline const detail::type_info* get_lowest_type(handle src, bool do_throw = true) { + auto* py_type = (PyTypeObject*)src.get_type().ptr(); + return detail::get_type_info(py_type, do_throw); +} + + /// Type caster for holder types like std::shared_ptr, etc. template struct copyable_holder_caster : public type_caster_base { @@ -1383,7 +1506,10 @@ struct copyable_holder_caster : public type_caster_base { using base::typeinfo; using base::value; - bool load(handle src, bool convert) { + handle src; + + bool load(handle src_in, bool convert) { + src = src_in; return base::template load_impl>(src, convert); } @@ -1399,11 +1525,20 @@ struct copyable_holder_caster : public type_caster_base { explicit operator holder_type&() { return holder; } #endif + // Risk increasing the `shared_ptr` ref count temporarily to maintain writeable + // semantics without too much `const_cast<>` ooginess. + static handle cast(holder_type &&src, return_value_policy, handle) { + const auto *ptr = holder_helper::get(src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); + } + static handle cast(const holder_type &src, return_value_policy, handle) { const auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); } + // TODO(eric.cousineau): Define cast_op_type??? + protected: friend class type_caster_generic; void check_holder_compat() { @@ -1411,11 +1546,32 @@ struct copyable_holder_caster : public type_caster_base { throw cast_error("Unable to load a custom holder type from a default-holder instance"); } - bool load_value(value_and_holder &&v_h) { + bool load_value(value_and_holder &&v_h, LoadType load_type) { + holder_type& v_holder = v_h.holder(); + bool do_release_to_cpp = false; + const type_info* lowest_type = nullptr; + if (src.ref_count() == 1 && load_type == LoadType::DerivedCppSinglePySingle) { + // Go ahead and release ownership to C++, if able. + auto* py_type = (PyTypeObject*)src.get_type().ptr(); + lowest_type = detail::get_type_info(py_type); + // Double-check that we did not get along C++ inheritance. + // TODO(eric.cousineau): Generalize this to unique_ptr<> case as well. + const bool is_actually_pure_cpp = lowest_type->type == py_type; + if (!is_actually_pure_cpp) { + if (lowest_type->release_info.can_derive_from_wrapper) { + do_release_to_cpp = true; + } else { + std::cerr << "WARNING! Casting to std::shared_ptr<> will cause Python subclass of pybind11 C++ instance to lose its Python portion. " + "Make your base class extend from pybind11::wrapper<> to prevent aliasing." + << std::endl; + } + } + } + if (v_h.holder_constructed()) { value = v_h.value_ptr(); - holder = v_h.holder(); - return true; + // Don't need to worry about double-counting the shared_ptr stuff. + holder = v_holder; } else { throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " #if defined(NDEBUG) @@ -1424,6 +1580,17 @@ struct copyable_holder_caster : public type_caster_base { "of type '" + type_id() + "''"); #endif } + + // Release *after* we already have + if (do_release_to_cpp) { + assert(v_h.inst->owned); + assert(lowest_type->release_info.release_to_cpp); + // Increase reference count to pass to release mechanism. + object obj = reinterpret_borrow(src); + lowest_type->release_info.release_to_cpp(v_h.inst, &holder, std::move(obj)); + } + + return true; } template ::value, int> = 0> @@ -1446,6 +1613,7 @@ struct copyable_holder_caster : public type_caster_base { holder_type holder; + constexpr static detail::HolderTypeId holder_type_id = detail::get_holder_type_id::value; }; /// Specialize for the common std::shared_ptr, so users don't need to @@ -1453,15 +1621,143 @@ template class type_caster> : public copyable_holder_caster> { }; template -struct move_only_holder_caster { +struct move_only_holder_caster : type_caster_base { + using base = type_caster_base; + static_assert(std::is_base_of>::value, + "Holder classes are only supported for custom types"); + using base::base; + using base::cast; + using base::typeinfo; + using base::value; + static_assert(std::is_base_of, type_caster>::value, "Holder classes are only supported for custom types"); static handle cast(holder_type &&src, return_value_policy, handle) { + // Move `src` so that `holder_helper<>::get()` can call `release` if need be. + // That way, if we mix `holder_type`s, we don't have to worry about `existing_holder` + // from being mistakenly reinterpret_cast'd to `shared_ptr` (#1138). auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); + } + + // Disable these? +// explicit operator type*() { return this->value; } +// explicit operator type&() { return *(this->value); } + +// explicit operator holder_type*() { return &holder; } + + // Force rvalue. + template + using cast_op_type = holder_type&&; + + explicit operator holder_type&&() { return std::move(holder); } + + object extract_from_container(handle src) { + // See if this is a supported `move` container. + if (isinstance(src, (PyObject*)&PyList_Type) && PyList_Size(src.ptr()) == 1) { + // Extract the object from a single-item list, and remove the existing reference so we have exclusive control. + // @note This will break implicit casting when constructing from vectors, but eh, who cares. + // Swap. + list li = src.cast(); + object obj = li[0]; + li[0] = none(); + return obj; + } else if (hasattr(src, "_is_move_container")) { + // Try to extract the value with `release()`. + return src.attr("release")(); + } else { + throw std::runtime_error( + "Only use cast>() with a Python move-container (such as a single-item list), " + "or ensure that you call cast(std::move(obj))"); + } + } + + bool load(handle src, bool convert) { + // Ensure that we have exclusive control (with `object` reference count control) over the entering object. + // That way, we maintain complete control, and do not need to worry about stacked function calls. + + // TODO(eric.cousineau): Inconsistency will be absolutely painful. Thinking about, move'ing the return value of a function + // may work with the exact type desired, but it most likely *won't* work if the type is wrapped in a `move` container. + // Solution: Check more aggressively if this is a move container. + + object obj_exclusive = extract_from_container(src); + if (obj_exclusive.ref_count() != 1) { + throw std::runtime_error("Non-unique reference, cannot cast to unique_ptr."); + } + + // TODO(eric.cousineau): To reduce number of references, require that a list be passed in, + // such that the value can be set to zero. + // That, or figure out where the `py::object` is in this call chain, and set that to zero. + // Specialize for unique_ptr<>? + + // Do not use `load_impl`, as it's not structured conveniently for `unique_ptr`. + // Specifically, trying to delegate to resolving to conversion. + // return base::template load_impl>(src, convert); + check_holder_compat(); + auto v_h = reinterpret_cast(obj_exclusive.ptr())->get_value_and_holder(); + LoadType load_type = determine_load_type(obj_exclusive, typeinfo); + return load_value(std::move(obj_exclusive), std::move(v_h), load_type); } + static PYBIND11_DESCR name() { return type_caster_base::name(); } + +protected: + friend class type_caster_generic; + void check_holder_compat() { + if (!typeinfo->default_holder) + throw cast_error("Unable to load a non-default holder type (unique_ptr)"); + } + + bool load_value(object obj_exclusive, value_and_holder &&v_h, LoadType load_type) { + // TODO(eric.cousineau): This should try and find the downcast-lowest + // level (closest to child) `release_to_cpp` method that is derived-releasable + // (which derives from `wrapper`). + // This should resolve general casting, and should also avoid alias + // branch issues: + // Example: `Base` has wrapper `PyBase` which extends `wrapper`. + // `Child` extends `Base`, has its own wrapper `PyChild`, which extends + // `wrapper`. + // Anything deriving from `Child` does not derive from `PyBase`, so we should + // NOT try to release using `PyBase`s mechanism. + // Additionally, if `Child` does not have a wrapper (for whatever reason) and is extended, + // then we still can NOT use `PyBase` since it's not part of the hierachy. + + // Try to get the lowest-hierarchy level of the type. + // This requires that we are single-inheritance at most. + const detail::type_info* lowest_type = nullptr; + switch (load_type) { + case LoadType::PureCpp: { + // We already have the lowest type. + lowest_type = typeinfo; + break; + } + // If the base type is explicitly mentioned, then we can rely on `DerivedCppSinglePySingle` being used. + case LoadType::DerivedCppSinglePySingle: + // However, if it is not, it may be that we have a C++ type inheriting from another C++ type without the inheritance being registered. + // In this case, we delegate by effectively downcasting in Python by finding the lowest-level type. + // @note No `break` here on purpose! + case LoadType::ConversionNeeded: { + // Try to get the lowest-hierarchy (closets to child class) of the type. + // The usage of `get_type_info` implicitly requires single inheritance. + auto* py_type = (PyTypeObject*)obj_exclusive.get_type().ptr(); + lowest_type = detail::get_type_info(py_type); + break; + } + default: { + throw std::runtime_error("Unsupported load type"); + } + } + if (!lowest_type) + throw std::runtime_error("No valid lowest type. Internal error?"); + auto& release_info = lowest_type->release_info; + if (!release_info.release_to_cpp) + throw std::runtime_error("No release mechanism in lowest type?"); + release_info.release_to_cpp(v_h.inst, &holder, std::move(obj_exclusive)); + return true; + } + + holder_type holder; }; template diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 8f763f08aa..f3635ae10a 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -354,6 +354,8 @@ enum class return_value_policy : uint8_t { reference_internal }; +class object; + NAMESPACE_BEGIN(detail) inline static constexpr int log2(size_t n, int k = 0) { return (n <= 1) ? k : log2(n >> 1, k + 1); } @@ -373,6 +375,83 @@ constexpr size_t instance_simple_holder_in_ptrs() { return size_in_ptrs(sizeof(std::shared_ptr)); } +enum class HolderTypeId { + Unknown, + UniquePtr, + SharedPtr, +}; +template +struct get_holder_type_id { + static constexpr HolderTypeId value = HolderTypeId::Unknown; +}; +template +struct get_holder_type_id, void> { + static constexpr HolderTypeId value = HolderTypeId::SharedPtr; +}; +template +struct get_holder_type_id, void> { + // TODO(eric.cousineau): Should this only specialize for `std::default_deleter`? + static constexpr HolderTypeId value = HolderTypeId::UniquePtr; +}; + +class holder_erased { + public: + holder_erased() = default; + holder_erased(const holder_erased&) = default; + holder_erased& operator=(const holder_erased&) = default; + + template + holder_erased(const holder_type* holder) + : ptr_(const_cast(holder)), + type_id_(get_holder_type_id::value), + is_const_(true) {} + + template + holder_erased(holder_type* holder) + : holder_erased(static_cast(holder)) { + is_const_ = false; + } + + holder_erased(const void* ptr, HolderTypeId type_id) + : ptr_(const_cast(ptr)), + type_id_(type_id), + is_const_(true) {} + + holder_erased(void* ptr, HolderTypeId type_id) + : ptr_(ptr), + type_id_(type_id), + is_const_(false) {} + + void* ptr() const { return ptr_; } + HolderTypeId type_id() const { return type_id_; } + + template + holder_type& mutable_cast() const { + if (is_const_) + throw std::runtime_error("Trying to mutate const reference?"); + return do_cast(); + } + + template + const holder_type& cast() const { + return do_cast(); + } + + operator bool() const { return ptr_; } + private: + template + holder_type& do_cast() const { + if (type_id_ != get_holder_type_id::value) { + throw std::runtime_error("Mismatch on holder type."); + } + return *reinterpret_cast(ptr_); + } + + void* ptr_{}; + HolderTypeId type_id_{HolderTypeId::Unknown}; + bool is_const_{true}; +}; + // Forward declarations struct type_info; struct value_and_holder; @@ -423,6 +502,26 @@ struct instance { /// If true, get_internals().patients has an entry for this object bool has_patients : 1; + typedef void (*release_to_cpp_t)(instance* inst, holder_erased external_holder, object&& obj); + typedef object (*reclaim_from_cpp_t)(instance* inst, holder_erased external_holder); + + struct type_release_info_t { + // Release an instance to C++ for pure C++ instances or Python-derived classes. + release_to_cpp_t release_to_cpp = nullptr; + + // For classes wrapped in `wrapper<>`. See `move_only_holder_caster` for more info. + // Pure / direct C++ objects do not need any fancy releasing mechanisms. They are simply + // unwrapped and passed back. + bool can_derive_from_wrapper = false; + + // The holder that is contained by this class. + HolderTypeId holder_type_id = HolderTypeId::Unknown; + }; + /// If the instance is a Python-derived type that is owned in C++, then this method + /// will permit the instance to be reclaimed back by Python. + // TODO(eric.cousineau): This may not be necessary. See note in `type_caster_generic::cast`. + reclaim_from_cpp_t reclaim_from_cpp = nullptr; + /// Initializes all of the above type/values/holders data (but not the instance values themselves) void allocate_layout(); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 213cbaeb21..0236e05ba1 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -108,6 +108,8 @@ struct type_info { bool default_holder : 1; /* true if this is a type registered with py::module_local */ bool module_local : 1; + + instance::type_release_info_t release_info; }; /// Tracks the `internals` and `type_info` ABI version independent of the main library version diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 613135a7a5..00fc0568bb 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -47,6 +47,9 @@ NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +template +void unused(Args&&...) {} + /// Wraps an arbitrary C++ function/method/lambda function/.. into a callable Python object class cpp_function : public function { public: @@ -69,14 +72,14 @@ class cpp_function : public function { /// Construct a cpp_function from a class method (non-const) template cpp_function(Return (Class::*f)(Arg...), const Extra&... extra) { - initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*) (Class *, Arg...)) nullptr, extra...); } /// Construct a cpp_function from a class method (const) template cpp_function(Return (Class::*f)(Arg...) const, const Extra&... extra) { - initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*)(const Class *, Arg ...)) nullptr, extra...); } @@ -910,6 +913,8 @@ class generic_type : public object { tinfo->default_holder = rec.default_holder; tinfo->module_local = rec.module_local; + tinfo->release_info = rec.release_info; + auto &internals = get_internals(); auto tindex = std::type_index(*rec.type); tinfo->direct_conversions = &internals.direct_conversions[tindex]; @@ -1011,6 +1016,129 @@ auto method_adaptor(Return (Class::*pmf)(Args...)) -> Return (Derived::*)(Args.. template auto method_adaptor(Return (Class::*pmf)(Args...) const) -> Return (Derived::*)(Args...) const { return pmf; } + + +template +struct wrapper_interface_impl { + static void use_cpp_lifetime(type* cppobj, object&& obj, detail::HolderTypeId holder_type_id) { + auto* tr = dynamic_cast*>(cppobj); + if (tr == nullptr) { + // This has been invoked at too high of a level; should use a + // downcast class's `release_to_cpp` mechanism (if it supports it). + throw std::runtime_error( + "Attempting to release to C++ using pybind11::wrapper<> " + "at too high of a level. Use a class type lower in the hierarchy, such that " + "the Python-derived instance actually is part of the lineage of " + "pybind11::wrapper"); + } + // Let the external holder take ownership, but keep instance registered. + tr->use_cpp_lifetime(std::move(obj), holder_type_id); + } + + static object release_cpp_lifetime(type* cppobj) { + auto* tr = dynamic_cast*>(cppobj); + if (tr == nullptr) { + // This shouldn't happen here... + throw std::runtime_error("Internal error?"); + } + // Return newly created object. + return tr->release_cpp_lifetime(); + } + static wrapper* run(type* value, std::false_type) { + return nullptr; + } +}; + +template +struct wrapper_interface_impl { + static void use_cpp_lifetime(type*, object&&, detail::HolderTypeId) { + // This should be captured by runtime flag. + // TODO(eric.cousineau): Runtime flag may not be necessary. + throw std::runtime_error("Internal error?"); + } + static object release_cpp_lifetime(type*) { + throw std::runtime_error("Internal error?"); + } +}; + +template +struct holder_check_impl { + template + static bool check_destruct(...) { + // Noop by default. + return true; + } + template + static bool allow_null_external_holder(const holder_type&) { + return false; + } + template + static bool attempt_holder_transfer(holder_type& holder, detail::holder_erased external_holder_raw) { + // Only called when holder types are different. + unused(holder, external_holder_raw); + throw std::runtime_error("Unable to transfer between holders of different types"); + } +}; + +template <> +struct holder_check_impl : public holder_check_impl { + template + static bool check_destruct(detail::instance* inst, detail::holder_erased holder_raw) { + const holder_type& h = holder_raw.cast(); + handle src((PyObject*)inst); + const detail::type_info *lowest_type = get_lowest_type(src, false); + if (!lowest_type) + // We have multiple inheritance, skip. + return true; + auto load_type = detail::determine_load_type(src, lowest_type); + // Check use_count(), assuming that we have an accurate count (no competing threads?) + if (load_type == detail::LoadType::DerivedCppSinglePySingle) { + if (h.use_count() > 1) { + // Increase reference count + const auto& release_info = lowest_type->release_info; + if (release_info.can_derive_from_wrapper) { + // Increase reference count + object obj = reinterpret_borrow(src); + // Release to C++. + holder_type* null_holder = nullptr; + release_info.release_to_cpp(inst, detail::holder_erased(null_holder), std::move(obj)); + return false; + } + } + } + return true; + } + + template + static bool allow_null_external_holder(const holder_type& holder) { + // Called by `release_to_cpp`. + if (holder.use_count() == 1) + // TODO(eric.cousineau): This may not hold true if we pass temporaries??? + // Or if we've copied a `holder` in copyable_holder_caster... + throw std::runtime_error("Internal error: Should have non-null shared_ptr<> external_holder if use_count() == 1"); + else + return true; + } +}; + + +template <> +struct holder_check_impl : public holder_check_impl { + template + static bool attempt_holder_transfer(holder_type& holder, detail::holder_erased external_holder_raw) { + // Only accept shared_ptr from `external_holder_raw`. + if (external_holder_raw.type_id() == detail::HolderTypeId::SharedPtr) { + using T = typename holder_type::element_type; + auto& external_holder = external_holder_raw.mutable_cast>(); + // Transfer to external. + external_holder = std::move(holder); + return true; + } else { + return false; + } + } +}; + template class class_ : public detail::generic_type { template using is_holder = detail::is_holder_type; @@ -1024,7 +1152,9 @@ class class_ : public detail::generic_type { using type = type_; using type_alias = detail::exactly_one_t; constexpr static bool has_alias = !std::is_void::value; + constexpr static bool has_wrapper = std::is_base_of, type_alias>::value; using holder_type = detail::exactly_one_t, options...>; + constexpr static detail::HolderTypeId holder_type_id = detail::get_holder_type_id::value; static_assert(detail::all_of...>::value, "Unknown/invalid class_ template parameters provided"); @@ -1056,6 +1186,12 @@ class class_ : public detail::generic_type { record.dealloc = dealloc; record.default_holder = std::is_same>::value; + // TODO(eric.cousineau): Determine if it is possible to permit releasing without a wrapper... + auto& release_info = record.release_info; + release_info.can_derive_from_wrapper = has_wrapper; + release_info.release_to_cpp = release_to_cpp; + release_info.holder_type_id = holder_type_id; + set_operator_new(&record); /* Register base classes specified via template arguments to class_, if any */ @@ -1072,6 +1208,167 @@ class class_ : public detail::generic_type { } } + static detail::type_info* get_type_info() { + std::type_index id(typeid(type)); + return detail::get_type_info(id); + } + + typedef wrapper_interface_impl wrapper_interface; + using holder_check = holder_check_impl; + + static bool allow_destruct(detail::instance* inst, detail::holder_erased holder) { + // TODO(eric.cousineau): There should not be a case where shared_ptr<> lives in + // C++ and Python, with it being owned by C++. Check this. + return holder_check::template check_destruct(inst, holder); + } + + static void del_wrapped(handle self, object del_orig) { + // This should be called when the item is *actually* being deleted + // TODO(eric.cousineau): Do we care about use cases where the user manually calls this? + detail::instance* inst = (detail::instance*)self.ptr(); + const detail::type_info *lowest_type = detail::get_lowest_type(self); + auto& release_info = lowest_type->release_info; + // The references are as follows: + // 1. When Python calls __del__ via tp_del (default slot) + // 2. When Python gets the instance-bound __del__ method. + // TODO(eric.cousineau): Confirm this ^ + // 3. When pybind11 gets the argument + int orig_count = self.ref_count(); + unused(orig_count); // Suppress release build warnings. + assert(orig_count == 3); + + auto v_h = inst->get_value_and_holder(lowest_type); + detail::holder_erased holder_raw(v_h.holder_ptr(), release_info.holder_type_id); + + // TODO(eric.cousineau): Ensure that this does not prevent destruction if + // the Python interpreter is finalizing... + // Is there a way to do this without a custom handler? + + // Purposely do NOT capture `object` to refcount low. + // TODO(eric.cousineau): `allow_destruct` should be registered in `type_info`. + // Right now, this doesn't really type-erase anything... + if (allow_destruct(inst, holder_raw)) { + // Call the old destructor. + del_orig(self); + } else { + // This should have been kept alive by an increment in number of references. + assert(self.ref_count() == orig_count + 1); + } + } + + static void release_to_cpp(detail::instance* inst, detail::holder_erased external_holder_raw, object&& obj) { + using detail::LoadType; + auto v_h = inst->get_value_and_holder(); + auto* tinfo = get_type_info(); + if (!inst->owned || !v_h.holder_constructed()) { + throw std::runtime_error("Python-extended C++ object should be owned by pybind11 when attempting to release to C++"); + } + LoadType load_type = determine_load_type(obj, tinfo); + switch (load_type) { + case LoadType::PureCpp: { + // Given that `obj` is now exclusive, then once it goes out of scope, + // then the registered instance for this object should be destroyed, and this + // should become a pure C++ object, without any ties to `pybind11`. + // Also, even if this instance is of a class derived from a Base that has a + // wrapper-wrapper alias, we do not need to worry about not being in the correct + // hierarchy, since we will simply release from it. + + // TODO(eric.cousineau): Presently, there is no support for a consistent use of weak references. + // If a PureCpp object is released from Python, then all weak references are invalidated, + // even if it comes back... + // Ideally, this could check if there are weak references. But to whom should the lifetime be extended? + // Perhaps the first weak reference that is available? + break; + } + case LoadType::DerivedCppSinglePySingle: { + if (!tinfo->release_info.can_derive_from_wrapper) { + throw std::runtime_error( + "Python-extended C++ class does not inherit from pybind11::wrapper<>, " + "so lifetime cannot be attached to a C++ representation of the objection. " + "To fix this, ensure registered type has an alias which extends " + "pybind11::wrapper<>."); + } + auto* cppobj = reinterpret_cast(v_h.value_ptr()); + wrapper_interface::use_cpp_lifetime(cppobj, std::move(obj), holder_type_id); + break; + } + default: { + throw std::runtime_error("Unsupported load type (multiple inheritance)"); + } + } + bool transfer_holder = true; + holder_type& holder = v_h.holder(); + if (!external_holder_raw.ptr()) { + if (holder_check::allow_null_external_holder(holder)) + transfer_holder = false; + else + throw std::runtime_error("Internal error: Null external holder"); + } + if (transfer_holder) { + if (external_holder_raw.type_id() == holder_type_id) { + holder_type& external_holder = external_holder_raw.mutable_cast(); + external_holder = std::move(holder); + } else { + // Only allow unique_ptr<> -> shared_ptr<> + holder_check::attempt_holder_transfer(holder, external_holder_raw); + } + } + holder.~holder_type(); + v_h.set_holder_constructed(false); + inst->owned = false; + // Register this type's reclamation procedure, since it's wrapper may have the contained object. + inst->reclaim_from_cpp = reclaim_from_cpp; + } + + static object reclaim_from_cpp(detail::instance* inst, detail::holder_erased external_holder_raw) { + using detail::LoadType; + auto v_h = inst->get_value_and_holder(); + auto* tinfo = get_type_info(); + // TODO(eric.cousineau): Should relax this to not require a holder be constructed, + // only that the holder itself be default (unique_ptr<>). + if (inst->owned || v_h.holder_constructed()) { + throw std::runtime_error("Derived Python object should live in C++"); + } + if (!external_holder_raw) { + throw std::runtime_error("Internal error - not external holder?"); + } + // Is this valid? + handle h(reinterpret_cast(inst)); + LoadType load_type = determine_load_type(h, tinfo); + { + // TODO(eric.cousineau): Consider releasing a raw pointer, to make it easier for + // interop with purely raw pointers? Nah, just rely on release. + holder_type& holder = v_h.holder(); + holder_type& external_holder = external_holder_raw.mutable_cast(); + new (&holder) holder_type(std::move(external_holder)); + v_h.set_holder_constructed(true); + + // Show that it has been reclaimed. + inst->reclaim_from_cpp = nullptr; + } + object obj; + switch (load_type) { + case LoadType::PureCpp: { + // This should generally not be called. + // However, in a direct pass-through case (which should be rare), the Python reference + // may stay alive before returning, in which case we do nothing, and just pass the + // existing reference. + // TODO(eric.cousineau): This would change if weak references were somehow supported. + throw std::runtime_error("Should not be called. Period."); + } + case LoadType::DerivedCppSinglePySingle: { + auto* cppobj = reinterpret_cast(v_h.value_ptr()); + obj = wrapper_interface::release_cpp_lifetime(cppobj); + break; + } + default: { + throw std::runtime_error("Unsupported load type"); + } + } + inst->owned = true; + return obj; + } + template ::value, int> = 0> static void add_base(detail::type_record &rec) { rec.add_base(typeid(Base), [](void *src) -> void * { @@ -1307,12 +1604,48 @@ class class_ : public detail::generic_type { /// optional pointer to an existing holder to use; if not specified and the instance is /// `.owned`, a new holder will be constructed to manage the value pointer. static void init_instance(detail::instance *inst, const void *holder_ptr) { + using namespace detail; auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); } init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); + + // TODO(eric.cousineau): Inject override of __del__ for intercepting C++ stuff + handle self((PyObject*)inst); + handle h_type = self.get_type(); + + // Use hacky Python-style inheritance check. + PyTypeObject *py_type = (PyTypeObject*)h_type.ptr(); + bool is_py_derived = py_type->tp_dealloc != detail::pybind11_object_dealloc; + + // Check tp_dealloc + if (is_py_derived) { + // TODO(eric.cousineau): Consider moving this outside of this class, + // to potentially enable multiple inheritance. + const std::string orig_field = "_pybind11_del_orig"; + object del_orig = getattr(h_type, orig_field.c_str(), none()); + if (del_orig.is(none())) { + // Get non-instance-bound method (analogous `tp_del`) + // Is there a way to check if `__del__` is an instance-assigned method? (Rather than a class method?) + del_orig = getattr(h_type, "__del__", none()); + holder_type& holder = v_h.holder(); + // The holder will not change address during the lifetime of this object, + // as it will always live in the instance (for `simple_holder` types). + holder_erased holder_raw(&holder); + // NOTE: This is NOT tied to this particular type. + auto del_new = [orig_field](handle h_self) { + // TODO(eric.cousineau): Make this global, not tied to this type. + object del_orig = getattr(h_self.get_type(), orig_field.c_str()); + del_wrapped(h_self, del_orig); + }; + // Replace with an Python-instance-unbound function. + object new_dtor_py = cpp_function(del_new, is_method(h_type)); + setattr(h_type, "__del__", new_dtor_py); + setattr(h_type, orig_field.c_str(), del_orig); + } + } } /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index d7fa17775c..6b28f5411c 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -11,6 +11,7 @@ #include "detail/common.h" #include "buffer_info.h" +#include #include #include @@ -1297,6 +1298,107 @@ inline iterator iter(handle obj) { } /// @} python_builtins + + +/// Trampoline class to permit attaching a derived Python object's data +/// (namely __dict__) to an actual C++ class. +/// If the object lives purely in C++, then there should only be one reference to +/// this data. +template +class wrapper : public Base { + protected: + using Base::Base; + + public: + // TODO(eric.cousineau): Complain if this is not virtual? (and remove `virtual` specifier in dtor?) + + virtual ~wrapper() { + delete_py_if_in_cpp(); + } + + /// To be used by `move_only_holder_caster`. + // TODO(eric.cousineau): Make this private to ensure contract? + void use_cpp_lifetime(object&& patient, detail::HolderTypeId holder_type_id) { + if (lives_in_cpp()) { + throw std::runtime_error("Instance already lives in C++"); + } + holder_type_id_ = holder_type_id; + patient_ = std::move(patient); + check("entering C++"); + } + + /// To be used by `move_only_holder_caster`. + object release_cpp_lifetime(bool on_destruct = false) { + if (!lives_in_cpp()) { + throw std::runtime_error("Instance does not live in C++"); + } + // Remove existing reference. + if (!on_destruct) + check("exiting C++"); + object tmp = std::move(patient_); + assert(!patient_); + return tmp; + } + + protected: + /// Call this if, for whatever reason, your C++ wrapper class `Base` has a non-trivial + /// destructor that needs to keep information available to the Python-extended class. + /// In this case, you want to delete the Python object *before* you do any work in your wrapper class. + /// + /// As an example, say you have `Base`, and `PyBase` is your wrapper class which extends `wrapper`. + /// By default, if the instance is owned in C++ and deleted, then the destructor order will be: + /// ~PyBase() + /// do_stuff() + /// ~wrapper() + /// delete_py_if_in_cpp() + /// PyChild.__del__ - ERROR: Should have been called before `do_stuff() + /// ~Base() + /// If you explicitly call `delete_py_if_in_cpp()`, then you will get the desired order: + /// ~PyBase() + /// delete_py_if_in_cpp() + /// PyChild.__del__ - GOOD: Workzzz. Called before `do_stuff()`. + /// do_stuff() + /// ~wrapper() + /// delete_py_if_in_cpp() - No-op. Python object has been released. + /// ~Base() + // TODO(eric.cousineau): Verify this with an example workflow. + void delete_py_if_in_cpp() { + if (lives_in_cpp()) { + // Ensure that we still are the unique one, such that the Python classes + // destructor will be called. + check("being destructed", false); + // Release object. + // TODO(eric.cousineau): Ensure that destructor is called instantly!!! + // Can we attach a listener to ensure that `dealloc` is called? + release_cpp_lifetime(true); + } + } + + private: + bool lives_in_cpp() const { + // NOTE: This is *false* if, for whatever reason, the wrapper class is + // constructed in C++... Meh. Not gonna worry about that situation. + return static_cast(patient_); + } + + // Throw an error if this stuff is not unique. + void check(const std::string& context, bool do_throw = true) { + if (holder_type_id_ == detail::HolderTypeId::UniquePtr) { + if (patient_.ref_count() != 1) { + std::string msg = "When " + context + ", ref_count != 1"; + if (do_throw) + throw std::runtime_error(msg); + else + std::cerr << "ERROR: " << msg << std::endl; + } + } + } + + object patient_; + detail::HolderTypeId holder_type_id_{detail::HolderTypeId::Unknown}; +}; + + NAMESPACE_BEGIN(detail) template iterator object_api::begin() const { return iter(derived()); } template iterator object_api::end() const { return iterator::sentinel(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 25e06662c8..4f5ff413e0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -51,6 +51,7 @@ set(PYBIND11_TEST_FILES test_numpy_vectorize.cpp test_opaque_types.cpp test_operator_overloading.cpp + test_ownership_transfer.cpp test_pickling.cpp test_pytypes.cpp test_sequences_and_iterators.cpp diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 9fd9cb75cd..59f13181d1 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -4,10 +4,10 @@ def test_methods_and_attributes(): - instance1 = m.ExampleMandA() - instance2 = m.ExampleMandA(32) + instance1 = m.ExampleMandA() # 1 def.ctor + instance2 = m.ExampleMandA(32) # 1 ctor - instance1.add1(instance2) + instance1.add1(instance2) # 1 copy ctor + 1 move instance1.add2(instance2) instance1.add3(instance2) instance1.add4(instance2) @@ -20,7 +20,7 @@ def test_methods_and_attributes(): assert str(instance1) == "ExampleMandA[value=320]" assert str(instance2) == "ExampleMandA[value=32]" - assert str(instance1.self1()) == "ExampleMandA[value=320]" + assert str(instance1.self1()) == "ExampleMandA[value=320]" # 1 copy ctor + 1 move assert str(instance1.self2()) == "ExampleMandA[value=320]" assert str(instance1.self3()) == "ExampleMandA[value=320]" assert str(instance1.self4()) == "ExampleMandA[value=320]" @@ -58,8 +58,8 @@ def test_methods_and_attributes(): assert cstats.alive() == 0 assert cstats.values() == ["32"] assert cstats.default_constructions == 1 - assert cstats.copy_constructions == 3 - assert cstats.move_constructions >= 1 + assert cstats.copy_constructions == 2 + assert cstats.move_constructions == 2 assert cstats.copy_assignments == 0 assert cstats.move_assignments == 0 diff --git a/tests/test_ownership_transfer.cpp b/tests/test_ownership_transfer.cpp new file mode 100644 index 0000000000..3d4ac820bc --- /dev/null +++ b/tests/test_ownership_transfer.cpp @@ -0,0 +1,123 @@ +/* + tests/test_ownership_transfer.cpp -- test ownership transfer semantics. + + Copyright (c) 2017 Eric Cousineau + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#if defined(_MSC_VER) && _MSC_VER < 1910 +# pragma warning(disable: 4702) // unreachable code in system header +#endif + +#include +#include "pybind11_tests.h" +#include "object.h" + +//#include "python/pybind11_include/pybind11/pybind11.h" + +enum Label : int { + BaseBadLabel = 0, + ChildBadLabel = 1, + BaseLabel = 2, + ChildLabel = 3, +}; + +// For attaching instances of `ConstructorStats`. +template +class Stats {}; + + +template +class DefineBase { + public: + DefineBase(int value) + : value_(value) { + track_created(this, value); + } + virtual ~DefineBase() { + track_destroyed(this); + } + virtual int value() const { return value_; } + private: + int value_{}; +}; + +template +class DefineBaseContainer { + public: + using T = DefineBase