Do not honor explicit alignment attribute on fields for PS4.

[Sunil_Srivastava]

Summary

This is the round 2 of the PS4 ABI. In this round:

1. A new value PS4 has been added to TargetCXXABI::Kind. It is being used for x86_64-scei-ps4 triple only.
2. RecordLayoutBuilder.cpp has been logically reverted back to pre r257462 behavior for PS4 abi.
3. The test Sema/bitfield-layout.c has been enhanced by adding the PS4 triple, and few test entries that differ between PS4 and other triples, have been put under '#ifdef PS4'. Logically, the test has not changed for triples other than x86_64-scei-ps4. For x86_64-scei-ps4 triple, the test matches pre r257462 behavior.

The test passes on all listed triples on x86 Linux and windows hosts.

[rsmith]

lib/AST/RecordLayoutBuilder.cpp

1598–1599

Please say what that behavior was here. "The PS4 ABI ignores explicit alignment attributes on bitfields." or similar.

[rjmccall]

include/clang/Basic/TargetCXXABI.h

118 ↗

(On Diff #46584)

I'm not sure why you added a new C++ ABI kind here. The bug fix you're opting out of is not at all specific to C++, and there are more straightforward ways to check the target than checking the C++ ABI kind.

I mean, I have no doubt that eventually there will be some significant C++ ABI bug fix that you don't want to pick up, so I'm not opposed to adding a new C++ ABI kind. It just seems inappropriate to do that in this patch.

lib/AST/RecordLayoutBuilder.cpp

1598–1599

Also, this should be added as a bit on TargetInfo instead of making it a target-specific check in random code. This also makes it easier to make the code self-documenting, because instead of:

} else if (ExplicitFieldAlign && !isPS4ABI) {

you'd have

} else if (ExplicitFieldAlign && Context.getTargetInfo().useExplicitBitFieldAlignment()) {

You can then document the fact that your ABI is explicitly opting out of this bug fix in a comment on the line in Targets.cpp where you set this field to false.

[probinson]

include/clang/Basic/TargetCXXABI.h

118 ↗

(On Diff #46584)

The alignment attribute is affecting layout, and layout would seem to be an ABI thing.

[rjmccall]

include/clang/Basic/TargetCXXABI.h

118 ↗

(On Diff #46584)

The CXXABI types dictate C++-specific ABI details, e.g. the layout of v-tables. Generic C-level ABI details like type sizes and alignments and struct layout quirks are provided by TargetInfo. In particular, TargetInfo already have 3 or 4 target-specific methods controlling various details of bit-field alignment; adding another for this will be very natural.

[Sunil_Srivastava]

Changed the patch based of feedback from John McCall.

There is new bit UseExplicitBitFieldAlignment, which is set for everything except for PS4. The r257462 change has been made conditional on this bit.

[majnemer]

include/clang/Basic/TargetInfo.h

206

I'd use unsigned to match the style of the surrounding code.

[rjmccall]

Yes, thank you, this looks great. I agree with David's review, but otherwise this LGTM.

include/clang/Basic/TargetInfo.h

protected:
   /// \brief Whether zero length bitfields (e.g., int : 0;) force alignment of
   /// the next bitfield.
   ///
   /// If the alignment of the zero length bitfield is greater than the member
   /// that follows it, `bar', `bar' will be aligned as the type of the
   /// zero-length bitfield.
   unsigned UseZeroLengthBitfieldAlignment : 1;
 
+  /// \brief  Whether explicit bit field alignment attributes are honored.
+  unsigned int UseExplicitBitFieldAlignment : 1;
+
   /// If non-zero, specifies a fixed alignment value for bitfields that follow
   /// zero length bitfield, regardless of the zero length bitfield type.
   unsigned ZeroLengthBitfieldBoundary;
 
   /// \brief Specify if mangling based on address space map should be used or
   /// not for language specific address spaces
   bool UseAddrSpaceMapMangling;
 
public:
   }
 
   /// \brief Get the fixed alignment value in bits for a member that follows
   /// a zero length bitfield.
   unsigned getZeroLengthBitfieldBoundary() const {
     return ZeroLengthBitfieldBoundary;
   }
 
+  /// \brief Check whether explicit bitfield alignment attributes should be
+  //  honored, as in "__attribute__((aligned(2))) int b : 1;".
+  bool useExplicitBitFieldAlignment() const {
+    return UseExplicitBitFieldAlignment;
+  }
+
   /// \brief Check whether this target support '\#pragma options align=mac68k'.
   bool hasAlignMac68kSupport() const {
     return HasAlignMac68kSupport;
   }
 
   /// \brief Return the user string for the specified integer type enum.
   ///
   /// For example, SignedShort -> "short".

lib/AST/RecordLayoutBuilder.cpp

if (FieldSize == 0 || FieldSize > UnfilledBitsInLastUnit) {
           llvm::alignTo(UnpackedFieldOffset, UnpackedFieldAlign);
       UnfilledBitsInLastUnit = 0;
     }
 
   } else {
     // #pragma pack, with any value, suppresses the insertion of padding.
     bool AllowPadding = MaxFieldAlignment.isZero();
 
     // Compute the real offset.
     if (FieldSize == 0 || 
         (AllowPadding &&
          (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) {
       FieldOffset = llvm::alignTo(FieldOffset, FieldAlign);
-    } else if (ExplicitFieldAlign) {
+    } else if (ExplicitFieldAlign &&
+               Context.getTargetInfo().useExplicitBitFieldAlignment()) {
       // TODO: figure it out what needs to be done on targets that don't honor
       // bit-field type alignment like ARM APCS ABI.
       FieldOffset = llvm::alignTo(FieldOffset, ExplicitFieldAlign);
     }
 
     // Repeat the computation for diagnostic purposes.
     if (FieldSize == 0 ||
         (AllowPadding &&
          (UnpackedFieldOffset & (UnpackedFieldAlign-1)) + FieldSize > TypeSize))
       UnpackedFieldOffset =
           llvm::alignTo(UnpackedFieldOffset, UnpackedFieldAlign);
-    else if (ExplicitFieldAlign)
+    else if (ExplicitFieldAlign &&
+             Context.getTargetInfo().useExplicitBitFieldAlignment())
       UnpackedFieldOffset =
           llvm::alignTo(UnpackedFieldOffset, ExplicitFieldAlign);
   }
 
   // If we're using external layout, give the external layout a chance
   // to override this information.
   if (UseExternalLayout)
     FieldOffset = updateExternalFieldOffset(D, FieldOffset);

lib/Basic/TargetInfo.cpp

TargetInfo::TargetInfo(const llvm::Triple &T) : TargetOpts(), Triple(T) {
   Char16Type = UnsignedShort;
   Char32Type = UnsignedInt;
   Int64Type = SignedLongLong;
   SigAtomicType = SignedInt;
   ProcessIDType = SignedInt;
   UseSignedCharForObjCBool = true;
   UseBitFieldTypeAlignment = true;
   UseZeroLengthBitfieldAlignment = false;
+  UseExplicitBitFieldAlignment = true;
   ZeroLengthBitfieldBoundary = 0;
   HalfFormat = &llvm::APFloat::IEEEhalf;
   FloatFormat = &llvm::APFloat::IEEEsingle;
   DoubleFormat = &llvm::APFloat::IEEEdouble;
   LongDoubleFormat = &llvm::APFloat::IEEEdouble;
   DataLayoutString = nullptr;
   UserLabelPrefix = "_";
   MCountName = "mcount";

lib/Basic/Targets.cpp

 public:
   PS4OSTargetInfo(const llvm::Triple &Triple) : OSTargetInfo<Target>(Triple) {
     this->WCharType = this->UnsignedShort;
 
     // On PS4, TLS variable cannot be aligned to more than 32 bytes (256 bits).
     this->MaxTLSAlign = 256;
     this->UserLabelPrefix = "";
 
+    // On PS4, do not honor explicit bit field alignment,
+    // as in "__attribute__((aligned(2))) int b : 1;".
+    this->UseExplicitBitFieldAlignment = false;
+
     switch (Triple.getArch()) {
     default:
     case llvm::Triple::x86_64:
       this->MCountName = ".mcount";
       break;
     }
   }
 };

test/Sema/bitfield-layout.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=arm-linux-gnueabihf
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=aarch64-linux-gnu
 // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-pc-linux-gnu
+// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-scei-ps4
 // expected-no-diagnostics
 #include <stddef.h>
 
 #define CHECK_SIZE(kind, name, size) \
   extern int name##_1[sizeof(kind name) == size ? 1 : -1];
 #define CHECK_ALIGN(kind, name, size) \
   extern int name##_2[__alignof(kind name) == size ? 1 : -1];
 #define CHECK_OFFSET(kind, name, member, offset) \

 
 // Bit-field with explicit align bigger than normal.
 struct g0 {
   char a;
   __attribute__((aligned(16))) int b : 1;
   char c;
 };
 
+#if defined(__PS4__)
+CHECK_SIZE(struct, g0, 16);
+CHECK_ALIGN(struct, g0, 16);
+CHECK_OFFSET(struct, g0, c, 2);
+#else
 CHECK_SIZE(struct, g0, 32);
 CHECK_ALIGN(struct, g0, 16);
 CHECK_OFFSET(struct, g0, c, 17);
+#endif
 
 // Bit-field with explicit align smaller than normal.
 struct g1 {
   char a;
   __attribute__((aligned(2))) int b : 1;
   char c;
 };
 
 CHECK_SIZE(struct, g1, 4);
 CHECK_ALIGN(struct, g1, 4);
+#if defined(__PS4__)
+CHECK_OFFSET(struct, g1, c, 2);
+#else
 CHECK_OFFSET(struct, g1, c, 3);
+#endif
 
 // Same as above but without explicit align.
 struct g2 {
   char a;
   int b : 1;
   char c;
 };
 
 CHECK_SIZE(struct, g2, 4);
 CHECK_ALIGN(struct, g2, 4);
 CHECK_OFFSET(struct, g2, c, 2);
 
 // Explicit attribute align on bit-field has precedence over packed attribute
 // applied too the struct.
 struct __attribute__((packed)) g3 {
   char a;
   __attribute__((aligned(16))) int b : 1;
   char c;
 };
 
-CHECK_SIZE(struct, g3, 32);
 CHECK_ALIGN(struct, g3, 16);
+#if defined(__PS4__)
+CHECK_SIZE(struct, g3, 16);
+CHECK_OFFSET(struct, g3, c, 2);
+#else
+CHECK_SIZE(struct, g3, 32);
 CHECK_OFFSET(struct, g3, c, 17);
+#endif
 
 struct __attribute__((packed)) g4 {
   char a;
   __attribute__((aligned(2))) int b : 1;
   char c;
 };
 
 CHECK_SIZE(struct, g4, 4);
 CHECK_ALIGN(struct, g4, 2);
+#if defined(__PS4__)
+CHECK_OFFSET(struct, g4, c, 2);
+#else
 CHECK_OFFSET(struct, g4, c, 3);
+#endif
 
 struct g5 {
   char : 1;
   __attribute__((aligned(1))) int n : 24;
 };
 CHECK_SIZE(struct, g5, 4);
 CHECK_ALIGN(struct, g5, 4);
 
 struct __attribute__((packed)) g6 {
   char : 1;
   __attribute__((aligned(1))) int n : 24;
 };
 CHECK_SIZE(struct, g6, 4);
 CHECK_ALIGN(struct, g6, 1);
 
 struct g7 {
   char : 1;
   __attribute__((aligned(1))) int n : 25;
 };
+#if defined(__PS4__)
+CHECK_SIZE(struct, g7, 4);
+#else
 CHECK_SIZE(struct, g7, 8);
+#endif
 CHECK_ALIGN(struct, g7, 4);
 
 struct __attribute__((packed)) g8 {
   char : 1;
   __attribute__((aligned(1))) int n : 25;
 };
+#if defined(__PS4__)
+CHECK_SIZE(struct, g8, 4);
+#else
 CHECK_SIZE(struct, g8, 5);
+#endif
 CHECK_ALIGN(struct, g8, 1);
 
 struct g9 {
   __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2, e : 2;
   int i;
 };
+#if defined(__PS4__)
+CHECK_SIZE(struct, g9, 8);
+#else
 CHECK_SIZE(struct, g9, 12);
+#endif
 CHECK_ALIGN(struct, g9, 4);
 
 struct __attribute__((packed)) g10 {
   __attribute__((aligned(1))) char a : 2, b : 2, c : 2, d : 2, e : 2;
   int i;
 };
+#if defined(__PS4__)
+CHECK_SIZE(struct, g10, 6);
+#else
 CHECK_SIZE(struct, g10, 9);
+#endif
 CHECK_ALIGN(struct, g10, 1);
 
 struct g11 {
   char a;
   __attribute__((aligned(1))) long long b : 62;
   char c;
 };
 #if defined(__arm__) || defined(__aarch64__) || defined(__x86_64__)