Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model relinquished instances as a separate state from not-ready #591

Merged
merged 1 commit into from
May 23, 2024
Merged

Model relinquished instances as a separate state from not-ready #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/nb_func.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ static PyObject *nb_func_vectorcall_complex(PyObject *self,
if (is_constructor) {
nb_inst *self_arg_nb = (nb_inst *) self_arg;
self_arg_nb->destruct = true;
self_arg_nb->ready = true;
self_arg_nb->state = nb_inst::state_ready;
if (NB_UNLIKELY(self_arg_nb->intrusive))
nb_type_data(Py_TYPE(self_arg))
->set_self_py(inst_ptr(self_arg_nb), self_arg);
Expand Down Expand Up @@ -845,7 +845,7 @@ static PyObject *nb_func_vectorcall_simple(PyObject *self,
if (is_constructor) {
nb_inst *self_arg_nb = (nb_inst *) self_arg;
self_arg_nb->destruct = true;
self_arg_nb->ready = true;
self_arg_nb->state = nb_inst::state_ready;
if (NB_UNLIKELY(self_arg_nb->intrusive))
nb_type_data(Py_TYPE(self_arg))
->set_self_py(inst_ptr(self_arg_nb), self_arg);
Expand Down
15 changes: 11 additions & 4 deletions src/nb_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,16 @@ struct nb_inst { // usually: 24 bytes
/// Offset to the actual instance data
int32_t offset;

/// State of the C++ object this instance points to: is it constructed?
/// can we use it?
uint32_t state : 2;

// Values for `state`. Note that the numeric values of these are relied upon
// for an optimization in `nb_type_get()`.
static constexpr uint32_t state_uninitialized = 0; // not constructed
static constexpr uint32_t state_relinquished = 1; // owned by C++, don't touch
static constexpr uint32_t state_ready = 2; // constructed and usable

/**
* The variable 'offset' can either encode an offset relative to the
* nb_inst address that leads to the instance data, or it can encode a
Expand All @@ -62,9 +72,6 @@ struct nb_inst { // usually: 24 bytes
/// Is the instance data co-located with the Python object?
uint32_t internal : 1;

/// Is the instance properly initialized?
uint32_t ready : 1;

/// Should the destructor be called when this instance is GCed?
uint32_t destruct : 1;

Expand All @@ -78,7 +85,7 @@ struct nb_inst { // usually: 24 bytes
uint32_t intrusive : 1;

// That's a lot of unused space. I wonder if there is a good use for it..
uint32_t unused: 25;
uint32_t unused : 24;
};

static_assert(sizeof(nb_inst) == sizeof(PyObject) + sizeof(uint32_t) * 2);
Expand Down
81 changes: 54 additions & 27 deletions src/nb_type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ PyObject *inst_new_int(PyTypeObject *tp) {
self->offset = (int32_t) ((intptr_t) payload - (intptr_t) self);
self->direct = 1;
self->internal = 1;
self->ready = 0;
self->state = nb_inst::state_uninitialized;
self->destruct = 0;
self->cpp_delete = 0;
self->clear_keep_alive = 0;
Expand Down Expand Up @@ -148,7 +148,7 @@ PyObject *inst_new_ext(PyTypeObject *tp, void *value) {
self->offset = offset;
self->direct = direct;
self->internal = 0;
self->ready = 0;
self->state = nb_inst::state_uninitialized;
self->destruct = 0;
self->cpp_delete = 0;
self->clear_keep_alive = 0;
Expand Down Expand Up @@ -1242,13 +1242,28 @@ bool nb_type_get(const std::type_info *cpp_type, PyObject *src, uint8_t flags,
if (NB_LIKELY(valid)) {
nb_inst *inst = (nb_inst *) src;

if (NB_UNLIKELY(((flags & (uint8_t) cast_flags::construct) != 0) == (bool) inst->ready)) {
static_assert(cast_flags::construct == nb_inst::state_ready,
"this function is optimized assuming that "
"cast_flags::construct == nb_inst::state_ready");

// (flags & construct) state xor-result should accept?
// [normal] 0 [uninit] 0 0 no
// [normal] 0 [relinq] 1 1 no
// [normal] 0 [ready] 2 2 yes
// [construct] 2 [uninit] 0 2 yes
// [construct] 2 [relinq] 1 3 no
// [construct] 2 [ready] 2 0 no

if (NB_UNLIKELY(((flags & (uint8_t) cast_flags::construct) ^ inst->state) != nb_inst::state_ready)) {
constexpr const char* errors[4] = {
/* 0=uninit */ "attempted to access an uninitialized instance",
/* 1=relinq */ "attempted to access a relinquished instance",
/* 2=ready */ "attempted to initialize an already-initialized instance",
/* 3=invalid */ "instance state has become corrupted",
};
PyErr_WarnFormat(
PyExc_RuntimeWarning, 1, "nanobind: %s of type '%s'!\n",
inst->ready
? "attempted to initialize an already-initialized instance"
: "attempted to access an uninitialized instance",
t->name);
errors[inst->state], t->name);
return false;
}

Expand Down Expand Up @@ -1436,7 +1451,7 @@ static PyObject *nb_type_put_common(void *value, type_data *t, rv_policy rvp,

inst->destruct = rvp != rv_policy::reference && rvp != rv_policy::reference_internal;
inst->cpp_delete = rvp == rv_policy::take_ownership;
inst->ready = true;
inst->state = nb_inst::state_ready;

if (rvp == rv_policy::reference_internal)
keep_alive((PyObject *) inst, cleanup->self());
Expand Down Expand Up @@ -1615,20 +1630,23 @@ static void nb_type_put_unique_finalize(PyObject *o,
nb_inst *inst = (nb_inst *) o;

if (cpp_delete) {
check((bool) inst->ready == is_new && (bool) inst->destruct == is_new &&
check(inst->state == (is_new ? nb_inst::state_ready
: nb_inst::state_relinquished) &&
(bool) inst->destruct == is_new &&
(bool) inst->cpp_delete == is_new,
"nanobind::detail::nb_type_put_unique(type='%s', cpp_delete=%i): "
"unexpected status flags! (ready=%i, destruct=%i, cpp_delete=%i)",
type_name(cpp_type), cpp_delete, inst->ready, inst->destruct,
"unexpected status flags! (state=%i, destruct=%i, cpp_delete=%i)",
type_name(cpp_type), cpp_delete, inst->state, inst->destruct,
inst->cpp_delete);

inst->ready = inst->destruct = inst->cpp_delete = true;
inst->state = nb_inst::state_ready;
inst->destruct = inst->cpp_delete = true;
} else {
check(!inst->ready,
check(inst->state == nb_inst::state_relinquished,
"nanobind::detail::nb_type_put_unique('%s'): ownership "
"status has become corrupted.", type_name(cpp_type));

inst->ready = true;
inst->state = nb_inst::state_ready;
}
}

Expand Down Expand Up @@ -1683,7 +1701,7 @@ bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept {
the same data structure. For example, converting Python (foo, foo) to C++
std::pair<std::unique_ptr<T>, std::unique_ptr<T>>. */

if (!inst->ready) {
if (inst->state != nb_inst::state_ready) {
warn_relinquish_failed(
"The resulting data structure would have multiple "
"std::unique_ptrs, each thinking that they own the same instance, "
Expand All @@ -1706,19 +1724,19 @@ bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept {
inst->destruct = false;
}

inst->ready = false;
inst->state = nb_inst::state_relinquished;
return true;
}

void nb_type_restore_ownership(PyObject *o, bool cpp_delete) noexcept {
nb_inst *inst = (nb_inst *) o;

check(!inst->ready,
check(inst->state == nb_inst::state_relinquished,
"nanobind::detail::nb_type_restore_ownership('%s'): ownership "
"status has become corrupted.",
PyUnicode_AsUTF8AndSize(nb_inst_name(o), nullptr));

inst->ready = true;
inst->state = nb_inst::state_ready;
if (cpp_delete) {
inst->cpp_delete = true;
inst->destruct = true;
Expand Down Expand Up @@ -1777,7 +1795,7 @@ PyObject *nb_inst_reference(PyTypeObject *t, void *ptr, PyObject *parent) {
raise_python_error();
nb_inst *nbi = (nb_inst *) result;
nbi->destruct = nbi->cpp_delete = false;
nbi->ready = true;
nbi->state = nb_inst::state_ready;
if (parent)
keep_alive(result, parent);
return result;
Expand All @@ -1789,7 +1807,7 @@ PyObject *nb_inst_take_ownership(PyTypeObject *t, void *ptr) {
raise_python_error();
nb_inst *nbi = (nb_inst *) result;
nbi->destruct = nbi->cpp_delete = true;
nbi->ready = true;
nbi->state = nb_inst::state_ready;
return result;
}

Expand All @@ -1801,7 +1819,8 @@ void nb_inst_zero(PyObject *o) noexcept {
nb_inst *nbi = (nb_inst *) o;
type_data *td = nb_type_data(Py_TYPE(o));
memset(inst_ptr(nbi), 0, td->size);
nbi->ready = nbi->destruct = true;
nbi->state = nb_inst::state_ready;
nbi->destruct = true;
}

PyObject *nb_inst_alloc_zero(PyTypeObject *t) {
Expand All @@ -1811,26 +1830,32 @@ PyObject *nb_inst_alloc_zero(PyTypeObject *t) {
nb_inst *nbi = (nb_inst *) result;
type_data *td = nb_type_data(t);
memset(inst_ptr(nbi), 0, td->size);
nbi->ready = nbi->destruct = true;
nbi->state = nb_inst::state_ready;
nbi->destruct = true;
return result;
}

void nb_inst_set_state(PyObject *o, bool ready, bool destruct) noexcept {
nb_inst *nbi = (nb_inst *) o;
nbi->ready = ready;
nbi->state = ready ? nb_inst::state_ready : nb_inst::state_uninitialized;
nbi->destruct = destruct;
nbi->cpp_delete = destruct && !nbi->internal;
}

std::pair<bool, bool> nb_inst_state(PyObject *o) noexcept {
nb_inst *nbi = (nb_inst *) o;
return { (bool) nbi->ready, (bool) nbi->destruct };
return { nbi->state == nb_inst::state_ready, (bool) nbi->destruct };
}

void nb_inst_destruct(PyObject *o) noexcept {
nb_inst *nbi = (nb_inst *) o;
type_data *t = nb_type_data(Py_TYPE(o));

check(nbi->state != nb_inst::state_relinquished,
"nanobind::detail::nb_inst_destruct(\"%s\"): attempted to destroy "
"an object whose ownership had been transferred away!",
t->name);

if (nbi->destruct) {
check(t->flags & (uint32_t) type_flags::is_destructible,
"nanobind::detail::nb_inst_destruct(\"%s\"): attempted to call "
Expand All @@ -1841,7 +1866,7 @@ void nb_inst_destruct(PyObject *o) noexcept {
nbi->destruct = false;
}

nbi->ready = false;
nbi->state = nb_inst::state_uninitialized;
}

void nb_inst_copy(PyObject *dst, const PyObject *src) noexcept {
Expand All @@ -1864,7 +1889,8 @@ void nb_inst_copy(PyObject *dst, const PyObject *src) noexcept {
else
memcpy(dst_data, src_data, t->size);

nbi->ready = nbi->destruct = true;
nbi->state = nb_inst::state_ready;
nbi->destruct = true;
}

void nb_inst_move(PyObject *dst, const PyObject *src) noexcept {
Expand All @@ -1889,7 +1915,8 @@ void nb_inst_move(PyObject *dst, const PyObject *src) noexcept {
memset(src_data, 0, t->size);
}

nbi->ready = nbi->destruct = true;
nbi->state = nb_inst::state_ready;
nbi->destruct = true;
}

void nb_inst_replace_move(PyObject *dst, const PyObject *src) noexcept {
Expand Down
21 changes: 17 additions & 4 deletions tests/test_holders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,11 @@ def test05a_uniqueptr_from_cpp(clean):
b = t.unique_from_cpp_2()
wa = t.UniqueWrapper(a)
wb = t.UniqueWrapper(b)
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'):
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError) as excinfo:
assert a.value == 1
assert 'incompatible function arguments' in str(excinfo.value)
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'):
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError) as excinfo:
assert b.value == 2
assert 'incompatible function arguments' in str(excinfo.value)
Expand Down Expand Up @@ -133,7 +133,7 @@ def test05b_uniqueptr_list(clean):
res = t.passthrough_unique_pairs([(k, v)], clear=True)
assert res == []
for obj in (k, v):
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'):
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError) as excinfo:
obj.value
collect()
Expand All @@ -154,6 +154,19 @@ def test05c_uniqueptr_structure_duplicate(clean):
collect()
assert t.stats() == (1, 1)

def test05d_uniqueptr_reinit(clean):
x = t.unique_from_cpp()
assert x.value == 1
w = t.UniqueWrapper(x)
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError):
x.value
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError):
x.__init__(3)
y = w.get()
assert y is x and y.value == 1


def test06_uniqueptr_from_py(clean):
# Test ownership exchange when the object has been created on the Python side
Expand All @@ -162,7 +175,7 @@ def test06_uniqueptr_from_py(clean):
with pytest.raises(TypeError) as excinfo:
wa = t.UniqueWrapper(a)
wa = t.UniqueWrapper2(a)
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'):
with pytest.warns(RuntimeWarning, match='nanobind: attempted to access a relinquished instance of type \'test_holders_ext.Example\'!'):
with pytest.raises(TypeError) as excinfo:
assert a.value == 1
assert 'incompatible function arguments' in str(excinfo.value)
Expand Down