Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure ScalarPair enums have ScalarPair variants; add some layout sanity checks #96872

Merged
merged 3 commits into from
May 10, 2022
Merged

make sure ScalarPair enums have ScalarPair variants; add some layout sanity checks #96872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c11c3d
make sure ScalarPair enums have ScalarPair variants; add some layout …
RalfJung May 9, 2022
04fb922
fix codegen test failure
RalfJung May 9, 2022
02eca34
also sanity-check Abi::Vector, and slight refactoring
RalfJung May 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 127 additions & 9 deletions compiler/rustc_middle/src/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,111 @@ impl<'tcx> fmt::Display for LayoutError<'tcx> {
}
}

/// Enforce some basic invariants on layouts.
fn sanity_check_layout<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
layout: &TyAndLayout<'tcx>,
) {
// Type-level uninhabitedness should always imply ABI uninhabitedness.
if tcx.conservative_is_privately_uninhabited(param_env.and(layout.ty)) {
assert!(layout.abi.is_uninhabited());
}

if cfg!(debug_assertions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at some point measure the cost of having this on by default.

fn check_layout_abi<'tcx>(tcx: TyCtxt<'tcx>, layout: Layout<'tcx>) {
match layout.abi() {
Abi::Scalar(_scalar) => {
// No padding in scalars.
/* FIXME(#96185):
assert_eq!(
layout.align().abi,
scalar.align(&tcx).abi,
"alignment mismatch between ABI and layout in {layout:#?}"
);
assert_eq!(
layout.size(),
scalar.size(&tcx),
"size mismatch between ABI and layout in {layout:#?}"
);*/
}
Abi::Vector { count, element } => {
// No padding in vectors. Alignment can be strengthened, though.
assert!(
layout.align().abi >= element.align(&tcx).abi,
"alignment mismatch between ABI and layout in {layout:#?}"
);
let size = element.size(&tcx) * count;
assert_eq!(
layout.size(),
size.align_to(tcx.data_layout().vector_align(size).abi),
"size mismatch between ABI and layout in {layout:#?}"
);
}
Abi::ScalarPair(scalar1, scalar2) => {
// Sanity-check scalar pairs. These are a bit more flexible and support
// padding, but we can at least ensure both fields actually fit into the layout
// and the alignment requirement has not been weakened.
let align1 = scalar1.align(&tcx).abi;
let align2 = scalar2.align(&tcx).abi;
assert!(
layout.align().abi >= cmp::max(align1, align2),
"alignment mismatch between ABI and layout in {layout:#?}",
);
let field2_offset = scalar1.size(&tcx).align_to(align2);
assert!(
layout.size() >= field2_offset + scalar2.size(&tcx),
"size mismatch between ABI and layout in {layout:#?}"
);
}
Abi::Uninhabited | Abi::Aggregate { .. } => {} // Nothing to check.
}
}

check_layout_abi(tcx, layout.layout);

if let Variants::Multiple { variants, .. } = &layout.variants {
for variant in variants {
check_layout_abi(tcx, *variant);
// No nested "multiple".
assert!(matches!(variant.variants(), Variants::Single { .. }));
// Skip empty variants.
if variant.size() == Size::ZERO
|| variant.fields().count() == 0
|| variant.abi().is_uninhabited()
{
// These are never actually accessed anyway, so we can skip them. (Note that
// sometimes, variants with fields have size 0, and sometimes, variants without
// fields have non-0 size.)
continue;
}
// Variants should have the same or a smaller size as the full thing.
if variant.size() > layout.size {
bug!(
"Type with size {} bytes has variant with size {} bytes: {layout:#?}",
layout.size.bytes(),
variant.size().bytes(),
)
}
// The top-level ABI and the ABI of the variants should be coherent.
let abi_coherent = match (layout.abi, variant.abi()) {
(Abi::Scalar(..), Abi::Scalar(..)) => true,
(Abi::ScalarPair(..), Abi::ScalarPair(..)) => true,
(Abi::Uninhabited, _) => true,
(Abi::Aggregate { .. }, _) => true,
_ => false,
};
if !abi_coherent {
bug!(
"Variant ABI is incompatible with top-level ABI:\nvariant={:#?}\nTop-level: {layout:#?}",
variant
);
}
}
}
}
}

#[instrument(skip(tcx, query), level = "debug")]
fn layout_of<'tcx>(
tcx: TyCtxt<'tcx>,
Expand Down Expand Up @@ -263,10 +368,7 @@ fn layout_of<'tcx>(

cx.record_layout_for_printing(layout);

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

Ok(layout)
})
Expand Down Expand Up @@ -1313,9 +1415,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
};
let mut abi = Abi::Aggregate { sized: true };

// Without latter check aligned enums with custom discriminant values
// Would result in ICE see the issue #92464 for more info
if tag.size(dl) == size || variants.iter().all(|layout| layout.is_empty()) {
if layout_variants.iter().all(|v| v.abi.is_uninhabited()) {
abi = Abi::Uninhabited;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reordered this a bit so it better fits the pattern of these three branches here computing the abi.

Rewriting this without mutable variables would be cleaner but that is an undertaking for someone else. ;)

} else if tag.size(dl) == size || variants.iter().all(|layout| layout.is_empty()) {
// Without latter check aligned enums with custom discriminant values
// Would result in ICE see the issue #92464 for more info
abi = Abi::Scalar(tag);
} else {
// Try to use a ScalarPair for all tagged enums.
Expand Down Expand Up @@ -1389,8 +1493,22 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}
}

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

let largest_niche = Niche::from_scalar(dl, Size::ZERO, tag);
Expand Down
2 changes: 1 addition & 1 deletion src/test/codegen/align-struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub enum Enum4 {
A(i32),
B(i32),
}
// CHECK: %"Enum4::A" = type { [1 x i32], i32 }
// No Aggregate type, and hence nothing in LLVM IR.

pub enum Enum64 {
A(Align64),
Expand Down
38 changes: 32 additions & 6 deletions src/test/ui/layout/debug.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,22 @@ error: layout_of(std::result::Result<i32, i32>) = Layout {
variants: Single {
index: 0,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I32,
false,
),
valid_range: 0..=1,
},
Initialized {
value: Int(
I32,
true,
),
valid_range: 0..=4294967295,
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(4 bytes),
Expand All @@ -206,9 +219,22 @@ error: layout_of(std::result::Result<i32, i32>) = Layout {
variants: Single {
index: 1,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I32,
false,
),
valid_range: 0..=1,
},
Initialized {
value: Int(
I32,
true,
),
valid_range: 0..=4294967295,
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(4 bytes),
Expand Down
92 changes: 77 additions & 15 deletions src/test/ui/layout/issue-96158-scalarpair-payload-might-be-uninit.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,21 @@ error: layout_of(MissingPayloadField) = Layout {
variants: Single {
index: 0,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
Union {
value: Int(
I8,
false,
),
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(1 bytes),
Expand Down Expand Up @@ -131,9 +143,22 @@ error: layout_of(CommonPayloadField) = Layout {
variants: Single {
index: 0,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=255,
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(1 bytes),
Expand All @@ -153,9 +178,22 @@ error: layout_of(CommonPayloadField) = Layout {
variants: Single {
index: 1,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=255,
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(1 bytes),
Expand Down Expand Up @@ -237,9 +275,21 @@ error: layout_of(CommonPayloadFieldIsMaybeUninit) = Layout {
variants: Single {
index: 0,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
Union {
value: Int(
I8,
false,
),
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(1 bytes),
Expand All @@ -259,9 +309,21 @@ error: layout_of(CommonPayloadFieldIsMaybeUninit) = Layout {
variants: Single {
index: 1,
},
abi: Aggregate {
sized: true,
},
abi: ScalarPair(
Initialized {
value: Int(
I8,
false,
),
valid_range: 0..=1,
},
Union {
value: Int(
I8,
false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this makes me wish there was an enum Signedness { Unsigned, Signed } instead of a bool, heh.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... and also nicer debug-printing of Scalar layouts. Another time.^^

),
},
),
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(1 bytes),
Expand Down