erik.pilkington created this revision.
erik.pilkington added reviewers: arphaman, aaron.ballman.
Herald added subscribers: dexonsmith, jkorous.
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 pop`s with a label will only pop a `push`ed 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
Repository:
rC Clang
https://reviews.llvm.org/D55628
Files:
clang/docs/LanguageExtensions.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Parse/ParsePragma.cpp
clang/lib/Sema/SemaAttr.cpp
clang/test/Sema/pragma-attribute-label.c
Index: clang/test/Sema/pragma-attribute-label.c
===================================================================
--- /dev/null
+++ clang/test/Sema/pragma-attribute-label.c
@@ -0,0 +1,19 @@
+// 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
Index: clang/lib/Sema/SemaAttr.cpp
===================================================================
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -631,28 +631,45 @@
{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) {
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;
+ IdentifierInfo *PushLabel = 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->PushLabel);
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->PushLabel);
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->PushLabel);
Actions.ActOnPragmaAttributeAttribute(Attribute, PragmaLoc,
std::move(SubjectMatchRules));
@@ -3163,6 +3164,14 @@
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) {
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 label of this push group.
+ IdentifierInfo *Label;
SmallVector<PragmaAttributeEntry, 2> Entries;
};
@@ -8498,10 +8500,12 @@
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.
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -837,6 +837,9 @@
"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>;
Index: clang/docs/LanguageExtensions.rst
===================================================================
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -2663,36 +2663,42 @@
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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
