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.

Sign up for GitHub

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

Jump to bottom

Wrong layout generated for some structures with complex bit fields #1947

Closed
varphone opened this issue Dec 18, 2020 · 1 comment · Fixed by #1950
Closed

Wrong layout generated for some structures with complex bit fields #1947

varphone opened this issue Dec 18, 2020 · 1 comment · Fixed by #1950
Labels
A-bitfields bug

Comments

@varphone
Copy link
Contributor

Input C/C++ Header

typedef unsigned char U8;
typedef unsigned short U16;

typedef struct {
    U16 MADZ : 10, MAI0 : 2, MAI1 : 2, MAI2 : 2;
    U8 MADK, MABR;
    U16 MATH : 10, MATE : 4, MATW : 2;
    U8 MASW : 4, MABW : 3, MAXN : 1, _rB_;
} V56AMDY;

Bindgen Invocation

bindgen::Builder::default()
    .header("input.h")
    .generate()
    .unwrap()

or

$ bindgen input.h

Actual Results

Bindings:

/* automatically generated by rust-bindgen 0.56.0 */
// ...
pub type U8 = ::std::os::raw::c_uchar;
pub type U16 = ::std::os::raw::c_ushort;
#[repr(C)]
#[repr(align(2))]
#[derive(Debug, Default, Copy, Clone)]
pub struct V56AMDY {
    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 2usize], u16>,
    pub MADK: U8,
    pub MABR: U8,
    pub _bitfield_2: __BindgenBitfieldUnit<[u8; 4usize], u16>,
    pub _rB_: U8,
}
#[test]
fn bindgen_test_layout_V56AMDY() {
    assert_eq!(
        ::std::mem::size_of::<V56AMDY>(),
        8usize,
        concat!("Size of: ", stringify!(V56AMDY))
    );
    // ...
}
// ...

Layout Test:

test bindgen_test_layout_V56AMDY ... FAILED

failures:

---- bindgen_test_layout_V56AMDY stdout ----
thread 'bindgen_test_layout_V56AMDY' panicked at 'assertion failed: `(left == right)`
  left: `10`,
 right: `8`: Size of: V56AMDY', tests/struct_with_complex_bitfields.rs:109:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    bindgen_test_layout_V56AMDY

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

Expected Results

Bindings:

/* automatically generated by rust-bindgen 0.56.0 */
// ...
pub type U8 = ::std::os::raw::c_uchar;
pub type U16 = ::std::os::raw::c_ushort;
#[repr(C)]
#[repr(align(2))]
#[derive(Debug, Default, Copy, Clone)]
pub struct V56AMDY {
    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 2usize], u16>,
    pub MADK: U8,
    pub MABR: U8,
    pub _bitfield_2: __BindgenBitfieldUnit<[u8; 3usize], u16>,
    pub _rB_: U8,
}
#[test]
fn bindgen_test_layout_V56AMDY() {
    assert_eq!(
        ::std::mem::size_of::<V56AMDY>(),
        8usize,
        concat!("Size of: ", stringify!(V56AMDY))
    );
    // ...
}
// ...

Layout Test:

running 1 test
test bindgen_test_layout_V56AMDY ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
emilio added a commit that referenced this issue Dec 20, 2020
@emilio
comp: Fix bitfields to allow fields after them to take space below th…
12aa252 
…eir alignment.

Fixes #1947.

There are two separate issues here: First, the change in comp.rs ensures
that we don't round up the amount of storage to the alignment of the
bitfield. That generates the "expected" output in #1947
(`__BindgenBitfieldUnit<[u8; 3], u16>`).

But that's still not enough to fix that test-case because
__BindgenBitfieldUnit would be aligned and have padding, and Rust won't
put the extra field in the padding.

In order to ensure the bitfield starts at the right alignment, but that
Rust can put stuff in the extra field, we need to make a breaking change
and split the generated fields in two: One preceding that guarantees
alignment, and the actual storage, bit-aligned.

This keeps the existing behavior while fixing that test-case.
emilio added a commit that referenced this issue Dec 20, 2020
@emilio
comp: Fix bitfields to allow underaligned fields after them to take p…
f1f1bca 
…adding space.

Fixes #1947.

There are two separate issues here: First, the change in comp.rs ensures
that we don't round up the amount of storage to the alignment of the
bitfield. That generates the "expected" output in #1947
(`__BindgenBitfieldUnit<[u8; 3], u16>`).

But that's still not enough to fix that test-case because
__BindgenBitfieldUnit would be aligned and have padding, and Rust won't
put the extra field in the padding.

In order to ensure the bitfield starts at the right alignment, but that
Rust can put stuff in the extra field, we need to make a breaking change
and split the generated fields in two: One preceding that guarantees
alignment, and the actual storage, bit-aligned.

This keeps the existing behavior while fixing that test-case.
@emilio emilio mentioned this issue Dec 20, 2020
Merged
@emilio
Copy link
Contributor

emilio commented Dec 20, 2020

#1950 has a test-case and a fix, though a bit unfortunate that it had to be a breaking change.

emilio added a commit that referenced this issue Dec 20, 2020
@emilio
comp: Fix bitfields to allow underaligned fields after them to take p…
a4558a2 
…adding space.

Fixes #1947.

There are two separate issues here: First, the change in comp.rs ensures
that we don't round up the amount of storage to the alignment of the
bitfield. That generates the "expected" output in #1947
(`__BindgenBitfieldUnit<[u8; 3], u16>`).

But that's still not enough to fix that test-case because
__BindgenBitfieldUnit would be aligned and have padding, and Rust won't
put the extra field in the padding.

In order to ensure the bitfield starts at the right alignment, but that
Rust can put stuff in the extra field, we need to make a breaking change
and split the generated fields in two: One preceding that guarantees
alignment, and the actual storage, bit-aligned.

This keeps the existing behavior while fixing that test-case.
emilio added a commit that referenced this issue Dec 20, 2020
@emilio
comp: Fix bitfields to allow underaligned fields after them to take p…
88a7849 
…adding space.

Fixes #1947.

There are two separate issues here: First, the change in comp.rs ensures
that we don't round up the amount of storage to the alignment of the
bitfield. That generates the "expected" output in #1947
(`__BindgenBitfieldUnit<[u8; 3], u16>`).

But that's still not enough to fix that test-case because
__BindgenBitfieldUnit would be aligned and have padding, and Rust won't
put the extra field in the padding.

In order to ensure the bitfield starts at the right alignment, but that
Rust can put stuff in the extra field, we need to make a breaking change
and split the generated fields in two: One preceding that guarantees
alignment, and the actual storage, bit-aligned.

This keeps the existing behavior while fixing that test-case.
emilio added a commit that referenced this issue Dec 20, 2020
@emilio
comp: Fix bitfields to allow underaligned fields after them to take p…
669dc1b 
…adding space.

Fixes #1947.

There are two separate issues here: First, the change in comp.rs ensures
that we don't round up the amount of storage to the alignment of the
bitfield. That generates the "expected" output in #1947
(`__BindgenBitfieldUnit<[u8; 3], u16>`).

But that's still not enough to fix that test-case because
__BindgenBitfieldUnit would be aligned and have padding, and Rust won't
put the extra field in the padding.

In order to ensure the bitfield starts at the right alignment, but that
Rust can put stuff in the extra field, we need to make a breaking change
and split the generated fields in two: One preceding that guarantees
alignment, and the actual storage, bit-aligned.

This keeps the existing behavior while fixing that test-case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-bitfields bug
Projects
None yet
Milestone
No milestone
2 participants
@varphone @emilio