diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 42720f844d..ed45f7ce8c 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -455,8 +455,6 @@ 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. */ @@ -492,7 +490,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")); @@ -717,7 +715,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 73767fe7d1..74e9c745da 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -534,6 +534,73 @@ 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 2ad636d6b1..1a06b37e03 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -3043,21 +3043,17 @@ void print(Args &&...args) { detail::print(c.args(), c.kwargs()); } -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(); +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(); + } } 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 6cdec72bcf..d16e865e1f 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -411,175 +411,7 @@ T reinterpret_steal(handle h) { } PYBIND11_NAMESPACE_BEGIN(detail) - -// 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(); -} - +std::string error_string(const char *called = nullptr); PYBIND11_NAMESPACE_END(detail) #if defined(_MSC_VER) @@ -592,30 +424,39 @@ 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::exception { +class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error { public: - /// 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} {} - - /// The what() result is built lazily on demand. - /// WARNING: This member function needs to acquire the Python GIL. This can lead to + /// 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()); + } + + /// 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 /// crashes (undefined behavior) if the Python interpreter is finalizing. - const char *what() const noexcept override; + inline ~error_already_set() override; /// Restores the currently-held Python error (which will clear the Python error indicator first - /// if already set). + /// 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. /// 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() { m_fetched_error->restore(); } + void restore() { + PyErr_Restore(m_type.release().ptr(), m_value.release().ptr(), m_trace.release().ptr()); + } /// 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. + /// 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. void discard_as_unraisable(object err_context) { restore(); PyErr_WriteUnraisable(err_context.ptr()); @@ -634,18 +475,16 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception { /// 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 m_fetched_error->matches(exc); } + bool matches(handle exc) const { + return (PyErr_GivenExceptionMatches(m_type.ptr(), exc.ptr()) != 0); + } - 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; } + const object &type() const { return m_type; } + const object &value() const { return m_value; } + const object &trace() const { return m_trace; } private: - 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); + object m_type, m_value, m_trace; }; #if defined(_MSC_VER) # pragma warning(pop) @@ -681,7 +520,8 @@ 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. +/// caused by the original error. After this function is called error_already_set will +/// no longer contain an 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 3ec999d1dc..9469b86724 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -105,11 +105,6 @@ 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."); }); @@ -274,9 +269,7 @@ TEST_SUBMODULE(exceptions, m) { if (ex.matches(exc_type)) { py::print(ex.what()); } else { - // 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. + throw; } } }); @@ -324,14 +317,4 @@ 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 a5984a142f..4dcbb83a36 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -305,16 +305,13 @@ 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(): - with pytest.raises(RuntimeError) as excinfo: - m.error_already_set_what(FlakyException, ("failure_point_init",)) - lines = str(excinfo.value).splitlines() + 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() # PyErr_NormalizeException replaces the original FlakyException with ValueError: - 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:", - ] + assert lines[:3] == ["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__") @@ -322,25 +319,10 @@ def test_flaky_exception_failure_point_init(): def test_flaky_exception_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] - ) + # 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" def test_cross_module_interleaved_error_already_set(): @@ -350,13 +332,3 @@ 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." - )