From 61118ffd04aa6d1f9ee92daae4deb28bd975d4ab Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Wed, 7 Feb 2024 10:27:57 -0500 Subject: [PATCH] Rewrite assert_unsafe_precondition around the new intrinsic --- library/core/src/char/convert.rs | 2 +- library/core/src/hint.rs | 2 +- library/core/src/intrinsics.rs | 123 +++++++++++++++++++++--------- library/core/src/ptr/const_ptr.rs | 8 +- library/core/src/ptr/mod.rs | 45 ++++++++--- library/core/src/ptr/non_null.rs | 5 +- library/core/src/slice/raw.rs | 21 ++++- 7 files changed, 147 insertions(+), 59 deletions(-) diff --git a/library/core/src/char/convert.rs b/library/core/src/char/convert.rs index 177f39b59ae..7bd592492a5 100644 --- a/library/core/src/char/convert.rs +++ b/library/core/src/char/convert.rs @@ -27,7 +27,7 @@ pub(super) const unsafe fn from_u32_unchecked(i: u32) -> char { unsafe { assert_unsafe_precondition!( "invalid value for `char`", - (i: u32) => char_try_from_u32(i).is_ok() + (i: u32 = i) => char_try_from_u32(i).is_ok() ); transmute(i) } diff --git a/library/core/src/hint.rs b/library/core/src/hint.rs index 5c44ca69451..97c3c9e6fae 100644 --- a/library/core/src/hint.rs +++ b/library/core/src/hint.rs @@ -148,7 +148,7 @@ pub const unsafe fn assert_unchecked(cond: bool) { unsafe { intrinsics::assert_unsafe_precondition!( "hint::assert_unchecked must never be called when the condition is false", - (cond: bool) => cond, + (cond: bool = cond) => cond, ); crate::intrinsics::assume(cond); } diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 7ff9730254c..a6311e41633 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -56,7 +56,7 @@ use crate::marker::DiscriminantKind; use crate::marker::Tuple; -use crate::mem; +use crate::mem::{self, align_of}; pub mod mir; pub mod simd; @@ -2598,10 +2598,27 @@ pub const unsafe fn is_val_statically_known(_arg: T) -> bool { /// Check that the preconditions of an unsafe function are followed, if debug_assertions are on, /// and only at runtime. /// -/// This macro should be called as `assert_unsafe_precondition!([Generics](name: Type) => Expression)` -/// where the names specified will be moved into the macro as captured variables, and defines an item -/// to call `const_eval_select` on. The tokens inside the square brackets are used to denote generics -/// for the function declarations and can be omitted if there is no generics. +/// This macro should be called as +/// `assert_unsafe_precondition!((expr => name: Type, expr => name: Type) => Expression)` +/// where each `expr` will be evaluated and passed in as function argument `name: Type`. Then all +/// those arguments are passed to a function via [`const_eval_select`]. +/// +/// These checks are behind a condition which is evaluated at codegen time, not expansion time like +/// [`debug_assert`]. This means that a standard library built with optimizations and debug +/// assertions disabled will have these checks optimized out of its monomorphizations, but if a +/// a caller of the standard library has debug assertions enabled and monomorphizes an expansion of +/// this macro, that monomorphization will contain the check. +/// +/// Since these checks cannot be optimized out in MIR, some care must be taken in both call and +/// implementation to mitigate their compile-time overhead. The runtime function that we +/// [`const_eval_select`] to is monomorphic, `#[inline(never)]`, and `#[rustc_nounwind]`. That +/// combination of properties ensures that the code for the checks is only compiled once, and has a +/// minimal impact on the caller's code size. +/// +/// Caller should also introducing any other `let` bindings or any code outside this macro in order +/// to call it. Since the precompiled standard library is built with full debuginfo and these +/// variables cannot be optimized out in MIR, an innocent-looking `let` can produce enough +/// debuginfo to have a measurable compile-time impact on debug builds. /// /// # Safety /// @@ -2615,26 +2632,24 @@ pub const unsafe fn is_val_statically_known(_arg: T) -> bool { /// /// So in a sense it is UB if this macro is useful, but we expect callers of `unsafe fn` to make /// the occasional mistake, and this check should help them figure things out. -#[allow_internal_unstable(const_eval_select)] // permit this to be called in stably-const fn +#[allow_internal_unstable(const_eval_select, delayed_debug_assertions)] // permit this to be called in stably-const fn macro_rules! assert_unsafe_precondition { - ($name:expr, $([$($tt:tt)*])?($($i:ident:$ty:ty),*$(,)?) => $e:expr $(,)?) => { - if cfg!(debug_assertions) { - // allow non_snake_case to allow capturing const generics - #[allow(non_snake_case)] - #[inline(always)] - fn runtime$(<$($tt)*>)?($($i:$ty),*) { + ($message:expr, ($($name:ident:$ty:ty = $arg:expr),*$(,)?) => $e:expr $(,)?) => { + { + #[inline(never)] + #[rustc_nounwind] + fn precondition_check($($name:$ty),*) { if !$e { - // don't unwind to reduce impact on code size ::core::panicking::panic_nounwind( - concat!("unsafe precondition(s) violated: ", $name) + concat!("unsafe precondition(s) violated: ", $message) ); } } - #[allow(non_snake_case)] - #[inline] - const fn comptime$(<$($tt)*>)?($(_:$ty),*) {} + const fn comptime($(_:$ty),*) {} - ::core::intrinsics::const_eval_select(($($i,)*), comptime, runtime); + if ::core::intrinsics::debug_assertions() { + ::core::intrinsics::const_eval_select(($($arg,)*), comptime, precondition_check); + } } }; } @@ -2643,30 +2658,47 @@ pub(crate) use assert_unsafe_precondition; /// Checks whether `ptr` is properly aligned with respect to /// `align_of::()`. #[inline] -pub(crate) fn is_aligned_and_not_null(ptr: *const T) -> bool { - !ptr.is_null() && ptr.is_aligned() +pub(crate) fn is_aligned_and_not_null(ptr: *const (), align: usize) -> bool { + !ptr.is_null() && ptr.is_aligned_to(align) } -/// Checks whether an allocation of `len` instances of `T` exceeds -/// the maximum allowed allocation size. #[inline] -pub(crate) fn is_valid_allocation_size(len: usize) -> bool { - let max_len = const { - let size = crate::mem::size_of::(); - if size == 0 { usize::MAX } else { isize::MAX as usize / size } - }; +pub(crate) fn is_valid_allocation_size(size: usize, len: usize) -> bool { + let max_len = if size == 0 { usize::MAX } else { isize::MAX as usize / size }; len <= max_len } +pub(crate) fn is_nonoverlapping_mono( + src: *const (), + dst: *const (), + size: usize, + count: usize, +) -> bool { + let src_usize = src.addr(); + let dst_usize = dst.addr(); + let Some(size) = size.checked_mul(count) else { + crate::panicking::panic_nounwind( + "is_nonoverlapping: `size_of::() * count` overflows a usize", + ) + }; + let diff = src_usize.abs_diff(dst_usize); + // If the absolute distance between the ptrs is at least as big as the size of the buffer, + // they do not overlap. + diff >= size +} + /// Checks whether the regions of memory starting at `src` and `dst` of size /// `count * size_of::()` do *not* overlap. #[inline] pub(crate) fn is_nonoverlapping(src: *const T, dst: *const T, count: usize) -> bool { let src_usize = src.addr(); let dst_usize = dst.addr(); - let size = mem::size_of::() - .checked_mul(count) - .expect("is_nonoverlapping: `size_of::() * count` overflows a usize"); + let Some(size) = mem::size_of::().checked_mul(count) else { + // Use panic_nounwind instead of Option::expect, so that this function is nounwind. + crate::panicking::panic_nounwind( + "is_nonoverlapping: `size_of::() * count` overflows a usize", + ) + }; let diff = src_usize.abs_diff(dst_usize); // If the absolute distance between the ptrs is at least as big as the size of the buffer, // they do not overlap. @@ -2777,10 +2809,16 @@ pub const unsafe fn copy_nonoverlapping(src: *const T, dst: *mut T, count: us assert_unsafe_precondition!( "ptr::copy_nonoverlapping requires that both pointer arguments are aligned and non-null \ and the specified memory ranges do not overlap", - [T](src: *const T, dst: *mut T, count: usize) => - is_aligned_and_not_null(src) - && is_aligned_and_not_null(dst) - && is_nonoverlapping(src, dst, count) + ( + src: *const () = src as *const (), + dst: *mut () = dst as *mut (), + size: usize = size_of::(), + align: usize = align_of::(), + count: usize = count, + ) => + is_aligned_and_not_null(src, align) + && is_aligned_and_not_null(dst, align) + && is_nonoverlapping_mono(src, dst, size, count) ); copy_nonoverlapping(src, dst, count) } @@ -2870,9 +2908,15 @@ pub const unsafe fn copy(src: *const T, dst: *mut T, count: usize) { // SAFETY: the safety contract for `copy` must be upheld by the caller. unsafe { assert_unsafe_precondition!( - "ptr::copy requires that both pointer arguments are aligned and non-null", - [T](src: *const T, dst: *mut T) => - is_aligned_and_not_null(src) && is_aligned_and_not_null(dst) + "ptr::copy_nonoverlapping requires that both pointer arguments are aligned and non-null \ + and the specified memory ranges do not overlap", + ( + src: *const () = src as *const (), + dst: *mut () = dst as *mut (), + align: usize = align_of::(), + ) => + is_aligned_and_not_null(src, align) + && is_aligned_and_not_null(dst, align) ); copy(src, dst, count) } @@ -2945,7 +2989,10 @@ pub const unsafe fn write_bytes(dst: *mut T, val: u8, count: usize) { unsafe { assert_unsafe_precondition!( "ptr::write_bytes requires that the destination pointer is aligned and non-null", - [T](dst: *mut T) => is_aligned_and_not_null(dst) + ( + addr: *const () = dst as *const (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); write_bytes(dst, val, count) } diff --git a/library/core/src/ptr/const_ptr.rs b/library/core/src/ptr/const_ptr.rs index 1d5d683fa16..c5e3df07a1c 100644 --- a/library/core/src/ptr/const_ptr.rs +++ b/library/core/src/ptr/const_ptr.rs @@ -806,13 +806,15 @@ impl *const T { where T: Sized, { - let this = self; // SAFETY: The comparison has no side-effects, and the intrinsic // does this check internally in the CTFE implementation. unsafe { assert_unsafe_precondition!( - "ptr::sub_ptr requires `this >= origin`", - [T](this: *const T, origin: *const T) => this >= origin + "ptr::sub_ptr requires `self >= origin`", + ( + this: *const () = self as *const (), + origin: *const () = origin as *const (), + ) => this >= origin ) }; diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index bc05b5b07de..0fb9017e6d9 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -381,11 +381,11 @@ use crate::cmp::Ordering; use crate::fmt; use crate::hash; use crate::intrinsics::{ - self, assert_unsafe_precondition, is_aligned_and_not_null, is_nonoverlapping, + self, assert_unsafe_precondition, is_aligned_and_not_null, is_nonoverlapping_mono, }; use crate::marker::FnPtr; -use crate::mem::{self, MaybeUninit}; +use crate::mem::{self, align_of, size_of, MaybeUninit}; mod alignment; #[unstable(feature = "ptr_alignment_type", issue = "102070")] @@ -967,10 +967,16 @@ pub const unsafe fn swap_nonoverlapping(x: *mut T, y: *mut T, count: usize) { assert_unsafe_precondition!( "ptr::swap_nonoverlapping requires that both pointer arguments are aligned and non-null \ and the specified memory ranges do not overlap", - [T](x: *mut T, y: *mut T, count: usize) => - is_aligned_and_not_null(x) - && is_aligned_and_not_null(y) - && is_nonoverlapping(x, y, count) + ( + x: *mut () = x as *mut (), + y: *mut () = y as *mut (), + size: usize = size_of::(), + align: usize = align_of::(), + count: usize = count, + ) => + is_aligned_and_not_null(x, align) + && is_aligned_and_not_null(y, align) + && is_nonoverlapping_mono(x, y, size, count) ); } @@ -1061,7 +1067,10 @@ pub const unsafe fn replace(dst: *mut T, mut src: T) -> T { unsafe { assert_unsafe_precondition!( "ptr::replace requires that the pointer argument is aligned and non-null", - [T](dst: *mut T) => is_aligned_and_not_null(dst) + ( + addr: *const () = dst as *const (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); mem::swap(&mut *dst, &mut src); // cannot overlap } @@ -1207,9 +1216,13 @@ pub const unsafe fn read(src: *const T) -> T { // SAFETY: the caller must guarantee that `src` is valid for reads. unsafe { + #[cfg(debug_assertions)] // Too expensive to always enable (for now?) assert_unsafe_precondition!( "ptr::read requires that the pointer argument is aligned and non-null", - [T](src: *const T) => is_aligned_and_not_null(src) + ( + addr: *const () = src as *const (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); crate::intrinsics::read_via_copy(src) } @@ -1411,9 +1424,13 @@ pub const unsafe fn write(dst: *mut T, src: T) { // `dst` cannot overlap `src` because the caller has mutable access // to `dst` while `src` is owned by this function. unsafe { + #[cfg(debug_assertions)] // Too expensive to always enable (for now?) assert_unsafe_precondition!( "ptr::write requires that the pointer argument is aligned and non-null", - [T](dst: *mut T) => is_aligned_and_not_null(dst) + ( + addr: *mut () = dst as *mut (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); intrinsics::write_via_move(dst, src) } @@ -1581,7 +1598,10 @@ pub unsafe fn read_volatile(src: *const T) -> T { unsafe { assert_unsafe_precondition!( "ptr::read_volatile requires that the pointer argument is aligned and non-null", - [T](src: *const T) => is_aligned_and_not_null(src) + ( + addr: *const () = src as *const (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); intrinsics::volatile_load(src) } @@ -1656,7 +1676,10 @@ pub unsafe fn write_volatile(dst: *mut T, src: T) { unsafe { assert_unsafe_precondition!( "ptr::write_volatile requires that the pointer argument is aligned and non-null", - [T](dst: *mut T) => is_aligned_and_not_null(dst) + ( + addr: *mut () = dst as *mut (), + align: usize = align_of::(), + ) => is_aligned_and_not_null(addr, align) ); intrinsics::volatile_store(dst, src); } diff --git a/library/core/src/ptr/non_null.rs b/library/core/src/ptr/non_null.rs index d18082c3048..d6266ba8da5 100644 --- a/library/core/src/ptr/non_null.rs +++ b/library/core/src/ptr/non_null.rs @@ -218,7 +218,10 @@ impl NonNull { pub const unsafe fn new_unchecked(ptr: *mut T) -> Self { // SAFETY: the caller must guarantee that `ptr` is non-null. unsafe { - assert_unsafe_precondition!("NonNull::new_unchecked requires that the pointer is non-null", [T: ?Sized](ptr: *mut T) => !ptr.is_null()); + assert_unsafe_precondition!( + "NonNull::new_unchecked requires that the pointer is non-null", + (ptr: *mut () = ptr as *mut ()) => !ptr.is_null() + ); NonNull { pointer: ptr as _ } } } diff --git a/library/core/src/slice/raw.rs b/library/core/src/slice/raw.rs index 9cdf9b68afb..571abc3e999 100644 --- a/library/core/src/slice/raw.rs +++ b/library/core/src/slice/raw.rs @@ -4,6 +4,7 @@ use crate::array; use crate::intrinsics::{ assert_unsafe_precondition, is_aligned_and_not_null, is_valid_allocation_size, }; +use crate::mem::{align_of, size_of}; use crate::ops::Range; use crate::ptr; @@ -96,8 +97,14 @@ pub const unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] unsafe { assert_unsafe_precondition!( "slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`", - [T](data: *const T, len: usize) => is_aligned_and_not_null(data) - && is_valid_allocation_size::(len) + ( + data: *mut () = data as *mut (), + size: usize = size_of::(), + align: usize = align_of::(), + len: usize = len, + ) => + is_aligned_and_not_null(data, align) + && is_valid_allocation_size(size, len) ); &*ptr::slice_from_raw_parts(data, len) } @@ -143,8 +150,14 @@ pub const unsafe fn from_raw_parts_mut<'a, T>(data: *mut T, len: usize) -> &'a m unsafe { assert_unsafe_precondition!( "slice::from_raw_parts_mut requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`", - [T](data: *mut T, len: usize) => is_aligned_and_not_null(data) - && is_valid_allocation_size::(len) + ( + data: *mut () = data as *mut (), + size: usize = size_of::(), + align: usize = align_of::(), + len: usize = len, + ) => + is_aligned_and_not_null(data, align) + && is_valid_allocation_size(size, len) ); &mut *ptr::slice_from_raw_parts_mut(data, len) }