Auto merge of #820 - RalfJung:intptrcast, r=RalfJung

Make most tests work with Intptrcast

The one that still fails (amusingly, that's ptr_int_casts) requires some help from the librustc_mir side.

Author: bors

README.md

@@ -279,6 +279,8 @@ Several `-Z` flags are relevant for Miri:
 * `-Zalways-encode-mir` makes rustc dump MIR even for completely monomorphic
   functions.  This is needed so that Miri can execute such functions, so Miri
   sets this flag per default.
+* `-Zmir-emit-retag` controls whether `Retag` statements are emitted. Miri
+  enables this per default because it is needed for validation.
 
 Moreover, Miri recognizes some environment variables:
 

src/eval.rs

@@ -32,11 +32,11 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
     let mut ecx = InterpretCx::new(
         tcx.at(syntax::source_map::DUMMY_SP),
         ty::ParamEnv::reveal_all(),
-        Evaluator::new(config.validate),
+        Evaluator::new(),
     );
 
     // FIXME: InterpretCx::new should take an initial MemoryExtra
-    ecx.memory_mut().extra = MemoryExtra::with_rng(config.seed.map(StdRng::seed_from_u64));
+    ecx.memory_mut().extra = MemoryExtra::new(config.seed.map(StdRng::seed_from_u64), config.validate);
 
     let main_instance = ty::Instance::mono(ecx.tcx.tcx, main_id);
     let main_mir = ecx.load_mir(main_instance.def)?;

src/helpers.rs

@@ -58,6 +58,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             .map(|(size, _)| size)
             .unwrap_or_else(|| place.layout.size)
         );
+        assert!(size.bytes() > 0);
         // Store how far we proceeded into the place so far. Everything to the left of
         // this offset has already been handled, in the sense that the frozen parts
         // have had `action` called on them.

src/intptrcast.rs

@@ -4,7 +4,8 @@ use std::cmp::max;
 
 use rand::Rng;
 
-use rustc_mir::interpret::{AllocId, Pointer, InterpResult, Memory, AllocCheck};
+use rustc::ty::layout::HasDataLayout;
+use rustc_mir::interpret::{AllocId, Pointer, InterpResult, Memory, AllocCheck, PointerArithmetic};
 use rustc_target::abi::Size;
 
 use crate::{Evaluator, Tag, STACK_ADDR};
@@ -75,7 +76,9 @@ impl<'mir, 'tcx> GlobalState {
         let mut global_state = memory.extra.intptrcast.borrow_mut();
         let global_state = &mut *global_state;
 
-        let (size, align) = memory.get_size_and_align(ptr.alloc_id, AllocCheck::Live)?;
+        // There is nothing wrong with a raw pointer being cast to an integer only after
+        // it became dangling.  Hence `MaybeDead`.
+        let (size, align) = memory.get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)?;
 
         let base_addr = match global_state.base_addr.entry(ptr.alloc_id) {
             Entry::Occupied(entry) => *entry.get(),
@@ -107,7 +110,9 @@ impl<'mir, 'tcx> GlobalState {
         };
 
         debug_assert_eq!(base_addr % align.bytes(), 0); // sanity check
-        Ok(base_addr + ptr.offset.bytes())
+        // Add offset with the right kind of pointer-overflowing arithmetic.
+        let dl = memory.data_layout();
+        Ok(dl.overflowing_offset(base_addr, ptr.offset.bytes()).0)
     }
 
     /// Shifts `addr` to make it aligned with `align` by rounding `addr` to the smallest multiple

src/machine.rs

@@ -41,25 +41,31 @@ impl Into<MemoryKind<MiriMemoryKind>> for MiriMemoryKind {
 /// Extra per-allocation data
 #[derive(Debug, Clone)]
 pub struct AllocExtra {
-    pub stacked_borrows: stacked_borrows::AllocExtra,
+    /// Stacked Borrows state is only added if validation is enabled.
+    pub stacked_borrows: Option<stacked_borrows::AllocExtra>,
 }
 
 /// Extra global memory data
 #[derive(Default, Clone, Debug)]
 pub struct MemoryExtra {
     pub stacked_borrows: stacked_borrows::MemoryExtra,
     pub intptrcast: intptrcast::MemoryExtra,
+
     /// The random number generator to use if Miri is running in non-deterministic mode and to
     /// enable intptrcast
-    pub(crate) rng: Option<RefCell<StdRng>>
+    pub(crate) rng: Option<RefCell<StdRng>>,
+
+    /// Whether to enforce the validity invariant.
+    pub(crate) validate: bool,
 }
 
 impl MemoryExtra {
-    pub fn with_rng(rng: Option<StdRng>) -> Self {
+    pub fn new(rng: Option<StdRng>, validate: bool) -> Self {
         MemoryExtra {
             stacked_borrows: Default::default(),
             intptrcast: Default::default(),
             rng: rng.map(RefCell::new),
+            validate,
         }
     }
 }
@@ -82,21 +88,17 @@ pub struct Evaluator<'tcx> {
 
     /// TLS state.
     pub(crate) tls: TlsData<'tcx>,
-
-    /// Whether to enforce the validity invariant.
-    pub(crate) validate: bool,
 }
 
 impl<'tcx> Evaluator<'tcx> {
-    pub(crate) fn new(validate: bool) -> Self {
+    pub(crate) fn new() -> Self {
         Evaluator {
             env_vars: HashMap::default(),
             argc: None,
             argv: None,
             cmd_line: None,
             last_error: 0,
             tls: TlsData::default(),
-            validate,
         }
     }
 }
@@ -135,7 +137,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
 
     #[inline(always)]
     fn enforce_validity(ecx: &InterpretCx<'mir, 'tcx, Self>) -> bool {
-        ecx.machine.validate
+        ecx.memory().extra.validate
     }
 
     /// Returns `Ok()` when the function was handled; fail otherwise.
@@ -251,12 +253,17 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     ) -> (Cow<'b, Allocation<Self::PointerTag, Self::AllocExtra>>, Self::PointerTag) {
         let kind = kind.expect("we set our STATIC_KIND so this cannot be None");
         let alloc = alloc.into_owned();
-        let (stacks, base_tag) = Stacks::new_allocation(
-            id,
-            Size::from_bytes(alloc.bytes.len() as u64),
-            Rc::clone(&memory.extra.stacked_borrows),
-            kind,
-        );
+        let (stacks, base_tag) = if !memory.extra.validate {
+            (None, Tag::Untagged)
+        } else {
+            let (stacks, base_tag) = Stacks::new_allocation(
+                id,
+                Size::from_bytes(alloc.bytes.len() as u64),
+                Rc::clone(&memory.extra.stacked_borrows),
+                kind,
+            );
+            (Some(stacks), base_tag)
+        };
         if kind != MiriMemoryKind::Static.into() {
             assert!(alloc.relocations.is_empty(), "Only statics can come initialized with inner pointers");
             // Now we can rely on the inner pointers being static, too.
@@ -268,7 +275,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
                 alloc.relocations.iter()
                     // The allocations in the relocations (pointers stored *inside* this allocation)
                     // all get the base pointer tag.
-                    .map(|&(offset, ((), alloc))| (offset, (memory_extra.static_base_ptr(alloc), alloc)))
+                    .map(|&(offset, ((), alloc))| {
+                        let tag = if !memory.extra.validate {
+                            Tag::Untagged
+                        } else {
+                            memory_extra.static_base_ptr(alloc)
+                        };
+                        (offset, (tag, alloc))
+                    })
                     .collect()
             ),
             undef_mask: alloc.undef_mask,
@@ -286,7 +300,11 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
         id: AllocId,
         memory: &Memory<'mir, 'tcx, Self>,
     ) -> Self::PointerTag {
-        memory.extra.stacked_borrows.borrow_mut().static_base_ptr(id)
+        if !memory.extra.validate {
+            Tag::Untagged
+        } else {
+            memory.extra.stacked_borrows.borrow_mut().static_base_ptr(id)
+        }
     }
 
     #[inline(always)]
@@ -295,12 +313,8 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
         kind: mir::RetagKind,
         place: PlaceTy<'tcx, Tag>,
     ) -> InterpResult<'tcx> {
-        if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !Self::enforce_validity(ecx) {
-            // No tracking, or no retagging. The latter is possible because a dependency of ours
-            // might be called with different flags than we are, so there are `Retag`
-            // statements but we do not want to execute them.
-            // Also, honor the whitelist in `enforce_validity` because otherwise we might retag
-            // uninitialized data.
+        if !Self::enforce_validity(ecx) {
+            // No tracking.
              Ok(())
         } else {
             ecx.retag(kind, place)
@@ -354,7 +368,11 @@ impl AllocationExtra<Tag> for AllocExtra {
         ptr: Pointer<Tag>,
         size: Size,
     ) -> InterpResult<'tcx> {
-        alloc.extra.stacked_borrows.memory_read(ptr, size)
+        if let Some(ref stacked_borrows) = alloc.extra.stacked_borrows {
+            stacked_borrows.memory_read(ptr, size)
+        } else {
+            Ok(())
+        }
     }
 
     #[inline(always)]
@@ -363,7 +381,11 @@ impl AllocationExtra<Tag> for AllocExtra {
         ptr: Pointer<Tag>,
         size: Size,
     ) -> InterpResult<'tcx> {
-        alloc.extra.stacked_borrows.memory_written(ptr, size)
+        if let Some(ref mut stacked_borrows) = alloc.extra.stacked_borrows {
+            stacked_borrows.memory_written(ptr, size)
+        } else {
+            Ok(())
+        }
     }
 
     #[inline(always)]
@@ -372,7 +394,11 @@ impl AllocationExtra<Tag> for AllocExtra {
         ptr: Pointer<Tag>,
         size: Size,
     ) -> InterpResult<'tcx> {
-        alloc.extra.stacked_borrows.memory_deallocated(ptr, size)
+        if let Some(ref mut stacked_borrows) = alloc.extra.stacked_borrows {
+            stacked_borrows.memory_deallocated(ptr, size)
+        } else {
+            Ok(())
+        }
     }
 }
 

src/operator.rs

@@ -56,10 +56,12 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> {
 
         trace!("ptr_op: {:?} {:?} {:?}", *left, bin_op, *right);
 
-        // If intptrcast is enabled and the operation is not an offset
-        // we can force the cast from pointers to integer addresses and
-        // then dispatch to rustc binary operation method
-        if self.memory().extra.rng.is_some() && bin_op != Offset {
+        // If intptrcast is enabled, treat everything of integer *type* at integer *value*.
+        if self.memory().extra.rng.is_some() && left.layout.ty.is_integral() {
+            // This is actually an integer operation, so dispatch back to the core engine.
+            // TODO: Once intptrcast is the default, librustc_mir should never even call us
+            // for integer types.
+            assert!(right.layout.ty.is_integral());
             let l_bits = self.force_bits(left.imm.to_scalar()?, left.layout.size)?;
             let r_bits = self.force_bits(right.imm.to_scalar()?, right.layout.size)?;
 
@@ -186,6 +188,13 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> {
         right: Scalar<Tag>,
     ) -> InterpResult<'tcx, bool> {
         let size = self.pointer_size();
+        if self.memory().extra.rng.is_some() {
+            // Just compare the integers.
+            // TODO: Do we really want to *always* do that, even when comparing two live in-bounds pointers?
+            let left = self.force_bits(left, size)?;
+            let right = self.force_bits(right, size)?;
+            return Ok(left == right);
+        }
         Ok(match (left, right) {
             (Scalar::Raw { .. }, Scalar::Raw { .. }) =>
                 left.to_bits(size)? == right.to_bits(size)?,
@@ -388,21 +397,24 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> {
             .checked_mul(pointee_size)
             .ok_or_else(|| InterpError::Overflow(mir::BinOp::Mul))?;
         // Now let's see what kind of pointer this is.
-        if let Scalar::Ptr(ptr) = ptr {
-            // Both old and new pointer must be in-bounds of a *live* allocation.
-            // (Of the same allocation, but that part is trivial with our representation.)
-            self.pointer_inbounds(ptr)?;
-            let ptr = ptr.signed_offset(offset, self)?;
-            self.pointer_inbounds(ptr)?;
-            Ok(Scalar::Ptr(ptr))
-        } else {
-            // An integer pointer. They can only be offset by 0, and we pretend there
-            // is a little zero-sized allocation here.
-            if offset == 0 {
-                Ok(ptr)
-            } else {
-                err!(InvalidPointerMath)
+        let ptr = if offset == 0 {
+            match ptr {
+                Scalar::Ptr(ptr) => ptr,
+                Scalar::Raw { .. } => {
+                    // Offset 0 on an integer. We accept that, pretending there is
+                    // a little zero-sized allocation here.
+                    return Ok(ptr);
+                }
             }
-        }
+        } else {
+            // Offset > 0. We *require* a pointer.
+            self.force_ptr(ptr)?
+        };
+        // Both old and new pointer must be in-bounds of a *live* allocation.
+        // (Of the same allocation, but that part is trivial with our representation.)
+        self.pointer_inbounds(ptr)?;
+        let ptr = ptr.signed_offset(offset, self)?;
+        self.pointer_inbounds(ptr)?;
+        Ok(Scalar::Ptr(ptr))
     }
 }

src/stacked_borrows.rs

@@ -538,6 +538,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         // Get the allocation. It might not be mutable, so we cannot use `get_mut`.
         let alloc = this.memory().get(ptr.alloc_id)?;
+        let stacked_borrows = alloc.extra.stacked_borrows.as_ref().expect("we should have Stacked Borrows data");
         // Update the stacks.
         // Make sure that raw pointers and mutable shared references are reborrowed "weak":
         // There could be existing unique pointers reborrowed from them that should remain valid!
@@ -553,14 +554,14 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     // We are only ever `SharedReadOnly` inside the frozen bits.
                     let perm = if frozen { Permission::SharedReadOnly } else { Permission::SharedReadWrite };
                     let item = Item { perm, tag: new_tag, protector };
-                    alloc.extra.stacked_borrows.for_each(cur_ptr, size, |stack, global| {
+                    stacked_borrows.for_each(cur_ptr, size, |stack, global| {
                         stack.grant(cur_ptr.tag, item, global)
                     })
                 });
             }
         };
         let item = Item { perm, tag: new_tag, protector };
-        alloc.extra.stacked_borrows.for_each(ptr, size, |stack, global| {
+        stacked_borrows.for_each(ptr, size, |stack, global| {
             stack.grant(ptr.tag, item, global)
         })
     }

tests/compile-fail/ptr_offset_int_plus_int.rs

@@ -1,4 +1,4 @@
-// error-pattern: invalid arithmetic on pointers
+// error-pattern: tried to interpret some bytes as a pointer
 
 fn main() {
     // Can't offset an integer pointer by non-zero offset.

tests/compiletest.rs

@@ -69,7 +69,7 @@ fn compile_fail(path: &str, target: &str, opt: bool) {
     run_tests("compile-fail", path, target, flags);
 }
 
-fn miri_pass(path: &str, target: &str, opt: bool) {
+fn miri_pass(path: &str, target: &str, opt: bool, noseed: bool) {
     let opt_str = if opt { " with optimizations" } else { "" };
     eprintln!("{}", format!(
         "## Running run-pass tests in {} against miri for target {}{}",
@@ -81,6 +81,9 @@ fn miri_pass(path: &str, target: &str, opt: bool) {
     let mut flags = Vec::new();
     if opt {
         flags.push("-Zmir-opt-level=3".to_owned());
+    } else if !noseed {
+        // Run with intptrcast.  Avoid test matrix explosion by doing either this or opt-level=3.
+        flags.push("-Zmiri-seed=".to_owned());
     }
 
     run_tests("ui", path, target, flags);
@@ -104,7 +107,8 @@ fn get_target() -> String {
 }
 
 fn run_pass_miri(opt: bool) {
-    miri_pass("tests/run-pass", &get_target(), opt);
+    miri_pass("tests/run-pass", &get_target(), opt, false);
+    miri_pass("tests/run-pass-noseed", &get_target(), opt, true);
 }
 
 fn compile_fail_miri(opt: bool) {