fix(frontend): Add type check for constant bit shift overflows#2497
fix(frontend): Add type check for constant bit shift overflows#2497
Conversation
| // and passing this if statement is a compiler bug. | ||
| if matches!(self.operator, BinaryOp::Div) && rhs == 0 { | ||
| return None; | ||
| unreachable!("ICE: the divisor of a binary op has been truncated into zero"); |
There was a problem hiding this comment.
I was thinking this was going to trigger if we did let z: u32 = 3 / 0, but it does not for some reason. Do you know why?
There was a problem hiding this comment.
eval_constants is only called after a check whether the rhs is 0 and we are not attempting a div. The truncate inside of eval_constants was truncating a value into the type of the lhs. In the case of let a: u1 = 1 >> 1 we are attempting to do 1 / 2, and thus are attempting to truncate 2 into a u1 which is why the rhs would be 0. For let z: u32 = 3 / 0 we never hit eval_constants.
There was a problem hiding this comment.
I understand that - I was missing that we actually have another of the same check on line 635 that checks if we're dividing with an rhs of zero. That prevents eval_constants from being called. Perhaps we only need one of these two checks.
There was a problem hiding this comment.
Yeah we should really only need the first one on 635. It was a mistake to originally have this check inside eval_constants, but I thought to leave it as a compiler sanity check for a more clear error than the panic shown in #2250. If we deem the check unnecessary I could just leave a comment stating that if we hit a divide by zero panic it is most likely due to a missing type check for an overflow.
| HirExpression::Infix(HirInfixExpression { operator, rhs, .. }) => { | ||
| if operator.is_bit_shift() { | ||
| let rhs_expr = self.interner.expression(&rhs); | ||
| let rhs_value: u128 = match rhs_expr { | ||
| HirExpression::Literal(HirLiteral::Integer(value)) => value.to_u128(), | ||
| _ => return, | ||
| }; | ||
| if let Type::Integer(_, bit_count) = annotated_type { | ||
| let max: u128 = 1 << bit_count; |
There was a problem hiding this comment.
This needs to be done in SSA or later. The type checker cannot catch cases like let z: u8 = 3 >> (4 + 4);
|
@vezenovm should this be closed? |
|
Yes this is superceded by #2713. Which is blocked at the moment, but this can be closed. |
3 participants
Description
Problem*
Resolves issue #2250 which was solved incorrectly previously. Reference the summary for details
Summary*
After #2481 running the
bit_shifts_comptimetest in debug mode an overflow was being triggered during ACIR generation. This overflow should have been caught earlier, thus I added a case to pass during type checking and added a compiler sanity check in case the bug comes back.An example error:
Runtime overflows are still possible, but these are being handled with (#2180).
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmton default settings.