Print spans where tags are created and invalidated
5225225 called this "automatic tag tracking" and I think that may be a reasonable description, but I would like to kill tag tracking as a primary use of Miri if possible. Tag tracking isn't always possible; for example if the UB is only detected with isolation off and the failing tag is made unstable by removing isolation. (also it's bad UX to run the tool twice)
This is just one of the things we can do with https://github.com/rust-lang/miri/pull/2024
The memory usage of this is _shockingly_ low, I think because the memory usage of Miri is driven by allocations where each byte ends up with its own very large stack. The memory usage in this change is linear with the number of tags, not tags * bytes. If memory usage gets out of control we can cap the number of events we save per allocation, from experience we tend to only use the most recent few in diagnostics but of course there's no guarantee of that so if we can manage to keep everything that would be best.
In many cases now I can tell exactly what these codebases are doing wrong just from the new outputs here, which I think is extremely cool.
New helps generated with plain old `cargo miri test` on `rust-argon2` v1.0.0:
```
test argon2::tests::single_thread_verification_multi_lane_hash ... error: Undefined Behavior: trying to reborrow <1485898> for Unique permission at alloc110523[0x0], but that tag does not exist in the borrow stack for this location
--> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/mem/manually_drop.rs:89:9
|
89 | slot.value
| ^^^^^^^^^^
| |
| trying to reborrow <1485898> for Unique permission at alloc110523[0x0], but that tag does not exist in the borrow stack for this location
| this error occurs as part of a reborrow at alloc110523[0x0..0x20]
|
= help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <1485898> was created by a retag at offsets [0x0..0x20]
--> src/memory.rs:42:13
|
42 | vec.push(unsafe { &mut (*ptr) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <1485898> was later invalidated at offsets [0x0..0x20]
--> src/memory.rs:42:31
|
42 | vec.push(unsafe { &mut (*ptr) });
| ^^^^^^^^^^^
```
And with `-Zmiri-tag-raw-pointers` on `slab` v0.4.5
```
error: Undefined Behavior: trying to reborrow <2915> for Unique permission at alloc1418[0x0], but that tag does not exist in the borrow stack for this location
--> /tmp/slab-0.4.5/src/lib.rs:835:16
|
835 | match (&mut *ptr1, &mut *ptr2) {
| ^^^^^^^^^^
| |
| trying to reborrow <2915> for Unique permission at alloc1418[0x0], but that tag does not exist in the borrow stack for this location
| this error occurs as part of a reborrow at alloc1418[0x0..0x10]
|
= help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <2915> was created by a retag at offsets [0x0..0x10]
--> /tmp/slab-0.4.5/src/lib.rs:833:20
|
833 | let ptr1 = self.entries.get_unchecked_mut(key1) as *mut Entry<T>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <2915> was later invalidated at offsets [0x0..0x20]
--> /tmp/slab-0.4.5/src/lib.rs:834:20
|
834 | let ptr2 = self.entries.get_unchecked_mut(key2) as *mut Entry<T>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
And without raw pointer tagging, `cargo miri test` on `half` v1.8.2
```
error: Undefined Behavior: trying to reborrow <untagged> for Unique permission at alloc1340[0x0], but that tag only grants SharedReadOnly permission for this location
--> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:141:9
|
141 | &mut *ptr::slice_from_raw_parts_mut(data, len)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| trying to reborrow <untagged> for Unique permission at alloc1340[0x0], but that tag only grants SharedReadOnly permission for this location
| this error occurs as part of a reborrow at alloc1340[0x0..0x6]
|
= help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: tag was most recently created at offsets [0x0..0x6]
--> /tmp/half-1.8.2/src/slice.rs:309:22
|
309 | let length = self.len();
| ^^^^^^^^^^
help: this tag was also created here at offsets [0x0..0x6]
--> /tmp/half-1.8.2/src/slice.rs:308:23
|
308 | let pointer = self.as_ptr() as *mut u16;
| ^^^^^^^^^^^^^
```
The second suggestion is close to guesswork, but from experience it tends to be correct (as in, it tends to locate the pointer the user wanted) more often that it doesn't.
* Store the local crates in an Rc<[CrateNum]>
* Move all the allocation history into Stacks
* Clean up the implementation of get_logs_relevant_to a bit
Use atomic RMW for `{mutex, rwlock, cond, srwlock}_get_or_create_id` functions
This is required for #1963
`{mutex, rwlock, cond, srwlock}_get_or_create_id()` currently checks whether an ID field is 0 using an atomic read, allocate one and get a new ID if it is, then write it in a separate atomic write. This is fine without weak memory. For instance, in `pthread_mutex_lock` which may be called by two threads concurrently, only one thread can read 0, create and then write a new ID, the later-run thread will always see the newly created ID and never 0.
```rust
fn pthread_mutex_lock(&mut self, mutex_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> {
let this = self.eval_context_mut();
let kind = mutex_get_kind(this, mutex_op)?.check_init()?;
let id = mutex_get_or_create_id(this, mutex_op)?;
let active_thread = this.get_active_thread();
```
However, with weak memory behaviour, both threads may read 0: the first thread has to see 0 because nothing else was written to it, and the second thread is not guaranteed to observe the latest value, causing a duplicate mutex to be created and both threads "successfully" acquiring the lock at the same time.
This is a pretty typical pattern requiring the use of atomic RMWs. RMW *always* reads the latest value in a location, so only one thread can create the new mutex and ID, all others scheduled later will see the new ID.
error: useless conversion to the same type: `rustc_const_eval::interpret::Pointer<std::option::Option<machine::Tag>>`
--> src/helpers.rs:668:36
|
668 | this.get_ptr_alloc(ptr.offset(len, this)?.into(), size1, Align::ONE)?.unwrap(); // not a ZST, so we will get a result
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `ptr.offset(len, this)?`
|
= note: `-D clippy::useless-conversion` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `rustc_const_eval::interpret::Pointer<std::option::Option<machine::Tag>>`
--> src/helpers.rs:678:29
|
678 | this.read_bytes_ptr(ptr.into(), len)
| ^^^^^^^^^^ help: consider removing `.into()`: `ptr`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `rustc_const_eval::interpret::Pointer<std::option::Option<machine::Tag>>`
--> src/helpers.rs:690:44
|
690 | let alloc = this.get_ptr_alloc(ptr.into(), size2, align2)?.unwrap(); // not a ZST, so we will get a result
| ^^^^^^^^^^ help: consider removing `.into()`: `ptr`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `rustc_const_eval::interpret::OpTy<machine::Tag>`
--> src/shims/intrinsics.rs:778:42
|
778 | .read_immediate(&this.operand_index(index, i)?.into())?
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `this.operand_index(index, i)?`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `u32`
--> src/shims/posix/fs.rs:1171:26
|
1171 | builder.mode(mode.into());
| ^^^^^^^^^^^ help: consider removing `.into()`: `mode`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `std::ffi::OsString`
--> src/shims/env.rs:67:53
|
67 | ecx.machine.env_vars.map.insert(OsString::from(name), var_ptr);
| ^^^^^^^^^^^^^^^^^^^^ help: consider removing `OsString::from()`: `name`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `rustc_const_eval::interpret::Scalar<machine::Tag>`
--> src/shims/tls.rs:102:44
|
102 | Ok(value.unwrap_or_else(|| Scalar::null_ptr(cx).into()))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `Scalar::null_ptr(cx)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: useless conversion to the same type: `u32`
--> src/thread.rs:73:26
|
73 | Scalar::from_u32(u32::try_from(self.0).unwrap())
| ^^^^^^^^^^^^^^^^^^^^^
|
= help: consider removing `u32::try_from()`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
error: single-character string constant used as pattern
--> src/helpers.rs:805:36
|
805 | .map(|crates| crates.split(",").map(|krate| krate.to_string()).collect::<Vec<_>>())
| ^^^ help: try using a `char` instead: `','`
|
= note: `-D clippy::single-char-pattern` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
error: redundant field names in struct initialization
--> src/helpers.rs:199:34
|
199 | let place = mir::Place { local: local, projection: List::empty() };
| ^^^^^^^^^^^^ help: replace it with: `local`
|
= note: `-D clippy::redundant-field-names` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
error: redundant field names in struct initialization
--> src/thread.rs:238:13
|
238 | threads: threads,
| ^^^^^^^^^^^^^^^^ help: replace it with: `threads`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
error: question mark operator is useless here
--> src/helpers.rs:86:16
|
86 | return Ok(const_val.check_init()?);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `const_val.check_init()`
|
= note: `-D clippy::needless-question-mark` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/data_race.rs:565:34
|
565 | this.validate_atomic_rmw(&place, atomic)?;
| ^^^^^^ help: change this to: `place`
|
= note: `-D clippy::needless-borrow` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/data_race.rs:1413:27
|
1413 | clocks.clock.join(&lock);
| ^^^^^ help: change this to: `lock`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/helpers.rs:326:51
|
326 | .size_and_align_of_mplace(&place)?
| ^^^^^^ help: change this to: `place`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/helpers.rs:365:17
|
365 | &self.ecx
| ^^^^^^^^^ help: change this to: `self.ecx`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/helpers.rs:634:47
|
634 | let seconds_place = this.mplace_field(&tp, 0)?;
| ^^^ help: change this to: `tp`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/helpers.rs:637:51
|
637 | let nanoseconds_place = this.mplace_field(&tp, 1)?;
| ^^^ help: change this to: `tp`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/machine.rs:547:73
|
547 | let link_name = match ecx.tcx.sess.first_attr_value_str_by_name(&attrs, sym::link_name) {
| ^^^^^^ help: change this to: `attrs`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/machine.rs:576:56
|
576 | Some(data_race::AllocExtra::new_allocation(&data_race, alloc.size(), kind))
| ^^^^^^^^^^ help: change this to: `data_race`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/foreign_items.rs:241:43
|
241 | .first_attr_value_str_by_name(&attrs, sym::link_name)
| ^^^^^^ help: change this to: `attrs`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/intrinsics.rs:778:61
|
778 | .read_immediate(&this.operand_index(&index, i)?.into())?
| ^^^^^^ help: change this to: `index`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/intrinsics.rs:1195:44
|
1195 | this.write_immediate(*old, &dest)?; // old value is returned
| ^^^^^ help: change this to: `dest`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/intrinsics.rs:1200:44
|
1200 | this.write_immediate(*old, &dest)?; // old value is returned
| ^^^^^ help: change this to: `dest`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:54:12
|
54 | Ok(&self)
| ^^^^^ help: change this to: `self`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:654:49
|
654 | let io_result = maybe_sync_file(&file, *writable, File::sync_all);
| ^^^^^ help: change this to: `file`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:746:52
|
746 | file_descriptor.write(communicate, &bytes)?.map(|c| i64::try_from(c).unwrap());
| ^^^^^^ help: change this to: `bytes`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:1494:45
|
1494 | let io_result = maybe_sync_file(&file, *writable, File::sync_all);
| ^^^^^ help: change this to: `file`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:1516:45
|
1516 | let io_result = maybe_sync_file(&file, *writable, File::sync_data);
| ^^^^^ help: change this to: `file`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/posix/fs.rs:1561:45
|
1561 | let io_result = maybe_sync_file(&file, *writable, File::sync_data);
| ^^^^^ help: change this to: `file`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/env.rs:232:65
|
232 | let var_ptr = alloc_env_var_as_c_str(&name, &value, &mut this)?;
| ^^^^^^^^^ help: change this to: `this`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/env.rs:277:68
|
277 | let var_ptr = alloc_env_var_as_wide_str(&name, &value, &mut this)?;
| ^^^^^^^^^ help: change this to: `this`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/env.rs:328:37
|
328 | let buf = this.read_pointer(&buf_op)?;
| ^^^^^^^ help: change this to: `buf_op`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: this expression creates a reference which is immediately dereferenced by the compiler
--> src/shims/env.rs:329:37
|
329 | let size = this.read_scalar(&size_op)?.to_machine_usize(&*this.tcx)?;
| ^^^^^^^^ help: change this to: `size_op`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> src/helpers.rs:54:29
|
54 | for item in mem::replace(&mut items, Default::default()).iter() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut items)`
|
= note: `-D clippy::mem-replace-with-default` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default
error: manual implementation of an assign operation
--> src/helpers.rs:673:17
|
673 | len = len + size1;
| ^^^^^^^^^^^^^^^^^ help: replace it with: `len += size1`
|
= note: `-D clippy::assign-op-pattern` implied by `-D clippy::all`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
Update posix fs shims to use new API `reject_in_isolation`, which
allows rejection with error code instead of always forcing abort.
Error code chosen for each op is the most appropriate one from the
list in corresponding syscall's manual.
Updated helper APIs to not use quotes (`) around input name while
preparing the message. This allows callers to pass multi-word string
like -- "`read` from stdin".
