This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nicolerabjohn on Jan 23 2023, 6:44 AM.

Details

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

Diff Detail

Event Timeline

nicolerabjohn created this revision.Jan 23 2023, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 6:44 AM
nicolerabjohn requested review of this revision.Jan 23 2023, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 6:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
daltenty added inline comments.Jan 23 2023, 7:33 AM
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.

Updating comment

nicolerabjohn marked an inline comment as done.Jan 23 2023, 7:45 AM
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.

dblaikie added inline comments.Jan 23 2023, 8:47 AM
clang/docs/ReleaseNotes.rst
822–823

Might need some line wrapping here?

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

nicolerabjohn marked 2 inline comments as done.Jan 23 2023, 10:22 AM

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

clang/docs/ReleaseNotes.rst
817–818

This line also needs update.

clang/lib/AST/RecordLayoutBuilder.cpp
1972

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.

hubert.reinterpretcast edited the summary of this revision. (Show Details)Jan 23 2023, 1:34 PM

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

nicolerabjohn marked 4 inline comments as done.Jan 23 2023, 1:44 PM
nicolerabjohn marked an inline comment as not done.Jan 23 2023, 1:51 PM
dblaikie added inline comments.Jan 23 2023, 1:52 PM
clang/test/SemaCXX/class-layout.cpp
18–20

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–20

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.

Reverting previous directive change.

nicolerabjohn marked 3 inline comments as done.Jan 23 2023, 2:00 PM
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.

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.

Update to assertions for z/OS case.

nicolerabjohn marked an inline comment as done.Jan 23 2023, 8:28 PM
nicolerabjohn added inline comments.
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.

nicolerabjohn marked an inline comment as done.Jan 23 2023, 8:28 PM

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.

This revision is now accepted and ready to land.Jan 23 2023, 8:43 PM

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.

hubert.reinterpretcast retitled this revision from Opting out of Clang 15 ABI Changes for AIX and z/OS to Opting out of Clang 16 ABI Changes for AIX and z/OS.Jan 23 2023, 9:09 PM
This revision was landed with ongoing or failed builds.Jan 23 2023, 10:14 PM
This revision was automatically updated to reflect the committed changes.