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

Improve diagnostics of evaluation of constant value failed errors #85155

Closed
Herschel opened this issue May 10, 2021 · 17 comments
Closed

Improve diagnostics of evaluation of constant value failed errors #85155

Herschel opened this issue May 10, 2021 · 17 comments
Assignees
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: Writing correctness tests. I-monomorphization Issue: An error at monomorphization time. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Herschel
Copy link
Contributor

Herschel commented May 10, 2021

Given the following code:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0a24628f94a07753519f86993f792ddb

#[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);
    }
}

The current output is:

error[E0080]: evaluation of constant value failed
 --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/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

error: internal compiler error: src/tools/miri/src/diagnostics.rs:94:21: This error should be impossible in Miri: encountered constants with type errors, stopping evaluation

Pointing to these lines inside std::arch:
https://github.com/rust-lang/stdarch/blob/master/crates/core_arch/src/macros.rs#L5-L10

The error message only points to the final site where const evaluation failed in std::arch. Ideally the error would point higher to the calling code that caused the const evaluation. If this error occurs deep within the dependency graph of a project, there's no easy way to deduce which crate is making the problematic call and causing the build to fail. I resorted to grepping .cargo for arch:: and lots of trial and error. Neither --verbose nor RUST_BACKTRACE were helpful. Someone later mentioned that RUSTC_LOG=rustc_mir::interpret=info could help track it down, but this is user-unfriendly.

Impacting this issue in the wild:
ejmahler/RustFFT#74
rust-lang/stdarch#1159

1.54.0-nightly

(2021-05-09 ca82264ec7556a6011b9)
@Herschel Herschel added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 10, 2021
@bjorn3 bjorn3 added D-confusing Diagnostics: Confusing error or lint that should be reworked. I-monomorphization Issue: An error at monomorphization time. labels May 10, 2021
@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label May 10, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 10, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.54.0 milestone May 10, 2021
@Mark-Simulacrum
Copy link
Member

Possibly a regression as a result of the stdarch PR landing, tagging as such so we eventually investigate.

@Amanieu
Copy link
Member

Amanieu commented May 11, 2021

cc @rust-lang/wg-const-eval

@Amanieu Amanieu added the A-const-eval Area: constant evaluation (mir interpretation) label May 11, 2021
@apiraino
Copy link
Contributor

I've tried bisecting this error, seems to point out to PR #83278

searched nightlies: from nightly-2021-04-01 to nightly-2021-05-11
regressed nightly: nightly-2021-05-09
searched commits: from 770792f to 881c1ac
regressed commit: 881c1ac

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2021-04-01 --regress error 

@oli-obk
Copy link
Contributor

oli-obk commented May 11, 2021

Oh wow, that is the error when run with miri. The error in regular builds has no information beyond "const eval failed"

@oli-obk
Copy link
Contributor

oli-obk commented May 11, 2021

Ah, this is a post-monomorphization error. This is already being tracked in #73961

While we want to improve post-monomorphization errors, the "right" way to do this is const_evaluatable_checked. Unfortunately that feature is nowhere near ready to even be used unstably.

@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 13, 2021
@Amanieu
Copy link
Member

Amanieu commented May 13, 2021

Just to clarify: stdarch was changed to reject invalid const operands to intrinsics which were previously accepted. This was done to align with the behavior of these intrinsics in C compilers.

However the method to do this involves post-monomorphization errors which unfortunately produce very poor error messages (just "const eval failed" with no line info).

@oli-obk
Copy link
Contributor

oli-obk commented May 13, 2021

cross post from zulip

I have an idea for how to create a backtrace for post monomorphization errors. It's not great, but better than the status quo: in the monomorphization collector, we already have that stack available, we just can't feed it into any queries or whatever is producing errors. So we wrap all possibly erroring calls with something that checks the error count before and after the call. If the count went up, print the stack mentioning that the last error occurred at this monomorphization stack. To reduce the noise, only do that if the current item is generic

@oli-obk
Copy link
Contributor

oli-obk commented May 13, 2021

I'd be happy to mentor someone who wants to implement that

@oli-obk oli-obk added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label May 13, 2021
@lqd
Copy link
Member

lqd commented May 13, 2021

@oli-obk I would like to help :)

@bjorn3
Copy link
Member

bjorn3 commented May 14, 2021

re #85155 (comment): I think that would require evaluating the required consts of a MIR body inside the mono item collector.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 26, 2021
Post-monomorphization errors traces MVP

This PR works towards better diagnostics for the errors encountered in rust-lang#85155 and similar.

We can encounter post-monomorphization errors (PMEs) when collecting mono items. The current diagnostics are confusing for these cases when they happen in a dependency (but are acceptable when they happen in the local crate).

These kinds of errors will be more likely now that `stdarch` uses const generics for its intrinsics' immediate arguments, and validates these const arguments with a mechanism that triggers such PMEs.

(Not to mention that the errors happen during codegen, so only when building code that actually uses these code paths. Check builds don't trigger them, neither does unused code)

So in this PR, we detect these kinds of errors during the mono item graph walk: if any error happens while collecting a node or its neighbors, we print a diagnostic about the current collection step, so that the user has at least some context of which erroneous code and dependency triggered the error.

The diagnostics for issue rust-lang#85155 now have this note showing the source of the erroneous const argument:
```
note: the above error was encountered while instantiating `fn std::arch::x86_64::_mm_blend_ps::<51_i32>`
  --> issue-85155.rs:11:24
   |
11 |         let _blended = _mm_blend_ps(a, b, 0x33);
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

Note that rust-lang#85155 is a reduced version of a case happening in the wild, to indirect users of the `rustfft` crate, as seen in ejmahler/RustFFT#74. The crate had a few of these out-of-range immediates. Here's how the diagnostics in this PR would have looked on one of its examples before it was fixed:

<details>

```
error[E0080]: evaluation of constant value failed
 --> ./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', ./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 _mm_blend_ps::<51_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1314:23
     |
1314 |         let blended = _mm_blend_ps(rows[0], rows[2], 0x33);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: the above error was encountered while instantiating `fn _mm_permute_pd::<5_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1859:9
     |
1859 |         _mm_permute_pd(self, 0x05)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^

note: the above error was encountered while instantiating `fn _mm_permute_pd::<15_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1863:32
     |
1863 |         (_mm_movedup_pd(self), _mm_permute_pd(self, 0x0F))
     |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
error: could not compile `rustfft`

To learn more, run the command again with --verbose.
```

</details>

I've developed and discussed this with them, so maybe r? `@oli-obk` -- but feel free to redirect to someone else of course.

(I'm not sure we can say that this PR definitely closes issue 85155, as it's still unclear exactly which diagnostics and information would be interesting to report in such cases -- and we've discussed printing backtraces before. I have prototypes of some complete and therefore noisy backtraces I showed Oli, but we decided to not include them in this PR for now)
@pietroalbini pietroalbini modified the milestones: 1.54.0, 1.55.0 Jul 17, 2021
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.55.0 milestone Sep 3, 2021
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 3, 2021
@Mark-Simulacrum
Copy link
Member

This was a regression but is no longer tracked for a particular release.

bors added a commit to rust-lang-ci/rust that referenced this issue May 1, 2022
…acktrace, r=lqd

Also report the call site of PME errors locally.

Note this does not produce a full stack all the way to the first call that specifies all monomorphic parameters, it's just shallowly mentioning the last call site.

previous work: rust-lang#85633
tracking issue: rust-lang#85155

r? `@lqd`

I figured we could get some improvement for traces in local crates without going into the backtrace hell you landed in last time
@pnkfelix
Copy link
Member

Current output as of 1.66:

error[[E0080]](https://doc.rust-lang.org/stable/error-index.html#E0080): evaluation of `core::core_arch::macros::ValidateConstImm::<51, 0, 15>::VALID` failed
  |
  = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `assert` (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>`
 --> src/main.rs:7:24
  |
7 |         let _blended = _mm_blend_ps(a, b, 0x33);
  |                        ^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0080`.
error: could not compile `playground` due to previous error

This seems leaps and bounds better than it was when this was first filed...

@pnkfelix
Copy link
Member

pnkfelix commented Dec 16, 2022

@lqd and @oli-obk: do you think we can close this? (and, if necessary, open a separate tracking issue outlining what further changes you think are necessary to improve error backtraces from const-eval?)

@pnkfelix pnkfelix added the E-needs-test Call for participation: Writing correctness tests. label Dec 16, 2022
@pnkfelix
Copy link
Member

(I suppose at minimum we should add a regression test, to ensure we catch if the output gets worse...)

@oli-obk
Copy link
Contributor

oli-obk commented Dec 16, 2022

Oh yea, this is fixed now

@oli-obk oli-obk closed this as completed Dec 16, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 16, 2022

This was fixed in #104449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: Writing correctness tests. I-monomorphization Issue: An error at monomorphization time. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants