erik.pilkington updated this revision to Diff 178837.
erik.pilkington added a comment.
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 `push`es 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?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55628/new/
https://reviews.llvm.org/D55628
Files:
clang/docs/LanguageExtensions.rst
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Parse/ParsePragma.cpp
clang/lib/Sema/SemaAttr.cpp
clang/test/Parser/pragma-attribute.cpp
clang/test/Sema/pragma-attribute-namespace.c
Index: clang/test/Sema/pragma-attribute-namespace.c
===================================================================
--- /dev/null
+++ clang/test/Sema/pragma-attribute-namespace.c
@@ -0,0 +1,23 @@
+// 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'}}
Index: clang/test/Parser/pragma-attribute.cpp
===================================================================
--- clang/test/Parser/pragma-attribute.cpp
+++ clang/test/Parser/pragma-attribute.cpp
@@ -102,7 +102,7 @@
#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
Index: clang/lib/Sema/SemaAttr.cpp
===================================================================
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -631,28 +631,46 @@
{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;
}
- 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 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[Index].Entries) {
+ if (!Entry.IsUsed) {
+ assert(Entry.Attribute && "Expected an attribute");
+ Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
+ << *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) {
Index: clang/lib/Parse/ParsePragma.cpp
===================================================================
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -1139,6 +1139,7 @@
enum ActionType { Push, Pop, Attribute };
ParsedAttributes &Attributes;
ActionType Action;
+ const IdentifierInfo *Namespace = nullptr;
ArrayRef<Token> Tokens;
PragmaAttributeInfo(ParsedAttributes &Attributes) : Attributes(Attributes) {}
@@ -1393,7 +1394,7 @@
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.
@@ -1403,7 +1404,7 @@
if (Info->Action == PragmaAttributeInfo::Push && Info->Tokens.empty()) {
ConsumeAnnotationToken();
- Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc);
+ Actions.ActOnPragmaAttributeEmptyPush(PragmaLoc, Info->Namespace);
return;
}
@@ -1555,7 +1556,7 @@
// 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));
@@ -3118,12 +3119,22 @@
///
/// 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.
@@ -3139,6 +3150,22 @@
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);
@@ -3146,9 +3173,16 @@
}
// 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;
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -503,6 +503,8 @@
struct PragmaAttributeGroup {
/// The location of the push attribute.
SourceLocation Loc;
+ /// The namespace of this push group.
+ const IdentifierInfo *Namespace;
SmallVector<PragmaAttributeEntry, 2> Entries;
};
@@ -8494,10 +8496,12 @@
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.
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -815,7 +815,8 @@
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>;
Index: clang/include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticParseKinds.td
+++ clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1100,6 +1100,13 @@
"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">;
Index: clang/docs/LanguageExtensions.rst
===================================================================
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -2697,6 +2697,22 @@
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. 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")
+
+ ASSUME_NORETURN_BEGIN
+ void function(); // function has [[noreturn]]
+ ASSUME_NORETURN_END
+
Subject Match Rules
-------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
