Fix incorrect tracking of sign bit #59525
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsFixes #59509 I made an invalid assumption that it was impossible for the last uint to be 0 after the TwosComplement unless the uint was all max. Now the logic explicitly checks that the inversion is all 0 with the first uint being 1
|
| @@ -2021,9 +2021,22 @@ private static BigInteger Subtract(uint[]? leftBits, int leftSign, uint[]? right | |||
| } | |||
|
|
|||
| NumericsHelpers.DangerousMakeTwosComplement(xd); // Mutates xd | |||
| if (xd[^1] == 0) | |||
|
|
|||
| if (smallShift == 0 && xd[0] == 1) | |||
There was a problem hiding this comment.
It might just be that I'm not awake enough, but why aren't we simply doing trackSignBit = (_sign & int.MinValue) != 0?
That is, if the most significant bit of _sign is set, we are a signed value and therefore this is an arithmetic right shift and the sign bit needs to be propagated. Otherwise, its a logical (or effectively logical) right shift and it doesn't.
There was a problem hiding this comment.
We only need to track if it's the special sequence that #54115 was meant to fix
Cuz effectively for that sequence the sign bit is lost after the twoscomplement
There was a problem hiding this comment.
An alternative solution is we can expand the array by 1 element before performing 2's complement and normalize the uint array at the end
There was a problem hiding this comment.
We only need to track if it's the special sequence that #54115 was meant to fix
Cuz effectively for that sequence the sign bit is lost after the twoscomplement
Ah, right. We don't store it directly as a negative value, we store the sign + magnitude, which makes this logic more complicated.
There was a problem hiding this comment.
Can you add a code comment describing why this logic is needed for future readers. Otherwise this LGTM.
There was a problem hiding this comment.
Oh, i discovered a simpler fix. I should have set the last uint as 0xFFFFFFFF before the 2's complement
as the logic has been restricted to that
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks correct to me.
I'm looking at what can be done to simplify this logic overall so we can avoid some of these issues in the future. I don't think having BigInteger store things as sign + magnitude is intuitive and it doesn't work well with the "natural" way to work with these types (nor many of the perf oriented algorithms for fast multiplication/division, etc).
|
CC. @pgovind could you give this a secondary review. |
| trackSignBit = true; | ||
| } | ||
|
|
||
| // For a shift of N x 32 bit, |
|
Any chance this fix will make it to the .NET 6 release? |
|
CC. @jeffhandley, this is technically fixing a regression, can we get a bar check for it? The original fix was for .NET 6 but introduced a different edge case that this now resolves. |
|
A regression introduced in the release that was reported and fixed by external contributors passes my bar and I expect would get tactics approval for .NET 6 GA. Please go ahead and initial the port and fill out the template, @tannergooding. |
|
/backport release/6.0 |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1304487290 |
ghost
locked as resolved and limited conversation to collaborators
Fixes #59509
The detection was sufficient but the sign tracking should have be done within the 2's complement space.
The fix is now to set the last uint element to be
0xFFFFFFFFbefore converting the value back.Also added an additional check to make sure this is only done for N x 32 bit right shifts