-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Skip unreachable catch clauses with constant false filter #19294
Conversation
@elijah6 I know a bunch of the best qualified reviewers for changes like this are busy preparing for Build. Even though it's not my area, I like what you're getting done here. I'm tracking this to try and get follow-up on it in a timely manner. |
@@ -880,7 +880,7 @@ static void Main() | |||
// System.Console.Write("catch"); | |||
Diagnostic(ErrorCode.WRN_UnreachableCode, "System").WithLocation(12, 13) | |||
); | |||
//CompileAndVerify(comp); // PEVerify failed with Branch out of the method. Follow-up issue: https://github.com/dotnet/roslyn/issues/18678 | |||
CompileAndVerify(comp); |
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.
Could you verify the execution of the compiled program by adding expectedOutput: ""
?
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.
The program has unhandled exception, and does not have an output. So I added another test that captures a specific exception outside the function (PEVerifyWithUnreachableCatch2
). Without the fix, this program will throw InvalidOperationException
and output "OtherExceptions"
instead.
@@ -739,8 +739,6 @@ public override void CloseScope(ILBuilder builder) | |||
|
|||
internal override void GetExceptionHandlerRegions(ArrayBuilder<Cci.ExceptionHandlerRegion> regions) | |||
{ | |||
Debug.Assert(_handlers.Count > 1); | |||
|
|||
ExceptionHandlerScope tryScope = null; |
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.
Your removing the assertion here implies that _handlers.Count
can be zero. Is that the case?
{ | ||
return sequence.Value is BoundSequencePointExpression expression | ||
&& expression.Expression?.ConstantValue?.BooleanValue == false; | ||
} else if(catchBlock.ExceptionFilterOpt is BoundSequencePointExpression expression) |
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.
Nit: formatting is incorrect in this method. The compiler code has spaces in conditionals if (
and braces on their own line.
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.
Thank you for the review. I will check the overall formatting again and add some extra tests you suggested first
{ | ||
throw new Exception(); | ||
} | ||
catch (Exception) when (false) |
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 add a test for some expression that is statically known to be false, but not literally false
, like true == false
.
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.
I've added more cases in TryCatchConstantFalseFilter3()
{ | ||
if(catchBlock.ExceptionFilterOpt is BoundSequence sequence) | ||
{ | ||
return sequence.Value is BoundSequencePointExpression expression |
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.
Use of expression
in different scopes is confusing.
@gafter, I don't remember exactly the scoping rules for is-pattern variable declarations. Does this seem right?
private static bool IsFilterConstantFalse(BoundCatchBlock catchBlock) | ||
{ | ||
// Depending on compilation options, BoundLiteral can be wrapped by BoundSequence or BoundSequencePointExpression | ||
if(catchBlock.ExceptionFilterOpt != null) |
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.
Consider flipping this condition around: if (... == null) { return false; }
(with correct formatting)
{ | ||
if(catchBlock.ExceptionFilterOpt is BoundSequence sequence) | ||
{ | ||
return sequence.Value is BoundSequencePointExpression expression |
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.
Hum, this feels strange and makes me think maybe this logic of removing a catch block might be better suited in earlier phases (lowering). @gafter what do you think?
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 should not mix two purposes of removing unreachable catch blocks
-
Fixing the bug by removal of formally unreachable catches (as per language spec and control flow rules).
This is easier to do in lowering. - take a look at VisitCatchBlock in LocalRewriter_TryStatements.cs
Checking that the filter is a constant false and returningnull
could be sufficient to fix the bug. -
Additional optimizations where the condition becomes
false
as a result of lowering and further optimizations. That would be harder to do and it would need to be done either in the codegen or in the optimizer pass. And only when /o+ flag is set.
This should be a separate item. Costs/Benefits/Risk for this part would not be as favorable as for the#1
above.
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, that makes more sense... thank you for the clarification. I will clean up the code and try that path
Thanks @elijah6 for the PR. @dotnet/roslyn-compiler for review. In particular, I'm not sure whether some of the logic would be more natural fit for lowering phase, instead of emit phase. |
if(catchBlock.ExceptionFilterOpt is BoundSequence sequence) | ||
{ | ||
return sequence.Value is BoundSequencePointExpression expression | ||
&& expression.Expression?.ConstantValue?.BooleanValue == false; |
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.
This is not correct. Sequence that returns a literal is not the same as a literal. Sequence may have observable sideeffects so one cannot be replaced with another.
@@ -78,6 +78,10 @@ public override BoundNode VisitCatchBlock(BoundCatchBlock node) | |||
{ | |||
return base.VisitCatchBlock(node); | |||
} | |||
if (node.ExceptionFilterOpt.ConstantValue?.BooleanValue == false) |
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 add a blank line here before the if
statement.
@@ -864,7 +864,7 @@ static void Main() | |||
{ | |||
throw new System.Exception(); | |||
} | |||
catch (System.Exception) when (false) | |||
catch (System.Exception) when (default) |
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.
Why did we delete the false
test case here? Sure default
should compile to false
in this case, but that's what testing should verify. My initial reaction here was to wonder why we didn't have a test for false
and default
.
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.
The test class is TargetTypedDefaultTest so I am assuming it is going to contain 'default' related tests. Other cases will be covered by the other set of additional tests that I've written, right? Or should I repeat them here as well as a pair?
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.
That's a fair point. Should probably leave as is .
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.
LGTM
/CC @MeiChin-Tsai for ask mode approval. This is a tiny and well-tested fix for bad code generation that was submitted by a community member. Most of the change is testing. |
Approved. |
Thank you for your contribution! |
Thank you! |
Customer scenario
Write a catch clause with a filter that happens to be a constant whose value is
false
. This doesn't typically occur intentionally. In that case the compiler produces bad code that will not verify.Bugs this fixes:
#18678
Workarounds, if any
Remove the "offending" catch block. The compiler produces a warning that can help guide the programmer to the issue.
Risk
Low. The fix is quite tiny and well-tested.
Performance impact
None expected.
Is this a regression from a previous update?
No.
Root cause analysis:
When the filter expression for a catch clause is false and the end of the try block is unreachable (e.g. because of
throw
intry
block), then the implicit return was not generated, and the unreachable catch block was branching out (leave.s
) to an invalid address. More details described in #18678How was the bug found?
Customer-reported. Note that the bug fix is community-contributed.