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 1 commit
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
123 changes: 112 additions & 11 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,91 @@ 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::ScalarPair(scalar1, scalar2) => {
// Sanity-check scalar pair size.
let field2_offset = scalar1.size(&tcx).align_to(scalar2.align(&tcx).abi);
let total = field2_offset + scalar2.size(&tcx);
assert!(
layout.size() >= total,
"size mismatch between ABI and layout in {layout:#?}"
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, shouldn't this also have an alignment check?

Honestly, reusing LayoutCx::scalar_pair would probably make this really straight-forward (and maybe some miri/codegen code as well, tho in that case it should be a Layout constructor rather than on LayoutCx) - except for the fact that it allocates two Vecs (if only they were SmallVec<[_; 2]>... - more things that aren't immediately actionable sigh)

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 wasn't sure what the alignment check would be, so this felt like better than nothing. 🤷

I don't see how this could reuse scalar_pair; things with ScalarPair ABI can have any number of fields since there can be newtypes around the underlying pair.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, the fields would need to be ignored wrt validation (but e.g. codegen could use the offset of the second field to avoid computing a_size.align_to(b_align) itself).

Anyway, not that viable today with the allocations.

_ => {}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}
}

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 +348,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,10 +1395,22 @@ 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);
// Make sure the variants with fields have the same ABI as the enum itself
// (since downcasting to them is a NOP).
for variant in &mut layout_variants {
if variant.fields.count() > 0
&& matches!(variant.abi, Abi::Aggregate { .. })
{
assert_eq!(variant.size, size);
variant.abi = abi;
}
}
} else {
// Try to use a ScalarPair for all tagged enums.
let mut common_prim = None;
Expand Down Expand Up @@ -1385,14 +1479,21 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
// We can use `ScalarPair` only when it matches our
// already computed layout (including `#[repr(C)]`).
abi = pair.abi;
// Make sure the variants with fields have the same ABI as the enum itself
// (since downcasting to them is a NOP).
for variant in &mut layout_variants {
if variant.fields.count() > 0
&& matches!(variant.abi, Abi::Aggregate { .. })
{
variant.abi = abi;
// Also need to bump up the size, so that the pair fits inside.
variant.size = size;
}
}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

if layout_variants.iter().all(|v| v.abi.is_uninhabited()) {
abi = Abi::Uninhabited;
}

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

let layout_variants =
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