This is an archive of the discontinued LLVM Phabricator instance.

Add support for '#pragma clang attribute'
ClosedPublic

Authored by arphaman on Feb 15 2017, 3:03 PM.

Details

Summary

This patch adds support for the #pragma clang attribute directive that was proposed recently at http://lists.llvm.org/pipermail/cfe-dev/2017-February/052689.html.

Initially it supports the annotate, require_constant_initialization and objc_subclassing_restricted attribute (I added support for the last two to verify that only those declarations that are specified in the Attr.td subject list can receive the attribute). The attributes are parsed immediately and are applied individually to each relevant declaration. The attribute application errors aren't reported more than once. The annotate attribute, which doesn't have the subject list, is applied only to those declarations that can receive explicit GNU-style attributes.

Thanks

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Feb 15 2017, 3:03 PM
efriedma added inline comments.
docs/LanguageExtensions.rst
2344 ↗(On Diff #88612)

I think "to each declaration individually" needs to expanded on a bit here. It's not clear how this interacts with namespaces, or classes, or function declarations, or function definitions, or templates. For example, if you declare a function inside of a #pragma clang attribute push(annotate("custom")), does that add an attribute to each parameter of the function?

2349 ↗(On Diff #88612)

I'm wondering if we can tweak the approach so that we don't have to separately define how this works for each attribute; for example, #pragma clang attribute_function_declaration push(...) would apply to each function declaration, #pragma clang attribute_global_variable_declaration push(...) would apply to each global variable declaration, etc.

arphaman added inline comments.Feb 15 2017, 4:37 PM
docs/LanguageExtensions.rst
2349 ↗(On Diff #88612)

I agree with this idea, I think it would be useful to have the ability to specify the target declarations. I do think it would be better to use the 'clang attribute' umbrella pragma, and maybe add an extra argument to the 'push', e.g.:

#pragma attribute push (annotate("functions-only"), applicable_to=function) // or maybe received_by=?
#pragma attribute push (annotate("struct+enum"), applicable_to=struct, applicable_to=enum)

I don't understand why it only supports some attributes. Is there some handling that needs to take place (I don't see anything obvious in this patch)? If most attributes will "just work", I'd much rather opt-out the few exceptions (which we can then explicitly document), if any, than using this opt-in solution.

I don't understand why it only supports some attributes. Is there some handling that needs to take place (I don't see anything obvious in this patch)? If most attributes will "just work", I'd much rather opt-out the few exceptions (which we can then explicitly document), if any, than using this opt-in solution.

I think it would be reasonable to use an opt-out approach. I think the following set of initial rules should determine when an attribute should not be supported by the pragma:

  • If an attribute is late parsed
  • Or if it has no GNU/CXX11 spelling
  • Or if it has no subject list, or its subject list is empty (excluding annotate)
  • Or if it derives from StmtAttr or TypeAttr

I don't understand why it only supports some attributes. Is there some handling that needs to take place (I don't see anything obvious in this patch)? If most attributes will "just work", I'd much rather opt-out the few exceptions (which we can then explicitly document), if any, than using this opt-in solution.

I think it would be reasonable to use an opt-out approach. I think the following set of initial rules should determine when an attribute should not be supported by the pragma:

  • If an attribute is late parsed
  • Or if it has no GNU/CXX11 spelling
  • Or if it has no subject list, or its subject list is empty (excluding annotate)
  • Or if it derives from StmtAttr or TypeAttr

SGTM.

arphaman updated this revision to Diff 88764.Feb 16 2017, 12:29 PM
arphaman marked an inline comment as done.

The updated patch switches over to the opt-out approach, allows the C++11 style syntax and improves documentation.

arphaman added inline comments.Feb 16 2017, 12:30 PM
docs/LanguageExtensions.rst
2349 ↗(On Diff #88612)

I think that the further tweaks that control which declarations receive the attributes should be kept for a follow-up patch.

efriedma added inline comments.Feb 16 2017, 1:05 PM
docs/LanguageExtensions.rst
2349 ↗(On Diff #88612)

I'm not sure we can wait to address this, especially if we're turning this on for all attributes by default. There's a compatibility hazard here: if the push doesn't specify what declarations it applies to, we can never change AttributeAppliesToDecl for any existing attribute.

arphaman added inline comments.Feb 16 2017, 1:18 PM
docs/LanguageExtensions.rst
2349 ↗(On Diff #88612)

Are you saying that we should never allow a push without specifying which declarations should the attribute apply to? I think that would make this feature less useful, as some attributes have a well defined set of declarations that they apply to. Even if we will change the attribute's subjects in the future, I think it might be more likely that the users would've wanted to apply the attribute to the updated compiler-determined subject list.

aaron.ballman added inline comments.Feb 16 2017, 1:26 PM
docs/LanguageExtensions.rst
2418 ↗(On Diff #88764)

So then this code does not produce an error?

#pragma clang attribute push(hot)

void func(void) __attribute__((cold));

#pragma clang attribute pop
2428 ↗(On Diff #88764)

This doesn't make clear what would happen with something like:

#pragma clang attribute push(cdecl)

void function(void (*fp)(int));

#pragma clang attribute pop

Naively, I would expect the cdecl attribute to appertain to both function and fp, but I'm guessing that's not what actually happens (because cdecl is both a decl and a type attribute at the same time).

Also, why is this not available for __declspec attributes?

Why do C++ attributes require the syntax to be present in the #pragma, but GNU-style attributes do not? There are also MS-style attribute (currently unsupported, but there have been some efforts to include them in the past) and __declspec attributes as well. Why not require the syntax so that any of them can be supported?

include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

I have strong reservations about this -- users are going to have no idea what attributes are and are not supported because they're not going to know whether the attribute has a subject list or requires delayed parsing. We have a considerable number of attributes for which the Subjects line is currently commented out simply because no one has bothered to fix that. This means those attributes can't be used with this pragma until someone fixes that, but when it happens, they magically can be used, which is a good thing. But the converse is more problematic -- if there's an existing Subjects line that gets removed because a subject is added that is difficult to express in TableGen it may break user code.

We can fix the discoverability issues somewhat by updating the documentation emitter to spit out some wording that says when an attribute is/is not supported by this feature, but that only works for attributes which have documentation and so it's not a particularly reliable workaround.

rnk added inline comments.Feb 16 2017, 1:33 PM
docs/LanguageExtensions.rst
2349 ↗(On Diff #88612)

I agree, users shouldn't have to repeat that attribute "target" applies to functions and attribute "warn_unused" applies to record types. Asking the user to figure it out just makes it harder to use.

arphaman added inline comments.Feb 16 2017, 1:52 PM
include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

I have strong reservations about this -- users are going to have no idea what attributes are and are not supported because they're not going to know whether the attribute has a subject list or requires delayed parsing. We have a considerable number of attributes for which the Subjects line is currently commented out simply because no one has bothered to fix that. This means those attributes can't be used with this pragma until someone fixes that, but when it happens, they magically can be used, which is a good thing. But the converse is more problematic -- if there's an existing Subjects line that gets removed because a subject is added that is difficult to express in TableGen it may break user code.

That's a good point. I think we can address this problem by creating a test that verifies the list of attributes that support the pragma. This would allow us to ensure that no attributes loose the ability to use the pragma.

We can fix the discoverability issues somewhat by updating the documentation emitter to spit out some wording that says when an attribute is/is not supported by this feature, but that only works for attributes which have documentation and so it's not a particularly reliable workaround.

We can enforce the rule that the attributes can only be used with '#pragma clang attribute' when they're documented. This way we can ensure that all attributes that can be used with the pragma are explicitly documented.

aaron.ballman added inline comments.Feb 16 2017, 3:20 PM
include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

That's a good point. I think we can address this problem by creating a test that verifies the list of attributes that support the pragma. This would allow us to ensure that no attributes loose the ability to use the pragma.

That would be good.

We can enforce the rule that the attributes can only be used with '#pragma clang attribute' when they're documented. This way we can ensure that all attributes that can be used with the pragma are explicitly documented.

This addresses the concern about discoverability, but it worsens the concerns about fragility. The biggest problem is: the user has very little hope of understanding what attributes will apply to what declarations with which version of the compiler they're using. With this sort of thing, the act of us adding documentation can impact the behavior of a user's program from one release to the next.

While I can imagine this pragma reducing some amount of code clutter, it is far too "magical" for me to feel comfortable with it (at least in the proposed form). I cannot read the user's source code and understand what attributes are going to be applied to which declarations, and that's not a good story for usability of the feature.

arphaman added inline comments.Feb 16 2017, 6:13 PM
include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

What about the following idea:

The user has to include a strict set of declarations that receive the attribute explicitly, e.g.:

#pragma clang attribute push([[noreturn]], apply_to={ function })

The compiler then would verify that the set of declarations (in this case just function) is strictly identical to the built-in compiler-defined set of declarations that can receive the attribute (i.e. the strict set has to include all of the supported declarations). This will ensure that the user will know what declarations receive the attribute. If the compiler changes the set of allowed attributes in the future, e.g. if clang can now apply [[noreturn]] to enums for some reason, the user would get a compilation error saying something like the [[noreturn]] attribute also applies to enum declarations with a fix-it that inserts the , enum.

The compiler would also provide code-completion and fix-it support that insert the default strict set into the pragma.

Then it would also provide an additional mode in which the explicitly specified list of declarations doesn't have to be strictly identical, but it still should be a subset of declarations that can receive the attribute, e.g.

#pragma clang attribute push(internal_linkage, apply_only_to={ function })

Clang could then extend the internal_linkage attribute so that it would apply to enums, which wouldn't trigger a compilation error since the set of declarations that receive the attribute isn't strict. However, the user would get a compilation error if clang removed function from the list of supported declarations.

aaron.ballman added inline comments.Feb 21 2017, 6:48 AM
include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

On the whole, this makes me more comfortable with the idea, though some questions remain.

How do you envision this working with custom subjects, like tls_model which only applies to variables with thread-local storage?

I see two options there:

(0) The user has to use a specific "strict" entity, like tls_var. This seems unlikely to scale well, unless we automate the generation and checking for those entities. It also seems somewhat user-hostile because users are unlikely to be able to guess what to write for that subject.

(1) The user specifies the base subject kind with no extra requirements (i.e., variables, but we don't care if they're thread-local). It would still be somewhat fragile because it means that custom subject lines are more subject to inadvertently changing the user's code, but it also means the user doesn't need to keep an arbitrary list of weird attribute subjects in their head.

The code completion and fix-it support would go a long ways towards helping with either choice.

How would both modes work for attributes without any Subjects list at all? I could imagine the non-strict mode simply attempting to apply the attribute to whatever subjects are listed, and the compiler would diagnose anything that was incorrect. e.g.,

#pragma clang attribute push([[gnu::cdecl]], apply_only_to={variables})
int x; // Gets diagnosed as a bad attribute subject.?

Perhaps if there's no Subjects list, there's no strict-mode support (attempting use it is diagnosed as an error)?

arphaman added inline comments.Feb 24 2017, 11:30 AM
include/clang/Basic/Attr.td
308–311 ↗(On Diff #88764)

How do you envision this working with custom subjects, like tls_model which only applies to variables with thread-local storage?

I see two options there:

(0) The user has to use a specific "strict" entity, like tls_var. This seems unlikely to scale well, unless we automate the generation and checking for those entities. It also seems somewhat user-hostile because users are unlikely to be able to guess what to write for that subject.

(1) The user specifies the base subject kind with no extra requirements (i.e., variables, but we don't care if they're thread-local). It would still be somewhat fragile because it means that custom subject lines are more subject to inadvertently changing the user's code, but it also means the user doesn't need to keep an arbitrary list of weird attribute subjects in their head.

It's tough to pick a one or the other, but in general I think we should make it more specific. I think we can try a syntax that's similar to clang's ASTMatchers:

e.g.

#pragma clang attribute push(tls_model, apply_to = { variable(isThreadLocal()) })

The code completion and fix-it support would go a long ways towards helping with either choice.

How would both modes work for attributes without any Subjects list at all? I could imagine the non-strict mode simply attempting to apply the attribute to whatever subjects are listed, and the compiler would diagnose anything that was incorrect. e.g.,

#pragma clang attribute push([[gnu::cdecl]], apply_only_to={variables})
int x; // Gets diagnosed as a bad attribute subject.?
Perhaps if there's no Subjects list, there's no strict-mode support (attempting use it is diagnosed as an error)?

Initially we will only allow attributes that a have subject list (except annotate). But for something like annotate, we should prohibit the strict version (apply_to) and only offer the non-strict version apply_only_to.

arphaman updated this revision to Diff 90696.EditedMar 6 2017, 7:29 AM

The updated patch changes how the #pragma clang attribute push directive operates. Now the user must specify the set of declarations that will receive the attribute in the #pragma. The declarations are specified using subject match rules which are based on attribute subjects and declaration nodes. The user can either use apply_to strict subject set which protects from future compiler additions or apply_only_to which must be a subset of the strict subset set.

arphaman updated this revision to Diff 91542.Mar 13 2017, 5:34 AM

Rebase on ToT

I really like the way this feature is shaping up! I apologize for how long it took to review the code (I was out for work for two weeks and this is a pretty extensive patch). Thank you for the continued efforts on this.

docs/LanguageExtensions.rst
2323 ↗(On Diff #91542)

Might want to mention explicitly that these directives nest.

2337 ↗(On Diff #91542)

just -> only

2352 ↗(On Diff #91542)

Drop the "Firstly, ".

2356 ↗(On Diff #91542)

"otherwise it results in an error." ?

2387 ↗(On Diff #91542)

declarations (plural).

2388 ↗(On Diff #91542)

of compiler -> of the compiler

2389 ↗(On Diff #91542)

will start supporting -> supports

2497 ↗(On Diff #91542)

We should also support __declspec attributes shouldn't we (I see no reason to support 2 out of the 3 attribute styles)? If so, the docs should state that, and if not, I'd like to understand why and the docs should still state that.

test/Sema/pragma-attribute.c
2 ↗(On Diff #91542)

-ast-dump tests live in Misc rather than Sema.

test/SemaCXX/pragma-attribute-subject-match-rules.cpp
170 ↗(On Diff #91542)

Please add the newline (and this test should be in Misc as well).

test/SemaCXX/pragma-attribute.cpp
1 ↗(On Diff #91542)

This should also be in Misc

test/SemaObjC/pragma-attribute-subject-match-rules.m
107 ↗(On Diff #91542)

Please add the newline, and move the test into Misc.

test/SemaObjC/pragma-attribute.m
2 ↗(On Diff #91542)

This test should also be in Misc.

aaron.ballman added inline comments.Mar 18 2017, 10:22 AM
docs/LanguageExtensions.rst
2378 ↗(On Diff #91542)

We don't describe any below, but should.

2430 ↗(On Diff #91542)

Instead of is_method, I think we should use is_instance, is_member, or something else. C++ doesn't really use the terminology "method", but Objective-C does, which is why this could be confusing. It might also be good to mention the behavior of a static member functions.

Whatever terminology gets picked, you should use that phrasing wherever you say "C++ method".

2435 ↗(On Diff #91542)

Replace the exclamation point with a full stop.

2440 ↗(On Diff #91542)

Insert a comma after class.

2443 ↗(On Diff #91542)

Does unless() work in other combinations? e.g., can I use record(unless(is_enum))? I'm not saying that it has to, but it would be good to document one way or the other.

2448 ↗(On Diff #91542)

I'd prefer these to be named enumeration and enumerator, enum and enum_constant, or something. enum_case is too novel of a term for me.

2451 ↗(On Diff #91542)

Comma after parameters.

2462 ↗(On Diff #91542)

Same question here as above with the unless(), can I specify any is_foo there?

2486 ↗(On Diff #91542)

Perhaps name this one objc_block to be consistent with the other objective-c features?

2502 ↗(On Diff #91542)

Typo: compilation error

include/clang/Basic/Attr.td
288 ↗(On Diff #91542)

space after the comma before list.

include/clang/Basic/DiagnosticParseKinds.td
999–1002 ↗(On Diff #91542)

I would combine these two and use a %select to distinguish between them.

1005–1008 ↗(On Diff #91542)

Same for these.

include/clang/Basic/DiagnosticSemaKinds.td
752 ↗(On Diff #91542)

subject set -> attribute subject set (since you used that phrasing in another diagnostic and the documentation).

761–763 ↗(On Diff #91542)

I'd combine these as well.

766 ↗(On Diff #91542)

We should have a test for a file that has an unmatched push as well. e.g.,

#pragma clang attribute push(annotate("blah"), apply_only_to=function)
\EOF
include/clang/Sema/Sema.h
8146 ↗(On Diff #91542)

well formed -> well-formed

8155 ↗(On Diff #91542)

well formed -> well-formed

8160 ↗(On Diff #91542)

I worry about new calls to ProcessDeclAttributes() that don't have a nearby call to AddPragmaAttributes(). I wonder if there's a way that we can combat that likely misuse?

lib/Parse/ParsePragma.cpp
1012 ↗(On Diff #91542)

Please don't use auto here, since the type is not immediately obvious from the initialization and it's not something trivially easy to figure out, like an iterator.

1060–1076 ↗(On Diff #91542)

Can this duplicate code be factored into a helper function? That might also help with the crazy formatting of the calls to Diag(), too. Same suggestion applies below.

1135 ↗(On Diff #91542)

It's usually good practice to have the && "This is what went wrong" text for all the asserts. Same comment applies elsewhere.

1158 ↗(On Diff #91542)

Is this going to cause a memory leak for those attributes that were parsed but dropped?

1201 ↗(On Diff #91542)

Might as well cache this above.

2556 ↗(On Diff #91542)

You should mention where the reader can find more information about subject-set.

2590 ↗(On Diff #91542)

Not that this impacts your patch, but we really should modify the BalancedDelimiterTracker to work for pragmas so that we don't have to do this dance manually.

lib/Sema/SemaAttr.cpp
401 ↗(On Diff #91542)

Why not move these static functions into the anonymous namespace (and remove the static)?

419 ↗(On Diff #91542)

I think it should be ", and ".

The down-side to having "and" in here is that it makes it harder to localize the diagnostics, but I think it's fine in this instance. I only mention it in case you have a better idea.

477 ↗(On Diff #91542)

Don't use auto.

499 ↗(On Diff #91542)

Don't use auto.

557 ↗(On Diff #91542)

Don't use auto.

565–566 ↗(On Diff #91542)

Can use use std::copy() with a back_inserter.

602–603 ↗(On Diff #91542)

I think that makes sense (I can imagine someone writing the pragma push and pop before writing all their declarations), but I might be misunderstanding the question.

631 ↗(On Diff #91542)

Do we only care about errors, or should we also care about warnings? Incorrect attributes that do not affect code generation often are treated as warnings (and the attribute is simply ignored) rather than errors, so the user may want to know about those issues.

test/lit.cfg
287 ↗(On Diff #91542)

Why is this change (and the other lit change) required?

utils/TableGen/ClangAttrEmitter.cpp
1558 ↗(On Diff #91542)

Don't use auto.

1639 ↗(On Diff #91542)

Can use const auto *

1648 ↗(On Diff #91542)

Can use const auto *.

1652 ↗(On Diff #91542)

Can use const auto *.

1669 ↗(On Diff #91542)

Can use const auto &.

1711 ↗(On Diff #91542)

Can use const auto *.

1712–1713 ↗(On Diff #91542)

No need to store It.

1738 ↗(On Diff #91542)

Can use const auto *.

Looking over the most recent version, I'm happy with the general semantics of push with apply_only_to. I'm not sure I see the point of apply_to: it doesn't allow the user to do anything that can't be done with apply_only_to. Also, if the apply_to list for an attribute ever changes, it becomes impossible to write code which supports both versions of the compiler.

Looking over the most recent version, I'm happy with the general semantics of push with apply_only_to. I'm not sure I see the point of apply_to: it doesn't allow the user to do anything that can't be done with apply_only_to. Also, if the apply_to list for an attribute ever changes, it becomes impossible to write code which supports both versions of the compiler.

You bring up a really good point about compiler versioning -- it would be pretty awful to force the user to use #ifs to deal with that.

I believe apply_to is somewhat useful so that the user knows what an attribute actually appertains to, and get a diagnostic if we ever extend the list of things their particular attribute can appertain to so they are forced to decide whether they want that new behavior or not.

@arphaman, what do you think about the idea of only having apply_to with the same semantics as you currently have for apply_only_to?

Looking over the most recent version, I'm happy with the general semantics of push with apply_only_to. I'm not sure I see the point of apply_to: it doesn't allow the user to do anything that can't be done with apply_only_to. Also, if the apply_to list for an attribute ever changes, it becomes impossible to write code which supports both versions of the compiler.

This is a good point that I haven't considered.

You bring up a really good point about compiler versioning -- it would be pretty awful to force the user to use #ifs to deal with that.

I believe apply_to is somewhat useful so that the user knows what an attribute actually appertains to, and get a diagnostic if we ever extend the list of things their particular attribute can appertain to so they are forced to decide whether they want that new behavior or not.

@arphaman, what do you think about the idea of only having apply_to with the same semantics as you currently have for apply_only_to?

I would be ok with that. We could merge apply_to and apply_only_to into a single apply_to matching rule set specifier (it would behave like apply_only_to).

I guess one downside would be is that it will become harder to fill out all the match rules if one wants to apply an attribute to all possible declarations. I suppose the attribute documentation generator can be updated to include the full match rule set for each attribute instead of just yes/no in the #pragma clang attribute documentation column.

I would be ok with that. We could merge apply_to and apply_only_to into a single apply_to matching rule set specifier (it would behave like apply_only_to).

That sounds sensible to me.

I guess one downside would be is that it will become harder to fill out all the match rules if one wants to apply an attribute to all possible declarations. I suppose the attribute documentation generator can be updated to include the full match rule set for each attribute instead of just yes/no in the #pragma clang attribute documentation column.

Okay, so here's a possibly crazy idea (and it may be way too magical): what if #pragma clang attribute push(foo) generated the fix-it hint to suggest all of the targets the attribute can apply to? Alternatively, what if any malformed parsing of #pragma clang attribute push(foo) automatically do this? We know the user is trying to apply attributes to declarations, and we know which attribute they're trying for, so it somewhat stands to reason that the rest of the syntax can be supplied for them... and we need *something* in apply_to, so why not default to everything?

I would be ok with that. We could merge apply_to and apply_only_to into a single apply_to matching rule set specifier (it would behave like apply_only_to).

That sounds sensible to me.

I guess one downside would be is that it will become harder to fill out all the match rules if one wants to apply an attribute to all possible declarations. I suppose the attribute documentation generator can be updated to include the full match rule set for each attribute instead of just yes/no in the #pragma clang attribute documentation column.

Okay, so here's a possibly crazy idea (and it may be way too magical): what if #pragma clang attribute push(foo) generated the fix-it hint to suggest all of the targets the attribute can apply to? Alternatively, what if any malformed parsing of #pragma clang attribute push(foo) automatically do this? We know the user is trying to apply attributes to declarations, and we know which attribute they're trying for, so it somewhat stands to reason that the rest of the syntax can be supplied for them... and we need *something* in apply_to, so why not default to everything?

Something like that should work. Although we probably still want to have them in the documentation as well.

arphaman updated this revision to Diff 93466.Mar 30 2017, 5:39 AM
arphaman marked 45 inline comments as done.

The rebased patch includes the following changes:

  • apply_to and apply_only_to are merged into one apply_to specifier with apply_only_to semantics.
  • The subject match rule list that's supported by an attribute is now reported in the fix-it for some of the parse diagnostics.
  • GNU style attributes must now use __attribute__` syntax.
  • __declspec attributes are now supported.
  • Errors and warnings are now always emitted for each receiving declaration.
  • The compiler now warns about unused attributes.
  • The compiler now warns about unterminated pushes.
  • Objective-C categories are now supported as well.

I couldn't find a good/suitable way to include the attribute subject match rule list that's supported by an attribute in the documentation. Maybe we could specify these lists in a new document?

docs/LanguageExtensions.rst
2430 ↗(On Diff #91542)

I think function(is_member) should work well.

2443 ↗(On Diff #91542)

No, right now unless is very rigid and only works with a select set of match sub rules which are needed for attributes. I have noted that in the docs.

2448 ↗(On Diff #91542)

I think enum_constant would work. I was thinking about enumeration and enumerator before, but they just look too alike so it might be easy to confuse them.

2486 ↗(On Diff #91542)

Blocks can be used outside of Objective-C.

2497 ↗(On Diff #91542)

Sure. A proper consistent style with __attribute__ as you mentioned earlier is better for us as well due to legacy macros. Now __attribute__ is required and I also added support for the __declspec attributes.

include/clang/Sema/Sema.h
8160 ↗(On Diff #91542)

Perhaps we could just always call AddPragmaAttributes from ProcessDeclAttributes (after the declarator attributes are applied). That would mean that #pragma clang attribute will apply its attributes earlier than other pragmas or implicit attributes.

lib/Parse/ParsePragma.cpp
1158 ↗(On Diff #91542)

Possibly, I fixed this up.

lib/Sema/SemaAttr.cpp
602–603 ↗(On Diff #91542)

That would make sense, I agree. I added a warning that warns about unapplied attributes in push pop regions. However, my original question was more about correctness of the attribute which we can’t check semantically unless we apply it first. I don’t think it’s really needed though, so I removed the “fixme”.

631 ↗(On Diff #91542)

I think it's important to try and get the warnings right as well. I think it would make sense to emit the warnings for each declaration that receives the attribute (including the first one). One issue that I found was that warnings didn't tell the user which declaration was receiving the attribute. The updated patch fixes that by ensuring that warnings are emitted for all receivers and that each warning has an additional note that points to the declaration that receives the attribute.

This made me also rethink the SFINAE trap completely. I decided to avoid it. Originally I thought that it would be nice to have it so that we can avoid duplicate attribute-related errors when applying an attribute to each declaration. There were a couple of issues with that approach:

  • Attributes have receiver dependent errors/warnings that should be reported for each declaration. Previously the patch was inconsistent for such errors; it stopped applying the attribute only when the first declaration had a receiver dependent error, but not the others.
  • ProcessDeclAttributeList actually applies the attribute, so we can't call it twice. In particular, some attributes might modify the attribute list when reporting warnings during the first call, so the user won't see the warnings when they're reported during the second call.

The updated patch now always report errors with a note that points to the receiver declaration. As a downside it would mean that when there are actual, non-receiver dependent attribute errors then the user is gonna get a bunch of duplicate errors when applying to multiple declarations.

test/lit.cfg
287 ↗(On Diff #91542)

test/Misc/pragma-attribute-supported-attributes-list.test Uses this path to find Clang's Attr.td file.

Thank you for the update! I think this is getting close, though I have a few more comments inline.

docs/LanguageExtensions.rst
2338 ↗(On Diff #93466)

Do we want to have the same restriction for GNU and declspec attributes, that only a single one can appear? If so, we should mention it more generally.

2420 ↗(On Diff #93466)

Are member variables included in this list? Perhaps this should also mention that it does not apply to Objective-C ivars.

include/clang/Basic/AttrSubjectMatchRules.h
17 ↗(On Diff #93466)

Can remove the newline.

include/clang/Basic/DiagnosticSemaKinds.td
765 ↗(On Diff #93466)

Typo: pragm

Is there value in this being in its own warning group from pragma-attribute-unused? It might make sense to disable any pragma attribute warning under the same group name, but I don't feel super strongly about it.

include/clang/Sema/Sema.h
8160 ↗(On Diff #91542)

I was wondering whether that would be a reasonable approach. I *think* it seems reasonable, however.

8160 ↗(On Diff #91542)

Phab won't let me edit my comment for some reason, so replying a second time. I don't think it's a problem for the attributes to be added earlier because attributes are essentially an unordered list on the declarations anyways. If this really is a problem, I would hope that there's a test that will suddenly start failing for us.

lib/Sema/SemaAttr.cpp
578 ↗(On Diff #93466)

Perhaps adding a FixIt here would be a kindness?

631 ↗(On Diff #91542)

Thank you, I think this approach makes sense. The downside is unfortunate, but doesn't seem to be actively harmful if I'm understanding properly.

Adding Richard, since this is a fairly extensive change to the frontend and he may have some opinions as well.

arphaman updated this revision to Diff 94202.Apr 5 2017, 4:49 AM
arphaman marked 5 inline comments as done.

Rebase and address Aaron's comments.

lib/Sema/SemaAttr.cpp
578 ↗(On Diff #93466)

Where would the fix-it point to? I think only the user will know the location at which they meant to insert #pragma clang attribute pop.

aaron.ballman added inline comments.Apr 5 2017, 5:13 AM
lib/Sema/SemaAttr.cpp
578 ↗(On Diff #93466)

Given that it's a warning rather than an error, and our recovery mechanism is to effectively pop the pragma at the end of the TU, I was thinking it could be added at the end of the TU. However, you are correct that it's probably not where the programmer needs it, so a FixIt likely isn't appropriate. Does that suggest the warning should be an error instead?

arphaman updated this revision to Diff 94208.Apr 5 2017, 5:49 AM
arphaman marked an inline comment as done.

The unterminated '#pragma clang attribute push' at end of file diagnostic is now an error.

aaron.ballman added inline comments.Apr 5 2017, 4:34 PM
docs/LanguageExtensions.rst
2380 ↗(On Diff #94208)

Might as well add an example for __declspec (since the other two have it), and move the restriction about the number of attributes supplied to its own paragraph for greater clarity.

include/clang/Basic/DiagnosticGroups.td
462 ↗(On Diff #94208)

Should PragmaClangAttribute be added here as well?

arphaman updated this revision to Diff 94366.Apr 6 2017, 7:06 AM
arphaman marked 2 inline comments as done.

Addressed Aaron's comments.

aaron.ballman accepted this revision.Apr 7 2017, 7:44 AM

This LGTM, but you should give @rsmith some time to review before committing in case he catches something I've missed.

This revision is now accepted and ready to land.Apr 7 2017, 7:44 AM
This revision was automatically updated to reflect the committed changes.

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute changes the behavior of Clang. That does not seem acceptable to me: documentation improvements should not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce), I think the best way to do that is to add another flag on the Attr instance in the .td file to opt the attribute out of #pragma clang support, opt out all the undocumented attributes, and add an error to TableGen if we find that an undocumented attribute has #pragma clang support enabled.

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute changes the behavior of Clang. That does not seem acceptable to me: documentation improvements should not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce), I think the best way to do that is to add another flag on the Attr instance in the .td file to opt the attribute out of #pragma clang support, opt out all the undocumented attributes, and add an error to TableGen if we find that an undocumented attribute has #pragma clang support enabled.

I've changed how we determine whether #pragma clang attribute is usable in rL341009 so that future documentation changes don't change clang's behavior. But we should still consider whether the attributes that were explicitly opted out of this feature in that revision really should be. What harm does allowing this feature for attributes that we don't document do? Most of those attributes are documented by GCC anyway, and the user experience of opting them out seems inconsistent and arbitrary. If the goal is to use this pragma as a forcing function to get documentation written for the attributes, it hasn't worked.

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute changes the behavior of Clang. That does not seem acceptable to me: documentation improvements should not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce

I also don't quite see why this is useful.

), I think the best way to do that is to add another flag on the Attr instance in the .td file to opt the attribute out of #pragma clang support, opt out all the undocumented attributes, and add an error to TableGen if we find that an undocumented attribute has #pragma clang support enabled.

It seems arbitrary not to support an attribute from #pragma clang attribute just because it was once undocumented. It does seem incrementally better, but still not great.

(FYI, @arphaman is on vacation until next week so he's unlikely to respond promptly.)

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce

I also don't quite see why this is useful.

OK, D51507 sent out for review to remove the rule.