From de80bcbdbfc5645efb1f03e8e8f72f9c4203a301 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 7 Aug 2017 15:39:27 -0700 Subject: [PATCH 1/8] some tests for RangeMap --- .travis.yml | 2 +- src/librustc_mir/Cargo.toml | 1 - src/librustc_mir/interpret/range_map.rs | 66 ++++++++++++++++++++++--- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/.travis.yml b/.travis.yml index 78de5b657edc..ef15fa98d3fc 100644 --- a/.travis.yml +++ b/.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 diff --git a/src/librustc_mir/Cargo.toml b/src/librustc_mir/Cargo.toml index 1a64bb8c7c79..4189f240c582 100644 --- a/src/librustc_mir/Cargo.toml +++ b/src/librustc_mir/Cargo.toml @@ -8,7 +8,6 @@ version = "0.1.0" workspace = "../.." [lib] -test = false path = "lib.rs" [dependencies] diff --git a/src/librustc_mir/interpret/range_map.rs b/src/librustc_mir/interpret/range_map.rs index 7c8debc270d0..e4db9b0e0fc4 100644 --- a/src/librustc_mir/interpret/range_map.rs +++ b/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 { + 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 @@ fn range(offset: u64, len: u64) -> ops::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 @@ pub fn new() -> RangeMap { } fn iter_with_range<'a>(&'a self, offset: u64, len: u64) -> impl Iterator + '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 @@ pub fn iter<'a>(&'a self, offset: u64, len: u64) -> impl Iterator + } 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 @@ pub fn iter_mut_all<'a>(&'a mut self) -> impl Iterator + 'a { pub fn iter_mut_with_gaps<'a>(&'a mut self, offset: u64, len: u64) -> impl Iterator + '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 @@ pub fn iter_mut<'a>(&'a mut self, offset: u64, len: u64) -> impl Iterator(&mut self, mut f: F) } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Query the map at every offset in the range and collect the results. + fn to_vec(map: &RangeMap, offset: u64, len: u64) -> Vec { + (offset..offset+len).into_iter().map(|i| *map.iter(i, 1).next().unwrap()).collect() + } + + #[test] + fn basic_insert() { + let mut map = RangeMap::::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::::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]); + } +} From 11f0aedc3dbff825f1450de59d8f3dc3ae5efc25 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 7 Aug 2017 17:39:09 -0700 Subject: [PATCH 2/8] add some tests making sure we get the alias checking right --- tests/compile-fail/validation_aliasing_mut1.rs | 10 ++++++++++ tests/compile-fail/validation_aliasing_mut2.rs | 10 ++++++++++ tests/compile-fail/validation_aliasing_mut3.rs | 10 ++++++++++ 3 files changed, 30 insertions(+) create mode 100644 tests/compile-fail/validation_aliasing_mut1.rs create mode 100644 tests/compile-fail/validation_aliasing_mut2.rs create mode 100644 tests/compile-fail/validation_aliasing_mut3.rs diff --git a/tests/compile-fail/validation_aliasing_mut1.rs b/tests/compile-fail/validation_aliasing_mut1.rs new file mode 100644 index 000000000000..86aa57447fe6 --- /dev/null +++ b/tests/compile-fail/validation_aliasing_mut1.rs @@ -0,0 +1,10 @@ +#![allow(unused_variables)] + +mod safe { + pub fn safe(x: &mut i32, y: &mut i32) {} //~ ERROR: in conflict with lock WriteLock +} + +fn main() { + let x = &mut 0 as *mut _; + unsafe { safe::safe(&mut *x, &mut *x) }; +} diff --git a/tests/compile-fail/validation_aliasing_mut2.rs b/tests/compile-fail/validation_aliasing_mut2.rs new file mode 100644 index 000000000000..ed7497e5e546 --- /dev/null +++ b/tests/compile-fail/validation_aliasing_mut2.rs @@ -0,0 +1,10 @@ +#![allow(unused_variables)] + +mod safe { + pub fn safe(x: &i32, y: &mut i32) {} //~ ERROR: in conflict with lock ReadLock +} + +fn main() { + let x = &mut 0 as *mut _; + unsafe { safe::safe(&*x, &mut *x) }; +} diff --git a/tests/compile-fail/validation_aliasing_mut3.rs b/tests/compile-fail/validation_aliasing_mut3.rs new file mode 100644 index 000000000000..69fbbc167ca0 --- /dev/null +++ b/tests/compile-fail/validation_aliasing_mut3.rs @@ -0,0 +1,10 @@ +#![allow(unused_variables)] + +mod safe { + pub fn safe(x: &mut i32, y: &i32) {} //~ ERROR: in conflict with lock WriteLock +} + +fn main() { + let x = &mut 0 as *mut _; + unsafe { safe::safe(&mut *x, &*x) }; +} From 668491a89276a9b359b70d1ba44191bf1f1d4604 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 7 Aug 2017 17:40:18 -0700 Subject: [PATCH 3/8] Work on making validation test pass again Turns out that tracking write locks by their lifetime is not precise enough, but for now, we don't have an alternative. Also, we need to force_allocate what we acquire or else the memory will not be in the right state. --- src/librustc_mir/interpret/memory.rs | 104 ++++++++---------- src/librustc_mir/interpret/validation.rs | 83 +++++++------- tests/compile-fail/oom2.rs | 2 + tests/{run-pass => run-pass-fullmir}/catch.rs | 0 tests/run-pass/packed_struct.rs | 3 + 5 files changed, 96 insertions(+), 96 deletions(-) rename tests/{run-pass => run-pass-fullmir}/catch.rs (100%) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index f068bc839d1e..4fe612ac81b6 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/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, + /// Stores for which lifetimes (of the original write lock) we got + /// which suspensions. + suspended: HashMap>, + /// 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, -} - -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub enum Lock { NoLock, WriteLock(DynamicLifetime), @@ -57,7 +52,7 @@ fn default() -> Self { 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, access: AccessKind) -> bool { @@ -513,9 +508,10 @@ pub(crate) fn acquire_lock(&mut self, ptr: MemoryPointer, len: u64, region: Opti } /// 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, suspend: Option) -> EvalResult<'tcx> { assert!(len > 0); let cur_frame = self.cur_frame; @@ -523,7 +519,6 @@ pub(crate) fn release_write_lock(&mut self, ptr: MemoryPointer, len: u64, 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,31 +528,25 @@ pub(crate) fn release_write_lock(&mut self, ptr: MemoryPointer, len: u64, } }; if is_our_lock { + trace!("Releasing {:?} at {:?}", lock.active, lock_lft); // Disable the lock lock.active = NoLock; } 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() }); - } + // 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 { - return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() }); + // Make sure we did not try to release someone else's lock. + if !is_our_lock && lock.active != NoLock { + // FIXME: For the same reason that we have to live with suspensions already existing, + // we also cannot be sure here if things really are going wrong. So accept this for now. + //return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() }); } } } @@ -577,34 +566,33 @@ pub(crate) fn recover_write_lock(&mut self, ptr: MemoryPointer, len: u64, 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 +641,7 @@ pub(crate) fn locks_lifetime_ended(&mut self, ending_region: Option) 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| { diff --git a/src/librustc_mir/interpret/validation.rs b/src/librustc_mir/interpret/validation.rs index b291c639b9ca..edb9c657b491 100644 --- a/src/librustc_mir/interpret/validation.rs +++ b/src/librustc_mir/interpret/validation.rs @@ -18,7 +18,7 @@ pub type ValidationQuery<'tcx> = ValidationOperand<'tcx, Lvalue>; -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq)] enum ValidationMode { Acquire, /// Recover because the given region ended @@ -142,16 +142,15 @@ fn validate_ptr(&mut self, val: Value, pointee_ty: Ty<'tcx>, re: Option, mode: ValidationMode) -> EvalResult<'tcx> { match self.try_validate(query, mode) { - // Releasing an uninitalized variable is a NOP. This is needed because + // ReleaseUntil(None) of 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 !mode.acquiring() { - return Ok(()); - } - res + // release if it is. But of course that can't even always be statically determined. + Err(EvalError{ kind: EvalErrorKind::ReadUndefBytes, ..}) + if mode == ValidationMode::ReleaseUntil(None) + => { + return Ok(()); } res => res, } @@ -212,38 +211,46 @@ fn try_validate(&mut self, mut query: ValidationQuery<'tcx>, mode: ValidationMod TyParam(_) | TyInfer(_) | TyProjection(_) | TyAnon(..) | TyError => bug!("I got an incomplete/unnormalized type for validation"), }; if is_owning { - match query.lval { - Lvalue::Ptr { ptr, extra } => { - // Determine the size - // FIXME: Can we reuse size_and_align_of_dst for Lvalues? - let len = match self.type_size(query.ty)? { - Some(size) => { - assert_eq!(extra, LvalueExtra::None, "Got a fat ptr to a sized type"); - size - } - None => { - // The only unsized typ we concider "owning" is TyStr. - assert_eq!(query.ty.sty, TyStr, "Found a surprising unsized owning type"); - // The extra must be the length, in bytes. - match extra { - LvalueExtra::Length(len) => len, - _ => bug!("TyStr must have a length as extra"), - } - } - }; - // Handle locking - if len > 0 { - let ptr = ptr.to_ptr()?; - let access = match query.mutbl { MutMutable => AccessKind::Write, MutImmutable => AccessKind::Read }; - match mode { - ValidationMode::Acquire => self.memory.acquire_lock(ptr, len, query.re, access)?, - ValidationMode::Recover(ending_ce) => self.memory.recover_write_lock(ptr, len, query.re, ending_ce)?, - ValidationMode::ReleaseUntil(suspended_ce) => self.memory.release_write_lock(ptr, len, query.re, suspended_ce)?, - } + // We need to lock. So we need memory. So we have to force_acquire. + // Tracking the same state for locals not backed by memory would just duplicate too + // much machinery. + // FIXME: We ignore alignment. + let (ptr, extra, _aligned) = self.force_allocation(query.lval)?.to_ptr_extra_aligned(); + // Determine the size + // FIXME: Can we reuse size_and_align_of_dst for Lvalues? + let len = match self.type_size(query.ty)? { + Some(size) => { + assert_eq!(extra, LvalueExtra::None, "Got a fat ptr to a sized type"); + size + } + None => { + // The only unsized typ we concider "owning" is TyStr. + assert_eq!(query.ty.sty, TyStr, "Found a surprising unsized owning type"); + // The extra must be the length, in bytes. + match extra { + LvalueExtra::Length(len) => len, + _ => bug!("TyStr must have a length as extra"), } } - Lvalue::Local { .. } => { - // Not backed by memory, so we have nothing to do. + }; + // Handle locking + if len > 0 { + let ptr = ptr.to_ptr()?; + match query.mutbl { + MutImmutable => + if mode.acquiring() { + self.memory.acquire_lock(ptr, len, query.re, AccessKind::Read)?; + } + // No releasing of read locks, ever. + MutMutable => + match mode { + ValidationMode::Acquire => + self.memory.acquire_lock(ptr, len, query.re, AccessKind::Write)?, + ValidationMode::Recover(ending_ce) => + self.memory.recover_write_lock(ptr, len, query.re, ending_ce)?, + ValidationMode::ReleaseUntil(suspended_ce) => + self.memory.suspend_write_lock(ptr, len, query.re, suspended_ce)?, + } } } } diff --git a/tests/compile-fail/oom2.rs b/tests/compile-fail/oom2.rs index 1a4a47efe685..f439ac8c130e 100644 --- a/tests/compile-fail/oom2.rs +++ b/tests/compile-fail/oom2.rs @@ -1,3 +1,5 @@ +// Validation forces more allocation; disable it. +// compile-flags: -Zmir-emit-validate=0 #![feature(box_syntax, custom_attribute, attr_literals)] #![miri(memory_size=2048)] diff --git a/tests/run-pass/catch.rs b/tests/run-pass-fullmir/catch.rs similarity index 100% rename from tests/run-pass/catch.rs rename to tests/run-pass-fullmir/catch.rs diff --git a/tests/run-pass/packed_struct.rs b/tests/run-pass/packed_struct.rs index 0c4781198282..e0387a5f405f 100644 --- a/tests/run-pass/packed_struct.rs +++ b/tests/run-pass/packed_struct.rs @@ -1,3 +1,6 @@ +// FIXME: We have to disable this, force_allocation fails. +// TODO: I think this can be triggered even without validation. +// compile-flags: -Zmir-emit-validate=0 #![allow(dead_code)] #![feature(unsize, coerce_unsized)] From bff1ad156e6fd9c49e51028c56f2d091cc106495 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 8 Aug 2017 18:37:05 -0700 Subject: [PATCH 4/8] integer-ops needs a rustc patch to work again --- tests/run-pass-fullmir/integer-ops.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/run-pass-fullmir/integer-ops.rs b/tests/run-pass-fullmir/integer-ops.rs index 7f66dbd521f9..44030c3301c7 100644 --- a/tests/run-pass-fullmir/integer-ops.rs +++ b/tests/run-pass-fullmir/integer-ops.rs @@ -8,8 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// FIXME: remove the next line once https://github.com/rust-lang/rust/issues/43359 is fixed -// compile-flags: -Zmir-opt-level=0 +// FIXME: remove -Zmir-opt-level once https://github.com/rust-lang/rust/issues/43359 is fixed +// FIXME: remove -Zmir-emit-validate=0 once https://github.com/rust-lang/rust/pull/43748 is merged +// compile-flags: -Zmir-opt-level=0 -Zmir-emit-validate=0 use std::i32; From 7b5f8a36ab94695e8d9b48fa007d72e586e6b8df Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 8 Aug 2017 18:38:01 -0700 Subject: [PATCH 5/8] try harder to preserve regions when doing inference This is not complete yet, but it is enough to make unsized-tuple-impls work. --- src/librustc_mir/interpret/memory.rs | 3 + src/librustc_mir/interpret/validation.rs | 128 ++++++++++++++++++++--- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 4fe612ac81b6..468b2d71faed 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -531,9 +531,12 @@ pub(crate) fn suspend_write_lock(&mut self, ptr: MemoryPointer, len: u64, 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) => { + 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. diff --git a/src/librustc_mir/interpret/validation.rs b/src/librustc_mir/interpret/validation.rs index edb9c657b491..f23cf52c9bbe 100644 --- a/src/librustc_mir/interpret/validation.rs +++ b/src/librustc_mir/interpret/validation.rs @@ -1,10 +1,11 @@ use rustc::hir::Mutability; use rustc::hir::Mutability::*; use rustc::mir::{self, ValidationOp, ValidationOperand}; -use rustc::ty::{self, Ty, TypeFoldable}; -use rustc::ty::subst::Subst; +use rustc::ty::{self, Ty, TypeFoldable, TyCtxt}; +use rustc::ty::subst::{Substs, Subst}; +use rustc::traits; +use rustc::infer::InferCtxt; use rustc::traits::Reveal; -use rustc::infer::TransNormalize; use rustc::middle::region::CodeExtent; use super::{ @@ -110,6 +111,116 @@ pub(crate) fn end_region(&mut self, ce: CodeExtent) -> EvalResult<'tcx> { Ok(()) } + fn normalize_type_unerased(&self, ty: Ty<'tcx>) -> Ty<'tcx> { + return normalize_associated_type(self.tcx, &ty); + + use syntax::codemap::{Span, DUMMY_SP}; + + // We copy a bunch of stuff from rustc/infer/mod.rs to be able to tweak its behavior + fn normalize_projections_in<'a, 'gcx, 'tcx, T>( + self_: &InferCtxt<'a, 'gcx, 'tcx>, + param_env: ty::ParamEnv<'tcx>, + value: &T) + -> T::Lifted + where T: TypeFoldable<'tcx> + ty::Lift<'gcx> + { + let mut selcx = traits::SelectionContext::new(self_); + let cause = traits::ObligationCause::dummy(); + let traits::Normalized { value: result, obligations } = + traits::normalize(&mut selcx, param_env, cause, value); + + debug!("normalize_projections_in: result={:?} obligations={:?}", + result, obligations); + + let mut fulfill_cx = traits::FulfillmentContext::new(); + + for obligation in obligations { + fulfill_cx.register_predicate_obligation(self_, obligation); + } + + drain_fulfillment_cx_or_panic(self_, DUMMY_SP, &mut fulfill_cx, &result) + } + + fn drain_fulfillment_cx_or_panic<'a, 'gcx, 'tcx, T>( + self_: &InferCtxt<'a, 'gcx, 'tcx>, + span: Span, + fulfill_cx: &mut traits::FulfillmentContext<'tcx>, + result: &T) + -> T::Lifted + where T: TypeFoldable<'tcx> + ty::Lift<'gcx> + { + debug!("drain_fulfillment_cx_or_panic()"); + + // In principle, we only need to do this so long as `result` + // contains unbound type parameters. It could be a slight + // optimization to stop iterating early. + match fulfill_cx.select_all_or_error(self_) { + Ok(()) => { } + Err(errors) => { + span_bug!(span, "Encountered errors `{:?}` resolving bounds after type-checking", + errors); + } + } + + let result = self_.resolve_type_vars_if_possible(result); + let result = self_.tcx.fold_regions(&result, &mut false, |r, _| match *r { ty::ReVar(_) => self_.tcx.types.re_erased, _ => r }); + + match self_.tcx.lift_to_global(&result) { + Some(result) => result, + None => { + span_bug!(span, "Uninferred types/regions in `{:?}`", result); + } + } + } + + trait MyTransNormalize<'gcx>: TypeFoldable<'gcx> { + fn my_trans_normalize<'a, 'tcx>(&self, + infcx: &InferCtxt<'a, 'gcx, 'tcx>, + param_env: ty::ParamEnv<'tcx>) + -> Self; + } + + macro_rules! items { ($($item:item)+) => ($($item)+) } + macro_rules! impl_trans_normalize { + ($lt_gcx:tt, $($ty:ty),+) => { + items!($(impl<$lt_gcx> MyTransNormalize<$lt_gcx> for $ty { + fn my_trans_normalize<'a, 'tcx>(&self, + infcx: &InferCtxt<'a, $lt_gcx, 'tcx>, + param_env: ty::ParamEnv<'tcx>) + -> Self { + normalize_projections_in(infcx, param_env, self) + } + })+); + } + } + + impl_trans_normalize!('gcx, + Ty<'gcx>, + &'gcx Substs<'gcx>, + ty::FnSig<'gcx>, + ty::PolyFnSig<'gcx>, + ty::ClosureSubsts<'gcx>, + ty::PolyTraitRef<'gcx>, + ty::ExistentialTraitRef<'gcx> + ); + + fn normalize_associated_type<'a, 'tcx, T>(self_: TyCtxt<'a, 'tcx, 'tcx>, value: &T) -> T + where T: MyTransNormalize<'tcx> + { + debug!("normalize_associated_type(t={:?})", value); + + let param_env = ty::ParamEnv::empty(Reveal::All); + + if !value.has_projection_types() { + return value.clone(); + } + + self_.infer_ctxt().enter(|infcx| { + value.my_trans_normalize(&infcx, param_env) + }) + } + } + fn validate_variant( &mut self, query: ValidationQuery<'tcx>, @@ -189,14 +300,7 @@ fn try_validate(&mut self, mut query: ValidationQuery<'tcx>, mode: ValidationMod _ => {} } - // This is essentially a copy of normalize_associated_type, but without erasure - if query.ty.has_projection_types() { - let param_env = ty::ParamEnv::empty(Reveal::All); - let old_ty = query.ty; - query.ty = self.tcx.infer_ctxt().enter(move |infcx| { - old_ty.trans_normalize(&infcx, param_env) - }) - } + query.ty = self.normalize_type_unerased(&query.ty); trace!("{:?} on {:?}", mode, query); // Decide whether this type *owns* the memory it covers (like integers), or whether it @@ -215,7 +319,7 @@ fn try_validate(&mut self, mut query: ValidationQuery<'tcx>, mode: ValidationMod // Tracking the same state for locals not backed by memory would just duplicate too // much machinery. // FIXME: We ignore alignment. - let (ptr, extra, _aligned) = self.force_allocation(query.lval)?.to_ptr_extra_aligned(); + let (ptr, extra) = self.force_allocation(query.lval)?.to_ptr_extra_aligned(); // Determine the size // FIXME: Can we reuse size_and_align_of_dst for Lvalues? let len = match self.type_size(query.ty)? { From 5e018b1deb9aa9b485aa6372ee7254c70c119670 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 9 Aug 2017 15:45:09 -0700 Subject: [PATCH 6/8] analyzing hashmap.rs uncovered a deeper problem; disable validation there for now --- src/librustc_mir/interpret/validation.rs | 7 ------- tests/run-pass-fullmir/hashmap.rs | 2 ++ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/interpret/validation.rs b/src/librustc_mir/interpret/validation.rs index f23cf52c9bbe..6a8df1b52467 100644 --- a/src/librustc_mir/interpret/validation.rs +++ b/src/librustc_mir/interpret/validation.rs @@ -129,9 +129,6 @@ fn normalize_projections_in<'a, 'gcx, 'tcx, T>( let traits::Normalized { value: result, obligations } = traits::normalize(&mut selcx, param_env, cause, value); - debug!("normalize_projections_in: result={:?} obligations={:?}", - result, obligations); - let mut fulfill_cx = traits::FulfillmentContext::new(); for obligation in obligations { @@ -149,8 +146,6 @@ fn drain_fulfillment_cx_or_panic<'a, 'gcx, 'tcx, T>( -> T::Lifted where T: TypeFoldable<'tcx> + ty::Lift<'gcx> { - debug!("drain_fulfillment_cx_or_panic()"); - // In principle, we only need to do this so long as `result` // contains unbound type parameters. It could be a slight // optimization to stop iterating early. @@ -207,8 +202,6 @@ fn my_trans_normalize<'a, 'tcx>(&self, fn normalize_associated_type<'a, 'tcx, T>(self_: TyCtxt<'a, 'tcx, 'tcx>, value: &T) -> T where T: MyTransNormalize<'tcx> { - debug!("normalize_associated_type(t={:?})", value); - let param_env = ty::ParamEnv::empty(Reveal::All); if !value.has_projection_types() { diff --git a/tests/run-pass-fullmir/hashmap.rs b/tests/run-pass-fullmir/hashmap.rs index f4a358174f55..892518011dbc 100644 --- a/tests/run-pass-fullmir/hashmap.rs +++ b/tests/run-pass-fullmir/hashmap.rs @@ -1,3 +1,5 @@ +// FIXME: disable validation until we figure out how to handle . +// compile-flags: -Zmir-emit-validate=0 use std::collections::{self, HashMap}; use std::hash::BuildHasherDefault; From 34685044f9d6c01a87a05fde9778586079965958 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 9 Aug 2017 18:01:10 -0700 Subject: [PATCH 7/8] add a bunch of compile-fail tests for validation --- .../compile-fail/validation_aliasing_mut4.rs | 13 +++++++++++ .../validation_buggy_split_at_mut.rs | 22 +++++++++++++++++++ .../compile-fail/validation_illegal_write.rs | 15 +++++++++++++ .../validation_pointer_smuggling.rs | 20 +++++++++++++++++ tests/compile-fail/validation_recover1.rs | 16 ++++++++++++++ tests/compile-fail/validation_recover2.rs | 14 ++++++++++++ tests/compile-fail/validation_recover3.rs | 15 +++++++++++++ 7 files changed, 115 insertions(+) create mode 100644 tests/compile-fail/validation_aliasing_mut4.rs create mode 100644 tests/compile-fail/validation_buggy_split_at_mut.rs create mode 100644 tests/compile-fail/validation_illegal_write.rs create mode 100644 tests/compile-fail/validation_pointer_smuggling.rs create mode 100644 tests/compile-fail/validation_recover1.rs create mode 100644 tests/compile-fail/validation_recover2.rs create mode 100644 tests/compile-fail/validation_recover3.rs diff --git a/tests/compile-fail/validation_aliasing_mut4.rs b/tests/compile-fail/validation_aliasing_mut4.rs new file mode 100644 index 000000000000..3dac55aeaac9 --- /dev/null +++ b/tests/compile-fail/validation_aliasing_mut4.rs @@ -0,0 +1,13 @@ +#![allow(unused_variables)] + +mod safe { + use std::cell::Cell; + + // Make sure &mut UnsafeCell also has a lock to it + pub fn safe(x: &mut Cell, y: &i32) {} //~ ERROR: in conflict with lock WriteLock +} + +fn main() { + let x = &mut 0 as *mut _; + unsafe { safe::safe(&mut *(x as *mut _), &*x) }; +} diff --git a/tests/compile-fail/validation_buggy_split_at_mut.rs b/tests/compile-fail/validation_buggy_split_at_mut.rs new file mode 100644 index 000000000000..9e67b2a4ab18 --- /dev/null +++ b/tests/compile-fail/validation_buggy_split_at_mut.rs @@ -0,0 +1,22 @@ +#![allow(unused_variables)] + +mod safe { + use std::slice::from_raw_parts_mut; + + pub fn split_at_mut(self_: &mut [T], mid: usize) -> (&mut [T], &mut [T]) { + let len = self_.len(); + let ptr = self_.as_mut_ptr(); + + unsafe { + assert!(mid <= len); + + (from_raw_parts_mut(ptr, len - mid), // BUG: should be "mid" instead of "len - mid" + from_raw_parts_mut(ptr.offset(mid as isize), len - mid)) + } + } +} + +fn main() { + let mut array = [1,2,3,4]; + let _x = safe::split_at_mut(&mut array, 0); //~ ERROR: in conflict with lock WriteLock +} diff --git a/tests/compile-fail/validation_illegal_write.rs b/tests/compile-fail/validation_illegal_write.rs new file mode 100644 index 000000000000..1432f4cc9f17 --- /dev/null +++ b/tests/compile-fail/validation_illegal_write.rs @@ -0,0 +1,15 @@ +#![allow(unused_variables)] + +mod safe { + pub(crate) fn safe(x: &u32) { + let x : &mut u32 = unsafe { &mut *(x as *const _ as *mut _) }; + *x = 42; //~ ERROR: in conflict with lock ReadLock + } +} + +fn main() { + let target = &mut 42; + let target_ref = ⌖ + // do a reborrow, but we keep the lock + safe::safe(&*target); +} diff --git a/tests/compile-fail/validation_pointer_smuggling.rs b/tests/compile-fail/validation_pointer_smuggling.rs new file mode 100644 index 000000000000..3320d2a89d35 --- /dev/null +++ b/tests/compile-fail/validation_pointer_smuggling.rs @@ -0,0 +1,20 @@ +#![allow(unused_variables)] + +static mut PTR: *mut u8 = 0 as *mut _; + +fn fun1(x: &mut u8) { + unsafe { + PTR = x; + } +} + +fn fun2() { + // Now we use a pointer we are not allowed to use + let _x = unsafe { *PTR }; //~ ERROR: in conflict with lock WriteLock +} + +fn main() { + let mut val = 0; + fun1(&mut val); + fun2(); +} diff --git a/tests/compile-fail/validation_recover1.rs b/tests/compile-fail/validation_recover1.rs new file mode 100644 index 000000000000..55c38a694c55 --- /dev/null +++ b/tests/compile-fail/validation_recover1.rs @@ -0,0 +1,16 @@ +#![allow(unused_variables)] + +#[repr(u32)] +enum Bool { True } + +mod safe { + pub(crate) fn safe(x: &mut super::Bool) { + let x = x as *mut _ as *mut u32; + unsafe { *x = 44; } // out-of-bounds enum discriminant + } +} + +fn main() { + let mut x = Bool::True; + safe::safe(&mut x); //~ ERROR: invalid enum discriminant +} diff --git a/tests/compile-fail/validation_recover2.rs b/tests/compile-fail/validation_recover2.rs new file mode 100644 index 000000000000..756be9fde6fc --- /dev/null +++ b/tests/compile-fail/validation_recover2.rs @@ -0,0 +1,14 @@ +#![allow(unused_variables)] + +mod safe { + // This makes a ref that was passed to us via &mut alias with things it should not alias with + pub(crate) fn safe(x: &mut &u32, target: &mut u32) { + unsafe { *x = &mut *(target as *mut _); } + } +} + +fn main() { + let target = &mut 42; + let mut target_alias = &42; // initial dummy value + safe::safe(&mut target_alias, target); //~ ERROR: in conflict with lock ReadLock +} diff --git a/tests/compile-fail/validation_recover3.rs b/tests/compile-fail/validation_recover3.rs new file mode 100644 index 000000000000..afe6fe7c0bb9 --- /dev/null +++ b/tests/compile-fail/validation_recover3.rs @@ -0,0 +1,15 @@ +#![allow(unused_variables)] + +mod safe { + pub(crate) fn safe(x: *mut u32) { + unsafe { *x = 42; } //~ ERROR: in conflict with lock WriteLock + } +} + +fn main() { + let target = &mut 42u32; + let target2 = target as *mut _; + drop(&mut *target); // reborrow + // Now make sure we still got the lock + safe::safe(target2); +} From 8e8c9c862c50f7f29e99f9a80bdeb67d99393969 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 9 Aug 2017 22:32:13 -0700 Subject: [PATCH 8/8] turns out we can enable this sanity check now --- src/librustc_mir/interpret/memory.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 468b2d71faed..75acdbe778c3 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -547,9 +547,7 @@ pub(crate) fn suspend_write_lock(&mut self, ptr: MemoryPointer, len: u64, None => { // Make sure we did not try to release someone else's lock. if !is_our_lock && lock.active != NoLock { - // FIXME: For the same reason that we have to live with suspensions already existing, - // we also cannot be sure here if things really are going wrong. So accept this for now. - //return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() }); + return err!(InvalidMemoryLockRelease { ptr, len, frame: cur_frame, lock: lock.active.clone() }); } } }