-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
py: Fix unwinding of try-finally with break in finally clause #4504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
try: | ||
raise Exception | ||
finally: | ||
break |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
On my local machine minimal x86 ( |
No longer needed.
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.
e1ead40
to
ec32434
Compare
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. |
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. |
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge.
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:
Currently in uPy the above code will print:
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:
The bug with break/continue in (nested) finallys is fixed.
The opcodes POP_BLOCK and POP_EXCEPT are replaced by POP_EXCEPT_JUMP.
Generated bytecode is reduced by 2 bytes for each use of a try-except (due to point 2 above).
Code size is reduced: