Auto merge of rust-lang#120863 - saethlin:slice-get-checked, r=the8472

Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)

Author: bors

library/core/src/panic.rs

@@ -139,27 +139,38 @@ pub macro unreachable_2021 {
     ),
 }
 
-/// Asserts that a boolean expression is `true`, and perform a non-unwinding panic otherwise.
+/// Like `assert_unsafe_precondition!` the defining features of this macro are that its
+/// checks are enabled when they are monomorphized with debug assertions enabled, and upon failure
+/// a non-unwinding panic is launched so that failures cannot compromise unwind safety.
 ///
-/// This macro is similar to `debug_assert!`, but is intended to be used in code that should not
-/// unwind. For example, checks in `_unchecked` functions that are intended for debugging but should
-/// not compromise unwind safety.
+/// But there are many differences from `assert_unsafe_precondition!`. This macro does not use
+/// `const_eval_select` internally, and therefore it is sound to call this with an expression
+/// that evaluates to `false`. Also unlike `assert_unsafe_precondition!` the condition being
+/// checked here is not put in an outlined function. If the check compiles to a lot of IR, this
+/// can cause code bloat if the check is monomorphized many times. But it also means that the checks
+/// from this macro can be deduplicated or otherwise optimized out.
+///
+/// In general, this macro should be used to check all public-facing preconditions. But some
+/// preconditions may be called too often or instantiated too often to make the overhead of the
+/// checks tolerable. In such cases, place `#[cfg(debug_assertions)]` on the macro call. That will
+/// disable the check in our precompiled standard library, but if a user wishes, they can still
+/// enable the check by recompiling the standard library with debug assertions enabled.
 #[doc(hidden)]
 #[unstable(feature = "panic_internals", issue = "none")]
-#[allow_internal_unstable(panic_internals, const_format_args)]
+#[allow_internal_unstable(panic_internals, delayed_debug_assertions)]
 #[rustc_macro_transparency = "semitransparent"]
 pub macro debug_assert_nounwind {
     ($cond:expr $(,)?) => {
-        if $crate::cfg!(debug_assertions) {
+        if $crate::intrinsics::debug_assertions() {
             if !$cond {
                 $crate::panicking::panic_nounwind($crate::concat!("assertion failed: ", $crate::stringify!($cond)));
             }
         }
     },
-    ($cond:expr, $($arg:tt)+) => {
-        if $crate::cfg!(debug_assertions) {
+    ($cond:expr, $message:expr) => {
+        if $crate::intrinsics::debug_assertions() {
             if !$cond {
-                $crate::panicking::panic_nounwind_fmt($crate::const_format_args!($($arg)+), false);
+                $crate::panicking::panic_nounwind($message);
             }
         }
     },

library/core/src/ptr/alignment.rs

@@ -76,6 +76,7 @@ impl Alignment {
     #[rustc_const_unstable(feature = "ptr_alignment_type", issue = "102070")]
     #[inline]
     pub const unsafe fn new_unchecked(align: usize) -> Self {
+        #[cfg(debug_assertions)]
         crate::panic::debug_assert_nounwind!(
             align.is_power_of_two(),
             "Alignment::new_unchecked requires a power of two"

library/core/src/slice/index.rs

@@ -13,7 +13,7 @@ where
 {
     type Output = I::Output;
 
-    #[inline]
+    #[inline(always)]
     fn index(&self, index: I) -> &I::Output {
         index.index(self)
     }
@@ -24,7 +24,7 @@ impl<T, I> ops::IndexMut<I> for [T]
 where
     I: SliceIndex<[T]>,
 {
-    #[inline]
+    #[inline(always)]
     fn index_mut(&mut self, index: I) -> &mut I::Output {
         index.index_mut(self)
     }
@@ -227,14 +227,16 @@ unsafe impl<T> SliceIndex<[T]> for usize {
     unsafe fn get_unchecked(self, slice: *const [T]) -> *const T {
         debug_assert_nounwind!(
             self < slice.len(),
-            "slice::get_unchecked requires that the index is within the slice",
+            "slice::get_unchecked requires that the index is within the slice"
         );
         // SAFETY: the caller guarantees that `slice` is not dangling, so it
         // cannot be longer than `isize::MAX`. They also guarantee that
         // `self` is in bounds of `slice` so `self` cannot overflow an `isize`,
         // so the call to `add` is safe.
         unsafe {
-            crate::hint::assert_unchecked(self < slice.len());
+            // Use intrinsics::assume instead of hint::assert_unchecked so that we don't check the
+            // precondition of this function twice.
+            crate::intrinsics::assume(self < slice.len());
             slice.as_ptr().add(self)
         }
     }
@@ -243,7 +245,7 @@ unsafe impl<T> SliceIndex<[T]> for usize {
     unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut T {
         debug_assert_nounwind!(
             self < slice.len(),
-            "slice::get_unchecked_mut requires that the index is within the slice",
+            "slice::get_unchecked_mut requires that the index is within the slice"
         );
         // SAFETY: see comments for `get_unchecked` above.
         unsafe { slice.as_mut_ptr().add(self) }
@@ -305,8 +307,9 @@ unsafe impl<T> SliceIndex<[T]> for ops::IndexRange {
     unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] {
         debug_assert_nounwind!(
             self.end() <= slice.len(),
-            "slice::get_unchecked_mut requires that the index is within the slice",
+            "slice::get_unchecked_mut requires that the index is within the slice"
         );
+
         // SAFETY: see comments for `get_unchecked` above.
         unsafe { ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start()), self.len()) }
     }
@@ -361,8 +364,9 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {
     unsafe fn get_unchecked(self, slice: *const [T]) -> *const [T] {
         debug_assert_nounwind!(
             self.end >= self.start && self.end <= slice.len(),
-            "slice::get_unchecked requires that the range is within the slice",
+            "slice::get_unchecked requires that the range is within the slice"
         );
+
         // SAFETY: the caller guarantees that `slice` is not dangling, so it
         // cannot be longer than `isize::MAX`. They also guarantee that
         // `self` is in bounds of `slice` so `self` cannot overflow an `isize`,
@@ -377,7 +381,7 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {
     unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] {
         debug_assert_nounwind!(
             self.end >= self.start && self.end <= slice.len(),
-            "slice::get_unchecked_mut requires that the range is within the slice",
+            "slice::get_unchecked_mut requires that the range is within the slice"
         );
         // SAFETY: see comments for `get_unchecked` above.
         unsafe {
@@ -386,7 +390,7 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {
         }
     }
 
-    #[inline]
+    #[inline(always)]
     fn index(self, slice: &[T]) -> &[T] {
         if self.start > self.end {
             slice_index_order_fail(self.start, self.end);
@@ -436,7 +440,7 @@ unsafe impl<T> SliceIndex<[T]> for ops::RangeTo<usize> {
         unsafe { (0..self.end).get_unchecked_mut(slice) }
     }
 
-    #[inline]
+    #[inline(always)]
     fn index(self, slice: &[T]) -> &[T] {
         (0..self.end).index(slice)
     }

library/core/src/slice/mod.rs

@@ -938,7 +938,7 @@ impl<T> [T] {
     pub const unsafe fn swap_unchecked(&mut self, a: usize, b: usize) {
         debug_assert_nounwind!(
             a < self.len() && b < self.len(),
-            "slice::swap_unchecked requires that the indices are within the slice",
+            "slice::swap_unchecked requires that the indices are within the slice"
         );
 
         let ptr = self.as_mut_ptr();
@@ -1278,7 +1278,7 @@ impl<T> [T] {
     pub const unsafe fn as_chunks_unchecked<const N: usize>(&self) -> &[[T; N]] {
         debug_assert_nounwind!(
             N != 0 && self.len() % N == 0,
-            "slice::as_chunks_unchecked requires `N != 0` and the slice to split exactly into `N`-element chunks",
+            "slice::as_chunks_unchecked requires `N != 0` and the slice to split exactly into `N`-element chunks"
         );
         // SAFETY: Caller must guarantee that `N` is nonzero and exactly divides the slice length
         let new_len = unsafe { exact_div(self.len(), N) };
@@ -1432,7 +1432,7 @@ impl<T> [T] {
     pub const unsafe fn as_chunks_unchecked_mut<const N: usize>(&mut self) -> &mut [[T; N]] {
         debug_assert_nounwind!(
             N != 0 && self.len() % N == 0,
-            "slice::as_chunks_unchecked requires `N != 0` and the slice to split exactly into `N`-element chunks",
+            "slice::as_chunks_unchecked requires `N != 0` and the slice to split exactly into `N`-element chunks"
         );
         // SAFETY: Caller must guarantee that `N` is nonzero and exactly divides the slice length
         let new_len = unsafe { exact_div(self.len(), N) };
@@ -1964,7 +1964,7 @@ impl<T> [T] {
 
         debug_assert_nounwind!(
             mid <= len,
-            "slice::split_at_unchecked requires the index to be within the slice",
+            "slice::split_at_unchecked requires the index to be within the slice"
         );
 
         // SAFETY: Caller has to check that `0 <= mid <= self.len()`
@@ -2014,7 +2014,7 @@ impl<T> [T] {
 
         debug_assert_nounwind!(
             mid <= len,
-            "slice::split_at_mut_unchecked requires the index to be within the slice",
+            "slice::split_at_mut_unchecked requires the index to be within the slice"
         );
 
         // SAFETY: Caller has to check that `0 <= mid <= self.len()`.

library/core/src/str/traits.rs

@@ -200,7 +200,7 @@ unsafe impl SliceIndex<str> for ops::Range<usize> {
             // `str::get_unchecked` without adding a special function
             // to `SliceIndex` just for this.
             self.end >= self.start && self.end <= slice.len(),
-            "str::get_unchecked requires that the range is within the string slice",
+            "str::get_unchecked requires that the range is within the string slice"
         );
 
         // SAFETY: the caller guarantees that `self` is in bounds of `slice`
@@ -215,7 +215,7 @@ unsafe impl SliceIndex<str> for ops::Range<usize> {
 
         debug_assert_nounwind!(
             self.end >= self.start && self.end <= slice.len(),
-            "str::get_unchecked_mut requires that the range is within the string slice",
+            "str::get_unchecked_mut requires that the range is within the string slice"
         );
 
         // SAFETY: see comments for `get_unchecked`.

tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-abort.mir

@@ -7,89 +7,13 @@ fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> {
     scope 1 (inlined core::slice::<impl [u32]>::get_mut::<usize>) {
         debug self => _1;
         debug index => _2;
-        scope 2 (inlined <usize as SliceIndex<[u32]>>::get_mut) {
-            debug self => _2;
-            debug slice => _1;
-            let mut _3: usize;
-            let mut _4: bool;
-            let mut _5: *mut [u32];
-            let mut _7: *mut u32;
-            let mut _8: &mut u32;
-            scope 3 {
-                scope 4 (inlined <usize as SliceIndex<[u32]>>::get_unchecked_mut) {
-                    debug self => _2;
-                    debug slice => _5;
-                    let mut _6: *mut u32;
-                    let mut _9: *mut [u32];
-                    let mut _10: &[&str];
-                    scope 5 {
-                        scope 10 (inlined std::ptr::mut_ptr::<impl *mut [u32]>::as_mut_ptr) {
-                            debug self => _5;
-                        }
-                        scope 11 (inlined std::ptr::mut_ptr::<impl *mut u32>::add) {
-                            debug self => _6;
-                            debug count => _2;
-                            scope 12 {
-                            }
-                        }
-                    }
-                    scope 6 (inlined std::ptr::mut_ptr::<impl *mut [u32]>::len) {
-                        debug self => _9;
-                        let mut _11: *const [u32];
-                        scope 7 (inlined std::ptr::metadata::<[u32]>) {
-                            debug ptr => _11;
-                            scope 8 {
-                            }
-                        }
-                    }
-                    scope 9 (inlined Arguments::<'_>::new_const) {
-                        debug pieces => _10;
-                    }
-                }
-            }
-        }
     }
 
     bb0: {
-        StorageLive(_7);
-        StorageLive(_4);
-        StorageLive(_3);
-        _3 = Len((*_1));
-        _4 = Lt(_2, move _3);
-        switchInt(move _4) -> [0: bb1, otherwise: bb2];
+        _0 = <usize as SliceIndex<[u32]>>::get_mut(move _2, move _1) -> [return: bb1, unwind unreachable];
     }
 
     bb1: {
-        StorageDead(_3);
-        _0 = const Option::<&mut u32>::None;
-        goto -> bb3;
-    }
-
-    bb2: {
-        StorageDead(_3);
-        StorageLive(_8);
-        StorageLive(_5);
-        _5 = &raw mut (*_1);
-        StorageLive(_9);
-        StorageLive(_10);
-        StorageLive(_11);
-        StorageLive(_6);
-        _6 = _5 as *mut u32 (PtrToPtr);
-        _7 = Offset(_6, _2);
-        StorageDead(_6);
-        StorageDead(_11);
-        StorageDead(_10);
-        StorageDead(_9);
-        StorageDead(_5);
-        _8 = &mut (*_7);
-        _0 = Option::<&mut u32>::Some(move _8);
-        StorageDead(_8);
-        goto -> bb3;
-    }
-
-    bb3: {
-        StorageDead(_4);
-        StorageDead(_7);
         return;
     }
 }

tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-unwind.mir

@@ -7,89 +7,13 @@ fn slice_get_mut_usize(_1: &mut [u32], _2: usize) -> Option<&mut u32> {
     scope 1 (inlined core::slice::<impl [u32]>::get_mut::<usize>) {
         debug self => _1;
         debug index => _2;
-        scope 2 (inlined <usize as SliceIndex<[u32]>>::get_mut) {
-            debug self => _2;
-            debug slice => _1;
-            let mut _3: usize;
-            let mut _4: bool;
-            let mut _5: *mut [u32];
-            let mut _7: *mut u32;
-            let mut _8: &mut u32;
-            scope 3 {
-                scope 4 (inlined <usize as SliceIndex<[u32]>>::get_unchecked_mut) {
-                    debug self => _2;
-                    debug slice => _5;
-                    let mut _6: *mut u32;
-                    let mut _9: *mut [u32];
-                    let mut _10: &[&str];
-                    scope 5 {
-                        scope 10 (inlined std::ptr::mut_ptr::<impl *mut [u32]>::as_mut_ptr) {
-                            debug self => _5;
-                        }
-                        scope 11 (inlined std::ptr::mut_ptr::<impl *mut u32>::add) {
-                            debug self => _6;
-                            debug count => _2;
-                            scope 12 {
-                            }
-                        }
-                    }
-                    scope 6 (inlined std::ptr::mut_ptr::<impl *mut [u32]>::len) {
-                        debug self => _9;
-                        let mut _11: *const [u32];
-                        scope 7 (inlined std::ptr::metadata::<[u32]>) {
-                            debug ptr => _11;
-                            scope 8 {
-                            }
-                        }
-                    }
-                    scope 9 (inlined Arguments::<'_>::new_const) {
-                        debug pieces => _10;
-                    }
-                }
-            }
-        }
     }
 
     bb0: {
-        StorageLive(_7);
-        StorageLive(_4);
-        StorageLive(_3);
-        _3 = Len((*_1));
-        _4 = Lt(_2, move _3);
-        switchInt(move _4) -> [0: bb1, otherwise: bb2];
+        _0 = <usize as SliceIndex<[u32]>>::get_mut(move _2, move _1) -> [return: bb1, unwind continue];
     }
 
     bb1: {
-        StorageDead(_3);
-        _0 = const Option::<&mut u32>::None;
-        goto -> bb3;
-    }
-
-    bb2: {
-        StorageDead(_3);
-        StorageLive(_8);
-        StorageLive(_5);
-        _5 = &raw mut (*_1);
-        StorageLive(_9);
-        StorageLive(_10);
-        StorageLive(_11);
-        StorageLive(_6);
-        _6 = _5 as *mut u32 (PtrToPtr);
-        _7 = Offset(_6, _2);
-        StorageDead(_6);
-        StorageDead(_11);
-        StorageDead(_10);
-        StorageDead(_9);
-        StorageDead(_5);
-        _8 = &mut (*_7);
-        _0 = Option::<&mut u32>::Some(move _8);
-        StorageDead(_8);
-        goto -> bb3;
-    }
-
-    bb3: {
-        StorageDead(_4);
-        StorageDead(_7);
         return;
     }
 }