Fix incorrect tracking of sign bit (#59525)

* Fix incorrect tracking of sign bit * Perform the 1 check earlier * Add comment on checking special pattern * Simplify fix * Update the test to target 32 bit right shift as the logic has been restricted to that * Remove stray newline
dotnet · Sep 28, 2021 · 7a58bef · 7a58bef
1 parent 87a22d6
commit 7a58bef
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 7 deletions.
diff --git a/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs b/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
@@ -2021,10 +2021,14 @@ public static explicit operator decimal(BigInteger value)
                 }
 
                 NumericsHelpers.DangerousMakeTwosComplement(xd); // Mutates xd
-                if (xd[^1] == 0)
-                {
-                    trackSignBit = true;
-                }
+
+                // For a shift of N x 32 bit,
+                // We check for a special case where its sign bit could be outside the uint array after 2's complement conversion.
+                // For example given [0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF], its 2's complement is [0x01, 0x00, 0x00]
+                // After a 32 bit right shift, it becomes [0x00, 0x00] which is [0x00, 0x00] when converted back.
+                // The expected result is [0x00, 0x00, 0xFFFFFFFF] (2's complement) or [0x00, 0x00, 0x01] when converted back
+                // If the 2's component's last element is a 0, we will track the sign externally
+                trackSignBit = smallShift == 0 && xd[^1] == 0;
             }
 
             int zl = xd.Length - digitShift + (trackSignBit ? 1: 0);
@@ -2061,12 +2065,12 @@ stackalloc uint[StackallocUInt32Limit].Slice(0, zl) :
             }
             if (negx)
             {
-                NumericsHelpers.DangerousMakeTwosComplement(zd); // Mutates zd
-
+                // Set the tracked sign to the last element
                 if (trackSignBit)
                 {
-                    zd[^1] = 1;
+                    zd[^1] = 0xFFFFFFFF;
                 }
+                NumericsHelpers.DangerousMakeTwosComplement(zd); // Mutates zd
             }
 
             return new BigInteger(zd, zdArray, negx);

diff --git a/src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs b/src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
@@ -63,6 +63,14 @@ public static void RunRightShiftTests()
                 VerifyRightShiftString(Print(tempByteArray2) + Print(tempByteArray1) + "b>>");
             }
 
+            // RightShift Method - Uint 0xffffffff 0x8000000 ... Large BigIntegers - 32 bit Shift
+            for (int i = 0; i < s_samples; i++)
+            {
+                tempByteArray1 = GetRandomLengthFirstUIntMaxSecondUIntMSBMaxArray(s_random);
+                tempByteArray2 = new byte[] { (byte)32 };
+                VerifyRightShiftString(Print(tempByteArray2) + Print(tempByteArray1) + "b>>");
+            }
+
             // RightShift Method - Large BigIntegers - large - Shift
             for (int i = 0; i < s_samples; i++)
             {
@@ -229,6 +237,15 @@ private static byte[] GetRandomLengthAllOnesUIntByteArray(Random random)
             array[^1] = 0xFF;
             return array;
         }
+        private static byte[] GetRandomLengthFirstUIntMaxSecondUIntMSBMaxArray(Random random)
+        {
+            int gap = random.Next(0, 128);
+            int byteLength = 4 + gap * 4 + 1;
+            byte[] array = new byte[byteLength];
+            array[^6] = 0x80;
+            array[^1] = 0xFF;
+            return array;
+        }
 
         private static string Print(byte[] bytes)
         {