Opting out of Clang 16 ABI Changes for AIX and z/OS

[nicolerabjohn]

Summary

There is already a GCC compatibility gap on AIX, & GCC compatibility is not a concern on z/OS. GCC compatibility is not sufficient motivation for breaking ABI on AIX and z/OS. This opts out of changes introduced in https://reviews.llvm.org/D119051. For AIX only, also opt out of D117616 (which z/OS had picked up at the time ABI stabilization occurred).

[daltenty]

clang/test/SemaCXX/class-layout.cpp

616–617

nit: line numbers may shift on you as other commits touch this file. Prefer a same line comment that keeps the comment together with the actual directive.

[nicolerabjohn]

Updating comment

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

10

There are no AIX and z/OS RUN lines here? For AIX, we expect -DCLANG_ABI_COMPAT=15 behaviour with and without the -fclang-abi-compat=15 option.

[dblaikie]

clang/docs/ReleaseNotes.rst

822

Might need some line wrapping here?

[nicolerabjohn]

Adding run lines for AIX & z/OS, and updating comment line length.

[nicolerabjohn]

Opting out of clang ABI change in FieldPacked bool on AIX.

[hubert.reinterpretcast]

clang/docs/ReleaseNotes.rst

817

This line also needs update.

clang/lib/AST/RecordLayoutBuilder.cpp

1972 ↗

(On Diff #491477)

I agree we are good with z/OS having this change take effect going forward because the ABI was "stabilized" with a version where this change was effective.

I don't know if @SeanP has any comments on -fclang-abi-compat=15 exposing the pre-stable ABI behaviour (at least with the code here). My understanding is that the Open XL product exposed the "Clang 16" behaviour with -fclang-abi-compat=n for some n less than 16.

clang/test/SemaCXX/class-layout.cpp

632

I am having trouble understanding how this line is passing with

//RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15

The Clang 16 behaviour should be effective on z/OS given the state of this patch.

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

14

See comment for line 688 of this Diff.

681

Because z/OS has some of the Clang 16 changes but not all, I think we can't rely on claiming that it uses the Clang 15 ABI for the purposes of the testing (and need to make a change here to say that it does follow the "Clang 15 ABI" for this aspect).

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

14

Should change //RUN: to // RUN: to match other lines as well.

[nicolerabjohn]

Addressing review comments - spacing, updates to run lines, updates to a directive.

[dblaikie]

clang/test/SemaCXX/class-layout.cpp

11–13

What's the reason this part is #ifdef'd out? Does it contain code that's unsupported on these platforms?

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

681

Sorry, my last suggestion for this line should be reverted. z/OS works just like Linux for this right now: no special code needed here.

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

11–13

The correct values need to be evaluated for those parts. This test was never run with those platforms as targets. We're trying to get the newer tests enabled first.

[nicolerabjohn]

Reverting previous directive change.

[hubert.reinterpretcast]

clang/test/SemaCXX/class-layout.cpp

681

Sorry, my last suggestion for this line should be reverted. z/OS works just like Linux for this right now: no special code needed here.

I was actually confused about being confused before (and now I am just very confused). This is the test corresponding to the changes to areDefaultedSMFStillPOD. z/OS does not work just like Linux for this part. Under __MVS__, we really should always be returning 8 for the size of t3. I am not sure how we ended up with 12 as I was told offline.

[nicolerabjohn]

Update to assertions for z/OS case.

[nicolerabjohn]

clang/test/SemaCXX/class-layout.cpp

681

I discussed offline with Hubert, and we went through the test cases. My updated diff has applied Hubert's original suggestion for this line, as well as a similar change for line 684 to update the assertions for the z/OS case. This test case now passes all the AIX & z/OS run lines.

[hubert.reinterpretcast]

Thanks @nicolerabjohn; this LGTM!

clang/test/SemaCXX/class-layout.cpp

677

Just a note: As mentioned above, for some releases of Open XL on z/OS, the -fclang-abi-compat level that gives this result is less than 15 (but, then again, the behaviour expected by this line persisted in vanilla Clang 15). What is more important is the default behaviour (confirmed below) anyway.

[hubert.reinterpretcast]

Since this is a change only for AIX and z/OS, and considering that this is a patch to prevent ABI changes compared to the current ABI baselines for those platforms, I will land this ahead of the LLVM 16 branching.

clang/docs/ReleaseNotes.rst

 
 ...
 
 ABI Changes in Clang
 --------------------
 
 - GCC doesn't pack non-POD members in packed structs unless the packed
   attribute is also specified on the member. Clang historically did perform
   such packing. Clang now matches the gcc behavior (except on Darwin and PS4).
   You can switch back to the old ABI behavior with the flag:
   ``-fclang-abi-compat=15.0``.
 - GCC allows POD types to have defaulted special members. Clang historically
   classified such types as non-POD (for the purposes of Itanium ABI). Clang now
-  matches the gcc behavior (except on Darwin and PS4). You can switch back to
+  matches the gcc behavior (except on Darwin, PS4, AIX and z/OS). You can switch back to
   the old ABI behavior with the flag: ``-fclang-abi-compat=15.0``.
 
 OpenMP Support in Clang
 -----------------------
 
 ...
 
 CUDA Support in Clang

clang/lib/Basic/Targets/OSTargets.h

public:
   }
 
   // AIX sets FLT_EVAL_METHOD to be 1.
   LangOptions::FPEvalMethodKind getFPEvalMethod() const override {
     return LangOptions::FPEvalMethodKind::FEM_Double;
   }
 
   bool defaultsToAIXPowerAlignment() const override { return true; }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+    return false;
+  }
 };
 
 // z/OS target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY ZOSTargetInfo : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const override {
ZOSTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : OSTargetInfo<Target>(Triple, Opts) {
     this->WCharType = TargetInfo::UnsignedInt;
     this->MaxAlignedAttribute = 128;
     this->UseBitFieldTypeAlignment = false;
     this->UseZeroLengthBitfieldAlignment = true;
     this->UseLeadingZeroLengthBitfield = false;
     this->ZeroLengthBitfieldBoundary = 32;
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+    return false;
+  }
 };
 
 void addWindowsDefines(const llvm::Triple &Triple, const LangOptions &Opts,
                        MacroBuilder &Builder);
 
 // Windows target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY WindowsTargetInfo : public OSTargetInfo<Target> {

clang/test/SemaCXX/class-layout.cpp

 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin    %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
 // RUN: %clang_cc1 -triple x86_64-scei-ps4        %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5         %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
+#if !defined(__MVS__) && !defined(_AIX)
+
 #define SA(n, p) int a##n[(p) ? 1 : -1]
 
 struct A {
   int a;
   char b;
 };
 
 SA(0, sizeof(A) == 8);
 
#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 6
   _Static_assert(__builtin_offsetof(D, n) == 1, "");
 #else
   _Static_assert(_Alignof(D) == 2, "");
   _Static_assert(__builtin_offsetof(D, n) == 2, "");
 #endif
 #pragma pack(pop)
 }
 
+#endif // !defined(__MVS__) && !defined(__AIX__)
+
 namespace non_pod {
 struct t1 {
 protected:
   int a;
 };
 // GCC prints warning: ignoring packed attribute because of unpacked non-POD field 't1 t2::v1'`
 struct t2 {
   char c1;
   short s1;
   char c2;
   t1 v1;
 } __attribute__((packed));
 #if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
 _Static_assert(_Alignof(t1) == 4, "");
 _Static_assert(_Alignof(t2) == 1, "");
 #else
 _Static_assert(_Alignof(t1) == 4, "");
 _Static_assert(_Alignof(t2) == 4, "");
 #endif
 _Static_assert(sizeof(t2) == 8, ""); // it's still packing the rest of the struct
 } // namespace non_pod
 
 namespace non_pod_packed {
struct t1 {
   ~t1() = delete;
   t1(t1&&) = default;
   int a;
   char c;
 };
 struct t2 {
   t1 v1;
 } __attribute__((packed));
 _Static_assert(_Alignof(t2) == 1, "");
 struct t3 : t1 {
   char c;
 };
 #if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
 _Static_assert(sizeof(t3) == 8, "");
 #else
 _Static_assert(sizeof(t3) == 12, "");
 #endif
 }