Add flag to request Clang is ABI-compatible with older versions of itself

[rsmith]

Summary

This patch adds a flag -fclang-abi-compat that can be used to request that Clang attempts to be ABI-compatible with some older version of itself.

This is provided on a best-effort basis; right now, this can be used to undo the ABI change in r310401, reverting Clang to its prior C++ ABI for pass/return by value of class types affected by that change, and to undo the ABI change in r262688, reverting Clang to using integer registers rather than SSE registers for passing <1 x long long> vectors. The intent is that we will maintain this backwards compatibility path as we make ABI-breaking fixes in future.

[rjmccall]

Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all?

[rsmith]

In D36501#836257, @rjmccall wrote:

Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all?

I don't see any reason anyone would want to use this other than to restore the ABI to that of Clang <= 4, so an overall "clang ABI version" flag would seem reasonable to me.

[probinson]

Locally we have a couple different tactics for dealing with changes that we can't support. A more coherent approach would be great.
For example we defined a new TargetCXXABI::Kind value that is mostly GenericItaniumABI except where it isn't.
I personally did not do most of the various ABI-related tweaks so I don't claim to have a good handle on them, and we have been slow to get these things upstream; but I'd love to make that happen.

[hans]

I'm not familiar with the details of the ABI changes, but the mechanics of the code look good to me.

[probinson]

I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way.

[rsmith]

In D36501#852765, @probinson wrote:

I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way.

On reflection, I don't think doing anything special for PS4 here is really correct. (Suppose Clang 5 has some (accidental) ABI change in it for PS4, and we fix that in Clang 6 to once again follow the psABI and do what Clang 3.2 did. In that case, building with -fclang-abi-compat=5 should theoretically reinstate that accidental ABI change.) The old-Clang compatibility mode and the platform ABI are separate, and we should not conflate them. New patch on the way.

[rjmccall]

The only thing I would say here is that, as a platform vendor, I would like the ability to opt in to ABI fixes that aren't in my base-line version. In particular, we're generally more aggressive about taking C++ ABI fixes on Darwin when there's a strong argument that the impact is likely to be low, like for some of these indirect-ABI cases that only make a difference for classes with e.g. only deleted copy/move constructors but a trivial destructor.

[rsmith]

In D36501#852864, @rjmccall wrote:

The only thing I would say here is that, as a platform vendor, I would like the ability to opt in to ABI fixes that aren't in my base-line version. In particular, we're generally more aggressive about taking C++ ABI fixes on Darwin when there's a strong argument that the impact is likely to be low, like for some of these indirect-ABI cases that only make a difference for classes with e.g. only deleted copy/move constructors but a trivial destructor.

I don't think the idea of a base-line version matches with the direction of the patch, and I've removed the part of the patch that has a base-line Clang version for PS4. I think that if your platform ABI dictates that you do thing X differently from, say, the x86_64 psABI or the Itanium C++ ABI, then that's simply part of your platform ABI, and not a clang compatibility feature, regardless of whether the historical reason your platform ABI made that choice was due to clang compatibility.

[probinson]

lib/Driver/ToolChains/Clang.cpp

2939

else if (getToolChain().getTriple().isPS4())
  CmdArgs.push_back("-fclang-abi-compat=3.2");

Which lets us avoid piles of PS4 special cases all over everywhere.
Sony is historically very conservative about compatibility, and we'll be happier defaulting it this way. Setting platform-specific defaults in the driver seems to be pretty common already, this is just one more.

[rsmith]

lib/Driver/ToolChains/Clang.cpp

2939

I initially thought that made sense too, but I'm now fairly convinced that it doesn't.

Let's take Darwin as an example. There are some facets of Darwin's platform ABI that are determined by what old versions of Clang did, and other facets of its platform ABI that have changed to match changes to the x86_64 psABI and Itanium C++ ABI. But it's irrelevant where the platform ABI rules come from; the point is that there is some set of platform ABI rules that you could write down, and Clang attempts to implement those rules correctly by default.

The flag being added in this patch provides the ability to request that Clang does something else: that it produces code that is ABI-compatible with what a prior version of itself did when targeting that platform ABI. In particular, we fixed the C++ calling convention for certain rare class types in Clang 5 to conform to (an update to) the Itanium C++ ABI rules, and this patch allows that to be undone.

It seems to me that the situation for PS4 is exactly the same. There is some platform ABI that you could write down, and Clang attempts to be compatible with that by default. And it's irrelevant whether that's the ABI that Clang 3.2 used or the ABI of GCC 2.95; it's just the platform ABI. This is not a "be compatible with clang 3.2" mode, it is (as far as I can tell) the actual platform ABI.

Let me repeat an example I gave before: suppose Clang 5 has some (accidental) ABI change in it for PS4, and we fix that in Clang 6 to once again follow the platform ABI by doing what Clang 3.2 did. In that case, building with -fclang-abi-compat=5 should theoretically reinstate that accidental ABI change.

Hopefully that should clarify that this does *not* actually let us avoid PS4 special cases anywhere. ABI choices depend on both the platform ABI *and* on the version of Clang that we're providing compatibility with (if any).

That said, it's clearly not up to me what the PS4 platform ABI is. If you want to say that the PS4 platform ABI is actually something other than what Clang 3.2 does, but all object code on your system is compiled in a compatibility mode that diverges from the platform ABI and matches Clang 3.2, then I would concede that we can remove the PS4 platform special cases elsewhere and set a default here. But that sounds like a very strange decision to make, and it creates a horrible problem for the meaning of the -fclang-abi-compat flag: if someone in the future specifies -fclang-abi-compat=5 when targeting PS4, and Clang 5 by default set -fclang-abi-compat=3.2, then are we targeting what Clang 5 would have done by default or what Clang 5 would have done when told to be compatible with itself? As you can see, this default would create a lot of confusion.

[probinson]

lib/Driver/ToolChains/Clang.cpp

2939

On the other hand, if all the places that check ClangABICompat also check for PS4 and Darwin, then specifying -fclang-abi-compat while targeting PS4 or Darwin has no effect, and also no diagnostic. Which seems to make -fclang-abi-compat totally pointless. Is there a non-PS4/Darwin use-case for this flag?

[rsmith]

lib/Driver/ToolChains/Clang.cpp

2939

-fclang-abi-compat requests that we generate code that is ABI-compatible with a prior Clang version. The use case (for *any* platform) is that you have some code built with an earlier version of Clang that had a bug in its implementation of the platform ABI, and you want to generate more code that is ABI-compatible with it.

-fclang-abi-compat=4, for instance, has an effect on Darwin: it turns off the recent bug fix to the calling convention for passing trivially-copyable-but-not-trivially-moveable C++ class types. (But it does nothing on PS4, because -- with this patch -- Clang 5 does not change that aspect of the ABI for PS4.)

[probinson]

Sorry it took so long to wrap my head around this, but it has been about 20 years since I've had to accommodate an intentional ABI breakage, and that's actually what you want to do. Remembering that case, I have a model for this one, and I understand what you are doing now.

Be that as it may, -fclang-abi-compat is meaningless on PS4, because every time there's a case for checking ClangABICompat, PS4 will want to do it the 3.2 way regardless. And we'll just make those checks in all those places. At least it will be relatively easy to audit, and I'm sure our licensees will be happy to ignore the new flag.

So, I am convinced, and the patch LGTM.

[rsmith]

Committed as 311823.

include/clang/Driver/Options.td

   Flags<[DriverOption, CC1Option]>,
   HelpText<"Disable GNU style inline asm">;
 
+def fpass_indirect_ignore_move : Flag<["-"], "fpass-indirect-ignore-move">,
+    Group<f_Group>, Flags<[CC1Option]>;
+def fno_pass_indirect_ignore_move : Flag<["-"], "fno-pass-indirect-ignore-move">,
+    Group<f_Group>;
+
 def fprofile_sample_use : Flag<["-"], "fprofile-sample-use">, Group<f_Group>,
     Flags<[CoreOption]>;
 def fno_profile_sample_use : Flag<["-"], "fno-profile-sample-use">, Group<f_Group>,

include/clang/Frontend/CodeGenOptions.def

 VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
 VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is specified.
 
+/// Do not consider the move constructor when determining whether to pass a
+/// class type indirectly.
+CODEGENOPT(PassIndirectIgnoreMove, 1, 0)
+
 /// \brief Choose profile instrumenation kind or no instrumentation.
 ENUM_CODEGENOPT(ProfileInstr, ProfileInstrKind, 2, ProfileNone)
 /// \brief Choose profile kind for PGO use compilation.

lib/CodeGen/ItaniumCXXABI.cpp

   ItaniumCXXABI(CodeGen::CodeGenModule &CGM,
                 bool UseARMMethodPtrABI = false,
                 bool UseARMGuardVarABI = false) :
-    CGCXXABI(CGM), UseARMMethodPtrABI(UseARMMethodPtrABI),
+    CGCXXABI(CGM),
+    UseARMMethodPtrABI(UseARMMethodPtrABI),
     UseARMGuardVarABI(UseARMGuardVarABI),
     Use32BitVTableOffsetABI(false) { }
 
   bool classifyReturnType(CGFunctionInfo &FI) const override;
 
+  bool passClassIndirect(const CXXRecordDecl *RD) const {
+    if (CGM.getCodeGenOpts().PassIndirectIgnoreMove)
+      return RD->hasNonTrivialDestructor() ||
+             RD->hasNonTrivialCopyConstructor();
+    return !canCopyArgument(RD);
+  }
+
   RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const override {
     // If C++ prohibits us from making a copy, pass by address.
-    if (!canCopyArgument(RD))
+    if (passClassIndirect(RD))
       return RAA_Indirect;
     return RAA_Default;
   }

     return false;
 
   // If C++ prohibits us from making a copy, return by address.
-  if (!canCopyArgument(RD)) {
+  if (passClassIndirect(RD)) {
     auto Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
     return true;

lib/Driver/ToolChains/Clang.cpp

 
   addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
 
+  if (Args.hasFlag(options::OPT_fpass_indirect_ignore_move,
+                   options::OPT_fno_pass_indirect_ignore_move,
+                   getToolChain().getTriple().isPS4()))
+    CmdArgs.push_back("-fpass-indirect-ignore-move");
+
   // Add runtime flag for PS4 when PGO or Coverage are enabled.
   if (getToolChain().getTriple().isPS4CPU())
     PS4cpu::addProfileRTArgs(getToolChain(), Args, CmdArgs);

lib/Frontend/CompilerInvocation.cpp

   Opts.PIECopyRelocations =
       Args.hasArg(OPT_mpie_copy_relocations);
   Opts.OmitLeafFramePointer = Args.hasArg(OPT_momit_leaf_frame_pointer);
+  Opts.PassIndirectIgnoreMove = Args.hasArg(OPT_fpass_indirect_ignore_move);
   Opts.SaveTempLabels = Args.hasArg(OPT_msave_temp_labels);
   Opts.NoDwarfDirectoryAsm = Args.hasArg(OPT_fno_dwarf_directory_asm);
   Opts.SoftFloat = Args.hasArg(OPT_msoft_float);

test/CodeGenCXX/uncopyable-args.cpp

-// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK --check-prefix=NEWABI
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-unknown -fpass-indirect-ignore-move -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK --check-prefix=OLDABI
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -emit-llvm -o - %s -fms-compatibility -fms-compatibility-version=18 | FileCheck %s -check-prefix=WIN64 -check-prefix=WIN64-18
 // RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -emit-llvm -o - %s -fms-compatibility -fms-compatibility-version=19 | FileCheck %s -check-prefix=WIN64 -check-prefix=WIN64-19
 

 // CHECK-LABEL: define void @_ZN9move_ctor3barEv()
 // CHECK: call void @_Z{{.*}}C1Ev(
 // CHECK-NOT: call
-// CHECK: call void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"* %{{.*}})
-// CHECK-LABEL: declare void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"*)
+// NEWABI: call void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"* %{{.*}})
+// OLDABI: call void @_ZN9move_ctor3fooENS_1AE(i8* %{{.*}})
+// NEWABI-LABEL: declare void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"*)
+// OLDABI-LABEL: declare void @_ZN9move_ctor3fooENS_1AE(i8*)
 
 // WIN64-LABEL: declare void @"\01?foo@move_ctor@@YAXUA@1@@Z"(%"struct.move_ctor::A"*)
 }

 // CHECK-LABEL: define void @_ZN11all_deleted3barEv()
 // CHECK: call void @_Z{{.*}}C1Ev(
 // CHECK-NOT: call
-// CHECK: call void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"* %{{.*}})
-// CHECK-LABEL: declare void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"*)
+// NEWABI: call void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"* %{{.*}})
+// OLDABI: call void @_ZN11all_deleted3fooENS_1AE(i8* %{{.*}})
+// NEWABI-LABEL: declare void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"*)
+// OLDABI-LABEL: declare void @_ZN11all_deleted3fooENS_1AE(i8*)
 
 // WIN64-LABEL: declare void @"\01?foo@all_deleted@@YAXUA@1@@Z"(%"struct.all_deleted::A"*)
 }

 // CHECK-LABEL: define void @_ZN18implicitly_deleted3barEv()
 // CHECK: call void @_Z{{.*}}C1Ev(
 // CHECK-NOT: call
-// CHECK: call void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"* %{{.*}})
-// CHECK-LABEL: declare void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"*)
+// NEWABI: call void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"* %{{.*}})
+// OLDABI: call void @_ZN18implicitly_deleted3fooENS_1AE(i8* %{{.*}})
+// NEWABI-LABEL: declare void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"*)
+// OLDABI-LABEL: declare void @_ZN18implicitly_deleted3fooENS_1AE(i8*)
 
 // In MSVC 2013, the copy ctor is not deleted by a move assignment. In MSVC 2015, it is.
 // WIN64-18-LABEL: declare void @"\01?foo@implicitly_deleted@@YAXUA@1@@Z"(i64

 // CHECK-LABEL: define void @_ZN11one_deleted3barEv()
 // CHECK: call void @_Z{{.*}}C1Ev(
 // CHECK-NOT: call
-// CHECK: call void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"* %{{.*}})
-// CHECK-LABEL: declare void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"*)
+// NEWABI: call void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"* %{{.*}})
+// OLDABI: call void @_ZN11one_deleted3fooENS_1AE(i8* %{{.*}})
+// NEWABI-LABEL: declare void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"*)
+// OLDABI-LABEL: declare void @_ZN11one_deleted3fooENS_1AE(i8*)
 
 // WIN64-LABEL: declare void @"\01?foo@one_deleted@@YAXUA@1@@Z"(%"struct.one_deleted::A"*)
 }

 }
 // CHECK-LABEL: define void @_ZN14two_copy_ctors3barEv()
 // CHECK: call void @_Z{{.*}}C1Ev(
-// CHECK: call void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* %{{.*}})
-// CHECK-LABEL: declare void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"*)
+// NEWABI: call void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* %{{.*}})
+// OLDABI: call void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* byval
+// NEWABI-LABEL: declare void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"*)
+// OLDABI-LABEL: declare void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* byval
 
 // WIN64-LABEL: declare void @"\01?foo@two_copy_ctors@@YAXUB@1@@Z"(%"struct.two_copy_ctors::B"*)
 }

   void *p;
 };
 void *foo(A a) { return a.p; }
-// CHECK-LABEL: define i8* @_ZN15definition_only3fooENS_1AE(%"struct.definition_only::A"*
+// NEWABI-LABEL: define i8* @_ZN15definition_only3fooENS_1AE(%"struct.definition_only::A"*
+// OLDABI-LABEL: define i8* @_ZN15definition_only3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@definition_only@@YAPEAXUA@1@@Z"(%"struct.definition_only::A"*
 }
 

   B b;
 };
 void *foo(A a) { return a.b.p; }
-// CHECK-LABEL: define i8* @_ZN17deleted_by_member3fooENS_1AE(%"struct.deleted_by_member::A"*
+// NEWABI-LABEL: define i8* @_ZN17deleted_by_member3fooENS_1AE(%"struct.deleted_by_member::A"*
+// OLDABI-LABEL: define i8* @_ZN17deleted_by_member3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@deleted_by_member@@YAPEAXUA@1@@Z"(%"struct.deleted_by_member::A"*
 }
 

   A();
 };
 void *foo(A a) { return a.p; }
-// CHECK-LABEL: define i8* @_ZN15deleted_by_base3fooENS_1AE(%"struct.deleted_by_base::A"*
+// NEWABI-LABEL: define i8* @_ZN15deleted_by_base3fooENS_1AE(%"struct.deleted_by_base::A"*
+// OLDABI-LABEL: define i8* @_ZN15deleted_by_base3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@deleted_by_base@@YAPEAXUA@1@@Z"(%"struct.deleted_by_base::A"*
 }
 

   B b;
 };
 void *foo(A a) { return a.b.p; }
-// CHECK-LABEL: define i8* @_ZN22deleted_by_member_copy3fooENS_1AE(%"struct.deleted_by_member_copy::A"*
+// NEWABI-LABEL: define i8* @_ZN22deleted_by_member_copy3fooENS_1AE(%"struct.deleted_by_member_copy::A"*
+// OLDABI-LABEL: define i8* @_ZN22deleted_by_member_copy3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@deleted_by_member_copy@@YAPEAXUA@1@@Z"(%"struct.deleted_by_member_copy::A"*
 }
 

   A();
 };
 void *foo(A a) { return a.p; }
-// CHECK-LABEL: define i8* @_ZN20deleted_by_base_copy3fooENS_1AE(%"struct.deleted_by_base_copy::A"*
+// NEWABI-LABEL: define i8* @_ZN20deleted_by_base_copy3fooENS_1AE(%"struct.deleted_by_base_copy::A"*
+// OLDABI-LABEL: define i8* @_ZN20deleted_by_base_copy3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@deleted_by_base_copy@@YAPEAXUA@1@@Z"(%"struct.deleted_by_base_copy::A"*
 }
 

   A(const A &o) = delete;
   void *p;
 };
-// CHECK-LABEL: define i8* @_ZN15explicit_delete3fooENS_1AE(%"struct.explicit_delete::A"*
+// NEWABI-LABEL: define i8* @_ZN15explicit_delete3fooENS_1AE(%"struct.explicit_delete::A"*
+// OLDABI-LABEL: define i8* @_ZN15explicit_delete3fooENS_1AE(i8*
 // WIN64-LABEL: define i8* @"\01?foo@explicit_delete@@YAPEAXUA@1@@Z"(%"struct.explicit_delete::A"*
 void *foo(A a) { return a.p; }
 }

   // Deleted copy ctor due to rvalue ref member.
   int &&ref;
 };
-// CHECK-LABEL: define {{.*}} @_ZN28implicitly_deleted_copy_ctor3fooENS_1AE(%"struct.implicitly_deleted_copy_ctor::A"*
+// NEWABI-LABEL: define {{.*}} @_ZN28implicitly_deleted_copy_ctor3fooENS_1AE(%"struct.implicitly_deleted_copy_ctor::A"*
+// OLDABI-LABEL: define {{.*}} @_ZN28implicitly_deleted_copy_ctor3fooENS_1AE(i32*
 // WIN64-LABEL: define {{.*}} @"\01?foo@implicitly_deleted_copy_ctor@@YAAEAHUA@1@@Z"(%"struct.implicitly_deleted_copy_ctor::A"*
 int &foo(A a) { return a.ref; }
 

test/Driver/flags.c

 
 // RUN: %clang -target armv7-apple-darwin10 -### -S -mno-implicit-float -mimplicit-float %s 2>&1 | FileCheck -check-prefix=TEST8 %s
 // TEST8-NOT: "-no-implicit-float"
+
+// RUN: %clang -target x86_64-linux-gnu -### -c -fpass-indirect-ignore-move %s 2>&1 | FileCheck -check-prefix=TEST9 %s
+// RUN: %clang -target x86_64-scei-ps4 -### -c %s 2>&1 | FileCheck -check-prefix=TEST9 %s
+// TEST9: "-fpass-indirect-ignore-move"
+
+// RUN: %clang -target x86_64-linux-gnu -### -c -fno-pass-indirect-ignore-move %s 2>&1 | FileCheck -check-prefix=TEST10 %s
+// RUN: %clang -target x86_64-scei-ps4 -### -c -fno-pass-indirect-ignore-move %s 2>&1 | FileCheck -check-prefix=TEST10 %s
+// RUN: %clang -target x86_64-linux-gnu -### -c %s 2>&1 | FileCheck -check-prefix=TEST10 %s
+// TEST10-NOT: "-fpass-indirect-ignore-move"

test/Driver/ps4-cpu-defaults.cpp

 
 // RUN: %clang -target x86_64-scei-ps4 -c %s -### 2>&1 | FileCheck %s
 // CHECK: "-target-cpu" "btver2"
+// CHECK: "-fpass-indirect-ignore-move"
 // CHECK-NOT: exceptions