Disallow most calling convention attributes on PS4.

[Sunil_Srivastava]

Summary

We wish to disable most calling convention attributes for PS4, allowing just cdecl (and the equivalent sysv_abi on PS4), which are default.

[probinson]

This has my blessing as PS4 code owner, but I'd like other eyes on it with respect to how we've gone about it.
+ rnk, rjmccall as the most likely suspects.

[rjmccall]

I'm fine with the idea of targets making unsupported CCs hard errors.

test/Sema/no_callconv.cpp

44 ↗

(On Diff #209995)

Please include a newline at the end of this file.

[rnk]

Some suggestions, you don't have to take all of them.

include/clang/Basic/DiagnosticSemaKinds.td

2961 ↗

(On Diff #209995)

This duplication merely to say "not supported" instead of "ignored" seems unfortunate. We could reasonable change "ignored" to "unsupported" and then you could use the warn_cconv_ignored.Text accessor to share the text.

include/clang/Basic/TargetInfo.h

1271 ↗

(On Diff #209995)

I feel like perhaps a cleaner way of implementing this would be to have the driver pass down -Werror=unknown-calling-conv (sp) on PS4. That would also allow users to tell clang to ignore their CC annotations by disabling the warning, which could be useful when porting a codebase widely annotated with __stdcall, for example.

lib/Basic/Targets/X86.h

646–647 ↗

(On Diff #209995)

For better separation of concerns, I would suggest putting this in PS4OSTargetInfo::checkCallingConvention, so that the main x64 code doesn't have to consider that OS-specific concern.

[Sunil_Srivastava]

include/clang/Basic/DiagnosticSemaKinds.td

2961 ↗

(On Diff #209995)

Good point. I will make a pre-commit of just changing the wording (and renaming) of warn_cconv_ignored. Then the warn_conv_unsupported.Text will be used in the actual change.

[wristow]

include/clang/Basic/TargetInfo.h

1271 ↗

(On Diff #209995)

I can see the value of allowing users to ignore the CC annotations to ease a porting task, but we on PS4 prefer not to allow that. We're probably more cautious than most about ABI incompatibilities, so if others decide to restrict use of unsupported CCs, they may very well want to go this route of the driver disabling the CCs. But we'll still make them hard errors on PS4.

[aaron.ballman]

test/Sema/no_callconv.cpp

1 ↗

(On Diff #209995)

Does this really need an svn:executable property on the file?

[Sunil_Srivastava]

test/Sema/no_callconv.cpp

1 ↗

(On Diff #209995)

I am not sure where that comes from. I did an 'svn add' and 'svn diff'.
Regardless, it will be a new test file with normal lines.

[Sunil_Srivastava]

Pre-commit to change the wording and ID of warn_cconv_ignore has gone in in r366368.

I will be shortly updating this review to account for that, and for other points raised here.

[Sunil_Srivastava]

• Adapted suggestion from Ried.
• Cleaned up line ending and properties of the test case.

[aaron.ballman]

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

lib/Basic/Targets/OSTargets.h

564 ↗

(On Diff #210674)

Line length seems a bit long for coding style requirements.

[Sunil_Srivastava]

lib/Basic/Targets/OSTargets.h

564 ↗

(On Diff #210674)

Ooops. You are right. Will split according to clang-format. Would you want me to update the review?

[wristow]

In D64780#1593624, @aaron.ballman wrote:

LGTM aside from a nit. You should give other reviewers a chance to sign off in case they have additional comments.

LGTM too, once the line-length is fixed (and IMO, no need to update the review for just that line-length change).

[rnk]

lgtm

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_cconv_unsupported : Warning<
   "%0 calling convention is not supported %select{"
   // Use CallingConventionIgnoredReason Enum to specify these.
   "for this target"
   "|on variadic function"
   "|on constructor/destructor"
   "|on builtin function"
   "}1">,
   InGroup<IgnoredAttributes>;
+def error_cconv_unsupported : Error<warn_cconv_unsupported.Text>;
 def err_cconv_knr : Error<
   "function with no prototype cannot use the %0 calling convention">;
 def warn_cconv_knr : Warning<
   err_cconv_knr.Text>,
   InGroup<DiagGroup<"missing-prototype-for-cc">>;
 def err_cconv_varargs : Error<
   "variadic function cannot use %0 calling convention">;
 def err_regparm_mismatch : Error<"function declared with regparm(%0) "

cfe/trunk/include/clang/Basic/TargetInfo.h

virtual CallingConv getDefaultCallingConv(CallingConvMethodType MT) const {
     // an explicit calling convention.
     return CC_C;
   }
 
   enum CallingConvCheckResult {
     CCCR_OK,
     CCCR_Warning,
     CCCR_Ignore,
+    CCCR_Error,
   };
 
   /// Determines whether a given calling convention is valid for the
   /// target. A calling convention can either be accepted, produce a warning
   /// and be substituted with the default calling convention, or (someday)
   /// produce an error (such as using thiscall on a non-instance function).
   virtual CallingConvCheckResult checkCallingConvention(CallingConv CC) const {
     switch (CC) {

cfe/trunk/lib/Basic/Targets/OSTargets.h

PS4OSTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
     switch (Triple.getArch()) {
     default:
     case llvm::Triple::x86_64:
       this->MCountName = ".mcount";
       this->NewAlign = 256;
       break;
     }
   }
+  TargetInfo::CallingConvCheckResult
+  checkCallingConvention(CallingConv CC) const override {
+    return (CC == CC_C) ? TargetInfo::CCCR_OK : TargetInfo::CCCR_Error;
+  }
 };
 
 // RTEMS Target
 template <typename Target>
 class LLVM_LIBRARY_VISIBILITY RTEMSTargetInfo : public OSTargetInfo<Target> {
 protected:
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
                     MacroBuilder &Builder) const override {

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

bool Sema::CheckCallingConvAttr(const ParsedAttr &Attrs, CallingConv &CC,
   case TargetInfo::CCCR_Ignore:
     // Treat an ignored convention as if it was an explicit C calling convention
     // attribute. For example, __stdcall on Win x64 functions as __cdecl, so
     // that command line flags that change the default convention to
     // __vectorcall don't affect declarations marked __stdcall.
     CC = CC_C;
     break;
 
+  case TargetInfo::CCCR_Error:
+    Diag(Attrs.getLoc(), diag::error_cconv_unsupported)
+        << Attrs << (int)CallingConventionIgnoredReason::ForThisTarget;
+    break;
+
   case TargetInfo::CCCR_Warning: {
     Diag(Attrs.getLoc(), diag::warn_cconv_unsupported)
         << Attrs << (int)CallingConventionIgnoredReason::ForThisTarget;
 
     // This convention is not valid for the target. Use the default function or
     // method calling convention.
     bool IsCXXMethod = false, IsVariadic = false;
     if (FD) {

cfe/trunk/test/Sema/no_callconv.cpp

+// RUN: %clang_cc1 %s -triple x86_64-scei-ps4 -DPS4 -fsyntax-only -verify
+// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -fsyntax-only -verify
+
+#ifdef PS4
+
+// PS4 does not support these.
+void __vectorcall func_vc() {} // expected-error {{'__vectorcall' calling convention is not supported for this target}}
+void __regcall func_rc() {} // expected-error {{'__regcall' calling convention is not supported for this target}}
+void __attribute__((vectorcall)) funcA() {} // expected-error {{'vectorcall' calling convention is not supported for this target}}
+void __attribute__((regcall)) funcB() {} // expected-error {{'regcall' calling convention is not supported for this target}}
+void __attribute__((ms_abi)) funcH() {} // expected-error {{'ms_abi' calling convention is not supported for this target}}
+void __attribute__((intel_ocl_bicc)) funcJ() {} // expected-error {{'intel_ocl_bicc' calling convention is not supported for this target}}
+void __attribute__((swiftcall)) funcK() {} // expected-error {{'swiftcall' calling convention is not supported for this target}}
+void __attribute__((pascal)) funcG() {} // expected-error {{'pascal' calling convention is not supported for this target}}
+void __attribute__((preserve_most)) funcL() {} // expected-error {{'preserve_most' calling convention is not supported for this target}}
+void __attribute__((preserve_all)) funcM() {} // expected-error {{'preserve_all' calling convention is not supported for this target}}
+void __attribute__((stdcall)) funcD() {} // expected-error {{'stdcall' calling convention is not supported for this target}}
+void __attribute__((fastcall)) funcE() {} // expected-error {{'fastcall' calling convention is not supported for this target}}
+void __attribute__((thiscall)) funcF() {} // expected-error {{'thiscall' calling convention is not supported for this target}}
+#else
+
+void __vectorcall func_vc() {}
+void __regcall func_rc() {}
+void __attribute__((vectorcall)) funcA() {}
+void __attribute__((regcall)) funcB() {}
+void __attribute__((ms_abi)) funcH() {}
+void __attribute__((intel_ocl_bicc)) funcJ() {}
+void __attribute__((swiftcall)) funcK() {}
+void __attribute__((preserve_most)) funcL() {}
+void __attribute__((preserve_all)) funcM() {}
+
+// Same function with different calling conventions. Error with a note pointing to the last decl.
+void __attribute__((preserve_all)) funcR(); // expected-note {{previous declaration is here}}
+void __attribute__((preserve_most)) funcR(); // expected-error {{function declared 'preserve_most' here was previously declared 'preserve_all'}}
+
+void __attribute__((pascal)) funcG() {} // expected-warning {{'pascal' calling convention is not supported for this target}}
+
+void __attribute__((stdcall)) funcD() {} // expected-warning {{'stdcall' calling convention is not supported for this target}}
+void __attribute__((fastcall)) funcE() {} // expected-warning {{'fastcall' calling convention is not supported for this target}}
+void __attribute__((thiscall)) funcF() {} // expected-warning {{'thiscall' calling convention is not supported for this target}}
+#endif
+
+void __attribute__((sysv_abi)) funcI() {}
+void __attribute__((cdecl)) funcC() {}

cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp

TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   Visitor.ExpectMatch("", 1, 12);
   EXPECT_TRUE(Visitor.runOver("void f() { []{ return; }(); }",
                               LambdaExprVisitor::Lang_CXX11));
   EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
   EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
 }
 
 TEST(RecursiveASTVisitor, VisitsAttributedLambdaExpr) {
+  if (llvm::Triple(llvm::sys::getDefaultTargetTriple()).isPS4())
+    return; // PS4 does not support fastcall.
   LambdaExprVisitor Visitor;
   Visitor.ExpectMatch("", 1, 12);
   EXPECT_TRUE(Visitor.runOver(
       "void f() { [] () __attribute__ (( fastcall )) { return; }(); }",
       LambdaExprVisitor::Lang_CXX14));
 }
 
 } // end anonymous namespace