consolidate: fix a Miri error #394
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This looks good to me. Would you be willing to bundle in the same changes to |
|
The same failure also happens when running this repo's tests with As Frank noted, this also happens in |
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.
teskje
force-pushed
the
consolidate-fix-miri-error
branch
from
6e10ae0
to
ca75967
Compare
Of course! It's fixed now. |
|
Alternatively, we should explore what the performance cost of not using |
antiguru
approved these changes
Jun 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this change, Miri would produce the following error when executed on the code of
consolidate_updates_slice:The warning is fixed by making sure that
slice.get_mut_ptr()is only invoked a single time. It seems like callingget_mut_ptron a slice invalidates all existing pointers to the slice. My guess is that this is becauseget_mut_ptrtakes a&mut selfand could therefore in principle swap/replace/truncate the slice buffer, which could make existing pointers dangle.get_mut_ptrdoesn't do that but Rust cannot know based on the method signature only.Fixes https://github.com/MaterializeInc/database-issues/issues/5957.