Merge pull request rust-lang#188 from RalfJung/overflow

Bail out on overflow

Author: oli-obk

src/error.rs

@@ -23,7 +23,6 @@ pub enum EvalError<'tcx> {
     },
     ReadPointerAsBytes,
     InvalidPointerMath,
-    OverflowingPointerMath,
     ReadUndefBytes,
     DeadLocal,
     InvalidBoolOp(mir::BinOp),
@@ -32,6 +31,7 @@ pub enum EvalError<'tcx> {
     ExecuteMemory,
     ArrayIndexOutOfBounds(Span, u64, u64),
     Math(Span, ConstMathErr),
+    OverflowingMath,
     InvalidChar(u128),
     OutOfMemory {
         allocation_size: u64,
@@ -83,8 +83,6 @@ impl<'tcx> Error for EvalError<'tcx> {
                 "a raw memory access tried to access part of a pointer value as raw bytes",
             EvalError::InvalidPointerMath =>
                 "attempted to do math or a comparison on pointers into different allocations",
-            EvalError::OverflowingPointerMath =>
-                "attempted to do overflowing math on a pointer",
             EvalError::ReadUndefBytes =>
                 "attempted to read undefined bytes",
             EvalError::DeadLocal =>
@@ -100,6 +98,8 @@ impl<'tcx> Error for EvalError<'tcx> {
                 "array index out of bounds",
             EvalError::Math(..) =>
                 "mathematical operation failed",
+            EvalError::OverflowingMath =>
+                "attempted to do overflowing math",
             EvalError::NoMirFor(..) =>
                 "mir not found",
             EvalError::InvalidChar(..) =>

src/eval_context.rs

@@ -452,8 +452,14 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
             }
 
             BinaryOp(bin_op, ref left, ref right) => {
-                // ignore overflow bit, rustc inserts check branches for us
-                self.intrinsic_overflowing(bin_op, left, right, dest, dest_ty)?;
+                if self.intrinsic_overflowing(bin_op, left, right, dest, dest_ty)? {
+                    // There was an overflow in an unchecked binop.  Right now, we consider this an error and bail out.
+                    // The rationale is that the reason rustc emits unchecked binops in release mode (vs. the checked binops
+                    // it emits in debug mode) is performance, but it doesn't cost us any performance in miri.
+                    // If, however, the compiler ever starts transforming unchecked intrinsics into unchecked binops,
+                    // we have to go back to just ignoring the overflow here.
+                    return Err(EvalError::OverflowingMath);
+                }
             }
 
             CheckedBinaryOp(bin_op, ref left, ref right) => {
@@ -869,7 +875,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
             self.memory.check_bounds(ptr, false)?;
             Ok(ptr)
         } else {
-            Err(EvalError::OverflowingPointerMath)
+            Err(EvalError::OverflowingMath)
         }
     }
 

src/memory.rs

@@ -73,7 +73,7 @@ impl Pointer {
             if let Some(res) = self.offset.checked_sub(n) {
                 Ok(Pointer::new(self.alloc_id, res))
             } else {
-                Err(EvalError::OverflowingPointerMath)
+                Err(EvalError::OverflowingMath)
             }
         } else {
             self.offset(i as u64, layout)
@@ -83,12 +83,12 @@ impl Pointer {
     pub fn offset<'tcx>(self, i: u64, layout: &TargetDataLayout) -> EvalResult<'tcx, Self> {
         if let Some(res) = self.offset.checked_add(i) {
             if res as u128 >= (1u128 << layout.pointer_size.bits()) {
-                Err(EvalError::OverflowingPointerMath)
+                Err(EvalError::OverflowingMath)
             } else {
                 Ok(Pointer::new(self.alloc_id, res))
             }
         } else {
-            Err(EvalError::OverflowingPointerMath)
+            Err(EvalError::OverflowingMath)
         }
     }
 
@@ -166,7 +166,8 @@ pub struct Memory<'a, 'tcx> {
     /// We mark memory as "packed" or "unaligned" for a single statement, and clear the marking
     /// afterwards. In the case where no packed structs are present, it's just a single emptyness
     /// check of a set instead of heavily influencing all memory access code as other solutions
-    /// would.
+    /// would. This is simpler than the alternative of passing a "packed" parameter to every
+    /// load/store method.
     ///
     /// One disadvantage of this solution is the fact that you can cast a pointer to a packed
     /// struct to a pointer to a normal struct and if you access a field of both in the same MIR

src/terminator/intrinsic.rs

@@ -43,7 +43,6 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
 
 
             "arith_offset" => {
-                // FIXME: Switch to non-checked, wrapped version of pointer_offset
                 let offset = self.value_to_primval(arg_vals[1], isize)?.to_i128()? as i64;
                 let ptr = arg_vals[0].read_ptr(&self.memory)?;
                 let result_ptr = self.wrapping_pointer_offset(ptr, substs.type_at(0), offset)?;

tests/compile-fail/out_of_bounds_ptr_2.rs

@@ -1,4 +1,4 @@
-// error-pattern: overflowing math on a pointer
+// error-pattern: overflowing math
 fn main() {
     let v = [0i8; 4];
     let x = &v as *const i8;

tests/compile-fail/ptr_offset_overflow.rs

@@ -1,4 +1,4 @@
-//error-pattern: overflowing math on a pointer
+//error-pattern: overflowing math
 fn main() {
     let v = [1i8, 2];
     let x = &v[1] as *const i8;

tests/run-pass/aux_test.rs

@@ -1,6 +1,7 @@
 // aux-build:dep.rs
-// This ignores the test against rustc, but runs it against miri:
+
 // ignore-cross-compile
+// TODO: The above accidentally also ignores this test against rustc even when are are not cross-compiling.
 
 extern crate dep;