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

Kill remaining uses of mem::uninitialized in libcore, liballoc #57045

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@RalfJung
Copy link
Member

RalfJung commented Dec 21, 2018

Kill remaining uses of mem::uninitialized in libcore and liballoc, and enable a lint that will warn when uses are added again in the future.

To avoid casting raw pointers around (which is always very dangerous because it is not typechecked at all -- it doesn't even get the "same size" sanity check that transmute gets), I also added two new functions to MaybeUninit:

    /// Get a pointer to the first contained values.
    pub fn first_ptr(this: &[MaybeUninit<T>]) -> *const T {
        this as *const [MaybeUninit<T>] as *const T
    }

    /// Get a mutable pointer to the first contained values.
    pub fn first_mut_ptr(this: &mut [MaybeUninit<T>]) -> *mut T {
        this as *mut [MaybeUninit<T>] as *mut T
    }

I changed some of the existing code to use array-of-MaybeUninit instead of MaybeUninit-of-array, successfully removing raw pointer casts there as well.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 21, 2018

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Dec 21, 2018

I was surprised recently to find that MaybeUninit is still unstable. Perhaps it would be a good idea to first let it go stable before migrating the whole standard library to it?

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 21, 2018

I think it makes a lot of sense to migrate more things to it to get some experience with the API before stabilizing.

// `MaybeUninit<[MaybeUninit; N]>`; the `into_inner` is safe because the inner
// array does not require initialization.
keys: MaybeUninit::uninitialized().into_inner(),
vals: MaybeUninit::uninitialized().into_inner(),

This comment has been minimized.

@Centril

Centril Dec 21, 2018

Contributor

Might want to leave a FIXME for when we get #49147 (and in subsequent applicable places)?

// Creating a `[MaybeUninit; N]` array by first creating a
// `MaybeUninit<[MaybeUninit; N]>`; the `into_inner` is safe because the inner
// array does not require initialization.
keys: MaybeUninit::uninitialized().into_inner(),

This comment has been minimized.

@cramertj

cramertj Dec 21, 2018

Member

why do I have a terrible feeling I'm going to be reading MaybeUninit::uninitialized().into_inner() everywhere rather than mem::uninitialized()...

This comment has been minimized.

@fbstj

fbstj Dec 21, 2018

Contributor

is it possible to wrap it into a MaybeUninit::uninitialized_array() -> [MaybeUninit; #] static method that has the pre-copy as a doc-comment so as to remove the 7 times that exact comment is given? or is that (blocked by) what Centril meant by #49147 ?

This comment has been minimized.

@Centril

Centril Dec 21, 2018

Contributor

As I mentioned above, this can be made safe once #49147 is stable. Once that happens, you can just write [MaybeUninit::uninitialized(), N] directly.

This comment has been minimized.

@Centril

Centril Dec 21, 2018

Contributor

@fbstj You could make a fn cap_uninit_array<T>() -> [MaybeUninit<T>; N] function locally here for some specific Ns (e.g. CAPACITY) using the unsafe method but you need const generics to do it for all const N: usize. So waiting for #49147 seems better.

This comment has been minimized.

@RalfJung

RalfJung Dec 22, 2018

Member

why do I have a terrible feeling I'm going to be reading MaybeUninit::uninitialized().into_inner() everywhere rather than mem::uninitialized()...

In my defense, that's not the only change here. ;)

The main problem is that we have pretty bad support for working with arrays, and arrays of MaybeUninit are no exceptions. If we had functions generic over array lengths, this all would be much easier.

This comment has been minimized.

@cramertj

cramertj Dec 26, 2018

Member

Yeah-- it's just that working with uninitialized arrays is my only current use of mem::uninitialized, and it used to use ManuallyDrop etc. but that route is now broken. Seeing this working-around of MaybeUninit's semantics makes me sad, but I understand it's a stepping-stone to the eventual goal.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 22, 2018

I have added a macro to create uninitialized arrays. Unfortunately it requires repeating the array length, but there will be a compile error if you use the wrong one. This removes all the new into_inner. Do you think that is an improvement?

@comex

This comment has been minimized.

Copy link
Contributor

comex commented Dec 22, 2018

It's too bad the names as_ptr and as_mut_ptr are already taken, since using them for this would be consistent with the correspondingly named methods on slices.

On the other hand, it's nice to be explicit.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 22, 2018

It's too bad the names as_ptr and as_mut_ptr are already taken, since using them for this would be consistent with the correspondingly named methods on slices.

I actually feel like the methods on the slice should have been called differently to contain something like first or begin. When writing this patch, it happened several times that existing code that sued to call as_mut_ptr on slices now called the same-named method on MaybeUninit, leading to rather strange type error messages.

But well, too late for any of that. What I'd really like to do is make these functions methods that take self, but I don't think that is possible.

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 23, 2018

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

@RalfJung RalfJung force-pushed the RalfJung:kill-more-uninit branch from 36dcd75 to 4a6497b Dec 23, 2018

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 26, 2018

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

@RalfJung RalfJung force-pushed the RalfJung:kill-more-uninit branch from 4a6497b to b9595b3 Dec 26, 2018

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 26, 2018

Rebased. However, stdsimd introduced some new uses of mem::zeroed into libcore...

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 26, 2018

The job x86_64-gnu-llvm-6.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.
travis_time:end:17c68bd5:start=1545860357357052204,finish=1545860358412652664,duration=1055600460
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:03:08]    Compiling cc v1.0.25
[00:03:08]    Compiling core v0.0.0 (/checkout/src/libcore)
[00:03:08]    Compiling libc v0.2.45
[00:03:08]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:03:08] warning: unknown lint: `deprecated_in_future`
[00:03:08]    |
[00:03:08] 63 | #![warn(deprecated_in_future)]
[00:03:08]    |         ^^^^^^^^^^^^^^^^^^^^
[00:03:08]    |
---
[00:03:40]    Compiling rustc-std-workspace-core v1.0.0 (/checkout/src/tools/rustc-std-workspace-core)
[00:03:42]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:03:42]    Compiling rustc-demangle v0.1.10
[00:03:42]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:03:42] warning: unknown lint: `deprecated_in_future`
[00:03:42]    |
[00:03:42] 66 | #![warn(deprecated_in_future)]
[00:03:42]    |         ^^^^^^^^^^^^^^^^^^^^
[00:03:42]    |
---
[00:23:00]    Compiling compiler_builtins v0.1.2
[00:23:00]    Compiling cmake v0.1.33
[00:23:00]    Compiling backtrace-sys v0.1.27
[00:23:02]    Compiling std v0.0.0 (/checkout/src/libstd)
[00:23:03] error: use of item 'mem::zeroed' that will be deprecated in future version 2.0.0: use `mem::MaybeUninit::zeroed` instead
[00:23:03]   --> src/libcore/../stdsimd/coresimd/x86/avx512f.rs:17:24
[00:23:03]    |
[00:23:03] 17 |     let zero: i32x16 = mem::zeroed();
[00:23:03]    |
[00:23:03]    = note: `-D deprecated-in-future` implied by `-D warnings`
[00:23:03] 
[00:23:03] 
[00:23:03] error: use of item 'mem::zeroed' that will be deprecated in future version 2.0.0: use `mem::MaybeUninit::zeroed` instead
[00:23:03]   --> src/libcore/../stdsimd/coresimd/x86/avx512f.rs:57:5
[00:23:03] 57 |     mem::zeroed()
[00:23:03]    |     ^^^^^^^^^^^
[00:23:03] 
[00:23:03]    Compiling rustc_asan v0.0.0 (/checkout/src/librustc_asan)
---
[00:23:17] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:23:17] expected success, got: exit code: 101
[00:23:17] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:23:17] Build completed unsuccessfully in 0:20:11
[00:23:17] Makefile:18: recipe for target 'all' failed
[00:23:17] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:071a91d4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 26 22:02:44 UTC 2018
---
travis_time:end:034eb440:start=1545861764505307330,finish=1545861764511783214,duration=6475884
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09499598
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:192467bf
travis_time:start:192467bf
$ 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:012fde9e
$ 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)

@RalfJung RalfJung force-pushed the RalfJung:kill-more-uninit branch from b9595b3 to 8e312e7 Dec 30, 2018

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 30, 2018

Updated stdsimd.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 1, 2019

So, how do we proceed with this?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2019

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

@RalfJung RalfJung force-pushed the RalfJung:kill-more-uninit branch from 8e312e7 to f4dd732 Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment