Fix a bug in code gen for switch on byte or ushort #18521
Conversation
gafter
changed the title
Can you link to where this line occurred? |
|
It looks like release CI tests are failing due to identical problem. Could changes in this PR be the root cause of the failure? #Closed |
|
The line occurs here: |
| @@ -609,6 +625,22 @@ public override ushort UInt16Value | |||
| } | |||
| } | |||
|
|
|||
| public override int Int32Value | |||
There was a problem hiding this comment.
public override int Int32Value [](start = 12, length = 30)
Should we also override UInt32Value property? In general, I find semantics of properties that return unsigned values unclear. For example, the following code prints two different numbers
short x = -1; // ConstantValueTypeDiscriminator.Int16
System.Console.WriteLine((uint)x);
System.Console.WriteLine((uint)(ushort)x);
Which one do we want to be the result of UInt32Value property?
| @@ -609,6 +625,22 @@ public override ushort UInt16Value | |||
| } | |||
| } | |||
|
|
|||
| public override int Int32Value | |||
There was a problem hiding this comment.
public override int Int32Value [](start = 12, length = 30)
The same question is for UInt64Value property, the following code prints three different numbers, which one is the expected result?
short x = -1; // ConstantValueTypeDiscriminator.Int16
System.Console.WriteLine((ulong)x);
System.Console.WriteLine((ulong)(uint)x);
System.Console.WriteLine((ulong)(ushort)x);
| { | ||
| get | ||
| { | ||
| switch (this.Discriminator) |
There was a problem hiding this comment.
What does Discriminator track for ConstantValue? There is no comment and reading the code it's hard to understand why only the values specified here would be valued for each of the new overrides.
There was a problem hiding this comment.
This method appears in the specialized class whose purpose is to implement precisely these two discriminator kinds.
| @@ -560,6 +560,22 @@ public override sbyte SByteValue | |||
| } | |||
| } | |||
|
|
|||
| public override short Int16Value | |||
There was a problem hiding this comment.
public override short Int16Value [](start = 12, length = 32)
Should we also override UInt16Value, Uint32Value and UInt64Value properties? For example, what value do we want to get from UInt16Value property for the following scenario (two different values are printed)?
byte x = byte.MaxValue; // ConstantValueTypeDiscriminator.SByte
System.Console.WriteLine((ushort)x);
System.Console.WriteLine((ushort)(sbyte)x);
There was a problem hiding this comment.
No, unsigned operations do not need special casing.
The underlying field is unsigned, so it will be properly zero-extended. (returning 255 for byte.MaxValue)
In reply to: 110446716 [](ancestors = 110446716)
There was a problem hiding this comment.
Actually, no. I changed my mind :-)
However intuitive it might be to return same 255, it is not what C# constant conversion does, and it might be more consistent to be consistent with that:
I.E. UInt16Value should have same behavior as in:
System.Console.WriteLine(unchecked((ushort)sbyte.MinValue));
System.Console.WriteLine(unchecked((ushort)sbyte.MaxValue));
System.Console.WriteLine(unchecked((ushort)byte.MinValue));
System.Console.WriteLine(unchecked((ushort)byte.MaxValue));
That would imply sign-extending when underlying is a signed value according to the discriminant.
Then we truly can, one day, use this in constant folding.
In reply to: 110450281 [](ancestors = 110450281,110446716)
| @@ -660,6 +692,22 @@ public override uint UInt32Value | |||
| } | |||
| } | |||
|
|
|||
| public override long Int64Value | |||
There was a problem hiding this comment.
public override long Int64Value [](start = 12, length = 31)
Should we also override UInt64Value property?
|
I think we:
|
|
I'm thinking of placing this approach on the back burner. I think that ConstantValue needs an overhaul, but that would be too disruptive a fix for #18188 to get it into a patch release. I'll prepare a more localized fix in a fresh PR. |
The bug arose because of an interaction of this line (new in VS2017)
``` c#
_builder.EmitIntConstant(endConstant.Int32Value - startConstant.Int32Value);
```
in `SwitchIntegralJumpTableEmitter` and an incorrect implementation of `Int32Value` for constants of type `ushort` and `ubyte`, which is recorded as a separate bug in dotnet#18579. Specifically, the implementation causes improper sign extension of what should be treated as an unsigned value.
We never caught this before because we didn't rely on it. Constant folding in the compiler goes through another (redundant) code path that gets the padding vs sign extension correct. Because this is a serious regression in the compiler, I am hoping we can include this fix is some upcoming patch at our earliest opportunity. Therefore I have included a simple, local fix for the symptom, while leaving the larger issue of `ConstantValue` to be dealt with in a later larger change.
|
@VSadov @AlekseyTs I've reverted the changes to |
|
@VSadov Can you please re-review? I simplified the fix. |
2 similar comments
|
@VSadov Can you please re-review? I simplified the fix. |
|
@VSadov Can you please re-review? I simplified the fix. |
| @@ -466,7 +466,20 @@ private void EmitRangeCheckedBranch(ConstantValue startConstant, ConstantValue e | |||
| } | |||
| else | |||
| { | |||
| _builder.EmitIntConstant(endConstant.Int32Value - startConstant.Int32Value); | |||
| int Int32Value(ConstantValue value) | |||
There was a problem hiding this comment.
Do we need to shim Int64Value in the code above the same way?
Basically, if the testcase is changed slightly to be void RunTest(ulong testUlong) with everything else the same, do we still have a problem?
I think we do not have a problem since labels will be statically promoted to ulong.
Probably worth adding a testcase though.
There was a problem hiding this comment.
Ah, I see the literals must have signed bits, which will force them to be 64bit and thus there will be no sign or zero extending.
Basically the case is only relevant for switch types shorter than uint
|
@gafter - perhaps need to change the title of the PR. |
gafter
changed the title
Fixes #18188
The bug arose because of an interaction of this line (new in VS2017)
in
SwitchIntegralJumpTableEmitterand an incorrect implementation ofInt32Valuefor constants of typeushortandubyte, which is recorded as a separate bug in #18579. Specifically, the implementation causes improper sign extension of what should be treated as an unsigned value.We never caught this before because we didn't rely on it. Constant folding in the compiler goes through another (redundant) code path that gets the padding vs sign extension correct. Because this is a serious regression in the compiler, I am hoping we can include this fix is some upcoming patch at our earliest opportunity. Therefore I have included a simple, local fix for the symptom, while leaving the larger issue of
ConstantValueto be dealt with in a later larger change.@VSadov @dotnet/roslyn-compiler May I please have a couple of reviews of this simple fix to a nasty bug?