From 7d01fb48089872155e1721ba0a8cc27ee5c4fecd Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Wed, 3 Jan 2024 16:57:48 +0000 Subject: gh-113603: Compiler no longer tries to maintain the no-empty-block invariant (#113636) --- Lib/test/test_compile.py | 13 +++ .../2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst | 1 + Python/flowgraph.c | 116 +++++++-------------- 3 files changed, 52 insertions(+), 78 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py index 906e16c..7850977 100644 --- a/Lib/test/test_compile.py +++ b/Lib/test/test_compile.py @@ -448,6 +448,19 @@ class TestSpecifics(unittest.TestCase): # See gh-113054 compile('if (5 if 5 else T): 0', '', 'exec') + def test_condition_expression_with_redundant_comparisons_compiles(self): + # See gh-113054 + compile('if 9<9<9and 9or 9:9', '', 'exec') + + def test_dead_code_with_except_handler_compiles(self): + compile(textwrap.dedent(""" + if None: + with CM: + x = 1 + else: + x = 2 + """), '', 'exec') + def test_compile_invalid_namedexpr(self): # gh-109351 m = ast.Module( diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-01-23-57-24.gh-issue-113603.ySwovr.rst new file mode 100644 index 0000000..5fe6d80 --- /dev/null +++ b/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. diff --git a/Python/flowgraph.c b/Python/flowgraph.c index 0e6ffbc3..5bb1198 100644 --- a/Python/flowgraph.c +++ b/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 @@ -465,23 +474,15 @@ no_redundant_nops(cfg_builder *g) { } 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,25 +1074,22 @@ 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); @@ -1138,10 +1097,6 @@ remove_redundant_jumps(cfg_builder *g) { } } } - 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,8 +2258,6 @@ 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. @@ -2308,9 +2265,11 @@ duplicate_exits_without_lineno(cfg_builder *g) 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)) { -- cgit v0.12