Check for overflow when extending signed integers to unsigned integers #10000
Conversation
straight-shoota
added
kind:bug
| if checked | ||
| if from_type.signed? && to_type.unsigned? | ||
| overflow = codegen_out_of_range(to_type, from_type, arg) | ||
| codegen_raise_overflow_cond(overflow) |
There was a problem hiding this comment.
Somehow I feel the code below this shouldn't execute... 🤔
There was a problem hiding this comment.
I don't follow the comment @asterite. What do you mean?
There was a problem hiding this comment.
Two lines below this there's more code and it will be generated, but it shouldn't. This line is missing a return.
There was a problem hiding this comment.
oh, maybe not? I thought the check would always raise... nevermind
There was a problem hiding this comment.
The codegen will emit a raise block and leave the builder in place to continue the normal path which in this case is the extend_int. But this method itself won't raise. It's generated code will.
| @@ -211,6 +211,23 @@ describe "Code gen: arithmetic primitives" do | |||
| end | |||
| {% end %} | |||
|
|
|||
| {% if [UInt16, UInt32, UInt64].includes?(type) %} | |||
There was a problem hiding this comment.
Was there a reason to exclude UInt8 and UInt128 from this list?
There was a problem hiding this comment.
UInt8 is because the issue isn't concerned with same-rank conversions. UInt128 is because I'm not quite clear how much support for 128-bit integers is actually there (especially for CI).
There was a problem hiding this comment.
I see. It bugs me slightly that we are not covering all the cases despite the implementation.
I would express that any unsigned int should raise on -1_{{type}} for all signed types. For this kind of specs I started spec/support/number.cr.
Let us know if you want to tweak these specs further if this makes sense to you. Otherwise it might come in another PR at some point.
| if checked | ||
| if from_type.signed? && to_type.unsigned? | ||
| overflow = codegen_out_of_range(to_type, from_type, arg) | ||
| codegen_raise_overflow_cond(overflow) |
There was a problem hiding this comment.
I don't follow the comment @asterite. What do you mean?
Fixes #9997. Code like
-1_i8.to_u16will raise after this PR.Crystal relies on wrapping conversions from
Int32toUInt64in a few places, but incorrectly used the safe variants:Pointer.new(address : Int): Since pointers are already fairly unsafe, this change should be fine. Without this, Crystal would complain about lib constants likeLibC::MAP_FAILED = Pointer(Void).new(-1); the alternative fix is to call.to_u64!on those-1instead.Random::PCG32#jump(delta): From the original source code:Even though delta is an unsigned integer, we can pass a signed integer to go backwards, it just goes "the long way round".Thus wrapping conversion is indeed correct here.spec/compiler/codegen/tuple_spec.cr: The same spec uses&+instead of+, so.to_u64!is probably the right thing to do already even without this PR.