[cfe-dev] Minor patch controversial

[John Brawn 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: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On Behalf
> Of Aaron Ballman
> Sent: 14 March 2017 18:29
> To: John Brawn
> Cc: Richard Smith; Marcwell Helpdesk; Richard Smith; cfe-
> dev at lists.llvm.org; nd
> Subject: Re: [cfe-dev] Minor patch controversial
> 
> On Tue, Mar 14, 2017 at 1:02 PM, John Brawn <John.Brawn at arm.com> 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:cfe-dev-bounces at lists.llvm.org] 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
> <aaron at aaronballman.com>
> >> wrote:
> >> > On Fri, Mar 3, 2017 at 4:06 PM, Richard Smith
> <richard at metafoo.co.uk>
> >> wrote:
> >> >> On 3 March 2017 at 09:32, Aaron Ballman via cfe-dev <cfe-
> >> dev at lists.llvm.org>
> >> >> wrote:
> >> >>>
> >> >>> On Fri, Mar 3, 2017 at 1:04 AM, Marcwell Helpdesk via cfe-dev
> >> >>> <cfe-dev at lists.llvm.org> 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
> >> >>> > cfe-dev at lists.llvm.org
> >> >>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >> >>> _______________________________________________
> >> >>> cfe-dev mailing list
> >> >>> cfe-dev at lists.llvm.org
> >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >> >>
> >> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev