[PATCH] Delete CC_Default and use the target default CC everywhere

[Reid Kleckner]

================
Comment at: lib/AST/ASTContext.cpp:7942
@@ -7946,7 +7941,3 @@
 
-CallingConv ASTContext::getCanonicalCallConv(CallingConv CC) const {
-  if (CC == CC_C && !LangOpts.MRTD &&
-      getTargetInfo().getCXXABI().isMemberFunctionCCDefault())
-    return CC_Default;
-  return CC;
+  return (LangOpts.MRTD && !IsVariadic) ? CC_X86StdCall : CC_C;
 }
----------------
Richard Smith wrote:
> Reid Kleckner wrote:
> > Richard Smith wrote:
> > > The IsVariadic check appears to be new here, but I believe it's correct. Do we have test coverage for this?
> > It looks like the LLVM x86 backend simply lowers x86_stdcallcc as caller cleanup, so I don't think this IsVariadic check is necessary.  I added a variadic function to the existing -mrtd test, but should it use x86_stdcallcc on variadic functions or not?
> Per the GCC manual, `-mrtd` only affects the calling convention of "functions that take a fixed number of arguments." Consider:
> 
>     extern void (*a)(int, int);
>     __attribute__((cdecl)) extern void (*a)(int, int);
>     extern void (*b)(int, ...);
>     __attribute__((cdecl)) extern void (*b)(int, ...);
>     extern void (*c)(int, int);
>     __attribute__((stdcall)) extern void (*c)(int, int);
>     extern void (*d)(int, ...);
>     __attribute__((stdcall)) extern void (*d)(int, ...);
> 
> GCC accepts `b` and always rejects `d` irrespective of `-mrtd`.
> Without `-mrtd`, it accepts `a` and rejects `c`.
> With `-mrtd`, it accepts `c` and rejects `a`.
Sounds good.  Updated the test to lock in this behavior.

================
Comment at: lib/AST/ASTContext.cpp:7765-7766
@@ -7771,3 +7764,4 @@
 
   FunctionType::ExtInfo EI;
+  EI = EI.withCallingConv(CC_C);
   if (BuiltinInfo.isNoReturn(Id)) EI = EI.withNoReturn(true);
----------------
Richard Smith wrote:
> Reid Kleckner wrote:
> > Richard Smith wrote:
> > > This worries me. We default-construct ExtInfo within ExtProtoInfo instances all over the place. Can we make ExtInfo not be default-constructible?
> > It could be done, but it would require significantly rewriting every construction of ExtProtoInfo as well.  ExtInfo() currently zero initializes, which happens to be equivalent to CC_C.  How about I make ExtInfo default construct with CC_C?
> That's what it's already doing, since `CC_C` is the new calling convention zero. My concern is that `CC_C` is not an appropriate default, since it's wrong when creating a method. How many places construct an `ExtProtoInfo` (and don't already set a calling convention)?
This turns out to be a lot of work.  There are many calls to FunctionType::ExtInfo() in CodeGen/CGObjC*.cpp, for example.

On the other hand, attempting this API change has uncovered a few bad call sites, so perhaps it's worth it. Perhaps I should make it a followup change, where I redo a library at a time and then make the default constructor private / deleted?

================
Comment at: lib/Sema/SemaDecl.cpp:2363
@@ -2382,11 +2362,3 @@
 
-  } else if (!Context.isSameCallConv(OldTypeInfo.getCC(),
-                                     NewTypeInfo.getCC())) {
-    // Calling conventions really aren't compatible, so complain.
-    Diag(New->getLocation(), diag::err_cconv_change)
-      << FunctionType::getNameForCallConv(NewTypeInfo.getCC())
-      << (OldTypeInfo.getCC() == CC_Default)
-      << (OldTypeInfo.getCC() == CC_Default ? "" :
-          FunctionType::getNameForCallConv(OldTypeInfo.getCC()));
-    Diag(Old->getLocation(), diag::note_previous_declaration);
-    return true;
+  if (FirstTypeInfo.getCC() != NewTypeInfo.getCC()) {
+    bool NewCCExplicit = getCCTypeAttr(New->getType());
----------------
Richard Smith wrote:
> Please compare against `OldTypeInfo` for consistency with the rest of this function, and sink `First` into the `else` below. This makes no difference, because if we don't succeed in unifying the calling conventions, we return `false`, which marks the declaration invalid.
Done.

================
Comment at: lib/Sema/SemaDecl.cpp:2373-2376
@@ +2372,6 @@
+      bool FirstCCExplicit = getCCTypeAttr(First->getType());
+      assert((FirstCCExplicit || NewCCExplicit ||
+              !(isa<CXXMethodDecl>(Old) &&
+                cast<CXXMethodDecl>(Old)->isStatic())) &&
+             "clang inferred mismatched implicit CCs");
+      Diag(New->getLocation(), diag::err_cconv_change)
----------------
Richard Smith wrote:
> This `assert` trivially holds: `NewCCExplicit` is `true` here. Also, drop the "clang" in the assert text.
Dropped it.

================
Comment at: lib/Sema/SemaType.cpp:4475
@@ +4474,3 @@
+    while (AT && (!AT->isCallingConv() || AT->getAttrKind() == CCAttrKind))
+      AT = AT->getModifiedType()->getAs<AttributedType>();
+
----------------
Richard Smith wrote:
> Should we walk through `typedef`s here? I seem to recall we determined that we're only supposed to check this against other attributes on the current declarator.
Yes, I can stop on the first CC attribute.  I added a test for it.  With this change I can share the loop in SemaDecl.cpp as a method on Sema.

I believe we should walk through typedefs here.  The logic below in adjustMemberFunctionCC() avoids walking through typedefs when adjusting CCs.  I'll use that for member function pointers later.

================
Comment at: lib/Sema/SemaType.cpp:4548
@@ +4547,3 @@
+  FunctionTypeUnwrapper Unwrapped(*this, T);
+  T = Unwrapped.wrap(*this, FT);
+}
----------------
Richard Smith wrote:
> I think we lose source information here if a member function is declared through a `typedef`, and the member and non-member calling conventions differ. Please add a FIXME for this (we should probably produce a type sugar node to track the pre-adjustment type).
FIXMEd.  I'll need it for member function pointers anyway.


http://llvm-reviews.chandlerc.com/D1231