Add Global support for #pragma clang attributes

[ashi1]

Summary

Allow the user to use #pragma clang attribute without
requiring corresponding push/pop. This aligns better with
pragma diagnostics, where the usage of namespace push/pop
is optional. The user is expected to be aware of included
files that change the namespace attribute. Also, change
unterminated push error into a warning. This aligns with
other unbalanced push/pop pragmas.

[tra]

An alternative proposed in https://reviews.llvm.org/D98201 is to allow unpopped pragmas by default with a warning.

I like the no_pop because it's explicit, but being permissive, with a warning, by default would indeed be more consistent with the way other pragmas behave. E.g. #pragma push_macro().
I'm OK with both approaches.

@aaron.ballman , @arphaman -- WDYT?

[palves]

Back in https://reviews.llvm.org/D98201 we had a larger discussion where I gave my rationale for why I thought that Clang should instead not error out when the pop is missing, so I'm not going to repeat it here.

Looking deeper at the description of the pragma at https://clang.llvm.org/docs/LanguageExtensions.html#specifying-an-attribute-for-multiple-declarations-pragma-clang-attribute , I have to wonder though, if instead of adding this new odd "no_pop" syntax (basically looks like a magic namespace), we shouldn't instead let the syntax without "push" at all work before any namespace was pushed.

The docs say:

Specifying an attribute for multiple declarations (#pragma clang attribute)

The #pragma clang attribute directive can be used to apply an attribute to 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.

So the "#pragma clang attribute (...)" syntax is already supported, and it works if you've already opened a new scope with "push" before. Like:

#pragma clang attribute push
#pragma clang attribute (__attribute__((annotate("custom"))), apply_to = function)
#pragma clang attribute pop

But it doesn't work if you haven't pushed a new scope before:

$ cat clang-attribute.cc
#pragma clang attribute (__attribute__((annotate("custom"))), apply_to = function)

$ clang clang-attribute.cc -c
clang-attribute.cc:1:15: error: '#pragma clang attribute' attribute with no matching '#pragma clang attribute push'
#pragma clang attribute (__attribute__((annotate("custom"))), apply_to = function)
              ^
1 error generated.

Maybe it should work? As if there was a "global scope"?

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

WDYT?

(I totally missed that the "#pragma clang attribute (...)" syntax exists already until now, because the documentation does not include an example using it AFAICS. It would be nice to fix that.

[ashi1]

Ping arphaman and aaron.ballman. Also adding @erik.pilkington, who introduced namespaces to clang attribute, for opinions.

For me, I am okay with either having a global scope, adding no_pop namespace, or trying to remove balanced push/pop requirement.

[ashi1]

Ping

If no additional opinions, I think this current patch with no_pop serves the purpose and can be committed, while we discuss if we want to add global scope. @palves, does that work for you?

[palves]

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

[yaxunl]

You get pre-merge failures due to format issues. Better clean them up.

[ashi1]

Fixed formatting issues.

[yaxunl]

there are still pre-merge failures. you may need update your test

[ashi1]

Fix unrelated pre-merge test failures. Something changed upstream, unrelated to this patch.

[t-tye]

In D100404#2701630, @palves wrote:

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

I agree with @palves that this seems the least consistent of the options. Is there an objection to to the more consistent and more intuitive alternative?

[erik.pilkington]

In D100404#2686753, @palves wrote:

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

Hmm, the problem I see with that is that you have to know that nobody else push-ed a #pca scope that you're inadvertently adding to. I'd prefer having a syntax that always added to the "global scope" regardless of the #pca context, which is what no_pop is effectively doing. I think I'd also be okay with downgrading the EOF error into a warning (which you could suppress in the -include file), assuming that we still warned when the #pca comes from a macro declared in a system header.

clang/lib/Parse/ParsePragma.cpp

3561

Would it be possible to check if the namespace is no_pop in Sema? It seems like this would be simpler if you don't touch the parser at all.

clang/lib/Sema/SemaAttr.cpp

951

I think there is a bug here with the existing patch:

#pragma clang attribute push ( __attribute__((whatever)), apply_to=whoever )
#pragma clang attribute no_pop.push ( __attribute__((whatever)), apply_to=whoever )
#pragma clang attribute pop

It looks to me like the pop on line 3 will pop off the no_pop, since they both have a nullptr namespace.

1019

ISTM that this will fail to emit the error on the following:

#pragma clang attribute my_ns.push (__attribute__((whatever)), apply_to=whoever)
#pragma clang attribute no_pop.push (__attribute__((whatever)), apply_to=whoever)

I think you should walk back through the stack and see if any of the entities are not no_pop.

[tra]

In D100404#2701902, @t-tye wrote:

In D100404#2701630, @palves wrote:

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

I agree with @palves that this seems the least consistent of the options. Is there an objection to to the more consistent and more intuitive alternative?

OK. Scratch the no_pop. While it does the job, the syntax is indeed odd. Functionality wise, it's equivalent to using #pragma clang attribute (...) that @palves has suggested, which is a better way to express the intent.

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

I'd be fine with that. Making push w/o pop a warning would also work.

[ashi1]

In D100404#2701988, @erik.pilkington wrote:

In D100404#2686753, @palves wrote:

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

Hmm, the problem I see with that is that you have to know that nobody else push-ed a #pca scope that you're inadvertently adding to. I'd prefer having a syntax that always added to the "global scope" regardless of the #pca context, which is what no_pop is effectively doing. I think I'd also be okay with downgrading the EOF error into a warning (which you could suppress in the -include file), assuming that we still warned when the #pca comes from a macro declared in a system header.

Based on Erik's concerns, it would be hard for the user to ensure their #pragma clang attribute (...) is not added to someone else's pushed namespace.

In D100404#2702032, @tra wrote:

In D100404#2701902, @t-tye wrote:

In D100404#2701630, @palves wrote:

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

I agree with @palves that this seems the least consistent of the options. Is there an objection to to the more consistent and more intuitive alternative?

OK. Scratch the no_pop. While it does the job, the syntax is indeed odd. Functionality wise, it's equivalent to using #pragma clang attribute (...) that @palves has suggested, which is a better way to express the intent.

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

I'd be fine with that. Making push w/o pop a warning would also work.

Changing the push w/o pop error into a warning is probably the easiest solution. Using a global scope is more complicated. We need to properly handle Erik's concerns about nested namespaces.
How would a user choose to pop their global namespace attribute, or prevent changing a parent namespace? Inherited pragmas seem a bit more tricky to implement correctly.

[palves]

erik.pilkington added a comment.
In D100404#2686753 https://reviews.llvm.org/D100404#2686753, @palves wrote:

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

Hmm, the problem I see with that is that you have to know that nobody else push-ed a #pca scope that you're inadvertently adding to.

It seems to me like the exact same concern applies to any other "#pca ()" call, regardless of a global scope existing.

[palves]

Using a global scope is more complicated. We need to properly handle Erik's concerns about nested namespaces.

His concern seems to me to apply to any scope, even without a global namespace. I.e., how do you know nobody else push-ed (or pushes in the future) a #pca scope in the "lots of code" parts below?

#pragma clang attribute push
/ lots of code
#pragma clang attribute (....)
/ lots of code
/// code that depends on attribute
#pragma clang attribute pop

Might go undetected if "code that depends on attribute" includes conditional compilation with #ifdef

Thus, I don't see this as a problem particular to there being a global scope.

How would a user choose to pop their global namespace attribute, or prevent changing a parent namespace?

These seem like invalid questions to make to me -- a user would not pop a global namespace attribute, ever, because, well, they're global. If a user wants to pop, then they create a push/pop scope. Just like global variables in C++ and namespaces.

int global;  // how do I remove this from scope below?  Answer, you don't.
namespace ns { int ns_var; }

Inherited pragmas seem a bit more tricky to implement correctly.

WDYM?

[erik.pilkington]

In D100404#2702245, @palves wrote:

Using a global scope is more complicated. We need to properly handle Erik's concerns about nested namespaces.

His concern seems to me to apply to any scope, even without a global namespace. I.e., how do you know nobody else push-ed (or pushes in the future) a #pca scope in the "lots of code" parts below?

#pragma clang attribute push
/ lots of code
#pragma clang attribute (....)
/ lots of code
/// code that depends on attribute
#pragma clang attribute pop

Might go undetected if "code that depends on attribute" includes conditional compilation with #ifdef

Thus, I don't see this as a problem particular to there being a global scope.

The reason I added the #pca () syntax in D53621 was so that you could add multiple attributes to a single pushed scope in a context where you knew exactly which scope you were adding them to (i.e., where it immediately follows a #pca push). I don't think it makes sense to use it when you aren't aware of the #pca context. There really should never be "lots of code" between a #pca push and a #pca ().

[palves]

The reason I added the #pca () syntax in D53621 https://reviews.llvm.org/D53621 was so that you could add multiple attributes to a single pushed scope in a context where you knew
exactly which scope you were adding them to (i.e., where it immediately follows a #pca push). I don't think it makes sense to use it when you aren't aware of the #pca context. There really
should never be "lots of code" between a #pca push and a #pca ().

Well, that "there should never be "lots of code"" reasoning eliminates your concern then:

"you have to know that nobody else `push`-ed a #pca scope that you're inadvertently adding to."

because if there won't be "lot of code", there's no reason to worry about inadvertently adding to the wrong scope, since the push/pop scope is tight and you see all the code easily.

Also, that might be the reason you created the syntax, but it doesn't mean that other people don't have "lots of code" scenarios, and valid ones, the compiler doesn't error out if your pop/push are "too many" lines apart... :-)

[ashi1]

I agree that the no_pop variant may not be what we want, so removing it.

@palves, thanks for answering my questions. I agree that it is cleaner to support a global attribute group. The user can expect similar usage as #pragma GCC diagnostic, where the #pragma clang attribute (...) without push/pop is valid, and the user is given the option to use push/pop to control the scope of their attributes. Also, I have changed the error unterminated push, to a warning instead, which aligns with other pragmas.

[ashi1]

Please review again, I have changed the patch description, and revised to the comments.

This new patch will accept #pragma clang attribute (...) without push/pop, and store the filter details into a global attribute group.

[palves]

In D100404#2708957, @ashi1 wrote:

Please review again, I have changed the patch description, and revised to the comments.

This new patch will accept #pragma clang attribute (...) without push/pop, and store the filter details into a global attribute group.

Thank you! I really like the "#pragma diagnostics" parallel, makes it even easier to justify.

I've tested the patch against the GDB testsuite, using the global #pragma clang attribute (...) form, and it worked against the couple testcases I tried.

(Running the whole testsuite will take a while, but I don't expect problems, if one test works, others should too.)

Noobie question -- I didn't see any change to the documentation in the patch. Is that a separate repo?

[ashi1]

In D100404#2709916, @palves wrote:

In D100404#2708957, @ashi1 wrote:

Please review again, I have changed the patch description, and revised to the comments.

This new patch will accept #pragma clang attribute (...) without push/pop, and store the filter details into a global attribute group.

Thank you! I really like the "#pragma diagnostics" parallel, makes it even easier to justify.

I've tested the patch against the GDB testsuite, using the global #pragma clang attribute (...) form, and it worked against the couple testcases I tried.

(Running the whole testsuite will take a while, but I don't expect problems, if one test works, others should too.)

Noobie question -- I didn't see any change to the documentation in the patch. Is that a separate repo?

Thanks @palves for testing this out, looking into adding that documentation.

[ashi1]

Added documentation changes.

[tra]

clang/docs/LanguageExtensions.rst

3382–3385

When no push directives are used is a bit ambiguous. E.g. in the following example, despite pus/pop being used, we'd still would expect attrA1 to be added to the scope of AttrA, but attrB to be added to the global scope.

#pragma clang attribute push attrA
#pragma clang attribute attrA1
#pragma clang attribute pop attrA

#pragma clang attribute attrB

Perhaps we should rephrase it along the lines of when "#pragma clang attribute ()" is used outside of a scope created by other "PCA push" directives, it will add the attribute to the global scope.

[ashi1]

Revised to Artem's comments.

[palves]

clang/docs/LanguageExtensions.rst

3393

IIUC, this function actually has both the "glob" and "custom" attributes. I think the comment should make that explicit.

It would be nice if we had an example with plain push, like:

#pragma clang attribute push
#pragma clang attribute (....)
#pragma clang attribute pop

AFAICS, there's no example for this currently, we only have example for the combined "#pca push (...)" syntax.

Actually, I think this whole section would benefit from some juggling around, to first document the "#pca (...)" syntax, then the plain "#pca push" syntax, explaining nesting, and then the "#pca push (...)" shorthand. This way I don't even feel that it's explicitly necessary to describe a "global" scope, it just falls out naturally. Like this:

https://github.com/palves/llvm-project/commit/49462ec4ef46f9a370e371ab7ed8c3b746c86dad

Untested because I did not figure out yet how does not build the Clang docs.

[ashi1]

Revised to @palves' comments.

[aaron.ballman]

In D100404#2702328, @palves wrote:

The reason I added the #pca () syntax in D53621 https://reviews.llvm.org/D53621 was so that you could add multiple attributes to a single pushed scope in a context where you knew
exactly which scope you were adding them to (i.e., where it immediately follows a #pca push). I don't think it makes sense to use it when you aren't aware of the #pca context. There really
should never be "lots of code" between a #pca push and a #pca ().

Well, that "there should never be "lots of code"" reasoning eliminates your concern then:

"you have to know that nobody else `push`-ed a #pca scope that you're inadvertently adding to."

because if there won't be "lot of code", there's no reason to worry about inadvertently adding to the wrong scope, since the push/pop scope is tight and you see all the code easily.

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

clang/docs/LanguageExtensions.rst

3384

This implies to me that it only impacts declarations at the global scope. e.g.,

#pragma clang attribute ([[clang::whatever]], apply_to = function)
namespace blech {
void f(); // Does not get [[clang::whatever]]
}

This might be because we're overloading "scope" -- there's the scopes of the pca annotations and the scopes of the declarations that get the attributes applied.

clang/test/Misc/warning-flags.c

21

"The list of warnings below should NEVER grow. It should gradually shrink to 0."

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

5

There's a whole lot more tests missing -- this only hits the most trivial of cases (and doesn't even verify that the attribute actually applies to the declaration).

I'd like to see tests for:

• entities before the unprotected pca do not get marked with the attribute
• entities after the unprotected pca do get marked with the attribute
• various kinds of scopes (namespaces, static member functions in a structure, etc)
• popping after the unprotected pca (which could be a reasonable place to suggest a fixit for the unprotected pca should have had a push in it?)
• adding the unprotected pca into a header file that is included from a test file with entities that can be attributed to show which entities get the attributes (e.g., does the "global" scope extend past the header file and into the TU including the header?)
• same as the previous point, but with an additional include without an pca in it. (e.g., does the "global" scope in Header A have impact on the declarations in Header B?)

[palves]

In D100404#2729968, @aaron.ballman wrote:

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

C and C++ have plenty of ways to shoot oneself in the foot. Heck, pointers are expressed with a single character ('*').
Putting "using namespace std;" in some central header is a bad idea. Etc. Yet, the languages don't prevent these things,
or try to make them harder to type.

Also, I don't see how dropping 4 characters ("push") makes it any more "dangerous" (whatever dangerous means). I mean, users can already put:

#pragma clang attribute push (....)

at the top of a translation unit. They get all the same breakage you imagine as with the proposed
global scope. The only difference is that with that syntax, users need to put a pop at the end of the translation unit.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

[palves]

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

5

It seems to me that those tests should already exist even with the current supported syntax, since you can nest #pca scopes. I.e., replace all those testing ideas with testing with an explicitly pushed outer pca scope and then an inner pca scope. Not sure it's fair to request a bunch of extra testing, when the only new feature is an extra scope level. I'd think the bar should be to test that the logic related to the extra scope works as intended. Anything else is not specific to the global scope. Examples below:

• adding the unprotected pca into a header file that is included from a test file with entities that can be attributed to show which entities get the attributes (e.g., does the "global" scope extend past the header file and into the TU including the header?)

That should be no different from what you can do today with:

#pragma clang attribute push
#pragma clang attribute (...)
#include "foo.h"
#pragma clang attribute pop

• same as the previous point, but with an additional include without an pca in it. (e.g., does the "global"

scope in Header A have impact on the declarations in Header B?)

Same as above. Are there already tests for these matters, without a global scope?

[aaron.ballman]

In D100404#2731431, @palves wrote:

In D100404#2729968, @aaron.ballman wrote:

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

C and C++ have plenty of ways to shoot oneself in the foot.

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

Heck, pointers are expressed with a single character ('*').
Putting "using namespace std;" in some central header is a bad idea. Etc. Yet, the languages don't prevent these things,
or try to make them harder to type.

Also, I don't see how dropping 4 characters ("push") makes it any more "dangerous" (whatever dangerous means). I mean, users can already put:

Because today, push requires a pop or you get an error. So accidentally getting the scope wrong is a loud issue for the programmer to respond to. What is being proposed here can accidentally apply attributes to *anything in the TU* with no notice that you've done something wrong. What's more, it's now easy to get into that situation by omission -- if you forget to write "push", your code compiles and works fine. Then you go and add more declarations *and who knows what happens*, but sometime later the programmer will likely discover that they missed the "push". That's what I mean by "more dangerous".

#pragma clang attribute push (....)

at the top of a translation unit. They get all the same breakage you imagine as with the proposed
global scope. The only difference is that with that syntax, users need to put a pop at the end of the translation unit.

That difference is quite large and important to me. If you forget to put the pop, the compiler tells you that you've made a logical mistake.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

I don't find that to be compelling rationale for this design.

With the other features that allow push/pop to be optional, problems arising from accidents are generally less severe. If you disable a diagnostic for a wider scope than you intended, that is a bad thing, but it doesn't (typically) leave you in potentially dangerous situations where you suddenly get UB in the code that wasn't already there before. With this feature, you can apply arbitrary attributes to arbitrary entities silently and that has semantic impact on the program.

I would like to better understand the motivation for why this extension is necessary. It seems like it has risk but very little reward -- it allows users to avoid writing one extra line of code, but that one line of code is quite important for expressing programmer intent. With the no_pop variant, the programmer at least has to explicitly opt into the dangerous behavior, making it less likely they'd do so unintentionally, but even there I wonder: what problem is being solved?

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

5

Not sure it's fair to request a bunch of extra testing, when the only new feature is an extra scope level.

If there are existing tests that you believe cover the behavior of this change in functionality, then great, please augment them with the new behavior so we can verify how the feature behaves. Saying "this feature is like that other feature so tests are not needed" is not sufficient -- I'm asking for test coverage of the new feature. This not only tests how the feature behaves, but it helps to capture design intent when we invariably do code archaeology on some aspect of the feature in the future. "Did they intend for this behavior" is important to demonstrate.

That should be no different from what you can do today with:

#pragma clang attribute push
#pragma clang attribute (...)
#include "foo.h"
#pragma clang attribute pop

This is testing something different than what I requested. I was asking for a test like:

// foo.h
#pragma clang attribute (...)
// some declarations that should be marked and some that should not

// foo.cpp
#include "foo.h"

// more declarations

// tests as to which declarations from both foo.h and foo.cpp were marked with the attributes

[palves]

On 5/2/21 2:18 PM, Aaron Ballman via Phabricator wrote:

aaron.ballman added a comment.

In D100404#2731431 https://reviews.llvm.org/D100404#2731431, @palves wrote:

In D100404#2729968 https://reviews.llvm.org/D100404#2729968, @aaron.ballman wrote:

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

C and C++ have plenty of ways to shoot oneself in the foot.

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

Heck, pointers are expressed with a single character ('*').
Putting "using namespace std;" in some central header is a bad idea. Etc. Yet, the languages don't prevent these things,
or try to make them harder to type.

Also, I don't see how dropping 4 characters ("push") makes it any more "dangerous" (whatever dangerous means). I mean, users can already put:

Because today, push requires a pop or you get an error. So accidentally getting the scope wrong is a loud issue for the programmer to respond to. What is being proposed here can accidentally apply attributes to *anything in the TU* with no notice that you've done something wrong. What's more, it's now easy to get into that situation by omission -- if you forget to write "push", your code compiles and works fine. Then you go and add more declarations *and who knows what happens*, but sometime later the programmer will likely discover that they missed the "push". That's what I mean by "more dangerous".

Sorry for the delay responding.

Again, there are an infinite number of situations where something similar can happen with many other constructs in the language.

Even preprocessor defines can trigger similar problems.

Programmers are not clueless. People who use these pragmas very naturally worry about scope of application. We can explain to users that creating a push/pop scope is a good idea and should be preferred -- I think we already do that? But preventing the natural syntax of adding attributes that apply to the whole TU when you need it on fear grounds seems like the wrong approach to me.

We should strive for orthogonality and composition. Allowing adding attributes to the global scope just makes sense from that perspective, and unlocks a useful use case. Some magical "no_pop" named scope namespace OTOH requires inventing special rules. E.g. of further rules that would need to the specified for clarity -- does "#pca pop" actually pop a no_pop scope, or its outer scope?

And really, "no_pop" does not prevent the same bugs you fear from arising. For example, if someone puts:

#pragma clang attribute no_pop.push (...)

in some central header (which is very much my use case), then this, without a push/pop, far away from the #cla no_pop.push:

#pragma clang attribute (...)

would add attributes to the "no_pop" scope. I.e., the no_pop solution does not prevent the problem of someone accidentally forgetting to wrap the "#pca (...)" with push/pop. The compiler can't help here, in exactly the same way it can't help the the global scope attributes solution.

#pragma clang attribute push (....)

at the top of a translation unit. They get all the same breakage you imagine as with the proposed
global scope. The only difference is that with that syntax, users need to put a pop at the end of the translation unit.

That difference is quite large and important to me. If you forget to put the pop, the compiler tells you that you've made a logical mistake.

See above.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

I don't find that to be compelling rationale for this design.

With the other features that allow push/pop to be optional, problems arising from accidents are generally less severe. If you disable a diagnostic for a wider scope than you intended, that is a bad thing, but it doesn't (typically) leave you in potentially dangerous situations where you suddenly get UB in the code that wasn't already there before. With this feature, you can apply arbitrary attributes to arbitrary entities silently and that has semantic impact on the program.

Let's agree to disagree then. I am not sympathetic to the view that users are not ultimately responsible for what they write. Adding warnings and such to help guide the user is fine, but preventing logical syntax on the idea that users require babysitting and hang holding is hard to grasp. This is an advanced feature, some level of "know what you're doing" should be expected.

I would like to better understand the motivation for why this extension is necessary. It seems like it has risk but very little reward -- it allows users to avoid writing one extra line of code, but that one line of code is quite important for expressing programmer intent. With the no_pop variant, the programmer at least has to explicitly opt into the dangerous behavior, making it less likely they'd do so unintentionally, but even there I wonder: what problem is being solved?

Ah, I thought you had seen it in the other bug. But wow, I had not realized how it's so hard to find comments in Phabricator, seemingly when new patch revisions are uploaded. I had trouble finding my own comment! :-/

Anyhow -- see the full description for the motivation here, in the original proposal patch, before we were pointed at #pragma clang attribute:

https://reviews.llvm.org/D98201#2630072

In a nutshell, we have a use case where we _want_ to apply attributes to the whole TU, and we can't modify the original sources, so we put "#pragma clang attribute (...)" in a header that is force-included with "-include" on the command line. There is nowhere you can put a corresponding pop.

[aaron.ballman]

In D100404#2738707, @palves wrote:

On 5/2/21 2:18 PM, Aaron Ballman via Phabricator wrote:

aaron.ballman added a comment.

In D100404#2731431 https://reviews.llvm.org/D100404#2731431, @palves wrote:

In D100404#2729968 https://reviews.llvm.org/D100404#2729968, @aaron.ballman wrote:

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

C and C++ have plenty of ways to shoot oneself in the foot.

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

Heck, pointers are expressed with a single character ('*').
Putting "using namespace std;" in some central header is a bad idea. Etc. Yet, the languages don't prevent these things,
or try to make them harder to type.

Also, I don't see how dropping 4 characters ("push") makes it any more "dangerous" (whatever dangerous means). I mean, users can already put:

Because today, push requires a pop or you get an error. So accidentally getting the scope wrong is a loud issue for the programmer to respond to. What is being proposed here can accidentally apply attributes to *anything in the TU* with no notice that you've done something wrong. What's more, it's now easy to get into that situation by omission -- if you forget to write "push", your code compiles and works fine. Then you go and add more declarations *and who knows what happens*, but sometime later the programmer will likely discover that they missed the "push". That's what I mean by "more dangerous".

Sorry for the delay responding.

No worries!

Again, there are an infinite number of situations where something similar can happen with many other constructs in the language.

Even preprocessor defines can trigger similar problems.

Programmers are not clueless. People who use these pragmas very naturally worry about scope of application. We can explain to users that creating a push/pop scope is a good idea and should be preferred -- I think we already do that? But preventing the natural syntax of adding attributes that apply to the whole TU when you need it on fear grounds seems like the wrong approach to me.

We should strive for orthogonality and composition. Allowing adding attributes to the global scope just makes sense from that perspective, and unlocks a useful use case. Some magical "no_pop" named scope namespace OTOH requires inventing special rules. E.g. of further rules that would need to the specified for clarity -- does "#pca pop" actually pop a no_pop scope, or its outer scope?

And really, "no_pop" does not prevent the same bugs you fear from arising. For example, if someone puts:

#pragma clang attribute no_pop.push (...)

in some central header (which is very much my use case), then this, without a push/pop, far away from the #cla no_pop.push:

#pragma clang attribute (...)

would add attributes to the "no_pop" scope. I.e., the no_pop solution does not prevent the problem of someone accidentally forgetting to wrap the "#pca (...)" with push/pop. The compiler can't help here, in exactly the same way it can't help the the global scope attributes solution.

I feel like you're hand-waving away the usability concerns with "well just don't do that" and "but C++ already lets you shoot yourself in the foot", neither of which address the concerns. push/pop is the 99% use case for this feature, while whole TU is the 1% case (if that), and the way this proposed extension is currently designed makes it easier for the people in the most common use case to accidentally (and silently) use the special case instead.

The no_pop design was weird and problematic, but it was at least opt-in. Another possibility would be to use a command line flag to enable unterminated pushes. Alternatively, given that whole-TU can be emulated with push/pop anyway except for the case of a forced pre-include, you could introduce the notion of a forced post-include that gets automatically included at the end of a TU (which may be a more generally useful feature). Basically, there are different ways to approach the design and I'm not convinced the current approach is making the right tradeoffs.

#pragma clang attribute push (....)

at the top of a translation unit. They get all the same breakage you imagine as with the proposed
global scope. The only difference is that with that syntax, users need to put a pop at the end of the translation unit.

That difference is quite large and important to me. If you forget to put the pop, the compiler tells you that you've made a logical mistake.

See above.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

I don't find that to be compelling rationale for this design.

With the other features that allow push/pop to be optional, problems arising from accidents are generally less severe. If you disable a diagnostic for a wider scope than you intended, that is a bad thing, but it doesn't (typically) leave you in potentially dangerous situations where you suddenly get UB in the code that wasn't already there before. With this feature, you can apply arbitrary attributes to arbitrary entities silently and that has semantic impact on the program.

Let's agree to disagree then.

I don't think that's sufficient for this patch to move forward.

I am not sympathetic to the view that users are not ultimately responsible for what they write. Adding warnings and such to help guide the user is fine, but preventing logical syntax on the idea that users require babysitting and hang holding is hard to grasp. This is an advanced feature, some level of "know what you're doing" should be expected.

I would like to better understand the motivation for why this extension is necessary. It seems like it has risk but very little reward -- it allows users to avoid writing one extra line of code, but that one line of code is quite important for expressing programmer intent. With the no_pop variant, the programmer at least has to explicitly opt into the dangerous behavior, making it less likely they'd do so unintentionally, but even there I wonder: what problem is being solved?

Ah, I thought you had seen it in the other bug. But wow, I had not realized how it's so hard to find comments in Phabricator, seemingly when new patch revisions are uploaded. I had trouble finding my own comment! :-/

If the new patch is sufficiently different from the old one, I've found that comments sometimes become "unassociated". They remain in the older views of the patch, but I can never remember to hunt for them there. :-(

Anyhow -- see the full description for the motivation here, in the original proposal patch, before we were pointed at #pragma clang attribute:

https://reviews.llvm.org/D98201#2630072

Thank you, this gave me a lot of good background on the issue.

In a nutshell, we have a use case where we _want_ to apply attributes to the whole TU, and we can't modify the original sources, so we put "#pragma clang attribute (...)" in a header that is force-included with "-include" on the command line. There is nowhere you can put a corresponding pop.

This seems like a special-case situation specific to your needs. It was observed in that thread "In general, relaxing error checking, which affects all users, for the sake of a niche use case does not look like a good trade-off to me." and I think that also applies to the design with the proposed patch because it's relaxing a purposefully restrictive syntax for a special case.

One alternative idea specific to your situation is that you can create a test harness with the contents:

#pragma clang attribute push(...)
#include YOUR_TEST_FILE
#pragma clang attribute pop

and then to run the test, you would pass -DYOUR_TEST_FILE=\"whatever.cpp\" and compile the harness file rather than whatever.cpp. This doesn't require you to change the contents of the actual test files and it doesn't require any new features to support. Have you tried a solution like that and run into problems with it?

[palves]

In D100404#2738957, @aaron.ballman wrote:

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

See below.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

I don't find that to be compelling rationale for this design.

With the other features that allow push/pop to be optional, problems arising from accidents are generally less severe. If you disable a diagnostic for a wider scope than you intended, that is a bad thing, but it doesn't (typically) leave you in potentially dangerous situations where you suddenly get UB in the code that wasn't already there before. With this feature, you can apply arbitrary attributes to arbitrary entities silently and that has semantic impact on the program.

Let's agree to disagree then.

I don't think that's sufficient for this patch to move forward.

How about this, then? For years/decades, people have been using other #pragmas with the same properties, like, e.g:

https://gcc.gnu.org/onlinedocs/gcc/ARM-Pragmas.html#ARM-Pragmas

...
long_calls
Set all subsequent functions to have the long_call attribute.
...

Not sure that one is supported by Clang, but that doesn't remove from the point. If you go to:

https://gcc.gnu.org/onlinedocs/gcc/Pragmas.html

and then follow to the architecture-specific pragmas, I'm sure you'll find some that are (supported by Clang), with the
same "dangerous" ABI-changing-until-end-of-TU property.

There's also this one, surely supported by Clang:

#pragma GCC visibility push(visibility)

Again, ABI changing, and doesn't require a pop, it just goes all the way until the end of the TU if you don't pop.

You mentioned that with #pragma diagnostics it's less dangerous to miss a push/pop. But how about #pragma pack, for example?

https://gcc.gnu.org/onlinedocs/gcc/Structure-Layout-Pragmas.html

...
#pragma pack(n) simply sets the new alignment.
#pragma pack() sets the alignment to the one that was in effect when compilation started (see also command-line option -fpack-struct[=n] see Code Gen Options).
#pragma pack(push[,n]) pushes the current alignment setting on an internal stack and then optionally sets the new alignment.
#pragma pack(pop) restores the alignment setting to the one saved at the top of the internal stack (and removes that stack entry). Note that #pragma pack([n]) does not influence this internal stack; thus it is possible to have #pragma pack(push) followed by multiple #pragma pack(n) instances and finalized by a single #pragma pack(pop).
the #pragma ms_struct directive which lays out structures and unions subsequently defined
...

You should have the exact the same concern with this as with #pca. You can "accidentally" change the ABI of more structures than you intend to, and cause undefined behavior.
It has a way to create push/pop scopes as well, but you can use #pragma pack(N) to apply the pragma without a scope, like:

#pragma pack(4)
struct A { long a; }; 4 bytes packing
struct B { long b; }; 4 bytes packing

Other pragmas listed on that page also affect ABI and don't even have a push/pop variant.

You argue that we shouldn't repeat the mistakes of the past, that the existence of these examples does not justify repeating the mistake in a newer feature.

And I argue that the existence of these examples instead helps show that the "safety" concern is imaginary, and restricts useful syntax for no good reason, forcing us to think about alternative and more hacky ways to skin the cat.

Anyhow -- see the full description for the motivation here, in the original proposal patch, before we were pointed at #pragma clang attribute:

https://reviews.llvm.org/D98201#2630072

Thank you, this gave me a lot of good background on the issue.

In a nutshell, we have a use case where we _want_ to apply attributes to the whole TU, and we can't modify the original sources, so we put "#pragma clang attribute (...)" in a header that is force-included with "-include" on the command line. There is nowhere you can put a corresponding pop.

This seems like a special-case situation specific to your needs. It was observed in that thread "In general, relaxing error checking, which affects all users, for the sake of a niche use case does not look like a good trade-off to me." and I think that also applies to the design with the proposed patch because it's relaxing a purposefully restrictive syntax for a special case.

I understand that view but disagree with it, because I view the syntax restriction as arbitrary, and not really that protective, as per my examples in prior messages.

Plus, no one ever thought that it was problematic that:

#pragma clang force_cuda_host_device begin

doesn't require a pop at the end of the translation unit, yet that's how it works. That #pragma is basically a subset of "#pragma clang attribute" (it applies the device attribute to functions) !
We were originally proposing an equivalent #pragma but for global variables, and back then that patch was reviewed, no one thought that Clang should give an hard error for missing pop... It's just the more generic #pca that is more restrictive. Score -1 for consistency. :-)

One alternative idea specific to your situation is that you can create a test harness with the contents:

#pragma clang attribute push(...)
#include YOUR_TEST_FILE
#pragma clang attribute pop

and then to run the test, you would pass -DYOUR_TEST_FILE=\"whatever.cpp\" and compile the harness file rather than whatever.cpp. This doesn't require you to change the contents of the actual test files and it doesn't require any new features to support. Have you tried a solution like that and run into problems with it?

I had tried something like this, but while it works for some simpler tests, I had ran into issues with others, and I worry about whether it affects what some GDB testcases might be testing, that with that creating that artificial extra CU. I had originally assumed that these proposed extensions would be seen as good additions and would be accepted, so I didn't pursue it. Let me give it another go. (It takes a while, hours at a time, to run the whole testsuite, and I haven't had the time to look into this in the past weeks, hence the slowness.) If this doesn't work, then I think the "-include" variant that pulls a header at the end of the translation unit would work. But let me give your idea another try. Thanks for the patience.

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

5

Not sure it's fair to request a bunch of extra testing, when the only new feature is an extra scope level.

If there are existing tests that you believe cover the behavior of this change in functionality, then great, please augment them with the new behavior so we can verify how the feature behaves. Saying "this feature is like that other feature so tests are not needed" is not sufficient -- I'm asking for test coverage of the new feature. This not only tests how the feature behaves, but it helps to capture design intent when we invariably do code archaeology on some aspect of the feature in the future. "Did they intend for this behavior" is important to demonstrate.

That should be no different from what you can do today with:

#pragma clang attribute push
#pragma clang attribute (...)
#include "foo.h"
#pragma clang attribute pop

This is testing something different than what I requested. I was asking for a test like:

// foo.h
#pragma clang attribute (...)
// some declarations that should be marked and some that should not

// foo.cpp
#include "foo.h"

// more declarations

// tests as to which declarations from both foo.h and foo.cpp were marked with the attributes

5

If there are existing tests that you believe cover the behavior of this change in functionality, then great, please augment them with the new behavior so we can verify how the feature behaves. Saying "this feature is like that other feature so tests are not needed" is not sufficient -- I'm asking for test coverage of the new feature. This not only tests how the feature behaves, but it helps to capture design intent when we invariably do code archaeology on some aspect of the feature in the future. "Did they intend for this behavior" is important to demonstrate.

Please take a look at the clang/test/SemaCUDA/force-device-globals.cu file added by this patch. It covers the semantics we need (the test case was written well before we switched to #pca, even). It already tests some of the things you are asking for. E.g., "entities before the unprotected pca do not get marked with the attribute" is tested, "various kinds of scopes (namespaces, static member functions in a structure, etc)".

Some of the use cases you mention do not seem like new functionality, so we'd either be duplicating testcases without actually adding extra coverage, or we'd be writing testcases that should have been requested out of whom added the original feature. FWIW, I wouldn't object to having them, but I wouldn't block the feature without them. But I'm not the one doing the leg work, so up to Aaron, really.

See below.

This is testing something different than what I requested. I was asking for a test like:

// foo.h
#pragma clang attribute (...)
// some declarations that should be marked and some that should not

// foo.cpp
#include "foo.h"

// more declarations

// tests as to which declarations from both foo.h and foo.cpp were marked with the attributes

OK, I slightly misunderstood what you wanted tested, but the previous point still applies. If you just add a scope to your example, so it works with current Clang, it's the same thing - a test about whether #pca crosses #include. I don't know whether this test already exists. Vis:

// foo.h
#pragma clang attribute (...)
// some declarations that should be marked and some that should not

// foo.cpp

+ #pragma clang attribute push

#include "foo.h"

// more declarations

// tests as to which declarations from both foo.h and foo.cpp were marked with the attributes

+ #pragma clang attribute pop

[aaron.ballman]

In D100404#2754106, @palves wrote:

In D100404#2738957, @aaron.ballman wrote:

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

See below.

I.e., it's just as "dangerous".

If someone adds an attribute to a wider scope than it should, they'll break their code. Just like anything else in coding.

I don't find that to be compelling rationale for this design.

With the other features that allow push/pop to be optional, problems arising from accidents are generally less severe. If you disable a diagnostic for a wider scope than you intended, that is a bad thing, but it doesn't (typically) leave you in potentially dangerous situations where you suddenly get UB in the code that wasn't already there before. With this feature, you can apply arbitrary attributes to arbitrary entities silently and that has semantic impact on the program.

Let's agree to disagree then.

I don't think that's sufficient for this patch to move forward.

How about this, then? For years/decades, people have been using other #pragmas with the same properties, like, e.g:

https://gcc.gnu.org/onlinedocs/gcc/ARM-Pragmas.html#ARM-Pragmas

...
long_calls
Set all subsequent functions to have the long_call attribute.
...

Not sure that one is supported by Clang, but that doesn't remove from the point.

If you go to:

https://gcc.gnu.org/onlinedocs/gcc/Pragmas.html

and then follow to the architecture-specific pragmas, I'm sure you'll find some that are (supported by Clang), with the
same "dangerous" ABI-changing-until-end-of-TU property.

There's also this one, surely supported by Clang:

#pragma GCC visibility push(visibility)

Again, ABI changing, and doesn't require a pop, it just goes all the way until the end of the TU if you don't pop.

You mentioned that with #pragma diagnostics it's less dangerous to miss a push/pop. But how about #pragma pack, for example?

https://gcc.gnu.org/onlinedocs/gcc/Structure-Layout-Pragmas.html

...
#pragma pack(n) simply sets the new alignment.
#pragma pack() sets the alignment to the one that was in effect when compilation started (see also command-line option -fpack-struct[=n] see Code Gen Options).
#pragma pack(push[,n]) pushes the current alignment setting on an internal stack and then optionally sets the new alignment.
#pragma pack(pop) restores the alignment setting to the one saved at the top of the internal stack (and removes that stack entry). Note that #pragma pack([n]) does not influence this internal stack; thus it is possible to have #pragma pack(push) followed by multiple #pragma pack(n) instances and finalized by a single #pragma pack(pop).
the #pragma ms_struct directive which lays out structures and unions subsequently defined
...

You should have the exact the same concern with this as with #pca. You can "accidentally" change the ABI of more structures than you intend to, and cause undefined behavior.

It has a way to create push/pop scopes as well, but you can use #pragma pack(N) to apply the pragma without a scope, like:

#pragma pack(4)
struct A { long a; }; 4 bytes packing
struct B { long b; }; 4 bytes packing

Other pragmas listed on that page also affect ABI and don't even have a push/pop variant.

You argue that we shouldn't repeat the mistakes of the past, that the existence of these examples does not justify repeating the mistake in a newer feature.

And I argue that the existence of these examples instead helps show that the "safety" concern is imaginary, and restricts useful syntax for no good reason, forcing us to think about alternative and more hacky ways to skin the cat.

Anyhow -- see the full description for the motivation here, in the original proposal patch, before we were pointed at #pragma clang attribute:

https://reviews.llvm.org/D98201#2630072

Thank you, this gave me a lot of good background on the issue.

In a nutshell, we have a use case where we _want_ to apply attributes to the whole TU, and we can't modify the original sources, so we put "#pragma clang attribute (...)" in a header that is force-included with "-include" on the command line. There is nowhere you can put a corresponding pop.

This seems like a special-case situation specific to your needs. It was observed in that thread "In general, relaxing error checking, which affects all users, for the sake of a niche use case does not look like a good trade-off to me." and I think that also applies to the design with the proposed patch because it's relaxing a purposefully restrictive syntax for a special case.

I understand that view but disagree with it, because I view the syntax restriction as arbitrary, and not really that protective, as per my examples in prior messages.

Plus, no one ever thought that it was problematic that:

#pragma clang force_cuda_host_device begin

doesn't require a pop at the end of the translation unit, yet that's how it works. That #pragma is basically a subset of "#pragma clang attribute" (it applies the device attribute to functions) !
We were originally proposing an equivalent #pragma but for global variables, and back then that patch was reviewed, no one thought that Clang should give an hard error for missing pop... It's just the more generic #pca that is more restrictive. Score -1 for consistency. :-)

One alternative idea specific to your situation is that you can create a test harness with the contents:

#pragma clang attribute push(...)
#include YOUR_TEST_FILE
#pragma clang attribute pop

and then to run the test, you would pass -DYOUR_TEST_FILE=\"whatever.cpp\" and compile the harness file rather than whatever.cpp. This doesn't require you to change the contents of the actual test files and it doesn't require any new features to support. Have you tried a solution like that and run into problems with it?

I had tried something like this, but while it works for some simpler tests, I had ran into issues with others, and I worry about whether it affects what some GDB testcases might be testing, that with that creating that artificial extra CU. I had originally assumed that these proposed extensions would be seen as good additions and would be accepted, so I didn't pursue it. Let me give it another go. (It takes a while, hours at a time, to run the whole testsuite, and I haven't had the time to look into this in the past weeks, hence the slowness.) If this doesn't work, then I think the "-include" variant that pulls a header at the end of the translation unit would work. But let me give your idea another try. Thanks for the patience.

Thank you for giving the alternative approaches a shot! I would be especially curious to hear if there are critical flaws with either of those approaches.

I've been thinking about this for the past week or so and I'm not comfortable adding the feature that this patch proposes. The original design of pca was for a very limited purpose where a library author wants to blast attributes onto everything within a header file they control (so it was already known to not be a particularly generally useful feature). While I personally think this is a bit of a dangerous practice, it was at least palatable because the functionality was limited in scope due to the push/pop nature of things. Now there's a single use case where it's observed that it would be helpful to allow blasting attributes to everything with a TU, which is a very different feature with even less general applicability. I don't think this new feature carries its weight, and it's really not compelling to me that other questionable features exist (many of which Clang doesn't support). We could go back and forth on whether those features should or should not exist and how they relate to the proposed feature, but at the end of the day I keep coming back to the point that *this* particular feature is not generally useful for most programmers and it is dangerous to use when you get it wrong. I'd rather see it the needs from this feature worked around locally or by introducing a more general feature like a post-include, than to introduce a feature with this sharp of user experience edges but without many legitimate uses.

[ashi1]

@palves was able to resolve his request following @aaron.ballman's proposal. Closing this patch.

clang/docs/LanguageExtensions.rst

for(...) {
     #pragma float_control(precise, on)
     a = b[i] * c[i] + e;
   }
 
 Specifying an attribute for multiple declarations (#pragma clang attribute)
 ===========================================================================
 
 The ``#pragma clang attribute`` directive can be used to apply an attribute to
-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.
+multiple declarations. When the directive is used outside of a scope created by
+a ``#pragma clang attribute push`` directive, the attribute will be added
+to the global scope.
 
 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 (__attribute__((annotate("custom"))), apply_to = function)
 
-  void function(); // The function now has the annotate("custom") attribute
+  void function1(); // These functions now both ...
 
-  #pragma clang attribute pop
+  void function2(); // ... have the annotate("custom") attribute.
 
 The attributes can also be written using the C++11 style syntax:
 
 .. code-block:: c++
 
-  #pragma clang attribute push ([[noreturn]], apply_to = function)
-
-  void function(); // The function now has the [[noreturn]] attribute
+  #pragma clang attribute ([[noreturn]], apply_to = function)
 
-  #pragma clang attribute pop
+  void function(); // The function now has the [[noreturn]] attribute.
 
 The ``__declspec`` style syntax is also supported:
 
 .. code-block:: c++
 
-  #pragma clang attribute push (__declspec(dllexport), apply_to = function)
+  #pragma clang attribute (__declspec(dllexport), apply_to = function)
 
-  void function(); // The function now has the __declspec(dllexport) attribute
+  void function(); // The function now has the __declspec(dllexport) attribute.
 
-  #pragma clang attribute pop
+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 pop`` variation pops the scope. Multiple push
+directives can be nested inside each other. Attributes added to outer scopes
+continue to apply in inner scopes.
+
+.. code-block:: c++
+
+  #pragma clang attribute push
+  #pragma clang attribute (__attribute__((annotate("custom1"))), apply_to = function)
+
+  void function1(); // This function has the annotate("custom1") attribute.
+
+  #pragma clang attribute push
+  #pragma clang attribute (__attribute__((annotate("custom2"))), apply_to = function)
+
+  void function2(); // This function has both annotate("custom1") and annotate("custom2") attributes.
 
-A single push directive accepts only one attribute regardless of the syntax
-used.
+  #pragma clang attribute pop // pops "custom2" scope
+
+  #pragma clang attribute pop // pops "custom1" scope
+
+  void function3(); // This function does not have the annotate("custom1") or annotate("custom2") attributes.
+
+You can also use ``#pragma clang attribute push (...)``, which is a shorthand
+for when you want to add one attribute to a new scope. A single push directive
+accepts only one attribute regardless of the attribute syntax used.
+
+.. code-block:: c++
+
+  #pragma clang attribute push (__attribute__((annotate("custom"))), apply_to = function)
+
+  void function(); // The function now has the annotate("custom") attribute.
+
+  #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
 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
+that a ``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")
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_pragma_attribute_stack_mismatch : Error<
   "'#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 "
+def warn_pragma_attribute_no_pop_eof : Warning<"unterminated "
   "'#pragma clang attribute push' at end of file">;
 def note_pragma_attribute_applied_decl_here : Note<
   "when applied to this declaration">;
-def err_pragma_attr_attr_no_push : Error<
-  "'#pragma clang attribute' attribute with no matching "
-  "'#pragma clang attribute push'">;
 
 /// Objective-C parser diagnostics
 def err_duplicate_class_def : Error<
   "duplicate interface definition for class %0">;
 def err_undef_superclass : Error<
   "cannot find interface declaration for %0, superclass of %1">;
 def err_forward_superclass : Error<
   "attempting to use the forward class %0 as superclass of %1">;

clang/include/clang/Sema/Sema.h

public:
   };
 
   /// 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;
+    /// Flag indicating if this is the global group.
+    bool IsGlobal;
     SmallVector<PragmaAttributeEntry, 2> Entries;
   };
 
   SmallVector<PragmaAttributeGroup, 2> PragmaAttributeStack;
 
   /// The declaration that is currently receiving an attribute from the
   /// #pragma attribute stack.
   const Decl *PragmaAttributeCurrentTargetDecl;

   /// FreeVisContext - Deallocate and null out VisContext.
   void FreeVisContext();
 
   /// 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);
 
+  /// AddPragmaAttributeGlobalOnce - Add a Global Attribute Group, which
+  /// allows usage of '\#pragma clang attribute (...)' without push/pop.
+  void AddPragmaAttributeGlobalOnce();
+
   void ActOnPragmaAttributeAttribute(ParsedAttr &Attribute,
                                      SourceLocation PragmaLoc,
                                      attr::ParsedSubjectMatchRuleSet Rules);
   void ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
                                      const IdentifierInfo *Namespace);
 
   /// Called on well-formed '\#pragma clang attribute pop'.
   void ActOnPragmaAttributePop(SourceLocation PragmaLoc,

clang/lib/Parse/ParsePragma.cpp

 
 } // 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());
+  // Add the Global attribute when namespace push/pop is unused.
+  Actions.AddPragmaAttributeGlobalOnce();
   if (Info->Action == PragmaAttributeInfo::Pop) {
     ConsumeAnnotationToken();
     Actions.ActOnPragmaAttributePop(PragmaLoc, Info->Namespace);
     return;
   }
   // Parse the actual attribute with its arguments.
   assert((Info->Action == PragmaAttributeInfo::Push ||
           Info->Action == PragmaAttributeInfo::Attribute) &&
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 (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.
+/// or global 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,
                                           PragmaIntroducer 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;

clang/lib/Sema/SemaAttr.cpp

if (I.index())
       OS << (I.index() == Rules.size() - 1 ? ", and " : ", ");
     OS << "'" << attr::getSubjectMatchRuleSpelling(I.value()) << "'";
   }
   return OS.str();
 }
 
 } // end anonymous namespace
 
+void Sema::AddPragmaAttributeGlobalOnce() {
+  if (!PragmaAttributeStack.empty())
+    return;
+  PragmaAttributeStack.emplace_back();
+  PragmaAttributeStack.back().Namespace = nullptr;
+  PragmaAttributeStack.back().IsGlobal = true;
+}
+
 void Sema::ActOnPragmaAttributeAttribute(
     ParsedAttr &Attribute, SourceLocation PragmaLoc,
     attr::ParsedSubjectMatchRuleSet Rules) {
   Attribute.setIsPragmaClangAttribute();
   SmallVector<attr::SubjectMatchRule, 4> SubjectMatchRules;
   // Gather the subject match rules that are supported by the attribute.
   SmallVector<std::pair<attr::SubjectMatchRule, bool>, 4>
       StrictSubjectMatchRuleSet;
if (!Rules.empty()) {
     for (const auto &Rule : Rules) {
       ExtraRules.push_back(attr::SubjectMatchRule(Rule.first));
       Diagnostic << FixItHint::CreateRemoval(
           replacementRangeForListElement(*this, Rule.second));
     }
     Diagnostic << attrMatcherRuleListToString(ExtraRules);
   }
 
-  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,
                                          const IdentifierInfo *Namespace) {
   PragmaAttributeStack.emplace_back();
   PragmaAttributeStack.back().Loc = PragmaLoc;
   PragmaAttributeStack.back().Namespace = Namespace;
+  PragmaAttributeStack.back().IsGlobal = false;
 }
 
 void Sema::ActOnPragmaAttributePop(SourceLocation PragmaLoc,
                                    const IdentifierInfo *Namespace) {
-  if (PragmaAttributeStack.empty()) {
+  // Only the global namespace exists, pop without matching push.
+  if (PragmaAttributeStack.size() == 1) {
     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) {
+    if (PragmaAttributeStack[Index].Namespace == Namespace &&
+        !PragmaAttributeStack[Index].IsGlobal) {
       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;
     }
   }
 
   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;
+  // Expected that at least Global Attribute Group exists.
   for (auto &Group : PragmaAttributeStack) {
     for (auto &Entry : Group.Entries) {
       ParsedAttr *Attribute = Entry.Attribute;
       assert(Attribute && "Expected an attribute");
       assert(Attribute->isPragmaClangAttribute() &&
              "expected #pragma clang attribute");
 
       // Ensure that the attribute can be applied to the given declaration.

 
 void Sema::PrintPragmaAttributeInstantiationPoint() {
   assert(PragmaAttributeCurrentTargetDecl && "Expected an active declaration");
   Diags.Report(PragmaAttributeCurrentTargetDecl->getBeginLoc(),
                diag::note_pragma_attribute_applied_decl_here);
 }
 
 void Sema::DiagnoseUnterminatedPragmaAttribute() {
-  if (PragmaAttributeStack.empty())
+  // There can be a Global Pragma Attribute.
+  if (PragmaAttributeStack.size() <= 1)
     return;
-  Diag(PragmaAttributeStack.back().Loc, diag::err_pragma_attribute_no_pop_eof);
+  Diag(PragmaAttributeStack.back().Loc, diag::warn_pragma_attribute_no_pop_eof);
 }
 
 void Sema::ActOnPragmaOptimize(bool On, SourceLocation PragmaLoc) {
   if(On)
     OptimizeOffPragmaLocation = SourceLocation();
   else
     OptimizeOffPragmaLocation = PragmaLoc;
 }
 

clang/test/Misc/warning-flags.c

(2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (68):
+CHECK: Warnings without flags (69):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_declspec
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens

 CHECK-NEXT:   warn_no_constructor_for_refconst
 CHECK-NEXT:   warn_not_compound_assign
 CHECK-NEXT:   warn_objc_property_copy_missing_on_block
 CHECK-NEXT:   warn_objc_protocol_qualifier_missing_id
 CHECK-NEXT:   warn_on_superclass_use
 CHECK-NEXT:   warn_pp_convert_to_positive
 CHECK-NEXT:   warn_pp_expr_overflow
 CHECK-NEXT:   warn_pp_line_decimal
+CHECK-NEXT:   warn_pragma_attribute_no_pop_eof
 CHECK-NEXT:   warn_pragma_pack_pop_identifier_and_alignment
 CHECK-NEXT:   warn_pragma_pack_show
 CHECK-NEXT:   warn_property_getter_owning_mismatch
 CHECK-NEXT:   warn_register_objc_catch_parm
 CHECK-NEXT:   warn_related_result_type_compatibility_class
 CHECK-NEXT:   warn_related_result_type_compatibility_protocol
 CHECK-NEXT:   warn_template_export_unsupported
 CHECK-NEXT:   warn_template_spec_extra_headers

clang/test/Parser/pragma-attribute.cpp

 #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'}}
 // expected-error@-1 {{expected unqualified-id}}
-// expected-error@-2 {{unterminated '#pragma clang attribute push' at end of file}}
+// expected-warning@-2 {{unterminated '#pragma clang attribute push' at end of file}}
 
 #pragma clang attribute pop () // expected-warning {{extra tokens at end of '#pragma clang attribute'}}
 
 ;
 
 #pragma clang attribute push (__attribute__((42))) // expected-error {{expected identifier that represents an attribute name}}
 
 #pragma clang attribute push (__attribute__((annotate)) foo) // expected-error {{expected ','}}

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

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Valid to add attribute to global attribute group.
+#pragma clang attribute (__attribute__((annotate("func"))), apply_to = function)
+
+void function();

clang/test/Sema/pragma-attribute.c

 
 #pragma clang attribute push (__attribute__((annotate())), apply_to = function) // expected-error{{'annotate' attribute takes at least 1 argument}}
 void fun3(); // expected-note 2 {{when applied to this declaration}}
 #pragma clang attribute pop
 
 #pragma clang attribute pop
 #pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
 
-#pragma clang attribute (__attribute__((annotate)), apply_to = function) // expected-error{{'#pragma clang attribute' attribute with no matching '#pragma clang attribute push}}
-
 #pragma clang attribute push ([[]], apply_to = function) // A noop
 
 #pragma clang attribute pop // expected-error {{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
 
-#pragma clang attribute push (__attribute__((annotate("func"))), apply_to = function) // expected-error {{unterminated '#pragma clang attribute push' at end of file}}
+#pragma clang attribute push (__attribute__((annotate("func"))), apply_to = function) // expected-warning {{unterminated '#pragma clang attribute push' at end of file}}
 
 void function();

clang/test/SemaCUDA/force-device-globals.cu

+// RUN: %clang_cc1 -std=c++14 %s -o - -triple nvptx64-nvidia-cuda \
+// RUN:   -fcuda-is-device -verify -fsyntax-only
+#include "Inputs/cuda.h"
+
+static int global_before_pragma = 1;
+
+#pragma clang attribute pop
+// expected-error@-1 {{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
+
+#pragma clang attribute push (__device__, apply_to = variable(is_global))
+
+static int global = 1;
+static const int const_global = 1;
+
+struct S {
+  static const int static_const_field = 1;
+  static int static_field;
+};
+
+namespace NS1 {
+  namespace NS2 {
+    int ns_global = 1;
+  }
+
+  struct S {
+    static const int ns1_static_const_field = 1;
+    static int ns1_static_field;
+  };
+}
+
+int S::static_field = 1;
+int NS1::S::ns1_static_field = 1;
+
+#pragma clang attribute pop
+
+__device__ void device_func(int &a) {
+  static constexpr int local_static_constexpr = 1;
+  constexpr int local_constexpr = 2;
+  static int local_static = 3;
+  a += local_static_constexpr;
+  a += local_constexpr;
+  a += local_static;
+  a += global_before_pragma;
+  // expected-error@-1 {{reference to __host__ variable 'global_before_pragma' in __device__ function}}
+  a += global;
+  a += const_global;
+  a += S::static_const_field;
+  a += S::static_field;
+  a += NS1::S::ns1_static_const_field;
+  a += NS1::S::ns1_static_field;
+  a += NS1::NS2::ns_global;
+}
+
+void host_func(int &a) {
+  static constexpr int local_static_constexpr = 1;
+  constexpr int local_constexpr = 2;
+  static int local_static = 3;
+  a += local_static_constexpr;
+  a += local_constexpr;
+  a += local_static;
+  a += global_before_pragma;
+  // Note: variables below should not be allowed to read from host.
+  // Clang allows reading their shadows, but that is not OK.
+  a += global;
+  a += const_global;
+  a += S::static_const_field;
+  a += S::static_field;
+  a += NS1::S::ns1_static_const_field;
+  a += NS1::S::ns1_static_field;
+  a += NS1::NS2::ns_global;
+}
+
+__host__ __device__ void host_device_func(int &a) {
+  static constexpr int local_static_constexpr = 1;
+  constexpr int local_constexpr = 2;
+  static int local_static = 3;
+  a += local_static_constexpr;
+  a += local_constexpr;
+  a += local_static;
+  a += global_before_pragma;
+  // expected-error@-1 {{reference to __host__ variable 'global_before_pragma' in __host__ __device__ function}}
+  a += global;
+  a += const_global;
+  a += S::static_const_field;
+  a += S::static_field;
+  a += NS1::S::ns1_static_const_field;
+  a += NS1::S::ns1_static_field;
+  a += NS1::NS2::ns_global;
+}
+
+#pragma clang attribute push (__device__, apply_to = variable(is_global))
+// expected-warning@-1 {{unterminated '#pragma clang attribute push' at end of file}}