-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Allow borrowing array elements from packed structs with ABI align <= packed align #145419
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
base: master
Are you sure you want to change the base?
Changes from all commits
301137d
19f51f2
a2a94b9
2e84ab8
d8785b9
dd1537c
99dedb0
4620527
2fe4b7f
f0c8e9e
4a65fdc
585af05
b37f571
ef2538b
8c07e0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
use rustc_abi::Align; | ||
use rustc_middle::mir::*; | ||
use rustc_middle::ty::{self, TyCtxt}; | ||
use rustc_middle::ty::{self, Ty, TyCtxt}; | ||
use tracing::debug; | ||
|
||
/// Returns `true` if this place is allowed to be less aligned | ||
/// than its containing struct (because it is within a packed | ||
/// struct). | ||
pub fn is_disaligned<'tcx, L>( | ||
pub fn is_potentially_misaligned<'tcx, L>( | ||
tcx: TyCtxt<'tcx>, | ||
local_decls: &L, | ||
typing_env: ty::TypingEnv<'tcx>, | ||
|
@@ -15,38 +15,71 @@ pub fn is_disaligned<'tcx, L>( | |
where | ||
L: HasLocalDecls<'tcx>, | ||
{ | ||
debug!("is_disaligned({:?})", place); | ||
debug!("is_potentially_misaligned({:?})", place); | ||
let Some(pack) = is_within_packed(tcx, local_decls, place) else { | ||
debug!("is_disaligned({:?}) - not within packed", place); | ||
debug!("is_potentially_misaligned({:?}) - not within packed", place); | ||
return false; | ||
}; | ||
|
||
let ty = place.ty(local_decls, tcx).ty; | ||
let unsized_tail = || tcx.struct_tail_for_codegen(ty, typing_env); | ||
|
||
match tcx.layout_of(typing_env.as_query_input(ty)) { | ||
Ok(layout) | ||
Ok(layout) => { | ||
if layout.align.abi <= pack | ||
&& (layout.is_sized() | ||
|| matches!(unsized_tail().kind(), ty::Slice(..) | ty::Str)) => | ||
{ | ||
// If the packed alignment is greater or equal to the field alignment, the type won't be | ||
// further disaligned. | ||
// However we need to ensure the field is sized; for unsized fields, `layout.align` is | ||
// just an approximation -- except when the unsized tail is a slice, where the alignment | ||
// is fully determined by the type. | ||
debug!( | ||
"is_disaligned({:?}) - align = {}, packed = {}; not disaligned", | ||
place, | ||
layout.align.abi.bytes(), | ||
pack.bytes() | ||
); | ||
false | ||
&& (layout.is_sized() || matches!(unsized_tail().kind(), ty::Slice(..) | ty::Str)) | ||
{ | ||
// If the packed alignment is greater or equal to the field alignment, the type won't be | ||
// further disaligned. | ||
// However we need to ensure the field is sized; for unsized fields, `layout.align` is | ||
// just an approximation -- except when the unsized tail is a slice, where the alignment | ||
// is fully determined by the type. | ||
debug!( | ||
"is_potentially_misaligned({:?}) - align = {}, packed = {}; not disaligned", | ||
place, | ||
layout.align.abi.bytes(), | ||
pack.bytes() | ||
); | ||
false | ||
} else { | ||
true | ||
} | ||
} | ||
Err(_) => { | ||
// Soundness: For any `T`, the ABI alignment requirement of `[T]` equals that of `T`. | ||
// Proof sketch: | ||
// (1) From `&[T]` we can obtain `&T`, hence align([T]) >= align(T). | ||
// (2) Using `std::array::from_ref(&T)` we can obtain `&[T; 1]` (and thus `&[T]`), | ||
// hence align(T) >= align([T]). | ||
// Therefore align([T]) == align(T). Length does not affect alignment. | ||
|
||
// Try to determine alignment from the type structure | ||
if let Some(element_align) = get_element_alignment(tcx, typing_env, ty) { | ||
element_align > pack | ||
} else { | ||
// If we still can't determine alignment, conservatively assume disaligned | ||
true | ||
} | ||
} | ||
_ => { | ||
// We cannot figure out the layout. Conservatively assume that this is disaligned. | ||
debug!("is_disaligned({:?}) - true", place); | ||
true | ||
} | ||
} | ||
|
||
// For arrays/slices, `align([T]) == align(T)` (independent of length). | ||
// So if layout_of([T; N]) is unavailable, we can fall back to layout_of(T). | ||
fn get_element_alignment<'tcx>( | ||
tcx: TyCtxt<'tcx>, | ||
typing_env: ty::TypingEnv<'tcx>, | ||
ty: Ty<'tcx>, | ||
) -> Option<Align> { | ||
match ty.kind() { | ||
ty::Array(elem_ty, _) | ty::Slice(elem_ty) => { | ||
// Try to obtain the element's layout; if we can, use its ABI align. | ||
match tcx.layout_of(typing_env.as_query_input(*elem_ty)) { | ||
Comment on lines
+74
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend inlining this function at its only call site, this seems unnecessarily verbose. If you want to make a helper function, it'd be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still relevant, I would personally prefer adding a single |
||
Ok(layout) => Some(layout.align.abi), | ||
Err(_) => None, // stay conservative when even the element's layout is unknown | ||
} | ||
} | ||
_ => None, | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
//! Borrowing from packed arrays with generic length: | ||
//! - allowed if ABI align([T]) == align(T) <= packed alignment | ||
//! - hard error (E0793) otherwise | ||
#![allow(dead_code, unused_variables, unused_mut)] | ||
use std::mem::MaybeUninit; | ||
use std::num::{NonZeroU8, NonZeroU16}; | ||
|
||
// | ||
// -------- PASS CASES -------- | ||
// | ||
|
||
mod pass_u8 { | ||
#[repr(C, packed)] | ||
pub struct PascalString<const N: usize> { | ||
len: u8, | ||
buf: [u8; N], | ||
} | ||
|
||
pub fn bar<const N: usize>(s: &PascalString<N>) -> &str { | ||
// should NOT trigger E0793 | ||
std::str::from_utf8(&s.buf[0..s.len as usize]).unwrap() | ||
} | ||
|
||
pub fn run() { | ||
let p = PascalString::<10> { len: 3, buf: *b"abc\0\0\0\0\0\0\0" }; | ||
let s = bar(&p); | ||
assert_eq!(s, "abc"); | ||
} | ||
Comment on lines
+24
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no reason I can see for this function to exist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still relevant |
||
} | ||
|
||
mod pass_i8 { | ||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [i8; N], | ||
} | ||
pub fn run() { | ||
let s = S::<4> { buf: [1, 2, 3, 4] }; | ||
let _ok = &s.buf[..]; // no E0793 | ||
} | ||
} | ||
|
||
mod pass_nonzero_u8 { | ||
use super::*; | ||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [NonZeroU8; N], | ||
} | ||
pub fn run() { | ||
let s = S::<3> { | ||
buf: [ | ||
NonZeroU8::new(1).unwrap(), | ||
NonZeroU8::new(2).unwrap(), | ||
NonZeroU8::new(3).unwrap(), | ||
], | ||
}; | ||
let _ok = &s.buf[..]; // no E0793 | ||
} | ||
} | ||
|
||
mod pass_maybeuninit_u8 { | ||
use super::*; | ||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [MaybeUninit<u8>; N], | ||
} | ||
pub fn run() { | ||
let s = S::<2> { buf: [MaybeUninit::new(1), MaybeUninit::new(2)] }; | ||
let _ok = &s.buf[..]; // no E0793 | ||
} | ||
} | ||
|
||
mod pass_transparent_u8 { | ||
#[repr(transparent)] | ||
pub struct WrapU8(u8); | ||
|
||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [WrapU8; N], | ||
} | ||
pub fn run() { | ||
let s = S::<2> { buf: [WrapU8(1), WrapU8(2)] }; | ||
let _ok = &s.buf[..]; // no E0793 | ||
} | ||
} | ||
Comment on lines
+31
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of these already pass before your PR, so they don't actually test what you want them to test. You need to make the |
||
|
||
// | ||
// -------- FAIL CASES (expect E0793) -------- | ||
// | ||
|
||
mod fail_u16 { | ||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [u16; N], | ||
} | ||
pub fn run() { | ||
let s = S::<2> { buf: [1, 2] }; | ||
let _err = &s.buf[..]; | ||
//~^ ERROR: reference to packed field is unaligned | ||
} | ||
} | ||
|
||
mod fail_nonzero_u16 { | ||
use super::*; | ||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [NonZeroU16; N], | ||
} | ||
pub fn run() { | ||
let s = S::<1> { buf: [NonZeroU16::new(1).unwrap()] }; | ||
let _err = &s.buf[..]; | ||
//~^ ERROR: reference to packed field is unaligned | ||
} | ||
} | ||
|
||
mod fail_transparent_u16 { | ||
#[repr(transparent)] | ||
pub struct WrapU16(u16); | ||
|
||
#[repr(C, packed)] | ||
pub struct S<const N: usize> { | ||
buf: [WrapU16; N], | ||
} | ||
pub fn run() { | ||
let s = S::<1> { buf: [WrapU16(42)] }; | ||
let _err = &s.buf[..]; | ||
//~^ ERROR: reference to packed field is unaligned | ||
} | ||
} | ||
|
||
fn main() { | ||
// Run pass cases (fail cases only check diagnostics) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These never get run anyway since the file fails to compile, so this is quite pointless. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would split this test into a pass and a fail version. Having them in the same test may not actually test the pass cases as we often end up hiding errors if others have been emitted. I don't have an opinion on whether to make that a |
||
pass_u8::run(); | ||
pass_i8::run(); | ||
pass_nonzero_u8::run(); | ||
pass_maybeuninit_u8::run(); | ||
pass_transparent_u8::run(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
error[E0793]: reference to packed field is unaligned | ||
--> $DIR/packed-array-generic-length.rs:97:21 | ||
| | ||
LL | let _err = &s.buf[..]; | ||
| ^^^^^ | ||
| | ||
= note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses | ||
= note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced) | ||
= help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers) | ||
|
||
error[E0793]: reference to packed field is unaligned | ||
--> $DIR/packed-array-generic-length.rs:110:21 | ||
| | ||
LL | let _err = &s.buf[..]; | ||
| ^^^^^ | ||
| | ||
= note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses | ||
= note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced) | ||
= help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers) | ||
|
||
error[E0793]: reference to packed field is unaligned | ||
--> $DIR/packed-array-generic-length.rs:125:21 | ||
| | ||
LL | let _err = &s.buf[..]; | ||
| ^^^^^ | ||
| | ||
= note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses | ||
= note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced) | ||
= help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers) | ||
|
||
error: aborting due to 3 previous errors | ||
|
||
For more information about this error, try `rustc --explain E0793`. |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think referencing standard library operations here makes much sense. Maybe we can reference the Reference wherever it defines the alignment of arrays, but TBH that doesn't seem necessary. What's relevant is to explicitly invoke the fact o that arrays are aligned like their element type, that's the one key reasoning step needed here. Let's not drown that in noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still relevant