Skip to content

stabilize const_swap #134757

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

Merged
merged 5 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// If we're swapping something that's *not* an `OperandValue::Ref`,
// then we can do it directly and avoid the alloca.
// Otherwise, we'll let the fallback MIR body take care of it.
if let sym::typed_swap = name {
if let sym::typed_swap_nonoverlapping = name {
let pointee_ty = fn_args.type_at(0);
let pointee_layout = bx.layout_of(pointee_ty);
if !bx.is_backend_ref(pointee_layout)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/traits/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ pub trait BuilderMethods<'a, 'tcx>:
/// Avoids `alloca`s for Immediates and ScalarPairs.
///
/// FIXME: Maybe do something smarter for Ref types too?
/// For now, the `typed_swap` intrinsic just doesn't call this for those
/// For now, the `typed_swap_nonoverlapping` intrinsic just doesn't call this for those
/// cases (in non-debug), preferring the fallback body instead.
fn typed_place_swap(
&mut self,
Expand Down
14 changes: 1 addition & 13 deletions compiler/rustc_const_eval/src/interpret/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -883,19 +883,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
.local_to_op(mir::RETURN_PLACE, None)
.expect("return place should always be live");
let dest = self.frame().return_place.clone();
let res = if self.stack().len() == 1 {
// The initializer of constants and statics will get validated separately
// after the constant has been fully evaluated. While we could fall back to the default
// code path, that will cause -Zenforce-validity to cycle on static initializers.
// Reading from a static's memory is not allowed during its evaluation, and will always
// trigger a cycle error. Validation must read from the memory of the current item.
// For Miri this means we do not validate the root frame return value,
// but Miri anyway calls `read_target_isize` on that so separate validation
// is not needed.
Comment on lines -887 to -894
Copy link
Member Author

@RalfJung RalfJung Dec 30, 2024

Choose a reason for hiding this comment

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

I think this comment is outdated ever since we changed how statics get interned and when exactly validation runs... but if there's still a risk of an ICE here I am sure matthiaskrgr will find it in no time and then we'll have a testcase. ;)

self.copy_op_no_dest_validation(&op, &dest)
} else {
self.copy_op_allow_transmute(&op, &dest)
};
let res = self.copy_op_allow_transmute(&op, &dest);
trace!("return value: {:?}", self.dump_place(&dest.into()));
// We delay actually short-circuiting on this error until *after* the stack frame is
// popped, since we want this error to be attributed to the caller, whose type defines
Expand Down
30 changes: 23 additions & 7 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,8 +424,8 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
let result = self.raw_eq_intrinsic(&args[0], &args[1])?;
self.write_scalar(result, dest)?;
}
sym::typed_swap => {
self.typed_swap_intrinsic(&args[0], &args[1])?;
sym::typed_swap_nonoverlapping => {
self.typed_swap_nonoverlapping_intrinsic(&args[0], &args[1])?;
}

sym::vtable_size => {
Expand Down Expand Up @@ -638,19 +638,35 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}

/// Does a *typed* swap of `*left` and `*right`.
fn typed_swap_intrinsic(
fn typed_swap_nonoverlapping_intrinsic(
&mut self,
left: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
right: &OpTy<'tcx, <M as Machine<'tcx>>::Provenance>,
) -> InterpResult<'tcx> {
let left = self.deref_pointer(left)?;
let right = self.deref_pointer(right)?;
debug_assert_eq!(left.layout, right.layout);
assert_eq!(left.layout, right.layout);
assert!(left.layout.is_sized());
let kind = MemoryKind::Stack;
let temp = self.allocate(left.layout, kind)?;
self.copy_op(&left, &temp)?;
self.copy_op(&right, &left)?;
self.copy_op(&temp, &right)?;
self.copy_op(&left, &temp)?; // checks alignment of `left`

// We want to always enforce non-overlapping, even if this is a scalar type.
// Therefore we directly use the underlying `mem_copy` here.
self.mem_copy(right.ptr(), left.ptr(), left.layout.size, /*nonoverlapping*/ true)?;
// This means we also need to do the validation of the value that used to be in `right`
// ourselves. This value is now in `left.` The one that started out in `left` already got
// validated by the copy above.
if M::enforce_validity(self, left.layout) {
self.validate_operand(
&left.clone().into(),
M::enforce_validity_recursively(self, left.layout),
/*reset_provenance_and_padding*/ true,
)?;
}

self.copy_op(&temp, &right)?; // checks alignment of `right`

self.deallocate_ptr(temp.ptr(), None, kind)?;
interp_ok(())
}
Expand Down
31 changes: 5 additions & 26 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -773,22 +773,6 @@ where
interp_ok(())
}

/// Copies the data from an operand to a place.
/// The layouts of the `src` and `dest` may disagree.
/// Does not perform validation of the destination.
/// The only known use case for this function is checking the return
/// value of a static during stack frame popping.
#[inline(always)]
pub(super) fn copy_op_no_dest_validation(
&mut self,
src: &impl Projectable<'tcx, M::Provenance>,
dest: &impl Writeable<'tcx, M::Provenance>,
) -> InterpResult<'tcx> {
self.copy_op_inner(
src, dest, /* allow_transmute */ true, /* validate_dest */ false,
)
}

/// Copies the data from an operand to a place.
/// The layouts of the `src` and `dest` may disagree.
#[inline(always)]
Expand All @@ -797,9 +781,7 @@ where
src: &impl Projectable<'tcx, M::Provenance>,
dest: &impl Writeable<'tcx, M::Provenance>,
) -> InterpResult<'tcx> {
self.copy_op_inner(
src, dest, /* allow_transmute */ true, /* validate_dest */ true,
)
self.copy_op_inner(src, dest, /* allow_transmute */ true)
}

/// Copies the data from an operand to a place.
Expand All @@ -810,9 +792,7 @@ where
src: &impl Projectable<'tcx, M::Provenance>,
dest: &impl Writeable<'tcx, M::Provenance>,
) -> InterpResult<'tcx> {
self.copy_op_inner(
src, dest, /* allow_transmute */ false, /* validate_dest */ true,
)
self.copy_op_inner(src, dest, /* allow_transmute */ false)
}

/// Copies the data from an operand to a place.
Expand All @@ -824,22 +804,21 @@ where
src: &impl Projectable<'tcx, M::Provenance>,
dest: &impl Writeable<'tcx, M::Provenance>,
allow_transmute: bool,
validate_dest: bool,
) -> InterpResult<'tcx> {
// These are technically *two* typed copies: `src` is a not-yet-loaded value,
// so we're going a typed copy at `src` type from there to some intermediate storage.
// so we're doing a typed copy at `src` type from there to some intermediate storage.
// And then we're doing a second typed copy from that intermediate storage to `dest`.
// But as an optimization, we only make a single direct copy here.

// Do the actual copy.
self.copy_op_no_validate(src, dest, allow_transmute)?;

if validate_dest && M::enforce_validity(self, dest.layout()) {
if M::enforce_validity(self, dest.layout()) {
let dest = dest.to_place();
// Given that there were two typed copies, we have to ensure this is valid at both types,
// and we have to ensure this loses provenance and padding according to both types.
// But if the types are identical, we only do one pass.
if allow_transmute && src.layout().ty != dest.layout().ty {
if src.layout().ty != dest.layout().ty {
self.validate_operand(
&dest.transmute(src.layout(), self)?,
M::enforce_validity_recursively(self, src.layout()),
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_hir_analysis/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,9 @@ pub fn check_intrinsic_type(
(1, 0, vec![Ty::new_mut_ptr(tcx, param(0)), param(0)], tcx.types.unit)
}

sym::typed_swap => (1, 0, vec![Ty::new_mut_ptr(tcx, param(0)); 2], tcx.types.unit),
sym::typed_swap_nonoverlapping => {
(1, 0, vec![Ty::new_mut_ptr(tcx, param(0)); 2], tcx.types.unit)
}

sym::discriminant_value => {
let assoc_items = tcx.associated_item_def_ids(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2058,7 +2058,7 @@ symbols! {
type_macros,
type_name,
type_privacy_lints,
typed_swap,
typed_swap_nonoverlapping,
u128,
u128_legacy_const_max,
u128_legacy_const_min,
Expand Down
22 changes: 19 additions & 3 deletions library/core/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3940,6 +3940,21 @@ pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
false
}

#[rustc_nounwind]
#[inline]
#[rustc_intrinsic]
#[rustc_intrinsic_const_stable_indirect]
#[rustc_allow_const_fn_unstable(const_swap_nonoverlapping)] // this is anyway not called since CTFE implements the intrinsic
#[cfg(bootstrap)]
pub const unsafe fn typed_swap<T>(x: *mut T, y: *mut T) {
// SAFETY: The caller provided single non-overlapping items behind
// pointers, so swapping them with `count: 1` is fine.
unsafe { ptr::swap_nonoverlapping(x, y, 1) };
}

#[cfg(bootstrap)]
pub use typed_swap as typed_swap_nonoverlapping;

/// Non-overlapping *typed* swap of a single value.
///
/// The codegen backends will replace this with a better implementation when
Expand All @@ -3953,9 +3968,10 @@ pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
#[rustc_nounwind]
#[inline]
#[rustc_intrinsic]
// Const-unstable because `swap_nonoverlapping` is const-unstable.
#[rustc_const_unstable(feature = "const_typed_swap", issue = "none")]
pub const unsafe fn typed_swap<T>(x: *mut T, y: *mut T) {
#[rustc_intrinsic_const_stable_indirect]
#[rustc_allow_const_fn_unstable(const_swap_nonoverlapping)] // this is anyway not called since CTFE implements the intrinsic
#[cfg(not(bootstrap))]
pub const unsafe fn typed_swap_nonoverlapping<T>(x: *mut T, y: *mut T) {
// SAFETY: The caller provided single non-overlapping items behind
// pointers, so swapping them with `count: 1` is fine.
unsafe { ptr::swap_nonoverlapping(x, y, 1) };
Expand Down
1 change: 0 additions & 1 deletion library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@
#![feature(array_ptr_get)]
#![feature(asm_experimental_arch)]
#![feature(const_eval_select)]
#![feature(const_typed_swap)]
#![feature(core_intrinsics)]
#![feature(coverage_attribute)]
#![feature(internal_impls_macro)]
Expand Down
4 changes: 2 additions & 2 deletions library/core/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -725,12 +725,12 @@ pub unsafe fn uninitialized<T>() -> T {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
#[rustc_const_stable(feature = "const_swap", since = "CURRENT_RUSTC_VERSION")]
#[rustc_diagnostic_item = "mem_swap"]
pub const fn swap<T>(x: &mut T, y: &mut T) {
// SAFETY: `&mut` guarantees these are typed readable and writable
// as well as non-overlapping.
unsafe { intrinsics::typed_swap(x, y) }
unsafe { intrinsics::typed_swap_nonoverlapping(x, y) }
}

/// Replaces `dest` with the default value of `T`, returning the previous `dest` value.
Expand Down
3 changes: 1 addition & 2 deletions library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1009,9 +1009,8 @@ pub const fn slice_from_raw_parts_mut<T>(data: *mut T, len: usize) -> *mut [T] {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
#[rustc_const_stable(feature = "const_swap", since = "CURRENT_RUSTC_VERSION")]
#[rustc_diagnostic_item = "ptr_swap"]
#[rustc_const_stable_indirect]
pub const unsafe fn swap<T>(x: *mut T, y: *mut T) {
// Give ourselves some scratch space to work with.
// We do not have to worry about drops: `MaybeUninit` does nothing when dropped.
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1594,7 +1594,7 @@ impl<T: ?Sized> *mut T {
///
/// [`ptr::swap`]: crate::ptr::swap()
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
#[rustc_const_stable(feature = "const_swap", since = "CURRENT_RUSTC_VERSION")]
#[inline(always)]
pub const unsafe fn swap(self, with: *mut T)
where
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,7 @@ impl<T: ?Sized> NonNull<T> {
/// [`ptr::swap`]: crate::ptr::swap()
#[inline(always)]
#[stable(feature = "non_null_convenience", since = "1.80.0")]
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
#[rustc_const_stable(feature = "const_swap", since = "CURRENT_RUSTC_VERSION")]
pub const unsafe fn swap(self, with: NonNull<T>)
where
T: Sized,
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,7 @@ impl<T> [T] {
/// assert!(v == ["a", "b", "e", "d", "c"]);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
#[rustc_const_stable(feature = "const_swap", since = "CURRENT_RUSTC_VERSION")]
#[inline]
#[track_caller]
pub const fn swap(&mut self, a: usize, b: usize) {
Expand Down
1 change: 0 additions & 1 deletion library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#![feature(clone_to_uninit)]
#![feature(const_black_box)]
#![feature(const_eval_select)]
#![feature(const_swap)]
#![feature(const_swap_nonoverlapping)]
#![feature(const_trait_impl)]
#![feature(core_intrinsics)]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![feature(core_intrinsics)]
#![feature(rustc_attrs)]

use std::intrinsics::typed_swap;
use std::intrinsics::typed_swap_nonoverlapping;
use std::ptr::addr_of_mut;

fn invalid_array() {
Expand All @@ -10,7 +10,7 @@ fn invalid_array() {
unsafe {
let a = addr_of_mut!(a).cast::<[bool; 100]>();
let b = addr_of_mut!(b).cast::<[bool; 100]>();
typed_swap(a, b); //~ERROR: constructing invalid value
typed_swap_nonoverlapping(a, b); //~ERROR: constructing invalid value
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: constructing invalid value at [0]: encountered 0x02, but expected a boolean
--> tests/fail/intrinsics/typed-swap-invalid-array.rs:LL:CC
|
LL | typed_swap(a, b);
| ^^^^^^^^^^^^^^^^ constructing invalid value at [0]: encountered 0x02, but expected a boolean
LL | typed_swap_nonoverlapping(a, b);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at [0]: encountered 0x02, but expected a boolean
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: constructing invalid value: encountered 0x02, but expected a boolean
--> tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
|
LL | typed_swap(a, b);
| ^^^^^^^^^^^^^^^^ constructing invalid value: encountered 0x02, but expected a boolean
LL | typed_swap_nonoverlapping(a, b);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered 0x02, but expected a boolean
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error: Undefined Behavior: constructing invalid value: encountered 0x03, but expected a boolean
--> tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
|
LL | typed_swap_nonoverlapping(a, b);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered 0x03, but expected a boolean
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `invalid_scalar` at tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
note: inside `main`
--> tests/fail/intrinsics/typed-swap-invalid-scalar.rs:LL:CC
|
LL | invalid_scalar();
| ^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
//@revisions: left right
#![feature(core_intrinsics)]
#![feature(rustc_attrs)]

use std::intrinsics::typed_swap;
use std::intrinsics::typed_swap_nonoverlapping;
use std::ptr::addr_of_mut;

fn invalid_scalar() {
let mut a = 1_u8;
let mut b = 2_u8;
// We run the test twice, with either the left or the right side being invalid.
let mut a = if cfg!(left) { 2_u8 } else { 1_u8 };
let mut b = if cfg!(right) { 3_u8 } else { 1_u8 };
unsafe {
let a = addr_of_mut!(a).cast::<bool>();
let b = addr_of_mut!(b).cast::<bool>();
typed_swap(a, b); //~ERROR: constructing invalid value
typed_swap_nonoverlapping(a, b); //~ERROR: constructing invalid value
}
}

Expand Down
13 changes: 13 additions & 0 deletions src/tools/miri/tests/fail/intrinsics/typed-swap-overlap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![feature(core_intrinsics)]
#![feature(rustc_attrs)]

use std::intrinsics::typed_swap_nonoverlapping;
use std::ptr::addr_of_mut;

fn main() {
let mut a = 0_u8;
unsafe {
let a = addr_of_mut!(a);
typed_swap_nonoverlapping(a, a); //~ERROR: called on overlapping ranges
}
}
Loading
Loading