Skip to content
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

Proof of Concept for exception flow with finally's #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mwijngaard
Copy link
Contributor

@mwijngaard mwijngaard commented Apr 22, 2016

I was hoping you could take a look at this, it's not meant to be merged in its current form. If we can come to some concencus about how this should work then I will add a lot of tests for weird edge cases before merging.

This change implements control flow for try-catch-finally constructs. It only takes explicitly thrown exceptions into account, and does not (yet?) support exceptions thrown in method/function calls. I also implemented the finally's for goto & return.

The finally's were actually a big pain to get working, but I think I got there now. The idea is that we inline finally code in every possible path that can go through it. This is apparently how the JVM does it since 1.6. I guess it could be possible to do it without inlining, but that would require some kind of subroutine or weird jumping logic at the end of a finally block.

I added some Ops for dealing specifically with the current exception. They assume the exception gets stored in some globally available location, and does not use a variable. But maybe some other types of Ops could be more suited?

Any feedback/comments would be much appreciated.

@nikic
Copy link
Collaborator

nikic commented Apr 25, 2016

Not looked at this thoroughly yet, just some first thoughts:

  • This is causing test failures due to extra trailing blocks. Would be nice to get rid of those :)
  • I like the approach of duplicating the finally code(s) for every path, rather than trying to share it. The PHP implementation uses a shared code segment and you don't want to know how many bugs relating to this we've had to fix (in PHP 7.0 the finally implementation is finally something close to "decent" with only a handful of known bugs...)
  • From what I gather, your implementation approach is that you use a stack of goto labels for each level of try/finally and resolve those within a level directly, while those that cross a level need to execute the finally block. (And same for return etc.) Is that about right? (We should check that there are no gotos into finally blocks -- for that matter we should also check that there are no goto's into loops, don't think we have this yet.)
  • For the exception handling, I don't think we should treat throw specially. (As mentioned, in PHP nearly everything can throw.) The mechanism should be generic.

@mwijngaard
Copy link
Contributor Author

  1. I'll take a look at how I can fix the failures, but the trailing blocks are intended to be there so as to signify the difference between a normal and exceptional exit out of the function. I think making this distinction is important, as the behavior of a call Op changes depending on whether the called function exits normally or with an exception. I intend to use something like this for constructing a system dependence graph with exception propagation.

  2. Yes, that is the general idea of the implementation. You're right in that when exiting a loop the labels inside that loop should be removed from the label scope. Same goes for finally. I'll add this to the implementation. It is apparently valid to use a goto for jumping into a catch, which I found pretty weird...

  3. I'm not exactly sure how that could work yet. If anything can throw, that makes it really hard to do any useful reasoning about the code. At least with throw there is an explicitly expected behavior to consider. If we truly want to implement exceptions at any operation, then exception flow should probably be integrated into the Op class, linking any Op to its exception handler. (Perhaps this could also replace the trailing blocks mentioned in 1)) I understand that Java has the same kind of problem, as you could make a thread throw an exception at any time. Maybe I can find something useful there.

@nikic
Copy link
Collaborator

nikic commented May 5, 2016

Regarding 4), linking every Op to the exception handler as an explicit branch target seems like overkill, but we could add it the to the containing block. I.e. either introduce a special TryBlock that contains an exception target, or just add this is a potentially null field to all blocks (the latter may be better to avoid negative interference with ErrorSuppression blocks).

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