This is an archive of the discontinued LLVM Phabricator instance.

[Parse] Use CapturedStmt for @finally on MSVC
ClosedPublic

Authored by smeenai on May 30 2018, 4:47 PM.

Details

Summary

The body of a @finally needs to be executed on both exceptional and
non-exceptional paths. On landingpad platforms, this is straightforward:
the @finally body is emitted as a normal (non-exceptional) cleanup,
and then a catch-all is emitted which branches to that cleanup (the
cleanup has code to conditionally re-throw based on a flag which is set
by the catch-all).

Unfortunately, we can't use the same approach for MSVC exceptions, where
the catch-all will be emitted as a catchpad. We can't just branch to the
cleanup from within the catchpad, since we can only exit it via a
catchret, at which point the exception is destroyed and we can't
rethrow. We could potentially emit the finally body inside the catchpad
and have the normal cleanup path somehow branch into it, but that would
require some new IR construct that could branch into a catchpad.

Instead, after discussing it with Reid Kleckner, we decided that
frontend outlining was the best approach, similar to how SEH __finally
works today. We decided to use CapturedStmt (which was also suggested by
Reid) rather than CaptureFinder (which is what __finally uses) since
the latter doesn't handle a lot of cases we care about, e.g. self
accesses, property accesses, block captures, etc. Extending
CaptureFinder to handle those additional cases proved unwieldy, whereas
CapturedStmt already took care of all of those. In theory __finally
could also be moved over to CapturedStmt, which would remove some
existing limitations (e.g. the inability to capture this), although
CaptureFinder would still be needed for SEH filters.

The one case supported by @finally but not CapturedStmt (or
CaptureFinder for that matter) is arbitrary control flow out of the
@finally, e.g. having a return statement inside a @finally. We can
add that support as a follow-up, but in practice we've found it to be
used very rarely anyway.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai created this revision.May 30 2018, 4:47 PM
smeenai updated this revision to Diff 149220.May 30 2018, 4:48 PM

Proper diff

rnk added a comment.May 31 2018, 1:33 PM

Using CapturedStmt to do frontend outlining was the direction I suggested. I want to hear what @rsmith and @rjmccall think, though.

lib/Parse/ParseObjc.cpp
2588 ↗(On Diff #149220)

Parser has a getTargetInfo() method, so this can be shorter.

smeenai updated this revision to Diff 149351.May 31 2018, 1:40 PM
smeenai edited the summary of this revision. (Show Details)

@rnk comment

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

There's already some logic in CapturedStmt to prevent branches out of the block:

  • Attempting to return will produce "cannot return from Objective-C @finally statement"
  • Attempting to goto out of the block will result in "use of undeclared label", which is a bad diagnostic (and should be improved), but it does error

Are there any other branches you had in mind? I could add tests for them, perhaps, though it's also covered by test/Sema/captured-statements.c.

It should be possible to add support for returns, at least; the idea we'd discussed with @rnk was setting a flag in the captured function to indicate a return having been executed, and then reading that flag outside the captured function and acting on it appropriately. gotos would be more complicated, but I think we could make them work if we really wanted to.

Throwing an exception out should just work, I think; the outlined function will just participate normally in exception handling. Did you have a specific case you were thinking of?

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

There's already some logic in CapturedStmt to prevent branches out of the block:

  • Attempting to return will produce "cannot return from Objective-C @finally statement"
  • Attempting to goto out of the block will result in "use of undeclared label", which is a bad diagnostic (and should be improved), but it does error

Alright, that makes sense.

It should be possible to add support for returns, at least; the idea we'd discussed with @rnk was setting a flag in the captured function to indicate a return having been executed, and then reading that flag outside the captured function and acting on it appropriately. gotos would be more complicated, but I think we could make them work if we really wanted to.

Throwing an exception out should just work, I think; the outlined function will just participate normally in exception handling. Did you have a specific case you were thinking of?

No, it was just a general question. Have you gotten this to a point where it's testable?

That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of the block, and I don't know how this will interact with attempts to throw an exception out.

There's already some logic in CapturedStmt to prevent branches out of the block:

  • Attempting to return will produce "cannot return from Objective-C @finally statement"
  • Attempting to goto out of the block will result in "use of undeclared label", which is a bad diagnostic (and should be improved), but it does error

Alright, that makes sense.

It should be possible to add support for returns, at least; the idea we'd discussed with @rnk was setting a flag in the captured function to indicate a return having been executed, and then reading that flag outside the captured function and acting on it appropriately. gotos would be more complicated, but I think we could make them work if we really wanted to.

Throwing an exception out should just work, I think; the outlined function will just participate normally in exception handling. Did you have a specific case you were thinking of?

No, it was just a general question. Have you gotten this to a point where it's testable?

Yup, it's been working fine in my local testing. There's one more patch that I need to put up, which actually handles doing proper codegen for @try/@catch/@finally; I'm working on cleaning that up right now. The other piece of the puzzle is D47233, which emits the proper typeinfo required for this.

No, it was just a general question. Have you gotten this to a point where it's testable?

Yup, it's been working fine in my local testing. There's one more patch that I need to put up, which actually handles doing proper codegen for @try/@catch/@finally; I'm working on cleaning that up right now.

Okay. And simple tests with throwing exceptions out of the @finally block seem to work?

The other piece of the puzzle is D47233, which emits the proper typeinfo required for this.

Yes, I'll try to get to that, sorry.

No, it was just a general question. Have you gotten this to a point where it's testable?

Yup, it's been working fine in my local testing. There's one more patch that I need to put up, which actually handles doing proper codegen for @try/@catch/@finally; I'm working on cleaning that up right now.

Okay. And simple tests with throwing exceptions out of the @finally block seem to work?

Yup, it works fine. It's essentially the same as calling a function inside a catch block which throws (since the finally is modeled as a catch-all, and the finally body is outlined into a function which gets called by that catch-all).

rjmccall accepted this revision.Jun 4 2018, 3:26 PM

Okay, We can try this, then.

This revision is now accepted and ready to land.Jun 4 2018, 3:26 PM
This revision was automatically updated to reflect the committed changes.