Skip to content

Fix test case from #2246 #2248

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

Closed
wants to merge 3 commits into from
Closed
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
59 changes: 46 additions & 13 deletions src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2009,30 +2009,39 @@ 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)
};
attributes.push(attributes::repr_list(&["C", &packed_repr]));
} else {
attributes.push(attributes::repr("C"));
}

if ctx.options().rust_features().repr_align {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing the feature-check for repr(align)?

Copy link
Author

Choose a reason for hiding this comment

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

accidently happened during multiple revision 😂

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))]
});
}
}
}

Expand Down Expand Up @@ -2086,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
}
};

Expand All @@ -2105,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))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better if it'd be #[repr(transparent)], that'd match the ABI exactly, though I don't think it would matter in practice.

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't work. we need align here but transparent seems doesn't accept other things.

error[E0692]: transparent struct cannot have other repr hints
  --> tests/packed-align-conflict.rs:16:8
   |
16 | #[repr(transparent, align(2))]
   |        

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
Expand Down Expand Up @@ -2204,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
Expand Down
13 changes: 11 additions & 2 deletions src/codegen/struct_layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub struct StructLayoutTracker<'a> {
padding_count: usize,
latest_field_layout: Option<Layout>,
max_field_align: usize,
has_bitfield: bool,
last_field_was_bitfield: bool,
}

Expand Down Expand Up @@ -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,
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -365,8 +368,14 @@ impl<'a> StructLayoutTracker<'a> {
return true;
}

if 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
Expand Down
1 change: 0 additions & 1 deletion tests/expectations/struct_with_anon_struct_array_float.rs

This file was deleted.

48 changes: 48 additions & 0 deletions tests/expectations/tests/packed-align-conflict.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions tests/headers/packed-align-conflict.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
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));