Merge branch 'pybind:master' into master

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());

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() {
+PYBIND11_NOINLINE std::string error_string(const char *called) {
-    if (!PyErr_Occurred()) {
+    error_scope scope; // Fetch error state (will be restored when this function returns).
-        PyErr_SetString(PyExc_RuntimeError, "Unknown internal error occurred");
+    if (scope.type == nullptr) {
-        return "Unknown internal error occurred";
+        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);

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
+    /// Restores the currently-held Python error (which will clear the Python error indicator first
-    /// already set it is cleared first.  After this call, the current object no longer stores the
+    /// if already set). After this call, the current object no longer stores the error variables.
-    /// error variables (but the `.what()` string is still available).
+    /// 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<object>(PYBIND11_FROM_STRING(err_context)));
     }

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

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 <pybind11/pybind11.h>
+
+// 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<void *>(&interleaved_error_already_set)));
+    }
+    return m;
+}

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<void (*)()>(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr()));
+        interleaved_error_already_set();
+    });
 }

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).
+    )

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(),

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]