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 checking for operations with mixed sign #9403

Merged
merged 3 commits into from
Jun 3, 2020

Conversation

waj
Copy link
Member

@waj waj commented Jun 2, 2020

This is a rebuilt of arithmetic operations with overflow checking (+, -, *) to fix issues on cases with arguments of different sign.

I implemented the same approach used by clang (https://github.com/llvm/llvm-project/blob/796898172c48a475f27f038e587c35dbba9ab7a6/clang/lib/CodeGen/CGBuiltin.cpp#L3378-L3465) for the builtin operations (__builtin_add_overflow, etc.) and using the same workaround for multiplication of integers mixed sign. This workaround is needed to avoid internal i128 multiplication for i64 values and to avoid sigfaults for i128 multiplications.

This also adds specs to test operations between all integer types, using some extreme values from each type and contrasting results using BigInt.

Before implementing this approach I've been testing a port of SafeInt. It's a more exhaustive case by case approach that produces shorter binary code, but it requires more work and detailed testing. I assume shorter also means faster although I didn't perform any benchmark. I decided to go with this easier but correct code first.

Fixes #9277

@waj waj added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:codegen labels Jun 2, 2020
@bcardiff bcardiff added this to the 0.35.0 milestone Jun 2, 2020
@waj waj marked this pull request as ready for review June 2, 2020 18:34
@bcardiff bcardiff merged commit 1a8ea04 into crystal-lang:master Jun 3, 2020
@RX14
Copy link
Member

RX14 commented Jun 5, 2020

Is it correct that this uses non-power-of-two LLVM types like i33?

@bcardiff
Copy link
Member

bcardiff commented Jun 5, 2020

Yes, since LLVM 2.5. Spooky 👻

The LLVM native code generator now supports arbitrary precision integers. Types like i33 have long been valid in the LLVM IR, but were previously only supported by the interpreter. Note that the C backend still does not support these.
https://releases.llvm.org/2.5/docs/ReleaseNotes.html

@asterite
Copy link
Member

asterite commented Jun 5, 2020

Hmm... yeah, like i128 is "supported". But I guess if CI passed then everything's fine :-)

@waj waj deleted the fix/checked-arithmetics branch June 5, 2020 14:18
@waj
Copy link
Member Author

waj commented Jun 5, 2020

i128 is supported but it requires compiler-rt. In macOS normaly clang is used during linking and it makes it automatically available. That's why I enabled Int128 and UInt128 tests only in darwin. We need to fix it but that's a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:codegen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i32 + u64 produces an overflow error
4 participants