Make core::mem::MaybeUninit::zeroed a const fn by mjbshaw · Pull Request #54832 · rust-lang/rust

mjbshaw · 2018-10-05T00:27:26Z

First, let me say I have no idea what I'm doing, so this might be completely wrong. @RalfJung (and others), please let me know if this accidentally causes undefined behavior.

I believe miri zero-fills allocations, and that force_allocation is the right way to allocate something in miri, but if either of those assumptions are incorrect please let me know.

I'm happy to add this behind a feature gate (as suggested by @Centril). I didn't here since MaybeUninit is already unstable and I'm hoping this isn't too controversial.

rust-highfive · 2018-10-05T00:27:36Z

r? @alexcrichton

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

oli-obk

I believe miri zero-fills allocations, and that force_allocation is the right way to allocate something in miri, but if either of those assumptions are incorrect please let me know.

While the "byte" representation is zero filled, there's also a layer marking these bytes as "uninitialized". Only after looking at all three layers (bytes, initialized, relocation/pointer) can one make a statement about the value.

oli-obk · 2018-10-05T07:43:43Z

 self.write_scalar(val, dest)?;
 }
+ "init" => {
+ self.force_allocation(dest)?;


You can just copy the implementation from https://github.com/solson/miri/blob/master/src/intrinsic.rs#L238
Your current implementation is a nop, it literally does nothing ;)

Dang, that was awfully naive of me ha. Done.
Side question: is there any reason that entire file isn't just copied into here?

oli-obk · 2018-10-05T07:47:24Z

r? @oli-obk

Taken from https://github.com/solson/miri/blob/317f90525a9620d9957b3bf37f8d605f50a8c889/src/intrinsic.rs#L238-L260

mjbshaw · 2018-10-05T14:01:27Z

Thank you, @oli-obk, for helping walk me through this one!

rust-highfive · 2018-10-05T14:18:04Z

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:04:11] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:12] tidy error: /checkout/src/librustc_mir/interpret/intrinsics.rs:154: line longer than 100 chars
[00:04:12] tidy error: /checkout/src/librustc_mir/interpret/intrinsics.rs:155: line longer than 100 chars
[00:04:13] some tidy checks failed
[00:04:13] [00:04:13] [00:04:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:13] [00:04:13] [00:04:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:13] Build completed unsuccessfully in 0:00:46
[00:04:13] Build completed unsuccessfully in 0:00:46
[00:04:13] make: *** [tidy] Error 1
[00:04:13] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:3a0f6938
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:107a52e0:start=1538749055238973001,finish=1538749055244199578,duration=5226577
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0ee16fd8
$ 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:01a35a5b
travis_time:start:01a35a5b
$ 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:135e5594
$ 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 · 2018-10-05T14:22:01Z

Uh... I am not a big fan. :/

The plan is to get rid of these horrible intrinsics (init and uninit). We are going to deprecate them for a reason. Please can we just not have them at all, ever, in CTFE?

I'd much prefer keeping the original implementation of MaybeUninit, and beefing up CTFE to support that.

RalfJung · 2018-10-05T14:28:10Z

(Sorry, somehow I managed to submit the same thing twice and close the PR and I have no idea how that happened.)

So, to be clear: If you are asking for my opinion, I'll r- anything that involves implementing the init or uninit intrinsics in CTFE. We have a chance not to repeat the same mistakes that "run-time Rust" made, let's use it. That might mean it takes a bit longer to get MaybeUninit::zeroed in const fn as other CTFE work has to happen first, but that one function is not worth sacrificing the sanity of our CTFE engine.

rust-highfive · 2018-10-05T14:32:02Z

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:04:20] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:21] tidy error: /checkout/src/librustc_mir/interpret/intrinsics.rs:154: line longer than 100 chars
[00:04:21] tidy error: /checkout/src/librustc_mir/interpret/intrinsics.rs:155: line longer than 100 chars
[00:04:22] some tidy checks failed
[00:04:22] [00:04:22] [00:04:22] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:22] [00:04:22] [00:04:22] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:22] Build completed unsuccessfully in 0:00:47
[00:04:22] Build completed unsuccessfully in 0:00:47
[00:04:22] Makefile:79: recipe for target 'tidy' failed
[00:04:22] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:123907be
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

mjbshaw · 2018-10-05T15:18:14Z

Dang. I originally wanted to implement this by just transmuting a [0u8; size_of::<T>()], but unfortunately Rust doesn't support that. The init intrinsic was the only way I could think of doing this in present-day Rust.

rust-highfive · 2018-10-05T15:21:02Z

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:04:35] Compiling rustc_tsan v0.0.0 (/checkout/src/librustc_tsan)
[00:04:36] Compiling rustc_asan v0.0.0 (/checkout/src/librustc_asan)
[00:04:37] Compiling rustc_msan v0.0.0 (/checkout/src/librustc_msan)
[00:04:38] Compiling rustc_lsan v0.0.0 (/checkout/src/librustc_lsan)
[00:04:42] error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
[00:04:42] |
[00:04:42] |
[00:04:42] 1044 | MaybeUninit { value: unsafe { intrinsics::init() } }
[00:04:42] [00:04:45] error: aborting due to previous error
[00:04:45] [00:04:45] For more information about this error, try `rustc --explain E0015`.
[00:04:45] For more information about this error, try `rustc --explain E0015`.
[00:04:45] error: Could not compile `core`.
[00:04:45] warning: build failed, waiting for other jobs to finish...
[00:04:47] error: build failed
[00:04:47] 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 jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:47] expected success, got: exit code: 101
[00:04:47] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1112:9
[00:04:47] travis_fold:end:stage0-std
[00:04:47] travis_time:end:stage0-std:start=1538752784643223586,finish=1538752809104880801,duration=24461657215
[00:04:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:47] Build completed unsuccessfully in 0:00:25
[00:04:47] Makefile:28: recipe for target 'all' failed
[00:04:47] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:058ec358
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:08a77faf:start=1538752809713581766,finish=1538752809720478920,duration=6897154
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:205e952e
$ 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:11f26c0c
travis_time:start:11f26c0c
$ 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:0e62d467
$ 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 · 2018-10-05T15:55:01Z

let exists for const fn behind a feature gate. So the only thing missing is write_bytes, right?

Use this to implement MaybeUninit::zeroed, and make the init intrinsic no longer const

mjbshaw · 2018-10-05T22:22:55Z

Okay, I've added a commit that reverts init and instead const-ifies write_bytes . I haven't created a separate tracking issue for const_write_bytes. I figure I'll do that (and revise this pull request) if I'm given the green light on this.

RalfJung · 2018-10-06T08:48:56Z

+ pub const fn zeroed() -> MaybeUninit<T> {
+ let mut u = MaybeUninit::<T>::uninitialized();
+ unsafe {
+ (&mut *u.value as *mut T).write_bytes(0u8, 1);


Why not make as_mut_ptr a const fn as well and avoid duplicating?

I've been digging into making that change, but I keep running into issues like this, and now that I think of it, I can't figure out why this even compiles in the first place:
ManuallyDrop implements the deref traits, but those functions aren't const.
I only made the intrinsic write_bytesconst, not the write_bytes method for mut_ptr lang item. This should be using the latter, not the intrinsic.
This requires taking a mutable reference.
I don't understand why this even works as-is. Trying to make as_mut_ptr a const fn reveals all these issues, but manually inlining it here does not...

O_o, that looks like a hole in the const checks, but I don't even know where to start debugging this. Even if the function isn't checked for min_const_fn, it should definitely fail to build due to the regular const fn checks.

Yea, in the playground this won't compile: https://play.rust-lang.org/?gist=adce30a361142dcea174aee030ea8a91&version=nightly&mode=debug&edition=2018

Could this be related to what #54715 fixes?

No, I think I found it. is_min_const_fn doesn't check for is_const_fn as a prerequisite.
The fix is to add self.is_const_fn(def_id) && infront of
rust/src/librustc/ty/constness.rs
Line 43 in 567557f
ifself.features().staged_api{

RalfJung · 2018-10-06T08:50:13Z

+ count, mplace.layout.size.bytes(), intrinsic_name),
+ ));
+ }
+ }


Why is this code so different from https://github.com/solson/miri/blob/master/src/intrinsic.rs#L420 ?
Also note that the ZST optimization here is wrong. It is UB to call write_bytes on an unaligned pointer even for a ZST. Your code fails to check for that.

RalfJung · 2018-10-06T08:50:52Z

+ &format!("The use of std::ptr::write_bytes() \
+ is gated in {}s", self.mode));
+ }
+ }


Why does this logic look different than the one for transmute above?

RalfJung · 2018-10-06T08:51:19Z

Yes, I'm in favor of this approach. :) Please remove the old commits though, no reason to keep them.

mjbshaw · 2018-10-22T19:10:56Z

I'm unsure how to proceed here. As best as I can tell, in order to implement this (without a const init intrinsic) it will require:

Making std::ptr::write_bytes a const fn.
Maybe making (*mut T)::write_bytes a const fn. Not really required but would be nice for parity with std::ptr::write_bytes.
This requires taking a mutable reference, which isn't permitted in const fn.

Items 1 and 2 are easy, but item 3 seems to be the real blocker. Any advice on how to proceed?

oli-obk · 2018-10-23T08:54:08Z

is 3. solvable with the const_let feature gate? If not, then the feature gate is broken

mjbshaw · 2018-10-23T14:13:23Z

No, const_let doesn't permit taking mutable references. Compiling the linked code results in:

error[E0017]: references in constants may only refer to immutable values
--> src/main.rs:5:7
|
5 | { &mut x; } // Delete `mut` and this will work.
| ^^^^^^ constants require immutable values
error: aborting due to previous error

oli-obk · 2018-10-26T08:30:11Z

hmm... this is a little tricky to solve. The checks are there so we don't end up with a &mut T in a static or const. While I am fairly certain we could simply do this as a check on the final value/type, we should explore this in rust-lang/const-eval#16 before changing anything

TimNN · 2018-11-06T18:48:56Z

Ping from triage! What is the status of this PR? Is it blocked on an external issue?

oli-obk · 2018-11-06T19:06:22Z

yea, @mjbshaw unfortunately I think we can't really do this right now. We need to figure out how to do mutable references in const fns first. This is going to take a while (both in figuring out and implementing).

Once we've done that, don't hesitate to reopen your PR

Make core::mem::MaybeUninit::zeroed a const fn
0b6a44c

rust-highfive assigned alexcrichton Oct 5, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2018

oli-obk requested changes Oct 5, 2018
View reviewed changes

rust-highfive assigned oli-obk and unassigned alexcrichton Oct 5, 2018

mjbshaw added 2 commits October 5, 2018 06:23

Add a feature gate for MaybeUninit::zeroed's constness
255f723

Fix the emulation of the init intrinsic
b18f946
Taken from https://github.com/solson/miri/blob/317f90525a9620d9957b3bf37f8d605f50a8c889/src/intrinsic.rs#L238-L260

RalfJung closed this Oct 5, 2018

RalfJung reopened this Oct 5, 2018

Wrap to 100 columns
fde2e91

Make core::ptr::write_bytes a const fn gated behind const_write_bytes
e5eb54c
Use this to implement MaybeUninit::zeroed, and make the init intrinsic no longer const

RalfJung reviewed Oct 6, 2018
View reviewed changes

oli-obk closed this Nov 6, 2018

mjbshaw mentioned this pull request Feb 8, 2019
Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type" #53491
Closed
3 tasks

RalfJung mentioned this pull request Jun 26, 2019
Make core::mem::zeroed() a const fn #62061
Closed

Uh oh!

Uh oh!

Conversation

mjbshaw commented Oct 5, 2018

Uh oh!

rust-highfive commented Oct 5, 2018

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 5, 2018

Uh oh!

mjbshaw commented Oct 5, 2018

Uh oh!

rust-highfive commented Oct 5, 2018

Uh oh!

RalfJung commented Oct 5, 2018

Uh oh!

RalfJung commented Oct 5, 2018

Uh oh!

rust-highfive commented Oct 5, 2018

Uh oh!

mjbshaw commented Oct 5, 2018

Uh oh!

rust-highfive commented Oct 5, 2018

Uh oh!

RalfJung commented Oct 5, 2018

Uh oh!

mjbshaw commented Oct 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Oct 6, 2018

Uh oh!

mjbshaw commented Oct 22, 2018

Uh oh!

oli-obk commented Oct 23, 2018

Uh oh!

mjbshaw commented Oct 23, 2018

Uh oh!

oli-obk commented Oct 26, 2018

Uh oh!

TimNN commented Nov 6, 2018

Uh oh!

oli-obk commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants