[PATCH] Fixing a compiler assertion with zero-width bit-fields in packed structs

[John McCall]

On Feb 4, 2014, at 5:08 PM, Yunzhong Gao <Yunzhong_Gao at playstation.sony.com> wrote:
> Hi,
> The following patch fixed a compiler assertion when compiling the following test case:
> ```
> /* test.c
> * $ clang -S test.c
> */
> struct __attribute__((packed)) f { 
>  int v1:9; 
>  int :0; 
>  int v2:17;
> };
> struct f f1 = {};
> /* end of test.c */
> ```
> 
> According to the GNU docs, the zero-sized bitfields should not be affected by the packed attribute
> Reference: http://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html
> ```
> packed
>  This attribute, attached to struct or union type definition, specifies that each member (other than zero-width bit-fields)
>  of the structure or union is placed to minimize the memory required. When attached to an enum definition, it indicates
>  that the smallest integral type should be used. 
> ```
> 
> Also, the C++11 memory model requires that the members after a zero-width bit-fields be accessible as a separate
> object, which implies that they be aligned at least to byte boundaries.
> 
> I am under the impression that the packed attribute is not related to the target ABI spec and hence there
> should be no need to check the return value of "Context.getTargetInfo().useZeroLengthBitfieldAlignment()"
> in this part of the code. Please let me know if that is not the case.

That’s not strictly the case.  Packed structures are not exempt from ABI compatibility just because they’re not documented in the platform ABI; if a platform difference like useZeroLengthBitfieldAlignment() caused changes to packed structures, we’d need to emulate that.

That said, I was reviewing this patch, and I was looking at the current code, and my first thought was “if the problem is generally with zero-length bitfields that don’t cause something to start at a new byte offset, then you can get exactly that condition on a platform that doesn’t honor bitfield alignment, even on zero-length bitfields.”  So I was all set to tell you that you needed to go check the right behavior of packed structs on a platform like that.  But as far as I can tell, there is no platform like that, and the code above is dead which sets FieldAlign = 1 is dead.

So your change is fine, but please also modify the code above (within the !Context.getTargetInfo().useBitFieldTypeAlignment() block) to assert that UseZeroLengthBitfieldAlignment is true, and please leave a comment explaining that zero-width bitfields are immune from packed as a special case.

If there was a platform that didn’t respect type alignment even on zero-width bitfields, we would have to modify IR-generation to not force a new allocation unit on a zero-width bitfield (it would, of course, still break up the bitfield if it did fall on a char boundary).  The fact that this wouldn’t be consistent with the C++11 memory model is not, ultimately, something that lets us unilaterally override a platform ABI.

John.