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

Address bugs in BigInteger #27280

Merged
merged 6 commits into from Nov 11, 2019

Conversation

@ts2do
Copy link
Contributor

ts2do commented Oct 18, 2019

Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result
Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument not evenly divisible by 32 would generally compute the higher blocks incorrectly

Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result
Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly
@jkotas jkotas requested a review from tannergooding Oct 18, 2019
Copy link
Member

AntonLapounov left a comment

Too many bugs in this code. For Add we should also set result._length in case carry == 0. For ShiftLeft the first if misses setting output. It should read like this:

if ((input == 0) || (shift == 0))
{
    output.SetUInt64(input);
    return;
}
@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Oct 18, 2019

Looking at the things being fixed in this PR and the item called out by Anton. These bugs don't cause issues in production because the code code paths/failure cases in question aren't currently hit (and I don't believe ever will be hit).

This type is only meant for (and is designed around) float/double formatting/parsing; so I think the appropriate way to fix these bugs would be to remove the unnecessary logic. That should both "fix" the bugs and would potentially make the code faster for its intended purpose.

For example, public static void Add(ref BigInteger lhs, uint value, ref BigInteger result) is only ever called as public void Add(uint value) => Add(ref this, value, ref this); and public static void ShiftLeft(ulong input, uint shift, ref BigInteger output) is only ever used as ShiftLeft(1, exponent, ref result); (where result = new BigInteger(0)).

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Oct 18, 2019

Also, for reference, it looks like some of these bugs existed in the original BigNum.cpp code that this code was ported from: #19999

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 18, 2019

@tannergooding I would not trust other parts of this code either. For instance, SetUInt32 and SetUInt64 allow creating non-canonical zero values with _length greater than zero. That means IsZero may not work correctly.

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Oct 18, 2019

For instance, SetUInt32 and SetUInt64 allow creating non-canonical zero values with _length greater than zero.

Yes, but it should never actually happen given the existing usages and if it did; would just result in a slower computation; not necessarily an incorrect computation. That is, the code as is being used; and as was ported from the cpp code; is still functioning correctly.

There are certainly cases where Debug.Assert could be added and where the code is needlessly complicated (as a given scenario will never be hit). So, I'm just saying it would likely be better to simplify the code as part of fixing those; rather than trying to fix the logic to cover a case that will never be hit.

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 18, 2019

not necessarily an incorrect computation

Well, Compare is also affected and we use it for control flow. For example:

var x = new BigInteger();
x.SetUInt64(0);
var y = new BigInteger(0);

// Outputs 1
Console.WriteLine(BigInteger.Compare(ref x, ref y));

I agree that if there were an easy way to hit one of these bugs, we would have hit it a long time ago. Still that makes it very challenging to reason about this code.

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 18, 2019

@ts2do Thank you for spotting these bugs. As @tannergooding mentioned, the best approach is to remove both problematic methods by inlining them into their callers and simplifying. Please let us know if you might help with that or prefer us to change the code.

@ts2do

This comment has been minimized.

Copy link
Contributor Author

ts2do commented Oct 19, 2019

The static Add method overload is indeed being used directly by in src/System.Private.CoreLib/shared/System/Number.Dragon4.cs (which is invoked by FormatSingle and FormatDouble in some cases).
As for ShiftLeft, wouldn't it be better to fix the bug to simply support arbitrary shifting instead of limiting the shift parameter to 32? Without looking too far into it, ShiftLeft is used in Dragon4 with no apparent guarantee that the value will not exceed 32.

@ts2do

This comment has been minimized.

Copy link
Contributor Author

ts2do commented Oct 19, 2019

I've made the requested changes.

I also have a more heavily modified version of BigInteger that I believe makes it 1) more consistent (e.g., results of static methods are always provided via out parameters), 2) safer (e.g., added Debug.Assert calls which ensure that the result argument for Multiply and DivRem does not share the same address as one of the operands), and 3) more correct (e.g., skip Buffer.Memcpy if rhs is the same address as this). I figured it would be prudent to offer critical bug fixes instead of offering it all at once and having them lost in the noise.

Here's a gist with all of the changes:
https://gist.github.com/ts2do/629f3bc1a92eb10be18c1898b6169094

A few more notes:
I removed the BigInteger(uint) and BigInteger(ulong) constructors, as benchmarks results compared against directly setting fields have me thinking the fixed array is being zeroed. They would be replaced by static void SetUInt32(out BigInteger result, uint value) and static void SetUInt32(out BigInteger result, ulong value) to avoid that behavior.
I added an AsString method to BigInteger to help me debug it a little.

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 19, 2019

@ts2do Note that Dragon4 uses different ShiftLeft and Add overloads. The two problematic overloads have only a single caller each, which are in the same Number.BigInteger.cs file. In particular, ShiftLeft is used for Pow2 only. We want to remove these problematic methods, not to fix them. There are more issues, even with your latest commit.

ts2do added 2 commits Oct 21, 2019
Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint)
Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2
Inline ExtendBlock and ExtendBlocks into Pow2
Handle 0 in SetUInt32 and SetUInt64
@ts2do

This comment has been minimized.

Copy link
Contributor Author

ts2do commented Oct 23, 2019

My mistake, I was searching some local changes that I was toying around with. Sorry for the delay, I've been having some trouble running tests against the changes (though I got it building). I plan to redo my setup with a fresh installs soon, so until then, I guess I'll use CI to test it?

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 23, 2019

@ts2do Thank you. Your changes look great in general. Reviewing now.

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 24, 2019

@tannergooding Don't we have a real product bug in Multiply(ref, uint, ref)? We clearly call that method with lhsresult in four places in Dragon4. For instance:

var x = new BigInteger(7);
var y = new BigInteger(0);
BigInteger.Multiply(ref x, 6, ref y);
// Expected value: 42, actual value: 0
Console.WriteLine(y.ToUInt64());
@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Oct 24, 2019

Don't we have a real product bug in Multiply(ref, uint, ref)

There is a bug, but I don't believe it is one that can repro in production. The usages in Number.Dragon4 are around scaledMarginLow and pScaledMarginHigh and we only call Multiply when pScaledMarginHigh != &scaledMarginLow. Due to how the code works, the length of pScaledMarginHigh will always be greater than or equal to scaledMarginLow and will either stay the same (no carry) or will grow by one (in which case we do update it -- this is due to us only multiply by 2, which is a left shift by 1).

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 24, 2019

the length of pScaledMarginHigh will always be greater than or equal to scaledMarginLow

@tannergooding What about this code path, where we multiply scaledMarginLow by a presumably big power of 10, which may make its length greater than the length of pScaledMarginHigh?

scaledMarginLow.Multiply(ref pow10);
if (pScaledMarginHigh != &scaledMarginLow)
{
BigInteger.Multiply(ref scaledMarginLow, 2, ref *pScaledMarginHigh);

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Oct 24, 2019

What about this code path, where we multiply scaledMarginLow by a presumably big power of 10, which may make its length greater than the length of pScaledMarginHigh?

I'd need to check some math to determine if that is fine or not, but I believe it still works out due to how the margins exist and where they exist.

Still noting that this was ported from the native code and so the bug, if it exists, has been around basically forever.

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 28, 2019

@tannergooding I have tried

Dragon4Double(1.0 / (1L << 31), -1, true, ref buffer);

under a debugger and noticed that *pScaledMarginHigh was indeed calculated incorrectly as I anticipated. Namely, after multiplying by a power of 10, scaledMarginLow equals to 10⁹ and *pScaledMarginHigh must be 2×10⁹; however, its actual value was 2,820,130,816 due to not setting its _length field. Then Dragon4 used that incorrect value in a loop.

After fixing IsZero calculation, some tests are failing with:

Process terminated. Assertion failed.
   at System.Number.NumberToFloatingPointBitsSlow(NumberBuffer& number, FloatingPointInfo& info, UInt32 positiveExponent, UInt32 integerDigitsPresent, UInt32 fractionalDigitsPresent) in /_/src/System.Private.CoreLib/shared/System/Number.NumberToFloatingPointBits.cs:line 489
   at System.Number.NumberToFloatingPointBits(NumberBuffer& number, FloatingPointInfo& info) in /_/src/System.Private.CoreLib/shared/System/Number.NumberToFloatingPointBits.cs:line 414
   at System.Number.NumberToDouble(NumberBuffer& number) in /_/src/System.Private.CoreLib/shared/System/Number.Parsing.cs:line 1996

Would you be able to look at them?

@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Oct 29, 2019

It seems that before this fix Dragon4Double used to work incorrectly for 39% of the powers of two. Fortunately, we use this algorithm as a fallback only.

For instance, 2⁵⁵ is converted to 36028797018963968 before the fix and 36028797018963970 after the fix (one trailing digit shorter). 2⁹⁵⁷ is converted to 1.21816425142499988e+288 before the fix and 1.218164251425e+288 after the fix (five trailing digits shorter).

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Nov 4, 2019

Looks like its failing because the fractionalNumerator is asserted to be non-zero: https://source.dot.net/#System.Private.CoreLib/shared/System/Number.NumberToFloatingPointBits.cs,487

This assertion "should" still hold true as we have at least 1 fractional digit present

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Nov 5, 2019

Put up a PR for the fix here: #27688

@maryamariyan

This comment has been minimized.

Copy link
Member

maryamariyan commented Nov 6, 2019

Thank you for your contribution. As announced in #27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>
@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Nov 7, 2019

@ts2do, if you rebase your changes ontop of (or merge with) the latest master, everything should pass now and we can get this merged 😄

@ts2do

This comment has been minimized.

Copy link
Contributor Author

ts2do commented Nov 11, 2019

Everything should be all set.

@AntonLapounov AntonLapounov merged commit 65a7947 into dotnet:master Nov 11, 2019
50 checks passed
50 checks passed
WIP Ready for review
Details
coreclr-ci Build #20191109.41 succeeded
Details
coreclr-ci (Build Linux arm checked) Build Linux arm checked succeeded
Details
coreclr-ci (Build Linux arm64 checked) Build Linux arm64 checked succeeded
Details
coreclr-ci (Build Linux arm64 release) Build Linux arm64 release succeeded
Details
coreclr-ci (Build Linux x64 checked) Build Linux x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 checked) Build Linux_musl x64 checked succeeded
Details
coreclr-ci (Build Linux_musl x64 release) Build Linux_musl x64 release succeeded
Details
coreclr-ci (Build Linux_rhel6 x64 release) Build Linux_rhel6 x64 release succeeded
Details
coreclr-ci (Build OSX x64 checked) Build OSX x64 checked succeeded
Details
coreclr-ci (Build OSX x64 release) Build OSX x64 release succeeded
Details
coreclr-ci (Build Test Pri0 Linux arm checked) Build Test Pri0 Linux arm checked succeeded
Details
coreclr-ci (Build Test Pri0 Linux arm64 checked) Build Test Pri0 Linux arm64 checked succeeded
Details
coreclr-ci (Build Test Pri0 OSX x64 checked) Build Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Build Test Pri0 OSX x64 release) Build Test Pri0 OSX x64 release succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT arm checked) Build Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT arm64 checked) Build Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT x64 checked) Build Test Pri0 Windows_NT x64 checked succeeded
Details
coreclr-ci (Build Test Pri0 Windows_NT x86 checked) Build Test Pri0 Windows_NT x86 checked succeeded
Details
coreclr-ci (Build Windows_NT arm checked) Build Windows_NT arm checked succeeded
Details
coreclr-ci (Build Windows_NT arm release) Build Windows_NT arm release succeeded
Details
coreclr-ci (Build Windows_NT arm64 checked) Build Windows_NT arm64 checked succeeded
Details
coreclr-ci (Build Windows_NT arm64 release) Build Windows_NT arm64 release succeeded
Details
coreclr-ci (Build Windows_NT x64 checked) Build Windows_NT x64 checked succeeded
Details
coreclr-ci (Build Windows_NT x64 debug) Build Windows_NT x64 debug succeeded
Details
coreclr-ci (Build Windows_NT x64 release) Build Windows_NT x64 release succeeded
Details
coreclr-ci (Build Windows_NT x86 checked) Build Windows_NT x86 checked succeeded
Details
coreclr-ci (Build Windows_NT x86 debug) Build Windows_NT x86 debug succeeded
Details
coreclr-ci (Checkout (Unix)) Checkout (Unix) succeeded
Details
coreclr-ci (Checkout (Windows)) Checkout (Windows) succeeded
Details
coreclr-ci (Formatting Linux x64) Formatting Linux x64 succeeded
Details
coreclr-ci (Formatting Windows_NT x64) Formatting Windows_NT x64 succeeded
Details
coreclr-ci (Run Test Pri0 Linux arm checked) Run Test Pri0 Linux arm checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux arm64 checked) Run Test Pri0 Linux arm64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux x64 checked) Run Test Pri0 Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux_musl x64 checked) Run Test Pri0 Linux_musl x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Linux_musl x64 release) Run Test Pri0 Linux_musl x64 release succeeded
Details
coreclr-ci (Run Test Pri0 OSX x64 checked) Run Test Pri0 OSX x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT arm checked) Run Test Pri0 Windows_NT arm checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT arm64 checked) Run Test Pri0 Windows_NT arm64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT x64 checked) Run Test Pri0 Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 Windows_NT x86 checked) Run Test Pri0 Windows_NT x86 checked succeeded
Details
coreclr-ci (Run Test Pri0 CoreFX Linux x64 checked) Run Test Pri0 CoreFX Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 CoreFX Windows_NT x64 checked) Run Test Pri0 CoreFX Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Linux x64 checked) Run Test Pri0 R2R Linux x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R OSX x64 checked) Run Test Pri0 R2R OSX x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Windows_NT x64 checked) Run Test Pri0 R2R Windows_NT x64 checked succeeded
Details
coreclr-ci (Run Test Pri0 R2R Windows_NT x86 checked) Run Test Pri0 R2R Windows_NT x86 checked succeeded
Details
coreclr-ci (Test crossgen-comparison Linux arm checked) Test crossgen-comparison Linux arm checked succeeded
Details
license/cla All CLA requirements met.
Details
@AntonLapounov

This comment has been minimized.

Copy link
Member

AntonLapounov commented Nov 11, 2019

@tsdo Nice work — thanks a lot!

Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Nov 11, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Nov 12, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas added a commit to dotnet/corert that referenced this pull request Nov 12, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub added a commit to dotnet/corefx that referenced this pull request Nov 12, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Nov 14, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
akoeplinger added a commit to mono/mono that referenced this pull request Nov 15, 2019
* Method Add(ref BigInteger lhs, uint value, ref BigInteger result) would store most of the result blocks into lhs instead of result.
* Method ShiftLeft(ulong input, uint shift, ref BigInteger output) with a shift argument exceeding 32 would generally compute the higher blocks incorrectly.
* Multiply(ref BigInteger lhs, uint value, ref BigInteger result) would not set result._length in some cases.
* IsZero() would incorrectly return false for non-canonical zeros with _length > 0.

Fix:
* Inline Add(ref BigInteger, uint, ref BigInteger) into Add(uint).
* Inline ShiftLeft(ulong, uint, ref BigInteger) into Pow2.
* Inline ExtendBlock and ExtendBlocks into Pow2.
* Properly handle 0 in SetUInt32 and SetUInt64.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.