From c07ec31edf5c032da8d22155ac98edbecba51aa7 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sun, 6 Nov 2016 13:12:48 -0500 Subject: [PATCH] Don't construct unique_ptr around unowned pointers (#478) If we need to initialize a holder around an unowned instance, and the holder type is non-copyable (i.e. a unique_ptr), we currently construct the holder type around the value pointer, but then never actually destruct the holder: the holder destructor is called only for the instance that actually has `inst->owned = true` set. This seems no pointer, however, in creating such a holder around an unowned instance: we never actually intend to use anything that the unique_ptr gives us: and, in fact, do not want the unique_ptr (because if it ever actually got destroyed, it would cause destruction of the wrapped pointer, despite the fact that that wrapped pointer isn't owned). This commit changes the logic to only create a unique_ptr holder if we actually own the instance, and to destruct via the constructed holder whenever we have a constructed holder--which will now only be the case for owned-unique-holder or shared-holder types. Other changes include: * Added test for non-movable holder constructor/destructor counts The three alive assertions now pass, before #478 they fail with counts of 2/2/1 respectively, because of the unique_ptr that we don't want and don't destroy (because we don't *want* its destructor to run). * Return cstats reference; fix ConstructStats doc Small cleanup to the #478 test code, and fix to the ConstructStats documentation (the static method definition should use `reference` not `reference_internal`). * Rename inst->constructed to inst->holder_constructed This makes it clearer exactly what it's referring to. --- include/pybind11/common.h | 2 +- include/pybind11/pybind11.h | 23 ++++++++++++----------- tests/constructor_stats.h | 2 +- tests/test_issues.cpp | 37 +++++++++++++++++++++++++++++++++++++ tests/test_issues.py | 17 +++++++++++++++++ 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/include/pybind11/common.h b/include/pybind11/common.h index e684b187e..c422081e1 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -311,7 +311,7 @@ template struct instance_essentials { type *value; PyObject *weakrefs; bool owned : 1; - bool constructed : 1; + bool holder_constructed : 1; }; /// PyObject wrapper around generic types, includes a special holder type that is responsible for lifetime management diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d845d18a7..93f55ef96 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -821,7 +821,7 @@ protected: auto tinfo = detail::get_type_info(type); self->value = ::operator new(tinfo->type_size); self->owned = true; - self->constructed = false; + self->holder_constructed = false; detail::get_internals().registered_instances.emplace(self->value, (PyObject *) self); return (PyObject *) self; } @@ -1134,7 +1134,7 @@ private: } catch (const std::bad_weak_ptr &) { new (&inst->holder) holder_type(inst->value); } - inst->owned = true; + inst->holder_constructed = true; } /// Initialize holder object, variant 2: try to construct from existing holder object, if possible @@ -1145,31 +1145,32 @@ private: new (&inst->holder) holder_type(*holder_ptr); else new (&inst->holder) holder_type(inst->value); - inst->owned = true; + inst->holder_constructed = true; } /// Initialize holder object, variant 3: holder is not copy constructible (e.g. unique_ptr), always initialize from raw pointer template ::value, int> = 0> static void init_holder_helper(instance_type *inst, const holder_type * /* unused */, const void * /* dummy */) { - new (&inst->holder) holder_type(inst->value); + if (inst->owned) { + new (&inst->holder) holder_type(inst->value); + inst->holder_constructed = true; + } } /// Initialize holder object of an instance, possibly given a pointer to an existing holder static void init_holder(PyObject *inst_, const void *holder_ptr) { auto inst = (instance_type *) inst_; init_holder_helper(inst, (const holder_type *) holder_ptr, inst->value); - inst->constructed = true; } static void dealloc(PyObject *inst_) { instance_type *inst = (instance_type *) inst_; - if (inst->owned) { - if (inst->constructed) - inst->holder.~holder_type(); - else - ::operator delete(inst->value); - } + if (inst->holder_constructed) + inst->holder.~holder_type(); + else if (inst->owned) + ::operator delete(inst->value); + generic_type::dealloc((detail::instance *) inst); } diff --git a/tests/constructor_stats.h b/tests/constructor_stats.h index 5dd215f19..eb3e49cab 100644 --- a/tests/constructor_stats.h +++ b/tests/constructor_stats.h @@ -56,7 +56,7 @@ from the ConstructorStats instance `.values()` method. In some cases, when you need to track instances of a C++ class not registered with pybind11, you need to add a function returning the ConstructorStats for the C++ class; this can be done with: - m.def("get_special_cstats", &ConstructorStats::get, py::return_value_policy::reference_internal) + m.def("get_special_cstats", &ConstructorStats::get, py::return_value_policy::reference) Finally, you can suppress the output messages, but keep the constructor tracking (for inspection/testing in python) by using the functions with `print_` replaced with `track_` (e.g. diff --git a/tests/test_issues.cpp b/tests/test_issues.cpp index 362ad448b..1374afeb7 100644 --- a/tests/test_issues.cpp +++ b/tests/test_issues.cpp @@ -48,6 +48,24 @@ class Dupe2 {}; class Dupe3 {}; class DupeException : public std::runtime_error {}; +// #478 +template class custom_unique_ptr { +public: + custom_unique_ptr() { print_default_created(this); } + custom_unique_ptr(T *ptr) : _ptr{ptr} { print_created(this, ptr); } + custom_unique_ptr(custom_unique_ptr &&move) : _ptr{move._ptr} { move._ptr = nullptr; print_move_created(this); } + custom_unique_ptr &operator=(custom_unique_ptr &&move) { print_move_assigned(this); if (_ptr) destruct_ptr(); _ptr = move._ptr; move._ptr = nullptr; return *this; } + custom_unique_ptr(const custom_unique_ptr &) = delete; + void operator=(const custom_unique_ptr ©) = delete; + ~custom_unique_ptr() { print_destroyed(this); if (_ptr) destruct_ptr(); } +private: + T *_ptr = nullptr; + void destruct_ptr() { delete _ptr; } +}; +PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr); + + + void init_issues(py::module &m) { py::module m2 = m.def_submodule("issues"); @@ -311,6 +329,25 @@ void init_issues(py::module &m) { py::class_>(m, "SharedParent") .def(py::init<>()) .def("get_child", &SharedParent::get_child, py::return_value_policy::reference); + + /// Issue/PR #478: unique ptrs constructed and freed without destruction + class SpecialHolderObj { + public: + int val = 0; + SpecialHolderObj *ch = nullptr; + SpecialHolderObj(int v, bool make_child = true) : val{v}, ch{make_child ? new SpecialHolderObj(val+1, false) : nullptr} + { print_created(this, val); } + ~SpecialHolderObj() { delete ch; print_destroyed(this); } + SpecialHolderObj *child() { return ch; } + }; + + py::class_>(m, "SpecialHolderObj") + .def(py::init()) + .def("child", &SpecialHolderObj::child, pybind11::return_value_policy::reference_internal) + .def_readwrite("val", &SpecialHolderObj::val) + .def_static("holder_cstats", &ConstructorStats::get>, + py::return_value_policy::reference) + ; }; diff --git a/tests/test_issues.py b/tests/test_issues.py index 208cf3b41..6f84f777b 100644 --- a/tests/test_issues.py +++ b/tests/test_issues.py @@ -201,3 +201,20 @@ def test_enable_shared_from_this_with_reference_rvp(): assert cstats.alive() == 1 del child, parent assert cstats.alive() == 0 + +def test_non_destructed_holders(): + """ Issue #478: unique ptrs constructed and freed without destruction """ + from pybind11_tests import SpecialHolderObj + + a = SpecialHolderObj(123) + b = a.child() + + assert a.val == 123 + assert b.val == 124 + + cstats = SpecialHolderObj.holder_cstats() + assert cstats.alive() == 1 + del b + assert cstats.alive() == 1 + del a + assert cstats.alive() == 0