Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Resurrecting this old patch. This is the fourth of four patches to make it possible for clang plugins to define attributes.
clang/examples/Attribute/Attribute.cpp | ||
---|---|---|
26 | It would be really handy for the example to show how to list subjects for the declaration attribute, as that's a very common need. | |
30 | Nothing here suggests the argument must be a string. It would be ideal to show users how to list out arguments and their types. | |
33–34 | Showing how to have a vendor namespace would also be especially helpful. As a point of design, I wonder if we should be documenting (or diagnosing?) attributes from plugins that are added to the clang vendor namespace, as that does not seem like something we want to encourage? | |
36 | It is unclear to me how a user would add an attribute that needs to follow the merge pattern we use in mergeDeclAttribute(). | |
54 | Do we have a way for plugin authors to introduce their own diagnostics, or are they limited to just what diagnostics we already provide? |
clang/test/Frontend/plugin-attribute.cpp | ||
---|---|---|
17 | Typo: scope |
clang/examples/Attribute/Attribute.cpp | ||
---|---|---|
36 | This is not a valid spelling for an attribute (we should probably assert that an attribute-scoped-token does not have an empty scope). | |
36 | That... seems like a problem that we should fix (or at least have a plan for fixing). Merging attributes is important so that we can have attributes with consistency checks between declarations for anything other than basic attribute inheritance. FWIW, there are 20+ attributes that already need this functionality, so it does get used relatively often. | |
43 | I don't think D can ever be null, can it? |
clang/examples/Attribute/Attribute.cpp | ||
---|---|---|
36 | "::example" is actually how an unscoped spelling is specified - normalizeName in Basic/Attributes.cpp adds a "::" to the start if there's no scope, and the tablegen-generated getAttrKind expects it as well. | |
43 | I don't know, but this is how the auto-generated diagAppertainsToDecl functions do it. |
I've been looking into attribute merging, and as far as I can tell there's two things going on:
- In SemaDeclAttr.cpp, the various handleXyzAttr functions may call mergeXyzAttr to handle clashes with other attributes within the same declaration.
- In SemaDecl.cpp, several functions call mergeDeclAttributes, which calls mergeXyzAttr, to handle clashes with attributes in a previous declaration of the same thing.
For the first kind, the handleDeclAttribute function defined for a plugin attribute can check for and handle clashes with other attributes. For the second kind, at that point we're using subclasses of Attr which means we're beyond the scope of plugin attributes which just work in the realm of ParsedAttrs.
So I think things are OK, though I guess it depends on what exactly your plugin is doing - our use-case is that the plugin-defined attribute is just used to set an AnnotateAttr, which is used to tell a plugin-defined LLVM pass that it needs to do something in that function, so we don't need anything special to happen with regards to attribute merging.
Agreed.
For the first kind, the handleDeclAttribute function defined for a plugin attribute can check for and handle clashes with other attributes.
Agreed.
For the second kind, at that point we're using subclasses of Attr which means we're beyond the scope of plugin attributes which just work in the realm of ParsedAttrs.
Yeah, we are using semantic attributes at that point rather than parsed attributes.
So I think things are OK, though I guess it depends on what exactly your plugin is doing - our use-case is that the plugin-defined attribute is just used to set an AnnotateAttr, which is used to tell a plugin-defined LLVM pass that it needs to do something in that function, so we don't need anything special to happen with regards to attribute merging.
I think things are fine for your example plugin. The scenario I'm worried about is when users want to add their own semantic attributes that need to be merged, or if they're using an existing semantic attribute but need to add custom merging logic to it for their own needs.
clang/examples/Attribute/Attribute.cpp | ||
---|---|---|
36 | It's not how an unscoped spelling is specified by either C or C++ -- in both languages, that would be an error. I don't think we want to leak this implementation detail out to the user as it will be confusing to them. | |
43 | I'll fix that up -- I just verified that D can never be null. Sema::ProcessDeclAttributeList dereferences D unconditionally and has for a long time. |
Hello John!
I was encouraged by Erich after a talk I had with him and Aaron on IRC to contact you about an use case for this plugin.
I wanted to use user-specified/unknown attributes to annotate code and was told that they are not propagated through
the AST and therefore it is not possible at the moment to use them for that purpose. Aaron's and Erich's idea was to
allow kind of no-op attributes in the plugin so that the compiler would not issue a warning about the unknown attribute.
This would be helpful for the user being able to define a list of attributes the user knows and expects, so that a compilation
would not be spammed unnecessarily and misspelled attributes would still stand out.
Being able to get the cursor or displayName of the attribute would essentially be enough to use it in tooling.
We can use attribute((annotate(smth))) at the time being to achieve the same goal, but it is much more writing to do
and also vendor specific. Being able to do the same with attributes would give them a real purpose, other than having
to be accepted and not causing an error. Also they have to be supported by the language and we don't have to use
macros to annotate the code.
To summarize it would be nice to have:
- user supplied unknown attributes to suppress a warning
- unknown attributes propagated in the AST
I hope I summarized this correctly and could get through the gist of this idea.
Looking forward to hearing from you.
Best regards, Jasmin
For reference: the current set of patches John has been working on allow you to load a plugin to let the frontend parse an attribute with an arbitrary spelling, but it does not have a way for you to register a new attribute AST node that gets attached to anything in the AST. Instead, you create an existing semantic attribute (such as an AnnotateAttr) and add that to the AST.
I think this is a great request to keep in mind for John or anyone else who wants to extend this plugin functionality to allow for generating custom attribute AST nodes (which is of interest for tools like clang static analyzer or clang tidy plugins that may consume the clang AST). This functionality would also have to keep the libclang C indexing interface in mind, as that's another area of the compiler where this functionality would be useful.
Hmm... I wonder if there is value to having these plugin-attributes auto-create a PluginAnnotateAttr or something (some otherwise bikeshedded attribute) for exactly Jasmin's use case. That way the plugin author simply needs to list the possible spellings.
A few small changes:
- Don't start C++11 spelling with :: (changes to name normalisation to allow this are in D76704)
- Don't check that the Decl is null in diagAppertainsToDecl
- Return AttributeNotApplied on errors in handleDeclAttribute
I think this was the last remaining review for the feature to support attributes as plugins, yes? If so, can you please add a release note about the feature to the release notes?
It would be really handy for the example to show how to list subjects for the declaration attribute, as that's a very common need.