diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 198e5696357..23054975548 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -322,8 +322,13 @@ pub fn cast_shift_expr_rhs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( if lhs_sz < rhs_sz { bx.trunc(rhs, lhs_llty) } else if lhs_sz > rhs_sz { - // FIXME (#1877: If in the future shifting by negative - // values is no longer undefined then this is wrong. + // We zero-extend even if the RHS is signed. So e.g. `(x: i32) << -1i8` will zero-extend the + // RHS to `255i32`. But then we mask the shift amount to be within the size of the LHS + // anyway so the result is `31` as it should be. All the extra bits introduced by zext + // are masked off so their value does not matter. + // FIXME: if we ever support 512bit integers, this will be wrong! For such large integers, + // the extra bits introduced by zext are *not* all masked away any more. + assert!(lhs_sz <= 256); bx.zext(rhs, lhs_llty) } else { rhs diff --git a/compiler/rustc_const_eval/src/interpret/operator.rs b/compiler/rustc_const_eval/src/interpret/operator.rs index a3ba9530f9d..5a3c85ab694 100644 --- a/compiler/rustc_const_eval/src/interpret/operator.rs +++ b/compiler/rustc_const_eval/src/interpret/operator.rs @@ -157,41 +157,35 @@ fn binary_int_op( // Shift ops can have an RHS with a different numeric type. if matches!(bin_op, Shl | ShlUnchecked | Shr | ShrUnchecked) { - let size = u128::from(left_layout.size.bits()); - // Even if `r` is signed, we treat it as if it was unsigned (i.e., we use its - // zero-extended form). This matches the codegen backend: - // . - // The overflow check is also ignorant to the sign: - // . - // This would behave rather strangely if we had integer types of size 256: a shift by - // -1i8 would actually shift by 255, but that would *not* be considered overflowing. A - // shift by -1i16 though would be considered overflowing. If we had integers of size - // 512, then a shift by -1i8 would even produce a different result than one by -1i16: - // the first shifts by 255, the latter by u16::MAX % 512 = 511. Lucky enough, our - // integers are maximally 128bits wide, so negative shifts *always* overflow and we have - // consistent results for the same value represented at different bit widths. - assert!(size <= 128); - let original_r = r; - let overflow = r >= size; - // The shift offset is implicitly masked to the type size, to make sure this operation - // is always defined. This is the one MIR operator that does *not* directly map to a - // single LLVM operation. See - // - // for the corresponding truncation in our codegen backends. - let r = r % size; - let r = u32::try_from(r).unwrap(); // we masked so this will always fit + let size = left_layout.size.bits(); + // The shift offset is implicitly masked to the type size. (This is the one MIR operator + // that does *not* directly map to a single LLVM operation.) Compute how much we + // actually shift and whether there was an overflow due to shifting too much. + let (shift_amount, overflow) = if right_layout.abi.is_signed() { + let shift_amount = self.sign_extend(r, right_layout) as i128; + let overflow = shift_amount < 0 || shift_amount >= i128::from(size); + let masked_amount = (shift_amount as u128) % u128::from(size); + debug_assert_eq!(overflow, shift_amount != (masked_amount as i128)); + (masked_amount, overflow) + } else { + let shift_amount = r; + let masked_amount = shift_amount % u128::from(size); + (masked_amount, shift_amount != masked_amount) + }; + let shift_amount = u32::try_from(shift_amount).unwrap(); // we masked so this will always fit + // Compute the shifted result. let result = if left_layout.abi.is_signed() { let l = self.sign_extend(l, left_layout) as i128; let result = match bin_op { - Shl | ShlUnchecked => l.checked_shl(r).unwrap(), - Shr | ShrUnchecked => l.checked_shr(r).unwrap(), + Shl | ShlUnchecked => l.checked_shl(shift_amount).unwrap(), + Shr | ShrUnchecked => l.checked_shr(shift_amount).unwrap(), _ => bug!(), }; result as u128 } else { match bin_op { - Shl | ShlUnchecked => l.checked_shl(r).unwrap(), - Shr | ShrUnchecked => l.checked_shr(r).unwrap(), + Shl | ShlUnchecked => l.checked_shl(shift_amount).unwrap(), + Shr | ShrUnchecked => l.checked_shr(shift_amount).unwrap(), _ => bug!(), } }; @@ -200,7 +194,11 @@ fn binary_int_op( if overflow && let Some(intrinsic_name) = throw_ub_on_overflow { throw_ub_custom!( fluent::const_eval_overflow_shift, - val = original_r, + val = if right_layout.abi.is_signed() { + (self.sign_extend(r, right_layout) as i128).to_string() + } else { + r.to_string() + }, name = intrinsic_name ); } diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index 7b0f27f9b34..8cf9e55f0b6 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -1404,18 +1404,18 @@ pub enum BinOp { BitOr, /// The `<<` operator (shift left) /// - /// The offset is truncated to the size of the first operand before shifting. + /// The offset is truncated to the size of the first operand and made unsigned before shifting. Shl, - /// Like `Shl`, but is UB if the RHS >= LHS::BITS + /// Like `Shl`, but is UB if the RHS >= LHS::BITS or RHS < 0 ShlUnchecked, /// The `>>` operator (shift right) /// - /// The offset is truncated to the size of the first operand before shifting. + /// The offset is truncated to the size of the first operand and made unsigned before shifting. /// /// This is an arithmetic shift if the LHS is signed /// and a logical shift if the LHS is unsigned. Shr, - /// Like `Shl`, but is UB if the RHS >= LHS::BITS + /// Like `Shl`, but is UB if the RHS >= LHS::BITS or RHS < 0 ShrUnchecked, /// The `==` operator (equality) Eq, diff --git a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs index eece8684e36..2130dbdc033 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs @@ -600,10 +600,12 @@ pub(crate) fn build_binary_op( BinOp::Shl | BinOp::Shr if self.check_overflow && ty.is_integral() => { // For an unsigned RHS, the shift is in-range for `rhs < bits`. // For a signed RHS, `IntToInt` cast to the equivalent unsigned - // type and do that same comparison. Because the type is the - // same size, there's no negative shift amount that ends up - // overlapping with valid ones, thus it catches negatives too. + // type and do that same comparison. + // A negative value will be *at least* 128 after the cast (that's i8::MIN), + // and 128 is an overflowing shift amount for all our currently existing types, + // so this cast can never make us miss an overflow. let (lhs_size, _) = ty.int_size_and_signed(self.tcx); + assert!(lhs_size.bits() <= 128); let rhs_ty = rhs.ty(&self.local_decls, self.tcx); let (rhs_size, _) = rhs_ty.int_size_and_signed(self.tcx); @@ -625,7 +627,6 @@ pub(crate) fn build_binary_op( // This can't overflow because the largest shiftable types are 128-bit, // which fits in `u8`, the smallest possible `unsigned_ty`. - // (And `from_uint` will `bug!` if that's ever no longer true.) let lhs_bits = Operand::const_from_scalar( self.tcx, unsigned_ty, diff --git a/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.rs b/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.rs new file mode 100644 index 00000000000..a5777ebbb7f --- /dev/null +++ b/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.rs @@ -0,0 +1,9 @@ +#![feature(core_intrinsics)] +use std::intrinsics; + +fn main() { + unsafe { + let _n = intrinsics::unchecked_shl(1i8, -1); + //~^ ERROR: overflowing shift by -1 in `unchecked_shl` + } +} diff --git a/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.stderr b/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.stderr new file mode 100644 index 00000000000..ab27c9fe911 --- /dev/null +++ b/src/tools/miri/tests/fail/intrinsics/unchecked_shl2.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: overflowing shift by -1 in `unchecked_shl` + --> $DIR/unchecked_shl2.rs:LL:CC + | +LL | let _n = intrinsics::unchecked_shl(1i8, -1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/unchecked_shl2.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/tests/ui/consts/const-int-unchecked.stderr b/tests/ui/consts/const-int-unchecked.stderr index ad880d56d90..ad14c8f68f8 100644 --- a/tests/ui/consts/const-int-unchecked.stderr +++ b/tests/ui/consts/const-int-unchecked.stderr @@ -62,61 +62,61 @@ error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:41:33 | LL | const SHL_I8_NEG: i8 = unsafe { intrinsics::unchecked_shl(5_i8, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 255 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:43:35 | LL | const SHL_I16_NEG: i16 = unsafe { intrinsics::unchecked_shl(5_16, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 65535 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:45:35 | LL | const SHL_I32_NEG: i32 = unsafe { intrinsics::unchecked_shl(5_i32, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 4294967295 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:47:35 | LL | const SHL_I64_NEG: i64 = unsafe { intrinsics::unchecked_shl(5_i64, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 18446744073709551615 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:49:37 | LL | const SHL_I128_NEG: i128 = unsafe { intrinsics::unchecked_shl(5_i128, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 340282366920938463463374607431768211455 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:55:40 | LL | const SHL_I8_NEG_RANDOM: i8 = unsafe { intrinsics::unchecked_shl(5_i8, -6) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 250 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -6 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:57:42 | LL | const SHL_I16_NEG_RANDOM: i16 = unsafe { intrinsics::unchecked_shl(5_16, -13) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 65523 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -13 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:59:42 | LL | const SHL_I32_NEG_RANDOM: i32 = unsafe { intrinsics::unchecked_shl(5_i32, -25) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 4294967271 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -25 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:61:42 | LL | const SHL_I64_NEG_RANDOM: i64 = unsafe { intrinsics::unchecked_shl(5_i64, -30) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 18446744073709551586 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -30 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:63:44 | LL | const SHL_I128_NEG_RANDOM: i128 = unsafe { intrinsics::unchecked_shl(5_i128, -93) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 340282366920938463463374607431768211363 in `unchecked_shl` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -93 in `unchecked_shl` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:70:29 @@ -182,61 +182,61 @@ error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:96:33 | LL | const SHR_I8_NEG: i8 = unsafe { intrinsics::unchecked_shr(5_i8, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 255 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:98:35 | LL | const SHR_I16_NEG: i16 = unsafe { intrinsics::unchecked_shr(5_16, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 65535 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:100:35 | LL | const SHR_I32_NEG: i32 = unsafe { intrinsics::unchecked_shr(5_i32, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 4294967295 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:102:35 | LL | const SHR_I64_NEG: i64 = unsafe { intrinsics::unchecked_shr(5_i64, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 18446744073709551615 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:104:37 | LL | const SHR_I128_NEG: i128 = unsafe { intrinsics::unchecked_shr(5_i128, -1) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 340282366920938463463374607431768211455 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -1 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:110:40 | LL | const SHR_I8_NEG_RANDOM: i8 = unsafe { intrinsics::unchecked_shr(5_i8, -6) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 250 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -6 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:112:42 | LL | const SHR_I16_NEG_RANDOM: i16 = unsafe { intrinsics::unchecked_shr(5_16, -13) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 65523 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -13 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:114:42 | LL | const SHR_I32_NEG_RANDOM: i32 = unsafe { intrinsics::unchecked_shr(5_i32, -25) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 4294967271 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -25 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:116:42 | LL | const SHR_I64_NEG_RANDOM: i64 = unsafe { intrinsics::unchecked_shr(5_i64, -30) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 18446744073709551586 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -30 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:118:44 | LL | const SHR_I128_NEG_RANDOM: i128 = unsafe { intrinsics::unchecked_shr(5_i128, -93) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 340282366920938463463374607431768211363 in `unchecked_shr` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by -93 in `unchecked_shr` error[E0080]: evaluation of constant value failed --> $DIR/const-int-unchecked.rs:123:25