From ca759671d665af7c21588013999ba101613c57b2 Mon Sep 17 00:00:00 2001 From: Jan Teske Date: Thu, 15 Jun 2023 14:25:58 +0200 Subject: [PATCH] consolidate: fix a Miri error 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. --- src/consolidation.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/consolidation.rs b/src/consolidation.rs index 69b5f4570..b4087bcc9 100644 --- a/src/consolidation.rs +++ b/src/consolidation.rs @@ -38,6 +38,8 @@ pub fn consolidate_slice(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() { @@ -55,8 +57,8 @@ pub fn consolidate_slice(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); @@ -65,7 +67,7 @@ pub fn consolidate_slice(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); } } @@ -103,6 +105,8 @@ pub fn consolidate_updates_slice(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() { @@ -118,8 +122,8 @@ pub fn consolidate_updates_slice(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); @@ -128,7 +132,7 @@ pub fn consolidate_updates_slice(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); }