Page MenuHomePhabricator

Add an attribute plugin example
ClosedPublic

Authored by john.brawn on Mar 24 2017, 9:31 AM.

Diff Detail

Event Timeline

john.brawn created this revision.Mar 24 2017, 9:31 AM
john.brawn set the repository for this revision to rG LLVM Github Monorepo.
john.brawn added a subscriber: cfe-commits.

Resurrecting this old patch. This is the fourth of four patches to make it possible for clang plugins to define attributes.

Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2020, 6:36 AM
carwil added a subscriber: carwil.Jan 27 2020, 2:13 AM
aaron.ballman added inline comments.Feb 2 2020, 10:10 AM
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?

sorawee added inline comments.
clang/test/Frontend/plugin-attribute.cpp
17

Typo: scope

john.brawn marked an inline comment as done.

Update based on review comments.

john.brawn marked 5 inline comments as done.Feb 27 2020, 9:45 AM
john.brawn added inline comments.
clang/examples/Attribute/Attribute.cpp
26

This is done via diagAppertainsToDecl.

36

I'm guessing it's probably not possible, from a quick look at mergeDeclAttribute.

54

Custom diagnostics can be done with getCustomDiagID, and there's an example on line 54.

aaron.ballman added inline comments.Feb 27 2020, 11:55 AM
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?

Rebased and added link in documentation.

john.brawn marked 2 inline comments as done.Mar 2 2020, 10:18 AM
john.brawn added inline comments.
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.

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.

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.

Updated to match the changes to D31342

aaron.ballman added inline comments.Mar 20 2020, 11:05 AM
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.

Jasmin added a subscriber: Jasmin.Mar 23 2020, 7:07 AM

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

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.

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.

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.

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
aaron.ballman accepted this revision.Mar 24 2020, 10:54 AM

LGTM, thank you!

This revision is now accepted and ready to land.Mar 24 2020, 10:54 AM

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?

This revision was automatically updated to reflect the committed changes.