Skip to content

Commit

Permalink
consolidate: fix a Miri error (#394)
Browse files Browse the repository at this point in the history
Prior to this commit, Miri would produce the following error when
executed on the code of `consolidate_updates_slice`:

```
error: Undefined Behavior: attempting a read access using <3403> at alloc1431[0x8], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:12:16
   |
12 |             if (*ptr1).0 == (*ptr2).0 && (*ptr1).1 == (*ptr2).1 {
   |                ^^^^^^^^^
   |                |
   |                attempting a read access using <3403> at alloc1431[0x8], but that tag does not exist in the borrow stack for this location
   |                this error occurs as part of an access at alloc1431[0x8..0xc]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows 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: <3403> was created by a SharedReadWrite retag at offsets [0x0..0x48]
  --> src/main.rs:9:24
   |
9  |             let ptr1 = slice.as_mut_ptr().offset(offset as isize);
   |                        ^^^^^^^^^^^^^^^^^^
help: <3403> was later invalidated at offsets [0x0..0x48] by a Unique function-entry retag inside this call
  --> src/main.rs:10:24
   |
10 |             let ptr2 = slice.as_mut_ptr().offset(index as isize);
   |                        ^^^^^^^^^^^^^^^^^^
   = note: BACKTRACE (of the first span):
   = note: inside `consolidate_updates_slice` at src/main.rs:12:16: 12:25
note: inside `main`
  --> src/main.rs:34:5
   |
34 |     consolidate_updates_slice(&mut v);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

The same is true for `consolidate_slice`.

The warning is fixed by making sure that `slice.get_mut_ptr()` is only
invoked a single time. It seems like calling `get_mut_ptr` on a slice
invalidates all existing pointers to the slice. My guess is that this is
because `get_mut_ptr` takes a `&mut self` and could therefore in
principle swap/replace/truncate the slice buffer, which could make
existing pointers dangle. `get_mut_ptr` doesn't do that but Rust cannot
know based on the method signature only.
  • Loading branch information
teskje committed Jun 15, 2023
1 parent 90a1eac commit 14417e6
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions src/consolidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ pub fn consolidate_slice<T: Ord, R: Semigroup>(slice: &mut [(T, R)]) -> usize {
// In a world where there are not many results, we may never even need to call in to merge sort.
slice.sort_by(|x,y| x.0.cmp(&y.0));

let slice_ptr = slice.as_mut_ptr();

// Counts the number of distinct known-non-zero accumulations. Indexes the write location.
let mut offset = 0;
for index in 1 .. slice.len() {
Expand All @@ -55,8 +57,8 @@ pub fn consolidate_slice<T: Ord, R: Semigroup>(slice: &mut [(T, R)]) -> usize {
assert!(offset < index);

// LOOP INVARIANT: offset < index
let ptr1 = slice.as_mut_ptr().offset(offset as isize);
let ptr2 = slice.as_mut_ptr().offset(index as isize);
let ptr1 = slice_ptr.add(offset);
let ptr2 = slice_ptr.add(index);

if (*ptr1).0 == (*ptr2).0 {
(*ptr1).1.plus_equals(&(*ptr2).1);
Expand All @@ -65,7 +67,7 @@ pub fn consolidate_slice<T: Ord, R: Semigroup>(slice: &mut [(T, R)]) -> usize {
if !(*ptr1).1.is_zero() {
offset += 1;
}
let ptr1 = slice.as_mut_ptr().offset(offset as isize);
let ptr1 = slice_ptr.add(offset);
std::ptr::swap(ptr1, ptr2);
}
}
Expand Down Expand Up @@ -103,6 +105,8 @@ pub fn consolidate_updates_slice<D: Ord, T: Ord, R: Semigroup>(slice: &mut [(D,
// In a world where there are not many results, we may never even need to call in to merge sort.
slice.sort_unstable_by(|x,y| (&x.0, &x.1).cmp(&(&y.0, &y.1)));

let slice_ptr = slice.as_mut_ptr();

// Counts the number of distinct known-non-zero accumulations. Indexes the write location.
let mut offset = 0;
for index in 1 .. slice.len() {
Expand All @@ -118,8 +122,8 @@ pub fn consolidate_updates_slice<D: Ord, T: Ord, R: Semigroup>(slice: &mut [(D,
unsafe {

// LOOP INVARIANT: offset < index
let ptr1 = slice.as_mut_ptr().offset(offset as isize);
let ptr2 = slice.as_mut_ptr().offset(index as isize);
let ptr1 = slice_ptr.add(offset);
let ptr2 = slice_ptr.add(index);

if (*ptr1).0 == (*ptr2).0 && (*ptr1).1 == (*ptr2).1 {
(*ptr1).2.plus_equals(&(*ptr2).2);
Expand All @@ -128,7 +132,7 @@ pub fn consolidate_updates_slice<D: Ord, T: Ord, R: Semigroup>(slice: &mut [(D,
if !(*ptr1).2.is_zero() {
offset += 1;
}
let ptr1 = slice.as_mut_ptr().offset(offset as isize);
let ptr1 = slice_ptr.add(offset);
std::ptr::swap(ptr1, ptr2);
}

Expand Down

0 comments on commit 14417e6

Please sign in to comment.