Adding virtual_overrider_self_life_support. (#2902)

* Initial version of virtual_overrider_self_life_support (enables safely passing unique_ptr to C++).

* Clang 3.6, 3.7 compatibility.

* Adding missing default constructor.

* Restoring test for exception for the case that virtual_overrider_self_life_support is not used.

* Fixing oversight: Adding missing holder().ensure_was_not_disowned().

* Adding unit tests for new `struct smart_holder` member functions.

* Moving virtual_overrider_self_life_support to separate include file, with iwyu cleanup.
This commit is contained in:
Ralf W. Grosse-Kunstleve 2021-03-16 06:31:24 -07:00 committed by GitHub
parent d6cf6dfed3
commit 469792032a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 225 additions and 58 deletions

View File

@ -110,6 +110,7 @@ set(PYBIND11_HEADERS
include/pybind11/detail/smart_holder_type_casters.h
include/pybind11/detail/type_caster_base.h
include/pybind11/detail/typeid.h
include/pybind11/detail/virtual_overrider_self_life_support.h
include/pybind11/attr.h
include/pybind11/buffer_info.h
include/pybind11/cast.h

View File

@ -95,13 +95,14 @@ inline bool is_std_default_delete(const std::type_info &rtti_deleter) {
}
struct smart_holder {
const std::type_info *rtti_uqp_del;
const std::type_info *rtti_uqp_del = nullptr;
std::shared_ptr<bool> vptr_deleter_armed_flag_ptr;
std::shared_ptr<void> vptr;
bool vptr_is_using_noop_deleter : 1;
bool vptr_is_using_builtin_delete : 1;
bool vptr_is_external_shared_ptr : 1;
bool is_populated : 1;
bool was_disowned : 1;
bool pointee_depends_on_holder_owner : 1; // SMART_HOLDER_WIP: See PR #2839.
// Design choice: smart_holder is movable but not copyable.
@ -111,15 +112,15 @@ struct smart_holder {
smart_holder &operator=(const smart_holder &) = delete;
smart_holder()
: rtti_uqp_del{nullptr}, vptr_is_using_noop_deleter{false},
vptr_is_using_builtin_delete{false}, vptr_is_external_shared_ptr{false},
is_populated{false}, pointee_depends_on_holder_owner{false} {}
: vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
pointee_depends_on_holder_owner{false} {}
explicit smart_holder(bool vptr_deleter_armed_flag)
: rtti_uqp_del{nullptr}, vptr_deleter_armed_flag_ptr{new bool{vptr_deleter_armed_flag}},
: vptr_deleter_armed_flag_ptr{new bool{vptr_deleter_armed_flag}},
vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
vptr_is_external_shared_ptr{false}, is_populated{false}, pointee_depends_on_holder_owner{
false} {}
vptr_is_external_shared_ptr{false}, is_populated{false}, was_disowned{false},
pointee_depends_on_holder_owner{false} {}
bool has_pointee() const { return vptr.get() != nullptr; }
@ -135,6 +136,12 @@ struct smart_holder {
throw std::runtime_error(std::string("Unpopulated holder (") + context + ").");
}
}
void ensure_was_not_disowned(const char *context) const {
if (was_disowned) {
throw std::runtime_error(std::string("Holder was disowned already (") + context
+ ").");
}
}
void ensure_vptr_is_using_builtin_delete(const char *context) const {
if (vptr_is_external_shared_ptr) {
@ -227,16 +234,29 @@ struct smart_holder {
return hld;
}
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
void disown() {
*vptr_deleter_armed_flag_ptr = false;
was_disowned = true;
}
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
void release_disowned() {
vptr.reset();
vptr_deleter_armed_flag_ptr.reset();
}
// SMART_HOLDER_WIP: review this function.
void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") {
ensure_was_not_disowned(context);
ensure_vptr_is_using_builtin_delete(context);
ensure_use_count_1(context);
}
// Caller is responsible for calling ensure_can_release_ownership().
// Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
void release_ownership() {
*vptr_deleter_armed_flag_ptr = false;
vptr.reset();
vptr_deleter_armed_flag_ptr.reset();
release_disowned();
}
template <typename T>

View File

@ -13,6 +13,7 @@
#include "smart_holder_sfinae_hooks_only.h"
#include "type_caster_base.h"
#include "typeid.h"
#include "virtual_overrider_self_life_support.h"
#include <cstddef>
#include <memory>
@ -352,6 +353,7 @@ struct smart_holder_type_caster_load {
if (!have_holder())
return nullptr;
throw_if_uninitialized_or_disowned_holder();
holder().ensure_was_not_disowned("loaded_as_shared_ptr");
auto void_raw_ptr = holder().template as_raw_ptr_unowned<void>();
auto type_raw_ptr = convert_type(void_raw_ptr);
if (holder().pointee_depends_on_holder_owner) {
@ -373,28 +375,44 @@ struct smart_holder_type_caster_load {
if (!have_holder())
return nullptr;
throw_if_uninitialized_or_disowned_holder();
holder().ensure_was_not_disowned(context);
holder().template ensure_compatible_rtti_uqp_del<T, D>(context);
holder().ensure_use_count_1(context);
if (holder().pointee_depends_on_holder_owner) {
throw value_error("Ownership of instance with virtual overrides in Python cannot be "
"transferred to C++.");
}
auto raw_void_ptr = holder().template as_raw_ptr_unowned<void>();
// SMART_HOLDER_WIP: MISSING: Safety checks for type conversions
// (T must be polymorphic or meet certain other conditions).
T *raw_type_ptr = convert_type(raw_void_ptr);
// Critical transfer-of-ownership section. This must stay together.
holder().release_ownership();
auto result = std::unique_ptr<T, D>(raw_type_ptr);
void *value_void_ptr = load_impl.loaded_v_h.value_ptr();
if (value_void_ptr != raw_void_ptr) {
pybind11_fail("smart_holder_type_casters: loaded_as_unique_ptr failure:"
" value_void_ptr != raw_void_ptr");
}
load_impl.loaded_v_h.value_ptr() = nullptr;
deregister_instance(load_impl.loaded_v_h.inst, value_void_ptr, load_impl.loaded_v_h.type);
// SMART_HOLDER_WIP: MISSING: Safety checks for type conversions
// (T must be polymorphic or meet certain other conditions).
T *raw_type_ptr = convert_type(raw_void_ptr);
auto *self_life_support
= dynamic_cast_virtual_overrider_self_life_support_ptr(raw_type_ptr);
if (self_life_support == nullptr && holder().pointee_depends_on_holder_owner) {
throw value_error("Ownership of instance with virtual overrides in Python cannot be "
"transferred to C++.");
}
// Critical transfer-of-ownership section. This must stay together.
if (self_life_support != nullptr) {
holder().disown();
} else {
holder().release_ownership();
}
auto result = std::unique_ptr<T, D>(raw_type_ptr);
if (self_life_support != nullptr) {
Py_INCREF((PyObject *) load_impl.loaded_v_h.inst);
self_life_support->loaded_v_h = load_impl.loaded_v_h;
} else {
load_impl.loaded_v_h.value_ptr() = nullptr;
deregister_instance(
load_impl.loaded_v_h.inst, value_void_ptr, load_impl.loaded_v_h.type);
}
// Critical section end.
return result;
}

View File

@ -0,0 +1,62 @@
// 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.
#pragma once
#include "common.h"
#include "smart_holder_poc.h"
#include "type_caster_base.h"
#include <type_traits>
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)
// SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code.
inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
// The original core idea for this struct goes back to PyCLIF:
// https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37
// URL provided here mainly to give proper credit. To fully explain the `HoldPyObj` feature, more
// context is needed (SMART_HOLDER_WIP).
struct virtual_overrider_self_life_support {
value_and_holder loaded_v_h;
~virtual_overrider_self_life_support() {
if (loaded_v_h.inst != nullptr && loaded_v_h.vh != nullptr) {
void *value_void_ptr = loaded_v_h.value_ptr();
if (value_void_ptr != nullptr) {
PyGILState_STATE threadstate = PyGILState_Ensure();
Py_DECREF((PyObject *) loaded_v_h.inst);
loaded_v_h.value_ptr() = nullptr;
loaded_v_h.holder<pybindit::memory::smart_holder>().release_disowned();
deregister_instance(loaded_v_h.inst, value_void_ptr, loaded_v_h.type);
PyGILState_Release(threadstate);
}
}
}
// Some compilers complain about implicitly defined versions of some of the following:
virtual_overrider_self_life_support() = default;
virtual_overrider_self_life_support(const virtual_overrider_self_life_support &) = default;
virtual_overrider_self_life_support(virtual_overrider_self_life_support &&) = default;
virtual_overrider_self_life_support &operator=(const virtual_overrider_self_life_support &)
= default;
virtual_overrider_self_life_support &operator=(virtual_overrider_self_life_support &&)
= default;
};
template <typename T, detail::enable_if_t<!std::is_polymorphic<T>::value, int> = 0>
virtual_overrider_self_life_support *
dynamic_cast_virtual_overrider_self_life_support_ptr(T * /*raw_type_ptr*/) {
return nullptr;
}
template <typename T, detail::enable_if_t<std::is_polymorphic<T>::value, int> = 0>
virtual_overrider_self_life_support *
dynamic_cast_virtual_overrider_self_life_support_ptr(T *raw_type_ptr) {
return dynamic_cast<virtual_overrider_self_life_support *>(raw_type_ptr);
}
PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

View File

@ -48,6 +48,7 @@ detail_headers = {
"include/pybind11/detail/smart_holder_type_casters.h",
"include/pybind11/detail/type_caster_base.h",
"include/pybind11/detail/typeid.h",
"include/pybind11/detail/virtual_overrider_self_life_support.h",
}
cmake_files = {

View File

@ -129,6 +129,26 @@ TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
REQUIRE(*new_owner == 19);
}
TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
hld.disown();
REQUIRE(hld.as_lvalue_ref<int>() == 19);
REQUIRE(*new_owner == 19);
hld.release_disowned();
REQUIRE(!hld.has_pointee());
}
TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_was_not_disowned", "[E]") {
const char *context = "test_case";
auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
hld.ensure_was_not_disowned(context); // Does not throw.
std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
hld.disown();
REQUIRE_THROWS_WITH(hld.ensure_was_not_disowned(context),
"Holder was disowned already (test_case).");
}
TEST_CASE("from_unique_ptr+as_lvalue_ref", "[S]") {
std::unique_ptr<int> orig_owner(new int(19));
auto hld = smart_holder::from_unique_ptr(std::move(orig_owner));

View File

@ -7,6 +7,7 @@
namespace pybind11_tests {
namespace test_class_sh_with_alias {
template <int SerNo> // Using int as a trick to easily generate a series of types.
struct Abase {
int val = 0;
virtual ~Abase() = default;
@ -21,41 +22,65 @@ struct Abase {
Abase &operator=(Abase &&) = default;
};
struct AbaseAlias : Abase {
using Abase::Abase;
template <int SerNo>
struct AbaseAlias : Abase<SerNo> {
using Abase<SerNo>::Abase;
int Add(int other_val) const override {
PYBIND11_OVERRIDE_PURE(int, /* Return type */
Abase, /* Parent class */
Add, /* Name of function in C++ (must match Python name) */
PYBIND11_OVERRIDE_PURE(int, /* Return type */
Abase<SerNo>, /* Parent class */
Add, /* Name of function in C++ (must match Python name) */
other_val);
}
};
int AddInCppRawPtr(const Abase *obj, int other_val) { return obj->Add(other_val) * 10 + 7; }
template <>
struct AbaseAlias<1> : Abase<1>, py::detail::virtual_overrider_self_life_support {
using Abase<1>::Abase;
int AddInCppSharedPtr(std::shared_ptr<Abase> obj, int other_val) {
int Add(int other_val) const override {
PYBIND11_OVERRIDE_PURE(int, /* Return type */
Abase<1>, /* Parent class */
Add, /* Name of function in C++ (must match Python name) */
other_val);
}
};
template <int SerNo>
int AddInCppRawPtr(const Abase<SerNo> *obj, int other_val) {
return obj->Add(other_val) * 10 + 7;
}
template <int SerNo>
int AddInCppSharedPtr(std::shared_ptr<Abase<SerNo>> obj, int other_val) {
return obj->Add(other_val) * 100 + 11;
}
int AddInCppUniquePtr(std::unique_ptr<Abase> obj, int other_val) {
template <int SerNo>
int AddInCppUniquePtr(std::unique_ptr<Abase<SerNo>> obj, int other_val) {
return obj->Add(other_val) * 100 + 13;
}
template <int SerNo>
void wrap(py::module_ m, const char *py_class_name) {
py::classh<Abase<SerNo>, AbaseAlias<SerNo>>(m, py_class_name)
.def(py::init<int>(), py::arg("val"))
.def("Get", &Abase<SerNo>::Get)
.def("Add", &Abase<SerNo>::Add, py::arg("other_val"));
m.def("AddInCppRawPtr", AddInCppRawPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
m.def("AddInCppSharedPtr", AddInCppSharedPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
m.def("AddInCppUniquePtr", AddInCppUniquePtr<SerNo>, py::arg("obj"), py::arg("other_val"));
}
} // namespace test_class_sh_with_alias
} // namespace pybind11_tests
PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase)
PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<0>)
PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_with_alias::Abase<1>)
TEST_SUBMODULE(class_sh_with_alias, m) {
using namespace pybind11_tests::test_class_sh_with_alias;
py::classh<Abase, AbaseAlias>(m, "Abase")
.def(py::init<int>(), py::arg("val"))
.def("Get", &Abase::Get)
.def("Add", &Abase::Add, py::arg("other_val"));
m.def("AddInCppRawPtr", AddInCppRawPtr, py::arg("obj"), py::arg("other_val"));
m.def("AddInCppSharedPtr", AddInCppSharedPtr, py::arg("obj"), py::arg("other_val"));
m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val"));
wrap<0>(m, "Abase0");
wrap<1>(m, "Abase1");
}

View File

@ -4,34 +4,54 @@ import pytest
from pybind11_tests import class_sh_with_alias as m
class PyDrvd(m.Abase):
class PyDrvd0(m.Abase0):
def __init__(self, val):
super(PyDrvd, self).__init__(val)
super(PyDrvd0, self).__init__(val)
def Add(self, other_val): # noqa: N802
return self.Get() * 100 + other_val
def test_drvd_add():
drvd = PyDrvd(74)
class PyDrvd1(m.Abase1):
def __init__(self, val):
super(PyDrvd1, self).__init__(val)
def Add(self, other_val): # noqa: N802
return self.Get() * 200 + other_val
def test_drvd0_add():
drvd = PyDrvd0(74)
assert drvd.Add(38) == (74 * 10 + 3) * 100 + 38
def test_add_in_cpp_raw_ptr():
drvd = PyDrvd(52)
def test_drvd0_add_in_cpp_raw_ptr():
drvd = PyDrvd0(52)
assert m.AddInCppRawPtr(drvd, 27) == ((52 * 10 + 3) * 100 + 27) * 10 + 7
def test_add_in_cpp_shared_ptr():
drvd = PyDrvd(36)
assert m.AddInCppSharedPtr(drvd, 56) == ((36 * 10 + 3) * 100 + 56) * 100 + 11
def test_drvd0_add_in_cpp_shared_ptr():
while True:
drvd = PyDrvd0(36)
assert m.AddInCppSharedPtr(drvd, 56) == ((36 * 10 + 3) * 100 + 56) * 100 + 11
return # Comment out for manual leak checking (use `top` command).
def test_add_in_cpp_unique_ptr():
drvd = PyDrvd(0)
with pytest.raises(ValueError) as exc_info:
m.AddInCppUniquePtr(drvd, 0)
assert (
str(exc_info.value)
== "Ownership of instance with virtual overrides in Python cannot be transferred to C++."
)
def test_drvd0_add_in_cpp_unique_ptr():
while True:
drvd = PyDrvd0(0)
with pytest.raises(ValueError) as exc_info:
m.AddInCppUniquePtr(drvd, 0)
assert (
str(exc_info.value)
== "Ownership of instance with virtual overrides in Python"
" cannot be transferred to C++."
)
return # Comment out for manual leak checking (use `top` command).
def test_drvd1_add_in_cpp_unique_ptr():
while True:
drvd = PyDrvd1(25)
assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13
return # Comment out for manual leak checking (use `top` command).