Skip to content

Commit

Permalink
Rollup merge of #103035 - saethlin:assert_unsafe_precondition3, r=thomcc
Browse files Browse the repository at this point in the history
Even nicer errors from assert_unsafe_precondition

For example, now running `cargo test` with this patch I get things like:
```
$ cargo +stage1 test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running unittests src/lib.rs (target/debug/deps/malloc_buf-9d105ddf86862995)

running 5 tests
thread 'tests::test_null_buf' panicked at 'unsafe precondition violated: is_aligned_and_not_null(data) &&
    crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize', /home/ben/rust/library/core/src/slice/raw.rs:93:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread panicked while panicking. aborting.
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/tmp/malloc_buf-1.0.0/target/debug/deps/malloc_buf-9d105ddf86862995` (signal: 6, SIGABRT: process abort signal)
```

This is still not perfect, but these are better for another PR:
* `stringify!` is trying to do clever pretty-printing on the `expr` inside `assert_unsafe_precondition` and can even add a newline.
* It would be nice to print a bit more information about where the problem is. Perhaps this is `cfg_attr(debug_assertions, track_caller)`, or perhaps it the function name added to `Location`.

cc ``@RalfJung`` this is what I was thinking of for #102732 (comment)
  • Loading branch information
matthiaskrgr committed Oct 27, 2022
2 parents 44fcfb0 + 458aaa5 commit 2937621
Show file tree
Hide file tree
Showing 12 changed files with 138 additions and 36 deletions.
2 changes: 1 addition & 1 deletion library/core/src/hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub const unsafe fn unreachable_unchecked() -> ! {
// SAFETY: the safety contract for `intrinsics::unreachable` must
// be upheld by the caller.
unsafe {
intrinsics::assert_unsafe_precondition!(() => false);
intrinsics::assert_unsafe_precondition!("hint::unreachable_unchecked must never be reached", () => false);
intrinsics::unreachable()
}
}
Expand Down
23 changes: 17 additions & 6 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2203,15 +2203,17 @@ extern "rust-intrinsic" {
/// 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
macro_rules! assert_unsafe_precondition {
($([$($tt:tt)*])?($($i:ident:$ty:ty),*$(,)?) => $e:expr) => {
($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),*) {
if !$e {
// don't unwind to reduce impact on code size
::core::panicking::panic_str_nounwind("unsafe precondition violated");
::core::panicking::panic_str_nounwind(
concat!("unsafe precondition(s) violated: ", $name)
);
}
}
#[allow(non_snake_case)]
Expand Down Expand Up @@ -2350,7 +2352,10 @@ pub const unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: us
// SAFETY: the safety contract for `copy_nonoverlapping` must be
// upheld by the caller.
unsafe {
assert_unsafe_precondition!([T](src: *const T, dst: *mut T, count: usize) =>
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)
Expand Down Expand Up @@ -2436,8 +2441,11 @@ pub const unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {

// SAFETY: the safety contract for `copy` must be upheld by the caller.
unsafe {
assert_unsafe_precondition!([T](src: *const T, dst: *mut T) =>
is_aligned_and_not_null(src) && is_aligned_and_not_null(dst));
assert_unsafe_precondition!(
"ptr::copy requires that both pointer arguments are aligned aligned and non-null",
[T](src: *const T, dst: *mut T) =>
is_aligned_and_not_null(src) && is_aligned_and_not_null(dst)
);
copy(src, dst, count)
}
}
Expand Down Expand Up @@ -2505,7 +2513,10 @@ pub const unsafe fn write_bytes<T>(dst: *mut T, val: u8, count: usize) {

// SAFETY: the safety contract for `write_bytes` must be upheld by the caller.
unsafe {
assert_unsafe_precondition!([T](dst: *mut T) => is_aligned_and_not_null(dst));
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)
);
write_bytes(dst, val, count)
}
}
5 changes: 4 additions & 1 deletion library/core/src/num/nonzero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ macro_rules! nonzero_integers {
pub const unsafe fn new_unchecked(n: $Int) -> Self {
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
core::intrinsics::assert_unsafe_precondition!((n: $Int) => n != 0);
core::intrinsics::assert_unsafe_precondition!(
concat!(stringify!($Ty), "::new_unchecked requires a non-zero argument"),
(n: $Int) => n != 0
);
Self(n)
}
}
Expand Down
7 changes: 6 additions & 1 deletion library/core/src/ops/index_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ impl IndexRange {
#[inline]
pub const unsafe fn new_unchecked(start: usize, end: usize) -> Self {
// SAFETY: comparisons on usize are pure
unsafe { assert_unsafe_precondition!((start: usize, end: usize) => start <= end) };
unsafe {
assert_unsafe_precondition!(
"IndexRange::new_unchecked requires `start <= end`",
(start: usize, end: usize) => start <= end
)
};
IndexRange { start, end }
}

Expand Down
7 changes: 6 additions & 1 deletion library/core/src/ptr/alignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,12 @@ impl Alignment {
#[inline]
pub const unsafe fn new_unchecked(align: usize) -> Self {
// SAFETY: Precondition passed to the caller.
unsafe { assert_unsafe_precondition!((align: usize) => align.is_power_of_two()) };
unsafe {
assert_unsafe_precondition!(
"Alignment::new_unchecked requires a power of two",
(align: usize) => align.is_power_of_two()
)
};

// SAFETY: By precondition, this must be a power of two, and
// our variants encompass all possible powers of two.
Expand Down
5 changes: 4 additions & 1 deletion library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,10 @@ impl<T: ?Sized> *const T {
// SAFETY: The comparison has no side-effects, and the intrinsic
// does this check internally in the CTFE implementation.
unsafe {
assert_unsafe_precondition!([T](this: *const T, origin: *const T) => this >= origin)
assert_unsafe_precondition!(
"ptr::sub_ptr requires `this >= origin`",
[T](this: *const T, origin: *const T) => this >= origin
)
};

let pointee_size = mem::size_of::<T>();
Expand Down
30 changes: 24 additions & 6 deletions library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,10 @@ pub const unsafe fn swap_nonoverlapping<T>(x: *mut T, y: *mut T, count: usize) {
// SAFETY: the caller must guarantee that `x` and `y` are
// valid for writes and properly aligned.
unsafe {
assert_unsafe_precondition!([T](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)
Expand Down Expand Up @@ -986,7 +989,10 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
// and cannot overlap `src` since `dst` must point to a distinct
// allocated object.
unsafe {
assert_unsafe_precondition!([T](dst: *mut T) => is_aligned_and_not_null(dst));
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)
);
mem::swap(&mut *dst, &mut src); // cannot overlap
}
src
Expand Down Expand Up @@ -1117,7 +1123,10 @@ pub const unsafe fn read<T>(src: *const T) -> T {
// Also, since we just wrote a valid value into `tmp`, it is guaranteed
// to be properly initialized.
unsafe {
assert_unsafe_precondition!([T](src: *const T) => is_aligned_and_not_null(src));
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)
);
copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
tmp.assume_init()
}
Expand Down Expand Up @@ -1311,7 +1320,10 @@ pub const unsafe fn write<T>(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 {
assert_unsafe_precondition!([T](dst: *mut T) => is_aligned_and_not_null(dst));
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)
);
copy_nonoverlapping(&src as *const T, dst, 1);
intrinsics::forget(src);
}
Expand Down Expand Up @@ -1475,7 +1487,10 @@ pub const unsafe fn write_unaligned<T>(dst: *mut T, src: T) {
pub unsafe fn read_volatile<T>(src: *const T) -> T {
// SAFETY: the caller must uphold the safety contract for `volatile_load`.
unsafe {
assert_unsafe_precondition!([T](src: *const T) => is_aligned_and_not_null(src));
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)
);
intrinsics::volatile_load(src)
}
}
Expand Down Expand Up @@ -1546,7 +1561,10 @@ pub unsafe fn read_volatile<T>(src: *const T) -> T {
pub unsafe fn write_volatile<T>(dst: *mut T, src: T) {
// SAFETY: the caller must uphold the safety contract for `volatile_store`.
unsafe {
assert_unsafe_precondition!([T](dst: *mut T) => is_aligned_and_not_null(dst));
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)
);
intrinsics::volatile_store(dst, src);
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ impl<T: ?Sized> NonNull<T> {
pub const unsafe fn new_unchecked(ptr: *mut T) -> Self {
// SAFETY: the caller must guarantee that `ptr` is non-null.
unsafe {
assert_unsafe_precondition!([T: ?Sized](ptr: *mut T) => !ptr.is_null());
assert_unsafe_precondition!("NonNull::new_unchecked requires that the pointer is non-null", [T: ?Sized](ptr: *mut T) => !ptr.is_null());
NonNull { pointer: ptr as _ }
}
}
Expand Down
36 changes: 26 additions & 10 deletions library/core/src/slice/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,10 @@ unsafe impl<T> const SliceIndex<[T]> for usize {
// `self` is in bounds of `slice` so `self` cannot overflow an `isize`,
// so the call to `add` is safe.
unsafe {
assert_unsafe_precondition!([T](this: usize, slice: *const [T]) => this < slice.len());
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)
}
}
Expand All @@ -242,7 +245,10 @@ unsafe impl<T> const SliceIndex<[T]> for usize {
let this = self;
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!([T](this: usize, slice: *mut [T]) => this < slice.len());
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)
}
}
Expand Down Expand Up @@ -295,8 +301,10 @@ unsafe impl<T> const SliceIndex<[T]> for ops::IndexRange {
// so the call to `add` is safe.

unsafe {
assert_unsafe_precondition!([T](end: usize, slice: *const [T]) =>
end <= slice.len());
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())
}
}
Expand All @@ -306,8 +314,10 @@ unsafe impl<T> const SliceIndex<[T]> for ops::IndexRange {
let end = self.end();
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!([T](end: usize, slice: *mut [T]) =>
end <= slice.len());
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())
}
}
Expand Down Expand Up @@ -367,8 +377,11 @@ unsafe impl<T> const SliceIndex<[T]> for ops::Range<usize> {
// so the call to `add` is safe.

unsafe {
assert_unsafe_precondition!([T](this: ops::Range<usize>, slice: *const [T]) =>
this.end >= this.start && this.end <= slice.len());
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()
);
ptr::slice_from_raw_parts(slice.as_ptr().add(self.start), self.end - self.start)
}
}
Expand All @@ -378,8 +391,11 @@ unsafe impl<T> const SliceIndex<[T]> for ops::Range<usize> {
let this = ops::Range { start: self.start, end: self.end };
// SAFETY: see comments for `get_unchecked` above.
unsafe {
assert_unsafe_precondition!([T](this: ops::Range<usize>, slice: *mut [T]) =>
this.end >= this.start && this.end <= slice.len());
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()
);
ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start), self.end - self.start)
}
}
Expand Down
20 changes: 16 additions & 4 deletions library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,10 @@ impl<T> [T] {
let ptr = this.as_mut_ptr();
// SAFETY: caller has to guarantee that `a < self.len()` and `b < self.len()`
unsafe {
assert_unsafe_precondition!([T](a: usize, b: usize, this: &mut [T]) => a < this.len() && b < this.len());
assert_unsafe_precondition!(
"slice::swap_unchecked requires that the indices are within the slice",
[T](a: usize, b: usize, this: &mut [T]) => a < this.len() && b < this.len()
);
ptr::swap(ptr.add(a), ptr.add(b));
}
}
Expand Down Expand Up @@ -969,7 +972,10 @@ impl<T> [T] {
let this = self;
// SAFETY: Caller must guarantee that `N` is nonzero and exactly divides the slice length
let new_len = unsafe {
assert_unsafe_precondition!([T](this: &[T], N: usize) => N != 0 && this.len() % N == 0);
assert_unsafe_precondition!(
"slice::as_chunks_unchecked requires `N != 0` and the slice to split exactly into `N`-element chunks",
[T](this: &[T], N: usize) => N != 0 && this.len() % N == 0
);
exact_div(self.len(), N)
};
// SAFETY: We cast a slice of `new_len * N` elements into
Expand Down Expand Up @@ -1109,7 +1115,10 @@ impl<T> [T] {
let this = &*self;
// SAFETY: Caller must guarantee that `N` is nonzero and exactly divides the slice length
let new_len = unsafe {
assert_unsafe_precondition!([T](this: &[T], N: usize) => N != 0 && this.len() % N == 0);
assert_unsafe_precondition!(
"slice::as_chunks_unchecked_mut requires `N != 0` and the slice to split exactly into `N`-element chunks",
[T](this: &[T], N: usize) => N != 0 && this.len() % N == 0
);
exact_div(this.len(), N)
};
// SAFETY: We cast a slice of `new_len * N` elements into
Expand Down Expand Up @@ -1685,7 +1694,10 @@ impl<T> [T] {
// `[ptr; mid]` and `[mid; len]` are not overlapping, so returning a mutable reference
// is fine.
unsafe {
assert_unsafe_precondition!((mid: usize, len: usize) => mid <= len);
assert_unsafe_precondition!(
"slice::split_at_mut_unchecked requires the index to be within the slice",
(mid: usize, len: usize) => mid <= len
);
(from_raw_parts_mut(ptr, mid), from_raw_parts_mut(ptr.add(mid), len - mid))
}
}
Expand Down
12 changes: 8 additions & 4 deletions library/core/src/slice/raw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ use crate::ptr;
pub const unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] {
// SAFETY: the caller must uphold the safety contract for `from_raw_parts`.
unsafe {
assert_unsafe_precondition!([T](data: *const T, len: usize) =>
is_aligned_and_not_null(data) && is_valid_allocation_size::<T>(len)
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::<T>(len)
);
&*ptr::slice_from_raw_parts(data, len)
}
Expand Down Expand Up @@ -135,8 +137,10 @@ pub const unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T]
pub const unsafe fn from_raw_parts_mut<'a, T>(data: *mut T, len: usize) -> &'a mut [T] {
// SAFETY: the caller must uphold the safety contract for `from_raw_parts_mut`.
unsafe {
assert_unsafe_precondition!([T](data: *mut T, len: usize) =>
is_aligned_and_not_null(data) && is_valid_allocation_size::<T>(len)
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::<T>(len)
);
&mut *ptr::slice_from_raw_parts_mut(data, len)
}
Expand Down
25 changes: 25 additions & 0 deletions library/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#![feature(is_terminal)]
#![feature(staged_api)]
#![feature(process_exitcode_internals)]
#![feature(panic_can_unwind)]
#![feature(test)]

// Public reexports
Expand Down Expand Up @@ -54,6 +55,7 @@ use std::{
collections::VecDeque,
env, io,
io::prelude::Write,
mem::ManuallyDrop,
panic::{self, catch_unwind, AssertUnwindSafe, PanicInfo},
process::{self, Command, Termination},
sync::mpsc::{channel, Sender},
Expand Down Expand Up @@ -112,6 +114,29 @@ pub fn test_main(args: &[String], tests: Vec<TestDescAndFn>, options: Option<Opt
process::exit(ERROR_EXIT_CODE);
}
} else {
if !opts.nocapture {
// If we encounter a non-unwinding panic, flush any captured output from the current test,
// and stop capturing output to ensure that the non-unwinding panic message is visible.
// We also acquire the locks for both output streams to prevent output from other threads
// from interleaving with the panic message or appearing after it.
let builtin_panic_hook = panic::take_hook();
let hook = Box::new({
move |info: &'_ PanicInfo<'_>| {
if !info.can_unwind() {
std::mem::forget(std::io::stderr().lock());
let mut stdout = ManuallyDrop::new(std::io::stdout().lock());
if let Some(captured) = io::set_output_capture(None) {
if let Ok(data) = captured.lock() {
let _ = stdout.write_all(&data);
let _ = stdout.flush();
}
}
}
builtin_panic_hook(info);
}
});
panic::set_hook(hook);
}
match console::run_tests_console(&opts, tests) {
Ok(true) => {}
Ok(false) => process::exit(ERROR_EXIT_CODE),
Expand Down

0 comments on commit 2937621

Please sign in to comment.