Use delay_span_bug in validate-mir.#147712
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes a file inside |
|
rustbot has assigned @jdonszelmann. Use |
This comment has been minimized.
This comment has been minimized.
|
Reminder, once the PR becomes ready for a review, use |
| @@ -1,6 +1,6 @@ | |||
| //@ known-bug: #140850 | |||
| //@ compile-flags: -Zvalidate-mir | |||
| fn A() -> impl { | |||
| fn A() -> impl Copy { | |||
There was a problem hiding this comment.
why did this need to change?
There was a problem hiding this comment.
Because the syntax error would hide the validation ICE.
|
@rustbot author |
|
r=me after fixing ci |
8b3f715 to
71bc112
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
71bc112 to
c7401c8
Compare
This comment has been minimized.
This comment has been minimized.
f9cd048 to
946df3d
Compare
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
8db6721 to
b590ef0
Compare
This comment has been minimized.
This comment has been minimized.
b590ef0 to
58d5c30
Compare
| // MIR validation uses delayed bugs. Flush them as we won't return for `run_compiler` to do it. | ||
| ty::tls::with_opt(|opt_tcx| { | ||
| if let Some(tcx) = opt_tcx { | ||
| tcx.dcx().flush_delayed() |
There was a problem hiding this comment.
Seems a bit odd to only do this at the end... I assume this is fairly cheap? Maybe we should just do it in find_mir_or_eval_fn after load_mir?
There was a problem hiding this comment.
Are you sure delaying this is the right strategy? The reason the validator exists is that a bunch of things working on MIR (interpreter and MIR transforms) make assumptions about MIR having some reasonable shape. We don't want to litter them all with delay_span_bug, that will take up way too much room in the code. Delaying the bug in the validator will, I think, just push these ICEs to later in the execution.
There was a problem hiding this comment.
I could split the validator in two:
- syntactic and dialect checks (is this terminator is allowed in this phase?) ;
- type and layout checks, which can spuriously fail depending on weird predicates and normalization issues.
All crashes are from the second category.
There was a problem hiding this comment.
The validator is already split, right? There's the parts that need to be re-checked after a substitution is applied, and the parts that can't be broken by substitution.
What are examples of these "type and layout checks"?
For the interpreter we've so far had the rule that if the body doesn't typeck, it shouldn't be fed to the interpreter. A lot of that was type and layout stuff, which can have entirely nonsensical consequences, like nonsensical reference layout when it thinks [T] is sized (and therefore &[T] is a thin ptr), or when an unsized type gets plugged in for a T: Sized and so a ptr-sized &T suddenly is two prts large. If every MIR transform needs to be robust against such nonsense, we'll never be able to stop all the ICEs.
There was a problem hiding this comment.
What are examples of these "type and layout checks"?
Checking that the source and destination of a Unsize cast are related by a trait. Checking that the source and destination of a Subtype projection are related...
For the interpreter we've so far had the rule that if the body doesn't typeck, it shouldn't be fed to the interpreter. A lot of that was type and layout stuff, which can have entirely nonsensical consequences, like nonsensical reference layout when it thinks
[T]is sized (and therefore&[T]is a thin ptr), or when an unsized type gets plugged in for aT: Sizedand so a ptr-sized&Tsuddenly is two prts large. If every MIR transform needs to be robust against such nonsense, we'll never be able to stop all the ICEs.
This is ok for the interpreter, as it can only call functions that are reachable. But the MIR optimizer is called on all bodies. We filter out those that are obviously erroneous, and try to filter out those that have impossible predicates. But those filters are leaky, and don't catch spurious normalization failures for example.
There was a problem hiding this comment.
Some MIR opts call into the interpreter, so it does end up being called on dead code. IIRC @BoxyUwU was working on improving the guards that avoid nonsense code from getting so far into the compilation; not sure what the status of that is or whether it would help with the issues you are trying to fix here.
There was a problem hiding this comment.
I'm not sure I fully understand the context of the mir validator 🤔 I think we should be running on the assumption that in the long term the interpreter can't be called on items where .has_param() on the generic args returns true.
If we don't evaluate things in that case then we can always avoid running the interpreter on things with trivial bounds/bounds that would cause problems with the interpreter 🤔 I'm a bit blocked on actually making progress here though. And no idea how it interacts with this change :)
There was a problem hiding this comment.
This is not just about the interpreter, MIR opts also rely on MIR invariants.
There was a problem hiding this comment.
Yeah, I don't have much of an idea of what's currently going wrong there 😅 only familiar with all the problems around const generics and the interpreter being run on stuff when it shouldn't
58d5c30 to
99b2038
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
|
Hi @cjgillot I’ve been investigating a similar ICE in #150378 and found that it appears to be a duplicate of #137916, as they share the same query stack. By tracing the query dependencies, I pinpointed an issue where I’ve verified that ensuring Just wanted to share this in case it helps with your work here! |
Fixes #126680
Fixes #137916
Fixes #146210 (dup of #137916)