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.

Sign up for GitHub

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

Always inline check in assert_unsafe_precondition with cfg(debug_assertions) #121196

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Always inline check in assert_unsafe_precondition with cfg(debug_assertions) #121196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion library/core/src/intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2575,6 +2575,7 @@ pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
/// assertions disabled. This intrinsic is primarily used by [`assert_unsafe_precondition`].
#[rustc_const_unstable(feature = "delayed_debug_assertions", issue = "none")]
#[unstable(feature = "core_intrinsics", issue = "none")]
#[inline(always)]
#[cfg_attr(not(bootstrap), rustc_intrinsic)]
pub(crate) const fn debug_assertions() -> bool {
cfg!(debug_assertions)
Expand Down Expand Up @@ -2659,7 +2660,13 @@ pub const unsafe fn const_deallocate(_ptr: *mut u8, _size: usize, _align: usize)
macro_rules! assert_unsafe_precondition {
($message:expr, ($($name:ident:$ty:ty = $arg:expr),*$(,)?) => $e:expr $(,)?) => {
{
#[inline(never)]
// When the standard library is compiled with debug assertions, we want the check to inline for better performance.
// This is important when working on the compiler, which is compiled with debug assertions locally.
// When not compiled with debug assertions (so the precompiled std) we outline the check to minimize the compile
// time impact when debug assertions are disabled.
// It is not clear whether that is the best solution, see #120848.
#[cfg_attr(debug_assertions, inline(always))]
#[cfg_attr(not(debug_assertions), inline(never))]
Copy link
Member

@RalfJung RalfJung Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This checks cfg(debug_assertions) which is different from intrinsics::debug_assertions which is used to determine whether the function is even called. Shouldn't the comment explain why that mismatch is okay? It is certainly not obvious to me.

Copy link
Member Author

@Noratrieb Noratrieb Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why wouldn't the difference be okay? Sure, the behavior isn't exactly the same, but users who enable that will get more checks in case they don't enable debug_assertions downstream but that seems okay. We should be able to remove this again after #121114 anyways, I mostly PRed this to avoid thinking in #121114, but I have done the thinking now.

Copy link
Member

@RalfJung RalfJung Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why wouldn't the difference be okay?

There's a funny case to consider where debug assertions are disabled but intrinsics::debug_assertions returns true. (There's theoretically also the dual case.) I would have expected the comment to acknowledge this and explain why what happens in that case is what we want.

We should be able to remove this again after #121114 anyways

Ah, if this is temporary then never mind. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a funny case to consider where debug assertions are disabled but intrinsics::debug_assertions returns true.

when building core, that cannot happen. When building user code, yes, that's actually the expected and normal path, core without debug assertions, but debug assertions in the user case.

#[rustc_nounwind]
fn precondition_check($($name:$ty),*) {
if !$e {
Expand Down
Loading