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

Add overflow detection with preview opt-in #7206

Merged
merged 21 commits into from Jan 23, 2019

Conversation

@bcardiff
Copy link
Member

bcardiff commented Dec 20, 2018

This PR adds overflow checks for + - *, to number conversions to_iX, to_uX, to_fX and so to constructs like Int32.new(value).

The raised exception will contain, when possible, the location of the method/operator that originate it.

It adds some new methods to be able to keep previous wrapping behaviour. to_iX!/ Int32.new!(value). This methods should be used to be ready for upcoming breaking changes. (Split at: #7226)

It prepares some new primitives to be able to have cleaner compiler code in the future. That is drop the :cast internal in favor of :convert and :unchecked_convert to reflect better the nature of to_X/to_X! methods. (Split at: #7226)

There are many refactors to the stdlib (Split at #7256) and compiler code (Split at #7262) where the wrapping/unsafe behaviour was needed. Although some changes might look odd they should be reviewed as keeping the current code since operators and methods will be changing in a future version. Some changes show a need for future refactor (JSON/YAML parsing for dealing with UInt64, Enum flags with UInt64 base types, conversions for big types). In future version the overflow check on Time and other can be simplified.

In order to avoid breaking changes and to allow a preview of this feature, the overflow behavior is only enabled if compiled with -D preview_overflow.

I checked locally that even a 2nd compiler generation pass the specs. I did catch some left overs with that.

$ make clean_crystal crystal && sleep 1 && touch src/compiler/crystal.cr && make crystal
$ make std_spec compiler_spec

Closes #6223
Closes #3103

Sample

# file: foo.cr
Int32::MAX + 1
puts "ok"
$ ./bin/crystal foo.cr 
ok
$ ./bin/crystal foo.cr -D preview_overflow
Unhandled exception: Overflow (OverflowError)
...
Show resolved Hide resolved src/float.cr Outdated
Show resolved Hide resolved src/big/big_decimal.cr Outdated
Show resolved Hide resolved src/primitives.cr Outdated
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 20, 2018

Is there any way to make this multiple PRs? For example, a PR to add unsafe casts, a PR to update the stdlib, a PR to enable the option for standard operators to raise on overflow? Nearly 50 commits is a bit too much

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch 2 times, most recently from 934042a to 2a679bf Dec 20, 2018

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Dec 20, 2018

Once the build is happy I will split in probably 3 PRs.

  • prepare unsafe ops
  • refactor std libs and specs
  • implement overflow semantics

The Int128 support for 32 bits or even in linux is causing some issues.
Up to now Int128 multiplication was never stressed in the CI and now we are getting undefined reference to __muloti4.
I though it was due to llvm-4.0, but even building against llvm-6.0 is not changing that issue.
I will try to find a workaround for that mainly.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 20, 2018

Don't bother with Int128 specs which fail for now. Solving that will require a lot more work and can't block these PRs.

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch from fa087dc to efb0f0f Dec 24, 2018

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Dec 24, 2018

CircleCI is green! Travis has timed out. The arithmetic_specs are quite slow because prelude is included.

I need to polish last commits but it seems that the only part of compiler-rt that is needed for 128 bits ints can be defined in crystal. Since needing it would depend on platform/arch/llvm(?) i think that a flag -Dcompiler_rt could be added to include that definition on demand only.

I can’t avoid supporting 128bits in 32bits unless lots of specs are disabled or the whole existence of 128bits are excluded on a flag. This is because Int will be a union of all Ints and some code will requiere int128 mults. So fixing the support of 128bits on 32 bits seems to be the best option.

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Dec 24, 2018

I didn't have time to review this yet, but I will during this week (but feel free to merge it if you all think it's good)

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 24, 2018

@bcardiff Turns out that __muloti4 is actually an overflow-checking 64-bit multiply, not related to 128bit specs at all. Which seems pretty obvious looking at the code now...

I'm worried that introducing this symbol ourselves will break linking on platforms which link compiler-rt by default.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 24, 2018

Actually, it's a 64bit multiply with overflow checking used for the i128 support, weird.

Show resolved Hide resolved src/exception.cr
Show resolved Hide resolved src/exception.cr Outdated

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch 2 times, most recently from a658b3f to b807ce6 Dec 27, 2018

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch 2 times, most recently from 5fd3064 to f3171f4 Dec 28, 2018

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Dec 28, 2018

First stage of the PR split at #7226

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch 2 times, most recently from b59a051 to d89e663 Jan 2, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Jan 3, 2019

Second stage of the PR split at #7256.

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Jan 3, 2019

Third stage of the PR split at #7262 . The fourth and will be this PR after a merge & rebase.

@bcardiff bcardiff added this to the 0.27.1 milestone Jan 5, 2019

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 10, 2019

(needs a squash of the fixup commit before merge though)

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 10, 2019

@bcardiff the CI failure happened twice now on different CIs, I think you should run the spec in a loop and see if you can get a repro.

bcardiff added some commits Nov 21, 2018

Keep AST call operator location for primitive calls
for exact location information of the exception in the caller context
Codegen: Use llvm overflow intrinsics for safe int addition
* include overflow check before truncation
Built in implementation of __mulodi4 compiler-rt
required for 64 bits overflow in 32 bits arch, unless compiler-rt is built and linked.
Run Int128 spec only in darwin
Running Int128 specs in linux requires more compiler_rt symbols than __mulodi4
Refactor: Rename codegen_cast and codegen_convert for checked/unchecked
Allow :cast primitive to infer unchecked conversion from method ! suffix
Prepare :convert and :unsafe_convert primitives for better separation
Leave a codegen_cast that is used from codegen/cast.cr
Makefile: Use preview-overflow flag by default
-D compiler_rt is not needed in all platforms actually but trying to keep configuration simpler

@bcardiff bcardiff force-pushed the bcardiff:feature/preview-overflow branch from 3f32aeb to b66c6e7 Jan 22, 2019

@bcardiff

This comment has been minimized.

Copy link
Member Author

bcardiff commented Jan 22, 2019

I squashed the compiler_rt fix.
I left the refactor __crystal_raise_overflow alone in a commit to keep that part of the history.
I made some changes in codegen/debug.cr, the offset variable is expressed in bits, although I couldn't reproduce the Overflow, the argument is UInt64, so change the operation to take place in that type, despite the other operand been Int32, Int64, etc. That should fix the overflow, as long as the values are correct.
I also fix the binding of LibLLVM.offset_of_element to return UInt64.

For the record the largest number I got was when creating debug info for LibC::PthreadMutexT an offset of 4299161654. Which is still far from the overflow point.

Let's see what the CI thinks... 🍿

@bcardiff bcardiff merged commit adf215a into crystal-lang:master Jan 23, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff deleted the bcardiff:feature/preview-overflow branch Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment