Skip to content

Commit

Permalink
fix comments in dynamic size/align computation logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@RalfJung
RalfJung committed Dec 4, 2023
1 parent e281163 commit 0a2ac74
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 44 deletions.
57 changes: 38 additions & 19 deletions compiler/rustc_codegen_ssa/src/glue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,37 +59,41 @@ pub fn size_and_align_of_dst<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
debug!("DST {} layout: {:?}", t, layout);

let i = layout.fields.count() - 1;
let sized_size = layout.fields.offset(i).bytes();
let unsized_offset_unadjusted = layout.fields.offset(i).bytes();
let sized_align = layout.align.abi.bytes();
debug!("DST {} statically sized prefix size: {} align: {}", t, sized_size, sized_align);
let sized_size = bx.const_usize(sized_size);
debug!(
"DST {} offset of dyn field: {}, statically sized align: {}",
t, unsized_offset_unadjusted, sized_align
);
let unsized_offset_unadjusted = bx.const_usize(unsized_offset_unadjusted);
let sized_align = bx.const_usize(sized_align);

// Recurse to get the size of the dynamically sized field (must be
// the last field).
let field_ty = layout.field(bx, i).ty;
let (unsized_size, mut unsized_align) = size_and_align_of_dst(bx, field_ty, info);

// FIXME (#26403, #27023): We should be adding padding
// to `sized_size` (to accommodate the `unsized_align`
// required of the unsized field that follows) before
// summing it with `sized_size`. (Note that since #26403
// is unfixed, we do not yet add the necessary padding
// here. But this is where the add would go.)
// # First compute the dynamic alignment

// Return the sum of sizes and max of aligns.
let size = bx.add(sized_size, unsized_size);

// Packed types ignore the alignment of their fields.
// Packed type alignment would have to be capped, but their tails always have known alignment.
// Therefore, their alignment has already been taken into account when computing `sized_align`
// and `unsized_offset_unadjusted`, so no further adjustment is needed.
if let ty::Adt(def, _) = t.kind() {
if def.repr().packed() {
let unsized_tail =
bx.tcx().struct_tail_with_normalize(field_ty, |ty| ty, || {});
assert!(matches!(unsized_tail.kind(), ty::Slice(..) | ty::Str));
// Therefore we do not need to adjust anything.
// It's not actually correct to say that the unsized field has this alignment, but the
// code below works correctly if we set this and it avoids having to compute
// the actual alignment (which is `unsized_align.min(packed)`).
unsized_align = sized_align;
}
}

// Choose max of two known alignments (combined value must
// be aligned according to more restrictive of the two).
let align = match (
let full_align = match (
bx.const_to_opt_u128(sized_align, false),
bx.const_to_opt_u128(unsized_align, false),
) {
Expand All @@ -104,6 +108,21 @@ pub fn size_and_align_of_dst<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
}
};

// # Then compute the dynamic size

// The full formula for the size would be:
// let unsized_offset_adjusted = unsized_offset_unadjusted.align_to(unsized_align);
// let full_size = (unsized_offset_adjusted + unsized_size).align_to(full_align);
// However, `unsized_size` is a multiple of `unsized_align`.
// Therefore, we can equivalently do the `align_to(unsized_align)` *after* adding `unsized_size`:
// let full_size = (offset + unsized_size).align_to(unsized_align).align_to(full_align);
// Furthermore, `align >= unsized_align`, and therefore we only need to do:
// let full_size = (offset + unsized_size).align_to(full_align);
// This formula also has the advantage of working correctly when the type is packed
// and we set `unsized_align = sized_align`.

let full_size = bx.add(unsized_offset_unadjusted, unsized_size);

// Issue #27023: must add any necessary padding to `size`
// (to make it a multiple of `align`) before returning it.
//
Expand All @@ -115,12 +134,12 @@ pub fn size_and_align_of_dst<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
//
// `(size + (align-1)) & -align`
let one = bx.const_usize(1);
let addend = bx.sub(align, one);
let add = bx.add(size, addend);
let neg = bx.neg(align);
let size = bx.and(add, neg);
let addend = bx.sub(full_align, one);
let add = bx.add(full_size, addend);
let neg = bx.neg(full_align);
let full_size = bx.and(add, neg);

(size, align)
(full_size, full_align)
}
}
}
43 changes: 18 additions & 25 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,14 +684,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
assert!(layout.fields.count() > 0);
trace!("DST layout: {:?}", layout);

let sized_size = layout.fields.offset(layout.fields.count() - 1);
let unsized_offset_unadjusted = layout.fields.offset(layout.fields.count() - 1);
let sized_align = layout.align.abi;
trace!(
"DST {} statically sized prefix size: {:?} align: {:?}",
layout.ty,
sized_size,
sized_align
);

// Recurse to get the size of the dynamically sized field (must be
// the last field). Can't have foreign types here, how would we
Expand All @@ -705,36 +699,35 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
return Ok(None);
};

// FIXME (#26403, #27023): We should be adding padding
// to `sized_size` (to accommodate the `unsized_align`
// required of the unsized field that follows) before
// summing it with `sized_size`. (Note that since #26403
// is unfixed, we do not yet add the necessary padding
// here. But this is where the add would go.)

// Return the sum of sizes and max of aligns.
let size = sized_size + unsized_size; // `Size` addition
// # First compute the dynamic alignment

// Packed types ignore the alignment of their fields.
// Packed type alignment needs to be capped.
if let ty::Adt(def, _) = layout.ty.kind() {
if def.repr().packed() {
unsized_align = sized_align;
if let Some(packed) = def.repr().pack {
unsized_align = unsized_align.min(packed);
}
}

// Choose max of two known alignments (combined value must
// be aligned according to more restrictive of the two).
let align = sized_align.max(unsized_align);
let full_align = sized_align.max(unsized_align);

// # Then compute the dynamic size

// Issue #27023: must add any necessary padding to `size`
// (to make it a multiple of `align`) before returning it.
let size = size.align_to(align);
let unsized_offset_adjusted = unsized_offset_unadjusted.align_to(unsized_align);
let full_size = (unsized_offset_adjusted + unsized_size).align_to(full_align);

// Just for our sanitiy's sake, assert that this is equal to what codegen would compute.
assert_eq!(
full_size,
(unsized_offset_unadjusted + unsized_size).align_to(full_align)
);

// Check if this brought us over the size limit.
if size > self.max_size_of_val() {
if full_size > self.max_size_of_val() {
throw_ub!(InvalidMeta(InvalidMetaKind::TooBig));
}
Ok(Some((size, align)))
Ok(Some((full_size, full_align)))
}
ty::Dynamic(_, _, ty::Dyn) => {
let vtable = metadata.unwrap_meta().to_pointer(self)?;
Expand Down

0 comments on commit 0a2ac74

Please sign in to comment.