Compile-time semantics of ub_checks intrinsic are unsound

[RalfJung]

The ub_checks intrinsic currently just evaluates to the value of tcx.sess.ub_checks() at compile-time. However, tcx.sess can differ between different crates in the same crate graph, which can lead to unsoundness:

Crate A:

pub const N: usize = if intrinsics::ub_checks() { 100 } else { 1 };

Crate B:

pub static ARRAY: [i32; A::N] = [0; A::N];

Crate C:

#[inline(never)]
pub fn get_elem(arr: &[i32; A::N]) -> i32 { arr[50] }

Crate D:

fn main() {
    C::get_elem(&B::ARRAY);
}

If we now build crate C with -Zub-checks but the rest not, then in get_elem the value of A::N is 100 so the access is in-bounds, but the actual size of B::ARRAY is just 1 since in crate B, A::N evaluates to 1. (Actually causing this to crash may need slightly more elaborate tricks to ensure we truly evaluate the right queries at the right times and use the result in the right way, but you get the gist.)

We can't have a safe intrinsic evaluate to different values in different crates, that's unsound. I can think of two options:

1. We make the intrinsic always behave the same in all crates at compile-time (e.g. by using the fallback body, which is cfg!(ub_checks) and thus returns whether libcore was built with debug assertions).
2. We make the intrinsic unsafe and make it a safety requirement that the caller may only use the return value to decide whether a constant successfully evaluates, but not the value it evaluates to.

I feel a bit uneasy about the second option since it turns what is usually fully ensured by the interpreter itself (even in the presence of unsafe code) into an invariant upheld by user code -- albeit only by code we control, assuming we will never stabilize this intrinsic.

@lcnr @BoxyUwU For the second option to be sound, it must be okay for the same constant to sometimes return a value and sometimes fail to evaluate. Currently I am fairly sure that is the case since a constant failing to evaluate will stop compilation. However, if the type system ever needs to "speculatively" evaluate a constant in a way such that const-eval failure does not stop compilation, then I think option 2 above would be unsound and we have to go with option 1. And I seem to recall from the last time we spoke that indeed the type system would like to have such a "speculative" form of constant evaluation?

Cc @rust-lang/wg-const-eval @saethlin

[scottmcm]

Could we maybe change this from being a NullOp to being something more like a https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.TerminatorKind.html#variant.Assert?

I've always thought it really weird that it's a nullop in the first place, because it's the only nullop that you can't just const-fold once layout is available. All the other UnOps would probably be better as Operand::Consts instead, IMHO.

[saethlin]

I previously suggested something like option 1 but @RalfJung asked that I not do that in #121662 (comment).

I don't care very much whether these checks are enabled in Miri or in const eval, because every single bug I've seen these checks catch was in runtime code.

The current design with an intrinsic plus a NullOp is not precious to me, and I'd be happy to redesign our way out of this odd case. I think the ideal implementation would have a MIR statement that is passed a function, and when we lower that MIR statement we generate a function call if UB checks are enabled and just ignore the statement entirely if checks are not enabled. The problem with that design is that I don't know how to bolt the library part of writing some closure to the MIR I want to generate because I can't come up with design that doesn't have a ~const FnOnce parameter.

[RalfJung]

It doesn't matter whether this is a NullaryOp or a Terminator or whatever, that makes no difference for this issue. The question is how it should behave, not how it is represented.

[RalfJung]

@scottmcm

I've always thought it really weird that it's a nullop in the first place, because it's the only nullop that you can't just const-fold once layout is available.

I don't see why a nullary op should have to be const-evaluatable. It's an operator, it can still operate and do things like fetch some runtime state from the AM. But anyway that's largely off-topic for this issue.

@saethlin

I previously suggested something like option 1 but @RalfJung asked that I not do that in

Yeah I didn't realize the soundness concerns here.

Miri should still check tcx.sess though, I am only talking about compile-time semantics in this issue.

[BoxyUwU]

Chiming in a bit late, if we can keep the possibility open for "speculatively" evaluating constants then that seems ideal to me. Definitely happy with having landed on option 1 here :-)