Skip to content

Commit ec53c37

Browse files
Dylan-DPCDylan-DPC
authored
Rollup merge of #96872 - RalfJung:layout-sanity, r=eddyb
make sure ScalarPair enums have ScalarPair variants; add some layout sanity checks `@eddyb` suggested that it might be reasonable for `ScalarPair` enums to simply adjust the ABI of their variants accordingly, such that the layout invariant Miri expects actually holds. This PR implements that. I should note though that I don't know much about this layout computation code and what non-Miri consumers expect from it, so tread with caution! I also added a function to sanity-check that computed layouts are internally consistent. This helped a lot in figuring out the final shape of this PR, though I am also not 100% sure that these sanity checks are the right ones. Cc `@oli-obk` Fixes #96221
2 parents 9a3f17b + 02eca34 commit ec53c37

File tree

4 files changed

+237
-31
lines changed

4 files changed

+237
-31
lines changed

‎compiler/rustc_middle/src/ty/layout.rs‎

Lines changed: 127 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,111 @@ impl<'tcx> fmt::Display for LayoutError<'tcx> {
221221
}
222222
}
223223

224+
/// Enforce some basic invariants on layouts.
225+
fn sanity_check_layout<'tcx>(
226+
tcx: TyCtxt<'tcx>,
227+
param_env: ty::ParamEnv<'tcx>,
228+
layout: &TyAndLayout<'tcx>,
229+
) {
230+
// Type-level uninhabitedness should always imply ABI uninhabitedness.
231+
if tcx.conservative_is_privately_uninhabited(param_env.and(layout.ty)) {
232+
assert!(layout.abi.is_uninhabited());
233+
}
234+
235+
if cfg!(debug_assertions) {
236+
fn check_layout_abi<'tcx>(tcx: TyCtxt<'tcx>, layout: Layout<'tcx>) {
237+
match layout.abi() {
238+
Abi::Scalar(_scalar) => {
239+
// No padding in scalars.
240+
/* FIXME(#96185):
241+
assert_eq!(
242+
layout.align().abi,
243+
scalar.align(&tcx).abi,
244+
"alignment mismatch between ABI and layout in {layout:#?}"
245+
);
246+
assert_eq!(
247+
layout.size(),
248+
scalar.size(&tcx),
249+
"size mismatch between ABI and layout in {layout:#?}"
250+
);*/
251+
}
252+
Abi::Vector { count, element } => {
253+
// No padding in vectors. Alignment can be strengthened, though.
254+
assert!(
255+
layout.align().abi >= element.align(&tcx).abi,
256+
"alignment mismatch between ABI and layout in {layout:#?}"
257+
);
258+
let size = element.size(&tcx) * count;
259+
assert_eq!(
260+
layout.size(),
261+
size.align_to(tcx.data_layout().vector_align(size).abi),
262+
"size mismatch between ABI and layout in {layout:#?}"
263+
);
264+
}
265+
Abi::ScalarPair(scalar1, scalar2) => {
266+
// Sanity-check scalar pairs. These are a bit more flexible and support
267+
// padding, but we can at least ensure both fields actually fit into the layout
268+
// and the alignment requirement has not been weakened.
269+
let align1 = scalar1.align(&tcx).abi;
270+
let align2 = scalar2.align(&tcx).abi;
271+
assert!(
272+
layout.align().abi >= cmp::max(align1, align2),
273+
"alignment mismatch between ABI and layout in {layout:#?}",
274+
);
275+
let field2_offset = scalar1.size(&tcx).align_to(align2);
276+
assert!(
277+
layout.size() >= field2_offset + scalar2.size(&tcx),
278+
"size mismatch between ABI and layout in {layout:#?}"
279+
);
280+
}
281+
Abi::Uninhabited | Abi::Aggregate { .. } => {} // Nothing to check.
282+
}
283+
}
284+
285+
check_layout_abi(tcx, layout.layout);
286+
287+
if let Variants::Multiple { variants, .. } = &layout.variants {
288+
for variant in variants {
289+
check_layout_abi(tcx, *variant);
290+
// No nested "multiple".
291+
assert!(matches!(variant.variants(), Variants::Single { .. }));
292+
// Skip empty variants.
293+
if variant.size() == Size::ZERO
294+
|| variant.fields().count() == 0
295+
|| variant.abi().is_uninhabited()
296+
{
297+
// These are never actually accessed anyway, so we can skip them. (Note that
298+
// sometimes, variants with fields have size 0, and sometimes, variants without
299+
// fields have non-0 size.)
300+
continue;
301+
}
302+
// Variants should have the same or a smaller size as the full thing.
303+
if variant.size() > layout.size {
304+
bug!(
305+
"Type with size {} bytes has variant with size {} bytes: {layout:#?}",
306+
layout.size.bytes(),
307+
variant.size().bytes(),
308+
)
309+
}
310+
// The top-level ABI and the ABI of the variants should be coherent.
311+
let abi_coherent = match (layout.abi, variant.abi()) {
312+
(Abi::Scalar(..), Abi::Scalar(..)) => true,
313+
(Abi::ScalarPair(..), Abi::ScalarPair(..)) => true,
314+
(Abi::Uninhabited, _) => true,
315+
(Abi::Aggregate { .. }, _) => true,
316+
_ => false,
317+
};
318+
if !abi_coherent {
319+
bug!(
320+
"Variant ABI is incompatible with top-level ABI:\nvariant={:#?}\nTop-level: {layout:#?}",
321+
variant
322+
);
323+
}
324+
}
325+
}
326+
}
327+
}
328+
224329
#[instrument(skip(tcx, query), level = "debug")]
225330
fn layout_of<'tcx>(
226331
tcx: TyCtxt<'tcx>,
@@ -264,10 +369,7 @@ fn layout_of<'tcx>(
264369

265370
cx.record_layout_for_printing(layout);
266371

267-
// Type-level uninhabitedness should always imply ABI uninhabitedness.
268-
if tcx.conservative_is_privately_uninhabited(param_env.and(ty)) {
269-
assert!(layout.abi.is_uninhabited());
270-
}
372+
sanity_check_layout(tcx, param_env, &layout);
271373

272374
Ok(layout)
273375
})
@@ -1314,9 +1416,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
13141416
};
13151417
let mut abi = Abi::Aggregate { sized: true };
13161418

1317-
// Without latter check aligned enums with custom discriminant values
1318-
// Would result in ICE see the issue #92464 for more info
1319-
if tag.size(dl) == size || variants.iter().all(|layout| layout.is_empty()) {
1419+
if layout_variants.iter().all(|v| v.abi.is_uninhabited()) {
1420+
abi = Abi::Uninhabited;
1421+
} else if tag.size(dl) == size || variants.iter().all(|layout| layout.is_empty()) {
1422+
// Without latter check aligned enums with custom discriminant values
1423+
// Would result in ICE see the issue #92464 for more info
13201424
abi = Abi::Scalar(tag);
13211425
} else {
13221426
// Try to use a ScalarPair for all tagged enums.
@@ -1390,8 +1494,22 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
13901494
}
13911495
}
13921496

1393-
if layout_variants.iter().all(|v| v.abi.is_uninhabited()) {
1394-
abi = Abi::Uninhabited;
1497+
// If we pick a "clever" (by-value) ABI, we might have to adjust the ABI of the
1498+
// variants to ensure they are consistent. This is because a downcast is
1499+
// semantically a NOP, and thus should not affect layout.
1500+
if matches!(abi, Abi::Scalar(..) | Abi::ScalarPair(..)) {
1501+
for variant in &mut layout_variants {
1502+
// We only do this for variants with fields; the others are not accessed anyway.
1503+
// Also do not overwrite any already existing "clever" ABIs.
1504+
if variant.fields.count() > 0
1505+
&& matches!(variant.abi, Abi::Aggregate { .. })
1506+
{
1507+
variant.abi = abi;
1508+
// Also need to bump up the size and alignment, so that the entire value fits in here.
1509+
variant.size = cmp::max(variant.size, size);
1510+
variant.align.abi = cmp::max(variant.align.abi, align.abi);
1511+
}
1512+
}
13951513
}
13961514

13971515
let largest_niche = Niche::from_scalar(dl, Size::ZERO, tag);

‎src/test/codegen/align-struct.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub enum Enum4 {
1919
A(i32),
2020
B(i32),
2121
}
22-
// CHECK: %"Enum4::A" = type { [1 x i32], i32 }
22+
// No Aggregate type, and hence nothing in LLVM IR.
2323

2424
pub enum Enum64 {
2525
A(Align64),

‎src/test/ui/layout/debug.stderr‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,22 @@ error: layout_of(std::result::Result<i32, i32>) = Layout {
184184
variants: Single {
185185
index: 0,
186186
},
187-
abi: Aggregate {
188-
sized: true,
189-
},
187+
abi: ScalarPair(
188+
Initialized {
189+
value: Int(
190+
I32,
191+
false,
192+
),
193+
valid_range: 0..=1,
194+
},
195+
Initialized {
196+
value: Int(
197+
I32,
198+
true,
199+
),
200+
valid_range: 0..=4294967295,
201+
},
202+
),
190203
largest_niche: None,
191204
align: AbiAndPrefAlign {
192205
abi: Align(4 bytes),
@@ -206,9 +219,22 @@ error: layout_of(std::result::Result<i32, i32>) = Layout {
206219
variants: Single {
207220
index: 1,
208221
},
209-
abi: Aggregate {
210-
sized: true,
211-
},
222+
abi: ScalarPair(
223+
Initialized {
224+
value: Int(
225+
I32,
226+
false,
227+
),
228+
valid_range: 0..=1,
229+
},
230+
Initialized {
231+
value: Int(
232+
I32,
233+
true,
234+
),
235+
valid_range: 0..=4294967295,
236+
},
237+
),
212238
largest_niche: None,
213239
align: AbiAndPrefAlign {
214240
abi: Align(4 bytes),

‎src/test/ui/layout/issue-96158-scalarpair-payload-might-be-uninit.stderr‎

Lines changed: 77 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,21 @@ error: layout_of(MissingPayloadField) = Layout {
3030
variants: Single {
3131
index: 0,
3232
},
33-
abi: Aggregate {
34-
sized: true,
35-
},
33+
abi: ScalarPair(
34+
Initialized {
35+
value: Int(
36+
I8,
37+
false,
38+
),
39+
valid_range: 0..=1,
40+
},
41+
Union {
42+
value: Int(
43+
I8,
44+
false,
45+
),
46+
},
47+
),
3648
largest_niche: None,
3749
align: AbiAndPrefAlign {
3850
abi: Align(1 bytes),
@@ -131,9 +143,22 @@ error: layout_of(CommonPayloadField) = Layout {
131143
variants: Single {
132144
index: 0,
133145
},
134-
abi: Aggregate {
135-
sized: true,
136-
},
146+
abi: ScalarPair(
147+
Initialized {
148+
value: Int(
149+
I8,
150+
false,
151+
),
152+
valid_range: 0..=1,
153+
},
154+
Initialized {
155+
value: Int(
156+
I8,
157+
false,
158+
),
159+
valid_range: 0..=255,
160+
},
161+
),
137162
largest_niche: None,
138163
align: AbiAndPrefAlign {
139164
abi: Align(1 bytes),
@@ -153,9 +178,22 @@ error: layout_of(CommonPayloadField) = Layout {
153178
variants: Single {
154179
index: 1,
155180
},
156-
abi: Aggregate {
157-
sized: true,
158-
},
181+
abi: ScalarPair(
182+
Initialized {
183+
value: Int(
184+
I8,
185+
false,
186+
),
187+
valid_range: 0..=1,
188+
},
189+
Initialized {
190+
value: Int(
191+
I8,
192+
false,
193+
),
194+
valid_range: 0..=255,
195+
},
196+
),
159197
largest_niche: None,
160198
align: AbiAndPrefAlign {
161199
abi: Align(1 bytes),
@@ -237,9 +275,21 @@ error: layout_of(CommonPayloadFieldIsMaybeUninit) = Layout {
237275
variants: Single {
238276
index: 0,
239277
},
240-
abi: Aggregate {
241-
sized: true,
242-
},
278+
abi: ScalarPair(
279+
Initialized {
280+
value: Int(
281+
I8,
282+
false,
283+
),
284+
valid_range: 0..=1,
285+
},
286+
Union {
287+
value: Int(
288+
I8,
289+
false,
290+
),
291+
},
292+
),
243293
largest_niche: None,
244294
align: AbiAndPrefAlign {
245295
abi: Align(1 bytes),
@@ -259,9 +309,21 @@ error: layout_of(CommonPayloadFieldIsMaybeUninit) = Layout {
259309
variants: Single {
260310
index: 1,
261311
},
262-
abi: Aggregate {
263-
sized: true,
264-
},
312+
abi: ScalarPair(
313+
Initialized {
314+
value: Int(
315+
I8,
316+
false,
317+
),
318+
valid_range: 0..=1,
319+
},
320+
Union {
321+
value: Int(
322+
I8,
323+
false,
324+
),
325+
},
326+
),
265327
largest_niche: None,
266328
align: AbiAndPrefAlign {
267329
abi: Align(1 bytes),

0 commit comments

Comments
 (0)