Don't do intra-pass validation on MIR shims #115005
Conversation
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
| // only valid in a reveal-all param-env. However, since we do initial | ||
| // validation with the MirBuilt phase, which uses a user-facing param-env. | ||
| // This causes validation errors when TAITs are involved. | ||
| pm::run_passes_no_validate( |
There was a problem hiding this comment.
We could, perhaps, only do this for drop shims. Other shims are (probably?) alright to do validation on.
But I'm fine with ignoring all shims -- we still do final validation after passes are run since we're transitioning to MirPhase::Runtime(RuntimePhase::Optimized).
|
#113124 proposes to change the MirSource for generator drops to be |
|
I don't think so? This doesn't have anything specifically to do with drop shims for generator types, and the changes that #113124 makes to validation don't seem relevant, unless I'm misreading something. |
|
The tests in this PR are unaffected by #113124. |
| pm::run_passes( | ||
| // We don't validate MIR here because the shims may generate code that's | ||
| // only valid in a reveal-all param-env. However, since we do initial | ||
| // validation with the MirBuilt phase, which uses a user-facing param-env. |
There was a problem hiding this comment.
What about setting body.phase to Runtime(Initial)?
There was a problem hiding this comment.
That probably works. I'm not sure if that breaks any other invariants of validation, let me try it.
There was a problem hiding this comment.
This does not work, because validation fails before the Derefer is run.
There was a problem hiding this comment.
I went down this path last week but wasn't very happy with the result as I had to run Derefer and then it seemed wise to run the other passes to actually put us in the same state as we should be when phase = Runtime(Initial) but that required either duplicating the list of passes or splitting up run_runtime_lowering_passes() since running ElaborateDrops again caused other issues. In the end, it felt pretty hacky and I think @compiler-errors's solution here is better.
|
I'm out of alternate ideas. |
bors
added
S-waiting-on-bors
Don't do intra-pass validation on MIR shims Fixes rust-lang#114375 In the test that was committed, we end up generating the drop shim for `struct Foo` that looks like: ``` fn std::ptr::drop_in_place(_1: *mut Foo) -> () { let mut _0: (); bb0: { goto -> bb5; } bb1: { return; } bb2 (cleanup): { resume; } bb3: { goto -> bb1; } bb4 (cleanup): { drop(((*_1).0: foo::WrapperWithDrop<()>)) -> [return: bb2, unwind terminate]; } bb5: { drop(((*_1).0: foo::WrapperWithDrop<()>)) -> [return: bb3, unwind: bb2]; } } ``` In `bb4` and `bb5`, we assert that `(*_1).0` has type `WrapperWithDrop<()>`. However, In a user-facing param env, the type is actually `WrapperWithDrop<Tait>`. These types are not equal in a user-facing param-env (and can't be made equal even if we use `DefiningAnchor::Bubble`, since it's a non-local TAIT).
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c469197): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 635.404s -> 634.642s (-0.12%) |
|
@compiler-errors should we beta-nominate this? |
|
I don't think it particularly matters, since the only way to get the ICE is with |
Fixes #114375
In the test that was committed, we end up generating the drop shim for
struct Foothat looks like:In
bb4andbb5, we assert that(*_1).0has typeWrapperWithDrop<()>. However, In a user-facing param env, the type is actuallyWrapperWithDrop<Tait>. These types are not equal in a user-facing param-env (and can't be made equal even if we useDefiningAnchor::Bubble, since it's a non-local TAIT).