Constier maybe uninit #79621

usbalbin · 2020-12-02T02:50:14Z

I was playing around trying to make [T; N]::zip() in #79451 be const fn. One of the things I bumped into was MaybeUninit::assume_init. Is there any reason for the intrinsic assert_inhabited<T>() and therefore MaybeUninit::assume_init not being const?

I have as best as I could tried to follow the instruction in library/core/src/intrinsics.rs. I have no idea what I am doing but it seems to compile after some slight changes after the copy paste. Is this anywhere near how this should be done?

Also any ideas for name of the feature gate? I guess const_maybe_assume_init is quite misleading since I have added some more methods. Should I add test? If so what should be tested?

rust-highfive · 2020-12-02T02:50:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

oli-obk · 2020-12-02T09:45:23Z

cc @rust-lang/wg-const-eval

library/core/src/intrinsics.rs

RalfJung · 2020-12-02T09:49:00Z

compiler/rustc_mir/src/interpret/intrinsics.rs

+                let layout = self.layout_of(ty)?;
+
+                if layout.abi.is_uninhabited() {
+                    throw_ub_format!("attempted to instantiate uninhabited type `{}`", ty);


This is wrong. It is not UB to fail an assertion. That is the entire point of this check, to avoid UB and replace it by a well-defined assertion.

RalfJung · 2020-12-02T09:52:41Z

Making these intrinsics const fn is fine, but we have to decide what we actually want them to do. For reference, this is what Miri does. TerminationInfo::Abort is a Miri-specific mechanism; the CTFE equivalent would involve this enum.

If we want to share as much code as possible between Miri and CTFE, we should generalize this machine hook to also accept an error message. Then the Miri and CTFE machines can each behave appropriately.

library/core/src/mem/maybe_uninit.rs

RalfJung · 2020-12-02T09:56:28Z

library/core/src/intrinsics.rs

@@ -815,6 +815,7 @@ extern "rust-intrinsic" {
    /// This will statically either panic, or do nothing.
    ///
    /// This intrinsic does not have a stable counterpart.
+    #[rustc_const_unstable(feature = "const_maybe_assume_init", issue = "none")]


For the feature gate name... we do not intend to ever stabilize these intrinsics. @oli-obk what is the plan for that case? Usually perma-unstable intrinsics just do not have a feature gate name.

we don't stabilize the intrinsics, but we will "stabilize their constness", thus allowing them to be used in libcore/libstd from stable const fn

Oh, so even if it is const-stable it cannot be used outside libstd? Curious.

Sounds like we want a feature gate collecting the three intrinsics then. Something like const_assert_type or so?

Hmm, does that mean you can have a type that's rustc_const_stable without being stable? I think that will break rustdoc's rendering of const fn. #79551, #79548 (comment)

@RalfJung

Sounds like we want a feature gate collecting the three intrinsics then. Something like const_assert_type or so?

I have removed the constification of the other two intrinsics (see comments). Does the name const_assert_type still make sense or should i change it to something more specific like const_assert_inhabited?

Does the name const_assert_type still make sense or should i change it to something more specific like const_assert_inhabited?

Either is fine, I'd say.

RalfJung · 2020-12-02T09:59:02Z

compiler/rustc_mir/src/interpret/intrinsics.rs

@@ -407,6 +407,30 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            sym::transmute => {
                self.copy_op_transmute(args[0], dest)?;
            }
+            sym::assert_inhabited | sym::assert_zero_valid | sym::assert_uninit_valid => {


@oli-obk how will such aborting intrinsics interact with function memoization?

In particular with #75390 I am wondering if we'll get decent error messages here.

that works fine, it will cause the const eval queries to return an error, similar to how the memoization of size_of can error if the type is not representable on the platform

RalfJung · 2020-12-02T10:08:09Z

Thanks a lot for the PR, @usbalbin. :) Yes this is certainly in the right direction.

Also any ideas for name of the feature gate? I guess const_maybe_assume_init is quite misleading since I have added some more methods.

You are doing many things at once here, that IMO should have different feature gates. Maybe as a start it'd make more sense to restrict the PR to just the assume_init method(s).

Should I add test? If so what should be tested?

Is there even a way to use assume_init correctly given the current CTFE limitations? I think that will require mutable references; I am not sure what the status of this is.

A simple smoke test would be something like

const TRUE: bool = {
  let mut x = MaybeUninit::<bool>::uninit();
  x.as_mut_ptr().write(true);
  x.assume_init()
};
assert!(TRUE);

And also a compile-fail test like

const BAD: () = {
  MaybeUninit::<!>::uninit().assume_init();
}

* Undo fn -> const fn for some fns. * Split feature gate. * Made all three intrinsics const

usbalbin · 2020-12-02T19:54:09Z

@RalfJung

Making these intrinsics const fn is fine, but we have to decide what we actually want them to do. For reference, this is what Miri does. TerminationInfo::Abort is a Miri-specific mechanism; the CTFE equivalent would involve this enum.

If we want to share as much code as possible between Miri and CTFE, we should generalize this machine hook to also accept an error message. Then the Miri and CTFE machines can each behave appropriately.

Should I add an Abort variant to ConstEvalErrKind and change

fn abort(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx, !> {
    throw_unsup_format!("aborting execution is not supported")
}

to something like

fn abort(_ecx: &mut InterpCx<'mir, 'tcx, Self>, msg: String) -> InterpResult<'tcx, !> {
    Err(ConstEvalErrKind::Abort(msg))
}

and then call that if if layout.abi.is_uninhabited() { in compiler/rustc_mir/src/interpret/intrinsics.rs?

RalfJung · 2020-12-05T13:46:36Z

@usbalbin yes that sounds like a reasonable plan!

usbalbin · 2020-12-05T17:04:14Z

compiler/rustc_mir/src/interpret/intrinsics.rs

@@ -126,7 +126,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            None => match intrinsic_name {
                sym::transmute => throw_ub_format!("transmuting to uninhabited type"),
                sym::unreachable => throw_ub!(Unreachable),
-                sym::abort => M::abort(self)?,
+                sym::abort => M::abort(self, "aborted execution".to_owned())?,


What message do we want here?

I'd vote for "the program aborted execution".

usbalbin · 2020-12-05T17:04:34Z

compiler/rustc_mir/src/interpret/terminator.rs

@@ -110,7 +110,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            }

            Abort => {
-                M::abort(self)?;
+                M::abort(self, "aborted execution".to_owned())?;


What message do we want here?

The same as in the other place, I'd say.

library/core/tests/mem.rs

compiler/rustc_mir/src/interpret/machine.rs

Still have not figured out how to make it work

bors · 2020-12-09T09:13:02Z

📌 Commit 0775271 has been approved by RalfJung

bors · 2020-12-10T05:57:06Z

⌛ Testing commit 0775271 with merge a80568cf2a9375756a461552ee24bef08e362b7d...

bors · 2020-12-10T10:12:07Z

💥 Test timed out

RalfJung · 2020-12-10T10:21:04Z

@bors retry

bors · 2020-12-10T10:46:44Z

⌛ Testing commit 0775271 with merge 39b841d...

bors · 2020-12-10T12:52:11Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 39b841d to master...

rust-highfive · 2020-12-10T12:53:21Z

📣 Toolstate changed by #79621!

Tested on commit 39b841d.
Direct link to PR: #79621

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

@oli-obk

Tested on commit rust-lang/rust@39b841d. Direct link to PR: <rust-lang/rust#79621> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

RalfJung · 2020-12-10T19:25:42Z

src/test/ui/assume-type-intrinsics.rs

@@ -0,0 +1,13 @@
+// error-pattern: any use of this value will cause an error


Oh, I only just realized this file is in the wrong folder. It should be in an appropriate subfolder, in this case consts sounds best. I'll take care of that as part of #79910.

usbalbin · 2020-12-10T21:39:17Z

Thank you all for helping me with my first rust PR to get merged! :)

RalfJung · 2020-12-10T22:17:49Z

@usbalbin thanks for contributing to Rust. :-)

usbalbin · 2021-06-29T13:32:03Z

When looking back at this PR while writing the tracking issue for const_maybe_uninit_assume_init I noticed that both of the slice assume init methods are unstable behind the feature gate maybe_uninit_slice, with tracking issue #63569. Should the constness of those methods be under the const_maybe_uninit_assume_init as now or changed to the same as the methods feature gate maybe_uninit_slice.

If you want me to keep things as is then should I mention in the tracking issue for const_maybe_uninit_assume_init that the slice methods are also blocked on that other feature gate?

RalfJung · 2021-06-29T15:30:48Z

For these critical low-level unsafe things I think it makes sense tot rack const stability separately from stability.

usbalbin · 2021-06-29T18:32:02Z

Ok :)

Tracking issue #86722 created for const_maybe_uninit_assume_init

usbalbin added 2 commits December 2, 2020 03:22

Make some of MaybeUninit's methods const

8bd80e2

Even more const

91772c3

rust-highfive assigned estebank Dec 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2020

oli-obk added the A-const-eval Area: constant evaluation (mir interpretation) label Dec 2, 2020

oli-obk reviewed Dec 2, 2020

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2020

View reviewed changes

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2020

View reviewed changes

library/core/src/mem/maybe_uninit.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 2, 2020

View reviewed changes

usbalbin added 2 commits December 2, 2020 16:17

Resolved some of the comments

1ef5dbe

* Undo fn -> const fn for some fns. * Split feature gate. * Made all three intrinsics const

Added tests for assume_init

f311db1

Undo fn -> const fn for all intrinsics but assert_inhabited

4f9fd2a

abort() now takes a msg parameter

d366ed2

usbalbin commented Dec 5, 2020

View reviewed changes

usbalbin added 2 commits December 5, 2020 18:39

Fix tests (hopefully)

7bd754c

Still unable to get the tests working

9476241

usbalbin commented Dec 5, 2020

View reviewed changes

library/core/tests/mem.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 6, 2020

View reviewed changes

compiler/rustc_mir/src/interpret/machine.rs Outdated Show resolved Hide resolved

usbalbin added 2 commits December 6, 2020 19:01

Moved failing test to src/test/ui/

4255a5a

Still have not figured out how to make it work

Tests finally working

3282b54

usbalbin force-pushed the constier_maybe_uninit branch from e110b1a to 3282b54 Compare December 6, 2020 18:55

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 Dec 9, 2020

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 Dec 10, 2020

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 Dec 10, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 10, 2020

bors merged commit 39b841d into rust-lang:master Dec 10, 2020

rustbot added this to the 1.50.0 milestone Dec 10, 2020

rust-highfive mentioned this pull request Dec 10, 2020

miri no longer builds after rust-lang/rust#79621 #79897

Closed

bors mentioned this pull request Dec 10, 2020

MaybeUninit::copy/clone_from_slice #79607

Merged

RalfJung reviewed Dec 10, 2020

View reviewed changes

usbalbin mentioned this pull request Dec 21, 2020

Make copy[_nonoverlapping] const #79684

Merged

usbalbin mentioned this pull request Jun 29, 2021

Meta tracking issue for const fn #57563

Open

25 tasks

usbalbin deleted the constier_maybe_uninit branch June 29, 2021 13:35

usbalbin mentioned this pull request Jun 29, 2021

Tracking Issue for const_maybe_uninit_assume_init #86722

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constier maybe uninit #79621

Constier maybe uninit #79621

usbalbin commented Dec 2, 2020

rust-highfive commented Dec 2, 2020

oli-obk commented Dec 2, 2020

RalfJung Dec 2, 2020

RalfJung commented Dec 2, 2020

RalfJung Dec 2, 2020

oli-obk Dec 2, 2020

RalfJung Dec 2, 2020

jyn514 Dec 2, 2020

usbalbin Dec 6, 2020

RalfJung Dec 7, 2020

RalfJung Dec 2, 2020

RalfJung Dec 2, 2020

oli-obk Dec 2, 2020

RalfJung commented Dec 2, 2020 •

edited

Loading

usbalbin commented Dec 2, 2020 •

edited

Loading

RalfJung commented Dec 5, 2020

usbalbin Dec 5, 2020

RalfJung Dec 6, 2020

usbalbin Dec 5, 2020

RalfJung Dec 6, 2020

bors commented Dec 9, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

RalfJung commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

rust-highfive commented Dec 10, 2020

RalfJung Dec 10, 2020

usbalbin commented Dec 10, 2020

RalfJung commented Dec 10, 2020

usbalbin commented Jun 29, 2021

RalfJung commented Jun 29, 2021

usbalbin commented Jun 29, 2021

		@@ -0,0 +1,13 @@
		// error-pattern: any use of this value will cause an error

Constier maybe uninit #79621

Constier maybe uninit #79621

Conversation

usbalbin commented Dec 2, 2020

rust-highfive commented Dec 2, 2020

oli-obk commented Dec 2, 2020

Choose a reason for hiding this comment

RalfJung commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 2, 2020 • edited Loading

usbalbin commented Dec 2, 2020 • edited Loading

RalfJung commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 9, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

RalfJung commented Dec 10, 2020

bors commented Dec 10, 2020

bors commented Dec 10, 2020

rust-highfive commented Dec 10, 2020

Choose a reason for hiding this comment

usbalbin commented Dec 10, 2020

RalfJung commented Dec 10, 2020

usbalbin commented Jun 29, 2021

RalfJung commented Jun 29, 2021

usbalbin commented Jun 29, 2021

RalfJung commented Dec 2, 2020 •

edited

Loading

usbalbin commented Dec 2, 2020 •

edited

Loading