[PATCH] Delete CC_Default and use the target default CC everywhere
Reid Kleckner
rnk at google.com
Tue Jul 30 17:38:11 PDT 2013
================
Comment at: include/clang/AST/ASTContext.h:1754-1758
@@ -1758,7 +1753,7 @@
/// \brief Determines whether two calling conventions name the same
/// calling convention.
bool isSameCallConv(CallingConv lcc, CallingConv rcc) {
- return (getCanonicalCallConv(lcc) == getCanonicalCallConv(rcc));
+ return lcc == rcc;
}
----------------
Richard Smith wrote:
> Is it worth keeping this around?
No. My plan was to do a follow up to remove it, but there are only 4 callsites left, so I'll nuke it now.
================
Comment at: lib/Sema/SemaDecl.cpp:2376
@@ +2375,3 @@
+ if (FirstTypeInfo.getCC() != NewTypeInfo.getCC()) {
+ bool FirstCCExplicit = getCCTypeAttr(First);
+ bool NewCCExplicit = getCCTypeAttr(New);
----------------
Richard Smith wrote:
> I don't think it should matter whether the first CC was implicit or explicit; the rule seems to be simply that all subsequent decls must match the first decl.
That's basically true, but it's probably an error if clang deduces two different implicit CCs for the first decl and a redeclaration, with the exception of static method definitions.
I'll add an assert.
================
Comment at: lib/Sema/SemaDecl.cpp:2375-2416
@@ -2368,27 +2374,44 @@
- // Inherit the CC from the previous declaration if it was specified
- // there but not here.
- } else if (NewTypeInfo.getCC() == CC_Default) {
- NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
- RequiresAdjustment = true;
+ if (FirstTypeInfo.getCC() != NewTypeInfo.getCC()) {
+ bool FirstCCExplicit = getCCTypeAttr(First);
+ bool NewCCExplicit = getCCTypeAttr(New);
+ if (FirstCCExplicit && !NewCCExplicit) {
+ // Inherit the CC from the previous declaration if it was specified
+ // there but not here.
+ NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
+ RequiresAdjustment = true;
+
+ } else if (!FirstCCExplicit && !NewCCExplicit) {
+ if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(First)) {
+ // We can't tell if an out of line method definition is static until
+ // after we resolve overloads and find its canonical declaration.
+ bool IsVariadic = isa<FunctionProtoType>(FirstType) &&
+ cast<FunctionProtoType>(FirstType)->isVariadic();
+ CallingConv CCStatic =
+ Context.getDefaultCallingConvention(IsVariadic, false);
+ CallingConv CCInstance =
+ Context.getDefaultCallingConvention(IsVariadic, true);
+ if (MD->isStatic() && FirstTypeInfo.getCC() == CCStatic &&
+ NewTypeInfo.getCC() == CCInstance) {
+ NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
+ RequiresAdjustment = true;
+ }
+ }
+ }
- // Don't complain about mismatches when the default CC is
- // effectively the same as the explict one. Only Old decl contains correct
- // information about storage class of CXXMethod.
- } else if (OldTypeInfo.getCC() == CC_Default &&
- isABIDefaultCC(*this, NewTypeInfo.getCC(), Old)) {
- NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
- RequiresAdjustment = true;
+ if (FirstTypeInfo.getCC() != NewTypeInfo.getCC()) {
+ // Calling conventions aren't compatible, so complain.
+ assert(!Context.isSameCallConv(OldTypeInfo.getCC(), NewTypeInfo.getCC()));
+ Diag(New->getLocation(), diag::err_cconv_change)
+ << FunctionType::getNameForCallConv(NewTypeInfo.getCC())
+ << !FirstCCExplicit
+ << (!FirstCCExplicit ? "" :
+ FunctionType::getNameForCallConv(OldTypeInfo.getCC()));
- } 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;
+ // Put the note on the first decl, since it is the one that matters.
+ Diag(Old->getFirstDeclaration()->getLocation(),
+ diag::note_previous_declaration);
+ return true;
+ }
}
----------------
Richard Smith wrote:
> Can you collapse this into
>
> if (OldTypeInfo.getCC() != NewTypeInfo.getCC()) {
> if (!NewExplicit) {
> NewTypeInfo = NewTypeInfo.withCallingConv(OldTypeInfo.getCC());
> RequiresAdjustment = true;
> } else {
> issue diagnostic
> }
> }
Done, plus the assert.
================
Comment at: lib/AST/ASTContext.cpp:2803
@@ -2808,4 +2802,3 @@
CanonicalEPI.NumExceptions = 0;
- CanonicalEPI.ExtInfo
- = CanonicalEPI.ExtInfo.withCallingConv(getCanonicalCallConv(CallConv));
+ CanonicalEPI.ExtInfo = CanonicalEPI.ExtInfo.withCallingConv(CallConv);
----------------
Richard Smith wrote:
> This looks redundant.
Gone.
================
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:
> 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?
================
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:
> 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?
================
Comment at: lib/Sema/SemaDecl.cpp:6269-6271
@@ +6268,5 @@
+
+ // Ignore dependent types.
+ const FunctionType *FT = R->getAs<FunctionType>();
+ if (!FT) return R;
+
----------------
Richard Smith wrote:
> Can this really happen?
Seems to work with castAs.
================
Comment at: lib/Sema/SemaDecl.cpp:6276
@@ +6275,3 @@
+ while (AT && !AT->isCallingConv())
+ AT = AT->getEquivalentType()->getAsAttributedType();
+ if (AT) return R;
----------------
Richard Smith wrote:
> As before, this should presumably be getModifiedType. Can this share code with getCCTypeAttr?
Done.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:7922
@@ -7906,5 +7921,3 @@
- // Build an exception specification pointing back at this constructor.
- FunctionProtoType::ExtProtoInfo EPI;
- EPI.ExceptionSpecType = EST_Unevaluated;
- EPI.ExceptionSpecDecl = DefaultCon;
+ // Build an exception specification pointing back at this destructor.
+ FunctionProtoType::ExtProtoInfo EPI = getImplicitMethodEPI(*this, DefaultCon);
----------------
Richard Smith wrote:
> Copy/pasto?
Woops.
================
Comment at: lib/Sema/SemaLookup.cpp:731
@@ -730,3 +730,3 @@
FunctionProtoType::ExtProtoInfo EPI = ConvProto->getExtProtoInfo();
- EPI.ExtInfo = EPI.ExtInfo.withCallingConv(CC_Default);
+ EPI.ExtInfo = EPI.ExtInfo.withCallingConv(CC_C);
EPI.ExceptionSpecType = EST_None;
----------------
Richard Smith wrote:
> It would seem more appropriate to use ConvTemplate->getTemplatedDecl()'s calling convention here.
That should already be there from the copy. John McCall added this CC_Default assignment in r121763. I'm not sure why.
================
Comment at: lib/Sema/SemaType.cpp:2446-2450
@@ +2445,7 @@
+
+ if (FoundNonParen) {
+ // If we're not the declarator, we're a regular function type unless we're
+ // in a member pointer.
+ IsCXXInstanceMethod =
+ D.getTypeObject(I).Kind == DeclaratorChunk::MemberPointer;
+ } else {
----------------
Richard Smith wrote:
> What happens if a function type becomes a pointer-to-member-function through a typedef-name? For instance:
>
> typedef int fn();
> typedef fn S::*p;
>
> Presumably this should give a pointer-to-member-function type with a c++ method cc.
>
> Hmm, what happens here:
>
> struct S;
> template<typename Fn> struct X {
> typedef Fn S::*p;
> };
> X<void()>::p p1;
> X<__cdecl void()>::p p2;
> void (S::*p3)();
> __cdecl void (S::*p4)();
>
> Does MSVC believe that p1, p2, p3, and p4 have the same type?
MSVC doesn't model the difference between an implicit and explicit CC. I modified the example, and we get these demangled symbols from cl.exe:
struct S;
template<typename Fn> struct X {
typedef Fn S::*p;
};
X<void()>::p p1;
X<void __cdecl ()>::p p2;
void (S::*p3)();
void (__cdecl S::*p4)();
(void (__thiscall S::* p1)(void))
(void (__thiscall S::* p2)(void))
(void (__thiscall S::* p3)(void))
(void (__cdecl S::* p4)(void))
I added tests for this, but clang currently gets some of these wrong. As discussed, I will have to introduce an implicit AttributedType node so that MPT->getPointeeType() returns a function type which is canonically thiscall, without erasing the typedef from TSI. That's for a later patch.
================
Comment at: lib/AST/Type.cpp:519-532
@@ -518,1 +518,16 @@
+const AttributedType *Type::getAsAttributedType() const {
+ // AttributedTypes are non-canonical, so we can't use getAs<AttributedType>().
+ // Instead we have to unwrap each level of sugar iteratively until we get back
+ // the same type or find an AttributedType.
+ QualType Prev;
+ QualType T = QualType(this, 0);
+ while (T != Prev) {
+ if (const AttributedType *AT = dyn_cast<AttributedType>(T))
+ return AT;
+ Prev = T;
+ T = T->getLocallyUnqualifiedSingleStepDesugaredType();
+ }
+ return 0;
+}
+
----------------
Richard Smith wrote:
> Please add an explicit specialization for getAs<> here, as we do for TypedefType and TemplateSpecializationType.
Done.
================
Comment at: lib/Sema/SemaDecl.cpp:2243
@@ +2242,3 @@
+ while (AT && !AT->isCallingConv())
+ AT = AT->getEquivalentType()->getAsAttributedType();
+ if (AT) return AT;
----------------
Richard Smith wrote:
> This should presumably be getModifiedType(), in case the equivalent type has lost some of the sugar. (If you apply an attribute which modifies the function type on top of a function type with a calling convention, you're likely to lose the calling convention sugar from the equivalent type.)
Done. I can't really test this well because we don't build AttributedTypes for things like noreturn yet.
http://llvm-reviews.chandlerc.com/D1231
More information about the cfe-commits
mailing list