Incorrect layout with large bitfield

[fitzgen]

Follow up to #1001

Input C/C++ Header

struct {
  unsigned : 632;
} a;

Bindgen Invocation

$ bindgen input.h

Actual Results

Panic when running the layout tests: our generated struct ends up with the wrong size.

Expected Results

We generate a struct with the correct layout, and it passes its layout tests.

[fitzgen]

Here is where we allocate logical bitfields into physical units:

https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/ir/comp.rs#L432-L617

If we discover the root cause of the issue, and it doesn't appear to be a simple fix, we should investigate falling back to making the whole struct opaque. Perhaps extending impl IsOpaque for Comp with something like:

self.fields().iter().any(|f| match *f {
    Field::Bitfields(ref unit) => unit.bitfields().iter().any(|b| b.width() > 64),
    _ => false,
})

which would make any bitfield that is bigger than a u64 trigger the whole struct being opaque. I think that is the right check to do here, but I'm not sure. Maybe we should be checking the bitfield unit's layout instead.

cc @aeleos

[aeleos]

@highfive: assign me

[highfive]

Hey @aeleos! Thanks for your interest in working on this issue. It's now assigned to you!

[aeleos]

@fitzgen so when the Item info for the bitfield is being printed out, it outputs this.

ir: ItemId(1) = Item {
    id: ItemId(
        1
    ),
    local_id: Cell {
        value: Some(
            1
        )
    },
    next_child_local_id: Cell {
        value: 1
    },
    canonical_name_cache: RefCell {
        value: None
    }, "correct" behavior for rust
    comment: None,
    annotations: Annotations {
        opaque: false,
        hide: false,
        use_instead_of: None,
        disallow_copy: false,
        private_fields: None,
        accessor_kind: None,
        constify_enum_variant: false
    },
    parent_id: ItemId(
        0
    ),
    kind: Type(
        Type {
            name: None,
            layout: Some(
                Layout {
                    size: 80,
                    align: 8,
                    packed: false
                }
            ),
            kind: Comp(
                CompInfo {
                    kind: Struct,
                    fields: AfterComputingBitfieldUnits(
                        [
                            Bitfields(
                                BitfieldUnit {
                                    nth: 1,
                                    layout: Layout {
                                        size: 128,
                                        align: 128,
                                        packed: false
                                    },
                                    bitfields: []
                                }
                            )
                        ]
                    ),
                    template_params: [],
                    methods: [],
                    constructors: [],
                    destructor: None,
                    base_members: [],
                    inner_types: [],
                    inner_vars: [],
                    has_own_virtual_method: false,
                    has_destructor: false,
                    has_nonempty_base: false,
                    has_non_type_template_params: false,
                    packed: false,
                    found_unknown_attr: false,
                    is_forward_declaration: false
                }
            ),
            is_const: false
        }
    )
}

So there are 2 different layouts being generated, one with size 80, align 8 and the other with size 128, align 128. Do you know where each of them are generated and what each of them means?

Another question I have is that when the header in question is compiled with clang++ it gives this warning

tests/headers/bitfield_large_overflow.hpp:2:12: warning: width of anonymous bit-field (632 bits) exceeds width of
      its type; value will be truncated to 32 bits [-Wbitfield-width]
  unsigned : 632;
           ^

If clang is truncating the value to 32 bits, does that mean the correct behavior would be to treat it as a 32 bit bitfield?

[fitzgen]

So there are 2 different layouts being generated, one with size 80, align 8 and the other with size 128, align 128. Do you know where each of them are generated and what each of them means?

The former is the type's layout we get directly from libclang, the latter is the layout we compute for the bitfield allocation unit. I linked where the latter was computed in an earlier comment. The former is the result of calling clang::Type::{size, align} from src/clang.rs.

If clang is truncating the value to 32 bits, does that mean the correct behavior would be to treat it as a 32 bit bitfield?

It sounds like we should be performing that same truncation inside the bitfield method in impl FieldMethod for FieldData. We should also warn!("..."), with a similar message that clang has.

I expect this is the root cause and performing the truncation is the correct fix.

Does gcc have the same behavior?

[aeleos]

Here is the warning from gcc

tests/headers/bitfield_large_overflow.hpp:2:14: warning: width of ‘<anonymous struct>::<anonymous>’ exceeds its type
   unsigned : 632;
              ^
tests/headers/bitfield_large_overflow.hpp:3:3: warning: anonymous type with no linkage used to declare variable ‘<anonymous struct> a’ with linkage
 } a;
   ^

It doesn't explicitly say that it is truncating, so I am not 100% sure, but I think that is what it is doing.
Another interesting behavior is that this only actually works with c++. Both gcc and clang fail to compile this code if the file is a .h, rather than just throwing an warning.

test.h:2:3: error: width of ‘<anonymous>’ exceeds its type
   unsigned : 632;
   ^

[fitzgen]

Looks like g++ agrees with clang++ on a size of 80:

$ cat big-bitfield.cpp 
struct HasBigBitfield {
    unsigned : 632;
};

int f(HasBigBitfield*);

int main() {
    HasBigBitfield b;
    return f(&b);
}
$ g++ -g -c big-bitfield.cpp -o big-bitfield.o
big-bitfield.cpp:2:16: warning: width of ‘HasBigBitfield::<anonymous>’ exceeds its type
     unsigned : 632;
                ^~~
$ dwarfdump big-bitfield.o
...
< 1><0x0000002d>    DW_TAG_structure_type
                      DW_AT_name                  HasBigBitfield
                      DW_AT_byte_size             0x00000050
                      DW_AT_decl_file             0x00000001 /home/fitzgen/scratch/big-bitfield.cpp
                      DW_AT_decl_line             0x00000001
...
$ 

[fitzgen]

Another interesting behavior is that this only actually works with c++. Both gcc and clang fail to compile this code if the file is a .h, rather than just throwing an warning.

I think we can ignore this because libclang will fail to parse + type check the C headers before we even get our hands on it.

[aeleos]

@fitzgen Another interesting thing I just found, the actual type assigned to the anonymous bitfield doesn't change the size of the struct.

$ cat big-bitfield.cpp 
struct HasBigBitfield {
    unsigned char: 632;
};

int f(HasBigBitfield*);

int main() {
    HasBigBitfield b;
    return f(&b);
}
$ clang++ -g -c big-bitfield.cpp -o big-bitfield.o
big-bitfield.cpp:2:19: warning: width of anonymous bit-field (632 bits) exceeds width of its type; value will be
      truncated to 8 bits [-Wbitfield-width]
    unsigned char : 632;
                  ^
1 warning generated.
$ dwarfdump big-bitfield.o
...
< 1><0x0000002a>    DW_TAG_structure_type
                      DW_AT_name                  "HasBigBitfield"
                      DW_AT_byte_size             0x00000050
                      DW_AT_decl_file             0x00000001 /home/oliver/rust-bindgen/big-bitfield.cpp
                      DW_AT_decl_line             0x00000001
...
$ 

g++ also has the same behavior. Considering that it specifically says that the value is being truncated, I find it odd that the memory is being allocated to fit the entire bitfield.

I tried a few different combinations of bitfield sizes are the memory size pattern seems to be 0x40, 0x48, 0x50, 0x58, etc. Is it possible to just check whatever memory size libclang outputs and use that as the memory size? But does that mean we are just allocating extra memory for no reason?

[fitzgen]

The number one priority is to end up with the same physical representation as the C code. If we don't, then passing these structs across FFI boundaries will lead to memory unsafety.

That certainly is odd behavior. I think the best thing to do is to treat the whole struct as opaque if any bitfields are wider than its type. So we would add something like this to impl IsOpaque for Comp:

self.field().iter().any(|f| match *f {
    Field::DataMember(_) => false,
    Field::Bitfields(ref unit) => unit.bitfields().iter().any(|bf| {
        bf.width() / 8 > ctx.resolve_type(bf.ty()).layout().size()
    })
})

(Probably with a comment explaining why we're checking bitfield widths there and linking back to this issue)

[aeleos]

@fitzgen sorry about getting back to you so late, I have been busy the past few days. Do you mean add it to impl IsOpaque for CompInfo? I can't find anything other than that.

[fitzgen]

Yep!

$ git grep "impl IsOpaque"
src/ir/comp.rs:1455:impl IsOpaque for CompInfo {
# ...

[aeleos]

@fitzgen so I tried to implement something, but there are a few issues with it. This is my new is_opaque function

    fn is_opaque(&self, ctx: &BindgenContext, _: &()) -> bool {
        //Early return to avoid extra computation
        if self.has_non_type_template_params {
            return self.has_non_type_template_params
        }

        let has_large_bitfield = self.fields().iter().any(|f| match *f {
            Field::DataMember(_) => {
                false
            },
            Field::Bitfields(ref unit) => {
                    println!("Bitfield Unit: {:?}", unit);
                    unit.bitfields().iter().any(|bf| {
                    let bitfield_layout = ctx.resolve_type(bf.ty())
                        .layout(ctx)
                        .expect("Bitfield without layout? Gah!");
                    println!("Bitfield width: {}", bf.width());
                    bf.width() / 8 > bitfield_layout.size as u32
                })
            }
        });

        println!("Does have large bitfield: {}", has_large_bitfield);

        self.has_non_type_template_params || has_large_bitfield
    }

Right now when it runs with the failing test case, I get this as the output

Bitfield Unit: BitfieldUnit { nth: 1, layout: Layout { size: 128, align: 128, packed: false }, bitfields: [] }
Does have large bitfield: false

I am not sure how the BitfieldUnit struct is supposed to work, but there is nothing inside the bitfields array if in this case. Since we can't see how large the bitfield is in any way other than the layout according to this, what do you think is the best thing to to do?

I did also notice that this line is output
ERROR:bindgen::codegen::struct_layout: Calculated wrong layout for _bindgen_ty_1, too more 40 bytes
It seems that there is at least one point where the issues with the size and layout are found, is it possible to just copy that functionality and if it has been miscalculated then we make the struct opaque?

[fitzgen]

This is my new is_opaque function

That looks like what I'd expect, although

        if self.has_non_type_template_params {
            return self.has_non_type_template_params
        }

should be

        if self.has_non_type_template_params {
            return true;
        }

and then we don't need to check self.has_non_type_template_params again at the end.

I am not sure how the BitfieldUnit struct is supposed to work,

Pretty much what it says on the tin:

/// A contiguous set of logical bitfields that live within the same physical
/// allocation unit. See 9.2.4 [class.bit] in the C++ standard and [section
/// 2.4.II.1 in the Itanium C++
/// ABI](http://itanium-cxx-abi.github.io/cxx-abi/abi.html#class-types).

Logical bitfields get allocated into physical units. The units end up as a field in the resulting struct, and the bitfields become getters/setters that are masking out everything but their own relevant bits from the unit.

but there is nothing inside the bitfields array if in this case.

And I think this is because we eagerly filter out anonymous bitfields when computing which bitfields end up in which units. We're going to have to stop doing that filtering to make these changes work.

https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/ir/comp.rs#L583-L589

Sorry I didn't remember this earlier.

Removing the eager filtering, removing the Bitfield::name(&self) -> &str method in favor for of the FieldMethods::name(&self) -> Option<&str> method and updating callers (mostly in codegen) to deal with and ignore anonymous bitfileds could be its own little PR. After that was done, it would make circling back to this issue's changes easier.

I did also notice that this line is output
ERROR:bindgen::codegen::struct_layout: Calculated wrong layout for _bindgen_ty_1, too more 40 bytes
It seems that there is at least one point where the issues with the size and layout are found, is it possible to just copy that functionality and if it has been miscalculated then we make the struct opaque?

That would be desirable, but is a bit orthogonal to this issue, IMO. It also depends on #959. If you want to help out, it would be very appreciated :) But I don't think it needs to block this issue.

---

Does that help clear things up? Anything not make sense? Feel like you know where to go from here?

Cheers! 😸

[aeleos]

@fitzgen I think I get whats happening, here is what I am thinking are the next steps

1. Remove the Bitfield::name(&self) -> &str method and replace all calls with the FieldMethods version
2. Remove the if statement around checking if a bitfield is anonymous before adding it into the array
3. Check to make sure these changes fix the new is_opaque function
4. Submit PR for the above changes referencing this issue
5. Submit PR for this issue with new is_opaque function and some tests to make sure everything is working

That seems like everything does that sound right to you?

[fitzgen]

That sounds 💯 !

[aeleos]

@fitzgen so the first issue I have run into is with this line
let param_name = bitfield_getter_name(ctx, parent, bf.name());
What is the best way to handle an anonymous bitfield name here? Is there a function that a name for anonymous fields? Or can it just be set to something random if its going removed when the struct is made opaque later?

[fitzgen]

I code generation, we can simply skip bitfields that don't have names. The allocation units are always emitted, so skipping the bitfield won't cause the struct layout to go wrong, unlike normal data members.