[Parse] Fix `TryParsePtrOperatorSeq`.

[michele.scandale]

Summary

The syntax rules for ptr-operator allow attributes after *, &,
&&, therefore we should be able to parse the following:

void fn() {
    void (*[[attr]] x)() = &fn;
    void (&[[attr]] y)() = fn;
    void (&&[[attr]] z)() = fn;
}

However the current logic in TryParsePtrOperatorSeq does not consider
the presence of attributes leading to unexpected parsing errors.

Moreover we should also consider _Atomic a possible qualifier that can
appear after the sequence of attribute specifiers.

[michele.scandale]

Missed GNU style attributes

[rjmccall]

clang/lib/Parse/ParseTentative.cpp

811

Do you want to check for double-brackets here?

[michele.scandale]

Check for double brackets for C++11 style attributes, and outline the loop to skip attributes in TrySkipAttributes to fix also the other use in TryConsumeDeclarationSpecifier

[michele.scandale]

clang/lib/Parse/ParseTentative.cpp

811

The copy of the code I was replicating here wasn't doing that. But it makes sense to check at least that.

[rjmccall]

clang/lib/Parse/ParseTentative.cpp

780

Okay, great. The check for adjacent squares at the end makes sure this doesn't catch an ObjC message send, in case this is ever used in a context where there's attribute/expression ambiguity; that might be worth a comment. Otherwise LGTM.

[michele.scandale]

Add comment about ObjC message send being rejected + rebase

[michele.scandale]

Assuming this is fine, I would need someone landing this change on my behalf since I do not have commit rights. Thanks!

clang/lib/Parse/ParseTentative.cpp

780

Comment added. Let me know if the wording is fine.

[rjmccall]

LGTM.

[michele.scandale]

@rsmith: is this fine for you as well?

If this is ready for landing, I would appreciate if somebody can land this on my behalf since I do not have commit rights. Thanks!

[rsmith]

clang/include/clang/Parse/Parser.h

2445

Stray "of' here. (And an 's' after "attribute-specifier" would improve readability.)

[michele.scandale]

Fixed comment wording + rebase.

[aaron.ballman]

clang/lib/Parse/ParseTentative.cpp

773

Do we need to also skip other keyword attributes as well as alignas? For instance, tok::kw__Noreturn or tok::kw__Alignas

779

I think this gets the parsing logic wrong when there is a balanced [] that is not part of the attribute introducer syntax, such as finding an array in the middle of the attribute argument clause. A test case would be handy, like:

constexpr int foo[] = { 2, 4 };
[[gnu::assume_aligned(foo[0])]] int *func() { return nullptr; }

You should probably use a BalancedDelimiterTracker to handle the brackets.

789

If we're skipping __attribute__(()), this looks to miss the second closing right paren. Also, this logic won't work if we need to skip _Noreturn, which has no parens.

This branch is missing test coverage.

[rsmith]

clang/lib/Parse/ParseTentative.cpp

773

_Alignas and _Noreturn are their own kinds of decl-specifiers, not attributes. In particular, they're not syntactically valid in any of the places that call this (after a * or a tag keyword). It looks like we're missing support for them, but we should recognize them in isCXXDeclarationSpecifier, not here -- but I think that's unrelated to this patch.

779

SkipUntil properly handles nested brackets; I think this is fine (though more testcases can't hurt!). I don't think BalancedDelimiterTracker would help -- this function can intentionally terminate with unbalanced delimiters, if it bails out part-way through skipping an attribute.

As for a testcase, the example you give above doesn't get here. Perhaps something like:

int *p;
void f(float *q) {
  float(* [[attr([])]] p);
  p = q; // check we disambiguated as a declaration not an expression
}

789

Because SkipUntil handles nested parens, this does the right thing for __attribute__((...)). Agreed on test coverage :)

811–813

We're missing _Atomic from this list. Eg:

void f() {
  int (*_Atomic p);
}

... is incorrectly disambiguated as an expression.

[aaron.ballman]

clang/lib/Parse/ParseTentative.cpp

773

Okay, that's a good point. Thanks!

789

I'll update the docs for SkipUntil(), because they don't suggest that this magic takes place. :-)

[michele.scandale]

Tests for __attribute__(()) syntax + attribute arguments + fix for _Atomic qualifier.

[aaron.ballman]

Thank you for the new tests and drive-by fix for _Atomic! LGTM

[aaron.ballman]

Thank you for the patch -- I've commit on your behalf in bd5b22070b6984d89c13b6cf38c3e54fc98ce291

clang/include/clang/Parse/Parser.h

TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = true,
                               bool mayHaveDirectInit = false);
   TPResult
   TryParseParameterDeclarationClause(bool *InvalidAsDeclaration = nullptr,
                                      bool VersusTemplateArg = false);
   TPResult TryParseFunctionDeclarator();
   TPResult TryParseBracketDeclarator();
   TPResult TryConsumeDeclarationSpecifier();
 
+  /// Try to skip a possibly empty sequence of 'attribute-specifier's without
+  /// full validation of the syntactic structure of attributes.
+  bool TrySkipAttributes();
+
 public:
   TypeResult ParseTypeName(SourceRange *Range = nullptr,
                            DeclaratorContext Context
                              = DeclaratorContext::TypeNameContext,
                            AccessSpecifier AS = AS_none,
                            Decl **OwnedType = nullptr,
                            ParsedAttributes *Attrs = nullptr);
 

clang/lib/Parse/ParseTentative.cpp

case tok::kw_enum:
     //         nested-name-specifier[opt] identifier
     //     class-key nested-name-specifier[opt] template[opt] simple-template-id
     //     enum nested-name-specifier[opt] identifier
     //
     // FIXME: We don't support class-specifiers nor enum-specifiers here.
     ConsumeToken();
 
     // Skip attributes.
-    while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
-                       tok::kw_alignas)) {
-      if (Tok.is(tok::l_square)) {
-        ConsumeBracket();
-        if (!SkipUntil(tok::r_square))
-          return TPResult::Error;
-      } else {
-        ConsumeToken();
-        if (Tok.isNot(tok::l_paren))
-          return TPResult::Error;
-        ConsumeParen();
-        if (!SkipUntil(tok::r_paren))
-          return TPResult::Error;
-      }
-    }
+    if (!TrySkipAttributes())
 
     if (TryAnnotateOptionalCXXScopeToken())
       return TPResult::Error;
     if (Tok.is(tok::annot_cxxscope))
       ConsumeAnnotationToken();
     if (Tok.is(tok::identifier))
       ConsumeToken();
     else if (Tok.is(tok::annot_template_id))
Parser::isCXX11AttributeSpecifier(bool Disambiguate,
   if (IsAttribute)
     // Case 1: C++11 statement attribute.
     return CAK_AttributeSpecifier;
 
   // Case 3: Message send.
   return CAK_NotAttributeSpecifier;
 }
 
+bool Parser::TrySkipAttributes() {
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+                     tok::kw_alignas)) {
+    if (Tok.is(tok::l_square)) {
+      ConsumeBracket();
+      if (Tok.isNot(tok::l_square))
+        return false;
+      ConsumeBracket();
+      if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square))
+        return false;
+      // Note that explicitly checking for `[[` and `]]` allows to fail as
+      // expected in the case of the Objective-C message send syntax.
+      ConsumeBracket();
+    } else {
+      ConsumeToken();
+      if (Tok.isNot(tok::l_paren))
+        return false;
+      ConsumeParen();
+      if (!SkipUntil(tok::r_paren))
+        return false;
+    }
+  }
+
+  return true;
+}
+
 Parser::TPResult Parser::TryParsePtrOperatorSeq() {
   while (true) {
     if (TryAnnotateOptionalCXXScopeToken(true))
       return TPResult::Error;
 
     if (Tok.isOneOf(tok::star, tok::amp, tok::caret, tok::ampamp) ||
         (Tok.is(tok::annot_cxxscope) && NextToken().is(tok::star))) {
       // ptr-operator
       ConsumeAnyToken();
+
+      // Skip attributes.
+      if (!TrySkipAttributes())
+        return TPResult::Error;
+
       while (Tok.isOneOf(tok::kw_const, tok::kw_volatile, tok::kw_restrict,
                          tok::kw__Nonnull, tok::kw__Nullable,
-                         tok::kw__Null_unspecified))
+                         tok::kw__Null_unspecified, tok::kw__Atomic))
         ConsumeToken();
     } else {
       return TPResult::True;
     }
   }
 }
 
 ///         operator-function-id:

clang/test/CXX/dcl.decl/p4-0x.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 struct X {
   void f() &;
   void g() &&;
 };
 
 void (X::*pmf)() & = &X::f;
+
+void fn() {
+  void (*[[attr]] fn_ptr)() = &fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (*[[attrA]] *[[attrB]] fn_ptr_ptr)() = &fn_ptr; // expected-warning{{unknown attribute 'attrA' ignored}} expected-warning{{unknown attribute 'attrB' ignored}}
+
+  void (&[[attr]] fn_lref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (&&[[attr]] fn_rref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+
+  int i[5];
+  int (*[[attr(i[1])]] pi);  // expected-warning{{unknown attribute 'attr' ignored}}
+  pi = &i[0];
+}

clang/test/Parser/cxx-ambig-decl-expr.cpp

struct A {
     struct Q { int n; };
     int operator[](Q);
   } a;
   int(a[{0}]); // expected-warning {{unused}}
 
   // These are array declarations.
   int(x[(1,1)]); // expected-error {{redefinition}}
   int(x[true ? 1,1 : 1]); // expected-error {{redefinition}}
+
+  int (*_Atomic atomic_ptr_to_int);
+  *atomic_ptr_to_int = 42;
 }

clang/test/Parser/cxx-attributes.cpp

namespace PR17666 {
   typedef int __attribute__((__aligned__(A))) T1;
   int check1[__alignof__(T1) == 1 ? 1 : -1];
 
   typedef int __attribute__((aligned(int(1)))) T1;
   typedef int __attribute__((aligned(int))) T2; // expected-error {{expected '(' for function-style cast}}
 }
 
 __attribute((typename)) int x; // expected-warning {{unknown attribute 'typename' ignored}}
+
+void fn() {
+  void (*__attribute__((attr)) fn_ptr)() = &fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (*__attribute__((attrA)) *__attribute__((attrB)) fn_ptr_ptr)() = &fn_ptr; // expected-warning{{unknown attribute 'attrA' ignored}} expected-warning{{unknown attribute 'attrB' ignored}}
+
+  void (&__attribute__((attr)) fn_lref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (&&__attribute__((attr)) fn_rref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+
+  int i[5];
+  int (*__attribute__((attr(i[1]))) pi);  // expected-warning{{unknown attribute 'attr' ignored}}
+  pi = &i[0];
+}