Support bitfield allocation units larger than 64 bits #1158

glyn · 2017-11-20T15:16:08Z

Individual bitfields are still limited to at most 64 bits, but this restriction can be weakened when Rust supports u128.

This implements issue #816.

Usage notes:

Since common code is added to each generated binding, a program which uses
more than one binding may need to work around the duplication by including
each binding in its own module.
The values created by bitfield allocation unit constructors can be assigned
directly to the corresponding struct fields with no need for transmutation.

Implementation notes:

__BindgenBitfieldUnit represents a bitfield allocation unit using a Storage
type accessible as a slice of u8. The alignment of the unit is inherited from
an Align type by virtue of the field:

align: [Align; 0],

The position of this field in the struct is irrelevant.

It is assumed that the alignment of the Storage type is no larger than the
alignment of the Align type, which will be true if the Storage type is, for
example, an array of u8. This assumption is checked in a debug assertion.

Although the double underscore (__) prefix is reserved for implementations of
C++, there are precedents for this convention elsewhere in bindgen and so the
convention is adopted here too.

Acknowledgement:

Thanks to @fitzgen for an initial implementation of __BindgenBitfieldUnit and
code to integrate it into bindgen.

r? @emilio

highfive · 2017-11-20T15:16:13Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-11-20T16:25:22Z

src/codegen/bitfield_unit.rs

@@ -0,0 +1,83 @@
+#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct BindgenBitfieldUnit<Storage, Align>


This definitely needs to be repr(C).

Also, I'd recommend maybe to put a double underscore as the prefix for the name, since that's technically reserved in C. Not sure what's @fitzgen's opinion on that.

@emilio Nice catch! Thanks. Fixed.

glyn · 2017-11-21T09:39:40Z

@emilio Thanks for the feedback. I added repr(C) and the double underscore prefix. Since @fitgen doesn't seem to be around at the moment and you have begun a review, I've changed the reviewer to you. Let's hope @highfive notices...

glyn · 2017-11-21T09:39:56Z

r? @emilio

emilio

looks pretty nice to me, but I think it can merge as-is. at least the use_core issue needs to be resolved. Maybe we can just get away fine without const fn...

emilio · 2017-11-21T12:58:54Z

src/codegen/bitfield_unit.rs

+{
+    #[inline]
+    pub fn new(storage: Storage) -> Self {
+        use std::mem::align_of;


This somehow needs to be core when --use-core is passed.

@emilio The simplest solution is to remove the assertion (and its use of align_of) as it's not needed currently and it's very unlikely that anyone will trip over the assumption in the future.

emilio · 2017-11-21T12:59:19Z

src/codegen/bitfield_unit.rs

+    #[inline]
+    pub fn new(storage: Storage) -> Self {
+        use std::mem::align_of;
+        debug_assert!(align_of::<Storage>() <= align_of::<Align>());


This can probably be a release assert!. It will go away anyway.

@emilio The simplest solution is to remove the assertion as it's not needed currently and it's very unlikely that anyone will trip over the assumption in the future.

emilio · 2017-11-21T13:01:30Z

src/features.rs

@@ -140,8 +140,6 @@ macro_rules! rust_feature_def {
 rust_feature_def!(
    /// Untagged unions ([RFC 1444](https://github.com/rust-lang/rfcs/blob/master/text/1444-union.md))
    => untagged_union;
-    /// Constant function ([RFC 911](https://github.com/rust-lang/rfcs/blob/master/text/0911-const-fn.md))
-    => const_fn;


I think const fn was also needed for SpiderMonkey, so not sure we can get away without it so easily...

@emilio I'll leave it out for now because it would be easy to add back in. Certainly the bitfield allocation unit constructors have no hope of being const fn.

emilio · 2017-11-21T13:01:59Z

bindgen-integration/cpp/Test.cc

-      this->nMonthDay == nMonthDay &&
-      this->nMonth == nMonth &&
-      this->byte == byte;
+    printf("\n");


Not sure how worth are the printfs? i'd usually just use gdb for this.

@emilio Agreed. Better to delete this clutter. It's trivial to add back in for debugging if someone doesn't have gdb.

@fitzgen

Individual bitfields are still limited to at most 64 bits, but this restriction can be weakened when Rust supports u128. This implements issue #816. Usage notes: * Since common code is added to each generated binding, a program which uses more than one binding may need to work around the duplication by including each binding in its own module. * The values created by bitfield allocation unit constructors can be assigned directly to the corresponding struct fields with no need for transmutation. Implementation notes: __BindgenBitfieldUnit represents a bitfield allocation unit using a Storage type accessible as a slice of u8. The alignment of the unit is inherited from an Align type by virtue of the field: align: [Align; 0], The position of this field in the struct is irrelevant. The alignment of the Storage type is intended to be no larger than the alignment of the Align type, which will be true if the Storage type is, for example, an array of u8. Although the double underscore (__) prefix is reserved for implementations of C++, there are precedents for this convention elsewhere in bindgen and so the convention is adopted here too. Acknowledgement: Thanks to @fitzgen for an initial implementation of __BindgenBitfieldUnit and code to integrate it into bindgen.

emilio · 2017-11-23T10:31:07Z

I took a closer look, and I can't think of why this shouldn't get merged. Thanks a lot @glyn!

@bors-servo r+

bors-servo · 2017-11-23T10:31:08Z

📌 Commit f0e0531 has been approved by emilio

bors-servo · 2017-11-23T10:31:11Z

⌛ Testing commit f0e0531 with merge 7c3584d...

@fitzgen

Support bitfield allocation units larger than 64 bits Individual bitfields are still limited to at most 64 bits, but this restriction can be weakened when Rust supports `u128`. This implements issue #816. Usage notes: * Since common code is added to each generated binding, a program which uses more than one binding may need to work around the duplication by including each binding in its own module. * The values created by bitfield allocation unit constructors can be assigned directly to the corresponding struct fields with no need for transmutation. Implementation notes: `__BindgenBitfieldUnit` represents a bitfield allocation unit using a `Storage` type accessible as a slice of `u8`. The alignment of the unit is inherited from an `Align` type by virtue of the field: ``` align: [Align; 0], ``` The position of this field in the struct is irrelevant. It is assumed that the alignment of the `Storage` type is no larger than the alignment of the `Align` type, which will be true if the `Storage` type is, for example, an array of `u8`. This assumption is checked in a debug assertion. Although the double underscore (__) prefix is reserved for implementations of C++, there are precedents for this convention elsewhere in bindgen and so the convention is adopted here too. Acknowledgement: Thanks to @fitzgen for an initial implementation of `__BindgenBitfieldUnit` and code to integrate it into bindgen. r? @emilio

bors-servo · 2017-11-23T10:49:37Z

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

fitzgen · 2017-11-27T18:51:02Z

Thanks for taking this over the finishing line @glyn!

We should probably go back and re-test all of our bitfield bugs and see if we fixed any of them with this change.

highfive assigned fitzgen Nov 20, 2017

highfive added the S-awaiting-review label Nov 20, 2017

emilio reviewed Nov 20, 2017

View reviewed changes

highfive assigned emilio and unassigned fitzgen Nov 21, 2017

emilio reviewed Nov 21, 2017

View reviewed changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 23, 2017

bors-servo merged commit f0e0531 into rust-lang:master Nov 23, 2017

highfive removed the S-awaiting-merge label Nov 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bitfield allocation units larger than 64 bits #1158

Support bitfield allocation units larger than 64 bits #1158

glyn commented Nov 20, 2017 •

edited

Loading

highfive commented Nov 20, 2017

emilio Nov 20, 2017

emilio Nov 20, 2017

glyn Nov 20, 2017

glyn commented Nov 21, 2017

glyn commented Nov 21, 2017

emilio left a comment

emilio Nov 21, 2017

glyn Nov 21, 2017

emilio Nov 21, 2017

glyn Nov 21, 2017

emilio Nov 21, 2017

glyn Nov 21, 2017

emilio Nov 21, 2017

glyn Nov 21, 2017

emilio commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

fitzgen commented Nov 27, 2017

		@@ -0,0 +1,83 @@
		#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
		pub struct BindgenBitfieldUnit<Storage, Align>

Support bitfield allocation units larger than 64 bits #1158

Support bitfield allocation units larger than 64 bits #1158

Conversation

glyn commented Nov 20, 2017 • edited Loading

highfive commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyn commented Nov 21, 2017

glyn commented Nov 21, 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

emilio commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

bors-servo commented Nov 23, 2017

fitzgen commented Nov 27, 2017

glyn commented Nov 20, 2017 •

edited

Loading