Repository: cython/cython
Pull request: Improve performance hints for nogil + pxd
In order to be called efficiently, these functions must have the exception specification set in the pxd file since Cython is unable to assume an implicit exception specification.

This improves the quality of the messages to draw attention to this detail.

Closes #6001

Problem statement:
[BUG] Getting "performance hint: ..."
### Describe the bug

I keep getting these "performance hint" bellow.

Goal is to run `io_uring_queue_init` with `nogil`, since nogil required other functions to be nogil as well I have set `trap_error` to be `nogil` as well and only trigger error when return value is `< 0`, why `with gil:` block is used.

I don't want to use `noexcept` for `trap_error` since half the function does need to trigger error.

### Code to reproduce the behaviour:

```cython
# liburing.pyx
cpdef int io_uring_queue_init(unsigned int entries, io_uring ring, unsigned int flags=0) nogil:
    return trap_error(io_uring_queue_init_c(entries, ring.ptr, flags))
```

```cython
# error.pyx
cpdef inline int trap_error(int no) nogil:
    if no >= 0:
        return no
    with gil:
        raise_error(no)


cdef inline void raise_error(signed int no=-1) except *:
    no = -errno or no
    cdef str error = strerror(-no).decode()
    raise OSError(-no, error)
```

```bash
performance hint: src/liburing/liburing.pyx:61:21: Exception check after calling 'trap_error' will always
require the GIL to be acquired. Declare 'trap_error' as 'noexcept' if you control the definition and
you're sure you don't want the function to raise
exceptions.
```

### Expected behaviour

_No response_

### OS

Linux

### Python version

3.12

### Cython version

3.0.8

### Additional context

_No response_

Hints from issue discussion:
I can't reproduce with the code given here.  To make it compile I slightly modified it to the below:

```
# liburing.pyx

from error cimport trap_error

cpdef int io_uring_queue_init(int x) nogil:
    return trap_error(x)
```

```
# error.pxd

cpdef int trap_error(int no) nogil
```

```
# error.pyx

from libc.errno cimport errno
from libc.string cimport strerror

cpdef inline int trap_error(int no) nogil:
    if no >= 0:
        return no
    with gil:
        raise_error(no)


cdef inline void raise_error(signed int no=-1) except *:
    no = -errno or no
    cdef str error = strerror(-no).decode()
    raise OSError(-no, error)

````

I get no performance warnings, which I believe is correct.
@da-woods I had the same code before(tried all the combination). When you first compile the code you get the `performance hint:` warning, the second time you don't get the message because the `./build/` folder needs to be deleted and compile again fresh to see the message again.

It kept tripping me as well...

p.s. you can see actual code at: https://github.com/YoSTEALTH/Liburing/tree/master/src/liburing
@da-woods have you tried the code after deleting `./build/`?
Sorry you're right. There is a ~warning~ performance hint.

That looks to be by design and that we don't assume the `-1` return value from functions declared in pxd files:

https://github.com/cython/cython/blob/03d982be010b7b94a3c3317c462b2b803ce916af/Cython/Compiler/Nodes.py#L729

I think unfortunately that is correct, and that you can compile something with:

```
# in the .pxd
cpdef int trap_error(int no) nogil
```

```
# in the .pyx file
cpdef inline int trap_error(int no) except -2 nogil:
```

so if you just see the pxd file you can't reason about the error code.

That suggests we need to improve the performance hint to tell you that since I don't think we can change the behaviour easily.

It might also we worth warning about "exception value in pyx but not in pxd" but I'm not sure about that.
That didn't work, was still getting the same message. So, I tried random stuff and this seem to work:

``` python

# error.pxd
cpdef int trap_error(int no) except -1 nogil
```

``` python
# error.pyx
cpdef inline int trap_error(int no) except -1 nogil:
```
Don't even know whats going on! But happy the message is gone :)
Your solution is right. The code I posted was an explanation, not something you should do
@da-woods Cool, Thank your very much for your amazing support.
I'm going to reopen this because I think we could improve the text of the performance hints here.

Commit message: Fix code style

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index cb1565536c8..5dfef749f1d 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -6511,7 +6511,8 @@ def generate_result_code(self, code):
                             PyrexTypes.write_noexcept_performance_hint(
                                 self.pos, code.funcstate.scope,
                                 function_name=perf_hint_entry.name if perf_hint_entry else None,
-                                void_return=self.type.is_void, is_call=True)
+                                void_return=self.type.is_void, is_call=True,
+                                is_from_pxd=(perf_hint_entry and perf_hint_entry.defined_in_pxd))
                         code.globalstate.use_utility_code(
                             UtilityCode.load_cached("ErrOccurredWithGIL", "Exceptions.c"))
                         exc_checks.append("__Pyx_ErrOccurredWithGIL()")
diff --git a/Cython/Compiler/PyrexTypes.py b/Cython/Compiler/PyrexTypes.py
index 1d77cec4a49..8b8c325dec3 100644
--- a/Cython/Compiler/PyrexTypes.py
+++ b/Cython/Compiler/PyrexTypes.py
@@ -5492,7 +5492,9 @@ def cap_length(s, max_prefix=63, max_len=1024):
     hash_prefix = hashlib.sha256(s.encode('ascii')).hexdigest()[:6]
     return '%s__%s__etc' % (hash_prefix, s[:max_len-17])
 
-def write_noexcept_performance_hint(pos, env, function_name=None, void_return=False, is_call=False):
+def write_noexcept_performance_hint(pos, env,
+                                    function_name=None, void_return=False, is_call=False,
+                                    is_from_pxd=False):
     if function_name:
         # we need it escaped everywhere we use it
         function_name = "'%s'" % function_name
@@ -5515,6 +5517,9 @@ def write_noexcept_performance_hint(pos, env, function_name=None, void_return=Fa
         solutions.append(
             "Use an 'int' return type on %s to allow an error code to be returned." %
             the_function)
+    if is_from_pxd and not void_return:
+        solutions.append(
+            "Declare any exception value explicitly for functions in pxd files.")
     if len(solutions) == 1:
         msg = "%s %s" % (msg, solutions[0])
     else:
diff --git a/Cython/Compiler/Symtab.py b/Cython/Compiler/Symtab.py
index 3137861dade..c47927a8830 100644
--- a/Cython/Compiler/Symtab.py
+++ b/Cython/Compiler/Symtab.py
@@ -8,7 +8,7 @@
 import operator
 
 from ..Utils import try_finally_contextmanager
-from .Errors import warning, error, InternalError
+from .Errors import warning, error, InternalError, performance_hint
 from .StringEncoding import EncodedString
 from . import Options, Naming
 from . import PyrexTypes
@@ -896,6 +896,17 @@ def declare_cfunction(self, name, type, pos,
                 elif not in_pxd and entry.defined_in_pxd and type.compatible_signature_with(entry.type):
                     # TODO: check that this was done by a signature optimisation and not a user error.
                     #warning(pos, "Function signature does not match previous declaration", 1)
+
+                    # Cython can't assume anything about cimported functions declared without
+                    # an exception value. This is a performance problem mainly for nogil functions.
+                    if entry.type.nogil and entry.type.exception_value is None and type.exception_value:
+                        performance_hint(
+                            entry.pos,
+                            f"No exception value declared for '{entry.name}' in pxd file.\n"
+                            "Users cimporting this function and calling it without the gil "
+                            f"will always require an exception check.\n"
+                            "Suggest adding an explicit exception value.",
+                            self)
                     entry.type = type
                 else:
                     error(pos, "Function signature does not match previous declaration")
diff --git a/tests/compile/nogil_perf_hints.pyx b/tests/compile/nogil_perf_hints.pyx
new file mode 100644
index 00000000000..191fe954615
--- /dev/null
+++ b/tests/compile/nogil_perf_hints.pyx
@@ -0,0 +1,20 @@
+# mode: compile
+# tag: perf_hints
+
+# Compile only to avoid needing to compile definitions of the functions
+# declared in another pxd file
+
+from nogil_perf_hints_pxd cimport f_has_except_value, f_missing_except_value, f_noexcept
+
+def test():
+    with nogil:
+        # should not generate performance hints
+        f_has_except_value()
+        f_noexcept()
+        # should generate performance hints.
+        # (Unfortunately it's difficult to check the extra information on the 2nd+ line of the hint)
+        f_missing_except_value()
+
+_PERFORMANCE_HINTS = """
+16:30: Exception check after calling 'f_missing_except_value' will always require the GIL to be acquired.
+"""
diff --git a/tests/compile/nogil_perf_hints_pxd.pxd b/tests/compile/nogil_perf_hints_pxd.pxd
new file mode 100644
index 00000000000..25ec4f3d705
--- /dev/null
+++ b/tests/compile/nogil_perf_hints_pxd.pxd
@@ -0,0 +1,3 @@
+cdef int f_has_except_value() nogil except -1
+cdef int f_missing_except_value() nogil
+cdef int f_noexcept() nogil noexcept
diff --git a/tests/run/nogil.pxd b/tests/run/nogil.pxd
index 60481437d6e..bc91fdfd955 100644
--- a/tests/run/nogil.pxd
+++ b/tests/run/nogil.pxd
@@ -1 +1,6 @@
 cdef void voidexceptnogil_in_pxd() nogil
+
+# These definitions are unhelpful to people cimporting
+# them because the exception value isn't in the pxd.
+cdef int f_in_pxd1() nogil
+cdef int f_in_pxd2() nogil
diff --git a/tests/run/nogil.pyx b/tests/run/nogil.pyx
index feed30ed324..1a1ef26f2ae 100644
--- a/tests/run/nogil.pyx
+++ b/tests/run/nogil.pyx
@@ -41,6 +41,12 @@ cpdef void release_gil_in_nogil2() nogil:
     with nogil:
         pass
 
+cdef int f_in_pxd1() nogil except -1:
+    return 0
+
+cdef int f_in_pxd2() nogil:  # implicit except -1?
+    return 0
+
 def test_release_gil_in_nogil():
     """
     >>> test_release_gil_in_nogil()
@@ -197,22 +203,33 @@ def test_performance_hint_nogil():
         # not (since it's in an external pxd we don't control)
         voidexceptnogil_in_other_pxd()
 
+def test_declared_in_pxd():
+    """
+    >>> test_declared_in_pxd()
+    """
+    with nogil:
+        # no warnings here because we're in the same file as the declaration
+        f_in_pxd1()
+        f_in_pxd2()
+
 
 # Note that we're only able to check the first line of the performance hint
 _PERFORMANCE_HINTS = """
+5:18: No exception value declared for 'f_in_pxd1' in pxd file.
+6:18: No exception value declared for 'f_in_pxd2' in pxd file.
 20:9: Exception check after calling 'f' will always require the GIL to be acquired.
 24:5: Exception check on 'f' will always require the GIL to be acquired.
 34:5: Exception check on 'release_gil_in_nogil' will always require the GIL to be acquired.
 39:6: Exception check on 'release_gil_in_nogil2' will always require the GIL to be acquired.
-49:28: Exception check after calling 'release_gil_in_nogil' will always require the GIL to be acquired.
-51:29: Exception check after calling 'release_gil_in_nogil2' will always require the GIL to be acquired.
-55:5: Exception check on 'get_gil_in_nogil' will always require the GIL to be acquired.
-59:6: Exception check on 'get_gil_in_nogil2' will always require the GIL to be acquired.
-68:24: Exception check after calling 'get_gil_in_nogil' will always require the GIL to be acquired.
-70:25: Exception check after calling 'get_gil_in_nogil2' will always require the GIL to be acquired.
-133:5: Exception check on 'copy_array_exception' will always require the GIL to be acquired.
-184:28: Exception check after calling 'copy_array_exception' will always require the GIL to be acquired.
-187:5: Exception check on 'voidexceptnogil_in_pxd' will always require the GIL to be acquired.
-195:30: Exception check after calling 'voidexceptnogil_in_pxd' will always require the GIL to be acquired.
-198:36: Exception check after calling 'voidexceptnogil_in_other_pxd' will always require the GIL to be acquired.
+55:28: Exception check after calling 'release_gil_in_nogil' will always require the GIL to be acquired.
+57:29: Exception check after calling 'release_gil_in_nogil2' will always require the GIL to be acquired.
+61:5: Exception check on 'get_gil_in_nogil' will always require the GIL to be acquired.
+65:6: Exception check on 'get_gil_in_nogil2' will always require the GIL to be acquired.
+74:24: Exception check after calling 'get_gil_in_nogil' will always require the GIL to be acquired.
+76:25: Exception check after calling 'get_gil_in_nogil2' will always require the GIL to be acquired.
+139:5: Exception check on 'copy_array_exception' will always require the GIL to be acquired.
+190:28: Exception check after calling 'copy_array_exception' will always require the GIL to be acquired.
+193:5: Exception check on 'voidexceptnogil_in_pxd' will always require the GIL to be acquired.
+201:30: Exception check after calling 'voidexceptnogil_in_pxd' will always require the GIL to be acquired.
+204:36: Exception check after calling 'voidexceptnogil_in_other_pxd' will always require the GIL to be acquired.
 """


Code review comments:
diff_hunk: @@ -41,6 +41,12 @@ cpdef void release_gil_in_nogil2() nogil:
     with nogil:
         pass
 
+cdef int f_in_pxd1() nogil except -1:
+    return 0
+
+cdef int f_in_pxd2() nogil:  # implicit except -1?
+    return 0
+
path: tests/run/nogil.pyx
text: @user1:
Can't these go to the end, so that we don't need to update all performance hint warning lines?