bitfield methods not generated #816
Comments
|
Thanks for the regression report! |
|
So this is because with the correct bitfield layout, that bitfield is actually > 64 bits, so we can't use a rust integer to represent it... |
|
@emilio I see. How difficult would it be to use a representation which is not a single integer? |
|
It'd require some rework I personally don't have the time to do right now... but it'd be nice, and it should be doable (you just need to calculate a window where you can do a load for said bitfield, and choose one of them with the proper bitmask) |
|
Please note this bug is blocking our project from upgrading from bindgen v0.24.x |
Hi @glyn, if you're interested in unblocking your project, here are some tips for navigating around this code:
What I believe is involved in fixing this bug is
|
|
Thanks @fitzgen - very helpful. |
|
Progress has been slow in understanding the code, partly because some of it appears to be based on an invalid assumption that masks can always be 64 bits wide and that bitfield allocation units can always be manipulated as 64 bit entities (see #954). I'm not sure how performance critical the generated code is, but I'm inclined to say that we should make it correct first and optimise it later. So I'm thinking of treating bitfields as arrays of Testing is a bit of a concern as that kind of code will be quite fiddly and would benefit from a good unit test suite. But I think that would be inconvenient to do through codegen. I wonder if we could develop some kind of helper type and make the generated code depend on that type. Any comments on the general approach gratefully received from @emilio, @fitzgen, or others. |
|
I'll be doing my best to support @glyn with this issue. |
With this, you mean actually generating arrays of |
|
My understanding, such as it is, is that bitfields are grouped into contiguous blocks of bytes which start on a byte boundary. Look at this for example to see that bindgen already generates arrays of |
|
The final alignment depends on the types specified as members. See: For an example where we generate 4-byte-aligned bitfields. |
|
A status update: @pylbrecht is unable to commit time to this issue. I am also finding it hard to get traction on this issue with the maximum of one day per week I get to work on it. I have been exploring a generic overlay of an array of unsigned integers of any supported precision. The goal was to unit test masking and other bitfield operations thoroughly and independently of codegen. However that approach ran aground because of Rust restrictions in the way arrays are handled as generic type parameters. I think the way forward is to generate a specific overlay and do a bit more work during codegen to tailor the code to the array length and integer precision. It will be messier to get good unit tests, although bindgen doesn't seem particularly reliant on unit tests, so maybe that would be acceptable. To gain traction, I need to understand the codegen code in more detail. The code overview in CONTRIBUTING.md is great, but I'm still trying to figure out which pieces of code will need to be changed to implement this issue. Alternatively, it might be better to revisit this issue after #954 has been fixed as there will then be less confusion in the implementation. |
|
Regarding the "generic overlay": were you thinking emitting in the bindings' prelude something like union BindgenBitfieldUnit<Storage, Align>
where
Storage: Copy + AsRef<[u8]>,
Align: Copy
{
storage: Storage,
align: [Align; 0],
}
impl<Storage, Align> BindgenBitfieldUnit<Storage, Align>
where
Storage: Copy + AsRef<[u8]>,
Align: Copy
{
#[inline]
fn get(&self, bit_offset: usize, bit_width: usize) -> u64 {
unimplemented!()
}
fn set(&mut self, bit_offset: usize, bit_width: usize, val: u64) {
unimplemented!()
}
}Which only manages storage and reading/writing that storage? And then the generated code for bitfields would do something like this: /*
This would be generated from:
struct TaggedPtr {
uintptr_t ptr : 61;
char tag : 3;
};
*/
pub struct TaggedPtr {
_bitfield_unit_1: BindgenBitfieldUnit<[u8; 8], u64>,
}
impl TaggedPtr {
pub fn ptr(&self) -> usize {
self._bitfield_unit_1.get(0, 61) as usize
}
pub fn set_ptr(&mut self, ptr: usize) {
self._bitfield_unit_1.set(0, 61, ptr as u64)
}
pub fn tag(&self) -> ::std::os::raw::c_char {
self._bitfield_unit_1.get(62, 3) as ::std::os::raw::c_char
}
pub fn set_tag(&mut self, tag: ::std::os::raw::c_char) {
self._bitfield_unit_1.set(62, 3, tag as u64);
}
}This seems like a reasonable way to split responsibilities and break the problem down into more manageable chunks.
Agreed. Breaking things down into smaller blocks is almost always helpful. |
|
@fitgen Thanks for your reply.
My approach was roughly like that, but yours is more likely to work with current Rust since it avoids dealing with the full array as a generic type parameter.
I'll take a look at #954 but, unfortunately, it won't be any time soon. |
|
I poked at this a little while ago. The
#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct BindgenBitfieldUnit<Storage, Align>
where
Storage: AsRef<[u8]> + AsMut<[u8]>,
{
storage: Storage,
align: [Align; 0],
}
impl<Storage, Align> BindgenBitfieldUnit<Storage, Align>
where
Storage: AsRef<[u8]> + AsMut<[u8]>,
{
#[inline]
pub fn new(storage: Storage) -> Self {
Self {
storage,
align: [],
}
}
#[inline]
pub fn get_bit(&self, index: usize) -> bool {
debug_assert!(index / 8 < self.storage.as_ref().len());
let byte_index = self.storage.as_ref().len() - (index / 8) - 1;
let byte = self.storage.as_ref()[byte_index];
let bit_index = index % 8;
let mask = 1 << bit_index;
byte & mask == mask
}
#[inline]
pub fn set_bit(&mut self, index: usize, val: bool) {
debug_assert!(index / 8 < self.storage.as_ref().len());
let byte_index = self.storage.as_ref().len() - (index / 8) - 1;
let byte = &mut self.storage.as_mut()[byte_index];
let bit_index = index % 8;
let mask = 1 << bit_index;
if val {
*byte |= mask;
} else {
*byte &= !mask;
}
}
#[inline]
pub fn get(&self, bit_offset: usize, bit_width: u8) -> u64 {
debug_assert!(bit_width <= 64);
debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
debug_assert!((bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len());
let mut val = 0;
for i in 0..(bit_width as usize) {
if self.get_bit(i + bit_offset) {
val |= 1 << i;
}
}
val
}
#[inline]
pub fn set(&mut self, bit_offset: usize, bit_width: u8, val: u64) {
debug_assert!(bit_width <= 64);
debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
debug_assert!((bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len());
for i in 0..(bit_width as usize) {
let mask = 1 << i;
let val_bit_is_set = val & mask == mask;
self.set_bit(i + bit_offset, val_bit_is_set);
}
}
}WIP commit defining WIP tree with integration into codegen is here: https://github.com/fitzgen/rust-bindgen/commits/bindgen-bitfield-unit |
|
Thanks @fitzgen. I take it that you would prefer to avoid |
|
Yeah, if possible, and in this case it isn't too difficult :) |
|
@fitzgen Correction: re-reading your code, I see the |
|
@fitzgen I corrected a little bug in your WIP commit and now all the tests pass. Is this suitable for merging to master? We can then work on lifting the 64 bit restriction. Please see this commit. |
|
@glyn Awesome! Thank you very much! Can you open a PR with the WIP commit and your fix squashed together with a nice commit message? Then I can do a final double check and merge it. Thanks! |
|
@fitzgen Since it was then relatively easy to lift the 64 bit unit restriction and that is the real motivation for the above code, I've submitted a PR with this all done together. Hope that works for you. |
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
|
@fitzgen Yes. I checked the behaviour using the example at the start of this issue. |
Some bitfield getters and setter methods which were generated by bindgen 24.x are no longer generated by bindgen 26.x.
The following is a stripped down branch of my project which reproduces the problem with a single C struct.
https://github.com/glyn/jvmkill/tree/reproduce-bindgen-bug
Input C/C++ Header
Bindgen Invocation
Actual Results
Incorrect bindings from bindgen 26.3:
Expected Results
Correct bindings from bindgen 24.x:
RUST_LOG=bindgenOutputPlease see this gist since the log was too long for a github issue body.
The text was updated successfully, but these errors were encountered: