Skip to content

Fix BigInteger.Rotate{Left,Right} #112864

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix BigInteger.Rotate{Left,Right} #112864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7245cf3
Add BigInteger.Rotate* tests
kzrnm Feb 24, 2025
ff68a43
Fix BigInteger.Rotate*
kzrnm Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Runtime.Numerics/src/System.Runtime.Numerics.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<Compile Include="System\Numerics\BigIntegerCalculator.GcdInv.cs" />
<Compile Include="System\Numerics\BigIntegerCalculator.PowMod.cs" />
<Compile Include="System\Numerics\BigIntegerCalculator.SquMul.cs" />
<Compile Include="System\Numerics\BigIntegerCalculator.ShiftRot.cs" />
<Compile Include="System\Numerics\BigIntegerCalculator.Utils.cs" />
<Compile Include="System\Numerics\BigInteger.cs" />
<Compile Include="System\Number.BigInteger.cs" />
Expand Down
271 changes: 46 additions & 225 deletions src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ private static BigInteger Add(ReadOnlySpan<uint> leftBits, int leftSign, ReadOnl
}

if (bitsFromPool != null)
ArrayPool<uint>.Shared.Return(bitsFromPool);
ArrayPool<uint>.Shared.Return(bitsFromPool);

return result;
}
Expand Down Expand Up @@ -2636,7 +2636,7 @@ public static implicit operator BigInteger(nuint value)

if (zdFromPool != null)
ArrayPool<uint>.Shared.Return(zdFromPool);
exit:
exit:
if (xdFromPool != null)
ArrayPool<uint>.Shared.Return(xdFromPool);

Expand Down Expand Up @@ -3227,7 +3227,6 @@ public static BigInteger PopCount(BigInteger value)

part = ~value._bits[i];
result += uint.PopCount(part);

i++;
}
}
Expand All @@ -3239,267 +3238,89 @@ public static BigInteger PopCount(BigInteger value)
public static BigInteger RotateLeft(BigInteger value, int rotateAmount)
{
value.AssertValid();
int byteCount = (value._bits is null) ? sizeof(int) : (value._bits.Length * 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this initially be done as a smaller fix that doesn't rewrite the whole algorithm?

We likely need to backport this fix and a less complex change is more desirable for that, as it reduces risk.

A separate PR that also rewrites it for improved performance just for .NET 10 is fine, just ideally separate from the bug fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#112878

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be backported.

  1. What is the correct definition on RotateLeft/RotateRight of BigInteger? BigInteger doesn't have a fixed bits length and doesn't use 2's complement code to store data. I'd rather document the behavior as undefined.
  2. The issue is said to have existed since .NET 7. Who using an old version .NET would want this new behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is functionally "by design".
There were two options possible, one of which always treated the bit sequence as the shortest possible two's complement representation and thus -1 is 1 while +1 is 01. The other of which is to treat it in terms of the underlying storage unit. The latter was chosen as it was overall more consistent with existing behavior, especially when viewed across all possible types.

Originally posted by @tannergooding in #91169

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skyoxZ, the backport isn't about changing the behavior; it's about fixing an edge case where the behavior is broken because RotateRight is effectively propagating the carry bit left instead of right in certain edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user I can't predict the result of BigInteger.RotateLeft so I don't care if it's a bug. I just expect the same input always leads to the same result and believe .NET team is unlikely to change it. A backport will break my expectation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The possible approaches for handling bitwise operations are as follows:

  1. Do not implement this operations. Throw an exception if called via an interface.
  2. Provide an explicit implementation based on a specific policy.
  3. Implement it as a public method based on a specific policy (chosen).
  4. Return an arbitrary, meaningless value (which is effectively the case for RotateRight).

Personally, I believe option 1 is the most desirable in an ideal scenario. An example of this approach is Java's BigInteger, which does not implement unsigned right shifts, stating that they "make little sense". @skyoxZ's argument seems to be based on the same idea.

However, changing to option 1 would be far too breaking and likely unfeasible.

@skyoxZ's stance (for the current version) aligns with option 4, advocating that breaking changes should be avoided. Breaking changes guideline - Bucket 2: Reasonable grey area
No one knows the specification, no one uses the method, and therefore no one would be affected even if its implementation has a bug.

On the other hand, even if no one knows the specification, it is reasonable to expect a public method to have a sensible implementation.

I will leave the final decision to .NET team.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I believe option 1 is the most desirable in an ideal scenario

1 isn't done because it breaks user expectations of how operating with binary integers in generic contexts works. It also prevents doing common and sensible optimizations, like treating division by 2 as shift operation or being able to permute the underlying bits.

The behavior chosen for 3 is then "sensible" for singular rotations and it behaves in a well defined way. Which is that functionally ROL(x, amount) will produce the same result as would happen for a standard integer x stored with a precision of n-bits. The biggest nuance is that ROR(ROL(x, amount), amount) may not return x since the stored precision might change and there's no way to track how many implied leading zero/sign bits were originally there.

The behavior is specifically that we determine the number of bits required for the "shortest" two's complement representation rounded up to the nearest 32-bits. We then rotate the number of bits specified. This produces a new BigInteger value and is therefore logically similar to had you manually done the two bit shifts as a single combined operation without increasing the backing storage. Because we produce a new BigInteger, that is then trimmed for storage and computation efficiency to remove unnecessary leading zero/sign bits, which is what can lead to not being able to recover the original value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because its meant to behave in a well-defined way, you can fully predict the answer for a given integer x. Because there is then a bug, it is applicable to apply a bug fix for the broken behavior especially as it is more likely to cause problems that it is returning an incorrect result.


// Normalize the rotate amount to drop full rotations
rotateAmount = (int)(rotateAmount % (byteCount * 8L));

if (rotateAmount == 0)
return value;

if (rotateAmount == int.MinValue)
return RotateRight(RotateRight(value, int.MaxValue), 1);

if (rotateAmount < 0)
return RotateRight(value, -rotateAmount);

(int digitShift, int smallShift) = Math.DivRem(rotateAmount, kcbitUint);

uint[]? xdFromPool = null;
int xl = value._bits?.Length ?? 1;

Span<uint> xd = (xl <= BigIntegerCalculator.StackAllocThreshold)
? stackalloc uint[BigIntegerCalculator.StackAllocThreshold]
: xdFromPool = ArrayPool<uint>.Shared.Rent(xl);
xd = xd.Slice(0, xl);

bool negx = value.GetPartsForBitManipulation(xd);

int zl = xl;
uint[]? zdFromPool = null;

Span<uint> zd = (zl <= BigIntegerCalculator.StackAllocThreshold)
? stackalloc uint[BigIntegerCalculator.StackAllocThreshold]
: zdFromPool = ArrayPool<uint>.Shared.Rent(zl);
zd = zd.Slice(0, zl);

zd.Clear();

if (negx)
{
NumericsHelpers.DangerousMakeTwosComplement(xd);
}

if (smallShift == 0)
{
int dstIndex = 0;
int srcIndex = xd.Length - digitShift;

do
{
// Copy last digitShift elements from xd to the start of zd
zd[dstIndex] = xd[srcIndex];

dstIndex++;
srcIndex++;
}
while (srcIndex < xd.Length);

srcIndex = 0;

while (dstIndex < zd.Length)
{
// Copy remaining elements from start of xd to end of zd
zd[dstIndex] = xd[srcIndex];
bool neg = value._sign < 0;

dstIndex++;
srcIndex++;
}
}
else
{
int carryShift = kcbitUint - smallShift;

int dstIndex = 0;
int srcIndex = 0;

uint carry = 0;

if (digitShift == 0)
{
carry = xd[^1] >> carryShift;
}
else
{
srcIndex = xd.Length - digitShift;
carry = xd[srcIndex - 1] >> carryShift;
}

do
{
uint part = xd[srcIndex];

zd[dstIndex] = (part << smallShift) | carry;
carry = part >> carryShift;

dstIndex++;
srcIndex++;
}
while (srcIndex < xd.Length);

srcIndex = 0;

while (dstIndex < zd.Length)
{
uint part = xd[srcIndex];

zd[dstIndex] = (part << smallShift) | carry;
carry = part >> carryShift;

dstIndex++;
srcIndex++;
}
}

if (negx && (int)zd[^1] < 0)
{
NumericsHelpers.DangerousMakeTwosComplement(zd);
}
else
if (value._bits is null)
{
negx = false;
uint rs = BitOperations.RotateLeft((uint)value._sign, rotateAmount);
return neg
? new BigInteger((int)rs)
: new BigInteger(rs);
}

var result = new BigInteger(zd, negx);

if (xdFromPool != null)
ArrayPool<uint>.Shared.Return(xdFromPool);
if (zdFromPool != null)
ArrayPool<uint>.Shared.Return(zdFromPool);

return result;
return Rotate(value._bits, neg, rotateAmount);
}

/// <inheritdoc cref="IBinaryInteger{TSelf}.RotateRight(TSelf, int)" />
public static BigInteger RotateRight(BigInteger value, int rotateAmount)
{
value.AssertValid();
int byteCount = (value._bits is null) ? sizeof(int) : (value._bits.Length * 4);

// Normalize the rotate amount to drop full rotations
rotateAmount = (int)(rotateAmount % (byteCount * 8L));

if (rotateAmount == 0)
return value;

if (rotateAmount == int.MinValue)
return RotateLeft(RotateLeft(value, int.MaxValue), 1);

if (rotateAmount < 0)
return RotateLeft(value, -rotateAmount);

(int digitShift, int smallShift) = Math.DivRem(rotateAmount, kcbitUint);
bool neg = value._sign < 0;

uint[]? xdFromPool = null;
int xl = value._bits?.Length ?? 1;

Span<uint> xd = (xl <= BigIntegerCalculator.StackAllocThreshold)
? stackalloc uint[BigIntegerCalculator.StackAllocThreshold]
: xdFromPool = ArrayPool<uint>.Shared.Rent(xl);
xd = xd.Slice(0, xl);

bool negx = value.GetPartsForBitManipulation(xd);

int zl = xl;
uint[]? zdFromPool = null;

Span<uint> zd = (zl <= BigIntegerCalculator.StackAllocThreshold)
? stackalloc uint[BigIntegerCalculator.StackAllocThreshold]
: zdFromPool = ArrayPool<uint>.Shared.Rent(zl);
zd = zd.Slice(0, zl);

zd.Clear();

if (negx)
if (value._bits is null)
{
NumericsHelpers.DangerousMakeTwosComplement(xd);
uint rs = BitOperations.RotateRight((uint)value._sign, rotateAmount);
return neg
? new BigInteger((int)rs)
: new BigInteger(rs);
}

if (smallShift == 0)
{
int dstIndex = 0;
int srcIndex = digitShift;

do
{
// Copy first digitShift elements from xd to the end of zd
zd[dstIndex] = xd[srcIndex];

dstIndex++;
srcIndex++;
}
while (srcIndex < xd.Length);

srcIndex = 0;

while (dstIndex < zd.Length)
{
// Copy remaining elements from end of xd to start of zd
zd[dstIndex] = xd[srcIndex];

dstIndex++;
srcIndex++;
}
}
else
{
int carryShift = kcbitUint - smallShift;

int dstIndex = 0;
int srcIndex = digitShift;

uint carry = 0;

if (digitShift == 0)
{
carry = xd[^1] << carryShift;
}
else
{
carry = xd[srcIndex - 1] << carryShift;
}
return Rotate(value._bits, neg, -(long)rotateAmount);
}

do
{
uint part = xd[srcIndex];
private static BigInteger Rotate(ReadOnlySpan<uint> bits, bool negative, long rotateLeftAmount)
{
Debug.Assert(bits.Length > 0);
Debug.Assert(Math.Abs(rotateLeftAmount) <= 0x80000000);

zd[dstIndex] = (part >> smallShift) | carry;
carry = part << carryShift;
int zLength = bits.Length;
int leadingZeroCount = negative ? bits.IndexOfAnyExcept(0u) : 0;

dstIndex++;
srcIndex++;
}
while (srcIndex < xd.Length);
if (negative && bits[^1] >= kuMaskHighBit
&& !(leadingZeroCount == bits.Length - 1 && bits[^1] == kuMaskHighBit))
++zLength;

srcIndex = 0;
uint[]? zFromPool = null;
Span<uint> zd = ((uint)zLength <= BigIntegerCalculator.StackAllocThreshold
? stackalloc uint[BigIntegerCalculator.StackAllocThreshold]
: zFromPool = ArrayPool<uint>.Shared.Rent(zLength)).Slice(0, zLength);

while (dstIndex < zd.Length)
{
uint part = xd[srcIndex];
zd[^1] = 0;
bits.CopyTo(zd);

zd[dstIndex] = (part >> smallShift) | carry;
carry = part << carryShift;
if (negative)
{
Debug.Assert((uint)leadingZeroCount < (uint)zd.Length);

dstIndex++;
srcIndex++;
}
// Same as NumericsHelpers.DangerousMakeTwosComplement(zd);
// Leading zero count is already calculated.
zd[leadingZeroCount] = (uint)(-(int)zd[leadingZeroCount]);
NumericsHelpers.DangerousMakeOnesComplement(zd.Slice(leadingZeroCount + 1));
}

if (negx && (int)zd[^1] < 0)
BigIntegerCalculator.RotateLeft(zd, rotateLeftAmount);

if (negative && (int)zd[^1] < 0)
{
NumericsHelpers.DangerousMakeTwosComplement(zd);
}
else
{
negx = false;
negative = false;
}

var result = new BigInteger(zd, negx);
var result = new BigInteger(zd, negative);

if (xdFromPool != null)
ArrayPool<uint>.Shared.Return(xdFromPool);
if (zdFromPool != null)
ArrayPool<uint>.Shared.Return(zdFromPool);
if (zFromPool != null)
ArrayPool<uint>.Shared.Return(zFromPool);

return result;
}
Expand Down
Loading