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.

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

Regression: Compilation failure from imm2, imm4 etc static asserts #1159

Open
ejmahler opened this issue May 10, 2021 · 4 comments
Open

Regression: Compilation failure from imm2, imm4 etc static asserts #1159

ejmahler opened this issue May 10, 2021 · 4 comments

Comments

@ejmahler
Copy link

RustFFT recently had a bug report: ejmahler/RustFFT#74

On the latest nightly, RustFFT no longer compiles due to the following error:

error[E0080]: evaluation of constant value failed
 --> C:\Users\x\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\..\..\stdarch\crates\core_arch\src\macros.rs:8:17
  |
8 |         let _ = 1 / ((IMM >= MIN && IMM <= MAX) as usize);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to divide `1_usize` by zero

A contributor was able to determine that the error was being caused by a _mm_blend_ps intrinsic:

let blended = _mm_blend_ps(rows[0], rows[2], 0x33);

The fix was to change it to the following, removing unused bits from the imm4 value:

let blended = _mm_blend_ps(rows[0], rows[2], 0x03);

The reporter traced it back to the following code in this repo:

#[allow(unused_macros)]
macro_rules! static_assert_imm4 {
    ($imm:ident) => {
        let _ = $crate::core_arch::macros::ValidateConstImm::<$imm, 0, { (1 << 4) - 1 }>::VALID;
    };
}

I was able to trace this static assert back to this PR, although it appears to be part of a larger initiative to apply this static assert to as many intrinsics as possible.

A fix has been submitted to RustFFT, and I'm happy to make my library more correct, but I'm creating this issue to raise 2 items of feedback with this new static assert:

  1. The error isn't user-friendly. Instead of directly saying what I did wrong, I'm presented with a "secondary error" about dividing by zero. Additionally, if I decide to dig into the core::arch the code to investigate the error, there aren't any comments in the code surrounding this "divide by zero" line explaining the intent, so I can't even do the bare minimum of trying to intuit what I did wrong from reading the library code. Finally, the error is deep the guts of core::arch, and there's no hint of which of RustFFT's thousands of lines are triggering this error.
  2. If this change makes it into stable Rust, I'm worried that more than just RustFFT will break. I tested rustc 1.52 and rustc beta 1.53, and both accept the RustFFT code. so right now this is a nightly-only issue. Are there any plans to put this static assert change behind an edition flag, to avoid breaking working code?
@Herschel
Copy link

Herschel commented May 10, 2021

As the original reporter of the issue, it also was frustrating to track down even to make a report:

  • The error provided no information of the origin beyond pointing to the line inside stdarch. (https://github.com/ruffle-rs/ruffle/runs/2543159319?check_suite_focus=true) I knew it was one of my dependencies, but there was no easy way to deduce which one. Neither --verbose nor RUST_BACKTRACE were helpful
  • rustfft is somewhat deep in the dependency graph of my project
  • I resorted to grepping .cargo for arch:: and trying each of the dozens of crates by hand to track down the culprit. (Someone in zulip later mentioned RUSTC_LOG=rustc_mir::interpret=info would help).

So unfortunately these static asserts may be a bit too-clever and I suspect this will break other code as well. This should possibly be reported as an issue on rust-lang also, to provide better diagnostics for errors in const evaluation?

@lu-zero
Copy link
Contributor

lu-zero commented May 10, 2021

Please do, having better diagnostic would make tracking this kind of problem less gruesome :) (also thank you a lot for the report)

@Amanieu
Copy link
Member

Amanieu commented May 11, 2021

This is mostly a compiler issue: note that without this static_assert you will most likely get an assert/crash in LLVM instead.

@ejmahler
Copy link
Author

note that without this static_assert you will most likely get an assert/crash in LLVM instead.

This may be true in general, but in the case of the specific intrinsics used by RustFFT (_mm_blend_ps, _mm256_blend_pd, _mm_permute_pd), it compiles correctly and tests pass on stable and beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants