Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden

[dyung]

Summary

For compatibility reasons, the PS4 clang ABI must use version 6, so we force the compiler to use this version if the PS4 target triple is specified. We also emit a new warning now if a clang abi version was specified which is not 6 to let the user know we are ignoring their request.

[probinson]

LGTM

[rsmith]

Everything old is new again. This was discussed when -fclang-abi-compat was introduced; see https://reviews.llvm.org/D36501 for the argument why this patch is the wrong way of modeling an ABI. Forcing the ClangABICompat language option as a way of "tricking" Clang into producing the PS4 ABI is a hack. The various ABI changes that -fclang-abi-compat= controls are simply part of the PS4 ABI, and that knowledge should idealistically be carried by the CodeGen (etc) code that knows about PS4, rather than by imagining that there is some other PS4 ABI that Clang would produces at version Latest, and that we're asking for a compatibility version of it.

That said, pragmatically speaking, that approach isn't working out well. We have systematically forgotten to special-case PS4 when adding ABI compatibility features, so I think I'm convinced that this hack is better than the status quo. This will go wrong if we ever release (or have ever released) a Clang version that fails to properly implement the PS4 ABI. In such a case, -fclang-abi-compat should be usable to request that we emulate past ABI bugs, but would actually have no effect on PS4. I think it's OK to cross that bridge if/when we come to it.

However, we should not issue a warning for use of the flag. Remember that the flag means "please be ABI-compatible with Clang version X.Y". Because all versions of Clang that target PS4 use the same ABI, the flag is a no-op on that target (at least for now, until we accidentally introduce an ABI break). So we should not be warning on it, just silently accepting it and doing what it says -- which for now is nothing.

lib/Frontend/CompilerInvocation.cpp

2762

Is Ver6 really the right setting? When determining whether to pass objects of class type indirect, PS4 uses the Clang <= 4 rule, when determining how to pass a vector of 1 long long, it uses the Clang <= 3.8 rule, and so on. In SemaDeclCXX.cpp (in paramCanBeDestoyedInCallee), I found this comment:

// The PS4 platform ABI follows the behavior of Clang 3.2.

So I *suspect* you should actually be setting this to Ver3_8 (which is the oldest version we provide a compatibility flag for).

[probinson]

In D46767#1096746, @rsmith wrote:

Everything old is new again.

If only that were true about my brain. :-P

This was discussed when -fclang-abi-compat was introduced; see https://reviews.llvm.org/D36501 for the argument why this patch is the wrong way of modeling an ABI. Forcing the ClangABICompat language option as a way of "tricking" Clang into producing the PS4 ABI is a hack. The various ABI changes that -fclang-abi-compat= controls are simply part of the PS4 ABI, and that knowledge should idealistically be carried by the CodeGen (etc) code that knows about PS4, rather than by imagining that there is some other PS4 ABI that Clang would produces at version Latest, and that we're asking for a compatibility version of it.

Muchas gracias for the reminder of the previous discussion; it's quite true that on our side we have not done our due diligence in making sure that upstream Clang fully supports the PS4 ABI, and that -fclang-abi-compat is the wrong way to do this. It needs to become part of my team's consciousness and collective memory that these sorts of expedient hacks are the wrong approach.

This will go wrong if we ever release (or have ever released) a Clang version that fails to properly implement the PS4 ABI.

Yeah, we crossed that bridge years ago, but nobody has been brave enough to try to deliver that patch upstream. Actually I think there are two, but as they typically don't cause any merge conflicts they're not at top-of-mind for anybody, even me.

However, we should not issue a warning for use of the flag. Remember that the flag means "please be ABI-compatible with Clang version X.Y". Because all versions of Clang that target PS4 use the same ABI, the flag is a no-op on that target (at least for now, until we accidentally introduce an ABI break). So we should not be warning on it, just silently accepting it and doing what it says -- which for now is nothing.

Got it. I'll take an action to straighten this one out.

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_unknown_argument_with_suggestion
   : Error<"unknown argument '%0', did you mean '%1'?">;
 def warn_drv_unknown_argument_clang_cl : Warning<
   "unknown argument ignored in clang-cl: '%0'">,
   InGroup<UnknownArgument>;
 def warn_drv_unknown_argument_clang_cl_with_suggestion : Warning<
   "unknown argument ignored in clang-cl '%0' (did you mean '%1'?)">,
   InGroup<UnknownArgument>;
+def warn_drv_ignored_clang_abi_version : Warning<
+  "target requires clang ABI version %0, ignoring requested version">,
+  InGroup<UnusedCommandLineArgument>;
 
 def warn_drv_ycyu_no_arg_clang_cl : Warning<
   "support for '%0' without a filename not implemented yet; flag ignored">,
   InGroup<ClangClPch>;
 def warn_drv_ycyu_different_arg_clang_cl : Warning<
   "support for '/Yc' and '/Yu' with different filenames not implemented yet; flags ignored">,
   InGroup<ClangClPch>;
 def warn_drv_ycyu_no_fi_arg_clang_cl : Warning<

lib/Frontend/CompilerInvocation.cpp

if (!VerParts.first.startswith("0") &&
         Opts.setClangABICompat(LangOptions::ClangABI::Ver4);
       else if (Major <= 6)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver6);
     } else if (Ver != "latest") {
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
     }
   }
+  // The PS4 requires version 6 of the clang ABI.
+  if (T.isPS4()) {
+    // Issue a warning if another version of the ABI was requested.
+    if (Args.getLastArg(OPT_fclang_abi_compat_EQ) &&
+        Opts.getClangABICompat() != LangOptions::ClangABI::Ver6) {
+      Diags.Report(diag::warn_drv_ignored_clang_abi_version)
+        << 6;
+    }
+    Opts.setClangABICompat(LangOptions::ClangABI::Ver6);
+  }
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   switch (Action) {
   case frontend::ASTDeclList:
   case frontend::ASTDump:
   case frontend::ASTPrint:
   case frontend::ASTView:

test/CodeGenCXX/alignment.cpp

 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-NOCOMPAT
 // RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 -fclang-abi-compat=6.0 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V6COMPAT
+// Check that the PS4 target uses the 6.0 compat settings.
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-scei-ps4 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V6COMPAT
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-scei-ps4 -fclang-abi-compat=latest | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-V6COMPAT
 
 extern int int_source();
 extern void int_sink(int x);
 
 namespace test0 {
   struct A {
     int aField;
     int bField;

test/Driver/clang-abi-compat.cpp

+// PS4 target requires clang ABI version 6, check that a warning is emitted when a version other than 6 is requested.
+// RUN: %clang -S --target=x86_64-scei-ps4 -fclang-abi-compat=4 %s 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+// RUN: %clang -S --target=x86_64-scei-ps4 -fclang-abi-compat=latest %s 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
+
+// CHECK-WARNING: warning: target requires clang ABI version 6, ignoring requested version
+