-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[release/8.0] Fix BigInteger.Rotate{Left,Right}
for backport
#112992
base: release/8.0-staging
Are you sure you want to change the base?
[release/8.0] Fix BigInteger.Rotate{Left,Right}
for backport
#112992
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics |
76d6406
to
4756852
Compare
* Add tests for the shift operator of BigInteger * Fix the unsigned right shift operator of BigInteger * avoid stackalloc * external sign element
4756852
to
660fa1c
Compare
CC. @jeffhandley, @artl93 |
@tannergooding @jeffhandley Today is code complete for the April Release. If you want this fix included in this release, please merge it before 4pm PT. |
We are going to hold this one for a month, merging the 9.0 backport first and then giving it a month of bake-time before merging the 8.0 backport. |
Backport of #112878 and #112879 to release/8.0
/cc @tannergooding @kzrnm
Customer Impact
This was customer reported via #112564. The customer contributed the fix and validated it for their scenarios.
Shifting and bit-rotation have defined behaviors for fixed sized integers and while the behaviors are harder to map to the concept of a
BigInteger
which can have an arbitrary number of bits, there is still an expected behavior that maps to those semantics. In particular it's expected to behave similarly to as if theBigInteger
was represented as the shortest two's complement representation rounded up to the nearest multiple of 32-bits.Customers were getting incorrect results for certain inputs due to the internal representation of
BigInteger
causing the sign bit to be lost after the value was converted to its two's complement representation.Regression
The addition of
unsigned right shift
andbitwise rotatation
APIs is recent to .NET and was done as part of the push to supportGeneric Math
.Testing
Some comprehensive tests were added that test the various edge case bit patterns that could be prone to triggering such issues.
Risk
Medium
The APIs impacted in BigInteger are relatively new (.NET 7) and so the risk of negatively impacting existing customers is relatively low. The largest risk effectively being that some customer is accidentally depending on the broken behavior without realizing it.
However, these breaks are also additional examples of some of the problems caused by the current internal representation of
BigInteger
(which has been that way since it was introduced in 2010). The current representation is effectively that it is "ones complement + sign" and while that can make some basic arithmetic operations (such as addition, subtraction, multiplication, and division) simpler, it makes many other operations more complex or slower than necessary (particularly where they involve considering the bits of the two's complement representation). This in turn means that the confidence bar in such changes tends to be lower and needs more comprehensive testing (as was added in this fix).The required confidence bar that the fix is correct has been met here, but the representation and additional testing nuance for edge cases make all such changes riskier, hence the categorization as
medium
.