Re-enable debug checks in copy[_nonoverlapping]

[WaffleLapkin]

#79684 removed debug_assert! from intrinsic::copy[_nonoverlapping] to make it const:

rust/library/core/src/intrinsics.rs

Lines 2045 to 2049 in 5dab47d

	// FIXME: Perform these checks only at run time
	/*if cfg!(debug_assertions)
	&& !(is_aligned_and_not_null(src)
	&& is_aligned_and_not_null(dst)
	&& is_nonoverlapping(src, dst, count))

rust/library/core/src/intrinsics.rs

Lines 2130 to 2131 in 5dab47d

	// FIXME: Perform these checks only at run time
	/*if cfg!(debug_assertions) && !(is_aligned_and_not_null(src) && is_aligned_and_not_null(dst)) {

We can't do these checks at compile-time, since is_aligned_and_not_null, for example, involves ptr->int cast to check the alignment:

rust/library/core/src/intrinsics.rs

Lines 1950 to 1952 in 5dab47d

	pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
	!ptr.is_null() && ptr as usize % mem::align_of::<T>() == 0
	}

So, as the FIXME suggests, we should enable the checks only at runtime. Recently const_eval_select intrinsic was implemented, it allows for exactly this use case - running different code in CTFE and runtime.

cc @rust-lang/lang, @rust-lang/libs and @rust-lang/wg-const-eval (it seems like use of const_eval_select requires approval of all of the above teams)

@rustbot label +T-lang +T-libs +A-const-eval +A-const-fn

[jfrimmel]

@rustbot claim

[joshtriplett]

@oli-obk Did you intend to nominate this for lang, or for libs?

[oli-obk]

kinda both:

lang: for whether const_eval_select is ok in general
libs: whether we should start using it in stable const fn

[Mark-Simulacrum]

As the assigned reviewer on #90041, and per some of the discussion in today's T-libs meeting, it feels like it would be good to have a more thorough writeup from @oli-obk or someone else on wg-const-eval about what is being asked here.

In particular, "lang: for whether const_eval_select is ok in general" seems like a question best asked with a considerable amount of context lacking in this issue, perhaps even RFC-like text targeted at the T-lang audience. While I'm not on T-lang, I definitely couldn't make a decision without that document on what the requirements for its usage should be, and this currently doesn't place it into that context. It also seems like the question is better suited to a tracking issue (or perhaps ideally the PR adding it, rather than this issue), but that's a separate question.

I imagine a similar level of context would be helpful on the libs side as well, but again, not on that team either.

[oli-obk]

Thanks for the very kind way of informing me that I was writing useless comments again. I should know better, I do know better, but idk... Anyway:

The const_eval_select intrinsic (initially considered as a solution to rust-lang/const-eval#7, but also useful in several other situations), allows calling one of two different functions, where the function is chosen depending on whether we are const evaluating or not. This allows you to write code that behaves differently between runtime and compile-time, and we have no way of checking whether that happened. You are not allowed to use the intrinsic to actually do that though. Both code paths should behave exactly the same, but you may make the const-eval path slower or otherwise different so that it can actually be const evaluated, without losing runtime performance. Notable examples of where it would be ok to use them as far as @rust-lang/wg-const-eval is concerned:

• implementing SIMD or platform intrinsics, where the const-eval part is just a slow, pure-Rust version of the same logic
• skipping pointer alignment or other pointer math checks that are going to get checked by the const eval machinery anyway
• unsure yet: avoiding debug! and dbg! statements at compile-time (or having a different mechanism entirely for reporting them)

There are no plans to expose such behaviour to users at present, this is solely for libstd/libcore implementations.

The intrinsic itself (from a runtime perspective) is a trivial type-safe function and completely written in safe Rust code. Its sole magicness comes from being a lang-item that const evaluation intercepts and then chooses to behave differently on.

The intrinsic is documented at https://doc.rust-lang.org/nightly/std/intrinsics/fn.const_eval_select.html

T-lang

Do we want to use such behaviour in an easily accessible way in std? Up to now we hid each such case behind a custom intrinsic for just that specific case. Now we have a generic mean to do so.

Furthermore, but not relevant this issue, we could in the future, instead of implementing intrinsics like exact_div in CTFE wrap it in const_eval_select and have const path execute a regular checked division. Similar changes may simplify the CTFE machinery by transferring some of its logic into libcore

T-libs

Assuming T-lang has no concerns, are you willing to stabilize some const fns that are impossible to stabilize without this. The main concern is that if we ever want to back out of const_eval_select, doing so may require significant work in the CTFE machinery (per function using const_eval_select) in order to actually remove const_eval_select.

[RalfJung]

skipping pointer alignment or other pointer math checks that are going to get checked by the const eval machinery anyway

Correction: the const eval machinery does not do pointer alignment checks (Cc #63085). But if those checks are optional anyway (as is the case here, since these used to be debug assertions), then that is okay.

[RalfJung]

Furthermore, but not relevant this issue, we could in the future, instead of implementing intrinsics like exact_div in CTFE wrap it in const_eval_select and have const path execute a regular checked division. Similar changes may simplify the CTFE machinery by transferring some of its logic into libcore

Note that this does not really save code, since Miri still needs to implement the intrinsics (it will run the run-time path, not the compile-time path). So I am not sure I like this proposal -- having the intrinsic shared between CTFE and Miri gives a lot of coverage to the Miri core engine intrinsic implementation that we would not otherwise get.

These comments would probably better fit some const_eval_select tracking issue though?

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting.

We don't have any issue with adding these checks to copy or copy_nonoverlapping.

Also, as general guidance, nobody in the lang team meeting felt that non-observable uses of const_eval_select in this style need direct lang team review. We'd like to see such usages CCed to @rust-lang/wg-const-eval for review, and cases that wg-const-eval thinks are fine (such as the three "invisible" cases in @oli-obk's comment above) seem fine and entirely in the libs domain.

[m-ou-se]

@oli-obk We discussed your question in the libs meeting just now. We're happy with 'low risk' usages of this intrinsic in the standard library, even in stable functions. What counts as 'low risk' we'll leave up to @rust-lang/wg-const-eval, since y'all can best judge what the alternative would look like if const_eval_select would have to be removed.