From b735208403a54774f9fd3d966f7c1a194c41870f Mon Sep 17 00:00:00 2001 From: Damien George Date: Mon, 3 Sep 2018 13:08:16 +1000 Subject: [PATCH] py/vm: Fix handling of finally-return with complex nested finallys. Back in 8047340d7532ec32bc9f2d603bffc0bc9544297f basic support was added in the VM to handle return statements within a finally block. But it didn't cover all cases, in particular when some finally's were active and others inactive when the "return" was executed. This patch adds further support for return-within-finally by correctly managing the currently_in_except_block flag, and should fix all cases. The main point is that finally handlers remain on the exception stack even if they are active (currently being executed), and the unwind return code should only execute those finally's which are inactive. New tests are added for the cases which now pass. --- py/vm.c | 16 ++--- tests/basics/try_finally_return3.py | 103 ++++++++++++++++++++++++++++ tests/run-tests | 1 + 3 files changed, 109 insertions(+), 11 deletions(-) create mode 100644 tests/basics/try_finally_return3.py diff --git a/py/vm.c b/py/vm.c index 498ecb491d..bc596b0e86 100644 --- a/py/vm.c +++ b/py/vm.c @@ -1063,17 +1063,11 @@ unwind_jump:; ENTRY(MP_BC_RETURN_VALUE): MARK_EXC_IP_SELECTIVE(); - // These next 3 lines pop a try-finally exception handler, if one - // is there on the exception stack. Without this the finally block - // is executed a second time when the return is executed, because - // the try-finally exception handler is still on the stack. - // TODO Possibly find a better way to handle this case. - if (currently_in_except_block) { - POP_EXC_BLOCK(); - } unwind_return: + // Search for and execute finally handlers that aren't already active while (exc_sp >= exc_stack) { - if (MP_TAGPTR_TAG1(exc_sp->val_sp)) { + if (!currently_in_except_block && MP_TAGPTR_TAG1(exc_sp->val_sp)) { + // Found a finally handler that isn't active. // Getting here the stack looks like: // (..., X, [iter0, iter1, ...,] ret_val) // where X is pointed to by exc_sp->val_sp and in the case @@ -1092,10 +1086,10 @@ unwind_return: // done (when WITH_CLEANUP or END_FINALLY reached). PUSH(MP_OBJ_NEW_SMALL_INT(-1)); ip = exc_sp->handler; - exc_sp--; + POP_EXC_BLOCK(); goto dispatch_loop; } - exc_sp--; + POP_EXC_BLOCK(); } nlr_pop(); code_state->sp = sp; diff --git a/tests/basics/try_finally_return3.py b/tests/basics/try_finally_return3.py new file mode 100644 index 0000000000..a2a06ee975 --- /dev/null +++ b/tests/basics/try_finally_return3.py @@ -0,0 +1,103 @@ +# test 'return' within the finally block, with nested finally's +# only inactive finally's should be executed, and only once + +# basic nested finally's, the print should only be executed once +def f(): + try: + raise TypeError + finally: + print(1) + try: + raise ValueError + finally: + return 42 +print(f()) + +# similar to above but more nesting +def f(): + try: + raise ValueError + finally: + print(1) + try: + raise TypeError + finally: + print(2) + try: + pass + finally: + return 42 +print(f()) + +# similar to above but even more nesting +def f(): + try: + raise ValueError + finally: + print(1) + try: + raise TypeError + finally: + print(2) + try: + raise Exception + finally: + print(3) + return 42 +print(f()) + +# upon return some try's are active, some finally's are active, some inactive +def f(): + try: + try: + pass + finally: + print(2) + return 42 + finally: + print(1) +print(f()) + +# same as above but raise instead of pass +def f(): + try: + try: + raise ValueError + finally: + print(2) + return 42 + finally: + print(1) +print(f()) + +# upon return exception stack holds: active finally, inactive finally, active finally +def f(): + try: + raise Exception + finally: + print(1) + try: + try: + pass + finally: + print(3) + return 42 + finally: + print(2) +print(f()) + +# same as above but raise instead of pass in innermost try block +def f(): + try: + raise Exception + finally: + print(1) + try: + try: + raise Exception + finally: + print(3) + return 42 + finally: + print(2) +print(f()) diff --git a/tests/run-tests b/tests/run-tests index a3263fff8a..cfbd017774 100755 --- a/tests/run-tests +++ b/tests/run-tests @@ -368,6 +368,7 @@ def run_tests(pyb, tests, args, base_path="."): skip_tests.add('basics/try_finally_loops.py') # requires proper try finally code skip_tests.add('basics/try_finally_return.py') # requires proper try finally code skip_tests.add('basics/try_finally_return2.py') # requires proper try finally code + skip_tests.add('basics/try_finally_return3.py') # requires proper try finally code skip_tests.add('basics/unboundlocal.py') # requires checking for unbound local skip_tests.add('import/gen_context.py') # requires yield_value skip_tests.add('misc/features.py') # requires raise_varargs