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

codegen, miri: fix computing the offset of an unsized field in a packed struct #118540

Merged
merged 1 commit into from
Dec 4, 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
22 changes: 12 additions & 10 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
// Simple cases, which don't need DST adjustment:
// * no metadata available - just log the case
// * known alignment - sized types, `[T]`, `str` or a foreign type
// * packed struct - there is no alignment padding
// Note that looking at `field.align` is incorrect since that is not necessarily equal
// to the dynamic alignment of the type.
match field.ty.kind() {
_ if self.llextra.is_none() => {
debug!(
Expand All @@ -154,14 +155,6 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
}
_ if field.is_sized() => return simple(),
ty::Slice(..) | ty::Str | ty::Foreign(..) => return simple(),
ty::Adt(def, _) => {
if def.repr().packed() {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could have tried to preserve this fast-path for some packed structs, but I don't think that's worth it.

// FIXME(eddyb) generalize the adjustment when we
// start supporting packing to larger alignments.
assert_eq!(self.layout.align.abi.bytes(), 1);
return simple();
}
}
_ => {}
}

Expand All @@ -186,7 +179,16 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
let unaligned_offset = bx.cx().const_usize(offset.bytes());

// Get the alignment of the field
let (_, unsized_align) = glue::size_and_align_of_dst(bx, field.ty, meta);
let (_, mut unsized_align) = glue::size_and_align_of_dst(bx, field.ty, meta);

// For packed types, we need to cap alignment.
if let ty::Adt(def, _) = self.layout.ty.kind()
&& let Some(packed) = def.repr().pack
{
let packed = bx.const_usize(packed.bytes());
let cmp = bx.icmp(IntPredicate::IntULT, unsized_align, packed);
unsized_align = bx.select(cmp, unsized_align, packed)
}

// Bump the unaligned offset up to the appropriate alignment
let offset = round_up_const_value_to_alignment(bx, unaligned_offset, unsized_align);
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_const_eval/src/interpret/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,17 @@ where
// With custom DSTS, this *will* execute user-defined code, but the same
// happens at run-time so that's okay.
match self.size_and_align_of(&base_meta, &field_layout)? {
Some((_, align)) => (base_meta, offset.align_to(align)),
Some((_, align)) => {
// For packed types, we need to cap alignment.
let align = if let ty::Adt(def, _) = base.layout().ty.kind()
&& let Some(packed) = def.repr().pack
{
align.min(packed)
} else {
align
};
(base_meta, offset.align_to(align))
}
None => {
// For unsized types with an extern type tail we perform no adjustments.
// NOTE: keep this in sync with `PlaceRef::project_field` in the codegen backend.
Expand Down
35 changes: 35 additions & 0 deletions src/tools/miri/tests/pass/issues/issue-3200-packed-field-offset.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![feature(layout_for_ptr)]
use std::mem;

#[repr(packed, C)]
struct PackedSized {
f: u8,
d: [u32; 4],
}

#[repr(packed, C)]
struct PackedUnsized {
f: u8,
d: [u32],
}

impl PackedSized {
fn unsize(&self) -> &PackedUnsized {
// We can't unsize via a generic type since then we get the error
// that packed structs with unsized tail don't work if the tail
// might need dropping.
let len = 4usize;
unsafe { mem::transmute((self, len)) }
}
}

fn main() { unsafe {
let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
let p = p.unsize() as *const PackedUnsized;
// Make sure the size computation does *not* think there is
// any padding in front of the `d` field.
assert_eq!(mem::size_of_val_raw(p), 1 + 4*4);
// And likewise for the offset computation.
let d = std::ptr::addr_of!((*p).d);
assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#![feature(layout_for_ptr)]
use std::mem;

#[repr(packed(4))]
struct Slice([u32]);

#[repr(packed(2), C)]
struct PackedSized {
f: u8,
d: [u32; 4],
}

#[repr(packed(2), C)]
struct PackedUnsized {
f: u8,
d: Slice,
}

impl PackedSized {
fn unsize(&self) -> &PackedUnsized {
// We can't unsize via a generic type since then we get the error
// that packed structs with unsized tail don't work if the tail
// might need dropping.
let len = 4usize;
unsafe { mem::transmute((self, len)) }
}
}

fn main() { unsafe {
let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
let p = p.unsize() as *const PackedUnsized;
// Make sure the size computation correctly adds exact 1 byte of padding
// in front of the `d` field.
assert_eq!(mem::size_of_val_raw(p), 1 + 1 + 4*4);
// And likewise for the offset computation.
let d = std::ptr::addr_of!((*p).d);
assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }
39 changes: 39 additions & 0 deletions tests/ui/packed/issue-118537-field-offset-ice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// run-pass
#![feature(layout_for_ptr)]
use std::mem;

#[repr(packed(4))]
struct Slice([u32]);

#[repr(packed(2), C)]
struct PackedSized {
f: u8,
d: [u32; 4],
}

#[repr(packed(2), C)]
struct PackedUnsized {
f: u8,
d: Slice,
}

impl PackedSized {
fn unsize(&self) -> &PackedUnsized {
// We can't unsize via a generic type since then we get the error
// that packed structs with unsized tail don't work if the tail
// might need dropping.
let len = 4usize;
unsafe { mem::transmute((self, len)) }
}
}

fn main() { unsafe {
let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
let p = p.unsize() as *const PackedUnsized;
// Make sure the size computation correctly adds exact 1 byte of padding
// in front of the `d` field.
assert_eq!(mem::size_of_val_raw(p), 1 + 1 + 4*4);
// And likewise for the offset computation.
let d = std::ptr::addr_of!((*p).d);
assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }
36 changes: 36 additions & 0 deletions tests/ui/packed/issue-118537-field-offset.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// run-pass
#![feature(layout_for_ptr)]
use std::mem;

#[repr(packed, C)]
struct PackedSized {
f: u8,
d: [u32; 4],
}

#[repr(packed, C)]
struct PackedUnsized {
f: u8,
d: [u32],
}

impl PackedSized {
fn unsize(&self) -> &PackedUnsized {
// We can't unsize via a generic type since then we get the error
// that packed structs with unsized tail don't work if the tail
// might need dropping.
let len = 4usize;
unsafe { mem::transmute((self, len)) }
}
}

fn main() { unsafe {
let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
let p = p.unsize() as *const PackedUnsized;
// Make sure the size computation does *not* think there is
// any padding in front of the `d` field.
assert_eq!(mem::size_of_val_raw(p), 1 + 4*4);
// And likewise for the offset computation.
let d = std::ptr::addr_of!((*p).d);
assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }
Loading