'with' statement doesn't compile on C++

[robertwb]

This crashes due to incorrect temp deallocation:

    with ContextManager(u"value") as x:
        return x

The generated code is:

  __pyx_1 = __Pyx_GetName(__pyx_m, __pyx_kp_ContextManager); if [goto](error)
  /* __pyx_t_6 allocated */
  __pyx_t_6 = PyTuple_New(1); if [goto](error)
  Py_INCREF(((PyObject *)__pyx_kp_8));
  PyTuple_SET_ITEM(__pyx_t_6, 0, ((PyObject *)__pyx_kp_8));
  /* __pyx_t_7 allocated */
  __pyx_t_7 = PyObject_Call(__pyx_1, ((PyObject *)__pyx_t_6), NULL); if [goto](error)
  Py_DECREF(__pyx_1); __pyx_1 = 0;
  Py_DECREF(((PyObject *)__pyx_t_6)); __pyx_t_6 = 0;
  /* __pyx_t_6 released */
  Py_XDECREF(__pyx_t_2);
  __pyx_t_2 = __pyx_t_7;
  __pyx_t_7 = 0;
...
  /*try:*/ {
    {
...
        Py_DECREF(__pyx_t_6); __pyx_t_6 = 0;
        Py_DECREF(__pyx_t_7); __pyx_t_7 = 0;
        Py_DECREF(__pyx_t_8); __pyx_t_8 = 0;
        goto __pyx_L5;

Note how __pyx_t_7 is DECREF-ed on return, as it is not released after retrieving the context manager in the temp variable and assigning it to a new temp.

There's a disabled test case in run/withstat.pyx that tests for this.

Migrated from http://trac.cython.org/ticket/151

[robertwb]

scoder changed description from

This crashes due to incorrect temp deallocation:

with ContextManager(u"value") as x:
    return x

The generated code is:

pyx_1 = __Pyx_GetName(pyx_m, pyx_kp_ContextManager); if goto /* pyx_t6 allocated / pyx_t_6 = PyTuple_New(1); if goto Py_INCREF(((PyObject *)pyx_kp_8)); PyTuple_SET_ITEM(pyx_t_6, 0, ((PyObject *)pyx_kp8)); / pyx_t7 allocated / __pyx_t_7 = PyObject_Call(pyx_1, ((PyObject )pyx_t_6), NULL); if goto Py_DECREF(__pyx_1); pyx_1 = 0; Py_DECREF(((PyObject )pyx_t_6)); pyx_t6 = 0; / pyx_t6 released / Py_XDECREF(pyx_t_2); pyx_t_2 = pyx_t_7; pyx_t_7 = 0; ... /_try:*/ { { ... Py_DECREF(pyx_t_6); pyx_t_6 = 0; Py_DECREF(pyx_t_7); pyx_t_7 = 0; Py_DECREF(pyx_t_8); pyx_t_8 = 0; goto pyx_L5;

Note how __pyx_t_7 is DECREF-ed on return, as it is not released after retrieving the context manager in the temp variable and assigning it to a new temp.

There's a disabled test case in run/withstat.pyx that tests for this. to

This crashes due to incorrect temp deallocation:

    with ContextManager(u"value") as x:
        return x

The generated code is:

  __pyx_1 = __Pyx_GetName(__pyx_m, __pyx_kp_ContextManager); if [goto](error)
  /* __pyx_t_6 allocated */
  __pyx_t_6 = PyTuple_New(1); if [goto](error)
  Py_INCREF(((PyObject *)__pyx_kp_8));
  PyTuple_SET_ITEM(__pyx_t_6, 0, ((PyObject *)__pyx_kp_8));
  /* __pyx_t_7 allocated */
  __pyx_t_7 = PyObject_Call(__pyx_1, ((PyObject *)__pyx_t_6), NULL); if [goto](error)
  Py_DECREF(__pyx_1); __pyx_1 = 0;
  Py_DECREF(((PyObject *)__pyx_t_6)); __pyx_t_6 = 0;
  /* __pyx_t_6 released */
  Py_XDECREF(__pyx_t_2);
  __pyx_t_2 = __pyx_t_7;
  __pyx_t_7 = 0;
...
  /*try:*/ {
    {
...
        Py_DECREF(__pyx_t_6); __pyx_t_6 = 0;
        Py_DECREF(__pyx_t_7); __pyx_t_7 = 0;
        Py_DECREF(__pyx_t_8); __pyx_t_8 = 0;
        goto __pyx_L5;

Note how __pyx_t_7 is DECREF-ed on return, as it is not released after retrieving the context manager in the temp variable and assigning it to a new temp.

There's a disabled test case in run/withstat.pyx that tests for this.

[robertwb]

scoder changed The problem described above is fixed in rev 753f53660cc1, however, the test case still crashes due to temp handling.

http://hg.cython.org/cython-devel/rev/753f53660cc1

The 'return' statement is a clear example of goto being harmful. It cleans up all temps, but since it's surrounded by a try-finally in the case of a 'with' statement, the code in the 'finally' clause accesses temps that the return statement already cleared.

[robertwb]

scoder changed I think the real problem here are long-living temps. They are not made for living longer than one statement or one expression. The 'temps' of a TreeFragment should be held in a kind of specially named, block-local variable, not in a temp.

[robertwb]

scoder changed component from Build System to Code Generation

[robertwb]

@robertwb changed priority from major to critical Temps can live longer than a statement--for example the temps used in a for loop.

The issue is (I think) that at any state there should be a running list of "possibly used temps" and it needs to be updated when things are decref'ed.

[robertwb]

@dagss changed owner from somebody to dagss status from new to assigned

[robertwb]

@dagss changed resolution to fixed status from assigned to closed OK I went back to the older strategy of having TreeFragment use NameNodes, just with strange names. This will fail if the user uses a variable name on the form __tmpvar_3, so should perhaps be done something about. Follow-up in http://cython.trac.org/ticket/197.

[robertwb]

@dagss changed resolution from fixed to empty status from closed to reopened There's still an issue with excinfo_temp (~ ParseTreeTransforms.py:550)

[robertwb]

@dagss changed resolution to fixed status from reopened to closed OK done: http://hg.cython.org/cython-devel/rev/9d0ac0d9659b

Sorry about the quality of these commits; but at least now it is no longer blocking up and one can revisit in http://cython.trac.org/ticket/197.

[robertwb]

@dagss changed resolution from fixed to empty status from closed to reopened summary from

'with' statement crashes on contained return statement to

'with' statement doesn't compile on C++ This is very odd: 'with' statement doesn't compile on C++, due to the variables not getting declared. They ''are'' declared in the C test run though.

[robertwb]

@dagss changed resolution to fixed status from reopened to closed Duh! http://hg.cython.org/cython-devel/rev/3304bc6fd2c3