Add support for "labels" on push/pop directives in #pragma clang attribute

[erik.pilkington]

Summary

One problem with defining macros that expand to _Pragma("clang attribute")` is that they don't nest very well:

// In some header...
#define ASSUME_X_BEGIN _Pragma ("clang attribute push(__attribute__((x)), apply_to=variable)")
#define ASSUME_X_END _Pragma ("clang attribute pop")

#define ASSUME_Y_BEGIN _Pragma("clang attribute push(__attribute__((y)), apply_to=variable)")
#define ASSUME_Y_END _Pragma("clang attribute pop")

// in some source file...

ASSUME_X_BEGIN
// lots of code
ASSUME_Y_BEGIN
// some more code
ASSUME_X_END // oops! Popped Y, but meant to pop X.
// some other code
ASSUME_Y_END // ditto

This patch adds a way to fix this by adding labels to push/pop directives. A clang attribute pops with a label will only pop a pushed group with that same label. Macro authors can then add a unique label to their BEGIN/END functions, which would cause the code above to work fine.

Thanks for taking a look!
Erik

[aaron.ballman]

clang/docs/LanguageExtensions.rst

2667

The documentation reads like the label is optional, but the examples make it look required (because all examples have the label present). Can you clarify what happens when the label is omitted and keep an example that shows it?

clang/include/clang/Basic/DiagnosticSemaKinds.td

840–842

Can this be combined with the diagnostic above using %select (given that the wording is basically identical)?

clang/include/clang/Sema/Sema.h

8504

Can you sprinkle some const-correctness around to make this const IdentifierInfo *?

clang/lib/Sema/SemaAttr.cpp

648

corrisponds -> corresponds

659

You can pass in *Entry.Attribute here instead of calling getName() and it will properly quote the results.

clang/test/Sema/pragma-attribute-label.c

7

Should we really treat this as an error? It seems to me that this should be a warning because pop without a label could be viewed as "I don't care what I'm popping, just pop it". Still worth warning about, but maybe not worth stopping a build over.

15

I feel like this should be diagnosed, perhaps even as an error. The user provided labels but then got the push and pop order wrong when explicitly saying what to pop. That seems more likely to be a logic error on the user's part.

[dexonsmith]

clang/test/Sema/pragma-attribute-label.c

7

IMO this is most likely to be an implementation error on the part of a macro author, where the END macro is missing the label used in BEGIN. This makes the macro pair unsafe to mix with other macros. If the macro author doesn’t want safety, why use a label in the BEGIN macro at all?

I see you’re envisioning this being used directly by an end-user, which I suppose is plausible, but I think the same logic applies. Why add a label to push if you don’t want to be precise about pop?

15

On the contrary, the user is using two differently-named and independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are implemented with _Pragma(“clang attribute ...”) under the hood. The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

[aaron.ballman]

clang/test/Sema/pragma-attribute-label.c

7

Why add a label to push if you don’t want to be precise about pop?

Why is this important enough to fail everyone's build over it as opposed to warning users that they've done something that could be a bad code smell and let -Werror usage decide whether to fail the build or not? It seems like an extreme measure for something that has explicable "fallback" behavior.

15

On the contrary, the user is using two differently-named and independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END)

I don't think this is a safe assumption to make, and in this case, is false. There are no macros anywhere in this test case.

The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

I don't find this to be intuitive behavior. These are stack manipulations with the names push and pop -- pretending that they're overlapping rather than a stack in the presence of labels is confusing. If I saw the code from this test case during a code review, I would flag it as being incorrect because the labels do not match -- I don't think I'd be the only one.

[erik.pilkington]

clang/test/Sema/pragma-attribute-label.c

7

My implicit assumption (which I should have been more clear about!) was that you'd only really ever write a label on a push in a BEGIN/END macro. In that world, you'd only ever see this case if 1) you're interacting with another macro that doesn't use the label convention, or 2) if you're interacting with manual push/pop code. In both of those cases, you'd end up popping the wrong attribute group and start applying attributes onto declarations that the programmer didn't intend.

I'm fine with downgrading this to a warning, but IMO an error seems more appropriate. If we wanted to force 1) or 2) through the compiler then we'd also need to downgrade pop UNPUSHED_LABEL to a warning, which doesn't seem like the end of the world either.

15

I think the labels only really makes sense if you're writing macros that hide the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), which for better or for worse people do write, and in fact was the intended use case for #pragma clang attribute. I think if we were to write this feature again, we'd forgo the stack entirly and require every push to have a label and be in its own namespace. But this is the best we can do now.

I don't really think that anyone should write a push label outside of a macro definition, since I agree that the semantics are a bit surprising when you're writing the #pragmas yourself without macros. I'll update this test case and the documentation to stress this point more. If you think this is going to be a potential pain point, maybe we can even warn on using a label outside of a macro definition.

[erik.pilkington]

Address @aaron.ballman comments. Thanks!

clang/docs/LanguageExtensions.rst

2667

Sure, I moved this to the end of the section, added an example, and left the manual #pragma clang attribute below intact to make it more clear.

clang/include/clang/Basic/DiagnosticSemaKinds.td

840–842

Sure, good point.

[dexonsmith]

clang/test/Sema/pragma-attribute-label.c

7

I'm not fine with this just being a warning. The entire point is that the stacks are independent; having them suddenly mixed here would be super confusing. @aaron.ballman, please see my other comment for a longer explanation.

15

The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

I don't find this to be intuitive behavior. These are stack manipulations with the names push and pop -- pretending that they're overlapping rather than a stack in the presence of labels is confusing. If I saw the code from this test case during a code review, I would flag it as being incorrect because the labels do not match -- I don't think I'd be the only one.

As Erik says, the intention is that these are only used by macros.

Stepping back, the goal of this pragma (which we added in https://reviews.llvm.org/D30009) is to avoid adding a new region-based pragma every time we want to apply an attribute within a region. Clang has a lot of these pragmas, in order to support independent macros like this:

#define A_BEGIN _Pragma("clang a push")
#define A_END   _Pragma("clang a pop")
#define B_BEGIN _Pragma("clang b push")
#define B_END   _Pragma("clang b pop")
#define C_BEGIN _Pragma("clang c push")
#define C_END   _Pragma("clang c pop")

@arphaman came up with the idea of introduce "one pragma to rule them all", pragma clang attribute, so that we didn't need to introduce a new pragma each time we wanted an independent region:

#define A_BEGIN _Pragma("clang attribute push(a)")
#define A_END   _Pragma("clang attribute pop")
#define B_BEGIN _Pragma("clang attribute push(b)")
#define B_END   _Pragma("clang attribute pop")
#define C_BEGIN _Pragma("clang attribute push(c)")
#define C_END   _Pragma("clang attribute pop")

However, we've realized that there is a major design flaw: these macros are not independent, because they're using the same stack. @erik.pilkington has come up with this solution using identifiers to namespace the attribute stacks, allowing the macros to be independent (like they were before, when each had a dedicated pragma):

#define A_BEGIN _Pragma("clang attribute push A (a)")
#define A_END   _Pragma("clang attribute pop  A")
#define B_BEGIN _Pragma("clang attribute push B (b)")
#define B_END   _Pragma("clang attribute pop  B")
#define C_BEGIN _Pragma("clang attribute push C (c)")
#define C_END   _Pragma("clang attribute pop  C")

To be clear, without this behaviour (or something equivalent) #pragma clang attribute is completely broken for its motivating use case. If we can't find a design that allows us to interleave these macros, we will have to abandon this pragma entirely (and go back to adding a significant number of dedicated one-off pragmas).

But it's the behaviour we care about, not this particular syntax. Since this pragma is designed specifically to be hidden behind macros, it can be as ugly as you want. Is there another way of spelling this that you find more intuitive? Or should we just improve the docs as Erik has done?

[aaron.ballman]

clang/test/Sema/pragma-attribute-label.c

15

As Erik says, the intention is that these are only used by macros.

That's a nice intention, but that doesn't mean all users are going to do that. I'm worried about the users who don't adhere to that intention and use the pragmas in their more exposed format, given that it's a supported syntax.

But it's the behaviour we care about, not this particular syntax. Since this pragma is designed specifically to be hidden behind macros, it can be as ugly as you want.

I'm not worried about the ugliness, I'm worried about whether it's understandable behavior when the pragma is not hidden behind a macro.

Is there another way of spelling this that you find more intuitive? Or should we just improve the docs as Erik has done?

@erik.pilkington and I talked over IRC and I have a better understanding of where my disconnect is here. It seems as though push/pop was the incorrect initial design for the feature and what we really wish we had was a region-based approach rather than a stack-based approach. Would it make sense to add #pragma clang attribute begin/end syntax with labels instead? Then the macros could switch to using that approach to solve the problems you're running into. We could still have push/pop with labels, but they could warn on label mismatches (which would also help alert users to problems like what is in the summary for this patch) instead of treating it as a region.

[dexonsmith]

clang/test/Sema/pragma-attribute-label.c

15

Sounds great; thanks Aaron.

Another idea I had over lunch was #pragma clang attribute IDENTIFIER push/pop, making it clear that each identifier has an independent stack.

[erik.pilkington]

After looking through some users of #pragma clang attribute, I don't think that the begin/end solution is what we want here. Some users of this attribute write macros that can expand to different attributes depending on their arguments, for instance:

AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
AVAILABILITY_BEGIN(ios(10))
// some code...
AVAILABILITY_END
AVAILABILITY_END

This code makes sense and is safe, but in this case rewriting AVAILABILITY_BEGIN to use a hypothetical pragma clang attribute begin/end would be a breaking change, which isn't acceptable. So I think that we want stack semantics, but scoped within the AVAILABILITY_BEGIN macro's namespace. That way, we can support multiple pushes in the same macro, without having having different macros accidentally pop each other.

As for a syntax for this, I chose the following (basically, @dexonsmith's idea with a '.'):

#pragma clang attribute NoReturn.push (__attribute__((noreturn)), apply_to=function)
int foo();
#pragma clang attribute NoReturn.pop

I think the '.' makes the nested relationship (i.e. the push is contained within the namespace) more clear to C programmers, and hopefully clears up the confusion. @AaronBallman: WDYT?

[erik.pilkington]

Just to be clear, this is just a syntactic change from what I originally posted. The old #pragma clang attribute push NoReturn (...) is semantically equivalent to the new #pragma clang attribute NoReturn.push (...)

[aaron.ballman]

In D55628#1335841, @erik.pilkington wrote:

After looking through some users of #pragma clang attribute, I don't think that the begin/end solution is what we want here. Some users of this attribute write macros that can expand to different attributes depending on their arguments, for instance:

AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
AVAILABILITY_BEGIN(ios(10))
// some code...
AVAILABILITY_END
AVAILABILITY_END

This code makes sense and is safe, but in this case rewriting AVAILABILITY_BEGIN to use a hypothetical pragma clang attribute begin/end would be a breaking change, which isn't acceptable.

Good catch!

So I think that we want stack semantics, but scoped within the AVAILABILITY_BEGIN macro's namespace. That way, we can support multiple pushes in the same macro, without having having different macros accidentally pop each other.

As for a syntax for this, I chose the following (basically, @dexonsmith's idea with a '.'):

#pragma clang attribute NoReturn.push (__attribute__((noreturn)), apply_to=function)
int foo();
#pragma clang attribute NoReturn.pop

I think the '.' makes the nested relationship (i.e. the push is contained within the namespace) more clear to C programmers, and hopefully clears up the confusion. @AaronBallman: WDYT?

This is definitely a novel syntax, but I like how clear it is that the label acts as the scope being pushed and popped. One question I have is with interactions from unlabeled pushes and pops:

#pragma clang attribute MyNamespace.push (__attribute__((annotate)), apply_to=function)
#pragma clang attribute push (__attribute__((annotate)), apply_to=function)

int some_func1();

#pragma clang attribute pop // does this pop the unlabeled push, or is it an error?
#pragma clang attribute MyNamespace.pop

Or, similarly:

#pragma clang attribute push (__attribute__((annotate)), apply_to=function)
#pragma clang attribute MyNamespace.push (__attribute__((annotate)), apply_to=function)

int some_func2();

#pragma clang attribute pop // does this pop the unlabeled push, or is it an error?
#pragma clang attribute MyNamespace.pop

I suspect the answer is that it pops the unlabeled push and is not an error, but I'd like to see a test case with it.

clang/docs/LanguageExtensions.rst

2700

Somewhere around here you should probably mention that an unlabeled pop does *not* act like a "pop any namespace" action but instead acts as a "pop the last unlabeled namespace" action. Some users may find that behavior surprising otherwise.

2709–2714

I think it would be useful for the example to show the problematic situation this feature is meant to avoid.

clang/lib/Parse/ParsePragma.cpp

3161

Can you reuse diag::err_expected_after instead of making a new diagnostic?

[erik.pilkington]

Add Aaron's testcase, improve the documentation a bit.

clang/lib/Parse/ParsePragma.cpp

3161

I was kinda concerned about how we would diagnose a case like this:

#pragma clang attribute add (...) // add isn't a thing!

A generic diagnostic about the missing . would be pretty unhelpful. The custom diagnostic reads as "expected '.' after pragma attribute namespace 'add'", which makes how the parser interpreted the code a lot more clear.

[aaron.ballman]

LGTM!

clang/lib/Parse/ParsePragma.cpp

3161

Okay, that's a good reason to go with a separate diagnostic, thanks!

clang/docs/LanguageExtensions.rst

 multiple declarations. The ``#pragma clang attribute push`` variation of the
 directive pushes a new "scope" of ``#pragma clang attribute`` that attributes
 can be added to. The ``#pragma clang attribute (...)`` variation adds an
 attribute to that scope, and the ``#pragma clang attribute pop`` variation pops
 the scope. You can also use ``#pragma clang attribute push (...)``, which is a
 shorthand for when you want to add one attribute to a new scope. Multiple push
 directives can be nested inside each other.
 
+Because multiple push directives can be nested, if you're writing a macro that
+expands to ``_Pragma("clang attribute")`` it's good hygiene to add a label to
+your push/pop directives. A pop directive with a label will pop the innermost
+push that has that same label. This will ensure that another macro's ``pop``
+won't inadvertently pop your attribute.
+
 The attributes that are used in the ``#pragma clang attribute`` directives
 can be written using the GNU-style syntax:
 
 .. code-block:: c++
 
-  #pragma clang attribute push (__attribute__((annotate("custom"))), apply_to = function)
+  #pragma clang attribute push ANNOTATE_CUSTOM (__attribute__((annotate("custom"))), apply_to = function)
 
   void function(); // The function now has the annotate("custom") attribute
 
-  #pragma clang attribute pop
+  #pragma clang attribute pop ANNOTATE_CUSTOM
 
 The attributes can also be written using the C++11 style syntax:
 
 .. code-block:: c++
 
-  #pragma clang attribute push ([[noreturn]], apply_to = function)
+  #pragma clang attribute push NO_RETURN ([[noreturn]], apply_to = function)
 
   void function(); // The function now has the [[noreturn]] attribute
 
-  #pragma clang attribute pop
+  #pragma clang attribute pop NO_RETURN
 
 The ``__declspec`` style syntax is also supported:
 
 .. code-block:: c++
 
-  #pragma clang attribute push (__declspec(dllexport), apply_to = function)
+  #pragma clang attribute push DLL_EXP (__declspec(dllexport), apply_to = function)
 
   void function(); // The function now has the __declspec(dllexport) attribute
 
-  #pragma clang attribute pop
+  #pragma clang attribute pop DLL_EXP
 
 A single push directive accepts only one attribute regardless of the syntax
 used.
 
 Subject Match Rules
 -------------------
 
 The set of declarations that receive a single attribute from the attribute stack
 depends on the subject match rules that were specified in the pragma. Subject
 match rules are specified after the attribute. The compiler expects an
 identifier that corresponds to the subject set specifier. The ``apply_to``
 specifier is currently the only supported subject set specifier. It allows you
 to specify match rules that form a subset of the attribute's allowed subject
 set, i.e. the compiler doesn't require all of the attribute's subjects. For
 example, an attribute like ``[[nodiscard]]`` whose subject set includes
 ``enum``, ``record`` and ``hasType(functionType)``, requires the presence of at
 least one of these rules after ``apply_to``:
 
 .. code-block:: c++
 
   #pragma clang attribute push([[nodiscard]], apply_to = enum)

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "redundant attribute subject matcher sub-rule '%0'; '%1' already matches "
   "those declarations">;
 def err_pragma_attribute_matcher_negated_subrule_contradicts_subrule : Error<
   "negated attribute subject matcher sub-rule '%0' contradicts sub-rule '%1'">;
 def err_pragma_attribute_invalid_matchers : Error<
   "attribute %0 can't be applied to %1">;
 def err_pragma_attribute_stack_mismatch : Error<
   "'#pragma clang attribute pop' with no matching '#pragma clang attribute push'">;
+def err_pragma_attribute_stack_mismatch_label : Error<
+  "'#pragma clang attribute pop %0' with no matching "
+  "'#pragma clang attribute push %0'">;
 def warn_pragma_attribute_unused : Warning<
   "unused attribute %0 in '#pragma clang attribute push' region">,
   InGroup<PragmaClangAttribute>;
 def note_pragma_attribute_region_ends_here : Note<
   "'#pragma clang attribute push' regions ends here">;
 def err_pragma_attribute_no_pop_eof : Error<"unterminated "
   "'#pragma clang attribute push' at end of file">;
 def note_pragma_attribute_applied_decl_here : Note<

clang/include/clang/Sema/Sema.h

     SmallVector<attr::SubjectMatchRule, 4> MatchRules;
     bool IsUsed;
   };
 
   /// A push'd group of PragmaAttributeEntries.
   struct PragmaAttributeGroup {
     /// The location of the push attribute.
     SourceLocation Loc;
+    /// The label of this push group.
+    IdentifierInfo *Label;
     SmallVector<PragmaAttributeEntry, 2> Entries;
   };
 
   SmallVector<PragmaAttributeGroup, 2> PragmaAttributeStack;
 
   /// The declaration that is currently receiving an attribute from the
   /// #pragma attribute stack.
   const Decl *PragmaAttributeCurrentTargetDecl;

   /// AddCFAuditedAttribute - Check whether we're currently within
   /// '\#pragma clang arc_cf_code_audited' and, if so, consider adding
   /// the appropriate attribute.
   void AddCFAuditedAttribute(Decl *D);
 
   void ActOnPragmaAttributeAttribute(ParsedAttr &Attribute,
                                      SourceLocation PragmaLoc,
                                      attr::ParsedSubjectMatchRuleSet Rules);
-  void ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc);
+  void ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
+                                     IdentifierInfo *PushLabel);
 
   /// Called on well-formed '\#pragma clang attribute pop'.
-  void ActOnPragmaAttributePop(SourceLocation PragmaLoc);
+  void ActOnPragmaAttributePop(SourceLocation PragmaLoc,
+                               IdentifierInfo *PushLabel);
 
   /// Adds the attributes that have been specified using the
   /// '\#pragma clang attribute push' directives to the given declaration.
   void AddPragmaAttributes(Scope *S, Decl *D);
 
   void DiagnoseUnterminatedPragmaAttribute();
 
   /// Called on well formed \#pragma clang optimize.

clang/lib/Parse/ParsePragma.cpp

   return true;
 }
 
 namespace {
 struct PragmaAttributeInfo {
   enum ActionType { Push, Pop, Attribute };
   ParsedAttributes &Attributes;
   ActionType Action;
+  IdentifierInfo *PushLabel = nullptr;
   ArrayRef<Token> Tokens;
 
   PragmaAttributeInfo(ParsedAttributes &Attributes) : Attributes(Attributes) {}
 };
 
 #include "clang/Parse/AttrSubMatchRulesParserStringSwitches.inc"
 
 } // end anonymous namespace

 
 void Parser::HandlePragmaAttribute() {
   assert(Tok.is(tok::annot_pragma_attribute) &&
          "Expected #pragma attribute annotation token");
   SourceLocation PragmaLoc = Tok.getLocation();
   auto *Info = static_cast<PragmaAttributeInfo *>(Tok.getAnnotationValue());
   if (Info->Action == PragmaAttributeInfo::Pop) {
     ConsumeAnnotationToken();
-    Actions.ActOnPragmaAttributePop(PragmaLoc);
+    Actions.ActOnPragmaAttributePop(PragmaLoc, Info->PushLabel);
     return;
   }
   // Parse the actual attribute with its arguments.
   assert((Info->Action == PragmaAttributeInfo::Push ||
           Info->Action == PragmaAttributeInfo::Attribute) &&
          "Unexpected #pragma attribute command");
 
   if (Info->Action == PragmaAttributeInfo::Push && Info->Tokens.empty()) {
     ConsumeAnnotationToken();
-    Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc);
+    Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc, Info->PushLabel);
     return;
   }
 
   PP.EnterTokenStream(Info->Tokens, /*DisableMacroExpansion=*/false);
   ConsumeAnnotationToken();
 
   ParsedAttributes &Attrs = Info->Attributes;
   Attrs.clearListOnly();

     return;
   }
 
   // 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);
+    Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc, Info->PushLabel);
 
   Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
                                         std::move(SubjectMatchRules));
 }
 
 // #pragma GCC visibility comes in two variants:
 //   'push' '(' [visibility] ')'
 //   'pop'

     const IdentifierInfo *II = Tok.getIdentifierInfo();
     if (II->isStr("push"))
       Info->Action = PragmaAttributeInfo::Push;
     else if (II->isStr("pop"))
       Info->Action = PragmaAttributeInfo::Pop;
     else {
       PP.Diag(Tok.getLocation(), diag::err_pragma_attribute_invalid_argument)
           << PP.getSpelling(Tok);
       return;
     }
 
     PP.Lex(Tok);
   }
 
+  // Parse the optional push/pop label.
+  if ((Info->Action == PragmaAttributeInfo::Push ||
+       Info->Action == PragmaAttributeInfo::Pop) &&
+      Tok.is(tok::identifier)) {
+    Info->PushLabel = Tok.getIdentifierInfo();
+    PP.Lex(Tok);
+  }
+
   // Parse the actual attribute.
   if ((Info->Action == PragmaAttributeInfo::Push && Tok.isNot(tok::eod)) ||
       Info->Action == PragmaAttributeInfo::Attribute) {
     if (Tok.isNot(tok::l_paren)) {
       PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
       return;
     }
     PP.Lex(Tok);

clang/lib/Sema/SemaAttr.cpp

     Diag(PragmaLoc, diag::err_pragma_attr_attr_no_push);
     return;
   }
 
   PragmaAttributeStack.back().Entries.push_back(
       {PragmaLoc, &Attribute, std::move(SubjectMatchRules), /*IsUsed=*/false});
 }
 
-void Sema::ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc) {
+void Sema::ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
+                                         IdentifierInfo *PushLabel) {
   PragmaAttributeStack.emplace_back();
   PragmaAttributeStack.back().Loc = PragmaLoc;
+  PragmaAttributeStack.back().Label = PushLabel;
 }
 
-void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc) {
+void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc,
+                                   IdentifierInfo *PushLabel) {
   if (PragmaAttributeStack.empty()) {
     Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch);
     return;
   }
 
-  for (const PragmaAttributeEntry &Entry :
-       PragmaAttributeStack.back().Entries) {
-    if (!Entry.IsUsed) {
-      assert(Entry.Attribute && "Expected an attribute");
-      Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
-          << Entry.Attribute->getName();
-      Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+  // Dig back through the stack trying to find the group that corrisponds to
+  // PushLabel. Note that this works fine if no label is present, think of
+  // unlabeled push/pops as having an implicit "nullptr" label.
+  for (size_t Index = PragmaAttributeStack.size(); Index;) {
+    --Index;
+    if (PragmaAttributeStack[Index].Label == PushLabel) {
+      for (const PragmaAttributeEntry &Entry :
+           PragmaAttributeStack[Index].Entries) {
+        if (!Entry.IsUsed) {
+          assert(Entry.Attribute && "Expected an attribute");
+          Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
+              << Entry.Attribute->getName();
+          Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+        }
+      }
+      PragmaAttributeStack.erase(PragmaAttributeStack.begin() + Index);
+      return;
     }
   }
 
-  PragmaAttributeStack.pop_back();
+  if (PushLabel)
+    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch_label)
+        << PushLabel->getName();
+  else
+    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch);
 }
 
 void Sema::AddPragmaAttributes(Scope *S, Decl *D) {
   if (PragmaAttributeStack.empty())
     return;
   for (auto &Group : PragmaAttributeStack) {
     for (auto &Entry : Group.Entries) {
       ParsedAttr *Attribute = Entry.Attribute;

clang/test/Sema/pragma-attribute-label.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#pragma clang attribute push MY_LABEL (__attribute__((annotate)), apply_to=function) // expected-error 2 {{'annotate' attribute}}
+
+int some_func(); // expected-note{{when applied to this declaration}}
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push NOT_MY_LABEL'}}
+
+#pragma clang attribute push MY_OTHER_LABEL (__attribute__((annotate)), apply_to=function) // expected-error 2 {{'annotate' attribute}}
+
+int some_other_func(); // expected-note 2 {{when applied to this declaration}}
+
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+
+int some_other_other_func(); // expected-note 1 {{when applied to this declaration}}
+
+#pragma clang attribute pop MY_OTHER_LABEL