merge 3.4-slp (Stackless python#117, fix frame reference leaks)

Author: Anselm Kruis

Objects/object.c

@@ -3,6 +3,9 @@
 
 #include "Python.h"
 #include "frameobject.h"
+#ifdef STACKLESS
+#include "core/stackless_impl.h"
+#endif
 
 #ifdef __cplusplus
 extern "C" {
@@ -1762,7 +1765,11 @@ _Py_Dealloc(PyObject *op)
 {
     destructor dealloc = Py_TYPE(op)->tp_dealloc;
     _Py_ForgetReference(op);
+#ifdef STACKLESS
+    SLP_WITH_VALID_CURRENT_FRAME((*dealloc)(op));
+#else
     (*dealloc)(op);
+#endif
 }
 
 /* Print all live objects.  Because PyObject_Print is called, the

Objects/typeobject.c

@@ -6356,8 +6356,7 @@ slp_tp_init_callback(PyFrameObject *f, int exc, PyObject *retval)
             cf->ob1 = NULL;
         }
     }
-    ts->frame = f;
-    Py_DECREF(cf);
+    SLP_STORE_NEXT_FRAME(ts, f);
     return STACKLESS_PACK(ts, retval);
 }
 #endif
@@ -6369,17 +6368,23 @@ slot_tp_init(PyObject *self, PyObject *args, PyObject *kwds)
     _Py_IDENTIFIER(__init__);
     PyObject *meth = lookup_method(self, &PyId___init__);
     PyObject *res;
+#ifdef STACKLESS
+    PyCFrameObject *f = NULL;
+#endif
 
     if (meth == NULL)
         return -1;
 #ifdef STACKLESS
     if (stackless) {
-        PyCFrameObject *f = slp_cframe_new(slp_tp_init_callback, 1);
+        f = slp_cframe_new(slp_tp_init_callback, 1);
         if (f == NULL)
             return -1;
         Py_INCREF(self);
         f->ob1 = self;
-        PyThreadState_GET()->frame = (PyFrameObject *) f;
+        SLP_SET_CURRENT_FRAME(PyThreadState_GET(), (PyFrameObject *) f);
+        /* f contains the only counted reference to current frame. This reference
+         * keeps the fame alive during the following PyObject_Call().
+         */
     }
 #endif
     STACKLESS_PROMOTE_ALL();
@@ -6388,10 +6393,16 @@ slot_tp_init(PyObject *self, PyObject *args, PyObject *kwds)
     Py_DECREF(meth);
 #ifdef STACKLESS
     if (stackless && !STACKLESS_UNWINDING(res)) {
-        /* required, because added a C-frame */
-        STACKLESS_PACK(PyThreadState_GET(), res);
+        /* required, because we added a C-frame */
+        PyThreadState *ts = PyThreadState_GET();
+        STACKLESS_PACK(ts, res);
+        assert(f);
+        assert((PyFrameObject *)f == SLP_CURRENT_FRAME(ts));
+        SLP_STORE_NEXT_FRAME(ts, (PyFrameObject *)f);
+        Py_DECREF(f);
         return STACKLESS_UNWINDING_MAGIC;
     }
+    Py_XDECREF(f);
     if (STACKLESS_UNWINDING(res)) {
         return STACKLESS_UNWINDING_MAGIC;
     }

Python/ceval.c

@@ -1167,8 +1167,7 @@ PyEval_EvalFrameEx(PyFrameObject *f, int throwflag)
     /* push frame */
     if (Py_EnterRecursiveCall("")) {
         Py_XDECREF(retval);
-        tstate->frame = f->f_back;
-        Py_DECREF(f);
+        SLP_STORE_NEXT_FRAME(tstate, f->f_back);
         return NULL;
     }
 #else
@@ -1221,7 +1220,7 @@ PyEval_EvalFrameEx(PyFrameObject *f, int throwflag)
     Py_XDECREF(retval);
     Py_LeaveRecursiveCall();
     f->f_executing = 0;
-    tstate->frame = f->f_back;
+    SLP_STORE_NEXT_FRAME(tstate, f->f_back);
     return NULL;
 }
 
@@ -4080,9 +4079,8 @@ slp_eval_frame_value(PyFrameObject *f, int throwflag, PyObject *retval)
 #else
     Py_LeaveRecursiveCall();
     f->f_executing = 0;
-    tstate->frame = f->f_back;
+    SLP_STORE_NEXT_FRAME(tstate, f->f_back);
 
-    Py_DECREF(f);
     return _Py_CheckFunctionResult(NULL, retval, "PyEval_EvalFrameEx");
 
 stackless_setup_with:
@@ -4108,16 +4106,23 @@ slp_eval_frame_value(PyFrameObject *f, int throwflag, PyObject *retval)
     /* the -1 is to adjust for the f_lasti change.
        (look for the word 'Promise' above) */
     f->f_lasti = INSTR_OFFSET() - 1;
-    if (tstate->frame->f_back != f)
+    if (SLP_PEEK_NEXT_FRAME(tstate)->f_back != f)
         return retval;
     STACKLESS_UNPACK(tstate, retval);
-    retval = tstate->frame->f_execute(tstate->frame, 0, retval);
-    if (tstate->frame != f) {
-        assert(f->f_execute == slp_eval_frame_value || f->f_execute == slp_eval_frame_noval ||
-            f->f_execute == slp_eval_frame_setup_with || f->f_execute == slp_eval_frame_with_cleanup);
-        if (f->f_execute == slp_eval_frame_noval)
-            f->f_execute = slp_eval_frame_value;
-        return retval;
+    {
+        PyFrameObject *f2 = SLP_CLAIM_NEXT_FRAME(tstate);
+        retval = CALL_FRAME_FUNCTION(f2, 0, retval);
+        Py_DECREF(f2);
+        if (SLP_PEEK_NEXT_FRAME(tstate) != f) {
+            assert(f->f_execute == slp_eval_frame_value || f->f_execute == slp_eval_frame_noval ||
+                f->f_execute == slp_eval_frame_setup_with || f->f_execute == slp_eval_frame_with_cleanup);
+            if (f->f_execute == slp_eval_frame_noval)
+                f->f_execute = slp_eval_frame_value;
+            return retval;
+        }
+        f2 = SLP_CLAIM_NEXT_FRAME(tstate);
+        assert(f == f2);
+        Py_DECREF(f2);
     }
     if (STACKLESS_UNWINDING(retval))
         STACKLESS_UNPACK(tstate, retval);
@@ -4141,14 +4146,14 @@ slp_eval_frame_value(PyFrameObject *f, int throwflag, PyObject *retval)
     goto stackless_call_return;
 
 stackless_interrupt_call:
+    /* interrupted during unwinding */
 
     f->f_execute = slp_eval_frame_noval;
     f->f_stacktop = stack_pointer;
 
     /* the -1 is to adjust for the f_lasti change.
        (look for the word 'Promise' above) */
     f->f_lasti = INSTR_OFFSET() - 1;
-    f = tstate->frame;
     return (PyObject *) Py_UnwindToken;
 #endif
 }
@@ -4514,17 +4519,23 @@ _PyEval_EvalCodeWithName(PyObject *_co, PyObject *globals, PyObject *locals,
     Py_INCREF(Py_None);
     retval = Py_None;
     if (stackless) {
-        tstate->frame = f;
+        SLP_STORE_NEXT_FRAME(tstate, f);
+        Py_DECREF(f);
         return STACKLESS_PACK(tstate, retval);
     }
     else {
-        if (f->f_back != NULL)
+        if (f->f_back != NULL) {
             /* use the faster path */
-            retval = slp_frame_dispatch(f, f->f_back, 0, retval);
-            else {
+            PyFrameObject *back = f->f_back;
+            Py_INCREF(back);
+            retval = slp_frame_dispatch(f, back, 0, retval);
+            Py_DECREF(back);
+        }
+        else {
             Py_DECREF(retval);
             retval = slp_eval_frame(f);
         }
+        Py_DECREF(f);
         return retval;
     }
 #else
@@ -5292,7 +5303,7 @@ fast_function(PyObject *func, PyObject ***pp_stack, int n, int na, int nk)
     PCALL(PCALL_FAST_FUNCTION);
     if (argdefs == NULL && co->co_argcount == n &&
         co->co_kwonlyargcount == 0 && nk==0 &&
-        co->co_flags == (CO_OPTIMIZED | CO_NEWLOCALS | CO_NOFREE)) {
+        (co->co_flags & (~PyCF_MASK)) == (CO_OPTIMIZED | CO_NEWLOCALS | CO_NOFREE)) {
         PyFrameObject *f;
         PyObject *retval = NULL;
         PyThreadState *tstate = PyThreadState_GET();
@@ -5322,10 +5333,21 @@ fast_function(PyObject *func, PyObject ***pp_stack, int n, int na, int nk)
         if (STACKLESS_POSSIBLE()) {
             Py_INCREF(Py_None);
             retval = Py_None;
-            tstate->frame = f;
+            SLP_STORE_NEXT_FRAME(tstate, f);
+            Py_DECREF(f);
             return STACKLESS_PACK(tstate, retval);
         }
-        return slp_eval_frame(f);
+        if (f->f_back != NULL) {
+            /* use the faster path */
+            PyFrameObject *back = f->f_back;
+            Py_INCREF(Py_None);
+            Py_INCREF(back);
+            retval = slp_frame_dispatch(f, back, 0, Py_None);
+            Py_DECREF(back);
+        }
+        else {
+            retval = slp_eval_frame(f);
+        }
 #else
         retval = PyEval_EvalFrameEx(f,0);
 #endif

Python/pystate.c

@@ -3,10 +3,9 @@
 
 #include "Python.h"
 #ifdef STACKLESS
-/* XXX this should vanish! */
-#include "compile.h"
 #include "frameobject.h"
 #endif
+#include "core/stackless_impl.h"
 
 /* --------------------------------------------------------------------------
 CAUTION
@@ -171,7 +170,7 @@ threadstate_getframe(PyThreadState *self)
 {
 #ifdef STACKLESS
     /* make sure to return a real frame */
-    struct _frame *f = self->frame;
+    struct _frame *f = SLP_CURRENT_FRAME(self);
     while (f != NULL && !PyFrame_Check(f))
         f = f->f_back;
     return f;
@@ -645,7 +644,7 @@ _PyThread_CurrentFrames(void)
         for (t = i->tstate_head; t != NULL; t = t->next) {
             PyObject *id;
             int stat;
-            struct _frame *frame = t->frame;
+            struct _frame *frame = SLP_PEEK_NEXT_FRAME(t);
             if (frame == NULL)
                 continue;
             id = PyLong_FromLong(t->thread_id);

Stackless/changelog.txt

@@ -30,8 +30,12 @@ What's New in Stackless 3.X.X?
   - Leak to an object, if a Python profile or trace function fails.
   - Various leaks, if stackless._wrap.frame.__setstate__() fails, because its
     state argument is invalid.
+  - Leak of references to Python stack frames or Stackless C-frames, if
+    a tasklet didn't run to its end or various other conditions. This part
+    of the fix changes the Stackless reference counting for frames to follow
+    the general rules for Python objects.
 
-  Additionally this change brings the handling of caught exceptions more in
+  Additionally, this change brings the handling of caught exceptions more in
   line with C-Python.
 
 - https://bitbucket.org/stackless-dev/stackless/issues/112

Stackless/core/cframeobject.c

@@ -66,10 +66,23 @@ cframe_traverse(PyCFrameObject *cf, visitproc visit, void *arg)
 static void
 cframe_clear(PyCFrameObject *cf)
 {
-    Py_CLEAR(cf->f_back);
-    Py_CLEAR(cf->ob1);
-    Py_CLEAR(cf->ob2);
-    Py_CLEAR(cf->ob3);
+    /* The Python C-API documentation recomends to use Py_CLEAR() to release
+     * references held by container objects. The following code is an unrolled
+     * version of four Py_CLEAR() macros. It is more robust at no additional
+     * costs.
+     */
+    PyFrameObject *tmp_f_back;
+    PyObject *tmp_ob1, *tmp_ob2, *tmp_ob3;
+    tmp_f_back = cf->f_back;
+    tmp_ob1 = cf->ob1;
+    tmp_ob2 = cf->ob2;
+    tmp_ob3 = cf->ob3;
+    cf->f_back = NULL;
+    cf->ob1 = cf->ob2 = cf->ob3 = NULL;
+    Py_XDECREF(tmp_f_back);
+    Py_XDECREF(tmp_ob1);
+    Py_XDECREF(tmp_ob2);
+    Py_XDECREF(tmp_ob3);
 }
 
 
@@ -93,8 +106,9 @@ slp_cframe_new(PyFrame_ExecFunc *exec, unsigned int linked)
         _Py_NewReference((PyObject *) cf);
     }
 
-    if (linked)
-        back = ts->frame;
+    if (linked) {
+        back = SLP_CURRENT_FRAME(ts);
+    }
     else
         back = NULL;
     Py_XINCREF(back);
@@ -203,7 +217,7 @@ static PyObject * run_cframe(PyFrameObject *f, int exc, PyObject *retval)
     PyTaskletObject *task = ts->st.current;
     int done = cf->i;
 
-    ts->frame = f;
+    SLP_SET_CURRENT_FRAME(ts, f);
 
     if (retval == NULL || done)
         goto exit_run_cframe;
@@ -218,18 +232,17 @@ static PyObject * run_cframe(PyFrameObject *f, int exc, PyObject *retval)
 
     if (STACKLESS_UNWINDING(retval)) {
         /* try to shortcut */
-        if (ts->st.current == task && ts->frame != NULL &&
-            ts->frame->f_back == (PyFrameObject *) cf) {
-            Py_CLEAR(ts->frame->f_back);
-            ts->frame->f_back = cf->f_back;
-            Py_DECREF(cf); /* the exec reference */
+        PyFrameObject *f;  /* a borrowed ref */
+        if (ts->st.current == task && (f = SLP_PEEK_NEXT_FRAME(ts)) != NULL &&
+            f->f_back == (PyFrameObject *) cf) {
+                Py_XINCREF(cf->f_back);
+                Py_SETREF(f->f_back, cf->f_back);
         }
         return retval;
     }
     /* pop frame */
 exit_run_cframe:
-    ts->frame = cf->f_back;
-    Py_DECREF(cf);
+    SLP_STORE_NEXT_FRAME(ts, cf->f_back);
     return retval;
 }
 

Stackless/core/slp_transfer.c

@@ -121,6 +121,7 @@ slp_transfer(PyCStackObject **cstprev, PyCStackObject *cst,
 
     /* since we change the stack we must assure that the protocol was met */
     STACKLESS_ASSERT();
+    SLP_ASSERT_FRAME_IN_TRANSFER(ts);
 
     if ((intptr_t *) &ts > ts->st.cstack_base)
         return climb_stack_and_transfer(cstprev, cst, prev);
@@ -149,6 +150,7 @@ slp_transfer(PyCStackObject **cstprev, PyCStackObject *cst,
     _cst = cst;
     _prev = prev;
     result = slp_switch();
+    SLP_ASSERT_FRAME_IN_TRANSFER(ts);
     if (!result) {
         if (_cst) {
             /* record the context of the target stack.  Can't do it before the switch because