Fix incorrect results of right shifting huge BigIntegers error reported in Issue #65633 #67867
Conversation
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
area-System.Numerics
and removed
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis bug was caused by a multiplication overflow leading to an incorrect early exit from the right shift operation (returning MinusOne). The immediate solution is to cast one of the operands from Notably, this is just a short-term fix for this specific bug. As discussed with @tannergooding, a larger overhaul is planned for this class. For example, usage of this class should be validated based on the size constraints of the internal implementation of BigInteger (the current implementation does not truly support arbitrary sizes, due to storing the BigInteger as a uint array, which has a maximum length). Also included is a test case reproducing the bug, which now passes thanks to the change.
|
|
|
There was a problem hiding this comment.
Looks OK to me, but I'd like to get @tannergooding's review since he's familiar with this implementation.
| // Right shift the BigInteger and ensure the operation behaves as expected | ||
| BigInteger shiftedBigInt = bigInt >> 1; | ||
| Assert.Equal(2147483646, shiftedBigInt.GetBitLength()); | ||
| Assert.True(shiftedBigInt != new BigInteger(-1)); |
There was a problem hiding this comment.
Should we verify the new value? How do we know it's not all zero..
There was a problem hiding this comment.
Would be great to validate the result is equal as well here (might be worth a quick check that any other tests are also validating result and not just "shape"). Otherwise LGTM.
There was a problem hiding this comment.
Good suggestion, taking a look at this now.
…ivate BigInteger members to tests
|
@tannergooding, @danmoseley, @jeffhandley, I added a commit to expand on the LargeNegativeBigIntegerShiftTest. If there are any style changes you recommend, let me know. |
src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
Outdated
Show resolved
Hide resolved
|
Looks like there is a x86 test failure as well |
src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
src/libraries/System.Runtime.Numerics/tests/BigInteger/op_rightshift.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Moseley <danmose@microsoft.com>
…ed in Issue dotnet#65633 (dotnet#67867) * Fix dotnet#65633, multiplication overflow causing incorrect right shift results * Add unit test reproducing incorrect right shift results for BigInteger (dotnet#65633) * Fix inconsistent formatting * Expand LargeNegativeBigIntegerShiftTest to validate values, expose private BigInteger members to tests * Apply suggestions from code review Co-authored-by: Tanner Gooding <tagoo@outlook.com> * Restrict unit test to 64-bit processors * Document reasoning for restricting the test to 64-bit Co-authored-by: Dan Moseley <danmose@microsoft.com> Co-authored-by: Tanner Gooding <tagoo@outlook.com> Co-authored-by: Dan Moseley <danmose@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Fixes #65633
This bug was caused by a multiplication overflow leading to an incorrect early exit from the right shift operation (returning MinusOne). The immediate solution is to cast one of the operands from
inttolongbefore multiplying, as is done elsewhere in this file.Notably, this is just a short-term fix for this specific bug. As discussed with @tannergooding, a larger overhaul is planned for this class. For example, usage of this class should be validated based on the size constraints of the internal implementation of BigInteger (the current implementation does not truly support arbitrary sizes, due to storing the BigInteger as a uint array, which has a maximum length).
Also included is a test case reproducing the bug, which now passes thanks to the change.