Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Mar 18 2017, 10:22 AM
docs/LanguageExtensions.rst
2337 ↗(On Diff #91542)

just -> only

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?

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.

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.