Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement MaybeUninit #53508

Merged
merged 19 commits into from
Sep 23, 2018
Merged

Implement MaybeUninit #53508

merged 19 commits into from
Sep 23, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Aug 19, 2018

This PR:

There are still several instances of mem::{uninitialized,zeroed} in std that this PR doesn't address.

r? @RalfJung
cc @eddyb you may want to look at the new panicking logic

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2018
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there might be a few cases where we're not performing the right change here?

/// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a rustc_const_unstable attribute here as well? Or at least make a note to do so eventually?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not stabilize as const fn?


// Perform the swap
copy_nonoverlapping(x, &mut tmp, 1);
copy_nonoverlapping(x, tmp.get_mut(), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be using as_mut_ptr?

copy_nonoverlapping(src, &mut tmp, 1);
tmp
let mut tmp = MaybeUninit::<T>::uninitialized();
copy_nonoverlapping(src, tmp.get_mut(), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like it should be as_mut_ptr? tmp is not initialized at this point.

let rawarray = RawArray::new();
let buf = rawarray.ptr();
let rawarray = MaybeUninit::<RawArray<T>>::uninitialized();
let buf = rawarray.get_ref().ptr();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this also invalid? I believe offsets_l is uninitialized at this point?

@@ -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() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a pretty equivalent type to me. Which comment above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document that you can partially-initialize MaybeUninit, i.e. that it "distributes over products"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is forth saying, yes.

Unfortunately actually implementing this is not easy, same problem as for Cell (which also distributes over products).

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:52:51] ....................................................................................................
[00:52:54] ....................................................................................................
[00:52:57] .....i..............................................................................................
[00:53:00] ....................................................................................................
[00:53:03] ......................................................iiiiiiiii.....................................
[00:53:08] ....................................................................................................
[00:53:12] ....................................................................................................
[00:53:15] ...................................i................................................................
[00:53:17] .....................................................................................i.i..ii........
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-gdb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:01:01] 
[01:01:01] running 110 tests
[01:01:12] iiii.......i..i........i..i.i.............i..........iiii...........i.F..i....F.....ii.i.i.......ii.
[01:01:13] failures:
[01:01:13] 
[01:01:13] ---- [debuginfo-gdb] debuginfo/nil-enum.rs stdout ----
[01:01:13] ---- [debuginfo-gdb] debuginfo/nil-enum.rs stdout ----
[01:01:13] NOTE: compiletest thinks it is using GDB without native rust support
[01:01:13] NOTE: compiletest thinks it is using GDB version 7011001
[01:01:13] 
[01:01:13] error: line not found in debugger output: $1 = {<No data fields>}
[01:01:13] status: exit code: 0
[01:01:13] command: "/usr/bin/gdb" "-quiet" "-batch" "-nx" "-command=/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/nil-enum/nil-enum.debugger.script"
[01:01:13] ------------------------------------------
[01:01:13] GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
[01:01:13] Copyright (C) 2016 Free Software Foundation, Inc.
[01:01:13] Copyright (C) 2016 Free Software Foundation, Inc.
[01:01:13] License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
[01:01:13] This is free software: you are free to change and redistribute it.
[01:01:13] There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
[01:01:13] and "show warranty" for details.
[01:01:13] This GDB was configured as "x86_64-linux-gnu".
[01:01:13] Type "show configuration" for configuration details.
[01:01:13] For bug reporting instructions, please see:
[01:01:13] <http://www.gnu.org/software/gdb/bugs/>.
[01:01:13] Find the GDB manual and other documentation resources online at:
[01:01:13] <http://www.gnu.org/software/gdb/documentation/>.
[01:01:13] For help, type "help".
[01:01:13] Type "apropos word" to search for commands related to "word".
[01retty-std-collections.debugger.script"
[01:01:13] ------------------------------------------
[01:01:13] GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
[01:01:13] Copyright (C) 2016 Free Software Foundation, Inc.
[01:01:13] Copyright (C) 2016 Free Software Foundation, Inc.
[01:01:13] License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
[01:01:13] This is free software: you are free to change and redistribute it.
[01:01:13] There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
[01:01:13] and "show warranty" for details.
[01:01:13] This GDB was configured as "x86_64-linux-gnu".
[01:01:13] Type "show configuration" for configuration details.
[01:01:13] For bug reporting instructions, please see:
[01:01:13] <http://www.gnu.org/software/gdb/bugs/>.
[01:01:13] Find the GDB manual and other documentation resources online at:
[01:01:13] <http://www.gnu.org/software/gdb/documentation/>.
[01:01:13] For help, type "help".
[01:01:13] Type "apropos word" to search for commands related to "word".
[01:01:13] Breakpoint 1 at 0x144f6: file /checkout/src/test/debuginfo/pretty-std-collections.rs, line 57.
[01:01:13] [Thread debugging using libthread_db enabled]
[01:01:13] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[01:01:13] 
[01:01:13] Breakpoint 1, pretty_std_collections::main::h617474859dc5b54e () at /checkout/src/test/debuginfo/pretty-std-collections.rs:57
[01:01:13] 57     zzz(); // #break
[01:01:13] $1 = BTreeSet<i32>(len: 3)
[01:01:13] $2 = BTreeMap<i32, i32>(len: 3)
[01:01:13] $3 = VecDeque<i32>(len: 3, cap: 8) = {5, 3, 7}
[01:01:13] A debugging session is active.
[01:01:13] 
[01:01:13]  Inferior 1 [process 15246] will be killed.
[01:01:13] 
[01:01:13] Quit anyway? (y or n) [answered Y; input not from terminal]
[01:01:13] ------------------------------------------
[01:01:13] stderr:
[01:01:13] ------------------------------------------
[01:01:13] ------------------------------------------
[01:01:13] Python Exception <class 'gdb.error'> Cannot subscript requested type.: 
[01:01:13] Python Exception <class 'gdb.error'> Cannot subscript requested type.: 
[01:01:13] ------------------------------------------
[01:01:13] 
[01:01:13] thread '[debuginfo-gdb] debuginfo/pretty-std-collections.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3189:9
[01:01:13] 
---
[01:01:13] 
[01:01:13] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[01:01:13] 
[01:01:13] 
[01:01:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/debuginfo" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "debuginfo-gdb" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:01:13] 
[01:01:13] 
[01:01:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:01:13] Build completed unsuccessfully in 0:12:31
[01:01:13] Build completed unsuccessfully in 0:12:31
[01:01:13] make: *** [check] Error 1
[01:01:13] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ef71254
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.
travis_fold:start:after_failure.1
travis_time:start:0aaf1540
$ echo/bootstrap-1v3ifugz4t07z/s-f40eeanft4-1h7wppw-1iplbejydu42a
128740 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
128648 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
128644 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
125880 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps

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 @TimNN. (Feature Requests)

@@ -784,6 +784,14 @@ impl Abi {
_ => false,
}
}

/// Returns true if this is an uninhabited type
pub fn is_uninhabited(&self) -> bool {
Copy link
Member

@eddyb eddyb Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()"
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

@eddyb eddyb Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also do:

assert_eq!(....downcast_ref::<&'static str>(), Some("panic message"));

@eddyb
Copy link
Member

eddyb commented Aug 20, 2018

Looks like the gdb pretty-printing scripts make assumptions about the fields of libstd datastructures:

def extract_length_and_ptr_from_std_btreemap(vec_val):
assert vec_val.type.get_type_kind() == TYPE_KIND_STD_BTREEMAP
root = vec_val.get_child_at_index(0)
length = vec_val.get_child_at_index(1).as_integer()
node = root.get_child_at_index(0)
ptr = node.get_child_at_index(0)
unique_ptr_val = ptr.get_child_at_index(0)
data_ptr = unique_ptr_val.get_child_at_index(0)
assert data_ptr.type.get_dwarf_type_kind() == DWARF_TYPE_CODE_PTR
return (length, data_ptr)

@@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another case of using &mut as &out.

struct Foo {
x: u8,
y: !,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for an empty enum?

Also, AFAIK Foo is NOT layout-uninhabited, because of #49298?

Copy link
Member

@eddyb eddyb Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, you need to change x's type to ().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nocall would be more correct than noreturn though :P

Which "check here"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the panicking intrinsics.

@bors
Copy link
Contributor

bors commented Aug 21, 2018

☔ The latest upstream changes (presumably #53530) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

I finally wrote that blog post about whether and when &mut T must be initialized.

@japaric
Copy link
Member Author

japaric commented Aug 23, 2018

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!

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:49:22] ....................................................................................................
[00:49:25] ....................................................................................................
[00:49:28] ........i...........................................................................................
[00:49:31] ....................................................................................................
[00:49:34] .........................................................iiiiiiiii..................................
[00:49:39] ....................................................................................................
[00:49:43] ....................................................................................................
[00:49:46] ......................................i.............................................................
[00:49:49] ........................................................................................i.i..ii.....
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-gdb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:19] 
[00:57:19] running 110 tests
[00:57:30] iiii.......i..i........i..i.i.............i...........iiii..........i.F..i....F.....ii.i.i.......ii.
[00:57:31] failures:
[00:57:31] 
[00:57:31] ---- [debuginfo-gdb] debuginfo/nil-enum.rs stdout ----
[00:57:31] ---- [debuginfo-gdb] debuginfo/nil-enum.rs stdout ----
[00:57:31] NOTE: compiletest thinks it is using GDB without native rust support
[00:57:31] NOTE: compiletest thinks it is using GDB version 7011001
[00:57:31] 
[00:57:31] error: line not found in debugger output: $1 = {<No data fields>}
[00:57:31] status: exit code: 0
[00:57:31] command: "/usr/bin/gdb" "-quiet" "-batch" "-nx" "-command=/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/nil-enum/nil-enum.debugger.script"
[00:57:31] ------------------------------------------
[00:57:31] GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
[00:57:31] Copyright (C) 2016 Free Software Foundation, Inc.
[00:57:31] Copyright (C) 2016 Free Software Foundation, Inc.
[00:57:31] License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
[00:57:31] This is free software: you are free to change and redistribute it.
[00:57:31] There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
[00:57:31] and "show warranty" for details.
[00:57:31] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[00:57:31] This GDB was configured as "x86_64-linux-gnu".
[00:57:31] Type "show configuration" for configuration details.
[00:57:31] For bug reporting instructions, please see:
[00:57:31] <http://www.gnu.org/software/gdb/bugs/>.
[00:57:31] Find the GDB manual and other documentation resources online at:
[00:57:31] <http://www.gnu.org/software/gdb/documentation/>.
[00:57:31] For help, type "help".
[00:57:31] Type "apropos word" to search for commands related to "word".
[00:57:31] Breakpoint 1 at 0xb5e: file /checkout/src/test/debuginfo/nil-enum.rs, line 41.
[00:57:31] [Thread debugging using libthread_db enabled]
[00:57:31] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[00:57:31] [Inferior 1 (process 15233) exited with code 0145]
[00:57:31] ------------------------------------------
[00:57:31] stderr:
[00:57:31] ------------------------------------------
[00:57:31] ------------------------------------------
[00:57:31] thread 'main' panicked at 'Attempted to instantiate uninhabited type ANilEnum using mem::zeroed', /checkout/src/libcore/mem.rs:526:5
[00:57:31] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:57:31] /checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/nil-enum/nil-enum.debugger.script:9: Error in sourced command file:
[00:57:31] No symbol "first" in current context.
[00:57:31] ------------------------------------------
[00:57:31] 
[00:57:31] thread '[debuginfo-gdb] debuginfo/nil-enum.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3196:9
[00:57:31] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:57:31] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:57:31] 
[00:57:31] ---- [debuginfo-gdb] debuginfo/pretty-std-collections.rs stdout ----
[00:57:31] NOTE: compiletest thinks it is using GDB without native rust support
[00:57:31] NOTE: compiletest thinks it is using GDB version 7011001
[00:57:31] 
[00:57:31] error: line not found in debugger output: $1 = BTreeSet<i32>(len: 3) = {3, 5, 7}
[00:57:31] status: exit code: 0
[00:57:31] command: "/usr/bin/gdb" "-quiet" "-batch" "-nx" "-com32>(len: 3, cap: 8) = {5, 3, 7}
[00:57:31] A debugging session is active.
[00:57:31] 
[00:57:31]  Inferior 1 [process 15413] will be killed.
[00:57:31] 
[00:57:31] Quit anyway? (y or n) [answered Y; input not from terminal]
[00:57:31] ------------------------------------------
[00:57:31] stderr:
[00:57:31] ------------------------------------------
[00:57:31] ------------------------------------------
[00:57:31] Python Exception <class 'gdb.error'> Cannot subscript requested type.: 
[00:57:31] Python Exception <class 'gdb.error'> Cannot subscript requested type.: 
[00:57:31] ------------------------------------------
[00:57:31] 
[00:57:31] thread '[debuginfo-gdb] debuginfo/pretty-std-collections.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3196:9
[00:57:31] 
---
[00:57:31] test result: FAILED. 84 passed; 2 failed; 24 ignored; 0 measured; 0 filtered out
[00:57:31] 
[00:57:31] 
[00:57:31] 
[00:57:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/debuginfo" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "debuginfo-gdb" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:57:31] 
[00:57:31] 
[00:57:31] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:57:31] Build completed unsuccessfully in 0:12:10
[00:57:31] Build completed unsuccessfully in 0:12:10
[00:57:31] Makefile:58: recipe for target 'check' failed
[00:57:31] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1f3abcda
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:298d5800:start=1535045034615376822,finish=1535045034696368596,duration=80991774
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0b1189f0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:01bf1338
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@pietroalbini
Copy link
Member

Ping from triage @RalfJung! This PR needs your review.

@bors
Copy link
Contributor

bors commented Aug 28, 2018

☔ The latest upstream changes (presumably #53227) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

There are still several uses of &mut as &out in src/libcore/fmt/float.rs -- if the rules in the MaybeUninit doc are strictly applied, that could would be UB. However, it is certainly no more UB than it was before this PR, so I am included to accept anyway. What would be interesting to know (as a data point for the unsafe code guidelines around &mut) is how hard it would be to change that code to use stretch the (current) rules. @japaric can you have a look?

Otherwise, r=me once Travis is happy with the gdb stuff.

@RalfJung RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2018
@japaric
Copy link
Member Author

japaric commented Sep 1, 2018

Sorry for the delay! I'm looking again at this. This is one of the debuginfo tests that's failing:

enum ANilEnum {}
enum AnotherNilEnum {}
// This test relies on gdbg printing the string "{<No data fields>}" for empty
// structs (which may change some time)
// The error from gdbr is expected since nil enums are not supposed to exist.
fn main() {
unsafe {
let first: ANilEnum = ::std::mem::zeroed();
let second: AnotherNilEnum = ::std::mem::zeroed();
zzz(); // #break
}
}

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 MaybeUninit<ANilEnum> and inspect that? (Would that make the test moot since it would become a union test?). Should I continue to instantiate ANilEnum using MaybeUninit::zeroed().into_inner()? (Sounds bad because UB) Or should I just remove the test? (because no one should be instantiating nil enums anyway).

cc @michaelwoerister

@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2018

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 MaybeUninit::uninitialized().into_inner()? That replicates the previous code as closely as possible.

@japaric
Copy link
Member Author

japaric commented Sep 1, 2018

Rebased and fixed the debuginfo tests in the last two commits. Regarding using MaybeUninit in fmt/float.rs I see two ways to replace &mut [T] in argument position where the passed buffer is uninitialized:

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 [MaybeUninit<T>; N] requires writing MaybeUninit::uninitialized() N times. Could we make the compiler accept [MaybeUninit<T>; N]?

@abonander
Copy link
Contributor

Is it UB to transmute MaybeUninit<[T; N]> to [MaybeUninit<T>; N]? That'd be a more convenient way to get the latter.

@RalfJung
Copy link
Member

No, definitely not.

@nnethercote
Copy link
Contributor

It's a significant perf regression:

tokio-webpush-simple-opt
	avg: 14.3%	min: 1.5%	max: 37.6%
issue-46449-opt
	avg: 8.4%	min: 1.3%	max: 13.1%
script-servo-opt
	avg: 4.1%	min: 0.8%	max: 6.4%
issue-46449-debug
	avg: 4.1%	min: 1.0%	max: 4.8%
cargo-opt
	avg: 3.5%	min: 1.4%	max: 4.7%
webrender-opt
	avg: 2.4%	min: 1.1%	max: 3.5%
ripgrep-opt
	avg: 1.8%	min: 1.0%	max: 3.4%
futures-opt
	avg: 1.8%	min: 0.4%	max: 3.4%
script-servo-debug
	avg: 1.8%	min: 1.1%	max: 3.3%
sentry-cli-opt
	avg: 2.3%	min: 1.0%	max: 3.0%
regex-opt
	avg: 1.7%	min: 0.5%	max: 2.9%
tokio-webpush-simple-debug
	avg: 2.3%	min: 2.0%	max: 2.8%
inflate-opt
	avg: -0.9%?	min: -2.3%?	max: 0.7%?
clap-rs-debug
	avg: 0.9%	min: 0.5%	max: 2.1%
ctfe-stress-opt
	avg: -0.8%?	min: -2.1%?	max: 0.5%?
hyper-opt
	avg: 1.2%	min: 0.5%	max: 2.1%
ripgrep-debug
	avg: 1.7%	min: 1.2%	max: 2.0%
futures-debug
	avg: 1.1%	min: 0.4%	max: 2.0%
crates.io-opt
	avg: 1.2%	min: 0.5%	max: 1.9%
html5ever-opt
	avg: 0.6%	min: -0.1%	max: 1.8%
sentry-cli-debug
	avg: 1.2%	min: 0.8%	max: 1.8%
clap-rs-check
	avg: 1.0%	min: 0.6%	max: 1.7%
deeply-nested-check
	avg: 1.2%	min: 0.0%	max: 1.7%
piston-image-opt
	avg: 1.2%	min: 1.0%	max: 1.7%
serde-debug
	avg: 1.1%	min: 0.4%	max: 1.7%
serde-check
	avg: 1.2%	min: 0.2%	max: 1.6%
serde-opt
	avg: 1.0%	min: 0.3%	max: 1.6%
encoding-opt
	avg: 1.0%	min: 0.3%	max: 1.6%

@japaric: can you investigate? Here are docs on benchmarking and profiling. Cachegrind can be particularly useful for examining the difference between two compiler versions.

@nnethercote
Copy link
Contributor

On second thought, this regression is bad enough that I think we should consider backing the changes out.

@nnethercote
Copy link
Contributor

nnethercote commented Sep 25, 2018

CC @rust-lang/wg-compiler-performance

@Mark-Simulacrum
Copy link
Member

I support backing this change out to investigate performance losses.

@RalfJung
Copy link
Member

Fine for me. I assume the regression comes from one of the places where mem::uninitialized was replaced by MaybeUninit, so we could then land the new union separately from the part which starts to make use of it in the compiler.

@RalfJung
Copy link
Member

Revert submitted as #54554

RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 29, 2018
…ung"

This reverts commit c6e3d7f, reversing
changes made to 4591a24.
bors added a commit that referenced this pull request Sep 29, 2018
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).
bors added a commit that referenced this pull request Oct 1, 2018
Panic when using mem::uninitialized or mem::zeroed on an uninhabited type

All code by @japaric. This re-submits one half of #53508. This is likely not the one that introduced the perf regression, but just to be sure I'll do a perf run anyway.
bors added a commit that referenced this pull request Oct 8, 2018
Use MaybeUninit

All code by @japaric. This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.
bors added a commit that referenced this pull request Oct 9, 2018
Use MaybeUninit in liballoc

All code by @japaric. This is a re-submission of a part of #53508 that hopefully does not regress performance.
bors added a commit that referenced this pull request Oct 10, 2018
Use MaybeUninit in libcore

All code by @japaric. This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.
bors added a commit that referenced this pull request Oct 12, 2018
Use MaybeUninit in liballoc

All code by @japaric. This is a re-submission of a part of #53508 that hopefully does not regress performance.
bors added a commit that referenced this pull request Nov 26, 2018
Use MaybeUninit in libcore

All code by @japaric. This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.