[AIX] Implement AIX special bitfield related alignment rules

[Xiangling_L]

Summary

1.[bool, char, short] bitfields have the same alignment as unsigned int
2.Adjust alignment on typedef field decls/honor align attribute
3.Fix alignment for scoped enum class
4.Long long bitfield has 4bytes alignment and StorageUnitSize under 32 bit compile mode

[Xiangling_L]

Rebased on the pragma/pack patch;
Added packed related testcase for bitfield;

[jasonliu]

I think it would help the review if we could put the NFC portion(e.g. TypeSize -> StorageUnitSize) to a new patch, and give some rationale about the NFC change.

clang/lib/AST/RecordLayoutBuilder.cpp

626–627

[Xiangling_L]

In D87029#2285398, @jasonliu wrote:

I think it would help the review if we could put the NFC portion(e.g. TypeSize -> StorageUnitSize) to a new patch, and give some rationale about the NFC change.

Sure, will do.

[Xiangling_L]

Rebased on the NFC patch;

[sfertile]

clang/lib/Sema/SemaDecl.cpp

16447 ↗

(On Diff #294157)

Can this change can be split out into its own patch? If it can i would suggest doing so.

clang/test/Layout/aix-oversized-bitfield.cpp

1 ↗

(On Diff #294157)

Is it possible to run this test as both C and C++?

[sfertile]

clang/lib/AST/RecordLayoutBuilder.cpp

1644

I get different results for using a long long bitfield, and using an enum with an underlying long long type:

struct S {
    unsigned long long  field : 32;
};

extern "C" int printf(const char*, ...);

int main(void) {
    printf("%lu %lu\n", sizeof(struct S), alignof(struct S));
}

*** Dumping AST Record Layout
         0 | struct S
    0:0-31 |   unsigned long long field
           | [sizeof=4, dsize=4, align=4, preferredalign=4,
           |  nvsize=4, nvalign=4, preferrednvalign=4]

---

enum e : unsigned long long { E = 1 };

struct S {
    enum e field : 32;
};

extern "C" int printf(const char*, ...);

int main(void) {
    printf("%lu %lu\n", sizeof(struct S), alignof(struct S));
}

*** Dumping AST Record Layout
         0 | struct S
    0:0-31 |   enum e field
           | [sizeof=8, dsize=8, align=8, preferredalign=8,
           |  nvsize=8, nvalign=8, preferrednvalign=8]

Shouldn't we be truncating the size/align of the enum typed bitfield as well?

[sfertile]

clang/test/Layout/aix-bitfield-alignment.cpp

119

We should have a similar test for an overaligned long or long long.

[Xiangling_L]

clang/lib/Sema/SemaDecl.cpp

16447 ↗

(On Diff #294157)

I was expecting our buildbot can pick up all bitfield related changes at one time. Also if we split this out, that means we either need to wait for this second part to land first or need to add more LIT to oversized long long to the first part, which then needs to be removed whenever this second part land. It seems we are complicating the patch. Can you give me your rationale about why we want to split out this part?

[Xiangling_L]

• Fixed the bug of getting underlying type of enum;
• Fixed the bug to respect align attribute;
• Add more testcases;

[sfertile]

clang/lib/Sema/SemaDecl.cpp

16447 ↗

(On Diff #294157)

I was expecting our buildbot can pick up all bitfield related changes at one time.

IIUC clang/test/Layout/aix-oversized-bitfield.cpp works with just this change and isn't dependent on D87702. Its disjoint from the other changes in this patch, and packaging it into a commit with unrelated changes even if they are on the same theme is not beneficial. Its better to have those run through the build bot (or be bisectable) as distinct changes.

Also if we split this out, that means we either need to wait for this second part to land first or need to add more LIT to oversized long long to the first part, which then needs to be removed whenever this second part land. It seems we are complicating the patch.

I don't understand why it would need to wait or require extra testing to be added. Its a diagnostic and your lit test shows the error for 32-bit (where we want it emitted) and expected layout for 64-bit. The whole point of splitting it out is that its simple,does exactly one thing, is testable on its own, and we don't need the context of the other changes packaged with it to properly review it. I am asking to split it out because I see it as making this easier to review and commit.

[Xiangling_L]

clang/lib/Sema/SemaDecl.cpp

16447 ↗

(On Diff #294157)

Sure, I will split this patch into two as you suggested. By either need to wait for this second part to land first or need to add more LIT , I thought we would like to also add test coverage and later remove it for oversize bitfield. Since StorageUnitSize > 32 && Context.getTargetInfo().getTriple().isArch32Bit() does affect how oversize bitfield get laid out on AIX. But I guess it's more convenient to just split this patch as you suggested.

[Xiangling_L]

Remove emit errors for oversized long long bitfield and related testcase;

[sfertile]

clang/lib/AST/RecordLayoutBuilder.cpp

1633

When can BTy be null? Should this instead be an assert?

Instead can we implement this without looking at the underlying type of the bitfield?
Considering StorageUnitSize:

• for types smaller then unsigned we always increase the storage size to that of unsigned.
• for int, and long in 32-bit the storage size is already 32 and we don't adjust it.
• for long long in 32-bit we decrease the size to 32.
• For long long and long in 64-bit we leave the storage size as 64.

This could be implemented as

if (StorageUnitSize <  Context.getTypeSize(Context.UnsignedIntTy) || (StorageUnitSize > Context.getTypeSize(Context.UnsignedIntTy) &&  Context.getTargetInfo().getTriple().isArch32Bit()))
  StorageUnitSize =   Context.getTypeSize(Context.UnsignedIntTy);

Without relying on extracting the underlying type and having to worry about the case where BTy is null.
If we can also handle FieldAlign without knowing the underlying type then I think we should do so and avoid trying to extract the underlying type altogether.

1638

to unsigned --> to alignof(unsigned)

clang/test/Layout/aix-bitfield-alignment.cpp

1

I suggest we split the test cases which are C++ only into a sperate file, and run the cases that are valid in both C and C++ as both to ensure there are no differences between C and C++ layout fir bit fields.

[Xiangling_L]

Split testcases;
Simplified code;
Don't need to respect attribute align within typedef when the alignment value is less than bitcontainer size;

clang/test/Layout/aix-bitfield-alignment.cpp

1

Good idea. I split the testcase as you suggested. But since C and C++ layout dumping are slightly different in format, the CHECKs are adjusted accordingly to avoid making testcase too messy.

[Xiangling_L]

ping.

[Xiangling_L]

Rebased on the latest master and updated the testcase accordingly;

[sfertile]

LGTM.

clang/lib/AST/RecordLayoutBuilder.cpp

protected:
   unsigned IsNaturalAlign : 1;
 
   unsigned IsMsStruct : 1;
 
   /// UnfilledBitsInLastUnit - If the last field laid out was a bitfield,
   /// this contains the number of bits in the last unit that can be used for
   /// an adjacent bitfield if necessary.  The unit in question is usually
   /// a byte, but larger units are used if IsMsStruct.
   unsigned char UnfilledBitsInLastUnit;
 
   /// LastBitfieldStorageUnitSize - If IsMsStruct, represents the size of the
   /// storage unit of the previous field if it was a bitfield.
   unsigned char LastBitfieldStorageUnitSize;
 
   /// MaxFieldAlignment - The maximum allowed field alignment. This is set by
   /// #pragma pack.
   CharUnits MaxFieldAlignment;
 
void ItaniumRecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
 
   // Update the size.
   setSize(std::max(getSizeInBits(), getDataSizeInBits()));
 
   // Remember max struct/class alignment.
   UpdateAlignment(TypeAlign);
 }
 
+static bool isAIXLayout(const ASTContext &Context) {
+  return Context.getTargetInfo().getTriple().getOS() == llvm::Triple::AIX;
+}
+
 void ItaniumRecordLayoutBuilder::LayoutBitField(const FieldDecl *D) {
   bool FieldPacked = Packed || D->hasAttr<PackedAttr>();
   uint64_t FieldSize = D->getBitWidthValue(Context);
   TypeInfo FieldInfo = Context.getTypeInfo(D->getType());
   uint64_t StorageUnitSize = FieldInfo.Width;
   unsigned FieldAlign = FieldInfo.Align;
+  bool AlignIsRequired = FieldInfo.AlignIsRequired;
 
   // UnfilledBitsInLastUnit is the difference between the end of the
   // last allocated bitfield (i.e. the first bit offset available for
   // bitfields) and the end of the current data size in bits (i.e. the
   // first bit offset available for non-bitfields).  The current data
   // size in bits is always a multiple of the char size; additionally,
   // for ms_struct records it's also a multiple of the
   // LastBitfieldStorageUnitSize (if set).
if (LastBitfieldStorageUnitSize != StorageUnitSize ||
       if (!LastBitfieldStorageUnitSize && !FieldSize)
         FieldAlign = 1;
 
       UnfilledBitsInLastUnit = 0;
       LastBitfieldStorageUnitSize = 0;
     }
   }
 
+  // On AIX, [bool, char, short] bitfields have the same alignment
+  // as [unsigned].
+  if (isAIXLayout(Context)) {
+    const BuiltinType *BTy = nullptr;
+
+    // Get the underlying type from EnumDecl.
+    if (const EnumType *ET = dyn_cast<EnumType>(
+            D->getType().getDesugaredType(Context).getTypePtr())) {
+      const EnumDecl *ED = ET->getDecl();
+      if (const BuiltinType *BT =
+              dyn_cast<BuiltinType>(ED->getPromotionType().getTypePtr()))
+        BTy = BT;
+    } else {
+      BTy = Context.getBaseElementType(D->getType())->getAs<BuiltinType>();
+    }
+
+    if (BTy) {
+      BuiltinType::Kind BTyKind = BTy->getKind();
+      if (BTyKind != BuiltinType::ULong && BTyKind != BuiltinType::Long &&
+          BTyKind != BuiltinType::ULongLong &&
+          BTyKind != BuiltinType::LongLong) {
+        // Only set bitfields alignment to unsigned when it does
+        // not have attribute align specified or is less than
+        // unsigned alignment.
+        if (FieldAlign < Context.getTypeSize(Context.UnsignedIntTy))
+          FieldAlign = Context.getTypeSize(Context.UnsignedIntTy);
+        StorageUnitSize = Context.getTypeSize(Context.UnsignedIntTy);
+      } else {
+        // Handle AIX oversized Long Long bitfield under 32 bit compile mode.
+        if (StorageUnitSize > 32 &&
+            Context.getTargetInfo().getTriple().isArch32Bit()) {
+          StorageUnitSize = 32;
+          if (!AlignIsRequired)
+            FieldAlign = 32;
+        }
+      }
+    }
+  }
+
   // If the field is wider than its declared type, it follows
-  // different rules in all cases.
-  if (FieldSize > StorageUnitSize) {
+  // different rules in all cases, except on AIX.
+  // On AIX, wide bitfield follows the same rules as normal bitfield.
+  if (FieldSize > StorageUnitSize && !isAIXLayout(Context)) {
     LayoutWideBitField(FieldSize, StorageUnitSize, FieldPacked, D);
     return;
   }
 
   // Compute the next available bit offset.
   uint64_t FieldOffset =
     IsUnion ? 0 : (getDataSizeInBits() - UnfilledBitsInLastUnit);
 

clang/test/Layout/aix-bitfield-alignment.cpp

+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN:     -fsyntax-only -faix-pragma-pack -x c++ %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK32 %s
+
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -fdump-record-layouts \
+// RUN:     -fsyntax-only -faix-pragma-pack -x c++ %s | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK64 %s
+
+struct A {
+  bool b : 3;
+  unsigned char c : 2;
+  unsigned short s : 6;
+};
+
+int a = sizeof(A);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct A
+// CHECK-NEXT:      0:0-2 |   _Bool b
+// CHECK-NEXT:      0:3-4 |   unsigned char c
+// CHECK-NEXT:     0:5-10 |   unsigned short s
+// CHECK-NEXT:            | [sizeof=4, dsize=4, align=4, preferredalign=4,
+// CHECK-NEXT:            |  nvsize=4, nvalign=4, preferrednvalign=4]
+
+struct B {
+  char c;
+  int : 0;
+};
+
+int b = sizeof(B);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct B
+// CHECK-NEXT:          0 |   char c
+// CHECK-NEXT:        4:- |   int
+// CHECK-NEXT:            | [sizeof=4, dsize=4, align=4, preferredalign=4,
+// CHECK-NEXT:            |  nvsize=4, nvalign=4, preferrednvalign=4]
+
+struct C {
+  signed int a1 : 6;
+  signed char a2 : 4;
+  short int a3 : 2;
+  int a4 : 2;
+  signed long a5 : 5;
+  long long int a6 : 6;
+  unsigned long a7 : 8;
+};
+
+int c = sizeof(C);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct C
+// CHECK-NEXT:      0:0-5 |   int a1
+// CHECK-NEXT:      0:6-9 |   signed char a2
+// CHECK-NEXT:      1:2-3 |   short a3
+// CHECK-NEXT:      1:4-5 |   int a4
+// CHECK-NEXT:     1:6-10 |   long a5
+// CHECK-NEXT:      2:3-8 |   long long a6
+// CHECK32:         4:0-7 |   unsigned long a7
+// CHECK32:               | [sizeof=8, dsize=8, align=4, preferredalign=4,
+// CHECK32:               |  nvsize=8, nvalign=4, preferrednvalign=4]
+// CHECK64:         3:1-8 |   unsigned long a7
+// CHECK64:               | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK64:               |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+#pragma align(packed)
+struct C1 {
+  signed int a1 : 6;
+  signed char a2 : 4;
+  short int a3 : 2;
+  int a4 : 2;
+  signed long a5 : 5;
+  long long int a6 : 6;
+  unsigned long a7 : 8;
+};
+#pragma align(reset)
+
+int c1 = sizeof(C1);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct C1
+// CHECK-NEXT:      0:0-5 |   int a1
+// CHECK-NEXT:      0:6-9 |   signed char a2
+// CHECK-NEXT:      1:2-3 |   short a3
+// CHECK-NEXT:      1:4-5 |   int a4
+// CHECK-NEXT:     1:6-10 |   long a5
+// CHECK-NEXT:      2:3-8 |   long long a6
+// CHECK-NEXT:      3:1-8 |   unsigned long a7
+// CHECK-NEXT:            | [sizeof=5, dsize=5, align=1, preferredalign=1,
+// CHECK-NEXT:            |  nvsize=5, nvalign=1, preferrednvalign=1]
+
+#pragma pack(4)
+struct C2 {
+  signed int a1 : 6;
+  signed char a2 : 4;
+  short int a3 : 2;
+  int a4 : 2;
+  signed long a5 : 5;
+  long long int a6 : 6;
+  unsigned long a7 : 8;
+};
+#pragma pack(pop)
+
+int c2 = sizeof(C2);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct C2
+// CHECK-NEXT:      0:0-5 |   int a1
+// CHECK-NEXT:      0:6-9 |   signed char a2
+// CHECK-NEXT:      1:2-3 |   short a3
+// CHECK-NEXT:      1:4-5 |   int a4
+// CHECK-NEXT:     1:6-10 |   long a5
+// CHECK-NEXT:      2:3-8 |   long long a6
+// CHECK-NEXT:      3:1-8 |   unsigned long a7
+// CHECK-NEXT:            | [sizeof=8, dsize=8, align=4, preferredalign=4,
+// CHECK-NEXT:            |  nvsize=8, nvalign=4, preferrednvalign=4]
+
+typedef __attribute__((aligned(32))) short mySHORT;
+struct D {
+  char c : 8;
+  mySHORT : 0;
+};
+
+int d = sizeof(D);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct D
+// CHECK-NEXT:      0:0-7 |   char c
+// CHECK-NEXT:       32:- |   mySHORT
+// CHECK-NEXT:            | [sizeof=32, dsize=32, align=32, preferredalign=32,
+// CHECK-NEXT:            |  nvsize=32, nvalign=32, preferrednvalign=32]
+
+typedef __attribute__((aligned(32))) long myLONG;
+struct D1 {
+  char c : 8;
+  myLONG : 0;
+};
+
+int d1 = sizeof(D1);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct D1
+// CHECK-NEXT:      0:0-7 |   char c
+// CHECK-NEXT:       32:- |   myLONG
+// CHECK-NEXT:            | [sizeof=32, dsize=32, align=32, preferredalign=32,
+// CHECK-NEXT:            |  nvsize=32, nvalign=32, preferrednvalign=32]
+
+typedef __attribute__((aligned(32))) long long myLONGLONG;
+struct D2 {
+  char c : 8;
+  myLONGLONG : 0;
+};
+
+int d2 = sizeof(D2);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct D2
+// CHECK-NEXT:      0:0-7 |   char c
+// CHECK-NEXT:       32:- |   myLONGLONG
+// CHECK-NEXT:            | [sizeof=32, dsize=32, align=32, preferredalign=32,
+// CHECK-NEXT:            |  nvsize=32, nvalign=32, preferrednvalign=32]
+
+enum class Bool : bool { False = 0,
+                         True = 1 };
+
+struct E {
+  Bool b : 1;
+};
+
+int e = sizeof(E);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct E
+// CHECK-NEXT:      0:0-0 |   enum Bool b
+// CHECK-NEXT:            | [sizeof=4, dsize=4, align=4, preferredalign=4,
+// CHECK-NEXT:            |  nvsize=4, nvalign=4, preferrednvalign=4]
+
+enum LL : unsigned long long { val = 1 };
+
+struct F {
+  enum LL e : 32;
+};
+
+int f = sizeof(F);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct F
+// CHECK-NEXT:     0:0-31 |   enum LL e
+// CHECK32-NEXT:            | [sizeof=4, dsize=4, align=4, preferredalign=4,
+// CHECK32-NEXT:            |  nvsize=4, nvalign=4, preferrednvalign=4]
+// CHECK64-NEXT:            | [sizeof=8, dsize=8, align=8, preferredalign=8,
+// CHECK64-NEXT:            |  nvsize=8, nvalign=8, preferrednvalign=8]
+
+enum LL1 : unsigned long long { val1 = 1 } __attribute__((aligned(16)));
+struct F1 {
+  enum LL1 e : 32;
+};
+
+int f1 = sizeof(F1);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct F1
+// CHECK-NEXT:     0:0-31 |   enum LL1 e
+// CHECK-NEXT:            | [sizeof=16, dsize=16, align=16, preferredalign=16,
+// CHECK-NEXT:            |  nvsize=16, nvalign=16, preferrednvalign=16]
+
+struct G {
+  long long l : 32 __attribute__((aligned(16)));
+};
+
+int g = sizeof(G);
+
+// CHECK:      *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct G
+// CHECK-NEXT:     0:0-31 |   long long l
+// CHECK-NEXT:            | [sizeof=16, dsize=16, align=16, preferredalign=16,
+// CHECK-NEXT:            |  nvsize=16, nvalign=16, preferrednvalign=16]