[Clang] Support multiple attributes in a single pragma

[egorzhdan]

Summary

This adds support for multiple attributes in #pragma clang attribute push, for example:

#pragma clang attribute push (__attribute__((disable_sanitizer_instrumentation)) __attribute__((section("S"))), apply_to=variable(is_global))

or

#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)

Related attributes can now be applied with a single pragma, which makes it harder for developers to make an accidental error later when editing the code.

rdar://78269653

[egorzhdan]

Remove unused include

[aaron.ballman]

Thanks for this, I plan on giving it a more thorough review when I can. But the summary led me to a design question: why do we support multiple attribute *specifiers* in the same pragma? I would not expect to be able to mix attribute styles in the same pragma given that all of the individual styles allow you to specify multiple attributes within a single specifier. e.g., https://godbolt.org/z/9v7r6Eanz

__declspec(deprecated, naked) void f();
__attribute__((annotate("test"), constructor(0))) void g();
[[clang::annotate("test"), gnu::constructor(0)]] void h();

What's the use case for letting someone mix and match attribute styles as in:

#pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((annotate("test"))), apply_to = function)

and why is whitespace the correct separator as opposed to a comma-delimited list?

(Note, I'm still thinking about whether this approach poses any actual problems; it may be perfectly fine, but it'd help to know why we're being lax in mixing forms and what the rationale is for whitespace separation; that's not a common approach to lists of things in C or C++).

Also, I'd expect there to be some documentation changes along with this patch and a release note.

[egorzhdan]

In D121283#3373560, @aaron.ballman wrote:

why do we support multiple attribute *specifiers* in the same pragma? I would not expect to be able to mix attribute styles in the same pragma given that all of the individual styles allow you to specify multiple attributes within a single specifier

I don't think I have a strong use case for this. It seemed consistent with the way multiple attributes can be added for individual declarations, e.g. __attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(). But we can prohibit multiple specifiers within a single pragma if you think that this is not a good construct to support.

In D121283#3373560, @aaron.ballman wrote:

why is whitespace the correct separator as opposed to a comma-delimited list?

My motivation for this was also consistency with the syntax for attributes in individual declarations. Given that attribute specifiers are delimited by space for individual declarations (__attribute__((cdecl)) __attribute__((noreturn)) void f()), I think it would be unintuitive to require commas for attribute specifiers in pragmas when we could instead reuse the existing syntax of space-delimited attribute specifiers.

In D121283#3373560, @aaron.ballman wrote:

Also, I'd expect there to be some documentation changes along with this patch and a release note.

Good point, thanks, I will add those to this patch.

[aaron.ballman]

In D121283#3375806, @egorzhdan wrote:

In D121283#3373560, @aaron.ballman wrote:

why do we support multiple attribute *specifiers* in the same pragma? I would not expect to be able to mix attribute styles in the same pragma given that all of the individual styles allow you to specify multiple attributes within a single specifier

I don't think I have a strong use case for this. It seemed consistent with the way multiple attributes can be added for individual declarations, e.g. __attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(). But we can prohibit multiple specifiers within a single pragma if you think that this is not a good construct to support.

I don't yet think it's a *bad* construct to support...

In D121283#3373560, @aaron.ballman wrote:

why is whitespace the correct separator as opposed to a comma-delimited list?

My motivation for this was also consistency with the syntax for attributes in individual declarations. Given that attribute specifiers are delimited by space for individual declarations (__attribute__((cdecl)) __attribute__((noreturn)) void f()), I think it would be unintuitive to require commas for attribute specifiers in pragmas when we could instead reuse the existing syntax of space-delimited attribute specifiers.

Thanks, that makes some sense to me, but at the same time, I can't think of another pragma that behaves similarly off the top of my head (usually, lists of things have a delimiter other than whitespace), so it's kind of unintuitive either way.

As a thought experiment, would it make sense to lift the restriction on the number of attributes allowed in a pragma, but not allow multiple attribute specifiers? e.g.,

// These are fine
#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
#pragma clang attribute pop

#pragma clang attribute push (__attribute__((noreturn, noinline)), apply_to = function)
#pragma clang attribute pop

// These are not allowed
#pragma clang attribute push ([[clang::disable_tail_calls]] [[noreturn]], apply_to = function)
#pragma clang attribute pop

#pragma clang attribute push (__attribute__((noreturn)) __attribute__((noinline)), apply_to = function)
#pragma clang attribute pop

#pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((noreturn)), apply_to = function)
#pragma clang attribute pop

This still allows you to specify more than one attribute in the pragma, but removes the concerns about how to separate the syntaxes (whitespace or another delimiter) while still leaving that design open in the future if there's a strong need to mix syntaxes.

The pragma attribute feature has a lot of power to it, but it also comes with a lot of risk to users, so it's a feature that I think we should be cautious about extending (in general). I think what you propose here could very well be reasonable, but I'm a bit worried that there's not a clear need for that amount of flexibility, which is why I'm sort of leaning towards being more restrictive here. However, this isn't exposing any functionality that users can't already do the long way with multiple pragmas, so I don't see a blocking concern with the current patch either.

[egorzhdan]

In D121283#3379225, @aaron.ballman wrote:

As a thought experiment, would it make sense to lift the restriction on the number of attributes allowed in a pragma, but not allow multiple attribute specifiers?

I think this is reasonable, and as you've mentioned, it also leaves the opportunity to extend this syntax later should the need arise.
I will adjust this patch shortly.

[egorzhdan]

• Only allow one attribute syntax style per directive
• Adjust documentation
• Add a release note

[aaron.ballman]

Aside from a minor testing nit, this LGTM. Thank you for the new functionality!

clang/test/Parser/pragma-multiple-attributes.cpp

1–11 ↗

(On Diff #415199)

I think this entire test file can now be folded back into pragma-attribute.cpp (this way we don't have to pay the overhead of another execution of Clang to test the functionality).

[egorzhdan]

Merge two test files into one to speed up testing

[egorzhdan]

Thanks for the review @aaron.ballman!

[egorzhdan]

• Fix off-by-one error in fix-it generation logic: the last SubjectMatchRule

(SubjectMatchRule_variable_not_is_parameter) was not handled properly

• Add a test for this

[thakis]

The commit message got a bit mutilated: 33a9eac6aaa495fce6fd9b17cd48aa57a95461e6

Just FYI in case you didn't notice. In that case, update your commit workflow to make sure this doesn't happen next time :)

[egorzhdan]

In D121283#3392137, @thakis wrote:

The commit message got a bit mutilated: 33a9eac6aaa495fce6fd9b17cd48aa57a95461e6

Just FYI in case you didn't notice. In that case, update your commit workflow to make sure this doesn't happen next time :)

That's strange, I didn't notice this, thanks for the heads up.

clang/docs/LanguageExtensions.rst

 .. code-block:: c++
 
   #pragma clang attribute push (__declspec(dllexport), apply_to = function)
 
   void function(); // The function now has the __declspec(dllexport) attribute
 
   #pragma clang attribute pop
 
-A single push directive accepts only one attribute regardless of the syntax
-used.
+A single push directive can contain multiple attributes, however, 
+only one syntax style can be used within a single directive:
+
+.. code-block:: c++
+
+  #pragma clang attribute push ([[noreturn, noinline]], apply_to = function)
+
+  void function1(); // The function now has the [[noreturn]] and [[noinline]] attributes
+
+  #pragma clang attribute pop
+  
+  #pragma clang attribute push (__attribute((noreturn, noinline)), apply_to = function)
+
+  void function2(); // The function now has the __attribute((noreturn)) and __attribute((noinline)) attributes
+
+  #pragma clang attribute pop
 
 Because multiple push directives can be nested, if you're writing a macro that
 expands to ``_Pragma("clang attribute")`` it's good hygiene (though not
 required) to add a namespace to your push/pop directives. A pop directive with a
 namespace will pop the innermost push that has that same namespace. This will
 ensure that another macro's ``pop`` won't inadvertently pop your attribute. Note
 that an ``pop`` without a namespace will pop the innermost ``push`` without a
 namespace. ``push``es with a namespace can only be popped by ``pop`` with the

clang/docs/ReleaseNotes.rst

- Improved namespace attributes handling:
 
   - Handle GNU attributes before a namespace identifier and subsequent
     attributes of different kinds.
   - Emit error on GNU attributes for a nested namespace definition.
 
 - Statement attributes ``[[clang::noinline]]`` and  ``[[clang::always_inline]]``
   can be used to control inlining decisions at callsites.
 
+- ``#pragma clang attribute push`` now supports multiple attributes within a single directive.
+
 Windows Support
 ---------------
 
 - Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl (supports
   X86/X64/ARM/ARM64). ``/JMC`` could only be used when ``/Zi`` or ``/Z7`` is
   turned on. With this addition, clang-cl can be used in Visual Studio for the
   JustMyCode feature. Note, you may need to manually add ``/JMC`` as additional
   compile options in the Visual Studio since it currently assumes clang-cl does not support ``/JMC``.

clang/include/clang/Basic/AttrSubjectMatchRules.h

 
 namespace clang {
 namespace attr {
 
 /// A list of all the recognized kinds of attributes.
 enum SubjectMatchRule {
 #define ATTR_MATCH_RULE(X, Spelling, IsAbstract) X,
 #include "clang/Basic/AttrSubMatchRulesList.inc"
+  SubjectMatchRule_Last = -1
+#define ATTR_MATCH_RULE(X, Spelling, IsAbstract) +1
+#include "clang/Basic/AttrSubMatchRulesList.inc"
 };
 
 const char *getSubjectMatchRuleSpelling(SubjectMatchRule Rule);
 
 using ParsedSubjectMatchRuleSet = llvm::DenseMap<int, SourceRange>;
 
 } // end namespace attr
 } // end namespace clang
 
 #endif

clang/include/clang/Basic/DiagnosticParseKinds.td

 def err_pragma_attribute_expected_attribute : Error<
   "expected an attribute after '('">;
 def err_pragma_attribute_expected_attribute_name : Error<
   "expected identifier that represents an attribute name">;
 def err_pragma_attribute_extra_tokens_after_attribute : Error<
   "extra tokens after attribute in a '#pragma clang attribute push'">;
 def err_pragma_attribute_unsupported_attribute : Error<
   "attribute %0 is not supported by '#pragma clang attribute'">;
-def err_pragma_attribute_multiple_attributes : Error<
-  "more than one attribute specified in '#pragma clang attribute push'">;
 def err_pragma_attribute_expected_attribute_syntax : Error<
   "expected an attribute that is specified using the GNU, C++11 or '__declspec'"
   " syntax">;
 def note_pragma_attribute_use_attribute_kw : Note<"use the GNU '__attribute__' "
   "syntax">;
 def err_pragma_attribute_invalid_subject_set_specifier : Error<
   "expected attribute subject set specifier 'apply_to'">;
 def err_pragma_attribute_expected_subject_identifier : Error<

clang/lib/Parse/ParseDecl.cpp

case ParsedAttr::AT_TypeTagForDatatype:
     break;
   }
   return !Attrs.empty() ? Attrs.begin()->getNumArgs() : 0;
 }
 
 bool Parser::ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
                                         SourceLocation AttrNameLoc,
                                         ParsedAttributes &Attrs) {
+  unsigned ExistingAttrs = Attrs.size();
+
   // If the attribute isn't known, we will not attempt to parse any
   // arguments.
   if (!hasAttribute(AttrSyntax::Declspec, nullptr, AttrName,
                     getTargetInfo(), getLangOpts())) {
     // Eat the left paren, then skip to the ending right paren.
     ConsumeParen();
     SkipUntil(tok::r_paren);
     return false;
bool Parser::ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
   }
 
   unsigned NumArgs =
       ParseAttributeArgsCommon(AttrName, AttrNameLoc, Attrs, nullptr, nullptr,
                                SourceLocation(), ParsedAttr::AS_Declspec);
 
   // If this attribute's args were parsed, and it was expected to have
   // arguments but none were provided, emit a diagnostic.
-  if (!Attrs.empty() && Attrs.begin()->getMaxArgs() && !NumArgs) {
+  if (ExistingAttrs < Attrs.size() && Attrs.back().getMaxArgs() && !NumArgs) {
     Diag(OpenParenLoc, diag::err_attribute_requires_arguments) << AttrName;
     return false;
   }
   return true;
 }
 
 /// [MS] decl-specifier:
 ///             __declspec ( extended-decl-modifier-seq )

clang/lib/Parse/ParsePragma.cpp

if (Tok.is(tok::equal))
     return MissingAttributeSubjectRulesRecoveryPoint::Equals;
   return MissingAttributeSubjectRulesRecoveryPoint::None;
 }
 
 /// Creates a diagnostic for the attribute subject rule parsing diagnostic that
 /// suggests the possible attribute subject rules in a fix-it together with
 /// any other missing tokens.
 DiagnosticBuilder createExpectedAttributeSubjectRulesTokenDiagnostic(
-    unsigned DiagID, ParsedAttr &Attribute,
+    unsigned DiagID, ParsedAttributes &Attrs,
     MissingAttributeSubjectRulesRecoveryPoint Point, Parser &PRef) {
   SourceLocation Loc = PRef.getEndOfPreviousToken();
   if (Loc.isInvalid())
     Loc = PRef.getCurToken().getLocation();
   auto Diagnostic = PRef.Diag(Loc, DiagID);
   std::string FixIt;
   MissingAttributeSubjectRulesRecoveryPoint EndPoint =
       getAttributeSubjectRulesRecoveryPointForToken(PRef.getCurToken());
   if (Point == MissingAttributeSubjectRulesRecoveryPoint::Comma)
     FixIt = ", ";
   if (Point <= MissingAttributeSubjectRulesRecoveryPoint::ApplyTo &&
       EndPoint > MissingAttributeSubjectRulesRecoveryPoint::ApplyTo)
     FixIt += "apply_to";
   if (Point <= MissingAttributeSubjectRulesRecoveryPoint::Equals &&
       EndPoint > MissingAttributeSubjectRulesRecoveryPoint::Equals)
     FixIt += " = ";
   SourceRange FixItRange(Loc);
   if (EndPoint == MissingAttributeSubjectRulesRecoveryPoint::None) {
     // Gather the subject match rules that are supported by the attribute.
-    SmallVector<std::pair<attr::SubjectMatchRule, bool>, 4> SubjectMatchRuleSet;
-    Attribute.getMatchRules(PRef.getLangOpts(), SubjectMatchRuleSet);
-    if (SubjectMatchRuleSet.empty()) {
+    // Add all the possible rules initially.
+    llvm::BitVector IsMatchRuleAvailable(attr::SubjectMatchRule_Last + 1, true);
+    // Remove the ones that are not supported by any of the attributes.
+    for (const ParsedAttr &Attribute : Attrs) {
+      SmallVector<std::pair<attr::SubjectMatchRule, bool>, 4> MatchRules;
+      Attribute.getMatchRules(PRef.getLangOpts(), MatchRules);
+      llvm::BitVector IsSupported(attr::SubjectMatchRule_Last + 1);
+      for (const auto &Rule : MatchRules) {
+        // Ensure that the missing rule is reported in the fix-it only when it's
+        // supported in the current language mode.
+        if (!Rule.second)
+          continue;
+        IsSupported[Rule.first] = true;
+      }
+      IsMatchRuleAvailable &= IsSupported;
+    }
+    if (IsMatchRuleAvailable.count() == 0) {
       // FIXME: We can emit a "fix-it" with a subject list placeholder when
       // placeholders will be supported by the fix-its.
       return Diagnostic;
     }
     FixIt += "any(";
     bool NeedsComma = false;
-    for (const auto &I : SubjectMatchRuleSet) {
-      // Ensure that the missing rule is reported in the fix-it only when it's
-      // supported in the current language mode.
-      if (!I.second)
+    for (unsigned I = 0; I <= attr::SubjectMatchRule_Last; I++) {
+      if (!IsMatchRuleAvailable[I])
         continue;
       if (NeedsComma)
         FixIt += ", ";
       else
         NeedsComma = true;
-      FixIt += attr::getSubjectMatchRuleSpelling(I.first);
+      FixIt += attr::getSubjectMatchRuleSpelling(
+          static_cast<attr::SubjectMatchRule>(I));
     }
     FixIt += ")";
     // Check if we need to remove the range
     PRef.SkipUntil(tok::eof, Parser::StopBeforeMatch);
     FixItRange.setEnd(PRef.getCurToken().getLocation());
   }
   if (FixItRange.getBegin() == FixItRange.getEnd())
     Diagnostic << FixItHint::CreateInsertion(FixItRange.getBegin(), FixIt);
if (Tok.is(tok::l_square) && NextToken().is(tok::l_square)) {
     // we only get here if the line has balanced parens.
     if (Tok.is(tok::code_completion)) {
       cutOffParsing();
       // FIXME: suppress completion of unsupported attributes?
       Actions.CodeCompleteAttribute(AttributeCommonInfo::Syntax::AS_GNU);
       return SkipToEnd();
     }
 
+    // Parse the comma-separated list of attributes.
+    do {
       if (Tok.isNot(tok::identifier)) {
         Diag(Tok, diag::err_pragma_attribute_expected_attribute_name);
         SkipToEnd();
         return;
       }
       IdentifierInfo *AttrName = Tok.getIdentifierInfo();
       SourceLocation AttrNameLoc = ConsumeToken();
 
       if (Tok.isNot(tok::l_paren))
         Attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
                      ParsedAttr::AS_GNU);
       else
         ParseGNUAttributeArgs(AttrName, AttrNameLoc, Attrs, /*EndLoc=*/nullptr,
                               /*ScopeName=*/nullptr,
                               /*ScopeLoc=*/SourceLocation(), ParsedAttr::AS_GNU,
                               /*Declarator=*/nullptr);
+    } while (TryConsumeToken(tok::comma));
 
     if (ExpectAndConsume(tok::r_paren))
       return SkipToEnd();
     if (ExpectAndConsume(tok::r_paren))
       return SkipToEnd();
   } else if (Tok.is(tok::kw___declspec)) {
     ParseMicrosoftDeclSpecs(Attrs);
   } else {
if (Tok.is(tok::l_square) && NextToken().is(tok::l_square)) {
     return;
   }
 
   if (Attrs.empty() || Attrs.begin()->isInvalid()) {
     SkipToEnd();
     return;
   }
 
-  // Ensure that we don't have more than one attribute.
-  if (Attrs.size() > 1) {
-    SourceLocation Loc = Attrs[1].getLoc();
-    Diag(Loc, diag::err_pragma_attribute_multiple_attributes);
-    SkipToEnd();
-    return;
-  }
-
-  ParsedAttr &Attribute = *Attrs.begin();
+  for (const ParsedAttr &Attribute : Attrs) {
     if (!Attribute.isSupportedByPragmaAttribute()) {
       Diag(PragmaLoc, diag::err_pragma_attribute_unsupported_attribute)
           << Attribute;
       SkipToEnd();
       return;
     }
+  }
 
   // Parse the subject-list.
   if (!TryConsumeToken(tok::comma)) {
     createExpectedAttributeSubjectRulesTokenDiagnostic(
-        diag::err_expected, Attribute,
+        diag::err_expected, Attrs,
         MissingAttributeSubjectRulesRecoveryPoint::Comma, *this)
         << tok::comma;
     SkipToEnd();
     return;
   }
 
   if (Tok.isNot(tok::identifier)) {
     createExpectedAttributeSubjectRulesTokenDiagnostic(
-        diag::err_pragma_attribute_invalid_subject_set_specifier, Attribute,
+        diag::err_pragma_attribute_invalid_subject_set_specifier, Attrs,
         MissingAttributeSubjectRulesRecoveryPoint::ApplyTo, *this);
     SkipToEnd();
     return;
   }
   const IdentifierInfo *II = Tok.getIdentifierInfo();
   if (!II->isStr("apply_to")) {
     createExpectedAttributeSubjectRulesTokenDiagnostic(
-        diag::err_pragma_attribute_invalid_subject_set_specifier, Attribute,
+        diag::err_pragma_attribute_invalid_subject_set_specifier, Attrs,
         MissingAttributeSubjectRulesRecoveryPoint::ApplyTo, *this);
     SkipToEnd();
     return;
   }
   ConsumeToken();
 
   if (!TryConsumeToken(tok::equal)) {
     createExpectedAttributeSubjectRulesTokenDiagnostic(
-        diag::err_expected, Attribute,
+        diag::err_expected, Attrs,
         MissingAttributeSubjectRulesRecoveryPoint::Equals, *this)
         << tok::equal;
     SkipToEnd();
     return;
   }
 
   attr::ParsedSubjectMatchRuleSet SubjectMatchRules;
   SourceLocation AnyLoc, LastMatchRuleEndLoc;
void Parser::HandlePragmaAttribute() {
 
   // Consume the eof terminator token.
   ConsumeToken();
 
   // Handle a mixed push/attribute by desurging to a push, then an attribute.
   if (Info->Action == PragmaAttributeInfo::Push)
     Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc, Info->Namespace);
 
+  for (ParsedAttr &Attribute : Attrs) {
     Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
-                                        std::move(SubjectMatchRules));
+                                          SubjectMatchRules);
+  }
 }
 
 // #pragma GCC visibility comes in two variants:
 //   'push' '(' [visibility] ')'
 //   'pop'
 void PragmaGCCVisibilityHandler::HandlePragma(Preprocessor &PP,
                                               PragmaIntroducer Introducer,
                                               Token &VisTok) {

clang/test/AST/pragma-multiple-attributes-declspec.cpp

+// RUN: %clang_cc1 -triple i386-pc-win32 -fms-extensions -fms-compatibility -fsyntax-only -ast-dump %s | FileCheck %s
+
+#pragma clang attribute push (__declspec(dllexport, noinline), apply_to=function)
+void func1();
+#pragma clang attribute pop
+// CHECK: FunctionDecl {{.*}} func1
+// CHECK-NEXT: DLLExportAttr {{.*}}
+// CHECK-NEXT: NoInlineAttr {{.*}}

clang/test/AST/pragma-multiple-attributes.cpp

+// RUN: %clang_cc1 -fsyntax-only -ast-dump %s | FileCheck %s
+
+#pragma clang attribute push (__attribute__((disable_sanitizer_instrumentation, annotate("test1"))), apply_to=variable(is_global))
+int var1;
+#pragma clang attribute pop
+// CHECK: VarDecl {{.*}} var1
+// CHECK-NEXT: DisableSanitizerInstrumentationAttr {{.*}}
+// CHECK-NEXT: AnnotateAttr {{.*}} "test1"
+
+#pragma clang attribute push ([[clang::disable_sanitizer_instrumentation, clang::annotate("test2")]], apply_to=variable(is_global))
+int var2;
+#pragma clang attribute pop
+// CHECK: VarDecl {{.*}} var2
+// CHECK-NEXT: DisableSanitizerInstrumentationAttr {{.*}}
+// CHECK-NEXT: AnnotateAttr {{.*}} "test2"

clang/test/FixIt/fixit-pragma-attribute.c

 // RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 // Verify that the suggested attribute subject match rules don't include the
 // rules that are not applicable in the current language mode.
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, record(unless(is_union)), variable)"

clang/test/FixIt/fixit-pragma-attribute.cpp

 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:106-[[@LINE-1]]:137}:""
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(enum, variable))
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:77-[[@LINE-1]]:82}:""
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) apply_to=function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", "
 #pragma clang attribute push (__attribute__((abi_tag("a"))) = function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) any(function))
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:60}:", apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))) 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:63}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:69}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))) (function))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:71}:", apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:60-[[@LINE-1]]:71}:", apply_to = any(function, namespace, record(unless(is_union)), variable)"
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), )
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:62}:"apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:62}:"apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), = function)
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:61}:"apply_to"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), any(function))
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:61}:"apply_to = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), 22)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:64}:"apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:64}:"apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), 1, 2)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:66}:"apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:66}:"apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), function)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:70}:"apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:70}:"apply_to = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), (function))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:72}:"apply_to = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:61-[[@LINE-1]]:72}:"apply_to = any(function, namespace, record(unless(is_union)), variable)"
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to)
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:70-[[@LINE-1]]:70}:" = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:70-[[@LINE-1]]:70}:" = any(function, namespace, record(unless(is_union)), variable)"
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to any(function))
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:70-[[@LINE-1]]:70}:" = "
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to 41 (22))
-// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:70-[[@LINE-1]]:78}:" = any(record(unless(is_union)), variable, function, namespace)"
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:70-[[@LINE-1]]:78}:" = any(function, namespace, record(unless(is_union)), variable)"
 
 // Don't give fix-it to attributes without a strict subject set
 #pragma clang attribute push (__attribute__((annotate("a"))))
 // CHECK-NO: [[@LINE-1]]:61
+
+#pragma clang attribute push (__attribute__((objc_externally_retained)), apply_to)
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:82-[[@LINE-1]]:82}:" = any(function, variable(unless(is_parameter)))"

clang/test/Parser/pragma-attribute-declspec.cpp

 // RUN: %clang_cc1 -triple i386-pc-win32 -std=c++11  -verify -Wno-pragma-clang-attribute -fms-extensions -fms-compatibility %s
 
 #pragma clang attribute push(__declspec(dllexport), apply_to = function)
 
 void function();
 
 #pragma clang attribute pop
 
-#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
+#pragma clang attribute push(__declspec(dllexport, dllimport), apply_to = function)
+#pragma clang attribute pop
 
 #pragma clang attribute push(__declspec(align), apply_to = variable) // expected-error {{attribute 'align' is not supported by '#pragma clang attribute'}}
 
 #pragma clang attribute push(__declspec(), apply_to = variable) // A noop

clang/test/Parser/pragma-attribute.cpp

 #pragma clang attribute pop
 
 #pragma clang attribute push ([[clang::disable_tail_calls]], apply_to = function)
 #pragma clang attribute pop
 
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=any(function))
 #pragma clang attribute pop
 
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{more than one attribute specified in '#pragma clang attribute push'}}
-
 #pragma clang attribute push ([[gnu::abi_tag]], apply_to=namespace)
 #pragma clang attribute pop
 
 #pragma clang attribute push ([[fallthrough]], apply_to=function) // expected-error {{attribute 'fallthrough' is not supported by '#pragma clang attribute'}}
 #pragma clang attribute push ([[clang::fallthrough]], apply_to=function) // expected-error {{attribute 'fallthrough' is not supported by '#pragma clang attribute'}}
 
 #pragma clang attribute push ([[]], apply_to = function) // A noop
 

 #pragma clang attribute push([[clang::no_destroy]], apply_to = any(variable(is_parameter), variable(unless(is_parameter))))
 #pragma clang attribute pop
 
 // We explicitly do not wish to allow users to blast an attribute to everything
 // using an unconstrained "any", so "any" must have a valid argument list.
 #pragma clang attribute push([[clang::uninitialized]], apply_to=any) // expected-error {{expected '('}}
 #pragma clang attribute push([[clang::uninitialized]], apply_to = any()) // expected-error {{expected an identifier that corresponds to an attribute subject rule}}
 // NB: neither of these need to be popped; they were never successfully pushed.
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls, annotate("test"))), apply_to = function)
+#pragma clang attribute pop
+
+#pragma clang attribute push (__attribute__((disable_tail_calls,)), apply_to = function) // expected-error {{expected identifier that represents an attribute name}}
+
+#pragma clang attribute push ([[clang::disable_tail_calls, noreturn]]) // expected-error {{expected ','}}