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
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
88 changes: 66 additions & 22 deletions library/core/src/num/int_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down Expand Up @@ -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`,
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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) }
Expand Down Expand Up @@ -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
Expand All @@ -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) }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

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::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.
Expand Down
77 changes: 58 additions & 19 deletions library/core/src/num/uint_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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) }
Expand Down Expand Up @@ -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`
Expand All @@ -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) }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}

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;

Expand Down Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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
}
Expand All @@ -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
};
Expand All @@ -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
};
Expand Down
2 changes: 1 addition & 1 deletion library/core/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 5 additions & 2 deletions library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)) }
}
}

Expand Down
Loading
Loading