Merge pull request #297 from RalfJung/mir-validate

Validation update

Author: oli-obk

.travis.yml

@@ -16,7 +16,7 @@ script:
 - |
   # Test plain miri
   cargo build --release --features "cargo_miri" &&
-  cargo test --release &&
+  cargo test --release --all &&
   cargo install --features "cargo_miri"
 - |
   # Test cargo miri

src/librustc_mir/Cargo.toml

@@ -8,7 +8,6 @@ version = "0.1.0"
 workspace = "../.."
 
 [lib]
-test = false
 path = "lib.rs"
 
 [dependencies]

src/librustc_mir/interpret/memory.rs

@@ -29,19 +29,14 @@ pub enum AccessKind {
 /// Information about a lock that is currently held.
 #[derive(Clone, Debug)]
 struct LockInfo {
-    suspended: Vec<SuspendedWriteLock>,
+    /// Stores for which lifetimes (of the original write lock) we got
+    /// which suspensions.
+    suspended: HashMap<DynamicLifetime, Vec<CodeExtent>>,
+    /// The current state of the lock that's actually effective.
     active: Lock,
 }
 
-#[derive(Clone, Debug)]
-struct SuspendedWriteLock  {
-    /// Original lifetime of the lock that is now suspended
-    lft: DynamicLifetime,
-    /// Regions that all have to end to reenable this suspension
-    suspensions: Vec<CodeExtent>,
-}
-
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum Lock {
     NoLock,
     WriteLock(DynamicLifetime),
@@ -57,7 +52,7 @@ impl Default for LockInfo {
 
 impl LockInfo {
     fn new(lock: Lock) -> LockInfo {
-        LockInfo { suspended: Vec::new(), active: lock }
+        LockInfo { suspended: HashMap::new(), active: lock }
     }
 
     fn access_permitted(&self, frame: Option<usize>, access: AccessKind) -> bool {
@@ -513,17 +508,17 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
     }
 
     /// Release or suspend a write lock of the given lifetime prematurely.
-    /// When releasing, if there is no write lock or someone else's write lock, that's an error.
+    /// When releasing, if there is a read lock or someone else's write lock, that's an error.
+    /// We *do* accept relasing a NoLock, as this can happen when a local is first acquired and later force_allocate'd.
     /// When suspending, the same cases are fine; we just register an additional suspension.
-    pub(crate) fn release_write_lock(&mut self, ptr: MemoryPointer, len: u64,
+    pub(crate) fn suspend_write_lock(&mut self, ptr: MemoryPointer, len: u64,
                                      lock_region: Option<CodeExtent>, suspend: Option<CodeExtent>) -> EvalResult<'tcx> {
         assert!(len > 0);
         let cur_frame = self.cur_frame;
         let lock_lft = DynamicLifetime { frame: cur_frame, region: lock_region };
         let alloc = self.get_mut_unchecked(ptr.alloc_id)?;
 
         'locks: for lock in alloc.locks.iter_mut(ptr.offset, len) {
-            trace!("Releasing {:?}", lock);
             let is_our_lock = match lock.active {
                 WriteLock(lft) => {
                     lft == lock_lft
@@ -533,30 +528,25 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
                 }
             };
             if is_our_lock {
+                trace!("Releasing {:?} at {:?}", lock.active, lock_lft);
                 // Disable the lock
                 lock.active = NoLock;
+            } else {
+                trace!("Not touching {:?} at {:?} as its not our lock", lock.active, lock_lft);
             }
             match suspend {
                 Some(suspend_region) => {
-                    if is_our_lock {
-                        // We just released this lock, so add a new suspension
-                        lock.suspended.push(SuspendedWriteLock { lft: lock_lft, suspensions: vec![suspend_region] });
-                    } else {
-                        // Find our lock in the suspended ones
-                        for suspended_lock in lock.suspended.iter_mut().rev() {
-                            if suspended_lock.lft == lock_lft {
-                                // Found it!
-                                suspended_lock.suspensions.push(suspend_region);
-                                continue 'locks;
-                            }
-                        }
-                        // We did not find it.  Someone else had the lock and we have not suspended it, that's just wrong.
-                        return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() });
-                    }
+                    trace!("Adding suspension to {:?} at {:?}", lock.active, lock_lft);
+                    // We just released this lock, so add a new suspension.
+                    // FIXME: Really, if there ever already is a suspension when is_our_lock, or if there is no suspension when !is_our_lock, something is amiss.
+                    // But this model is not good enough yet to prevent that.
+                    lock.suspended.entry(lock_lft)
+                        .or_insert_with(|| Vec::new())
+                        .push(suspend_region);
                 }
                 None => {
-                    // If we do not suspend, make sure we actually released something
-                    if !is_our_lock {
+                    // Make sure we did not try to release someone else's lock.
+                    if !is_our_lock && lock.active != NoLock {
                         return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() });
                     }
                 }
@@ -577,34 +567,33 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
         let alloc = self.get_mut_unchecked(ptr.alloc_id)?;
 
         for lock in alloc.locks.iter_mut(ptr.offset, len) {
-            // If we have a suspension here, it will be the topmost one
-            let (got_the_lock, pop_suspension) = match lock.suspended.last_mut() {
-                None => (true, false),
-                Some(suspended_lock) => {
-                    if suspended_lock.lft == lock_lft {
-                        // That's us!  Remove suspension (it should be in there).  The same suspension can
-                        // occur multiple times (when there are multiple shared borrows of this that have the same
-                        // lifetime); only remove one of them.
-                        let idx = match suspended_lock.suspensions.iter().enumerate().find(|&(_, re)| re == &suspended_region) {
-                            None => // TODO: Can the user trigger this?
-                                bug!("We have this lock suspended, but not for the given region."),
-                            Some((idx, _)) => idx
-                        };
-                        suspended_lock.suspensions.remove(idx);
-                        let got_lock = suspended_lock.suspensions.is_empty();
-                        (got_lock, got_lock)
-                    } else {
-                        // Someone else's suspension up top, we should be able to grab the lock
-                        (true, false)
+            // Check if we have a suspension here
+            let (got_the_lock, remove_suspension) = match lock.suspended.get_mut(&lock_lft) {
+                None => {
+                    trace!("No suspension around, we can just acquire");
+                    (true, false)
+                }
+                Some(suspensions) => {
+                    trace!("Found suspension of {:?}, removing it", lock_lft);
+                    // That's us!  Remove suspension (it should be in there).  The same suspension can
+                    // occur multiple times (when there are multiple shared borrows of this that have the same
+                    // lifetime); only remove one of them.
+                    let idx = match suspensions.iter().enumerate().find(|&(_, re)| re == &suspended_region) {
+                        None => // TODO: Can the user trigger this?
+                            bug!("We have this lock suspended, but not for the given region."),
+                        Some((idx, _)) => idx
+                    };
+                    suspensions.remove(idx);
+                    let got_lock = suspensions.is_empty();
+                    if got_lock {
+                        trace!("All suspensions are gone, we can have the lock again");
                     }
+                    (got_lock, got_lock)
                 }
             };
-            if pop_suspension { // with NLL; we could do that up in the match above...
-                lock.suspended.pop();
-            } else {
-                // Sanity check: Our lock should not be in the suspension list
-                let found = lock.suspended.iter().find(|suspended_lock| suspended_lock.lft == lock_lft);
-                assert!(found.is_none());
+            if remove_suspension { // with NLL, we could do that up in the match above...
+                assert!(got_the_lock);
+                lock.suspended.remove(&lock_lft);
             }
             if got_the_lock {
                 match lock.active {
@@ -653,7 +642,7 @@ impl<'a, 'tcx, M: Machine<'tcx>> Memory<'a, 'tcx, M> {
                     lock.active = NoLock;
                 }
                 // Also clean up suspended write locks
-                lock.suspended.retain(|suspended_lock| !has_ended(&suspended_lock.lft));
+                lock.suspended.retain(|lft, _suspensions| !has_ended(lft));
             }
             // Clean up the map
             alloc.locks.retain(|lock| {

src/librustc_mir/interpret/range_map.rs

@@ -1,4 +1,9 @@
-//! Implements a map from disjoint non-empty integer ranges to data associated with those ranges
+//! Implements a map from integer indices to data.
+//! Rather than storing data for every index, internally, this maps entire ranges to the data.
+//! To this end, the APIs all work on ranges, not on individual integers. Ranges are split as
+//! necessary (e.g. when [0,5) is first associated with X, and then [1,2) is mutated).
+//! Users must not depend on whether a range is coalesced or not, even though this is observable
+//! via the iteration APIs.
 use std::collections::{BTreeMap};
 use std::ops;
 
@@ -21,6 +26,7 @@ struct Range {
 
 impl Range {
     fn range(offset: u64, len: u64) -> ops::Range<Range> {
+        assert!(len > 0);
         // We select all elements that are within
         // the range given by the offset into the allocation and the length.
         // This is sound if all ranges that intersect with the argument range, are in the
@@ -36,6 +42,7 @@ impl Range {
         left..right
     }
 
+    /// Tests if all of [offset, offset+len) are contained in this range.
     fn overlaps(&self, offset: u64, len: u64) -> bool {
         assert!(len > 0);
         offset < self.end && offset+len >= self.start
@@ -48,6 +55,7 @@ impl<T> RangeMap<T> {
     }
 
     fn iter_with_range<'a>(&'a self, offset: u64, len: u64) -> impl Iterator<Item=(&'a Range, &'a T)> + 'a {
+        assert!(len > 0);
         self.map.range(Range::range(offset, len))
             .filter_map(move |(range, data)| {
                 if range.overlaps(offset, len) {
@@ -63,7 +71,7 @@ impl<T> RangeMap<T> {
     }
 
     fn split_entry_at(&mut self, offset: u64) where T: Clone {
-        let range = match self.iter_with_range(offset, 0).next() {
+        let range = match self.iter_with_range(offset, 1).next() {
             Some((&range, _)) => range,
             None => return,
         };
@@ -88,6 +96,7 @@ impl<T> RangeMap<T> {
     pub fn iter_mut_with_gaps<'a>(&'a mut self, offset: u64, len: u64) -> impl Iterator<Item=&'a mut T> + 'a
         where T: Clone
     {
+        assert!(len > 0);
         // Preparation: Split first and last entry as needed.
         self.split_entry_at(offset);
         self.split_entry_at(offset+len);
@@ -112,14 +121,15 @@ impl<T> RangeMap<T> {
     {
         // Do a first iteration to collect the gaps
         let mut gaps = Vec::new();
-        let mut last_end = None;
+        let mut last_end = offset;
         for (range, _) in self.iter_with_range(offset, len) {
-            if let Some(last_end) = last_end {
-                if last_end < range.start {
-                    gaps.push(Range { start: last_end, end: range.start });
-                }
+            if last_end < range.start {
+                gaps.push(Range { start: last_end, end: range.start });
             }
-            last_end = Some(range.end);
+            last_end = range.end;
+        }
+        if last_end < offset+len {
+            gaps.push(Range { start: last_end, end: offset+len });
         }
 
         // Add default for all gaps
@@ -147,3 +157,43 @@ impl<T> RangeMap<T> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// Query the map at every offset in the range and collect the results.
+    fn to_vec<T: Copy>(map: &RangeMap<T>, offset: u64, len: u64) -> Vec<T> {
+        (offset..offset+len).into_iter().map(|i| *map.iter(i, 1).next().unwrap()).collect()
+    }
+
+    #[test]
+    fn basic_insert() {
+        let mut map = RangeMap::<i32>::new();
+        // Insert
+        for x in map.iter_mut(10, 1) {
+            *x = 42;
+        }
+        // Check
+        assert_eq!(to_vec(&map, 10, 1), vec![42]);
+    }
+
+    #[test]
+    fn gaps() {
+        let mut map = RangeMap::<i32>::new();
+        for x in map.iter_mut(11, 1) {
+            *x = 42;
+        }
+        for x in map.iter_mut(15, 1) {
+            *x = 42;
+        }
+
+        // Now request a range that needs three gaps filled
+        for x in map.iter_mut(10, 10) {
+            if *x != 42 { *x = 23; }
+        }
+
+        assert_eq!(to_vec(&map, 10, 10), vec![23, 42, 23, 23, 23, 42, 23, 23, 23, 23]);
+        assert_eq!(to_vec(&map, 13, 5), vec![23, 23, 42, 23, 23]);
+    }
+}