Enable scatters with &mut [MaybeUninit<T>]
#213
Conversation
| /// as even deriving `&[T]` from by calling `fn len` creates an `&[T]` which, | ||
| /// as an immutable borrow derived from a mutable (and therefore unique) borrow, | ||
| /// interacts in a way that invalidates then-live raw `*mut T`s according to the | ||
| /// rules of "Stacked Borrows", which can lead to *[undefined behavior]*. |
There was a problem hiding this comment.
I'm still not really convinced this is currently the case, rather than a proposed rule...
There was a problem hiding this comment.
Right, stacked borrows is a proposed aliasing model, rather than an accepted one. There are things in the language that it does not properly model, and so it probably cannot be the final one. I think we can probably be a lot more vague about what is UB here if needed, though (see https://doc.rust-lang.org/nightly/core/cell/struct.UnsafeCell.html).
Although I'm not sure that's needed: I'd expect it to have rules derived from a combination of the rules of https://doc.rust-lang.org/std/ptr/fn.write.html (which heavily leans on https://doc.rust-lang.org/std/ptr/index.html) and the rules of https://doc.rust-lang.org/std/primitive.pointer.html#method.offset
There was a problem hiding this comment.
As there is no rule that is known to be true,
there is also no rule that is certain to be false.
therefore, in the interests of users not having to rewrite their code
we should advise them to duck the maybe-brick that may or may not collide with their skull.
It is true the user's cat may still be alive if we open the box,
or perhaps the user will be a butterfly.
There was a problem hiding this comment.
Ah yes, I need to specify the aligned/non-null? clause.
There was a problem hiding this comment.
I think it is even more confusing to bring up rules that don't currently exist. This is certainly more strict than LLVM's rules so I don't think it's right to portray it as the current state of rust. In my opinion scatter is just one of many, many APIs that use pointers and isn't special enough to warrant any explanation at all. I think the simplest and most correct way to document this is to specify that it follows all the same requirements as ptr::write (whatever those may be at any particular point in time).
There was a problem hiding this comment.
Lokathor is correct, yes. I just think it is important to reiterate the warning in appropriately flamingly large text, because 99% of the time this will be called with *mut T being derived from &mut [T], but if I say that, then everyone complains at me that Miri doesn't issue a warning on that code.
The oddity of the (old) internal comments being distracting in the presence of the (new) external documentation is merely a relic of me focusing on trying to be agonizingly clear in the external documentation.
There was a problem hiding this comment.
Okay, I see -- thanks.
I think people were mostly complaining that because this is such a general issue, it seems odd to have it explained in user-visible documentation in this particular function. If we want to warn people in the docs about these patterns which Stacked Borrows disallows (but which other future aliasing models might allow), we should probably do that more centrally and more consistently.
There was a problem hiding this comment.
Sure. But my primary concern isn't really if all of core::ptr is properly documented, it's that one method, core::simd::Simd::scatter_select_unchecked, is properly documented, one likely to be used by people who don't want to read a ton of general documentation.
If that is truly the concern, then we could have figured out where to move the docs from this method (and replace it with links to that location) after this PR.
There was a problem hiding this comment.
If that is truly the concern, then we could have figured out where to move the docs from this method (and replace it with links to that location) after this PR.
That seems to be assuming the consensus that we want to have Stacked Borrows specific docs in the official doc comments at all, which AFAIK is an open question.
There was a problem hiding this comment.
And so we come full circle to "But as far as I have observed, the users of unsafe code complain most about the prospect of having to rewrite that code based on future model changes."
¯\_(ツ)_/¯
|
Maybe unchecked gather should use a pointer, as well? |
|
I quite like the new scatter API, cuz it's general enough to let a user construct their own poor-man's vector of pointers: pub struct SimdPtr<T, const N: usize>(Simd<usize, N>, PhantomData<*mut T>);
impl SimdPtr<T, const N: usize> where ... {
pub unsafe fn scatter_store(self, values: Simd<T, N>, mask: Mask<isize, N>) {
values.scatter_select_unchecked(null_mut(), mask, self.0);
}
} |
This is unsound because you are producing a null Please at least enable us to use raw pointers for this purpose.. |
|
That never creates any slice, let alone a null slice, so it's not UB. Using a null base pointer is however incompatible with strict provenance. We do intend on allowing vectors of raw pointers (see #200). |
|
portable-simd/crates/core_simd/src/vector.rs Lines 409 to 411 in fcc5ca0 |
This PR makes
unsafe fn scatter_select_uncheckedmore unsafe, by allowing it to accept a raw*mut T, and thus exposing all the invariants underneath the previous function but also allowing more uses. It then addsscatter_uninitas a variant onscatter_select, for the safe scatter to&mut [MaybeUninit<T>].This closes #205.