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

Post-monomorphization errors traces MVP #85633

Merged
merged 2 commits into from
May 26, 2021

Conversation

lqd
Copy link
Member

@lqd lqd commented May 24, 2021

This PR works towards better diagnostics for the errors encountered in #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 #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 #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:

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.

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)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2021
@rust-log-analyzer

This comment has been minimized.

@lqd lqd force-pushed the stackless_span_stacks branch from 7d4b518 to c0e57d1 Compare May 24, 2021 15:22
@samlh
Copy link

samlh commented May 24, 2021

I found the message somewhat opaque - maybe change the text to something like this?

note: error found while monomorphizing this function call

Even better would be to avoid having to mention monomorphization entirely.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2021

We could rewrite the message as the above error was encountered while instantiating generic thing X with concrete parameters A, B and C

lqd added 2 commits May 25, 2021 18:39
Emit a diagnostic when the monomorphized item collector
encounters errors during a step of the recursive item collection.

These post-monomorphization errors otherwise only show the
erroneous expression without a trace, making them very obscure
and hard to pinpoint whenever they happen in dependencies.
This test reproduces post-monomorphization errors one can encounter
when using incorrect immediate arguments to some of the stdarch
intrinsics using const generics.
@lqd lqd force-pushed the stackless_span_stacks branch from c0e57d1 to 6f61456 Compare May 25, 2021 16:47
@lqd
Copy link
Member Author

lqd commented May 25, 2021

The people have spoken! I've removed "monomorphization" (post-, or otherwise) from the diagnostic note, to be closer to Oli's suggestion (with the erroneous item and its substs, like the other MonoItems are usually displayed).

I've updated the OP to show the result on the snippet from issue 85155, and the more complete rustfft example.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2021

📌 Commit 6f61456 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#85478 (Disallow shadowing const parameters)
 - rust-lang#85625 (Prevent double drop in `Vec::dedup_by` if a destructor panics)
 - rust-lang#85627 (Fix a few details in THIR unsafeck)
 - rust-lang#85633 (Post-monomorphization errors traces MVP)
 - rust-lang#85670 (Remove arrays/IntoIterator message from Iterator trait.)
 - rust-lang#85678 (fix `matches!` and `assert_matches!` on edition 2021)
 - rust-lang#85679 (Remove num_as_ne_bytes feature)
 - rust-lang#85712 (Fix typo in core::array::IntoIter comment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4b0014e into rust-lang:master May 26, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 26, 2021
@lqd lqd deleted the stackless_span_stacks branch May 26, 2021 19:38
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2021
A bit more polish on const eval errors

This PR adds a bit more polish to the const eval errors:
- a slight improvement to the PME messages from rust-lang#85633: I mentioned there that the erroneous item's paths were dependent on the environment, and could be displayed fully qualified or not. This can obscure the items when they come from a dependency. This PR uses the pretty-printing code ensuring the items' paths are not trimmed.
- whenever there are generics involved in an item where const evaluation errors out, the error message now displays the instance and its const arguments, so that we can see which instantiated item and compile-time values lead to the error.

So we get this slight improvement for our beloved `stdarch` example, on nightly:
```
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', /rustc/9111b8ae9793f18179a1336417618fc07a9cac85/library/core/src/../../stdarch/crates/core_arch/src/macros.rs:8:9
  |
```

to this PR's:

```
error[E0080]: evaluation of `core::core_arch::macros::ValidateConstImm::<51_i32, 0_i32, 15_i32>::VALID` 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
  |
```

with this PR.

Of course this is an idea from Oli, so maybe r? `@oli-obk` if they have the time.
bors added a commit to rust-lang-ci/rust that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants