Separate MIR lints from validation #119077
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
Could you mention the new flag in the pr body somewhere so that |
I can see this being true for dead code, but I don't understand how this is true in general. Can you explain? |
Validator should only check that MIR is well-formed. It shouldn't be concerned with behaviour as such, and the fact that it encountered MIR with an undefined behaviour is not a reason to raise an internal compiler error. |
rustbot
added
A-testsuite
|
|
@bors r+ |
bors
added
S-waiting-on-bors
…mpiler-errors Rollup of 6 pull requests Successful merges: - rust-lang#119012 (Extract `layout_of_{struct,enum}` fn) - rust-lang#119077 (Separate MIR lints from validation) - rust-lang#119171 (Cleanup error handlers: round 4) - rust-lang#119198 (Split coroutine desugaring kind from source) - rust-lang#119222 (Add `IntoAsyncIterator`) - rust-lang#119230 (Exhaustiveness: clean up after librarification) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#119077 - tmiasko:lint, r=cjgillot Separate MIR lints from validation Add a MIR lint pass, enabled with -Zlint-mir, which identifies undefined or likely erroneous behaviour. The initial implementation mostly migrates existing checks of this nature from MIR validator, where they did not belong (those checks have false positives and there is nothing inherently invalid about MIR with undefined behaviour). Fixes rust-lang#104736 Fixes rust-lang#104843 Fixes rust-lang#116079 Fixes rust-lang#116736 Fixes rust-lang#118990
Hm, so I guess all of these still ICE with |
|
I don't understand either why these issues were closed. Were they closed because they are false positives? Because I think that any case is a bug where you have safe Rust leading to UB in the generated MIR, whether there is an ICE about it or not. I think at least #104736 and #118990 fall under this. There is two more issues that don't have unsafe but use nightly features: those issues are also still important (unless they are false positives). |
|
I think all those issues are false positives. Those false positives arise from our MIR building strategy for temporaries, where StorageLive doesn't always dominate drop and StorageDead. |
|
In what sense are those false positives? Our documentation for the semantics of StorageLive/StorageDead clearly says such code is UB, and therefore it is a bug for MIR building to produce such code. |
|
In all those issues the MIR after building looks roughly as follows: flowchart TD
Start --> Drop["Drop(x)"];
Live["StorageLive(x)"] --> Init["x = ..."];
Init --> Drop;
Drop --> Dead["StorageDead(x)"];
Before drop elaboration |
|
That sounds to me like a rather specific issue with the use of storage check disagreeing with pre-drop-elaboration MIR. Why didn't you propose a targeted fix? |
Why would it be specific to pre drop elaboration? Because drop is definitely dead and removed? It happens sometimes, for example in #118990. In general, the same situation could arise later on. Continuing with example from #118990, Replace |
|
FWIW there are more UB checks in the validator, such as the check whether LHS and RHS overlap in assignment. |
|
I was planning to do some follow up work, including migrating remaining checks as well. In fact, we already had a false positive regarding memory overlap. |
Do you have a link to that? |
|
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Rollup merge of rust-lang#119577 - tmiasko:lint, r=oli-obk Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
Add a MIR lint pass, enabled with -Zlint-mir, which identifies undefined or
likely erroneous behaviour.
The initial implementation mostly migrates existing checks of this nature from
MIR validator, where they did not belong (those checks have false positives and
there is nothing inherently invalid about MIR with undefined behaviour).
Fixes #104736
Fixes #104843
Fixes #116079
Fixes #116736
Fixes #118990