Skip to content

Fix incorrect tracking of sign bit #59525

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

Merged
pgovind merged 6 commits into from
Sep 28, 2021
Merged

Fix incorrect tracking of sign bit #59525

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6c4a57
Fix incorrect tracking of sign bit
wzchua Sep 23, 2021
e83054f
Perform the 1 check earlier
wzchua Sep 23, 2021
7bbeb97
Add comment on checking special pattern
wzchua Sep 25, 2021
a14e984
Simplify fix
wzchua Sep 25, 2021
cd3c158
Update the test to target 32 bit right shift
wzchua Sep 25, 2021
5abd54a
Remove stray newline
wzchua Sep 28, 2021
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
Prev Previous commit
Next Next commit
Perform the 1 check earlier
  • Loading branch information
@wzchua
wzchua committed Sep 23, 2021
commit e83054f9f6a79ec7c33e2d3921e2083aae86f6a4
4 changes: 2 additions & 2 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 @@ -2022,9 +2022,9 @@ public static explicit operator decimal(BigInteger value)

NumericsHelpers.DangerousMakeTwosComplement(xd); // Mutates xd

if (smallShift == 0)
if (smallShift == 0 && xd[0] == 1)
Copy link
Member

@tannergooding tannergooding Sep 23, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@wzchua wzchua Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@wzchua wzchua Sep 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@tannergooding tannergooding Sep 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tannergooding tannergooding Sep 24, 2021

Choose a reason for hiding this comment

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

Can you add a code comment describing why this logic is needed for future readers. Otherwise this LGTM.

Copy link
Contributor Author

@wzchua wzchua Sep 25, 2021

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

@wzchua wzchua Sep 25, 2021

Choose a reason for hiding this comment

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

Oh, i discovered a simpler fix. I should have set the last uint as 0xFFFFFFFF before the 2's complement

{
bool allZero = xd[0] == 1;
bool allZero = true;
foreach (uint b in xd[1..])
{
if (b != 0)
Expand Down