From 6eb0745f2a28cbf60bfbe4feb40f95ddb5b54c46 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Mat=C4=9Bj=20Laitl?= <matej@laitl.cz>
Date: Fri, 25 Jan 2013 11:27:49 +0100
Subject: [PATCH] Fix calling an "except +" cpp function in a nogil function

For a source:
|cdef extern from "<foo>":
|    cdef void foo_cpp() nogil except +
|
|cdef call_foo() nogil:
|    foo_cpp()

We used to generate something like this to actually call foo_cpp() in call_foo():
|try{foo_cpp();}
|catch(...) {
|  Py_BLOCK_THREADS; __Pyx_CppExn2PyErr(); Py_UNBLOCK_THREADS
|  `code.error_goto(self.pos))`
|}

where Py_BLOCK_THREADS expands to "PyEval_RestoreThread(_save);".
__Pyx_CppExn2PyErr() (and alternatives, see SimpleCallNode) calls CPython A API
methods so it needs to be guarded in a nogil environment.

One problem is that "PyThreadState *_save" is only declared by "with nogil:"
block, so a .cpp file that doesn't compile is generated for the above code.

However, I think the real issue is that Py_(UN)BLOCK_THREADS is inappropriate
here, as it isn't allowed to be called recursively and is valid only directly
in a Py_BEGIN_ALLOW_THREADS ... Py_END_ALLOW_THREADS. IMO PyGILState_Ensure()
and PyGILState_Release() (through `code.put_ensure_gil()` and a friend) is the
correct thing to call here as it is allowed to be called recursively and
actually ensures the current thread can call CPython C API.

This patch does exactly this (and it breaks the generated code to multiple
lines as it would be way too long otherwise), plus it extends the
cpp_exceptions_nogil.pyx test with above example that doesn't compile with this
fix not applied.

Note that we explicitly pass declare_gilstate=True to put_ensure_gil(), as
PyGILState_Ensure() docs state that recursive calls to it must not share the
PyGILState_STATE. C++-style declaring a variable inside a block should be
no-problem here, as try{} .. catch() is obviously valid only in a C++ code.

--HG--
extra : transplant_source : %AA%F3%BDk%FE%F9%01%7F%8C%A4n%5E%DA4%97%A5%D9%AF%D60
---
 Cython/Compiler/ExprNodes.py       | 22 ++++++++++++----------
 tests/run/cpp_exceptions_nogil.pyx | 11 +++++++++++
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 91d91a0..0c09b1d 100755
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -4110,21 +4110,23 @@ class SimpleCallNode(CallNode):
                     lhs = ""
                 if func_type.exception_check == '+':
                     if func_type.exception_value is None:
-                        raise_py_exception = "__Pyx_CppExn2PyErr()"
+                        raise_py_exception = "__Pyx_CppExn2PyErr();"
                     elif func_type.exception_value.type.is_pyobject:
-                        raise_py_exception = ' try { throw; } catch(const std::exception& exn) { PyErr_SetString(%s, exn.what()); } catch(...) { PyErr_SetNone(%s); }' % (
+                        raise_py_exception = 'try { throw; } catch(const std::exception& exn) { PyErr_SetString(%s, exn.what()); } catch(...) { PyErr_SetNone(%s); }' % (
                             func_type.exception_value.entry.cname,
                             func_type.exception_value.entry.cname)
                     else:
-                        raise_py_exception = '%s(); if (!PyErr_Occurred()) PyErr_SetString(PyExc_RuntimeError , "Error converting c++ exception.")' % func_type.exception_value.entry.cname
+                        raise_py_exception = '%s(); if (!PyErr_Occurred()) PyErr_SetString(PyExc_RuntimeError , "Error converting c++ exception.");' % func_type.exception_value.entry.cname
+                    code.putln("try {")
+                    code.putln("%s%s;" % (lhs, rhs))
+                    code.putln("} catch(...) {")
                     if self.nogil:
-                        raise_py_exception = 'Py_BLOCK_THREADS; %s; Py_UNBLOCK_THREADS' % raise_py_exception
-                    code.putln(
-                    "try {%s%s;} catch(...) {%s; %s}" % (
-                        lhs,
-                        rhs,
-                        raise_py_exception,
-                        code.error_goto(self.pos)))
+                        code.put_ensure_gil(declare_gilstate=True)
+                    code.putln(raise_py_exception)
+                    if self.nogil:
+                        code.put_release_ensured_gil()
+                    code.putln(code.error_goto(self.pos))
+                    code.putln("}")
                 else:
                     if exc_checks:
                         goto_error = code.error_goto_if(" && ".join(exc_checks), self.pos)
diff --git a/tests/run/cpp_exceptions_nogil.pyx b/tests/run/cpp_exceptions_nogil.pyx
index 49acad9..7a05f6f 100644
--- a/tests/run/cpp_exceptions_nogil.pyx
+++ b/tests/run/cpp_exceptions_nogil.pyx
@@ -8,6 +8,17 @@ cdef extern from "cpp_exceptions_nogil_helper.h" nogil:
     cdef void bar "foo"(int i) except +ValueError
     cdef void spam"foo"(int i) except +raise_TypeError
 
+cdef int foo_nogil(int i) nogil except *:
+    foo(i)
+
+def test_foo_nogil():
+    """
+    >>> test_foo_nogil()
+    """
+    foo_nogil(0)
+    with nogil:
+        foo_nogil(0)
+
 def test_foo():
     """
     >>> test_foo()
-- 
2.7.4