Merge pull request rust-lang#28 from oli-obk/oflo

cleanup overflow binop code

src/error.rs

@@ -3,6 +3,8 @@ use std::fmt;
 use rustc::mir::repr as mir;
 use rustc::ty::BareFnTy;
 use memory::Pointer;
+use rustc_const_math::ConstMathErr;
+use syntax::codemap::Span;
 
 #[derive(Clone, Debug)]
 pub enum EvalError<'tcx> {
@@ -24,6 +26,8 @@ pub enum EvalError<'tcx> {
     Unimplemented(String),
     DerefFunctionPointer,
     ExecuteMemory,
+    ArrayIndexOutOfBounds(Span, u64, u64),
+    Math(Span, ConstMathErr),
 }
 
 pub type EvalResult<'tcx, T> = Result<T, EvalError<'tcx>>;
@@ -58,6 +62,10 @@ impl<'tcx> Error for EvalError<'tcx> {
                 "tried to dereference a function pointer",
             EvalError::ExecuteMemory =>
                 "tried to treat a memory pointer as a function pointer",
+            EvalError::ArrayIndexOutOfBounds(..) =>
+                "array index out of bounds",
+            EvalError::Math(..) =>
+                "mathematical operation failed",
         }
     }
 
@@ -73,6 +81,10 @@ impl<'tcx> fmt::Display for EvalError<'tcx> {
             },
             EvalError::FunctionPointerTyMismatch(expected, got) =>
                 write!(f, "tried to call a function of type {:?} through a function pointer of type {:?}", expected, got),
+            EvalError::ArrayIndexOutOfBounds(span, len, index) =>
+                write!(f, "array index {} out of bounds {} at {:?}", index, len, span),
+            EvalError::Math(span, ref err) =>
+                write!(f, "mathematical operation at {:?} failed with {:?}", span, err),
             _ => write!(f, "{}", self.description()),
         }
     }

src/lib.rs

@@ -2,7 +2,6 @@
     btree_range,
     collections,
     collections_bound,
-    core_intrinsics,
     filling_drop,
     question_mark,
     rustc_private,
@@ -14,6 +13,7 @@
 extern crate rustc_data_structures;
 extern crate rustc_mir;
 extern crate rustc_trans;
+extern crate rustc_const_math;
 extern crate syntax;
 #[macro_use] extern crate log;
 extern crate log_settings;

src/memory.rs

@@ -42,7 +42,7 @@ impl Pointer {
     }
 }
 
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq)]
 pub struct FunctionDefinition<'tcx> {
     pub def_id: DefId,
     pub substs: &'tcx Substs<'tcx>,
@@ -59,6 +59,8 @@ pub struct Memory<'tcx> {
     /// Function "allocations". They exist solely so pointers have something to point to, and
     /// we can figure out what they point to.
     functions: HashMap<AllocId, FunctionDefinition<'tcx>>,
+    /// Inverse map of `functions` so we don't allocate a new pointer every time we need one
+    function_alloc_cache: HashMap<FunctionDefinition<'tcx>, AllocId>,
     next_id: AllocId,
     pub pointer_size: usize,
 }
@@ -69,22 +71,29 @@ impl<'tcx> Memory<'tcx> {
         Memory {
             alloc_map: HashMap::new(),
             functions: HashMap::new(),
+            function_alloc_cache: HashMap::new(),
             next_id: AllocId(0),
             pointer_size: pointer_size,
         }
     }
 
-    // FIXME: never create two pointers to the same def_id + substs combination
-    // maybe re-use the statics cache of the EvalContext?
     pub fn create_fn_ptr(&mut self, def_id: DefId, substs: &'tcx Substs<'tcx>, fn_ty: &'tcx BareFnTy<'tcx>) -> Pointer {
-        let id = self.next_id;
-        debug!("creating fn ptr: {}", id);
-        self.next_id.0 += 1;
-        self.functions.insert(id, FunctionDefinition {
+        let def = FunctionDefinition {
             def_id: def_id,
             substs: substs,
             fn_ty: fn_ty,
-        });
+        };
+        if let Some(&alloc_id) = self.function_alloc_cache.get(&def) {
+            return Pointer {
+                alloc_id: alloc_id,
+                offset: 0,
+            };
+        }
+        let id = self.next_id;
+        debug!("creating fn ptr: {}", id);
+        self.next_id.0 += 1;
+        self.functions.insert(id, def);
+        self.function_alloc_cache.insert(def, id);
         Pointer {
             alloc_id: id,
             offset: 0,
@@ -361,6 +370,7 @@ impl<'tcx> Memory<'tcx> {
             PrimVal::U32(n)  => self.write_uint(ptr, n as u64, 4),
             PrimVal::U64(n)  => self.write_uint(ptr, n as u64, 8),
             PrimVal::IntegerPtr(n) => self.write_uint(ptr, n as u64, pointer_size),
+            PrimVal::FnPtr(_p) |
             PrimVal::AbstractPtr(_p) => unimplemented!(),
         }
     }

src/primval.rs

@@ -10,28 +10,41 @@ pub enum PrimVal {
     U8(u8), U16(u16), U32(u32), U64(u64),
 
     AbstractPtr(Pointer),
+    FnPtr(Pointer),
     IntegerPtr(u64),
 }
 
-pub fn binary_op<'tcx>(bin_op: mir::BinOp, left: PrimVal, right: PrimVal) -> EvalResult<'tcx, PrimVal> {
+/// returns the result of the operation and whether the operation overflowed
+pub fn binary_op<'tcx>(bin_op: mir::BinOp, left: PrimVal, right: PrimVal) -> EvalResult<'tcx, (PrimVal, bool)> {
     use rustc::mir::repr::BinOp::*;
     use self::PrimVal::*;
 
+    macro_rules! overflow {
+        ($v:ident, $v2:ident, $l:ident, $op:ident, $r:ident) => ({
+            let (val, of) = $l.$op($r);
+            if of {
+                return Ok(($v(val), true));
+            } else {
+                $v(val)
+            }
+        })
+    }
+
     macro_rules! int_binops {
         ($v:ident, $l:ident, $r:ident) => ({
             match bin_op {
-                Add    => $v($l + $r),
-                Sub    => $v($l - $r),
-                Mul    => $v($l * $r),
-                Div    => $v($l / $r),
-                Rem    => $v($l % $r),
+                Add    => overflow!($v, $v, $l, overflowing_add, $r),
+                Sub    => overflow!($v, $v, $l, overflowing_sub, $r),
+                Mul    => overflow!($v, $v, $l, overflowing_mul, $r),
+                Div    => overflow!($v, $v, $l, overflowing_div, $r),
+                Rem    => overflow!($v, $v, $l, overflowing_rem, $r),
                 BitXor => $v($l ^ $r),
                 BitAnd => $v($l & $r),
                 BitOr  => $v($l | $r),
 
-                // TODO(solson): Can have differently-typed RHS.
-                Shl => $v($l << $r),
-                Shr => $v($l >> $r),
+                // these have already been handled
+                Shl => unreachable!(),
+                Shr => unreachable!(),
 
                 Eq => Bool($l == $r),
                 Ne => Bool($l != $r),
@@ -53,6 +66,58 @@ pub fn binary_op<'tcx>(bin_op: mir::BinOp, left: PrimVal, right: PrimVal) -> Eva
         }
     }
 
+    match bin_op {
+        // can have rhs with a different numeric type
+        Shl | Shr => {
+            // these numbers are the maximum number a bitshift rhs could possibly have
+            // e.g. u16 can be bitshifted by 0..16, so masking with 0b1111 (16 - 1) will ensure we are in that range
+            let type_bits: u32 = match left {
+                I8(_) | U8(_) => 8,
+                I16(_) | U16(_) => 16,
+                I32(_) | U32(_) => 32,
+                I64(_) | U64(_) => 64,
+                _ => unreachable!(),
+            };
+            assert!(type_bits.is_power_of_two());
+            // turn into `u32` because `overflowing_sh{l,r}` only take `u32`
+            let r = match right {
+                I8(i) => i as u32,
+                I16(i) => i as u32,
+                I32(i) => i as u32,
+                I64(i) => i as u32,
+                U8(i) => i as u32,
+                U16(i) => i as u32,
+                U32(i) => i as u32,
+                U64(i) => i as u32,
+                _ => panic!("bad MIR: bitshift rhs is not integral"),
+            };
+            // apply mask
+            let r = r & (type_bits - 1);
+            macro_rules! shift {
+                ($v:ident, $l:ident, $r:ident) => ({
+                    match bin_op {
+                        Shl => overflow!($v, U32, $l, overflowing_shl, $r),
+                        Shr => overflow!($v, U32, $l, overflowing_shr, $r),
+                        _ => unreachable!(),
+                    }
+                })
+            }
+            let val = match left {
+                I8(l) => shift!(I8, l, r),
+                I16(l) => shift!(I16, l, r),
+                I32(l) => shift!(I32, l, r),
+                I64(l) => shift!(I64, l, r),
+                U8(l) => shift!(U8, l, r),
+                U16(l) => shift!(U16, l, r),
+                U32(l) => shift!(U32, l, r),
+                U64(l) => shift!(U64, l, r),
+                _ => unreachable!(),
+            };
+            return Ok((val, false));
+        },
+        _ => {},
+    }
+
     let val = match (left, right) {
         (I8(l),  I8(r))  => int_binops!(I8, l, r),
         (I16(l), I16(r)) => int_binops!(I16, l, r),
@@ -80,12 +145,23 @@ pub fn binary_op<'tcx>(bin_op: mir::BinOp, left: PrimVal, right: PrimVal) -> Eva
 
         (IntegerPtr(l), IntegerPtr(r)) => int_binops!(IntegerPtr, l, r),
 
-        (AbstractPtr(_), IntegerPtr(_)) | (IntegerPtr(_), AbstractPtr(_)) =>
-            return unrelated_ptr_ops(bin_op),
+        (AbstractPtr(_), IntegerPtr(_)) |
+        (IntegerPtr(_), AbstractPtr(_)) |
+        (FnPtr(_), AbstractPtr(_)) |
+        (AbstractPtr(_), FnPtr(_)) |
+        (FnPtr(_), IntegerPtr(_)) |
+        (IntegerPtr(_), FnPtr(_)) =>
+            unrelated_ptr_ops(bin_op)?,
+
+        (FnPtr(l_ptr), FnPtr(r_ptr)) => match bin_op {
+            Eq => Bool(l_ptr == r_ptr),
+            Ne => Bool(l_ptr != r_ptr),
+            _ => return Err(EvalError::Unimplemented(format!("unimplemented fn ptr comparison: {:?}", bin_op))),
+        },
 
         (AbstractPtr(l_ptr), AbstractPtr(r_ptr)) => {
             if l_ptr.alloc_id != r_ptr.alloc_id {
-                return unrelated_ptr_ops(bin_op);
+                return Ok((unrelated_ptr_ops(bin_op)?, false));
             }
 
             let l = l_ptr.offset;
@@ -105,7 +181,7 @@ pub fn binary_op<'tcx>(bin_op: mir::BinOp, left: PrimVal, right: PrimVal) -> Eva
         (l, r) => return Err(EvalError::Unimplemented(format!("unimplemented binary op: {:?}, {:?}, {:?}", l, r, bin_op))),
     };
 
-    Ok(val)
+    Ok((val, false))
 }
 
 pub fn unary_op<'tcx>(un_op: mir::UnOp, val: PrimVal) -> EvalResult<'tcx, PrimVal> {

tests/compile-fail/cast_fn_ptr_unsafe.rs

@@ -0,0 +1,10 @@
+// just making sure that fn -> unsafe fn casts are handled by rustc so miri doesn't have to
+fn main() {
+    fn f() {}
+
+    let g = f as fn() as unsafe fn(i32); //~ERROR: non-scalar cast: `fn()` as `unsafe fn(i32)`
+
+    unsafe {
+        g(42);
+    }
+}

tests/compile-fail/cast_fn_ptr_unsafe2.rs

@@ -0,0 +1,10 @@
+// just making sure that fn -> unsafe fn casts are handled by rustc so miri doesn't have to
+fn main() {
+    fn f() {}
+
+    let g = f as fn() as fn(i32) as unsafe fn(i32); //~ERROR: non-scalar cast: `fn()` as `fn(i32)`
+
+    unsafe {
+        g(42);
+    }
+}

tests/compile-fail/env_args.rs

@@ -0,0 +1,6 @@
+//error-pattern: no mir for `std
+
+fn main() {
+    let x = std::env::args();
+    assert_eq!(x.count(), 1);
+}

tests/compiletest.rs

@@ -3,23 +3,28 @@ extern crate compiletest_rs as compiletest;
 use std::path::{PathBuf, Path};
 use std::io::Write;
 
-fn run_mode(dir: &'static str, mode: &'static str, sysroot: &str) {
-    // Disable rustc's new error fomatting. It breaks these tests.
-    std::env::remove_var("RUST_NEW_ERROR_FORMAT");
+fn compile_fail(sysroot: &str) {
     let flags = format!("--sysroot {} -Dwarnings", sysroot);
     for_all_targets(sysroot, |target| {
         let mut config = compiletest::default_config();
         config.host_rustcflags = Some(flags.clone());
-        config.mode = mode.parse().expect("Invalid mode");
+        config.mode = "compile-fail".parse().expect("Invalid mode");
         config.run_lib_path = Path::new(sysroot).join("lib").join("rustlib").join(&target).join("lib");
         config.rustc_path = "target/debug/miri".into();
-        config.src_base = PathBuf::from(format!("tests/{}", dir));
+        config.src_base = PathBuf::from("tests/compile-fail".to_string());
         config.target = target.to_owned();
         config.target_rustcflags = Some(flags.clone());
         compiletest::run_tests(&config);
     });
 }
 
+fn run_pass() {
+    let mut config = compiletest::default_config();
+    config.mode = "run-pass".parse().expect("Invalid mode");
+    config.src_base = PathBuf::from("tests/run-pass".to_string());
+    compiletest::run_tests(&config);
+}
+
 fn for_all_targets<F: FnMut(String)>(sysroot: &str, mut f: F) {
     for target in std::fs::read_dir(format!("{}/lib/rustlib/", sysroot)).unwrap() {
         let target = target.unwrap();
@@ -38,7 +43,6 @@ fn for_all_targets<F: FnMut(String)>(sysroot: &str, mut f: F) {
 
 #[test]
 fn compile_test() {
-    let mut failed = false;
     // Taken from https://github.com/Manishearth/rust-clippy/pull/911.
     let home = option_env!("RUSTUP_HOME").or(option_env!("MULTIRUST_HOME"));
     let toolchain = option_env!("RUSTUP_TOOLCHAIN").or(option_env!("MULTIRUST_TOOLCHAIN"));
@@ -48,7 +52,8 @@ fn compile_test() {
             .expect("need to specify RUST_SYSROOT env var or use rustup or multirust")
             .to_owned(),
     };
-    run_mode("compile-fail", "compile-fail", &sysroot);
+    compile_fail(&sysroot);
+    run_pass();
     for_all_targets(&sysroot, |target| {
         for file in std::fs::read_dir("tests/run-pass").unwrap() {
             let file = file.unwrap();
@@ -72,21 +77,18 @@ fn compile_test() {
             match cmd.output() {
                 Ok(ref output) if output.status.success() => writeln!(stderr.lock(), "ok").unwrap(),
                 Ok(output) => {
-                    failed = true;
                     writeln!(stderr.lock(), "FAILED with exit code {}", output.status.code().unwrap_or(0)).unwrap();
                     writeln!(stderr.lock(), "stdout: \n {}", std::str::from_utf8(&output.stdout).unwrap()).unwrap();
                     writeln!(stderr.lock(), "stderr: \n {}", std::str::from_utf8(&output.stderr).unwrap()).unwrap();
+                    panic!("some tests failed");
                 }
                 Err(e) => {
-                    failed = true;
                     writeln!(stderr.lock(), "FAILED: {}", e).unwrap();
+                    panic!("some tests failed");
                 },
             }
         }
         let stderr = std::io::stderr();
         writeln!(stderr.lock(), "").unwrap();
     });
-    if failed {
-        panic!("some tests failed");
-    }
 }

tests/run-pass/cast_fn_ptr_unsafe.rs

@@ -0,0 +1,8 @@
+fn main() {
+    fn f() {}
+
+    let g = f as fn() as unsafe fn();
+    unsafe {
+        g();
+    }
+}

tests/run-pass/function_pointers.rs

@@ -12,4 +12,6 @@ fn call_fn_ptr() -> i32 {
 
 fn main() {
     assert_eq!(call_fn_ptr(), 42);
+    assert!(return_fn_ptr() == f);
+    assert!(return_fn_ptr() as unsafe fn() -> i32 == f as fn() -> i32 as unsafe fn() -> i32);
 }