Further SET_LINENO reomval fixes. See comments in patch #587933.

Use a slightly different strategy to determine when not to call the line
trace function.  This removes the need for the RETURN_NONE opcode, so
that's gone again.  Update docs and comments to match.

Thanks to Neal and Armin!

Also add a test suite.  This should have come with the original patch...

Doc/lib/libdis.tex

@@ -27,7 +27,8 @@ the following command can be used to get the disassembly of
               3 LOAD_FAST                0 (alist)
               6 CALL_FUNCTION            1
               9 RETURN_VALUE        
-             10 RETURN_NONE
+             10 LOAD_CONST               0 (None)
+             13 RETURN_VALUE
 \end{verbatim}
 
 (The ``2'' is a line number).
@@ -401,14 +402,6 @@ is evaluated, the locals are passed to the class definition.
 Returns with TOS to the caller of the function.
 \end{opcodedesc}
 
-\begin{opcodedesc}{RETURN_NONE}{}
-Returns \constant{None} to the caller of the function.  This opcode is
-generated as the last opcode of every function and only then, for
-reasons to do with tracing support.  See the comments in the function
-\cfunction{maybe_call_line_trace} in \file{Python/ceval.c} for the
-gory details.  \versionadded{2.3}.
-\end{opcodedesc}
-
 \begin{opcodedesc}{YIELD_VALUE}{}
 Pops \code{TOS} and yields it from a generator.
 \end{opcodedesc}

Doc/whatsnew/whatsnew23.tex

@@ -1225,11 +1225,6 @@ should instead call \code{PyCode_Addr2Line(f->f_code, f->f_lasti)}.
 This will have the added effect of making the code work as desired
 under ``python -O'' in earlier versions of Python.
 
-To make tracing work as expected, it was found necessary to add a new
-opcode, \cdata{RETURN_NONE}, to the VM.  If you want to know why, read
-the comments in the function \cfunction{maybe_call_line_trace} in
-\file{Python/ceval.c}.
-
 \end{itemize}
 
 

Include/opcode.h

@@ -71,9 +71,6 @@ extern "C" {
 #define INPLACE_OR	79
 #define BREAK_LOOP	80
 
-#define RETURN_NONE	81 /* *only* for function epilogues 
-			      -- see comments in 
-			      ceval.c:maybe_call_line_trace for why */
 #define LOAD_LOCALS	82
 #define RETURN_VALUE	83
 #define IMPORT_STAR	84

Lib/dis.py

@@ -254,7 +254,6 @@ def_op('INPLACE_XOR', 78)
 def_op('INPLACE_OR', 79)
 def_op('BREAK_LOOP', 80)
 
-def_op('RETURN_NONE', 81)
 def_op('LOAD_LOCALS', 82)
 def_op('RETURN_VALUE', 83)
 def_op('IMPORT_STAR', 84)

Lib/test/test_trace.py

@@ -0,0 +1,110 @@
+# Testing the line trace facility.
+
+from test import test_support
+import unittest
+import sys
+import difflib
+
+# A very basic example.  If this fails, we're in deep trouble.
+def basic():
+    return 1
+
+basic.events = [(0, 'call'),
+                (1, 'line'),
+                (1, 'return')]
+
+# Armin Rigo's failing example:
+def arigo_example():
+    x = 1
+    del x
+    while 0:
+        pass
+    x = 1
+
+arigo_example.events = [(0, 'call'),
+                        (1, 'line'),
+                        (2, 'line'),
+                        (3, 'line'),
+                        (5, 'line'),
+                        (5, 'return')]
+
+# check that lines consisting of just one instruction get traced:
+def one_instr_line():
+    x = 1
+    del x
+    x = 1
+
+one_instr_line.events = [(0, 'call'),
+                         (1, 'line'),
+                         (2, 'line'),
+                         (3, 'line'),
+                         (3, 'return')]
+
+def no_pop_tops():      # 0
+    x = 1               # 1
+    for a in range(2):  # 2
+        if a:           # 3
+            x = 1       # 4
+        else:           # 5
+            x = 1       # 6
+
+no_pop_tops.events = [(0, 'call'),
+                      (1, 'line'),
+                      (2, 'line'),
+                      (3, 'line'),
+                      (6, 'line'),
+                      (2, 'line'),
+                      (3, 'line'),
+                      (4, 'line'),
+                      (2, 'line'),
+                      (6, 'return')]
+
+def no_pop_blocks():
+    while 0:
+        bla
+    x = 1
+
+no_pop_blocks.events = [(0, 'call'),
+                        (1, 'line'),
+                        (3, 'line'),
+                        (3, 'return')]
+
+class Tracer:
+    def __init__(self):
+        self.events = []
+    def trace(self, frame, event, arg):
+        self.events.append((frame.f_lineno, event))
+        return self.trace
+
+class TraceTestCase(unittest.TestCase):
+    def run_test(self, func):
+        tracer = Tracer()
+        sys.settrace(tracer.trace)
+        func()
+        sys.settrace(None)
+        fl = func.func_code.co_firstlineno
+        events = [(l - fl, e) for (l, e) in tracer.events]
+        if events != func.events:
+            self.fail(
+                "events did not match expectation:\n" +
+                "\n".join(difflib.ndiff(map(str, func.events),
+                                        map(str, events))))
+    
+    def test_1_basic(self):
+        self.run_test(basic)
+    def test_2_arigo(self):
+        self.run_test(arigo_example)
+    def test_3_one_instr(self):
+        self.run_test(one_instr_line)
+    def test_4_no_pop_blocks(self):
+        self.run_test(no_pop_blocks)
+    def test_5_no_pop_tops(self):
+        self.run_test(no_pop_tops)
+                                      
+                                  
+
+def test_main():
+    test_support.run_unittest(TraceTestCase)
+
+if __name__ == "__main__":
+    test_main()

Python/ceval.c

@@ -1515,12 +1515,6 @@ eval_frame(PyFrameObject *f)
 			why = WHY_RETURN;
 			break;
 
-		case RETURN_NONE:
-			retval = Py_None;
-			Py_INCREF(retval);
-			why = WHY_RETURN;
-			break;
-
 		case YIELD_VALUE:
 			retval = POP();
 			f->f_stacktop = stack_pointer;
@@ -2880,9 +2874,8 @@ maybe_call_line_trace(int opcode, Py_tracefunc func, PyObject *obj,
 	   This is all fairly simple.  Digging the information out of
 	   co_lnotab takes some work, but is conceptually clear.
 
-	   Somewhat harder to explain is why we don't call the line
+	   Somewhat harder to explain is why we don't *always* call the
-	   trace function when executing a POP_TOP or RETURN_NONE
+	   line trace function when the above test fails.
-	   opcodes.  An example probably serves best.
 
 	   Consider this code:
 
@@ -2907,7 +2900,8 @@ maybe_call_line_trace(int opcode, Py_tracefunc func, PyObject *obj,
 	   5          16 LOAD_CONST               2 (2)
 		      19 PRINT_ITEM          
 		      20 PRINT_NEWLINE       
-		 >>   21 RETURN_NONE         
+		 >>   21 LOAD_CONST               0 (None)
+		      24 RETURN_VALUE
 
 	   If a is false, execution will jump to instruction at offset
 	   15 and the co_lnotab will claim that execution has moved to
@@ -2915,56 +2909,48 @@ maybe_call_line_trace(int opcode, Py_tracefunc func, PyObject *obj,
 	   associate the POP_TOP with line 4, but that doesn't make
 	   sense in all cases (I think).
 
-	   On the other hand, if a is true, execution will jump from
+	   What we do is only call the line trace function if the co_lnotab
-	   instruction offset 12 to offset 21.  Then the co_lnotab would
+	   indicates we have jumped to the *start* of a line, i.e. if the
-	   imply that execution has moved to line 5, which is again
+	   current instruction offset matches the offset given for the
-	   misleading.
+	   start of a line by the co_lnotab.
 
-	   This is why it is important that RETURN_NONE is *only* used
+	   This also takes care of the situation where a is true.
-	   for the "falling off the end of the function" form of
+	   Execution will jump from instruction offset 12 to offset 21.
-	   returning None -- using it for code like
+	   Then the co_lnotab would imply that execution has moved to line
-
+	   5, which is again misleading.
-	   1: def f():
-	   2:     return
-
-	   would, once again, lead to misleading tracing behaviour.
-
-	   It is also worth mentioning that getting tracing behaviour
-	   right is the *entire* motivation for adding the RETURN_NONE
-	   opcode.
 	*/
 
-	if (opcode != POP_TOP && opcode != RETURN_NONE &&
+	if ((frame->f_lasti < *instr_lb || frame->f_lasti >= *instr_ub)) {
-	    (frame->f_lasti < *instr_lb || frame->f_lasti > *instr_ub)) {
 		PyCodeObject* co = frame->f_code;
 		int size, addr;
 		unsigned char* p;
 
-		call_trace(func, obj, frame, PyTrace_LINE, Py_None);
+		size = PyString_GET_SIZE(co->co_lnotab) / 2;
+		p = (unsigned char*)PyString_AS_STRING(co->co_lnotab);
 
-		size = PyString_Size(co->co_lnotab) / 2;
+		addr = 0;
-		p = (unsigned char*)PyString_AsString(co->co_lnotab);
 
 		/* possible optimization: if f->f_lasti == instr_ub
 		   (likely to be a common case) then we already know
 		   instr_lb -- if we stored the matching value of p
 		   somwhere we could skip the first while loop. */
 
-		addr = 0;
-
 		/* see comments in compile.c for the description of
 		   co_lnotab.  A point to remember: increments to p
 		   should come in pairs -- although we don't care about
 		   the line increments here, treating them as byte
 		   increments gets confusing, to say the least. */
 
-		while (size >= 0) {
+		while (size > 0) {
 			if (addr + *p > frame->f_lasti)
 				break;
 			addr += *p++;
 			p++;
 			--size;
 		}
+		if (addr == frame->f_lasti)
+			call_trace(func, obj, frame, 
+				   PyTrace_LINE, Py_None);
 		*instr_lb = addr;
 		if (size > 0) {
 			while (--size >= 0) {

Python/compile.c

@@ -4014,7 +4014,10 @@ compile_funcdef(struct compiling *c, node *n)
 	c->c_infunction = 1;
 	com_node(c, CHILD(n, 4));
 	c->c_infunction = 0;
-	com_addbyte(c, RETURN_NONE);
+	com_addoparg(c, LOAD_CONST, com_addconst(c, Py_None));
+	com_push(c, 1);
+	com_addbyte(c, RETURN_VALUE);
+	com_pop(c, 1);
 }
 
 static void
@@ -4081,13 +4084,19 @@ compile_node(struct compiling *c, node *n)
 		n = CHILD(n, 0);
 		if (TYPE(n) != NEWLINE)
 			com_node(c, n);
-		com_addbyte(c, RETURN_NONE);
+		com_addoparg(c, LOAD_CONST, com_addconst(c, Py_None));
+		com_push(c, 1);
+		com_addbyte(c, RETURN_VALUE);
+		com_pop(c, 1);
 		c->c_interactive--;
 		break;
 	
 	case file_input: /* A whole file, or built-in function exec() */
 		com_file_input(c, n);
-		com_addbyte(c, RETURN_NONE);
+		com_addoparg(c, LOAD_CONST, com_addconst(c, Py_None));
+		com_push(c, 1);
+		com_addbyte(c, RETURN_VALUE);
+		com_pop(c, 1);
 		break;
 	
 	case eval_input: /* Built-in function input() */