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

Make core::mem::zeroed() a const fn #62061

Closed
alex opened this issue Jun 22, 2019 · 38 comments
Closed

Make core::mem::zeroed() a const fn #62061

alex opened this issue Jun 22, 2019 · 38 comments
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alex
Copy link
Member

alex commented Jun 22, 2019

Right now this isn't callable from const fns. This would be useful for using const fns in combination with FFI.

@alex
Copy link
Member Author

alex commented Jun 22, 2019

(Apologies if this shouldn't have a dedicated issue, it didn't seem to be covered by any of the existing A-const-fn bugs)

@jonas-schievink jonas-schievink added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 22, 2019
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jun 22, 2019
@alex
Copy link
Member Author

alex commented Jun 23, 2019

I copied mem::zeroed into my project and marked it as as const fn:

error: any use of this value will cause an error
   --> /home/alexgaynor/projects/linux-kernel-module-rust/src/chrdev.rs:125:5
    |
125 |       intrinsics::panic_if_uninhabited::<T>();
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |       |
    |       "calling intrinsic `panic_if_uninhabited`" needs an rfc before being allowed inside constants
    |       inside call to `linux_kernel_module::chrdev::zeroed::<linux_kernel_module::bindings::file_operations>` at /home/alexgaynor/projects/linux-kernel-module-rust/src/chrdev.rs:132:71
    |       inside call to `linux_kernel_module::chrdev::FileOperationsVtable::new::<CycleFile>` at src/lib.rs:14:9
    | 
   ::: src/lib.rs:13:5
    |
13  | /     const VTABLE: linux_kernel_module::chrdev::FileOperationsVtable =
14  | |         linux_kernel_module::chrdev::FileOperationsVtable::new::<Self>();
    | |_________________________________________________________________________-
    |
    = note: #[deny(const_err)] on by default

So there's more work here than simply marking it const. I don't fully understand this error message -- it seems to happen for calls to any intrinsic, so I guess there's a deeper question there.

@alex
Copy link
Member Author

alex commented Jun 23, 2019

Ah, source diving it seems that message simply means that support for the intrisinc hasn't been added to the MIR interpreter: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/intrinsics.rs#L44

Does adding support for these need their own bugs, or can they just be covered here?

@alex
Copy link
Member Author

alex commented Jun 23, 2019

After source diving a bit, I'd be happy to provide a PR to implement panic_if_uninhabited, if it's wanted. I'd be happy to do init as well, but I'd need a bit more guidance, I don't quite understand how that'd work just from looking at the other intrinsic implementations.

@RalfJung
Copy link
Member

Cc @oli-obk

mem::zeroed and mem::uninitialized are a mess (the latter is deprecated), so I am opposed to making either of them const fn.

What should become a const fn is MaybeUninit::zeroed. But that is still waiting on some more CTFE work, I think.

@RalfJung
Copy link
Member

Ah, source diving it seems that message simply means that support for the intrisinc hasn't been added to the MIR interpreter

All of these intrinsics are already implemented in Miri at https://github.com/rust-lang/miri/blob/master/src/intrinsic.rs. But so far they deliberately were not moved into rustc proper.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

What should become a const fn is MaybeUninit::zeroed. But that is still waiting on some more CTFE work, I think.

We could make just the zeroed intrinsic const fn then MaybeUninit::zeroed can be const fn immediately and we can un-const-fn the zeroed intrinsic (or even remove it entirely, once we get loops and conditions in const fn)

Or we can try getting

union Foo<T> {
    a: [u8; std::mem::size_of::<T>()],
    b: T,
}

to work, but that's lazy normalization, so... 😆

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

mem::zeroed and mem::uninitialized are a mess (the latter is deprecated), so I am opposed to making either of them const fn.

I'm in full agreement, the mem::zeroed and mem::uninitialized functions should never become const fn.

@RalfJung
Copy link
Member

My concern is with the implementation of the zeroed intrinsic (actually called init).

What's missing to make MaybeUninit::zeroed a const fn? Probably support for mutable references?

@RalfJung
Copy link
Member

But given that people have been asking for this for quite a while... do you think it is realistic to add an implementation of the init intrinsic now, but remove it again later once we got everything together to have the current implementation of zeroed in CTFE?

@RalfJung
Copy link
Member

Oh, you basically suggested that above.^^ Sorry.

So I think I wouldn't block that plan:

  1. Move the init and uninit implementations from Miri to librustc_mir. (Having the two not in the same place seems odd.)
  2. Make init unstable available as a const fn, use that to implement MaybeUninit::zeroed.
  3. Open an issue that says we will want to completely kill both of these intrinsics as soon as CTFE gets powerful enough to make MaybeUninit::zeroed a const fn without them.

If we want to avoid seeing uninit in librustc_mir, maybe we can kill that intrinsic today (and implement mem::uninitialized as MaybeUninit::uninit().assert_init()). That would be a separate 0th step then.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

That would be a separate 0th step then.

Let's do that first, yes

Couldn't we also make mem::zeroed call MaybeUninit::zeroed().assert_init() so that we only have a single use site of intrinsics::zeroed

The rest of the plan sounds good to me. We can also mark the init intrinsic as deprecated and allow that lint in the two places where it's "ok for now". This would help preventing new uses in libstd from popping up.

@RalfJung
Copy link
Member

Couldn't we also make mem::zeroed call MaybeUninit::zeroed().assert_init() so that we only have a single use site of intrinsics::zeroed

We could.

@alex
Copy link
Member Author

alex commented Jun 26, 2019

Just to be clear, the proposal is first a PR that does:

diff --git a/src/libcore/mem/mod.rs b/src/libcore/mem/mod.rs
index 770d1ca8e7..487785b878 100644
--- a/src/libcore/mem/mod.rs
+++ b/src/libcore/mem/mod.rs
@@ -450,8 +450,7 @@ pub const fn needs_drop<T>() -> bool {
 #[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub unsafe fn zeroed<T>() -> T {
-    intrinsics::panic_if_uninhabited::<T>();
-    intrinsics::init()
+    MaybeUninit::zeroed().assert_init()
 }
 
 /// Bypasses Rust's normal memory-initialization checks by pretending to
@@ -476,8 +475,7 @@ pub unsafe fn zeroed<T>() -> T {
 #[rustc_deprecated(since = "1.38.0", reason = "use `mem::MaybeUninit` instead")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub unsafe fn uninitialized<T>() -> T {
-    intrinsics::panic_if_uninhabited::<T>();
-    intrinsics::uninit()
+    MaybeUninit::uninit().assert_init()
 }
 
 /// Swaps the values at two mutable locations, without deinitializing either one.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

that and completely remove the uninit intrinsic and any code in the compiler that supports it

@oli-obk

This comment has been minimized.

@alex

This comment has been minimized.

@alex
Copy link
Member Author

alex commented Jun 26, 2019

Cool, I'll hopefully have a patch up for that shortly.

bors pushed a commit that referenced this issue Jun 26, 2019
bors added a commit that referenced this issue Jun 26, 2019
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs #62061

r? @oli-obk
@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

As discussed in the old PR (#54832 (comment)) we could also allow &mut references in const fn first, then make ptr::write_bytes const fn. That would allow us to skip the init minefield entirely

@RalfJung
Copy link
Member

What would it take to allow mutable references in const fn? And given that I could write const FOO: &mut T = foo();, how does that make things any easier compared to allowing them in all consts?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

Oh, I mean enable them in all consts. I'm not sure about the const qualif implications right now, but we could start by adding them behind a WIP feature gate without having to worry about accidental stabilizations. Wherever a &mut error is reported, only report if the relevant feature gate is not active. Should be a fairly simple PR (add feature gate and use it + some tests)

cc #57349

Centril added a commit to Centril/rust that referenced this issue Jun 28, 2019
…g,oli-obk,Centril

Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue Jun 28, 2019
…g,oli-obk,Centril

Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue Jun 28, 2019
…g,oli-obk,Centril

Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
@alex

This comment has been minimized.

@RalfJung

This comment has been minimized.

@alex

This comment has been minimized.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 4, 2019
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs rust-lang#62061

r? @oli-obk
@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Okay, uninit is gone for good. :-)

init stiill exists, but is deprecated. Progress on this issue will require deciding if we want to go with a minimal-mutable-references-for-CTFE (and killing init), or using init for MaybeUninit::zeroed until CTFE got more powerful.

Also @oli-obk is on vacation so we should probably wait until they are back.^^

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

deciding if we want to go with a minimal-mutable-references-for-CTFE (and killing init)

I'm strongly in favor of killing init and going with mutable references as the cleaner and principled approach (which is consistent with the principled approach we've taken thus far with const fn). As aforementioned it's also a good incentive to get &mut place/T working in const fn which seems more important.

@mjbshaw
Copy link
Contributor

mjbshaw commented Jul 17, 2019

For some prior art, see #54832, where I tried to make MaybeUninit::zeroed a const fn, but was ultimately rejected due to it relying on init (and didn't have any viable alternative since &mut isn't const-friendly) (edit: whoops, I overlooked the comment above that mentions this).

I would love to revive #54832 if people are okay with using init temporarily until CTFE has minimal support for mutable references.

As aforementioned it's also a good incentive to get &mut place/T working in const fn

I get that, and to a degree I agree, but to be honest I don't think it's working. We have people who could really use a const MaybeUninit::zeroed today, but this has been blocked on CTFE for a really long time.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2019

I get that, and to a degree I agree, but to be honest I don't think it's working.

I can guide you through implementing mutable references in const fn if you want. I believe it's not much work and stabilization wise it's just an extension of let bindings in const fn. The only reason I didn't add mutable references together with let bindings was a "small steps" approach which resulted in a standstill (and noone was pushing for mutable references).

@RalfJung
Copy link
Member

Okay, uninit is gone for good. :-)

Status update: that PR was reverted because it broke some programs. Those programs all had UB, but we reverted to give them more time to fix their stuff.

But the main blocker for this issue anyway is &mut T in const, and I heard rumors that @christianpoveda is working on this? Any updates?

@pvdrz
Copy link
Contributor

pvdrz commented Nov 21, 2019

Hi Ralf, here is the related PR: #66606

@Diggsey
Copy link
Contributor

Diggsey commented Dec 31, 2020

What's the status on this? It's quite problematic that mem::zeroed() is not const... I'm having to do this just to create a semaphore:

static mut STOP_SEM: UnsafeCell<MaybeUninit<libc::sem_t>> = UnsafeCell::new(MaybeUninit::uninit());

fn sem_ptr(sem: &UnsafeCell<MaybeUninit<libc::sem_t>>) -> *mut libc::sem_t {
    sem.get() as *mut libc::sem_t
}

Which is way more complicated and error-prone than it should be...

@oli-obk
Copy link
Contributor

oli-obk commented Jan 1, 2021

still blocked on #57349 for which I have a PR open. We just need a bit of time to figure out all the edge cases.

@maxbla
Copy link
Contributor

maxbla commented Feb 26, 2021

Until this issue is closed, anyone who wants a const mem::zeroed can use the const-zero crate that I recently released after facing this problem myself. Alternatively, to remove dependencies, see the trick that crate is based on.

@Lokathor
Copy link
Contributor

Max, you're truly doing Ferris' work.

@RalfJung
Copy link
Member

This has been a const fn since Rust 1.75.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants