[PATCH] D30009: Add support for '#pragma clang attribute'

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:308-311
+  // - An attribute requires delayed parsing (LateParsed is on)
+  // - An attribute has no GNU/CXX11 spelling
+  // - An attribute has no subject list
+  // - An attribute derives from a StmtAttr or a TypeAttr
----------------
aaron.ballman wrote:
> arphaman wrote:
> > aaron.ballman wrote:
> > > arphaman wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > > 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.
> > > > 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.
> > 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.
> 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)?
> 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`.



Repository:
  rL LLVM

https://reviews.llvm.org/D30009