bpo-37213: Handle negative line deltas correctly in the peephole optimizer (GH-13969)

The peephole optimizer was not optimizing correctly bytecode after negative deltas were introduced. This is due to the fact that some special values (255) were being searched for in both instruction pointer delta and line number deltas.

Lib/test/test_peepholer.py

 import dis
 import unittest
+import types
+import textwrap
 
 from test.bytecode_helper import BytecodeTestCase
 
@@ -18,6 +20,27 @@ def count_instr_recursively(f, opname):
 
 class TestTranforms(BytecodeTestCase):
 
+    def check_jump_targets(self, code):
+        instructions = list(dis.get_instructions(code))
+        targets = {instr.offset: instr for instr in instructions}
+        for instr in instructions:
+            if 'JUMP_' not in instr.opname:
+                continue
+            tgt = targets[instr.argval]
+            # jump to unconditional jump
+            if tgt.opname in ('JUMP_ABSOLUTE', 'JUMP_FORWARD'):
+                self.fail(f'{instr.opname} at {instr.offset} '
+                          f'jumps to {tgt.opname} at {tgt.offset}')
+            # unconditional jump to RETURN_VALUE
+            if (instr.opname in ('JUMP_ABSOLUTE', 'JUMP_FORWARD') and
+                tgt.opname == 'RETURN_VALUE'):
+                self.fail(f'{instr.opname} at {instr.offset} '
+                          f'jumps to {tgt.opname} at {tgt.offset}')
+            # JUMP_IF_*_OR_POP jump to conditional jump
+            if '_OR_POP' in instr.opname and 'JUMP_IF_' in tgt.opname:
+                self.fail(f'{instr.opname} at {instr.offset} '
+                          f'jumps to {tgt.opname} at {tgt.offset}')
+
     def test_unot(self):
         # UNARY_NOT POP_JUMP_IF_FALSE  -->  POP_JUMP_IF_TRUE'
         def unot(x):
@@ -259,13 +282,69 @@ class TestTranforms(BytecodeTestCase):
     def test_elim_jump_to_return(self):
         # JUMP_FORWARD to RETURN -->  RETURN
         def f(cond, true_value, false_value):
-            return true_value if cond else false_value
+            # Intentionally use two-line expression to test issue37213.
+            return (true_value if cond
+                    else false_value)
+        self.check_jump_targets(f)
         self.assertNotInBytecode(f, 'JUMP_FORWARD')
         self.assertNotInBytecode(f, 'JUMP_ABSOLUTE')
         returns = [instr for instr in dis.get_instructions(f)
                           if instr.opname == 'RETURN_VALUE']
         self.assertEqual(len(returns), 2)
 
+    def test_elim_jump_to_uncond_jump(self):
+        # POP_JUMP_IF_FALSE to JUMP_FORWARD --> POP_JUMP_IF_FALSE to non-jump
+        def f():
+            if a:
+                # Intentionally use two-line expression to test issue37213.
+                if (c
+                    or d):
+                    foo()
+            else:
+                baz()
+        self.check_jump_targets(f)
+
+    def test_elim_jump_to_uncond_jump2(self):
+        # POP_JUMP_IF_FALSE to JUMP_ABSOLUTE --> POP_JUMP_IF_FALSE to non-jump
+        def f():
+            while a:
+                # Intentionally use two-line expression to test issue37213.
+                if (c
+                    or d):
+                    a = foo()
+        self.check_jump_targets(f)
+
+    def test_elim_jump_to_uncond_jump3(self):
+        # Intentionally use two-line expressions to test issue37213.
+        # JUMP_IF_FALSE_OR_POP to JUMP_IF_FALSE_OR_POP --> JUMP_IF_FALSE_OR_POP to non-jump
+        def f(a, b, c):
+            return ((a and b)
+                    and c)
+        self.check_jump_targets(f)
+        self.assertEqual(count_instr_recursively(f, 'JUMP_IF_FALSE_OR_POP'), 2)
+        # JUMP_IF_TRUE_OR_POP to JUMP_IF_TRUE_OR_POP --> JUMP_IF_TRUE_OR_POP to non-jump
+        def f(a, b, c):
+            return ((a or b)
+                    or c)
+        self.check_jump_targets(f)
+        self.assertEqual(count_instr_recursively(f, 'JUMP_IF_TRUE_OR_POP'), 2)
+        # JUMP_IF_FALSE_OR_POP to JUMP_IF_TRUE_OR_POP --> POP_JUMP_IF_FALSE to non-jump
+        def f(a, b, c):
+            return ((a and b)
+                    or c)
+        self.check_jump_targets(f)
+        self.assertNotInBytecode(f, 'JUMP_IF_FALSE_OR_POP')
+        self.assertInBytecode(f, 'JUMP_IF_TRUE_OR_POP')
+        self.assertInBytecode(f, 'POP_JUMP_IF_FALSE')
+        # JUMP_IF_TRUE_OR_POP to JUMP_IF_FALSE_OR_POP --> POP_JUMP_IF_TRUE to non-jump
+        def f(a, b, c):
+            return ((a or b)
+                    and c)
+        self.check_jump_targets(f)
+        self.assertNotInBytecode(f, 'JUMP_IF_TRUE_OR_POP')
+        self.assertInBytecode(f, 'JUMP_IF_FALSE_OR_POP')
+        self.assertInBytecode(f, 'POP_JUMP_IF_TRUE')
+
     def test_elim_jump_after_return1(self):
         # Eliminate dead code: jumps immediately after returns can't be reached
         def f(cond1, cond2):

Misc/NEWS.d/next/Core and Builtins/2019-06-11-11-15-19.bpo-37213.UPii5K.rst

+Handle correctly negative line offsets in the peephole optimizer. Patch by
+Pablo Galindo.

Python/peephole.c

@@ -250,12 +250,16 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names,
     lnotab = (unsigned char*)PyBytes_AS_STRING(lnotab_obj);
     tabsiz = PyBytes_GET_SIZE(lnotab_obj);
     assert(tabsiz == 0 || Py_REFCNT(lnotab_obj) == 1);
-    if (memchr(lnotab, 255, tabsiz) != NULL) {
-        /* 255 value are used for multibyte bytecode instructions */
-        goto exitUnchanged;
+
+    /* Don't optimize if lnotab contains instruction pointer delta larger
+       than +255 (encoded as multiple bytes), just to keep the peephole optimizer
+       simple. The optimizer leaves line number deltas unchanged. */
+
+    for (j = 0; j < tabsiz; j += 2) {
+        if (lnotab[j] == 255) {
+            goto exitUnchanged;
+        }
     }
-    /* Note: -128 and 127 special values for line number delta are ok,
-       the peephole optimizer doesn't modify line numbers. */
 
     assert(PyBytes_Check(code));
     Py_ssize_t codesize = PyBytes_GET_SIZE(code);