[PowerPC][AIX] Packed zero-width bitfields do not affect alignment.

[sfertile]

Summary

Zero-width bitfields on AIX pad out to the natral alignment boundary but do not change the containing records alignment when they are packed.

[sfertile]

clang-formatted

[stevewan]

LGTM with some nits.

clang/lib/AST/RecordLayoutBuilder.cpp

1781

Just noting that the comment says MaxFieldAlignment - The maximum allowed field alignment. This is set by #pragma pack, but __attribute__(packed) also seems to set it to some large value that is at least as large as the FieldAlign. Maybe edit the comment accordingly for now, and a future follow-on patch if necessary.

clang/test/Layout/aix-packed-bitfields.c

95

nit: might be helpful to use a different type for the zero-width bitfield here. (e.g., long long : 0

[ZarkoCA]

LGTM also. Just a comment typo.

clang/lib/AST/RecordLayoutBuilder.cpp

1779

1779

[sfertile]

clang/lib/AST/RecordLayoutBuilder.cpp

1781

Sorry Steven, not sure what you are asking for.

attribute packed will set 'FieldPacked` to true, in which case we ignore the max field alignment (and why the new comment says, or 1 if packed). IIUC the MaXFieldAlign is set by only by pragma pack and pragma align in the Itanium recored layout builder, and the place where it is set based on attribute packed is int he Microsoft record layout builder and doesn't affect us here.

[sfertile]

Fixed spelling and added a 'long long' zero width bitfield to the testing.

[stevewan]

clang/lib/AST/RecordLayoutBuilder.cpp

1781

Sorry Sean, my previous comment was indeed confusing. The old buggy behaviour confused me, and for some reason I thought the code must had went down this if statement when attribute packed is set, hence the previous comment. Now that I take a second look, I realize the problem was actually that we never went into the if to fix the FieldAlign. Sorry for the churn.

[stevewan]

clang/lib/AST/RecordLayoutBuilder.cpp

1783

UnpackedFieldAlign is used to check if the packed attribute is unnecessary (-Wpacked). here the attribute is making a difference, we probably shouldn't set FieldAlign = UnpackedFieldAlign?

[sfertile]

Don't update the unpacked field align based on IsPacked.

[sfertile]

clang/lib/AST/RecordLayoutBuilder.cpp

1783

Good catch, updated.

[stevewan]

LGTM. Thanks.

clang/lib/AST/RecordLayoutBuilder.cpp

void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
 
   // Anonymous members don't affect the overall record alignment,
   // except on targets where they do.
   if (!IsMsStruct &&
       !Context.getTargetInfo().useZeroLengthBitfieldAlignment() &&
       !D->getIdentifier())
     FieldAlign = UnpackedFieldAlign = 1;
 
-  // On AIX, zero-width bitfields pad out to the alignment boundary, but then
-  // do not affect overall record alignment if there is a pragma pack or
-  // pragma align(packed).
-  if (isAIXLayout(Context) && !MaxFieldAlignment.isZero() && !FieldSize)
+  // On AIX, zero-width bitfields pad out to the natural alignment boundary,
+  // but dont increase the alignment greater than the MaxFieldAlignment, or 1
+  // if packed.
+  if (isAIXLayout(Context) && !FieldSize) {
+    if (FieldPacked) {
+      FieldAlign = UnpackedFieldAlign = 1;
+    } else if (!MaxFieldAlignment.isZero()) {
+      UnpackedFieldAlign =
+          std::min(UnpackedFieldAlign, MaxFieldAlignmentInBits);
       FieldAlign = std::min(FieldAlign, MaxFieldAlignmentInBits);
+    }
+  }
 
   // Diagnose differences in layout due to padding or packing.
   if (!UseExternalLayout)
     CheckFieldPadding(FieldOffset, UnpaddedFieldOffset, UnpackedFieldOffset,
                       UnpackedFieldAlign, FieldPacked, D);
 
   // Update DataSize to include the last byte containing (part of) the bitfield.
 

clang/test/Layout/aix-packed-bitfields.c

 int d = sizeof(struct Pack2);
 
 // CHECK:      *** Dumping AST Record Layout
 // CHECK-NEXT:      0 | struct Pack2
 // CHECK-NEXT: 0:0-29 |   int a1
 // CHECK-NEXT: 3:6-35 |   int a2
 // CHECK-NEXT:  7:4-7 |   int a3
 // CHECK-NEXT:          sizeof=8, {{(dsize=8, )?}}align=2, preferredalign=2
+//
+struct __attribute__((packed)) PackedAttr {
+  char f1;
+  int : 0;
+  short : 3;
+  char f4 : 2;
+};
+
+int e = sizeof(struct PackedAttr);
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:      0 | struct PackedAttr
+// CHECK-NEXT:      0 |   char f1
+// CHECK-NEXT:    4:- |   int
+// CHECK-NEXT:  4:0-2 |   short
+// CHECK-NEXT:  4:3-4 |   char f4
+// CHECK-NEXT:          sizeof=5, {{(dsize=5, )?}}align=1, preferredalign=1
+
+#pragma pack(2)
+struct __attribute__((packed)) PackedAttrAndPragma {
+  char f1;
+  int : 0;
+};
+#pragma pack(pop)
+
+int f = sizeof(struct PackedAttrAndPragma);
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:      0 | struct PackedAttrAndPragma
+// CHECK-NEXT:      0 |   char f1
+// CHECK-NEXT:    4:- |   int
+// CHECK-NEXT:          sizeof=4, {{(dsize=4, )?}}align=1, preferredalign=1