interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch

Author: RalfJung

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

compiler/rustc_const_eval/src/interpret/operator.rs

@@ -157,41 +157,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!(),
                 }
             };
@@ -200,7 +194,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
                 );
             }

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,

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,

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`
+    }
+}

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
+