Auto merge of #69257 - RalfJung:layout-visitor, r=eddyb

Adjust Miri value visitor, and doc-comment layout components

I realized that I still didn't have quite the right intuition for how our `LayoutDetails` work, so I had to adjust the Miri value visitor to the things I understood better now. I also added some doc-comments to `LayoutDetails` as a hopefully canonical place to note such things.

The main visitor change is that we *first* look at all the fields (according to `FieldPlacement`), and *then* check the variants and handle `Multiple` appropriately. I did not quite realize how orthogonal "fields" and "variants" are.
I also moved the check for the scalar ABI to *after* checking all the fields; this leads to better (more type-driven) error messages.

And it looks like we can finally remove that magic hack for `ty::Generator`. :D

r? @oli-obk for the Miri/visitor changes and @eddyb for the layout docs
The Miri PR is at: rust-lang/miri#1178

src/librustc_mir/interpret/intern.rs

@@ -187,7 +187,7 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx
         self.walk_aggregate(mplace, fields)
     }
 
-    fn visit_primitive(&mut self, mplace: MPlaceTy<'tcx>) -> InterpResult<'tcx> {
+    fn visit_value(&mut self, mplace: MPlaceTy<'tcx>) -> InterpResult<'tcx> {
         // Handle Reference types, as these are the only relocations supported by const eval.
         // Raw pointers (and boxes) are handled by the `leftover_relocations` logic.
         let ty = mplace.layout.ty;
@@ -263,8 +263,11 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx
                     None => self.ref_tracking.track((mplace, mutability, mode), || ()),
                 }
             }
+            Ok(())
+        } else {
+            // Not a reference -- proceed recursively.
+            self.walk_value(mplace)
         }
-        Ok(())
     }
 }
 

src/librustc_mir/interpret/visitor.rs

@@ -120,7 +120,7 @@ macro_rules! make_value_visitor {
             }
             /// Visits the given value as a union. No automatic recursion can happen here.
             #[inline(always)]
-            fn visit_union(&mut self, _v: Self::V) -> InterpResult<'tcx>
+            fn visit_union(&mut self, _v: Self::V, _fields: usize) -> InterpResult<'tcx>
             {
                 Ok(())
             }
@@ -150,8 +150,9 @@ macro_rules! make_value_visitor {
             ) -> InterpResult<'tcx> {
                 self.visit_value(new_val)
             }
-
             /// Called when recursing into an enum variant.
+            /// This gives the visitor the chance to track the stack of nested fields that
+            /// we are descending through.
             #[inline(always)]
             fn visit_variant(
                 &mut self,
@@ -162,33 +163,6 @@ macro_rules! make_value_visitor {
                 self.visit_value(new_val)
             }
 
-            /// Called whenever we reach a value with uninhabited layout.
-            /// Recursing to fields will *always* continue after this!  This is not meant to control
-            /// whether and how we descend recursively/ into the scalar's fields if there are any,
-            /// it is meant to provide the chance for additional checks when a value of uninhabited
-            /// layout is detected.
-            #[inline(always)]
-            fn visit_uninhabited(&mut self) -> InterpResult<'tcx>
-            { Ok(()) }
-            /// Called whenever we reach a value with scalar layout.
-            /// We do NOT provide a `ScalarMaybeUndef` here to avoid accessing memory if the
-            /// visitor is not even interested in scalars.
-            /// Recursing to fields will *always* continue after this!  This is not meant to control
-            /// whether and how we descend recursively/ into the scalar's fields if there are any,
-            /// it is meant to provide the chance for additional checks when a value of scalar
-            /// layout is detected.
-            #[inline(always)]
-            fn visit_scalar(&mut self, _v: Self::V, _layout: &layout::Scalar) -> InterpResult<'tcx>
-            { Ok(()) }
-
-            /// Called whenever we reach a value of primitive type. There can be no recursion
-            /// below such a value. This is the leaf function.
-            /// We do *not* provide an `ImmTy` here because some implementations might want
-            /// to write to the place this primitive lives in.
-            #[inline(always)]
-            fn visit_primitive(&mut self, _v: Self::V) -> InterpResult<'tcx>
-            { Ok(()) }
-
             // Default recursors. Not meant to be overloaded.
             fn walk_aggregate(
                 &mut self,
@@ -204,23 +178,10 @@ macro_rules! make_value_visitor {
             fn walk_value(&mut self, v: Self::V) -> InterpResult<'tcx>
             {
                 trace!("walk_value: type: {}", v.layout().ty);
-                // If this is a multi-variant layout, we have to find the right one and proceed with
-                // that.
-                match v.layout().variants {
-                    layout::Variants::Multiple { .. } => {
-                        let op = v.to_op(self.ecx())?;
-                        let idx = self.ecx().read_discriminant(op)?.1;
-                        let inner = v.project_downcast(self.ecx(), idx)?;
-                        trace!("walk_value: variant layout: {:#?}", inner.layout());
-                        // recurse with the inner type
-                        return self.visit_variant(v, idx, inner);
-                    }
-                    layout::Variants::Single { .. } => {}
-                }
 
-                // Even for single variants, we might be able to get a more refined type:
-                // If it is a trait object, switch to the actual type that was used to create it.
+                // Special treatment for special types, where the (static) layout is not sufficient.
                 match v.layout().ty.kind {
+                    // If it is a trait object, switch to the real type that was used to create it.
                     ty::Dynamic(..) => {
                         // immediate trait objects are not a thing
                         let dest = v.to_op(self.ecx())?.assert_mem_place(self.ecx());
@@ -229,56 +190,16 @@ macro_rules! make_value_visitor {
                         // recurse with the inner type
                         return self.visit_field(v, 0, Value::from_mem_place(inner));
                     },
-                    ty::Generator(..) => {
-                        // FIXME: Generator layout is lying: it claims a whole bunch of fields exist
-                        // when really many of them can be uninitialized.
-                        // Just treat them as a union for now, until hopefully the layout
-                        // computation is fixed.
-                        return self.visit_union(v);
-                    }
+                    // Slices do not need special handling here: they have `Array` field
+                    // placement with length 0, so we enter the `Array` case below which
+                    // indirectly uses the metadata to determine the actual length.
                     _ => {},
                 };
 
-                // If this is a scalar, visit it as such.
-                // Things can be aggregates and have scalar layout at the same time, and that
-                // is very relevant for `NonNull` and similar structs: We need to visit them
-                // at their scalar layout *before* descending into their fields.
-                // FIXME: We could avoid some redundant checks here. For newtypes wrapping
-                // scalars, we do the same check on every "level" (e.g., first we check
-                // MyNewtype and then the scalar in there).
-                match v.layout().abi {
-                    layout::Abi::Uninhabited => {
-                        self.visit_uninhabited()?;
-                    }
-                    layout::Abi::Scalar(ref layout) => {
-                        self.visit_scalar(v, layout)?;
-                    }
-                    // FIXME: Should we do something for ScalarPair? Vector?
-                    _ => {}
-                }
-
-                // Check primitive types.  We do this after checking the scalar layout,
-                // just to have that done as well.  Primitives can have varying layout,
-                // so we check them separately and before aggregate handling.
-                // It is CRITICAL that we get this check right, or we might be
-                // validating the wrong thing!
-                let primitive = match v.layout().fields {
-                    // Primitives appear as Union with 0 fields - except for Boxes and fat pointers.
-                    layout::FieldPlacement::Union(0) => true,
-                    _ => v.layout().ty.builtin_deref(true).is_some(),
-                };
-                if primitive {
-                    return self.visit_primitive(v);
-                }
-
-                // Proceed into the fields.
+                // Visit the fields of this value.
                 match v.layout().fields {
                     layout::FieldPlacement::Union(fields) => {
-                        // Empty unions are not accepted by rustc. That's great, it means we can
-                        // use that as an unambiguous signal for detecting primitives.  Make sure
-                        // we did not miss any primitive.
-                        assert!(fields > 0);
-                        self.visit_union(v)
+                        self.visit_union(v, fields)?;
                     },
                     layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
                         // FIXME: We collect in a vec because otherwise there are lifetime
@@ -288,18 +209,35 @@ macro_rules! make_value_visitor {
                                 v.project_field(self.ecx(), i as u64)
                             })
                             .collect();
-                        self.visit_aggregate(v, fields.into_iter())
+                        self.visit_aggregate(v, fields.into_iter())?;
                     },
                     layout::FieldPlacement::Array { .. } => {
                         // Let's get an mplace first.
                         let mplace = v.to_op(self.ecx())?.assert_mem_place(self.ecx());
                         // Now we can go over all the fields.
+                        // This uses the *run-time length*, i.e., if we are a slice,
+                        // the dynamic info from the metadata is used.
                         let iter = self.ecx().mplace_array_fields(mplace)?
                             .map(|f| f.and_then(|f| {
                                 Ok(Value::from_mem_place(f))
                             }));
-                        self.visit_aggregate(v, iter)
+                        self.visit_aggregate(v, iter)?;
+                    }
+                }
+
+                match v.layout().variants {
+                    // If this is a multi-variant layout, find the right variant and proceed
+                    // with *its* fields.
+                    layout::Variants::Multiple { .. } => {
+                        let op = v.to_op(self.ecx())?;
+                        let idx = self.ecx().read_discriminant(op)?.1;
+                        let inner = v.project_downcast(self.ecx(), idx)?;
+                        trace!("walk_value: variant layout: {:#?}", inner.layout());
+                        // recurse with the inner type
+                        self.visit_variant(v, idx, inner)
                     }
+                    // For single-variant layouts, we already did anything there is to do.
+                    layout::Variants::Single { .. } => Ok(())
                 }
             }
         }

src/librustc_target/abi/mod.rs

@@ -871,8 +871,26 @@ impl Niche {
 
 #[derive(PartialEq, Eq, Hash, Debug, HashStable_Generic)]
 pub struct LayoutDetails {
-    pub variants: Variants,
+    /// Says where the fields are located within the layout.
+    /// Primitives and fieldless enums appear as unions without fields.
     pub fields: FieldPlacement,
+
+    /// Encodes information about multi-variant layouts.
+    /// Even with `Multiple` variants, a layout still has its own fields! Those are then
+    /// shared between all variants. One of them will be the discriminant,
+    /// but e.g. generators can have more.
+    ///
+    /// To access all fields of this layout, both `fields` and the fields of the active variant
+    /// must be taken into account.
+    pub variants: Variants,
+
+    /// The `abi` defines how this data is passed between functions, and it defines
+    /// value restrictions via `valid_range`.
+    ///
+    /// Note that this is entirely orthogonal to the recursive structure defined by
+    /// `variants` and `fields`; for example, `ManuallyDrop<Result<isize, isize>>` has
+    /// `Abi::ScalarPair`! So, even with non-`Aggregate` `abi`, `fields` and `variants`
+    /// have to be taken into account to find all fields of this layout.
     pub abi: Abi,
 
     /// The leaf scalar with the largest number of invalid values

src/test/ui/consts/const-eval/transmute-const.stderr

@@ -2,7 +2,7 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/transmute-const.rs:5:1
    |
 LL | static FOO: bool = unsafe { mem::transmute(3u8) };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected something less or equal to 1
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected a boolean
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 

src/test/ui/consts/const-eval/ub-enum.stderr

@@ -10,15 +10,15 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:26:1
    |
 LL | const BAD_ENUM_PTR: Enum = unsafe { TransmuteEnum { in1: &1 }.out1 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:29:1
    |
 LL | const BAD_ENUM_WRAPPED: Wrap<Enum> = unsafe { TransmuteEnum { in1: &1 }.out2 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected something that cannot possibly fail to be equal to 0
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .0.<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
@@ -34,39 +34,39 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:50:1
    |
 LL | const BAD_ENUM2_PTR: Enum2 = unsafe { TransmuteEnum2 { in2: &0 }.out1 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:52:1
    |
 LL | const BAD_ENUM2_WRAPPED: Wrap<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out2 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected something that cannot possibly fail to be equal to 2
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .0.<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:56:1
    |
 LL | const BAD_ENUM2_UNDEF : Enum2 = unsafe { TransmuteEnum2 { in3: () }.out1 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected a valid enum discriminant
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes at .<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:60:1
    |
 LL | const BAD_ENUM2_OPTION_PTR: Option<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out3 };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-enum.rs:71:1
    |
 LL | const BAD_OPTION_CHAR: Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b }));
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .<downcast-variant(Some)>.0.1, but expected something less or equal to 1114111
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .<enum-variant(Some)>.0.1, but expected a valid unicode codepoint
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 

src/test/ui/consts/const-eval/ub-nonnull.stderr

@@ -44,7 +44,7 @@ error[E0080]: it is undefined behavior to use this value
   --> $DIR/ub-nonnull.rs:32:1
    |
 LL | const UNINIT: NonZeroU8 = unsafe { Transmute { uninit: () }.out };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes at .0, but expected initialized plain (non-pointer) bytes
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 

src/test/ui/consts/const-eval/ub-ref.rs

@@ -6,7 +6,7 @@ use std::mem;
 
 const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) };
 //~^ ERROR it is undefined behavior to use this value
-//~^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1)
+//~^^ type validation failed: encountered an unaligned reference (required 2 byte alignment but found 1)
 
 const NULL: &u16 = unsafe { mem::transmute(0usize) };
 //~^ ERROR it is undefined behavior to use this value