Re-introduce bitfield accessors.

[emilio]

The refactor at #512 is dropping them intentionally since the way bitfields were handled was so broken.

They need to be re-implemented, it's not exceptionally hard, but it's a bit of work.

[emilio]

Presumably, given we now have the exact field offsets from clang, it's easier and more bulletproof to just use the raw bit offset instead of using the fields. Both approaches work (the fields one will work for structs with template parameters too, though that's probably not a huge priority).

[emilio]

Please comment if you want to work on this, happy to mentor it. You can see the code that generated the accessors here.

This will require some tests in the bindgen-integration crate, to ensure that the fields read and written from C++ are set correctly.

[emilio]

@cseale said he was interested in working on this :)

[cseale]

@emilio happy to work on this, just not sure of the requirements

[emilio]

So, mainly, before #512, we had accessors to the C/C++ bitfields from rust (those functions my comment above points to). This is an example of a header with bitfields, and these are the getters and setters generated for that struct.

Turns out that some of those (like the ones I added in the test cases of #512) were pretty broken, but #512 should fix the layout of those.

So now we need to create those accessors again, but taking into account that the generated bitfield may now be an array like [u8; 2] instead of a u16 as it was before.

There are multiple ways to fix this, you only need to implement one of these.

One of them is re-using the code that was before and just generating methods that transmute between the correct types. That way, a getter would look something like:

    pub fn foo(&self) -> BitfieldType {
        unsafe {
            ::std::mem::transmute::<BitfieldIntegerType, BitfieldType>(
                (::std::mem::transmute::<BitfieldMemberType, BitfieldIntegerType>(self.bitfield_field) & Constant) >> Offset))
        }
}

Where Constant and Offset are constants calculated the way code pre-#512 did.

Other approach is doing the arithmetic necessary to know if the relevant bitfield is stored as part of an array or a plain integer, and generate in the same fashion different kind of getters depending on the case.

Other approach which may be more straight-forward, though wouldn't take into account structs with template parameters (that's fine though, I believe), would be getting the field width (which is the amount of bits of that bitfield), and offset, which is the index in bits of the bitfield member in the struct. We can avoid generating the accessors and setters if we don't know the offsets.

Once we have the offset, we can just do some math to get the relevant bits. For example, if the bitfield is 5 bits width starting at bit 10, we should grab the second byte, and get the bits in that byte in the range [3..7]. We could do that with something like the following (note that Offset and Constant are not the offset of the field, it's the offset in the byte we should shift to to get the correct output):

    pub fn foo(&self) -> BitfieldType {
        unsafe {
            ((*(self as *const u8).offset(1)) & Constant) >> Offset
        }
    }

(We probably still need some transmute to support enums in the above case, but let's put the easy cases first).

Of course, generalizing this is also a bit of work.

I don't know if I have given you enough info, let me know if something is unclear or you need anything else.

Thanks again for working on this!

[emilio]

Also, don't hesitate on just bailing out on stuff that is hard for now. It's better to have a few easy cases working that having none at all. We could start supporting bitfields that take only one slot of memory and are aligned, for example:

    uint32_t foo : 31;
    uint32_t bar: 1;

Then we can build on that.

[cseale]

@emilio cool, thanks for that. I will give your comments a detailed read and I will start working on something this weekend.

[cseale]

Hi @emilio. If it's possible, could I get a bit more details on what each of the code samples provided in your above comments are doing? I am super new to rust, so a lot of the syntax is very unfamiliar.

[emilio]

Sorry, I haven't forgotten about this, I've just been ultra-busy with other stuff and haven't found the time to write everything down. I'll hopefully get to write a more detailed description tomorrow.

Meanwhile, you can read on the docs on transmute, and pointer.offset. The rest are mostly bitmasking.

Sorry again :(

[cseale]

no worries, I'm actually up the walls myself on other stuff

[fitzgen]

@cseale how's this going? Any questions I can answer? Anything I can do to help out?

The spidermonkey bindings I'm working on will be blocked on the reintroduction of these bit field accessors pretty soon, so I'd like to make sure we're making forward progress here :)

[emilio]

@fitzgen if you could elaborate a bit on the options (answering #519 (comment)), it'd be quite helpful, since I have been doing some exams lately and haven't had the time to page that back in.

[cseale]

I'm actually in the same boat guys, I may have bitten off more than I can chew with my time, and I recently injured myself making it harder to get my higher priority work done. I don't think I'll get any reasonable time to commit to servo and don't want to block you guys on this. Sorry

[fitzgen]

Oh no! I hope you recover well!

The project will still be here once you're healthy, if you want to contribute again at that time :)

[fitzgen]

As far as this issue goes, I can take it over.