Page MenuHomePhabricator

Add Global support for #pragma clang attributes
AbandonedPublic

Authored by ashi1 on Apr 13 2021, 11:53 AM.

Details

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.

Diff Detail

Event Timeline

ashi1 requested review of this revision.Apr 13 2021, 11:53 AM
ashi1 created this revision.
tra added subscribers: arphaman, aaron.ballman.

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?

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.

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 added a comment.Apr 20 2021, 6:46 AM

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?

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?

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

ashi1 updated this revision to Diff 338862.Apr 20 2021, 7:26 AM

Fixed formatting issues.

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

ashi1 updated this revision to Diff 338885.Apr 20 2021, 8:47 AM

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

t-tye added a comment.Apr 20 2021, 9:04 AM

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?

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 added a comment.Apr 20 2021, 9:34 AM

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.

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.

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.

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.

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?

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 ().

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 marked 3 inline comments as done.Apr 21 2021, 2:24 PM

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 updated this revision to Diff 339388.Apr 21 2021, 2:35 PM
ashi1 retitled this revision from Add no_pop variant to pragma attributes to Add Global support for #pragma clang attributes.
ashi1 edited the summary of this revision. (Show Details)
ashi1 added a reviewer: erik.pilkington.
ashi1 added a comment.Apr 22 2021, 9:00 AM

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.

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?

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 updated this revision to Diff 340123.Apr 23 2021, 11:40 AM

Added documentation changes.

tra added inline comments.Apr 26 2021, 10:54 AM
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 updated this revision to Diff 340605.Apr 26 2021, 12:02 PM

Revised to Artem's comments.

ashi1 marked an inline comment as done.Apr 26 2021, 12:02 PM
palves added inline comments.Apr 28 2021, 6:36 AM
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 updated this revision to Diff 341530.Apr 29 2021, 8:40 AM

Revised to @palves' comments.

ashi1 marked an inline comment as done.Apr 29 2021, 8:40 AM

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?)

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 added inline comments.May 1 2021, 12:36 PM
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?

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 added a comment.May 5 2021, 5:46 AM

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.

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?

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

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 abandoned this revision.Wed, Jun 2, 10:47 AM

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