parse: process GNU and standard attributes on top-level decls

[compnerd]

Summary

We would previously reject valid input where GNU attributes preceded the
standard attributes on top-level declarations. A previous attribute
handling change had begun rejecting this whilst GCC does honour this
layout. In practice, this breaks use of extern "C" attributed
functions which use both standard and GNU attributes as experienced by
the Swift runtime.

Objective-C deserves an honourable mention for requiring some additional
special casing. Because attributes on declarations and definitions
differ in semantics, we need to replicate some of the logic for
detecting attributes to declarations to which they appertain cannot be
attributed. This should match the existing case for the application of
GNU attributes to interfaces, protocols, and implementations.

Take the opportunity to split out the tooling tests into two cases: ones
which process macros and ones which do not.

Special thanks to Aaron Ballman for the many hints and extensive rubber
ducking that was involved in identifying the various places where we
accidentally dropped attributes.

Fixes: #58229

[ymandel]

clang/unittests/Tooling/SourceCodeTest.cpp

249–271

Thanks for trying to fix these! The changed test cases seem to be doing two things at once: macros and attributes, and I don't remember why. We should test the behavior separately. So, I think you're new test cases are good regardless. But, we still want the old tests to test the behavior for attributes that are hidden behind a macro, since this is used not infrequently in my exprience.

IIUC, the current code isn't properly handling attributes that appear inside macros, which is why this fix breaks the code. Specifically, it is not considering the case that the location in Attr->getLocation() is in a macro, while Range.getBegin() is in the original file, and hence the locations are not comparable. I'm surprised this ever worked, tbh.

My preference would be to update the code, if you know how, so that both the old and new tests pass. But, I realize this may be a lot to ask. So, at the least, please comment out, but keep, the old tests; or, copy the old tests to a new test and mark it disabled. Either way, prefix with a FIXME indicating the issue.

WDYT?

[aaron.ballman]

Adding some additional reviewers for more awareness.

[compnerd]

clang/unittests/Tooling/SourceCodeTest.cpp

249–271

Unfortunately, I couldn't figure out how to track through the expansion so I am going to take you up on the commented out case option.

[compnerd]

Add unsupported test cases

[aaron.ballman]

I added the wrong account for Elizabeth, so fixing that up.

[ymandel]

clang/unittests/Tooling/SourceCodeTest.cpp

259–268

Added tests, here and below, look good to me. Thanks!

[arphaman]

clang/lib/Parse/ParseObjc.cpp

70

I think it might be better to still report this error at the location of the @ token (Tok.getLocation()). This will let us report the error at the actual Objective-C keyword which the user should note should not be used with the attribute.

[arphaman]

Thanks, this LGTM. You might want to get another reviewers approval as well.

[aaron.ballman]

Thank you for working on this, I know it was a slog! I think this is pretty close to ready, though you should definitely add a release note about the changes.

clang/include/clang/Parse/Parser.h

1605–1606

Should we rename Attrs to DeclaratorAttrs or something along those lines, so it's easier for someone to distinguish which attributes go where? (Same suggestion applies throughout the review.)

clang/lib/Parse/ParseObjc.cpp

68–70

llvm::for_each?

clang/lib/Parse/ParseOpenMP.cpp

1997

We can sink this down to the only place it's used in the function, right?

clang/lib/Parse/Parser.cpp

733–734

We probably could use some comments here explaining why we parse some into one container and others into another container.

1100–1101

How sure are you that this isn't overwriting existing range data that's potentially relevant? e.g., where declaration specifiers and attributes are mixed together such that the attribute range doesn't cover all of the declaration specifiers?

clang/test/Parser/attr-order.cpp

20

This makes me happy!!

[compnerd]

clang/include/clang/Parse/Parser.h

1605–1606

Yes, DeclAttrs would be much nicer, but I wanted to get everything right before doing that and then forgot. Thanks for the reminder.

clang/lib/Parse/Parser.cpp

1100–1101

I don't believe it can - the range hasn't been setup yet when coming into this function, only the storage has been allocated. This is the first place where we are initializing the source range. If there are other unstated invariants, it would be nice to have some sort of assertion for them.

[compnerd]

Address review feedback

[aaron.ballman]

clang/lib/Parse/Parser.cpp

1100–1101

Ah, so we could perhaps assert that the range is invalid on entry and leave a comment explaining why we're asserting/what's going on?

[aaron.ballman]

LGTM aside from the question about whether we want to add an assert or not (I don't insist on adding one) and adding a release note (which doesn't need further review unless you want it). Thank you for these changes!

clang/docs/ReleaseNotes.rst

 - Fix a number of recursively-instantiated constraint issues, which would possibly
   result in a stack overflow.
   `Issue 44304 <https://github.com/llvm/llvm-project/issues/44304>`_
   `Issue 50891 <https://github.com/llvm/llvm-project/issues/50891>`_
 - Clang 14 predeclared some builtin POSIX library functions in ``gnu2x`` mode,
   and Clang 15 accidentally stopped predeclaring those functions in that
   language mode. Clang 16 now predeclares those functions again. This fixes
   `Issue 56607 <https://github.com/llvm/llvm-project/issues/56607>`_.
+- GNU attributes being applied prior to standard attributes would be handled
+  improperly, which was corrected to match the behaviour exhibited by GCC.
+  `Issue 58229 <https://github.com/llvm/llvm-project/issues/58229>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/include/clang/Parse/Parser.h

private:
   }
   bool ConsumeAndStoreUntil(tok::TokenKind T1, tok::TokenKind T2,
                             CachedTokens &Toks,
                             bool StopAtSemi = true,
                             bool ConsumeFinalToken = true);
 
   //===--------------------------------------------------------------------===//
   // C99 6.9: External Definitions.
-  DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &Attrs,
+  DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &DeclAttrs,
+                                          ParsedAttributes &DeclSpecAttrs,
                                           ParsingDeclSpec *DS = nullptr);
   bool isDeclarationAfterDeclarator();
   bool isStartOfFunctionDefinition(const ParsingDeclarator &Declarator);
-  DeclGroupPtrTy
-  ParseDeclarationOrFunctionDefinition(ParsedAttributes &Attrs,
-                                       ParsingDeclSpec *DS = nullptr,
-                                       AccessSpecifier AS = AS_none);
+  DeclGroupPtrTy ParseDeclarationOrFunctionDefinition(
+      ParsedAttributes &DeclAttrs, ParsedAttributes &DeclSpecAttrs,
+      ParsingDeclSpec *DS = nullptr, AccessSpecifier AS = AS_none);
   DeclGroupPtrTy ParseDeclOrFunctionDefInternal(ParsedAttributes &Attrs,
+                                                ParsedAttributes &DeclSpecAttrs,
                                                 ParsingDeclSpec &DS,
                                                 AccessSpecifier AS);
 
   void SkipFunctionBody();
   Decl *ParseFunctionDefinition(ParsingDeclarator &D,
                  const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo(),
                  LateParsedAttrList *LateParsedAttrs = nullptr);
   void ParseKNRParamDeclarations(Declarator &D);
   // EndLoc is filled with the location of the last token of the simple-asm.
   ExprResult ParseSimpleAsm(bool ForAsmLabel, SourceLocation *EndLoc);
   ExprResult ParseAsmStringLiteral(bool ForAsmLabel);
 
   // Objective-C External Declarations
   void MaybeSkipAttributes(tok::ObjCKeywordKind Kind);
-  DeclGroupPtrTy ParseObjCAtDirectives(ParsedAttributes &Attrs);
+  DeclGroupPtrTy ParseObjCAtDirectives(ParsedAttributes &DeclAttrs,
+                                       ParsedAttributes &DeclSpecAttrs);
   DeclGroupPtrTy ParseObjCAtClassDeclaration(SourceLocation atLoc);
   Decl *ParseObjCAtInterfaceDeclaration(SourceLocation AtLoc,
                                         ParsedAttributes &prefixAttrs);
   class ObjCTypeParamListScope;
   ObjCTypeParamList *parseObjCTypeParamList();
   ObjCTypeParamList *parseObjCTypeParamListOrProtocolRefs(
       ObjCTypeParamListScope &Scope, SourceLocation &lAngleLoc,
       SmallVectorImpl<IdentifierLocPair> &protocolIdents,

clang/lib/Parse/ParseDeclCXX.cpp

 /// ParseInnerNamespace - Parse the contents of a namespace.
 void Parser::ParseInnerNamespace(const InnerNamespaceInfoList &InnerNSs,
                                  unsigned int index, SourceLocation &InlineLoc,
                                  ParsedAttributes &attrs,
                                  BalancedDelimiterTracker &Tracker) {
   if (index == InnerNSs.size()) {
     while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
            Tok.isNot(tok::eof)) {
-      ParsedAttributes Attrs(AttrFactory);
-      MaybeParseCXX11Attributes(Attrs);
-      ParseExternalDeclaration(Attrs);
+      ParsedAttributes DeclAttrs(AttrFactory);
+      MaybeParseCXX11Attributes(DeclAttrs);
+      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+      ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
     }
 
     // The caller is what called check -- we are simply calling
     // the close for it.
     Tracker.consumeClose();
 
     return;
   }
Decl *LinkageSpec =
       Lang.isInvalid()
           ? nullptr
           : Actions.ActOnStartLinkageSpecification(
                 getCurScope(), DS.getSourceRange().getBegin(), Lang.get(),
                 Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   ParsedAttributes DeclAttrs(AttrFactory);
   MaybeParseCXX11Attributes(DeclAttrs);
+  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
 
   if (Tok.isNot(tok::l_brace)) {
     // Reset the source range in DS, as the leading "extern"
     // does not really belong to the inner declaration ...
     DS.SetRangeStart(SourceLocation());
     DS.SetRangeEnd(SourceLocation());
     // ... but anyway remember that such an "extern" was seen.
     DS.setExternInLinkageSpec(true);
-    ParseExternalDeclaration(DeclAttrs, &DS);
+    ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs, &DS);
     return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
                              getCurScope(), LinkageSpec, SourceLocation())
                        : nullptr;
   }
 
   DS.abort();
 
   ProhibitAttributes(DeclAttrs);
while (true) {
     case tok::eof:
       break;
 
     case tok::r_brace:
       if (!NestedModules)
         break;
       [[fallthrough]];
     default:
-      ParsedAttributes Attrs(AttrFactory);
-      MaybeParseCXX11Attributes(Attrs);
-      ParseExternalDeclaration(Attrs);
+      ParsedAttributes DeclAttrs(AttrFactory);
+      MaybeParseCXX11Attributes(DeclAttrs);
+      ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
       continue;
     }
 
     break;
   }
 
   T.consumeClose();
   return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
Decl *Parser::ParseExportDeclaration() {
 
   ParseScope ExportScope(this, Scope::DeclScope);
   Decl *ExportDecl = Actions.ActOnStartExportDecl(
       getCurScope(), ExportLoc,
       Tok.is(tok::l_brace) ? Tok.getLocation() : SourceLocation());
 
   if (Tok.isNot(tok::l_brace)) {
     // FIXME: Factor out a ParseExternalDeclarationWithAttrs.
-    ParsedAttributes Attrs(AttrFactory);
-    MaybeParseCXX11Attributes(Attrs);
-    ParseExternalDeclaration(Attrs);
+    ParsedAttributes DeclAttrs(AttrFactory);
+    MaybeParseCXX11Attributes(DeclAttrs);
+    ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+    ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
     return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl,
                                          SourceLocation());
   }
 
   BalancedDelimiterTracker T(*this, tok::l_brace);
   T.consumeOpen();
 
   // The Modules TS draft says "An export-declaration shall declare at least one
   // entity", but the intent is that it shall contain at least one declaration.
   if (Tok.is(tok::r_brace) && getLangOpts().ModulesTS) {
     Diag(ExportLoc, diag::err_export_empty)
         << SourceRange(ExportLoc, Tok.getLocation());
   }
 
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
          Tok.isNot(tok::eof)) {
-    ParsedAttributes Attrs(AttrFactory);
-    MaybeParseCXX11Attributes(Attrs);
-    ParseExternalDeclaration(Attrs);
+    ParsedAttributes DeclAttrs(AttrFactory);
+    MaybeParseCXX11Attributes(DeclAttrs);
+    ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+    ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
   }
 
   T.consumeClose();
   return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl,
                                        T.getCloseLocation());
 }
 
 /// ParseUsingDirectiveOrDeclaration - Parse C++ using using-declaration or

clang/lib/Parse/ParseHLSL.cpp

Decl *Parser::ParseHLSLBuffer(SourceLocation &DeclEnd) {
   }
 
   Decl *D = Actions.ActOnStartHLSLBuffer(getCurScope(), IsCBuffer, BufferLoc,
                                          Identifier, IdentifierLoc,
                                          T.getOpenLocation());
 
   while (Tok.isNot(tok::r_brace) && Tok.isNot(tok::eof)) {
     // FIXME: support attribute on constants inside cbuffer/tbuffer.
-    ParsedAttributes Attrs(AttrFactory);
+    ParsedAttributes DeclAttrs(AttrFactory);
+    ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
 
-    DeclGroupPtrTy Result = ParseExternalDeclaration(Attrs);
+    DeclGroupPtrTy Result =
+        ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
     if (!validateDeclsInsideHLSLBuffer(Result, IdentifierLoc, IsCBuffer,
                                        *this)) {
       T.skipToEnd();
       DeclEnd = T.getCloseLocation();
       BufferScope.Exit();
       Actions.ActOnFinishHLSLBuffer(D, DeclEnd);
       return nullptr;
     }

clang/lib/Parse/ParseObjc.cpp

 /// ParseObjCAtDirectives - Handle parts of the external-declaration production:
 ///       external-declaration: [C99 6.9]
 /// [OBJC]  objc-class-definition
 /// [OBJC]  objc-class-declaration
 /// [OBJC]  objc-alias-declaration
 /// [OBJC]  objc-protocol-definition
 /// [OBJC]  objc-method-definition
 /// [OBJC]  '@' 'end'
-Parser::DeclGroupPtrTy Parser::ParseObjCAtDirectives(ParsedAttributes &Attrs) {
+Parser::DeclGroupPtrTy
+Parser::ParseObjCAtDirectives(ParsedAttributes &DeclAttrs,
+                              ParsedAttributes &DeclSpecAttrs) {
+  DeclAttrs.takeAllFrom(DeclSpecAttrs);
+
   SourceLocation AtLoc = ConsumeToken(); // the "@"
 
   if (Tok.is(tok::code_completion)) {
     cutOffParsing();
     Actions.CodeCompleteObjCAtDirective(getCurScope());
     return nullptr;
   }
 
+  switch (Tok.getObjCKeywordID()) {
+  case tok::objc_interface:
+  case tok::objc_protocol:
+  case tok::objc_implementation:
+    break;
+  default:
+    llvm::for_each(DeclAttrs, [this](const auto &Attr) {
+      if (Attr.isGNUAttribute())
+        Diag(Tok.getLocation(), diag::err_objc_unexpected_attr);
+    });
+  }
+
   Decl *SingleDecl = nullptr;
   switch (Tok.getObjCKeywordID()) {
   case tok::objc_class:
     return ParseObjCAtClassDeclaration(AtLoc);
   case tok::objc_interface:
-    SingleDecl = ParseObjCAtInterfaceDeclaration(AtLoc, Attrs);
+    SingleDecl = ParseObjCAtInterfaceDeclaration(AtLoc, DeclAttrs);
     break;
   case tok::objc_protocol:
-    return ParseObjCAtProtocolDeclaration(AtLoc, Attrs);
+    return ParseObjCAtProtocolDeclaration(AtLoc, DeclAttrs);
   case tok::objc_implementation:
-    return ParseObjCAtImplementationDeclaration(AtLoc, Attrs);
+    return ParseObjCAtImplementationDeclaration(AtLoc, DeclAttrs);
   case tok::objc_end:
     return ParseObjCAtEndDeclaration(AtLoc);
   case tok::objc_compatibility_alias:
     SingleDecl = ParseObjCAtAliasDeclaration(AtLoc);
     break;
   case tok::objc_synthesize:
     SingleDecl = ParseObjCPropertySynthesize(AtLoc);
     break;
while (true) {
     // If we don't have an @ directive, parse it as a function definition.
     if (Tok.isNot(tok::at)) {
       // The code below does not consume '}'s because it is afraid of eating the
       // end of a namespace.  Because of the way this code is structured, an
       // erroneous r_brace would cause an infinite loop if not handled here.
       if (Tok.is(tok::r_brace))
         break;
 
-      ParsedAttributes EmptyAttrs(AttrFactory);
+      ParsedAttributes EmptyDeclAttrs(AttrFactory);
+      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
 
       // Since we call ParseDeclarationOrFunctionDefinition() instead of
       // ParseExternalDeclaration() below (so that this doesn't parse nested
       // @interfaces), this needs to duplicate some code from the latter.
       if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert)) {
         SourceLocation DeclEnd;
         ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
-        allTUVariables.push_back(ParseDeclaration(
-            DeclaratorContext::File, DeclEnd, EmptyAttrs, EmptyDeclSpecAttrs));
+        allTUVariables.push_back(ParseDeclaration(DeclaratorContext::File,
+                                                  DeclEnd, EmptyDeclAttrs,
+                                                  EmptyDeclSpecAttrs));
         continue;
       }
 
-      allTUVariables.push_back(
-          ParseDeclarationOrFunctionDefinition(EmptyAttrs));
+      allTUVariables.push_back(ParseDeclarationOrFunctionDefinition(
+          EmptyDeclAttrs, EmptyDeclSpecAttrs));
       continue;
     }
 
     // Otherwise, we have an @ directive, eat the @.
     SourceLocation AtLoc = ConsumeToken(); // the "@"
     if (Tok.is(tok::code_completion)) {
       cutOffParsing();
       Actions.CodeCompleteObjCAtDirective(getCurScope());
Parser::ParseObjCAtImplementationDeclaration(SourceLocation AtLoc,
   }
   assert(ObjCImpDecl);
 
   SmallVector<Decl *, 8> DeclsInGroup;
 
   {
     ObjCImplParsingDataRAII ObjCImplParsing(*this, ObjCImpDecl);
     while (!ObjCImplParsing.isFinished() && !isEofOrEom()) {
-      ParsedAttributes attrs(AttrFactory);
-      MaybeParseCXX11Attributes(attrs);
-      if (DeclGroupPtrTy DGP = ParseExternalDeclaration(attrs)) {
+      ParsedAttributes DeclAttrs(AttrFactory);
+      MaybeParseCXX11Attributes(DeclAttrs);
+      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+      if (DeclGroupPtrTy DGP =
+              ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs)) {
         DeclGroupRef DG = DGP.get();
         DeclsInGroup.append(DG.begin(), DG.end());
       }
     }
   }
 
   return Actions.ActOnFinishObjCImplementation(ObjCImpDecl, DeclsInGroup);
 }

clang/lib/Parse/ParseOpenMP.cpp

void Parser::parseOMPEndDirective(OpenMPDirectiveKind BeginKind,
   if (SkipUntilOpenMPEnd)
     SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
 }
 
 void Parser::ParseOMPEndDeclareTargetDirective(OpenMPDirectiveKind BeginDKind,
                                                OpenMPDirectiveKind EndDKind,
                                                SourceLocation DKLoc) {
   parseOMPEndDirective(BeginDKind, OMPD_end_declare_target, EndDKind, DKLoc,
                        Tok.getLocation(),
                        /* SkipUntilOpenMPEnd */ false);
   // Skip the last annot_pragma_openmp_end.
   if (Tok.is(tok::annot_pragma_openmp_end))
     ConsumeAnnotationToken();
 }
 
 /// Parsing of declarative OpenMP directives.
 ///
case OMPD_declare_simd: {
     DeclGroupPtrTy Ptr;
     if (Tok.isOneOf(tok::annot_pragma_openmp, tok::annot_attr_openmp)) {
       Ptr = ParseOpenMPDeclarativeDirectiveWithExtDecl(AS, Attrs, Delayed,
                                                        TagType, Tag);
     } else if (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
       // Here we expect to see some function declaration.
       if (AS == AS_none) {
         assert(TagType == DeclSpec::TST_unspecified);
+        ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
         MaybeParseCXX11Attributes(Attrs);
         ParsingDeclSpec PDS(*this);
-        Ptr = ParseExternalDeclaration(Attrs, &PDS);
+        Ptr = ParseExternalDeclaration(Attrs, EmptyDeclSpecAttrs, &PDS);
       } else {
         Ptr =
             ParseCXXClassMemberDeclarationWithPragmas(AS, Attrs, TagType, Tag);
       }
     }
     if (!Ptr) {
       Diag(Loc, diag::err_omp_decl_in_declare_simd_variant)
           << (DKind == OMPD_declare_simd ? 0 : 1);

clang/lib/Parse/Parser.cpp

if ((Tok.getIdentifierInfo() == Ident_module ||
       else
         goto import_decl;
     }
     break;
 
   default:
     break;
   }
 
-  ParsedAttributes attrs(AttrFactory);
-  MaybeParseCXX11Attributes(attrs);
+  ParsedAttributes DeclAttrs(AttrFactory);
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  // GNU attributes are applied to the declaration specification while the
+  // standard attributes are applied to the declaration.  We parse the two
+  // attribute sets into different containters so we can apply them during
+  // the regular parsing process.
+  while (MaybeParseCXX11Attributes(DeclAttrs) ||
+         MaybeParseGNUAttributes(DeclSpecAttrs))
+    ;
 
-  Result = ParseExternalDeclaration(attrs);
+  Result = ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs);
   // An empty Result might mean a line with ';' or some parsing error, ignore
   // it.
   if (Result) {
     if (ImportState == Sema::ModuleImportState::FirstDecl)
       // First decl was not modular.
       ImportState = Sema::ModuleImportState::NotACXX20Module;
     else if (ImportState == Sema::ModuleImportState::ImportAllowed)
       // Non-imports disallow further imports.

 ///
 /// [C++11] empty-declaration:
 ///           ';'
 ///
 /// [C++0x/GNU] 'extern' 'template' declaration
 ///
 /// [Modules-TS] module-import-declaration
 ///
-Parser::DeclGroupPtrTy Parser::ParseExternalDeclaration(ParsedAttributes &Attrs,
-                                                        ParsingDeclSpec *DS) {
+Parser::DeclGroupPtrTy
+Parser::ParseExternalDeclaration(ParsedAttributes &Attrs,
+                                 ParsedAttributes &DeclSpecAttrs,
   DestroyTemplateIdAnnotationsRAIIObj CleanupRAII(*this);
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
   if (PP.isCodeCompletionReached()) {
     cutOffParsing();
     return nullptr;
   }
 
case tok::r_brace:
     return nullptr;
   case tok::eof:
     Diag(Tok, diag::err_expected_external_declaration);
     return nullptr;
   case tok::kw___extension__: {
     // __extension__ silences extension warnings in the subexpression.
     ExtensionRAIIObject O(Diags);  // Use RAII to do this.
     ConsumeToken();
-    return ParseExternalDeclaration(Attrs);
+    return ParseExternalDeclaration(Attrs, DeclSpecAttrs);
   }
   case tok::kw_asm: {
     ProhibitAttributes(Attrs);
 
     SourceLocation StartLoc = Tok.getLocation();
     SourceLocation EndLoc;
 
     ExprResult Result(ParseSimpleAsm(/*ForAsmLabel*/ false, &EndLoc));
ExpectAndConsume(tok::semi, diag::err_expected_after,
                      "top-level asm block");
 
     if (Result.isInvalid())
       return nullptr;
     SingleDecl = Actions.ActOnFileScopeAsmDecl(Result.get(), StartLoc, EndLoc);
     break;
   }
   case tok::at:
-    return ParseObjCAtDirectives(Attrs);
+    return ParseObjCAtDirectives(Attrs, DeclSpecAttrs);
   case tok::minus:
   case tok::plus:
     if (!getLangOpts().ObjC) {
       Diag(Tok, diag::err_expected_external_declaration);
       ConsumeToken();
       return nullptr;
     }
     SingleDecl = ParseObjCMethodDefinition();
Parser::ParseExternalDeclaration(ParsedAttributes &Attrs,
   case tok::kw_namespace:
   case tok::kw_typedef:
   case tok::kw_template:
   case tok::kw_static_assert:
   case tok::kw__Static_assert:
     // A function definition cannot start with any of these keywords.
     {
       SourceLocation DeclEnd;
-      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
       return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
-                              EmptyDeclSpecAttrs);
+                              DeclSpecAttrs);
     }
 
   case tok::kw_cbuffer:
   case tok::kw_tbuffer:
     if (getLangOpts().HLSL) {
       SourceLocation DeclEnd;
-      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
       return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
-                              EmptyDeclSpecAttrs);
+                              DeclSpecAttrs);
     }
     goto dont_know;
 
   case tok::kw_static:
     // Parse (then ignore) 'static' prior to a template instantiation. This is
     // a GCC extension that we intentionally do not support.
     if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_template)) {
       Diag(ConsumeToken(), diag::warn_static_inline_explicit_inst_ignored)
         << 0;
       SourceLocation DeclEnd;
-      ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
       return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
-                              EmptyDeclSpecAttrs);
+                              DeclSpecAttrs);
     }
     goto dont_know;
 
   case tok::kw_inline:
     if (getLangOpts().CPlusPlus) {
       tok::TokenKind NextKind = NextToken().getKind();
 
       // Inline namespaces. Allowed as an extension even in C++03.
       if (NextKind == tok::kw_namespace) {
         SourceLocation DeclEnd;
-        ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
         return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
-                                EmptyDeclSpecAttrs);
+                                DeclSpecAttrs);
       }
 
       // Parse (then ignore) 'inline' prior to a template instantiation. This is
       // a GCC extension that we intentionally do not support.
       if (NextKind == tok::kw_template) {
         Diag(ConsumeToken(), diag::warn_static_inline_explicit_inst_ignored)
           << 1;
         SourceLocation DeclEnd;
-        ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
         return ParseDeclaration(DeclaratorContext::File, DeclEnd, Attrs,
-                                EmptyDeclSpecAttrs);
+                                DeclSpecAttrs);
       }
     }
     goto dont_know;
 
   case tok::kw_extern:
     if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_template)) {
       // Extern templates
       SourceLocation ExternLoc = ConsumeToken();
Parser::ParseExternalDeclaration(ParsedAttributes &Attrs,
 
   default:
   dont_know:
     if (Tok.isEditorPlaceholder()) {
       ConsumeToken();
       return nullptr;
     }
     // We can't tell whether this is a function-definition or declaration yet.
-    return ParseDeclarationOrFunctionDefinition(Attrs, DS);
+    return ParseDeclarationOrFunctionDefinition(Attrs, DeclSpecAttrs, DS);
   }
 
   // This routine returns a DeclGroup, if the thing we parsed only contains a
   // single decl, convert it now.
   return Actions.ConvertDeclToDeclGroup(SingleDecl);
 }
 
 /// Determine whether the current token, if it occurs after a

 ///
 ///       declaration: [C99 6.7]
 ///         declaration-specifiers init-declarator-list[opt] ';'
 /// [!C99]  init-declarator-list ';'                   [TODO: warn in c99 mode]
 /// [OMP]   threadprivate-directive
 /// [OMP]   allocate-directive                         [TODO]
 ///
 Parser::DeclGroupPtrTy Parser::ParseDeclOrFunctionDefInternal(
-    ParsedAttributes &Attrs, ParsingDeclSpec &DS, AccessSpecifier AS) {
+    ParsedAttributes &Attrs, ParsedAttributes &DeclSpecAttrs,
+    ParsingDeclSpec &DS, AccessSpecifier AS) {
+  // Because we assume that the DeclSpec has not yet been initialised, we simply
+  // overwrite the source range and attribute the provided leading declspec
+  // attributes.
+  assert(DS.getSourceRange().isInvalid() &&
+         "expected uninitialised source range");
+  DS.SetRangeStart(DeclSpecAttrs.Range.getBegin());
+  DS.SetRangeEnd(DeclSpecAttrs.Range.getEnd());
+  DS.takeAttributesFrom(DeclSpecAttrs);
+
   MaybeParseMicrosoftAttributes(DS.getAttributes());
   // Parse the common declaration-specifiers piece.
   ParseDeclarationSpecifiers(DS, ParsedTemplateInfo(), AS,
                              DeclSpecContext::DSC_top_level);
 
   // If we had a free-standing type definition with a missing semicolon, we
   // may get this far before the problem becomes obvious.
   if (DS.hasTagDefinition() && DiagnoseMissingSemiAfterTagDefinition(
if (getLangOpts().CPlusPlus && isTokenStringLiteral() &&
     Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File);
     return Actions.ConvertDeclToDeclGroup(TheDecl);
   }
 
   return ParseDeclGroup(DS, DeclaratorContext::File, Attrs);
 }
 
 Parser::DeclGroupPtrTy Parser::ParseDeclarationOrFunctionDefinition(
-    ParsedAttributes &Attrs, ParsingDeclSpec *DS, AccessSpecifier AS) {
+    ParsedAttributes &Attrs, ParsedAttributes &DeclSpecAttrs,
+    ParsingDeclSpec *DS, AccessSpecifier AS) {
   if (DS) {
-    return ParseDeclOrFunctionDefInternal(Attrs, *DS, AS);
+    return ParseDeclOrFunctionDefInternal(Attrs, DeclSpecAttrs, *DS, AS);
   } else {
     ParsingDeclSpec PDS(*this);
     // Must temporarily exit the objective-c container scope for
     // parsing c constructs and re-enter objc container scope
     // afterwards.
     ObjCDeclContextSwitch ObjCDC(*this);
 
-    return ParseDeclOrFunctionDefInternal(Attrs, PDS, AS);
+    return ParseDeclOrFunctionDefInternal(Attrs, DeclSpecAttrs, PDS, AS);
   }
 }
 
 /// ParseFunctionDefinition - We parsed and verified that the specified
 /// Declarator is well formed.  If this is a K&R-style function, read the
 /// parameters declaration-list, then start the compound-statement.
 ///
 ///       function-definition: [C99 6.9.1]
case IEB_Skip:
     return;
   }
 
   // Parse the declarations.
   // FIXME: Support module import within __if_exists?
   while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     ParsedAttributes Attrs(AttrFactory);
     MaybeParseCXX11Attributes(Attrs);
-    DeclGroupPtrTy Result = ParseExternalDeclaration(Attrs);
+    ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+    DeclGroupPtrTy Result = ParseExternalDeclaration(Attrs, EmptyDeclSpecAttrs);
     if (Result && !getCurScope()->getParent())
       Actions.getASTConsumer().HandleTopLevelDecl(Result.get());
   }
   Braces.consumeClose();
 }
 
 /// Parse a declaration beginning with the 'module' keyword or C++20
 /// context-sensitive keyword (optionally preceded by 'export').

clang/test/Parser/attr-order.cpp

 [[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void a(); // ok
 [[noreturn]] __declspec(dllexport) __attribute__((cdecl)) void b(); // ok
 [[]] [[noreturn]] __attribute__((cdecl)) __declspec(dllexport) void c(); // ok
 
 // [[]] attributes before a declaration must be at the start of the line.
 __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}}
+__attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void g();
 
 [[noreturn]] __attribute__((cdecl))
-[[]] // expected-error {{an attribute list cannot appear here}}
+[[]]
 __declspec(dllexport) void h();

clang/test/Parser/cxx-attributes.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
+// GH#58229 - rejects-valid
+__attribute__((__visibility__("default"))) [[nodiscard]] int f();
+[[nodiscard]] __attribute__((__visibility__("default"))) int f();
+
 class c {
   virtual void f1(const char* a, ...)
     __attribute__ (( __format__(__printf__,2,3) )) = 0;
   virtual void f2(const char* a, ...)
     __attribute__ (( __format__(__printf__,2,3) )) {}
 };
 
 template <typename T> class X {

clang/test/SemaCXX/attr-unavailable.cpp

void unavail(short* sp) {
   foo(sp);
   foo();
 }
 
 // Show that delayed processing of 'unavailable' is the same
 // delayed process for 'deprecated'.
 // <rdar://problem/12241361> and <rdar://problem/15584219>
 enum DeprecatedEnum { DE_A, DE_B } __attribute__((deprecated)); // expected-note {{'DeprecatedEnum' has been explicitly marked deprecated here}}
-__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 typedef enum DeprecatedEnum AnotherDeprecatedEnum; // expected-warning {{'DeprecatedEnum' is deprecated}}
 
+__attribute__((deprecated)) typedef enum DeprecatedEnum DeprecatedEnum;
 __attribute__((deprecated))
 DeprecatedEnum testDeprecated(DeprecatedEnum X) { return X; }
 
 
 enum UnavailableEnum { UE_A, UE_B } __attribute__((unavailable)); // expected-note {{'UnavailableEnum' has been explicitly marked unavailable here}}
-__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 typedef enum UnavailableEnum AnotherUnavailableEnum; // expected-error {{'UnavailableEnum' is unavailable}}
+                                                     //
 
-
+__attribute__((unavailable)) typedef enum UnavailableEnum UnavailableEnum;
 __attribute__((unavailable))
 UnavailableEnum testUnavailable(UnavailableEnum X) { return X; }
 
 
 // Check that unavailable classes can be used as arguments to unavailable
 // function, particularly in template functions.
 #if !__has_feature(attribute_availability_in_templates)
 #error "Missing __has_feature"

clang/unittests/Tooling/SourceCodeTest.cpp

TEST(SourceCodeTest, getAssociatedRange) {
   Visitor.runOverAnnotated("$r[[int x = 4; // Comment\n]]");
   Visitor.runOverAnnotated("$r[[int x = 4; /* Comment */\n]]");
 
   // Does *not* include trailing comments when another entity appears between
   // the decl and the comment.
   Visitor.runOverAnnotated("$r[[int x = 4;]] class C {}; // Comment\n");
 
   // Includes attributes.
   Visitor.runOverAnnotated(R"cpp(
-      #define ATTR __attribute__((deprecated("message")))
-      $r[[ATTR
+      $r[[__attribute__((deprecated("message")))
       int x;]])cpp");
 
   // Includes attributes and comments together.
   Visitor.runOverAnnotated(R"cpp(
-      #define ATTR __attribute__((deprecated("message")))
-      $r[[ATTR
+      $r[[__attribute__((deprecated("message")))
+      // Comment.
+      int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+      #define MACRO_EXPANSION __attribute__((deprecated("message")))
+      $r[[MACRO_EXPANSION
+      int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+      #define MACRO_EXPANSION __attribute__((deprecated("message")))
+      $r[[MACRO_EXPANSION
       // Comment.
       int x;]])cpp");
 }
 
 TEST(SourceCodeTest, getAssociatedRangeClasses) {
   struct RecordDeclsVisitor : AnnotatedCodeVisitor<RecordDeclsVisitor> {
     bool VisitRecordDecl(RecordDecl *Decl) { return VisitDeclHelper(Decl); }
   };
   RecordDeclsVisitor Visitor;
 
   Visitor.runOverAnnotated("$r[[class A;]]");
Visit(R"cpp(
       ]]class foo {};)cpp");
 
   // Does not include IfThisThenThat comments
   Visit("// LINT.IfChange.\n$r[[int x = 4;]]");
   Visit("// LINT.ThenChange.\n$r[[int x = 4;]]");
 
   // Includes attributes.
   Visit(R"cpp(
-      #define ATTR __attribute__((deprecated("message")))
-      $r[[ATTR
+      $r[[__attribute__((deprecated("message")))
       int x;]])cpp");
 
   // Includes attributes and comments together.
   Visit(R"cpp(
-      #define ATTR __attribute__((deprecated("message")))
-      $r[[ATTR
+      $r[[__attribute__((deprecated("message")))
+      // Comment.
+      int x;]])cpp");
+
+  // Includes attributes through macro expansion.
+  Visitor.runOverAnnotated(R"cpp(
+      #define MACRO_EXPANSION __attribute__((deprecated("message")))
+      $r[[MACRO_EXPANSION
+      int x;]])cpp");
+
+  // Includes attributes through macro expansion with comments.
+  Visitor.runOverAnnotated(R"cpp(
+      #define MACRO_EXPANSION __attribute__((deprecated("message")))
+      $r[[MACRO_EXPANSION
       // Comment.
       int x;]])cpp");
 }
 
 TEST(SourceCodeTest, getAssociatedRangeInvalidForPartialExpansions) {
   struct FailingVarDeclsVisitor : TestVisitor<FailingVarDeclsVisitor> {
     FailingVarDeclsVisitor() {}
     bool VisitVarDecl(VarDecl *Decl) {