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

Converting a `float/double` to `byte/sbyte` or `short/ushort` does not correctly account for overflow #461

Closed
tannergooding opened this issue Dec 3, 2019 · 6 comments
Labels

Comments

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Dec 3, 2019

Issue

Take the following program:

using System;

class Program
{
  static void Main()
  {
    double aslocal;

    aslocal = 65535.17567;
    Console.WriteLine(unchecked((short)aslocal));

    aslocal = 65536.17567;
    Console.WriteLine(unchecked((short)aslocal));
  }
}

Both Intel and AMD produce the same result (whether using the x87 FPU or SSE+):

-1
0

Both inputs generate the same assembly.

SSE+:

00007FFDC205093F  vmovsd      xmm0,qword ptr [address]
00007FFDC2050948  vmovsd      qword ptr [stack_addr],xmm0  
00007FFDC205094E  vcvttsd2si  ecx,mmword ptr [stack_addr]  
00007FFDC2050954  movsx       rcx,cx  
00007FFDC2050958  call        00007FFE17952980  
00007FFDC205095D  nop  

x87 FPU:

00DF08C7 DD 05 10 09 DF 00    fld         qword ptr ds:[addr]  
00DF08CD DD 5D C0             fstp        qword ptr [stack_addr]  
00DF08D0 F2 0F 10 45 C0       movsd       xmm0,mmword ptr [stack_addr]  
00DF08D5 F2 0F 2C C8          cvttsd2si   ecx,xmm0  
00DF08D9 0F BF C9             movsx       ecx,cx  
00DF08DC E8 37 11 1C 73       call        73FB1A18  
00DF08E1 90                   nop 

The SSE+ instructions that support converting (cvtsd2si, cvtss2si, cvttsd2si, and cvttss2si) all deal with only 32-bit or 64-bit results.

The x87 FPU instructions that support converting (fist and fistp) supports 16-bit, 32-bit, or 64-bit results. However, it looks like even under the legacy JIT, it still emits cvtts*2si rather than fist/fistp

Given that the instruction emitted only supports 32-bit/64-bit results, the results are 65535 (0xFFFF) and 65536 (0x10000). These, when cast to short and then sign-extended back to int are -1 and 0, respectively.

Proposed Fix

The .NET Runtime should correctly account for overflow and return a "correct" value in this scenario. On x86, the "indefinite integer value" is defined to be:

(2^(w-1), where w represents the number of bits in the destination format)

@tannergooding

This comment has been minimized.

Copy link
Member Author

@tannergooding tannergooding commented Dec 3, 2019

See dotnet/roslyn#7333 (comment) and surrounding discussion for more details.

@tannergooding

This comment has been minimized.

Copy link
Member Author

@tannergooding tannergooding commented Dec 3, 2019

CC. @gafter

@tannergooding

This comment has been minimized.

Copy link
Member Author

@tannergooding tannergooding commented Dec 3, 2019

It looks like ARM might have similar problems given the conversion instructions I see are available.

@tannergooding

This comment has been minimized.

Copy link
Member Author

@tannergooding tannergooding commented Dec 3, 2019

More noticeable errors can be seen with values such as 65538.0 which returns 2, etc.

C# has started using 0 as the result for cases of constant folding to ensure cross-platform/architecture determinism.

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Dec 3, 2019

This behavior is unspecified per the ECMA-335 spec.

If overflow occurs converting a floating-point type to an integer, or if the floating-point value
being converted to an integer is a NaN, the value returned is unspecified.

@tannergooding

This comment has been minimized.

Copy link
Member Author

@tannergooding tannergooding commented Dec 3, 2019

Hmmm, that's unfortunate.

Perhaps this is something we could normalize if we ever provided a "strict"/"determinstic" vs "fast" mode for floating-point operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.