Skip to content

Commit

Permalink
Auto merge of #120863 - saethlin:slice-get-checked, r=the8472
Browse files Browse the repository at this point in the history 
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in #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: #120863 (comment)
  • Loading branch information
@bors
bors committed Feb 20, 2024
2 parents 29f87ad + 4a12f82 commit 2b43e75
Show file tree
Hide file tree
Showing 10 changed files with 67 additions and 367 deletions.
29 changes: 20 additions & 9 deletions library/core/src/panic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
},
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ptr/alignment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
24 changes: 14 additions & 10 deletions library/core/src/slice/index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ where
{
type Output = I::Output;

#[inline]
#[inline(always)]
fn index(&self, index: I) -> &I::Output {
index.index(self)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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) }
Expand Down Expand Up @@ -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()) }
}
Expand Down Expand Up @@ -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`,
Expand All @@ -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 {
Expand All @@ -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);
Expand Down Expand Up @@ -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)
}
Expand Down
10 changes: 5 additions & 5 deletions library/core/src/slice/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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) };
Expand Down Expand Up @@ -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) };
Expand Down Expand Up @@ -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()`
Expand Down Expand Up @@ -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()`.
Expand Down
4 changes: 2 additions & 2 deletions library/core/src/str/traits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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`.
Expand Down
78 changes: 1 addition & 77 deletions tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-abort.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
78 changes: 1 addition & 77 deletions tests/mir-opt/pre-codegen/slice_index.slice_get_mut_usize.PreCodegen.after.panic-unwind.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Loading

0 comments on commit 2b43e75

Please sign in to comment.