Include overflow checks in tuple conversions #19418

cston · 2017-05-10T23:40:25Z

Customer scenario

Tuple conversions in checked contexts should include overflow checks.

Bugs this fixes:

Workarounds, if any

Deconstruct and reconstruct tuples, applying conversions on each element individually.

Risk

Low. Note however that the conditions in which BoundConversion.Checked is set have been simplified significantly. For non-Expression trees, binding and lowering set the state based only on whether the expression is in a checked context. For Expression trees, the conditions are now implemented in lowering only rather than split between binding and lowering. The code generator, which is responsible for emitting overflow checks, has not changed however and the code generator implements a set of checks based on the actual types if BoundConversion.Checked is set.

Note, this is a breaking change since overflow checks were not included within tuple conversions in 15.0.

Performance impact

Low

How was the bug found?

Customer reported

cston · 2017-05-11T16:19:11Z

@dotnet/roslyn-compiler please review.

TyOverby · 2017-05-11T17:55:57Z

LGTM, but you might want to check out the test failure, those unit tests are typically not very flaky.

cston · 2017-05-11T18:12:37Z

@dotnet-bot retest windows_release_unit32_prtest please

cston · 2017-05-11T18:13:03Z

@dotnet-bot retest windows_release_vs-integration_prtest please

AlekseyTs · 2017-05-11T18:13:36Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

-            return
-                sourceIsNumeric && (targetIsNumeric || target.IsPointerType()) ||
-                targetIsNumeric && source.IsPointerType();
+            return CheckOverflowAtRuntime && LocalRewriter.NeedsCheckedConversion(source, target, compareNumericTypes: false);


return CheckOverflowAtRuntime && LocalRewriter.NeedsCheckedConversion(source, target, compareNumericTypes: false); [](start = 12, length = 114)

I do not understand why do we need this complicated analysis in binding, simply capturing the value of CheckOverflowAtRuntime should be good enough, I believe. #Closed

jcouv · 2017-05-11T18:14:52Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+    {
+        try
+        {
+            var u = Unchecked((-1, -1));


Consider move Unchecked call out of try/catch.

jcouv · 2017-05-11T18:16:58Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+                Diagnostic(ErrorCode.ERR_ConstOutOfRangeChecked, "-2").WithArguments("-2", "byte").WithLocation(14, 44));
+        }
+
+        [Fact]


Would it be useful to check emitted IL when neither checked or unchecked is used? Which one is the default behavior?

jcouv · 2017-05-11T18:17:22Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+overflow");
+            verifier.VerifyIL("C.Unchecked",
+@"{
+ ...


Is this right?

Oh, nevermind, this is skipped ;-)

In reply to: 116064973 [](ancestors = 116064973)

AlekseyTs · 2017-05-11T18:17:41Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -110,7 +110,7 @@ private static bool IsFloatPointExpressionOfUnknownPrecision(BoundExpression rew
            Debug.Assert((object)rewrittenType != null);
            @checked = @checked &&
                (_inExpressionLambda && (explicitCastInCode || DistinctSpecialTypes(rewrittenOperand.Type, rewrittenType)) ||
-                NeedsChecked(rewrittenOperand.Type, rewrittenType));
+                NeedsCheckedConversion(rewrittenOperand.Type, rewrittenType, compareNumericTypes: true));


NeedsCheckedConversion(rewrittenOperand.Type, rewrittenType, compareNumericTypes: true)); [](start = 16, length = 89)

The same comment is here, I think we should simply propagate what is given as an input and let IL generation to ignore it when it is not applicable. I think this is what VB does. #Closed

jcouv · 2017-05-11T18:20:09Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

@@ -22872,5 +22871,174 @@ void M<T>()
                Diagnostic(ErrorCode.ERR_TypelessTupleInAs, "(0, null) as (int, T)?").WithLocation(6, 17)
                );
        }
+
+        [Fact]


Do we need to notify/review this change with the compat council?

AlekseyTs · 2017-05-11T18:24:21Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

+    {
+        unchecked
+        {
+            (a, b) = ((long, byte))t;


(a, b) = ((long, byte))t; [](start = 12, length = 25)

(a, b) = ((long, byte))t; [](start = 12, length = 25)

It doesn't look like this tests deconstruction conversions, because types on both sides match. #Closed

Removed test.

AlekseyTs · 2017-05-11T18:26:01Z

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb

@@ -20200,6 +20200,51 @@ End Namespace

        End Sub

+        <Fact>
+        Public Sub CheckedConversions()


CheckedConversions [](start = 19, length = 18)

Should also test "unchecked" context. #Closed

AlekseyTs · 2017-05-11T18:27:26Z

Done with review pass. #Closed

jcouv · 2017-05-11T18:31:28Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

-
-            return
-                sourceIsNumeric && (targetIsNumeric || target.IsPointerType()) ||
-                targetIsNumeric && source.IsPointerType();


It looks like NeedsCheckedConversion handles more case than this method used to. The new method checks sourceIsNumeric && targetIsNumeric && (!compareNumericTypes || s_needsChecked[sourceST - SpecialType.System_Char, targetST - SpecialType.System_Char]) .

Does that mean we fixed another gap by this refactoring? If that gap exists and can be identified, maybe it should be tested.

This method calls NeedsCheckedConversion(..., compareNumericTypes: false) to skip that last check, for compatibility with the previous implementation.

That said, I'd prefer to remove the checks and just use CheckOverflowAtRuntime as @AlekseyTs suggested. I'll test that.

AlekseyTs · 2017-05-12T22:56:08Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -428,22 +385,22 @@ private static bool NeedsChecked(TypeSymbol source, TypeSymbol target)
                return false;
            }

-            if (source.IsDynamic())
+            if (source.IsTupleType)


if (source.IsTupleType) [](start = 12, length = 23)

if (source.IsTupleType) [](start = 12, length = 23)

I would remove this check if its effect cannot be observed today. #Closed

cston · 2017-05-13T00:48:42Z

@dotnet-bot retest this please

VSadov · 2017-05-13T01:20:57Z

We went with @cston over these changes trying to really understand the purpose of old code. It appears that we had several layers that would try removing checked bit from conversions when it does not make sense or does not make any difference.
However there is no dependency on this behavior in emit and in the case of expression trees there was some redundancy.

As we can see in the regular (not expression tree) case, removal of inapplicable checked is not observable since codegen ignores inapplicable checked anyways.

In the case of expression trees we keep the removal of inapplicable checked, but it was moved to the lowering and some redundant checks removed.

Other possible ways to see the difference are through semantic model or IOperation, but neither expose information about "checked".

So overall this change makes behavior of "checked" bit more rational, while we do not see any observable changes in the results.

cston · 2017-05-17T18:25:24Z

@dotnet/roslyn-compiler, all comments addressed, please review.

jcouv · 2017-05-18T03:17:34Z

Looks like there are some legitimate test failures.

jcouv · 2017-05-18T03:23:39Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

-                sourceIsNumeric && targetIsNumeric && s_needsChecked[sourceST - SpecialType.System_Char, targetST - SpecialType.System_Char];
+            return IsUnderlyingTypeInRange(source, SpecialType.System_Char, SpecialType.System_Double) &&
+                IsUnderlyingTypeInRange(target, SpecialType.System_Char, SpecialType.System_UInt64);
+        }


This simplification seems like magic to me. How did we do away with the truth table?

The table was not used for Expression lambas. See the previous code at LocalRewriter_Conversions.cs line 111, @checked depended on DistinctSpecialTypes only in that case. (And DistinctSpecialTypes had overlap with this method, so the two were combined.)

Ah, I see. Thanks for the clarification!

jcouv · 2017-05-18T03:39:04Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenCheckedTests.cs

+            CheckedConversionInExpressionTree_Implicit("ulong", "float", ConvertMethod.Convert);
+            CheckedConversionInExpressionTree_Implicit("ulong", "double", ConvertMethod.Convert);
+
+            // decimal


Sorry, there is something else I didn't understand here. I expected to see test for "decimal" to " "float" and "decimal" to "double", to complete the testing of half of the matrix.
On a related note, I didn't get why only testing half of the matrix. For instance, "decimal" to "ulong".

Oh, nevermind the second question. The other half of the matrix cannot be implicit conversion, right?

jcouv · 2017-05-18T03:39:27Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenCheckedTests.cs

+            CheckedConversionInExpressionTree_Implicit("uint", "double", ConvertMethod.Convert);
+
+            // long
+            CheckedConversionInExpressionTree_Implicit("long", "long", ConvertMethod.None);


"ulong" as well?

This test is testing implicit conversions, so some combinations such as long to ulong cannot be tested since there is no implicit conversion.

jcouv

LGTM

cston · 2017-05-18T20:48:03Z

@MeiChin-Tsai for approval.

MeiChin-Tsai · 2017-05-18T20:56:11Z

@cston How do we evaluate the breaking change impact?

cston · 2017-05-18T21:04:25Z

@MeiChin-Tsai The breaking change impact should be limited since the change affects checked conversions of tuples only, and tuples are new to C#7.0. Without this fix, overflow checks are dropped from checked conversions so the result is incorrect at runtime. With this fix, the conversion will result in a OverflowException at runtime.

The breaking change was approved by the compat council.

AlekseyTs

LGTM

MeiChin-Tsai · 2017-05-18T21:22:46Z

approved.

cston · 2017-05-18T22:19:41Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

cston · 2017-05-18T22:20:06Z

@dotnet-bot retest windows_release_vs-integration_prtest please

dnfclas added the cla-already-signed label May 10, 2017

cston force-pushed the 18459 branch from c1d4265 to 1e108a4 Compare May 11, 2017 16:18

AlekseyTs reviewed May 11, 2017

View reviewed changes

jcouv reviewed May 11, 2017

View reviewed changes

AlekseyTs reviewed May 11, 2017

View reviewed changes

jcouv reviewed May 11, 2017

View reviewed changes

AlekseyTs reviewed May 11, 2017

View reviewed changes

jcouv reviewed May 11, 2017

View reviewed changes

AlekseyTs reviewed May 12, 2017

View reviewed changes

cston added 2 commits May 17, 2017 08:30

Include overflow checks in tuple conversions

22a1588

PR feedback

1cad928

cston force-pushed the 18459 branch from b00b1f9 to 1cad928 Compare May 17, 2017 18:23

gafter added the Area-Compilers label May 17, 2017

jcouv reviewed May 18, 2017

View reviewed changes

jcouv approved these changes May 18, 2017

View reviewed changes

Fix unit test failures

1104a92

jcouv approved these changes May 18, 2017

View reviewed changes

AlekseyTs approved these changes May 18, 2017

View reviewed changes

cston merged commit ba0cefe into dotnet:master May 18, 2017

cston deleted the 18459 branch March 15, 2024 18:16

Include overflow checks in tuple conversions #19418

Include overflow checks in tuple conversions #19418

Conversation

cston commented May 10, 2017 • edited Loading

cston commented May 11, 2017

TyOverby commented May 11, 2017

cston commented May 11, 2017

cston commented May 11, 2017

AlekseyTs May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 13, 2017

VSadov commented May 13, 2017

cston commented May 17, 2017

jcouv commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston commented May 18, 2017

MeiChin-Tsai commented May 18, 2017

cston commented May 18, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

MeiChin-Tsai commented May 18, 2017

cston commented May 18, 2017

cston commented May 18, 2017

cston commented May 10, 2017 •

edited

Loading

AlekseyTs May 11, 2017 •

edited

Loading

AlekseyTs May 11, 2017 •

edited

Loading

AlekseyTs May 11, 2017 •

edited

Loading

AlekseyTs May 11, 2017 •

edited

Loading

AlekseyTs commented May 11, 2017 •

edited

Loading

AlekseyTs May 12, 2017 •

edited

Loading