From fba55ba2a9f9d906dc584e2c8dc6a97a8c137b92 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 24 Oct 2018 17:16:41 +0200 Subject: [PATCH 1/9] make the -Z flags we use more consistent --- appveyor.yml | 9 +++++---- src/bin/cargo-miri.rs | 2 +- src/bin/miri-rustc-tests.rs | 4 +--- src/bin/miri.rs | 18 ++++++++++++------ src/lib.rs | 14 ++++++++++++++ xargo/build.sh | 3 ++- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index cf578120c9..1e9ccbdf95 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -14,19 +14,20 @@ branches: - master install: - # install Rust + # Install Rust. - set PATH=C:\Program Files\Git\mingw64\bin;C:\msys64\mingw%MSYS2_BITS%\bin;%PATH% - set /p RUST_TOOLCHAIN= = std::env::args().collect(); - let sysroot_flag = String::from("--sysroot"); - if !args.contains(&sysroot_flag) { - args.push(sysroot_flag); - args.push(find_sysroot()); - } - + // Parse our own -Z flags and remove them before rustc gets their hand on them. let mut validate = true; args.retain(|arg| { match arg.as_str() { @@ -229,7 +226,16 @@ fn main() { } }); + // Determine sysroot and let rustc know about it + let sysroot_flag = String::from("--sysroot"); + if !args.contains(&sysroot_flag) { + args.push(sysroot_flag); + args.push(find_sysroot()); + } + // Finally, add the default flags all the way in the beginning. + miri::add_miri_default_args(&mut args); + trace!("rustc arguments: {:?}", args); let result = rustc_driver::run(move || { rustc_driver::run_compiler(&args, Box::new(MiriCompilerCalls { default: Box::new(RustcDefaultCalls), diff --git a/src/lib.rs b/src/lib.rs index 650998c3e1..2de6665186 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,6 +50,20 @@ use stacked_borrows::{EvalContextExt as StackedBorEvalContextExt}; // Used by priroda pub use stacked_borrows::{Borrow, Stacks, Mut as MutBorrow}; +/// Insert rustc arguments at the beginning of the argument listthat miri wants to be +/// set per default, for maximal validation power. +pub fn add_miri_default_args(args: &mut Vec) { + // The flags here should be kept in sync with what bootstrap adds when `test-miri` is + // set, which happens in `bootstrap/bin/rustc.rs` in the rustc sources; and also + // kept in sync with `xargo/build.sh` in this repo and `appveyor.yml`. + + // Inserting at index 1, after the binary name + args.splice(1..1, + ["-Zalways-encode-mir", "-Zmir-emit-retag", "-Zmir-opt-level=0"] + .iter().map(|s| s.to_string()) + ); +} + // Used by priroda pub fn create_ecx<'a, 'mir: 'a, 'tcx: 'mir>( tcx: TyCtxt<'a, 'tcx, 'tcx>, diff --git a/xargo/build.sh b/xargo/build.sh index 15a7c77091..25c56d31ab 100755 --- a/xargo/build.sh +++ b/xargo/build.sh @@ -1,3 +1,4 @@ #!/bin/sh cd "$(dirname "$0")" -RUSTFLAGS='-Zalways-encode-mir -Zmir-emit-validate=1' xargo build +# The flags here should be kept in sync with `add_miri_default_args` in `src/lib.rs`. +RUSTFLAGS='-Zalways-encode-mir -Zmir-emit-retag -Zmir-opt-level=0' xargo build From 7ac0e79ad5bcd8d7e4f8317c15238d5911faf11c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 24 Oct 2018 17:17:44 +0200 Subject: [PATCH 2/9] stub Retag hook; fix tests for removal of -Zmir-emit-validate --- src/lib.rs | 16 +++++++++++++++- src/stacked_borrows.rs | 17 ++++++++++++++++- tests/compile-fail-fullmir/stack_free.rs | 2 +- tests/compile-fail/cast_box_int_to_fn_ptr.rs | 2 +- tests/compile-fail/cast_int_to_fn_ptr.rs | 2 +- tests/compile-fail/execute_memory.rs | 2 +- tests/compile-fail/fn_ptr_offset.rs | 2 +- .../{invalid_bool2.rs => invalid_bool.rs} | 2 +- .../{invalid_char2.rs => invalid_char.rs} | 2 +- ...iminant2.rs => invalid_enum_discriminant.rs} | 2 +- tests/compile-fail/never_say_never.rs | 2 +- tests/compile-fail/never_transmute_humans.rs | 2 +- tests/compile-fail/never_transmute_void.rs | 2 +- tests/compile-fail/panic.rs | 2 -- tests/compile-fail/reference_to_packed.rs | 2 +- .../static_memory_modification.rs | 8 ++++++++ .../compile-fail/static_memory_modification.rs | 2 +- .../compile-fail/static_memory_modification2.rs | 2 +- .../compile-fail/static_memory_modification3.rs | 2 +- tests/compile-fail/storage_dead_dangling.rs | 2 +- tests/compile-fail/transmute_fat.rs | 2 +- tests/compile-fail/unaligned_ptr_cast.rs | 2 +- tests/compile-fail/unaligned_ptr_cast2.rs | 2 +- tests/compile-fail/undefined_byte_read.rs | 3 --- tests/compile-fail/validity/execute_memory.rs | 8 ++++++++ tests/compile-fail/validity/fn_ptr_offset.rs | 10 ++++++++++ tests/compiletest.rs | 7 +------ tests/run-pass/move-undef-primval.rs | 3 --- tests/run-pass/packed_struct.rs | 1 - tests/run-pass/recursive_static.rs | 3 --- tests/run-pass/regions-mock-trans.rs | 3 --- tests/run-pass/slice-of-zero-size-elements.rs | 2 -- 32 files changed, 77 insertions(+), 44 deletions(-) rename tests/compile-fail/{invalid_bool2.rs => invalid_bool.rs} (73%) rename tests/compile-fail/{invalid_char2.rs => invalid_char.rs} (80%) rename tests/compile-fail/{invalid_enum_discriminant2.rs => invalid_enum_discriminant.rs} (79%) create mode 100644 tests/compile-fail/stacked_borrows/static_memory_modification.rs create mode 100644 tests/compile-fail/validity/execute_memory.rs create mode 100644 tests/compile-fail/validity/fn_ptr_offset.rs diff --git a/src/lib.rs b/src/lib.rs index 2de6665186..5d20078082 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,7 +26,7 @@ use syntax::attr; pub use rustc_mir::interpret::*; -pub use rustc_mir::interpret::{self, AllocMap}; // resolve ambiguity +pub use rustc_mir::interpret::{self, AllocMap, PlaceTy}; // resolve ambiguity mod fn_call; mod operator; @@ -521,4 +521,18 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { Ok(Pointer::new_with_tag(ptr.alloc_id, ptr.offset, tag)) } } + + #[inline(always)] + fn retag( + ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, + fn_entry: bool, + place: PlaceTy<'tcx, Borrow>, + ) -> EvalResult<'tcx> { + if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !ecx.machine.validate { + // No tracking, or no retagging. This is possible because a dependency of ours might be + // called with different flags than we are, + return Ok(()) + } + ecx.retag(fn_entry, place) + } } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index a569ed4e55..22216345b6 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -5,7 +5,7 @@ use rustc::hir; use super::{ MemoryAccess, MemoryKind, MiriMemoryKind, RangeMap, EvalResult, AllocId, - Pointer, + Pointer, PlaceTy, }; pub type Timestamp = u64; @@ -362,6 +362,12 @@ pub trait EvalContextExt<'tcx> { id: AllocId, kind: MemoryKind, ) -> Borrow; + + fn retag( + &mut self, + fn_entry: bool, + place: PlaceTy<'tcx, Borrow> + ) -> EvalResult<'tcx>; } impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, 'tcx> { @@ -506,4 +512,13 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' alloc.extra.first_borrow(mut_borrow, size); Borrow::Mut(mut_borrow) } + + fn retag( + &mut self, + _fn_entry: bool, + _place: PlaceTy<'tcx, Borrow> + ) -> EvalResult<'tcx> { + // TODO do something + Ok(()) + } } diff --git a/tests/compile-fail-fullmir/stack_free.rs b/tests/compile-fail-fullmir/stack_free.rs index 6d853e75fb..a33bca1267 100644 --- a/tests/compile-fail-fullmir/stack_free.rs +++ b/tests/compile-fail-fullmir/stack_free.rs @@ -1,5 +1,5 @@ // Validation changes why we fail -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation // error-pattern: tried to deallocate Stack memory but gave Machine(Rust) as the kind diff --git a/tests/compile-fail/cast_box_int_to_fn_ptr.rs b/tests/compile-fail/cast_box_int_to_fn_ptr.rs index c3b1fa5958..7ee3bc6276 100644 --- a/tests/compile-fail/cast_box_int_to_fn_ptr.rs +++ b/tests/compile-fail/cast_box_int_to_fn_ptr.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { let b = Box::new(42); diff --git a/tests/compile-fail/cast_int_to_fn_ptr.rs b/tests/compile-fail/cast_int_to_fn_ptr.rs index 1971ce1557..207af4ae2c 100644 --- a/tests/compile-fail/cast_int_to_fn_ptr.rs +++ b/tests/compile-fail/cast_int_to_fn_ptr.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { let g = unsafe { diff --git a/tests/compile-fail/execute_memory.rs b/tests/compile-fail/execute_memory.rs index d859e9072e..295756ef0f 100644 --- a/tests/compile-fail/execute_memory.rs +++ b/tests/compile-fail/execute_memory.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation #![feature(box_syntax)] diff --git a/tests/compile-fail/fn_ptr_offset.rs b/tests/compile-fail/fn_ptr_offset.rs index cccb21790d..9d29316fe2 100644 --- a/tests/compile-fail/fn_ptr_offset.rs +++ b/tests/compile-fail/fn_ptr_offset.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation use std::mem; diff --git a/tests/compile-fail/invalid_bool2.rs b/tests/compile-fail/invalid_bool.rs similarity index 73% rename from tests/compile-fail/invalid_bool2.rs rename to tests/compile-fail/invalid_bool.rs index 2348c62559..e80dc15efa 100644 --- a/tests/compile-fail/invalid_bool2.rs +++ b/tests/compile-fail/invalid_bool.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { let b = unsafe { std::mem::transmute::(2) }; diff --git a/tests/compile-fail/invalid_char2.rs b/tests/compile-fail/invalid_char.rs similarity index 80% rename from tests/compile-fail/invalid_char2.rs rename to tests/compile-fail/invalid_char.rs index 5de2d073f3..b67ed9ba52 100644 --- a/tests/compile-fail/invalid_char2.rs +++ b/tests/compile-fail/invalid_char.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { assert!(std::char::from_u32(-1_i32 as u32).is_none()); diff --git a/tests/compile-fail/invalid_enum_discriminant2.rs b/tests/compile-fail/invalid_enum_discriminant.rs similarity index 79% rename from tests/compile-fail/invalid_enum_discriminant2.rs rename to tests/compile-fail/invalid_enum_discriminant.rs index ea94081693..bd5cb55b6c 100644 --- a/tests/compile-fail/invalid_enum_discriminant2.rs +++ b/tests/compile-fail/invalid_enum_discriminant.rs @@ -1,5 +1,5 @@ // Validation makes this fail in the wrong place -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation // error-pattern: invalid enum discriminant diff --git a/tests/compile-fail/never_say_never.rs b/tests/compile-fail/never_say_never.rs index 634488489b..d7e6a8c09f 100644 --- a/tests/compile-fail/never_say_never.rs +++ b/tests/compile-fail/never_say_never.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation #![feature(never_type)] #![allow(unreachable_code)] diff --git a/tests/compile-fail/never_transmute_humans.rs b/tests/compile-fail/never_transmute_humans.rs index c5c53d4231..169e861be0 100644 --- a/tests/compile-fail/never_transmute_humans.rs +++ b/tests/compile-fail/never_transmute_humans.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation #![feature(never_type)] #![allow(unreachable_code)] diff --git a/tests/compile-fail/never_transmute_void.rs b/tests/compile-fail/never_transmute_void.rs index 5620b6559c..9c0165fed2 100644 --- a/tests/compile-fail/never_transmute_void.rs +++ b/tests/compile-fail/never_transmute_void.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation #![feature(never_type)] #![allow(unreachable_code)] diff --git a/tests/compile-fail/panic.rs b/tests/compile-fail/panic.rs index 80149eeffa..0d594f9bd4 100644 --- a/tests/compile-fail/panic.rs +++ b/tests/compile-fail/panic.rs @@ -1,5 +1,3 @@ -// FIXME: Something in panic handling fails validation with full-MIR -// compile-flags: -Zmir-emit-validate=0 //error-pattern: the evaluated program panicked fn main() { diff --git a/tests/compile-fail/reference_to_packed.rs b/tests/compile-fail/reference_to_packed.rs index 14a2afc33f..befe96f2b3 100644 --- a/tests/compile-fail/reference_to_packed.rs +++ b/tests/compile-fail/reference_to_packed.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation #![allow(dead_code, unused_variables)] diff --git a/tests/compile-fail/stacked_borrows/static_memory_modification.rs b/tests/compile-fail/stacked_borrows/static_memory_modification.rs new file mode 100644 index 0000000000..c092cbfe50 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/static_memory_modification.rs @@ -0,0 +1,8 @@ +static X: usize = 5; + +#[allow(mutable_transmutes)] +fn main() { + let _x = unsafe { + std::mem::transmute::<&usize, &mut usize>(&X) //~ ERROR mutable reference with frozen tag + }; +} diff --git a/tests/compile-fail/static_memory_modification.rs b/tests/compile-fail/static_memory_modification.rs index c758926458..07a277a16f 100644 --- a/tests/compile-fail/static_memory_modification.rs +++ b/tests/compile-fail/static_memory_modification.rs @@ -1,5 +1,5 @@ // Validation detects that we are casting & to &mut and so it changes why we fail -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation static X: usize = 5; diff --git a/tests/compile-fail/static_memory_modification2.rs b/tests/compile-fail/static_memory_modification2.rs index c9857b2059..73928f533c 100644 --- a/tests/compile-fail/static_memory_modification2.rs +++ b/tests/compile-fail/static_memory_modification2.rs @@ -1,5 +1,5 @@ // Validation detects that we are casting & to &mut and so it changes why we fail -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation use std::mem::transmute; diff --git a/tests/compile-fail/static_memory_modification3.rs b/tests/compile-fail/static_memory_modification3.rs index 41a6278729..280f34a5a0 100644 --- a/tests/compile-fail/static_memory_modification3.rs +++ b/tests/compile-fail/static_memory_modification3.rs @@ -1,5 +1,5 @@ // Validation detects that we are casting & to &mut and so it changes why we fail -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation use std::mem::transmute; diff --git a/tests/compile-fail/storage_dead_dangling.rs b/tests/compile-fail/storage_dead_dangling.rs index 69917dce85..85c76f6f41 100644 --- a/tests/compile-fail/storage_dead_dangling.rs +++ b/tests/compile-fail/storage_dead_dangling.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation static mut LEAK: usize = 0; diff --git a/tests/compile-fail/transmute_fat.rs b/tests/compile-fail/transmute_fat.rs index e1f916910d..ede0486be4 100644 --- a/tests/compile-fail/transmute_fat.rs +++ b/tests/compile-fail/transmute_fat.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 +// compile-flags: -Zmiri-disable-validation fn main() { #[cfg(target_pointer_width="64")] diff --git a/tests/compile-fail/unaligned_ptr_cast.rs b/tests/compile-fail/unaligned_ptr_cast.rs index 47317afd36..e64594d3e7 100644 --- a/tests/compile-fail/unaligned_ptr_cast.rs +++ b/tests/compile-fail/unaligned_ptr_cast.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { let x = &2u16; diff --git a/tests/compile-fail/unaligned_ptr_cast2.rs b/tests/compile-fail/unaligned_ptr_cast2.rs index d146f21dfe..1112f2f33c 100644 --- a/tests/compile-fail/unaligned_ptr_cast2.rs +++ b/tests/compile-fail/unaligned_ptr_cast2.rs @@ -1,5 +1,5 @@ // This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation +// compile-flags: -Zmiri-disable-validation fn main() { let x = &2u16; diff --git a/tests/compile-fail/undefined_byte_read.rs b/tests/compile-fail/undefined_byte_read.rs index 1f09293614..fca749ef9c 100644 --- a/tests/compile-fail/undefined_byte_read.rs +++ b/tests/compile-fail/undefined_byte_read.rs @@ -1,6 +1,3 @@ -// This should fail even without validation -// compile-flags: -Zmir-emit-validate=0 - fn main() { let v: Vec = Vec::with_capacity(10); let undef = unsafe { *v.get_unchecked(5) }; diff --git a/tests/compile-fail/validity/execute_memory.rs b/tests/compile-fail/validity/execute_memory.rs new file mode 100644 index 0000000000..426a51faf8 --- /dev/null +++ b/tests/compile-fail/validity/execute_memory.rs @@ -0,0 +1,8 @@ +#![feature(box_syntax)] + +fn main() { + let x = box 42; + unsafe { + let _f = std::mem::transmute::, fn()>(x); //~ ERROR encountered a pointer, but expected a function pointer + } +} diff --git a/tests/compile-fail/validity/fn_ptr_offset.rs b/tests/compile-fail/validity/fn_ptr_offset.rs new file mode 100644 index 0000000000..4989f4d3af --- /dev/null +++ b/tests/compile-fail/validity/fn_ptr_offset.rs @@ -0,0 +1,10 @@ +use std::mem; + +fn f() {} + +fn main() { + let x : fn() = f; + let y : *mut u8 = unsafe { mem::transmute(x) }; + let y = y.wrapping_offset(1); + let _x : fn() = unsafe { mem::transmute(y) }; //~ ERROR encountered a potentially NULL pointer +} diff --git a/tests/compiletest.rs b/tests/compiletest.rs index 0b2058597e..98e3fde54e 100644 --- a/tests/compiletest.rs +++ b/tests/compiletest.rs @@ -58,13 +58,11 @@ fn compile_fail(sysroot: &Path, path: &str, target: &str, host: &str, need_fullm let mut flags = Vec::new(); flags.push(format!("--sysroot {}", sysroot.display())); flags.push("-Dwarnings -Dunused".to_owned()); // overwrite the -Aunused in compiletest-rs - flags.push("-Zmir-emit-validate=1".to_owned()); if opt { // Optimizing too aggressivley makes UB detection harder, but test at least // the default value. + // FIXME: Opt level 3 ICEs during stack trace generation. flags.push("-Zmir-opt-level=1".to_owned()); - } else { - flags.push("-Zmir-opt-level=0".to_owned()); } let mut config = compiletest::Config::default().tempdir(); @@ -102,11 +100,8 @@ fn miri_pass(sysroot: &Path, path: &str, target: &str, host: &str, need_fullmir: let mut flags = Vec::new(); flags.push(format!("--sysroot {}", sysroot.display())); flags.push("-Dwarnings -Dunused".to_owned()); // overwrite the -Aunused in compiletest-rs - flags.push("-Zmir-emit-validate=1".to_owned()); if opt { flags.push("-Zmir-opt-level=3".to_owned()); - } else { - flags.push("-Zmir-opt-level=0".to_owned()); } let mut config = compiletest::Config::default().tempdir(); diff --git a/tests/run-pass/move-undef-primval.rs b/tests/run-pass/move-undef-primval.rs index 2c18c2d368..73c33943a6 100644 --- a/tests/run-pass/move-undef-primval.rs +++ b/tests/run-pass/move-undef-primval.rs @@ -1,6 +1,3 @@ -// Moving around undef is not allowed by validation -// compile-flags: -Zmir-emit-validate=0 - struct Foo { _inner: i32, } diff --git a/tests/run-pass/packed_struct.rs b/tests/run-pass/packed_struct.rs index e10781e656..303e90742f 100644 --- a/tests/run-pass/packed_struct.rs +++ b/tests/run-pass/packed_struct.rs @@ -1,4 +1,3 @@ -// compile-flags: -Zmir-emit-validate=0 #![allow(dead_code)] #![feature(unsize, coerce_unsized)] diff --git a/tests/run-pass/recursive_static.rs b/tests/run-pass/recursive_static.rs index d259ca6361..77f2902917 100644 --- a/tests/run-pass/recursive_static.rs +++ b/tests/run-pass/recursive_static.rs @@ -1,6 +1,3 @@ -// 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); diff --git a/tests/run-pass/regions-mock-trans.rs b/tests/run-pass/regions-mock-trans.rs index 039175e9ac..130eaa288e 100644 --- a/tests/run-pass/regions-mock-trans.rs +++ b/tests/run-pass/regions-mock-trans.rs @@ -8,9 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// FIXME: We handle uninitialized storage here, which currently makes validation fail. -// compile-flags: -Zmir-emit-validate=0 - //ignore-windows: Uses POSIX APIs #![feature(libc)] diff --git a/tests/run-pass/slice-of-zero-size-elements.rs b/tests/run-pass/slice-of-zero-size-elements.rs index dbe8ec9add..aa9d117d72 100644 --- a/tests/run-pass/slice-of-zero-size-elements.rs +++ b/tests/run-pass/slice-of-zero-size-elements.rs @@ -8,8 +8,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// compile-flags: -C debug-assertions - use std::slice; fn foo(v: &[T]) -> Option<&[T]> { From bba3c49e844895cdc32dc6ea1acbf0c6555beefd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Oct 2018 11:31:20 +0200 Subject: [PATCH 3/9] basic retagging (no fn_entry); this also makes us catch more bugs even with optimizations and we can finally stop mutating the state on deref --- src/lib.rs | 4 +- src/stacked_borrows.rs | 76 +++++++++---------- .../stacked_borrows/alias_through_mutation.rs | 3 - .../stacked_borrows/buggy_as_mut_slice.rs | 3 - .../stacked_borrows/buggy_split_at_mut.rs | 3 - .../stacked_borrows/illegal_write2.rs | 3 - .../stacked_borrows/illegal_write4.rs | 34 ++------- .../stacked_borrows/shared_confusion.rs | 10 ++- .../stacked_borrows/shared_confusion_opt.rs | 25 ++++++ 9 files changed, 81 insertions(+), 80 deletions(-) create mode 100644 tests/compile-fail/stacked_borrows/shared_confusion_opt.rs diff --git a/src/lib.rs b/src/lib.rs index 5d20078082..e1ae805fb1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -528,9 +528,11 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { fn_entry: bool, place: PlaceTy<'tcx, Borrow>, ) -> EvalResult<'tcx> { - if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !ecx.machine.validate { + if !ecx.tcx.sess.opts.debugging_opts.mir_emit_retag || !Self::enforce_validity(ecx) { // No tracking, or no retagging. This is possible because a dependency of ours might be // called with different flags than we are, + // Also, honor the whitelist in `enforce_validity` because otherwise we might retag + // uninitialized data. return Ok(()) } ecx.retag(fn_entry, place) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 22216345b6..cb8c2a3e5e 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -1,6 +1,6 @@ -use std::cell::{Cell, RefCell}; +use std::cell::RefCell; -use rustc::ty::{Ty, layout::Size}; +use rustc::ty::{self, Ty, layout::Size}; use rustc::hir; use super::{ @@ -101,12 +101,12 @@ impl From> for RefKind { /// Extra global machine state #[derive(Clone, Debug)] pub struct State { - clock: Cell + clock: Timestamp } impl State { pub fn new() -> State { - State { clock: Cell::new(0) } + State { clock: 0 } } } @@ -129,6 +129,7 @@ impl Default for Stack { /// Extra per-allocation state #[derive(Clone, Debug, Default)] pub struct Stacks { + // Even reading memory can have effects on the stack, so we need a `RefCell` here. stacks: RefCell>, } @@ -249,9 +250,9 @@ impl<'tcx> Stack { } impl State { - fn increment_clock(&self) -> Timestamp { - let val = self.clock.get(); - self.clock.set(val + 1); + fn increment_clock(&mut self) -> Timestamp { + let val = self.clock; + self.clock = val + 1; val } } @@ -334,14 +335,8 @@ impl<'tcx> Stacks { } pub trait EvalContextExt<'tcx> { - fn tag_for_pointee( - &self, - pointee_ty: Ty<'tcx>, - ref_kind: RefKind, - ) -> Borrow; - fn tag_reference( - &self, + &mut self, ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, @@ -371,13 +366,16 @@ pub trait EvalContextExt<'tcx> { } impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, 'tcx> { - fn tag_for_pointee( - &self, + /// Called for place-to-value conversion. + fn tag_reference( + &mut self, + ptr: Pointer, pointee_ty: Ty<'tcx>, + size: Size, ref_kind: RefKind, - ) -> Borrow { + ) -> EvalResult<'tcx, Borrow> { let time = self.machine.stacked_borrows.increment_clock(); - match ref_kind { + let new_bor = match ref_kind { RefKind::Mut => Borrow::Mut(Mut::Uniq(time)), RefKind::Shr => // FIXME This does not do enough checking when only part of the data has @@ -390,18 +388,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' Borrow::Mut(Mut::Raw) }, RefKind::Raw => Borrow::Mut(Mut::Raw), - } - } - - /// Called for place-to-value conversion. - fn tag_reference( - &self, - ptr: Pointer, - pointee_ty: Ty<'tcx>, - size: Size, - ref_kind: RefKind, - ) -> EvalResult<'tcx, Borrow> { - let new_bor = self.tag_for_pointee(pointee_ty, ref_kind); + }; trace!("tag_reference: Creating new reference ({:?}) for {:?} (pointee {}, size {}): {:?}", ref_kind, ptr, pointee_ty, size.bytes(), new_bor); @@ -424,7 +411,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' fn tag_dereference( &self, ptr: Pointer, - pointee_ty: Ty<'tcx>, + _pointee_ty: Ty<'tcx>, size: Size, ref_kind: RefKind, ) -> EvalResult<'tcx, Borrow> { @@ -454,13 +441,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // also using `var`, and that would be okay. } (RefKind::Shr, Borrow::Mut(Mut::Uniq(_))) => { - // A mut got transmuted to shr. High time we freeze this location! - // Make this a delayed reborrow. Redundant reborows to shr are okay, - // so we do not have to be worried about doing too much. - // FIXME: Reconsider if we really want to mutate things while doing just a deref, - // which, in particular, validation does. - trace!("tag_dereference: Lazy freezing of {:?}", ptr); - return self.tag_reference(ptr, pointee_ty, size, ref_kind); + // A mut got transmuted to shr. The mut borrow must be reactivatable. } (RefKind::Mut, Borrow::Frz(_)) => { // This is just invalid. @@ -516,9 +497,24 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' fn retag( &mut self, _fn_entry: bool, - _place: PlaceTy<'tcx, Borrow> + place: PlaceTy<'tcx, Borrow> ) -> EvalResult<'tcx> { - // TODO do something + // For now, we only retag if the toplevel type is a reference. + // TODO: Recurse into structs and enums, sharing code with validation. + let mutbl = match place.layout.ty.sty { + ty::Ref(_, _, mutbl) => mutbl, // go ahead + _ => return Ok(()), // don't do a thing + }; + // We want to reborrow the reference stored there. This will call the hooks + // above. First deref. + // (This is somewhat redundant because validation already did the same thing, + // but what can you do.) + let val = self.read_value(self.place_to_op(place)?)?; + let dest = self.ref_to_mplace(val)?; + // Now put a new ref into the old place. + // FIXME: Honor `fn_entry`! + let val = self.create_ref(dest, Some(mutbl))?; + self.write_value(val, place)?; Ok(()) } } diff --git a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs index 7d56f30b3e..eb8966f3a5 100644 --- a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs +++ b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs @@ -1,6 +1,3 @@ -// With optimizations, we just store a raw in `x`, and there is no problem. -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] // This makes a ref that was passed to us via &mut alias with things it should not alias with diff --git a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs index dc1e3cc81e..e86eb9ba6d 100644 --- a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs +++ b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs @@ -1,6 +1,3 @@ -// FIXME: Without retagging, optimization kills finding this problem -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] mod safe { diff --git a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs index a697ba9167..a4f5f536b7 100644 --- a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs +++ b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs @@ -1,6 +1,3 @@ -// FIXME: Without retagging, optimization kills finding this problem -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] mod safe { diff --git a/tests/compile-fail/stacked_borrows/illegal_write2.rs b/tests/compile-fail/stacked_borrows/illegal_write2.rs index ac9c3397f5..f4fefaad5e 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write2.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write2.rs @@ -1,6 +1,3 @@ -// The reborow gets optimized away, so we can only detect this issue without optimizations -// compile-flags: -Zmir-opt-level=0 - #![allow(unused_variables)] fn main() { diff --git a/tests/compile-fail/stacked_borrows/illegal_write4.rs b/tests/compile-fail/stacked_borrows/illegal_write4.rs index 094a389519..12deb518b4 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write4.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write4.rs @@ -1,31 +1,13 @@ -// The compiler inserts some reborrows, enable optimizations to -// get rid of them. -// compile-flags: -Zmir-opt-level=1 - use std::mem; -// This is an example of a piece of code that intuitively seems like we might -// want to reject it, but that doesn't turn out to be possible. - fn main() { - let target = 42; - // Make sure a cannot use a raw-tagged `&mut` pointing to a frozen location, not - // even to create a raw. - let reference = ⌖ // freeze + let mut target = 42; + // Make sure we cannot use a raw-tagged `&mut` pointing to a frozen location. + // Even just creating it unfreezes. + let raw = &mut target as *mut _; // let this leak to raw + let reference = unsafe { &*raw }; // freeze let ptr = reference as *const _ as *mut i32; // raw ptr, with raw tag - let mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag - // Now we have an &mut to a frozen location, but that is completely normal: - // We'd just unfreeze the location if we used it. - let bad_ptr = mut_ref as *mut i32; // even just creating this is like a use of `mut_ref`. - // That violates the location being frozen! However, we do not properly detect this: - // We first see a `&mut` with a `Raw` tag being deref'd for a frozen location, - // which can happen legitimately if the compiler optimized away an `&mut*` that - // turns a raw into a `&mut`. Next, we create a raw ref to a frozen location - // from a `Raw` tag, which can happen legitimately when interior mutability - // is involved. - let _val = *reference; // Make sure it is still frozen. - - // We only actually unfreeze once we muteate through the bad pointer. - unsafe { *bad_ptr = 42 }; //~ ERROR does not exist on the stack - let _val = *reference; + let _mut_ref: &mut i32 = unsafe { mem::transmute(ptr) }; // &mut, with raw tag + // Now we retag, making our ref top-of-stack -- and, in particular, unfreezing. + let _val = *reference; //~ ERROR Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/shared_confusion.rs b/tests/compile-fail/stacked_borrows/shared_confusion.rs index e3e4c4da77..c02822ed4d 100644 --- a/tests/compile-fail/stacked_borrows/shared_confusion.rs +++ b/tests/compile-fail/stacked_borrows/shared_confusion.rs @@ -1,3 +1,11 @@ +// Optimization kills all the reborrows, enough to make this error go away. There are +// no retags either because we don't retag immediately after a `&[mut]`; we rely on +// that creating a fresh reference. +// See `shared_confusion_opt.rs` for a variant that is caught even with optimizations. +// Keep this test to make sure that without optimizations, we do not have to actually +// use the `x_inner_shr`. +// compile-flags: -Zmir-opt-level=0 + #![allow(unused_variables)] use std::cell::RefCell; @@ -9,7 +17,7 @@ fn test(r: &mut RefCell) { { let x_inner_shr = &*x_inner; // frozen let y = &*r; // outer ref, not freezing - let x_inner_shr2 = &*x_inner; // freezing again + let x_inner_shr = &*x_inner; // freezing again } // Our old raw should be dead by now unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack diff --git a/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs new file mode 100644 index 0000000000..4a88d4d641 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs @@ -0,0 +1,25 @@ +// A variant of `shared_confusion.rs` that gets flagged even with optimizations. + +#![allow(unused_variables)] +use std::cell::RefCell; + +fn test(r: &mut RefCell) { + let x = &*r; // not freezing because interior mutability + let mut x_ref = x.borrow_mut(); + let x_inner : &mut i32 = &mut *x_ref; // Uniq reference + let x_evil = x_inner as *mut _; + { + let x_inner_shr = &*x_inner; // frozen + let _val = *x_inner_shr; + let y = &*r; // outer ref, not freezing + let x_inner_shr = &*x_inner; // freezing again + let _val = *x_inner_shr; + } + // Our old raw should be dead by now + unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack + *x_inner = 12; //~ ERROR Mut reference with non-reactivatable tag +} + +fn main() { + test(&mut RefCell::new(0)); +} From 85f821d7e9c1f920e6584821ffa0dbccded8f657 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 29 Oct 2018 19:48:43 +0100 Subject: [PATCH 4/9] unify checks on memory access and reborrowing, and update for Machine trait change --- src/lib.rs | 17 ++- src/stacked_borrows.rs | 121 +++++++++++------- .../stacked_borrows/illegal_read1.rs | 15 +++ .../stacked_borrows/illegal_read2.rs | 18 +++ .../stacked_borrows/illegal_write5.rs | 15 +++ .../stacked_borrows/load_invalid_shr.rs | 2 +- .../stacked_borrows/pass_invalid_shr.rs | 2 +- .../stacked_borrows/return_invalid_shr.rs | 2 +- 8 files changed, 137 insertions(+), 55 deletions(-) create mode 100644 tests/compile-fail/stacked_borrows/illegal_read1.rs create mode 100644 tests/compile-fail/stacked_borrows/illegal_read2.rs create mode 100644 tests/compile-fail/stacked_borrows/illegal_write5.rs diff --git a/src/lib.rs b/src/lib.rs index e1ae805fb1..b397e9a6d2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -452,21 +452,30 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { } #[inline(always)] - fn memory_accessed( + fn memory_read( alloc: &Allocation, ptr: Pointer, size: Size, - access: MemoryAccess, ) -> EvalResult<'tcx> { - alloc.extra.memory_accessed(ptr, size, access) + alloc.extra.memory_read(ptr, size) + } + + #[inline(always)] + fn memory_written( + alloc: &mut Allocation, + ptr: Pointer, + size: Size, + ) -> EvalResult<'tcx> { + alloc.extra.memory_written(ptr, size) } #[inline(always)] fn memory_deallocated( alloc: &mut Allocation, ptr: Pointer, + size: Size, ) -> EvalResult<'tcx> { - alloc.extra.memory_deallocated(ptr) + alloc.extra.memory_deallocated(ptr, size) } #[inline(always)] diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index cb8c2a3e5e..baeb04b44e 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -4,7 +4,7 @@ use rustc::ty::{self, Ty, layout::Size}; use rustc::hir; use super::{ - MemoryAccess, MemoryKind, MiriMemoryKind, RangeMap, EvalResult, AllocId, + MemoryKind, MiriMemoryKind, RangeMap, EvalResult, AllocId, Pointer, PlaceTy, }; @@ -126,6 +126,13 @@ impl Default for Stack { } } +impl Stack { + #[inline(always)] + fn is_frozen(&self) -> bool { + self.frozen_since.is_some() + } +} + /// Extra per-allocation state #[derive(Clone, Debug, Default)] pub struct Stacks { @@ -206,8 +213,13 @@ impl<'tcx> Stack { match action { None => {}, // nothing to do Some(top) => { + if self.frozen_since.is_some() { + trace!("reactivate: Unfreezing"); + } self.frozen_since = None; - self.borrows.truncate(top+1); + for itm in self.borrows.drain(top+1..).rev() { + trace!("reactivate: Popping {:?}", itm); + } } } @@ -215,7 +227,9 @@ impl<'tcx> Stack { } /// Initiate `bor`; mostly this means freezing or pushing. - fn initiate(&mut self, bor: Borrow) -> EvalResult<'tcx> { + /// This operation cannot fail; it is up to the caller to ensure that the precondition + /// is met: We cannot push onto frozen stacks. + fn initiate(&mut self, bor: Borrow) { match bor { Borrow::Frz(t) => { match self.frozen_since { @@ -241,11 +255,10 @@ impl<'tcx> Stack { // from it is fine with this as well. trace!("initiate: Initiating a raw on a frozen location, not doing a thing"), Some(_) => - return err!(MachineError(format!("Trying to mutate frozen location"))) + bug!("Trying to mutate frozen location") } } } - Ok(()) } } @@ -259,49 +272,27 @@ impl State { /// Higher-level operations impl<'tcx> Stacks { - pub fn memory_accessed( - &self, - ptr: Pointer, - size: Size, - access: MemoryAccess, - ) -> EvalResult<'tcx> { - trace!("memory_accessed({:?}) with tag {:?}: {:?}, size {}", access, ptr.tag, ptr, size.bytes()); - let mut stacks = self.stacks.borrow_mut(); - for stack in stacks.iter_mut(ptr.offset, size) { - // FIXME: Compare this with what the blog post says. - stack.reactivate(ptr.tag, /*force_mut*/access == MemoryAccess::Write)?; - } - Ok(()) - } - - pub fn memory_deallocated( - &mut self, - ptr: Pointer, - ) -> EvalResult<'tcx> { - trace!("memory_deallocated with tag {:?}: {:?}", ptr.tag, ptr); - let stacks = self.stacks.get_mut(); - for stack in stacks.iter_mut_all() { - // This is like mutating. - stack.reactivate(ptr.tag, /*force_mut*/true)?; - } - Ok(()) - } - - fn reborrow( + /// The single most operation: Make sure that using `ptr` as `ref_kind` is okay, + /// and if `new_bor` is present then make that the new current borrow. + fn use_and_maybe_re_borrow( &self, ptr: Pointer, size: Size, - new_bor: Borrow, - permit_redundant: bool, + ref_kind: RefKind, + new_bor: Option, ) -> EvalResult<'tcx> { + trace!("use_and_maybe_re_borrow of tag {:?} as {:?}, new {:?}: {:?}, size {}", + ptr.tag, ref_kind, new_bor, ptr, size.bytes()); let mut stacks = self.stacks.borrow_mut(); for stack in stacks.iter_mut(ptr.offset, size) { - if permit_redundant && stack.check(new_bor) { - // The new borrow is already active! This can happen when creating multiple - // shared references from the same mutable reference. Do nothing. - trace!("reborrow: New borrow {:?} is already active, not doing a thing", new_bor); + if ref_kind == RefKind::Shr && stack.is_frozen() { + // Location already frozen. We don't want to unfreeze, but make sure + // the ref makes some sense. + if let Err(err) = stack.reactivatable(ptr.tag, /*force_mut*/false) { + return err!(MachineError(err)); + } } else { - // If we are creating a uniq ref, we certainly want to unfreeze. + // If we are creating a mutable ref, we certainly want to unfreeze. // Even if we are doing so from a raw. // Notice that if this is a local, whenever we access it directly the // tag here will be the bottommost `Uniq` for that local. That `Uniq` @@ -312,14 +303,47 @@ impl<'tcx> Stacks { // `reset` which the blog post [1] says to perform when accessing a local. // // [1] https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html - stack.reactivate(ptr.tag, /*force_mut*/new_bor.is_uniq())?; - stack.initiate(new_bor)?; + let force_mut = ref_kind == RefKind::Mut; + stack.reactivate(ptr.tag, force_mut)?; + } + if let Some(new_bor) = new_bor { + stack.initiate(new_bor); } } Ok(()) } + #[inline(always)] + pub fn memory_read( + &self, + ptr: Pointer, + size: Size, + ) -> EvalResult<'tcx> { + // Reads behave exactly like the first half of a reborrow-to-shr + self.use_and_maybe_re_borrow(ptr, size, RefKind::Shr, None) + } + + #[inline(always)] + pub fn memory_written( + &mut self, + ptr: Pointer, + size: Size, + ) -> EvalResult<'tcx> { + // Writes behave exactly like the first half of a reborrow-to-mut + self.use_and_maybe_re_borrow(ptr, size, RefKind::Mut, None) + } + + pub fn memory_deallocated( + &mut self, + ptr: Pointer, + size: Size, + ) -> EvalResult<'tcx> { + // This is like mutating + self.use_and_maybe_re_borrow(ptr, size, RefKind::Mut, None) + // FIXME: Error out of there are any barriers? + } + /// Pushes the first borrow to the stacks, must be a mutable one. pub fn first_borrow( &mut self, @@ -398,8 +422,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // Update the stacks. We cannot use `get_mut` becuse this might be immutable // memory. let alloc = self.memory().get(ptr.alloc_id).expect("We checked that the ptr is fine!"); - let permit_redundant = ref_kind == RefKind::Shr; // redundant shared refs are okay - alloc.extra.reborrow(ptr, size, new_bor, permit_redundant)?; + alloc.extra.use_and_maybe_re_borrow(ptr, size, ref_kind, Some(new_bor))?; Ok(new_bor) } @@ -411,10 +434,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' fn tag_dereference( &self, ptr: Pointer, - _pointee_ty: Ty<'tcx>, + pointee_ty: Ty<'tcx>, size: Size, ref_kind: RefKind, ) -> EvalResult<'tcx, Borrow> { + trace!("tag_reference: Accessing reference ({:?}) for {:?} (pointee {}, size {})", + ref_kind, ptr, pointee_ty, size.bytes()); // In principle we should not have to do anything here. However, with transmutes involved, // it can happen that the tag of `ptr` does not actually match `ref_kind`, and we // should adjust for that. @@ -506,12 +531,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' _ => return Ok(()), // don't do a thing }; // We want to reborrow the reference stored there. This will call the hooks - // above. First deref. + // above. First deref, which will call `tag_dereference`. // (This is somewhat redundant because validation already did the same thing, // but what can you do.) let val = self.read_value(self.place_to_op(place)?)?; let dest = self.ref_to_mplace(val)?; - // Now put a new ref into the old place. + // Now put a new ref into the old place, which will call `tag_reference`. // FIXME: Honor `fn_entry`! let val = self.create_ref(dest, Some(mutbl))?; self.write_value(val, place)?; diff --git a/tests/compile-fail/stacked_borrows/illegal_read1.rs b/tests/compile-fail/stacked_borrows/illegal_read1.rs new file mode 100644 index 0000000000..f0e696c457 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/illegal_read1.rs @@ -0,0 +1,15 @@ +// A callee may not read the destination of our `&mut` without +// us noticing. + +fn main() { + let mut x = 15; + let xraw = &mut x as *mut _; + let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... + callee(xraw); + let _val = *xref; // ...but any use of raw will invalidate our ref. + //~^ ERROR: Mut reference with non-reactivatable tag +} + +fn callee(xraw: *mut i32) { + let _val = unsafe { *xraw }; +} diff --git a/tests/compile-fail/stacked_borrows/illegal_read2.rs b/tests/compile-fail/stacked_borrows/illegal_read2.rs new file mode 100644 index 0000000000..ec59c57d31 --- /dev/null +++ b/tests/compile-fail/stacked_borrows/illegal_read2.rs @@ -0,0 +1,18 @@ +// A callee may not read the destination of our `&mut` without +// us noticing. + +fn main() { + let mut x = 15; + let xraw = &mut x as *mut _; + let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... + callee(xraw); + let _val = *xref; // ...but any use of raw will invalidate our ref. + //~^ ERROR: Mut reference with non-reactivatable tag +} + +fn callee(xraw: *mut i32) { + // We are a bit sneaky: We first create a shared ref, exploiting the reborrowing rules, + // and then we read through that. + let shr = unsafe { &*xraw }; + let _val = *shr; +} diff --git a/tests/compile-fail/stacked_borrows/illegal_write5.rs b/tests/compile-fail/stacked_borrows/illegal_write5.rs new file mode 100644 index 0000000000..4b5d08c03a --- /dev/null +++ b/tests/compile-fail/stacked_borrows/illegal_write5.rs @@ -0,0 +1,15 @@ +// A callee may not write to the destination of our `&mut` without +// us noticing. + +fn main() { + let mut x = 15; + let xraw = &mut x as *mut _; + let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... + callee(xraw); + let _val = *xref; // ...but any use of raw will invalidate our ref. + //~^ ERROR: Mut reference with non-reactivatable tag +} + +fn callee(xraw: *mut i32) { + unsafe { *xraw = 15 }; +} diff --git a/tests/compile-fail/stacked_borrows/load_invalid_shr.rs b/tests/compile-fail/stacked_borrows/load_invalid_shr.rs index 785a15c470..b4b180e350 100644 --- a/tests/compile-fail/stacked_borrows/load_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/load_invalid_shr.rs @@ -4,6 +4,6 @@ fn main() { let xraw = x as *mut _; let xref = unsafe { &*xraw }; let xref_in_mem = Box::new(xref); - let _val = *x; // invalidate xraw + *x = 42; // invalidate xraw let _val = *xref_in_mem; //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs b/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs index 8b7a846d84..477c86f606 100644 --- a/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs @@ -5,6 +5,6 @@ fn main() { let x = &mut 42; let xraw = &*x as *const _; let xref = unsafe { &*xraw }; - let _val = *x; // invalidate xraw + *x = 42; // invalidate xraw foo(xref); //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/return_invalid_shr.rs b/tests/compile-fail/stacked_borrows/return_invalid_shr.rs index 89c94127b0..954b5ec8e5 100644 --- a/tests/compile-fail/stacked_borrows/return_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/return_invalid_shr.rs @@ -2,7 +2,7 @@ fn foo(x: &mut (i32, i32)) -> &i32 { let xraw = x as *mut (i32, i32); let ret = unsafe { &(*xraw).1 }; - let _val = *x; // invalidate xraw and its children + x.1 = 42; // invalidate xraw on the 2nd field ret //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen } From 430e047a6f2364f9c6d7d950816eef09f3db0428 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 29 Oct 2018 21:04:56 +0100 Subject: [PATCH 5/9] start collecting some things ALLOWED by stacked borrows in a run-pass test --- .../run-pass/deref_partially_dangling_raw.rs | 9 ------- tests/run-pass/stacked-borrows.rs | 27 +++++++++++++++++++ 2 files changed, 27 insertions(+), 9 deletions(-) delete mode 100644 tests/run-pass/deref_partially_dangling_raw.rs create mode 100644 tests/run-pass/stacked-borrows.rs diff --git a/tests/run-pass/deref_partially_dangling_raw.rs b/tests/run-pass/deref_partially_dangling_raw.rs deleted file mode 100644 index 8639a28936..0000000000 --- a/tests/run-pass/deref_partially_dangling_raw.rs +++ /dev/null @@ -1,9 +0,0 @@ -// Deref a raw ptr to access a field of a large struct, where the field -// is allocated but not the entire struct is. -// For now, we want to allow this. - -fn main() { - let x = (1, 1); - let xptr = &x as *const _ as *const (i32, i32, i32); - let _val = unsafe { (*xptr).1 }; -} diff --git a/tests/run-pass/stacked-borrows.rs b/tests/run-pass/stacked-borrows.rs new file mode 100644 index 0000000000..cde6968a26 --- /dev/null +++ b/tests/run-pass/stacked-borrows.rs @@ -0,0 +1,27 @@ +// Test various stacked-borrows-related things. +fn main() { + deref_partially_dangling_raw(); + read_does_not_invalidate(); +} + +// Deref a raw ptr to access a field of a large struct, where the field +// is allocated but not the entire struct is. +// For now, we want to allow this. +fn deref_partially_dangling_raw() { + let x = (1, 1); + let xptr = &x as *const _ as *const (i32, i32, i32); + let _val = unsafe { (*xptr).1 }; +} + +// Make sure that reading from an `&mut` does, like reborrowing to `&`, +// NOT invalidate other reborrows. +fn read_does_not_invalidate() { + fn foo(x: &mut (i32, i32)) -> &i32 { + let xraw = x as *mut (i32, i32); + let ret = unsafe { &(*xraw).1 }; + let _val = x.1; // we just read, this does NOT invalidate the reborrows. + ret + } + + foo(&mut (1, 2)); +} From 3302656247438d89936bbbfd55fe696c82fa98e4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 30 Oct 2018 13:56:19 +0100 Subject: [PATCH 6/9] More extensive slice and vec tests Not all of them pass validation... --- tests/run-pass/slice-of-zero-size-elements.rs | 56 ------ tests/run-pass/slices.rs | 178 ++++++++++++++++++ tests/run-pass/vecs.rs | 38 ++++ 3 files changed, 216 insertions(+), 56 deletions(-) delete mode 100644 tests/run-pass/slice-of-zero-size-elements.rs create mode 100644 tests/run-pass/slices.rs diff --git a/tests/run-pass/slice-of-zero-size-elements.rs b/tests/run-pass/slice-of-zero-size-elements.rs deleted file mode 100644 index aa9d117d72..0000000000 --- a/tests/run-pass/slice-of-zero-size-elements.rs +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright 2015 The Rust Project Developers. See the COPYRIGHT -// file at the top-level directory of this distribution and at -// http://rust-lang.org/COPYRIGHT. -// -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -use std::slice; - -fn foo(v: &[T]) -> Option<&[T]> { - let mut it = v.iter(); - for _ in 0..5 { - let _ = it.next(); - } - Some(it.as_slice()) -} - -fn foo_mut(v: &mut [T]) -> Option<&mut [T]> { - let mut it = v.iter_mut(); - for _ in 0..5 { - let _ = it.next(); - } - Some(it.into_slice()) -} - -pub fn main() { - // In a slice of zero-size elements the pointer is meaningless. - // Ensure iteration still works even if the pointer is at the end of the address space. - let slice: &[()] = unsafe { slice::from_raw_parts(-5isize as *const (), 10) }; - assert_eq!(slice.len(), 10); - assert_eq!(slice.iter().count(), 10); - - // .nth() on the iterator should also behave correctly - let mut it = slice.iter(); - assert!(it.nth(5).is_some()); - assert_eq!(it.count(), 4); - - // Converting Iter to a slice should never have a null pointer - assert!(foo(slice).is_some()); - - // Test mutable iterators as well - let slice: &mut [()] = unsafe { slice::from_raw_parts_mut(-5isize as *mut (), 10) }; - assert_eq!(slice.len(), 10); - assert_eq!(slice.iter_mut().count(), 10); - - { - let mut it = slice.iter_mut(); - assert!(it.nth(5).is_some()); - assert_eq!(it.count(), 4); - } - - assert!(foo_mut(slice).is_some()) -} diff --git a/tests/run-pass/slices.rs b/tests/run-pass/slices.rs new file mode 100644 index 0000000000..119c9b90a0 --- /dev/null +++ b/tests/run-pass/slices.rs @@ -0,0 +1,178 @@ +// FIXME: Still investigating whether there is UB here +// compile-flags: -Zmiri-disable-validation + +use std::slice; + +fn slice_of_zst() { + fn foo(v: &[T]) -> Option<&[T]> { + let mut it = v.iter(); + for _ in 0..5 { + let _ = it.next(); + } + Some(it.as_slice()) + } + + fn foo_mut(v: &mut [T]) -> Option<&mut [T]> { + let mut it = v.iter_mut(); + for _ in 0..5 { + let _ = it.next(); + } + Some(it.into_slice()) + } + + // In a slice of zero-size elements the pointer is meaningless. + // Ensure iteration still works even if the pointer is at the end of the address space. + let slice: &[()] = unsafe { slice::from_raw_parts(-5isize as *const (), 10) }; + assert_eq!(slice.len(), 10); + assert_eq!(slice.iter().count(), 10); + + // .nth() on the iterator should also behave correctly + let mut it = slice.iter(); + assert!(it.nth(5).is_some()); + assert_eq!(it.count(), 4); + + // Converting Iter to a slice should never have a null pointer + assert!(foo(slice).is_some()); + + // Test mutable iterators as well + let slice: &mut [()] = unsafe { slice::from_raw_parts_mut(-5isize as *mut (), 10) }; + assert_eq!(slice.len(), 10); + assert_eq!(slice.iter_mut().count(), 10); + + { + let mut it = slice.iter_mut(); + assert!(it.nth(5).is_some()); + assert_eq!(it.count(), 4); + } + + assert!(foo_mut(slice).is_some()) +} + +fn test_iter_ref_consistency() { + use std::fmt::Debug; + + fn test(x : T) { + let v : &[T] = &[x, x, x]; + let v_ptrs : [*const T; 3] = match v { + [ref v1, ref v2, ref v3] => [v1 as *const _, v2 as *const _, v3 as *const _], + _ => unreachable!() + }; + let len = v.len(); + + // nth(i) + for i in 0..len { + assert_eq!(&v[i] as *const _, v_ptrs[i]); // check the v_ptrs array, just to be sure + let nth = v.iter().nth(i).unwrap(); + assert_eq!(nth as *const _, v_ptrs[i]); + } + assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + + // stepping through with nth(0) + { + let mut it = v.iter(); + for i in 0..len { + let next = it.nth(0).unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.nth(0), None); + } + + // next() + { + let mut it = v.iter(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let next = it.next().unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + // next_back() + { + let mut it = v.iter(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let prev = it.next_back().unwrap(); + assert_eq!(prev as *const _, v_ptrs[remaining-1]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next_back(), None, "The final call to next_back() should return None"); + } + } + + fn test_mut(x : T) { + let v : &mut [T] = &mut [x, x, x]; + let v_ptrs : [*mut T; 3] = match v { + [ref v1, ref v2, ref v3] => + [v1 as *const _ as *mut _, v2 as *const _ as *mut _, v3 as *const _ as *mut _], + _ => unreachable!() + }; + let len = v.len(); + + // nth(i) + for i in 0..len { + assert_eq!(&mut v[i] as *mut _, v_ptrs[i]); // check the v_ptrs array, just to be sure + let nth = v.iter_mut().nth(i).unwrap(); + assert_eq!(nth as *mut _, v_ptrs[i]); + } + assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + + // stepping through with nth(0) + { + let mut it = v.iter(); + for i in 0..len { + let next = it.nth(0).unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.nth(0), None); + } + + // next() + { + let mut it = v.iter_mut(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let next = it.next().unwrap(); + assert_eq!(next as *mut _, v_ptrs[i]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + // next_back() + { + let mut it = v.iter_mut(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let prev = it.next_back().unwrap(); + assert_eq!(prev as *mut _, v_ptrs[remaining-1]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next_back(), None, "The final call to next_back() should return None"); + } + } + + // Make sure iterators and slice patterns yield consistent addresses for various types, + // including ZSTs. + test(0u32); + test(()); + test([0u32; 0]); // ZST with alignment > 0 + test_mut(0u32); + test_mut(()); + test_mut([0u32; 0]); // ZST with alignment > 0 +} + +fn main() { + slice_of_zst(); + test_iter_ref_consistency(); +} diff --git a/tests/run-pass/vecs.rs b/tests/run-pass/vecs.rs index 776791bbc9..bb9e5068f9 100644 --- a/tests/run-pass/vecs.rs +++ b/tests/run-pass/vecs.rs @@ -24,13 +24,45 @@ fn vec_into_iter() -> u8 { .fold(0, |x, y| x + y) } +fn vec_into_iter_rev() -> u8 { + vec![1, 2, 3, 4] + .into_iter() + .map(|x| x * x) + .fold(0, |x, y| x + y) +} + fn vec_into_iter_zst() -> usize { vec![[0u64; 0], [0u64; 0]] .into_iter() + .rev() + .map(|x| x.len()) + .sum() +} + +fn vec_into_iter_rev_zst() -> usize { + vec![[0u64; 0], [0u64; 0]] + .into_iter() + .rev() .map(|x| x.len()) .sum() } +fn vec_iter_and_mut() { + let mut v = vec![1,2,3,4]; + for i in v.iter_mut() { + *i += 1; + } + assert_eq!(v.iter().sum::(), 2+3+4+5); +} + +fn vec_iter_and_mut_rev() { + let mut v = vec![1,2,3,4]; + for i in v.iter_mut().rev() { + *i += 1; + } + assert_eq!(v.iter().sum::(), 2+3+4+5); +} + fn vec_reallocate() -> Vec { let mut v = vec![1, 2]; v.push(3); @@ -41,8 +73,14 @@ fn vec_reallocate() -> Vec { fn main() { assert_eq!(vec_reallocate().len(), 5); + assert_eq!(vec_into_iter(), 30); + assert_eq!(vec_into_iter_rev(), 30); + vec_iter_and_mut(); assert_eq!(vec_into_iter_zst(), 0); + assert_eq!(vec_into_iter_rev_zst(), 0); + vec_iter_and_mut_rev(); + assert_eq!(make_vec().capacity(), 4); assert_eq!(make_vec_macro(), [1, 2]); assert_eq!(make_vec_macro_repeat(), [42; 5]); From 478f137c39b827e6de39056b5c27a8e5d797aa4d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 30 Oct 2018 15:08:18 +0100 Subject: [PATCH 7/9] put all the logic into reactivatable() --- src/stacked_borrows.rs | 140 +++++++++++++++++++---------------------- 1 file changed, 63 insertions(+), 77 deletions(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index baeb04b44e..dcb284b1bc 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -142,74 +142,76 @@ pub struct Stacks { /// Core operations impl<'tcx> Stack { - /// Check if `bor` is currently active. We accept a `Raw` on a frozen location - /// because this could be a shared (re)borrow. If you want to mutate, this - /// is not the right function to call! - fn check(&self, bor: Borrow) -> bool { - match bor { - Borrow::Frz(acc_t) => - // Must be frozen at least as long as the `acc_t` says. - self.frozen_since.map_or(false, |loc_t| loc_t <= acc_t), - Borrow::Mut(acc_m) => - // Raw pointers are fine with frozen locations. This is important because &Cell is raw! - if self.frozen_since.is_some() { - acc_m.is_raw() - } else { - self.borrows.last().map_or(false, |&loc_itm| loc_itm == BorStackItem::Mut(acc_m)) - } - } - } - /// Check if `bor` could be activated by unfreezing and popping. - /// `force_mut` indicates whether being frozen is potentially acceptable. + /// `ref_kind` indicates whether this is being used to read/write (or, equivalently, to + /// borrow as &/&mut), or to borrow as raw. /// Returns `Err` if the answer is "no"; otherwise the data says /// what needs to happen to activate this: `None` = nothing, /// `Some(n)` = unfreeze and make item `n` the top item of the stack. - fn reactivatable(&self, bor: Borrow, force_mut: bool) -> Result, String> { - // Unless mutation is bound to happen, do NOT change anything if `bor` is already active. - // In particular, if it is a `Mut(Raw)` and we are frozen, this should be a NOP. - if !force_mut && self.check(bor) { - return Ok(None); - } - - let acc_m = match bor { + fn reactivatable(&self, bor: Borrow, ref_kind: RefKind) -> Result, String> { + let mut_borrow = match bor { Borrow::Frz(since) => - return Err(if force_mut { - format!("Using a shared borrow for mutation") - } else { - format!( - "Location should be frozen since {} but {}", - since, - match self.frozen_since { - None => format!("it is not frozen at all"), - Some(since) => format!("it is only frozen since {}", since), - } - ) - }), - Borrow::Mut(acc_m) => acc_m + // The only way to reactivate a `Frz` is if this is already frozen. + return match self.frozen_since { + _ if ref_kind == RefKind::Mut => + Err(format!("Using a shared borrow for mutation")), + None => + Err(format!("Location should be frozen but it is not")), + Some(loc) if loc <= since => + Ok(None), + Some(loc) => + Err(format!("Location should be frozen since {} but it is only frozen \ + since {}", since, loc)), + }, + Borrow::Mut(Mut::Raw) if self.is_frozen() && ref_kind != RefKind::Mut => + // Non-mutating access with a raw from a frozen location is a special case: The + // shared refs do not mind raw reads, and the raw itself does not assume any + // exclusivity. So we do not even require there to be a raw on the stack. + // This does not break the assumption that an `&mut` we own is + // exclusive for reads, because there we have the invariant that + // the location is *not* frozen. + return Ok(None), + Borrow::Mut(mut_borrow) => mut_borrow }; - // This is where we would unfreeze. + // See if we can get there via popping. for (idx, &itm) in self.borrows.iter().enumerate().rev() { match itm { BorStackItem::FnBarrier(_) => - return Err(format!("Trying to reactivate a mutable borrow ({:?}) that lives behind a barrier", acc_m)), - BorStackItem::Mut(loc_m) => { - if loc_m == acc_m { return Ok(Some(idx)); } + return Err(format!("Trying to reactivate a mutable borrow ({:?}) that lives \ + behind a barrier", mut_borrow)), + BorStackItem::Mut(loc) => { + if loc == mut_borrow { + // We found it! This is good to know. + // Yet, maybe we do not really want to pop? + if ref_kind == RefKind::Shr && self.is_frozen() { + // Whoever had exclusive access to this location allowed it + // to become frozen. That can only happen if they reborrowed + // to a shared ref, at which point they gave up on exclusive access. + // Hence we allow more reads, entirely ignoring everything above + // on the stack (but still making sure it is on the stack). + // This does not break the assumption that an `&mut` we own is + // exclusive for reads, because there we have the invariant that + // the location is *not* frozen. + return Ok(None); + } else { + return Ok(Some(idx)); + } + } } } } // Nothing to be found. - Err(format!("Mutable borrow-to-reactivate ({:?}) does not exist on the stack", acc_m)) + Err(format!("Mutable borrow-to-reactivate ({:?}) does not exist on the stack", mut_borrow)) } - /// Reactive `bor` for this stack. If `force_mut` is set, we want to aggressively - /// unfreeze this location (because we are about to mutate, so a frozen `Raw` is not okay). - fn reactivate(&mut self, bor: Borrow, force_mut: bool) -> EvalResult<'tcx> { - let action = match self.reactivatable(bor, force_mut) { + /// Reactive `bor` for this stack. `ref_kind` indicates whether this is being + /// used to read/write (or, equivalently, to borrow as &/&mut), or to borrow as raw. + fn reactivate(&mut self, bor: Borrow, ref_kind: RefKind) -> EvalResult<'tcx> { + let action = match self.reactivatable(bor, ref_kind) { Ok(action) => action, Err(err) => return err!(MachineError(err)), }; - + // Execute what `reactivatable` told us to do. match action { None => {}, // nothing to do Some(top) => { @@ -285,27 +287,7 @@ impl<'tcx> Stacks { ptr.tag, ref_kind, new_bor, ptr, size.bytes()); let mut stacks = self.stacks.borrow_mut(); for stack in stacks.iter_mut(ptr.offset, size) { - if ref_kind == RefKind::Shr && stack.is_frozen() { - // Location already frozen. We don't want to unfreeze, but make sure - // the ref makes some sense. - if let Err(err) = stack.reactivatable(ptr.tag, /*force_mut*/false) { - return err!(MachineError(err)); - } - } else { - // If we are creating a mutable ref, we certainly want to unfreeze. - // Even if we are doing so from a raw. - // Notice that if this is a local, whenever we access it directly the - // tag here will be the bottommost `Uniq` for that local. That `Uniq` - // never is accessible by the program, so it will not be used by any - // other access. IOW, whenever we directly use a local this will pop - // everything else off the stack, invalidating all previous pointers - // and, in particular, *all* raw pointers. This subsumes the explicit - // `reset` which the blog post [1] says to perform when accessing a local. - // - // [1] https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html - let force_mut = ref_kind == RefKind::Mut; - stack.reactivate(ptr.tag, force_mut)?; - } + stack.reactivate(ptr.tag, ref_kind)?; if let Some(new_bor) = new_bor { stack.initiate(new_bor); } @@ -484,11 +466,8 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' let mut stacks = alloc.extra.stacks.borrow_mut(); // We need `iter_mut` because `iter` would skip gaps! for stack in stacks.iter_mut(ptr.offset, size) { - // We accept &mut to a frozen location here, that is just normal. There might - // be shared reborrows that we are about to invalidate with this access. - // We cannot invalidate them aggressively here because the deref might also be - // to just create more shared refs. - if let Err(err) = stack.reactivatable(ptr.tag, /*force_mut*/false) { + // Conservatively assume that we will only read. + if let Err(err) = stack.reactivatable(ptr.tag, RefKind::Shr) { return err!(MachineError(format!("Encountered {:?} reference with non-reactivatable tag: {}", ref_kind, err))) } } @@ -503,7 +482,14 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' ) -> Borrow { let mut_borrow = match kind { MemoryKind::Stack => { - // New unique borrow + // New unique borrow. This `Uniq` is not accessible by the program, + // so it will only ever be used when using the local directly (i.e., + // not through a pointer). IOW, whenever we directly use a local this will pop + // everything else off the stack, invalidating all previous pointers + // and, in particular, *all* raw pointers. This subsumes the explicit + // `reset` which the blog post [1] says to perform when accessing a local. + // + // [1] https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html let time = self.machine.stacked_borrows.increment_clock(); Mut::Uniq(time) } From 81534496dc3356d5794300d513be200f939350c7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 30 Oct 2018 16:46:28 +0100 Subject: [PATCH 8/9] rename RefKind to UsageKind, because it not only used for references now --- src/lib.rs | 2 +- src/stacked_borrows.rs | 118 ++++++++++-------- .../stacked_borrows/alias_through_mutation.rs | 2 +- .../stacked_borrows/buggy_as_mut_slice.rs | 2 +- .../stacked_borrows/buggy_split_at_mut.rs | 2 +- .../stacked_borrows/illegal_read1.rs | 2 +- .../stacked_borrows/illegal_read2.rs | 2 +- .../stacked_borrows/illegal_write1.rs | 2 +- .../stacked_borrows/illegal_write5.rs | 2 +- .../stacked_borrows/load_invalid_mut.rs | 2 +- .../stacked_borrows/load_invalid_shr.rs | 2 +- .../stacked_borrows/pass_invalid_mut.rs | 2 +- .../stacked_borrows/pass_invalid_shr.rs | 2 +- .../stacked_borrows/return_invalid_mut.rs | 2 +- .../stacked_borrows/return_invalid_shr.rs | 2 +- .../stacked_borrows/shared_confusion.rs | 2 +- .../stacked_borrows/shared_confusion_opt.rs | 2 +- 17 files changed, 84 insertions(+), 66 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b397e9a6d2..5a3e1c7cab 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,7 +48,7 @@ use mono_hash_map::MonoHashMap; use stacked_borrows::{EvalContextExt as StackedBorEvalContextExt}; // Used by priroda -pub use stacked_borrows::{Borrow, Stacks, Mut as MutBorrow}; +pub use stacked_borrows::{Borrow, Stack, Stacks, Mut as MutBorrow, BorStackItem}; /// Insert rustc arguments at the beginning of the argument listthat miri wants to be /// set per default, for maximal validation power. diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index dcb284b1bc..d520c6ff5d 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -64,6 +64,12 @@ impl Borrow { } } +impl Default for Borrow { + fn default() -> Self { + Borrow::Mut(Mut::Raw) + } +} + /// An item in the borrow stack #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum BorStackItem { @@ -74,26 +80,33 @@ pub enum BorStackItem { FnBarrier(usize) } -impl Default for Borrow { - fn default() -> Self { - Borrow::Mut(Mut::Raw) +impl BorStackItem { + #[inline(always)] + pub fn is_fn_barrier(self) -> bool { + match self { + BorStackItem::FnBarrier(_) => true, + _ => false, + } } } -/// What kind of reference are we talking about? +/// What kind of usage of the pointer are we talking about? #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] -pub enum RefKind { - Mut, - Shr, +pub enum UsageKind { + /// Write, or create &mut + Write, + /// Read, or create & + Read, + /// Create * Raw, } -impl From> for RefKind { +impl From> for UsageKind { fn from(mutbl: Option) -> Self { match mutbl { - None => RefKind::Raw, - Some(hir::MutMutable) => RefKind::Mut, - Some(hir::MutImmutable) => RefKind::Shr, + None => UsageKind::Raw, + Some(hir::MutMutable) => UsageKind::Write, + Some(hir::MutImmutable) => UsageKind::Read, } } } @@ -112,7 +125,7 @@ impl State { /// Extra per-location state #[derive(Clone, Debug)] -struct Stack { +pub struct Stack { borrows: Vec, // used as a stack frozen_since: Option, } @@ -143,17 +156,17 @@ pub struct Stacks { /// Core operations impl<'tcx> Stack { /// Check if `bor` could be activated by unfreezing and popping. - /// `ref_kind` indicates whether this is being used to read/write (or, equivalently, to + /// `usage` indicates whether this is being used to read/write (or, equivalently, to /// borrow as &/&mut), or to borrow as raw. /// Returns `Err` if the answer is "no"; otherwise the data says /// what needs to happen to activate this: `None` = nothing, /// `Some(n)` = unfreeze and make item `n` the top item of the stack. - fn reactivatable(&self, bor: Borrow, ref_kind: RefKind) -> Result, String> { + fn reactivatable(&self, bor: Borrow, usage: UsageKind) -> Result, String> { let mut_borrow = match bor { Borrow::Frz(since) => // The only way to reactivate a `Frz` is if this is already frozen. return match self.frozen_since { - _ if ref_kind == RefKind::Mut => + _ if usage == UsageKind::Write => Err(format!("Using a shared borrow for mutation")), None => Err(format!("Location should be frozen but it is not")), @@ -163,10 +176,11 @@ impl<'tcx> Stack { Err(format!("Location should be frozen since {} but it is only frozen \ since {}", since, loc)), }, - Borrow::Mut(Mut::Raw) if self.is_frozen() && ref_kind != RefKind::Mut => + Borrow::Mut(Mut::Raw) if self.is_frozen() && usage != UsageKind::Write => // Non-mutating access with a raw from a frozen location is a special case: The // shared refs do not mind raw reads, and the raw itself does not assume any - // exclusivity. So we do not even require there to be a raw on the stack. + // exclusivity. So we do not even require there to be a raw on the stack, + // the raw is instead "matched" by the fact that this location is frozen. // This does not break the assumption that an `&mut` we own is // exclusive for reads, because there we have the invariant that // the location is *not* frozen. @@ -183,7 +197,7 @@ impl<'tcx> Stack { if loc == mut_borrow { // We found it! This is good to know. // Yet, maybe we do not really want to pop? - if ref_kind == RefKind::Shr && self.is_frozen() { + if usage == UsageKind::Read && self.is_frozen() { // Whoever had exclusive access to this location allowed it // to become frozen. That can only happen if they reborrowed // to a shared ref, at which point they gave up on exclusive access. @@ -204,10 +218,10 @@ impl<'tcx> Stack { Err(format!("Mutable borrow-to-reactivate ({:?}) does not exist on the stack", mut_borrow)) } - /// Reactive `bor` for this stack. `ref_kind` indicates whether this is being + /// Reactive `bor` for this stack. `usage` indicates whether this is being /// used to read/write (or, equivalently, to borrow as &/&mut), or to borrow as raw. - fn reactivate(&mut self, bor: Borrow, ref_kind: RefKind) -> EvalResult<'tcx> { - let action = match self.reactivatable(bor, ref_kind) { + fn reactivate(&mut self, bor: Borrow, usage: UsageKind) -> EvalResult<'tcx> { + let action = match self.reactivatable(bor, usage) { Ok(action) => action, Err(err) => return err!(MachineError(err)), }; @@ -274,20 +288,20 @@ impl State { /// Higher-level operations impl<'tcx> Stacks { - /// The single most operation: Make sure that using `ptr` as `ref_kind` is okay, + /// The single most operation: Make sure that using `ptr` as `usage` is okay, /// and if `new_bor` is present then make that the new current borrow. fn use_and_maybe_re_borrow( &self, ptr: Pointer, size: Size, - ref_kind: RefKind, + usage: UsageKind, new_bor: Option, ) -> EvalResult<'tcx> { trace!("use_and_maybe_re_borrow of tag {:?} as {:?}, new {:?}: {:?}, size {}", - ptr.tag, ref_kind, new_bor, ptr, size.bytes()); + ptr.tag, usage, new_bor, ptr, size.bytes()); let mut stacks = self.stacks.borrow_mut(); for stack in stacks.iter_mut(ptr.offset, size) { - stack.reactivate(ptr.tag, ref_kind)?; + stack.reactivate(ptr.tag, usage)?; if let Some(new_bor) = new_bor { stack.initiate(new_bor); } @@ -303,7 +317,7 @@ impl<'tcx> Stacks { size: Size, ) -> EvalResult<'tcx> { // Reads behave exactly like the first half of a reborrow-to-shr - self.use_and_maybe_re_borrow(ptr, size, RefKind::Shr, None) + self.use_and_maybe_re_borrow(ptr, size, UsageKind::Read, None) } #[inline(always)] @@ -313,7 +327,7 @@ impl<'tcx> Stacks { size: Size, ) -> EvalResult<'tcx> { // Writes behave exactly like the first half of a reborrow-to-mut - self.use_and_maybe_re_borrow(ptr, size, RefKind::Mut, None) + self.use_and_maybe_re_borrow(ptr, size, UsageKind::Write, None) } pub fn memory_deallocated( @@ -322,7 +336,7 @@ impl<'tcx> Stacks { size: Size, ) -> EvalResult<'tcx> { // This is like mutating - self.use_and_maybe_re_borrow(ptr, size, RefKind::Mut, None) + self.use_and_maybe_re_borrow(ptr, size, UsageKind::Write, None) // FIXME: Error out of there are any barriers? } @@ -346,7 +360,7 @@ pub trait EvalContextExt<'tcx> { ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, - ref_kind: RefKind, + usage: UsageKind, ) -> EvalResult<'tcx, Borrow>; @@ -355,7 +369,7 @@ pub trait EvalContextExt<'tcx> { ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, - ref_kind: RefKind, + usage: UsageKind, ) -> EvalResult<'tcx, Borrow>; fn tag_new_allocation( @@ -378,12 +392,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, - ref_kind: RefKind, + usage: UsageKind, ) -> EvalResult<'tcx, Borrow> { let time = self.machine.stacked_borrows.increment_clock(); - let new_bor = match ref_kind { - RefKind::Mut => Borrow::Mut(Mut::Uniq(time)), - RefKind::Shr => + let new_bor = match usage { + UsageKind::Write => Borrow::Mut(Mut::Uniq(time)), + UsageKind::Read => // FIXME This does not do enough checking when only part of the data has // interior mutability. When the type is `(i32, Cell)`, we want the // first field to be frozen but not the second. @@ -393,10 +407,10 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // Shared reference with interior mutability. Borrow::Mut(Mut::Raw) }, - RefKind::Raw => Borrow::Mut(Mut::Raw), + UsageKind::Raw => Borrow::Mut(Mut::Raw), }; trace!("tag_reference: Creating new reference ({:?}) for {:?} (pointee {}, size {}): {:?}", - ref_kind, ptr, pointee_ty, size.bytes(), new_bor); + usage, ptr, pointee_ty, size.bytes(), new_bor); // Make sure this reference is not dangling or so self.memory().check_bounds(ptr, size, false)?; @@ -404,7 +418,7 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // Update the stacks. We cannot use `get_mut` becuse this might be immutable // memory. let alloc = self.memory().get(ptr.alloc_id).expect("We checked that the ptr is fine!"); - alloc.extra.use_and_maybe_re_borrow(ptr, size, ref_kind, Some(new_bor))?; + alloc.extra.use_and_maybe_re_borrow(ptr, size, usage, Some(new_bor))?; Ok(new_bor) } @@ -418,39 +432,39 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' ptr: Pointer, pointee_ty: Ty<'tcx>, size: Size, - ref_kind: RefKind, + usage: UsageKind, ) -> EvalResult<'tcx, Borrow> { trace!("tag_reference: Accessing reference ({:?}) for {:?} (pointee {}, size {})", - ref_kind, ptr, pointee_ty, size.bytes()); + usage, ptr, pointee_ty, size.bytes()); // In principle we should not have to do anything here. However, with transmutes involved, - // it can happen that the tag of `ptr` does not actually match `ref_kind`, and we + // it can happen that the tag of `ptr` does not actually match `usage`, and we // should adjust for that. // Notably, the compiler can introduce such transmutes by optimizing away `&[mut]*`. // That can transmute a raw ptr to a (shared/mut) ref, and a mut ref to a shared one. - match (ref_kind, ptr.tag) { - (RefKind::Raw, _) => { + match (usage, ptr.tag) { + (UsageKind::Raw, _) => { // Don't use the tag, this is a raw access! Even if there is a tag, // that means transmute happened and we ignore the tag. // Also don't do any further validation, this is raw after all. return Ok(Borrow::Mut(Mut::Raw)); } - (RefKind::Mut, Borrow::Mut(Mut::Uniq(_))) | - (RefKind::Shr, Borrow::Frz(_)) | - (RefKind::Shr, Borrow::Mut(Mut::Raw)) => { + (UsageKind::Write, Borrow::Mut(Mut::Uniq(_))) | + (UsageKind::Read, Borrow::Frz(_)) | + (UsageKind::Read, Borrow::Mut(Mut::Raw)) => { // Expected combinations. Nothing to do. // FIXME: We probably shouldn't accept this if we got a raw shr without // interior mutability. } - (RefKind::Mut, Borrow::Mut(Mut::Raw)) => { + (UsageKind::Write, Borrow::Mut(Mut::Raw)) => { // Raw transmuted to mut ref. Keep this as raw access. // We cannot reborrow here; there might be a raw in `&(*var).1` where // `var` is an `&mut`. The other field of the struct might be already frozen, // also using `var`, and that would be okay. } - (RefKind::Shr, Borrow::Mut(Mut::Uniq(_))) => { + (UsageKind::Read, Borrow::Mut(Mut::Uniq(_))) => { // A mut got transmuted to shr. The mut borrow must be reactivatable. } - (RefKind::Mut, Borrow::Frz(_)) => { + (UsageKind::Write, Borrow::Frz(_)) => { // This is just invalid. // If we ever allow this, we have to consider what we do when a turn a // `Raw`-tagged `&mut` into a raw pointer pointing to a frozen location. @@ -467,8 +481,12 @@ impl<'a, 'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'a, 'mir, ' // We need `iter_mut` because `iter` would skip gaps! for stack in stacks.iter_mut(ptr.offset, size) { // Conservatively assume that we will only read. - if let Err(err) = stack.reactivatable(ptr.tag, RefKind::Shr) { - return err!(MachineError(format!("Encountered {:?} reference with non-reactivatable tag: {}", ref_kind, err))) + if let Err(err) = stack.reactivatable(ptr.tag, UsageKind::Read) { + return err!(MachineError(format!( + "Encountered {} reference with non-reactivatable tag: {}", + if usage == UsageKind::Write { "mutable" } else { "shared" }, + err + ))) } } // All is good. diff --git a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs index eb8966f3a5..0b2d459366 100644 --- a/tests/compile-fail/stacked_borrows/alias_through_mutation.rs +++ b/tests/compile-fail/stacked_borrows/alias_through_mutation.rs @@ -11,5 +11,5 @@ fn main() { retarget(&mut target_alias, target); // now `target_alias` points to the same thing as `target` *target = 13; - let _val = *target_alias; //~ ERROR Shr reference with non-reactivatable tag + let _val = *target_alias; //~ ERROR reference with non-reactivatable tag } diff --git a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs index e86eb9ba6d..2c48404ddf 100644 --- a/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs +++ b/tests/compile-fail/stacked_borrows/buggy_as_mut_slice.rs @@ -14,6 +14,6 @@ fn main() { let v = vec![0,1,2]; let v1 = safe::as_mut_slice(&v); let v2 = safe::as_mut_slice(&v); - v1[1] = 5; //~ ERROR Mut reference with non-reactivatable tag + v1[1] = 5; //~ ERROR reference with non-reactivatable tag v1[1] = 6; } diff --git a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs index a4f5f536b7..d8a241cab5 100644 --- a/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs +++ b/tests/compile-fail/stacked_borrows/buggy_split_at_mut.rs @@ -11,7 +11,7 @@ mod safe { assert!(mid <= len); (from_raw_parts_mut(ptr, len - mid), // BUG: should be "mid" instead of "len - mid" - //~^ ERROR Mut reference with non-reactivatable tag + //~^ ERROR reference with non-reactivatable tag from_raw_parts_mut(ptr.offset(mid as isize), len - mid)) } } diff --git a/tests/compile-fail/stacked_borrows/illegal_read1.rs b/tests/compile-fail/stacked_borrows/illegal_read1.rs index f0e696c457..59190a15db 100644 --- a/tests/compile-fail/stacked_borrows/illegal_read1.rs +++ b/tests/compile-fail/stacked_borrows/illegal_read1.rs @@ -7,7 +7,7 @@ fn main() { let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... callee(xraw); let _val = *xref; // ...but any use of raw will invalidate our ref. - //~^ ERROR: Mut reference with non-reactivatable tag + //~^ ERROR: mutable reference with non-reactivatable tag } fn callee(xraw: *mut i32) { diff --git a/tests/compile-fail/stacked_borrows/illegal_read2.rs b/tests/compile-fail/stacked_borrows/illegal_read2.rs index ec59c57d31..594117d28a 100644 --- a/tests/compile-fail/stacked_borrows/illegal_read2.rs +++ b/tests/compile-fail/stacked_borrows/illegal_read2.rs @@ -7,7 +7,7 @@ fn main() { let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... callee(xraw); let _val = *xref; // ...but any use of raw will invalidate our ref. - //~^ ERROR: Mut reference with non-reactivatable tag + //~^ ERROR: mutable reference with non-reactivatable tag } fn callee(xraw: *mut i32) { diff --git a/tests/compile-fail/stacked_borrows/illegal_write1.rs b/tests/compile-fail/stacked_borrows/illegal_write1.rs index 131e89572a..ab951be5ec 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write1.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write1.rs @@ -8,5 +8,5 @@ fn main() { let target = Box::new(42); // has an implicit raw let ref_ = &*target; evil(ref_); // invalidates shared ref, activates raw - let _x = *ref_; //~ ERROR Shr reference with non-reactivatable tag + let _x = *ref_; //~ ERROR reference with non-reactivatable tag } diff --git a/tests/compile-fail/stacked_borrows/illegal_write5.rs b/tests/compile-fail/stacked_borrows/illegal_write5.rs index 4b5d08c03a..f4704ad571 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write5.rs +++ b/tests/compile-fail/stacked_borrows/illegal_write5.rs @@ -7,7 +7,7 @@ fn main() { let xref = unsafe { &mut *xraw }; // derived from raw, so using raw is still okay... callee(xraw); let _val = *xref; // ...but any use of raw will invalidate our ref. - //~^ ERROR: Mut reference with non-reactivatable tag + //~^ ERROR: reference with non-reactivatable tag } fn callee(xraw: *mut i32) { diff --git a/tests/compile-fail/stacked_borrows/load_invalid_mut.rs b/tests/compile-fail/stacked_borrows/load_invalid_mut.rs index 060cec962c..4ea61cd606 100644 --- a/tests/compile-fail/stacked_borrows/load_invalid_mut.rs +++ b/tests/compile-fail/stacked_borrows/load_invalid_mut.rs @@ -5,5 +5,5 @@ fn main() { let xref = unsafe { &mut *xraw }; let xref_in_mem = Box::new(xref); let _val = *x; // invalidate xraw - let _val = *xref_in_mem; //~ ERROR Mut reference with non-reactivatable tag + let _val = *xref_in_mem; //~ ERROR mutable reference with non-reactivatable tag } diff --git a/tests/compile-fail/stacked_borrows/load_invalid_shr.rs b/tests/compile-fail/stacked_borrows/load_invalid_shr.rs index b4b180e350..53179c954d 100644 --- a/tests/compile-fail/stacked_borrows/load_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/load_invalid_shr.rs @@ -5,5 +5,5 @@ fn main() { let xref = unsafe { &*xraw }; let xref_in_mem = Box::new(xref); *x = 42; // invalidate xraw - let _val = *xref_in_mem; //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen + let _val = *xref_in_mem; //~ ERROR shared reference with non-reactivatable tag: Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/pass_invalid_mut.rs b/tests/compile-fail/stacked_borrows/pass_invalid_mut.rs index bc950771ad..5e1118160a 100644 --- a/tests/compile-fail/stacked_borrows/pass_invalid_mut.rs +++ b/tests/compile-fail/stacked_borrows/pass_invalid_mut.rs @@ -6,5 +6,5 @@ fn main() { let xraw = x as *mut _; let xref = unsafe { &mut *xraw }; let _val = *x; // invalidate xraw - foo(xref); //~ ERROR Mut reference with non-reactivatable tag + foo(xref); //~ ERROR mutable reference with non-reactivatable tag } diff --git a/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs b/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs index 477c86f606..e4b26cfff6 100644 --- a/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/pass_invalid_shr.rs @@ -6,5 +6,5 @@ fn main() { let xraw = &*x as *const _; let xref = unsafe { &*xraw }; *x = 42; // invalidate xraw - foo(xref); //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen + foo(xref); //~ ERROR shared reference with non-reactivatable tag: Location should be frozen } diff --git a/tests/compile-fail/stacked_borrows/return_invalid_mut.rs b/tests/compile-fail/stacked_borrows/return_invalid_mut.rs index c02892671e..949b3829ff 100644 --- a/tests/compile-fail/stacked_borrows/return_invalid_mut.rs +++ b/tests/compile-fail/stacked_borrows/return_invalid_mut.rs @@ -3,7 +3,7 @@ fn foo(x: &mut (i32, i32)) -> &mut i32 { let xraw = x as *mut (i32, i32); let ret = unsafe { &mut (*xraw).1 }; let _val = *x; // invalidate xraw and its children - ret //~ ERROR Mut reference with non-reactivatable tag + ret //~ ERROR mutable reference with non-reactivatable tag } fn main() { diff --git a/tests/compile-fail/stacked_borrows/return_invalid_shr.rs b/tests/compile-fail/stacked_borrows/return_invalid_shr.rs index 954b5ec8e5..2d34350359 100644 --- a/tests/compile-fail/stacked_borrows/return_invalid_shr.rs +++ b/tests/compile-fail/stacked_borrows/return_invalid_shr.rs @@ -3,7 +3,7 @@ fn foo(x: &mut (i32, i32)) -> &i32 { let xraw = x as *mut (i32, i32); let ret = unsafe { &(*xraw).1 }; x.1 = 42; // invalidate xraw on the 2nd field - ret //~ ERROR Shr reference with non-reactivatable tag: Location should be frozen + ret //~ ERROR shared reference with non-reactivatable tag: Location should be frozen } fn main() { diff --git a/tests/compile-fail/stacked_borrows/shared_confusion.rs b/tests/compile-fail/stacked_borrows/shared_confusion.rs index c02822ed4d..624587932c 100644 --- a/tests/compile-fail/stacked_borrows/shared_confusion.rs +++ b/tests/compile-fail/stacked_borrows/shared_confusion.rs @@ -21,7 +21,7 @@ fn test(r: &mut RefCell) { } // Our old raw should be dead by now unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack - *x_inner = 12; //~ ERROR Mut reference with non-reactivatable tag + *x_inner = 12; //~ ERROR reference with non-reactivatable tag } fn main() { diff --git a/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs index 4a88d4d641..3030f5dd40 100644 --- a/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs +++ b/tests/compile-fail/stacked_borrows/shared_confusion_opt.rs @@ -17,7 +17,7 @@ fn test(r: &mut RefCell) { } // Our old raw should be dead by now unsafe { *x_evil = 0; } // this falls back to some Raw higher up the stack - *x_inner = 12; //~ ERROR Mut reference with non-reactivatable tag + *x_inner = 12; //~ ERROR reference with non-reactivatable tag } fn main() { From a68779fd16a851fbdb0e25297044da7f5a410845 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 1 Nov 2018 08:55:03 +0100 Subject: [PATCH 9/9] use crate:: import to make edition port easier later --- src/lib.rs | 2 +- src/stacked_borrows.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 5a3e1c7cab..82ffb5e454 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,7 +48,7 @@ use mono_hash_map::MonoHashMap; use stacked_borrows::{EvalContextExt as StackedBorEvalContextExt}; // Used by priroda -pub use stacked_borrows::{Borrow, Stack, Stacks, Mut as MutBorrow, BorStackItem}; +pub use crate::stacked_borrows::{Borrow, Stack, Stacks, Mut as MutBorrow, BorStackItem}; /// Insert rustc arguments at the beginning of the argument listthat miri wants to be /// set per default, for maximal validation power. diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index d520c6ff5d..4d78519be6 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -3,7 +3,7 @@ use std::cell::RefCell; use rustc::ty::{self, Ty, layout::Size}; use rustc::hir; -use super::{ +use crate::{ MemoryKind, MiriMemoryKind, RangeMap, EvalResult, AllocId, Pointer, PlaceTy, };