Commit 6795a2ba authored by Matěj Laitl's avatar Matěj Laitl

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.
parent d06c8958
......@@ -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)
......
......@@ -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()
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment