bpo-37830: Fix compilation of break and continue in finally. (GH-15320)

Fix compilation of "break" and "continue" in the "finally" block when the corresponding "try" block contains "return" with a non-constant value.

Author: serhiy-storchaka

Lib/importlib/_bootstrap_external.py

@@ -270,6 +270,7 @@ def _write_atomic(path, data, mode=0o666):
 # comprehensions #35224)
 # Python 3.8b2 3412 (Swap the position of positional args and positional
 # only args in ast.arguments #37593)
+# Python 3.8b4 3413 (Fix "break" and "continue" in "finally" #37830)
 #
 # MAGIC must change whenever the bytecode emitted by the compiler may no
 # longer be understood by older implementations of the eval loop (usually
@@ -278,7 +279,7 @@ def _write_atomic(path, data, mode=0o666):
 # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
 # in PC/launcher.c must also be updated.
 
-MAGIC_NUMBER = (3412).to_bytes(2, 'little') + b'\r\n'
+MAGIC_NUMBER = (3413).to_bytes(2, 'little') + b'\r\n'
 _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c
 
 _PYCACHE = '__pycache__'

Lib/test/test_dis.py

@@ -980,7 +980,7 @@ def jumpy():
  Instruction(opname='SETUP_FINALLY', opcode=122, arg=70, argval=174, argrepr='to 174', offset=102, starts_line=20, is_jump_target=True),
  Instruction(opname='SETUP_FINALLY', opcode=122, arg=12, argval=118, argrepr='to 118', offset=104, starts_line=None, is_jump_target=False),
  Instruction(opname='LOAD_CONST', opcode=100, arg=5, argval=1, argrepr='1', offset=106, starts_line=21, is_jump_target=False),
- Instruction(opname='LOAD_CONST', opcode=100, arg=7, argval=0, argrepr='0', offset=108, starts_line=None, is_jump_target=False),
+ Instruction(opname='LOAD_CONST', opcode=100, arg=8, argval=0, argrepr='0', offset=108, starts_line=None, is_jump_target=False),
  Instruction(opname='BINARY_TRUE_DIVIDE', opcode=27, arg=None, argval=None, argrepr='', offset=110, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=112, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_BLOCK', opcode=87, arg=None, argval=None, argrepr='', offset=114, starts_line=None, is_jump_target=False),
@@ -993,7 +993,7 @@ def jumpy():
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=128, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=130, starts_line=None, is_jump_target=False),
  Instruction(opname='LOAD_GLOBAL', opcode=116, arg=1, argval='print', argrepr='print', offset=132, starts_line=23, is_jump_target=False),
- Instruction(opname='LOAD_CONST', opcode=100, arg=8, argval='Here we go, here we go, here we go...', argrepr="'Here we go, here we go, here we go...'", offset=134, starts_line=None, is_jump_target=False),
+ Instruction(opname='LOAD_CONST', opcode=100, arg=9, argval='Here we go, here we go, here we go...', argrepr="'Here we go, here we go, here we go...'", offset=134, starts_line=None, is_jump_target=False),
  Instruction(opname='CALL_FUNCTION', opcode=131, arg=1, argval=1, argrepr='', offset=136, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=138, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_EXCEPT', opcode=89, arg=None, argval=None, argrepr='', offset=140, starts_line=None, is_jump_target=False),
@@ -1003,7 +1003,7 @@ def jumpy():
  Instruction(opname='SETUP_WITH', opcode=143, arg=14, argval=164, argrepr='to 164', offset=148, starts_line=None, is_jump_target=False),
  Instruction(opname='STORE_FAST', opcode=125, arg=1, argval='dodgy', argrepr='dodgy', offset=150, starts_line=None, is_jump_target=False),
  Instruction(opname='LOAD_GLOBAL', opcode=116, arg=1, argval='print', argrepr='print', offset=152, starts_line=26, is_jump_target=False),
- Instruction(opname='LOAD_CONST', opcode=100, arg=9, argval='Never reach this', argrepr="'Never reach this'", offset=154, starts_line=None, is_jump_target=False),
+ Instruction(opname='LOAD_CONST', opcode=100, arg=10, argval='Never reach this', argrepr="'Never reach this'", offset=154, starts_line=None, is_jump_target=False),
  Instruction(opname='CALL_FUNCTION', opcode=131, arg=1, argval=1, argrepr='', offset=156, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=158, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_BLOCK', opcode=87, arg=None, argval=None, argrepr='', offset=160, starts_line=None, is_jump_target=False),
@@ -1014,7 +1014,7 @@ def jumpy():
  Instruction(opname='POP_BLOCK', opcode=87, arg=None, argval=None, argrepr='', offset=170, starts_line=None, is_jump_target=True),
  Instruction(opname='BEGIN_FINALLY', opcode=53, arg=None, argval=None, argrepr='', offset=172, starts_line=None, is_jump_target=False),
  Instruction(opname='LOAD_GLOBAL', opcode=116, arg=1, argval='print', argrepr='print', offset=174, starts_line=28, is_jump_target=True),
- Instruction(opname='LOAD_CONST', opcode=100, arg=10, argval="OK, now we're done", argrepr='"OK, now we\'re done"', offset=176, starts_line=None, is_jump_target=False),
+ Instruction(opname='LOAD_CONST', opcode=100, arg=7, argval="OK, now we're done", argrepr='"OK, now we\'re done"', offset=176, starts_line=None, is_jump_target=False),
  Instruction(opname='CALL_FUNCTION', opcode=131, arg=1, argval=1, argrepr='', offset=178, starts_line=None, is_jump_target=False),
  Instruction(opname='POP_TOP', opcode=1, arg=None, argval=None, argrepr='', offset=180, starts_line=None, is_jump_target=False),
  Instruction(opname='END_FINALLY', opcode=88, arg=None, argval=None, argrepr='', offset=182, starts_line=None, is_jump_target=False),

Lib/test/test_grammar.py

@@ -991,6 +991,60 @@ def g3():
  return 4
  self.assertEqual(g3(), 4)
 
+ def test_break_in_finally_after_return(self):
+ # See issue #37830
+ def g1(x):
+ for count in [0, 1]:
+ count2 = 0
+ while count2 < 20:
+ count2 += 10
+ try:
+ return count + count2
+ finally:
+ if x:
+ break
+ return 'end', count, count2
+ self.assertEqual(g1(False), 10)
+ self.assertEqual(g1(True), ('end', 1, 10))
+
+ def g2(x):
+ for count in [0, 1]:
+ for count2 in [10, 20]:
+ try:
+ return count + count2
+ finally:
+ if x:
+ break
+ return 'end', count, count2
+ self.assertEqual(g2(False), 10)
+ self.assertEqual(g2(True), ('end', 1, 10))
+
+ def test_continue_in_finally_after_return(self):
+ # See issue #37830
+ def g1(x):
+ count = 0
+ while count < 100:
+ count += 1
+ try:
+ return count
+ finally:
+ if x:
+ continue
+ return 'end', count
+ self.assertEqual(g1(False), 1)
+ self.assertEqual(g1(True), ('end', 100))
+
+ def g2(x):
+ for count in [0, 1]:
+ try:
+ return count
+ finally:
+ if x:
+ continue
+ return 'end', count
+ self.assertEqual(g2(False), 0)
+ self.assertEqual(g2(True), ('end', 1))
+
  def test_yield(self):
  # Allowed as standalone statement
  def g(): yield 1

Lib/test/test_importlib/test_util.py

@@ -861,7 +861,7 @@ def test_magic_number(self):
  in advance. Such exceptional releases will then require an
  adjustment to this test case.
  """
- EXPECTED_MAGIC_NUMBER = 3410
+ EXPECTED_MAGIC_NUMBER = 3413
  actual = int.from_bytes(importlib.util.MAGIC_NUMBER[:2], 'little')
 
  msg = (

Misc/NEWS.d/next/Core and Builtins/2019-08-17-18-41-59.bpo-37830.fNfMbz.rst

@@ -0,0 +1,3 @@
+Fixed compilation of :keyword:`break` and :keyword:`continue` in the
+:keyword:`finally` block when the corresponding :keyword:`try` block
+contains :keyword:`return` with a non-constant value.

Objects/frameobject.c

@@ -233,6 +233,7 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore
  * the 'finally' blocks. */
  memset(blockstack, '\0', sizeof(blockstack));
  blockstack_top = 0;
+ unsigned char prevop = NOP;
  for (addr = 0; addr < code_len; addr += sizeof(_Py_CODEUNIT)) {
  unsigned char op = code[addr];
  switch (op) {
@@ -259,17 +260,24 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore
  "can't jump into the middle of a block");
  return -1;
  }
+ int in_for_loop = op == FOR_ITER || code[target_addr] == END_ASYNC_FOR;
  if (first_in && !second_in) {
- if (op != FOR_ITER && code[target_addr] != END_ASYNC_FOR) {
- delta_iblock++;
+ if (!delta_iblock) {
+ if (in_for_loop) {
+ /* Pop the iterators of any 'for' and 'async for' loop
+ * we're jumping out of. */
+ delta++;
+ }
+ else if (prevop == LOAD_CONST) {
+ /* Pops None pushed before SETUP_FINALLY. */
+ delta++;
+ }
  }
- else if (!delta_iblock) {
- /* Pop the iterators of any 'for' and 'async for' loop
- * we're jumping out of. */
- delta++;
+ if (!in_for_loop) {
+ delta_iblock++;
  }
  }
- if (op != FOR_ITER && code[target_addr] != END_ASYNC_FOR) {
+ if (!in_for_loop) {
  blockstack[blockstack_top++] = target_addr;
  }
  break;
@@ -293,6 +301,7 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno, void *Py_UNUSED(ignore
  break;
  }
  }
+ prevop = op;
  }
 
  /* Verify that the blockstack tracking code didn't get lost. */

PC/launcher.c

@@ -1139,7 +1139,7 @@ static PYC_MAGIC magic_values[] = {
  { 3320, 3351, L"3.5" },
  { 3360, 3379, L"3.6" },
  { 3390, 3399, L"3.7" },
- { 3400, 3410, L"3.8" },
+ { 3400, 3419, L"3.8" },
  { 0 }
 };
 

Python/compile.c

@@ -81,7 +81,7 @@ It's called a frame block to distinguish it from a basic block in the
 compiler IR.
 */
 
-enum fblocktype { WHILE_LOOP, FOR_LOOP, EXCEPT, FINALLY_TRY, FINALLY_END,
+enum fblocktype { WHILE_LOOP, FOR_LOOP, EXCEPT, FINALLY_TRY, FINALLY_TRY2, FINALLY_END,
  WITH, ASYNC_WITH, HANDLER_CLEANUP };
 
 struct fblockinfo {
@@ -1664,7 +1664,12 @@ compiler_unwind_fblock(struct compiler *c, struct fblockinfo *info,
  return 1;
 
  case FINALLY_END:
+ info->fb_exit = NULL;
  ADDOP_I(c, POP_FINALLY, preserve_tos);
+ if (preserve_tos) {
+ ADDOP(c, ROT_TWO);
+ }
+ ADDOP(c, POP_TOP);
  return 1;
 
  case FOR_LOOP:
@@ -1684,6 +1689,19 @@ compiler_unwind_fblock(struct compiler *c, struct fblockinfo *info,
  ADDOP_JREL(c, CALL_FINALLY, info->fb_exit);
  return 1;
 
+ case FINALLY_TRY2:
+ ADDOP(c, POP_BLOCK);
+ if (preserve_tos) {
+ ADDOP(c, ROT_TWO);
+ ADDOP(c, POP_TOP);
+ ADDOP_JREL(c, CALL_FINALLY, info->fb_exit);
+ }
+ else {
+ ADDOP_JREL(c, CALL_FINALLY, info->fb_exit);
+ ADDOP(c, POP_TOP);
+ }
+ return 1;
+
  case WITH:
  case ASYNC_WITH:
  ADDOP(c, POP_BLOCK);
@@ -2869,17 +2887,47 @@ compiler_continue(struct compiler *c)
 static int
 compiler_try_finally(struct compiler *c, stmt_ty s)
 {
- basicblock *body, *end;
+ basicblock *start, *newcurblock, *body, *end;
+ int break_finally = 1;
 
  body = compiler_new_block(c);
  end = compiler_new_block(c);
  if (body == NULL || end == NULL)
  return 0;
 
+ start = c->u->u_curblock;
+
+ /* `finally` block. Compile it first to determine if any of "break",
+ "continue" or "return" are used in it. */
+ compiler_use_next_block(c, end);
+ if (!compiler_push_fblock(c, FINALLY_END, end, end))
+ return 0;
+ VISIT_SEQ(c, stmt, s->v.Try.finalbody);
+ ADDOP(c, END_FINALLY);
+ break_finally = (c->u->u_fblock[c->u->u_nfblocks - 1].fb_exit == NULL);
+ if (break_finally) {
+ /* Pops a placeholder. See below */
+ ADDOP(c, POP_TOP);
+ }
+ compiler_pop_fblock(c, FINALLY_END, end);
+
+ newcurblock = c->u->u_curblock;
+ c->u->u_curblock = start;
+ start->b_next = NULL;
+
  /* `try` block */
+ c->u->u_lineno_set = 0;
+ c->u->u_lineno = s->lineno;
+ c->u->u_col_offset = s->col_offset;
+ if (break_finally) {
+ /* Pushes a placeholder for the value of "return" in the "try" block
+ to balance the stack for "break", "continue" and "return" in
+ the "finally" block. */
+ ADDOP_LOAD_CONST(c, Py_None);
+ }
  ADDOP_JREL(c, SETUP_FINALLY, end);
  compiler_use_next_block(c, body);
- if (!compiler_push_fblock(c, FINALLY_TRY, body, end))
+ if (!compiler_push_fblock(c, break_finally ? FINALLY_TRY2 : FINALLY_TRY, body, end))
  return 0;
  if (s->v.Try.handlers && asdl_seq_LEN(s->v.Try.handlers)) {
  if (!compiler_try_except(c, s))
@@ -2890,15 +2938,11 @@ compiler_try_finally(struct compiler *c, stmt_ty s)
  }
  ADDOP(c, POP_BLOCK);
  ADDOP(c, BEGIN_FINALLY);
- compiler_pop_fblock(c, FINALLY_TRY, body);
+ compiler_pop_fblock(c, break_finally ? FINALLY_TRY2 : FINALLY_TRY, body);
+
+ c->u->u_curblock->b_next = end;
+ c->u->u_curblock = newcurblock;
 
- /* `finally` block */
- compiler_use_next_block(c, end);
- if (!compiler_push_fblock(c, FINALLY_END, end, NULL))
- return 0;
- VISIT_SEQ(c, stmt, s->v.Try.finalbody);
- ADDOP(c, END_FINALLY);
- compiler_pop_fblock(c, FINALLY_END, end);
  return 1;
 }