[PATCH] Relax parse ordering rules for attributes

[aaron.ballman]

Summary

PR24559 brings up a long-standing issue in Clang where attribute order with differing syntax impacts whether a parse will succeed or fail with poor diagnostics. For instance:

struct attribute((lockable)) declspec(dllexport) S { }; ok
struct declspec(dllexport) attribute((lockable)) T { }; error about declaring anonymous structs and a warning about declarations not declaring anything

When you consider attributes as extraneous semantic information to attach to entities, ordering of the attributes really should not matter between the various attribute syntaxes we support -- they all serve the same purpose.

This patch begins to address the issue by providing a MaybeParseAttributes() function accepting a mask of what attribute syntaxes are allowed and parses the various syntaxes in a loop.

However, this patch is not complete, as the test cases for function attributes currently fail for the C++11-style attributes.

[[noreturn]] attribute((cdecl)) __declspec(dllexport) void f();

In this case, the [[noreturn]] is parsed as part of the top-level declaration, while the GNU and declspec attributes are parsed as part of the declaration specifier, and everything is accepted.

attribute((cdecl)) [[noreturn]] __declspec(dllexport) void f();

In this case, nothing is parsed as part of the top-level declaration, and everything is parsed as part of the declaration specifier. However, the C++ attribute is prohibited from appearing on a declaration specifier and so is diagnosed.

I think ParseDeclarationSpecifiers() needs to accept a ParsedAttributesWithRange object representing the top-level declaration attributes, and the parser needs to attach the C++ attributes to it up until we the first non-attribute token is found. But I'm not 100% certain that's the right approach.

Before taking the process further, I was wondering about two things:

1. Is the direction with MaybeParseAttributes() an acceptable approach?
2. If it is, is it reasonable to commit that, and work on the ParseDeclarationSpecifiers() portion in a follow-up commit?

Thanks!

~Aaron

[cfe-commits]

• msg-32543-315.txt159 BDownload

[aaron.ballman]

I've updated the patch to disallow reordering of C++11 attributes with relation to other attribute syntaxes. Additionally, if there is a reordering issue found, we now warn the user.

[rsmith]

lib/Parse/ParseDeclCXX.cpp

3923

Please provide an inlineable wrapper for this that checks if the current token is [, __declspec, or __attribute__ before calling into this, as we do for the existing MaybeParse* functions.

3942–3946

I'm not convinced this is right. It seems like there are two different cases:

1. Some context accepts a sequence of attributes before other things, such as:
   
   struct attribute((blah)) [[blah]] S;

There doesn't seem to be a principled reason to reject this.

2. Different attributes appear as different components of the grammar, such as:
   
   [[before_declaration]] attribute((im_a_decl_specifier)) int n; attribute((im_a_decl_specifier)) [[i_dont_go_here]] int n;

Only the second case should be rejected here. (And I think we should provide an error not just a warning if we don't like this case.)

I think the right thing to do is probably to always parse all attribute syntaxes (except for MS attributes, which are ambiguous with other constructs), and give a nicely-worded error if we see an unexpected syntax.

3956–3959

This seems wrong: given [[foo]] [[bar]], we'll now parse the second one as an MS attribute in a context where both are allowed.

[thakis]

Note to self: D95459 was a new attempt to fix this, which landed in https://reviews.llvm.org/rG9f2c7effd7f386e95aff3358500bc30974d35b0d

Thanks for that :)

include/clang/Basic/DiagnosticParseKinds.td

 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup<IgnoredAttributes>;
+def warn_attribute_ordering : Warning<
+  "C++11 attributes should precede all other attributes in an attribute list">,
+  InGroup<IgnoredAttributes>;
 def err_expected_method_body : Error<"expected method body">;
 def err_declspec_after_virtspec : Error<
   "'%0' qualifier may not appear after the virtual specifier '%1'">;

include/clang/Parse/Parser.h

       D.takeAttributes(attrs, endLoc);
     }
   }
-  void MaybeParseGNUAttributes(ParsedAttributes &attrs,
+  bool MaybeParseGNUAttributes(ParsedAttributes &attrs,
                                SourceLocation *endLoc = nullptr,
                                LateParsedAttrList *LateAttrs = nullptr) {
-    if (Tok.is(tok::kw___attribute))
+    if (Tok.is(tok::kw___attribute)) {
       ParseGNUAttributes(attrs, endLoc, LateAttrs);
+      return true;
+    }
+    return false;
   }
   void ParseGNUAttributes(ParsedAttributes &attrs,
                           SourceLocation *endLoc = nullptr,

       D.takeAttributes(attrs, endLoc);
     }
   }
-  void MaybeParseCXX11Attributes(ParsedAttributes &attrs,
+  bool MaybeParseCXX11Attributes(ParsedAttributes &attrs,
                                  SourceLocation *endLoc = nullptr) {
     if (getLangOpts().CPlusPlus11 && isCXX11AttributeSpecifier()) {
       ParsedAttributesWithRange attrsWithRange(AttrFactory);
       ParseCXX11Attributes(attrsWithRange, endLoc);
       attrs.takeAllFrom(attrsWithRange);
+      return true;
     }
+    return false;
   }
-  void MaybeParseCXX11Attributes(ParsedAttributesWithRange &attrs,
+  bool MaybeParseCXX11Attributes(ParsedAttributesWithRange &attrs,
                                  SourceLocation *endLoc = nullptr,
                                  bool OuterMightBeMessageSend = false) {
     if (getLangOpts().CPlusPlus11 &&
-        isCXX11AttributeSpecifier(false, OuterMightBeMessageSend))
+        isCXX11AttributeSpecifier(false, OuterMightBeMessageSend)) {
       ParseCXX11Attributes(attrs, endLoc);
+      return true;
+    }
+    return false;
   }
 
   void ParseCXX11AttributeSpecifier(ParsedAttributes &attrs,

 
   IdentifierInfo *TryParseCXX11AttributeIdentifier(SourceLocation &Loc);
 
-  void MaybeParseMicrosoftAttributes(ParsedAttributes &attrs,
+  bool MaybeParseMicrosoftAttributes(ParsedAttributes &attrs,
                                      SourceLocation *endLoc = nullptr) {
-    if (getLangOpts().MicrosoftExt && Tok.is(tok::l_square))
+    if (getLangOpts().MicrosoftExt && Tok.is(tok::l_square)) {
       ParseMicrosoftAttributes(attrs, endLoc);
+      return true;
+    }
+    return false;
   }
   void ParseMicrosoftAttributes(ParsedAttributes &attrs,
                                 SourceLocation *endLoc = nullptr);
-  void MaybeParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
+  bool MaybeParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                     SourceLocation *End = nullptr) {
     const auto &LO = getLangOpts();
     if ((LO.MicrosoftExt || LO.Borland || LO.CUDA) &&
-        Tok.is(tok::kw___declspec))
+        Tok.is(tok::kw___declspec)) {
       ParseMicrosoftDeclSpecs(Attrs, End);
+      return true;
+    }
+    return false;
   }
   void ParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                SourceLocation *End = nullptr);

   bool ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
                                   SourceLocation AttrNameLoc,
                                   ParsedAttributes &Attrs);
+
+  enum ParseAttrKindMask {
+    PAKM_GNU = 1 << 0,
+    PAKM_Declspec = 1 << 1,
+    PAKM_CXX11 = 1 << 2,
+    PAKM_Microsoft = 1 << 3
+  };
+  
+  /// \brief Possibly parse attributes based on what syntaxes are desired,
+  /// allowing for the order to vary. e.g. with PAKM_GNU | PAKM_Declspec:
+  /// __attribute__((...)) __declspec(...) __attribute__((...)))
+  void MaybeParseAttributes(unsigned WhichAttrKinds,
+                            ParsedAttributesWithRange &Attrs,
+                            SourceLocation *End = nullptr,
+                            LateParsedAttrList *LateAttrs = nullptr);
+  /// \brief Possibly parse attributes based on what syntaxes are desired,
+  /// allowing for the order to vary. e.g. with PAKM_GNU | PAKM_Declspec:
+  /// __attribute__((...)) __declspec(...) __attribute__((...)))
+  void MaybeParseAttributes(unsigned WhichAttrKinds, ParsedAttributes &Attrs,
+                            SourceLocation *End = nullptr,
+                            LateParsedAttrList *LateAttrs = nullptr) {
+    ParsedAttributesWithRange AttrsWithRange(AttrFactory);
+    MaybeParseAttributes(WhichAttrKinds, AttrsWithRange, End, LateAttrs);
+    Attrs.takeAllFrom(AttrsWithRange);
+  }
+
   void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs);
   void DiagnoseAndSkipExtendedMicrosoftTypeAttributes();
   SourceLocation SkipExtendedMicrosoftTypeAttributes();

lib/Parse/ParseDecl.cpp

       continue;
     }
 
-    // GNU attributes support.
+    // Attribute support.
     case tok::kw___attribute:
-      ParseGNUAttributes(DS.getAttributes(), nullptr, LateAttrs);
-      continue;
-
-    // Microsoft declspec support.
     case tok::kw___declspec:
-      ParseMicrosoftDeclSpecs(DS.getAttributes());
+      MaybeParseAttributes(PAKM_GNU | PAKM_Declspec, DS.getAttributes(),
+                           nullptr, LateAttrs);
       continue;
 
     // Microsoft single token adornments.

 
   // If attributes exist after tag, parse them.
   ParsedAttributesWithRange attrs(AttrFactory);
-  MaybeParseGNUAttributes(attrs);
-  MaybeParseCXX11Attributes(attrs);
-  MaybeParseMicrosoftDeclSpecs(attrs);
+  MaybeParseAttributes(PAKM_GNU | PAKM_CXX11 | PAKM_Declspec, attrs);
 
   SourceLocation ScopedEnumKWLoc;
   bool IsScopedUsingClassTag = false;

     ProhibitAttributes(attrs);
 
     // They are allowed afterwards, though.
-    MaybeParseGNUAttributes(attrs);
-    MaybeParseCXX11Attributes(attrs);
-    MaybeParseMicrosoftDeclSpecs(attrs);
+    MaybeParseAttributes(PAKM_GNU | PAKM_CXX11 | PAKM_Declspec, attrs);
   }
 
   // C++11 [temp.explicit]p12:

     // Just use the ParsingDeclaration "scope" of the declarator.
     DeclSpec DS(AttrFactory);
 
-    // Parse any C++11 attributes.
-    MaybeParseCXX11Attributes(DS.getAttributes());
+    // Parse any C++11 and Microsoft attributes.
+    MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, DS.getAttributes());
 
-    // Skip any Microsoft attributes before a param.
-    MaybeParseMicrosoftAttributes(DS.getAttributes());
-
     SourceLocation DSStart = Tok.getLocation();
 
     // If the caller parsed attributes for the first argument, add them now.

lib/Parse/ParseDeclCXX.cpp

   if (index == Ident.size()) {
     while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
       ParsedAttributesWithRange attrs(AttrFactory);
-      MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
+      MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
       ParseExternalDeclaration(attrs);
     }
 

                 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributesWithRange attrs(AttrFactory);
-  MaybeParseCXX11Attributes(attrs);
-  MaybeParseMicrosoftAttributes(attrs);
+  MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
 
   if (Tok.isNot(tok::l_brace)) {
     // Reset the source range in DS, as the leading "extern"

       // Fall through.
     default:
       ParsedAttributesWithRange attrs(AttrFactory);
-      MaybeParseCXX11Attributes(attrs);
-      MaybeParseMicrosoftAttributes(attrs);
+      MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
       ParseExternalDeclaration(attrs);
       continue;
     }

   }
 
   ParsedAttributesWithRange Attrs(AttrFactory);
-  MaybeParseGNUAttributes(Attrs);
-  MaybeParseCXX11Attributes(Attrs);
+  MaybeParseAttributes(PAKM_CXX11 | PAKM_GNU, Attrs);
 
   // Maybe this is an alias-declaration.
   TypeResult TypeAlias;

 
   ParsedAttributesWithRange attrs(AttrFactory);
   // If attributes exist after tag, parse them.
-  MaybeParseGNUAttributes(attrs);
-  MaybeParseMicrosoftDeclSpecs(attrs);
+  MaybeParseAttributes(PAKM_Declspec | PAKM_GNU | PAKM_CXX11, attrs);
 
   // Parse inheritance specifiers.
   if (Tok.isOneOf(tok::kw___single_inheritance,

                   tok::kw___virtual_inheritance))
     ParseMicrosoftInheritanceClassAttributes(attrs);
 
-  // If C++0x attributes exist here, parse them.
-  // FIXME: Are we consistent with the ordering of parsing of different
-  // styles of attributes?
-  MaybeParseCXX11Attributes(attrs);
-
   // Source location used by FIXIT to insert misplaced
   // C++11 attributes
   SourceLocation AttrFixitLoc = Tok.getLocation();

   
   Braces.consumeClose();
 }
+
+void Parser::MaybeParseAttributes(unsigned WhichAttrKinds,
+                                  ParsedAttributesWithRange &Attrs,
+                                  SourceLocation *End,
+                                  LateParsedAttrList *LateAttrs) {
+  // C++11-style attributes cannot be reordered with respect to other
+  // attribute syntaxes. Per [dcl.spec]p1, those attributes written in a
+  // decl-specifier position will appertain to the decl-specifier, and GNU-
+  // and Declspec-style attributes are decl-specifiers. So if C++ attributes
+  // are parsed, they must be the first attributes parsed in the group,
+  // otherwise we diagnose.
+  bool CXX11AttrAllowed = true;
+  bool MoreToParse;
+  do {
+    // Assume there's nothing left to parse, but if any attributes are in fact
+    // parsed, loop to ensure all attribute combinations are parsed.
+    MoreToParse = false;
+    if (WhichAttrKinds & PAKM_CXX11) {
+      SourceLocation AttrStartLoc = Tok.getLocation();
+      MoreToParse |= MaybeParseCXX11Attributes(Attrs, End);
+      // If C++11 attributes are not allowed because other attributes have
+      // already been parsed, we should diagnose if such an attribute was
+      // parsed.
+      if (MoreToParse && !CXX11AttrAllowed)
+        Diag(AttrStartLoc, diag::warn_attribute_ordering);
+    }
+    if (WhichAttrKinds & PAKM_GNU) {
+      MoreToParse |= MaybeParseGNUAttributes(Attrs, End, LateAttrs);
+      CXX11AttrAllowed = false;
+    }
+    if (WhichAttrKinds & PAKM_Declspec) {
+      MoreToParse |= MaybeParseMicrosoftDeclSpecs(Attrs, End);
+      CXX11AttrAllowed = false;
+    }
+    if (WhichAttrKinds & PAKM_Microsoft) {
+      MoreToParse |= MaybeParseMicrosoftAttributes(Attrs, End);
+      CXX11AttrAllowed = false;
+    }
+  } while (MoreToParse);
+}

lib/Parse/ParseExprCXX.cpp

     SourceLocation RParenLoc = T.getCloseLocation();
     DeclEndLoc = RParenLoc;
 
-    // GNU-style attributes must be parsed before the mutable specifier to be
-    // compatible with GCC.
-    MaybeParseGNUAttributes(Attr, &DeclEndLoc);
+    // GNU-style and __declspec attributes must be parsed before the mutable
+    // specifier to be compatible with GCC/MSVC.
+    MaybeParseAttributes(PAKM_GNU | PAKM_Declspec, Attr, &DeclEndLoc);
 
-    // MSVC-style attributes must be parsed before the mutable specifier to be
-    // compatible with MSVC.
-    MaybeParseMicrosoftDeclSpecs(Attr, &DeclEndLoc);
-
     // Parse 'mutable'[opt].
     SourceLocation MutableLoc;
     if (TryConsumeToken(tok::kw_mutable, MutableLoc))

lib/Parse/Parser.cpp

   }
 
   ParsedAttributesWithRange attrs(AttrFactory);
-  MaybeParseCXX11Attributes(attrs);
-  MaybeParseMicrosoftAttributes(attrs);
+  MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
 
   Result = ParseExternalDeclaration(attrs);
   return false;

   // FIXME: Support module import within __if_exists?
   while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     ParsedAttributesWithRange attrs(AttrFactory);
-    MaybeParseCXX11Attributes(attrs);
-    MaybeParseMicrosoftAttributes(attrs);
+    MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
     DeclGroupPtrTy Result = ParseExternalDeclaration(attrs);
     if (Result && !getCurScope()->getParent())
       Actions.getASTConsumer().HandleTopLevelDecl(Result.get());

test/Parser/attr-order.cpp

+// RUN: %clang_cc1 -fsyntax-only -fms-extensions -std=c++14 -verify %s
+
+struct [[]] __attribute__((lockable)) __declspec(dllexport) A {}; // ok
+struct [[]] __declspec(dllexport) __attribute__((lockable)) B {}; // ok
+struct [[]] [[]] __declspec(dllexport) __attribute__((lockable)) C {}; // ok
+struct __declspec(dllexport) [[]] __attribute__((lockable)) D {}; // expected-warning {{C++11 attributes should precede all other attributes in an attribute list}}
+struct __declspec(dllexport) __attribute__((lockable)) [[]] E {}; // expected-warning {{C++11 attributes should precede all other attributes in an attribute list}}
+struct __attribute__((lockable)) __declspec(dllexport) [[]] F {}; // expected-warning {{C++11 attributes should precede all other attributes in an attribute list}}
+struct __attribute__((lockable)) [[]] __declspec(dllexport) G {}; // expected-warning {{C++11 attributes should precede all other attributes in an attribute list}}
+struct [[]] __attribute__((lockable)) [[]] __declspec(dllexport) H {}; // expected-warning {{C++11 attributes should precede all other attributes in an attribute list}}
+
+[[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void a(); // ok
+[[noreturn]] __declspec(dllexport) __attribute__((cdecl)) void b(); // ok
+[[]] [[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void c(); // ok
+
+// FIXME: The following cases should report the same warning diagnostic as
+// above. However, that requires changing the way we parse decl specifiers vs
+// top-level declarations. See PR24559 for more details.
+__declspec(dllexport) [[noreturn]] __attribute__((cdecl)) void d(); // expected-error {{an attribute list cannot appear here}}
+__declspec(dllexport) __attribute__((cdecl)) [[noreturn]] void e(); // expected-error {{an attribute list cannot appear here}}
+__attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(); // expected-error {{an attribute list cannot appear here}}
+__attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void g(); // expected-error {{an attribute list cannot appear here}}
+[[noreturn]] __attribute__((cdecl)) [[]] __declspec(dllexport) void h(); // expected-error {{an attribute list cannot appear here}}