Fix Stacked Borrows violations in crate #222
Closed
Conversation
This file contains hidden or 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 required to rewrite `struct Drain` because old implementation had aliasing pointers in it which violated [Stacked Borrows][1]. New implementation separately borrows len field and parts of the slice to prevent aliasing. Also, we don't use slice iteration anymore because stacked borrows conflict with `ptr::copy` which we use to move the tail. We can wrote it valid for stacked borrows by using 2 memmoves (1 from tail beginning to removed slice and 1 from rest of the tail to beginning of the tail) but it is more complex to read and understand, more error prone and less effecient. This commit causes divergence with Rust `alloc::vec::Vec` implementation but we cannot implement it same way because it is only possible if `Vec` struct stored in different allocation than its buffer which isn't possible for `ArrayVec`. Also, added test that check drain behaviour when dropping. [1]: https://plv.mpi-sws.org/rustbelt/stacked-borrows/
It have 2 differences from std method: 1. We call `as_mut_ptr` exactly once per iteration because we break `Stacked Borrows` otherwise. Reason of that is different data layout of `Vec` and `ArrayVec`. While `Vec::as_mut_ptr` only copy internal pointer, `ArrayVec::as_mut_ptr` needs to borrow whole array which cause aliasing between first call result and second call which in turn cause miri error. 2. We check if we processed all items so we don't do final `memmove` call as [`std::vec::Vec` does][1]. I think, in most cases it can be optimized away but still it is not guaranteed. Also, added tests to check behaviour on panics. [1]: https://godbolt.org/z/h664E5MKE
It was invalid for ZSTs because it ignored alignment requirements. From [docs][1]: "Note that even if T has size 0, the pointer must be non-null and *properly aligned*." I just switched to using integer to bounds checks. [1]: https://doc.rust-lang.org/std/ptr/fn.write.html#safety
Just avoid calling `as_mut_ptr` twice.
|
Rebased and added some extra tests. |
|
Thanks. Are these violations that miri does not check? I.e does this do something beyond fixing errors that the miri CI reports? |
|
I have merged PR #226 which is on the same topic, but is easier to review and more in line with what I expect. There might be nuances I don't know about though. My main crime is not being an active maintainer (and maybe having no love for rustfmt), and I'm sorry for that. I haven't had time for Rust in a long while unfortunately. |
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.
It mainly caused by borrowing whole internal array in
as_mut_ptrandas_ptr. StdlibVecdoesn't suffer from this because it keeps pointer to data internally so it doesn't reborrow slice when getting pointers.This changes required complete rewrite of
Drainstruct and little adjustment ofretain.Also, there is little changes in handling ZSTs and in ArrayString.
Also, I run
cargo fmton code and cherry-picked commit from #215.Probably better to review one commit at time.