[Clang] Support multiple attributes in a single pragma

egorzhdan · egorzhdan · commit 33a9eac6aaa4 · 2022-03-18T12:20:41.000Z
This adds support for multiple attributes in `#pragma clang attribute push`, for example: ``` ``` or ``` ``` 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 Differential Revision: https://reviews.llvm.org/D121283
diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
@@ -4121,8 +4121,22 @@ The ``__declspec`` style syntax is also supported:
 
   #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
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -106,6 +106,8 @@ Attribute Changes in Clang
 - 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
 ---------------
 
diff --git a/clang/include/clang/Basic/AttrSubjectMatchRules.h b/clang/include/clang/Basic/AttrSubjectMatchRules.h
@@ -18,6 +18,9 @@ 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"
 };
 
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1237,8 +1237,6 @@ 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">;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
@@ -600,6 +600,8 @@ unsigned Parser::ParseClangAttributeArgs(
 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,
@@ -732,7 +734,7 @@ bool Parser::ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
 
   // 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;
   }
diff --git a/clang/lib/Parse/ParsePragma.cpp b/clang/lib/Parse/ParsePragma.cpp
@@ -1561,7 +1561,7 @@ getAttributeSubjectRulesRecoveryPointForToken(const Token &Tok) {
 /// 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())
@@ -1581,25 +1581,38 @@ DiagnosticBuilder createExpectedAttributeSubjectRulesTokenDiagnostic(
   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
@@ -1669,22 +1682,25 @@ void Parser::HandlePragmaAttribute() {
       return SkipToEnd();
     }
 
-    if (Tok.isNot(tok::identifier)) {
-      Diag(Tok, diag::err_pragma_attribute_expected_attribute_name);
-      SkipToEnd();
-      return;
-    }
-    IdentifierInfo *AttrName = Tok.getIdentifierInfo();
-    SourceLocation AttrNameLoc = ConsumeToken();
+    // 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);
+      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();
@@ -1722,26 +1738,19 @@ void Parser::HandlePragmaAttribute() {
     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();
-  if (!Attribute.isSupportedByPragmaAttribute()) {
-    Diag(PragmaLoc, diag::err_pragma_attribute_unsupported_attribute)
-        << Attribute;
-    SkipToEnd();
-    return;
+  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();
@@ -1750,15 +1759,15 @@ void Parser::HandlePragmaAttribute() {
 
   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;
@@ -1767,7 +1776,7 @@ void Parser::HandlePragmaAttribute() {
 
   if (!TryConsumeToken(tok::equal)) {
     createExpectedAttributeSubjectRulesTokenDiagnostic(
-        diag::err_expected, Attribute,
+        diag::err_expected, Attrs,
         MissingAttributeSubjectRulesRecoveryPoint::Equals, *this)
         << tok::equal;
     SkipToEnd();
@@ -1797,8 +1806,10 @@ void Parser::HandlePragmaAttribute() {
   if (Info->Action == PragmaAttributeInfo::Push)
     Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc, Info->Namespace);
 
-  Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
-                                        std::move(SubjectMatchRules));
+  for (ParsedAttr &Attribute : Attrs) {
+    Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
+                                          SubjectMatchRules);
+  }
 }
 
 // #pragma GCC visibility comes in two variants:
diff --git a/clang/test/AST/pragma-multiple-attributes-declspec.cpp b/clang/test/AST/pragma-multiple-attributes-declspec.cpp
@@ -0,0 +1,8 @@
+// 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 {{.*}}
diff --git a/clang/test/AST/pragma-multiple-attributes.cpp b/clang/test/AST/pragma-multiple-attributes.cpp
@@ -0,0 +1,15 @@
+// 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"
diff --git a/clang/test/FixIt/fixit-pragma-attribute.c b/clang/test/FixIt/fixit-pragma-attribute.c
@@ -3,4 +3,4 @@
 // 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)"
diff --git a/clang/test/FixIt/fixit-pragma-attribute.cpp b/clang/test/FixIt/fixit-pragma-attribute.cpp
@@ -39,7 +39,7 @@
 #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)
@@ -48,36 +48,39 @@
 // 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)))"
diff --git a/clang/test/Parser/pragma-attribute-declspec.cpp b/clang/test/Parser/pragma-attribute-declspec.cpp
@@ -6,7 +6,8 @@ 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'}}
 
diff --git a/clang/test/Parser/pragma-attribute.cpp b/clang/test/Parser/pragma-attribute.cpp
@@ -154,9 +154,6 @@ _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
 
@@ -210,3 +207,13 @@ _Pragma("clang attribute pop");
 #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 ','}}
