Skip to content

Commit

Permalink
Auto merge of rust-lang#121571 - clarfonthey:unchecked-math-precondit…
Browse files Browse the repository at this point in the history
…ions, r=saethlin

Add assert_unsafe_precondition to unchecked_{add,sub,neg,mul,shl,shr} methods

(Old PR is haunted, opening a new one. See rust-lang#117494 for previous discussion.)

This ensures that these preconditions are actually checked in debug mode, and hopefully should let people know if they messed up. I've also replaced the calls (I could find) in the code that use these intrinsics directly with those that use these methods, so that the asserts actually apply.

More discussions on people misusing these methods in the tracking issue: rust-lang#85122.
  • Loading branch information
bors committed May 25, 2024
2 parents 21e6de7 + 18cb2fa commit 402fc6b
Show file tree
Hide file tree
Showing 24 changed files with 291 additions and 199 deletions.
1 change: 1 addition & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@
#![feature(str_split_remainder)]
#![feature(strict_provenance)]
#![feature(ub_checks)]
#![feature(unchecked_neg)]
#![feature(unchecked_shifts)]
#![feature(utf16_extra)]
#![feature(utf16_extra_const)]
Expand Down
99 changes: 81 additions & 18 deletions library/core/src/num/int_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,19 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_add(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_add`.
unsafe { intrinsics::unchecked_add(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_add(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_add(self, rhs)
}
}

/// Checked addition with an unsigned integer. Computes `self + rhs`,
Expand Down Expand Up @@ -630,9 +640,19 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_sub(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_sub`.
unsafe { intrinsics::unchecked_sub(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_sub(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_sub(self, rhs)
}
}

/// Checked subtraction with an unsigned integer. Computes `self - rhs`,
Expand Down Expand Up @@ -772,9 +792,19 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_mul(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_mul`.
unsafe { intrinsics::unchecked_mul(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_mul(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_mul(self, rhs)
}
}

/// Checked integer division. Computes `self / rhs`, returning `None` if `rhs == 0`
Expand Down Expand Up @@ -1111,9 +1141,22 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_neg(self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_neg`.
unsafe { intrinsics::unchecked_sub(0, self) }
// ICE resolved by #125184 isn't in bootstrap compiler
#[cfg(not(bootstrap))]
{
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_neg cannot overflow"),
(
lhs: $SelfT = self,
) => !lhs.overflowing_neg().1,
);
}

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_sub(0, self)
}
}

/// Strict negation. Computes `-self`, panicking if `self == MIN`.
Expand Down Expand Up @@ -1234,9 +1277,19 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_shl(self, rhs: u32) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_shl`.
unsafe { intrinsics::unchecked_shl(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
(
rhs: u32 = rhs,
bits: u32 = Self::BITS,
) => rhs < bits,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_shl(self, rhs)
}
}

/// Checked shift right. Computes `self >> rhs`, returning `None` if `rhs` is
Expand Down Expand Up @@ -1323,9 +1376,19 @@ macro_rules! int_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_shr(self, rhs: u32) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_shr`.
unsafe { intrinsics::unchecked_shr(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
(
rhs: u32 = rhs,
bits: u32 = Self::BITS,
) => rhs < bits,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_shr(self, rhs)
}
}

/// Checked absolute value. Computes `self.abs()`, returning `None` if
Expand Down
1 change: 1 addition & 0 deletions library/core/src/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::hint;
use crate::intrinsics;
use crate::mem;
use crate::str::FromStr;
use crate::ub_checks::assert_unsafe_precondition;

// Used because the `?` operator is not allowed in a const context.
macro_rules! try_opt {
Expand Down
80 changes: 65 additions & 15 deletions library/core/src/num/uint_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,9 +495,19 @@ macro_rules! uint_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_add(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_add`.
unsafe { intrinsics::unchecked_add(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_add(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_add(self, rhs)
}
}

/// Checked addition with a signed integer. Computes `self + rhs`,
Expand Down Expand Up @@ -677,9 +687,19 @@ macro_rules! uint_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_sub(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_sub`.
unsafe { intrinsics::unchecked_sub(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_sub(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_sub(self, rhs)
}
}

/// Checked integer multiplication. Computes `self * rhs`, returning
Expand Down Expand Up @@ -763,9 +783,19 @@ macro_rules! uint_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_mul(self, rhs: Self) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_mul`.
unsafe { intrinsics::unchecked_mul(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
(
lhs: $SelfT = self,
rhs: $SelfT = rhs,
) => !lhs.overflowing_mul(rhs).1,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_mul(self, rhs)
}
}

/// Checked integer division. Computes `self / rhs`, returning `None`
Expand Down Expand Up @@ -1334,9 +1364,19 @@ macro_rules! uint_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_shl(self, rhs: u32) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_shl`.
unsafe { intrinsics::unchecked_shl(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
(
rhs: u32 = rhs,
bits: u32 = Self::BITS,
) => rhs < bits,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_shl(self, rhs)
}
}

/// Checked shift right. Computes `self >> rhs`, returning `None`
Expand Down Expand Up @@ -1423,9 +1463,19 @@ macro_rules! uint_impl {
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn unchecked_shr(self, rhs: u32) -> Self {
// SAFETY: the caller must uphold the safety contract for
// `unchecked_shr`.
unsafe { intrinsics::unchecked_shr(self, rhs) }
assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
(
rhs: u32 = rhs,
bits: u32 = Self::BITS,
) => rhs < bits,
);

// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
intrinsics::unchecked_shr(self, rhs)
}
}

/// Checked exponentiation. Computes `self.pow(exp)`, returning `None` if
Expand Down
11 changes: 5 additions & 6 deletions library/core/src/ops/index_range.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::intrinsics::{unchecked_add, unchecked_sub};
use crate::iter::{FusedIterator, TrustedLen};
use crate::num::NonZero;
use crate::ub_checks;
Expand Down Expand Up @@ -46,7 +45,7 @@ impl IndexRange {
#[inline]
pub const fn len(&self) -> usize {
// SAFETY: By invariant, this cannot wrap
unsafe { unchecked_sub(self.end, self.start) }
unsafe { self.end.unchecked_sub(self.start) }
}

/// # Safety
Expand All @@ -57,7 +56,7 @@ impl IndexRange {

let value = self.start;
// SAFETY: The range isn't empty, so this cannot overflow
self.start = unsafe { unchecked_add(value, 1) };
self.start = unsafe { value.unchecked_add(1) };
value
}

Expand All @@ -68,7 +67,7 @@ impl IndexRange {
debug_assert!(self.start < self.end);

// SAFETY: The range isn't empty, so this cannot overflow
let value = unsafe { unchecked_sub(self.end, 1) };
let value = unsafe { self.end.unchecked_sub(1) };
self.end = value;
value
}
Expand All @@ -83,7 +82,7 @@ impl IndexRange {
let mid = if n <= self.len() {
// SAFETY: We just checked that this will be between start and end,
// and thus the addition cannot overflow.
unsafe { unchecked_add(self.start, n) }
unsafe { self.start.unchecked_add(n) }
} else {
self.end
};
Expand All @@ -102,7 +101,7 @@ impl IndexRange {
let mid = if n <= self.len() {
// SAFETY: We just checked that this will be between start and end,
// and thus the addition cannot overflow.
unsafe { unchecked_sub(self.end, n) }
unsafe { self.end.unchecked_sub(n) }
} else {
self.start
};
Expand Down
3 changes: 2 additions & 1 deletion library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,7 @@ impl<T: ?Sized> *const T {
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[must_use = "returns a new pointer rather than modifying its argument"]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_allow_const_fn_unstable(unchecked_neg)]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn sub(self, count: usize) -> Self
Expand All @@ -1093,7 +1094,7 @@ impl<T: ?Sized> *const T {
// SAFETY: the caller must uphold the safety contract for `offset`.
// Because the pointee is *not* a ZST, that means that `count` is
// at most `isize::MAX`, and thus the negation cannot overflow.
unsafe { self.offset(intrinsics::unchecked_sub(0, count as isize)) }
unsafe { self.offset((count as isize).unchecked_neg()) }
}
}

Expand Down
3 changes: 2 additions & 1 deletion library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1224,6 +1224,7 @@ impl<T: ?Sized> *mut T {
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[must_use = "returns a new pointer rather than modifying its argument"]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_allow_const_fn_unstable(unchecked_neg)]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn sub(self, count: usize) -> Self
Expand All @@ -1237,7 +1238,7 @@ impl<T: ?Sized> *mut T {
// SAFETY: the caller must uphold the safety contract for `offset`.
// Because the pointee is *not* a ZST, that means that `count` is
// at most `isize::MAX`, and thus the negation cannot overflow.
unsafe { self.offset(intrinsics::unchecked_sub(0, count as isize)) }
unsafe { self.offset((count as isize).unchecked_neg()) }
}
}

Expand Down
3 changes: 2 additions & 1 deletion library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,7 @@ impl<T: ?Sized> NonNull<T> {
#[must_use = "returns a new pointer rather than modifying its argument"]
#[stable(feature = "non_null_convenience", since = "CURRENT_RUSTC_VERSION")]
#[rustc_const_stable(feature = "non_null_convenience", since = "CURRENT_RUSTC_VERSION")]
#[rustc_allow_const_fn_unstable(unchecked_neg)]
pub const unsafe fn sub(self, count: usize) -> Self
where
T: Sized,
Expand All @@ -712,7 +713,7 @@ impl<T: ?Sized> NonNull<T> {
// SAFETY: the caller must uphold the safety contract for `offset`.
// Because the pointee is *not* a ZST, that means that `count` is
// at most `isize::MAX`, and thus the negation cannot overflow.
unsafe { self.offset(intrinsics::unchecked_sub(0, count as isize)) }
unsafe { self.offset((count as isize).unchecked_neg()) }
}
}

Expand Down
5 changes: 2 additions & 3 deletions library/core/src/slice/index.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! Indexing implementations for `[T]`.

use crate::intrinsics::const_eval_select;
use crate::intrinsics::unchecked_sub;
use crate::ops;
use crate::ptr;
use crate::ub_checks::assert_unsafe_precondition;
Expand Down Expand Up @@ -374,7 +373,7 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {
// `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 {
let new_len = unchecked_sub(self.end, self.start);
let new_len = self.end.unchecked_sub(self.start);
ptr::slice_from_raw_parts(slice.as_ptr().add(self.start), new_len)
}
}
Expand All @@ -392,7 +391,7 @@ unsafe impl<T> SliceIndex<[T]> for ops::Range<usize> {
);
// SAFETY: see comments for `get_unchecked` above.
unsafe {
let new_len = unchecked_sub(self.end, self.start);
let new_len = self.end.unchecked_sub(self.start);
ptr::slice_from_raw_parts_mut(slice.as_mut_ptr().add(self.start), new_len)
}
}
Expand Down
Loading

0 comments on commit 402fc6b

Please sign in to comment.