aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, hans.
aaron.ballman added a subscriber: cfe-commits.
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
http://reviews.llvm.org/D12375
Files:
include/clang/Parse/Parser.h
lib/Parse/ParseDecl.cpp
lib/Parse/ParseDeclCXX.cpp
lib/Parse/ParseExprCXX.cpp
lib/Parse/Parser.cpp
test/Parser/attr-order.cpp
Index: test/Parser/attr-order.cpp
===================================================================
--- test/Parser/attr-order.cpp
+++ test/Parser/attr-order.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -fms-extensions -std=c++14 -verify %s
+// expected-no-diagnostics
+
+struct [[]] __attribute__((lockable)) __declspec(dllexport) A {};
+struct [[]] __declspec(dllexport) __attribute__((lockable)) B {};
+struct __declspec(dllexport) [[]] __attribute__((lockable)) C {};
+struct __declspec(dllexport) __attribute__((lockable)) [[]] D {};
+struct __attribute__((lockable)) __declspec(dllexport) [[]] E {};
+struct __attribute__((lockable)) [[]] __declspec(dllexport) F {};
+
+[[]] __attribute__((cdecl)) __declspec(dllexport) void a();
+[[]] __declspec(dllexport) __attribute__((cdecl)) void b();
+__declspec(dllexport) [[]] __attribute__((cdecl)) void c();
+__declspec(dllexport) __attribute__((cdecl)) [[]] void d();
+__attribute__((cdecl)) __declspec(dllexport) [[]] void e();
+__attribute__((cdecl)) [[]] __declspec(dllexport) void f();
Index: lib/Parse/Parser.cpp
===================================================================
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -586,8 +586,7 @@
}
ParsedAttributesWithRange attrs(AttrFactory);
- MaybeParseCXX11Attributes(attrs);
- MaybeParseMicrosoftAttributes(attrs);
+ MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
Result = ParseExternalDeclaration(attrs);
return false;
@@ -1962,8 +1961,7 @@
// 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());
Index: lib/Parse/ParseExprCXX.cpp
===================================================================
--- lib/Parse/ParseExprCXX.cpp
+++ lib/Parse/ParseExprCXX.cpp
@@ -1090,14 +1090,10 @@
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))
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -212,8 +212,7 @@
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);
}
@@ -303,8 +302,7 @@
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"
@@ -354,8 +352,7 @@
// Fall through.
default:
ParsedAttributesWithRange attrs(AttrFactory);
- MaybeParseCXX11Attributes(attrs);
- MaybeParseMicrosoftAttributes(attrs);
+ MaybeParseAttributes(PAKM_CXX11 | PAKM_Microsoft, attrs);
ParseExternalDeclaration(attrs);
continue;
}
@@ -554,8 +551,7 @@
}
ParsedAttributesWithRange Attrs(AttrFactory);
- MaybeParseGNUAttributes(Attrs);
- MaybeParseCXX11Attributes(Attrs);
+ MaybeParseAttributes(PAKM_CXX11 | PAKM_GNU, Attrs);
// Maybe this is an alias-declaration.
TypeResult TypeAlias;
@@ -1230,8 +1226,7 @@
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,
@@ -1239,11 +1234,6 @@
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();
Index: lib/Parse/ParseDecl.cpp
===================================================================
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -3046,14 +3046,11 @@
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.
@@ -3731,9 +3728,7 @@
// 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;
@@ -3750,9 +3745,7 @@
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:
@@ -5775,12 +5768,9 @@
// 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.
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -2097,11 +2097,14 @@
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,
@@ -2125,20 +2128,25 @@
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,
@@ -2155,19 +2163,25 @@
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);
@@ -2174,6 +2188,48 @@
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) {
+ 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_GNU)
+ MoreToParse |= MaybeParseGNUAttributes(Attrs, End, LateAttrs);
+ if (WhichAttrKinds & PAKM_Declspec)
+ MoreToParse |= MaybeParseMicrosoftDeclSpecs(Attrs, End);
+ if (WhichAttrKinds & PAKM_CXX11)
+ MoreToParse |= MaybeParseCXX11Attributes(Attrs, End);
+ if (WhichAttrKinds & PAKM_Microsoft)
+ MoreToParse |= MaybeParseMicrosoftAttributes(Attrs, End);
+ } while (MoreToParse);
+ }
+
+ /// \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();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
