Skip to content

Conversation

dpgeorge
Copy link
Member

This set of patches fixes a bug in the VM when breaking within a try-finally, and also follows up with optimisations that open up with the bug fixed.

The bug has to do with executing a break within the finally block of a try-finally statement. For example:

def f():
    for x in (1,):
        print('a', x)
        try:
            raise Exception
        finally:
            print(1)
            break
        print('b', x)
f()

Currently in uPy the above code will print:

a 1
1
1
segmentation fault (core dumped)  micropython

Not only is there a seg fault, but the "1" in the finally block is printed twice. This is because when the VM executes a finally block it doesn't really know if that block was executed due to a fall-through of the try (no exception raised), or because an exception is active. In particular, for nested finallys the VM has no idea which of the nested ones have active exceptions and which are just fall-throughs. So when a break (or continue) is executed it tries to unwind all of the finallys, when in fact only some may be active.

It's questionable whether break (or return or continue) should be allowed within a finally block, because they implicitly swallow any active exception, but nevertheless it's allowed by CPython (although almost never used in the standard library). And uPy should at least not crash in such a case.

It took me a while to find an acceptable solution to this bug. Initially I thought to just ban such a construct in the compiler, but eventually I came across the solution presented here. It relies on the fact that exception and finally handlers always appear in the bytecode after the try body.

Note: there was a similar bug with a return in a finally block, but that was previously fixed in b735208


To understand the fix it's best to look at each commit here independently. In the end the following statements can be made:

  1. The bug with break/continue in (nested) finallys is fixed.

  2. The opcodes POP_BLOCK and POP_EXCEPT are replaced by POP_EXCEPT_JUMP.

  3. Generated bytecode is reduced by 2 bytes for each use of a try-except (due to point 2 above).

  4. Code size is reduced:

   bare-arm:   -88 -0.133% 
minimal x86:  -200 -0.129% 
   unix x64:  -320 -0.065%
unix nanbox:  -412 -0.093%
      stm32:  -192 -0.052% 
     cc3200:  -128 -0.069% 
    esp8266:  -676 -0.104% 
      esp32:  -444 -0.040%

try:
raise Exception
finally:
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be interesting to add here more "finally" configuration cases, e.g. mix of where sub-finally is nested within parent's "finally" vs parent's body?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also don't have a single test for try/except/else in the testsuite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would be good to add both these kinds of tests. The former can go in this set of patches here. The latter (else) would be a separate thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottom line, is that merging this without testing the else: clauses shouldn't be done to avoid surprises.

printf("POP_EXCEPT");
case MP_BC_POP_EXCEPT_JUMP:
DECODE_ULABEL; // these labels are always forward
printf("POP_EXCEPT_JUMP " UINT_FMT, (mp_uint_t)(ip + unum - mp_showbc_code_start));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

printf() of course not supposed to go in master. And likely the reason why minimal port grew whole 80 bytes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just replacing one printf with another; this is not compiled into minimal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops sorry, missed this was showbc.c, thought it was vm.c.

@dpgeorge
Copy link
Member Author

On my local machine minimal x86 (make) decreased by 200 bytes, and minimal thumb (make CROSS=1) decreased by 104 bytes. So there's something wrong with the Travis size check.

POP_BLOCK and POP_EXCEPT are now the same, and are always followed by a
JUMP.  So this optimisation reduces code size, and RAM usage of bytecode by
two bytes for each try-except handler.
@dpgeorge dpgeorge force-pushed the py-fix-finally-unwind branch from e1ead40 to ec32434 Compare February 21, 2019 05:36
@dpgeorge
Copy link
Member Author

Bottom line, is that merging this without testing the else: clauses shouldn't be done to avoid surprises.

Agreed. I've now push some try-except-else(-finally) tests to master, rebased this on top of that, and added a test here for try-except-else-finally-break.

@pfalcon
Copy link
Contributor

pfalcon commented Feb 22, 2019

Thanks.

So, is it the case that MicroPython never used POP_BLOCK to implement for/while loops, and resolved break/continue to direct jumps? I missed that. Something they go to in CPython3.8 only ;-).

With that in mind intuitive concerns of "why it's different in CPython" should be silenced.

@dpgeorge
Copy link
Member Author

So, is it the case that MicroPython never used POP_BLOCK to implement for/while loops, and resolved break/continue to direct jumps?

Correct. The direct jumps are unwinding jumps that call any outstanding finally blocks. So the POP_BLOCK is implicit in the unwind jump, I guess you could say.

With that in mind intuitive concerns of "why it's different in CPython" should be silenced.

At the very start of this project I tried to match CPython opcode-for-opcode in the generated bytecode, to make sure I didn't miss anything subtle (hence the opcode names sometimes don't make sense because they corresponded to CPython's names). But since then uPy has moved on, so has CPython, and they now diverge a fair bit.

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge.

@dpgeorge
Copy link
Member Author

dpgeorge commented Mar 5, 2019

Merged in e1fb03f through 5a2599d

@dpgeorge dpgeorge closed this Mar 5, 2019
@dpgeorge dpgeorge deleted the py-fix-finally-unwind branch March 5, 2019 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants