variable '__pyx_tstate' is used uninitialized whenever 'if' condition is true

[frazenshtein]

Hi! I'm facing problem with generating valid c code with enabled linetracing for code like this:

try:
    return LoadJsonFromString(s, len(s))
except Exception as e:
   raise ValueError(str(e))

cython generates:

  /* "library/python/json/loads.pyx":9
 *         s = s.encode('utf-8')
 * 
 *     try:             # <<<<<<<<<<<<<<
 *         return LoadJsonFromString(s, len(s))
 *     except Exception as e:
 */
  __Pyx_TraceLine(9,0,__PYX_ERR(0, 9, __pyx_L4_error))
  {
    __Pyx_PyThreadState_declare
    __Pyx_PyThreadState_assign
    __Pyx_ExceptionSave(&__pyx_t_5, &__pyx_t_6, &__pyx_t_7);
    __Pyx_XGOTREF(__pyx_t_5);
    __Pyx_XGOTREF(__pyx_t_6);
    __Pyx_XGOTREF(__pyx_t_7);
    /*try:*/ {
[...skipped code for generated LoadJsonFromString call...]
      /* "library/python/json/loads.pyx":9
 *         s = s.encode('utf-8')
 * 
 *     try:             # <<<<<<<<<<<<<<
 *         return LoadJsonFromString(s, len(s))
 *     except Exception as e:
 */
    __pyx_L4_error:;
    __Pyx_XDECREF(__pyx_t_3); __pyx_t_3 = 0;
    __Pyx_XDECREF(__pyx_t_4); __pyx_t_4 = 0;
[...skipped code for exception handling...]
    /* "library/python/json/loads.pyx":9
 *         s = s.encode('utf-8')
 * 
 *     try:             # <<<<<<<<<<<<<<
 *         return LoadJsonFromString(s, len(s))
 *     except Exception as e:
 */
    __Pyx_XGIVEREF(__pyx_t_5);
    __Pyx_XGIVEREF(__pyx_t_6);
    __Pyx_XGIVEREF(__pyx_t_7);
    __Pyx_ExceptionReset(__pyx_t_5, __pyx_t_6, __pyx_t_7);
    goto __pyx_L1_error;
    __pyx_L8_try_return:;
    __Pyx_XGIVEREF(__pyx_t_5);
    __Pyx_XGIVEREF(__pyx_t_6);
    __Pyx_XGIVEREF(__pyx_t_7);
    __Pyx_ExceptionReset(__pyx_t_5, __pyx_t_6, __pyx_t_7);
    goto __pyx_L0;
  }

compiler error:

library/python/json/loads.pyx.cpp:1393:3: error: variable '__pyx_tstate' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
  __Pyx_TraceLine(9,0,__PYX_ERR(0, 9, __pyx_L4_error))
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
library/python/json/loads.pyx.cpp:1090:19: note: expanded from macro '__Pyx_TraceLine'
              if (unlikely(ret)) goto_error;\
                  ^~~~~~~~~~~~~
library/python/json/loads.pyx.cpp:770:23: note: expanded from macro 'unlikely'
  #define unlikely(x) __builtin_expect(!!(x), 0)
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
library/python/json/loads.pyx.cpp:1482:5: note: uninitialized use occurs here
    __Pyx_ExceptionReset(__pyx_t_5, __pyx_t_6, __pyx_t_7);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
library/python/json/loads.pyx.cpp:1119:70: note: expanded from macro '__Pyx_ExceptionReset'
#define __Pyx_ExceptionReset(type, value, tb)  __Pyx__ExceptionReset(__pyx_tstate, type, value, tb)
                                                                     ^~~~~~~~~~~~
library/python/json/loads.pyx.cpp:1393:3: note: remove the 'if' if its condition is always false
  __Pyx_TraceLine(9,0,__PYX_ERR(0, 9, __pyx_L4_error))
  ^
library/python/json/loads.pyx.cpp:1090:15: note: expanded from macro '__Pyx_TraceLine'
              if (unlikely(ret)) goto_error;\
              ^
library/python/json/loads.pyx.cpp:1395:5: note: variable '__pyx_tstate' is declared here
    __Pyx_PyThreadState_declare
    ^
library/python/json/loads.pyx.cpp:872:38: note: expanded from macro '__Pyx_PyThreadState_declare'
#define __Pyx_PyThreadState_declare  PyThreadState *__pyx_tstate;
                                     ^

which says that macro __Pyx_TraceLine(9,0,__PYX_ERR(0, 9, __pyx_L4_error)) expands into code that will jumps to __pyx_L4_error label if __Pyx_call_line_trace_func returns nonzero value. Jumping to __pyx_L4_error label will skip __pyx_tstate declaration:

    __Pyx_PyThreadState_declare
    __Pyx_PyThreadState_assign

which will be used later in the code expanded from __Pyx_ExceptionReset macro (__Pyx__ExceptionReset(__pyx_tstate, type, value, tb))

[gabrieldemarmiesse]

Hi, can you please provide a short standalone script to reproduce the bug? I cannot reproduce it.

Thanks.

[frazenshtein]

Thanks for your quick response! Will something like this be enough?
cat lib.pyx

cdef num(x):
    try:
        return x
    except AttributeError as e:
        raise ValueError(str(e))

git clone https://github.com/cython/cython
python cython/cython.py -X linetrace=True --cplus lib.pyx -o lib.pyx.cpp
clang++-5.0 -Wall -Werror -I/usr/include/python2.7/ -DCYTHON_TRACE=1 -DCYTHON_TRACE_NOGIL=1 -c lib.pyx.cpp

lib.pyx.cpp:1293:3: error: variable '__pyx_tstate' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
  __Pyx_TraceLine(2,0,__PYX_ERR(0, 2, __pyx_L3_error))
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib.pyx.cpp:1108:19: note: expanded from macro '__Pyx_TraceLine'
              if (unlikely(ret)) goto_error;\
                  ^~~~~~~~~~~~~
lib.pyx.cpp:788:23: note: expanded from macro 'unlikely'
  #define unlikely(x) __builtin_expect(!!(x), 0)
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
lib.pyx.cpp:1371:5: note: uninitialized use occurs here
    __Pyx_ExceptionReset(__pyx_t_1, __pyx_t_2, __pyx_t_3);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib.pyx.cpp:1130:70: note: expanded from macro '__Pyx_ExceptionReset'
#define __Pyx_ExceptionReset(type, value, tb)  __Pyx__ExceptionReset(__pyx_tstate, type, value, tb)
                                                                     ^~~~~~~~~~~~
lib.pyx.cpp:1293:3: note: remove the 'if' if its condition is always false
  __Pyx_TraceLine(2,0,__PYX_ERR(0, 2, __pyx_L3_error))
  ^
lib.pyx.cpp:1108:15: note: expanded from macro '__Pyx_TraceLine'
              if (unlikely(ret)) goto_error;\
              ^
lib.pyx.cpp:1295:5: note: variable '__pyx_tstate' is declared here
    __Pyx_PyThreadState_declare
    ^
lib.pyx.cpp:890:38: note: expanded from macro '__Pyx_PyThreadState_declare'
#define __Pyx_PyThreadState_declare  PyThreadState *__pyx_tstate;
                                     ^
1 error generated.

[gabrieldemarmiesse]

I tried to make a setup.py rather than manually run the cython/cython.py and clang.

This works.

lib.pyx

cdef num(x):
    try:
        return x
    except AttributeError as e:
        raise ValueError(str(e))

setup.py

from distutils.core import setup
from Cython.Build import cythonize
from distutils.extension import Extension

extensions = [
    Extension("lib", ["lib.pyx"], define_macros=[('CYTHON_TRACE', '1'),
                                                 ('DCYTHON_TRACE_NOGIL', '1')])
]

setup(
    name = "hello",
    ext_modules = cythonize(extensions, compiler_directives={'linetrace': True}),
)

Command to run:

python setup.py build_ext --inplace

Would this workaround suits you?

I'm in no way an expert in Cython, I'm only trying to help the maintainers. So maybe I'm missing something obvious.

[frazenshtein]

Yes, your example works for me too, but it's obviously hiding real problem. It uses gcc as compiler by default and due 14 years old gcc bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501) it doesn't detect uninitialized variable use.
To reproduce problem you can run command:

CC=clang++-5.0 python setup.py build_ext --inplace

I have found similar issue - #2269. Pitrou didn't provide enough information, but it seems to me that gcc (at least 4.9.4) is capable to detect some cases of uninitialized variable use. It does't look like my try-except-raise case, but i believe that it's different face of the same problem and cython generates code like

  __Pyx_TraceLine(9,0,__PYX_ERR(0, 9, __pyx_L4_error))
  {
    __Pyx_PyThreadState_declare
    __Pyx_PyThreadState_assign

which may lead to jump over __Pyx_PyThreadState_declare and __Pyx_PyThreadState_assign if__Pyx_call_line_trace_func() returns nonzero value.

My workaround:

+++ b/Cython/Utility/Profile.c
@@ -193,6 +193,7 @@
   #ifdef WITH_THREAD
   #define __Pyx_TraceLine(lineno, nogil, goto_error)                                       \
   if (likely(!__Pyx_use_tracing)); else {                                                  \
+      if ((1)); else goto_error;                                                           \
       if (nogil) {                                                                         \
           if (CYTHON_TRACE_NOGIL) {                                                        \
               int ret = 0;                                                                 \
@@ -203,23 +204,27 @@
                   ret = __Pyx_call_line_trace_func(tstate, $frame_cname, lineno);          \
               }                                                                            \
               PyGILState_Release(state);                                                   \
-              if (unlikely(ret)) goto_error;                                               \
+              // It's a bug - see https://github.com/cython/cython/issues/2274             \
+              if (unlikely(ret)) { fprintf(stderr, "cython: line_trace_func returned %d\n", ret); }  \
           }                                                                                \
       } else {                                                                             \
           PyThreadState* tstate = __Pyx_PyThreadState_Current;                             \
           if (unlikely(tstate->use_tracing && tstate->c_tracefunc && $frame_cname->f_trace)) { \
               int ret = __Pyx_call_line_trace_func(tstate, $frame_cname, lineno);          \
-              if (unlikely(ret)) goto_error;                                               \
+              // It's a bug - see https://github.com/cython/cython/issues/2274             \
+              if (unlikely(ret)) { fprintf(stderr, "cython: line_trace_func returned %d\n", ret); }  \
           }                                                                                \
       }                                                                                    \
   }
   #else
   #define __Pyx_TraceLine(lineno, nogil, goto_error)                                       \
   if (likely(!__Pyx_use_tracing)); else {                                                  \
+      if ((1)); else goto_error;                                                           \
       PyThreadState* tstate = __Pyx_PyThreadState_Current;                                 \
       if (unlikely(tstate->use_tracing && tstate->c_tracefunc && $frame_cname->f_trace)) { \
           int ret = __Pyx_call_line_trace_func(tstate, $frame_cname, lineno);              \
-          if (unlikely(ret)) goto_error;                                                   \
+          // It's a bug - see https://github.com/cython/cython/issues/2274                 \
+          if (unlikely(ret)) { fprintf(stderr, "cython: line_trace_func returned %d\n", ret); } \
       }                                                                                    \
   }
   #endif

It looks valid for me, because i don't understand why tracing errors should affect executing code. So, if error occurs in tracing code, we just accept this fact and should move on.

[frazenshtein]

Cython generated code:

  /* "lib.pyx":2
 * cdef num(x):
 *     try:             # <<<<<<<<<<<<<<
 *         return x
 *     except AttributeError as e:
 */
  __Pyx_TraceLine(2,0,__PYX_ERR(0, 2, __pyx_L3_error))
  {
    __Pyx_PyThreadState_declare
    __Pyx_PyThreadState_assign
    __Pyx_ExceptionSave(&__pyx_t_1, &__pyx_t_2, &__pyx_t_3);
    __Pyx_XGOTREF(__pyx_t_1);
    __Pyx_XGOTREF(__pyx_t_2);
    __Pyx_XGOTREF(__pyx_t_3);
    /*try:*/ {

      /* "lib.pyx":3
 * cdef num(x):
 *     try:
 *         return x             # <<<<<<<<<<<<<<
 *     except AttributeError as e:
 *         raise ValueError(str(e))
 */
      __Pyx_TraceLine(3,0,__PYX_ERR(0, 3, __pyx_L3_error))
      __Pyx_XDECREF(__pyx_r);
      __Pyx_INCREF(__pyx_v_x);
      __pyx_r = __pyx_v_x;
      goto __pyx_L7_try_return;

      /* "lib.pyx":2
 * cdef num(x):
 *     try:             # <<<<<<<<<<<<<<
 *         return x
 *     except AttributeError as e:
 */
    }
    __pyx_L3_error:;

    /* "lib.pyx":4
 *     try:
 *         return x
 *     except AttributeError as e:             # <<<<<<<<<<<<<<
 *         raise ValueError(str(e))
 *
 */
    __Pyx_TraceLine(4,0,__PYX_ERR(0, 4, __pyx_L5_except_error))
    __pyx_t_4 = __Pyx_PyErr_ExceptionMatches(__pyx_builtin_AttributeError);

Cython generated code with expanded macro (with my 'expanded' comments):

// expanded __Pyx_TraceLine(2,0,__PYX_ERR(0, 2, __pyx_L3_error))
  if (__builtin_expect(!!(!__Pyx_use_tracing), 1)); else { if (0) { if (0) { int ret = 0; PyThreadState *tstate; PyGILState_STATE state = PyGILState_Ensure(); tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing &&
 tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { ret = __Pyx_call_line_trace_func(tstate, __pyx_frame, 2); } PyGILState_Release(state); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 2; __pyx_clineno =
 1301; goto __pyx_L3_error; }; } } else { PyThreadState* tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing && tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { int ret = __Pyx_call_line_trace_func(tstate, __py
x_frame, 2); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 2; __pyx_clineno = 1301; goto __pyx_L3_error; }; } } }
  {
// expanded  __Pyx_PyThreadState_declare
    PyThreadState *__pyx_tstate;
// expanded __Pyx_PyThreadState_assign
    __pyx_tstate = _PyThreadState_Current;
// expanded  __Pyx_ExceptionSave(&__pyx_t_1, &__pyx_t_2, &__pyx_t_3);
    __Pyx__ExceptionSave(__pyx_tstate, &__pyx_t_1, &__pyx_t_2, &__pyx_t_3);
                            ;
                            ;
                            ;
             {
# 1318 "lib.c"
// expanded  __Pyx_TraceLine(3,0,__PYX_ERR(0, 3, __pyx_L3_error))
      if (__builtin_expect(!!(!__Pyx_use_tracing), 1)); else { if (0) { if (0) { int ret = 0; PyThreadState *tstate; PyGILState_STATE state = PyGILState_Ensure(); tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing && tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { ret = __Pyx_call_line_trace_func(tstate, __pyx_frame, 3); } PyGILState_Release(state); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 3; __pyx_clineno = 1318; goto __pyx_L3_error; }; } } else { PyThreadState* tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing && tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { int ret = __Pyx_call_line_trace_func(tstate, __pyx_frame, 3); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 3; __pyx_clineno = 1318; goto __pyx_L3_error; }; } } }
      do { if ((__pyx_r) == ((void*)0)) ; else do { if ( --((PyObject*)(__pyx_r))->ob_refcnt != 0) ; else ( (*(((PyObject*)((PyObject *)(__pyx_r)))->ob_type)->tp_dealloc)((PyObject *)((PyObject *)(__pyx_r)))); } while (0); } while (0);
      ( ((PyObject*)(__pyx_v_x))->ob_refcnt++);
      __pyx_r = __pyx_v_x;
      goto __pyx_L7_try_return;
    }
    __pyx_L3_error:;
# 1340 "lib.c"
// expanded __Pyx_TraceLine(4,0,__PYX_ERR(0, 4, __pyx_L5_except_error))
    if (__builtin_expect(!!(!__Pyx_use_tracing), 1)); else { if (0) { if (0) { int ret = 0; PyThreadState *tstate; PyGILState_STATE state = PyGILState_Ensure(); tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing && tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { ret = __Pyx_call_line_trace_func(tstate, __pyx_frame, 4); } PyGILState_Release(state); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 4; __pyx_clineno = 1340; goto __pyx_L5_except_error; }; } } else { PyThreadState* tstate = _PyThreadState_Current; if (__builtin_expect(!!(tstate->use_tracing && tstate->c_tracefunc && __pyx_frame->f_trace), 0)) { int ret = __Pyx_call_line_trace_func(tstate, __pyx_frame, 4); if (__builtin_expect(!!(ret), 0)) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 4; __pyx_clineno = 1340; goto __pyx_L5_except_error; }; } } }
// expanded __pyx_t_4 = __Pyx_PyErr_ExceptionMatches(__pyx_builtin_AttributeError);
    __pyx_t_4 = __Pyx_PyErr_ExceptionMatchesInState(__pyx_tstate, __pyx_builtin_AttributeError);
    if (__pyx_t_4) {
      __Pyx_AddTraceback("lib.num", __pyx_clineno, __pyx_lineno, __pyx_filename);
      if (__Pyx__GetException(__pyx_tstate, &__pyx_t_5, &__pyx_t_6, &__pyx_t_7) < 0) { __pyx_filename = __pyx_f[0]; __pyx_lineno = 4; __pyx_clineno = 1344; goto __pyx_L5_except_error; }
                             ;
                             ;
                             ;

As you can see at the first line we can perform goto __pyx_L3_error and skip PyThreadState *__pyx_tstate; __pyx_tstate = _PyThreadState_Current;, which is the cause of problem, because we will use __pyx_tstate later

[gabrieldemarmiesse]

Thanks for all those details.

I don't have enough knowledge to do a PR, but hopefully someone will figure out the error and fix it with all the information you provided.

[frazenshtein]

Some additional info, which might be useful: it looks like pure UB and behaviour may vary from compiler to compiler. A minimalist example:

#include <stdio.h>

int main(int c, char** v)
{
  if (c > 1) {
      fprintf(stderr, "goto __pyx_L3_error\n");
      goto __pyx_L3_error;
  }
  {
      fprintf(stderr, "declare *p\n");
      int *__pyx_tstate;
      __pyx_tstate = &c;
__pyx_L3_error:;
      fprintf(stderr, "val: %d\n", *__pyx_tstate);
  }
  return 0;
}

~> g++-7 -O2 1.cpp; ./a.out 2;
goto __pyx_L3_error
val: 2

~> g++-7 1.cpp; ./a.out 2;    
goto __pyx_L3_error
Segmentation fault (core dumped)

~> clang++-5.0 -O2 1.cpp; ./a.out 2; 
goto __pyx_L3_error
val: 2

~> clang++-5.0 1.cpp; ./a.out 2;
goto __pyx_L3_error
val: -1991643855

[pitrou]

I see this with clang 6.0.1 as well.

[scoder]

Thank you for the thorough analysis, @frazenshtein. It wasn't really easy to write a test for this, and I couldn't reproduce a crash on my side, but the code now handles the case of errors in the tracing function correctly. See 7ab11ec.