rename debug_assert_nounwind → debug_assert_ubcheck

[RalfJung]

This captures its changed meaning since #120863 a bit better.

However, the naming of debug_assert_ubcheck vs assert_unsafe_precondition is still unclear. Both are guarded by intrinsics::debug_assertions, the main difference is whether the check needs to be const-compatible and whether it is being outlined. So maybe assert_unsafe_precondition should be renamed to debug_assert_ubcheck_outline? Is the outlining even desirable? If we can make is_aligned_and_not_null and is_nonoverlapping const-compatible (e.g. by using const_eval_select inside them and making them always return falsetrue), then we could use debug_assert_ubcheck everywhere.

r? @saethlin

[Noratrieb]

Now it's not necessarily outlined, it can be inlined by LLVM (and really, I see no reason for these two macros to have different behavior in that regard). So now the distinctive feature is whether it's executed by CTFE (which, even with the const_eval_select semantics nonsense sorted out, is probably still useful for CTFE performance). I'm not sure what to best name that though.

[RalfJung]

the distinctive feature is whether it's executed by CTFE

Both are in an if ::core::intrinsics::debug_assertions(), so neither of them is executed by CTFE. But debug_assert_ubcheck uses regular control flow so while the check is dead code in CTFE, it is still being const-checked.

[saethlin]

If we can make is_aligned_and_not_null and is_nonoverlapping const-compatible (e.g. by using const_eval_select inside them and making them always return false), then we could use debug_assert_ubcheck everywhere.

That would mean such checks would always fail in const eval, so doesn't that introduce an implicit reliance on not actually using those functions in const eval? I'd prefer to reduce the amount of manual coordination required in here if possible, and I think this proposal would increase it, right?

---

I'm happy to approve a rename, because the current name is Quite Bad. I should have renamed this when I changed the implementation.

I've been thinking about what the right design is but I'm not sure of a solution yet so you're just getting my mental notes. Maybe you want to do something with them, if not I'll do something later.

I think the structure with #[inline] + #[rustc_no_mir_inline] is quite good, and I don't see any reason to not use it. It is very close to optimal for minimizing code size in unoptimized builds and permitting full optimization when optimizations are enabled.

The odd syntax used by assert_unsafe_precondition is my primary complaint about it, but I don't think that's really avoidable. It creates an increasing amount of work if we ever change the macro's patterns, but if some library maintenance effort is buying improved compile times for all our users I'm happy to do the work. The maintenance cost arises from both clearing the fn precondition_check and being able to construct the tuple of arguments without creating more debuginfo.

There are two variants of checks here; we have checks that should be enabled in CTFE/Miri and at runtime, and checks which should only be enabled at runtime. Adding a second intrinsic for this would duplicate a lot of work elsewhere in the compiler. Maybe we can just add a boolean argument to the intrinsic? Or a two-variant enum?

Then one version of the checks is:

if intrinsics::debug_asertions(Always) {
    precondition_check($($arg,)*);

and the other is

if intrinsics::debug_assertions(OnlyRuntime) {
    const_eval_select(($(arg,)*), comptime, precondition_check);
}

[RalfJung]

That would mean such checks would always fail in const eval, so doesn't that introduce an implicit reliance on not actually using those functions in const eval? I'd prefer to reduce the amount of manual coordination required in here if possible, and I think this proposal would increase it, right?

Eh, sorry, I meant always return true -- effectively skipping the test, but one abstraction layer down.

---

I think the structure with #[inline] + #[rustc_no_mir_inline] is quite good, and I don't see any reason to not use it. It is very close to optimal for minimizing code size in unoptimized builds and permitting full optimization when optimizations are enabled.

So that's assert_unsafe_precondition!? And the idea is to then basically ditch the other one?

There are two variants of checks here; we have checks that should be enabled in CTFE/Miri and at runtime, and checks which should only be enabled at runtime. Adding a second intrinsic for this would duplicate a lot of work elsewhere in the compiler. Maybe we can just add a boolean argument to the intrinsic? Or a two-variant enum?

Yes that makes sense. To propose a different color for our bikeshed though, I'd name the enum after the goal, not the mechanism, otherwise people are not going to know what they are supposed to do. So e.g., LanguageUb (checked only in binaries) vs LibraryUb (also checked in the interpreter).

That also makes me wonder whether intrinsics::debug_assertions (or maybe it should be intrinsics::ub_checks then) should be always true for LibraryUb in Miri. It's a UB detection tool after all...

[saethlin]

So that's assert_unsafe_precondition!? And the idea is to then basically ditch the other one?

Yes.

[RalfJung]

Okay in that case this PR is moot I guess, since the macro it renames is going to disappear. I'm happy to close it and let you take care of unifying the two macros. :)

[RalfJung]

Closing in favor of #121662.