Skip to content

stabilize ptr::swap_nonoverlapping in const #137280

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
1 change: 0 additions & 1 deletion library/core/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3319,7 +3319,6 @@ pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
#[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
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.
Expand Down
37 changes: 36 additions & 1 deletion library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1065,10 +1065,45 @@ pub const unsafe fn swap<T>(x: *mut T, y: *mut T) {
/// assert_eq!(x, [7, 8, 3, 4]);
/// assert_eq!(y, [1, 2, 9]);
/// ```
///
/// # Const evaluation limitations
///
/// If this function is invoked during const-evaluation, the current implementation has a small (and
/// rarely relevant) limitation: if `count` is at least 2 and the data pointed to by `x` or `y`
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it's a "current" thing not a promise, but specifically things like "at least 2" seem oddly specific.

Are we sure it's impossible to, say, create a failing version using a big packed struct that arranges to have an unaligned pointer that carries provenance but is unaligned so that it's copied in multiple parts by the implementation?

(I don't consider this an FCP blocker.)

Copy link
Member Author

@RalfJung RalfJung Feb 20, 2025

Choose a reason for hiding this comment

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

Yes we are. The const implementation will always swap entire T at once (this got fixed in #134689). We even have a test covering this:

fn test_const_swap_ptr() {
// The `swap` functions are implemented in the library, they are not primitives.
// Only `swap_nonoverlapping` takes a count; pointers that cross multiple elements
// are *not* supported.
// We put the pointer at an odd offset in the type and copy them as an array of bytes,
// which should catch most of the ways that the library implementation can get it wrong.
#[cfg(target_pointer_width = "32")]
type HalfPtr = i16;
#[cfg(target_pointer_width = "64")]
type HalfPtr = i32;
#[repr(C, packed)]
#[allow(unused)]
struct S {
f1: HalfPtr,
// Crucially this field is at an offset that is not a multiple of the pointer size.
ptr: &'static i32,
// Make sure the entire type does not have a power-of-2 size:
// make it 3 pointers in size. This used to hit a bug in `swap_nonoverlapping`.
f2: [HalfPtr; 3],
}
// Ensure the entire thing is usize-aligned, so in principle this
// looks like it could be eligible for a `usize` copying loop.
#[cfg_attr(target_pointer_width = "32", repr(align(4)))]
#[cfg_attr(target_pointer_width = "64", repr(align(8)))]
struct A(S);
const {
let mut s1 = A(S { ptr: &1, f1: 0, f2: [0; 3] });
let mut s2 = A(S { ptr: &666, f1: 0, f2: [0; 3] });
// Swap ptr1 and ptr2, as an array.
type T = [u8; mem::size_of::<A>()];
unsafe {
ptr::swap(ptr::from_mut(&mut s1).cast::<T>(), ptr::from_mut(&mut s2).cast::<T>());
}
// Make sure they still work.
assert!(*s1.0.ptr == 666);
assert!(*s2.0.ptr == 1);
// Swap them back, again as an array.
// (This is where we'd use a `u8` type and a `count` of `size_of::<T>()` if
// it were not for the limitation of `swap_nonoverlapping` around pointers crossing
// multiple elements.)
unsafe {
ptr::swap_nonoverlapping(
ptr::from_mut(&mut s1).cast::<T>(),
ptr::from_mut(&mut s2).cast::<T>(),
1,
);
}
// Make sure they still work.
assert!(*s1.0.ptr == 1);
assert!(*s2.0.ptr == 666);
};

"at least 2" is technically redundant since the text already says a pointer must span the boundary between two chunks being swapped, and if there's only one chunk there is no boundary. But I felt it helped make the point more clear.

/// contains a pointer that crosses the boundary of two `T`-sized chunks of memory, the function may
/// fail to evaluate (similar to a panic during const-evaluation). This behavior may change in the
/// future.
///
/// The limitation is illustrated by the following example:
///
/// ```
/// use std::mem::size_of;
/// use std::ptr;
///
/// const { unsafe {
/// const PTR_SIZE: usize = size_of::<*const i32>();
/// let mut data1 = [0u8; PTR_SIZE];
/// let mut data2 = [0u8; PTR_SIZE];
/// // Store a pointer in `data1`.
/// data1.as_mut_ptr().cast::<*const i32>().write_unaligned(&42);
/// // Swap the contents of `data1` and `data2` by swapping `PTR_SIZE` many `u8`-sized chunks.
/// // This call will fail, because the pointer in `data1` crosses the boundary
/// // between several of the 1-byte chunks that are being swapped here.
/// //ptr::swap_nonoverlapping(data1.as_mut_ptr(), data2.as_mut_ptr(), PTR_SIZE);
/// // Swap the contents of `data1` and `data2` by swapping a single chunk of size
/// // `[u8; PTR_SIZE]`. That works, as there is no pointer crossing the boundary between
/// // two chunks.
/// ptr::swap_nonoverlapping(&mut data1, &mut data2, 1);
/// // Read the pointer from `data2` and dereference it.
/// let ptr = data2.as_ptr().cast::<*const i32>().read_unaligned();
/// assert!(*ptr == 42);
/// } }
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Can you explicitly state that this may change in the future somewhere in the docs? I know the "current" wording seemingly implies that, but I'd rather do some very clear CYA here.

Also, the docs here seem to leave it a bit mysterious as to what "fail" means. Is it a panic? Silent logical error? Something else? Could that be clarified as well?

Copy link
Member

Choose a reason for hiding this comment

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

+1 for the last bit; saying "compilation may fail" or similar would be clearer.

Copy link
Member Author

@RalfJung RalfJung Apr 20, 2025

Choose a reason for hiding this comment

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

Can you explicitly state that this may change in the future somewhere in the docs? I know the "current" wording seemingly implies that, but I'd rather do some very clear CYA here.

Okay, done.

Copy link
Member Author

@RalfJung RalfJung Apr 20, 2025

Choose a reason for hiding this comment

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

Also, the docs here seem to leave it a bit mysterious as to what "fail" means. Is it a panic? Silent logical error? Something else? Could that be clarified as well?

It's a standard const-eval failure. I can say "fail to evaluate", not sure if that would help. I'm a bit worried if I say "compilation will fail" people think this is some sort of static analysis, when it is a runtime check -- but it's "at runtime of const-eval code". (So if a problematic call is in dead code, or you write a generic associated const that is never evaluated, compilation will still succeed.)

For is_null we said that the function panics in some cases in const. That's not 100% correct, but a const-eval failure is basically equivalent to a panic during const-eval.

#[inline]
#[stable(feature = "swap_nonoverlapping", since = "1.27.0")]
#[rustc_const_unstable(feature = "const_swap_nonoverlapping", issue = "133668")]
#[rustc_const_stable(feature = "const_swap_nonoverlapping", since = "CURRENT_RUSTC_VERSION")]
#[rustc_diagnostic_item = "ptr_swap_nonoverlapping"]
#[rustc_allow_const_fn_unstable(const_eval_select)] // both implementations behave the same
pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize) {
ub_checks::assert_unsafe_precondition!(
check_library_ub,
Expand Down
1 change: 0 additions & 1 deletion library/coretests/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#![feature(char_max_len)]
#![feature(clone_to_uninit)]
#![feature(const_eval_select)]
#![feature(const_swap_nonoverlapping)]
#![feature(const_trait_impl)]
#![feature(core_intrinsics)]
#![feature(core_intrinsics_fallbacks)]
Expand Down
4 changes: 4 additions & 0 deletions library/coretests/tests/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,10 @@ fn test_const_swap_ptr() {
// Make sure they still work.
assert!(*s1.0.ptr == 1);
assert!(*s2.0.ptr == 666);

// This is where we'd swap again using a `u8` type and a `count` of `size_of::<T>()` if it
// were not for the limitation of `swap_nonoverlapping` around pointers crossing multiple
// elements.
};
}

Expand Down
2 changes: 0 additions & 2 deletions tests/ui/consts/missing_span_in_backtrace.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//@ compile-flags: -Z ui-testing=no


#![feature(const_swap_nonoverlapping)]
use std::{
mem::{self, MaybeUninit},
ptr,
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/consts/missing_span_in_backtrace.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error[E0080]: evaluation of constant value failed
--> $DIR/missing_span_in_backtrace.rs:16:9
--> $DIR/missing_span_in_backtrace.rs:14:9
|
16 | / ptr::swap_nonoverlapping(
17 | | &mut ptr1 as *mut _ as *mut MaybeUninit<u8>,
18 | | &mut ptr2 as *mut _ as *mut MaybeUninit<u8>,
19 | | mem::size_of::<&i32>(),
20 | | );
14 | / ptr::swap_nonoverlapping(
15 | | &mut ptr1 as *mut _ as *mut MaybeUninit<u8>,
16 | | &mut ptr2 as *mut _ as *mut MaybeUninit<u8>,
17 | | mem::size_of::<&i32>(),
18 | | );
| |_________^ unable to copy parts of a pointer from memory at ALLOC0
|
note: inside `swap_nonoverlapping::<MaybeUninit<u8>>`
Expand Down
Loading