Skip to content

Commit c839a7b

Browse files
borsbors
committed
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
2 parents 6af4fd3 + 12054dc commit c839a7b

15 files changed

+238
-203
lines changed

‎src/librustc_mir/interpret/intern.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx
187187
self.walk_aggregate(mplace, fields)
188188
}
189189

190-
fn visit_primitive(&mut self, mplace: MPlaceTy<'tcx>) -> InterpResult<'tcx> {
190+
fn visit_value(&mut self, mplace: MPlaceTy<'tcx>) -> InterpResult<'tcx> {
191191
// Handle Reference types, as these are the only relocations supported by const eval.
192192
// Raw pointers (and boxes) are handled by the `leftover_relocations` logic.
193193
let ty = mplace.layout.ty;
@@ -263,8 +263,11 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx
263263
None => self.ref_tracking.track((mplace, mutability, mode), || ()),
264264
}
265265
}
266+
Ok(())
267+
} else {
268+
// Not a reference -- proceed recursively.
269+
self.walk_value(mplace)
266270
}
267-
Ok(())
268271
}
269272
}
270273

‎src/librustc_mir/interpret/validity.rs

+157-81
Large diffs are not rendered by default.

‎src/librustc_mir/interpret/visitor.rs

+29-91
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ macro_rules! make_value_visitor {
120120
}
121121
/// Visits the given value as a union. No automatic recursion can happen here.
122122
#[inline(always)]
123-
fn visit_union(&mut self, _v: Self::V) -> InterpResult<'tcx>
123+
fn visit_union(&mut self, _v: Self::V, _fields: usize) -> InterpResult<'tcx>
124124
{
125125
Ok(())
126126
}
@@ -150,8 +150,9 @@ macro_rules! make_value_visitor {
150150
) -> InterpResult<'tcx> {
151151
self.visit_value(new_val)
152152
}
153-
154153
/// Called when recursing into an enum variant.
154+
/// This gives the visitor the chance to track the stack of nested fields that
155+
/// we are descending through.
155156
#[inline(always)]
156157
fn visit_variant(
157158
&mut self,
@@ -162,33 +163,6 @@ macro_rules! make_value_visitor {
162163
self.visit_value(new_val)
163164
}
164165

165-
/// Called whenever we reach a value with uninhabited layout.
166-
/// Recursing to fields will *always* continue after this! This is not meant to control
167-
/// whether and how we descend recursively/ into the scalar's fields if there are any,
168-
/// it is meant to provide the chance for additional checks when a value of uninhabited
169-
/// layout is detected.
170-
#[inline(always)]
171-
fn visit_uninhabited(&mut self) -> InterpResult<'tcx>
172-
{ Ok(()) }
173-
/// Called whenever we reach a value with scalar layout.
174-
/// We do NOT provide a `ScalarMaybeUndef` here to avoid accessing memory if the
175-
/// visitor is not even interested in scalars.
176-
/// Recursing to fields will *always* continue after this! This is not meant to control
177-
/// whether and how we descend recursively/ into the scalar's fields if there are any,
178-
/// it is meant to provide the chance for additional checks when a value of scalar
179-
/// layout is detected.
180-
#[inline(always)]
181-
fn visit_scalar(&mut self, _v: Self::V, _layout: &layout::Scalar) -> InterpResult<'tcx>
182-
{ Ok(()) }
183-
184-
/// Called whenever we reach a value of primitive type. There can be no recursion
185-
/// below such a value. This is the leaf function.
186-
/// We do *not* provide an `ImmTy` here because some implementations might want
187-
/// to write to the place this primitive lives in.
188-
#[inline(always)]
189-
fn visit_primitive(&mut self, _v: Self::V) -> InterpResult<'tcx>
190-
{ Ok(()) }
191-
192166
// Default recursors. Not meant to be overloaded.
193167
fn walk_aggregate(
194168
&mut self,
@@ -204,23 +178,10 @@ macro_rules! make_value_visitor {
204178
fn walk_value(&mut self, v: Self::V) -> InterpResult<'tcx>
205179
{
206180
trace!("walk_value: type: {}", v.layout().ty);
207-
// If this is a multi-variant layout, we have to find the right one and proceed with
208-
// that.
209-
match v.layout().variants {
210-
layout::Variants::Multiple { .. } => {
211-
let op = v.to_op(self.ecx())?;
212-
let idx = self.ecx().read_discriminant(op)?.1;
213-
let inner = v.project_downcast(self.ecx(), idx)?;
214-
trace!("walk_value: variant layout: {:#?}", inner.layout());
215-
// recurse with the inner type
216-
return self.visit_variant(v, idx, inner);
217-
}
218-
layout::Variants::Single { .. } => {}
219-
}
220181

221-
// Even for single variants, we might be able to get a more refined type:
222-
// If it is a trait object, switch to the actual type that was used to create it.
182+
// Special treatment for special types, where the (static) layout is not sufficient.
223183
match v.layout().ty.kind {
184+
// If it is a trait object, switch to the real type that was used to create it.
224185
ty::Dynamic(..) => {
225186
// immediate trait objects are not a thing
226187
let dest = v.to_op(self.ecx())?.assert_mem_place(self.ecx());
@@ -229,56 +190,16 @@ macro_rules! make_value_visitor {
229190
// recurse with the inner type
230191
return self.visit_field(v, 0, Value::from_mem_place(inner));
231192
},
232-
ty::Generator(..) => {
233-
// FIXME: Generator layout is lying: it claims a whole bunch of fields exist
234-
// when really many of them can be uninitialized.
235-
// Just treat them as a union for now, until hopefully the layout
236-
// computation is fixed.
237-
return self.visit_union(v);
238-
}
193+
// Slices do not need special handling here: they have `Array` field
194+
// placement with length 0, so we enter the `Array` case below which
195+
// indirectly uses the metadata to determine the actual length.
239196
_ => {},
240197
};
241198

242-
// If this is a scalar, visit it as such.
243-
// Things can be aggregates and have scalar layout at the same time, and that
244-
// is very relevant for `NonNull` and similar structs: We need to visit them
245-
// at their scalar layout *before* descending into their fields.
246-
// FIXME: We could avoid some redundant checks here. For newtypes wrapping
247-
// scalars, we do the same check on every "level" (e.g., first we check
248-
// MyNewtype and then the scalar in there).
249-
match v.layout().abi {
250-
layout::Abi::Uninhabited => {
251-
self.visit_uninhabited()?;
252-
}
253-
layout::Abi::Scalar(ref layout) => {
254-
self.visit_scalar(v, layout)?;
255-
}
256-
// FIXME: Should we do something for ScalarPair? Vector?
257-
_ => {}
258-
}
259-
260-
// Check primitive types. We do this after checking the scalar layout,
261-
// just to have that done as well. Primitives can have varying layout,
262-
// so we check them separately and before aggregate handling.
263-
// It is CRITICAL that we get this check right, or we might be
264-
// validating the wrong thing!
265-
let primitive = match v.layout().fields {
266-
// Primitives appear as Union with 0 fields - except for Boxes and fat pointers.
267-
layout::FieldPlacement::Union(0) => true,
268-
_ => v.layout().ty.builtin_deref(true).is_some(),
269-
};
270-
if primitive {
271-
return self.visit_primitive(v);
272-
}
273-
274-
// Proceed into the fields.
199+
// Visit the fields of this value.
275200
match v.layout().fields {
276201
layout::FieldPlacement::Union(fields) => {
277-
// Empty unions are not accepted by rustc. That's great, it means we can
278-
// use that as an unambiguous signal for detecting primitives. Make sure
279-
// we did not miss any primitive.
280-
assert!(fields > 0);
281-
self.visit_union(v)
202+
self.visit_union(v, fields)?;
282203
},
283204
layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
284205
// FIXME: We collect in a vec because otherwise there are lifetime
@@ -288,18 +209,35 @@ macro_rules! make_value_visitor {
288209
v.project_field(self.ecx(), i as u64)
289210
})
290211
.collect();
291-
self.visit_aggregate(v, fields.into_iter())
212+
self.visit_aggregate(v, fields.into_iter())?;
292213
},
293214
layout::FieldPlacement::Array { .. } => {
294215
// Let's get an mplace first.
295216
let mplace = v.to_op(self.ecx())?.assert_mem_place(self.ecx());
296217
// Now we can go over all the fields.
218+
// This uses the *run-time length*, i.e., if we are a slice,
219+
// the dynamic info from the metadata is used.
297220
let iter = self.ecx().mplace_array_fields(mplace)?
298221
.map(|f| f.and_then(|f| {
299222
Ok(Value::from_mem_place(f))
300223
}));
301-
self.visit_aggregate(v, iter)
224+
self.visit_aggregate(v, iter)?;
225+
}
226+
}
227+
228+
match v.layout().variants {
229+
// If this is a multi-variant layout, find the right variant and proceed
230+
// with *its* fields.
231+
layout::Variants::Multiple { .. } => {
232+
let op = v.to_op(self.ecx())?;
233+
let idx = self.ecx().read_discriminant(op)?.1;
234+
let inner = v.project_downcast(self.ecx(), idx)?;
235+
trace!("walk_value: variant layout: {:#?}", inner.layout());
236+
// recurse with the inner type
237+
self.visit_variant(v, idx, inner)
302238
}
239+
// For single-variant layouts, we already did anything there is to do.
240+
layout::Variants::Single { .. } => Ok(())
303241
}
304242
}
305243
}

‎src/librustc_target/abi/mod.rs

+19-1
Original file line numberDiff line numberDiff line change
@@ -871,8 +871,26 @@ impl Niche {
871871

872872
#[derive(PartialEq, Eq, Hash, Debug, HashStable_Generic)]
873873
pub struct LayoutDetails {
874-
pub variants: Variants,
874+
/// Says where the fields are located within the layout.
875+
/// Primitives and fieldless enums appear as unions without fields.
875876
pub fields: FieldPlacement,
877+
878+
/// Encodes information about multi-variant layouts.
879+
/// Even with `Multiple` variants, a layout still has its own fields! Those are then
880+
/// shared between all variants. One of them will be the discriminant,
881+
/// but e.g. generators can have more.
882+
///
883+
/// To access all fields of this layout, both `fields` and the fields of the active variant
884+
/// must be taken into account.
885+
pub variants: Variants,
886+
887+
/// The `abi` defines how this data is passed between functions, and it defines
888+
/// value restrictions via `valid_range`.
889+
///
890+
/// Note that this is entirely orthogonal to the recursive structure defined by
891+
/// `variants` and `fields`; for example, `ManuallyDrop<Result<isize, isize>>` has
892+
/// `Abi::ScalarPair`! So, even with non-`Aggregate` `abi`, `fields` and `variants`
893+
/// have to be taken into account to find all fields of this layout.
876894
pub abi: Abi,
877895

878896
/// The leaf scalar with the largest number of invalid values

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ error[E0080]: it is undefined behavior to use this value
22
--> $DIR/transmute-const.rs:5:1
33
|
44
LL | static FOO: bool = unsafe { mem::transmute(3u8) };
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected something less or equal to 1
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected a boolean
66
|
77
= 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.
88

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

+7-7
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,15 @@ error[E0080]: it is undefined behavior to use this value
1010
--> $DIR/ub-enum.rs:26:1
1111
|
1212
LL | const BAD_ENUM_PTR: Enum = unsafe { TransmuteEnum { in1: &1 }.out1 };
13-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
1414
|
1515
= 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.
1616

1717
error[E0080]: it is undefined behavior to use this value
1818
--> $DIR/ub-enum.rs:29:1
1919
|
2020
LL | const BAD_ENUM_WRAPPED: Wrap<Enum> = unsafe { TransmuteEnum { in1: &1 }.out2 };
21-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected something that cannot possibly fail to be equal to 0
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .0.<enum-tag>, but expected initialized plain (non-pointer) bytes
2222
|
2323
= 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.
2424

@@ -34,39 +34,39 @@ error[E0080]: it is undefined behavior to use this value
3434
--> $DIR/ub-enum.rs:50:1
3535
|
3636
LL | const BAD_ENUM2_PTR: Enum2 = unsafe { TransmuteEnum2 { in2: &0 }.out1 };
37-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
3838
|
3939
= 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.
4040

4141
error[E0080]: it is undefined behavior to use this value
4242
--> $DIR/ub-enum.rs:52:1
4343
|
4444
LL | const BAD_ENUM2_WRAPPED: Wrap<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out2 };
45-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected something that cannot possibly fail to be equal to 2
45+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .0.<enum-tag>, but expected initialized plain (non-pointer) bytes
4646
|
4747
= 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.
4848

4949
error[E0080]: it is undefined behavior to use this value
5050
--> $DIR/ub-enum.rs:56:1
5151
|
5252
LL | const BAD_ENUM2_UNDEF : Enum2 = unsafe { TransmuteEnum2 { in3: () }.out1 };
53-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected a valid enum discriminant
53+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes at .<enum-tag>, but expected initialized plain (non-pointer) bytes
5454
|
5555
= 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.
5656

5757
error[E0080]: it is undefined behavior to use this value
5858
--> $DIR/ub-enum.rs:60:1
5959
|
6060
LL | const BAD_ENUM2_OPTION_PTR: Option<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out3 };
61-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at .<enum-tag>, but expected initialized plain (non-pointer) bytes
6262
|
6363
= 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.
6464

6565
error[E0080]: it is undefined behavior to use this value
6666
--> $DIR/ub-enum.rs:71:1
6767
|
6868
LL | const BAD_OPTION_CHAR: Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b }));
69-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .<downcast-variant(Some)>.0.1, but expected something less or equal to 1114111
69+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .<enum-variant(Some)>.0.1, but expected a valid unicode codepoint
7070
|
7171
= 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.
7272

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ error[E0080]: it is undefined behavior to use this value
4444
--> $DIR/ub-nonnull.rs:32:1
4545
|
4646
LL | const UNINIT: NonZeroU8 = unsafe { Transmute { uninit: () }.out };
47-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
47+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes at .0, but expected initialized plain (non-pointer) bytes
4848
|
4949
= 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.
5050

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::mem;
66

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

1111
const NULL: &u16 = unsafe { mem::transmute(0usize) };
1212
//~^ ERROR it is undefined behavior to use this value

0 commit comments

Comments
 (0)