Fix a parser bug where we let attributes interfere with our disambiguation

of whether a '(' was a grouping paren or the start of a function declarator.
This is PR2796.

Now we eat the attribute before deciding whether the paren is grouping or
not, then apply it to the resultant decl or to the first argument as needed.

One somewhat surprising aspect of this is that attributes interact with
implicit int in cases like this:

void a(x, y) // k&r style function
void b(__attribute__(()) x, y); // function with two implicit int arguments
void c(x, __attribute__(()) y); // error, can't have attr in identifier list.

Fun stuff.

llvm-svn: 57790

clang/include/clang/Basic/DiagnosticKinds.def

@@ -246,8 +246,7 @@ DIAG(err_pp_I_dash_not_supported, ERROR,
 //===----------------------------------------------------------------------===//
 // Parser Diagnostics
 //===----------------------------------------------------------------------===//
-DIAG(w_type_defaults_to_int, WARNING,
-     "type defaults to 'int'")
+
 DIAG(w_no_declarators, WARNING,
      "declaration does not declare anything")
 DIAG(w_asm_qualifier_ignored, WARNING,
@@ -757,6 +756,8 @@ DIAG(err_attribute_iboutlet_non_ivar, ERROR,
      "'iboutlet' attribute can only be applied to instance variables")
 
 // Function Parameter Semantic Analysis.
+DIAG(err_argument_required_after_attribute, ERROR,
+     "argument required after attribute")
 DIAG(err_param_with_void_type, ERROR,
      "argument may not have 'void' type")
 DIAG(err_void_only_param, ERROR,

clang/include/clang/Parse/Parser.h

@@ -692,7 +692,9 @@ private:
   void ParseTypeQualifierListOpt(DeclSpec &DS);
   void ParseDirectDeclarator(Declarator &D);
   void ParseParenDeclarator(Declarator &D);
-  void ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D);
+  void ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D,
+                               AttributeList *AttrList = 0,
+                               bool RequiresArg = false);
   void ParseFunctionDeclaratorIdentifierList(SourceLocation LParenLoc,
                                              Declarator &D);
   void ParseBracketDeclarator(Declarator &D);

clang/lib/Parse/ParseDecl.cpp

@@ -1227,15 +1227,16 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
   
   while (1) {
     if (Tok.is(tok::l_paren)) {
-      // When not in file scope, warn for ambiguous function declarators, just
-      // in case the author intended it as a variable definition.
-      bool warnIfAmbiguous = D.getContext() != Declarator::FileContext;
       // The paren may be part of a C++ direct initializer, eg. "int x(1);".
       // In such a case, check if we actually have a function declarator; if it
       // is not, the declarator has been fully parsed.
-      if (getLang().CPlusPlus && D.mayBeFollowedByCXXDirectInit() &&
-          !isCXXFunctionDeclarator(warnIfAmbiguous))
-        break;
+      if (getLang().CPlusPlus && D.mayBeFollowedByCXXDirectInit()) {
+        // When not in file scope, warn for ambiguous function declarators, just
+        // in case the author intended it as a variable definition.
+        bool warnIfAmbiguous = D.getContext() != Declarator::FileContext;
+        if (!isCXXFunctionDeclarator(warnIfAmbiguous))
+          break;
+      }
       ParseFunctionDeclarator(ConsumeParen(), D);
     } else if (Tok.is(tok::l_square)) {
       ParseBracketDeclarator(D);
@@ -1253,11 +1254,35 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
 ///       direct-declarator:
 ///         '(' declarator ')'
 /// [GNU]   '(' attributes declarator ')'
+///         direct-declarator '(' parameter-type-list ')'
+///         direct-declarator '(' identifier-list[opt] ')'
+/// [GNU]   direct-declarator '(' parameter-forward-declarations
+///                    parameter-type-list[opt] ')'
 ///
 void Parser::ParseParenDeclarator(Declarator &D) {
   SourceLocation StartLoc = ConsumeParen();
   assert(!D.isPastIdentifier() && "Should be called before passing identifier");
   
+  // Eat any attributes before we look at whether this is a grouping or function
+  // declarator paren.  If this is a grouping paren, the attribute applies to
+  // the type being built up, for example:
+  //     int (__attribute__(()) *x)(long y)
+  // If this ends up not being a grouping paren, the attribute applies to the
+  // first argument, for example:
+  //     int (__attribute__(()) int x)
+  // In either case, we need to eat any attributes to be able to determine what
+  // sort of paren this is.
+  //
+  AttributeList *AttrList = 0;
+  bool RequiresArg = false;
+  if (Tok.is(tok::kw___attribute)) {
+    AttrList = ParseAttributes();
+    
+    // We require that the argument list (if this is a non-grouping paren) be
+    // present even if the attribute list was empty.
+    RequiresArg = true;
+  }
+  
   // If we haven't past the identifier yet (or where the identifier would be
   // stored, if this is an abstract declarator), then this is probably just
   // grouping parens. However, if this could be an abstract-declarator, then
@@ -1285,10 +1310,9 @@ void Parser::ParseParenDeclarator(Declarator &D) {
   if (isGrouping) {
     bool hadGroupingParens = D.hasGroupingParens();
     D.setGroupingParens(true);
+    if (AttrList)
+      D.AddAttributes(AttrList);
 
-    if (Tok.is(tok::kw___attribute))
-      D.AddAttributes(ParseAttributes());
-    
     ParseDeclaratorInternal(D);
     // Match the ')'.
     MatchRHSPunctuation(tok::r_paren, StartLoc);
@@ -1299,17 +1323,23 @@ void Parser::ParseParenDeclarator(Declarator &D) {
   
   // Okay, if this wasn't a grouping paren, it must be the start of a function
   // argument list.  Recognize that this declarator will never have an
-  // identifier (and remember where it would have been), then fall through to
-  // the handling of argument lists.
+  // identifier (and remember where it would have been), then call into
+  // ParseFunctionDeclarator to handle of argument list.
   D.SetIdentifier(0, Tok.getLocation());
 
-  ParseFunctionDeclarator(StartLoc, D); 
+  ParseFunctionDeclarator(StartLoc, D, AttrList, RequiresArg);
 }
 
 /// ParseFunctionDeclarator - We are after the identifier and have parsed the
 /// declarator D up to a paren, which indicates that we are parsing function
 /// arguments.
 ///
+/// If AttrList is non-null, then the caller parsed those arguments immediately
+/// after the open paren - they should be considered to be the first argument of
+/// a parameter.  If RequiresArg is true, then the first argument of the
+/// function is required to be present and required to not be an identifier
+/// list.
+///
 /// This method also handles this portion of the grammar:
 ///       parameter-type-list: [C99 6.7.5]
 ///         parameter-list
@@ -1328,14 +1358,19 @@ void Parser::ParseParenDeclarator(Declarator &D) {
 ///           '=' assignment-expression
 /// [GNU]   declaration-specifiers abstract-declarator[opt] attributes
 ///
-void Parser::ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D) {
+void Parser::ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D,
+                                     AttributeList *AttrList,
+                                     bool RequiresArg) {
   // lparen is already consumed!
   assert(D.isPastIdentifier() && "Should not call before identifier!");
   
-  // Okay, this is the parameter list of a function definition, or it is an
-  // identifier list of a K&R-style function.
-  
+  // This parameter list may be empty.
   if (Tok.is(tok::r_paren)) {
+    if (RequiresArg) {
+      Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+      delete AttrList;
+    }
+    
     // Remember that we parsed a function type, and remember the attributes.
     // int() -> no prototype, no '...'.
     D.AddTypeInfo(DeclaratorChunk::getFunction(/*prototype*/ false,
@@ -1344,10 +1379,19 @@ void Parser::ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D) {
     
     ConsumeParen();  // Eat the closing ')'.
     return;
-  } else if (Tok.is(tok::identifier) &&
-             // K&R identifier lists can't have typedefs as identifiers, per
-             // C99 6.7.5.3p11.
-             !Actions.isTypeName(*Tok.getIdentifierInfo(), CurScope)) {
+  } 
+  
+  // Alternatively, this parameter list may be an identifier list form for a
+  // K&R-style function:  void foo(a,b,c)
+  if (Tok.is(tok::identifier) &&
+      // K&R identifier lists can't have typedefs as identifiers, per
+      // C99 6.7.5.3p11.
+      !Actions.isTypeName(*Tok.getIdentifierInfo(), CurScope)) {
+    if (RequiresArg) {
+      Diag(Tok.getLocation(), diag::err_argument_required_after_attribute);
+      delete AttrList;
+    }
+    
     // Identifier list.  Note that '(' identifier-list ')' is only allowed for
     // normal declarators, not for abstract-declarators.
     return ParseFunctionDeclaratorIdentifierList(LParenLoc, D);
@@ -1383,6 +1427,12 @@ void Parser::ParseFunctionDeclarator(SourceLocation LParenLoc, Declarator &D) {
     
     // Parse the declaration-specifiers.
     DeclSpec DS;
+
+    // If the caller parsed attributes for the first argument, add them now.
+    if (AttrList) {
+      DS.AddAttributes(AttrList);
+      AttrList = 0;  // Only apply the attributes to the first parameter.
+    }
     ParseDeclarationSpecifiers(DS);
 
     // Parse the declarator.  This is "PrototypeContext", because we must

clang/test/Parser/attributes.c

@@ -1,6 +1,45 @@
-// RUN: clang -fsyntax-only -verify %s -pedantic
+// RUN: clang -fsyntax-only -verify %s -pedantic -std=c99
 
-static __inline void __attribute__((__always_inline__, __nodebug__)) // expected-warning {{extension used}}
-foo (void)
-{
+int __attribute__(()) x;  // expected-warning {{extension used}}
+
+// Hide __attribute__ behind a macro, to silence extension warnings about
+// "__attribute__ being an extension".
+#define attribute __attribute__
+
+__inline void attribute((__always_inline__, __nodebug__))
+foo(void) {
 }
+
+
+attribute(()) y;   // expected-warning {{defaults to 'int'}}
+
+// PR2796
+int (attribute(()) *z)(long y);
+
+
+void f1(attribute(()) int x);
+
+int f2(y, attribute(()) x);     // expected-error {{expected identifier}}
+
+// This is parsed as a normal argument list (with two args that are implicit
+// int) because the attribute is a declspec.
+void f3(attribute(()) x,  // expected-warning {{defaults to 'int'}}
+        y);               // expected-warning {{defaults to 'int'}}
+
+void f4(attribute(()));   // expected-error {{expected parameter declarator}}
+
+
+// This is ok, the attribute applies to the pointer.
+int baz(int (attribute(()) *x)(long y));
+
+void g1(void (*f1)(attribute(()) int x));
+void g2(int (*f2)(y, attribute(()) x));    // expected-error {{expected identifier}}
+void g3(void (*f3)(attribute(()) x, int y));  // expected-warning {{defaults to 'int'}}
+void g4(void (*f4)(attribute(())));  // expected-error {{expected parameter declarator}}
+
+
+void (*h1)(void (*f1)(attribute(()) int x));
+void (*h2)(int (*f2)(y, attribute(()) x));    // expected-error {{expected identifier}}
+
+void (*h3)(void (*f3)(attribute(()) x));   // expected-warning {{defaults to 'int'}}
+void (*h4)(void (*f4)(attribute(())));  // expected-error {{expected parameter declarator}}