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 mem::{zeroed,uninitialized} in terms of MaybeUninit. #62150

Merged
merged 1 commit into from Jul 5, 2019

Conversation

Projects
None yet
@alex
Copy link
Contributor

commented Jun 26, 2019

Refs #62061

r? @oli-obk

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

r? @Kimundi

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

@alex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@rust-highfive rust-highfive assigned oli-obk and unassigned Kimundi Jun 26, 2019

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

looks fine to me from a compiler perspective

maybe someone from @rust-lang/libs wants to look at the MaybeUninit usage?

cc @RalfJung

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Fine from a MybeUninit perspective.

Let's ask perf though, just to be sure.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

⌛️ Trying commit 5c150fc with merge c6c5b77...

bors added a commit that referenced this pull request Jun 26, 2019

Auto merge of #62150 - alex:mem-uninit-refactor, r=<try>
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs #62061

r? @oli-obk
@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Note that the init intrinsic is unused now, but we won't remove it yet as it will be used again in a follow-up PR.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Maybe it makes sense to mark the init intrinsic as deprecated already in this PR? Seems to be in-line with the other changes here.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

good idea

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Why remove the calls to panic_if_uninhabited?

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@SimonSapin They happen in .assume_init().

@alex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Hmm, simply sticking a #[rustc_deprecated()] on the fn init results in an error that rustc_deprecated attribute must be paired with either stable or unstable attribute. The whole module is marked unstable though. Is there an appropriate idiom here? Simply mark the function unstable again?

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

r? @RalfJung

Feel free to review this since the PR doesn't change anything observably.

@rust-highfive rust-highfive assigned RalfJung and unassigned oli-obk Jun 26, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@alex Yes, items without a #[stable] or #[unstable] attribute “inherit” that of their parent item. (At least when the parent is a module.) So duplicating that attribute to satisfy the lint does not change the stability of the function.

@alex alex force-pushed the alex:mem-uninit-refactor branch from 5c150fc to b3b2d48 Jun 26, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

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:036aa8a0:start=1561552896655659899,finish=1561552898843198151,duration=2187538252
$ 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
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:57:31] 
[00:57:31] ---- [ui] ui/init-unsafe.rs stdout ----
[00:57:31] diff of stderr:
[00:57:31] 
[00:57:31] + warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31] +    |
[00:57:31] + LL | use std::intrinsics::{init};
[00:57:31] +    |                       ^^^^
[00:57:31] +    |
[00:57:31] +    |
[00:57:31] +    = note: #[warn(deprecated)] on by default
[00:57:31] + 
[00:57:31] + warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31] +    |
[00:57:31] +    |
[00:57:31] + LL |     let stuff = init::<isize>();
[00:57:31] + 
[00:57:31] 1 error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
[00:57:31] 2   --> $DIR/init-unsafe.rs:7:17
[00:57:31] 3    |
[00:57:31] 3    |
[00:57:31] 
[00:57:31] 
[00:57:31] The actual stderr differed from the expected stderr.
[00:57:31] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe/init-unsafe.stderr
[00:57:31] To update references, rerun the tests and pass the `--bless` flag
[00:57:31] To only update this specific test, also pass `--test-args init-unsafe.rs`
[00:57:31] error: 1 errors occurred comparing output.
[00:57:31] status: exit code: 1
[00:57:31] status: exit code: 1
[00:57:31] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/init-unsafe.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe/auxiliary" "-A" "unused"
[00:57:31] ------------------------------------------
[00:57:31] 
[00:57:31] ------------------------------------------
[00:57:31] stderr:
[00:57:31] stderr:
[00:57:31] ------------------------------------------
[00:57:31] warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31]    |
[00:57:31] LL | use std::intrinsics::{init};
[00:57:31]    |                       ^^^^
[00:57:31]    |
[00:57:31]    |
[00:57:31]    = note: #[warn(deprecated)] on by default
[00:57:31] 
[00:57:31] warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31]    |
[00:57:31]    |
[00:57:31] LL |     let stuff = init::<isize>(); //~ ERROR call to unsafe function is unsafe
[00:57:31] 
[00:57:31] error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
[00:57:31]   --> /checkout/src/test/ui/init-unsafe.rs:7:17
[00:57:31]    |
[00:57:31]    |
[00:57:31] LL |     let stuff = init::<isize>(); //~ ERROR call to unsafe function is unsafe
[00:57:31]    |                 ^^^^^^^^^^^^^^^ call to unsafe function
[00:57:31]    |
[00:57:31]    = note: consult the function's documentation for information on how to avoid undefined behavior
[00:57:31] error: aborting due to previous error
[00:57:31] 
[00:57:31] For more information about this error, try `rustc --explain E0133`.
[00:57:31] 
---
[00:57:31] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:521:22
[00:57:31] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[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/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -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" "6.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:52:45
---
travis_time:end:099e4630:start=1561556362959203865,finish=1561556362963903619,duration=4699754
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0fcb578c
$ 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:241f6aea
travis_time:start:241f6aea
$ 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:02c4dc68
$ 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)

@alex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Oof. Is the correct fix just to #![allow(deprecated) for that test?

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I think we should do the steps @oli-obk outlined in #62061 (comment). If we do that, then imo we can just remove intrinsics::init here and now.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

That seems like a change with enough potential for problems that I wouldn't want to block removing uninit on it.

Is the correct fix just to #![allow(deprecated) for that test?

Yes.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

That seems like a change with enough potential for problems that I wouldn't want to block removing uninit on it.

I think the idea was not to block this PR, but to just nix init here, too and think about future steps in separate PRs.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Well that would be fine for me. It would commit ourselves to not follow this plan and instead do the "proper" thing right away. I just wanted to not stand in the way of making MaybeUninit::zeroed a const fn ASAP since some people have now waited for it for more than half a year.

But I will never argue against nixing an intrinsic such as init. ;)

@alex alex force-pushed the alex:mem-uninit-refactor branch from b3b2d48 to b366f2b Jun 26, 2019

@alex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Ok, should be green now.

I don't know nearly enough about the &mut in const fn issue to take that on unfortunately.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

LGTM, unless @Centril and @oli-obk want to push through with the idea of nixing init as well?

We could easily do that in a separate PR though.

@alex

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Clippy side landed, #62246 is up now.

@@ -700,17 +700,9 @@ extern "rust-intrinsic" {
/// which is unsafe unless `T` is `Copy`. Also, even if T is
/// `Copy`, an all-zero value may not correspond to any legitimate
/// state for the type in question.
// FIXME: deprecate once clippy's tests are fixed

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 2, 2019

Member

The "no tools shall break" freeze is basically over... I think you can just deprecate this, and we can fix clippy asynchronously.

This comment has been minimized.

Copy link
@alex

alex Jul 4, 2019

Author Contributor

Will do once #62246 is merged

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 4, 2019

Member

Why wait? Breaking clippy tests brielfy is not very uncommon.

The reasong it didn't work before is that we have a one-week "freeze window" before the beta branches, where tools may not regress.

This comment has been minimized.

Copy link
@alex

alex Jul 4, 2019

Author Contributor

Ah, gotcha. Done!

@alex alex force-pushed the alex:mem-uninit-refactor branch from 49c81d4 to e4f250e Jul 4, 2019

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Thanks!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

📌 Commit e4f250e has been approved by RalfJung

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

@bors rollup- due to likely clippy breakage

bors added a commit that referenced this pull request Jul 4, 2019

Auto merge of #62246 - alex:bump-clippy, r=Centril
Update clippy in preperation for #62150

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 4, 2019

Rollup merge of rust-lang#62150 - alex:mem-uninit-refactor, r=RalfJung
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk

bors added a commit that referenced this pull request Jul 4, 2019

Auto merge of #62378 - Mark-Simulacrum:rollup-a21wrrg, r=Mark-Simulacrum
Rollup of 10 pull requests

Successful merges:

 - #61392 (Use a single CtxtInterners)
 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62123 ( Remove needless lifetimes (std))
 - #62133 (Feature gate `rustc` attributes harder)
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62169 (Derive which queries to save using the proc macro)
 - #62292 (Move `async || ...` closures into `#![feature(async_closure)]`)
 - #62323 (Clarify unaligned fields in ptr::{read,write}_unaligned)
 - #62324 (Reduce reliance on `await!(...)` macro)
 - #62371 (Add tracking issue for Box::into_pin)

Failed merges:

 - #62243 (Improve documentation for built-in macros)

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019

Rollup merge of rust-lang#62150 - alex:mem-uninit-refactor, r=RalfJung
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk

bors added a commit that referenced this pull request Jul 5, 2019

Auto merge of #62399 - Centril:rollup-du8hsoo, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #62123 ( Remove needless lifetimes (std))
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62169 (Derive which queries to save using the proc macro)
 - #62238 (Fix code block information icon position)
 - #62292 (Move `async || ...` closures into `#![feature(async_closure)]`)
 - #62323 (Clarify unaligned fields in ptr::{read,write}_unaligned)
 - #62324 (Reduce reliance on `await!(...)` macro)
 - #62331 (Fix leak when early returning out of `box` syntax)
 - #62371 (Add tracking issue for Box::into_pin)
 - #62383 (Improve error span for async type inference error)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019

Rollup merge of rust-lang#62150 - alex:mem-uninit-refactor, r=RalfJung
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk

bors added a commit that referenced this pull request Jul 5, 2019

Auto merge of #62407 - Centril:rollup-g0zmff7, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #62123 ( Remove needless lifetimes (std))
 - #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.)
 - #62169 (Derive which queries to save using the proc macro)
 - #62238 (Fix code block information icon position)
 - #62292 (Move `async || ...` closures into `#![feature(async_closure)]`)
 - #62323 (Clarify unaligned fields in ptr::{read,write}_unaligned)
 - #62324 (Reduce reliance on `await!(...)` macro)
 - #62371 (Add tracking issue for Box::into_pin)
 - #62383 (Improve error span for async type inference error)
 - #62388 (Break out of the correct number of scopes in loops)

Failed merges:

r? @ghost

@bors bors merged commit e4f250e into rust-lang:master Jul 5, 2019

3 checks passed

pr Build #20190704.37 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details

@alex alex deleted the alex:mem-uninit-refactor branch Jul 5, 2019

@EPashkin EPashkin referenced this pull request Jul 12, 2019

Merged

Zeroed #505

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Seems like this PR caused SIGILL in a bunch of libraries (see the backlinks GH inserted above), but it also seems like these libraries all had UB -- so tiny changes in the implementations of these methods can lead to arbitrary changes in behavior.

@ishitatsuyuki

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

Based on what described in amethyst/amethyst#1808 (comment), I suggest that this change to be backed out for at least one stable release. This kind of UB existed in old dependencies that are no longer maintained, therefore some crates have to migrate their dependency to the latest version to mitigate the issue, and we need some time for the ecosystem to do that major version upgrades.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

@ishitatsuyuki a closed PR is not a good place for a new discussion. Could you open a new issue and Cc me? Then I will nominate it for lang team discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.