diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 3f83113bd..6ca5e1482 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -415,6 +415,7 @@ PYBIND11_NOINLINE internals &get_internals() { ~gil_scoped_acquire_local() { PyGILState_Release(state); } const PyGILState_STATE state; } gil; + error_scope err_scope; PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID); auto builtins = handle(PyEval_GetBuiltins()); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 14561759b..dd8d03751 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -470,14 +470,16 @@ PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp) return isinstance(obj, type); } -PYBIND11_NOINLINE std::string error_string() { - if (!PyErr_Occurred()) { - PyErr_SetString(PyExc_RuntimeError, "Unknown internal error occurred"); - return "Unknown internal error occurred"; +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."); } - error_scope scope; // Preserve error state - PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace); if (scope.trace != nullptr) { PyException_SetTraceback(scope.value, scope.trace); diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 5b1c60e33..0c5690064 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -180,6 +180,10 @@ private: PYBIND11_NAMESPACE_END(detail) +#if !defined(PYBIND11_HANDLE_REF_DEBUG) && !defined(NDEBUG) +# define PYBIND11_HANDLE_REF_DEBUG +#endif + /** \rst Holds a reference to a Python object (no reference counting) @@ -209,6 +213,9 @@ public: this function automatically. Returns a reference to itself. \endrst */ const handle &inc_ref() const & { +#ifdef PYBIND11_HANDLE_REF_DEBUG + inc_ref_counter(1); +#endif Py_XINCREF(m_ptr); return *this; } @@ -244,6 +251,18 @@ public: protected: PyObject *m_ptr = nullptr; + +#ifdef PYBIND11_HANDLE_REF_DEBUG +private: + static std::size_t inc_ref_counter(std::size_t add) { + thread_local std::size_t counter = 0; + counter += add; + return counter; + } + +public: + static std::size_t inc_ref_counter() { return inc_ref_counter(0); } +#endif }; /** \rst @@ -360,7 +379,7 @@ T reinterpret_steal(handle h) { } PYBIND11_NAMESPACE_BEGIN(detail) -std::string error_string(); +std::string error_string(const char *called = nullptr); PYBIND11_NAMESPACE_END(detail) #if defined(_MSC_VER) @@ -375,20 +394,27 @@ PYBIND11_NAMESPACE_END(detail) /// python). class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error { public: - /// Constructs a new exception from the current Python error indicator, if any. The current + /// 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()) { + 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. inline ~error_already_set() override; - /// Give the currently-held error back to Python, if any. If there is currently a Python error - /// already set it is cleared first. After this call, the current object no longer stores the - /// error variables (but the `.what()` string is still available). + /// 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. + /// 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()); } @@ -405,6 +431,7 @@ public: } /// An alternate version of `discard_as_unraisable()`, where a string provides information on /// the location of the error. For example, `__func__` could be helpful. + /// WARNING: The GIL must be held when this member function is called! void discard_as_unraisable(const char *err_context) { discard_as_unraisable(reinterpret_steal(PYBIND11_FROM_STRING(err_context))); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f0c9ef54c..06d26ab3c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -216,6 +216,7 @@ tests_extra_targets("test_exceptions.py;test_local_bindings.py;test_stl.py;test_ "pybind11_cross_module_tests") # And add additional targets for other tests. +tests_extra_targets("test_exceptions.py" "cross_module_interleaved_error_already_set") tests_extra_targets("test_gil_scoped.py" "cross_module_gil_utils") set(PYBIND11_EIGEN_REPO diff --git a/tests/cross_module_interleaved_error_already_set.cpp b/tests/cross_module_interleaved_error_already_set.cpp new file mode 100644 index 000000000..fdd9939e4 --- /dev/null +++ b/tests/cross_module_interleaved_error_already_set.cpp @@ -0,0 +1,51 @@ +/* + Copyright (c) 2022 Google LLC + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#include + +// This file mimics a DSO that makes pybind11 calls but does not define a PYBIND11_MODULE, +// so that the first call of cross_module_error_already_set() triggers the first call of +// pybind11::detail::get_internals(). + +namespace { + +namespace py = pybind11; + +void interleaved_error_already_set() { + PyErr_SetString(PyExc_RuntimeError, "1st error."); + try { + throw py::error_already_set(); + } catch (const py::error_already_set &) { + // The 2nd error could be conditional in a real application. + PyErr_SetString(PyExc_RuntimeError, "2nd error."); + } // Here the 1st error is destroyed before the 2nd error is fetched. + // The error_already_set dtor triggers a pybind11::detail::get_internals() + // call via pybind11::gil_scoped_acquire. + if (PyErr_Occurred()) { + throw py::error_already_set(); + } +} + +constexpr char kModuleName[] = "cross_module_interleaved_error_already_set"; + +struct PyModuleDef moduledef = { + PyModuleDef_HEAD_INIT, kModuleName, nullptr, 0, nullptr, nullptr, nullptr, nullptr, nullptr}; + +} // namespace + +extern "C" PYBIND11_EXPORT PyObject *PyInit_cross_module_interleaved_error_already_set() { + PyObject *m = PyModule_Create(&moduledef); + if (m != nullptr) { + static_assert(sizeof(&interleaved_error_already_set) == sizeof(void *), + "Function pointer must have the same size as void *"); + PyModule_AddObject( + m, + "funcaddr", + PyLong_FromVoidPtr(reinterpret_cast(&interleaved_error_already_set))); + } + return m; +} diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index d7b31cf74..9469b8672 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -228,7 +228,10 @@ TEST_SUBMODULE(exceptions, m) { throw py::error_already_set(); } catch (const std::runtime_error &e) { if ((err && e.what() != std::string("ValueError: foo")) - || (!err && e.what() != std::string("Unknown internal error occurred"))) { + || (!err + && e.what() + != std::string("Internal error: pybind11::error_already_set called " + "while Python error indicator not set."))) { PyErr_Clear(); throw std::runtime_error("error message mismatch"); } @@ -307,4 +310,11 @@ TEST_SUBMODULE(exceptions, m) { PyErr_Clear(); return py::make_tuple(std::move(what), py_err_set_after_what); }); + + m.def("test_cross_module_interleaved_error_already_set", []() { + auto cm = py::module_::import("cross_module_interleaved_error_already_set"); + auto interleaved_error_already_set + = reinterpret_cast(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr())); + interleaved_error_already_set(); + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index dc7594113..4dcbb83a3 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -16,7 +16,10 @@ def test_std_exception(msg): def test_error_already_set(msg): with pytest.raises(RuntimeError) as excinfo: m.throw_already_set(False) - assert msg(excinfo.value) == "Unknown internal error occurred" + assert ( + msg(excinfo.value) + == "Internal error: pybind11::error_already_set called while Python error indicator not set." + ) with pytest.raises(ValueError) as excinfo: m.throw_already_set(True) @@ -320,3 +323,12 @@ def test_flaky_exception_failure_point_str(): 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(): + with pytest.raises(RuntimeError) as excinfo: + m.test_cross_module_interleaved_error_already_set() + assert str(excinfo.value) in ( + "2nd error.", # Almost all platforms. + "RuntimeError: 2nd error.", # Some PyPy builds (seen under macOS). + ) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 8d296f655..ef214acc3 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -297,6 +297,55 @@ TEST_SUBMODULE(pytypes, m) { return d; }); + m.def("accessor_moves", []() { // See PR #3970 + py::list return_list; +#ifdef PYBIND11_HANDLE_REF_DEBUG + py::int_ py_int_0(0); + py::int_ py_int_42(42); + py::str py_str_count("count"); + + auto tup = py::make_tuple(0); + + py::sequence seq(tup); + + py::list lst; + lst.append(0); + +# define PYBIND11_LOCAL_DEF(...) \ + { \ + std::size_t inc_refs = py::handle::inc_ref_counter(); \ + __VA_ARGS__; \ + inc_refs = py::handle::inc_ref_counter() - inc_refs; \ + return_list.append(inc_refs); \ + } + + PYBIND11_LOCAL_DEF(tup[py_int_0]) // l-value (to have a control) + PYBIND11_LOCAL_DEF(tup[py::int_(0)]) // r-value + + PYBIND11_LOCAL_DEF(tup.attr(py_str_count)) // l-value + PYBIND11_LOCAL_DEF(tup.attr(py::str("count"))) // r-value + + PYBIND11_LOCAL_DEF(seq[py_int_0]) // l-value + PYBIND11_LOCAL_DEF(seq[py::int_(0)]) // r-value + + PYBIND11_LOCAL_DEF(seq.attr(py_str_count)) // l-value + PYBIND11_LOCAL_DEF(seq.attr(py::str("count"))) // r-value + + PYBIND11_LOCAL_DEF(lst[py_int_0]) // l-value + PYBIND11_LOCAL_DEF(lst[py::int_(0)]) // r-value + + PYBIND11_LOCAL_DEF(lst.attr(py_str_count)) // l-value + PYBIND11_LOCAL_DEF(lst.attr(py::str("count"))) // r-value + + auto lst_acc = lst[py::int_(0)]; + lst_acc = py::int_(42); // Detaches lst_acc from lst. + PYBIND11_LOCAL_DEF(lst_acc = py_int_42) // l-value + PYBIND11_LOCAL_DEF(lst_acc = py::int_(42)) // r-value +# undef PYBIND11_LOCAL_DEF +#endif + return return_list; + }); + // test_constructors m.def("default_constructors", []() { return py::dict("bytes"_a = py::bytes(), diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 9afe62f42..b91a7e156 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -317,6 +317,15 @@ def test_accessors(): assert d["var"] == 99 +def test_accessor_moves(): + inc_refs = m.accessor_moves() + if inc_refs: + # To be changed in PR #3970: [1, 0, 1, 0, ...] + assert inc_refs == [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] + else: + pytest.skip("Not defined: PYBIND11_HANDLE_REF_DEBUG") + + def test_constructors(): """C++ default and converting constructors are equivalent to type calls in Python""" types = [bytes, bytearray, str, bool, int, float, tuple, list, dict, set]