From af2dda38ef01bddec4434450f76ebb5875f59df7 Mon Sep 17 00:00:00 2001 From: Dean Moldovan Date: Mon, 26 Jun 2017 20:34:06 +0200 Subject: [PATCH] Add a life support system for type_caster temporaries --- include/pybind11/cast.h | 59 ++++++++++++++++++++++++++++++++----- include/pybind11/common.h | 1 + include/pybind11/pybind11.h | 2 ++ tests/test_class.cpp | 34 +++++++++++++++++++++ tests/test_class.py | 8 +++++ 5 files changed, 97 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index d34afbc2a..27bd05359 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -111,6 +111,53 @@ PYBIND11_NOINLINE inline internals &get_internals() { return *internals_ptr; } +/// A life support system for temporary objects created by `type_caster::load()`. +/// Adding a patient will keep it alive up until the enclosing function returns. +class loader_life_support { +public: + /// A new patient frame is created when a function is entered + loader_life_support() { + get_internals().loader_patient_stack.push_back(nullptr); + } + + /// ... and destroyed after it returns + ~loader_life_support() { + auto &stack = get_internals().loader_patient_stack; + if (stack.empty()) + pybind11_fail("loader_life_support: internal error"); + + auto ptr = stack.back(); + stack.pop_back(); + Py_CLEAR(ptr); + + // A heuristic to reduce the stack's capacity (e.g. after long recursive calls) + if (stack.capacity() > 16 && stack.size() != 0 && stack.capacity() / stack.size() > 2) + stack.shrink_to_fit(); + } + + /// This can only be used inside a pybind11-bound function, either by `argument_loader` + /// at argument preparation time or by `py::cast()` at execution time. + PYBIND11_NOINLINE static void add_patient(handle h) { + auto &stack = get_internals().loader_patient_stack; + if (stack.empty()) + throw cast_error("When called outside a bound function, py::cast() cannot " + "do Python -> C++ conversions which require the creation " + "of temporary values"); + + auto &list_ptr = stack.back(); + if (list_ptr == nullptr) { + list_ptr = PyList_New(1); + if (!list_ptr) + pybind11_fail("loader_life_support: error allocating list"); + PyList_SET_ITEM(list_ptr, 0, h.inc_ref().ptr()); + } else { + auto result = PyList_Append(list_ptr, h.ptr()); + if (result == -1) + pybind11_fail("loader_life_support: error adding patient"); + } + } +}; + // Gets the cache entry for the given type, creating it if necessary. The return value is the pair // returned by emplace, i.e. an iterator for the entry and a bool set to `true` if the entry was // just created. @@ -643,9 +690,11 @@ protected: // Perform an implicit conversion if (convert) { for (auto &converter : typeinfo->implicit_conversions) { - temp = reinterpret_steal(converter(src.ptr(), typeinfo->type)); - if (load_impl(temp, false)) + auto temp = reinterpret_steal(converter(src.ptr(), typeinfo->type)); + if (load_impl(temp, false)) { + loader_life_support::add_patient(temp); return true; + } } if (this_.try_direct_conversions(src)) return true; @@ -674,7 +723,6 @@ protected: const type_info *typeinfo = nullptr; void *value = nullptr; - object temp; }; /** @@ -1064,7 +1112,7 @@ template struct string_caster { // If we're loading a string_view we need to keep the encoded Python object alive: if (IsView) - view_into = std::move(utfNbytes); + loader_life_support::add_patient(utfNbytes); return true; } @@ -1080,8 +1128,6 @@ template struct string_caster { PYBIND11_TYPE_CASTER(StringType, _(PYBIND11_STRING_NAME)); private: - object view_into; - static handle decode_utfN(const char *buffer, ssize_t nbytes) { #if !defined(PYPY_VERSION) return @@ -1336,7 +1382,6 @@ public: using base::cast; using base::typeinfo; using base::value; - using base::temp; bool load(handle src, bool convert) { return base::template load_impl>(src, convert); diff --git a/include/pybind11/common.h b/include/pybind11/common.h index 626178e4f..af0a8d0e3 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -474,6 +474,7 @@ struct internals { std::unordered_map> patients; std::forward_list registered_exception_translators; std::unordered_map shared_data; // Custom data to be shared across extensions + std::vector loader_patient_stack; // Used by `loader_life_support` PyTypeObject *static_property_type; PyTypeObject *default_metaclass; PyObject *instance_base; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 973dea924..e92290367 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -573,6 +573,7 @@ protected: // 6. Call the function. try { + loader_life_support guard{}; result = func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; @@ -601,6 +602,7 @@ protected: // The no-conversion pass finished without success, try again with conversion allowed for (auto &call : second_pass) { try { + loader_life_support guard{}; result = call.func.impl(call); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 4e0dd6237..f616ba771 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -150,6 +150,40 @@ TEST_SUBMODULE(class_, m) { py::class_(m, "MyDerived") .def_static("make", &MyDerived::make) .def_static("make2", &MyDerived::make); + + // test_implicit_conversion_life_support + struct ConvertibleFromUserType { + int i; + + ConvertibleFromUserType(UserType u) : i(u.value()) { } + }; + + py::class_(m, "AcceptsUserType") + .def(py::init()); + py::implicitly_convertible(); + + m.def("implicitly_convert_argument", [](const ConvertibleFromUserType &r) { return r.i; }); + m.def("implicitly_convert_variable", [](py::object o) { + // `o` is `UserType` and `r` is a reference to a temporary created by implicit + // conversion. This is valid when called inside a bound function because the temp + // object is attached to the same life support system as the arguments. + const auto &r = o.cast(); + return r.i; + }); + m.add_object("implicitly_convert_variable_fail", [&] { + auto f = [](PyObject *, PyObject *args) -> PyObject * { + auto o = py::reinterpret_borrow(args)[0]; + try { // It should fail here because there is no life support. + o.cast(); + } catch (const py::cast_error &e) { + return py::str(e.what()).release().ptr(); + } + return py::str().release().ptr(); + }; + + auto def = new PyMethodDef{"f", f, METH_VARARGS, nullptr}; + return py::reinterpret_steal(PyCFunction_NewEx(def, nullptr, m.ptr())); + }()); } template class BreaksBase {}; diff --git a/tests/test_class.py b/tests/test_class.py index 13c778f22..611a2870e 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -119,3 +119,11 @@ def test_override_static(): assert isinstance(b, m.MyBase) assert isinstance(d1, m.MyDerived) assert isinstance(d2, m.MyDerived) + + +def test_implicit_conversion_life_support(): + """Ensure the lifetime of temporary objects created for implicit conversions""" + assert m.implicitly_convert_argument(UserType(5)) == 5 + assert m.implicitly_convert_variable(UserType(5)) == 5 + + assert "outside a bound function" in m.implicitly_convert_variable_fail(UserType(5))