Improve struct alignment with padding bytes #468

flier · 2017-02-02T14:42:39Z

I known, it is a little tricky or dirty solution, I have to do a lot of calculate for alignment.

For example

typedef struct {
  long long __clang_max_align_nonce1
      __attribute__((__aligned__(__alignof__(long long))));
  long double __clang_max_align_nonce2
      __attribute__((__aligned__(__alignof__(long double))));
} max_align_t;

will be generated as

#[repr(C)]
#[derive(Debug, Copy)]
pub struct _bindgen_ty_1 {
    pub __clang_max_align_nonce1: ::std::os::raw::c_longlong,
    _padding_0: u64,
    pub __clang_max_align_nonce2: f64,
    _padding_1: u64,
}
#[test]
fn bindgen_test_layout__bindgen_ty_1() {
    assert_eq!(::std::mem::size_of::<_bindgen_ty_1>() , 32usize);
    assert_eq! (0usize , unsafe {
                & ( * ( 0 as * const _bindgen_ty_1 ) ) .
                __clang_max_align_nonce1 as * const _ as usize });
    assert_eq! (16usize , unsafe {
                & ( * ( 0 as * const _bindgen_ty_1 ) ) .
                __clang_max_align_nonce2 as * const _ as usize });
}

It may generate wrong layout when use template or multi virtual inheritance.

At least, it is a begining, pass all the test cases, and some complex struct in wild.

highfive · 2017-02-02T14:42:44Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-02-03T13:32:32Z

Just a quick update, I'm reviewing this, it's just kind of involved to do so :)

bors-servo · 2017-02-03T21:56:14Z

☔ The latest upstream changes (presumably #471) made this pull request unmergeable. Please resolve the merge conflicts.

emilio

Ok, looks pretty good! I have a few comments and questions here and there, but overall looks good, thanks!

emilio · 2017-02-04T10:53:59Z

src/codegen/mod.rs

+    }
+}
+
+struct StructLayout<'a, 'ctx: 'a> {


Let's move this to a helper module, maybe src/codegen/struct_layout.rs? That way it's easier to audit :)

Also, StructLayout isn't a super descriptive name, maybe StructLayoutTracker?

sure, codegen::struct_layout::StructLayoutTracker will be fine

emilio · 2017-02-04T10:56:02Z

src/codegen/mod.rs

+        };
+
+        if padding_bytes >= align || layout.align > mem::size_of::<* mut ()>()
+        {


nit: Brace on its own line.

move to use rustfmt on save

emilio · 2017-02-04T11:00:22Z

src/codegen/mod.rs

+    pub fn saw_base(&mut self, base_ty: &Type) {
+        if let Some(base_layout) = base_ty.layout(self.ctx) {
+            self.latest_offset += self.padding_bytes(base_layout) +
+                                    base_ty.calc_size(self.ctx).unwrap_or(base_layout.size);


is there any reason for not trusting base_layout.size here? If so, please add a comment, since it doesn't seem obvious to me.

Yes, we could trust base_layout.size here. In fact, I use base_ty.calc_size because I have tried to fix those virtual inheritance vtable padding issues, but I remove those code before submit, because it is a little complex, hard to cover everything in one patch :(

emilio · 2017-02-04T11:05:15Z

src/codegen/mod.rs

+                    )
+                };
+
+                let check_field_offset = if item.is_opaque(ctx) ||


Please add a FIXME comment pointing to #465 so people can understand the base_members check. It's probably worth moving this condition to its own variable. Something like:

let should_skip_field_offset_checks = item.is_opaque(ctx) || ...;

emilio · 2017-02-04T11:06:46Z

src/codegen/mod.rs

+                        field.name().and_then(|name| {
+                            field.offset().and_then(|offset| {
+                                let field_offset = offset / 8;
+                                let field_name = aster::AstBuilder::new().id(ctx.rust_mangle(name).into_owned());


I'd expect just:

let field_name = ctx.rust_mangle(name);

to work, doesn't it?

ctx.rust_mangle(name) is a string, but we need a ident, or

assert_eq! (0usize , unsafe { - & ( * ( 0 as * const Test ) ) . "helper" as * const _ - as usize }); + & ( * ( 0 as * const Test ) ) . helper as * const _ as + usize });

right, you can use ctx.rust_ident(&name) then :)

It works, thanks!

emilio · 2017-02-04T11:10:12Z

src/ir/comp.rs

-            let field_layout = ctx.resolve_type(field.ty)
-                .layout(ctx);
+    /// Compute the layout of this type.
+    pub fn calc_layout(&self, ctx: &BindgenContext) -> Option<Layout> {


I am unsure what's the purpose of this method. Are we getting better info that libclang? If so, please state above that we may be able to compute our layout even when libclang doesn't provide it.

libclang will give the struct size after alignment, but sometime we need padding bytes at the end, because Rust doesn't support alignment more than 8 bytes.

For example, in the layout_mbuf example, we need 24 padding bytes to ensure the rte_mbuf has the correct size.

struct rte_mbuf { MARKER cacheline0; ... /** Timesync flags for use with IEEE1588. */ uint16_t timesync; } __rte_cache_aligned;

#[repr(C)] #[derive(Debug, Copy)] pub struct rte_mbuf { pub cacheline0: MARKER, ... _padding_0: [u64; 3usize], }

CompInfo::calc_layout will do the dirty work to find out how many bytes we need padding at the end.

emilio · 2017-02-04T11:12:08Z

src/ir/comp.rs

@@ -532,7 +578,7 @@ impl CompInfo {
        cursor.visit(|cur| {
            if cur.kind() != CXCursor_FieldDecl {
                if let Some((ty, _)) = maybe_anonymous_struct_field {
-                    let field = Field::new(None, ty, None, None, None, false);
+                    let field = Field::new(None, ty, None, None, None, false, None);


This is not right, we should be able to know the field offset and track it in maybe_anonymous_struct_field.

Does the offset function return something when either called on the struct declaration (where we create the maybe_anonymous_struct_field?

sure, added the offset to maybe_anonymous_struct_field tuple

let mut maybe_anonymous_struct_field = None; cursor.visit(|cur| { if cur.kind() != CXCursor_FieldDecl { if let Some((ty, _, offset)) = maybe_anonymous_struct_field.take() { let field = Field::new(None, ty, None, None, None, false, offset); ci.fields.push(field); } } ...

emilio · 2017-02-04T11:13:38Z

src/ir/ty.rs

+            TypeKind::Alias(inner) |
+            TypeKind::TemplateAlias(inner, _) |
+            TypeKind::TemplateRef(inner, _) => ctx.resolve_type(inner).calc_size(ctx),
+            _ => None,


Please list the other types explicitly here so when we add more people don't forget to add it here.

emilio

Ok, so I think this is good to merge with that last comment addressed, and a squash so the merge commits go away.

I'll need to test this with Gecko just to double-check our C++ structs aren't completely broken with this patch, but I don't expect a lot of problems.

Thanks for this patch once again, it's really awesome! I don't love the extra complexity, but I think the improvements are quite worth it :)

emilio · 2017-02-04T16:15:55Z

src/ir/ty.rs

+            TypeKind::Named |
+            TypeKind::Function(..) |
+            TypeKind::ObjCInterface(..) |
+            TypeKind::UnresolvedTypeRef(..) => None,


I believe you can assume safely that all functions are pointer-sized, and use an unreachable! branch for UnresolvedTypeRef.

ok, it should be like this?

TypeKind::Function(..) | TypeKind::ObjCInterface(..) => Some(mem::size_of::<*mut ()>()), ... TypeKind::Void | TypeKind::Named => None, TypeKind::UnresolvedTypeRef(..) => unreachable!(),

I believe that's fine. Not totally sure about Objective C interfaces (@scoopr maybe knows?). I believe you can't have one in a field, only a pointer to it, so whatever it returns it's probably harmless.

Yes, its illegal to have Objective-C objects as anything but pointers

emilio · 2017-02-04T16:26:41Z

Also, maybe @fitzgen wants to take a quick look at it?

flier · 2017-02-04T18:48:14Z

Still adding more test cases from DPDK headers, there still a little layout error, I will back to it tomrrow :)

flier · 2017-02-05T18:58:03Z

Finally, it can pass all the 214 layout tests cases for DPDK :)

emilio · 2017-02-07T14:17:15Z

This looks great, thanks for it! I tested it locally with Gecko and caught just two field offset failures (I think because of #111).

I've got a few improvements on top of it (and I think perf regressed slightly, which is somewhat expected, but we can improve caching the layout, maybe).

This is pretty much appreciated, thanks!

@bors-servo r+

bors-servo · 2017-02-07T14:17:15Z

📌 Commit 7fd7070 has been approved by emilio

bors-servo · 2017-02-07T14:17:18Z

⌛ Testing commit 7fd7070 with merge f5394b3...

Improve struct alignment with padding bytes I known, it is a little tricky or dirty solution, I have to do a lot of calculate for alignment. For example ```c++ typedef struct { long long __clang_max_align_nonce1 __attribute__((__aligned__(__alignof__(long long)))); long double __clang_max_align_nonce2 __attribute__((__aligned__(__alignof__(long double)))); } max_align_t; ``` will be generated as ```rust pub struct _bindgen_ty_1 { pub __clang_max_align_nonce1: ::std::os::raw::c_longlong, _padding_0: u64, pub __clang_max_align_nonce2: f64, _padding_1: u64, } fn bindgen_test_layout__bindgen_ty_1() { assert_eq!(::std::mem::size_of::<_bindgen_ty_1>() , 32usize); assert_eq! (0usize , unsafe { & ( * ( 0 as * const _bindgen_ty_1 ) ) . __clang_max_align_nonce1 as * const _ as usize }); assert_eq! (16usize , unsafe { & ( * ( 0 as * const _bindgen_ty_1 ) ) . __clang_max_align_nonce2 as * const _ as usize }); } ``` It may generate wrong layout when use template or multi virtual inheritance. At least, it is a begining, pass all the test cases, and some [complex struct](https://github.com/servo/rust-bindgen/compare/master...flier:padding-bytes?expand=1#diff-eda352138aed047149ebeec72d19979d) in wild.

emilio · 2017-02-07T14:30:29Z

Well, actually there is a regression where we pad too much, but I'll work it out after this merges.

bors-servo · 2017-02-07T15:22:03Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing f5394b3 to master...

highfive added the S-awaiting-review label Feb 2, 2017

flier mentioned this pull request Feb 2, 2017

Extract and parse unexporsed attributes #459

Closed

fitzgen mentioned this pull request Feb 3, 2017

Run cargo fmt #471

Merged

highfive added the S-needs-rebase label Feb 3, 2017

emilio approved these changes Feb 4, 2017

View reviewed changes

emilio reviewed Feb 4, 2017

View reviewed changes

emilio mentioned this pull request Feb 5, 2017

Incorrect item parenthood while referencing incomplete types without a previous declaration. #482

Closed

add padding bytes to align strcture

7fd7070

flier force-pushed the padding-bytes branch from d447c37 to 7fd7070 Compare February 7, 2017 02:58

highfive assigned emilio Feb 7, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Feb 7, 2017

bors-servo merged commit 7fd7070 into rust-lang:master Feb 7, 2017

highfive removed the S-awaiting-merge label Feb 7, 2017

emilio mentioned this pull request Feb 16, 2017

Bindgen generates unnecessary padding inside nsIDocument on Windows #520

Closed

This was referenced Mar 1, 2017

Update to the latest bindgen tov/libffi-sys-rs#3

Merged

Generate sensible code for long double / stuff that has more than 64-bit alignment. #550

Closed

Improve struct alignment with padding bytes #468

Improve struct alignment with padding bytes #468

Conversation

flier commented Feb 2, 2017

highfive commented Feb 2, 2017

emilio commented Feb 3, 2017

bors-servo commented Feb 3, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flier Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Feb 4, 2017

flier commented Feb 4, 2017

flier commented Feb 5, 2017

emilio commented Feb 7, 2017

bors-servo commented Feb 7, 2017

bors-servo commented Feb 7, 2017

emilio commented Feb 7, 2017

bors-servo commented Feb 7, 2017

flier Feb 4, 2017 •

edited

Loading