diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index e52ec1d3e..1ddf1393b 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -455,6 +455,8 @@ extern "C" inline void pybind11_object_dealloc(PyObject *self) { #endif } +std::string error_string(); + /** 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. */ @@ -490,7 +492,7 @@ inline PyObject *make_object_base_type(PyTypeObject *metaclass) { type->tp_weaklistoffset = offsetof(instance, weakrefs); if (PyType_Ready(type) < 0) { - pybind11_fail("PyType_Ready failed in make_object_base_type():" + error_string()); + pybind11_fail("PyType_Ready failed in make_object_base_type(): " + error_string()); } setattr((PyObject *) type, "__module__", str("pybind11_builtins")); @@ -707,7 +709,7 @@ inline PyObject *make_new_python_type(const type_record &rec) { } if (PyType_Ready(type) < 0) { - pybind11_fail(std::string(rec.name) + ": PyType_Ready failed (" + error_string() + ")!"); + pybind11_fail(std::string(rec.name) + ": PyType_Ready failed: " + error_string()); } assert(!rec.dynamic_attr || PyType_HasFeature(type, Py_TPFLAGS_HAVE_GC)); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index dd8d03751..21f69c289 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -470,73 +470,6 @@ PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) return isinstance(obj, type); } -PYBIND11_NOINLINE std::string error_string(const char *called) { - error_scope scope; // Fetch error state (will be restored when this function returns). - if (scope.type == nullptr) { - if (called == nullptr) { - called = "pybind11::detail::error_string()"; - } - pybind11_fail("Internal error: " + std::string(called) - + " called while Python error indicator not set."); - } - - PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace); - if (scope.trace != nullptr) { - PyException_SetTraceback(scope.value, scope.trace); - } - - std::string errorString; - if (scope.type) { - errorString += handle(scope.type).attr("__name__").cast(); - errorString += ": "; - } - if (scope.value) { - errorString += (std::string) str(scope.value); - } - -#if !defined(PYPY_VERSION) - if (scope.trace) { - auto *trace = (PyTracebackObject *) scope.trace; - - /* Get the deepest trace possible */ - while (trace->tb_next) { - trace = trace->tb_next; - } - - PyFrameObject *frame = trace->tb_frame; - Py_XINCREF(frame); - errorString += "\n\nAt:\n"; - while (frame) { -# if PY_VERSION_HEX >= 0x030900B1 - PyCodeObject *f_code = PyFrame_GetCode(frame); -# else - PyCodeObject *f_code = frame->f_code; - Py_INCREF(f_code); -# endif - int lineno = PyFrame_GetLineNumber(frame); - errorString += " "; - errorString += handle(f_code->co_filename).cast(); - errorString += '('; - errorString += std::to_string(lineno); - errorString += "): "; - errorString += handle(f_code->co_name).cast(); - errorString += '\n'; - Py_DECREF(f_code); -# if PY_VERSION_HEX >= 0x030900B1 - auto *b_frame = PyFrame_GetBack(frame); -# else - auto *b_frame = frame->f_back; - Py_XINCREF(b_frame); -# endif - Py_DECREF(frame); - frame = b_frame; - } - } -#endif - - return errorString; -} - PYBIND11_NOINLINE handle get_object_handle(const void *ptr, const detail::type_info *type) { auto &instances = get_internals().registered_instances; auto range = instances.equal_range(ptr); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index cfa442067..d61dcd5c7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2625,17 +2625,21 @@ void print(Args &&...args) { detail::print(c.args(), c.kwargs()); } -error_already_set::~error_already_set() { - if (m_type) { - gil_scoped_acquire gil; - error_scope scope; - m_type.release().dec_ref(); - m_value.release().dec_ref(); - m_trace.release().dec_ref(); - } +inline void +error_already_set::m_fetched_error_deleter(detail::error_fetch_and_normalize *raw_ptr) { + gil_scoped_acquire gil; + error_scope scope; + delete raw_ptr; +} + +inline const char *error_already_set::what() const noexcept { + gil_scoped_acquire gil; + error_scope scope; + return m_fetched_error->error_string().c_str(); } PYBIND11_NAMESPACE_BEGIN(detail) + inline function get_type_override(const void *this_ptr, const type_info *this_type, const char *name) { handle self = get_object_handle(this_ptr, this_type); diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 27807953b..f9625e77e 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -12,7 +12,15 @@ #include "detail/common.h" #include "buffer_info.h" +#include +#include +#include +#include +#include +#include +#include #include +#include #include #if defined(PYBIND11_HAS_OPTIONAL) @@ -383,7 +391,175 @@ T reinterpret_steal(handle h) { } PYBIND11_NAMESPACE_BEGIN(detail) -std::string error_string(const char *called = nullptr); + +// Equivalent to obj.__class__.__name__ (or obj.__name__ if obj is a class). +inline const char *obj_class_name(PyObject *obj) { + if (Py_TYPE(obj) == &PyType_Type) { + return reinterpret_cast(obj)->tp_name; + } + return Py_TYPE(obj)->tp_name; +} + +std::string error_string(); + +struct error_fetch_and_normalize { + // Immediate normalization is long-established behavior (starting with + // https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011 + // from Sep 2016) and safest. Normalization could be deferred, but this could mask + // errors elsewhere, the performance gain is very minor in typical situations + // (usually the dominant bottleneck is EH unwinding), and the implementation here + // would be more complex. + explicit error_fetch_and_normalize(const char *called) { + PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr()); + if (!m_type) { + pybind11_fail("Internal error: " + std::string(called) + + " called while " + "Python error indicator not set."); + } + const char *exc_type_name_orig = detail::obj_class_name(m_type.ptr()); + if (exc_type_name_orig == nullptr) { + pybind11_fail("Internal error: " + std::string(called) + + " failed to obtain the name " + "of the original active exception type."); + } + m_lazy_error_string = exc_type_name_orig; + // PyErr_NormalizeException() may change the exception type if there are cascading + // failures. This can potentially be extremely confusing. + PyErr_NormalizeException(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr()); + if (m_type.ptr() == nullptr) { + pybind11_fail("Internal error: " + std::string(called) + + " failed to normalize the " + "active exception."); + } + const char *exc_type_name_norm = detail::obj_class_name(m_type.ptr()); + if (exc_type_name_orig == nullptr) { + pybind11_fail("Internal error: " + std::string(called) + + " failed to obtain the name " + "of the normalized active exception type."); + } + if (exc_type_name_norm != m_lazy_error_string) { + std::string msg = std::string(called) + + ": MISMATCH of original and normalized " + "active exception types: "; + msg += "ORIGINAL "; + msg += m_lazy_error_string; + msg += " REPLACED BY "; + msg += exc_type_name_norm; + msg += ": " + format_value_and_trace(); + pybind11_fail(msg); + } + } + + error_fetch_and_normalize(const error_fetch_and_normalize &) = delete; + error_fetch_and_normalize(error_fetch_and_normalize &&) = delete; + + std::string format_value_and_trace() const { + std::string result; + std::string message_error_string; + if (m_value) { + auto value_str = reinterpret_steal(PyObject_Str(m_value.ptr())); + if (!value_str) { + message_error_string = detail::error_string(); + result = ""; + } else { + result = value_str.cast(); + } + } else { + result = ""; + } + if (result.empty()) { + result = ""; + } + + bool have_trace = false; + if (m_trace) { +#if !defined(PYPY_VERSION) + auto *tb = reinterpret_cast(m_trace.ptr()); + + // Get the deepest trace possible. + while (tb->tb_next) { + tb = tb->tb_next; + } + + PyFrameObject *frame = tb->tb_frame; + Py_XINCREF(frame); + result += "\n\nAt:\n"; + while (frame) { +# if PY_VERSION_HEX >= 0x030900B1 + PyCodeObject *f_code = PyFrame_GetCode(frame); +# else + PyCodeObject *f_code = frame->f_code; + Py_INCREF(f_code); +# endif + int lineno = PyFrame_GetLineNumber(frame); + result += " "; + result += handle(f_code->co_filename).cast(); + result += '('; + result += std::to_string(lineno); + result += "): "; + result += handle(f_code->co_name).cast(); + result += '\n'; + Py_DECREF(f_code); +# if PY_VERSION_HEX >= 0x030900B1 + auto *b_frame = PyFrame_GetBack(frame); +# else + auto *b_frame = frame->f_back; + Py_XINCREF(b_frame); +# endif + Py_DECREF(frame); + frame = b_frame; + } + + have_trace = true; +#endif //! defined(PYPY_VERSION) + } + + if (!message_error_string.empty()) { + if (!have_trace) { + result += '\n'; + } + result += "\nMESSAGE UNAVAILABLE DUE TO EXCEPTION: " + message_error_string; + } + + return result; + } + + std::string const &error_string() const { + if (!m_lazy_error_string_completed) { + m_lazy_error_string += ": " + format_value_and_trace(); + m_lazy_error_string_completed = true; + } + return m_lazy_error_string; + } + + void restore() { + if (m_restore_called) { + pybind11_fail("Internal error: pybind11::detail::error_fetch_and_normalize::restore() " + "called a second time. ORIGINAL ERROR: " + + error_string()); + } + PyErr_Restore(m_type.inc_ref().ptr(), m_value.inc_ref().ptr(), m_trace.inc_ref().ptr()); + m_restore_called = true; + } + + bool matches(handle exc) const { + return (PyErr_GivenExceptionMatches(m_type.ptr(), exc.ptr()) != 0); + } + + // Not protecting these for simplicity. + object m_type, m_value, m_trace; + +private: + // Only protecting invariants. + mutable std::string m_lazy_error_string; + mutable bool m_lazy_error_string_completed = false; + mutable bool m_restore_called = false; +}; + +inline std::string error_string() { + return error_fetch_and_normalize("pybind11::detail::error_string").error_string(); +} + PYBIND11_NAMESPACE_END(detail) #if defined(_MSC_VER) @@ -396,39 +572,30 @@ PYBIND11_NAMESPACE_END(detail) /// thrown to propagate python-side errors back through C++ which can either be caught manually or /// else falls back to the function dispatcher (which then raises the captured error back to /// python). -class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error { +class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception { public: - /// Constructs a new exception from the current Python error indicator. The current - /// Python error indicator will be cleared. - error_already_set() : std::runtime_error(detail::error_string("pybind11::error_already_set")) { - PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr()); - } + /// Fetches the current Python exception (using PyErr_Fetch()), which will clear the + /// current Python error indicator. + error_already_set() + : m_fetched_error{new detail::error_fetch_and_normalize("pybind11::error_already_set"), + m_fetched_error_deleter} {} - /// WARNING: The GIL must be held when this copy constructor is invoked! - error_already_set(const error_already_set &) = default; - error_already_set(error_already_set &&) = default; - - /// WARNING: This destructor needs to acquire the Python GIL. This can lead to + /// The what() result is built lazily on demand. + /// WARNING: This member function needs to acquire the Python GIL. This can lead to /// crashes (undefined behavior) if the Python interpreter is finalizing. - inline ~error_already_set() override; + const char *what() const noexcept override; /// Restores the currently-held Python error (which will clear the Python error indicator first - /// if already set). After this call, the current object no longer stores the error variables. - /// NOTE: Any copies of this object may still store the error variables. Currently there is no - // protection against calling restore() from multiple copies. + /// if already set). /// NOTE: This member function will always restore the normalized exception, which may or may /// not be the original Python exception. /// WARNING: The GIL must be held when this member function is called! - void restore() { - PyErr_Restore(m_type.release().ptr(), m_value.release().ptr(), m_trace.release().ptr()); - } + void restore() { m_fetched_error->restore(); } /// If it is impossible to raise the currently-held error, such as in a destructor, we can /// write it out using Python's unraisable hook (`sys.unraisablehook`). The error context /// should be some object whose `repr()` helps identify the location of the error. Python - /// already knows the type and value of the error, so there is no need to repeat that. After - /// this call, the current object no longer stores the error variables, and neither does - /// Python. + /// already knows the type and value of the error, so there is no need to repeat that. void discard_as_unraisable(object err_context) { restore(); PyErr_WriteUnraisable(err_context.ptr()); @@ -447,16 +614,18 @@ public: /// Check if the currently trapped error type matches the given Python exception class (or a /// subclass thereof). May also be passed a tuple to search for any exception class matches in /// the given tuple. - bool matches(handle exc) const { - return (PyErr_GivenExceptionMatches(m_type.ptr(), exc.ptr()) != 0); - } + bool matches(handle exc) const { return m_fetched_error->matches(exc); } - const object &type() const { return m_type; } - const object &value() const { return m_value; } - const object &trace() const { return m_trace; } + const object &type() const { return m_fetched_error->m_type; } + const object &value() const { return m_fetched_error->m_value; } + const object &trace() const { return m_fetched_error->m_trace; } private: - object m_type, m_value, m_trace; + std::shared_ptr m_fetched_error; + + /// WARNING: This custom deleter needs to acquire the Python GIL. This can lead to + /// crashes (undefined behavior) if the Python interpreter is finalizing. + static void m_fetched_error_deleter(detail::error_fetch_and_normalize *raw_ptr); }; #if defined(_MSC_VER) # pragma warning(pop) @@ -492,8 +661,7 @@ inline void raise_from(PyObject *type, const char *message) { /// Sets the current Python error indicator with the chosen error, performing a 'raise from' /// from the error contained in error_already_set to indicate that the chosen error was -/// caused by the original error. After this function is called error_already_set will -/// no longer contain an error. +/// caused by the original error. inline void raise_from(error_already_set &err, PyObject *type, const char *message) { err.restore(); raise_from(type, message); diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 9469b8672..3ec999d1d 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -105,6 +105,11 @@ struct PythonAlreadySetInDestructor { py::str s; }; +std::string error_already_set_what(const py::object &exc_type, const py::object &exc_value) { + PyErr_SetObject(exc_type.ptr(), exc_value.ptr()); + return py::error_already_set().what(); +} + TEST_SUBMODULE(exceptions, m) { m.def("throw_std_exception", []() { throw std::runtime_error("This exception was intentionally thrown."); }); @@ -269,7 +274,9 @@ TEST_SUBMODULE(exceptions, m) { if (ex.matches(exc_type)) { py::print(ex.what()); } else { - throw; + // Simply `throw;` also works and is better, but using `throw ex;` + // here to cover that situation (as observed in the wild). + throw ex; // Invokes the copy ctor. } } }); @@ -317,4 +324,14 @@ TEST_SUBMODULE(exceptions, m) { = reinterpret_cast(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr())); interleaved_error_already_set(); }); + + m.def("test_error_already_set_double_restore", [](bool dry_run) { + PyErr_SetString(PyExc_ValueError, "Random error."); + py::error_already_set e; + e.restore(); + PyErr_Clear(); + if (!dry_run) { + e.restore(); + } + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 4dcbb83a3..a5984a142 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -305,13 +305,16 @@ def test_error_already_set_what_with_happy_exceptions( @pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault") def test_flaky_exception_failure_point_init(): - what, py_err_set_after_what = m.error_already_set_what( - FlakyException, ("failure_point_init",) - ) - assert not py_err_set_after_what - lines = what.splitlines() + with pytest.raises(RuntimeError) as excinfo: + m.error_already_set_what(FlakyException, ("failure_point_init",)) + lines = str(excinfo.value).splitlines() # PyErr_NormalizeException replaces the original FlakyException with ValueError: - assert lines[:3] == ["ValueError: triggered_failure_point_init", "", "At:"] + assert lines[:3] == [ + "pybind11::error_already_set: MISMATCH of original and normalized active exception types:" + " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init", + "", + "At:", + ] # Checking the first two lines of the traceback as formatted in error_string(): assert "test_exceptions.py(" in lines[3] assert lines[3].endswith("): __init__") @@ -319,10 +322,25 @@ def test_flaky_exception_failure_point_init(): def test_flaky_exception_failure_point_str(): - # The error_already_set ctor fails due to a ValueError in error_string(): - with pytest.raises(ValueError) as excinfo: - m.error_already_set_what(FlakyException, ("failure_point_str",)) - assert str(excinfo.value) == "triggered_failure_point_str" + what, py_err_set_after_what = m.error_already_set_what( + FlakyException, ("failure_point_str",) + ) + assert not py_err_set_after_what + lines = what.splitlines() + if env.PYPY and len(lines) == 3: + n = 3 # Traceback is missing. + else: + n = 5 + assert ( + lines[:n] + == [ + "FlakyException: ", + "", + "MESSAGE UNAVAILABLE DUE TO EXCEPTION: ValueError: triggered_failure_point_str", + "", + "At:", + ][:n] + ) def test_cross_module_interleaved_error_already_set(): @@ -332,3 +350,13 @@ def test_cross_module_interleaved_error_already_set(): "2nd error.", # Almost all platforms. "RuntimeError: 2nd error.", # Some PyPy builds (seen under macOS). ) + + +def test_error_already_set_double_restore(): + m.test_error_already_set_double_restore(True) # dry_run + with pytest.raises(RuntimeError) as excinfo: + m.test_error_already_set_double_restore(False) + assert str(excinfo.value) == ( + "Internal error: pybind11::detail::error_fetch_and_normalize::restore()" + " called a second time. ORIGINAL ERROR: ValueError: Random error." + )