interpret: project_downcast: do not ICE for uninhabited variants #120367
Conversation
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
project_downcast_uninhabited
branch
3 times, most recently
from
7799a86
to
70b9165
Compare
|
This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras |
| @@ -956,8 +956,7 @@ pub type AssertMessage<'tcx> = AssertKind<Operand<'tcx>>; | |||
| /// element: | |||
| /// | |||
| /// - [`Downcast`](ProjectionElem::Downcast): This projection sets the place's variant index to the | |||
| /// given one, and makes no other changes. A `Downcast` projection on a place with its variant | |||
| /// index already set is not well-formed. | |||
There was a problem hiding this comment.
I've taken the liberty to remove it. I am not even sure what "not well-formed" means. Is it UB or invalid MIR? The MIR validator does not seem to be checking anything like this.
Looks like this was added in dae5c84.
Cc @JakobDegen
There was a problem hiding this comment.
Oh wait I think this just means the place has to be an enum, not an enum variant.
RalfJung
force-pushed
the
project_downcast_uninhabited
branch
from
70b9165
to
1025a12
Compare
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@bors r+ p=10 fixes an ICE that breaks stable code |
bors
added
S-waiting-on-bors
| // If the field is uninhabited, we can forget the data (can happen in ConstProp). | ||
| // `enum S { A(!), B, C }` is an example of an enum with Scalar layout that | ||
| // has an `Uninhabited` variant, which means this case is possible. | ||
| _ if layout.abi.is_uninhabited() => Immediate::Uninit, |
There was a problem hiding this comment.
This here is a bit unfortunate, but enum layouts are weird...
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (cdd4ff8): 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: 663.074s -> 663.841s (0.12%) |
Fixes #120337
This assertion was already under discussion for a bit; I think the example @tmiasko found is the final nail in the coffin. One could argue maybe MIR building should read the discriminant before projecting, but even then MIR optimizations should be allowed to remove that read, so the downcast should still not ICE. Maybe the downcast should be UB, but in this example UB already arises earlier when a value of type
Eis constructed.r? @oli-obk