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.

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

fix some issues around ZST handling #115277

Merged
merged 4 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 13 additions & 12 deletions compiler/rustc_abi/src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,10 @@ pub trait LayoutCalculator {
// for non-ZST uninhabited data (mostly partial initialization).
let absent = |fields: &IndexSlice<FieldIdx, Layout<'_>>| {
let uninhabited = fields.iter().any(|f| f.abi().is_uninhabited());
let is_zst = fields.iter().all(|f| f.0.is_zst());
uninhabited && is_zst
// We cannot ignore alignment; that might lead us to entirely discard a variant and
// produce an enum that is less aligned than it should be!
let is_1zst = fields.iter().all(|f| f.0.is_1zst());
uninhabited && is_1zst
};
let (present_first, present_second) = {
let mut present_variants = variants
Expand Down Expand Up @@ -357,10 +359,8 @@ pub trait LayoutCalculator {
// It'll fit, but we need to make some adjustments.
match layout.fields {
FieldsShape::Arbitrary { ref mut offsets, .. } => {
for (j, offset) in offsets.iter_enumerated_mut() {
if !variants[i][j].0.is_zst() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of these places special-casing ZST that I removed

Copy link
Member Author

Choose a reason for hiding this comment

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

This got added in d7a750b by @mikebenfield. But their tests still pass after removing the if. So... maybe it just was never needed?

*offset += this_offset;
}
for offset in offsets.iter_mut() {
*offset += this_offset;
}
}
_ => {
Expand Down Expand Up @@ -504,7 +504,7 @@ pub trait LayoutCalculator {
// to make room for a larger discriminant.
for field_idx in st.fields.index_by_increasing_offset() {
let field = &field_layouts[FieldIdx::from_usize(field_idx)];
if !field.0.is_zst() || field.align().abi.bytes() != 1 {
if !field.0.is_1zst() {
start_align = start_align.min(field.align().abi);
break;
}
Expand Down Expand Up @@ -603,12 +603,15 @@ pub trait LayoutCalculator {
abi = Abi::Scalar(tag);
} else {
// Try to use a ScalarPair for all tagged enums.
// That's possible only if we can find a common primitive type for all variants.
let mut common_prim = None;
let mut common_prim_initialized_in_all_variants = true;
for (field_layouts, layout_variant) in iter::zip(variants, &layout_variants) {
let FieldsShape::Arbitrary { ref offsets, .. } = layout_variant.fields else {
panic!();
};
// We skip *all* ZST here and later check if we are good in terms of alignment.
// This lets us handle some cases involving aligned ZST.
let mut fields = iter::zip(field_layouts, offsets).filter(|p| !p.0.0.is_zst());
let (field, offset) = match (fields.next(), fields.next()) {
(None, None) => {
Expand Down Expand Up @@ -954,9 +957,6 @@ fn univariant(
};

(
// Place ZSTs first to avoid "interesting offsets", especially with only one
// or two non-ZST fields. This helps Scalar/ScalarPair layouts.
!f.0.is_zst(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the other place special-casing ZST that I removed

Copy link
Member Author

Choose a reason for hiding this comment

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

This was present since #45225 by @eddyb. Maybe at the time Scalar/ScalarPair computation still worked differently and the "interesting" offsets were an issue? It doesn't seem to be an issue any more today... 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

This was likely obviated by #73453

// Then place largest alignments first.
cmp::Reverse(alignment_group_key(f)),
// Then prioritize niche placement within alignment group according to
Expand Down Expand Up @@ -1073,9 +1073,10 @@ fn univariant(
let size = min_size.align_to(align.abi);
let mut layout_of_single_non_zst_field = None;
let mut abi = Abi::Aggregate { sized };
// Unpack newtype ABIs and find scalar pairs.
// Try to make this a Scalar/ScalarPair.
if sized && size.bytes() > 0 {
// All other fields must be ZSTs.
// We skip *all* ZST here and later check if we are good in terms of alignment.
// This lets us handle some cases involving aligned ZST.
let mut non_zst_fields = fields.iter_enumerated().filter(|&(_, f)| !f.0.is_zst());

match (non_zst_fields.next(), non_zst_fields.next(), non_zst_fields.next()) {
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1660,15 +1660,25 @@ pub struct PointeeInfo {

impl LayoutS {
/// Returns `true` if the layout corresponds to an unsized type.
#[inline]
pub fn is_unsized(&self) -> bool {
self.abi.is_unsized()
}

#[inline]
pub fn is_sized(&self) -> bool {
self.abi.is_sized()
}

/// Returns `true` if the type is sized and a 1-ZST (meaning it has size 0 and alignment 1).
pub fn is_1zst(&self) -> bool {
self.is_sized() && self.size.bytes() == 0 && self.align.abi.bytes() == 1
}

/// Returns `true` if the type is a ZST and not unsized.
///
/// Note that this does *not* imply that the type is irrelevant for layout! It can still have
/// non-trivial alignment constraints. You probably want to use `is_1zst` instead.
pub fn is_zst(&self) -> bool {
match self.abi {
Abi::Scalar(_) | Abi::ScalarPair(..) | Abi::Vector { .. } => false,
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_cranelift/src/unsize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ fn unsize_ptr<'tcx>(
let src_f = src_layout.field(fx, i);
assert_eq!(src_layout.fields.offset(i).bytes(), 0);
assert_eq!(dst_layout.fields.offset(i).bytes(), 0);
if src_f.is_zst() {
if src_f.is_1zst() {
// We are looking for the one non-1-ZST field; this is not it.
continue;
}
assert_eq!(src_layout.size, src_f.size);
Expand Down Expand Up @@ -151,6 +152,7 @@ pub(crate) fn coerce_unsized_into<'tcx>(
let dst_f = dst.place_field(fx, FieldIdx::new(i));

if dst_f.layout().is_zst() {
// No data here, nothing to copy/coerce.
continue;
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_cranelift/src/vtable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ pub(crate) fn get_ptr_and_method_ref<'tcx>(
'descend_newtypes: while !arg.layout().ty.is_unsafe_ptr() && !arg.layout().ty.is_ref() {
for i in 0..arg.layout().fields.count() {
let field = arg.value_field(fx, FieldIdx::new(i));
if !field.layout().is_zst() {
// we found the one non-zero-sized field that is allowed
if !field.layout().is_1zst() {
// we found the one non-1-ZST field that is allowed
// now find *its* non-zero-sized field, or stop if it's a
// pointer
arg = field;
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,9 @@ pub fn type_di_node<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>, t: Ty<'tcx>) -> &'ll D
ty::RawPtr(ty::TypeAndMut { ty: pointee_type, .. }) | ty::Ref(_, pointee_type, _) => {
build_pointer_or_reference_di_node(cx, t, pointee_type, unique_type_id)
}
// Box<T, A> may have a non-ZST allocator A. In that case, we
// Box<T, A> may have a non-1-ZST allocator A. In that case, we
// cannot treat Box<T, A> as just an owned alias of `*mut T`.
ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_zst() => {
ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_1zst() => {
build_pointer_or_reference_di_node(cx, t, t.boxed_ty(), unique_type_id)
}
ty::FnDef(..) | ty::FnPtr(_) => build_subroutine_type_di_node(cx, unique_type_id),
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ pub fn unsize_ptr<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
(src, unsized_info(bx, a, b, old_info))
}
(&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
assert_eq!(def_a, def_b);
assert_eq!(def_a, def_b); // implies same number of fields
let src_layout = bx.cx().layout_of(src_ty);
let dst_layout = bx.cx().layout_of(dst_ty);
if src_ty == dst_ty {
Expand All @@ -211,7 +211,8 @@ pub fn unsize_ptr<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let mut result = None;
for i in 0..src_layout.fields.count() {
let src_f = src_layout.field(bx.cx(), i);
if src_f.is_zst() {
if src_f.is_1zst() {
// We are looking for the one non-1-ZST field; this is not it.
continue;
}

Expand Down Expand Up @@ -272,13 +273,14 @@ pub fn coerce_unsized_into<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
}

(&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
assert_eq!(def_a, def_b);
assert_eq!(def_a, def_b); // implies same number of fields

for i in def_a.variant(FIRST_VARIANT).fields.indices() {
let src_f = src.project_field(bx, i.as_usize());
let dst_f = dst.project_field(bx, i.as_usize());

if dst_f.layout.is_zst() {
// No data here, nothing to copy/coerce.
continue;
}

Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -933,8 +933,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
{
for i in 0..op.layout.fields.count() {
let field = op.extract_field(bx, i);
if !field.layout.is_zst() {
// we found the one non-zero-sized field that is allowed
if !field.layout.is_1zst() {
// we found the one non-1-ZST field that is allowed
// now find *its* non-zero-sized field, or stop if it's a
// pointer
op = field;
Expand Down Expand Up @@ -975,10 +975,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
{
for i in 0..op.layout.fields.count() {
let field = op.extract_field(bx, i);
if !field.layout.is_zst() {
// we found the one non-zero-sized field that is allowed
// now find *its* non-zero-sized field, or stop if it's a
// pointer
if !field.layout.is_1zst() {
// We found the one non-1-ZST field that is allowed. (The rules
// for `DispatchFromDyn` ensure there's exactly one such field.)
// Now find *its* non-zero-sized field, or stop if it's a
// pointer.
op = field;
continue 'descend_newtypes;
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<'tcx, V: CodegenObject> LocalRef<'tcx, V> {
if layout.is_zst() {
// Zero-size temporaries aren't always initialized, which
// doesn't matter because they don't contain data, but
// we need something in the operand.
// we need something sufficiently aligned in the operand.
LocalRef::Operand(OperandRef::zero_sized(layout))
} else {
LocalRef::PendingOperand
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ pub enum OperandValue<V> {
/// from [`ConstMethods::const_poison`].
///
/// An `OperandValue` *must* be this variant for any type for which
/// `is_zst` on its `Layout` returns `true`.
/// `is_zst` on its `Layout` returns `true`. Note however that
/// these values can still require alignment.
ZeroSized,
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
bx.struct_gep(ty, self.llval, 1)
}
Abi::Scalar(_) | Abi::ScalarPair(..) | Abi::Vector { .. } if field.is_zst() => {
// ZST fields are not included in Scalar, ScalarPair, and Vector layouts, so manually offset the pointer.
// ZST fields (even some that require alignment) are not included in Scalar,
// ScalarPair, and Vector layouts, so manually offset the pointer.
bx.gep(bx.cx().type_i8(), self.llval, &[bx.const_usize(offset.bytes())])
}
Abi::Scalar(_) | Abi::ScalarPair(..) => {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::Rvalue::Aggregate(..) => {
let ty = rvalue.ty(self.mir, self.cx.tcx());
let ty = self.monomorphize(ty);
// For ZST this can be `OperandValueKind::ZeroSized`.
self.cx.spanned_layout_of(ty, span).is_zst()
}
}
Expand Down
20 changes: 12 additions & 8 deletions compiler/rustc_const_eval/src/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,21 +410,25 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.unsize_into_ptr(src, dest, *s, *c)
}
(&ty::Adt(def_a, _), &ty::Adt(def_b, _)) => {
assert_eq!(def_a, def_b);
assert_eq!(def_a, def_b); // implies same number of fields

// unsizing of generic struct with pointer fields
// Example: `Arc<T>` -> `Arc<Trait>`
// here we need to increase the size of every &T thin ptr field to a fat ptr
// Unsizing of generic struct with pointer fields, like `Arc<T>` -> `Arc<Trait>`.
// There can be extra fields as long as they don't change their type or are 1-ZST.
// There might also be no field that actually needs unsizing.
let mut found_cast_field = false;
for i in 0..src.layout.fields.count() {
let cast_ty_field = cast_ty.field(self, i);
if cast_ty_field.is_zst() {
continue;
}
let src_field = self.project_field(src, i)?;
let dst_field = self.project_field(dest, i)?;
if src_field.layout.ty == cast_ty_field.ty {
if src_field.layout.is_1zst() && cast_ty_field.is_1zst() {
// Skip 1-ZST fields.
} else if src_field.layout.ty == cast_ty_field.ty {
self.copy_op(&src_field, &dst_field, /*allow_transmute*/ false)?;
} else {
if found_cast_field {
span_bug!(self.cur_span(), "unsize_into: more than one field to cast");
}
found_cast_field = true;
self.unsize_into(&src_field, cast_ty_field, &dst_field)?;
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
// if the entire value is uninit, then so is the field (can happen in ConstProp)
(Immediate::Uninit, _) => Immediate::Uninit,
// the field contains no information, can be left uninit
// (Scalar/ScalarPair can contain even aligned ZST, not just 1-ZST)
_ if layout.is_zst() => Immediate::Uninit,
// some fieldless enum variants can have non-zero size but still `Aggregate` ABI... try
// to detect those here and also give them no data
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,23 +684,23 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
_ => {
// Not there yet, search for the only non-ZST field.
// (The rules for `DispatchFromDyn` ensure there's exactly one such field.)
let mut non_zst_field = None;
for i in 0..receiver.layout.fields.count() {
let field = self.project_field(&receiver, i)?;
let zst =
field.layout.is_zst() && field.layout.align.abi.bytes() == 1;
let zst = field.layout.is_1zst();
if !zst {
assert!(
non_zst_field.is_none(),
"multiple non-ZST fields in dyn receiver type {}",
"multiple non-1-ZST fields in dyn receiver type {}",
receiver.layout.ty
);
non_zst_field = Some(field);
}
}
receiver = non_zst_field.unwrap_or_else(|| {
panic!(
"no non-ZST fields in dyn receiver type {}",
"no non-1-ZST fields in dyn receiver type {}",
receiver.layout.ty
)
});
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/layout/debug.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,13 @@ error: layout_of(S) = Layout {
fields: Arbitrary {
offsets: [
Size(0 bytes),
Size(0 bytes),
Size(8 bytes),
Size(4 bytes),
],
memory_index: [
1,
0,
2,
1,
],
},
largest_niche: None,
Expand Down
18 changes: 18 additions & 0 deletions tests/ui/layout/enum.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN"
//! Various enum layout tests.

#![feature(rustc_attrs)]
#![feature(never_type)]
#![crate_type = "lib"]

#[rustc_layout(align)]
enum UninhabitedVariantAlign { //~ERROR: abi: Align(2 bytes)
A([u8; 32]),
B([u16; 0], !), // make sure alignment in uninhabited fields is respected
}

#[rustc_layout(size)]
enum UninhabitedVariantSpace { //~ERROR: size: Size(16 bytes)
A,
B([u8; 15], !), // make sure there is space being reserved for this field.
}
14 changes: 14 additions & 0 deletions tests/ui/layout/enum.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: align: AbiAndPrefAlign { abi: Align(2 bytes), pref: $PREF_ALIGN }
--> $DIR/enum.rs:9:1
|
LL | enum UninhabitedVariantAlign {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: size: Size(16 bytes)
--> $DIR/enum.rs:15:1
|
LL | enum UninhabitedVariantSpace {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

12 changes: 12 additions & 0 deletions tests/ui/layout/struct.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN"
//! Various struct layout tests.

#![feature(rustc_attrs)]
#![feature(never_type)]
#![crate_type = "lib"]

#[rustc_layout(abi)]
struct AlignedZstPreventsScalar(i16, [i32; 0]); //~ERROR: abi: Aggregate

#[rustc_layout(abi)]
struct AlignedZstButStillScalar(i32, [i16; 0]); //~ERROR: abi: Scalar
14 changes: 14 additions & 0 deletions tests/ui/layout/struct.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: abi: Aggregate { sized: true }
--> $DIR/struct.rs:9:1
|
LL | struct AlignedZstPreventsScalar(i16, [i32; 0]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: abi: Scalar(Initialized { value: Int(I32, true), valid_range: 0..=4294967295 })
--> $DIR/struct.rs:12:1
|
LL | struct AlignedZstButStillScalar(i32, [i16; 0]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Loading
Loading