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.

Sign up for GitHub

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

Jump to bottom

Stabilize const_maybe_uninit_zeroed and const_mem_zeroed #116218

Merged
merged 2 commits into from
Nov 5, 2023
Merged

Stabilize const_maybe_uninit_zeroed and const_mem_zeroed #116218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6ce646
Stabilize `const_maybe_uninit_zeroed`
tgross35 Sep 28, 2023
5e5f334
Stabilize `const_mem_zeroed`
tgross35 Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion library/alloc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@
#![feature(const_eval_select)]
#![feature(const_maybe_uninit_as_mut_ptr)]
#![feature(const_maybe_uninit_write)]
#![feature(const_maybe_uninit_zeroed)]
#![feature(const_pin)]
#![feature(const_refs_to_cell)]
#![feature(const_size_of_val)]
Expand Down
4 changes: 2 additions & 2 deletions library/core/src/intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,15 +1072,15 @@ extern "rust-intrinsic" {
/// zero-initialization: This will statically either panic, or do nothing.
///
/// This intrinsic does not have a stable counterpart.
#[rustc_const_unstable(feature = "const_assert_type2", issue = "none")]
#[rustc_const_stable(feature = "const_assert_type2", since = "CURRENT_RUSTC_VERSION")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn assert_zero_valid<T>();

/// A guard for `std::mem::uninitialized`. This will statically either panic, or do nothing.
///
/// This intrinsic does not have a stable counterpart.
#[rustc_const_unstable(feature = "const_assert_type2", issue = "none")]
#[rustc_const_stable(feature = "const_assert_type2", since = "CURRENT_RUSTC_VERSION")]
#[rustc_safe_intrinsic]
#[rustc_nounwind]
pub fn assert_mem_uninitialized_valid<T>();
Expand Down
17 changes: 11 additions & 6 deletions library/core/src/mem/maybe_uninit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,9 @@ impl<T> MaybeUninit<T> {
/// assert_eq!(x, (0, false));
/// ```
///
/// This can be used in const contexts, such as to indicate the end of static arrays for
/// plugin registration.
///
/// *Incorrect* usage of this function: calling `x.zeroed().assume_init()`
/// when `0` is not a valid bit-pattern for the type:
///
Expand All @@ -387,17 +390,19 @@ impl<T> MaybeUninit<T> {
/// // Inside a pair, we create a `NotZero` that does not have a valid discriminant.
/// // This is undefined behavior. ⚠️
/// ```
#[stable(feature = "maybe_uninit", since = "1.36.0")]
#[rustc_const_unstable(feature = "const_maybe_uninit_zeroed", issue = "91850")]
#[must_use]
#[inline]
#[must_use]
#[rustc_diagnostic_item = "maybe_uninit_zeroed"]
#[stable(feature = "maybe_uninit", since = "1.36.0")]
// These are OK to allow since we do not leak &mut to user-visible API
#[rustc_allow_const_fn_unstable(const_mut_refs)]
#[rustc_allow_const_fn_unstable(const_ptr_write)]
#[rustc_allow_const_fn_unstable(const_maybe_uninit_as_mut_ptr)]
#[rustc_const_stable(feature = "const_maybe_uninit_zeroed", since = "CURRENT_RUSTC_VERSION")]
pub const fn zeroed() -> MaybeUninit<T> {
let mut u = MaybeUninit::<T>::uninit();
// SAFETY: `u.as_mut_ptr()` points to allocated memory.
unsafe {
u.as_mut_ptr().write_bytes(0u8, 1);
}
unsafe { u.as_mut_ptr().write_bytes(0u8, 1) };
u
}

Expand Down
3 changes: 2 additions & 1 deletion library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ pub const fn needs_drop<T: ?Sized>() -> bool {
#[allow(deprecated)]
#[rustc_diagnostic_item = "mem_zeroed"]
#[track_caller]
pub unsafe fn zeroed<T>() -> T {
#[rustc_const_stable(feature = "const_mem_zeroed", since = "CURRENT_RUSTC_VERSION")]
pub const unsafe fn zeroed<T>() -> T {
// SAFETY: the caller must guarantee that an all-zero value is valid for `T`.
unsafe {
intrinsics::assert_zero_valid::<T>();
Expand Down
21 changes: 21 additions & 0 deletions library/core/tests/mem.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,24 @@ fn offset_of_addr() {
assert_eq!(ptr::addr_of!(base).addr() + offset_of!(Foo, z.0), ptr::addr_of!(base.z.0).addr());
assert_eq!(ptr::addr_of!(base).addr() + offset_of!(Foo, z.1), ptr::addr_of!(base.z.1).addr());
}

#[test]
fn const_maybe_uninit_zeroed() {
// Sanity check for `MaybeUninit::zeroed` in a realistic const situation (plugin array term)
#[repr(C)]
struct Foo {
a: Option<&'static str>,
b: Bar,
c: f32,
d: *const u8,
}
#[repr(C)]
struct Bar(usize);
struct FooPtr(*const Foo);
unsafe impl Sync for FooPtr {}

static UNINIT: FooPtr = FooPtr([unsafe { MaybeUninit::zeroed().assume_init() }].as_ptr());
Comment on lines +570 to +584
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what motivated this test but there actually isn't any guarantee that all-zero bytes are a valid value for Option<&'static str> since that's a fat pointer.

NPO is only guaranteed for thin pointers

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but just because there is no guarantee doesn't mean we should have no test.

Or put differently, library and ui tests are a rustc implementation detail and not source for stable guarantees. Only docs are stable guarantees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should make that a docs guarantee for &str, as a separate future PR

Copy link
Member

Choose a reason for hiding this comment

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

That'd conflict with niches-for-metadata that I'm working on. The length field will have a larger niche which means it becomes preferred by the layout algorithm and Option::None is no longer zero but some other value.
That's how I found this.

Or put differently, library and ui tests are a rustc implementation detail and not source for stable guarantees.

I mean that this looks like it has been inspired by some real code and that that code is suspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds extremely unfortunate and that a special case should be put back in or something. But we can go into that on zulip or whatever PR you open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was just a sanity check that it does indeed do the right thing. I suppose &str might not have been the best optional reference, but that specific choice wasn't motivated by any real code.

Otoh, it should still zero everything even if invalid, even if Miri one day starts flagging this test.

Copy link
Member

Choose a reason for hiding this comment

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

error[E0080]: it is undefined behavior to use this value
   --> library/core/tests/mem.rs:789:5
    |
789 |     static UNINIT: FooPtr = FooPtr([unsafe { MaybeUninit::zeroed().assume_init() }].as_ptr());
    |     ^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .<deref>[0].a.<enum-variant(Some)>.0: encountered a null reference
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
    = note: the raw bytes of the constant (size: 8, align: 8) {
                ╾───alloc7286<imm>────╼                         │ ╾──────╼
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otoh, it should still zero everything even if invalid, even if Miri one day starts flagging this test.

...or the compiler itself I suppose. Feel free to change or #[allow(...)] this test as needed, as long as there isn't any risk of something like MaybeUninit::zeroed()'s output later being fixed up with a non-zero niche in the pointer metadata (seems unlikely)

Copy link
Member

Choose a reason for hiding this comment

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

Even if we never guaranteed NPO for wide pointers, changing that seems very risky and I expect it will break some unsafe code out there. Most people won't expect such a discontinuity between thin and wide ptrs.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out the actual problem with this test is that it is UB because the type has padding, so the memory will not be all-zeroes after being returned by assume_init(). ;)

const SIZE: usize = size_of::<Foo>();

assert_eq!(unsafe { (*UNINIT.0.cast::<[[u8; SIZE]; 1]>())[0] }, [0u8; SIZE]);
}
1 change: 0 additions & 1 deletion tests/ui/consts/assert-type-intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![feature(never_type)]
#![feature(const_assert_type2)]
#![feature(core_intrinsics)]

use std::intrinsics;
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/consts/assert-type-intrinsics.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
error[E0080]: evaluation of constant value failed
--> $DIR/assert-type-intrinsics.rs:12:9
--> $DIR/assert-type-intrinsics.rs:11:9
|
LL | MaybeUninit::<!>::uninit().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to instantiate uninhabited type `!`', $DIR/assert-type-intrinsics.rs:12:36
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to instantiate uninhabited type `!`', $DIR/assert-type-intrinsics.rs:11:36

error[E0080]: evaluation of constant value failed
--> $DIR/assert-type-intrinsics.rs:16:9
--> $DIR/assert-type-intrinsics.rs:15:9
|
LL | intrinsics::assert_mem_uninitialized_valid::<&'static i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to leave type `&i32` uninitialized, which is invalid', $DIR/assert-type-intrinsics.rs:16:9
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to leave type `&i32` uninitialized, which is invalid', $DIR/assert-type-intrinsics.rs:15:9

error[E0080]: evaluation of constant value failed
--> $DIR/assert-type-intrinsics.rs:20:9
--> $DIR/assert-type-intrinsics.rs:19:9
|
LL | intrinsics::assert_zero_valid::<&'static i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to zero-initialize type `&i32`, which is invalid', $DIR/assert-type-intrinsics.rs:20:9
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'aborted execution: attempted to zero-initialize type `&i32`, which is invalid', $DIR/assert-type-intrinsics.rs:19:9

error: aborting due to 3 previous errors

Expand Down
Loading