Skip to content

Use intrinsics::debug_assertions in debug_assert_nounwind #120863

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

Merged
merged 2 commits into from
Feb 20, 2024
Merged
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
29 changes: 20 additions & 9 deletions library/core/src/panic.rs
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() {
Copy link
Member

@RalfJung RalfJung Feb 25, 2024

Choose a reason for hiding this comment

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

I think this violates the contract for intrinsics::debug_assertions. We said this should only be used if there is anyway UB (meaning: language UB) when the condition is violated, but debug_assert_nounwind is used in places where there is only library UB.

EDIT: #121583 adds some FIXMEs.

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ where
{
type Output = I::Output;

#[inline]
#[inline(always)]
Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to apply this to fix the test added in #119878, I don't know if this deserves a comment? And if it does, I'm not sure if I should cite the test specifically; that seems rather fragile if the test is renamed.

Copy link
Member

Choose a reason for hiding this comment

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

That PR did have the always too so it must have been removed in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

That PR added an always to Result::unwrap, this one is in the Index impl for slices. The assertion in this PR blows up the size of the MIR for slice indexing, so I think LLVM needs the always boost. But this function is also a trivial wrapper, so it's reasonably defensible in any case.

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
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
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
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;
}
}
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