Minor patch controversial

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Minor patch controversial

James Y Knight via cfe-dev
Hi everyone!

A few weeks back we sent a minor patch to the cfe-commit list that we thought would be a non-issue once it had passed the review phase, but that was not the case, instead we were told that the patch was too controversial so we ask for a general opinion on the matter.

The current implementation of the annotate attribute supports annotation of declarations using the GNU syntax, with its data forwarded into AST and further down to IR. The supported set of declarations includes classes, variables, fields, functions and methods. The patch extends this to annotation of statements and the use of C++11 syntax but with the lack of support in LLVM IR the data is only forwarded onto AST.

Overall, this is a very minor change to what already is present and being used, but the small addition could benefit many plugin and tool developers readily today. The concept of annotations are very generic and makes it possible to insert, for example, control directives or tags directly into the source code that a plugin or tool may read and utilize. Please note that the intention of the patch is to make a small adjustment, not to alter the meaning of annotations into something it is not.

Here is an example usage:

    File: xtool.hpp
    #define XTOOL_A “xtool:directiveA”
    #define XTOOL_B “xtool:directiveB"

    File: sample.cpp
    #include <xlibrary.hpp>

    [[clang::annotate(XTOOL_A]]
    int main(int argc, char* argv[])
    {
        // This is what the patch adds, possibility to annotate a statement
        [[clang::annotate(XTOOL_B)]]
        while( N )
        {  . . .  }
        return 0;
    }

The controversy of the patch appears when comparing annotations with pluggable attributes and suggesting that the two technologies competes for the same goal. No, they do not, for the simple reason that an annotation is an annotation, nothing more, nothing less, and should stay that way. It should not have the same streamlined functionality of PA, such as proper namespacing, argument checking etc. And let us be very clear on one thing; having pluggable attributes would be a great addition to Clang and we’re not trying dissuade anyone from implementing it by promoting annotations instead. The patch does not in any way make annotations move closer to PA than it was before.

One of the complaints in the "annotations vs PA” discussion is how the functionality/information are exposed to both end-users and attribute authors, being error-prone to use. This is a complaint that actually underlines that annotations are not competing with pluggable attributes, it may be fit for some solutions but for others a more strict and controlled environment, like pluggable attributes, may be required. Diversity and different levels of support is what makes Clang superior to the competition. There is no reason to stop using an existing functionality until an alternative is available and to our humble understanding, implementing an architecture that supports PA up front and in both AST and IR probably needs a few iteration to set things straight, pushing the availability date into the distant future.

We ask for this small patch to be committed since it makes a minor enhancement (annotated statements) to an existing functionality that benefits the community of plugin and tool writers today by providing a generic and consistent way of communicating control directives/tags directly from the source code to itself, such as source code transformers, generators, collectors and similar.

Cheers,
Chris

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
<[hidden email]> wrote:

> Hi everyone!
>
> A few weeks back we sent a minor patch to the cfe-commit list that we thought would be a non-issue once it had passed the review phase, but that was not the case, instead we were told that the patch was too controversial so we ask for a general opinion on the matter.
>
> The current implementation of the annotate attribute supports annotation of declarations using the GNU syntax, with its data forwarded into AST and further down to IR. The supported set of declarations includes classes, variables, fields, functions and methods. The patch extends this to annotation of statements and the use of C++11 syntax but with the lack of support in LLVM IR the data is only forwarded onto AST.
>
> Overall, this is a very minor change to what already is present and being used, but the small addition could benefit many plugin and tool developers readily today. The concept of annotations are very generic and makes it possible to insert, for example, control directives or tags directly into the source code that a plugin or tool may read and utilize. Please note that the intention of the patch is to make a small adjustment, not to alter the meaning of annotations into something it is not.
>
> Here is an example usage:
>
>     File: xtool.hpp
>     #define XTOOL_A “xtool:directiveA”
>     #define XTOOL_B “xtool:directiveB"
>
>     File: sample.cpp
>     #include <xlibrary.hpp>
>
>     [[clang::annotate(XTOOL_A]]
>     int main(int argc, char* argv[])
>     {
>         // This is what the patch adds, possibility to annotate a statement
>         [[clang::annotate(XTOOL_B)]]
>         while( N )
>         {  . . .  }
>         return 0;
>     }
>
> The controversy of the patch appears when comparing annotations with pluggable attributes and suggesting that the two technologies competes for the same goal. No, they do not, for the simple reason that an annotation is an annotation, nothing more, nothing less, and should stay that way. It should not have the same streamlined functionality of PA, such as proper namespacing, argument checking etc. And let us be very clear on one thing; having pluggable attributes would be a great addition to Clang and we’re not trying dissuade anyone from implementing it by promoting annotations instead. The patch does not in any way make annotations move closer to PA than it was before.
>
> One of the complaints in the "annotations vs PA” discussion is how the functionality/information are exposed to both end-users and attribute authors, being error-prone to use. This is a complaint that actually underlines that annotations are not competing with pluggable attributes, it may be fit for some solutions but for others a more strict and controlled environment, like pluggable attributes, may be required. Diversity and different levels of support is what makes Clang superior to the competition. There is no reason to stop using an existing functionality until an alternative is available and to our humble understanding, implementing an architecture that supports PA up front and in both AST and IR probably needs a few iteration to set things straight, pushing the availability date into the distant future.
>
> We ask for this small patch to be committed since it makes a minor enhancement (annotated statements) to an existing functionality that benefits the community of plugin and tool writers today by providing a generic and consistent way of communicating control directives/tags directly from the source code to itself, such as source code transformers, generators, collectors and similar.

For reference, the review thread starts at:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170130/184405.html

(Unfortunately, it appears to not be threaded properly, so you may
have to search for the replies to follow along with the discussion.)

While this is a simple adjustment to what the annotate attribute
appertains to, I do not want to extend the attribute in that
direction. As stated during the review, the annotate attribute is
already problematic in that there is no indication of what the
specific annotations should appertain to, no ability to supply
arguments, no way to prevent different tools from name collisions,
etc. Basically, the annotate attribute was a very quick way to do what
it needs to do back when we didn't have a good path towards pluggable
attributes. Now that we do have the good path forward, we should not
expand the capabilities of a deficient, competing solution simply
because it's the path of least resistance -- that does not provide our
users or tool authors with a good experience. I do not find the
argument that extending the annotate attribute does not compete with
pluggable attributes because it is more error-prone to be a compelling
rationale.

As mentioned in the review, I am fine with adding the C++ spelling for
the annotate attribute as that is certainly non-controversial and is
an incremental improvement.

~Aaron

>
> Cheers,
> Chris
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <[hidden email]> wrote:
On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
<[hidden email]> wrote:
> Hi everyone!
>
> A few weeks back we sent a minor patch to the cfe-commit list that we thought would be a non-issue once it had passed the review phase, but that was not the case, instead we were told that the patch was too controversial so we ask for a general opinion on the matter.
>
> The current implementation of the annotate attribute supports annotation of declarations using the GNU syntax, with its data forwarded into AST and further down to IR. The supported set of declarations includes classes, variables, fields, functions and methods. The patch extends this to annotation of statements and the use of C++11 syntax but with the lack of support in LLVM IR the data is only forwarded onto AST.
>
> Overall, this is a very minor change to what already is present and being used, but the small addition could benefit many plugin and tool developers readily today. The concept of annotations are very generic and makes it possible to insert, for example, control directives or tags directly into the source code that a plugin or tool may read and utilize. Please note that the intention of the patch is to make a small adjustment, not to alter the meaning of annotations into something it is not.
>
> Here is an example usage:
>
>     File: xtool.hpp
>     #define XTOOL_A “xtool:directiveA”
>     #define XTOOL_B “xtool:directiveB"
>
>     File: sample.cpp
>     #include <xlibrary.hpp>
>
>     [[clang::annotate(XTOOL_A]]
>     int main(int argc, char* argv[])
>     {
>         // This is what the patch adds, possibility to annotate a statement
>         [[clang::annotate(XTOOL_B)]]
>         while( N )
>         {  . . .  }
>         return 0;
>     }
>
> The controversy of the patch appears when comparing annotations with pluggable attributes and suggesting that the two technologies competes for the same goal. No, they do not, for the simple reason that an annotation is an annotation, nothing more, nothing less, and should stay that way. It should not have the same streamlined functionality of PA, such as proper namespacing, argument checking etc. And let us be very clear on one thing; having pluggable attributes would be a great addition to Clang and we’re not trying dissuade anyone from implementing it by promoting annotations instead. The patch does not in any way make annotations move closer to PA than it was before.
>
> One of the complaints in the "annotations vs PA” discussion is how the functionality/information are exposed to both end-users and attribute authors, being error-prone to use. This is a complaint that actually underlines that annotations are not competing with pluggable attributes, it may be fit for some solutions but for others a more strict and controlled environment, like pluggable attributes, may be required. Diversity and different levels of support is what makes Clang superior to the competition. There is no reason to stop using an existing functionality until an alternative is available and to our humble understanding, implementing an architecture that supports PA up front and in both AST and IR probably needs a few iteration to set things straight, pushing the availability date into the distant future.
>
> We ask for this small patch to be committed since it makes a minor enhancement (annotated statements) to an existing functionality that benefits the community of plugin and tool writers today by providing a generic and consistent way of communicating control directives/tags directly from the source code to itself, such as source code transformers, generators, collectors and similar.

For reference, the review thread starts at:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170130/184405.html

(Unfortunately, it appears to not be threaded properly, so you may
have to search for the replies to follow along with the discussion.)

While this is a simple adjustment to what the annotate attribute
appertains to, I do not want to extend the attribute in that
direction. As stated during the review, the annotate attribute is
already problematic in that there is no indication of what the
specific annotations should appertain to, no ability to supply
arguments, no way to prevent different tools from name collisions,
etc. Basically, the annotate attribute was a very quick way to do what
it needs to do back when we didn't have a good path towards pluggable
attributes. Now that we do have the good path forward, we should not
expand the capabilities of a deficient, competing solution simply
because it's the path of least resistance -- that does not provide our
users or tool authors with a good experience. I do not find the
argument that extending the annotate attribute does not compete with
pluggable attributes because it is more error-prone to be a compelling
rationale.

Thanks, having an eye on the big picture, particularly regarding our vendor-specific extensions, is important.

How far along is the work on pluggable attributes -- is there anything that would help make progress on that?

As mentioned in the review, I am fine with adding the C++ spelling for
the annotate attribute as that is certainly non-controversial and is
an incremental improvement.

~Aaron

>
> Cheers,
> Chris
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith <[hidden email]> wrote:

> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <[hidden email]>
> wrote:
>>
>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
>> <[hidden email]> wrote:
>> > Hi everyone!
>> >
>> > A few weeks back we sent a minor patch to the cfe-commit list that we
>> > thought would be a non-issue once it had passed the review phase, but that
>> > was not the case, instead we were told that the patch was too controversial
>> > so we ask for a general opinion on the matter.
>> >
>> > The current implementation of the annotate attribute supports annotation
>> > of declarations using the GNU syntax, with its data forwarded into AST and
>> > further down to IR. The supported set of declarations includes classes,
>> > variables, fields, functions and methods. The patch extends this to
>> > annotation of statements and the use of C++11 syntax but with the lack of
>> > support in LLVM IR the data is only forwarded onto AST.
>> >
>> > Overall, this is a very minor change to what already is present and
>> > being used, but the small addition could benefit many plugin and tool
>> > developers readily today. The concept of annotations are very generic and
>> > makes it possible to insert, for example, control directives or tags
>> > directly into the source code that a plugin or tool may read and utilize.
>> > Please note that the intention of the patch is to make a small adjustment,
>> > not to alter the meaning of annotations into something it is not.
>> >
>> > Here is an example usage:
>> >
>> >     File: xtool.hpp
>> >     #define XTOOL_A “xtool:directiveA”
>> >     #define XTOOL_B “xtool:directiveB"
>> >
>> >     File: sample.cpp
>> >     #include <xlibrary.hpp>
>> >
>> >     [[clang::annotate(XTOOL_A]]
>> >     int main(int argc, char* argv[])
>> >     {
>> >         // This is what the patch adds, possibility to annotate a
>> > statement
>> >         [[clang::annotate(XTOOL_B)]]
>> >         while( N )
>> >         {  . . .  }
>> >         return 0;
>> >     }
>> >
>> > The controversy of the patch appears when comparing annotations with
>> > pluggable attributes and suggesting that the two technologies competes for
>> > the same goal. No, they do not, for the simple reason that an annotation is
>> > an annotation, nothing more, nothing less, and should stay that way. It
>> > should not have the same streamlined functionality of PA, such as proper
>> > namespacing, argument checking etc. And let us be very clear on one thing;
>> > having pluggable attributes would be a great addition to Clang and we’re not
>> > trying dissuade anyone from implementing it by promoting annotations
>> > instead. The patch does not in any way make annotations move closer to PA
>> > than it was before.
>> >
>> > One of the complaints in the "annotations vs PA” discussion is how the
>> > functionality/information are exposed to both end-users and attribute
>> > authors, being error-prone to use. This is a complaint that actually
>> > underlines that annotations are not competing with pluggable attributes, it
>> > may be fit for some solutions but for others a more strict and controlled
>> > environment, like pluggable attributes, may be required. Diversity and
>> > different levels of support is what makes Clang superior to the competition.
>> > There is no reason to stop using an existing functionality until an
>> > alternative is available and to our humble understanding, implementing an
>> > architecture that supports PA up front and in both AST and IR probably needs
>> > a few iteration to set things straight, pushing the availability date into
>> > the distant future.
>> >
>> > We ask for this small patch to be committed since it makes a minor
>> > enhancement (annotated statements) to an existing functionality that
>> > benefits the community of plugin and tool writers today by providing a
>> > generic and consistent way of communicating control directives/tags directly
>> > from the source code to itself, such as source code transformers,
>> > generators, collectors and similar.
>>
>> For reference, the review thread starts at:
>>
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170130/184405.html
>>
>> (Unfortunately, it appears to not be threaded properly, so you may
>> have to search for the replies to follow along with the discussion.)
>>
>> While this is a simple adjustment to what the annotate attribute
>> appertains to, I do not want to extend the attribute in that
>> direction. As stated during the review, the annotate attribute is
>> already problematic in that there is no indication of what the
>> specific annotations should appertain to, no ability to supply
>> arguments, no way to prevent different tools from name collisions,
>> etc. Basically, the annotate attribute was a very quick way to do what
>> it needs to do back when we didn't have a good path towards pluggable
>> attributes. Now that we do have the good path forward, we should not
>> expand the capabilities of a deficient, competing solution simply
>> because it's the path of least resistance -- that does not provide our
>> users or tool authors with a good experience. I do not find the
>> argument that extending the annotate attribute does not compete with
>> pluggable attributes because it is more error-prone to be a compelling
>> rationale.
>
>
> Thanks, having an eye on the big picture, particularly regarding our
> vendor-specific extensions, is important.
>
> How far along is the work on pluggable attributes -- is there anything that
> would help make progress on that?

I believe we now have all the components needed to implement them, but
do not have concrete progress that surfaces the feature.
Unfortunately, I do not have time to work on it in the short term, but
I believe the implementation should be relatively straight-forward and
I am happy to provide direction and reviews to anyone interested in
working on it.

~Aaron

>
>> As mentioned in the review, I am fine with adding the C++ spelling for
>> the annotate attribute as that is certainly non-controversial and is
>> an incremental improvement.
>>
>> ~Aaron
>>
>> >
>> > Cheers,
>> > Chris
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
On Fri, Mar 3, 2017 at 4:17 PM, Aaron Ballman <[hidden email]> wrote:

> On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith <[hidden email]> wrote:
>> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <[hidden email]>
>> wrote:
>>>
>>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
>>> <[hidden email]> wrote:
>>> > Hi everyone!
>>> >
>>> > A few weeks back we sent a minor patch to the cfe-commit list that we
>>> > thought would be a non-issue once it had passed the review phase, but that
>>> > was not the case, instead we were told that the patch was too controversial
>>> > so we ask for a general opinion on the matter.
>>> >
>>> > The current implementation of the annotate attribute supports annotation
>>> > of declarations using the GNU syntax, with its data forwarded into AST and
>>> > further down to IR. The supported set of declarations includes classes,
>>> > variables, fields, functions and methods. The patch extends this to
>>> > annotation of statements and the use of C++11 syntax but with the lack of
>>> > support in LLVM IR the data is only forwarded onto AST.
>>> >
>>> > Overall, this is a very minor change to what already is present and
>>> > being used, but the small addition could benefit many plugin and tool
>>> > developers readily today. The concept of annotations are very generic and
>>> > makes it possible to insert, for example, control directives or tags
>>> > directly into the source code that a plugin or tool may read and utilize.
>>> > Please note that the intention of the patch is to make a small adjustment,
>>> > not to alter the meaning of annotations into something it is not.
>>> >
>>> > Here is an example usage:
>>> >
>>> >     File: xtool.hpp
>>> >     #define XTOOL_A “xtool:directiveA”
>>> >     #define XTOOL_B “xtool:directiveB"
>>> >
>>> >     File: sample.cpp
>>> >     #include <xlibrary.hpp>
>>> >
>>> >     [[clang::annotate(XTOOL_A]]
>>> >     int main(int argc, char* argv[])
>>> >     {
>>> >         // This is what the patch adds, possibility to annotate a
>>> > statement
>>> >         [[clang::annotate(XTOOL_B)]]
>>> >         while( N )
>>> >         {  . . .  }
>>> >         return 0;
>>> >     }
>>> >
>>> > The controversy of the patch appears when comparing annotations with
>>> > pluggable attributes and suggesting that the two technologies competes for
>>> > the same goal. No, they do not, for the simple reason that an annotation is
>>> > an annotation, nothing more, nothing less, and should stay that way. It
>>> > should not have the same streamlined functionality of PA, such as proper
>>> > namespacing, argument checking etc. And let us be very clear on one thing;
>>> > having pluggable attributes would be a great addition to Clang and we’re not
>>> > trying dissuade anyone from implementing it by promoting annotations
>>> > instead. The patch does not in any way make annotations move closer to PA
>>> > than it was before.
>>> >
>>> > One of the complaints in the "annotations vs PA” discussion is how the
>>> > functionality/information are exposed to both end-users and attribute
>>> > authors, being error-prone to use. This is a complaint that actually
>>> > underlines that annotations are not competing with pluggable attributes, it
>>> > may be fit for some solutions but for others a more strict and controlled
>>> > environment, like pluggable attributes, may be required. Diversity and
>>> > different levels of support is what makes Clang superior to the competition.
>>> > There is no reason to stop using an existing functionality until an
>>> > alternative is available and to our humble understanding, implementing an
>>> > architecture that supports PA up front and in both AST and IR probably needs
>>> > a few iteration to set things straight, pushing the availability date into
>>> > the distant future.
>>> >
>>> > We ask for this small patch to be committed since it makes a minor
>>> > enhancement (annotated statements) to an existing functionality that
>>> > benefits the community of plugin and tool writers today by providing a
>>> > generic and consistent way of communicating control directives/tags directly
>>> > from the source code to itself, such as source code transformers,
>>> > generators, collectors and similar.
>>>
>>> For reference, the review thread starts at:
>>>
>>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170130/184405.html
>>>
>>> (Unfortunately, it appears to not be threaded properly, so you may
>>> have to search for the replies to follow along with the discussion.)
>>>
>>> While this is a simple adjustment to what the annotate attribute
>>> appertains to, I do not want to extend the attribute in that
>>> direction. As stated during the review, the annotate attribute is
>>> already problematic in that there is no indication of what the
>>> specific annotations should appertain to, no ability to supply
>>> arguments, no way to prevent different tools from name collisions,
>>> etc. Basically, the annotate attribute was a very quick way to do what
>>> it needs to do back when we didn't have a good path towards pluggable
>>> attributes. Now that we do have the good path forward, we should not
>>> expand the capabilities of a deficient, competing solution simply
>>> because it's the path of least resistance -- that does not provide our
>>> users or tool authors with a good experience. I do not find the
>>> argument that extending the annotate attribute does not compete with
>>> pluggable attributes because it is more error-prone to be a compelling
>>> rationale.
>>
>>
>> Thanks, having an eye on the big picture, particularly regarding our
>> vendor-specific extensions, is important.
>>
>> How far along is the work on pluggable attributes -- is there anything that
>> would help make progress on that?
>
> I believe we now have all the components needed to implement them, but
> do not have concrete progress that surfaces the feature.
> Unfortunately, I do not have time to work on it in the short term, but
> I believe the implementation should be relatively straight-forward and
> I am happy to provide direction and reviews to anyone interested in
> working on it.

The high-level design looks something like this:
We already parse attributes in a pretty generic way (except for
custom-parsed attributes, which are not in scope for the initial
design). The plugin will need to tell the parser that a particular
name represents a pluggable attribute (so the AttributeList::AttrKind
can be set appropriately to prevent "unknown attribute" diagnostics).
When doing the semantic checking (in SemaDeclAttr.cppp) and receiving
a pluggable parsed attribute kind, the common feature checking done by
handleCommonAttributeFeatures() (proper subject, argument counts, etc)
can pull information from the plugin to automate the simple checking
we already have for attributes. The plugin can also provide the
ability to perform custom semantic checking (the handleFooAttr()
stuff). Sema should create a ProxyAttr/ProxyInheritableAttr/etc
attribute object for the AST that forwards requests for information to
the plugin for the concrete implementation. This should get the
pluggable attribute into the AST for declaration attributes -- you can
use Decl::hasAttr<>() and friends to see whether something is a
ProxyAttr and Attr->getKind() to see what the plugin attribute is
(which means AST matchers should do the right thing out of the box).
Some similar changes will be needed in SemaStmtAttr.cpp for statement
attributes. I think that pluggable type attributes require further
thought and should not be allowed in the initial implementation.

The code generator can be modified (look for uses of AnnotateAttr, but
I believe it's EmitFooAnnotations() that needs modification) to allow
the plugin to specify what to lower to LLVM IR, and similar
modifications to the C source indexing stuff in CIndex.cpp. However,
these could easily be follow-up patches.

~Aaron

>
> ~Aaron
>
>>
>>> As mentioned in the review, I am fine with adding the C++ spelling for
>>> the annotate attribute as that is certainly non-controversial and is
>>> an incremental improvement.
>>>
>>> ~Aaron
>>>
>>> >
>>> > Cheers,
>>> > Chris
>>> >
>>> > _______________________________________________
>>> > cfe-dev mailing list
>>> > [hidden email]
>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
We actually have an implementation of pluggable attributes which we may be
contributing, though I don't know when that would be exactly.

Firstly, there's two halves to attribute handling: the frontend part, which
revolves around the ParsedAttrInfo class, and the Sema part, which revolved
around the Attr class and the attr::Kind enum. Our focus was mostly on the
frontend part.

For the frontend part the general approach taken was to rewrite the
tablegen'd part of attribute handling so that instead of having loads of
switches all over the place instead the ParsedAttrInfo class has virtual
function members and each attribute defines a subclass which defines those
functions appropriately. ParseAttrInfo is then moved into an llvm::Registry,
so then plugins just need to define their own subclass of ParseAttrInfo and
add it to the registry.

The Sema part wasn't touched much, as for our purposes all we need is to
set an LLVM attribute, so all we did was add a PluginAttr attribute which
does just that. ProcessDeclAttribute is modified to ask the ParsedAttrInfo
to attach an Attr to a Decl, but that's it. I can imagine that you could
so something with a ProxyAttr here though which forwards things through
to functions defined in a plugin, but I imagine it would require a lot
of changes to various parts of Sema (because as I recall there are checks
of hasAttr<FooAttr> scattered all over the place).

John

> -----Original Message-----
> From: cfe-dev [mailto:[hidden email]] On Behalf Of Aaron
> Ballman via cfe-dev
> Sent: 04 March 2017 19:39
> To: Richard Smith
> Cc: cfe-dev; Marcwell Helpdesk; Richard Smith
> Subject: Re: [cfe-dev] Minor patch controversial
>
> On Fri, Mar 3, 2017 at 4:17 PM, Aaron Ballman <[hidden email]>
> wrote:
> > On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith <[hidden email]>
> wrote:
> >> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <cfe-
> [hidden email]>
> >> wrote:
> >>>
> >>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
> >>> <[hidden email]> wrote:
> >>> > Hi everyone!
> >>> >
> >>> > A few weeks back we sent a minor patch to the cfe-commit list that
> we
> >>> > thought would be a non-issue once it had passed the review phase,
> but that
> >>> > was not the case, instead we were told that the patch was too
> controversial
> >>> > so we ask for a general opinion on the matter.
> >>> >
> >>> > The current implementation of the annotate attribute supports
> annotation
> >>> > of declarations using the GNU syntax, with its data forwarded into
> AST and
> >>> > further down to IR. The supported set of declarations includes
> classes,
> >>> > variables, fields, functions and methods. The patch extends this
> to
> >>> > annotation of statements and the use of C++11 syntax but with the
> lack of
> >>> > support in LLVM IR the data is only forwarded onto AST.
> >>> >
> >>> > Overall, this is a very minor change to what already is present
> and
> >>> > being used, but the small addition could benefit many plugin and
> tool
> >>> > developers readily today. The concept of annotations are very
> generic and
> >>> > makes it possible to insert, for example, control directives or
> tags
> >>> > directly into the source code that a plugin or tool may read and
> utilize.
> >>> > Please note that the intention of the patch is to make a small
> adjustment,
> >>> > not to alter the meaning of annotations into something it is not.
> >>> >
> >>> > Here is an example usage:
> >>> >
> >>> >     File: xtool.hpp
> >>> >     #define XTOOL_A “xtool:directiveA”
> >>> >     #define XTOOL_B “xtool:directiveB"
> >>> >
> >>> >     File: sample.cpp
> >>> >     #include <xlibrary.hpp>
> >>> >
> >>> >     [[clang::annotate(XTOOL_A]]
> >>> >     int main(int argc, char* argv[])
> >>> >     {
> >>> >         // This is what the patch adds, possibility to annotate a
> >>> > statement
> >>> >         [[clang::annotate(XTOOL_B)]]
> >>> >         while( N )
> >>> >         {  . . .  }
> >>> >         return 0;
> >>> >     }
> >>> >
> >>> > The controversy of the patch appears when comparing annotations
> with
> >>> > pluggable attributes and suggesting that the two technologies
> competes for
> >>> > the same goal. No, they do not, for the simple reason that an
> annotation is
> >>> > an annotation, nothing more, nothing less, and should stay that
> way. It
> >>> > should not have the same streamlined functionality of PA, such as
> proper
> >>> > namespacing, argument checking etc. And let us be very clear on
> one thing;
> >>> > having pluggable attributes would be a great addition to Clang and
> we’re not
> >>> > trying dissuade anyone from implementing it by promoting
> annotations
> >>> > instead. The patch does not in any way make annotations move
> closer to PA
> >>> > than it was before.
> >>> >
> >>> > One of the complaints in the "annotations vs PA” discussion is how
> the
> >>> > functionality/information are exposed to both end-users and
> attribute
> >>> > authors, being error-prone to use. This is a complaint that
> actually
> >>> > underlines that annotations are not competing with pluggable
> attributes, it
> >>> > may be fit for some solutions but for others a more strict and
> controlled
> >>> > environment, like pluggable attributes, may be required. Diversity
> and
> >>> > different levels of support is what makes Clang superior to the
> competition.
> >>> > There is no reason to stop using an existing functionality until
> an
> >>> > alternative is available and to our humble understanding,
> implementing an
> >>> > architecture that supports PA up front and in both AST and IR
> probably needs
> >>> > a few iteration to set things straight, pushing the availability
> date into
> >>> > the distant future.
> >>> >
> >>> > We ask for this small patch to be committed since it makes a minor
> >>> > enhancement (annotated statements) to an existing functionality
> that
> >>> > benefits the community of plugin and tool writers today by
> providing a
> >>> > generic and consistent way of communicating control
> directives/tags directly
> >>> > from the source code to itself, such as source code transformers,
> >>> > generators, collectors and similar.
> >>>
> >>> For reference, the review thread starts at:
> >>>
> >>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-
> 20170130/184405.html
> >>>
> >>> (Unfortunately, it appears to not be threaded properly, so you may
> >>> have to search for the replies to follow along with the discussion.)
> >>>
> >>> While this is a simple adjustment to what the annotate attribute
> >>> appertains to, I do not want to extend the attribute in that
> >>> direction. As stated during the review, the annotate attribute is
> >>> already problematic in that there is no indication of what the
> >>> specific annotations should appertain to, no ability to supply
> >>> arguments, no way to prevent different tools from name collisions,
> >>> etc. Basically, the annotate attribute was a very quick way to do
> what
> >>> it needs to do back when we didn't have a good path towards
> pluggable
> >>> attributes. Now that we do have the good path forward, we should not
> >>> expand the capabilities of a deficient, competing solution simply
> >>> because it's the path of least resistance -- that does not provide
> our
> >>> users or tool authors with a good experience. I do not find the
> >>> argument that extending the annotate attribute does not compete with
> >>> pluggable attributes because it is more error-prone to be a
> compelling
> >>> rationale.
> >>
> >>
> >> Thanks, having an eye on the big picture, particularly regarding our
> >> vendor-specific extensions, is important.
> >>
> >> How far along is the work on pluggable attributes -- is there
> anything that
> >> would help make progress on that?
> >
> > I believe we now have all the components needed to implement them, but
> > do not have concrete progress that surfaces the feature.
> > Unfortunately, I do not have time to work on it in the short term, but
> > I believe the implementation should be relatively straight-forward and
> > I am happy to provide direction and reviews to anyone interested in
> > working on it.
>
> The high-level design looks something like this:
> We already parse attributes in a pretty generic way (except for
> custom-parsed attributes, which are not in scope for the initial
> design). The plugin will need to tell the parser that a particular
> name represents a pluggable attribute (so the AttributeList::AttrKind
> can be set appropriately to prevent "unknown attribute" diagnostics).
> When doing the semantic checking (in SemaDeclAttr.cppp) and receiving
> a pluggable parsed attribute kind, the common feature checking done by
> handleCommonAttributeFeatures() (proper subject, argument counts, etc)
> can pull information from the plugin to automate the simple checking
> we already have for attributes. The plugin can also provide the
> ability to perform custom semantic checking (the handleFooAttr()
> stuff). Sema should create a ProxyAttr/ProxyInheritableAttr/etc
> attribute object for the AST that forwards requests for information to
> the plugin for the concrete implementation. This should get the
> pluggable attribute into the AST for declaration attributes -- you can
> use Decl::hasAttr<>() and friends to see whether something is a
> ProxyAttr and Attr->getKind() to see what the plugin attribute is
> (which means AST matchers should do the right thing out of the box).
> Some similar changes will be needed in SemaStmtAttr.cpp for statement
> attributes. I think that pluggable type attributes require further
> thought and should not be allowed in the initial implementation.
>
> The code generator can be modified (look for uses of AnnotateAttr, but
> I believe it's EmitFooAnnotations() that needs modification) to allow
> the plugin to specify what to lower to LLVM IR, and similar
> modifications to the C source indexing stuff in CIndex.cpp. However,
> these could easily be follow-up patches.
>
> ~Aaron
>
> >
> > ~Aaron
> >
> >>
> >>> As mentioned in the review, I am fine with adding the C++ spelling
> for
> >>> the annotate attribute as that is certainly non-controversial and is
> >>> an incremental improvement.
> >>>
> >>> ~Aaron
> >>>
> >>> >
> >>> > Cheers,
> >>> > Chris
> >>> >
> >>> > _______________________________________________
> >>> > cfe-dev mailing list
> >>> > [hidden email]
> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >>> _______________________________________________
> >>> cfe-dev mailing list
> >>> [hidden email]
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >>
> >>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
On Tue, Mar 14, 2017 at 1:02 PM, John Brawn <[hidden email]> wrote:
> We actually have an implementation of pluggable attributes which we may be
> contributing, though I don't know when that would be exactly.

That's great!

> Firstly, there's two halves to attribute handling: the frontend part, which
> revolves around the ParsedAttrInfo class, and the Sema part, which revolved
> around the Attr class and the attr::Kind enum. Our focus was mostly on the
> frontend part.
>
> For the frontend part the general approach taken was to rewrite the
> tablegen'd part of attribute handling so that instead of having loads of
> switches all over the place instead the ParsedAttrInfo class has virtual
> function members and each attribute defines a subclass which defines those
> functions appropriately. ParseAttrInfo is then moved into an llvm::Registry,
> so then plugins just need to define their own subclass of ParseAttrInfo and
> add it to the registry.

Which switches, in particular?

I was hoping that plugins could still reuse the work done in
ClangAttrEmitter.cpp to convert their own .td file of attributes into
generate the information needed for the plugin -- this reduces the
chances of getting divergent behavior between builtin attributes and
plugin attributes, and reduces the cognitive burden from having two
distinct ways to define attributes. Does your design still make use of
the table generator, or do you have to fill out the ParsedAttrInfo
subclass information by hand?

> The Sema part wasn't touched much, as for our purposes all we need is to
> set an LLVM attribute, so all we did was add a PluginAttr attribute which
> does just that. ProcessDeclAttribute is modified to ask the ParsedAttrInfo
> to attach an Attr to a Decl, but that's it. I can imagine that you could
> so something with a ProxyAttr here though which forwards things through
> to functions defined in a plugin, but I imagine it would require a lot
> of changes to various parts of Sema (because as I recall there are checks
> of hasAttr<FooAttr> scattered all over the place).

The calls to hasAttr<> use the llvm casting system, and so you could
do hasAttr<ProxyAttr>() if you cared about proxy attribute support
generically in Sema. However, you can still use Attr::getKind() to get
to the actual concrete attribute implementation when needed (which
means that dynamic tooling like AST matchers work out of the box). I
imagine the changes to Sema shouldn't be that drastic because I
believe you generically only care about "is this a proxy attribute" in
a few places (using hasAttr<>), such as when lowering to an LLVM
attribute in codegen.

Thank you for sharing this information!

~Aaron

>
> John
>
>> -----Original Message-----
>> From: cfe-dev [mailto:[hidden email]] On Behalf Of Aaron
>> Ballman via cfe-dev
>> Sent: 04 March 2017 19:39
>> To: Richard Smith
>> Cc: cfe-dev; Marcwell Helpdesk; Richard Smith
>> Subject: Re: [cfe-dev] Minor patch controversial
>>
>> On Fri, Mar 3, 2017 at 4:17 PM, Aaron Ballman <[hidden email]>
>> wrote:
>> > On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith <[hidden email]>
>> wrote:
>> >> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <cfe-
>> [hidden email]>
>> >> wrote:
>> >>>
>> >>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
>> >>> <[hidden email]> wrote:
>> >>> > Hi everyone!
>> >>> >
>> >>> > A few weeks back we sent a minor patch to the cfe-commit list that
>> we
>> >>> > thought would be a non-issue once it had passed the review phase,
>> but that
>> >>> > was not the case, instead we were told that the patch was too
>> controversial
>> >>> > so we ask for a general opinion on the matter.
>> >>> >
>> >>> > The current implementation of the annotate attribute supports
>> annotation
>> >>> > of declarations using the GNU syntax, with its data forwarded into
>> AST and
>> >>> > further down to IR. The supported set of declarations includes
>> classes,
>> >>> > variables, fields, functions and methods. The patch extends this
>> to
>> >>> > annotation of statements and the use of C++11 syntax but with the
>> lack of
>> >>> > support in LLVM IR the data is only forwarded onto AST.
>> >>> >
>> >>> > Overall, this is a very minor change to what already is present
>> and
>> >>> > being used, but the small addition could benefit many plugin and
>> tool
>> >>> > developers readily today. The concept of annotations are very
>> generic and
>> >>> > makes it possible to insert, for example, control directives or
>> tags
>> >>> > directly into the source code that a plugin or tool may read and
>> utilize.
>> >>> > Please note that the intention of the patch is to make a small
>> adjustment,
>> >>> > not to alter the meaning of annotations into something it is not.
>> >>> >
>> >>> > Here is an example usage:
>> >>> >
>> >>> >     File: xtool.hpp
>> >>> >     #define XTOOL_A “xtool:directiveA”
>> >>> >     #define XTOOL_B “xtool:directiveB"
>> >>> >
>> >>> >     File: sample.cpp
>> >>> >     #include <xlibrary.hpp>
>> >>> >
>> >>> >     [[clang::annotate(XTOOL_A]]
>> >>> >     int main(int argc, char* argv[])
>> >>> >     {
>> >>> >         // This is what the patch adds, possibility to annotate a
>> >>> > statement
>> >>> >         [[clang::annotate(XTOOL_B)]]
>> >>> >         while( N )
>> >>> >         {  . . .  }
>> >>> >         return 0;
>> >>> >     }
>> >>> >
>> >>> > The controversy of the patch appears when comparing annotations
>> with
>> >>> > pluggable attributes and suggesting that the two technologies
>> competes for
>> >>> > the same goal. No, they do not, for the simple reason that an
>> annotation is
>> >>> > an annotation, nothing more, nothing less, and should stay that
>> way. It
>> >>> > should not have the same streamlined functionality of PA, such as
>> proper
>> >>> > namespacing, argument checking etc. And let us be very clear on
>> one thing;
>> >>> > having pluggable attributes would be a great addition to Clang and
>> we’re not
>> >>> > trying dissuade anyone from implementing it by promoting
>> annotations
>> >>> > instead. The patch does not in any way make annotations move
>> closer to PA
>> >>> > than it was before.
>> >>> >
>> >>> > One of the complaints in the "annotations vs PA” discussion is how
>> the
>> >>> > functionality/information are exposed to both end-users and
>> attribute
>> >>> > authors, being error-prone to use. This is a complaint that
>> actually
>> >>> > underlines that annotations are not competing with pluggable
>> attributes, it
>> >>> > may be fit for some solutions but for others a more strict and
>> controlled
>> >>> > environment, like pluggable attributes, may be required. Diversity
>> and
>> >>> > different levels of support is what makes Clang superior to the
>> competition.
>> >>> > There is no reason to stop using an existing functionality until
>> an
>> >>> > alternative is available and to our humble understanding,
>> implementing an
>> >>> > architecture that supports PA up front and in both AST and IR
>> probably needs
>> >>> > a few iteration to set things straight, pushing the availability
>> date into
>> >>> > the distant future.
>> >>> >
>> >>> > We ask for this small patch to be committed since it makes a minor
>> >>> > enhancement (annotated statements) to an existing functionality
>> that
>> >>> > benefits the community of plugin and tool writers today by
>> providing a
>> >>> > generic and consistent way of communicating control
>> directives/tags directly
>> >>> > from the source code to itself, such as source code transformers,
>> >>> > generators, collectors and similar.
>> >>>
>> >>> For reference, the review thread starts at:
>> >>>
>> >>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-
>> 20170130/184405.html
>> >>>
>> >>> (Unfortunately, it appears to not be threaded properly, so you may
>> >>> have to search for the replies to follow along with the discussion.)
>> >>>
>> >>> While this is a simple adjustment to what the annotate attribute
>> >>> appertains to, I do not want to extend the attribute in that
>> >>> direction. As stated during the review, the annotate attribute is
>> >>> already problematic in that there is no indication of what the
>> >>> specific annotations should appertain to, no ability to supply
>> >>> arguments, no way to prevent different tools from name collisions,
>> >>> etc. Basically, the annotate attribute was a very quick way to do
>> what
>> >>> it needs to do back when we didn't have a good path towards
>> pluggable
>> >>> attributes. Now that we do have the good path forward, we should not
>> >>> expand the capabilities of a deficient, competing solution simply
>> >>> because it's the path of least resistance -- that does not provide
>> our
>> >>> users or tool authors with a good experience. I do not find the
>> >>> argument that extending the annotate attribute does not compete with
>> >>> pluggable attributes because it is more error-prone to be a
>> compelling
>> >>> rationale.
>> >>
>> >>
>> >> Thanks, having an eye on the big picture, particularly regarding our
>> >> vendor-specific extensions, is important.
>> >>
>> >> How far along is the work on pluggable attributes -- is there
>> anything that
>> >> would help make progress on that?
>> >
>> > I believe we now have all the components needed to implement them, but
>> > do not have concrete progress that surfaces the feature.
>> > Unfortunately, I do not have time to work on it in the short term, but
>> > I believe the implementation should be relatively straight-forward and
>> > I am happy to provide direction and reviews to anyone interested in
>> > working on it.
>>
>> The high-level design looks something like this:
>> We already parse attributes in a pretty generic way (except for
>> custom-parsed attributes, which are not in scope for the initial
>> design). The plugin will need to tell the parser that a particular
>> name represents a pluggable attribute (so the AttributeList::AttrKind
>> can be set appropriately to prevent "unknown attribute" diagnostics).
>> When doing the semantic checking (in SemaDeclAttr.cppp) and receiving
>> a pluggable parsed attribute kind, the common feature checking done by
>> handleCommonAttributeFeatures() (proper subject, argument counts, etc)
>> can pull information from the plugin to automate the simple checking
>> we already have for attributes. The plugin can also provide the
>> ability to perform custom semantic checking (the handleFooAttr()
>> stuff). Sema should create a ProxyAttr/ProxyInheritableAttr/etc
>> attribute object for the AST that forwards requests for information to
>> the plugin for the concrete implementation. This should get the
>> pluggable attribute into the AST for declaration attributes -- you can
>> use Decl::hasAttr<>() and friends to see whether something is a
>> ProxyAttr and Attr->getKind() to see what the plugin attribute is
>> (which means AST matchers should do the right thing out of the box).
>> Some similar changes will be needed in SemaStmtAttr.cpp for statement
>> attributes. I think that pluggable type attributes require further
>> thought and should not be allowed in the initial implementation.
>>
>> The code generator can be modified (look for uses of AnnotateAttr, but
>> I believe it's EmitFooAnnotations() that needs modification) to allow
>> the plugin to specify what to lower to LLVM IR, and similar
>> modifications to the C source indexing stuff in CIndex.cpp. However,
>> these could easily be follow-up patches.
>>
>> ~Aaron
>>
>> >
>> > ~Aaron
>> >
>> >>
>> >>> As mentioned in the review, I am fine with adding the C++ spelling
>> for
>> >>> the annotate attribute as that is certainly non-controversial and is
>> >>> an incremental improvement.
>> >>>
>> >>> ~Aaron
>> >>>
>> >>> >
>> >>> > Cheers,
>> >>> > Chris
>> >>> >
>> >>> > _______________________________________________
>> >>> > cfe-dev mailing list
>> >>> > [hidden email]
>> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>> _______________________________________________
>> >>> cfe-dev mailing list
>> >>> [hidden email]
>> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Minor patch controversial

James Y Knight via cfe-dev
> Which switches, in particular?

Everything in AttrParserStringSwitches.inc and AttrSpellingListIndex.inc,
though actually going back and looking at it for these we just have a bit
in the ParsedAttrInfo instead of a virtual function.

> I was hoping that plugins could still reuse the work done in
> ClangAttrEmitter.cpp to convert their own .td file of attributes into
> generate the information needed for the plugin -- this reduces the
> chances of getting divergent behavior between builtin attributes and
> plugin attributes, and reduces the cognitive burden from having two
> distinct ways to define attributes. Does your design still make use of
> the table generator, or do you have to fill out the ParsedAttrInfo
> subclass information by hand?

I didn't do this and just filled in the ParsedAttrInfo by hand.

John

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf
> Of Aaron Ballman
> Sent: 14 March 2017 18:29
> To: John Brawn
> Cc: Richard Smith; Marcwell Helpdesk; Richard Smith; cfe-
> [hidden email]; nd
> Subject: Re: [cfe-dev] Minor patch controversial
>
> On Tue, Mar 14, 2017 at 1:02 PM, John Brawn <[hidden email]> wrote:
> > We actually have an implementation of pluggable attributes which we
> may be
> > contributing, though I don't know when that would be exactly.
>
> That's great!
>
> > Firstly, there's two halves to attribute handling: the frontend part,
> which
> > revolves around the ParsedAttrInfo class, and the Sema part, which
> revolved
> > around the Attr class and the attr::Kind enum. Our focus was mostly on
> the
> > frontend part.
> >
> > For the frontend part the general approach taken was to rewrite the
> > tablegen'd part of attribute handling so that instead of having loads
> of
> > switches all over the place instead the ParsedAttrInfo class has
> virtual
> > function members and each attribute defines a subclass which defines
> those
> > functions appropriately. ParseAttrInfo is then moved into an
> llvm::Registry,
> > so then plugins just need to define their own subclass of
> ParseAttrInfo and
> > add it to the registry.
>
> Which switches, in particular?
>
> I was hoping that plugins could still reuse the work done in
> ClangAttrEmitter.cpp to convert their own .td file of attributes into
> generate the information needed for the plugin -- this reduces the
> chances of getting divergent behavior between builtin attributes and
> plugin attributes, and reduces the cognitive burden from having two
> distinct ways to define attributes. Does your design still make use of
> the table generator, or do you have to fill out the ParsedAttrInfo
> subclass information by hand?
>
> > The Sema part wasn't touched much, as for our purposes all we need is
> to
> > set an LLVM attribute, so all we did was add a PluginAttr attribute
> which
> > does just that. ProcessDeclAttribute is modified to ask the
> ParsedAttrInfo
> > to attach an Attr to a Decl, but that's it. I can imagine that you
> could
> > so something with a ProxyAttr here though which forwards things
> through
> > to functions defined in a plugin, but I imagine it would require a lot
> > of changes to various parts of Sema (because as I recall there are
> checks
> > of hasAttr<FooAttr> scattered all over the place).
>
> The calls to hasAttr<> use the llvm casting system, and so you could
> do hasAttr<ProxyAttr>() if you cared about proxy attribute support
> generically in Sema. However, you can still use Attr::getKind() to get
> to the actual concrete attribute implementation when needed (which
> means that dynamic tooling like AST matchers work out of the box). I
> imagine the changes to Sema shouldn't be that drastic because I
> believe you generically only care about "is this a proxy attribute" in
> a few places (using hasAttr<>), such as when lowering to an LLVM
> attribute in codegen.
>
> Thank you for sharing this information!
>
> ~Aaron
>
> >
> > John
> >
> >> -----Original Message-----
> >> From: cfe-dev [mailto:[hidden email]] On Behalf Of
> Aaron
> >> Ballman via cfe-dev
> >> Sent: 04 March 2017 19:39
> >> To: Richard Smith
> >> Cc: cfe-dev; Marcwell Helpdesk; Richard Smith
> >> Subject: Re: [cfe-dev] Minor patch controversial
> >>
> >> On Fri, Mar 3, 2017 at 4:17 PM, Aaron Ballman
> <[hidden email]>
> >> wrote:
> >> > On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith
> <[hidden email]>
> >> wrote:
> >> >> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <cfe-
> >> [hidden email]>
> >> >> wrote:
> >> >>>
> >> >>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
> >> >>> <[hidden email]> wrote:
> >> >>> > Hi everyone!
> >> >>> >
> >> >>> > A few weeks back we sent a minor patch to the cfe-commit list
> that
> >> we
> >> >>> > thought would be a non-issue once it had passed the review
> phase,
> >> but that
> >> >>> > was not the case, instead we were told that the patch was too
> >> controversial
> >> >>> > so we ask for a general opinion on the matter.
> >> >>> >
> >> >>> > The current implementation of the annotate attribute supports
> >> annotation
> >> >>> > of declarations using the GNU syntax, with its data forwarded
> into
> >> AST and
> >> >>> > further down to IR. The supported set of declarations includes
> >> classes,
> >> >>> > variables, fields, functions and methods. The patch extends
> this
> >> to
> >> >>> > annotation of statements and the use of C++11 syntax but with
> the
> >> lack of
> >> >>> > support in LLVM IR the data is only forwarded onto AST.
> >> >>> >
> >> >>> > Overall, this is a very minor change to what already is present
> >> and
> >> >>> > being used, but the small addition could benefit many plugin
> and
> >> tool
> >> >>> > developers readily today. The concept of annotations are very
> >> generic and
> >> >>> > makes it possible to insert, for example, control directives or
> >> tags
> >> >>> > directly into the source code that a plugin or tool may read
> and
> >> utilize.
> >> >>> > Please note that the intention of the patch is to make a small
> >> adjustment,
> >> >>> > not to alter the meaning of annotations into something it is
> not.
> >> >>> >
> >> >>> > Here is an example usage:
> >> >>> >
> >> >>> >     File: xtool.hpp
> >> >>> >     #define XTOOL_A “xtool:directiveA”
> >> >>> >     #define XTOOL_B “xtool:directiveB"
> >> >>> >
> >> >>> >     File: sample.cpp
> >> >>> >     #include <xlibrary.hpp>
> >> >>> >
> >> >>> >     [[clang::annotate(XTOOL_A]]
> >> >>> >     int main(int argc, char* argv[])
> >> >>> >     {
> >> >>> >         // This is what the patch adds, possibility to annotate
> a
> >> >>> > statement
> >> >>> >         [[clang::annotate(XTOOL_B)]]
> >> >>> >         while( N )
> >> >>> >         {  . . .  }
> >> >>> >         return 0;
> >> >>> >     }
> >> >>> >
> >> >>> > The controversy of the patch appears when comparing annotations
> >> with
> >> >>> > pluggable attributes and suggesting that the two technologies
> >> competes for
> >> >>> > the same goal. No, they do not, for the simple reason that an
> >> annotation is
> >> >>> > an annotation, nothing more, nothing less, and should stay that
> >> way. It
> >> >>> > should not have the same streamlined functionality of PA, such
> as
> >> proper
> >> >>> > namespacing, argument checking etc. And let us be very clear on
> >> one thing;
> >> >>> > having pluggable attributes would be a great addition to Clang
> and
> >> we’re not
> >> >>> > trying dissuade anyone from implementing it by promoting
> >> annotations
> >> >>> > instead. The patch does not in any way make annotations move
> >> closer to PA
> >> >>> > than it was before.
> >> >>> >
> >> >>> > One of the complaints in the "annotations vs PA” discussion is
> how
> >> the
> >> >>> > functionality/information are exposed to both end-users and
> >> attribute
> >> >>> > authors, being error-prone to use. This is a complaint that
> >> actually
> >> >>> > underlines that annotations are not competing with pluggable
> >> attributes, it
> >> >>> > may be fit for some solutions but for others a more strict and
> >> controlled
> >> >>> > environment, like pluggable attributes, may be required.
> Diversity
> >> and
> >> >>> > different levels of support is what makes Clang superior to the
> >> competition.
> >> >>> > There is no reason to stop using an existing functionality
> until
> >> an
> >> >>> > alternative is available and to our humble understanding,
> >> implementing an
> >> >>> > architecture that supports PA up front and in both AST and IR
> >> probably needs
> >> >>> > a few iteration to set things straight, pushing the
> availability
> >> date into
> >> >>> > the distant future.
> >> >>> >
> >> >>> > We ask for this small patch to be committed since it makes a
> minor
> >> >>> > enhancement (annotated statements) to an existing functionality
> >> that
> >> >>> > benefits the community of plugin and tool writers today by
> >> providing a
> >> >>> > generic and consistent way of communicating control
> >> directives/tags directly
> >> >>> > from the source code to itself, such as source code
> transformers,
> >> >>> > generators, collectors and similar.
> >> >>>
> >> >>> For reference, the review thread starts at:
> >> >>>
> >> >>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-
> >> 20170130/184405.html
> >> >>>
> >> >>> (Unfortunately, it appears to not be threaded properly, so you
> may
> >> >>> have to search for the replies to follow along with the
> discussion.)
> >> >>>
> >> >>> While this is a simple adjustment to what the annotate attribute
> >> >>> appertains to, I do not want to extend the attribute in that
> >> >>> direction. As stated during the review, the annotate attribute is
> >> >>> already problematic in that there is no indication of what the
> >> >>> specific annotations should appertain to, no ability to supply
> >> >>> arguments, no way to prevent different tools from name
> collisions,
> >> >>> etc. Basically, the annotate attribute was a very quick way to do
> >> what
> >> >>> it needs to do back when we didn't have a good path towards
> >> pluggable
> >> >>> attributes. Now that we do have the good path forward, we should
> not
> >> >>> expand the capabilities of a deficient, competing solution simply
> >> >>> because it's the path of least resistance -- that does not
> provide
> >> our
> >> >>> users or tool authors with a good experience. I do not find the
> >> >>> argument that extending the annotate attribute does not compete
> with
> >> >>> pluggable attributes because it is more error-prone to be a
> >> compelling
> >> >>> rationale.
> >> >>
> >> >>
> >> >> Thanks, having an eye on the big picture, particularly regarding
> our
> >> >> vendor-specific extensions, is important.
> >> >>
> >> >> How far along is the work on pluggable attributes -- is there
> >> anything that
> >> >> would help make progress on that?
> >> >
> >> > I believe we now have all the components needed to implement them,
> but
> >> > do not have concrete progress that surfaces the feature.
> >> > Unfortunately, I do not have time to work on it in the short term,
> but
> >> > I believe the implementation should be relatively straight-forward
> and
> >> > I am happy to provide direction and reviews to anyone interested in
> >> > working on it.
> >>
> >> The high-level design looks something like this:
> >> We already parse attributes in a pretty generic way (except for
> >> custom-parsed attributes, which are not in scope for the initial
> >> design). The plugin will need to tell the parser that a particular
> >> name represents a pluggable attribute (so the AttributeList::AttrKind
> >> can be set appropriately to prevent "unknown attribute" diagnostics).
> >> When doing the semantic checking (in SemaDeclAttr.cppp) and receiving
> >> a pluggable parsed attribute kind, the common feature checking done
> by
> >> handleCommonAttributeFeatures() (proper subject, argument counts,
> etc)
> >> can pull information from the plugin to automate the simple checking
> >> we already have for attributes. The plugin can also provide the
> >> ability to perform custom semantic checking (the handleFooAttr()
> >> stuff). Sema should create a ProxyAttr/ProxyInheritableAttr/etc
> >> attribute object for the AST that forwards requests for information
> to
> >> the plugin for the concrete implementation. This should get the
> >> pluggable attribute into the AST for declaration attributes -- you
> can
> >> use Decl::hasAttr<>() and friends to see whether something is a
> >> ProxyAttr and Attr->getKind() to see what the plugin attribute is
> >> (which means AST matchers should do the right thing out of the box).
> >> Some similar changes will be needed in SemaStmtAttr.cpp for statement
> >> attributes. I think that pluggable type attributes require further
> >> thought and should not be allowed in the initial implementation.
> >>
> >> The code generator can be modified (look for uses of AnnotateAttr,
> but
> >> I believe it's EmitFooAnnotations() that needs modification) to allow
> >> the plugin to specify what to lower to LLVM IR, and similar
> >> modifications to the C source indexing stuff in CIndex.cpp. However,
> >> these could easily be follow-up patches.
> >>
> >> ~Aaron
> >>
> >> >
> >> > ~Aaron
> >> >
> >> >>
> >> >>> As mentioned in the review, I am fine with adding the C++
> spelling
> >> for
> >> >>> the annotate attribute as that is certainly non-controversial and
> is
> >> >>> an incremental improvement.
> >> >>>
> >> >>> ~Aaron
> >> >>>
> >> >>> >
> >> >>> > Cheers,
> >> >>> > Chris
> >> >>> >
> >> >>> > _______________________________________________
> >> >>> > cfe-dev mailing list
> >> >>> > [hidden email]
> >> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >> >>> _______________________________________________
> >> >>> cfe-dev mailing list
> >> >>> [hidden email]
> >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >> >>
> >> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> [hidden email]
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev