Skip to content
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

alignment of integer field holding bitfield values is not correct #111

Closed
heycam opened this issue Oct 24, 2016 · 2 comments · Fixed by #512
Closed

alignment of integer field holding bitfield values is not correct #111

heycam opened this issue Oct 24, 2016 · 2 comments · Fixed by #512
Assignees

Comments

@heycam
Copy link
Contributor

heycam commented Oct 24, 2016

The use of a single integer-typed field to represent contiguous bitfield members results in layout that doesn't match the layout generated by the corresponding C++ struct. For example with:

struct A
{
  uint8_t x;
  unsigned b1 : 1;
  unsigned b2 : 1;
  unsigned b3 : 1;
  unsigned b4 : 1;
  unsigned b5 : 1;
  unsigned b6 : 1;
  unsigned b7 : 1;
  unsigned b8 : 1;
  unsigned b9 : 1;
  unsigned b10 : 1;
  uint8_t y;
};
...
printf("offsetof(A, y) = %d\n", (int) offsetof(A, y));

I get an output of 3 when compiled with clang, but rust-bindgen generates:

#[repr(C)]
#[derive(Debug, Copy)]
pub struct A {
    pub x: u8,
    pub _bitfield_1: u16,
    pub y: u8,
}

which places y at offset 4.

Packing and alignment of bitfields is probably implementation dependent, but we should generate the same layout that clang does. It might just be a matter of generating a sequence of u8s to hold the contiguous bitfields.

cc @emilio

@emilio
Copy link
Contributor

emilio commented Oct 24, 2016

Thanks for filling this, I have it in the radar, though I doubt it's so as easy as generating u8s, specially taking alignment into account.

For example, unsigned int foo: 32 provokes the container struct to be 4-byte aligned.

Probably the easiest way to do this properly is to look at llvm's source.

@emilio
Copy link
Contributor

emilio commented Feb 13, 2017

I'm looking into this right now since bindgenup in stylo is blocked on this now we check for field offsets.

bors-servo pushed a commit that referenced this issue Feb 16, 2017
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 pushed a commit that referenced this issue Feb 16, 2017
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants