Skip to content

/rust-bindgen

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

Support bitfield allocation units larger than 64 bits #1158

Merged
merged 1 commit into from Nov 23, 2017

Conversation

Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@glyn @highfive @emilio @bors-servo @fitzgen
@glyn
Copy link
Contributor

glyn commented Nov 20, 2017 •
edited

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 highfive assigned fitzgen Nov 20, 2017

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

@highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

highfive commented Nov 20, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@emilio

emilio reviewed Nov 20, 2017
View changes

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

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 20, 2017

Collaborator

This definitely needs to be repr(C).

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 20, 2017

Collaborator

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.

This comment has been minimized.

Sign in to view
Copy link
@glyn

glyn Nov 20, 2017

Author Contributor

@emilio Nice catch! Thanks. Fixed.
@glyn

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

glyn commented Nov 21, 2017

@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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

glyn commented Nov 21, 2017

r? @emilio

@highfive highfive assigned emilio and unassigned fitzgen Nov 21, 2017

@emilio

emilio reviewed Nov 21, 2017
View changes

Copy link
Collaborator

emilio left a comment

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...
src/codegen/bitfield_unit.rs Outdated
{
#[inline]
pub fn new(storage: Storage) -> Self {
use std::mem::align_of;

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 21, 2017

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
@glyn

glyn Nov 21, 2017

Author Contributor

@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.
src/codegen/bitfield_unit.rs Outdated
#[inline]
pub fn new(storage: Storage) -> Self {
use std::mem::align_of;
debug_assert!(align_of::<Storage>() <= align_of::<Align>());

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 21, 2017

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
@glyn

glyn Nov 21, 2017

Author Contributor

@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.
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;

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 21, 2017

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
@glyn

glyn Nov 21, 2017

Author Contributor

@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.
bindgen-integration/cpp/Test.cc Outdated
this->nMonthDay == nMonthDay &&
this->nMonth == nMonth &&
this->byte == byte;
printf("\n");

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 21, 2017

Collaborator

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

This comment has been minimized.

Sign in to view
Copy link
@glyn

glyn Nov 21, 2017

Author Contributor

@emilio Agreed. Better to delete this clutter. It's trivial to add back in for debugging if someone doesn't have gdb.
@fitzgen @glyn
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.

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.
Loading status checks…
f0e0531
@emilio

This comment has been minimized.

Sign in to view
Copy link
Collaborator

emilio commented Nov 23, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Nov 23, 2017

📌 Commit f0e0531 has been approved by emilio

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

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Nov 23, 2017

⌛️ Testing commit f0e0531 with merge 7c3584d...

bors-servo added a commit that referenced this pull request Nov 23, 2017

@bors-servo
Auto merge of #1158 - glyn:large-bitfield-units, r=emilio 
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
Loading status checks…
7c3584d
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Nov 23, 2017

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

@bors-servo bors-servo merged commit f0e0531 into rust-lang:master Nov 23, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

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

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Nov 27, 2017

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.