-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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 8 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,6 +1,6 @@ | ||
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 | ||
|
@@ -23,33 +23,79 @@ where | |
|
||
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) | ||
|
||
// Try to normalize the type to resolve any generic parameters | ||
let normalized_ty = match tcx.try_normalize_erasing_regions(typing_env, ty) { | ||
Ok(normalized) => normalized, | ||
Err(_) => { | ||
// If normalization fails, fall back to the original type | ||
ty | ||
} | ||
}; | ||
|
||
match tcx.layout_of(typing_env.as_query_input(normalized_ty)) { | ||
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_disaligned({:?}) - align = {}, packed = {}; not disaligned", | ||
place, | ||
layout.align.abi.bytes(), | ||
pack.bytes() | ||
); | ||
false | ||
} else { | ||
true | ||
} | ||
} | ||
_ => { | ||
// We cannot figure out the layout. Conservatively assume that this is disaligned. | ||
debug!("is_disaligned({:?}) - true", place); | ||
true | ||
Err(_) => { | ||
// We cannot figure out the layout. This often happens with generic types. | ||
// For const generic arrays like [u8; CAP], we can make a reasonable assumption | ||
// about their alignment based on the element type. | ||
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. we're not making a reasonable assumption. Their layout literally only depend son the layout of their element type, doesn't it? Getting this right is soundness critical, so we need to be confident about that we don#t introduce false negatives. Also, please rename this function to 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. We use "misaligned" elsewhere in the compiler, maybe let's go with that instead of "disaligned" (which I never saw/heard before). 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. also still relevant 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. Thanks for the detailed feedback! I agree with the soundness concern and with using the term “misaligned”.
To still address the motivating case without introducing false negatives, I’ll only add a very small, layout-free special-case: This fixes the If you’d prefer the even stricter variant (no special-casing at all), I can drop the exception and keep returning 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 think ignoring the length and only checking the argument is fine. My issue was with the comment, not the behavior. We already guarantee that the alignment constraints are equal: you can get a you can get a 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. Thanks! I’ve updated the comment to formally justify that align([T]) == align(T), |
||
|
||
// Try to determine alignment from the type structure | ||
if let Some(element_align) = get_element_alignment(tcx, normalized_ty) { | ||
element_align > pack | ||
} else { | ||
// If we still can't determine alignment, conservatively assume disaligned | ||
true | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// Try to determine the alignment of an array element type | ||
fn get_element_alignment<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<Align> { | ||
match ty.kind() { | ||
ty::Array(element_ty, _) => { | ||
oli-obk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// For arrays, the alignment is the same as the element type | ||
let param_env = ty::ParamEnv::empty(); | ||
let typing_env = | ||
ty::TypingEnv { typing_mode: ty::TypingMode::non_body_analysis(), param_env }; | ||
match tcx.layout_of(typing_env.as_query_input(*element_ty)) { | ||
Ok(layout) => Some(layout.align.abi), | ||
Err(_) => None, | ||
} | ||
} | ||
ty::Slice(element_ty) => { | ||
// For slices, the alignment is the same as the element type | ||
let param_env = ty::ParamEnv::empty(); | ||
let typing_env = | ||
ty::TypingEnv { typing_mode: ty::TypingMode::non_body_analysis(), param_env }; | ||
match tcx.layout_of(typing_env.as_query_input(*element_ty)) { | ||
Ok(layout) => Some(layout.align.abi), | ||
Err(_) => None, | ||
} | ||
} | ||
_ => None, | ||
} | ||
} | ||
pub fn is_within_packed<'tcx, L>( | ||
tcx: TyCtxt<'tcx>, | ||
local_decls: &L, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
//@ check-pass | ||
#![allow(dead_code)] | ||
|
||
#[repr(C, packed)] | ||
struct PascalString<const CAP: usize> { | ||
len: u8, | ||
buf: [u8; CAP], | ||
} | ||
|
||
fn bar<const CAP: usize>(s: &PascalString<CAP>) -> &str { | ||
// 目标:这行不应触发 E0793 | ||
oli-obk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
std::str::from_utf8(&s.buf[0..s.len as usize]).unwrap() | ||
} | ||
|
||
fn main() { | ||
let p = PascalString::<10> { len: 3, buf: *b"abc\0\0\0\0\0\0\0" }; | ||
let s = bar(&p); | ||
assert_eq!(s, "abc"); | ||
} |
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.
why? it feels a lot safer to conservatively return
true
here, same as whenlayout_of
failsThere 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