Simplify windows rescue block codegen #6649
Conversation
RX14
added
kind:bug
RX14
force-pushed
the
bugfix/windows-exceptions-break-next
branch
from
98c363c to
ce5bf8e
Compare
|
Please can someone review this. The code comments should be fairly self-explanatory. |
|
Sorry, I'm not capable to review this correctly without deep diving into exceptions and the windows specific stuff. I'm going to trust you here. |
|
@ysbaddaden I was largely expecting bcardiff to review it, since he reviewed the original PR. |
There was a problem hiding this comment.
Double checking: the llvm for non windows seems to be the same, right?
I would like to translate the: .. It's difficult .., to a minimal snippet at least in a comment in the PR for future reference. What is the scenario in crystal code that is hard to track and is not handle by the current code structure and needs this change in how @rescue_block value is kept?
| @catch_pad = old_catch_pad | ||
|
|
||
| # We are generating code for rescues, so set up the rescue block. | ||
| @rescue_block = rescue_ensure_block |
There was a problem hiding this comment.
why there is a need to set the rescue_block in every codegen of the node_rescues instead of setting the @rescue_block only once as before?
There was a problem hiding this comment.
match_any_type_id can generate a call, and in that case on windows, the call would be before we catchret, but it would be inside the rescue_ensure_block so would generate an invoke. This would mean we'd have invokes to the a catchswitch which is outside the catchpad from inside the catchpad which is disallowed. All this moving around is simply to prevent the call to ~match<Type> from being inside the rescue_ensure_block.
Fixes #6605