Implement MaybeUninit
#53508
Implement MaybeUninit
#53508
Conversation
rust-highfive
added
the
S-waiting-on-review
| /// Note that dropping a `MaybeUninit` will never call `T`'s drop code. | ||
| /// It is your responsibility to make sure `T` gets dropped if it got initialized. | ||
| #[unstable(feature = "maybe_uninit", issue = "53491")] | ||
| pub const fn uninitialized() -> MaybeUninit<T> { |
There was a problem hiding this comment.
Do we need a rustc_const_unstable attribute here as well? Or at least make a note to do so eventually?
There was a problem hiding this comment.
Why not stabilize as const fn?
src/libcore/ptr.rs
Outdated
|
|
||
| // Perform the swap | ||
| copy_nonoverlapping(x, &mut tmp, 1); | ||
| copy_nonoverlapping(x, tmp.get_mut(), 1); |
There was a problem hiding this comment.
Shouldn't this be using as_mut_ptr?
src/libcore/ptr.rs
Outdated
| copy_nonoverlapping(src, &mut tmp, 1); | ||
| tmp | ||
| let mut tmp = MaybeUninit::<T>::uninitialized(); | ||
| copy_nonoverlapping(src, tmp.get_mut(), 1); |
There was a problem hiding this comment.
This also looks like it should be as_mut_ptr? tmp is not initialized at this point.
src/libcore/slice/rotate.rs
Outdated
| let rawarray = RawArray::new(); | ||
| let buf = rawarray.ptr(); | ||
| let rawarray = MaybeUninit::<RawArray<T>>::uninitialized(); | ||
| let buf = rawarray.get_ref().ptr(); |
There was a problem hiding this comment.
Isn't this also not legitimate code? get_ref is only defined as far as I can tell on initialized memory, but it's not initialized here.
src/libcore/slice/sort.rs
Outdated
| start_l = offsets_l.as_mut_ptr(); | ||
| end_l = offsets_l.as_mut_ptr(); | ||
| start_l = unsafe { offsets_l.get_mut().as_mut_ptr() }; | ||
| end_l = unsafe { offsets_l.get_mut().as_mut_ptr() }; |
There was a problem hiding this comment.
Isn't this also invalid? I believe offsets_l is uninitialized at this point?
src/libcore/slice/sort.rs
Outdated
| @@ -288,8 +288,8 @@ fn partition_in_blocks<T, F>(v: &mut [T], pivot: &T, is_less: &mut F) -> usize | |||
|
|
|||
| if start_r == end_r { | |||
| // Trace `block_r` elements from the right side. | |||
| start_r = offsets_r.as_mut_ptr(); | |||
| end_r = offsets_r.as_mut_ptr(); | |||
| start_r = unsafe { offsets_r.get_mut().as_mut_ptr() }; | |||
There was a problem hiding this comment.
This code also looks like this should be invalid? At least, offsets_r is not necessarily initialized, right?
| keys: [K; CAPACITY], | ||
| vals: [V; CAPACITY], | ||
| keys: MaybeUninit<[K; CAPACITY]>, | ||
| vals: MaybeUninit<[V; CAPACITY]>, |
There was a problem hiding this comment.
Based on the comment above, should this instead be [MaybeUninit<V>; CAPACITY]?
(That might be a pain, though, and this is no worse than before, so might not be this PR.)
There was a problem hiding this comment.
That seems like a pretty equivalent type to me. Which comment above?
There was a problem hiding this comment.
Should we document that you can partially-initialize MaybeUninit, i.e. that it "distributes over products"?
There was a problem hiding this comment.
I guess this is forth saying, yes.
Unfortunately actually implementing this is not easy, same problem as for Cell (which also distributes over products).
|
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 |
| @@ -784,6 +784,14 @@ impl Abi { | |||
| _ => false, | |||
| } | |||
| } | |||
|
|
|||
| /// Returns true if this is an uninhabited type | |||
| pub fn is_uninhabited(&self) -> bool { | |||
There was a problem hiding this comment.
For some reason I would've avoided this but I guess it's fine.
Could you also go through rg '==.*::Uninhabited' and change all of those to use is_uninhabited?
| } else { | ||
| "Attempted to instantiate an uninhabited type (e.g. `!`) \ | ||
| using mem::uninitialized()" | ||
| }; |
There was a problem hiding this comment.
You can actually use format!("... uninhabited type {} using ...", sig.output()) here, heh.
| @@ -462,6 +462,55 @@ impl FunctionCx<'a, 'll, 'tcx> { | |||
| return; | |||
| } | |||
|
|
|||
| if (intrinsic == Some("init") || intrinsic == Some("uninit")) && | |||
| bx.cx.layout_of(sig.output()).abi.is_uninhabited() | |||
There was a problem hiding this comment.
Can you move this below let fn_ty? Then you can use fn_ty.ret.layout instead of calling layout_of.
| panic::catch_unwind(|| mem::zeroed::<!>()).is_err(); | ||
|
|
||
| panic::catch_unwind(|| mem::uninitialized::<Foo>()).is_err(); | ||
| panic::catch_unwind(|| mem::zeroed::<Foo>()).is_err(); |
There was a problem hiding this comment.
I think you can also do:
assert_eq!(....downcast_ref::<&'static str>(), Some("panic message"));
|
Looks like the gdb pretty-printing scripts make assumptions about the fields of libstd datastructures: rust/src/etc/debugger_pretty_printers_common.py Lines 392 to 401 in 6bf6d50 |
| @@ -567,7 +567,7 @@ impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Immut<'a>, K, V, Type> { | |||
| // the node, which is allowed by LLVM. | |||
| unsafe { | |||
| slice::from_raw_parts( | |||
| self.as_leaf().keys.as_ptr(), | |||
| self.as_leaf().keys.get_ref().as_ptr(), | |||
There was a problem hiding this comment.
Here and below, can't you use MaybeUninit::as_ptr/MaybeUninit::as_mut_ptr?
| let formatted = flt2dec::to_exact_fixed_str(flt2dec::strategy::grisu::format_exact, | ||
| *num, sign, precision, | ||
| false, &mut buf, &mut parts); | ||
| false, buf.get_mut(), parts.get_mut()); |
There was a problem hiding this comment.
Another case of using &mut as &out.
| struct Foo { | ||
| x: u8, | ||
| y: !, | ||
| } |
There was a problem hiding this comment.
Could you add a test for an empty enum?
Also, AFAIK Foo is NOT layout-uninhabited, because of #49298?
There was a problem hiding this comment.
Correct, you need to change x's type to ().
There was a problem hiding this comment.
Wait, no, check this out:
#![feature(never_type)]
pub struct Foo { x: u8, y: ! }
pub fn foo() -> Foo { loop {} }That gets noreturn in the LLVM IR, with u8 or () in the first field, but if you change the ! to (), you lose the noreturn.
What happened in the first for #49298 is we kept the layout-uninhabited property propagated, but it wouldn't affect type sizes the same.
Which means we can use the check here to detect noreturn UB in all relevant cases!
There was a problem hiding this comment.
nocall would be more correct than noreturn though :P
Which "check here"?
|
☔ The latest upstream changes (presumably #53530) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I finally wrote that blog post about whether and when |
|
I believe I have addressed all the review comments, except for the one about gdb scripts -- I'll check that one later. Let me know if I missed something! |
|
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 |
|
Ping from triage @RalfJung! This PR needs your review. |
|
☔ The latest upstream changes (presumably #53227) made this pull request unmergeable. Please resolve the merge conflicts. |
|
There are still several uses of Otherwise, r=me once Travis is happy with the gdb stuff. |
RalfJung
added
S-waiting-on-author
|
Sorry for the delay! I'm looking again at this. This is one of the debuginfo tests that's failing: rust/src/test/debuginfo/nil-enum.rs Lines 30 to 43 in fea32f1 The test is instantiating nil enums and then inspecting them in GDB. The program now panics with the changes mode in this PR so the test broke. I'm not quite sure what to do here. Should I instantiate |
|
Yeah that test is definitely UB. It might still be worth seeing how gbd displays that. So maybe add a comment saying it is UB, and then use |
japaric
force-pushed
the
maybe-uninit
branch
from
a56ee4e
to
2327942
Compare
|
Rebased and fixed the debuginfo tests in the last two commits. Regarding using I have explored the two options by re-implementing one function using both approaches. I have pushed my work in two branches which are linked above. Neither feels ergonomic (the first approach actually feels wrong because it constrains the argument) but the second option is slightly better except for the fact that instantiating |
|
Is it UB to transmute |
|
No, definitely not. |
|
It's a significant perf regression: @japaric: can you investigate? Here are docs on benchmarking and profiling. Cachegrind can be particularly useful for examining the difference between two compiler versions. |
|
On second thought, this regression is bad enough that I think we should consider backing the changes out. |
|
CC @rust-lang/wg-compiler-performance |
|
I support backing this change out to investigate performance losses. |
|
Fine for me. I assume the regression comes from one of the places where |
|
Revert submitted as #54554 |
Revert most of MaybeUninit, except for the new API itself This reverts most of #53508 for perf reasons (first commit reverts that entire PR), except for the new API itself (added back in 2nd commit).
This PR:
MaybeUninit(see Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type" #53491) to{core,std}::mem.mem::{uninitialized,zeroed}panic when they are used to instantiate an uninhabited type.mem::{uninitialized,zeroed}just yet. As per Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type" #53491 (comment), we should not deprecate them untilMaybeUninitis stabilized.mem::{uninitialized,zeroed}in core and alloc withMaybeUninit.There are still several instances of
mem::{uninitialized,zeroed}instdthat this PR doesn't address.r? @RalfJung
cc @eddyb you may want to look at the new panicking logic