Auto merge of #64890 - wesleywiser:const_prop_rvalue, r=oli-obk

[const-prop] Handle remaining MIR Rvalue cases

r? @oli-obk

Author: bors

src/librustc/mir/interpret/error.rs

@@ -363,6 +363,8 @@ pub enum UndefinedBehaviorInfo {
     UbExperimental(String),
     /// Unreachable code was executed.
     Unreachable,
+    /// An enum discriminant was set to a value which was outside the range of valid values.
+    InvalidDiscriminant(ScalarMaybeUndef),
 }
 
 impl fmt::Debug for UndefinedBehaviorInfo {
@@ -373,6 +375,8 @@ impl fmt::Debug for UndefinedBehaviorInfo {
                 write!(f, "{}", msg),
             Unreachable =>
                 write!(f, "entered unreachable code"),
+            InvalidDiscriminant(val) =>
+                write!(f, "encountered invalid enum discriminant {}", val),
         }
     }
 }
@@ -400,7 +404,6 @@ pub enum UnsupportedOpInfo<'tcx> {
     InvalidMemoryAccess,
     InvalidFunctionPointer,
     InvalidBool,
-    InvalidDiscriminant(ScalarMaybeUndef),
     PointerOutOfBounds {
         ptr: Pointer,
         msg: CheckInAllocMsg,
@@ -485,8 +488,6 @@ impl fmt::Debug for UnsupportedOpInfo<'tcx> {
                 write!(f, "incorrect alloc info: expected size {} and align {}, \
                            got size {} and align {}",
                     size.bytes(), align.bytes(), size2.bytes(), align2.bytes()),
-            InvalidDiscriminant(val) =>
-                write!(f, "encountered invalid enum discriminant {}", val),
             InvalidMemoryAccess =>
                 write!(f, "tried to access memory through an invalid pointer"),
             DanglingPointerDeref =>

src/librustc_mir/interpret/operand.rs

@@ -647,7 +647,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let bits_discr = raw_discr
                     .not_undef()
                     .and_then(|raw_discr| self.force_bits(raw_discr, discr_val.layout.size))
-                    .map_err(|_| err_unsup!(InvalidDiscriminant(raw_discr.erase_tag())))?;
+                    .map_err(|_| err_ub!(InvalidDiscriminant(raw_discr.erase_tag())))?;
                 let real_discr = if discr_val.layout.ty.is_signed() {
                     // going from layout tag type to typeck discriminant type
                     // requires first sign extending with the discriminant layout
@@ -677,7 +677,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     _ => bug!("tagged layout for non-adt non-generator"),
 
                 }.ok_or_else(
-                    || err_unsup!(InvalidDiscriminant(raw_discr.erase_tag()))
+                    || err_ub!(InvalidDiscriminant(raw_discr.erase_tag()))
                 )?;
                 (real_discr, index.0)
             },
@@ -689,15 +689,15 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 let variants_start = niche_variants.start().as_u32();
                 let variants_end = niche_variants.end().as_u32();
                 let raw_discr = raw_discr.not_undef().map_err(|_| {
-                    err_unsup!(InvalidDiscriminant(ScalarMaybeUndef::Undef))
+                    err_ub!(InvalidDiscriminant(ScalarMaybeUndef::Undef))
                 })?;
                 match raw_discr.to_bits_or_ptr(discr_val.layout.size, self) {
                     Err(ptr) => {
                         // The niche must be just 0 (which an inbounds pointer value never is)
                         let ptr_valid = niche_start == 0 && variants_start == variants_end &&
                             !self.memory.ptr_may_be_null(ptr);
                         if !ptr_valid {
-                            throw_unsup!(InvalidDiscriminant(raw_discr.erase_tag().into()))
+                            throw_ub!(InvalidDiscriminant(raw_discr.erase_tag().into()))
                         }
                         (dataful_variant.as_u32() as u128, dataful_variant)
                     },

src/librustc_mir/interpret/place.rs

@@ -1031,17 +1031,23 @@ where
         variant_index: VariantIdx,
         dest: PlaceTy<'tcx, M::PointerTag>,
     ) -> InterpResult<'tcx> {
+        let variant_scalar = Scalar::from_u32(variant_index.as_u32()).into();
+
         match dest.layout.variants {
             layout::Variants::Single { index } => {
-                assert_eq!(index, variant_index);
+                if index != variant_index {
+                    throw_ub!(InvalidDiscriminant(variant_scalar));
+                }
             }
             layout::Variants::Multiple {
                 discr_kind: layout::DiscriminantKind::Tag,
                 discr: ref discr_layout,
                 discr_index,
                 ..
             } => {
-                assert!(dest.layout.ty.variant_range(*self.tcx).unwrap().contains(&variant_index));
+                if !dest.layout.ty.variant_range(*self.tcx).unwrap().contains(&variant_index) {
+                    throw_ub!(InvalidDiscriminant(variant_scalar));
+                }
                 let discr_val =
                     dest.layout.ty.discriminant_for_variant(*self.tcx, variant_index).unwrap().val;
 
@@ -1064,9 +1070,9 @@ where
                 discr_index,
                 ..
             } => {
-                assert!(
-                    variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len(),
-                );
+                if !variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len() {
+                    throw_ub!(InvalidDiscriminant(variant_scalar));
+                }
                 if variant_index != dataful_variant {
                     let variants_start = niche_variants.start().as_u32();
                     let variant_index_relative = variant_index.as_u32()

src/librustc_mir/interpret/validity.rs

@@ -344,7 +344,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
         match self.walk_value(op) {
             Ok(()) => Ok(()),
             Err(err) => match err.kind {
-                err_unsup!(InvalidDiscriminant(val)) =>
+                err_ub!(InvalidDiscriminant(val)) =>
                     throw_validation_failure!(
                         val, self.path, "a valid enum discriminant"
                     ),

src/librustc_mir/transform/const_prop.rs

@@ -8,7 +8,7 @@ use rustc::hir::def::DefKind;
 use rustc::hir::def_id::DefId;
 use rustc::mir::{
     AggregateKind, Constant, Location, Place, PlaceBase, Body, Operand, Rvalue,
-    Local, NullOp, UnOp, StatementKind, Statement, LocalKind,
+    Local, UnOp, StatementKind, Statement, LocalKind,
     TerminatorKind, Terminator,  ClearCrossCrate, SourceInfo, BinOp,
     SourceScope, SourceScopeLocalData, LocalDecl, BasicBlock,
 };
@@ -118,7 +118,7 @@ impl<'tcx> MirPass<'tcx> for ConstProp {
 struct ConstPropMachine;
 
 impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
-    type MemoryKinds= !;
+    type MemoryKinds = !;
     type PointerTag = ();
     type ExtraFnVal = !;
 
@@ -434,32 +434,23 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
     ) -> Option<Const<'tcx>> {
         let span = source_info.span;
 
-        // if this isn't a supported operation, then return None
-        match rvalue {
-            Rvalue::Repeat(..) |
-            Rvalue::Aggregate(..) |
-            Rvalue::NullaryOp(NullOp::Box, _) |
-            Rvalue::Discriminant(..) => return None,
-
-            Rvalue::Use(_) |
-            Rvalue::Len(_) |
-            Rvalue::Cast(..) |
-            Rvalue::NullaryOp(..) |
-            Rvalue::CheckedBinaryOp(..) |
-            Rvalue::Ref(..) |
-            Rvalue::UnaryOp(..) |
-            Rvalue::BinaryOp(..) => { }
-        }
+        let overflow_check = self.tcx.sess.overflow_checks();
 
-        // perform any special checking for specific Rvalue types
-        if let Rvalue::UnaryOp(op, arg) = rvalue {
-            trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg);
-            let overflow_check = self.tcx.sess.overflow_checks();
+        // Perform any special handling for specific Rvalue types.
+        // Generally, checks here fall into one of two categories:
+        //   1. Additional checking to provide useful lints to the user
+        //        - In this case, we will do some validation and then fall through to the
+        //          end of the function which evals the assignment.
+        //   2. Working around bugs in other parts of the compiler
+        //        - In this case, we'll return `None` from this function to stop evaluation.
+        match rvalue {
+            // Additional checking: if overflow checks are disabled (which is usually the case in
+            // release mode), then we need to do additional checking here to give lints to the user
+            // if an overflow would occur.
+            Rvalue::UnaryOp(UnOp::Neg, arg) if !overflow_check => {
+                trace!("checking UnaryOp(op = Neg, arg = {:?})", arg);
 
-            self.use_ecx(source_info, |this| {
-                // We check overflow in debug mode already
-                // so should only check in release mode.
-                if *op == UnOp::Neg && !overflow_check {
+                self.use_ecx(source_info, |this| {
                     let ty = arg.ty(&this.local_decls, this.tcx);
 
                     if ty.is_integral() {
@@ -471,60 +462,70 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
                             throw_panic!(OverflowNeg)
                         }
                     }
+
+                    Ok(())
+                })?;
+            }
+
+            // Additional checking: check for overflows on integer binary operations and report
+            // them to the user as lints.
+            Rvalue::BinaryOp(op, left, right) => {
+                trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right);
+
+                let r = self.use_ecx(source_info, |this| {
+                    this.ecx.read_immediate(this.ecx.eval_operand(right, None)?)
+                })?;
+                if *op == BinOp::Shr || *op == BinOp::Shl {
+                    let left_bits = place_layout.size.bits();
+                    let right_size = r.layout.size;
+                    let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size));
+                    if r_bits.ok().map_or(false, |b| b >= left_bits as u128) {
+                        let source_scope_local_data = match self.source_scope_local_data {
+                            ClearCrossCrate::Set(ref data) => data,
+                            ClearCrossCrate::Clear => return None,
+                        };
+                        let dir = if *op == BinOp::Shr {
+                            "right"
+                        } else {
+                            "left"
+                        };
+                        let hir_id = source_scope_local_data[source_info.scope].lint_root;
+                        self.tcx.lint_hir(
+                            ::rustc::lint::builtin::EXCEEDING_BITSHIFTS,
+                            hir_id,
+                            span,
+                            &format!("attempt to shift {} with overflow", dir));
+                        return None;
+                    }
                 }
 
-                Ok(())
-            })?;
-        } else if let Rvalue::BinaryOp(op, left, right) = rvalue {
-            trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right);
-
-            let r = self.use_ecx(source_info, |this| {
-                this.ecx.read_immediate(this.ecx.eval_operand(right, None)?)
-            })?;
-            if *op == BinOp::Shr || *op == BinOp::Shl {
-                let left_bits = place_layout.size.bits();
-                let right_size = r.layout.size;
-                let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size));
-                if r_bits.ok().map_or(false, |b| b >= left_bits as u128) {
-                    let source_scope_local_data = match self.source_scope_local_data {
-                        ClearCrossCrate::Set(ref data) => data,
-                        ClearCrossCrate::Clear => return None,
-                    };
-                    let dir = if *op == BinOp::Shr {
-                        "right"
-                    } else {
-                        "left"
-                    };
-                    let hir_id = source_scope_local_data[source_info.scope].lint_root;
-                    self.tcx.lint_hir(
-                        ::rustc::lint::builtin::EXCEEDING_BITSHIFTS,
-                        hir_id,
-                        span,
-                        &format!("attempt to shift {} with overflow", dir));
-                    return None;
+                // If overflow checking is enabled (like in debug mode by default),
+                // then we'll already catch overflow when we evaluate the `Assert` statement
+                // in MIR. However, if overflow checking is disabled, then there won't be any
+                // `Assert` statement and so we have to do additional checking here.
+                if !overflow_check {
+                    self.use_ecx(source_info, |this| {
+                        let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?;
+                        let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?;
+
+                        if overflow {
+                            let err = err_panic!(Overflow(*op)).into();
+                            return Err(err);
+                        }
+
+                        Ok(())
+                    })?;
                 }
             }
-            self.use_ecx(source_info, |this| {
-                let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?;
-                let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?;
-
-                // We check overflow in debug mode already
-                // so should only check in release mode.
-                if !this.tcx.sess.overflow_checks() && overflow {
-                    let err = err_panic!(Overflow(*op)).into();
-                    return Err(err);
-                }
 
-                Ok(())
-            })?;
-        } else if let Rvalue::Ref(_, _, place) = rvalue {
-            trace!("checking Ref({:?})", place);
+            // Work around: avoid ICE in miri.
             // FIXME(wesleywiser) we don't currently handle the case where we try to make a ref
-            // from a function argument that hasn't been assigned to in this function.
-            if let Place {
-                base: PlaceBase::Local(local),
-                projection: box []
-            } = place {
+            // from a function argument that hasn't been assigned to in this function. The main
+            // issue is if an arg is a fat-pointer, miri `expects()` to be able to read the value
+            // of that pointer to get size info. However, since this is `ConstProp`, that argument
+            // doesn't actually have a backing value and so this causes an ICE.
+            Rvalue::Ref(_, _, Place { base: PlaceBase::Local(local), projection: box [] }) => {
+                trace!("checking Ref({:?})", place);
                 let alive =
                     if let LocalValue::Live(_) = self.ecx.frame().locals[*local].value {
                         true
@@ -535,6 +536,15 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
                     return None;
                 }
             }
+
+            // Work around: avoid extra unnecessary locals.
+            // FIXME(wesleywiser): const eval will turn this into a `const Scalar(<ZST>)` that
+            // `SimplifyLocals` doesn't know it can remove.
+            Rvalue::Aggregate(_, operands) if operands.len() == 0 => {
+                return None;
+            }
+
+            _ => { }
         }
 
         self.use_ecx(source_info, |this| {

src/test/compile-fail/consts/const-err3.rs

@@ -14,6 +14,7 @@ fn main() {
     //~^ ERROR const_err
     let _e = [5u8][1];
     //~^ ERROR const_err
+    //~| ERROR this expression will panic at runtime
     black_box(b);
     black_box(c);
     black_box(d);

src/test/mir-opt/const_prop/aggregate.rs

@@ -0,0 +1,25 @@
+// compile-flags: -O
+
+fn main() {
+    let x = (0, 1, 2).1 + 0;
+}
+
+// END RUST SOURCE
+// START rustc.main.ConstProp.before.mir
+//  bb0: {
+//      ...
+//      _3 = (const 0i32, const 1i32, const 2i32);
+//      _2 = (_3.1: i32);
+//      _1 = Add(move _2, const 0i32);
+//      ...
+//  }
+// END rustc.main.ConstProp.before.mir
+// START rustc.main.ConstProp.after.mir
+//  bb0: {
+//      ...
+//      _3 = (const 0i32, const 1i32, const 2i32);
+//      _2 = const 1i32;
+//      _1 = Add(move _2, const 0i32);
+//      ...
+//  }
+// END rustc.main.ConstProp.after.mir