Skip to content
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

Add debug_assert_nounwind and convert assert_unsafe_precondition #110303

Merged
merged 5 commits into from
Nov 26, 2023
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
8 changes: 4 additions & 4 deletions library/core/src/num/nonzero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ macro_rules! nonzero_integers {
#[must_use]
#[inline]
pub const unsafe fn new_unchecked(n: $Int) -> Self {
crate::panic::debug_assert_nounwind!(
n != 0,
concat!(stringify!($Ty), "::new_unchecked requires a non-zero argument")
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
core::intrinsics::assert_unsafe_precondition!(
concat!(stringify!($Ty), "::new_unchecked requires a non-zero argument"),
(n: $Int) => n != 0
);
Self(n)
}
}
Expand Down
13 changes: 5 additions & 8 deletions library/core/src/ops/index_range.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::intrinsics::{assert_unsafe_precondition, unchecked_add, unchecked_sub};
use crate::intrinsics::{unchecked_add, unchecked_sub};
use crate::iter::{FusedIterator, TrustedLen};
use crate::num::NonZeroUsize;

Expand All @@ -19,13 +19,10 @@ impl IndexRange {
/// - `start <= end`
#[inline]
pub const unsafe fn new_unchecked(start: usize, end: usize) -> Self {
// SAFETY: comparisons on usize are pure
unsafe {
assert_unsafe_precondition!(
"IndexRange::new_unchecked requires `start <= end`",
(start: usize, end: usize) => start <= end
)
};
crate::panic::debug_assert_nounwind!(
start <= end,
"IndexRange::new_unchecked requires `start <= end`"
);
IndexRange { start, end }
}

Expand Down
26 changes: 26 additions & 0 deletions library/core/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,32 @@ pub macro unreachable_2021 {
),
}

/// Asserts that a boolean expression is `true`, and perform a non-unwinding panic otherwise.
///
/// 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.
#[doc(hidden)]
#[unstable(feature = "core_panic", issue = "none")]
#[allow_internal_unstable(core_panic, const_format_args)]
#[rustc_macro_transparency = "semitransparent"]
pub macro debug_assert_nounwind {
Copy link
Member

@saethlin saethlin Apr 14, 2023

Choose a reason for hiding this comment

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

This macro should have documentation that explains that it should be used in unsafe contexts because it cannot compromise unwind safety. (and that debug_assert! shouldn't be because it can)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually no unwind safety is compromised and debug_assert! can be used in these contexts, because if unwinding would only happen when there's an UB, and unwinding happening at undesirable place is an allowed behaviour for UB.

That said, causing UB by unwinding certainly defeats the purpose of having these assertions for debugging.

($cond:expr $(,)?) => {
if $crate::cfg!(debug_assertions) {
if !$cond {
$crate::panicking::panic_nounwind($crate::concat!("assertion failed: ", $crate::stringify!($cond)));
}
}
},
($cond:expr, $($arg:tt)+) => {
if $crate::cfg!(debug_assertions) {
if !$cond {
$crate::panicking::panic_nounwind_fmt($crate::const_format_args!($($arg)+), false);
}
}
},
}

/// An internal trait used by std to pass data from std to `panic_unwind` and
/// other panic runtimes. Not intended to be stabilized any time soon, do not
/// use.
Expand Down
54 changes: 35 additions & 19 deletions library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,28 +82,43 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
// and unwinds anyway, we will hit the "unwinding out of nounwind function" guard,
// which causes a "panic in a function that cannot unwind".
#[rustc_nounwind]
pub fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}
#[rustc_const_unstable(feature = "core_panic", issue = "none")]
pub const fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
#[track_caller]
fn runtime(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably have at least #[cfg_attr(feature = "panic_immediate_abort", inline)], like almost all the functions in this file.

Copy link
Member

Choose a reason for hiding this comment

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

Done in #118362

if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}

// NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call
// that gets resolved to the `#[panic_handler]` function.
extern "Rust" {
#[lang = "panic_impl"]
fn panic_impl(pi: &PanicInfo<'_>) -> !;
// NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call
// that gets resolved to the `#[panic_handler]` function.
extern "Rust" {
#[lang = "panic_impl"]
fn panic_impl(pi: &PanicInfo<'_>) -> !;
}

// PanicInfo with the `can_unwind` flag set to false forces an abort.
let pi = PanicInfo::internal_constructor(
Some(&fmt),
Location::caller(),
/* can_unwind */ false,
force_no_backtrace,
);

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
}

// PanicInfo with the `can_unwind` flag set to false forces an abort.
let pi = PanicInfo::internal_constructor(
Some(&fmt),
Location::caller(),
/* can_unwind */ false,
force_no_backtrace,
);
#[inline]
#[track_caller]
const fn comptime(fmt: fmt::Arguments<'_>, _force_no_backtrace: bool) -> ! {
panic_fmt(fmt);
}

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
// SAFETY: const panic does not care about unwinding
unsafe {
super::intrinsics::const_eval_select((fmt, force_no_backtrace), comptime, runtime);
}
}

// Next we define a bunch of higher-level wrappers that all bottom out in the two core functions
Expand Down Expand Up @@ -132,7 +147,8 @@ pub const fn panic(expr: &'static str) -> ! {
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[lang = "panic_nounwind"] // needed by codegen for non-unwinding panics
#[rustc_nounwind]
pub fn panic_nounwind(expr: &'static str) -> ! {
#[rustc_const_unstable(feature = "core_panic", issue = "none")]
pub const fn panic_nounwind(expr: &'static str) -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

What is the impact of adding #[track_caller] here? That would improve the const panic.

Copy link
Member

Choose a reason for hiding this comment

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

That will require codegen backend changes to add the implicit caller location argument when it is manually called (as opposed through a Call MIR terminator).

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine? Debug assertions are expected to have overhead, sometimes a lot of overhead. If people are concerned about debug assertions, they should turn them off. Or we should factor these assertions out to a separate flag. Either option is preferable to sacrificing UX for those who can afford the overhead, which as far as I can tell is most people?

Copy link
Member

@RalfJung RalfJung Nov 27, 2023

Choose a reason for hiding this comment

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

panic_nounwind is not just for debug assertions. panic_nounwind is used in situations where it is important to keep the code size small, and having track_caller would be contrary to that goal.

The comment above the function (helpfully hidden by github) explains this:

/// Like `panic`, but without unwinding and track_caller to reduce the impact on codesize.

Copy link
Member

Choose a reason for hiding this comment

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

Callers that do not care about code size can just call panic_nounwind_fmt(format_args!("my message")) instead, then they will get track_caller.

panic_nounwind_fmt(fmt::Arguments::new_const(&[expr]), /* force_no_backtrace */ false);
}

Expand Down
12 changes: 4 additions & 8 deletions library/core/src/ptr/alignment.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::convert::{TryFrom, TryInto};
use crate::intrinsics::assert_unsafe_precondition;
use crate::num::NonZeroUsize;
use crate::{cmp, fmt, hash, mem, num};

Expand Down Expand Up @@ -77,13 +76,10 @@ impl Alignment {
#[rustc_const_unstable(feature = "ptr_alignment_type", issue = "102070")]
#[inline]
pub const unsafe fn new_unchecked(align: usize) -> Self {
// SAFETY: Precondition passed to the caller.
unsafe {
assert_unsafe_precondition!(
"Alignment::new_unchecked requires a power of two",
(align: usize) => align.is_power_of_two()
)
};
crate::panic::debug_assert_nounwind!(
align.is_power_of_two(),
"Alignment::new_unchecked requires a power of two"
);

// SAFETY: By precondition, this must be a power of two, and
// our variants encompass all possible powers of two.
Expand Down
75 changes: 29 additions & 46 deletions library/core/src/slice/index.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
//! Indexing implementations for `[T]`.

use crate::intrinsics::assert_unsafe_precondition;
use crate::intrinsics::const_eval_select;
use crate::intrinsics::unchecked_sub;
use crate::ops;
use crate::panic::debug_assert_nounwind;
use crate::ptr;

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -225,31 +225,25 @@ unsafe impl<T> SliceIndex<[T]> for usize {

#[inline]
unsafe fn get_unchecked(self, slice: *const [T]) -> *const T {
let this = self;
debug_assert_nounwind!(
self < slice.len(),
"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 {
assert_unsafe_precondition!(
"slice::get_unchecked requires that the index is within the slice",
[T](this: usize, slice: *const [T]) => this < slice.len()
);
slice.as_ptr().add(self)
}
unsafe { slice.as_ptr().add(self) }
}

#[inline]
unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut T {
let this = self;
debug_assert_nounwind!(
self < slice.len(),
"slice::get_unchecked_mut requires that the index is within the slice",
);
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!(
"slice::get_unchecked_mut requires that the index is within the slice",
[T](this: usize, slice: *mut [T]) => this < slice.len()
);
slice.as_mut_ptr().add(self)
}
unsafe { slice.as_mut_ptr().add(self) }
}

#[inline]
Expand Down Expand Up @@ -293,32 +287,25 @@ unsafe impl<T> SliceIndex<[T]> for ops::IndexRange {

#[inline]
unsafe fn get_unchecked(self, slice: *const [T]) -> *const [T] {
let end = self.end();
debug_assert_nounwind!(
self.end() <= slice.len(),
"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 {
assert_unsafe_precondition!(
"slice::get_unchecked requires that the index is within the slice",
[T](end: usize, slice: *const [T]) => end <= slice.len()
);
ptr::slice_from_raw_parts(slice.as_ptr().add(self.start()), self.len())
}
unsafe { ptr::slice_from_raw_parts(slice.as_ptr().add(self.start()), self.len()) }
}

#[inline]
unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] {
let end = self.end();
debug_assert_nounwind!(
self.end() <= slice.len(),
"slice::get_unchecked_mut requires that the index is within the slice",
);
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!(
"slice::get_unchecked_mut requires that the index is within the slice",
[T](end: usize, slice: *mut [T]) => end <= slice.len()
);
ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start()), self.len())
}
unsafe { ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start()), self.len()) }
}

#[inline]
Expand Down Expand Up @@ -369,32 +356,28 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {

#[inline]
unsafe fn get_unchecked(self, slice: *const [T]) -> *const [T] {
let this = ops::Range { ..self };
debug_assert_nounwind!(
self.end >= self.start && self.end <= slice.len(),
"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`,
// so the call to `add` is safe and the length calculation cannot overflow.
unsafe {
assert_unsafe_precondition!(
"slice::get_unchecked requires that the range is within the slice",
[T](this: ops::Range<usize>, slice: *const [T]) =>
this.end >= this.start && this.end <= slice.len()
);
let new_len = unchecked_sub(self.end, self.start);
ptr::slice_from_raw_parts(slice.as_ptr().add(self.start), new_len)
}
}

#[inline]
unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] {
let this = ops::Range { ..self };
debug_assert_nounwind!(
self.end >= self.start && self.end <= slice.len(),
"slice::get_unchecked_mut requires that the range is within the slice",
);
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!(
"slice::get_unchecked_mut requires that the range is within the slice",
[T](this: ops::Range<usize>, slice: *mut [T]) =>
this.end >= this.start && this.end <= slice.len()
);
let new_len = unchecked_sub(self.end, self.start);
ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start), new_len)
}
Expand Down
Loading