-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Exception Handler support #78773
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
Exception Handler support #78773
Changes from all commits
db7bfb8
fbd9e44
9336c5e
0738b1d
6ce727a
ecfd34b
785d299
f939591
8bec7ab
07200ba
0c1b3eb
92399e0
503c056
6be552f
a7f5e4e
06a5bd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ internal sealed class AsyncExceptionHandlerRewriter : BoundTreeRewriterWithStack | |
| private AwaitCatchFrame _currentAwaitCatchFrame; | ||
| private AwaitFinallyFrame _currentAwaitFinallyFrame = new AwaitFinallyFrame(); | ||
| private bool _inCatchWithoutAwaits; | ||
| private bool _needsFinalThrow; | ||
333fred marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| private AsyncExceptionHandlerRewriter( | ||
| MethodSymbol containingMethod, | ||
|
|
@@ -129,9 +130,45 @@ public static BoundStatement Rewrite( | |
| var rewriter = new AsyncExceptionHandlerRewriter(containingSymbol, containingType, factory, analysis); | ||
| var loweredStatement = (BoundStatement)rewriter.Visit(statement); | ||
|
|
||
| loweredStatement = rewriter.FinalizeMethodBody(loweredStatement); | ||
|
|
||
| return loweredStatement; | ||
| } | ||
|
|
||
| private BoundStatement FinalizeMethodBody(BoundStatement loweredStatement) | ||
| { | ||
| if (loweredStatement == null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| // When we add a `switch (pendingBranch)` to the end of the try block, | ||
| // this can result in a method body that cannot be proven to terminate. | ||
| // While we can technically prove it by doing a full data flow analysis, | ||
| // this is effectively the halting problem, and the runtime will not do | ||
| // this analysis. The resulting IL will be technically invalid, and if it's | ||
| // not wrapped in another state machine (a la the compiler async rewriter), | ||
| // the runtime will refuse to load it. For runtime async, where we are effectively | ||
| // emitting the result of this rewriter directly, we need to ensure that | ||
| // we always emit a throw at the end of the try block when the switch is present. | ||
| // This ensures that the method can be proven to terminate, and the runtime will | ||
| // accept it. This throw will never be reached, and we could potentially do a | ||
| // more sophisticated analysis to determine if it is needed by pushing control | ||
| // flow analysis through the bound nodes, see https://github.com/dotnet/roslyn/pull/78970. | ||
| // This is risky, however, and for now we are taking the conservative approach | ||
| // of always emitting the throw. | ||
| BoundStatement result = loweredStatement; | ||
| if (_needsFinalThrow) | ||
| { | ||
| result = _F.Block( | ||
| loweredStatement, | ||
| _F.Throw(_F.Null(_F.SpecialType(SpecialType.System_Object))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope it will, since I expect the automatic implicit return code to not care about whether it's just
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the automatic implicit return worked here. See |
||
| ); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public override BoundNode VisitTryStatement(BoundTryStatement node) | ||
| { | ||
| var tryStatementSyntax = node.Syntax; | ||
|
|
@@ -354,6 +391,7 @@ private BoundStatement UnpendBranches( | |
| cases.Add(caseStatement); | ||
| } | ||
|
|
||
| _needsFinalThrow = true; | ||
333fred marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return _F.Switch(_F.Local(pendingBranchVar), cases.ToImmutableAndFree()); | ||
| } | ||
|
|
||
|
|
@@ -402,22 +440,37 @@ public override BoundNode VisitReturnStatement(BoundReturnStatement node) | |
|
|
||
| private BoundStatement UnpendException(LocalSymbol pendingExceptionLocal) | ||
| { | ||
| // If this is runtime async, we don't need to create a second local for the exception, | ||
| // as the pendingExceptionLocal will not be hoisted to a state machine by a future rewrite. | ||
| if (_F.Compilation.IsRuntimeAsyncEnabledIn(_F.CurrentFunction)) | ||
| { | ||
| // pendingExceptionLocal is already an object | ||
| // so we can just use it directly | ||
| return checkAndThrow(pendingExceptionLocal); | ||
| } | ||
|
|
||
| // create a temp. | ||
| // pendingExceptionLocal will certainly be captured, no need to access it over and over. | ||
| LocalSymbol obj = _F.SynthesizedLocal(_F.SpecialType(SpecialType.System_Object)); | ||
| var objInit = _F.Assignment(_F.Local(obj), _F.Local(pendingExceptionLocal)); | ||
|
|
||
| // throw pendingExceptionLocal; | ||
| BoundStatement rethrow = Rethrow(obj); | ||
|
|
||
| return _F.Block( | ||
| ImmutableArray.Create<LocalSymbol>(obj), | ||
| objInit, | ||
| _F.If( | ||
| _F.ObjectNotEqual( | ||
| _F.Local(obj), | ||
| _F.Null(obj.Type)), | ||
| rethrow)); | ||
| checkAndThrow(obj)); | ||
|
|
||
| BoundStatement checkAndThrow(LocalSymbol obj) | ||
| { | ||
| BoundStatement rethrow = Rethrow(obj); | ||
|
|
||
| BoundStatement checkAndThrow = _F.If( | ||
| _F.ObjectNotEqual( | ||
| _F.Local(obj), | ||
| _F.Null(obj.Type)), | ||
| rethrow); | ||
| return checkAndThrow; | ||
| } | ||
| } | ||
|
|
||
| private BoundStatement Rethrow(LocalSymbol obj) | ||
|
|
@@ -706,14 +759,24 @@ public override BoundNode VisitLambda(BoundLambda node) | |
| { | ||
| var oldContainingSymbol = _F.CurrentFunction; | ||
| var oldAwaitFinallyFrame = _currentAwaitFinallyFrame; | ||
| var oldNeedsFinalThrow = _needsFinalThrow; | ||
|
|
||
| _F.CurrentFunction = node.Symbol; | ||
| _currentAwaitFinallyFrame = new AwaitFinallyFrame(); | ||
| _needsFinalThrow = false; | ||
|
|
||
| var result = base.VisitLambda(node); | ||
| var result = (BoundLambda)base.VisitLambda(node); | ||
| result = result.Update( | ||
| result.UnboundLambda, | ||
| result.Symbol, | ||
| (BoundBlock)FinalizeMethodBody(result.Body), | ||
| node.Diagnostics, | ||
| node.Binder, | ||
| node.Type); | ||
|
|
||
| _F.CurrentFunction = oldContainingSymbol; | ||
| _currentAwaitFinallyFrame = oldAwaitFinallyFrame; | ||
| _needsFinalThrow = oldNeedsFinalThrow; | ||
|
|
||
| return result; | ||
| } | ||
|
|
@@ -722,14 +785,18 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen | |
| { | ||
| var oldContainingSymbol = _F.CurrentFunction; | ||
| var oldAwaitFinallyFrame = _currentAwaitFinallyFrame; | ||
| var oldNeedsFinalThrow = _needsFinalThrow; | ||
|
|
||
| _F.CurrentFunction = node.Symbol; | ||
| _currentAwaitFinallyFrame = new AwaitFinallyFrame(); | ||
| _needsFinalThrow = false; | ||
|
|
||
| var result = base.VisitLocalFunctionStatement(node); | ||
| var result = (BoundLocalFunctionStatement)base.VisitLocalFunctionStatement(node); | ||
| result = result.Update(node.Symbol, (BoundBlock)FinalizeMethodBody(result.Body), (BoundBlock)FinalizeMethodBody(result.ExpressionBody)); | ||
|
|
||
| _F.CurrentFunction = oldContainingSymbol; | ||
| _currentAwaitFinallyFrame = oldAwaitFinallyFrame; | ||
| _needsFinalThrow = oldNeedsFinalThrow; | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
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.
Can
awaitInfo.GetResult ?? awaitInfo.RuntimeAsyncAwaitMethodbe null when there's no errors?I still think it's be a good idea to add a
Validatemethod toBoundAwaitableInfoto codify expectations.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 can, when
dynamicis involved.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.
FWIW, I made the following addition and ran
build.cmd -testCompilerOnly -testCoreClrand go no hit: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 possible dynamically-bound
awaits do not go through here, but you absolutely can have both methods benulland not have errors.