[PATCH] Improved plugin/tool support by expanding an existing attribute

Marcwell Helpdesk via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 24 10:24:23 PST 2017


As interesting the subject of pluggable attributes may be could we please drop that discussion and focus on the intention of and what the patch actually does? Two revisions of the patch have been supplied and both should reasonable not cause any controversy since they only enriches an already existing functionality.

You left out the statement annotation when asking to split the patch, if this is causing the controversy please explain why. It would be very interesting to know the original intention of the annotate attribute should it deviate in any way from being a general purpose information bearer. And as I have tried to explain several times, the statement annotations are as important/irrelevant as any other type of annotations and with the current support in LLVM/clang it won’t support forwarding to IR but it may be used in the AST. Furthermore, if you don’t like the what the documentation says about using the annotations for tools then drop the documentation or write it yourself, it must not be the show-stopper.

Cheers,
Chris

> On 21 feb 2017, at 15:11, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> On Tue, Feb 14, 2017 at 6:05 AM, Marcwell Helpdesk wrote:
>> The intention of the patch is to extend the functionality of annotations
>> while utilizing existing infrastructure in the AST and IR as much as
>> possible. Annotations are indeed a general purpose solution, sufficient for
>> many use cases, and a complement, not mutual exclusive to pluggable
>> attributes. And don’t forget the realm of AST tools. The compiler should not
>> perform any content checking of annotations but leave that to tool
>> implementations if it is used for other purposes than merely tags. For a
>> more complete support of annotations the AST and IR needs further work but
>> this patch is a step on the way.
>> 
>> Having pluggable attributes would be cool but it would face even greater
>> problems than you describe. Unless pluggable attributes involves writing
>> pluggable AST and IR modules the AST and IR must both support some form of
>> generic containers that could handle any type of attribute with any number
>> of parameters. In either case, it would most likely involve substantial
>> architectural changes to many parts.
> 
> Pluggable attributes do not require these component up front (though
> they could be explored for later iterations), but you are correct in
> that they involve more work than simply piggy-backing off the annotate
> attribute. However, that extra work comes with benefits that expanding
> the annotate attribute will never realize with the end results being
> roughly the same (the attribute metadata still gets passed down to the
> LLVM IR and represented in a manner that AST tools can handle).
> 
>> I have revised the patch by adding a second, optional parameter that makes
>> it possible to group annotations, reducing the risk for collisions and since
>> it is optional it won’t break any existing code. A non-zero group string is
>> prepended to the value string when forwarded to IR. The C++11 attribute has
>> been moved to the clang namespace as requested and unit tests and
>> documentation are updated to reflect the changes.
> 
> Can you please split this patch? The C++11 attribute name, associated
> tests, and documentation of what the attribute does currently are not
> controversial and those changes can go in pretty quickly if separated
> from the greater discussion about how to allow generic, custom
> attributes to be exposed.
> 
> ~Aaron
> 
>> 
>> Cheers,
>> Chris
>> 
>> Modified
>>  include/clang/Basic/Attr.td
>>  include/clang/Basic/AttrDocs.td
>>  lib/CodeGen/CodeGenFunction.cpp
>>  lib/CodeGen/CodeGenModule.cpp
>>  lib/CodeGen/CodeGenModule.h
>>  lib/Sema/SemaDeclAttr.cpp
>>  lib/Sema/SemaStmtAttr.cpp
>>  test/Parser/access-spec-attrs.cpp
>>  test/Sema/annotate.c
>> 
>> Added
>>  test/CodeGenCXX/annotations-field.cpp
>>  test/CodeGenCXX/annotations-global.cpp
>>  test/CodeGenCXX/annotations-loc.cpp
>>  test/CodeGenCXX/annotations-var.cpp
>>  test/SemaCXX/attr-annotate.cpp
>> 
>> 
>> 
>> On 9 feb 2017, at 15:07, Aaron Ballman <aaron at aaronballman.com> wrote:
>> 
>> On Sat, Feb 4, 2017 at 8:26 AM, Marcwell Helpdesk via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>> 
>> Many plugins/tools could benefit from having a generic way for communicating
>> control directives directly from the source code to itself (via the AST)
>> when acting, for example, as source code transformers, generators,
>> collectors and the like. Attributes are a suitable way of doing this but
>> most available attributes have predefined functionality and modifying the
>> compiler for every plugin/tool is obviously not an option. There is however
>> one undocumented but existing attribute that could be used for a generic
>> solution if it was slightly modified and expanded - the annotate attribute.
>> 
>> The current functionality of the annotate attribute encompass annotating
>> declarations with arbitrary strings using the GNU spelling. The attached
>> patch expands on this functionality by adding annotation of statements,
>> C++11 spelling and documentation. With the patch applied most of the code
>> could be annotated for the use by any Clang plugin or tool. For a more
>> detailed description of the updated attribute, see patch for "AttrDocs.td".
>> 
>> 
>> I definitely agree with the premise for this work -- having a generic
>> way for Clang to pass attributed information down to LLVM IR is
>> desirable. However, I'm not convinced that the annotate attribute is
>> the correct approach over something like pluggable attributes. My
>> primary concerns stem from the fact that the annotate attribute has no
>> safe guards for feature collisions (you have to pick a unique string,
>> or else you collide with someone else's string, but there's nothing
>> that forces you to do this), has no mechanisms for accepting arguments
>> in a consistent fashion, and provides no way to check the semantics of
>> the attribute. It's basically a minimum viable option for lowering
>> attributed information down to LLVM IR, which is fine for things that
>> aren't in the user's face, but isn't a good general-purpose solution.
>> 
>> I do not have a problem with adding a C++11 spelling to the annotate
>> attribute for the cases we already support, and I definitely think we
>> should document the attribute. But I don't think it makes sense to add
>> it as a statement attribute (and the current patch also leaves out
>> type attributes).
>> 
>> Some quick thoughts on the patch:
>> 
>> Index: include/clang/Basic/Attr.td
>> ===================================================================
>> --- include/clang/Basic/Attr.td (revision 293612)
>> +++ include/clang/Basic/Attr.td (working copy)
>> @@ -462,9 +462,10 @@
>> }
>> 
>> def Annotate : InheritableParamAttr {
>> -  let Spellings = [GNU<"annotate">];
>> +  let Spellings = [GNU<"annotate">,
>> +                   CXX11<"", "annotate", 201612>];
>> 
>> 
>> This should be in the clang namespace and should not be given a third
>> argument (it's not a standards-based attribute).
>> 
>>  let Args = [StringArgument<"Annotation">];
>> -  let Documentation = [Undocumented];
>> +  let Documentation = [AnnotateDocs];
>> }
>> 
>> def ARMInterrupt : InheritableAttr, TargetSpecificAttr<TargetARM> {
>> Index: lib/Sema/SemaStmtAttr.cpp
>> ===================================================================
>> --- lib/Sema/SemaStmtAttr.cpp (revision 293612)
>> +++ lib/Sema/SemaStmtAttr.cpp (working copy)
>> @@ -23,6 +23,24 @@
>> using namespace clang;
>> using namespace sema;
>> 
>> +static Attr *handleAnnotateAttr(Sema &S, Stmt *St, const AttributeList &A,
>> +                                SourceRange Range) {
>> +  // Assert string literal as the annotation's argument.
>> +  StringRef Str;
>> +  if (!S.checkStringLiteralArgumentAttr(A, 0, Str))
>> +    return nullptr;
>> +
>> +  // Assert single argument
>> +  if (A.getNumArgs() > 1) {
>> +    S.Diag(A.getLoc(), diag::err_attribute_wrong_number_arguments)
>> +           << A.getName() << 1;
>> +    return nullptr;
>> +  }
>> +
>> +  return ::new (S.Context) AnnotateAttr(A.getRange(), S.Context, Str,
>> +                                        A.getAttributeSpellingListIndex());
>> +}
>> +
>> static Attr *handleFallThroughAttr(Sema &S, Stmt *St, const AttributeList
>> &A,
>>                                   SourceRange Range) {
>>  FallThroughAttr Attr(A.getRange(), S.Context,
>> @@ -273,6 +291,8 @@
>>           diag::warn_unhandled_ms_attribute_ignored :
>>           diag::warn_unknown_attribute_ignored) << A.getName();
>>    return nullptr;
>> +  case AttributeList::AT_Annotate:
>> +    return handleAnnotateAttr(S, St, A, Range);
>>  case AttributeList::AT_FallThrough:
>>    return handleFallThroughAttr(S, St, A, Range);
>>  case AttributeList::AT_LoopHint:
>> Index: test/SemaCXX/attr-annotate.cpp
>> ===================================================================
>> --- test/SemaCXX/attr-annotate.cpp (nonexistent)
>> +++ test/SemaCXX/attr-annotate.cpp (working copy)
>> @@ -0,0 +1,17 @@
>> +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
>> +
>> +[[annotate("foo")]] void foo() {
>> +  // C++11 decl annotations
>> +  [[annotate("bar")]] int x;
>> +  [[annotate(1)]] int y; // expected-error {{'annotate' attribute requires
>> a string}}
>> +  [[annotate("bar", 1)]] int z1; // expected-error {{'annotate' attribute
>> takes one argument}}
>> +  [[annotate()]] int z2; // expected-error {{'annotate' attribute takes one
>> argument}}
>> +  [[annotate]] int z3; // expected-error {{'annotate' attribute takes one
>> argument}}
>> +
>> +  // C++11 NullStmt annotations
>> +  [[annotate("bar")]];
>> +  [[annotate(1)]]; // expected-error {{'annotate' attribute requires a
>> string}}
>> +  [[annotate("bar", 1)]]; // expected-error {{'annotate' attribute takes
>> one argument}}
>> +  [[annotate()]]; // expected-error {{'annotate' attribute takes one
>> argument}}
>> +  [[annotate]]; // expected-error {{'annotate' attribute takes one
>> argument}}
>> +}
>> 
>> 
>> There should be some codegen tests that show the attribute is properly
>> finding its way down to the LLVM IR. For instance, what would an
>> annotate attribute on a null statement get lowered to? How about a
>> switch statement?
>> 
>> Index: include/clang/Basic/AttrDocs.td
>> ===================================================================
>> --- include/clang/Basic/AttrDocs.td (revision 293612)
>> +++ include/clang/Basic/AttrDocs.td (working copy)
>> @@ -2812,3 +2812,97 @@
>> ensure that this class cannot be subclassed.
>>  }];
>> }
>> +
>> +def DocCatGeneric : DocumentationCategory<"Generic Attributes"> {
>> +  let Content = [{
>> +These attributes are suitable for passing arbitrary information from the
>> code to
>> +a plugin or other clang based tools such as source code transformers,
>> generators,
>> +collectors and similar. The attributes are not tied to any type of solution
>> or
>> +predefined functionality and may be used in a variety of situations when a
>> generic
>> +attribute suffice.
>> +  }];
>> +}
>> +
>> +def AnnotateDocs : Documentation {
>> +  let Category = DocCatGeneric;
>> +  let Content = [{
>> +The ``annotate`` attribute may be used to annotate any type of declarations
>> or
>> +statements with arbitrary information that are propagated to the AST. The
>> attribute
>> +takes one string parameter containing the annotation value. The content,
>> format
>> +and the exact meaning of the value is up to the programmer to decide.
>> +
>> +C++11 spelling examples:
>> +
>> +.. code-block:: c++
>> +
>> +  class [[annotate("plain")]] Object
>> +  { . . . };
>> +
>> +  int main(int argc, char* argv[])
>> +  {
>> +    [[annotate("local_var")]]   // Decl annotation
>> +    int n = 1 ;
>> +
>> +    // Multiple Stmt annotations
>> +    [[annotate("group-A"), annotate("opt-1:2")]]
>> +    switch(n)
>> +    { . . . }
>> +    return 0;
>> +  }
>> +
>> +Looking at the AST for the example above reveals that any annotated
>> statements
>> +are wrapped into an AttributedStmt node together with the annotation
>> attributes
>> +in comparison to annotated declarations where the annotation attributes are
>> +attached directly beneath the declaration node.
>> +
>> +.. code-block:: text
>> +
>> +  TranslationUnitDecl 0x345fb20 <<invalid sloc>> <invalid sloc>
>> +  | . . .
>> +  |-CXXRecordDecl 0x34b9b58 <example.cpp:2:1, line:10:1> line:2:29 class
>> Object definition
>> +  | |-AnnotateAttr 0x34b9c10 <col:9, col:25> "plain"
>> +  | |-CXXRecordDecl 0x34b9cb8 <col:1, col:29> col:29 implicit class Object
>> +  | |-AccessSpecDecl 0x34b9d50 <line:4:3, col:9> col:3 public
>> +  | ` . . .
>> +  `-FunctionDecl 0x34ba240 <line:12:1, line:28:1> line:12:5 main 'int (int,
>> char **)'
>> +    |-ParmVarDecl 0x34ba020 <col:10, col:14> col:14 argc 'int'
>> +    |-ParmVarDecl 0x34ba130 <col:20, col:31> col:26 argv 'char **':'char
>> **'
>> +    `-CompoundStmt 0x34ba7f0 <line:13:1, line:28:1>
>> +      |-DeclStmt 0x34ba4d0 <line:15:2, col:12>
>> +      | `-VarDecl 0x34ba400 <col:2, col:10> col:6 used n 'int' cinit
>> +      |   |-IntegerLiteral 0x34ba4b0 <col:10> 'int' 1
>> +      |   `-AnnotateAttr 0x34ba460 <line:14:4, col:24> "local_var"
>> +      |-AttributedStmt 0x34ba790 <line:17:2, line:26:2>
>> +      | |-AnnotateAttr 0x34ba750 <line:17:25, col:43> "opt-1:2"
>> +      | |-AnnotateAttr 0x34ba770 <col:4, col:22> "group-A"
>> +      | `-SwitchStmt 0x34ba608 <line:18:2, line:26:2>
>> +      |  . . .
>> +      `-ReturnStmt 0x34ba7d8 <line:27:2, col:9>
>> +        `-IntegerLiteral 0x34ba7b8 <col:9> 'int' 0
>> +
>> +The GNU spelling, ``__attribute((annotate("str")))``, is limited to
>> annotating
>> +declarations but is at the same time a bit more relaxed on the allowed
>> insertion
>> +points compared to the C++11 spelling that needs to be defined prior to any
>> +declaration or statement.
>> +
>> +In the example below the GNU-only insertion points are shown as __attr.
>> +
>> +.. code-block:: c++
>> +
>> +  void __attr function/method(int __attr n) __attr
>> +  int __attr n = 1;
>> +
>> +(Note that it is also possible to annotate arbitrary expressions using the
>> GCC
>> +syntax ``int __builtin_annotation(int, "str")`` where the ``int`` is an
>> integer
>> +of any compiler supported bit-width and with the return value set to the
>> same
>> +as the parameter value. However, the annotation does not show up in the AST
>> as
>> +an AnnotateAttr but as an ordinary CallExpr).
>> +
>> +The annotations of variable, function and method declarations (and
>> expressions)
>> +are forwarded into IR during code generation using the LLVM annotation
>> intrinsic
>> +functions or by adding the information to a global annotation table. The
>> +information added, in either case, are the annotation string, file name and
>> +line number of the declaration or expression. These IR annotations may be
>> used
>> +by optimizers or other back-end tools.
>> +  }];
>> +}
>> 
>> 
>> ~Aaron
>> 
>> 
>> An example demonstratiing the use and syntax of the updated attribute:
>> 
>> class [[annotate("plain")]] Object
>> { . . . };
>> 
>> int main(int argc, char* argv[]) __attribute((annotate("entry-point")))
>> {
>>   [[annotate("local_var")]]   // Decl annotation
>>   int n = 1 ;
>> 
>>   // Multiple Stmt annotations
>>   [[annotate("group-A"), annotate("opt-1:2")]]
>>   switch(n)
>>   { . . . }
>>   return 0;
>> }
>> 
>> Cheers,
>> Chris
>> 
>> Modified:
>> include/clang/Basic/Attr.td
>> include/clang/Basic/AttrDocs.td
>> lib/Sema/SemaStmtAttr.cpp
>> Added:
>> test/SemaCXX/attr-annotate.cpp
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> 
>> 
>> 



More information about the cfe-commits mailing list