[PR32482] Fix bitfield layout for -mms-bitfield and pragma pack

[arphaman]

Summary

Clang currently generates wrong record layout for -mms-bitfield and #pragma pack. https://godbolt.org/g/nQ4rVW shows how MSVC and GCC generate different layout to Clang.

This patch fixes the issue by ensuring that bitfields are not packed, but the record's alignment is still updated. This seems to be in line with logic used by GCC and MSVC.

rdar://36343145

[rjmccall]

lib/AST/RecordLayoutBuilder.cpp

1565

I think naming this 'FieldAlignForRecord' would be clearer.

Other this LGTM.

[arphaman]

Fix packing logic.

[arphaman]

Oops, that logic turned out to be incorrect. We simply have to start a new storage unit when the new bitfield's size is wider than the available bits.

[rjmccall]

Oh, that makes much more sense, thanks.

lib/AST/RecordLayoutBuilder.cpp

void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   // all types (e.g. Darwin PPC32, where alignof(long long) == 4).
 
   // First, some simple bookkeeping to perform for ms_struct structs.
   if (IsMsStruct) {
     // The field alignment for integer types is always the size.
     FieldAlign = TypeSize;
 
     // If the previous field was not a bitfield, or was a bitfield
-    // with a different storage unit size, we're done with that
-    // storage unit.
-    if (LastBitfieldTypeSize != TypeSize) {
+    // with a different storage unit size, or if this field doesn't fit into
+    // the current storage unit, we're done with that storage unit.
+    if (LastBitfieldTypeSize != TypeSize ||
+        UnfilledBitsInLastUnit < FieldSize) {
       // Also, ignore zero-length bitfields after non-bitfields.
       if (!LastBitfieldTypeSize && !FieldSize)
         FieldAlign = 1;
 
       UnfilledBitsInLastUnit = 0;
       LastBitfieldTypeSize = 0;
     }
   }
void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   if (ExplicitFieldAlign) {
     FieldAlign = std::max(FieldAlign, ExplicitFieldAlign);
     UnpackedFieldAlign = std::max(UnpackedFieldAlign, ExplicitFieldAlign);
   }
 
   // But, if there's a #pragma pack in play, that takes precedent over
   // even the 'aligned' attribute, for non-zero-width bitfields.
   unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment);
   if (!MaxFieldAlignment.isZero() && FieldSize) {
     UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
     if (FieldPacked)
       FieldAlign = UnpackedFieldAlign;
     else
       FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
   }
 
   // But, ms_struct just ignores all of that in unions, even explicit

test/CodeGen/mms-bitfields.c

 // CHECK: %struct.s2 = type { i32, [4 x i8], [4 x i64] }
 
 struct s3 {
   int       f32;
   struct s1 s;
 } s3;
 
 // CHECK: %struct.s3 = type { i32, [4 x i8], %struct.s1 }
+
+// PR32482:
+
+#pragma pack (push,1)
+
+typedef unsigned int UINT32;
+
+struct Inner {
+  UINT32    A    :  1;
+  UINT32    B    :  1;
+  UINT32    C    :  1;
+  UINT32    D    : 30;
+} Inner;
+
+#pragma pack (pop)
+
+// CHECK: %struct.Inner = type { i32, i32 }
+
+// CHECK: %struct.A = type { i32, i32, i32 }
+
+#pragma pack(push, 1)
+
+union HEADER {
+  struct A {
+    int                                         :  3;  // Bits 2:0
+    int a                                       :  9;  // Bits 11:3
+    int                                         :  12;  // Bits 23:12
+    int b                                       :  17;  // Bits 40:24
+    int                                         :  7;  // Bits 47:41
+    int c                                       :  4;  // Bits 51:48
+    int                                         :  4;  // Bits 55:52
+    int d                                       :  3;  // Bits 58:56
+    int                                         :  5;  // Bits 63:59
+  } Bits;
+} HEADER;
+
+#pragma pack(pop)
+
+struct Inner variable = { 1,0,1, 21 };
+union HEADER hdr = {{1,2,3,4}};
+
+// CHECK: @variable = global { i8, [3 x i8], i8, i8, i8, i8 } { i8 5, [3 x i8] undef, i8 21, i8 0, i8 0, i8 0 }, align 1
+// CHECK: @hdr = global { { i8, i8, [2 x i8], i8, i8, i8, i8, i8, [3 x i8] } } { { i8, i8, [2 x i8], i8, i8, i8, i8, i8, [3 x i8] } { i8 8, i8 0, [2 x i8] undef, i8 2, i8 0, i8 0, i8 3, i8 4, [3 x i8] undef } }, align 1

test/Sema/mms-bitfields.c

 // RUN: %clang_cc1 -mms-bitfields -fsyntax-only -verify -triple x86_64-apple-darwin9 %s
 // expected-no-diagnostics
 
 // The -mms-bitfields commandline parameter should behave the same
 // as the ms_struct attribute.
 struct
 {
    int a : 1;
    short b : 1;
 } t;
 
 // MS pads out bitfields between different types.
 static int arr[(sizeof(t) == 8) ? 1 : -1];
+
+#pragma pack (push,1)
+
+typedef unsigned int UINT32;
+
+struct Inner {
+  UINT32    A    :  1;
+  UINT32    B    :  1;
+  UINT32    C    :  1;
+  UINT32    D    : 30;
+} Inner;
+
+#pragma pack (pop)
+
+static int arr2[(sizeof(Inner) == 8) ? 1 : -1];