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

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

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

[rjmccall]

include/clang/Basic/TargetCXXABI.h

118

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 ↗

(On Diff #47026)

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/TargetCXXABI.h

enum Kind {
     WebAssembly,
 
     /// The Microsoft ABI is the ABI used by Microsoft Visual Studio (and
     /// compatible compilers).
     ///
     /// FIXME: should this be split into Win32 and Win64 variants?
     ///
     /// Only scattered and incomplete official documentation exists.
-    Microsoft
+    Microsoft,
+
+    /// The PS4 ABI is basically GenericItaniumABI, as it was implemented
+    /// in LLVM 3.2.
+    PS4
   };
 
 private:
   // Right now, this class is passed around as a cheap value type.
   // If you add more members, especially non-POD members, please
   // audit the users to pass it by reference instead.
   Kind TheKind;
 
bool isItaniumFamily() const {
     case GenericAArch64:
     case GenericItanium:
     case GenericARM:
     case iOS:
     case iOS64:
     case WatchOS:
     case GenericMIPS:
     case WebAssembly:
+    case PS4:
       return true;
 
     case Microsoft:
       return false;
     }
     llvm_unreachable("bad ABI kind");
   }
 
   /// \brief Is this ABI an MSVC-compatible ABI?
   bool isMicrosoft() const {
     switch (getKind()) {
     case GenericAArch64:
     case GenericItanium:
     case GenericARM:
     case iOS:
     case iOS64:
     case WatchOS:
     case GenericMIPS:
     case WebAssembly:
+    case PS4:
       return false;
 
     case Microsoft:
       return true;
     }
     llvm_unreachable("bad ABI kind");
   }
 
case GenericMIPS:
       // TODO: ARM-style pointers to member functions put the discriminator in
       //       the this adjustment, so they don't require functions to have any
       //       special alignment and could therefore also return false.
     case GenericItanium:
     case iOS:
     case iOS64:
     case WatchOS:
     case Microsoft:
+    case PS4:
       return true;
     }
     llvm_unreachable("bad ABI kind");
   }
 
   /// \brief Is the default C++ member function calling convention
   /// the same as the default calling convention?
   bool isMemberFunctionCCDefault() const {
bool canKeyFunctionBeInline() const {
     case WatchOS:
       return false;
 
     case GenericAArch64:
     case GenericItanium:
     case iOS:   // old iOS compilers did not follow this rule
     case Microsoft:
     case GenericMIPS:
+    case PS4:
       return true;
     }
     llvm_unreachable("bad ABI kind");
   }
 
   /// When is record layout allowed to allocate objects in the tail
   /// padding of a base class?
   ///
TailPaddingUseRules getTailPaddingUseRules() const {
     // To preserve binary compatibility, the generic Itanium ABI has
     // permanently locked the definition of POD to the rules of C++ TR1,
     // and that trickles down to derived ABIs.
     case GenericItanium:
     case GenericAArch64:
     case GenericARM:
     case iOS:
     case GenericMIPS:
+    case PS4:
       return UseTailPaddingUnlessPOD03;
 
     // iOS on ARM64 and WebAssembly use the C++11 POD rules.  They do not honor
     // the Itanium exception about classes with over-large bitfields.
     case iOS64:
     case WebAssembly:
     case WatchOS:
       return UseTailPaddingUnlessPOD11;

lib/AST/ASTContext.cpp

CXXABI *ASTContext::createCXXABI(const TargetInfo &T) {
   switch (T.getCXXABI().getKind()) {
   case TargetCXXABI::GenericARM: // Same as Itanium at this level
   case TargetCXXABI::iOS:
   case TargetCXXABI::iOS64:
   case TargetCXXABI::WatchOS:
   case TargetCXXABI::GenericAArch64:
   case TargetCXXABI::GenericMIPS:
   case TargetCXXABI::GenericItanium:
+  case TargetCXXABI::PS4:
   case TargetCXXABI::WebAssembly:
     return CreateItaniumCXXABI(*this);
   case TargetCXXABI::Microsoft:
     return CreateMicrosoftCXXABI(*this);
   }
   llvm_unreachable("Invalid CXXABI type!");
 }
 

 MangleContext *ASTContext::createMangleContext() {
   switch (Target->getCXXABI().getKind()) {
   case TargetCXXABI::GenericAArch64:
   case TargetCXXABI::GenericItanium:
   case TargetCXXABI::GenericARM:
   case TargetCXXABI::GenericMIPS:
   case TargetCXXABI::iOS:
   case TargetCXXABI::iOS64:
+  case TargetCXXABI::PS4:
   case TargetCXXABI::WebAssembly:
   case TargetCXXABI::WatchOS:
     return ItaniumMangleContext::create(*this, getDiagnostics());
   case TargetCXXABI::Microsoft:
     return MicrosoftMangleContext::create(*this, getDiagnostics());
   }
   llvm_unreachable("Unsupported ABI");
 }

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();
 
+    // PS4 remains compatible to pre r257462 behavior.
+    bool isPS4ABI = (Context.getTargetInfo().getCXXABI().getKind() == TargetCXXABI::PS4);
+
     // Compute the real offset.
     if (FieldSize == 0 || 
         (AllowPadding &&
          (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)) {
       FieldOffset = llvm::alignTo(FieldOffset, FieldAlign);
-    } else if (ExplicitFieldAlign) {
+    } else if (ExplicitFieldAlign && !isPS4ABI) {
       // 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 && !isPS4ABI)
       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/Targets.cpp

void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
     DefineStd(Builder, "unix", Opts);
     Builder.defineMacro("__ELF__");
     Builder.defineMacro("__PS4__");
   }
 public:
   PS4OSTargetInfo(const llvm::Triple &Triple) : OSTargetInfo<Target>(Triple) {
     this->WCharType = this->UnsignedShort;
 
+    // PS4 uses a variant of the C++11 ABI.
+    this->TheCXXABI.set(TargetCXXABI::PS4);
+
     // On PS4, TLS variable cannot be aligned to more than 32 bytes (256 bits).
     this->MaxTLSAlign = 256;
     this->UserLabelPrefix = "";
 
     switch (Triple.getArch()) {
     default:
     case llvm::Triple::x86_64:
       this->MCountName = ".mcount";

lib/CodeGen/CodeGenModule.cpp

static CGCXXABI *createCXXABI(CodeGenModule &CGM) {
   switch (CGM.getTarget().getCXXABI().getKind()) {
   case TargetCXXABI::GenericAArch64:
   case TargetCXXABI::GenericARM:
   case TargetCXXABI::iOS:
   case TargetCXXABI::iOS64:
   case TargetCXXABI::WatchOS:
   case TargetCXXABI::GenericMIPS:
   case TargetCXXABI::GenericItanium:
+  case TargetCXXABI::PS4:
   case TargetCXXABI::WebAssembly:
     return CreateItaniumCXXABI(CGM);
   case TargetCXXABI::Microsoft:
     return CreateMicrosoftCXXABI(CGM);
   }
 
   llvm_unreachable("invalid C++ ABI kind");
 }

lib/CodeGen/ItaniumCXXABI.cpp

CodeGen::CGCXXABI *CodeGen::CreateItaniumCXXABI(CodeGenModule &CGM) {
 
   case TargetCXXABI::GenericMIPS:
     return new ItaniumCXXABI(CGM, /* UseARMMethodPtrABI = */ true);
 
   case TargetCXXABI::WebAssembly:
     return new WebAssemblyCXXABI(CGM);
 
   case TargetCXXABI::GenericItanium:
+  case TargetCXXABI::PS4:
     if (CGM.getContext().getTargetInfo().getTriple().getArch()
         == llvm::Triple::le32) {
       // For PNaCl, use ARM-style method pointers so that PNaCl code
       // does not assume anything about the alignment of function
       // pointers.
       return new ItaniumCXXABI(CGM, /* UseARMMethodPtrABI = */ true,
                                /* UseARMGuardVarABI = */ false);
     }

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__)