Commits

Ned Batchelder committed 0c2f51a

A better way to fix the missing-return-after-exception problem in the trace function: no pyexpat specifics, and py 2.3 still uses C trace function.

Comments (0)

Files changed (3)

   invocations could be overlooked.
 
 - On Python 2.3, coverage.py could mis-measure code with exceptions being
-  raised.  This is now fixed, but as a result, on Python 2.3, the Python trace
-  function is used, reversing speed gains acheived in coverage.py 3.0.
+  raised.  This is now fixed.
 
 
 Version 3.0, 13 June 2009

coverage/collector.py

 
 import sys, threading
 
-# Before Python 2.4, the trace function could be called with "call", and then
-# leave the scope without calling "return", for example when an exception
-# caused the scope to end.  We use the matching call and return traces to do
-# bookkeeping for speed, so we need to know up front if they match our
-# expectations.
-CALL_AND_RETURN_MATCH = (sys.hexversion >= 0x02040000)
+try:
+    # Use the C extension code when we can, for speed.
+    from coverage.tracer import Tracer
+except ImportError:
+    # Couldn't import the C extension, maybe it isn't built.
 
-
-Tracer = None
-if CALL_AND_RETURN_MATCH:
-    # We have matching calls and returns, so we can use the C extension.
-    try:
-        # Use the C extension code when we can, for speed.
-        from coverage.tracer import Tracer
-    except ImportError:
-        # Couldn't import the C extension, maybe it isn't built.
-        pass
-
-
-if not Tracer:
-    # If for whatever reason we don't have a Tracer yet, use this Python one.
-    class Tracer:   # pylint: disable-msg=E0102
+    class Tracer:
         """Python implementation of the raw data tracer."""
         def __init__(self):
             self.data = None
             self.should_trace_cache = None
             self.cur_filename = None
             self.filename_stack = []
-            
+            self.last_exc_back = None
+
         def _global_trace(self, frame, event, arg_unused):
             """The trace function passed to sys.settrace."""
             if event == 'call':
                     return None
             return self._global_trace
     
-        if CALL_AND_RETURN_MATCH:
-            def _local_trace(self, frame, event, arg_unused):
-                """The trace function used within a function."""
-                if event == 'line':
-                    # Record an executed line.
-                    self.data[(self.cur_filename, frame.f_lineno)] = True
-                elif event == 'return':
-                    # Leaving this function, pop the filename stack.
+        def _local_trace(self, frame, event, arg_unused):
+            """The trace function used within a function."""
+            if self.last_exc_back:
+                if frame == self.last_exc_back:
+                    # Someone forgot a return event.
                     self.cur_filename = self.filename_stack.pop()
-                return self._local_trace
-        else:
-            def _local_trace(self, frame, event, arg_unused):
-                """The trace function used within a function."""
-                if event == 'line':
-                    if not self.cur_filename:
-                        # Kinda hacky: call _global_trace to do the work of
-                        # setting the cur_filename.
-                        if not self._global_trace(frame, 'call', None):
-                            return None
-                    # Record an executed line.
-                    self.data[(self.cur_filename, frame.f_lineno)] = True
-                elif event == 'return' or event == 'exception':
-                    # Leaving this function, discard the current filename so
-                    # we'll re-compute it at the next line.
-                    self.cur_filename = None
-                return self._local_trace
+                self.last_exc_back = None
+                
+            if event == 'line':
+                # Record an executed line.
+                self.data[(self.cur_filename, frame.f_lineno)] = True
+            elif event == 'return':
+                # Leaving this function, pop the filename stack.
+                self.cur_filename = self.filename_stack.pop()
+            elif event == 'exception':
+                self.last_exc_back = frame.f_back
+            return self._local_trace
     
         def start(self):
             """Start this Tracer."""

coverage/tracer.c

     /* Filenames to record at each level, or NULL if not recording. */
     PyObject ** tracenames;     /* PyMem_Malloc'ed. */
     int tracenames_alloc;       /* number of entries at tracenames. */
+    
+    /* The parent frame for the last exception event, to fix missing returns. */
+    PyFrameObject * last_exc_back;
 } Tracer;
 
 #define TRACENAMES_DELTA    100
         return -1;
     }
     self->tracenames_alloc = TRACENAMES_DELTA;
+    self->last_exc_back = NULL;
     return 0;
 }
 
     }
     #endif
 
+    /* See below for details on missing-return detection. */
+    if (self->last_exc_back) {
+        if (frame == self->last_exc_back) {
+            /* Looks like someone forgot to send a return event. We'll clear
+               the exception state and do the RETURN code here.  Notice that the
+               frame we have in hand here is not the correct frame for the RETURN,
+               that frame is gone.  Our handling for RETURN doesn't need the
+               actual frame, but we do log it, so that will look a little off if
+               you're looking at the detailed log.
+               
+               If someday we need to examine the frame when doing RETURN, then
+               we'll need to keep more of the missed frame's state.
+            */
+            if (self->depth >= 0) {
+                SHOWLOG(self->depth, frame->f_lineno, frame->f_code->co_filename, "missedreturn");
+                Py_XDECREF(self->tracenames[self->depth]);
+                self->depth--;
+            }
+        }
+        self->last_exc_back = NULL;
+    }
+    
+
     switch (what) {
     case PyTrace_CALL:      /* 0 */
         self->depth++;
         break;
     
     case PyTrace_RETURN:    /* 3 */
+        /* A near-copy of this code is above in the missing-return handler. */
         if (self->depth >= 0) {
             SHOWLOG(self->depth, frame->f_lineno, frame->f_code->co_filename, "return");
             Py_XDECREF(self->tracenames[self->depth]);
             }
         }
         break;
-    }
-
-    /* UGLY HACK: for some reason, pyexpat invokes the systrace function directly.
-       It uses "pyexpat.c" as the filename, which is strange enough, but it calls
-       it incorrectly: when an exception passes through the C code, it calls trace
-       with an EXCEPTION, but never calls RETURN.  This throws off our bookkeeping.
-       To make things right, if this is an EXCEPTION from pyexpat.c, then inject
-       a RETURN event also.  
-       
-       I've reported the problem with pyexpat.c as http://bugs.python.org/issue6359 .
-       If the bug in pyexpat.c gets fixed someday, we'll either have to put a 
-       version check here, or do something more sophisticated to detect the 
-       EXCEPTION-without-RETURN case that has to be fixed up.
-    */
-    if (what == PyTrace_EXCEPTION) {
-        if (strstr(PyString_AS_STRING(frame->f_code->co_filename), "pyexpat.c")) {
-            /* Stupid pyexpat: pretend it gave us the RETURN it should have. */
-            SHOWLOG(self->depth, frame->f_lineno, frame->f_code->co_filename, "wrongexc");
-            if (Tracer_trace(self, frame, PyTrace_RETURN, arg) < 0) {
-                return -1;
-            }
-        }
+    
+    case PyTrace_EXCEPTION:
+        /* Some code (Python 2.3, and pyexpat anywhere) fires an exception event
+           without a return event.  To detect that, we'll keep a copy of the
+           parent frame for an exception event.  If the next event is in that
+           frame, then we must have returned without a return event.  We can
+           synthesize the missing event then.
+           
+           Python itself fixed this problem in 2.4.  Pyexpat still has the bug.
+           I've reported the problem with pyexpat as http://bugs.python.org/issue6359 .
+           If it gets fixed, this code should still work properly.  Maybe someday
+           the bug will be fixed everywhere coverage.py is supported, and we can
+           remove this missing-return detection.
+           
+           More about this fix: http://nedbatchelder.com/blog/200907/a_nasty_little_bug.html
+        */
+        self->last_exc_back = frame->f_back;
+        break;
     }
 
     return 0;