JIT generate wrong code when enabling overflow compiler flag

[tarekgh]

When enabling the overflow/underflow compiler flag, there is a case when JIT generate wrong code. basically we have a method

        internal unsafe void AdjustBytes(int count)
        {
            _bytes += count;
        }

in the file
https://github.com/dotnet/corefx/blob/10733b7bb84d6132f4ac6fd83f3405f3ebd689a6/src/System.Text.Encoding.CodePages/src/System/Text/EncodingCharBuffer.cs#L91

When compiling with overflow flag on, the jit will produce the following

00007FFF46E67D50  push        rbp  
00007FFF46E67D51  push        rdi  
00007FFF46E67D52  push        rsi  
00007FFF46E67D53  sub         rsp,30h  
00007FFF46E67D57  mov         rbp,rsp  
00007FFF46E67D5A  xor         eax,eax  
00007FFF46E67D5C  mov         qword ptr [rbp+28h],rax  
00007FFF46E67D60  mov         qword ptr [rbp+50h],rcx  
00007FFF46E67D64  mov         dword ptr [rbp+58h],edx  
00007FFF46E67D67  cmp         dword ptr [7FFF46DFB728h],0  
00007FFF46E67D6E  je          00007FFF46E67D75  
00007FFF46E67D70  call        00007FFFA65B0EB0  
00007FFF46E67D75  nop  
            _bytes += count;
00007FFF46E67D76  mov         rax,qword ptr [rbp+50h]  
00007FFF46E67D7A  mov         edx,dword ptr [rbp+58h]  
00007FFF46E67D7D  mov         edx,edx  
00007FFF46E67D7F  add         rdx,qword ptr [rax+48h]  
00007FFF46E67D83  jae         00007FFF46E67D8A  
00007FFF46E67D85  call        00007FFFA65AFBD0  
00007FFF46E67D8A  mov         rax,qword ptr [rbp+50h]  
00007FFF46E67D8E  mov         qword ptr [rax+48h],rdx  
        }
00007FFF46E67D92  nop  
00007FFF46E67D93  lea         rsp,[rbp+30h]  
00007FFF46E67D97  pop         rsi  
00007FFF46E67D98  pop         rdi  
00007FFF46E67D99  pop         rbp  
00007FFF46E67D9A  ret  

you can notice the operations neglect the sign. so if someone pass negative number to the method we'll end up with wrong result. if we compile with overflow flag off, JIT produce the following right code:

00007FFF46E877B0  push        rbp  
00007FFF46E877B1  push        rdi  
00007FFF46E877B2  push        rsi  
00007FFF46E877B3  sub         rsp,30h  
00007FFF46E877B7  mov         rbp,rsp  
00007FFF46E877BA  xor         eax,eax  
00007FFF46E877BC  mov         qword ptr [rbp+28h],rax  
00007FFF46E877C0  mov         qword ptr [rbp+50h],rcx  
00007FFF46E877C4  mov         dword ptr [rbp+58h],edx  
00007FFF46E877C7  cmp         dword ptr [7FFF46E1B728h],0  
00007FFF46E877CE  je          00007FFF46E877D5  
00007FFF46E877D0  call        00007FFFA65B0EB0  
00007FFF46E877D5  nop  
            _bytes += count;
00007FFF46E877D6  mov         eax,dword ptr [rbp+58h]  
00007FFF46E877D9  movsxd      rax,eax  
00007FFF46E877DC  mov         rdx,qword ptr [rbp+50h]  
00007FFF46E877E0  add         qword ptr [rdx+48h],rax  
        }
00007FFF46E877E4  nop  
00007FFF46E877E5  lea         rsp,[rbp+30h]  
00007FFF46E877E9  pop         rsi  
00007FFF46E877EA  pop         rdi  
00007FFF46E877EB  pop         rbp  
00007FFF46E877EC  ret  

to repro the issue, on corefx repo do the following:

• enable compiling with overflow flag by doing the change mentioned in the following line https://github.com/dennisdietrich/corefx/blob/private/Unchecked/dir.props#L169
• remove the uncheck from the line https://github.com/dotnet/corefx/blob/10733b7bb84d6132f4ac6fd83f3405f3ebd689a6/src/System.Text.Encoding.CodePages/src/System/Text/EncodingCharBuffer.cs#L91
• build the repo (from the root folder run build.cmd)
• go to the folder src\System.Text.Encoding.CodePages\tests then build and run using the command msbuild /t:rebuildandtest

you'll get one test failed because of the wrong code generated for the AdjustBytes method in the library System.Text.Encoding.CodePages

dotnet/corefx#16160

[tarekgh]

CC @russellhadley

[mikedn]

Is this really a JIT bug? Or is it a C# compiler bug and/or messed up CIL spec?

The IL code looks like:

IL_0001:  ldarg.0
IL_0002:  ldfld      uint8* Program/Bar::p
IL_0007:  ldarg.1
IL_0008:  add.ovf.un

On 64 bit this means that the addition operation has mismatched operand types - long and int. For better or worse the CIL spec allows this but it's not very clear how the smaller operand should be widened.

The description of add.ovf.un says

Add unsigned integer values with overflow check.

so it seems that the smaller operand should be zero extended and that's exactly what the JIT does.

But it can be argued that the .un part of add.ovf.un affects only the overflow check and that the JIT should always sign extend the int32 value that's on the stack. And what happens if you want to add an uint to a pointer? Well, you have to insert a conv.u and that's exactly what the C# compiler does.

So the C# compiler and the JIT compiler seem interpret the spec differently. Conclusion: the spec is flawed :)

[stephentoub]

cc: @jaredpar

[jaredpar]

CC @VSadov, @jcouv

[VSadov]

[mikedn]

@VSadov It's not a native int/native int case, it's an int32/native int case.

[VSadov]

@mikedn - yes, that is correct. checked( nint + int32 ) is a valid operation

[mikedn]

@VSadov Ah, you highlighted the results. But this is about the operands. checked(nint + int32) is a valid operation but it's not clear how that int32 is to be widened to nint. The JIT assumes it is unsigned and zero extends while the C# compiler assumes that it is signed and so it will be sign extended.

[tannergooding]

I agree, I think the spec is flawed here. Currently it indicates that all unmanaged pointers are unsigned native int and that int32 is zero extended to become unsigned native int.

However, when dealing with native int op int32, this means that addition and subtraction will be incorrect on 64-bit platforms.

What should be happening here is that int32 is coerced into native int of the same sign and then the operation should be executed (so int32 would be sign extended and unsigned int32 would be zero extended).

[VSadov]

implicit conversion from int32 to nint should always sign-extend.
CLI, itself, does not track unsigned types, it knows only int32, int64, nint, F, & and O

Obviously, int32 must become a nint before add.ovf.un or any kind of add can be performed.

un in ovf.un suffix does not specify the operand types of add, has nothing to do with operands really, it just specifies how overflow is determined in two's-complement math - CF vs. OF.

Basically we start with signed int32 and nint and the result of the operation is nint with a guarantee that the carry flag was not set while adding.
I do not see how 0-extending would be involved.

The spec is not vague about this, but is admittedly terse and relevant pieces are scattered around.

[mikedn]

The spec is not vague about this, but is admittedly terse and relevant pieces are scattered around.

Is there any text in the spec that is actually relevant in this case? Because the only obvious piece right now is the description of add.ovf.un and that one is interpretable.

Also, this isn't the only case where an int32 on the stack ends up being zero extended. See III.1.6 "Implicit argument coercion" for another case.

That said, it may be that sign extension makes more sense. Probably the only reason why such mixed type operations are valid is to avoid a bunch of casts in unsafe code. And since in .NET signed integer types are far more common than unsigned types it makes sense to sign extend by default and require a conv.u only when unsigned types are involved.

And considering that just a week ago I run into a real JIT bug related to add.ovf.un it may be that the JIT is wrong on this too. But it's far from obvious.

[tannergooding]

Basically we start with signed int32 and nint

@VSadov, it is true that we start with a signed int32, but we do not start with a signed native int. The spec explicitly states that unmanaged pointers are unsigned native int and the result of that is a unsigned native int (the stack does not itself track whether the type is signed/unsigned, so it is up to the IL emitted to indicate the appropriate types by using the appropriate instructions).

[BruceForstall]

@VSadov as @mikedn wrote, the spec for add.ovf.un says:

Add unsigned integer values with overflow check.

It seems pretty clear that this says the values being added should be treated as unsigned. So, when you write:

un in ovf.un suffix does not specify the operand types of add, has nothing to do with operands really, it just specifies how overflow is determined in two's-complement math - CF vs. OF.

I would disagree.

It looks like JIT64 and RyuJIT agree on this interpretation. What about earlier, pre-Roslyn C# compilers?

[tannergooding]

I think we can all agree that, regardless of what the spec says right now, the behavior here is wrong (with regards to how pointer arithmetic is meant to work).

That is, for pointer + int32, the int32 should be sign extended and then added to the pointer so that negative int32 results in a subtraction.