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

Fix overflow bug in DividePrim #292

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Conversation

fniephaus
Copy link
Contributor

Long.MIN_VALUE / -1 is the only division that overflows.
Since the result is well-known, it can be precomputed.

"In cases where the size is int or long and overflow errors need to be detected, the methods addExact, subtractExact, multiplyExact, and toIntExact throw an ArithmeticException when the results overflow. For other arithmetic operations such as divide, absolute value, increment, decrement, and negation overflow occurs only with a specific minimum or maximum value and should be checked against the minimum or maximum as appropriate."
See https://docs.oracle.com/javase/8/docs/api/java/lang/Math.html

@smarr
Copy link
Owner

smarr commented Feb 12, 2019

Thanks a lot of the PR, I opened issues on the related repos.

Will have a look at it, and let the benchmarks run to get an impression of the impact of this change.
Not sure we can do much better though. And it's a correctness issue, so, yeah, difficult.

Perhaps one idea for an experiment to see how performance is impacted is to do this as a post-computation check.
Thus, check whether the result is what the overflow would give, then check the preconditions, and then deopt the thing to a specialization that produces big integers.

Todos for myself:

  • check benchmarks
  • try post-compute guard/deopt
  • add a test

@smarr
Copy link
Owner

smarr commented Aug 5, 2019

@fniephaus I am confused.

I added a test, and it passes without your change.

diff --git a/core-lib/TestSuite/IntegerTests.ns b/core-lib/TestSuite/IntegerTests.ns
index 5f6aac59f..f95ba7132 100644
--- a/core-lib/TestSuite/IntegerTests.ns
+++ b/core-lib/TestSuite/IntegerTests.ns
@@ -195,6 +195,8 @@ class IntegerTests usingPlatform: platform testFramework: minitest = (
 
       self assert:  3 equals: 7 / 2.0.
       self assert:  2 equals: 7 / 3.5.
+
+      self assert: 9223372036854775808 equals: (* Java.MIN_VALUE *) -9223372036854775808 / -1.
     )
 
     public testDouble = (

What am I doing wrong?

fniephaus and others added 2 commits August 5, 2019 17:55
`Long.MIN_VALUE / -1` is the only division that overflows.
Since the result is well-known, it can be precomputed.

"In cases where the size is int or long and overflow errors need to be detected, the methods addExact, subtractExact, multiplyExact, and toIntExact throw an ArithmeticException when the results overflow. For other arithmetic operations such as divide, absolute value, increment, decrement, and negation overflow occurs only with a specific minimum or maximum value and should be checked against the minimum or maximum as appropriate."
See https://docs.oracle.com/javase/8/docs/api/java/lang/Math.html

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
@smarr
Copy link
Owner

smarr commented Aug 5, 2019

Ok, I figured it out. The first tests literal was directly parsed into BigIntegers, which hid the issue.

@fniephaus
Copy link
Contributor Author

Whoops...fun! :)

@smarr smarr self-requested a review August 5, 2019 18:31
@smarr smarr self-assigned this Aug 5, 2019
@smarr smarr added the bug Fixes an issue, incorrect implementation label Aug 5, 2019
@smarr smarr added this to the v0.8.0 milestone Aug 5, 2019
@smarr smarr merged commit eb16019 into smarr:dev Aug 5, 2019
@smarr
Copy link
Owner

smarr commented Aug 5, 2019

Thanks! Benchmarks look ok. Change seems to work. So, I merged it.

@fniephaus fniephaus deleted the wip/divide-overflow branch August 5, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes an issue, incorrect implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants