Rollup merge of #63448 - RalfJung:miri-discriminant, r=eddyb

fix Miri discriminant handling

This can be reviewed commit-by-commit fairly well.

The Miri side is at rust-lang/miri#903.

Fixes #62138

r? @eddyb @oli-obk

Author: Centril

src/librustc/ty/layout.rs

@@ -127,6 +127,7 @@ impl IntegerExt for Integer {
 
 pub trait PrimitiveExt {
     fn to_ty<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx>;
+    fn to_int_ty<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx>;
 }
 
 impl PrimitiveExt for Primitive {
@@ -138,6 +139,16 @@ impl PrimitiveExt for Primitive {
             Pointer => tcx.mk_mut_ptr(tcx.mk_unit()),
         }
     }
+
+    /// Return an *integer* type matching this primitive.
+    /// Useful in particular when dealing with enum discriminants.
+    fn to_int_ty(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> {
+        match *self {
+            Int(i, signed) => i.to_ty(tcx, signed),
+            Pointer => tcx.types.usize,
+            Float(..) => bug!("floats do not have an int type"),
+        }
+    }
 }
 
 /// The first half of a fat pointer.

src/librustc_mir/interpret/operand.rs

@@ -1,11 +1,11 @@
 //! Functions concerning immediate values and operands, and reading from operands.
 //! All high-level functions to read from memory work on operands as sources.
 
-use std::convert::TryInto;
+use std::convert::{TryInto, TryFrom};
 
 use rustc::{mir, ty};
 use rustc::ty::layout::{
-    self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, VariantIdx,
+    self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, PrimitiveExt, VariantIdx,
 };
 
 use rustc::mir::interpret::{
@@ -609,15 +609,20 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     ) -> InterpResult<'tcx, (u128, VariantIdx)> {
         trace!("read_discriminant_value {:#?}", rval.layout);
 
-        let (discr_kind, discr_index) = match rval.layout.variants {
+        let (discr_layout, discr_kind, discr_index) = match rval.layout.variants {
             layout::Variants::Single { index } => {
                 let discr_val = rval.layout.ty.discriminant_for_variant(*self.tcx, index).map_or(
                     index.as_u32() as u128,
                     |discr| discr.val);
                 return Ok((discr_val, index));
             }
-            layout::Variants::Multiple { ref discr_kind, discr_index, .. } =>
-                (discr_kind, discr_index),
+            layout::Variants::Multiple {
+                discr: ref discr_layout,
+                ref discr_kind,
+                discr_index,
+                ..
+            } =>
+                (discr_layout, discr_kind, discr_index),
         };
 
         // read raw discriminant value
@@ -634,7 +639,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     .map_err(|_| err_unsup!(InvalidDiscriminant(raw_discr.erase_tag())))?;
                 let real_discr = if discr_val.layout.ty.is_signed() {
                     // going from layout tag type to typeck discriminant type
-                    // requires first sign extending with the layout discriminant
+                    // requires first sign extending with the discriminant layout
                     let sexted = sign_extend(bits_discr, discr_val.layout.size) as i128;
                     // and then zeroing with the typeck discriminant type
                     let discr_ty = rval.layout.ty
@@ -666,8 +671,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 ref niche_variants,
                 niche_start,
             } => {
-                let variants_start = niche_variants.start().as_u32() as u128;
-                let variants_end = niche_variants.end().as_u32() as u128;
+                let variants_start = niche_variants.start().as_u32();
+                let variants_end = niche_variants.end().as_u32();
                 let raw_discr = raw_discr.not_undef().map_err(|_| {
                     err_unsup!(InvalidDiscriminant(ScalarMaybeUndef::Undef))
                 })?;
@@ -682,18 +687,34 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                         (dataful_variant.as_u32() as u128, dataful_variant)
                     },
                     Ok(raw_discr) => {
-                        let adjusted_discr = raw_discr.wrapping_sub(niche_start)
-                            .wrapping_add(variants_start);
-                        if variants_start <= adjusted_discr && adjusted_discr <= variants_end {
-                            let index = adjusted_discr as usize;
-                            assert_eq!(index as u128, adjusted_discr);
-                            assert!(index < rval.layout.ty
+                        // We need to use machine arithmetic to get the relative variant idx:
+                        // variant_index_relative = discr_val - niche_start_val
+                        let discr_layout = self.layout_of(discr_layout.value.to_int_ty(*self.tcx))?;
+                        let discr_val = ImmTy::from_uint(raw_discr, discr_layout);
+                        let niche_start_val = ImmTy::from_uint(niche_start, discr_layout);
+                        let variant_index_relative_val = self.binary_op(
+                            mir::BinOp::Sub,
+                            discr_val,
+                            niche_start_val,
+                        )?;
+                        let variant_index_relative = variant_index_relative_val
+                            .to_scalar()?
+                            .assert_bits(discr_val.layout.size);
+                        // Check if this is in the range that indicates an actual discriminant.
+                        if variant_index_relative <= u128::from(variants_end - variants_start) {
+                            let variant_index_relative = u32::try_from(variant_index_relative)
+                                .expect("we checked that this fits into a u32");
+                            // Then computing the absolute variant idx should not overflow any more.
+                            let variant_index = variants_start
+                                .checked_add(variant_index_relative)
+                                .expect("oveflow computing absolute variant idx");
+                            assert!((variant_index as usize) < rval.layout.ty
                                 .ty_adt_def()
                                 .expect("tagged layout for non adt")
                                 .variants.len());
-                            (adjusted_discr, VariantIdx::from_usize(index))
+                            (u128::from(variant_index), VariantIdx::from_u32(variant_index))
                         } else {
-                            (dataful_variant.as_u32() as u128, dataful_variant)
+                            (u128::from(dataful_variant.as_u32()), dataful_variant)
                         }
                     },
                 }

src/librustc_mir/interpret/place.rs

@@ -8,7 +8,9 @@ use std::hash::Hash;
 use rustc::mir;
 use rustc::mir::interpret::truncate;
 use rustc::ty::{self, Ty};
-use rustc::ty::layout::{self, Size, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx};
+use rustc::ty::layout::{
+    self, Size, Align, LayoutOf, TyLayout, HasDataLayout, VariantIdx, PrimitiveExt
+};
 use rustc::ty::TypeFoldable;
 
 use super::{
@@ -1027,7 +1029,7 @@ where
             }
             layout::Variants::Multiple {
                 discr_kind: layout::DiscriminantKind::Tag,
-                ref discr,
+                discr: ref discr_layout,
                 discr_index,
                 ..
             } => {
@@ -1038,7 +1040,7 @@ where
                 // raw discriminants for enums are isize or bigger during
                 // their computation, but the in-memory tag is the smallest possible
                 // representation
-                let size = discr.value.size(self);
+                let size = discr_layout.value.size(self);
                 let discr_val = truncate(discr_val, size);
 
                 let discr_dest = self.place_field(dest, discr_index as u64)?;
@@ -1050,22 +1052,32 @@ where
                     ref niche_variants,
                     niche_start,
                 },
+                discr: ref discr_layout,
                 discr_index,
                 ..
             } => {
                 assert!(
                     variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len(),
                 );
                 if variant_index != dataful_variant {
-                    let niche_dest =
-                        self.place_field(dest, discr_index as u64)?;
-                    let niche_value = variant_index.as_u32() - niche_variants.start().as_u32();
-                    let niche_value = (niche_value as u128)
-                        .wrapping_add(niche_start);
-                    self.write_scalar(
-                        Scalar::from_uint(niche_value, niche_dest.layout.size),
-                        niche_dest
+                    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");
+                    // 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,
+                        variant_index_relative_val,
+                        niche_start_val,
                     )?;
+                    // Write result.
+                    let niche_dest = self.place_field(dest, discr_index as u64)?;
+                    self.write_immediate(*discr_val, niche_dest)?;
                 }
             }
         }

src/test/ui/consts/miri_unleashed/enum_discriminants.rs

@@ -0,0 +1,110 @@
+// compile-flags: -Zunleash-the-miri-inside-of-you
+// run-pass
+
+//! Make sure that we read and write enum discriminants correctly for corner cases caused
+//! by layout optimizations.
+
+const OVERFLOW: usize = {
+    // Tests for https://github.com/rust-lang/rust/issues/62138.
+    #[repr(u8)]
+    #[allow(dead_code)]
+    enum WithWraparoundInvalidValues {
+        X = 1,
+        Y = 254,
+    }
+
+    #[allow(dead_code)]
+    enum Foo {
+        A,
+        B,
+        C(WithWraparoundInvalidValues),
+    }
+
+    let x = Foo::B;
+    match x {
+        Foo::B => 0,
+        _ => panic!(),
+    }
+};
+
+const MORE_OVERFLOW: usize = {
+    pub enum Infallible {}
+
+    // The check that the `bool` field of `V1` is encoding a "niche variant"
+    // (i.e. not `V1`, so `V3` or `V4`) used to be mathematically incorrect,
+    // causing valid `V1` values to be interpreted as other variants.
+    #[allow(dead_code)]
+    pub enum E1 {
+        V1 { f: bool },
+        V2 { f: Infallible },
+        V3,
+        V4,
+    }
+
+    // Computing the discriminant used to be done using the niche type (here `u8`,
+    // from the `bool` field of `V1`), overflowing for variants with large enough
+    // indices (`V3` and `V4`), causing them to be interpreted as other variants.
+    #[allow(dead_code)]
+    pub enum E2<X> {
+        V1 { f: bool },
+
+        /*_00*/ _01(X), _02(X), _03(X), _04(X), _05(X), _06(X), _07(X),
+        _08(X), _09(X), _0A(X), _0B(X), _0C(X), _0D(X), _0E(X), _0F(X),
+        _10(X), _11(X), _12(X), _13(X), _14(X), _15(X), _16(X), _17(X),
+        _18(X), _19(X), _1A(X), _1B(X), _1C(X), _1D(X), _1E(X), _1F(X),
+        _20(X), _21(X), _22(X), _23(X), _24(X), _25(X), _26(X), _27(X),
+        _28(X), _29(X), _2A(X), _2B(X), _2C(X), _2D(X), _2E(X), _2F(X),
+        _30(X), _31(X), _32(X), _33(X), _34(X), _35(X), _36(X), _37(X),
+        _38(X), _39(X), _3A(X), _3B(X), _3C(X), _3D(X), _3E(X), _3F(X),
+        _40(X), _41(X), _42(X), _43(X), _44(X), _45(X), _46(X), _47(X),
+        _48(X), _49(X), _4A(X), _4B(X), _4C(X), _4D(X), _4E(X), _4F(X),
+        _50(X), _51(X), _52(X), _53(X), _54(X), _55(X), _56(X), _57(X),
+        _58(X), _59(X), _5A(X), _5B(X), _5C(X), _5D(X), _5E(X), _5F(X),
+        _60(X), _61(X), _62(X), _63(X), _64(X), _65(X), _66(X), _67(X),
+        _68(X), _69(X), _6A(X), _6B(X), _6C(X), _6D(X), _6E(X), _6F(X),
+        _70(X), _71(X), _72(X), _73(X), _74(X), _75(X), _76(X), _77(X),
+        _78(X), _79(X), _7A(X), _7B(X), _7C(X), _7D(X), _7E(X), _7F(X),
+        _80(X), _81(X), _82(X), _83(X), _84(X), _85(X), _86(X), _87(X),
+        _88(X), _89(X), _8A(X), _8B(X), _8C(X), _8D(X), _8E(X), _8F(X),
+        _90(X), _91(X), _92(X), _93(X), _94(X), _95(X), _96(X), _97(X),
+        _98(X), _99(X), _9A(X), _9B(X), _9C(X), _9D(X), _9E(X), _9F(X),
+        _A0(X), _A1(X), _A2(X), _A3(X), _A4(X), _A5(X), _A6(X), _A7(X),
+        _A8(X), _A9(X), _AA(X), _AB(X), _AC(X), _AD(X), _AE(X), _AF(X),
+        _B0(X), _B1(X), _B2(X), _B3(X), _B4(X), _B5(X), _B6(X), _B7(X),
+        _B8(X), _B9(X), _BA(X), _BB(X), _BC(X), _BD(X), _BE(X), _BF(X),
+        _C0(X), _C1(X), _C2(X), _C3(X), _C4(X), _C5(X), _C6(X), _C7(X),
+        _C8(X), _C9(X), _CA(X), _CB(X), _CC(X), _CD(X), _CE(X), _CF(X),
+        _D0(X), _D1(X), _D2(X), _D3(X), _D4(X), _D5(X), _D6(X), _D7(X),
+        _D8(X), _D9(X), _DA(X), _DB(X), _DC(X), _DD(X), _DE(X), _DF(X),
+        _E0(X), _E1(X), _E2(X), _E3(X), _E4(X), _E5(X), _E6(X), _E7(X),
+        _E8(X), _E9(X), _EA(X), _EB(X), _EC(X), _ED(X), _EE(X), _EF(X),
+        _F0(X), _F1(X), _F2(X), _F3(X), _F4(X), _F5(X), _F6(X), _F7(X),
+        _F8(X), _F9(X), _FA(X), _FB(X), _FC(X), _FD(X), _FE(X), _FF(X),
+
+        V3,
+        V4,
+    }
+
+    if let E1::V2 { .. } = (E1::V1 { f: true }) {
+        unreachable!()
+    }
+    if let E1::V1 { .. } = (E1::V1 { f: true }) {
+    } else {
+        unreachable!()
+    }
+
+    if let E2::V1 { .. } = E2::V3::<Infallible> {
+        unreachable!()
+    }
+    if let E2::V3 { .. } = E2::V3::<Infallible> {
+    } else {
+        unreachable!()
+    }
+
+    0
+};
+
+fn main() {
+    assert_eq!(OVERFLOW, 0);
+    assert_eq!(MORE_OVERFLOW, 0);
+}

src/test/ui/consts/miri_unleashed/enum_discriminants.stderr

@@ -0,0 +1,72 @@
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:23:13
+   |
+LL |     let x = Foo::B;
+   |             ^^^^^^
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:25:9
+   |
+LL |         Foo::B => 0,
+   |         ^^^^^^
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:88:28
+   |
+LL |     if let E1::V2 { .. } = (E1::V1 { f: true }) {
+   |                            ^^^^^^^^^^^^^^^^^^^^
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:88:12
+   |
+LL |     if let E1::V2 { .. } = (E1::V1 { f: true }) {
+   |            ^^^^^^^^^^^^^
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:108:5
+   |
+LL |     assert_eq!(OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:108:5
+   |
+LL |     assert_eq!(OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:108:5
+   |
+LL |     assert_eq!(OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:109:5
+   |
+LL |     assert_eq!(MORE_OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:109:5
+   |
+LL |     assert_eq!(MORE_OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+
+warning: skipping const checks
+  --> $DIR/enum_discriminants.rs:109:5
+   |
+LL |     assert_eq!(MORE_OVERFLOW, 0);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
+