Skip to content

Commit

Permalink
Model relinquished instances as a separate state from not-ready
Browse files Browse the repository at this point in the history
  • Loading branch information
@oremanj @wjakob
oremanj authored and wjakob committed May 23, 2024
1 parent fa059d7 commit 16a48d2
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 37 deletions.
4 changes: 2 additions & 2 deletions src/nb_func.cpp
View file
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
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
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
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

0 comments on commit 16a48d2

Please sign in to comment.