[s390x] Undefined behaviour in float32

[saitama951]

Recently we started seeing test case failures relating to System.Buffers.Binary.Tests.BinaryWriterUnitTests.SpanWriteSingle
Test case:

   public void SpanWriteSingle(float value)
   {
            var value = float.NegativeInfinity;
            WriteSingleLittleEndian(span, value);
            read = ReadSingleLittleEndian(span);
            Assert.Equal(value, read);
   }

the corresponding tests fails with

  Error Message:
Assert.Equal() Failure: Values differ
Expected: -∞
Actual:   ∞
Stack Trace:
  at System.Buffers.Binary.Tests.BinaryWriterUnitTests.SpanWriteSingle(Single value) in /home/sanjam/repro/runtime/src/libraries/System.Memory/tests/Binary/BinaryWriterUnitTests.cs:line 335
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in /home/sanjam/repro/runtime/src/mono/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.Mono.cs:line 22
at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr) in /home/sanjam/repro/runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs:line 174

I had extracted out the test case for my simplicity with various console printing

using System.Reflection;
using System;
using System.Runtime.InteropServices;
using System.Buffers.Binary;
using System.Runtime.CompilerServices;
class Attribute
{
        public static void Main(string [] args)
        {
                var span = new Span<byte>(new byte[4]);
                float value = float.NegativeInfinity;
                Console.WriteLine("float: "+value);
                BinaryPrimitives.WriteSingleLittleEndian(span, value);
                int temp=0;
                if (!BitConverter.IsLittleEndian){
                temp = BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(span));
                //Console.WriteLine("value of temp: "+temp);
                }
                unsafe{
                Console.WriteLine("Int32ToSingle  " + BitConverter.Int32BitsToSingle(temp));
                //Console.WriteLine("bitcast: "+Unsafe.BitCast<int,float>(temp)); //Unsafe.Bitcase -> ReadUnaligned+Unsafe.As
                //Console.WriteLine("As: "+Unsafe.As<int,byte>(ref temp));
                //Console.WriteLine("ReadUnaligned: "+Unsafe.ReadUnaligned<float> ( ref Unsafe.As<int,byte>(ref temp)));
                }
                /*Console.WriteLine("span: "+span[0]+""+span[1]+""+span[2]+""+span[3]);
                Console.WriteLine(read);*/

        }

}

the following program gives me the expected output

[root@sanjam findhash]# dotnet bin/Debug/net9.0/findhash.dll
float: -∞
Int32ToSingle  ∞

when Uncommenting the a simple Console.WriteLine from the if condition the output changes.

using System.Reflection;
using System;
using System.Runtime.InteropServices;
using System.Buffers.Binary;
using System.Runtime.CompilerServices;
class Attribute
{
        public static void Main(string [] args)
        {
                var span = new Span<byte>(new byte[4]);
                float value = float.NegativeInfinity;
                Console.WriteLine("float: "+value);
                BinaryPrimitives.WriteSingleLittleEndian(span, value);
                int temp=0;
                if (!BitConverter.IsLittleEndian){
                temp = BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(span));
                Console.WriteLine("value of temp: "+temp);  //UNCOMMENTED LINE
                }
                unsafe{
                Console.WriteLine("Int32ToSingle  " + BitConverter.Int32BitsToSingle(temp));
                //Console.WriteLine("bitcast: "+Unsafe.BitCast<int,float>(temp)); //Unsafe.Bitcase -> ReadUnaligned+Unsafe.As
                //Console.WriteLine("As: "+Unsafe.As<int,byte>(ref temp));
                //Console.WriteLine("ReadUnaligned: "+Unsafe.ReadUnaligned<float> ( ref Unsafe.As<int,byte>(ref temp)));
                }

        }

}

output:

[root@sanjam findhash]# dotnet bin/Debug/net9.0/findhash.dll
float: -∞
value of temp: -8388608
Int32ToSingle  -∞

cc: @giritrivedi @omajid @uweigand @iii-i

[dotnet-policy-service]

Tagging subscribers to this area: @lambdageek, @steveisok
See info in area-owners.md if you want to be subscribed.

[uweigand]

Hi @saitama951, this looks suspiciously like a codegen bug. Can you extract the IL and generated assembly for your small test case in both the good and the bad case?

Also, maybe you can identify a specific runtime commit (via bisect) that introduced this behavior?

[saitama951]

Hi @uweigand,
This starts to be failing from here #105150
I have attached the disassembled IL and mono dump (link) .Although the IL looks good.

here is a more clean reproducible of the behavior.

                var span = new Span<byte>(new byte[4]{0,0,128,191});
                int temp = BinaryPrimitives.ReverseEndianness(MemoryMarshal.Read<int>(span));
//              Console.WriteLine("temp:"+temp);
                Console.WriteLine("Int32BitsToSingle: "+ BitConverter.Int32BitsToSingle(temp));

Now we have the uncommented case (here the output is -1 ) and the commented case (here the output is 1)
The expected value is -1 always.
In BitConverter.Int32BitsToSingle we have

case 1: Commented

 0000000000000000 <tem_BitConverter_Int32BitsToSingle__int_>:
   0:   eb 6f f0 30 00 24       stmg    %r6,%r15,48(%r15)
   6:   b9 04 00 bf             lgr     %r11,%r15
   a:   a7 fb ff 58             aghi    %r15,-168
   e:   e3 b0 f0 00 00 24       stg     %r11,0(%r15)
  14:   50 20 f0 a4             st      %r2,164(%r15)
  18:   eb 02 00 20 00 0b       slag    %r0,%r2,32  // here r2 = 0xbf800000 and we get r0 = 0x3f80000000000000. cc=3
  1e:   b3 c1 00 10             ldgr    %f1,%r0
  22:   28 01                   ldr     %f0,%f1
  24:   28 00                   ldr     %f0,%f0
  26:   a7 fb 00 a8             aghi    %r15,168
  2a:   eb 6e f0 30 00 04       lmg     %r6,%r14,48(%r15)
  30:   07 fe                   br      %r14
  32:   00 0e 00 00             .long   0x000e0000
  36:   00 4a                   .short  0x004a

here only the sign bit is shifted away for some reason resulting the cc to 3, I don't exactly understand why? (probably anything that is overflowed into the sign bit is undefined?)

case 2: Uncommented

 0000000000000000 <tem_BitConverter_Int32BitsToSingle__int_>:
  0:   eb 6f f0 30 00 24       stmg    %r6,%r15,48(%r15)
  6:   b9 04 00 bf             lgr     %r11,%r15
  a:   a7 fb ff 58             aghi    %r15,-168
  e:   e3 b0 f0 00 00 24       stg     %r11,0(%r15)
 14:   50 20 f0 a4             st      %r2,164(%r15)
 18:   eb 02 00 20 00 0b       slag    %r0,%r2,32  // here r2 = 0xffffffffbf800000 and we get r0 = 0xbf80000000000000.
 1e:   b3 c1 00 10             ldgr    %f1,%r0
 22:   28 01                   ldr     %f0,%f1
 24:   28 00                   ldr     %f0,%f0
 26:   a7 fb 00 a8             aghi    %r15,168
 2a:   eb 6e f0 30 00 04       lmg     %r6,%r14,48(%r15)
 30:   07 fe                   br      %r14
 32:   00 0e 00 00             .long   0x000e0000
 36:   00 4a                   .short  0x004a

now the r2 values differ which are sent from the Main Method

case 1: Commented

 10a:   c4 e8 00 00 00 4f       lgrl    %r14,1a8 <ribute_Main__string___+0x1a8> //r2=0xbf800000 as return register from ReverseEndianness function
110:   0d ee                   basr    %r14,%r14
112:   c4 e8 00 00 00 47       lgrl    %r14,1a0 <ribute_Main__string___+0x1a0>   //r2 passed as argument 0xbf800000 into BitConverter.Int32BitsToSingle
118:   0d ee                   basr    %r14,%r14
11a:   01 ff                   trap2 //for debugging purpose
11c:   28 10                   ldr     %f1,%f0

case 2: Uncommented

 116:   c4 e8 00 00 00 79       lgrl    %r14,208 <ribute_Main__string___+0x208>
11c:   0d ee                   basr    %r14,%r14
11e:   e3 20 f0 e8 00 50       sty     %r2,232(%r15) //r2 is preserved after returning from ReverseEndianness here r2 = 0xbf800000 
                              .
                              .
                   assembly from Console.WriteLine(temp)
                              .
                              .
152:   e3 20 f0 e8 00 14       lgf     %r2,232(%r15)                      retrieve the preserved r2 and r2 = 0xffffffffbf800000
158:   c4 e8 00 00 00 48       lgrl    %r14,1e8 <ribute_Main__string___+0x1e8> //r2 passed as argument 0xbf800000 into BitConverter.Int32BitsToSingle
15e:   0d ee                   basr    %r14,%r14
160:   01 ff                   trap2                                           //for debugging.
162:   28 10                   ldr     %f1,%f0

here what I don't understand is why do we do a 32 bit store when we do a 64 bit load into r2 in RevereseEndianness?
and when doing a 64 bit load from the stack we get a 0xffffffffbf800000 (is this extended operation expected)?
reference ReverseEndianness:-

Disassembly of section .text:

0000000000000000 <tem_Buffers_Binary_BinaryPrimitives_ReverseEndianness__int_>:
  0:   eb 6f f0 30 00 24       stmg    %r6,%r15,48(%r15)
  6:   b9 04 00 bf             lgr     %r11,%r15
  a:   a7 fb ff 58             aghi    %r15,-168
  e:   e3 b0 f0 00 00 24       stg     %r11,0(%r15)
 14:   50 20 f0 a4             st      %r2,164(%r15)
 18:   c0 28 00 00 03 ff       iihf    %r2,1023
 1e:   c0 29 8c bf b0 38       iilf    %r2,2361372728
 24:   e3 00 20 00 00 02       ltg     %r0,0(%r2)
 2a:   a7 84 00 06             je      36 <tem_Buffers_Binary_BinaryPrimitives_ReverseEndianness__int_+0x36>
 2e:   c4 e8 00 00 00 15       lgrl    %r14,58 <tem_Buffers_Binary_BinaryPrimitives_ReverseEndianness__int_+0x58>
 34:   0d ee                   basr    %r14,%r14
 36:   e3 20 f0 a4 00 14       lgf     %r2,164(%r15) //returning 64 bit value into r2
 3c:   c4 e8 00 00 00 0a       lgrl    %r14,50 <tem_Buffers_Binary_BinaryPrimitives_ReverseEndianness__int_+0x50>
 42:   0d ee                   basr    %r14,%r14
 44:   a7 fb 00 a8             aghi    %r15,168
 48:   eb 6e f0 30 00 04       lmg     %r6,%r14,48(%r15)
 4e:   07 fe                   br      %r14
***

I think replacing the slag instruction sllg would be a good idea? since we are dealing with byte and bit patterns and BitConverter reinterprets the integer bit pattern as a floating point value, while testing it out this change gives me consistent results.

[uweigand]

Hi @saitama951, there's two issues I can see here.

First of all, I agree that use of slag is quite weird here - for OP_MOVE_I4_TO_F we definitely should use sllg instead. (In fact, slag is nearly never useful on Linux, no other Linux compiler ever generates it ...)

While this would already fix the current issue, there's still another concern. According to the s390x ELF ABI, signed 32-bit integers must always passed as sign-extended 64-bit values. This means that r2 should in fact contain 0xffffffffbf800000 when calling a function. Note that the same applies to return values, so the int variant of ReverseEndianness should already have returned 0xffffffffbf800000 as well. We should also check why this doesn't happen - this could cause other problems elsewhere.

[saitama951]

@uweigand Thank you for clearing my doubts, I will continue to debug this.