stdarch - compile-time errors on invalid arguments

[Mark-Simulacrum]

As noted in #85155 which tracks the diagnostic issue, the standard library previously accepted code (with differing behavior) on a number of intrinsics with "intended to be const" arguments. Since #83278, though, this code is rejected at compile-time.

For example, this code compiles on stable but not 1.54 (nightly as of this writing). I think at minimum we need to identify the set of code that changed behaviors and document that in release notes.

#[cfg(target_arch = "x86_64")]
fn main() {
    use std::arch::x86_64::*;
    unsafe {
        let a: __m128 = _mm_setzero_ps();
        let b: __m128 = _mm_setzero_ps();
        let _blended = _mm_blend_ps(a, b, 0x33);
    }
}

[Mark-Simulacrum]

cc @Amanieu -- on the point about documenting this change in behavior in the release notes, would you be up for either identifying the set of intrinsics which changed behavior or documenting how to do so?

[Amanieu]

Looking back at the crater run, we only ran it in check mode, which doesn't perform monomorphization. This is the reason why this wasn't caught earlier.

The full crater run for beta is currently running, so we should be able to get a clearer picture of the breakage once it is done.

Documenting this is difficult: the new behavior affects all intrinsics which use const arguments. A quick grep of stdarch shows 1741 intrinsics, of which about ~150 are stable.

[Mark-Simulacrum]

This is in 1.54, not 1.53, so the currently running beta run won't catch anything, right? It may make sense to run a crater on the PR itself, though.

It is worrying that so many intrinsics are affected towards a compile-error, vs. e.g. having a warning - perhaps deny-by-default - like clippy's on the atomic Ordering params...

[Amanieu]

@craterbot run mode=build-only end=881c1ac408d93bb7adaa3a51dabab9266e82eee8 start=ff34b919075f35a1787659e9c448a34b06bab8de name=issue-85264

[craterbot]

👌 Experiment issue-85264 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lqd]

The mechanism used to validate the immediates using const generics is:

• this ValidateConstImm<const IMM: i32, const MIN: i32, const MAX: i32> struct offering the validation feature: a post-monomorphization error while evaluating const VALID: () = { let _ = 1 / ((IMM >= MIN && IMM <= MAX) as usize); }
• a set of static_assert_ macros forcing the evaluation of the VALID const for the const generic parameters of a given intrinsic: e.g. the _mm_blend_ps above is defined here as _mm_blend_ps<const IMM4: i32> and the body of the intrinsic will start with a static assertion ensuring IMM4 is indeed a 4-bit value.

Note that I've linked to the more common macros, but this mechanism is applied throughout including arch-specific intrinsics and validations: e.g. SAE/scale on Intel, or signed widths on MIPS.

Any intrinsic using these macros can cause an error like in the OP with an argument that doesn't pass the checks, but this set of functions should be equivalent to: all the intrinsics which have been ported to #[rustc_legacy_const_generics].

This means this mechanism is very well centralized and could be deactivated depending on the new crater run results, however that'd mean that we would return to the current situation of, depending on the intrinsics, having assertions or crashes in LLVM instead.

[Mark-Simulacrum]

And the move to 'proper' const generics and the new validation are coupled because we would otherwise have undefined behavior (or be unable to codegen? -- what would happen)? Or can we decouple them?

[Amanieu]

We get LLVM assertion failures if invalid values reach codegen.

I'm not sure if decoupling is even possible. For example, these don't work in debug builds:

if VAL <= 3 {
    // The call to intrinsic is still emitted with an out-of-range value
    intrinsic(VAL);
} else {
    panic!();
}

if VAL <= 3 {
    // LLVM asserts because the argument is not a constant
    intrinsic(VAL & 3);
} else {
    panic!();
}

[RalfJung]

I'm not sure if decoupling is even possible.

It might be possible to do something involving specialization on const arguments... that seems rather fragile though.

An alternative might be to change the LLVM IR generated by rustc, but it seems like currently rustc has no idea about the structure of these LLVM intrinsics so that probably won't be easy either.

@Amanieu

We get LLVM assertion failures if invalid values reach codegen.

How is it then possible that the example in the OP here compiles and runs on stable?

[Amanieu]

How is it then possible that the example in the OP here compiles and runs on stable?

Previously stdarch had massive match statements to convert the value to a constant argument for the intrinsic.

[Mark-Simulacrum]

@craterbot name=issue-85264 end=master#881c1ac408d93bb7adaa3a51dabab9266e82eee8 start=master#ff34b919075f35a1787659e9c448a34b06bab8de

[craterbot]

📝 Configuration of the issue-85264 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment issue-85264 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment issue-85264 is completed!
📊 56 regressed and 14 fixed (162407 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Amanieu]

All of the new failures that weren't in the previous report seem to come from ejmahler/RustFFT#74, which is already fixed.

[lqd]

And it seems it's only 2 new crates that depend on this version of rustfft. So while post-monomorphization errors are tough errors to trigger and find consistently, the results are at least encouraging.

It can also be noted that the rough "evaluation of constant failed" error in the OP, has since improved on nightly, to somewhat acceptable levels.

Here's how it looks now

error[E0080]: evaluation of `core::core_arch::macros::ValidateConstImm::<51_i32, 0_i32, 15_i32>::VALID` failed
 --> /home/lqd/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/../../stdarch/crates/core_arch/src/macros.rs:8:9
  |
8 |         assert!(IMM >= MIN && IMM <= MAX, "IMM value not in expected range");
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'IMM value not in expected range', /rustc/dbe459dedd33470f2cb28101157de316caaffa66/library/core/src/../../stdarch/crates/core_arch/src/macros.rs:8:9
  |
  = note: this error originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn std::arch::x86_64::_mm_blend_ps::<51_i32>`
    --> /home/lqd/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/rustfft-5.0.1/src/avx/avx_vector.rs:1314:23
     |
1314 |         let blended = _mm_blend_ps(rows[0], rows[2], 0x33);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And could be improved still if we, for example, specialized in stdarch the handful of actually used ValidateConstImm widths. It would allow mentioning the range bounds in the error message (or we could do some of the const fn string formatting hacks here of course).

[Amanieu]

Closing as working as intended. The error message has been improved, and a description of the potential breakage will be included in the 1.54 release notes (#86696).