diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 15d62086e..b4137cb2b 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -216,8 +216,8 @@ struct type_record { /// The global operator new can be overridden with a class-specific variant void *(*operator_new)(size_t) = ::operator new; - /// Function pointer to class_<..>::init_holder - void (*init_holder)(instance *, const void *) = nullptr; + /// Function pointer to class_<..>::init_instance + void (*init_instance)(instance *, const void *) = nullptr; /// Function pointer to class_<..>::dealloc void (*dealloc)(const detail::value_and_holder &) = nullptr; diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8c8de5b83..9c171aeda 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -44,7 +44,7 @@ struct type_info { const std::type_info *cpptype; size_t type_size, holder_size_in_ptrs; void *(*operator_new)(size_t); - void (*init_holder)(instance *, const void *); + void (*init_instance)(instance *, const void *); void (*dealloc)(const value_and_holder &v_h); std::vector implicit_conversions; std::vector> implicit_casts; @@ -515,7 +515,6 @@ inline PyThreadState *get_thread_state_unchecked() { // Forward declarations inline void keep_alive_impl(handle nurse, handle patient); -inline void register_instance(instance *self, void *valptr, const type_info *tinfo); inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value = true); class type_caster_generic { @@ -595,8 +594,7 @@ public: throw cast_error("unhandled return_value_policy: should not happen!"); } - register_instance(wrapper, valueptr, tinfo); - tinfo->init_holder(wrapper, existing_holder); + tinfo->init_instance(wrapper, existing_holder); return inst.release(); } diff --git a/include/pybind11/class_support.h b/include/pybind11/class_support.h index 9516bd646..aee159146 100644 --- a/include/pybind11/class_support.h +++ b/include/pybind11/class_support.h @@ -234,9 +234,8 @@ inline bool deregister_instance(instance *self, void *valptr, const type_info *t /// Instance creation function for all pybind11 types. It only allocates space for the C++ object /// (or multiple objects, for Python-side inheritance from multiple pybind11 types), but doesn't -/// call the constructor -- an `__init__` function must do that. If allocating value, the instance -/// is registered; otherwise register_instance will need to be called once the value has been -/// assigned. +/// call the constructor -- an `__init__` function must do that. `register_instance` will need to +/// be called after the object has been initialized. inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value /*= true (in cast.h)*/) { #if defined(PYPY_VERSION) // PyPy gets tp_basicsize wrong (issue 2482) under multiple inheritance when the first inherited @@ -257,7 +256,6 @@ inline PyObject *make_new_instance(PyTypeObject *type, bool allocate_value /*= t for (auto &v_h : values_and_holders(inst)) { void *&vptr = v_h.value_ptr(); vptr = v_h.type->operator_new(v_h.type->type_size); - register_instance(inst, vptr, v_h.type); } } @@ -316,11 +314,12 @@ inline void clear_instance(PyObject *self) { // Deallocate any values/holders, if present: for (auto &v_h : values_and_holders(instance)) { if (v_h) { + // This is allowed to fail (the type might have been allocated but not + // initialized/registered): + deregister_instance(instance, v_h.value_ptr(), v_h.type); + if (instance->owned || v_h.holder_constructed()) v_h.type->dealloc(v_h); - - if (!deregister_instance(instance, v_h.value_ptr(), v_h.type)) - pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!"); } } // Deallocate the value/holder layout internals: diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 25824362d..079d35457 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -714,7 +714,7 @@ protected: } else { if (overloads->is_constructor) { auto tinfo = get_type_info((PyTypeObject *) overloads->scope.ptr()); - tinfo->init_holder(reinterpret_cast(parent.ptr()), nullptr); + tinfo->init_instance(reinterpret_cast(parent.ptr()), nullptr); } return result.ptr(); } @@ -835,7 +835,7 @@ protected: tinfo->type_size = rec.type_size; tinfo->operator_new = rec.operator_new; tinfo->holder_size_in_ptrs = size_in_ptrs(rec.holder_size); - tinfo->init_holder = rec.init_holder; + tinfo->init_instance = rec.init_instance; tinfo->dealloc = rec.dealloc; tinfo->simple_type = true; tinfo->simple_ancestors = true; @@ -971,7 +971,7 @@ public: record.type = &typeid(type); record.type_size = sizeof(conditional_t); record.holder_size = sizeof(holder_type); - record.init_holder = init_holder; + record.init_instance = init_instance; record.dealloc = dealloc; record.default_holder = std::is_same>::value; @@ -1170,7 +1170,7 @@ public: private: /// Initialize holder object, variant 1: object derives from enable_shared_from_this template - static void init_holder_helper(detail::instance *inst, detail::value_and_holder &v_h, + static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { try { auto sh = std::dynamic_pointer_cast( @@ -1198,7 +1198,7 @@ private: } /// Initialize holder object, variant 2: try to construct from existing holder object, if possible - static void init_holder_helper(detail::instance *inst, detail::value_and_holder &v_h, + static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this) */) { if (holder_ptr) { init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible()); @@ -1209,10 +1209,14 @@ private: } } - /// Initialize holder object of an instance, possibly given a pointer to an existing holder - static void init_holder(detail::instance *inst, const void *holder_ptr) { + /// Performs instance initialization including constructing a holder and registering the known + /// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes an + /// 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) { auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); - init_holder_helper(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); + register_instance(inst, v_h.value_ptr(), v_h.type); + init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); } /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. diff --git a/tests/test_call_policies.py b/tests/test_call_policies.py index de4ec9690..42d0ffd7b 100644 --- a/tests/test_call_policies.py +++ b/tests/test_call_policies.py @@ -116,7 +116,9 @@ def test_alive_gc_multi_derived(capture): from pybind11_tests import Parent, Child, ConstructorStats class Derived(Parent, Child): - pass + def __init__(self): + Parent.__init__(self) + Child.__init__(self) n_inst = ConstructorStats.detail_reg_inst() p = Derived() @@ -131,6 +133,7 @@ def test_alive_gc_multi_derived(capture): assert capture == """ Releasing parent. Releasing child. + Releasing child. """ diff --git a/tests/test_multiple_inheritance.cpp b/tests/test_multiple_inheritance.cpp index c52b991aa..35f9d9c4e 100644 --- a/tests/test_multiple_inheritance.cpp +++ b/tests/test_multiple_inheritance.cpp @@ -201,4 +201,20 @@ TEST_SUBMODULE(multiple_inheritance, m) { py::class_(m, "VanillaDictMix1").def(py::init<>()); py::class_(m, "VanillaDictMix2").def(py::init<>()); #endif + + // test_diamond_inheritance + // Issue #959: segfault when constructing diamond inheritance instance + // All of these have int members so that there will be various unequal pointers involved. + struct B { int b; virtual ~B() = default; }; + struct C0 : public virtual B { int c0; }; + struct C1 : public virtual B { int c1; }; + struct D : public C0, public C1 { int d; }; + py::class_(m, "B") + .def("b", [](B *self) { return self; }); + py::class_(m, "C0") + .def("c0", [](C0 *self) { return self; }); + py::class_(m, "C1") + .def("c1", [](C1 *self) { return self; }); + py::class_(m, "D") + .def(py::init<>()); } diff --git a/tests/test_multiple_inheritance.py b/tests/test_multiple_inheritance.py index 7121c81ab..434f477ae 100644 --- a/tests/test_multiple_inheritance.py +++ b/tests/test_multiple_inheritance.py @@ -331,3 +331,18 @@ def test_mi_base_return(): e2 = m.i801e_b2() assert type(e2) is m.I801B2 assert e2.b == 2 + + +def test_diamond_inheritance(): + """Tests that diamond inheritance works as expected (issue #959)""" + + # Issue #959: this shouldn't segfault: + d = m.D() + + # Make sure all the various distinct pointers are all recognized as registered instances: + assert d is d.c0() + assert d is d.c1() + assert d is d.b() + assert d is d.c0().b() + assert d is d.c1().b() + assert d is d.c0().c1().b().c0().b()