From c02188212bfe0b8258f37d638b0f36f45977778c Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 10:07:46 -0400 Subject: [PATCH 1/7] Add a is_except attr that can be applied to py:class_ --- include/pybind11/attr.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 60ed9fd90e..e87c21cb13 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -78,6 +78,9 @@ struct arithmetic { }; /// Mark a function for addition at the beginning of the existing overload chain instead of the end struct prepend { }; +// Annotation to mark a class as needing an exception base type. +struct is_except {}; + /** \rst A call policy which places one or more guard variables (``Ts...``) around the function call. @@ -222,7 +225,8 @@ struct function_record { struct type_record { PYBIND11_NOINLINE type_record() : multiple_inheritance(false), dynamic_attr(false), buffer_protocol(false), - default_holder(true), module_local(false), is_final(false) { } + default_holder(true), module_local(false), is_final(false), + is_except(false) { } /// Handle to the parent scope handle scope; @@ -278,6 +282,9 @@ struct type_record { /// Is the class inheritable from python classes? bool is_final : 1; + // Does the class need an exception base type? + bool is_except : 1; + PYBIND11_NOINLINE void add_base(const std::type_info &base, void *(*caster)(void *)) { auto base_info = detail::get_type_info(base, false); if (!base_info) { @@ -469,6 +476,11 @@ struct process_attribute : process_attribute_default { static void init(const is_final &, type_record *r) { r->is_final = true; } }; +template <> +struct process_attribute : process_attribute_default { + static void init(const is_except &, type_record *r) { r->is_except = true; } +}; + template <> struct process_attribute : process_attribute_default { static void init(const buffer_protocol &, type_record *r) { r->buffer_protocol = true; } From 31f0fc58bf8e1802259c5a8ea89359e56232c308 Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 10:09:18 -0400 Subject: [PATCH 2/7] Update make_object_base_type to take an exception flag And add a separate base class into the internals for when an extension is being created/extended. --- include/pybind11/detail/internals.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index a621aed2a6..225448a90d 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -20,7 +20,7 @@ PYBIND11_NAMESPACE_BEGIN(detail) // Forward declarations inline PyTypeObject *make_static_property_type(); inline PyTypeObject *make_default_metaclass(); -inline PyObject *make_object_base_type(PyTypeObject *metaclass); +inline PyObject *make_object_base_type(PyTypeObject *metaclass, bool is_except); // The old Python Thread Local Storage (TLS) API is deprecated in Python 3.7 in favor of the new // Thread Specific Storage (TSS) API. @@ -111,6 +111,7 @@ struct internals { PyTypeObject *static_property_type; PyTypeObject *default_metaclass; PyObject *instance_base; + PyObject *exception_base; #if defined(WITH_THREAD) PYBIND11_TLS_KEY_INIT(tstate); PyInterpreterState *istate = nullptr; @@ -312,7 +313,8 @@ PYBIND11_NOINLINE inline internals &get_internals() { internals_ptr->registered_exception_translators.push_front(&translate_exception); internals_ptr->static_property_type = make_static_property_type(); internals_ptr->default_metaclass = make_default_metaclass(); - internals_ptr->instance_base = make_object_base_type(internals_ptr->default_metaclass); + internals_ptr->instance_base = make_object_base_type(internals_ptr->default_metaclass, false); + internals_ptr->exception_base = make_object_base_type(internals_ptr->default_metaclass, true); } return **internals_pp; } From 923cbfed87cadbd1cc734094ce675fbfbfce9a2d Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 10:12:27 -0400 Subject: [PATCH 3/7] Update make_object_base_type implementation to handle exceptions When the exception flag is passed, we need to inherit from PyExc_Exception instead of from PyBaseObject_Type. Since PyExc_Exception isn't recognized as a PyTypeObject, we have to cast it. Since exceptions support dynamic attributes, we have to make sure that GC is turned on for classes which are following that class path. Update make_new_python_type to choose the exception_base type when is_except has been set. --- include/pybind11/detail/class.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 15e09165de..9385ff8ba3 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -438,7 +438,7 @@ extern "C" inline void pybind11_object_dealloc(PyObject *self) { /** Create the type which can be used as a common base for all classes. This is needed in order to satisfy Python's requirements for multiple inheritance. Return value: New reference. */ -inline PyObject *make_object_base_type(PyTypeObject *metaclass) { +inline PyObject *make_object_base_type(PyTypeObject *metaclass, bool is_except=false) { constexpr auto *name = "pybind11_object"; auto name_obj = reinterpret_steal(PYBIND11_FROM_STRING(name)); @@ -457,7 +457,12 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass) { auto type = &heap_type->ht_type; type->tp_name = name; - type->tp_base = type_incref(&PyBaseObject_Type); + if (is_except) { + type->tp_base = type_incref(reinterpret_cast(PyExc_Exception)); + } + else { + type->tp_base = type_incref(&PyBaseObject_Type); + } type->tp_basicsize = static_cast(sizeof(instance)); type->tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HEAPTYPE; @@ -474,7 +479,9 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass) { setattr((PyObject *) type, "__module__", str("pybind11_builtins")); PYBIND11_SET_OLDPY_QUALNAME(type, name_obj); - assert(!PyType_HasFeature(type, Py_TPFLAGS_HAVE_GC)); + if (!is_except) { + assert(!PyType_HasFeature(type, Py_TPFLAGS_HAVE_GC)); + } return (PyObject *) heap_type; } @@ -630,8 +637,9 @@ inline PyObject* make_new_python_type(const type_record &rec) { auto &internals = get_internals(); auto bases = tuple(rec.bases); - auto base = (bases.empty()) ? internals.instance_base - : bases[0].ptr(); + auto base = (bases.empty()) ? (rec.is_except ? internals.exception_base + : internals.instance_base) + : bases[0].ptr(); /* Danger zone: from now (and until PyType_Ready), make sure to issue no Python C API calls which could potentially invoke the From ffe3fff72fc2b4daf9679a0957bb63e1f89da1ba Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 10:14:06 -0400 Subject: [PATCH 4/7] Add the extract PyObject fields to support extending Exceptions Any pybind11 type that extends PyExc_Exception has to have a instance struct that contains the same initial layout. And in pybind11 currently, the instance struct just has PyObject_HEAD. That means if you don't change the instance struct, this will all compile, but when python seeks into this object, it will do with the assumption that those extra fields exist and then it will seek right off the end of viable memory and you'll get all sorts of fun segfaults. So this change adds those extra fields to every class_ in pybind11. It does not seem to break normal classes to have these extra fields and it definitely seems to make exceptions work correctly. I am not super happy about this approach, so consider a proof of concept that should probably be improved. --- include/pybind11/detail/common.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 3faf3ea327..6edb329d90 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -445,6 +445,10 @@ struct nonsimple_values_and_holders { /// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof') struct instance { PyObject_HEAD + // Necessary to support exceptions. + PyObject *dict; + PyObject *args; + PyObject *message; /// Storage for pointers and holder; see simple_layout, below, for a description union { void *simple_value_holder[1 + instance_simple_holder_in_ptrs()]; From af9e0e1912ed397470208f93555009a78e4de97c Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 10:55:32 -0400 Subject: [PATCH 5/7] Add unit tests showing basic exception as class workflow --- tests/test_exceptions.cpp | 37 ++++++++++++++++++++++++++++++++----- tests/test_exceptions.py | 15 +++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index e28f0bb798..6d8b69f417 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -88,7 +88,6 @@ struct PythonCallInDestructor { }; - struct PythonAlreadySetInDestructor { PythonAlreadySetInDestructor(const py::str &s) : s(s) {} ~PythonAlreadySetInDestructor() { @@ -106,7 +105,30 @@ struct PythonAlreadySetInDestructor { }; +// An Exception that is going to be bound as a py:class_ +class BoundException : public std::exception { +public: + BoundException(const std::string& m, int e) : message{m}, m_errorCode(e) {} + virtual const char * what() const noexcept override {return message.c_str();} + int errorCode() const noexcept { return m_errorCode;} +private: + std::string message; + int m_errorCode; +}; + + + TEST_SUBMODULE(exceptions, m) { + // Provide a class binding for BoundException. This is providing the + // dynamic_attr because the parent type (PyExc_Exception) permits dynamic + // attributes. One improvement might be to make is_except force + // dynamic_attr + auto PyBoundException = py::class_< BoundException>(m, "BoundException", py::is_except(), py::dynamic_attr()) + .def(py::init< std::string, int>()) + .def("getErrorCode", &BoundException::errorCode) + .def("getMessage", &BoundException::what) + .def_property_readonly("message", &BoundException::what); + m.def("throw_std_exception", []() { throw std::runtime_error("This exception was intentionally thrown."); }); @@ -150,15 +172,19 @@ TEST_SUBMODULE(exceptions, m) { // A slightly more complicated one that declares MyException5_1 as a subclass of MyException5 py::register_exception(m, "MyException5_1", ex5.ptr()); - //py::register_local_exception(m, "LocalSimpleException") + py::register_local_exception(m, "LocalSimpleException"); - py::register_local_exception_translator([](std::exception_ptr p) { + // An exception translator for the exception that is bound as a class + // This is using PyErr_SetObject instead of PyErr_SetString. + py::register_exception_translator([](std::exception_ptr p) { try { if (p) { std::rethrow_exception(p); } - } catch (const MyException6 &e) { - PyErr_SetString(PyExc_RuntimeError, e.what()); + } catch (const BoundException &e) { + auto err = py::cast(e); + auto errType = err.get_type().ptr(); + PyErr_SetObject(errType, err.ptr()); } }); @@ -173,6 +199,7 @@ TEST_SUBMODULE(exceptions, m) { m.def("throws_overflow_error", []() { throw std::overflow_error(""); }); m.def("throws_local_error", []() { throw LocalException("never caught"); }); m.def("throws_local_simple_error", []() { throw LocalSimpleException("this mod"); }); + m.def("throws_bound_exception", []() { throw BoundException("this error is a class", 42); }); m.def("exception_matches", []() { py::dict foo; try { diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 966ae07fc0..7d1c90e199 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -248,3 +248,18 @@ def test_local_translator(msg): m.throws_local_simple_error() assert not isinstance(excinfo.value, cm.LocalSimpleException) assert msg(excinfo.value) == "this mod" + + +def test_bound_exceptions(): + """Tests throwing/catching an exception bound as a class""" + try: + m.throws_bound_exception() + except m.BoundException as ex: + assert ex.message == "this error is a class" + assert ex.getErrorCode() == 42 + + try: + raise m.BoundException("raising from python", 14) + except m.BoundException as ex: + assert ex.message == "raising from python" + assert ex.getErrorCode() == 14 From a52634a9cf8fa79f55453cad4fde056370b86e59 Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Mon, 27 Jul 2020 11:00:04 -0400 Subject: [PATCH 6/7] Update unit tests to make flake8 happy --- tests/test_exceptions.cpp | 2 +- tests/test_exceptions.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 6d8b69f417..3c304213fb 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -127,7 +127,7 @@ TEST_SUBMODULE(exceptions, m) { .def(py::init< std::string, int>()) .def("getErrorCode", &BoundException::errorCode) .def("getMessage", &BoundException::what) - .def_property_readonly("message", &BoundException::what); + .def("__str__", &BoundException::what); m.def("throw_std_exception", []() { throw std::runtime_error("This exception was intentionally thrown."); diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 7d1c90e199..89ebaa6388 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -255,11 +255,11 @@ def test_bound_exceptions(): try: m.throws_bound_exception() except m.BoundException as ex: - assert ex.message == "this error is a class" + assert str(ex) == "this error is a class" assert ex.getErrorCode() == 42 try: raise m.BoundException("raising from python", 14) except m.BoundException as ex: - assert ex.message == "raising from python" + assert str(ex) == "raising from python" assert ex.getErrorCode() == 14 From ae8757c39a4488fd534d38efd68ff2f0440e246a Mon Sep 17 00:00:00 2001 From: Jesse Clemens Date: Thu, 20 Aug 2020 15:43:35 -0400 Subject: [PATCH 7/7] Add the correct exception HEAD for python 3 --- include/pybind11/detail/common.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 6edb329d90..e0da44b89e 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -444,11 +444,16 @@ struct nonsimple_values_and_holders { /// The 'instance' type which needs to be standard layout (need to be able to use 'offsetof') struct instance { +#if (PY_MAJOR_VERSION == 3) + PyException_HEAD +#else PyObject_HEAD // Necessary to support exceptions. PyObject *dict; PyObject *args; PyObject *message; +#endif + /// Storage for pointers and holder; see simple_layout, below, for a description union { void *simple_value_holder[1 + instance_simple_holder_in_ptrs()];