Skip to content

Commit

Permalink
Detect #pragma pack(...) and make pack(n) where n > 1 opaque
Browse files Browse the repository at this point in the history
This is a bandaid for #537. It does *not* fix the underlying issue, which
requires `#[repr(packed = "N")]` support in Rust. However, it does make sure
that we don't generate type definitions with the wrong layout, or fail our
generated layout tests.
  • Loading branch information
fitzgen committed Nov 2, 2017
1 parent f059ede commit 79dbe8b
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 63 deletions.
8 changes: 5 additions & 3 deletions src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,9 @@ impl CodeGenerator for CompInfo {

let used_template_params = item.used_template_params(ctx);

let mut packed = self.packed();
let ty = item.expect_type();
let layout = ty.layout(ctx);
let mut packed = self.is_packed(ctx, &layout);

// generate tuple struct if struct or union is a forward declaration,
// skip for now if template parameters are needed.
Expand Down Expand Up @@ -1431,7 +1433,7 @@ impl CodeGenerator for CompInfo {
let is_opaque = item.is_opaque(ctx, &());
let mut fields = vec![];
let mut struct_layout =
StructLayoutTracker::new(ctx, self, &canonical_name);
StructLayoutTracker::new(ctx, self, ty, &canonical_name);

if !is_opaque {
if item.has_vtable_ptr(ctx) {
Expand Down Expand Up @@ -1618,7 +1620,7 @@ impl CodeGenerator for CompInfo {
if let Some(comment) = item.comment(ctx) {
attributes.push(attributes::doc(comment));
}
if packed {
if packed && !is_opaque {
attributes.push(attributes::repr_list(&["C", "packed"]));
} else {
attributes.push(attributes::repr("C"));
Expand Down
9 changes: 6 additions & 3 deletions src/codegen/struct_layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub struct StructLayoutTracker<'a> {
name: &'a str,
ctx: &'a BindgenContext,
comp: &'a CompInfo,
is_packed: bool,
latest_offset: usize,
padding_count: usize,
latest_field_layout: Option<Layout>,
Expand Down Expand Up @@ -81,12 +82,14 @@ impl<'a> StructLayoutTracker<'a> {
pub fn new(
ctx: &'a BindgenContext,
comp: &'a CompInfo,
ty: &'a Type,
name: &'a str,
) -> Self {
StructLayoutTracker {
name: name,
ctx: ctx,
comp: comp,
is_packed: comp.is_packed(ctx, &ty.layout(ctx)),
latest_offset: 0,
padding_count: 0,
latest_field_layout: None,
Expand Down Expand Up @@ -180,7 +183,7 @@ impl<'a> StructLayoutTracker<'a> {

let will_merge_with_bitfield = self.align_to_latest_field(field_layout);

let padding_layout = if self.comp.packed() {
let padding_layout = if self.is_packed {
None
} else {
let padding_bytes = match field_offset {
Expand Down Expand Up @@ -269,7 +272,7 @@ impl<'a> StructLayoutTracker<'a> {
self.latest_field_layout.unwrap().align) ||
layout.align > mem::size_of::<*mut ()>())
{
let layout = if self.comp.packed() {
let layout = if self.is_packed {
Layout::new(padding_bytes, 1)
} else if self.last_field_was_bitfield ||
layout.align > mem::size_of::<*mut ()>()
Expand Down Expand Up @@ -316,7 +319,7 @@ impl<'a> StructLayoutTracker<'a> {
///
/// This is just to avoid doing the same check also in pad_field.
fn align_to_latest_field(&mut self, new_field_layout: Layout) -> bool {
if self.comp.packed() {
if self.is_packed {
// Skip to align fields when packed.
return false;
}
Expand Down
67 changes: 54 additions & 13 deletions src/ir/comp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -974,8 +974,8 @@ pub struct CompInfo {
/// size_t)
has_non_type_template_params: bool,

/// Whether this struct layout is packed.
packed: bool,
/// Whether we saw `__attribute__((packed))` on or within this type.
packed_attr: bool,

/// Used to know if we've found an opaque attribute that could cause us to
/// generate a type with invalid layout. This is explicitly used to avoid us
Expand Down Expand Up @@ -1007,7 +1007,7 @@ impl CompInfo {
has_destructor: false,
has_nonempty_base: false,
has_non_type_template_params: false,
packed: false,
packed_attr: false,
found_unknown_attr: false,
is_forward_declaration: false,
}
Expand Down Expand Up @@ -1261,7 +1261,7 @@ impl CompInfo {
}
}
CXCursor_PackedAttr => {
ci.packed = true;
ci.packed_attr = true;
}
CXCursor_TemplateTypeParameter => {
let param = Item::type_param(None, cur, ctx)
Expand Down Expand Up @@ -1439,8 +1439,31 @@ impl CompInfo {
}

/// Is this compound type packed?
pub fn packed(&self) -> bool {
self.packed
pub fn is_packed(&self, ctx: &BindgenContext, layout: &Option<Layout>) -> bool {
if self.packed_attr {
return true
}

// Even though `libclang` doesn't expose `#pragma packed(...)`, we can
// detect it through its effects.
if let Some(ref parent_layout) = *layout {
if self.fields().iter().any(|f| match *f {
Field::Bitfields(ref unit) => {
unit.layout().align > parent_layout.align
}
Field::DataMember(ref data) => {
let field_ty = ctx.resolve_type(data.ty());
field_ty.layout(ctx).map_or(false, |field_ty_layout| {
field_ty_layout.align > parent_layout.align
})
}
}) {
info!("Found a struct that was defined within `#pragma packed(...)`");
return true;
}
}

false
}

/// Returns true if compound type has been forward declared
Expand Down Expand Up @@ -1504,8 +1527,8 @@ impl DotAttributes for CompInfo {
)?;
}

if self.packed {
writeln!(out, "<tr><td>packed</td><td>true</td></tr>")?;
if self.packed_attr {
writeln!(out, "<tr><td>packed_attr</td><td>true</td></tr>")?;
}

if self.is_forward_declaration {
Expand All @@ -1528,15 +1551,17 @@ impl DotAttributes for CompInfo {
}

impl IsOpaque for CompInfo {
type Extra = ();
type Extra = Option<Layout>;

fn is_opaque(&self, ctx: &BindgenContext, _: &()) -> bool {
// Early return to avoid extra computation
fn is_opaque(&self, ctx: &BindgenContext, layout: &Option<Layout>) -> bool {
if self.has_non_type_template_params {
return true
}

self.fields().iter().any(|f| match *f {
// Bitfields with a width that is larger than their unit's width have
// some strange things going on, and the best we can do is make the
// whole struct opaque.
if self.fields().iter().any(|f| match *f {
Field::DataMember(_) => {
false
},
Expand All @@ -1548,7 +1573,23 @@ impl IsOpaque for CompInfo {
bf.width() / 8 > bitfield_layout.size as u32
})
}
})
}) {
return true;
}

// We don't have `#[repr(packed = "N")]` in Rust yet, so the best we can
// do is make this struct opaque.
//
// See https://github.com/rust-lang-nursery/rust-bindgen/issues/537 and
// https://github.com/rust-lang/rust/issues/33158
if self.is_packed(ctx, layout) && layout.map_or(false, |l| l.align > 1) {
warn!("Found a type that is both packed and aligned to greater than \
1; Rust doesn't have `#[repr(packed = \"N\")]` yet, so we \
are treating it as opaque");
return true;
}

false
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ir/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ impl IsOpaque for Type {
TypeKind::TemplateInstantiation(ref inst) => {
inst.is_opaque(ctx, item)
}
TypeKind::Comp(ref comp) => comp.is_opaque(ctx, &()),
TypeKind::Comp(ref comp) => comp.is_opaque(ctx, &self.layout),
TypeKind::ResolvedTypeRef(to) => to.is_opaque(ctx, &()),
_ => false,
}
Expand Down
3 changes: 2 additions & 1 deletion tests/expectations/tests/divide-by-zero-in-struct-layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ impl WithBitfieldAndAttrPacked {
0
}
}
#[repr(C)]
#[repr(C, packed)]
#[derive(Debug, Default, Copy, Clone)]
pub struct WithBitfieldAndPacked {
pub _bitfield_1: [u8; 0usize],
pub a: ::std::os::raw::c_uint,
pub __bindgen_padding_0: u8,
}
impl WithBitfieldAndPacked {
#[inline]
Expand Down
48 changes: 6 additions & 42 deletions tests/expectations/tests/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,49 +5,8 @@


#[repr(C)]
#[derive(Default)]
pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>);
impl<T> __IncompleteArrayField<T> {
#[inline]
pub fn new() -> Self {
__IncompleteArrayField(::std::marker::PhantomData)
}
#[inline]
pub unsafe fn as_ptr(&self) -> *const T {
::std::mem::transmute(self)
}
#[inline]
pub unsafe fn as_mut_ptr(&mut self) -> *mut T {
::std::mem::transmute(self)
}
#[inline]
pub unsafe fn as_slice(&self, len: usize) -> &[T] {
::std::slice::from_raw_parts(self.as_ptr(), len)
}
#[inline]
pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
}
}
impl<T> ::std::fmt::Debug for __IncompleteArrayField<T> {
fn fmt(&self, fmt: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
fmt.write_str("__IncompleteArrayField")
}
}
impl<T> ::std::clone::Clone for __IncompleteArrayField<T> {
#[inline]
fn clone(&self) -> Self {
Self::new()
}
}
impl<T> ::std::marker::Copy for __IncompleteArrayField<T> {}
#[repr(C, packed)]
#[derive(Debug, Default)]
pub struct header {
pub proto: ::std::os::raw::c_char,
pub size: ::std::os::raw::c_uint,
pub data: __IncompleteArrayField<::std::os::raw::c_uchar>,
pub __bindgen_padding_0: [u8; 11usize],
pub _bindgen_opaque_blob: [u8; 16usize],
}
#[test]
fn bindgen_test_layout_header() {
Expand All @@ -57,3 +16,8 @@ fn bindgen_test_layout_header() {
concat!("Size of: ", stringify!(header))
);
}
impl Default for header {
fn default() -> Self {
unsafe { ::std::mem::zeroed() }
}
}
35 changes: 35 additions & 0 deletions tests/headers/issue-537.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/// This should not be opaque; we can see the attributes and can pack the
/// struct.
struct AlignedToOne {
int i;
} __attribute__ ((packed,aligned(1)));

/// This should be opaque because although we can see the attributes, Rust
/// doesn't have `#[repr(packed = "N")]` yet.
struct AlignedToTwo {
int i;
} __attribute__ ((packed,aligned(2)));

#pragma pack(1)

/// This should not be opaque because although `libclang` doesn't give us the
/// `#pragma pack(1)`, we can detect that alignment is 1 and add
/// `#[repr(packed)]` to the struct ourselves.
struct PackedToOne {
int x;
int y;
};

#pragma pack()

#pragma pack(2)

/// 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 doesn't have
/// `#[repr(packed = "N")]`. Therefore, we must make it opaque.
struct PackedToTwo {
int x;
int y;
};

#pragma pack()

0 comments on commit 79dbe8b

Please sign in to comment.