From 61f307d3e80d2f47e44f4430494ef4ad08a1defc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 25 Jul 2022 11:33:21 +0200 Subject: [PATCH 1/3] codegen: Don't generate conflicting packed() and align() representation hints. Fixes #2240 --- src/codegen/mod.rs | 4 +- .../struct_with_anon_struct_array_float.rs | 1 - .../tests/packed-align-conflict.rs | 38 +++++++++++++++++++ tests/headers/packed-align-conflict.h | 3 ++ 4 files changed, 44 insertions(+), 2 deletions(-) delete mode 100644 tests/expectations/struct_with_anon_struct_array_float.rs create mode 100644 tests/expectations/tests/packed-align-conflict.rs create mode 100644 tests/headers/packed-align-conflict.h diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index 6bbb7ab591..72bea7ea59 100644 --- a/src/codegen/mod.rs +++ b/src/codegen/mod.rs @@ -2025,7 +2025,9 @@ impl CodeGenerator for CompInfo { attributes.push(attributes::repr("C")); } - if ctx.options().rust_features().repr_align { + if ctx.options().rust_features().repr_align && !packed { + // We can't specify both packed(N) and align(N), but the align() + // should be redundant in this case. if let Some(explicit) = explicit_align { // Ensure that the struct has the correct alignment even in // presence of alignas. diff --git a/tests/expectations/struct_with_anon_struct_array_float.rs b/tests/expectations/struct_with_anon_struct_array_float.rs deleted file mode 100644 index 8b13789179..0000000000 --- a/tests/expectations/struct_with_anon_struct_array_float.rs +++ /dev/null @@ -1 +0,0 @@ - diff --git a/tests/expectations/tests/packed-align-conflict.rs b/tests/expectations/tests/packed-align-conflict.rs new file mode 100644 index 0000000000..e78e5f9c75 --- /dev/null +++ b/tests/expectations/tests/packed-align-conflict.rs @@ -0,0 +1,38 @@ +#![allow( + dead_code, + non_snake_case, + non_camel_case_types, + non_upper_case_globals +)] + +#[repr(C, packed(2))] +#[derive(Debug, Default, Copy, Clone)] +pub struct FndrOpaqueInfo { + pub opaque: [::std::os::raw::c_char; 16usize], +} +#[test] +fn bindgen_test_layout_FndrOpaqueInfo() { + const UNINIT: ::std::mem::MaybeUninit = + ::std::mem::MaybeUninit::uninit(); + let ptr = UNINIT.as_ptr(); + assert_eq!( + ::std::mem::size_of::(), + 16usize, + concat!("Size of: ", stringify!(FndrOpaqueInfo)) + ); + assert_eq!( + ::std::mem::align_of::(), + 2usize, + concat!("Alignment of ", stringify!(FndrOpaqueInfo)) + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).opaque) as usize - ptr as usize }, + 0usize, + concat!( + "Offset of field: ", + stringify!(FndrOpaqueInfo), + "::", + stringify!(opaque) + ) + ); +} diff --git a/tests/headers/packed-align-conflict.h b/tests/headers/packed-align-conflict.h new file mode 100644 index 0000000000..aca17a64b4 --- /dev/null +++ b/tests/headers/packed-align-conflict.h @@ -0,0 +1,3 @@ +struct FndrOpaqueInfo { + char opaque[16]; +} __attribute__((aligned(2), packed)); From 3785a8ee6875fd68f9f3f9efe4109a2d16bcf59f Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Thu, 28 Jul 2022 04:41:52 +0900 Subject: [PATCH 2/3] poc packed wrapper --- src/codegen/mod.rs | 61 ++++++++++++++----- src/codegen/struct_layout.rs | 2 +- .../tests/issue-537-repr-packed-n.rs | 18 ++++-- tests/expectations/tests/issue-537.rs | 18 ++++-- .../tests/packed-align-conflict.rs | 46 ++++++++------ tests/headers/packed-align-conflict.h | 9 ++- 6 files changed, 106 insertions(+), 48 deletions(-) diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index 72bea7ea59..a686bdec32 100644 --- a/src/codegen/mod.rs +++ b/src/codegen/mod.rs @@ -2009,13 +2009,22 @@ impl CodeGenerator for CompInfo { let mut needs_default_impl = false; let mut needs_debug_impl = false; let mut needs_partialeq_impl = false; + let mut needs_packed_wrapper = false; if let Some(comment) = item.comment(ctx) { attributes.push(attributes::doc(comment)); } + + // We can't specify both packed(N) and align(N), but the packed() + // should be redundant in this case. if packed && !is_opaque { let n = layout.map_or(1, |l| l.align); assert!(ctx.options().rust_features().repr_packed_n || n == 1); - let packed_repr = if n == 1 { + if ctx.options().rust_features().repr_align && + explicit_align.is_some() + { + needs_packed_wrapper = true; + } + let packed_repr = if n == 1 || needs_packed_wrapper { "packed".to_string() } else { format!("packed({})", n) @@ -2023,18 +2032,16 @@ impl CodeGenerator for CompInfo { attributes.push(attributes::repr_list(&["C", &packed_repr])); } else { attributes.push(attributes::repr("C")); - } - if ctx.options().rust_features().repr_align && !packed { - // We can't specify both packed(N) and align(N), but the align() - // should be redundant in this case. - if let Some(explicit) = explicit_align { - // Ensure that the struct has the correct alignment even in - // presence of alignas. - let explicit = helpers::ast_ty::int_expr(explicit as i64); - attributes.push(quote! { - #[repr(align(#explicit))] - }); + if ctx.options().rust_features().repr_align { + if let Some(explicit) = explicit_align { + // Ensure that the struct has the correct alignment even in + // presence of alignas. + let explicit = helpers::ast_ty::int_expr(explicit as i64); + attributes.push(quote! { + #[repr(align(#explicit))] + }); + } } } @@ -2088,15 +2095,21 @@ impl CodeGenerator for CompInfo { attributes.push(attributes::must_use()); } + let layout_ident = if needs_packed_wrapper { + ctx.rust_ident(canonical_name.to_owned() + "__packed") + } else { + canonical_ident.clone() + }; + let mut tokens = if is_union && struct_layout.is_rust_union() { quote! { #( #attributes )* - pub union #canonical_ident + pub union #layout_ident } } else { quote! { #( #attributes )* - pub struct #canonical_ident + pub struct #layout_ident } }; @@ -2107,6 +2120,19 @@ impl CodeGenerator for CompInfo { }); result.push(tokens); + if needs_packed_wrapper { + let attributes = attributes::derives(&derives); + let align = proc_macro2::TokenStream::from_str( + &explicit_align.unwrap().to_string(), + ) + .unwrap(); + result.push(quote! { + #attributes + #[repr(C, align(#align))] + pub struct #canonical_ident(pub #layout_ident); + }); + } + // Generate the inner types and all that stuff. // // TODO: In the future we might want to be smart, and use nested @@ -2206,12 +2232,17 @@ impl CodeGenerator for CompInfo { let uninit_decl = if !check_field_offset.is_empty() { // FIXME: When MSRV >= 1.59.0, we can use // > const PTR: *const #canonical_ident = ::#prefix::mem::MaybeUninit::uninit().as_ptr(); + let layout_cast = if needs_packed_wrapper { + Some(quote!(as *const #layout_ident)) + } else { + None + }; Some(quote! { // Use a shared MaybeUninit so that rustc with // opt-level=0 doesn't take too much stack space, // see #2218. const UNINIT: ::#prefix::mem::MaybeUninit<#canonical_ident> = ::#prefix::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr()#layout_cast; }) } else { None diff --git a/src/codegen/struct_layout.rs b/src/codegen/struct_layout.rs index 657be0b489..d8a785cbe8 100644 --- a/src/codegen/struct_layout.rs +++ b/src/codegen/struct_layout.rs @@ -365,7 +365,7 @@ impl<'a> StructLayoutTracker<'a> { return true; } - if self.max_field_align >= layout.align { + if !self.is_packed && self.max_field_align >= layout.align { return false; } diff --git a/tests/expectations/tests/issue-537-repr-packed-n.rs b/tests/expectations/tests/issue-537-repr-packed-n.rs index b1e42ca72a..6d8bec7fdc 100644 --- a/tests/expectations/tests/issue-537-repr-packed-n.rs +++ b/tests/expectations/tests/issue-537-repr-packed-n.rs @@ -40,16 +40,19 @@ fn bindgen_test_layout_AlignedToOne() { ); } /// This should be be packed because Rust 1.33 has `#[repr(packed(N))]`. -#[repr(C, packed(2))] +#[repr(C, packed)] #[derive(Debug, Default, Copy, Clone)] -pub struct AlignedToTwo { +pub struct AlignedToTwo__packed { pub i: ::std::os::raw::c_int, } +#[derive(Debug, Default, Copy, Clone)] +#[repr(C, align(2))] +pub struct AlignedToTwo(pub AlignedToTwo__packed); #[test] fn bindgen_test_layout_AlignedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr() as *const AlignedToTwo__packed; assert_eq!( ::std::mem::size_of::(), 4usize, @@ -117,17 +120,20 @@ fn bindgen_test_layout_PackedToOne() { ); } /// This should be be packed because Rust 1.33 has `#[repr(packed(N))]`. -#[repr(C, packed(2))] +#[repr(C, packed)] #[derive(Debug, Default, Copy, Clone)] -pub struct PackedToTwo { +pub struct PackedToTwo__packed { pub x: ::std::os::raw::c_int, pub y: ::std::os::raw::c_int, } +#[derive(Debug, Default, Copy, Clone)] +#[repr(C, align(2))] +pub struct PackedToTwo(pub PackedToTwo__packed); #[test] fn bindgen_test_layout_PackedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr() as *const PackedToTwo__packed; assert_eq!( ::std::mem::size_of::(), 8usize, diff --git a/tests/expectations/tests/issue-537.rs b/tests/expectations/tests/issue-537.rs index e2be9d63ba..1e840a28cc 100644 --- a/tests/expectations/tests/issue-537.rs +++ b/tests/expectations/tests/issue-537.rs @@ -40,16 +40,19 @@ fn bindgen_test_layout_AlignedToOne() { } /// This should be opaque because although we can see the attributes, Rust before /// 1.33 doesn't have `#[repr(packed(N))]`. -#[repr(C, packed(2))] +#[repr(C, packed)] #[derive(Debug, Default, Copy, Clone)] -pub struct AlignedToTwo { +pub struct AlignedToTwo__packed { pub i: ::std::os::raw::c_int, } +#[derive(Debug, Default, Copy, Clone)] +#[repr(C, align(2))] +pub struct AlignedToTwo(pub AlignedToTwo__packed); #[test] fn bindgen_test_layout_AlignedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr() as *const AlignedToTwo__packed; assert_eq!( ::std::mem::size_of::(), 4usize, @@ -119,17 +122,20 @@ fn bindgen_test_layout_PackedToOne() { /// In this case, even if we can detect the weird alignment triggered by /// `#pragma pack(2)`, we can't do anything about it because Rust before 1.33 /// doesn't have `#[repr(packed(N))]`. Therefore, we must make it opaque. -#[repr(C, packed(2))] +#[repr(C, packed)] #[derive(Debug, Default, Copy, Clone)] -pub struct PackedToTwo { +pub struct PackedToTwo__packed { pub x: ::std::os::raw::c_int, pub y: ::std::os::raw::c_int, } +#[derive(Debug, Default, Copy, Clone)] +#[repr(C, align(2))] +pub struct PackedToTwo(pub PackedToTwo__packed); #[test] fn bindgen_test_layout_PackedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr() as *const PackedToTwo__packed; assert_eq!( ::std::mem::size_of::(), 8usize, diff --git a/tests/expectations/tests/packed-align-conflict.rs b/tests/expectations/tests/packed-align-conflict.rs index e78e5f9c75..c57fbc6fbf 100644 --- a/tests/expectations/tests/packed-align-conflict.rs +++ b/tests/expectations/tests/packed-align-conflict.rs @@ -5,34 +5,44 @@ non_upper_case_globals )] -#[repr(C, packed(2))] +#[repr(C, packed)] #[derive(Debug, Default, Copy, Clone)] -pub struct FndrOpaqueInfo { - pub opaque: [::std::os::raw::c_char; 16usize], +pub struct B__packed { + pub a: u8, + pub b: u16, + pub c: u8, } +#[derive(Debug, Default, Copy, Clone)] +#[repr(C, align(2))] +pub struct B(pub B__packed); #[test] -fn bindgen_test_layout_FndrOpaqueInfo() { - const UNINIT: ::std::mem::MaybeUninit = +fn bindgen_test_layout_B() { + const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr(); + let ptr = UNINIT.as_ptr() as *const B__packed; assert_eq!( - ::std::mem::size_of::(), - 16usize, - concat!("Size of: ", stringify!(FndrOpaqueInfo)) + ::std::mem::size_of::(), + 4usize, + concat!("Size of: ", stringify!(B)) ); assert_eq!( - ::std::mem::align_of::(), + ::std::mem::align_of::(), 2usize, - concat!("Alignment of ", stringify!(FndrOpaqueInfo)) + concat!("Alignment of ", stringify!(B)) ); assert_eq!( - unsafe { ::std::ptr::addr_of!((*ptr).opaque) as usize - ptr as usize }, + unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize }, 0usize, - concat!( - "Offset of field: ", - stringify!(FndrOpaqueInfo), - "::", - stringify!(opaque) - ) + concat!("Offset of field: ", stringify!(B), "::", stringify!(a)) + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize }, + 1usize, + concat!("Offset of field: ", stringify!(B), "::", stringify!(b)) + ); + assert_eq!( + unsafe { ::std::ptr::addr_of!((*ptr).c) as usize - ptr as usize }, + 3usize, + concat!("Offset of field: ", stringify!(B), "::", stringify!(c)) ); } diff --git a/tests/headers/packed-align-conflict.h b/tests/headers/packed-align-conflict.h index aca17a64b4..e1ac00904d 100644 --- a/tests/headers/packed-align-conflict.h +++ b/tests/headers/packed-align-conflict.h @@ -1,3 +1,8 @@ -struct FndrOpaqueInfo { - char opaque[16]; +typedef unsigned char uint8_t; +typedef unsigned short uint16_t; + +struct B { + uint8_t a; + uint16_t b; + uint8_t c; } __attribute__((aligned(2), packed)); From 8f31017ee80d8afcaa2b1c503e3fd4877d42aefa Mon Sep 17 00:00:00 2001 From: Jeong YunWon Date: Fri, 29 Jul 2022 05:57:24 +0900 Subject: [PATCH 3/3] XXX: I am not understanding how things going. don't merge this commit --- src/codegen/struct_layout.rs | 13 +++++++++++-- .../tests/issue-537-repr-packed-n.rs | 18 ++++++------------ tests/expectations/tests/issue-537.rs | 18 ++++++------------ 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/codegen/struct_layout.rs b/src/codegen/struct_layout.rs index d8a785cbe8..9e47dbd412 100644 --- a/src/codegen/struct_layout.rs +++ b/src/codegen/struct_layout.rs @@ -24,6 +24,7 @@ pub struct StructLayoutTracker<'a> { padding_count: usize, latest_field_layout: Option, max_field_align: usize, + has_bitfield: bool, last_field_was_bitfield: bool, } @@ -103,6 +104,7 @@ impl<'a> StructLayoutTracker<'a> { padding_count: 0, latest_field_layout: None, max_field_align: 0, + has_bitfield: false, last_field_was_bitfield: false, } } @@ -145,6 +147,7 @@ impl<'a> StructLayoutTracker<'a> { ); self.latest_field_layout = Some(layout); + self.has_bitfield = true; self.last_field_was_bitfield = true; // NB: We intentionally don't update the max_field_align here, since our // bitfields code doesn't necessarily guarantee it, so we need to @@ -365,8 +368,14 @@ impl<'a> StructLayoutTracker<'a> { return true; } - if !self.is_packed && self.max_field_align >= layout.align { - return false; + if self.is_packed && !self.has_bitfield { + if self.max_field_align > layout.align { + return false; + } + } else { + if self.max_field_align >= layout.align { + return false; + } } // We can only generate up-to a 8-bytes of alignment unless we support diff --git a/tests/expectations/tests/issue-537-repr-packed-n.rs b/tests/expectations/tests/issue-537-repr-packed-n.rs index 6d8bec7fdc..b1e42ca72a 100644 --- a/tests/expectations/tests/issue-537-repr-packed-n.rs +++ b/tests/expectations/tests/issue-537-repr-packed-n.rs @@ -40,19 +40,16 @@ fn bindgen_test_layout_AlignedToOne() { ); } /// This should be be packed because Rust 1.33 has `#[repr(packed(N))]`. -#[repr(C, packed)] +#[repr(C, packed(2))] #[derive(Debug, Default, Copy, Clone)] -pub struct AlignedToTwo__packed { +pub struct AlignedToTwo { pub i: ::std::os::raw::c_int, } -#[derive(Debug, Default, Copy, Clone)] -#[repr(C, align(2))] -pub struct AlignedToTwo(pub AlignedToTwo__packed); #[test] fn bindgen_test_layout_AlignedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr() as *const AlignedToTwo__packed; + let ptr = UNINIT.as_ptr(); assert_eq!( ::std::mem::size_of::(), 4usize, @@ -120,20 +117,17 @@ fn bindgen_test_layout_PackedToOne() { ); } /// This should be be packed because Rust 1.33 has `#[repr(packed(N))]`. -#[repr(C, packed)] +#[repr(C, packed(2))] #[derive(Debug, Default, Copy, Clone)] -pub struct PackedToTwo__packed { +pub struct PackedToTwo { pub x: ::std::os::raw::c_int, pub y: ::std::os::raw::c_int, } -#[derive(Debug, Default, Copy, Clone)] -#[repr(C, align(2))] -pub struct PackedToTwo(pub PackedToTwo__packed); #[test] fn bindgen_test_layout_PackedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr() as *const PackedToTwo__packed; + let ptr = UNINIT.as_ptr(); assert_eq!( ::std::mem::size_of::(), 8usize, diff --git a/tests/expectations/tests/issue-537.rs b/tests/expectations/tests/issue-537.rs index 1e840a28cc..e2be9d63ba 100644 --- a/tests/expectations/tests/issue-537.rs +++ b/tests/expectations/tests/issue-537.rs @@ -40,19 +40,16 @@ fn bindgen_test_layout_AlignedToOne() { } /// This should be opaque because although we can see the attributes, Rust before /// 1.33 doesn't have `#[repr(packed(N))]`. -#[repr(C, packed)] +#[repr(C, packed(2))] #[derive(Debug, Default, Copy, Clone)] -pub struct AlignedToTwo__packed { +pub struct AlignedToTwo { pub i: ::std::os::raw::c_int, } -#[derive(Debug, Default, Copy, Clone)] -#[repr(C, align(2))] -pub struct AlignedToTwo(pub AlignedToTwo__packed); #[test] fn bindgen_test_layout_AlignedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr() as *const AlignedToTwo__packed; + let ptr = UNINIT.as_ptr(); assert_eq!( ::std::mem::size_of::(), 4usize, @@ -122,20 +119,17 @@ fn bindgen_test_layout_PackedToOne() { /// In this case, even if we can detect the weird alignment triggered by /// `#pragma pack(2)`, we can't do anything about it because Rust before 1.33 /// doesn't have `#[repr(packed(N))]`. Therefore, we must make it opaque. -#[repr(C, packed)] +#[repr(C, packed(2))] #[derive(Debug, Default, Copy, Clone)] -pub struct PackedToTwo__packed { +pub struct PackedToTwo { pub x: ::std::os::raw::c_int, pub y: ::std::os::raw::c_int, } -#[derive(Debug, Default, Copy, Clone)] -#[repr(C, align(2))] -pub struct PackedToTwo(pub PackedToTwo__packed); #[test] fn bindgen_test_layout_PackedToTwo() { const UNINIT: ::std::mem::MaybeUninit = ::std::mem::MaybeUninit::uninit(); - let ptr = UNINIT.as_ptr() as *const PackedToTwo__packed; + let ptr = UNINIT.as_ptr(); assert_eq!( ::std::mem::size_of::(), 8usize,