py/vm: Change comparison for finally handler search from > to >=.

dpgeorge · dpgeorge · commit 0db046b67b8e · 2022-06-20T22:28:18.000+10:00
The search in these cases should include all finally handlers that are
after the current ip.  If a handler starts at exactly ip then it is
considered "after" the ip.  This can happen when END_FINALLY is followed
immediately by a finally handler (from a different finally).

Consider the function:

    def f():
        try:
            return 0
        finally:
            print(1)

The current bytecode emitter generates the following code:

    00 SETUP_FINALLY 5
    02 LOAD_CONST_SMALL_INT 0
    03 RETURN_VALUE
    04 LOAD_CONST_NONE              ****
    05 LOAD_GLOBAL print
    07 LOAD_CONST_SMALL_INT 1
    08 CALL_FUNCTION n=1 nkw=0
    10 POP_TOP
    11 END_FINALLY
    12 LOAD_CONST_NONE
    13 RETURN_VALUE

The LOAD_CONST_NONE marked with **** is dead code because it follows a
RETURN_VALUE, and nothing jumps to this LOAD_CONST_NONE.  If the emitter
could remove this this dead code it would produce:

    00 SETUP_FINALLY 4
    02 LOAD_CONST_SMALL_INT 0
    03 RETURN_VALUE
    04 LOAD_GLOBAL print
    06 LOAD_CONST_SMALL_INT 1
    07 CALL_FUNCTION n=1 nkw=0
    09 POP_TOP
    10 END_FINALLY
    11 LOAD_CONST_NONE
    12 RETURN_VALUE

In this case the finally block (which starts at offset 4) immediately
follows the RETURN_VALUE.  When RETURN_VALUE executes ip will point to
offset 4 in the bytecode (because the dispatch of the opcode does *ip++)
and so the finally handler will only be found if a &gt;= comparison is used.

It's a similar story for break/continue:

    while True:
        try:
            break
        finally:
            print(1)

Although technically in this case the &gt; comparison still works because the
extra byte from the UNWIND_JUMP (encoding the number of exception handlers
to unwind) doesn't have a *ip++ (just a *ip) so ip remains pointing within
the UNWIND_JUMP opcode, and not at the start of the following finally
handler.  Nevertheless, the change is made to use &gt;= for consistency with
the RETURN_VALUE change.

Signed-off-by: Damien George &lt;damien@micropython.org&gt;
diff --git a/py/vm.c b/py/vm.c
@@ -658,7 +658,7 @@ unwind_jump:;
                         assert(exc_sp >= exc_stack);
 
                         if (MP_TAGPTR_TAG1(exc_sp->val_sp)) {
-                            if (exc_sp->handler > ip) {
+                            if (exc_sp->handler >= ip) {
                                 // Found a finally handler that isn't active; run it.
                                 // Getting here the stack looks like:
                                 //     (..., X, dest_ip)
@@ -1079,7 +1079,7 @@ unwind_jump:;
                     // 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 (exc_sp->handler > ip) {
+                            if (exc_sp->handler >= ip) {
                                 // Found a finally handler that isn't active; run it.
                                 // Getting here the stack looks like:
                                 //     (..., X, [iter0, iter1, ...,] ret_val)
