Skip to content
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

Incorrect results of right shifting huge BigIntegers #65633

Closed
Tracked by #47244
gaazkam opened this issue Feb 20, 2022 · 5 comments · Fixed by #67867
Closed
Tracked by #47244

Incorrect results of right shifting huge BigIntegers #65633

gaazkam opened this issue Feb 20, 2022 · 5 comments · Fixed by #67867
Assignees
Milestone

Comments

@gaazkam
Copy link

gaazkam commented Feb 20, 2022

Description

Enormous negative BigIntegers produce -1 when right shifted by any value.

Reproduction Steps

using System.Numerics;

BigInteger bigInt = new BigInteger(-1) << int.MaxValue;
Console.WriteLine(bigInt.GetBitLength());
BigInteger shiftedBigInt = bigInt >> 1;
Console.WriteLine(shiftedBigInt.GetBitLength());
Console.WriteLine(shiftedBigInt == new BigInteger(-1));

Expected behavior

The program should output:

2147483647
2147483646
False

Actual behavior

The program outputs:

2147483647
0
True

Regression?

No response

Known Workarounds

No response

Configuration

Which version of .NET is the code running on?

<TargetFramework>net6.0</TargetFramework>

What OS and version, and what distro if applicable?

Windows 11 Pro Version 21H2

What is the architecture (x64, x86, ARM, ARM64)?

x64

Other information

  • Positive BigIntegers seem to not suffer from this issue, even if they are as enormous: (new BigInteger(1) << int.MaxValue) >> 1 seems to give expected results.
  • Slightly less enormous negative BigIntegers also seem to not suffer from this issue: (new BigInteger(-1) << (int.MaxValue - 1000)) >> 1 seems to give expected results. (new BigInteger(-1) << (int.MaxValue - 10)) >> 1, however, still returns -1.
  • How much we right shift the enormous big integer does not seem to affect the result. (new BigInteger(-1) << int.MaxValue) >> (int.MaxValue - 4) still gives -1.
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Feb 20, 2022
@ghost
Copy link

ghost commented Feb 20, 2022

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Enormous negative BigIntegers produce -1 when right shifted by any value.

Reproduction Steps

using System.Numerics;

BigInteger bigInt = new BigInteger(-1) << int.MaxValue;
Console.WriteLine(bigInt.GetBitLength());
BigInteger shiftedBigInt = bigInt >> 1;
Console.WriteLine(shiftedBigInt.GetBitLength());
Console.WriteLine(shiftedBigInt == new BigInteger(-1));

Expected behavior

The program should output:

2147483647
2147483646
False

Actual behavior

The program outputs:

2147483647
0
True

Regression?

No response

Known Workarounds

No response

Configuration

Which version of .NET is the code running on?

<TargetFramework>net6.0</TargetFramework>

What OS and version, and what distro if applicable?

Windows 11 Pro Version 21H2

What is the architecture (x64, x86, ARM, ARM64)?

x64

Other information

  • Positive BigIntegers seem to not suffer from this issue, even if they are as enormous: (new BigInteger(1) << int.MaxValue) >> 1 seems to give expected results.
  • Slightly less enormous negative BigIntegers also seem to not suffer from this issue: (new BigInteger(-1) << (int.MaxValue - 1000)) >> 1 seems to give expected results. (new BigInteger(-1) << (int.MaxValue - 10)) >> 1, however, still returns -1.
  • How much we right shift the enormous big integer does not seem to affect the result. (new BigInteger(-1) << int.MaxValue) >> (int.MaxValue - 4) still gives -1.
Author: gaazkam
Assignees: -
Labels:

area-System.Numerics, untriaged

Milestone: -

@tannergooding tannergooding added bug and removed untriaged New issue has not been triaged by the area owner labels Mar 14, 2022
@tannergooding tannergooding added this to the 7.0.0 milestone Mar 14, 2022
@jeffhandley
Copy link
Member

@tannergooding Do you think this bug can be fixed without the significant refactoring we've been talking about for BigInteger, or should we wait to address it until we have that work scheduled?

@tannergooding
Copy link
Member

Yes, it should be. It would probably be easier after, but I don't think that its necessary or good to wait if someone has time to look at this for .NET 7

dakersnar added a commit to dakersnar/runtime that referenced this issue Apr 11, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 11, 2022
@jeffhandley
Copy link
Member

jeffhandley commented Apr 11, 2022

Per discussion with the team, we will consider backporting the fix for this issue into .NET 6 if we can get validation that the user's scenario is addressed with the fix. We'd also need to get information that describes the types of applications affected, if the issues are blocking production scenarios, and an understanding of when .NET 7 would be adopted into that environment.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 19, 2022
dakersnar added a commit that referenced this issue Apr 19, 2022
…ed in Issue #65633 (#67867)

* Fix #65633, multiplication overflow causing incorrect right shift results

* Add unit test reproducing incorrect right shift results for BigInteger (#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 <[email protected]>

* Restrict unit test to 64-bit processors

* Document reasoning for restricting the test to 64-bit

Co-authored-by: Dan Moseley <[email protected]>

Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Dan Moseley <[email protected]>
directhex pushed a commit to directhex/runtime that referenced this issue Apr 21, 2022
…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 <[email protected]>

* Restrict unit test to 64-bit processors

* Document reasoning for restricting the test to 64-bit

Co-authored-by: Dan Moseley <[email protected]>

Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Dan Moseley <[email protected]>
@gaazkam
Copy link
Author

gaazkam commented May 10, 2022

Per discussion with the team, we will consider backporting the fix for this issue into .NET 6 if we can get validation that the user's scenario is addressed with the fix. We'd also need to get information that describes the types of applications affected, if the issues are blocking production scenarios, and an understanding of when .NET 7 would be adopted into that environment.

I am sorry for my late reply.

But no, this doesn't block any production scenarios. I hit this issue when I was trying to self-educate by implementing a program that would compute lots of digits of pi. This has never been intended to be used in production and even for this purpose I don't need this functionality.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants