Skip to content
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

ConstantValue needs an overhaul #18579

Open
gafter opened this issue Apr 10, 2017 · 2 comments
Open

ConstantValue needs an overhaul #18579

gafter opened this issue Apr 10, 2017 · 2 comments
Labels
Area-Compilers Bug Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Milestone

Comments

@gafter
Copy link
Member

gafter commented Apr 10, 2017

There are numerous inconsistencies and irregularities in ConstantValue. For one, the conversions implemented do not match either language's conversion rules. For example, a value of type byte or ushort is sign-extended rather than padded when converted to int. We should have more extensive documentation and tests, and the implementation should be simplified so as to be more reliable.

See also #18521 and https://github.com/gafter/roslyn/tree/ConstantValue

@gafter gafter added Area-Compilers Bug Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality labels Apr 10, 2017
@gafter gafter added this to the 16.0 milestone Apr 10, 2017
@gafter gafter self-assigned this Apr 10, 2017
gafter added a commit to gafter/roslyn that referenced this issue Apr 10, 2017
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.
@gafter
Copy link
Member Author

gafter commented Apr 11, 2017

I used this snippet to produce some tests:

        static void PrintTest(string type, string typeName)
        {
            Console.WriteLine($@"        [Fact]
        public void Test{typeName}Constant()
        {{
            unchecked
            {{
                void Test{typeName}Helper({type} value)
                {{
                    var k1 = ConstantValue.Create(value);
                    Assert.Equal((byte)value, k1.ByteValue);
                    Assert.Equal((sbyte)value, k1.SByteValue);
                    Assert.Equal((short)value, k1.Int16Value);
                    Assert.Equal((ushort)value, k1.UInt16Value);
                    Assert.Equal((int)value, k1.Int32Value);
                    Assert.Equal((uint)value, k1.UInt32Value);
                    Assert.Equal((long)value, k1.Int64Value);
                    Assert.Equal((ulong)value, k1.UInt64Value);
                }}
                Test{typeName}Helper(({type})(-2));
                Test{typeName}Helper(({type})(-1));
                Test{typeName}Helper(({type})(0));
                Test{typeName}Helper(({type})(1));
                Test{typeName}Helper(({type})(2));
                Test{typeName}Helper({type}.MinValue);
                Test{typeName}Helper(({type})({type}.MinValue + 1));
                Test{typeName}Helper(({type})({type}.MinValue + 2));
                Test{typeName}Helper(({type})({type}.MaxValue / 2 - 2));
                Test{typeName}Helper(({type})({type}.MaxValue / 2 - 1));
                Test{typeName}Helper(({type})({type}.MaxValue / 2));
                Test{typeName}Helper(({type})({type}.MaxValue / 2 + 1));
                Test{typeName}Helper(({type})({type}.MaxValue / 2 + 2));
                Test{typeName}Helper(({type})({type}.MaxValue - 2));
                Test{typeName}Helper(({type})({type}.MaxValue - 1));
                Test{typeName}Helper({type}.MaxValue);
            }}
        }}
");
        }
        static void Main(string[] args)
        {
            PrintTest("byte", "Byte");
            PrintTest("sbyte", "SByte");
            PrintTest("char", "Char");
            PrintTest("short", "Int16");
            PrintTest("ushort", "UInt16");
            PrintTest("int", "Int32");
            PrintTest("uint", "UInt32");
            PrintTest("long", "Int64");
            PrintTest("ulong", "UInt64");
        }

@prjm
Copy link

prjm commented Dec 19, 2017

Just a question from an outsider: I was just looking around the code related to the ConstantValue class (because I tried to understand how constant propagation / constant value binding is working).
Is there any public documentation available which led to this design? Surely, one can read the code, but it's really a lot of code and it is not so easy to understand the big picture while skipping from one method to another. Otherwise, I'm amazed by the clarity of the code: it is very understandable and identifiers are self-explaining.

@gafter gafter modified the milestones: 16.0, Backlog Jan 28, 2019
@gafter gafter removed their assignment Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality
Projects
None yet
Development

No branches or pull requests

2 participants