Add comment about UnsafeCell in MaybeUninit #66051
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
This touches on #65216 a little bit |
|
r? @RalfJung |
src/libcore/mem/maybe_uninit.rs
Outdated
| @@ -324,7 +324,7 @@ impl<T> MaybeUninit<T> { | |||
| /// Gets a pointer to the contained value. Reading from this pointer or turning it | |||
| /// into a reference is undefined behavior unless the `MaybeUninit<T>` is initialized. | |||
| /// Writing to memory that this pointer (non-transitively) points to is undefined behavior | |||
| /// (except inside an `UnsafeCell<T>`). | |||
| /// (except inside an `UnsafeCell<T>` when already initialized). | |||
There was a problem hiding this comment.
I don't really understand the goal of this change. It is also not correct; the interior mutability exception is entirely orthogonal to initialization.
Could you elaborate a bit more?
There was a problem hiding this comment.
Hm, maybe this change isn't helping to make things more clear...
This is roughly what I was doing:
let value: MaybeUninit<UnsafeCell<bool>> = MaybeUninit::uninit();
let slot = (&*value.as_ptr()).get() as *mut T;
slot.write(true);But because UnsafeCell::get takes a reference, this would be invalid, right? There would temporary be a reference to an uninitialized UnsafeCell<bool>.
So I would think the exception that you can write through MaybeUninit::as_ptr when it wraps an UnsafeCell would only be correct if it was already initialized.
There was a problem hiding this comment.
So I would think the exception that you can write through MaybeUninit::as_ptr when it wraps an UnsafeCell would only be correct if it was already initialized.
No, the exception always applies. It's just you cannot make use of it through UnsafeCell::get. When writing unsafe code, you have to get all clauses of all constructs you use satisfied. Your example is fine as far as MaybeUninit::as_ptr is concerned -- it it what you are doing later that breaks it. Under some proposed memory model, if you use transmute to turn *const UnsafeCell<bool> into *mut bool, that is allowed and then you may use ptr:.write to initialize the bool.
Why just "touches on"? Looks like, if done right, this PR should resolve that issue? |
|
I wouldn't have thought about transmuting the I'll see if adding a note + example to |
In my opinion it would be so much more clearifying when there's a little code example with a But as you said here #65216 (comment), there are alot of other What I think could maybe solve this is to put some examples in the Nomicon, and just link to that in the docs. The Nomicon can then explain all the dark magic that's going on, and why it's actually sound. |
|
Cc @danielhenrymantilla who's also currently doing some MaybeUninit docs work. |
|
The thing is that transmuting the We probably need a |
Agreed! |
Agreed; once we have a blessed way of doing that we should have an example here. I originally thought the confusion was about |
|
I just finished a documentation example, but it seems you guys have bigger concerns and idea's. Maybe it is better to close this PR? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| /// | ||
| /// If you have a raw reference to an `UnsafeCell` that can't be turned into a shared reference | ||
| /// (because for example the wrapped value may be uninitialized), use a pointer cast instead of | ||
| /// `get`: |
There was a problem hiding this comment.
I don't think we should bless this pattern yet, while the memory model is still so much in flux.
I like your other improvements, but for the interaction with UnsafeCell I'm afraid just changing docs won't cut it at this point.
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: Thanks |
|
@JohnCSimon Thank you for the reminder. I think it is better to close this PR for now. |
…onSapin add raw ptr variant of UnsafeCell::get This has come up recently in rust-lang#66051 (Cc @Centril @pitdicker) as well as in discussion with @nikomatsakis and in unrelated discussion with @withoutboats.
…onSapin add raw ptr variant of UnsafeCell::get This has come up recently in rust-lang#66051 (Cc @Centril @pitdicker) as well as in discussion with @nikomatsakis and in unrelated discussion with @withoutboats.
The mention of
UnsafeCellinMaybeUninit::as_ptrmade me thinkMaybeUninit<UnsafeCell<T>>could be a valid alternative toUnsafeCell<MaybeUninit<T>>. But initialization viaas_ptrjust doesn't work out. Stumbled upon in matklad/once_cell#72 (comment).