[cpyrt] Raise C++ exceptions from destructors in Python

src/cpyrt/CPPInstance.cxx

@@ -14,6 +14,7 @@ using namespace cppjit;
 #include "cppjit_interop.h"
 
 #include "cpyrt/DispatchPtr.h"
+#include "cpyrt/PyException.h"
 
 // Standard
 #include <algorithm>
@@ -213,8 +214,9 @@ void cpyrt::CPPInstance::SetDispatchPtr(void* ptr) {
 }
 
 //----------------------------------------------------------------------------
-void cpyrt::op_dealloc_nofree(CPPInstance* pyobj) {
+bool cpyrt::op_dealloc_nofree(CPPInstance* pyobj) {
   // Destroy the held C++ object, if owned; does not deallocate the proxy.
+  // Returns false with a Python error set if the destructor throws.
 
   interop::TCppScope_t klass = pyobj->ObjectIsA(false /* check_smart */);
   void*& cppobj = pyobj->GetObjectRaw();
@@ -223,18 +225,36 @@ void cpyrt::op_dealloc_nofree(CPPInstance* pyobj) {
     MemoryRegulator::UnregisterPyObject(pyobj,
                                         (PyObject*)Py_TYPE((PyObject*)pyobj));
 
+  bool ok = true;
   if (cppobj && (pyobj->fFlags & CPPInstance::kIsOwner)) {
-    if (pyobj->fFlags & CPPInstance::kIsValue) {
-      interop::CallDestructor(klass, cppobj);
-      interop::Deallocate(klass, cppobj);
-    } else
-      interop::Destruct(klass, cppobj);
+    try {
+      if (pyobj->fFlags & CPPInstance::kIsValue) {
+        // [expr.delete]: the storage is freed even if the destructor throws
+        try {
+          interop::CallDestructor(klass, cppobj);
+        } catch (...) {
+          interop::Deallocate(klass, cppobj);
+          throw;
+        }
+        interop::Deallocate(klass, cppobj);
+      } else
+        interop::Destruct(klass, cppobj);
+    } catch (PyException&) {
+      ok = false; // error already set
+    } catch (std::exception& e) {
+      SetPyErrFromCppException(e);
+      ok = false;
+    } catch (...) {
+      PyErr_SetString(PyExc_Exception, "unhandled, unknown C++ exception");
+      ok = false;
+    }
   }
   cppobj = nullptr;
 
   if (pyobj->IsExtended())
     delete (ExtendedData*)pyobj->fObject;
   pyobj->fFlags = CPPInstance::kNoWrapConv;
+  return ok;
 }
 
 namespace cppjit::cpyrt {
@@ -276,7 +296,8 @@ static PyObject* op_destruct(CPPInstance* self) {
   // garbage collector (like in PyPy), to allow the user control over when
   // the C++ destructor is called. This method requires that the C++ object
   // is owned (no-op otherwise).
-  op_dealloc_nofree(self);
+  if (!op_dealloc_nofree(self))
+    return nullptr;
   Py_RETURN_NONE;
 }
 
@@ -454,7 +475,12 @@ static CPPInstance* op_new(PyTypeObject* subtype, PyObject*, PyObject*) {
 static void op_dealloc(CPPInstance* pyobj) {
   // Remove (Python-side) memory held by the object proxy.
   PyObject_GC_UnTrack((PyObject*)pyobj);
-  op_dealloc_nofree(pyobj);
+  // dealloc cannot raise, so report the way CPython reports __del__ errors
+  PyObject *pytype, *pyvalue, *pytrace;
+  PyErr_Fetch(&pytype, &pyvalue, &pytrace);
+  if (!op_dealloc_nofree(pyobj))
+    PyErr_WriteUnraisable((PyObject*)Py_TYPE(pyobj));
+  PyErr_Restore(pytype, pyvalue, pytrace);
   PyObject_GC_Del((PyObject*)pyobj);
 }
 

src/cpyrt/CPPInstance.h

@@ -142,7 +142,7 @@ template <typename T> inline bool CPPInstance_CheckExact(T* object) {
 }
 
 //- helper for memory regulation (no PyTypeObject equiv. member in p2.2) -----
-void op_dealloc_nofree(CPPInstance*);
+bool op_dealloc_nofree(CPPInstance*);
 
 } // namespace cppjit::cpyrt
 

src/cpyrt/CPPMethod.cxx

@@ -119,60 +119,8 @@ inline PyObject* cpyrt::CPPMethod::ExecuteFast(void* self, ptrdiff_t offset,
     ctxt->fFlags |= CallContext::kPyException;
     result = nullptr; // error already set
   } catch (std::exception& e) {
-    // attempt to set the exception to the actual type, to allow catching with
-    // the Python C++ type
-    static interop::TCppScope_t exc_type =
-        interop::GetFullScope("std::exception");
-
     ctxt->fFlags |= CallContext::kCppException;
-
-    PyObject* pyexc_type = nullptr;
-    PyObject* pyexc_obj = nullptr;
-
-    // TODO: factor this code with the same in ProxyWrappers (and cache it there
-    // to be able to look up based on TCppType_t):
-    interop::TCppScope_t actual = interop::GetActualClass(exc_type, &e);
-    const std::string& finalname = interop::GetScopedFinalName(actual);
-    const std::string& parentname = TypeManip::extract_namespace(finalname);
-    PyObject* parent = CreateScopeProxy(parentname);
-    if (parent) {
-      pyexc_type = PyObject_GetAttrString(
-          parent,
-          parentname.empty()
-              ? finalname.c_str()
-              : finalname.substr(parentname.size() + 2, std::string::npos)
-                    .c_str());
-      Py_DECREF(parent);
-    }
-
-    if (pyexc_type) {
-      // create a copy of the exception (TODO: factor this code with the same in
-      // ProxyWrappers)
-      PyObject* pyclass = cpyrt::GetScopeProxy(actual);
-      PyObject* source = BindCppObjectNoCast(&e, actual);
-      PyObject* pyexc_copy =
-          PyObject_CallFunctionObjArgs(pyclass, source, nullptr);
-      Py_DECREF(source);
-      Py_DECREF(pyclass);
-      if (pyexc_copy) {
-        pyexc_obj = CPPExcInstance_Type.tp_new((PyTypeObject*)pyexc_type,
-                                               nullptr, nullptr);
-        ((CPPExcInstance*)pyexc_obj)->fCppInstance = (PyObject*)pyexc_copy;
-      } else
-        PyErr_Clear();
-    } else
-      PyErr_Clear();
-
-    if (pyexc_type && pyexc_obj) {
-      PyErr_SetObject(pyexc_type, pyexc_obj);
-      Py_DECREF(pyexc_obj);
-      Py_DECREF(pyexc_type);
-    } else {
-      PyErr_Format(PyExc_Exception, "%s (C++ exception)", e.what());
-      Py_XDECREF(pyexc_obj);
-      Py_XDECREF(pyexc_type);
-    }
-
+    SetPyErrFromCppException(e);
     result = nullptr;
   } catch (...) {
     // don't set the kCppException flag here, as there is basically no useful

src/cpyrt/ProxyWrappers.cxx

@@ -1058,3 +1058,55 @@ PyObject* cpyrt::BindCppObjectArray(interop::TCppObject_t address,
   // TODO: this function exists for symmetry; need to figure out if it's useful
   return TupleOfInstances_New(address, klass, dims);
 }
+
+//----------------------------------------------------------------------------
+void cpyrt::SetPyErrFromCppException(std::exception& e) {
+  // attempt to set the exception to the actual type, to allow catching with
+  // the Python C++ type
+  static interop::TCppScope_t exc_type =
+      interop::GetFullScope("std::exception");
+
+  PyObject* pyexc_type = nullptr;
+  PyObject* pyexc_obj = nullptr;
+
+  // TODO: cache the lookup based on TCppType_t
+  interop::TCppScope_t actual = interop::GetActualClass(exc_type, &e);
+  const std::string& finalname = interop::GetScopedFinalName(actual);
+  const std::string& parentname = TypeManip::extract_namespace(finalname);
+  PyObject* parent = CreateScopeProxy(parentname);
+  if (parent) {
+    pyexc_type = PyObject_GetAttrString(
+        parent, parentname.empty()
+                    ? finalname.c_str()
+                    : finalname.substr(parentname.size() + 2, std::string::npos)
+                          .c_str());
+    Py_DECREF(parent);
+  }
+
+  if (pyexc_type) {
+    // create a copy of the exception
+    PyObject* pyclass = cpyrt::GetScopeProxy(actual);
+    PyObject* source = BindCppObjectNoCast(&e, actual);
+    PyObject* pyexc_copy =
+        PyObject_CallFunctionObjArgs(pyclass, source, nullptr);
+    Py_DECREF(source);
+    Py_DECREF(pyclass);
+    if (pyexc_copy) {
+      pyexc_obj = CPPExcInstance_Type.tp_new((PyTypeObject*)pyexc_type, nullptr,
+                                             nullptr);
+      ((CPPExcInstance*)pyexc_obj)->fCppInstance = (PyObject*)pyexc_copy;
+    } else
+      PyErr_Clear();
+  } else
+    PyErr_Clear();
+
+  if (pyexc_type && pyexc_obj) {
+    PyErr_SetObject(pyexc_type, pyexc_obj);
+    Py_DECREF(pyexc_obj);
+    Py_DECREF(pyexc_type);
+  } else {
+    PyErr_Format(PyExc_Exception, "%s (C++ exception)", e.what());
+    Py_XDECREF(pyexc_obj);
+    Py_XDECREF(pyexc_type);
+  }
+}

src/cpyrt/ProxyWrappers.h

@@ -5,6 +5,7 @@
 #include "Dimensions.h"
 
 // Standard
+#include <exception>
 #include <string>
 
 namespace cppjit::cpyrt {
@@ -32,6 +33,9 @@ PyObject* BindCppObject(interop::TCppObject_t object,
 PyObject* BindCppObjectArray(interop::TCppObject_t address,
                              interop::TCppScope_t klass, cdims_t dims);
 
+// set the Python error to a proxy of the C++ exception, or a plain Exception
+void SetPyErrFromCppException(std::exception& e);
+
 } // namespace cppjit::cpyrt
 
 #endif // !CPYRT_PROXYWRAPPERS_H

test/test_advancedcpp.py

@@ -1098,3 +1098,55 @@ def test31_virtual_base_downcast(self):
         assert isinstance(obj3, ns.Visible3)
         assert obj3.value == 23
         assert obj3.z == 7
+
+    @mark.xfail(
+        condition=IS_MAC_ARM,
+        run=False,
+        reason="Crashes with exception not being caught on Apple Silicon",
+    )
+    def test33_throwing_destructor(self):
+        """A noexcept(false) destructor that throws raises in Python"""
+
+        import sys
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        #include <stdexcept>
+        namespace throwing_dtor {
+        int g_count = 0;
+        struct Throws {
+            ~Throws() noexcept(false) {
+                ++g_count;
+                throw std::runtime_error("from dtor");
+            }
+        };
+        Throws make() { return {}; }
+        }""")
+
+        ns = cppjit.gbl.throwing_dtor
+
+        t = ns.Throws()
+        with raises(cppjit.gbl.std.runtime_error, match="from dtor"):
+            t.__destruct__()
+        assert ns.g_count == 1
+        t.__destruct__()  # no longer owned: no second destructor call
+        assert ns.g_count == 1
+
+        v = ns.make()
+        with raises(cppjit.gbl.std.runtime_error, match="from dtor"):
+            v.__destruct__()
+        assert ns.g_count == 2
+
+        # dealloc cannot raise: the error goes to sys.unraisablehook
+        unraisable = []
+        old_hook = sys.unraisablehook
+        sys.unraisablehook = unraisable.append
+        try:
+            t = ns.Throws()
+            del t
+        finally:
+            sys.unraisablehook = old_hook
+        assert ns.g_count == 3
+        assert len(unraisable) == 1
+        assert "from dtor" in str(unraisable[0].exc_value)