Rollup merge of #117832 - RalfJung:interpret-shift, r=cjgillot

compiler-errors · web-flow · commit 94d9b7e7084a · 2023-11-19T19:14:33.000-08:00
interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch

While we're at it, also update comments in codegen and MIR building related to shifts, and fix the overflow error printed by Miri on negative shift amounts.
diff --git 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
@@ -156,41 +156,35 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
         // 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:
-            // <https://github.com/rust-lang/rust/blob/c274e4969f058b1c644243181ece9f829efa7594/compiler/rustc_codegen_ssa/src/base.rs#L315-L317>.
-            // The overflow check is also ignorant to the sign:
-            // <https://github.com/rust-lang/rust/blob/c274e4969f058b1c644243181ece9f829efa7594/compiler/rustc_codegen_ssa/src/mir/rvalue.rs#L728>.
-            // 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
-            // <https://github.com/rust-lang/rust/blob/c274e4969f058b1c644243181ece9f829efa7594/compiler/rustc_codegen_ssa/src/common.rs#L131-L158>
-            // 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!(),
                 }
             };
@@ -199,7 +193,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             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
@@ -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
@@ -600,10 +600,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             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 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 // 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
@@ -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
@@ -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
@@ -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
