From 11cab960d6423b012a5d1021bb31177f8c517a73 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 31 May 2024 01:07:20 -0600 Subject: [PATCH] WIP: Handle dangling objects and rv_policy changes robustly This is a potential answer to #888, also inspired by the discussion in #589. Seeking feedback on the approach before I work on docs and tests. When returning unique or shared ownership to Python when some Python instances already hold a non-owning reference to the same C++ object, we have a few choices: * Create a new owning instance in addition to the existing non-owning one. (This PR's approach.) * Upgrade the existing non-owning instance to owning. (More appealing in some ways, but hard to implement with shared ownership since nanobind doesn't have holders.) * Just return the non-owning instance and forget about the ownership. (nanobind's current approach, which sometimes causes problems.) This PR also handles the fact that a non-owning instance can dangle. That is, the referenced C++ object could be destroyed while the Python instance is still alive -- especially on PyPy, where it's hard to control when a Python instance dies. Dangling instances are always non-owning, so they are mostly handled by the same logic that handles rv_policy changes. The remaining piece of support for dangling instances is to acknowledge that, if the C++ referent is freed, a new instance could be allocated with its same address, even one with internal storage. (I have observed this in production.) So, there is some new logic in `inst_new_int` to remove the previous must-be-dangling instances from `inst_c2p`, rather than crashing because they exist. This also implies a new state that a nanobind instance can be in: if inst->offset == 0, the instance refers to no C++ object and is not stored in the inst_c2p map. --- docs/api_core.rst | 6 +- docs/changelog.rst | 7 + include/nanobind/intrusive/ref.h | 15 +- include/nanobind/nb_cast.h | 13 +- include/nanobind/nb_enums.h | 17 ++- include/nanobind/stl/shared_ptr.h | 19 +-- src/nb_internals.h | 10 +- src/nb_type.cpp | 222 ++++++++++++++++++++++++------ 8 files changed, 241 insertions(+), 68 deletions(-) diff --git a/docs/api_core.rst b/docs/api_core.rst index ccc2c89d..f6648460 100644 --- a/docs/api_core.rst +++ b/docs/api_core.rst @@ -2929,7 +2929,8 @@ The documentation below refers to two per-instance flags with the following mean set to ``true`` and ``false``. This is analogous to casting a C++ object with return value policy - :cpp:enumerator:`rv_policy::reference`. + :cpp:enumerator:`rv_policy::reference`, except that it will create a + new Python instance even if one wrapping this object already exists. If a `parent` object is specified, the instance keeps this parent alive while the newly created object exists. This is analogous to casting a C++ @@ -2946,7 +2947,8 @@ The documentation below refers to two per-instance flags with the following mean ``true``. This is analogous to casting a C++ object with return value policy - :cpp:enumerator:`rv_policy::take_ownership`. + :cpp:enumerator:`rv_policy::take_ownership`, except that it will create a + new Python instance even if one wrapping this object already exists. .. cpp:function:: void inst_zero(handle h) diff --git a/docs/changelog.rst b/docs/changelog.rst index 530a6b2a..074e8819 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -73,6 +73,13 @@ Version TBD (not yet released) binding abstractions that "feel like" the built-in ones. (PR `#884 `__) +* Nanobind now avoids returning an existing non-owning Python instance when + an owning Python instance was requested. Instead, it will create a new + Python instance that holds (unique or shared) ownership of the C++ object, + and allow the existing instance to continue to hold a reference to it. + This also helps in situations where + (PR `#889 `__) + Version 2.4.0 (Dec 6, 2024) --------------------------- diff --git a/include/nanobind/intrusive/ref.h b/include/nanobind/intrusive/ref.h index 3f113b6d..4a0133be 100644 --- a/include/nanobind/intrusive/ref.h +++ b/include/nanobind/intrusive/ref.h @@ -139,12 +139,23 @@ template struct type_caster> { static handle from_cpp(const ref &value, rv_policy policy, cleanup_list *cleanup) noexcept { + bool force_new = policy == rv_policy::copy || policy == rv_policy::move; + if constexpr (std::is_base_of_v) - if (policy != rv_policy::copy && policy != rv_policy::move && value.get()) + if (!force_new && value.get()) if (PyObject* obj = value->self_py()) return handle(obj).inc_ref(); - return Caster::from_cpp(value.get(), policy, cleanup); + /* The combination of rv_policy::_shared_ownership + not asking + for an is_new response, is treated as requiring intrusive ownership. + We could pass any policy here except copy/move, but using this one + allows nanobind to check that the intrusive_ptr() annotation was + specified correctly. */ + if (!force_new) + policy = rv_policy::_shared_ownership; + + return Caster::from_cpp_raw((std::decay_t *) value.get(), policy, + cleanup, nullptr); } }; NAMESPACE_END(detail) diff --git a/include/nanobind/nb_cast.h b/include/nanobind/nb_cast.h index 2865ded7..083f4758 100644 --- a/include/nanobind/nb_cast.h +++ b/include/nanobind/nb_cast.h @@ -481,7 +481,14 @@ template struct type_caster_base : type_caster_base_tag { else ptr = (Type *) &value; - policy = infer_policy(policy); + return from_cpp_raw(ptr, infer_policy(policy), cleanup, nullptr); + } + + /* Version of from_cpp that uses the rv_policy you give it as-is, + without resolving through infer_policy */ + NB_INLINE static handle from_cpp_raw(Type *ptr, rv_policy policy, + cleanup_list *cleanup, + bool *is_new) noexcept { const std::type_info *type = &typeid(Type); constexpr bool has_type_hook = @@ -490,11 +497,11 @@ template struct type_caster_base : type_caster_base_tag { type = type_hook::get(ptr); if constexpr (!std::is_polymorphic_v) { - return nb_type_put(type, ptr, policy, cleanup); + return nb_type_put(type, ptr, policy, cleanup, is_new); } else { const std::type_info *type_p = (!has_type_hook && ptr) ? &typeid(*ptr) : nullptr; - return nb_type_put_p(type, type_p, ptr, policy, cleanup); + return nb_type_put_p(type, type_p, ptr, policy, cleanup, is_new); } } diff --git a/include/nanobind/nb_enums.h b/include/nanobind/nb_enums.h index cae5dcba..5ae5e015 100644 --- a/include/nanobind/nb_enums.h +++ b/include/nanobind/nb_enums.h @@ -18,9 +18,24 @@ enum class rv_policy { move, reference, reference_internal, - none + none, /* Note to self: nb_func.h assumes that this value fits into 3 bits, hence no further policies can be added. */ + + /* Special internal-use policy that can only be passed to `nb_type_put()` + or `type_caster_base::from_cpp_raw()`. (To keep rv_policy fitting + in 3 bits, it aliases `automatic_reference`, which would always have + been converted into `copy` or `move` or `reference` before reaching + those functions.) It means we are creating a Python instance that holds + shared ownership of the C++ object we're casting, so we shouldn't reuse + a Python instance that only holds a reference to that object. The actual + enforcement of the shared ownership must be done separately after the + cast completes, via a keep_alive callback or similar, to ensure that + the C++ object lives at least as long as the new Python instance does. + + This is an internal tool that is not part of nanobind's public API, + and may be changed without notice. */ + _shared_ownership = automatic_reference }; NAMESPACE_END(NB_NAMESPACE) diff --git a/include/nanobind/stl/shared_ptr.h b/include/nanobind/stl/shared_ptr.h index a9a86542..7de5dfa4 100644 --- a/include/nanobind/stl/shared_ptr.h +++ b/include/nanobind/stl/shared_ptr.h @@ -102,23 +102,8 @@ template struct type_caster> { handle result; Td *ptr = (Td *) value.get(); - const std::type_info *type = &typeid(Td); - - constexpr bool has_type_hook = - !std::is_base_of_v>; - if constexpr (has_type_hook) - type = type_hook::get(ptr); - - if constexpr (!std::is_polymorphic_v) { - result = nb_type_put(type, ptr, rv_policy::reference, - cleanup, &is_new); - } else { - const std::type_info *type_p = - (!has_type_hook && ptr) ? &typeid(*ptr) : nullptr; - - result = nb_type_put_p(type, type_p, ptr, rv_policy::reference, - cleanup, &is_new); - } + result = Caster::from_cpp_raw(ptr, rv_policy::_shared_ownership, + cleanup, &is_new); if (is_new) { std::shared_ptr pp; diff --git a/src/nb_internals.h b/src/nb_internals.h index ca79920d..a70548e9 100644 --- a/src/nb_internals.h +++ b/src/nb_internals.h @@ -51,7 +51,10 @@ struct func_data : func_data_prelim<0> { struct nb_inst { // usually: 24 bytes PyObject_HEAD - /// Offset to the actual instance data + /// Offset to the actual C++ object. A value of zero here is special; + /// it means this instance was "detached" (assumed dangling) because its + /// C++ object address collided with a newly allocated internal instance. + /// Detached instances are not stored in the inst_c2p map. int32_t offset; /// State of the C++ object this instance points to: is it constructed? @@ -87,8 +90,11 @@ struct nb_inst { // usually: 24 bytes /// Does this instance use intrusive reference counting? uint32_t intrusive : 1; + /// Does this instance hold partial/shared ownership of its C++ object? + uint32_t shared_ownership : 1; + // That's a lot of unused space. I wonder if there is a good use for it.. - uint32_t unused : 24; + uint32_t unused : 23; }; static_assert(sizeof(nb_inst) == sizeof(PyObject) + sizeof(uint32_t) * 2); diff --git a/src/nb_type.cpp b/src/nb_type.cpp index f8807f1b..59eb0b72 100644 --- a/src/nb_type.cpp +++ b/src/nb_type.cpp @@ -98,6 +98,7 @@ PyObject *inst_new_int(PyTypeObject *tp, PyObject * /* args */, self->cpp_delete = 0; self->clear_keep_alive = 0; self->intrusive = intrusive; + self->shared_ownership = 0; self->unused = 0; // Make the object compatible with nb_try_inc_ref (free-threaded builds only) @@ -107,7 +108,45 @@ PyObject *inst_new_int(PyTypeObject *tp, PyObject * /* args */, nb_shard &shard = internals->shard((void *) payload); lock_shard guard(shard); auto [it, success] = shard.inst_c2p.try_emplace((void *) payload, self); - check(success, "nanobind::detail::inst_new_int(): unexpected collision!"); + if (NB_UNLIKELY(!success)) { + // Collision: multiple Python instances wrap the same C++ address. + // Since the C++ address that we're trying to insert is from a + // fresh heap allocation, all instances that collide with it + // must be dangling. + void *entry = it.value(); + nb_inst_seq single_seq; + nb_inst_seq *seq; + if (nb_is_seq(entry)) { + seq = nb_get_seq(entry); + } else { + single_seq.inst = (PyObject *) entry; + single_seq.next = nullptr; + seq = &single_seq; + } + while (seq) { + nb_inst *inst = (nb_inst *) seq->inst; + check(!inst->internal && !inst->destruct && !inst->cpp_delete, + "nanobind::detail::inst_new_int(): newly " + "created Python instance would destroy a C++ object " + "already owned by an existing Python instance"); + inst->state = nb_inst::state_relinquished; + + // Make this instance pretend its C++ object starts at + // the same byte as the Python instance. That's not + // possible, so we can use it as a sentinel for "this + // instance is not registered". We don't bother + // actually adding {inst, inst} to the inst_c2p map. + inst->direct = 1; + inst->offset = 0; + + nb_inst_seq *prev = seq; + seq = seq->next; + if (single_seq.inst == nullptr) { + PyMem_Free(prev); + } + } + it.value() = self; + } } return (PyObject *) self; @@ -168,6 +207,7 @@ PyObject *inst_new_ext(PyTypeObject *tp, void *value) { self->cpp_delete = 0; self->clear_keep_alive = 0; self->intrusive = intrusive; + self->shared_ownership = 0; self->unused = 0; // Make the object compatible with nb_try_inc_ref (free-threaded builds only) @@ -185,35 +225,48 @@ static void inst_register(PyObject *inst, void *value) noexcept { auto [it, success] = shard.inst_c2p.try_emplace(value, inst); if (NB_UNLIKELY(!success)) { - void *entry = it->second; - - // Potentially convert the map value into linked list format - if (!nb_is_seq(entry)) { - nb_inst_seq *first = (nb_inst_seq *) PyMem_Malloc(sizeof(nb_inst_seq)); - check(first, "nanobind::detail::inst_new_ext(): list element " - "allocation failed!"); - first->inst = (PyObject *) entry; - first->next = nullptr; - entry = it.value() = nb_mark_seq(first); - } - - nb_inst_seq *seq = nb_get_seq(entry); - while (true) { - // The following should never happen - check(inst != seq->inst, "nanobind::detail::inst_new_ext(): duplicate instance!"); - - if (!seq->next) - break; - seq = seq->next; - } - - nb_inst_seq *next = (nb_inst_seq *) PyMem_Malloc(sizeof(nb_inst_seq)); - check(next, + // Collision: multiple Python instances wrap the same C++ address. + // This is rare; nb_type_put() and friends try to locate an existing + // Python instance wrapping the returned C++ object's address before + // creating a new one. It can happen anyway under a few circumstances: + // - Subobjects: one instance wraps an object, and another wraps its + // offset-zero subobject (first member of a structure, etc). + // - Dangling: an existing instance refers to an object that has been + // deallocated, and now we're wrapping a new object that was created + // at that address. + // - Different return value policies: previously only a reference + // to the C++ object was passed to Python, but now we're passing + // exclusive or shared ownership. + // - Low-level interface: inst_reference() and inst_take_ownership() + // don't check for existing instances before they create a new one. + + nb_inst_seq *inst_node = (nb_inst_seq *) PyMem_Malloc(sizeof(nb_inst_seq)); + check(inst_node, "nanobind::detail::inst_new_ext(): list element allocation failed!"); + inst_node->inst = (PyObject *) inst; + + void *prev_entry = it->second; + nb_inst_seq *prev_node; + if (!nb_is_seq(prev_entry)) { + // Convert the map value into linked list format + prev_node = (nb_inst_seq *) PyMem_Malloc(sizeof(nb_inst_seq)); + check(prev_node, "nanobind::detail::inst_new_ext(): list element " + "allocation failed!"); + prev_node->inst = (PyObject *) prev_entry; + prev_node->next = nullptr; + } else { + // The map value is already in linked list format + prev_node = nb_get_seq(prev_entry); + } - next->inst = (PyObject *) inst; - next->next = nullptr; - seq->next = next; + // Insert the new instance at the head of the linked list of instances + // for this C++ object address. This choice is relevant in the + // "dangling" and "different return value policies" cases described + // above; we want future lookups in inst_c2p to prefer the more + // recently allocated/wrapped instance or the one that carries more + // ownership, so that they don't return a dangling one. + inst_node->next = prev_node; + it.value() = nb_mark_seq(inst_node); } } @@ -321,7 +374,10 @@ static void inst_dealloc(PyObject *self) { } } - check(found, + // A detached dangling instance will have offset == 0 and will not be + // recorded in the map. We expect these to be rare, so we check for them + // only at the last minute before we would assert-fail. + check(found || inst->offset == 0, "nanobind::detail::inst_dealloc(\"%s\"): attempted to delete an " "unknown instance (%p)!", t->name, p); } @@ -1670,11 +1726,21 @@ static PyObject *nb_type_put_common(void *value, type_data *t, rv_policy rvp, if (rvp == rv_policy::reference_internal && (!cleanup || !cleanup->self())) return nullptr; - const bool intrusive = t->flags & (uint32_t) type_flags::intrusive_ptr; - if (intrusive) + const bool create_new = rvp == rv_policy::copy || rvp == rv_policy::move; + const bool intrusive = !create_new && (t->flags & (uint32_t) type_flags::intrusive_ptr); + if (intrusive) { rvp = rv_policy::take_ownership; + } else { + /* Shared ownership must be maintained either externally, in which case + our caller would need to know if this is a new instance or not, or + intrusively. If it's neither of those, then it was probably intended + as intrusive but the annotation was inadvertently omitted. */ + check(rvp != rv_policy::_shared_ownership || is_new != nullptr, + "nanobind::detail::nb_type_put(\"%s\"): attempted to cast an " + "intrusive nanobind::ref<> to Python, but the intrusive_ptr() " + "class binding annotation was not specified!", t->name); + } - const bool create_new = rvp == rv_policy::copy || rvp == rv_policy::move; nb_inst *inst; if (create_new) @@ -1734,12 +1800,13 @@ static PyObject *nb_type_put_common(void *value, type_data *t, rv_policy rvp, // returning shared_ptr to Python explicitly. if ((t->flags & (uint32_t) type_flags::has_shared_from_this) && !create_new && t->keep_shared_from_this_alive((PyObject *) inst)) - rvp = rv_policy::reference; + rvp = rv_policy::_shared_ownership; else if (is_new) *is_new = true; - inst->destruct = rvp != rv_policy::reference && rvp != rv_policy::reference_internal; + inst->destruct = create_new || rvp == rv_policy::take_ownership; inst->cpp_delete = rvp == rv_policy::take_ownership; + inst->shared_ownership = rvp == rv_policy::_shared_ownership; inst->state = nb_inst::state_ready; if (rvp == rv_policy::reference_internal) @@ -1754,6 +1821,72 @@ static PyObject *nb_type_put_common(void *value, type_data *t, rv_policy rvp, return (PyObject *) inst; } +/* Returns whether `existing`, which shares a type and address with a C++ + object that's being cast to Python, is suitable to use as the result + of that cast. Assumes rvp != rv_policy::copy, since if we're making a + copy, we wouldn't have even checked for existing instances. */ +static bool nb_type_put_can_reuse(PyObject *existing_obj, rv_policy rvp, + cleanup_list *cleanup) { + nb_inst *existing = (nb_inst *) existing_obj; + if (rvp == rv_policy::take_ownership) { + /* If we want to transfer ownership to Python, don't use an existing + object that only holds a reference, because then no one would be left + holding the C++ ownership, i.e., the returned object would be leaked. + This handles the case where an object was exposed via + rv_policy::reference before it participated in an ownership transfer, + as well as the case where the "existing" object is actually a + dangling non-owning reference to a different object that previously + existed (and was freed) at the same address we're now trying to cast. + + Any object that was initialized with rv_policy::take_ownership will + have destruct set to true, unless it transferred its ownership to a + a C++ unique_ptr, in which case its state will be `relinquished`. + Testing destruct instead of cpp_delete avoids issues if a pointer + from a Python-constructed instance is cast using nb::cast(p) rather + than nb::find(p), as some users porting from pybind11 might do. + + We allow reuse of a shared_ownership instance also, in case the + type supports enable_shared_from_this which might "downgrade" + take_ownership to shared_ownership when creating a new object. + The reuse-existing path skips that logic, but it remains true + that an instance with shared_ownership set doesn't dangle. */ + return existing->shared_ownership || existing->destruct || + existing->state == nb_inst::state_relinquished; + } + if (rvp == rv_policy::_shared_ownership) { + /* If we're transferring shared ownership, don't use an existing object + that only holds a reference, because if the cast completes without + creating a new instance, our caller will assume the shared ownership + is already set up. (See the shared_ptr type caster.) This also avoids + reusing dangling instances, as in the take_ownership case. + A Python instance with `shared_ownership` or `destruct` set + can't dangle. Note that intrusive implies destruct. */ + return existing->shared_ownership || existing->destruct; + } + if (rvp == rv_policy::move) { + /* Don't move from an address that is already owned by a Python + instance; just return a new reference to that same Python instance. + There's no potential lifetime issue in that case. + But if the moved-from address is only referenced by the existing + Python instance, we should create a new instance, since the user + might have requested rv_policy::move to avoid a lifetime issue. */ + return existing->destruct; + } + if (rvp == rv_policy::reference_internal) { + /* Make sure the returned object is keeping the requested parent + alive. If there's no parent available, then don't reuse, so + that we eventually fail in nb_type_put_common(). */ + if (cleanup && cleanup->self() && nb_try_inc_ref(existing_obj)) { + // This is a no-op if the keep_alive relationship already exists: + keep_alive(existing_obj, cleanup->self()); + Py_DECREF(existing_obj); + } else { + return false; + } + } + return true; +} + PyObject *nb_type_put(const std::type_info *cpp_type, void *value, rv_policy rvp, cleanup_list *cleanup, @@ -1800,7 +1933,8 @@ PyObject *nb_type_put(const std::type_info *cpp_type, PyTypeObject *tp = Py_TYPE(seq.inst); if (nb_type_data(tp)->type == cpp_type) { - if (nb_try_inc_ref(seq.inst)) + if (nb_type_put_can_reuse(seq.inst, rvp, cleanup) && + nb_try_inc_ref(seq.inst)) return seq.inst; } @@ -1808,7 +1942,8 @@ PyObject *nb_type_put(const std::type_info *cpp_type, return nullptr; if (PyType_IsSubtype(tp, td->type_py)) { - if (nb_try_inc_ref(seq.inst)) + if (nb_type_put_can_reuse(seq.inst, rvp, cleanup) && + nb_try_inc_ref(seq.inst)) return seq.inst; } @@ -1817,7 +1952,9 @@ PyObject *nb_type_put(const std::type_info *cpp_type, seq = *seq.next; } - } else if (rvp == rv_policy::none) { + } + + if (rvp == rv_policy::none) { return nullptr; } } @@ -1882,11 +2019,11 @@ PyObject *nb_type_put_p(const std::type_info *cpp_type, while (true) { PyTypeObject *tp = Py_TYPE(seq.inst); - const std::type_info *p = nb_type_data(tp)->type; if (p == cpp_type || p == cpp_type_p) { - if (nb_try_inc_ref(seq.inst)) + if (nb_type_put_can_reuse(seq.inst, rvp, cleanup) && + nb_try_inc_ref(seq.inst)) return seq.inst; } @@ -1895,7 +2032,8 @@ PyObject *nb_type_put_p(const std::type_info *cpp_type, if (PyType_IsSubtype(tp, td->type_py) || (td_p && PyType_IsSubtype(tp, td_p->type_py))) { - if (nb_try_inc_ref(seq.inst)) + if (nb_type_put_can_reuse(seq.inst, rvp, cleanup) && + nb_try_inc_ref(seq.inst)) return seq.inst; } @@ -1904,7 +2042,9 @@ PyObject *nb_type_put_p(const std::type_info *cpp_type, seq = *seq.next; } - } else if (rvp == rv_policy::none) { + } + + if (rvp == rv_policy::none) { return nullptr; } }