[patch] Delay __fastcall attribute checking utnil after merging declarations

[Nico Weber]

Thanks!

On Tue, Jul 29, 2014 at 10:40 AM, Reid Kleckner <rnk at google.com> wrote:

> +  // Diagnose the use of X86 fastcall on unprototyped functions.
>
> We should also diagnose unprototyped stdcall functions, but I'm not sure
> if that would be too disruptive.  Currently we accept this and always call
> _f at 0:
> void __stdcall f();
> int main() {
>   f(1);
>   f(1, 2);
>   f(1, 2, 3);
> }
>
> MSVC rejects with "t.c(5) : error C2708: 'f' : actual parameters length in
> bytes differs from previous call or reference".
>

I added a TODO for adding this and reshuffled the if to make this easy to
add. I'll give it a try once this patch is in.


>
> +  QualType NewQType = Context.getCanonicalType(NewFD->getType());
> +  const FunctionType *NewType = cast<FunctionType>(NewQType);
> +  FunctionType::ExtInfo NewTypeInfo = NewType->getExtInfo();
> +  if (NewTypeInfo.getCC() == CC_X86FastCall) {
> +    if (isa<FunctionNoProtoType>(NewType)) {
> +      Diag(NewFD->getLocation(), diag::err_cconv_knr)
> +          << FunctionType::getNameForCallConv(CC_X86FastCall);
> +    }
> +
> +    // Also diagnose fastcall with regparm.
>
> Before you moved this, there was other code to handle CC_X86FastCall and
> AT_Regparm nearby.  I think we should either move all regparm+fastcall
> diagnosis to after redeclaration merging or leave it where it is.
>

I moved the regparm code back to where it was.


>
> +    if (NewType->getHasRegParm()) {
> +      Diag(NewFD->getLocation(), diag::err_attributes_are_not_compatible)
> +          << "regparm" <<
> FunctionType::getNameForCallConv(CC_X86FastCall);
> +    }
> +  }
>
>
>
> On Sat, Jul 26, 2014 at 3:05 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> Hi,
>>
>> the attached patch delays semantic analysis of __fastcall until after
>> MergeFunctionDecl() has been called, PR20386.
>>
>> The motivation is code like this in a .c file:
>>
>> void __fastcall CrcGenerateTable(void);
>> void __fastcall CrcGenerateTable() {}
>>
>> Without this patch, __fastcall is analyzed before the definition of
>> CrcGenerateTable() is merged with the declaration, so Sema thinks the
>> function doesn't have a prototype. Since the error is only emitted on
>> FunctionNoProto functions, and functions in C++ or functions with more than
>> 0 arguments have a proto, this is only an issue for functions with 0
>> parameters in C files. See the bug for some more notes.
>>
>> (A side effect of moving the diagnostic is that the diagnostic now points
>> at the function instead of the attribute, and that the attribute is no
>> longer marked invalid. Neither seems like a problem.)
>>
>> Nico
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140731/bb768474/attachment.html>
-------------- next part --------------
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp	(revision 214046)
+++ lib/Sema/SemaDecl.cpp	(working copy)
@@ -7821,6 +7821,18 @@
   }
 
   // Semantic checking for this function declaration (in isolation).
+
+  // Diagnose the use of X86 fastcall on unprototyped functions.
+  QualType NewQType = Context.getCanonicalType(NewFD->getType());
+  const FunctionType *NewType = cast<FunctionType>(NewQType);
+  if (isa<FunctionNoProtoType>(NewType)) {
+    FunctionType::ExtInfo NewTypeInfo = NewType->getExtInfo();
+    if (NewTypeInfo.getCC() == CC_X86FastCall)
+      Diag(NewFD->getLocation(), diag::err_cconv_knr)
+          << FunctionType::getNameForCallConv(CC_X86FastCall);
+    // TODO: Also diagnose unprototyped stdcall functions?
+  }
+
   if (getLangOpts().CPlusPlus) {
     // C++-specific checks.
     if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(NewFD)) {
Index: lib/Sema/SemaType.cpp
===================================================================
--- lib/Sema/SemaType.cpp	(revision 214046)
+++ lib/Sema/SemaType.cpp	(working copy)
@@ -4564,23 +4564,12 @@
     }
   }
 
-  // Diagnose the use of X86 fastcall on unprototyped functions.
-  if (CC == CC_X86FastCall) {
-    if (isa<FunctionNoProtoType>(fn)) {
-      S.Diag(attr.getLoc(), diag::err_cconv_knr)
-        << FunctionType::getNameForCallConv(CC);
-      attr.setInvalid();
-      return true;
-    }
-
-    // Also diagnose fastcall with regparm.
-    if (fn->getHasRegParm()) {
-      S.Diag(attr.getLoc(), diag::err_attributes_are_not_compatible)
-        << "regparm"
-        << FunctionType::getNameForCallConv(CC);
-      attr.setInvalid();
-      return true;
-    }
+  // Also diagnose fastcall with regparm.
+  if (CC == CC_X86FastCall && fn->getHasRegParm()) {
+    S.Diag(attr.getLoc(), diag::err_attributes_are_not_compatible)
+        << "regparm" << FunctionType::getNameForCallConv(CC_X86FastCall);
+    attr.setInvalid();
+    return true;
   }
 
   // Modify the CC from the wrapped function type, wrap it all back, and then
Index: test/Sema/decl-microsoft-call-conv.c
===================================================================
--- test/Sema/decl-microsoft-call-conv.c	(revision 0)
+++ test/Sema/decl-microsoft-call-conv.c	(working copy)
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i686-pc-win32 -verify %s
+
+// It's important that this is a .c file.
+
+// This is fine, as CrcGenerateTable() has a prototype.
+void __fastcall CrcGenerateTable(void);
+void __fastcall CrcGenerateTable() {}
+
+void __fastcall CrcGenerateTableNoProto() {} // expected-error{{function with no prototype cannot use fastcall calling convention}}