Repository: cython/cython
Pull request: Fix handling of cpp_locals exception in nogil
We need to reacquire the GIL in this case (which we mostly didn't do before because it was just dealing with Python objects which always had the GIL)

Fixes #6834

Should probably wait until after 3.1 - it's a fairly niche issue (and not a regression)

Problem statement:
[BUG] __Pyx_RaiseCppAttributeError does not acquire the GIL
### Describe your issue

This affects the cpp_locals directive. Something like

```
from libcpp.string cimport string

@cython.cpp_locals
def f():
    cdef string s
    with nogil:
        return s=="123" 
```

will cause a segmentation fault (when it should raise an exception). Current master and probably current 3.0 branch.

Hints from issue discussion:

Commit message: Fix handling of cpp_locals exception in nogil

We need to reacquire the GIL in this case (which we mostly didn't
do before because it was just dealing with Python objects which
always had the GIL)

Patch to review:
diff --git a/Cython/Compiler/Code.py b/Cython/Compiler/Code.py
index d17d8f1c011..403a735897a 100644
--- a/Cython/Compiler/Code.py
+++ b/Cython/Compiler/Code.py
@@ -2961,28 +2961,25 @@ def put_error_if_neg(self, pos, value):
         return self.putln("if (%s < 0) %s" % (value, self.error_goto(pos)))
 
     def put_error_if_unbound(self, pos, entry, in_nogil_context=False, unbound_check_code=None):
+        nogil_tag = "Nogil" if in_nogil_context else ""
         if entry.from_closure:
-            func = '__Pyx_RaiseClosureNameError'
+            func = f'__Pyx_RaiseClosureNameError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseClosureNameError", "ObjectHandling.c"))
-        elif entry.type.is_memoryviewslice and in_nogil_context:
-            func = '__Pyx_RaiseUnboundMemoryviewSliceNogil'
-            self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseUnboundMemoryviewSliceNogil", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseClosureNameError{nogil_tag}", "ObjectHandling.c"))
         elif entry.type.is_cpp_class and entry.is_cglobal:
-            func = '__Pyx_RaiseCppGlobalNameError'
+            func = f'__Pyx_RaiseCppGlobalNameError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseCppGlobalNameError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseCppGlobalNameError{nogil_tag}", "ObjectHandling.c"))
         elif entry.type.is_cpp_class and entry.is_variable and not entry.is_member and entry.scope.is_c_class_scope:
             # there doesn't seem to be a good way to detecting an instance-attribute of a C class
             # (is_member is only set for class attributes)
-            func = '__Pyx_RaiseCppAttributeError'
+            func = f'__Pyx_RaiseCppAttributeError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseCppAttributeError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseCppAttributeError{nogil_tag}", "ObjectHandling.c"))
         else:
-            func = '__Pyx_RaiseUnboundLocalError'
+            func = f'__Pyx_RaiseUnboundLocalError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseUnboundLocalError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseUnboundLocalError{nogil_tag}", "ObjectHandling.c"))
 
         if not unbound_check_code:
             unbound_check_code = entry.type.check_for_null_code(entry.cname)
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index b02154a0b30..8769b2567c6 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -2541,7 +2541,7 @@ def generate_result_code(self, code):
 
         elif entry.is_cglobal and entry.is_cpp_optional and self.initialized_check:
             unbound_check_code = entry.type.cpp_optional_check_for_null_code(entry.cname)
-            code.put_error_if_unbound(self.pos, entry, unbound_check_code=unbound_check_code)
+            code.put_error_if_unbound(self.pos, entry, self.in_nogil_context, unbound_check_code=unbound_check_code)
 
     def generate_assignment_code(self, rhs, code, overloaded_assignment=False,
                                  exception_check=None, exception_value=None):
@@ -2735,7 +2735,7 @@ def generate_deletion_code(self, code, ignore_nonexisting=False):
         elif self.entry.type.is_pyobject or self.entry.type.is_memoryviewslice:
             if not self.cf_is_null:
                 if self.cf_maybe_null and not ignore_nonexisting:
-                    code.put_error_if_unbound(self.pos, self.entry)
+                    code.put_error_if_unbound(self.pos, self.entry, self.in_nogil_context)
 
                 if self.entry.in_closure:
                     # generator
@@ -8105,7 +8105,7 @@ def generate_result_code(self, code):
                 assert not self.is_temp  # calculate_access_code() only makes sense for non-temps
                 undereferenced_result = self.calculate_access_code()
             unbound_check_code = self.type.cpp_optional_check_for_null_code(undereferenced_result)
-            code.put_error_if_unbound(self.pos, self.entry, unbound_check_code=unbound_check_code)
+            code.put_error_if_unbound(self.pos, self.entry, self.in_nogil_context, unbound_check_code=unbound_check_code)
         else:
             # result_code contains what is needed, but we may need to insert
             # a check and raise an exception
diff --git a/Cython/Utility/ObjectHandling.c b/Cython/Utility/ObjectHandling.c
index f0c8664a695..c736837591f 100644
--- a/Cython/Utility/ObjectHandling.c
+++ b/Cython/Utility/ObjectHandling.c
@@ -3230,6 +3230,19 @@ static CYTHON_INLINE void __Pyx_RaiseUnboundLocalError(const char *varname) {
     PyErr_Format(PyExc_UnboundLocalError, "local variable '%s' referenced before assignment", varname);
 }
 
+/////////////// RaiseUnboundLocalErrorNogil.proto ///////////////
+static void __Pyx_RaiseUnboundLocalErrorNogil(const char *varname);/*proto*/
+
+/////////////// RaiseUnboundLocalErrorNogil ///////////////
+//@requires: RaiseUnboundLocalError
+
+// Don't inline the function, it should really never be called in production
+static void __Pyx_RaiseUnboundLocalErrorNogil(const char *varname) {
+    PyGILState_STATE gilstate = PyGILState_Ensure();
+    __Pyx_RaiseUnboundLocalError(varname);
+    PyGILState_Release(gilstate);
+}
+
 
 /////////////// RaiseClosureNameError.proto ///////////////
 static CYTHON_INLINE void __Pyx_RaiseClosureNameError(const char *varname);/*proto*/
@@ -3239,17 +3252,15 @@ static CYTHON_INLINE void __Pyx_RaiseClosureNameError(const char *varname) {
     PyErr_Format(PyExc_NameError, "free variable '%s' referenced before assignment in enclosing scope", varname);
 }
 
+/////////////// RaiseClosureNameErrorNogil.proto ///////////////
+static CYTHON_INLINE void __Pyx_RaiseClosureNameErrorNogil(const char *varname);/*proto*/
 
-/////////////// RaiseUnboundMemoryviewSliceNogil.proto ///////////////
-static void __Pyx_RaiseUnboundMemoryviewSliceNogil(const char *varname);/*proto*/
-
-/////////////// RaiseUnboundMemoryviewSliceNogil ///////////////
-//@requires: RaiseUnboundLocalError
+/////////////// RaiseClosureNameErrorNogil ///////////////
+//@requires: RaiseClosureNameError
 
-// Don't inline the function, it should really never be called in production
-static void __Pyx_RaiseUnboundMemoryviewSliceNogil(const char *varname) {
+static CYTHON_INLINE void __Pyx_RaiseClosureNameErrorNogil(const char *varname) {
     PyGILState_STATE gilstate = PyGILState_Ensure();
-    __Pyx_RaiseUnboundLocalError(varname);
+    __Pyx_RaiseClosureNameError(varname);
     PyGILState_Release(gilstate);
 }
 
@@ -3261,6 +3272,18 @@ static CYTHON_INLINE void __Pyx_RaiseCppGlobalNameError(const char *varname) {
     PyErr_Format(PyExc_NameError, "C++ global '%s' is not initialized", varname);
 }
 
+//////////////// RaiseCppGlobalNameErrorNogil.proto ///////////////////////
+static void __Pyx_RaiseCppGlobalNameErrorNogil(const char *varname); /*proto*/
+
+/////////////// RaiseCppGlobalNameErrorNogil //////////////////////////////
+//@requires: RaiseCppGlobalNameError
+
+static void __Pyx_RaiseCppGlobalNameErrorNogil(const char *varname) {
+    PyGILState_STATE gilstate = PyGILState_Ensure();
+    __Pyx_RaiseCppGlobalNameError(varname);
+    PyGILState_Release(gilstate);
+}
+
 //////////////// RaiseCppAttributeError.proto ///////////////////////
 static CYTHON_INLINE void __Pyx_RaiseCppAttributeError(const char *varname); /*proto*/
 
@@ -3269,6 +3292,18 @@ static CYTHON_INLINE void __Pyx_RaiseCppAttributeError(const char *varname) {
     PyErr_Format(PyExc_AttributeError, "C++ attribute '%s' is not initialized", varname);
 }
 
+//////////////// RaiseCppAttributeErrorNogil.proto ///////////////////////
+static void __Pyx_RaiseCppAttributeErrorNogil(const char *varname); /*proto*/
+
+/////////////// RaiseCppAttributeErrorNogil //////////////////////////////
+//@requires: RaiseCppGlobalNameError
+
+static void __Pyx_RaiseCppAttributeErrorNogil(const char *varname) {
+    PyGILState_STATE gilstate = PyGILState_Ensure();
+    __Pyx_RaiseCppAttributeError(varname);
+    PyGILState_Release(gilstate);
+}
+
 /////////////// ListPack.proto //////////////////////////////////////
 
 // Equivalent to PyTuple_Pack for sections where we want to reduce the code size
diff --git a/tests/run/cpp_locals_directive.pyx b/tests/run/cpp_locals_directive.pyx
index 359ae0b10e7..dd3f1de3c6d 100644
--- a/tests/run/cpp_locals_directive.pyx
+++ b/tests/run/cpp_locals_directive.pyx
@@ -41,7 +41,7 @@ cdef extern from *:
     cdef cppclass C:
         C(int)
         C(int, cppbool)
-        int getX() const
+        int getX() nogil const
     C make_C(int) except +  # needs a temp to receive
 
 # this function just makes sure the output from the destructor can be captured by doctest
@@ -137,6 +137,22 @@ def maybe_assign_nocheck(assign, value):
     with cython.initializedcheck(False):
         print(x.getX())
 
+def maybe_assign_nogil(assign, value):
+    """
+    >>> maybe_assign_nogil(True, 5)
+    5
+    ~C()
+    >>> maybe_assign_nogil(False, 0)
+    Traceback (most recent call last):
+        ...
+    UnboundLocalError: local variable 'x' referenced before assignment
+    """
+    if assign:
+        x = C(value)
+    with nogil:
+        read = x.getX()
+    print(read)
+
 def uses_temp(value):
     """
     needs a temp to handle the result of make_C - still doesn't use the default constructor
@@ -169,11 +185,17 @@ cdef class HoldsC:
     10
     >>> inst.getCX()  # it was changed in access_from_function_with_different_directive
     20
+    >>> inst.getCX_nogil()
+    20
     >>> inst = HoldsC(False, False)
     >>> inst.getCX()
     Traceback (most recent call last):
         ...
     AttributeError: C++ attribute 'value' is not initialized
+    >>> inst.getCX_nogil()
+    Traceback (most recent call last):
+        ...
+    AttributeError: C++ attribute 'value' is not initialized
     >>> access_from_function_with_different_directive(inst)
     Traceback (most recent call last):
         ...
@@ -187,6 +209,11 @@ cdef class HoldsC:
     def getCX(self):
         return self.value.getX()
 
+    def getCX_nogil(self):
+        with nogil:
+            read = self.value.getX()
+        return read
+
 cdef acceptC(C& c):
     return c.getX()
 
@@ -224,8 +251,22 @@ def read_global_var():
     Traceback (most recent call last):
         ...
     NameError: C++ global 'global_var' is not initialized
+    >>> read_global_var_nogil()
+    Traceback (most recent call last):
+        ...
+    NameError: C++ global 'global_var' is not initialized
     >>> initialize_global_var()
     >>> read_global_var()
     -1
+    >>> read_global_var_nogil()
+    -1
     """
     print(global_var.getX())
+
+def read_global_var_nogil():
+    """
+    Tests are in read_global_var docstring
+    """
+    with nogil:
+        val = global_var.getX()
+    print(val)


Code review comments:
diff_hunk: @@ -3239,17 +3252,15 @@ static CYTHON_INLINE void __Pyx_RaiseClosureNameError(const char *varname) {
     PyErr_Format(PyExc_NameError, "free variable '%s' referenced before assignment in enclosing scope", varname);
 }
 
+/////////////// RaiseClosureNameErrorNogil.proto ///////////////
+static CYTHON_INLINE void __Pyx_RaiseClosureNameErrorNogil(const char *varname);/*proto*/
 
-/////////////// RaiseUnboundMemoryviewSliceNogil.proto ///////////////
-static void __Pyx_RaiseUnboundMemoryviewSliceNogil(const char *varname);/*proto*/
-
-/////////////// RaiseUnboundMemoryviewSliceNogil ///////////////
-//@requires: RaiseUnboundLocalError
+/////////////// RaiseClosureNameErrorNogil ///////////////
+//@requires: RaiseClosureNameError
 
-// Don't inline the function, it should really never be called in production
-static void __Pyx_RaiseUnboundMemoryviewSliceNogil(const char *varname) {
+static CYTHON_INLINE void __Pyx_RaiseClosureNameErrorNogil(const char *varname) {
path: Cython/Utility/ObjectHandling.c
text: @user1:
What makes this special enough for inlining?

@user1:
I think we can remove the `CYTHON_INLINE` from both functions (this and the original). If it's included, it's always called and used, and error handling code like this is never worth inlining.

@author:
Yeah was a copy-paste mistake on this one. But I agree - I'll remove it from the inner layer too.

diff_hunk: @@ -2961,28 +2961,25 @@ def put_error_if_neg(self, pos, value):
         return self.putln("if (%s < 0) %s" % (value, self.error_goto(pos)))
 
     def put_error_if_unbound(self, pos, entry, in_nogil_context=False, unbound_check_code=None):
+        nogil_tag = "Nogil" if in_nogil_context else ""
         if entry.from_closure:
-            func = '__Pyx_RaiseClosureNameError'
+            func = f'__Pyx_RaiseClosureNameError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseClosureNameError", "ObjectHandling.c"))
-        elif entry.type.is_memoryviewslice and in_nogil_context:
-            func = '__Pyx_RaiseUnboundMemoryviewSliceNogil'
-            self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseUnboundMemoryviewSliceNogil", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseClosureNameError{nogil_tag}", "ObjectHandling.c"))
         elif entry.type.is_cpp_class and entry.is_cglobal:
-            func = '__Pyx_RaiseCppGlobalNameError'
+            func = f'__Pyx_RaiseCppGlobalNameError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseCppGlobalNameError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseCppGlobalNameError{nogil_tag}", "ObjectHandling.c"))
         elif entry.type.is_cpp_class and entry.is_variable and not entry.is_member and entry.scope.is_c_class_scope:
             # there doesn't seem to be a good way to detecting an instance-attribute of a C class
             # (is_member is only set for class attributes)
-            func = '__Pyx_RaiseCppAttributeError'
+            func = f'__Pyx_RaiseCppAttributeError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseCppAttributeError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseCppAttributeError{nogil_tag}", "ObjectHandling.c"))
         else:
-            func = '__Pyx_RaiseUnboundLocalError'
+            func = f'__Pyx_RaiseUnboundLocalError{nogil_tag}'
             self.globalstate.use_utility_code(
-                UtilityCode.load_cached("RaiseUnboundLocalError", "ObjectHandling.c"))
+                UtilityCode.load_cached(f"RaiseUnboundLocalError{nogil_tag}", "ObjectHandling.c"))
path: Cython/Compiler/Code.py
text: @user1:
Now that you're at it, could you also extract the `use_utility_code(load_cached())` calls? It seems enough to fill a variable with the section name in the conditionals.

And you can attach the `Nogil` marker to the function name afterwards as well, no need to do it in each condition.