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).
Details
Diff Detail
Unit Tests
Event Timeline
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
623–624 | 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. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
17 | 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. |
clang/docs/ReleaseNotes.rst | ||
---|---|---|
821–822 | Might need some line wrapping here? |
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 | ||
639 | 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. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
14 | See comment for line 688 of this Diff. | |
688 | 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). |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
14 | Should change //RUN: to // RUN: to match other lines as well. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
18 | What's the reason this part is #ifdef'd out? Does it contain code that's unsupported on these platforms? |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
688 | 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. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
18 | 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. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
688 |
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. |
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
688 | 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. |
Thanks @nicolerabjohn; this LGTM!
clang/test/SemaCXX/class-layout.cpp | ||
---|---|---|
684 | 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. |
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.
This line also needs update.