[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

1564

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) {
   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);
+  unsigned RecordFieldAlign = FieldAlign;
   if (!MaxFieldAlignment.isZero() && FieldSize) {
-    UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
+    // #pragma pack does not affect bitfield layout in ms_struct, but the
+    // record's alignment is still changed.
+    if (IsMsStruct) {
+      RecordFieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+    } else {
+      UnpackedFieldAlign =
+          std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
       if (FieldPacked)
         FieldAlign = UnpackedFieldAlign;
       else
         FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+      RecordFieldAlign = FieldAlign;
+    }
   }
 
   // But, ms_struct just ignores all of that in unions, even explicit
   // alignment attributes.
   if (IsMsStruct && IsUnion) {
     FieldAlign = UnpackedFieldAlign = 1;
   }
 
// bits we didn't use.
     // purposes of unfilled bits.
     LastBitfieldTypeSize = 0;
   }
 
   // Update the size.
   setSize(std::max(getSizeInBits(), getDataSizeInBits()));
 
   // Remember max struct/class alignment.
-  UpdateAlignment(Context.toCharUnitsFromBits(FieldAlign), 
+  UpdateAlignment(Context.toCharUnitsFromBits(RecordFieldAlign),
                   Context.toCharUnitsFromBits(UnpackedFieldAlign));
 }
 
 void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
                                              bool InsertExtraPadding) {
   if (D->isBitField()) {
     LayoutBitField(D);
     return;

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];