Skip to content

Commit 313c4bc

Browse files
RalfJungRalfJung
committed
core: fix const ptr::swap_nonoverlapping when there are pointers at odd offsets in the type
1 parent 85c3989 commit 313c4bc

File tree

4 files changed

+103
-48
lines changed

4 files changed

+103
-48
lines changed

‎library/core/src/intrinsics/mod.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3795,15 +3795,15 @@ where
37953795
/// See [`const_eval_select()`] for the rules and requirements around that intrinsic.
37963796
pub(crate) macro const_eval_select {
37973797
(
3798-
@capture { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
3798+
@capture$([$($binders:tt)*])? { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
37993799
if const
38003800
$(#[$compiletime_attr:meta])* $compiletime:block
38013801
else
38023802
$(#[$runtime_attr:meta])* $runtime:block
38033803
) => {
38043804
// Use the `noinline` arm, after adding explicit `inline` attributes
38053805
$crate::intrinsics::const_eval_select!(
3806-
@capture { $($arg : $ty = $val),* } $(-> $ret)? :
3806+
@capture$([$($binders)*])? { $($arg : $ty = $val),* } $(-> $ret)? :
38073807
#[noinline]
38083808
if const
38093809
#[inline] // prevent codegen on this function
@@ -3817,20 +3817,20 @@ pub(crate) macro const_eval_select {
38173817
},
38183818
// With a leading #[noinline], we don't add inline attributes
38193819
(
3820-
@capture { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
3820+
@capture$([$($binders:tt)*])? { $($arg:ident : $ty:ty = $val:expr),* $(,)? } $( -> $ret:ty )? :
38213821
#[noinline]
38223822
if const
38233823
$(#[$compiletime_attr:meta])* $compiletime:block
38243824
else
38253825
$(#[$runtime_attr:meta])* $runtime:block
38263826
) => {{
38273827
$(#[$runtime_attr])*
3828-
fn runtime($($arg: $ty),*) $( -> $ret )? {
3828+
fn runtime$(<$($binders)*>)?($($arg: $ty),*) $( -> $ret )? {
38293829
$runtime
38303830
}
38313831

38323832
$(#[$compiletime_attr])*
3833-
const fn compiletime($($arg: $ty),*) $( -> $ret )? {
3833+
const fn compiletime$(<$($binders)*>)?($($arg: $ty),*) $( -> $ret )? {
38343834
// Don't warn if one of the arguments is unused.
38353835
$(let _ = $arg;)*
38363836

@@ -3842,14 +3842,14 @@ pub(crate) macro const_eval_select {
38423842
// We support leaving away the `val` expressions for *all* arguments
38433843
// (but not for *some* arguments, that's too tricky).
38443844
(
3845-
@capture { $($arg:ident : $ty:ty),* $(,)? } $( -> $ret:ty )? :
3845+
@capture$([$($binders:tt)*])? { $($arg:ident : $ty:ty),* $(,)? } $( -> $ret:ty )? :
38463846
if const
38473847
$(#[$compiletime_attr:meta])* $compiletime:block
38483848
else
38493849
$(#[$runtime_attr:meta])* $runtime:block
38503850
) => {
38513851
$crate::intrinsics::const_eval_select!(
3852-
@capture { $($arg : $ty = $arg),* } $(-> $ret)? :
3852+
@capture$([$($binders)*])? { $($arg : $ty = $arg),* } $(-> $ret)? :
38533853
if const
38543854
$(#[$compiletime_attr])* $compiletime
38553855
else

‎library/core/src/ptr/mod.rs‎

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,7 @@
395395
#![allow(clippy::not_unsafe_ptr_arg_deref)]
396396

397397
use crate::cmp::Ordering;
398+
use crate::intrinsics::const_eval_select;
398399
use crate::marker::FnPtr;
399400
use crate::mem::{self, MaybeUninit, SizedTypeProperties};
400401
use crate::{fmt, hash, intrinsics, ub_checks};
@@ -1074,25 +1075,6 @@ pub const unsafe fn swap<T>(x: *mut T, y: *mut T) {
10741075
#[rustc_const_unstable(feature = "const_swap_nonoverlapping", issue = "133668")]
10751076
#[rustc_diagnostic_item = "ptr_swap_nonoverlapping"]
10761077
pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize) {
1077-
#[allow(unused)]
1078-
macro_rules! attempt_swap_as_chunks {
1079-
($ChunkTy:ty) => {
1080-
if mem::align_of::<T>() >= mem::align_of::<$ChunkTy>()
1081-
&& mem::size_of::<T>() % mem::size_of::<$ChunkTy>() == 0
1082-
{
1083-
let x: *mut $ChunkTy = x.cast();
1084-
let y: *mut $ChunkTy = y.cast();
1085-
let count = count * (mem::size_of::<T>() / mem::size_of::<$ChunkTy>());
1086-
// SAFETY: these are the same bytes that the caller promised were
1087-
// ok, just typed as `MaybeUninit<ChunkTy>`s instead of as `T`s.
1088-
// The `if` condition above ensures that we're not violating
1089-
// alignment requirements, and that the division is exact so
1090-
// that we don't lose any bytes off the end.
1091-
return unsafe { swap_nonoverlapping_simple_untyped(x, y, count) };
1092-
}
1093-
};
1094-
}
1095-
10961078
ub_checks::assert_unsafe_precondition!(
10971079
check_language_ub,
10981080
"ptr::swap_nonoverlapping requires that both pointer arguments are aligned and non-null \
@@ -1111,19 +1093,48 @@ pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize) {
11111093
}
11121094
);
11131095

1114-
// Split up the slice into small power-of-two-sized chunks that LLVM is able
1115-
// to vectorize (unless it's a special type with more-than-pointer alignment,
1116-
// because we don't want to pessimize things like slices of SIMD vectors.)
1117-
if mem::align_of::<T>() <= mem::size_of::<usize>()
1118-
&& (!mem::size_of::<T>().is_power_of_two()
1119-
|| mem::size_of::<T>() > mem::size_of::<usize>() * 2)
1120-
{
1121-
attempt_swap_as_chunks!(usize);
1122-
attempt_swap_as_chunks!(u8);
1123-
}
1096+
const_eval_select!(
1097+
@capture[T] { x: *mut T, y: *mut T, count: usize }:
1098+
if const {
1099+
// At compile-time we want to always copy this in chunks of `T`, to ensure that if there
1100+
// are pointers inside `T` we will copy them in one go rather than trying to copy a part
1101+
// of a pointer (which would not work).
1102+
// SAFETY: Same preconditions as this function
1103+
unsafe { swap_nonoverlapping_simple_untyped(x, y, count) }
1104+
} else {
1105+
macro_rules! attempt_swap_as_chunks {
1106+
($ChunkTy:ty) => {
1107+
if mem::align_of::<T>() >= mem::align_of::<$ChunkTy>()
1108+
&& mem::size_of::<T>() % mem::size_of::<$ChunkTy>() == 0
1109+
{
1110+
let x: *mut $ChunkTy = x.cast();
1111+
let y: *mut $ChunkTy = y.cast();
1112+
let count = count * (mem::size_of::<T>() / mem::size_of::<$ChunkTy>());
1113+
// SAFETY: these are the same bytes that the caller promised were
1114+
// ok, just typed as `MaybeUninit<ChunkTy>`s instead of as `T`s.
1115+
// The `if` condition above ensures that we're not violating
1116+
// alignment requirements, and that the division is exact so
1117+
// that we don't lose any bytes off the end.
1118+
return unsafe { swap_nonoverlapping_simple_untyped(x, y, count) };
1119+
}
1120+
};
1121+
}
1122+
1123+
// Split up the slice into small power-of-two-sized chunks that LLVM is able
1124+
// to vectorize (unless it's a special type with more-than-pointer alignment,
1125+
// because we don't want to pessimize things like slices of SIMD vectors.)
1126+
if mem::align_of::<T>() <= mem::size_of::<usize>()
1127+
&& (!mem::size_of::<T>().is_power_of_two()
1128+
|| mem::size_of::<T>() > mem::size_of::<usize>() * 2)
1129+
{
1130+
attempt_swap_as_chunks!(usize);
1131+
attempt_swap_as_chunks!(u8);
1132+
}
11241133

1125-
// SAFETY: Same preconditions as this function
1126-
unsafe { swap_nonoverlapping_simple_untyped(x, y, count) }
1134+
// SAFETY: Same preconditions as this function
1135+
unsafe { swap_nonoverlapping_simple_untyped(x, y, count) }
1136+
}
1137+
)
11271138
}
11281139

11291140
/// Same behavior and safety conditions as [`swap_nonoverlapping`]

‎library/core/tests/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#![feature(const_black_box)]
1717
#![feature(const_eval_select)]
1818
#![feature(const_swap)]
19+
#![feature(const_swap_nonoverlapping)]
1920
#![feature(const_trait_impl)]
2021
#![feature(core_intrinsics)]
2122
#![feature(core_io_borrowed_buf)]

‎library/core/tests/ptr.rs‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -860,7 +860,10 @@ fn swap_copy_untyped() {
860860
}
861861

862862
#[test]
863-
fn test_const_copy() {
863+
fn test_const_copy_ptr() {
864+
// `copy` and `copy_nonoverlapping` are thin layers on top of intrinsics. Ensure they correctly
865+
// deal with pointers even when the pointers cross the boundary from one "element" being copied
866+
// to another.
864867
const {
865868
let ptr1 = &1;
866869
let mut ptr2 = &666;
@@ -899,21 +902,61 @@ fn test_const_copy() {
899902
}
900903

901904
#[test]
902-
fn test_const_swap() {
905+
fn test_const_swap_ptr() {
906+
// The `swap` functions are implemented in the library, they are not primitives.
907+
// Only `swap_nonoverlapping` takes a count; pointers that cross multiple elements
908+
// are *not* supported.
909+
// We put the pointer at an odd offset in the type and copy them as an array of bytes,
910+
// which should catch most of the ways that the library implementation can get it wrong.
911+
912+
#[cfg(target_pointer_width = "32")]
913+
type HalfPtr = i16;
914+
#[cfg(target_pointer_width = "64")]
915+
type HalfPtr = i32;
916+
917+
#[repr(C, packed)]
918+
#[allow(unused)]
919+
struct S {
920+
f1: HalfPtr,
921+
// Crucially this field is at an offset that is not a multiple of the pointer size.
922+
ptr: &'static i32,
923+
// Make sure the entire type does not have a power-of-2 size:
924+
// make it 3 pointers in size. This used to hit a bug in `swap_nonoverlapping`.
925+
f2: [HalfPtr; 3],
926+
}
927+
928+
// Ensure the entire thing is usize-aligned, so in principle this
929+
// looks like it could be eligible for a `usize` copying loop.
930+
#[cfg_attr(target_pointer_width = "32", repr(align(4)))]
931+
#[cfg_attr(target_pointer_width = "64", repr(align(8)))]
932+
struct A(S);
933+
903934
const {
904-
let mut ptr1 = &1;
905-
let mut ptr2 = &666;
935+
let mut s1 = A(S { ptr: &1, f1: 0, f2: [0; 3] });
936+
let mut s2 = A(S { ptr: &666, f1: 0, f2: [0; 3] });
906937

907-
// Swap ptr1 and ptr2, bytewise. `swap` does not take a count
908-
// so the best we can do is use an array.
909-
type T = [u8; mem::size_of::<&i32>()];
938+
// Swap ptr1 and ptr2, as an array.
939+
type T = [u8; mem::size_of::<A>()];
910940
unsafe {
911-
ptr::swap(ptr::from_mut(&mut ptr1).cast::<T>(), ptr::from_mut(&mut ptr2).cast::<T>());
941+
ptr::swap(ptr::from_mut(&mut s1).cast::<T>(), ptr::from_mut(&mut s2).cast::<T>());
912942
}
913943

914944
// Make sure they still work.
915-
assert!(*ptr1 == 666);
916-
assert!(*ptr2 == 1);
945+
assert!(*s1.0.ptr == 666);
946+
assert!(*s2.0.ptr == 1);
947+
948+
// Swap them back, again as an array.
949+
unsafe {
950+
ptr::swap_nonoverlapping(
951+
ptr::from_mut(&mut s1).cast::<T>(),
952+
ptr::from_mut(&mut s2).cast::<T>(),
953+
1,
954+
);
955+
}
956+
957+
// Make sure they still work.
958+
assert!(*s1.0.ptr == 1);
959+
assert!(*s2.0.ptr == 666);
917960
};
918961
}
919962

0 commit comments

Comments
 (0)