gh-113603: Compiler no longer tries to maintain the no-empty-block invariant (#113636)

Author: iritkatriel

Lib/test/test_compile.py

@@ -448,6 +448,19 @@ def test_condition_expression_with_dead_blocks_compiles(self):
         # See gh-113054
         compile('if (5 if 5 else T): 0', '<eval>', 'exec')
 
+    def test_condition_expression_with_redundant_comparisons_compiles(self):
+        # See gh-113054
+        compile('if 9<9<9and 9or 9:9', '<eval>', 'exec')
+
+    def test_dead_code_with_except_handler_compiles(self):
+        compile(textwrap.dedent("""
+                if None:
+                    with CM:
+                        x = 1
+                else:
+                    x = 2
+               """), '<eval>', 'exec')
+
     def test_compile_invalid_namedexpr(self):
         # gh-109351
         m = ast.Module(

Misc/NEWS.d/next/Core and Builtins/2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst

@@ -0,0 +1 @@
+Fixed bug where a redundant NOP is not removed, causing an assertion to fail in the compiler in debug mode.

Python/flowgraph.c

@@ -449,6 +449,15 @@ _PyCfgBuilder_Addop(cfg_builder *g, int opcode, int oparg, location loc)
 }
 
 
+static basicblock *
+next_nonempty_block(basicblock *b)
+{
+    while (b && b->b_iused == 0) {
+        b = b->b_next;
+    }
+    return b;
+}
+
 /***** debugging helpers *****/
 
 #ifndef NDEBUG
@@ -464,24 +473,16 @@ no_redundant_nops(cfg_builder *g) {
     return true;
 }
 
-static bool
-no_empty_basic_blocks(cfg_builder *g) {
-    for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
-        if (b->b_iused == 0) {
-            return false;
-        }
-    }
-    return true;
-}
-
 static bool
 no_redundant_jumps(cfg_builder *g) {
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         cfg_instr *last = basicblock_last_instr(b);
         if (last != NULL) {
             if (IS_UNCONDITIONAL_JUMP_OPCODE(last->i_opcode)) {
-                assert(last->i_target != b->b_next);
-                if (last->i_target == b->b_next) {
+                basicblock *next = next_nonempty_block(b->b_next);
+                basicblock *jump_target = next_nonempty_block(last->i_target);
+                assert(jump_target != next);
+                if (jump_target == next) {
                     return false;
                 }
             }
@@ -961,42 +962,6 @@ mark_reachable(basicblock *entryblock) {
     return SUCCESS;
 }
 
-static void
-eliminate_empty_basic_blocks(cfg_builder *g) {
-    /* Eliminate empty blocks */
-    for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
-        basicblock *next = b->b_next;
-        while (next && next->b_iused == 0) {
-            next = next->b_next;
-        }
-        b->b_next = next;
-    }
-    while(g->g_entryblock && g->g_entryblock->b_iused == 0) {
-        g->g_entryblock = g->g_entryblock->b_next;
-    }
-    int next_lbl = get_max_label(g->g_entryblock) + 1;
-    for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
-        assert(b->b_iused > 0);
-        for (int i = 0; i < b->b_iused; i++) {
-            cfg_instr *instr = &b->b_instr[i];
-            if (HAS_TARGET(instr->i_opcode)) {
-                basicblock *target = instr->i_target;
-                while (target->b_iused == 0) {
-                    target = target->b_next;
-                }
-                if (instr->i_target != target) {
-                    if (!IS_LABEL(target->b_label)) {
-                        target->b_label.id = next_lbl++;
-                    }
-                    instr->i_target = target;
-                    instr->i_oparg = target->b_label.id;
-                }
-                assert(instr->i_target && instr->i_target->b_iused > 0);
-            }
-        }
-    }
-}
-
 static int
 remove_redundant_nops(basicblock *bb) {
     /* Remove NOPs when legal to do so. */
@@ -1025,10 +990,7 @@ remove_redundant_nops(basicblock *bb) {
                 }
             }
             else {
-                basicblock* next = bb->b_next;
-                while (next && next->b_iused == 0) {
-                    next = next->b_next;
-                }
+                basicblock *next = next_nonempty_block(bb->b_next);
                 /* or if last instruction in BB and next BB has same line number */
                 if (next) {
                     location next_loc = NO_LOCATION;
@@ -1112,36 +1074,29 @@ remove_redundant_jumps(cfg_builder *g) {
      * can be deleted.
      */
 
-    assert(no_empty_basic_blocks(g));
-
-    bool remove_empty_blocks = false;
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         cfg_instr *last = basicblock_last_instr(b);
-        assert(last != NULL);
+        if (last == NULL) {
+            continue;
+        }
         assert(!IS_ASSEMBLER_OPCODE(last->i_opcode));
         if (IS_UNCONDITIONAL_JUMP_OPCODE(last->i_opcode)) {
-            if (last->i_target == NULL) {
+            basicblock* jump_target = next_nonempty_block(last->i_target);
+            if (jump_target == NULL) {
                 PyErr_SetString(PyExc_SystemError, "jump with NULL target");
                 return ERROR;
             }
-            if (last->i_target == b->b_next) {
-                assert(b->b_next->b_iused);
+            basicblock *next = next_nonempty_block(b->b_next);
+            if (jump_target == next) {
                 if (last->i_loc.lineno == NO_LOCATION.lineno) {
                     b->b_iused--;
-                    if (b->b_iused == 0) {
-                        remove_empty_blocks = true;
-                    }
                 }
                 else {
                     INSTR_SET_OP0(last, NOP);
                 }
             }
         }
     }
-    if (remove_empty_blocks) {
-        eliminate_empty_basic_blocks(g);
-    }
-    assert(no_empty_basic_blocks(g));
     return SUCCESS;
 }
 
@@ -1749,11 +1704,9 @@ optimize_cfg(cfg_builder *g, PyObject *consts, PyObject *const_cache)
 {
     assert(PyDict_CheckExact(const_cache));
     RETURN_IF_ERROR(check_cfg(g));
-    eliminate_empty_basic_blocks(g);
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         RETURN_IF_ERROR(inline_small_exit_blocks(b));
     }
-    assert(no_empty_basic_blocks(g));
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         RETURN_IF_ERROR(optimize_basic_block(const_cache, b, consts));
         assert(b->b_predecessors == 0);
@@ -1768,14 +1721,21 @@ optimize_cfg(cfg_builder *g, PyObject *consts, PyObject *const_cache)
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
        if (b->b_predecessors == 0) {
             b->b_iused = 0;
+            b->b_except_handler = 0;
        }
     }
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         remove_redundant_nops(b);
     }
-    eliminate_empty_basic_blocks(g);
-    assert(no_redundant_nops(g));
     RETURN_IF_ERROR(remove_redundant_jumps(g));
+
+    for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
+        remove_redundant_nops(b);
+    }
+
+    RETURN_IF_ERROR(remove_redundant_jumps(g));
+
+    assert(no_redundant_jumps(g));
     return SUCCESS;
 }
 
@@ -1825,7 +1785,6 @@ insert_superinstructions(cfg_builder *g)
     for (basicblock *b = g->g_entryblock; b != NULL; b = b->b_next) {
         remove_redundant_nops(b);
     }
-    eliminate_empty_basic_blocks(g);
     assert(no_redundant_nops(g));
 }
 
@@ -2299,18 +2258,18 @@ is_exit_without_lineno(basicblock *b) {
 static int
 duplicate_exits_without_lineno(cfg_builder *g)
 {
-    assert(no_empty_basic_blocks(g));
-
     int next_lbl = get_max_label(g->g_entryblock) + 1;
 
     /* Copy all exit blocks without line number that are targets of a jump.
      */
     basicblock *entryblock = g->g_entryblock;
     for (basicblock *b = entryblock; b != NULL; b = b->b_next) {
         cfg_instr *last = basicblock_last_instr(b);
-        assert(last != NULL);
+        if (last == NULL) {
+            continue;
+        }
         if (is_jump(last)) {
-            basicblock *target = last->i_target;
+            basicblock *target = next_nonempty_block(last->i_target);
             if (is_exit_without_lineno(target) && target->b_predecessors > 1) {
                 basicblock *new_target = copy_basicblock(g, target);
                 if (new_target == NULL) {
@@ -2367,9 +2326,10 @@ propagate_line_numbers(basicblock *entryblock) {
             }
         }
         if (BB_HAS_FALLTHROUGH(b) && b->b_next->b_predecessors == 1) {
-            assert(b->b_next->b_iused);
-            if (b->b_next->b_instr[0].i_loc.lineno < 0) {
-                b->b_next->b_instr[0].i_loc = prev_location;
+            if (b->b_next->b_iused > 0) {
+                if (b->b_next->b_instr[0].i_loc.lineno < 0) {
+                    b->b_next->b_instr[0].i_loc = prev_location;
+                }
             }
         }
         if (is_jump(last)) {