Support 128-bit aligned unions

[leeopop]

When
https://github.com/servo/rust-bindgen/blob/4ac39de18b96bd9488a17cf2650994a3584d440c/libbindgen/src/codegen/mod.rs#L1092
this inserts new field into a struct definition, Debug or Clone is not removed if the bindgen_union_field is too large.

This is the generated code. (without unstable rust)
#[repr(C)] #[derive(Debug, Copy)] pub struct rte_thash_tuple { pub v4: __BindgenUnionField<rte_ipv4_tuple>, pub v6: __BindgenUnionField<rte_ipv6_tuple>, pub bindgen_union_field: [u8; 48usize], } impl Clone for rte_thash_tuple { fn clone(&self) -> Self { *self } }

I think can_derive_debug should be checked later or
inserted attribute should be removed later.

[emilio]

So the idea is that if all the inner fields can derive debug, the union should be able to debug too (since its alignment depends on the alignment of the fields).

This could break, I guess, if you explicitly align the union to a given width, so it's a bug (though arguable a pretty corner-case-y one).

[emilio]

Do you have the code that generated this? Is it this one? http://dpdk.org/doc/api-2.1/rte__ip_8h_source.html

[emilio]

Well, this one, I guess: http://dpdk.org/doc/api-2.1/rte__thash_8h_source.html

[emilio]

I can't replicate this with that code, and the code that adds the Debug annotation checks for the layout of that field here: https://github.com/servo/rust-bindgen/blob/master/libbindgen/src/ir/comp.rs#L850

[emilio]

Ohh, ok, there it is.

typedef unsigned char uint8_t;
typedef unsigned short uint16_t;
typedef unsigned int uint32_t;

struct rte_ipv4_tuple {
        uint32_t        src_addr;
        uint32_t        dst_addr;
        union {
                struct {
                        uint16_t dport;
                        uint16_t sport;
                };
                uint32_t        sctp_tag;
        };
};

struct rte_ipv6_tuple {
        uint8_t         src_addr[16];
        uint8_t         dst_addr[16];
        union {
                struct {
                        uint16_t dport;
                        uint16_t sport;
                };
                uint32_t        sctp_tag;
        };
};

union rte_thash_tuple {
        struct rte_ipv4_tuple   v4;
        struct rte_ipv6_tuple   v6;
} __attribute__((aligned(16)));

So the problem boils down to us not being able to generate 128-bit aligned structs. This is kind of a rust limitation until i128/u128 is stabilized though.

[emilio]

In any case, we shouldn't generate invalid code. Our alignment checker assumes we can build an array of u128, in which case this would be able to derive Debug. But the code generation says "hey! I don't know how to generate this", and falls back to byte alignment.

[emilio]

With the patch in #413 this won't generate invalid code. Still you need to make sure it's properly aligned before passing the struct to C.

[leeopop]

I meant that the generated code contains bindgen_union_field: [u8; 48usize],.
However, that array does not implement Debug trait because it is longer that 32.
The generated Rust code contains Debug because other fields __BindgenUnionField<rte_ipv4_tuple>,
and pub v6: __BindgenUnionField<rte_ipv6_tuple> are can_derive_debug.

The alignment field is added later and not Debug-able, but it is not checked.
This is the error when I tried to compile the generated Rust code.

error[E0277]: the trait bound `[u8; 48]: std::fmt::Debug` is not satisfied
     --> src/dpdk.rs:29074:5
      |
29074 |     pub bindgen_union_field: [u8; 48usize],
      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::fmt::Debug` is not implemented for `[u8; 48]`
      |
      = note: `[u8; 48]` cannot be formatted using `:?`; if it is defined in your crate, add `#[derive(Debug)]` or manually implement it
      = note: required because of the requirements on the impl of `std::fmt::Debug` for `&[u8; 48]`
      = note: required for the cast to the object type `std::fmt::Debug`

This is the generated code:

/**
 * IPv6 tuple
 * Addresses have to be filled by rte_thash_load_v6_addr()
 * ports/sctp_tag have to be CPU byte order
 */
#[repr(C)]
#[derive(Debug, Copy)]
pub struct rte_ipv6_tuple {
    pub src_addr: [u8; 16usize],
    pub dst_addr: [u8; 16usize],
    pub __bindgen_anon_1: rte_ipv6_tuple__bindgen_ty_1,
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct rte_ipv6_tuple__bindgen_ty_1 {
    pub __bindgen_anon_1: __BindgenUnionField<rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1>,
    pub sctp_tag: __BindgenUnionField<u32>,
    pub bindgen_union_field: u32,
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1 {
    pub dport: u16,
    pub sport: u16,
}
#[test]
fn bindgen_test_layout_rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1() {
    assert_eq!(::std::mem::size_of::<rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1>()
               , 4usize);
    assert_eq!(::std::mem::align_of::<rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1>()
               , 2usize);
}
impl Clone for rte_ipv6_tuple__bindgen_ty_1__bindgen_ty_1 {
    fn clone(&self) -> Self { *self }
}
#[test]
fn bindgen_test_layout_rte_ipv6_tuple__bindgen_ty_1() {
    assert_eq!(::std::mem::size_of::<rte_ipv6_tuple__bindgen_ty_1>() ,
               4usize);
    assert_eq!(::std::mem::align_of::<rte_ipv6_tuple__bindgen_ty_1>() ,
               4usize);
}
impl Clone for rte_ipv6_tuple__bindgen_ty_1 {
    fn clone(&self) -> Self { *self }
}
#[test]
fn bindgen_test_layout_rte_ipv6_tuple() {
    assert_eq!(::std::mem::size_of::<rte_ipv6_tuple>() , 36usize);
    assert_eq!(::std::mem::align_of::<rte_ipv6_tuple>() , 4usize);
}
impl Clone for rte_ipv6_tuple {
    fn clone(&self) -> Self { *self }
}
#[repr(C)]
#[derive(Debug, Copy)]
pub struct rte_thash_tuple {
    pub v4: __BindgenUnionField<rte_ipv4_tuple>,
    pub v6: __BindgenUnionField<rte_ipv6_tuple>,
    pub bindgen_union_field: [u8; 48usize],
}
impl Clone for rte_thash_tuple {
    fn clone(&self) -> Self { *self }
}

My opinion is that Debug trait should be removed from struct rte_thash_tuple
because its bindgen_union_field is longer than 32.
But currently, this is checked before the alignment field is added.

[leeopop]

If 128bit aligntmnets are possible, and the resulting alignment is longer than 32 entries, is there any checking mechanism that removes Debug from the struct?

[emilio]

Yes, that checking exists. It's added before the field, but not without checking. Our checking just didn't account for unknown alignments (that's what #413 fixes)

[leeopop]

Thank you! This hotfix #413 works for me!

[emilio]

This is partially fixed, the remaining work is figuring out what to do with stuff with >64bit alignment, which is #550.

Thanks for the report! (for all the reports, actually :))