[Clang] [Parser] Fixing all callers of ParseExternalDeclaration that didn't parse gnu attributes #117148
Conversation
…t didn't parse gnu attributes
| Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot added clang 
| @llvm/pr-subscribers-clang Author: Mathys Gasnier (Mathys-Gasnier) ChangesNot all callers of Fixes: #73890 Full diff: https://github.com/llvm/llvm-project/pull/117148.diff 5 Files Affected:
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp index 010ac9c1a3e3a9..4f050d6632c356 100644 --- a/clang/lib/Parse/ParseDeclCXX.cpp +++ b/clang/lib/Parse/ParseDeclCXX.cpp @@ -267,9 +267,11 @@ void Parser::ParseInnerNamespace(const InnerNamespaceInfoList &InnerNSs, while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) { ParsedAttributes DeclAttrs(AttrFactory); - MaybeParseCXX11Attributes(DeclAttrs); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); - ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(DeclAttrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; + ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs); } // The caller is what called check -- we are simply calling @@ -468,9 +470,11 @@ Decl *Parser::ParseExportDeclaration() { if (Tok.isNot(tok::l_brace)) { // FIXME: Factor out a ParseExternalDeclarationWithAttrs. ParsedAttributes DeclAttrs(AttrFactory); - MaybeParseCXX11Attributes(DeclAttrs); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); - ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(DeclAttrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; + ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs); return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl, SourceLocation()); } @@ -481,9 +485,11 @@ Decl *Parser::ParseExportDeclaration() { while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) { ParsedAttributes DeclAttrs(AttrFactory); - MaybeParseCXX11Attributes(DeclAttrs); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); - ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(DeclAttrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; + ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs); } T.consumeClose(); diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp index bcbf4dfbabafa8..ec0572d58b3552 100644 --- a/clang/lib/Parse/ParseObjc.cpp +++ b/clang/lib/Parse/ParseObjc.cpp @@ -2287,10 +2287,12 @@ Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc, ObjCImplParsingDataRAII ObjCImplParsing(*this, ObjCImpDecl); while (!ObjCImplParsing.isFinished() && !isEofOrEom()) { ParsedAttributes DeclAttrs(AttrFactory); - MaybeParseCXX11Attributes(DeclAttrs); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(DeclAttrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; if (DeclGroupPtrTy DGP = - ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs)) { + ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs)) { DeclGroupRef DG = DGP.get(); DeclsInGroup.append(DG.begin(), DG.end()); } diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp index b91928063169ee..debfc7bd898864 100644 --- a/clang/lib/Parse/ParseOpenMP.cpp +++ b/clang/lib/Parse/ParseOpenMP.cpp @@ -2314,10 +2314,12 @@ Parser::DeclGroupPtrTy Parser::ParseOpenMPDeclarativeDirectiveWithExtDecl( // Here we expect to see some function declaration. if (AS == AS_none) { assert(TagType == DeclSpec::TST_unspecified); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); - MaybeParseCXX11Attributes(Attrs); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(Attrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; ParsingDeclSpec PDS(*this); - Ptr = ParseExternalDeclaration(Attrs, EmptyDeclSpecAttrs, &PDS); + Ptr = ParseExternalDeclaration(Attrs, DeclSpecAttrs, &PDS); } else { Ptr = ParseCXXClassMemberDeclarationWithPragmas(AS, Attrs, TagType, Tag); diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp index 36e56a92c3092e..54246510cc2453 100644 --- a/clang/lib/Parse/Parser.cpp +++ b/clang/lib/Parse/Parser.cpp @@ -2432,9 +2432,11 @@ void Parser::ParseMicrosoftIfExistsExternalDeclaration() { // FIXME: Support module import within __if_exists? while (Tok.isNot(tok::r_brace) && !isEofOrEom()) { ParsedAttributes Attrs(AttrFactory); - MaybeParseCXX11Attributes(Attrs); - ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); - DeclGroupPtrTy Result = ParseExternalDeclaration(Attrs, EmptyDeclSpecAttrs); + ParsedAttributes DeclSpecAttrs(AttrFactory); + while (MaybeParseCXX11Attributes(Attrs) || + MaybeParseGNUAttributes(DeclSpecAttrs)) + ; + DeclGroupPtrTy Result = ParseExternalDeclaration(Attrs, DeclSpecAttrs); if (Result && !getCurScope()->getParent()) Actions.getASTConsumer().HandleTopLevelDecl(Result.get()); } diff --git a/clang/test/Parser/cxx-attributes.cpp b/clang/test/Parser/cxx-attributes.cpp index 51d1f9c8228121..efc4c562653140 100644 --- a/clang/test/Parser/cxx-attributes.cpp +++ b/clang/test/Parser/cxx-attributes.cpp @@ -25,6 +25,9 @@ namespace PR17666 { typedef int __attribute__((aligned(int(1)))) T1; typedef int __attribute__((aligned(int))) T2; // expected-error {{expected '(' for function-style cast}} + + class C; + __attribute__((attr)) [[nodiscard]] C f(); // expected-warning{{unknown attribute 'attr' ignored}} } __attribute((typename)) int x; // expected-warning {{unknown attribute 'typename' ignored}} |
clang/lib/Parse/ParseDeclCXX.cpp Outdated
| while (MaybeParseCXX11Attributes(DeclAttrs) || | ||
| MaybeParseGNUAttributes(DeclSpecAttrs)) | ||
| ; |
There was a problem hiding this comment.
I’m thinking we might want to factor this out into a separate function since this exact code is now used in, like, 5 different places; maybe call it MaybeParseCXX11AndGNUAttributes or sth like that (just calling it MaybeParseAttributes would be incorrect because this doesn’t handle Microsoft attributes, so we’re probably stuck w/ the longer name unfortunately...).
There was a problem hiding this comment.
Actually, there is a TODO in ParseDeclCXX.cpp to factor out this (including the call to ParseExternalDeclaration) in Parser::ParseExportDeclaration, so now might be a good time to do that.
There was a problem hiding this comment.
If we factor out ParseExternalDeclaration in a function that also parses the attributes, factoring out attributes parsing is probably not as usefull right ?
Since it's quite a lot of changes, should I do it in two commits ? One fixing the bug and adding missing attribute parsing, and the other factoring everything into a ParseExternalDeclarationWithAttrs and replacing all uses that fit the need.
There was a problem hiding this comment.
If we factor out
ParseExternalDeclarationin a function that also parses the attributes, factoring out attributes parsing is probably not as usefull right ?
Yeah, I think just making a new function that both parses attributes and then calls ParseExternalDeclaration seems to be enough.
Since it's quite a lot of changes, should I do it in two commits ?
The commit history of a pr isn’t really that important because we always squash on merge anyway, so use however many commits you need—I’d recommend avoiding force-pushing though because it can make reviews more complicated (I at least find it harder to figure out what’s changed since my last review if there was a force-push).
There was a problem hiding this comment.
Just pushed a new commit with what we talked about.
I'm not a big fan about the comment on the function I pushed, but I cannot find a wording that please me, I'm open to suggestion.
There was a problem hiding this comment.
Hmm, maybe just ‘Parse CXX11 and GNU attributes followed by a declaration’.
There was a problem hiding this comment.
Yes, but it could be understood the wrong way around I think...
Let's wait on feedback from others, maybe someone will have an eureka moment.
| assert(TagType == DeclSpec::TST_unspecified); | ||
| ParsedAttributes EmptyDeclSpecAttrs(AttrFactory); | ||
| MaybeParseCXX11Attributes(Attrs); | ||
| ParsedAttributes DeclSpecAttrs(AttrFactory); |
There was a problem hiding this comment.
Why isn't this calling the new function? It seems you've added the ParsingDeclSpec default argument correctly, so I don't see a reason to not have this call the new one too.
There was a problem hiding this comment.
Because Attrs is not a local variable but an argument passed to ParseOpenMPDeclarativeDirectiveWithExtDecl
There was a problem hiding this comment.
Ah! I missed that, thanks! @alexey-bataev : Are we sure this is intentional? Basically, we perhaps should be parsing these attributes 'together', and it isn't clear to me looking at this that this doesn't 'fix' a bug/should have some attention.
There was a problem hiding this comment.
It is hard to tell now. Attrs comes from Parser::ParseExternalDeclaration. I don't remember the details already, but looks like we saw some missed attrs, if skipped this argument
5 participants
Not all callers of
ParseExternalDeclarationwere parsing gnu attributes, this caused issues with namespaces declarations attributes needing to be in a specific order (See #73890).This PR fixes this in namespaces, exports, and three other places. It also adds a test to ensure a non-regression of this behavior.
Fixes: #73890