Rework how bitfields are handled. #512

emilio · 2017-02-14T00:12:07Z

This fixes #111, and unblocks stylo.

The problem with this as of right now is that it drops the accessors (though before that this code was buggy so I'm not sure it's a loss).

I can probably try to re-implement those (though it'd be more complex). WDYT @fitzgen?

Also, note that I changed the max_align_nonce because it was incorrect (we shouldn't generate padding, because long double was 128 bits).

highfive · 2017-02-14T00:12:10Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-02-14T00:47:57Z

r? @fitzgen

emilio · 2017-02-14T00:48:20Z

Also @flier may want to take a look to ensure I didn't mess badly ;)

fitzgen

Taking this one on faith...

fitzgen · 2017-02-14T00:56:58Z

src/codegen/mod.rs

@@ -732,89 +732,96 @@ impl<'a> Bitfield<'a> {
    fn codegen_fields(self,
                      ctx: &BindgenContext,
                      fields: &mut Vec<ast::StructField>,
-                      methods: &mut Vec<ast::ImplItem>)
+                      _methods: &mut Vec<ast::ImplItem>)


This isn't a trait implementation, so we can change the set of parameters. Let's remove this if it isn't used anymore.

^ this comment is still not addressed

I was assuming that given #519 is assigned, it'll be used soon :)

fitzgen · 2017-02-14T01:06:25Z

I can probably try to re-implement those (though it'd be more complex).

I think we need the accessors to make this usable.

flier

some compatibility issues

flier · 2017-02-14T04:04:01Z

src/codegen/mod.rs

+            // bitfield, and whether we have to index in it. Thus, we don't know
+            // which integer type do we need.
+            //
+            // We could push them to a Vec or something, but given how buggy


TODO: We need some unit tests to verify accessor use the right offset same to LLVM

Right, this is why I didn't want to deep-dive into accessors ;)

flier · 2017-02-14T04:04:24Z

src/codegen/mod.rs

+            fields.push(field);
+        }
+
+        Layout::new(bytes_from_bits(total_size_in_bits), max_align)


we need padding some bits to the latest bitfield base on its type instead of size

consider the example

struct Date { unsigned short nWeekDay : 3; // 0..7 (3 bits) unsigned short nMonthDay : 6; // 0..31 (6 bits) unsigned short nMonth : 5; // 0..12 (5 bits) unsigned short nYear : 8; // 0..100 (8 bits) };

nYear should be aligned to offset 16bits with 16bits size, not a 8bit bit field

#[repr(C)] #[derive(Debug, Default, Copy)] pub struct Date { pub _bitfield_1: [u8; 2usize], pub _bitfield_2: u8, pub __bindgen_align: [u16; 0usize], }

More detail

Hrm, you're right about this, but this needs to only happen when this is the last field in the struct. Otherwise, we'd generate wrong code for this one (reusing your example here):

struct Date { unsigned short nWeekDay : 3; // 0..7 (3 bits) unsigned short nMonthDay : 6; // 0..31 (6 bits) unsigned short nMonth : 5; // 0..12 (5 bits) unsigned short nYear : 8; // 0..100 (8 bits) unsigned char byte; };

Where the size should still be 4 bytes.

I'll special-case this.

flier · 2017-02-14T04:06:45Z

src/codegen/mod.rs

+            total_size_in_bits += width as usize;
+
+
+            let data_size = align_to(field_size_in_bits, field_align * 8);


TODO: When we implement accessor, the data_size must limited by its type, C++ allow bitfields exceeds its type limites

If the specified size of the bit field is greater than the size of its type, the value is limited by the type: a std::uint8_t b : 1000; would still hold values between 0 and 255. the extra bits become unused padding.

struct foo { unsigned char bar : 1000; };

will generated data size, but only 8 bits should be accessible

#[repr(C)] #[derive(Debug, Default, Copy)] pub struct foo { pub _bitfield_1: [u8; 128usize], pub __bindgen_align: [u64; 0usize], }

In the C programming language, the width of a bit field cannot exceed the width of the underlying type.

More detail

Right, I had already a note about packed and wide bitfields :)

emilio · 2017-02-15T16:08:07Z

So I pushed another wip that manages to get rid of the calc layout code, at the expense of failing the layout_array test.

That test generates an array of unaligned structs and previous to this commit, padded it. I'd argue that's pretty wrong, and it's wrong to not fail that test, since it's pretty unsafe (the structs in the array don't start where it should).

@flier, wdyt?

emilio · 2017-02-15T21:35:49Z

I fixed that test (if in a much hacky way).

emilio · 2017-02-15T23:02:14Z

Ok, so current commit passess all the layout and field offset tests here and gecko.

As I said, adding the getters and setters again is probably not too hard, but this has already went out of scope of what I initially intended.

@fitzgen mind taking a look again since a few things have changed?

fitzgen · 2017-02-15T23:14:46Z

Cargo.toml

@@ -13,7 +13,7 @@ name = "bindgen"
 readme = "README.md"
 repository = "https://github.com/servo/rust-bindgen"
 documentation = "https://docs.rs/bindgen"
-version = "0.21.3"
+version = "0.22.0"


We've almost caught up with emacs!

fitzgen · 2017-02-15T23:16:26Z

build.rs

@@ -11,6 +11,7 @@ mod codegen {
        quasi_codegen::expand(&src, &dst).unwrap();
        println!("cargo:rerun-if-changed=src/codegen/mod.rs");
        println!("cargo:rerun-if-changed=src/codegen/helpers.rs");
+        println!("cargo:rerun-if-changed=src/codegen/struct_layout.rs");


Do these things support wildcards? If so, is there a reason we shouldn't do this

println!("cargo:rerun-if-changed=src/codegen/*");

?

Nope, it's not supported I think, we'd need to walkdir or something.

fitzgen · 2017-02-15T23:16:51Z

src/codegen/mod.rs

@@ -732,89 +732,96 @@ impl<'a> Bitfield<'a> {
    fn codegen_fields(self,
                      ctx: &BindgenContext,
                      fields: &mut Vec<ast::StructField>,
-                      methods: &mut Vec<ast::ImplItem>)
+                      _methods: &mut Vec<ast::ImplItem>)


^ this comment is still not addressed

emilio · 2017-02-15T23:37:25Z

@bors-servo r=fitzgen

bors-servo · 2017-02-15T23:37:26Z

📌 Commit b14adc3 has been approved by fitzgen

bors-servo · 2017-02-16T00:26:43Z

⌛ Testing commit b14adc3 with merge 852db65...

@fitzgen

Rework how bitfields are handled. This fixes #111, and unblocks stylo. The problem with this as of right now is that it drops the accessors (though before that this code was buggy so I'm not sure it's a loss). I can probably try to re-implement those (though it'd be more complex). WDYT @fitzgen? Also, note that I changed the max_align_nonce because it was incorrect (we shouldn't generate padding, because `long double` was `128` bits).

emilio · 2017-02-16T02:01:23Z

@bors-servo r=fitzgen

Cargo.lock

bors-servo · 2017-02-16T02:01:23Z

📌 Commit 08d4b8e has been approved by fitzgen

bors-servo · 2017-02-16T04:34:37Z

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

bors-servo · 2017-02-16T04:34:38Z

🔒 Merge conflict

emilio · 2017-02-16T04:38:36Z

@bors-servo r=fitzgen

Grrr

bors-servo · 2017-02-16T04:38:36Z

📌 Commit 98f43d0 has been approved by fitzgen

bors-servo · 2017-02-16T04:38:39Z

⌛ Testing commit 98f43d0 with merge e57b2cb...

@fitzgen

Rework how bitfields are handled. This fixes #111, and unblocks stylo. The problem with this as of right now is that it drops the accessors (though before that this code was buggy so I'm not sure it's a loss). I can probably try to re-implement those (though it'd be more complex). WDYT @fitzgen? Also, note that I changed the max_align_nonce because it was incorrect (we shouldn't generate padding, because `long double` was `128` bits).

bors-servo · 2017-02-16T05:58:42Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing e57b2cb to master...

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

highfive assigned fitzgen Feb 14, 2017

fitzgen approved these changes Feb 14, 2017

View reviewed changes

flier suggested changes Feb 14, 2017

View reviewed changes

emilio force-pushed the bitfields branch from aa5b0f2 to 322c33c Compare February 15, 2017 16:06

emilio force-pushed the bitfields branch from 322c33c to a721557 Compare February 15, 2017 21:35

emilio force-pushed the bitfields branch from 71e43ed to 0b92a31 Compare February 15, 2017 23:00

emilio changed the title ~~[wip] Rework how bitfields are handled.~~ Rework how bitfields are handled. Feb 15, 2017

emilio mentioned this pull request Feb 15, 2017

Re-introduce bitfield accessors. #519

Closed

fitzgen approved these changes Feb 15, 2017

View reviewed changes

emilio mentioned this pull request Feb 15, 2017

Looking for something to work on? servo/servo#15162

Closed

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 15, 2017

emilio force-pushed the bitfields branch from b14adc3 to 67b2178 Compare February 16, 2017 02:00

highfive added S-awaiting-review and removed S-awaiting-merge labels Feb 16, 2017

emilio force-pushed the bitfields branch from 67b2178 to 08d4b8e Compare February 16, 2017 02:01

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 16, 2017

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

emilio added 2 commits February 16, 2017 05:38

Rework how bitfields are handled.

0d12882

Breaking version bump.

98f43d0

emilio force-pushed the bitfields branch from 08d4b8e to 98f43d0 Compare February 16, 2017 04:38

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

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

emilio mentioned this pull request Feb 16, 2017

Bindgen generates unnecessary padding inside nsIDocument on Windows #520

Closed

bors-servo merged commit 98f43d0 into rust-lang:master Feb 16, 2017

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

emilio deleted the bitfields branch February 16, 2017 06:00

emilio mentioned this pull request Mar 1, 2017

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

Closed

Manishearth unassigned fitzgen Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework how bitfields are handled. #512

Rework how bitfields are handled. #512

emilio commented Feb 14, 2017

highfive commented Feb 14, 2017

emilio commented Feb 14, 2017

emilio commented Feb 14, 2017

fitzgen left a comment

fitzgen Feb 14, 2017

fitzgen Feb 15, 2017

emilio Feb 15, 2017

fitzgen commented Feb 14, 2017

flier left a comment

flier Feb 14, 2017

emilio Feb 14, 2017

flier Feb 14, 2017

emilio Feb 14, 2017

flier Feb 14, 2017

emilio Feb 14, 2017

emilio commented Feb 15, 2017

emilio commented Feb 15, 2017

emilio commented Feb 15, 2017

fitzgen Feb 15, 2017

fitzgen Feb 15, 2017

emilio Feb 15, 2017

fitzgen Feb 15, 2017

emilio commented Feb 15, 2017

bors-servo commented Feb 15, 2017

bors-servo commented Feb 16, 2017

emilio commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

emilio commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

		total_size_in_bits += width as usize;


		let data_size = align_to(field_size_in_bits, field_align * 8);

Rework how bitfields are handled. #512

Rework how bitfields are handled. #512

Conversation

emilio commented Feb 14, 2017

highfive commented Feb 14, 2017

emilio commented Feb 14, 2017

emilio commented Feb 14, 2017

fitzgen 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

fitzgen commented Feb 14, 2017

flier 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

emilio commented Feb 15, 2017

emilio commented Feb 15, 2017

emilio commented Feb 15, 2017

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 15, 2017

bors-servo commented Feb 15, 2017

bors-servo commented Feb 16, 2017

emilio commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

emilio commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017

bors-servo commented Feb 16, 2017