Merge pull request #286 from RalfJung/mir-validate

Update MIR validation and test it

Author: RalfJung

src/librustc_mir/interpret/eval_context.rs

@@ -37,8 +37,6 @@ pub struct EvalContext<'a, 'tcx: 'a, M: Machine<'tcx>> {
     /// The virtual memory system.
     pub memory: Memory<'a, 'tcx, M>,
 
-    #[allow(dead_code)]
-    // FIXME(@RalfJung): validation branch
     /// Lvalues that were suspended by the validation subsystem, and will be recovered later
     pub(crate) suspended: HashMap<DynamicLifetime, Vec<ValidationQuery<'tcx>>>,
 

src/librustc_mir/interpret/memory.rs

@@ -35,8 +35,6 @@ mod range {
     }
 
     impl MemoryRange {
-        #[allow(dead_code)]
-        // FIXME(@RalfJung): validation branch
         pub fn new(offset: u64, len: u64) -> MemoryRange {
             assert!(len > 0);
             MemoryRange {
@@ -61,8 +59,6 @@ mod range {
             left..right
         }
 
-        #[allow(dead_code)]
-        // FIXME(@RalfJung): validation branch
         pub fn contained_in(&self, offset: u64, len: u64) -> bool {
             assert!(len > 0);
             offset <= self.start && self.end <= (offset + len)
@@ -143,8 +139,6 @@ impl<M> Allocation<M> {
             .filter(move |&(range, _)| range.overlaps(offset, len))
     }
 
-    #[allow(dead_code)]
-    // FIXME(@RalfJung): validation branch
     fn iter_locks_mut<'a>(&'a mut self, offset: u64, len: u64) -> impl Iterator<Item=(&'a MemoryRange, &'a mut LockInfo)> + 'a {
         self.locks.range_mut(MemoryRange::range(offset, len))
             .filter(move |&(range, _)| range.overlaps(offset, len))
@@ -474,8 +468,6 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
             .map_err(|lock| EvalErrorKind::MemoryLockViolation { ptr, len, frame, access, lock }.into())
     }
 
-    #[allow(dead_code)]
-    // FIXME(@RalfJung): validation branch
     /// Acquire the lock for the given lifetime
     pub(crate) fn acquire_lock(&mut self, ptr: MemoryPointer, len: u64, region: Option<CodeExtent>, kind: AccessKind) -> EvalResult<'tcx> {
         use std::collections::btree_map::Entry::*;
@@ -504,8 +496,6 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
         Ok(())
     }
 
-    #[allow(dead_code)]
-    // FIXME(@RalfJung): validation branch
     /// Release a write lock prematurely. If there's a read lock or someone else's lock, fail.
     pub(crate) fn release_write_lock(&mut self, ptr: MemoryPointer, len: u64) -> EvalResult<'tcx> {
         assert!(len > 0);

src/librustc_mir/interpret/step.rs

@@ -133,8 +133,15 @@ impl<'a, 'tcx, M: Machine<'tcx>> EvalContext<'a, 'tcx, M> {
                 self.deallocate_local(old_val)?;
             }
 
-            // NOPs for now.
-            EndRegion(_ce) => {}
+            // Validity checks.
+            Validate(op, ref lvalues) => {
+                for operand in lvalues {
+                    self.validation_op(op, operand)?;
+                }
+            }
+            EndRegion(ce) => {
+                self.end_region(ce)?;
+            }
 
             // Defined to do nothing. These are added by optimization passes, to avoid changing the
             // size of MIR constantly.

src/librustc_mir/interpret/validation.rs

@@ -1,9 +1,6 @@
-// code for @RalfJung's validation branch is dead for now
-#![allow(dead_code)]
-
 use rustc::hir::Mutability;
 use rustc::hir::Mutability::*;
-use rustc::mir;
+use rustc::mir::{self, ValidationOp, ValidationOperand};
 use rustc::ty::{self, Ty, TypeFoldable};
 use rustc::ty::subst::Subst;
 use rustc::traits::Reveal;
@@ -14,28 +11,11 @@ use super::{
     EvalError, EvalResult, EvalErrorKind,
     EvalContext, DynamicLifetime,
     AccessKind, LockInfo,
-    PrimVal, Value,
+    Value,
     Lvalue, LvalueExtra,
     Machine,
 };
 
-// FIXME remove this once it lands in rustc
-#[derive(Copy, Clone, PartialEq, Eq)]
-pub enum ValidationOp {
-    Acquire,
-    Release,
-    Suspend(CodeExtent),
-}
-
-#[derive(Clone, Debug)]
-pub struct ValidationOperand<'tcx, T> {
-    pub lval: T,
-    pub ty: Ty<'tcx>,
-    pub re: Option<CodeExtent>,
-    pub mutbl: Mutability,
-}
-// FIXME end
-
 pub type ValidationQuery<'tcx> = ValidationOperand<'tcx, Lvalue<'tcx>>;
 
 #[derive(Copy, Clone, Debug)]
@@ -59,26 +39,35 @@ impl ValidationMode {
 // Validity checks
 impl<'a, 'tcx, M: Machine<'tcx>> EvalContext<'a, 'tcx, M> {
     pub(crate) fn validation_op(&mut self, op: ValidationOp, operand: &ValidationOperand<'tcx, mir::Lvalue<'tcx>>) -> EvalResult<'tcx> {
+        // If mir-emit-validate is set to 0 (i.e., disabled), we may still see validation commands
+        // because other crates may have been compiled with mir-emit-validate > 0.  Ignore those
+        // commands.  This makes mir-emit-validate also a flag to control whether miri will do
+        // validation or not.
+        if self.tcx.sess.opts.debugging_opts.mir_emit_validate == 0 {
+            return Ok(());
+        }
+
         // HACK: Determine if this method is whitelisted and hence we do not perform any validation.
+        // We currently insta-UB on anything passing around uninitialized memory, so we have to whitelist
+        // the places that are allowed to do that.
+        // The second group is stuff libstd does that is forbidden even under relaxed validation.
         {
             // The regexp we use for filtering
             use regex::Regex;
             lazy_static! {
                 static ref RE: Regex = Regex::new("^(\
-std::mem::swap::|\
-std::mem::uninitialized::|\
-std::ptr::read::|\
-std::panicking::try::do_call::|\
-std::slice::from_raw_parts_mut::|\
-<std::heap::Heap as std::heap::Alloc>::|\
-<std::mem::ManuallyDrop<T>><std::heap::AllocErr>::new$|\
-<std::mem::ManuallyDrop<T> as std::ops::DerefMut><std::heap::AllocErr>::deref_mut$|\
-std::sync::atomic::AtomicBool::get_mut$|\
-<std::boxed::Box<T>><[a-zA-Z0-9_\\[\\]]+>::from_raw|\
-<[a-zA-Z0-9_:<>]+ as std::slice::SliceIndex<[a-zA-Z0-9_\\[\\]]+>><[a-zA-Z0-9_\\[\\]]+>::get_unchecked_mut$|\
-<alloc::raw_vec::RawVec<T, std::heap::Heap>><[a-zA-Z0-9_\\[\\]]+>::into_box$|\
-<std::vec::Vec<T>><[a-zA-Z0-9_\\[\\]]+>::into_boxed_slice$\
-)").unwrap();
+                    std::mem::uninitialized::|\
+                    std::mem::forget::|\
+                    <(std|alloc)::heap::Heap as (std::heap|alloc::allocator)::Alloc>::|\
+                    <std::mem::ManuallyDrop<T>><.*>::new$|\
+                    <std::mem::ManuallyDrop<T> as std::ops::DerefMut><.*>::deref_mut$|\
+                    std::ptr::read::|\
+                    \
+                    <std::sync::Arc<T>><.*>::inner$|\
+                    <std::sync::Arc<T>><.*>::drop_slow$|\
+                    (std::heap|alloc::allocator)::Layout::for_value::|\
+                    std::mem::(size|align)_of_val::\
+                )").unwrap();
             }
             // Now test
             let name = self.stack[self.cur_frame()].instance.to_string();
@@ -167,10 +156,11 @@ std::sync::atomic::AtomicBool::get_mut$|\
     fn validate(&mut self, query: ValidationQuery<'tcx>, mode: ValidationMode) -> EvalResult<'tcx>
     {
         match self.try_validate(query, mode) {
-            // HACK: If, during releasing, we hit memory we cannot use, we just ignore that.
-            // This can happen because releases are added before drop elaboration.
-            // TODO: Fix the MIR so that these releases do not happen.
-            res @ Err(EvalError{ kind: EvalErrorKind::DanglingPointerDeref, ..}) |
+            // Releasing an uninitalized variable is a NOP.  This is needed because
+            // we have to release the return value of a function; due to destination-passing-style
+            // the callee may directly write there.
+            // TODO: Ideally we would know whether the destination is already initialized, and only
+            // release if it is.
             res @ Err(EvalError{ kind: EvalErrorKind::ReadUndefBytes, ..}) => {
                 if let ValidationMode::Release = mode {
                     return Ok(());
@@ -199,18 +189,13 @@ std::sync::atomic::AtomicBool::get_mut$|\
         }
 
         // HACK: For now, bail out if we hit a dead local during recovery (can happen because sometimes we have
-        // StorageDead before EndRegion).
+        // StorageDead before EndRegion due to https://github.com/rust-lang/rust/issues/43481).
         // TODO: We should rather fix the MIR.
-        // HACK: Releasing on dead/undef local variables is a NOP.  This can happen because of releases being added
-        // before drop elaboration.
-        // TODO: Fix the MIR so that these releases do not happen.
         match query.lval {
             Lvalue::Local { frame, local } => {
                 let res = self.stack[frame].get_local(local);
                 match (res, mode) {
-                    (Err(EvalError{ kind: EvalErrorKind::DeadLocal, ..}), ValidationMode::Recover(_)) |
-                    (Err(EvalError{ kind: EvalErrorKind::DeadLocal, ..}), ValidationMode::Release) |
-                    (Ok(Value::ByVal(PrimVal::Undef)), ValidationMode::Release) => {
+                    (Err(EvalError{ kind: EvalErrorKind::DeadLocal, ..}), ValidationMode::Recover(_)) => {
                         return Ok(());
                     }
                     _ => {},

src/librustc_mir/interpret/value.rs

@@ -197,6 +197,7 @@ impl<'a, 'tcx: 'a> Value {
 
             ByValPair(ptr, vtable) => Ok((ptr.into(), vtable.to_ptr()?)),
 
+            ByVal(PrimVal::Undef) => err!(ReadUndefBytes),
             _ => bug!("expected ptr and vtable, got {:?}", self),
         }
     }
@@ -216,6 +217,7 @@ impl<'a, 'tcx: 'a> Value {
                 assert_eq!(len as u64 as u128, len);
                 Ok((ptr.into(), len as u64))
             },
+            ByVal(PrimVal::Undef) => err!(ReadUndefBytes),
             ByVal(_) => bug!("expected ptr and length, got {:?}", self),
         }
     }

tests/compile-fail/cast_box_int_to_fn_ptr.rs

@@ -1,3 +1,6 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmir-emit-validate=0
+
 fn main() {
     let b = Box::new(42);
     let g = unsafe {

tests/compile-fail/cast_int_to_fn_ptr.rs

@@ -1,3 +1,6 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmir-emit-validate=0
+
 fn main() {
     let g = unsafe {
         std::mem::transmute::<usize, fn(i32)>(42)

tests/compile-fail/execute_memory.rs

@@ -1,3 +1,6 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmir-emit-validate=0
+
 #![feature(box_syntax)]
 
 fn main() {

tests/compile-fail/fn_ptr_offset.rs

@@ -1,3 +1,6 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmir-emit-validate=0
+
 use std::mem;
 
 fn f() {}

tests/compile-fail/invalid_enum_discriminant.rs

@@ -1,3 +1,6 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmir-emit-validate=0
+
 #[repr(C)]
 pub enum Foo {
     A, B, C, D

tests/compile-fail/memleak_rc.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 //error-pattern: the evaluated program leaked memory
 
 use std::rc::Rc;

tests/compile-fail/panic.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 //error-pattern: the evaluated program panicked
 
 fn main() {

tests/compile-fail/static_memory_modification2.rs

@@ -1,3 +1,6 @@
+// Validation detects that we are casting & to &mut and so it changes why we fail
+// compile-flags: -Zmir-emit-validate=0
+
 use std::mem::transmute;
 
 #[allow(mutable_transmutes)]

tests/compile-fail/zst2.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 // error-pattern: the evaluated program panicked
 
 #[derive(Debug)]

tests/compile-fail/zst3.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 // error-pattern: the evaluated program panicked
 
 #[derive(Debug)]

tests/compiletest.rs

@@ -20,6 +20,7 @@ fn compile_fail(sysroot: &Path, path: &str, target: &str, host: &str, fullmir: b
     let mut config = compiletest::default_config();
     config.mode = "compile-fail".parse().expect("Invalid mode");
     config.rustc_path = MIRI_PATH.into();
+    let mut flags = Vec::new();
     if fullmir {
         if host != target {
             // skip fullmir on nonhost
@@ -32,6 +33,8 @@ fn compile_fail(sysroot: &Path, path: &str, target: &str, host: &str, fullmir: b
         config.target_rustcflags = Some(format!("--sysroot {}", sysroot.to_str().unwrap()));
         config.src_base = PathBuf::from(path.to_string());
     }
+    flags.push("-Zmir-emit-validate=1".to_owned());
+    config.target_rustcflags = Some(flags.join(" "));
     config.target = target.to_owned();
     compiletest::run_tests(&config);
 }
@@ -72,6 +75,8 @@ fn miri_pass(path: &str, target: &str, host: &str, fullmir: bool, opt: bool) {
         flags.push("-Zmir-opt-level=3".to_owned());
     } else {
         flags.push("-Zmir-opt-level=0".to_owned());
+        // For now, only validate without optimizations.  Inlining breaks validation.
+        flags.push("-Zmir-emit-validate=1".to_owned());
     }
     config.target_rustcflags = Some(flags.join(" "));
     // don't actually execute the final binary, it might be for other targets and we only care

tests/run-pass-fullmir/regions-mock-trans.rs

@@ -8,7 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-// pretty-expanded FIXME #23616
+// FIXME: We handle uninitialzied storage here, which currently makes validation fail.
+// compile-flags: -Zmir-emit-validate=0
 
 #![feature(libc)]
 

tests/run-pass/rc.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 use std::cell::RefCell;
 use std::rc::Rc;
 

tests/run-pass/recursive_static.rs

@@ -1,3 +1,6 @@
+// FIXME: Disable validation until we figure out how to handle recursive statics.
+// compile-flags: -Zmir-emit-validate=0
+
 struct S(&'static S);
 static S1: S = S(&S2);
 static S2: S = S(&S1);

tests/run-pass/send-is-not-static-par-for.rs

@@ -10,6 +10,9 @@
 
 //ignore-windows
 
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 use std::sync::Mutex;
 
 fn par_for<I, F>(iter: I, f: F)

tests/run-pass/std.rs

@@ -1,3 +1,6 @@
+// FIXME: Due to https://github.com/rust-lang/rust/issues/43457 we have to disable validation
+// compile-flags: -Zmir-emit-validate=0
+
 use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 use std::sync::Arc;

xargo/build.sh

@@ -1,3 +1,3 @@
 #!/bin/bash
 cd "$(readlink -e "$(dirname "$0")")"
-RUSTFLAGS='-Zalways-encode-mir' xargo build
+RUSTFLAGS='-Zalways-encode-mir -Zmir-emit-validate=1' xargo build