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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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

clang/include/clang/Sema/Sema.h

8504 ↗

(On Diff #177971)

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

clang/lib/Sema/SemaAttr.cpp

648 ↗

(On Diff #177971)

corrisponds -> corresponds

659 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 Diff #177971)

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 ↗

(On Diff #177971)

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 Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

Sure, good point.

[dexonsmith]

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

7 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #177971)

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 ↗

(On Diff #178837)

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 ↗

(On Diff #178837)

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 ↗

(On Diff #178837)

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 ↗

(On Diff #178837)

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 ↗

(On Diff #178837)

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

docs/LanguageExtensions.rst

.. code-block:: c++
 
   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.
 
+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
+same namespace. For instance:
+
+.. code-block:: c++
+
+   #define ASSUME_NORETURN_BEGIN _Pragma("clang attribute AssumeNoreturn.push ([[noreturn]], apply_to = function)")
+   #define ASSUME_NORETURN_END   _Pragma("clang attribute AssumeNoreturn.pop")
+
+   #define ASSUME_UNAVAILABLE_BEGIN _Pragma("clang attribute Unavailable.push (__attribute__((unavailable)), apply_to=function)")
+   #define ASSUME_UNAVAILABLE_END   _Pragma("clang attribute Unavailable.pop")
+
+
+   ASSUME_NORETURN_BEGIN
+   ASSUME_UNAVAILABLE_BEGIN
+   void function(); // function has [[noreturn]] and __attribute__((unavailable))
+   ASSUME_NORETURN_END
+   void other_function(); // function has __attribute__((unavailable))
+   ASSUME_UNAVAILABLE_END
+
+Without the namespaces on the macros, ``other_function`` will be annotated with
+``[[noreturn]]`` instead of ``__attribute__((unavailable))``. This may seem like
+a contrived example, but its very possible for this kind of situation to appear
+in real code if the pragmas are spread out accross a large file.
+
 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

include/clang/Basic/DiagnosticParseKinds.td

def err_pragma_attribute_expected_subject_sub_identifier : Error<
   "sub-rule; '%0' matcher %select{does not support sub-rules|supports the "
   "following sub-rules: %2|}1">;
 def err_pragma_attribute_unknown_subject_sub_rule : Error<
   "%select{invalid use of|unknown}2 attribute subject matcher sub-rule '%0'; "
   "'%1' matcher %select{does not support sub-rules|supports the following "
   "sub-rules: %3}2">;
 def err_pragma_attribute_duplicate_subject : Error<
   "duplicate attribute subject matcher '%0'">;
+def err_pragma_attribute_expected_period : Error<
+  "expected '.' after pragma attribute namespace %0">;
+def err_pragma_attribute_namespace_on_attribute : Error<
+  "namespace can only apply to 'push' or 'pop' directives">;
+def note_pragma_attribute_namespace_on_attribute : Note<
+  "omit the namespace to add attributes to the most-recently"
+  " pushed attribute group">;
 
 def err_opencl_unroll_hint_on_non_loop : Error<
   "OpenCL only supports 'opencl_unroll_hint' attribute on for, while, and do statements">;
 
 // OpenCL EXTENSION pragma (OpenCL 1.1 [9.1])
 def warn_pragma_expected_colon : Warning<
   "missing ':' after %0 - ignoring">, InGroup<IgnoredPragmas>;
 def warn_pragma_expected_predicate : Warning<

include/clang/Basic/DiagnosticSemaKinds.td

 def err_pragma_attribute_matcher_subrule_contradicts_rule : Error<
   "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'">;
+  "'#pragma clang attribute %select{%1.|}0pop' with no matching"
+  " '#pragma clang attribute %select{%1.|}0push'">;
 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<

include/clang/Sema/Sema.h

struct PragmaAttributeEntry {
     SmallVector<attr::SubjectMatchRule, 4> MatchRules;
     bool IsUsed;
   };
 
   /// A push'd group of PragmaAttributeEntries.
   struct PragmaAttributeGroup {
     /// The location of the push attribute.
     SourceLocation Loc;
+    /// The namespace of this push group.
+    const IdentifierInfo *Namespace;
     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,
+                                     const IdentifierInfo *Namespace);
 
   /// Called on well-formed '\#pragma clang attribute pop'.
-  void ActOnPragmaAttributePop(SourceLocation PragmaLoc);
+  void ActOnPragmaAttributePop(SourceLocation PragmaLoc,
+                               const IdentifierInfo *Namespace);
 
   /// 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.

lib/Parse/ParsePragma.cpp

bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
   return true;
 }
 
 namespace {
 struct PragmaAttributeInfo {
   enum ActionType { Push, Pop, Attribute };
   ParsedAttributes &Attributes;
   ActionType Action;
+  const IdentifierInfo *Namespace = 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->Namespace);
     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->Namespace);
     return;
   }
 
   PP.EnterTokenStream(Info->Tokens, /*DisableMacroExpansion=*/false);
   ConsumeAnnotationToken();
 
   ParsedAttributes &Attrs = Info->Attributes;
   Attrs.clearListOnly();
if (Tok.isNot(tok::eof)) {
     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->Namespace);
 
   Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
                                         std::move(SubjectMatchRules));
 }
 
 // #pragma GCC visibility comes in two variants:
 //   'push' '(' [visibility] ')'
 //   'pop'
if (!Tok.is(tok::eod))
     PP.Diag(FirstTok.getLocation(),
             diag::warn_pragma_force_cuda_host_device_bad_arg);
 }
 
 /// Handle the #pragma clang attribute directive.
 ///
 /// The syntax is:
 /// \code
-///  #pragma clang attribute push(attribute, subject-set)
+///  #pragma clang attribute push (attribute, subject-set)
 ///  #pragma clang attribute push
 ///  #pragma clang attribute (attribute, subject-set)
 ///  #pragma clang attribute pop
 /// \endcode
 ///
+/// There are also 'namespace' variants of push and pop directives. The bare
+/// '#pragma clang attribute (attribute, subject-set)' version doesn't require a
+/// namespace, since it always applies attributes to the most recently pushed
+/// group, regardless of namespace.
+/// \code
+///  #pragma clang attribute namespace.push (attribute, subject-set)
+///  #pragma clang attribute namespace.push
+///  #pragma clang attribute namespace.pop
+/// \endcode
+///
 /// The subject-set clause defines the set of declarations which receive the
 /// attribute. Its exact syntax is described in the LanguageExtensions document
 /// in Clang's documentation.
 ///
 /// This directive instructs the compiler to begin/finish applying the specified
 /// attribute to the set of attribute-specific declarations in the active range
 /// of the pragma.
 void PragmaAttributeHandler::HandlePragma(Preprocessor &PP,
                                           PragmaIntroducerKind Introducer,
                                           Token &FirstToken) {
   Token Tok;
   PP.Lex(Tok);
   auto *Info = new (PP.getPreprocessorAllocator())
       PragmaAttributeInfo(AttributesForPragmaAttribute);
 
+  // Parse the optional namespace followed by a period.
+  if (Tok.is(tok::identifier)) {
+    IdentifierInfo *II = Tok.getIdentifierInfo();
+    if (!II->isStr("push") && !II->isStr("pop")) {
+      Info->Namespace = II;
+      PP.Lex(Tok);
+
+      if (!Tok.is(tok::period)) {
+        PP.Diag(Tok.getLocation(), diag::err_pragma_attribute_expected_period)
+            << II;
+        return;
+      }
+      PP.Lex(Tok);
+    }
+  }
+
   if (!Tok.isOneOf(tok::identifier, tok::l_paren)) {
     PP.Diag(Tok.getLocation(),
             diag::err_pragma_attribute_expected_push_pop_paren);
     return;
   }
 
   // Determine what action this pragma clang attribute represents.
-  if (Tok.is(tok::l_paren))
+  if (Tok.is(tok::l_paren)) {
+    if (Info->Namespace) {
+      PP.Diag(Tok.getLocation(),
+              diag::err_pragma_attribute_namespace_on_attribute);
+      PP.Diag(Tok.getLocation(),
+              diag::note_pragma_attribute_namespace_on_attribute);
+      return;
+    }
     Info->Action = PragmaAttributeInfo::Attribute;
-  else {
+  } else {
     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);

lib/Sema/SemaAttr.cpp

if (PragmaAttributeStack.empty()) {
     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,
+                                         const IdentifierInfo *Namespace) {
   PragmaAttributeStack.emplace_back();
   PragmaAttributeStack.back().Loc = PragmaLoc;
+  PragmaAttributeStack.back().Namespace = Namespace;
 }
 
-void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc) {
+void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc,
+                                   const IdentifierInfo *Namespace) {
   if (PragmaAttributeStack.empty()) {
-    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch);
+    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch) << 1;
     return;
   }
 
+  // Dig back through the stack trying to find the most recently pushed group
+  // that in Namespace. Note that this works fine if no namespace is present,
+  // think of push/pops without namespaces as having an implicit "nullptr"
+  // namespace.
+  for (size_t Index = PragmaAttributeStack.size(); Index;) {
+    --Index;
+    if (PragmaAttributeStack[Index].Namespace == Namespace) {
       for (const PragmaAttributeEntry &Entry :
-       PragmaAttributeStack.back().Entries) {
+           PragmaAttributeStack[Index].Entries) {
         if (!Entry.IsUsed) {
           assert(Entry.Attribute && "Expected an attribute");
           Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
-          << Entry.Attribute->getName();
+              << *Entry.Attribute;
           Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
         }
       }
+      PragmaAttributeStack.erase(PragmaAttributeStack.begin() + Index);
+      return;
+    }
+  }
 
-  PragmaAttributeStack.pop_back();
+  if (Namespace)
+    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch)
+        << 0 << Namespace->getName();
+  else
+    Diag(PragmaLoc, diag::err_pragma_attribute_stack_mismatch) << 1;
 }
 
 void Sema::AddPragmaAttributes(Scope *S, Decl *D) {
   if (PragmaAttributeStack.empty())
     return;
   for (auto &Group : PragmaAttributeStack) {
     for (auto &Entry : Group.Entries) {
       ParsedAttr *Attribute = Entry.Attribute;

test/Parser/pragma-attribute.cpp

 
 #pragma clang attribute push(__attribute__((annotate("test"))), apply_to = any( variable(is_global), variable(is_global) )) // expected-error {{duplicate attribute subject matcher 'variable(is_global)'}}
 #pragma clang attribute push(__attribute__((annotate("test"))), apply_to = any( variable(is_global), function, variable(is_global), variable(is_global) )) // expected-error 2 {{duplicate attribute subject matcher 'variable(is_global)'}}
 #pragma clang attribute push(__attribute__((annotate("test"))), apply_to = any( variable(unless(is_parameter)), variable(unless(is_parameter)) )) // expected-error {{duplicate attribute subject matcher 'variable(unless(is_parameter))'}}
 #pragma clang attribute push(__attribute__((annotate("test"))), apply_to = any( variable(unless(is_parameter)), variable(unless(is_parameter)), enum, variable(unless(is_parameter)) )) // expected-error 2 {{duplicate attribute subject matcher 'variable(unless(is_parameter))'}}
 
 #pragma clang attribute // expected-error {{expected 'push', 'pop', or '(' after '#pragma clang attribute'}}
 #pragma clang attribute 42 // expected-error {{expected 'push', 'pop', or '(' after '#pragma clang attribute'}}
-#pragma clang attribute pushpop // expected-error {{unexpected argument 'pushpop' to '#pragma clang attribute'; expected 'push' or 'pop'}}
+#pragma clang attribute pushpop // expected-error {{expected '.' after pragma attribute namespace 'pushpop'}}
 
 #pragma clang attribute push
 #pragma clang attribute pop
 #pragma clang attribute push ( // expected-error {{expected an attribute after '('}}
 #pragma clang attribute push (__attribute__((annotate)) // expected-error {{expected ')'}}
 #pragma clang attribute push () // expected-error {{expected an attribute after '('}}
 
 #pragma clang attribute push (__attribute__((annotate("test"))), apply_to = function) () // expected-warning {{extra tokens at end of '#pragma clang attribute'}}

test/Sema/pragma-attribute-namespace.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#pragma clang attribute MyNamespace.push (__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 NotMyNamespace.pop // expected-error{{'#pragma clang attribute NotMyNamespace.pop' with no matching '#pragma clang attribute NotMyNamespace.push'}}
+
+#pragma clang attribute MyOtherNamespace.push (__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 MyNamespace.pop
+
+int some_other_other_func(); // expected-note 1 {{when applied to this declaration}}
+
+#pragma clang attribute MyOtherNamespace.pop
+
+#pragma clang attribute Misc. () // expected-error{{namespace can only apply to 'push' or 'pop' directives}} expected-note {{omit the namespace to add attributes to the most-recently pushed attribute group}}
+
+#pragma clang attribute Misc push // expected-error{{expected '.' after pragma attribute namespace 'Misc'}}
+
+// Test how pushes with namespaces interact with pushes without namespaces.
+
+#pragma clang attribute Merp.push (__attribute__((annotate)), apply_to=function) // expected-error{{'annotate' attribute}}
+#pragma clang attribute push (__attribute__((annotate)), apply_to=function) // expected-warning {{unused attribute}}
+#pragma clang attribute pop // expected-note{{ends here}}
+int test(); // expected-note{{when applied to this declaration}}
+#pragma clang attribute Merp.pop
+
+#pragma clang attribute push (__attribute__((annotate)), apply_to=function) // expected-warning {{unused attribute}}
+#pragma clang attribute Merp.push (__attribute__((annotate)), apply_to=function) // expected-error{{'annotate' attribute}}
+#pragma clang attribute pop // expected-note{{ends here}}
+int test2(); // expected-note{{when applied to this declaration}}
+#pragma clang attribute Merp.pop