[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

wesleywiser · 2019-11-29T02:39:01Z

oli-obk · 2019-11-29T07:48:59Z

src/librustc_mir/interpret/place.rs

-                        .checked_sub(variants_start)
-                        .expect("overflow computing relative variant idx");
+
+                    let (variant_index_relative, op) = if variant_index.as_u32() >= variants_start {


You can use overflowing_sub here to remove some duplication. The add/sub decision can be made on the overflow bool returned by overflowing_sub

oli-obk · 2019-11-29T07:49:55Z

cc @RalfJung

RalfJung · 2019-11-29T08:47:57Z

Cc @eddyb does that look right to you? I thought enum discriminant always use subtraction here.

RalfJung · 2019-11-29T08:50:18Z

src/librustc_mir/interpret/place.rs

                    // We need to use machine arithmetic when taking into account `niche_start`:
                    // discr_val = variant_index_relative + niche_start_val
                    let discr_layout = self.layout_of(discr_layout.value.to_int_ty(*self.tcx))?;
                    let niche_start_val = ImmTy::from_uint(niche_start, discr_layout);
                    let variant_index_relative_val =
                        ImmTy::from_uint(variant_index_relative, discr_layout);
                    let discr_val = self.binary_op(
-                        mir::BinOp::Add,
+                        op,


This doesn't seem right to me... in the 2nd branch above, what happens (compared to earlier) is that variant_index_relative is negative, but here by changing the operator you are negating the second operand, niche_start_val. So the entire result is the negation of what it used to be. Don't you also have to swap the operands?

Either way, this needs a detailed comment why both branches above are correct. Also we should understand why LLVM codegen doesn't need a branch like this.

RalfJung · 2019-11-29T08:51:50Z

src/test/ui/consts/issue-66787.rs

+// compile-flags: --crate-type lib
+
+// Regression test for ICE which occurred when const propagating an enum whose discriminant
+// niche triggered an integer underflow conmupting a delta.


Is there a way to also trigger this ICE in standalone Miri? I'd prefer to have such important parts of the Miri engine not just covered indirectly through an optimization.

Given #66857 (comment) I'm not sure that's possible, since this is necessarily an initialization of an uninhabited variant, and that's dead code.

Miri likely wouldn't have had this bug for so long if it was actually executing the code that is being constant-folded here.

eddyb · 2019-11-29T13:55:56Z

I'm sorry, what is going on? How can any niche variant be before the first niche variant?
These are not custom discriminants, they are indices, this problem should be impossible.

Are you sure miri isn't missing a check for uninhabited variants?
EDIT: indeed that seems to be the case. In codegen we do this first:

rust/src/librustc_codegen_ssa/mir/place.rs

Lines 335 to 337 in fe969f4

    
           if self.layout.for_variant(bx.cx(), variant_index).abi.is_uninhabited() { 
        
               return; 
        
           }

EDIT2: okay I was very shocked at first but I guess this would generally be dead code and only the const-prop optimization would attempt to execute it, so that's why miri didn't need to handle uninhabited variants like codegen does.

eddyb · 2019-11-29T13:56:46Z

src/librustc_mir/interpret/place.rs

@@ -1082,17 +1082,21 @@ where
                }
                if variant_index != dataful_variant {
                    let variants_start = niche_variants.start().as_u32();
-                    let variant_index_relative = variant_index.as_u32()
-                        .checked_sub(variants_start)
-                        .expect("overflow computing relative variant idx");


In codegen this isn't even checked, it's a plain -, because this is impossible for indices.

eddyb · 2019-11-29T15:52:48Z

src/librustc_mir/interpret/place.rs

@@ -1082,17 +1082,21 @@ where
                }


Oh, btw, this check seems pointless, it's impossible for variant_index to be out of range (without rustc being broken), so you could just ICE here if you wanted to (but there's not really any point, dest.layout.for_variant(variant_index) would panic trying to index with it, anyway).

eddyb · 2019-11-29T16:05:01Z

@RalfJung It occurs to me that miri might not want to handle SetDiscriminant of uninhabited variants like codegen (where it's a noop). May I suggest replacing the entire ADT with undef bytes?
That way even if there's no tag/niche for the discriminant, the value is still invalidated.

RalfJung · 2019-11-29T18:56:28Z

It occurs to me that miri might not want to handle SetDiscriminant of uninhabited variants like codegen (where it's a noop)

Could you explain in a bit more detail what this problem here has to do with uninhabited variants?

May I suggest replacing the entire ADT with undef bytes?

So the semantics of SetDiscriminant would be to clear that storage, and set the discriminant? Is it the case that we always do SetDiscriminant before setting the fields? How would that fix the ICE here?

eddyb · 2019-11-29T22:26:25Z

Could you explain in a bit more detail what this problem here has to do with uninhabited variants?

The bug here is that miri sees a variant_index outside of niche_variants (because it's an uninhabited variant), which is why the subtraction fails. But codegen never does the subtraction, because it bails out much sooner, and effectively does nothing in SetDiscriminant.

So the semantics of SetDiscriminant would be to clear that storage, and set the discriminant? Is it the case that we always do SetDiscriminant before setting the fields? How would that fix the ICE here?

Uhhh I was a bit unclear. I meant writing undef bytes specifically for uninhabited variants.
It's not the fix itself, but the fix is to not try to write a tag/niche value for uninhabited variants, and I was thinking you might find not find that ideal.

For the record, SetDiscriminant(place, variant_index) must happen after initializing all the of the variant fields (i.e. fields of Downcast(place, variant_index)).

cc @matthewjasper @spastorino perhaps borrowck could use such a rule for SetDiscriminant(place, variant_index) (i.e. require Downcast(place, variant_index) to be initialized, piece-wise, and mark place as initialized) to allow us to remove Rvalue::Aggregate (but this is really offtopic for this PR).

oli-obk · 2019-11-30T09:52:28Z

Since only const prop cares about this, maybe we can insert a machine hook that allows const prop to overwrite the destination with undef.

We only need this if the destination has gotten anything else written before the SetDiscriminant, since otherwise erroring with a UB error will cause no writes to the destination, leaving it undef. It seems generally safer to overwrite with undef (or we assert that the destination is undef?)

eddyb · 2019-11-30T14:40:11Z

I was under the assumption this wouldn't be considered an UB error, hence suggesting doing something other than a noop (which is what codegen does).

But maybe it makes sense as an error, just not an UB InvalidDiscriminant one. What does Unreachable do? Attempting to SetDiscriminant an uninhabited variant should probably be similar.

oli-obk · 2019-11-30T14:59:52Z

We can emit the Unreachable UB error: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/interpret/enum.UndefinedBehaviorInfo.html#variant.Unreachable

const prop already ignores it

wesleywiser · 2019-11-30T20:01:16Z

Are you sure miri isn't missing a check for uninhabited variants?

I think @eddyb hit the nail on the head here. If I add a similar check as the one in codegen, that resolves the issue as well.

wesleywiser@aec7714

Does anyone object to doing that patch instead?

Edit: @eddyb you mentioned to me that you'd prefer if this used something named more meaningful than InvalidDiscriminant. InvalidDiscriminant seems fine to me, could you explain that a bit more?

wesleywiser · 2019-12-02T13:21:56Z

Closing in favor of #66960

eddyb · 2019-12-02T13:33:36Z

InvalidDiscriminant seems fine to me, could you explain that a bit more?

I explained on the new PR, but I think InvalidDiscriminant makes more sense for reading discriminants.

@oli-obk

…obk,RalfJung [const-prop] Fix ICE calculating enum discriminant Fixes rust-lang#66787 Different approach than rust-lang#66857 r? @oli-obk cc @RalfJung @eddyb

@oli-obk

…obk,RalfJung [const-prop] Fix ICE calculating enum discriminant Fixes rust-lang#66787 Different approach than rust-lang#66857 r? @oli-obk cc @RalfJung @eddyb

[const-prop] Fix ICE due to underflow when calculating enum discriminant

5d7e2b6

Fixes rust-lang#66787

rust-highfive assigned oli-obk Nov 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2019

oli-obk reviewed Nov 29, 2019

View reviewed changes

RalfJung reviewed Nov 29, 2019

View reviewed changes

eddyb reviewed Nov 29, 2019

View reviewed changes

wesleywiser mentioned this pull request Dec 2, 2019

[const-prop] Fix ICE calculating enum discriminant #66960

Merged

wesleywiser closed this Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

wesleywiser commented Nov 29, 2019

oli-obk Nov 29, 2019

oli-obk commented Nov 29, 2019

RalfJung commented Nov 29, 2019

RalfJung Nov 29, 2019

RalfJung Nov 29, 2019

eddyb Nov 29, 2019

eddyb commented Nov 29, 2019 •

edited

Loading

eddyb Nov 29, 2019

eddyb Nov 29, 2019

eddyb commented Nov 29, 2019

RalfJung commented Nov 29, 2019 •

edited

Loading

eddyb commented Nov 29, 2019

oli-obk commented Nov 30, 2019 •

edited

Loading

eddyb commented Nov 30, 2019

oli-obk commented Nov 30, 2019

wesleywiser commented Nov 30, 2019 •

edited

Loading

wesleywiser commented Dec 2, 2019

eddyb commented Dec 2, 2019

[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

[const-prop] Fix ICE due to underflow when calculating enum discriminant #66857

Conversation

wesleywiser commented Nov 29, 2019

oli-obk Nov 29, 2019

Choose a reason for hiding this comment

oli-obk commented Nov 29, 2019

RalfJung commented Nov 29, 2019

RalfJung Nov 29, 2019

Choose a reason for hiding this comment

RalfJung Nov 29, 2019

Choose a reason for hiding this comment

eddyb Nov 29, 2019

Choose a reason for hiding this comment

eddyb commented Nov 29, 2019 • edited Loading

eddyb Nov 29, 2019

Choose a reason for hiding this comment

eddyb Nov 29, 2019

Choose a reason for hiding this comment

eddyb commented Nov 29, 2019

RalfJung commented Nov 29, 2019 • edited Loading

eddyb commented Nov 29, 2019

oli-obk commented Nov 30, 2019 • edited Loading

eddyb commented Nov 30, 2019

oli-obk commented Nov 30, 2019

wesleywiser commented Nov 30, 2019 • edited Loading

wesleywiser commented Dec 2, 2019

eddyb commented Dec 2, 2019

eddyb commented Nov 29, 2019 •

edited

Loading

RalfJung commented Nov 29, 2019 •

edited

Loading

oli-obk commented Nov 30, 2019 •

edited

Loading

wesleywiser commented Nov 30, 2019 •

edited

Loading