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 to unchecked_{add,sub,neg,mul,shl,shr} methods (haunted PR) #117494

Closed
Closed
Changes from 1 commit
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
88 changes: 66 additions & 22 deletions library/core/src/num/int_macros.rs
Original file line number Diff line number Diff line change
@@ -510,9 +510,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_add(rhs).1,
concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let lhs = self;
Copy link
Member

Choose a reason for hiding this comment

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

This extra let is going to cause more LLVM IR to be generated in order to have debuginfo for it. I learned about this unfortunate behavior only recently:

/// Callers should also avoid introducing any other `let` bindings or any code outside this macro in
/// order to call it. Since the precompiled standard library is built with full debuginfo and these
/// variables cannot be optimized out in MIR, an innocent-looking `let` can produce enough
/// debuginfo to have a measurable compile-time impact on debug builds.

I wish I had a way to tell rustc that we don't need variable-level debuginfo for a span of code, but here we are.

Copy link
Contributor Author

@clarfonthey clarfonthey Feb 24, 2024

Choose a reason for hiding this comment

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

Ugh, I didn't realise it got all the way to the LLVM IR level. It's unfortunate, but I'll remove the extra lets from these operations. (Will wait until current perf run finishes before doing this.)

intrinsics::unchecked_add(lhs, rhs)
}
}

/// Checked addition with an unsigned integer. Computes `self + rhs`,
@@ -648,9 +654,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_sub(rhs).1,
concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let lhs = self;
intrinsics::unchecked_sub(lhs, rhs)
}
}

/// Checked subtraction with an unsigned integer. Computes `self - rhs`,
@@ -786,9 +798,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_mul(rhs).1,
concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let lhs = self;
intrinsics::unchecked_mul(lhs, rhs)
}
}

/// Checked integer division. Computes `self / rhs`, returning `None` if `rhs == 0`
@@ -1125,9 +1143,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_neg().1,
concat!(stringify!($SelfT), "::unchecked_neg cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let n = self;
intrinsics::unchecked_sub(0, n)
}
}

/// Strict negation. Computes `-self`, panicking if `self == MIN`.
@@ -1179,7 +1203,7 @@ macro_rules! int_impl {
#[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
#[inline(always)]
Copy link
Member

Choose a reason for hiding this comment

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

FYI, functions which are inline(always) are inlined even in unoptimized builds. rustc takes extra steps to make this happen, and I disagree very strongly with that behavior, but I'm bringing this up because this may explain some surprising impacts of this PR on debug build times. Don't know yet.

My PR to stop this behavior of inline(always): #121417 (comment)

Copy link
Contributor Author

@clarfonthey clarfonthey Feb 24, 2024

Choose a reason for hiding this comment

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

So, I initially added this after the stuff I mentioned here, but I did just leave it based upon assumptions that are probably wrong, so, I can remove it. Note that this does show up for the other unchecked operations and that shift is the only one marking them without always.

pub const fn checked_shl(self, rhs: u32) -> Option<Self> {
let (a, b) = self.overflowing_shl(rhs);
if unlikely!(b) { None } else { Some(a) }
@@ -1241,10 +1265,17 @@ 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`.
debug_assert_nounwind!(
rhs < Self::BITS,
concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
// Any legal shift amount is losslessly representable in the self type.
unsafe { intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
unsafe {
let lhs = self;
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs);
intrinsics::unchecked_shl(lhs, rhs)
}
}

/// Checked shift right. Computes `self >> rhs`, returning `None` if `rhs` is
@@ -1262,7 +1293,7 @@ macro_rules! int_impl {
#[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
#[inline(always)]
pub const fn checked_shr(self, rhs: u32) -> Option<Self> {
let (a, b) = self.overflowing_shr(rhs);
if unlikely!(b) { None } else { Some(a) }
@@ -1324,10 +1355,17 @@ 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`.
debug_assert_nounwind!(
rhs < Self::BITS,
concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
// Any legal shift amount is losslessly representable in the self type.
unsafe { intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
unsafe {
let lhs = self;
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs);
intrinsics::unchecked_shr(lhs, rhs)
}
}

/// Checked absolute value. Computes `self.abs()`, returning `None` if
@@ -1991,7 +2029,10 @@ macro_rules! int_impl {
// SAFETY: the masking by the bitsize of the type ensures that we do not shift
// out of bounds
unsafe {
self.unchecked_shl(rhs & (Self::BITS - 1))
// FIXME: we can't optimize out the extra check here,
// so, we can't just call the method for now
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1));
intrinsics::unchecked_shl(self, rhs)
}
}

@@ -2021,7 +2062,10 @@ macro_rules! int_impl {
// SAFETY: the masking by the bitsize of the type ensures that we do not shift
// out of bounds
unsafe {
self.unchecked_shr(rhs & (Self::BITS - 1))
// FIXME: we can't optimize out the extra check here,
// so, we can't just call the method for now
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1));
intrinsics::unchecked_shr(self, rhs)
}
}

1 change: 1 addition & 0 deletions library/core/src/num/mod.rs
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ use crate::hint;
use crate::intrinsics;
use crate::mem;
use crate::ops::{Add, Mul, Sub};
use crate::panic::debug_assert_nounwind;
use crate::str::FromStr;

// Used because the `?` operator is not allowed in a const context.
77 changes: 58 additions & 19 deletions library/core/src/num/uint_macros.rs
Original file line number Diff line number Diff line change
@@ -518,9 +518,16 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_add(rhs).1,
concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
);

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

/// Checked addition with a signed integer. Computes `self + rhs`,
@@ -662,9 +669,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_sub(rhs).1,
concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let lhs = self;
intrinsics::unchecked_sub(lhs, rhs)
}
}

/// Checked integer multiplication. Computes `self * rhs`, returning
@@ -744,9 +757,15 @@ 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) }
debug_assert_nounwind!(
!self.overflowing_mul(rhs).1,
concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
unsafe {
let lhs = self;
intrinsics::unchecked_mul(lhs, rhs)
}
}

/// Checked integer division. Computes `self / rhs`, returning `None`
@@ -1239,7 +1258,7 @@ macro_rules! uint_impl {
#[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
#[inline(always)]
pub const fn checked_shl(self, rhs: u32) -> Option<Self> {
let (a, b) = self.overflowing_shl(rhs);
if unlikely!(b) { None } else { Some(a) }
@@ -1301,10 +1320,17 @@ 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`.
debug_assert_nounwind!(
rhs < Self::BITS,
concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
// Any legal shift amount is losslessly representable in the self type.
unsafe { intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
unsafe {
let lhs = self;
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs);
intrinsics::unchecked_shl(lhs, rhs)
}
}

/// Checked shift right. Computes `self >> rhs`, returning `None`
@@ -1322,7 +1348,7 @@ macro_rules! uint_impl {
#[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
#[must_use = "this returns the result of the operation, \
without modifying the original"]
#[inline]
#[inline(always)]
pub const fn checked_shr(self, rhs: u32) -> Option<Self> {
let (a, b) = self.overflowing_shr(rhs);
if unlikely!(b) { None } else { Some(a) }
@@ -1384,10 +1410,17 @@ 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`.
debug_assert_nounwind!(
rhs < Self::BITS,
concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
);
// SAFETY: this is guaranteed to be safe by the caller.
// Any legal shift amount is losslessly representable in the self type.
unsafe { intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
unsafe {
let lhs = self;
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs);
intrinsics::unchecked_shr(lhs, rhs)
}
}

/// Checked exponentiation. Computes `self.pow(exp)`, returning `None` if
@@ -1878,7 +1911,10 @@ macro_rules! uint_impl {
// SAFETY: the masking by the bitsize of the type ensures that we do not shift
// out of bounds
unsafe {
self.unchecked_shl(rhs & (Self::BITS - 1))
// FIXME: we can't optimize out the extra check here,
// so, we can't just call the method for now
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1));
intrinsics::unchecked_shl(self, rhs)
}
}

@@ -1911,7 +1947,10 @@ macro_rules! uint_impl {
// SAFETY: the masking by the bitsize of the type ensures that we do not shift
// out of bounds
unsafe {
self.unchecked_shr(rhs & (Self::BITS - 1))
// FIXME: we can't optimize out the extra check here,
// so, we can't just call the method for now
let rhs = conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1));
intrinsics::unchecked_shr(self, rhs)
}
}

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;

@@ -44,7 +43,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
@@ -55,7 +54,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
}

@@ -66,7 +65,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
}
@@ -81,7 +80,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
};
@@ -100,7 +99,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
};
2 changes: 1 addition & 1 deletion library/core/src/panic.rs
Original file line number Diff line number Diff line change
@@ -167,7 +167,7 @@ pub macro debug_assert_nounwind {
}
}
},
($cond:expr, $message:expr) => {
($cond:expr, $message:expr $(,)?) => {
if $crate::intrinsics::debug_assertions() {
if !$cond {
$crate::panicking::panic_nounwind($message);
7 changes: 5 additions & 2 deletions library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
@@ -1021,8 +1021,8 @@ impl<T: ?Sized> *const T {
#[must_use = "returns a new pointer rather than modifying its argument"]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
// We could always go back to wrapping if unchecked becomes unacceptable
#[rustc_allow_const_fn_unstable(const_int_unchecked_arith)]
#[inline(always)]
#[rustc_allow_const_fn_unstable(unchecked_math)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub const unsafe fn sub(self, count: usize) -> Self
where
@@ -1035,7 +1035,10 @@ 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)) }
// FIXME: replacing unchecked_sub with unchecked_neg and replacing the
// unchecked_math flag with unchecked_neg will anger the UnstableInStable lint
// and I cannot for the life of me understand why
unsafe { self.offset(0isize.unchecked_sub(count as isize)) }
}
}

Loading