Cython: Broken scikit-image build with 3.0a3 : redeclaration of ‘__pyx_gilstate_save’ with no linkage

[emmanuelle]

The scikit-image pre builds is broken since Cython 3.0a3 was released yesterday.

The error is as follows:

skimage/graph/heap.c: In function ‘__pyx_f_7skimage_5graph_4heap_10BinaryHeap__add_or_remove_level’:
skimage/graph/heap.c:3299:20: error: redeclaration of ‘__pyx_gilstate_save’ with no linkage
   PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
                    ^~~~~~~~~~~~~~~~~~~
skimage/graph/heap.c:3296:20: note: previous declaration of ‘__pyx_gilstate_save’ was here
   PyGILState_STATE __pyx_gilstate_save;
                    ^~~~~~~~~~~~~~~~~~~

corresponding to this part of heap.pyx (https://github.com/scikit-image/scikit-image/blob/master/skimage/graph/heap.pyx#L190)

cdef void _add_or_remove_level(self, LEVELS_T add_or_remove) nogil:
        # init indexing ints
        cdef INDEX_T i, i1, i2, n

        # new amount of levels
        cdef LEVELS_T new_levels = self.levels + add_or_remove

        # allocate new arrays
        cdef INDEX_T number = 2**new_levels
        cdef VALUE_T *values
        cdef REFERENCE_T *references
        values = <VALUE_T *>malloc(number * 2 * sizeof(VALUE_T))
        references = <REFERENCE_T *>malloc(number * sizeof(REFERENCE_T))
        if values is NULL or references is NULL:
            free(values)
            free(references)
            with gil:
                raise MemoryError()

When I compare the generated C code with 3.0a1 and 3.0a2, only these part changed
3.0a1

#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
#endif

and
3.0a3

#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save;
#endif
#ifdef WITH_THREAD
PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
#endif

so indeed __pyx_gilstate_save is defined twice.

Any advice about what is going on will be much appreciated :-).

If it helps, here is a bit more of the C code (for 3.0a3):

static void __pyx_f_7skimage_5graph_4heap_10BinaryHeap__add_or_remove_level(struct __pyx_obj_7skimage_5graph_4heap_BinaryHeap *__pyx_v_self, __pyx_t_7skimage_5graph_4heap_LEVELS_T __pyx_v_add_or_remove) {
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i1;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_i2;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_n;
  __pyx_t_7skimage_5graph_4heap_LEVELS_T __pyx_v_new_levels;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_v_number;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_v_values;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_v_references;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_v_old_values;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_v_old_references;
  __Pyx_RefNannyDeclarations
  int __pyx_t_1;
  int __pyx_t_2;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_3;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_4;
  __pyx_t_7skimage_5graph_4heap_INDEX_T __pyx_t_5;
  __pyx_t_7skimage_5graph_4heap_VALUE_T *__pyx_t_6;
  __pyx_t_7skimage_5graph_4heap_REFERENCE_T *__pyx_t_7;
  #ifdef WITH_THREAD
  PyGILState_STATE __pyx_gilstate_save;
  #endif
  #ifdef WITH_THREAD
  PyGILState_STATE __pyx_gilstate_save = __Pyx_PyGILState_Ensure();
  #endif
  __Pyx_RefNannySetupContext("_add_or_remove_level", 0);
  #ifdef WITH_THREAD
  __Pyx_PyGILState_Release(__pyx_gilstate_save);
  #endif

[scoder]

Thanks for the detailed report @emmanuelle. The change in 7d99b0f085bb254e9a9b4a9fe50aefe133837a81 didn't work in nogil functions/methods that received Python arguments as input. Fix pushed, test added.

[scoder]

Note, BTW, that the method signature prevents exception propagation:
cdef void _add_or_remove_level(self, LEVELS_T add_or_remove) nogil:
Instead, either change the return type from void to something else, e.g. int (with except -1 or similar), or declare the method as except * to make the caller always check for exceptions.

Oh, and the with gil: is no longer required for the raise since 0.29. It's injected automatically when raise is used in a nogil section.

[emmanuelle]

Thank you very much for the fast and detailed answer (as always ;-)), @scoder! We will improve this function as you suggested.

[da-woods]

Should having a function that can raise an exception but can't return it generate a warning when cythonized? It seems like Cython can identify this (since it generates code for warn-unraisable).

Sub-questions might be: is the identification good enough to avoid too many false positives? are the good reasons why users might want to write code that swallows exceptions like this (and if so is, would there be a good way to mark code like this e.g. except None so it doesn't create a warning)?

[scoder]

Cython generates a warning here, but with warning level 0, i.e. it's normally invisible. Setting Cython.Compiler.Errors.LEVEL = 0 will show it. (I guess there should be a better interface to that, preferably a keyword argument in cythonize()).