================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
