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

Int64 indices are silently truncated when used to access MD array elements #30648

Open
mikedn opened this issue Oct 22, 2018 · 1 comment
Open
Labels
Area-Compilers Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Milestone

Comments

@mikedn
Copy link

mikedn commented Oct 22, 2018

The following code should produce an exception (IndexOutOfRangeException but OverflowException would do too I guess) but it does not:

        static void Main() => Test(new int[42, 42], 41, 41);
        [MethodImpl(MethodImplOptions.NoInlining)]
        static void Test(int[,] a, long x, long y) => a[x | (0x12345678L << 32), y] = 42;

The generated IL code is:

    ldarg.0
    ldarg.1
    ldc.i8     0x1234567800000000
    or
    conv.ovf.i
    ldarg.2
    conv.ovf.i
    ldc.i4.s   42
    call instance void int32[0..., 0...]::Set(int32,int32,int32)

The spec appears to be silent about this case. From the generated code it looks like the intention was to handle this case by using a checked cast, the same is done with SZ arrays.

There must have been some confusion with ldelem. That one accepts both int32 and native int but MD array accessors support only int32 indices and passing a native int argument in this case would result in silent truncation. That conv.ovf.i should really be conv.ovf.i4.

Reproduced with VS 15.8.7 but I'd guess that the problem is much older.

@jaredpar jaredpar added this to the Unknown milestone Oct 24, 2018
@gafter gafter added this to Next Update in Compiler: Gafter Oct 24, 2018
@gafter gafter moved this from Next Update to Next Up in Compiler: Gafter Nov 6, 2018
@gafter gafter moved this from Next Up to Next Update in Compiler: Gafter Nov 9, 2018
@gafter gafter added the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label Nov 9, 2018
@gafter gafter removed their assignment Nov 12, 2018
@gafter gafter moved this from Next Update to Watching in Compiler: Gafter Nov 12, 2018
@mikedn
Copy link
Author

mikedn commented Dec 1, 2018

It seems that UInt32 indices also cause problems. The C# spec sort of rejects the idea of negative array indices, there's no way to create such an array in C#. But you can create one using Array.CreateInstance and then cast it to a C# array type. Then large unsigned indices get treated as valid negative indices:

var a = Array.CreateInstance(typeof(int), new int[] { 10, 10 }, new int[] { -10, -10 });
var md = (int[,])a;
md[-1, -1] = 42;
Console.WriteLine("Yay {0}!", md[uint.MaxValue, uint.MaxValue]); // Happily prints Yay 42!

It would seem that we need to insert a conv.ovf.i4 for UInt32 indices. Not particularly thrilled about that, sometimes it's useful to use uint instead of int for perf reasons. It would be a pity to end up with overflow checks when doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Projects
None yet
Development

No branches or pull requests

3 participants