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 zeroed and uninitialized with MaybeUninit #69922

Merged
merged 4 commits into from
Mar 17, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 11, 2020

This is the second attempt of doing such a change (first PR: #62150). The last change got reverted because it caused some issues in code that incorrectly used these functions.

Since then, the problematic code has been fixed, and rustc gained a lint that is able to detect many misuses of these functions statically and a dynamic check that panics instead of causing UB for some incorrect uses.

Fixes #62825

@Centril
Copy link
Contributor

Centril commented Mar 11, 2020

r? @oli-obk

@RalfJung RalfJung added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2020
@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 11, 2020

📌 Commit c7eb0f2 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2020
@RalfJung
Copy link
Member Author

@bors rollup=never to make this bisectable

@Dylan-DPC-zz
Copy link

@bors p=1

@RalfJung
Copy link
Member Author

@bors p=0
I'd prefer my other PR #69999 to go first.

@RalfJung
Copy link
Member Author

Giving back priority (#69922 (comment))
@bors p=1

@bors
Copy link
Contributor

bors commented Mar 16, 2020

⌛ Testing commit c7eb0f2 with merge 954159622b131f800ed65cbd9e60585d03dac100...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-nopt of your PR failed (pretty log, 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.
2020-03-16T13:20:38.5925791Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:348:22
2020-03-16T13:20:38.5927874Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2020-03-16T13:20:38.5938708Z 
2020-03-16T13:20:38.5938949Z 
2020-03-16T13:20:38.5947596Z 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" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--host-rustcflags" "-Crpath -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -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" "--llvm-version" "9.0.1-rust-1.43.0-nightly\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2020-03-16T13:20:38.5950748Z 
2020-03-16T13:20:38.5951040Z 
2020-03-16T13:20:38.5958754Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2020-03-16T13:20:38.5959079Z Build completed unsuccessfully in 1:32:00
2020-03-16T13:20:38.5959079Z Build completed unsuccessfully in 1:32:00
2020-03-16T13:20:38.6022898Z == clock drift check ==
2020-03-16T13:20:38.6046394Z   local time: Mon Mar 16 13:20:38 UTC 2020
2020-03-16T13:20:38.8733954Z   network time: Mon, 16 Mar 2020 13:20:38 GMT
2020-03-16T13:20:38.8740928Z == end clock drift check ==
2020-03-16T13:20:39.3346594Z 
2020-03-16T13:20:39.3401413Z ##[error]Bash exited with code '1'.
2020-03-16T13:20:39.3464197Z ##[section]Starting: Checkout rust-lang/rust@auto to s
2020-03-16T13:20:39.3469654Z ==============================================================================
2020-03-16T13:20:39.3469964Z Task         : Get sources
2020-03-16T13:20:39.3470726Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Mar 16, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 16, 2020

const SIZE: usize = 1024 * 1024;

fn main() {
// do the test in a new thread to avoid (spurious?) stack overflows
thread::spawn(|| {
let _memory: [u8; SIZE] = unsafe { init() };
let _memory: [u8; SIZE] = unsafe { mem::zeroed() };
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is failing on the "noopt" builder. And that makes sense; there's now a function call "in the way" here where previously the intrinsic was expanded in-line to zero-initializing LLVM IR.

@RalfJung
Copy link
Member Author

@oli-obk I added -O to the init-large-type test as that test now clearly needs inlining to work. Does this make sense?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2020

Maybe we should mark the wrapper functions as #[inline(always)]? Is there any reason for them to never get inlined?

@RalfJung
Copy link
Member Author

Sure, I can do that. Note that this already applied before this PR.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2020

Oh, right. so it's the more complex code in MaybeUninit that llvm doesn't handle without optimizations. I guess that's alright, though maybe we can teach mir optimizations about that at some point.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2020

📌 Commit a2160e6 has been approved by oli-obk

Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
implement zeroed and uninitialized with MaybeUninit

This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99).

Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses.

Fixes rust-lang#62825
@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

@bors retry rollup=maybe

Ignoring rollup=never directive due to constrained CI budget & combining with orthogonal PRs in rollup.

@bors
Copy link
Contributor

bors commented Mar 17, 2020

⌛ Testing commit a2160e6 with merge 1d1d192555bc828cdc42a61cd9b6a02c6b260960...

Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
implement zeroed and uninitialized with MaybeUninit

This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99).

Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses.

Fixes rust-lang#62825
@bors
Copy link
Contributor

bors commented Mar 17, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 17, 2020
@RalfJung
Copy link
Member Author

"Received request to deprovision: The request was cancelled by the remote provider."...?

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2020
@bors
Copy link
Contributor

bors commented Mar 17, 2020

⌛ Testing commit a2160e6 with merge 8da5b0313f3288a314a81f571c81ac9957b0df5e...

@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

@bors retry rolled up.

bors added a commit that referenced this pull request Mar 17, 2020
Rollup of 7 pull requests

Successful merges:

 - #68746 (Make macro metavars respect (non-)hygiene)
 - #69688 (Move tidy check to mingw-check)
 - #69735 (bootstrap: Use hash to determine if sanitizers needs to be rebuilt)
 - #69922 (implement zeroed and uninitialized with MaybeUninit)
 - #69956 (Ensure HAS_FREE_LOCAL_NAMES is set for ReFree)
 - #70061 (Cosmetic fixes in documentation)
 - #70064 (Update books)

Failed merges:

r? @ghost
@bors bors merged commit 7a7ca82 into rust-lang:master Mar 17, 2020
@RalfJung RalfJung deleted the less-intrinsic branch March 18, 2020 12:33
matthew-russo added a commit to matthew-russo/winit that referenced this pull request Mar 26, 2020
x11-dl was using std::mem::uninitialized incorrectly and when
rustlang added MaybeUninit and intrinsic panics on UB caused
by improper use of uninitialized (see rust-lang/rust/pull/69922)
it caused issues with X11 initialization. x11-dl pr
AltF02/x11-rs/pull/101 updated x11-dl to use MaybeUninit
correctly
murarth pushed a commit to rust-windowing/winit that referenced this pull request Mar 26, 2020
x11-dl was using std::mem::uninitialized incorrectly and when
rustlang added MaybeUninit and intrinsic panics on UB caused
by improper use of uninitialized (see rust-lang/rust/pull/69922)
it caused issues with X11 initialization. x11-dl pr
AltF02/x11-rs/pull/101 updated x11-dl to use MaybeUninit
correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

re-land #62150 (Implement mem::{zeroed,uninitialized} in terms of MaybeUninit)
6 participants