Refactoring the attribute plugin example to fit the new API

[psionic12]

Summary

Make the example compiled and the test case passed.

[psionic12]

ping

[aaron.ballman]

clang/examples/Attribute/Attribute.cpp

26–29

The comment is a bit misleading, how about: Can take up to 15 optional arguments, to emulate accepting a variadic number of arguments.?

59–69

This comment describes what the code is doing but not why. How about: The example attribute supports up to 15 optional arguments, but this demonstrates how to test for a specific number of arguments. or something along those lines?

67

If has arguments -> If there are arguments

(And add a full stop to the end of the comment.)

92

This looks dangerous to me -- if there are two or three arguments supplied, then this code will result in a buffer overrun.

clang/test/Frontend/plugin-attribute.cpp

1–2

I don't think we need a GOOD_ATTR and BAD_ATTR run line any longer. Instead, you can use a single RUN line with -verify (no =) and use expected-error and expected-warning on the lines that need the diagnostics. This means you can also get rid of the @ line number constructs and just write the expected comments on the source lines with diagnostics.

12

A downside to this is that we lose the verification that the plugin actually attaches the attribute to the AST node. Another solution that doesn't involve generating LLVM bitcode from a Frontend test would be to use -ast-dump and check the AST nodes.

[psionic12]

Add tests to check if attributes are attached to AST

[psionic12]

Fix grammar

[psionic12]

clang/examples/Attribute/Attribute.cpp

92

Oh, didn't noticed that, but I checked the code it's hard to get the address of the argument buffer, all APIs about it is private, even if ArgsUnion const *getArgsBuffer() const, which I think is not very reasonable. Do you think I should copy all the arguments to a new buffer (not very effective), or do you think I should contribute a patch to make the API public?

There's a third way, getTrailingObjects() is public in TrailingObjects, it is possible to cast an ParsedAttr to a TrailingObjects, but I think it's too hacky.

clang/test/Frontend/plugin-attribute.cpp

1–2

Seems the GOOD_ATTR and BAD_ATTR need to be exist since we use -ast-dump now, we need a clean build without error diagnostics to generate a nice AST.

[psionic12]

Use a vector to collect arguments

[psionic12]

clang/examples/Attribute/Attribute.cpp

92

I take times trying and found out it's more difficult to use the original buffer, so I use a vector to collect the arguments instead.

[aaron.ballman]

clang/examples/Attribute/Attribute.cpp

65

only allowed -> accepts

77

How about: first argument to the 'example' attribute must be a string literal

clang/test/Frontend/plugin-attribute.cpp

1–2

That suggests this should be two test files with one RUN line apiece.

[psionic12]

Fix grammar
Simplify the test case

[aaron.ballman]

clang/test/Frontend/plugin-attribute.cpp

1–2

I am unfamiliar with this construct -- is there a reason to do this as opposed to making two separate files with separate content (these are testing fundamentally different things)?

[psionic12]

clang/test/Frontend/plugin-attribute.cpp

1–2

I just followed the suggestion from this, the document suggest that if the testing files are small, considered to put them in a single file, and use the split-file command. I think the testing infrastructure treat this as two files when testing.

[aaron.ballman]

Thanks for the changes, LGTM!

clang/test/Frontend/plugin-attribute.cpp

1–2

Huh... we have exactly one other test case using this construct, which explains why I've never heard of it. :-D Thank you for pointing out that documentation.

I think it's usually best to keep test files focused on what they're testing, and this tests two different things (AST node attachment and semantic diagnostics) so my natural inclination is to have two separate files. But this provides the same test coverage, so I'd say we can leave it as-is until there's some other reason to split it up.

[psionic12]

Hi, could anyone help to commit this?

[aaron.ballman]

In D92006#2465239, @psionic12 wrote:

Hi, could anyone help to commit this?

I'm happy to do so, sorry about not asking earlier. I've commit on your behalf in b2ba6867eac10874bd279c739639bdb9e60c1996.

[DavidSpickett]

The last update didn't update the expected errors. I have done so in 9a93f95fce91fb4616cee0f307b564b253789282 and it's now passing. (I think it missed the bots since they don't build examples)

[aaron.ballman]

In D92006#2466114, @DavidSpickett wrote:

The last update didn't update the expected errors. I have done so in 9a93f95fce91fb4616cee0f307b564b253789282 and it's now passing. (I think it missed the bots since they don't build examples)

Oh, thank you for this! I'm guessing my local build also missed this for the same reason.

clang/examples/Attribute/Attribute.cpp

 #include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/IR/Attributes.h"
 using namespace clang;
 
 namespace {
 
 struct ExampleAttrInfo : public ParsedAttrInfo {
   ExampleAttrInfo() {
-    // Can take an optional string argument (the check that the argument
-    // actually is a string happens in handleDeclAttribute).
-    OptArgs = 1;
+    // Can take up to 15 optional arguments, to emulate accepting a variadic
+    // number of arguments. This just illustrates how many arguments a
+    // `ParsedAttrInfo` can hold, we will not use that much in this example.
+    OptArgs = 15;
     // GNU-style __attribute__(("example")) and C++-style [[example]] and
     // [[plugin::example]] supported.
     static constexpr Spelling S[] = {{ParsedAttr::AS_GNU, "example"},
                                      {ParsedAttr::AS_CXX11, "example"},
                                      {ParsedAttr::AS_CXX11, "plugin::example"}};
     Spellings = S;
   }
 
   bool diagAppertainsToDecl(Sema &S, const ParsedAttr &Attr,
                             const Decl *D) const override {
     // This attribute appertains to functions only.
     if (!isa<FunctionDecl>(D)) {
       S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
           << Attr << "functions";
       return false;
     }
     return true;
   }
 
   AttrHandling handleDeclAttribute(Sema &S, Decl *D,
                                    const ParsedAttr &Attr) const override {
     // Check if the decl is at file scope.
     if (!D->getDeclContext()->isFileContext()) {
       unsigned ID = S.getDiagnostics().getCustomDiagID(
           DiagnosticsEngine::Error,
           "'example' attribute only allowed at file scope");
       S.Diag(Attr.getLoc(), ID);
       return AttributeNotApplied;
     }
-    // Check if we have an optional string argument.
-    StringRef Str = "";
+    // We make some rules here:
+    // 1. Only accept at most 3 arguments here.
+    // 2. The first argument must be a string literal if it exists.
+    if (Attr.getNumArgs() > 3) {
+      unsigned ID = S.getDiagnostics().getCustomDiagID(
+          DiagnosticsEngine::Error,
+          "'example' attribute only allowed at most three arguments");
+      S.Diag(Attr.getLoc(), ID);
+      return AttributeNotApplied;
+    }
+    // If there are arguments, the first argument should be a string literal.
     if (Attr.getNumArgs() > 0) {
-      Expr *ArgExpr = Attr.getArgAsExpr(0);
+      auto *Arg0 = Attr.getArgAsExpr(0);
       StringLiteral *Literal =
-          dyn_cast<StringLiteral>(ArgExpr->IgnoreParenCasts());
-      if (Literal) {
-        Str = Literal->getString();
-      } else {
-        S.Diag(ArgExpr->getExprLoc(), diag::err_attribute_argument_type)
-            << Attr.getAttrName() << AANT_ArgumentString;
+          dyn_cast<StringLiteral>(Arg0->IgnoreParenCasts());
+      if (!Literal) {
+        unsigned ID = S.getDiagnostics().getCustomDiagID(
+            DiagnosticsEngine::Error,
+            "'example's first argument should be a string literal");
+        S.Diag(Attr.getLoc(), ID);
         return AttributeNotApplied;
       }
+      SmallVector<Expr *, 16> ArgsBuf;
+      for (unsigned i = 0; i < Attr.getNumArgs(); i++) {
+        ArgsBuf.push_back(Attr.getArgAsExpr(i));
       }
+      D->addAttr(AnnotateAttr::Create(S.Context, "example", ArgsBuf.data(),
+                                      ArgsBuf.size(), Attr.getRange()));
+    } else {
       // Attach an annotate attribute to the Decl.
-    D->addAttr(AnnotateAttr::Create(S.Context, "example(" + Str.str() + ")",
+      D->addAttr(AnnotateAttr::Create(S.Context, "example", nullptr, 0,
                                       Attr.getRange()));
+    }
     return AttributeApplied;
   }
 };
 
 } // namespace
 
 static ParsedAttrInfoRegistry::Add<ExampleAttrInfo> X("example", "");

clang/test/Frontend/plugin-attribute.cpp

-// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -S %s -o - 2>&1 | FileCheck %s --check-prefix=ATTRIBUTE
-// RUN: not %clang -fplugin=%llvmshlibdir/Attribute%pluginext -emit-llvm -DBAD_ATTRIBUTE -S %s -o - 2>&1 | FileCheck %s --check-prefix=BADATTRIBUTE
+// RUN: %clang -cc1 -load %llvmshlibdir/Attribute%pluginext -DGOOD_ATTR -fsyntax-only -ast-dump -verify=goodattr %s | FileCheck %s
+// RUN: %clang -fplugin=%llvmshlibdir/Attribute%pluginext -DBAD_ATTR -fsyntax-only -Xclang -verify=badattr %s
 // REQUIRES: plugins, examples
-
-void fn1a() __attribute__((example)) { }
-[[example]] void fn1b() { }
-[[plugin::example]] void fn1c() { }
-void fn2() __attribute__((example("somestring"))) { }
-// ATTRIBUTE: warning: 'example' attribute only applies to functions
-int var1 __attribute__((example("otherstring"))) = 1;
-
-// ATTRIBUTE: [[STR1_VAR:@.+]] = private unnamed_addr constant [10 x i8] c"example()\00"
-// ATTRIBUTE: [[STR2_VAR:@.+]] = private unnamed_addr constant [20 x i8] c"example(somestring)\00"
-// ATTRIBUTE: @llvm.global.annotations = {{.*}}@{{.*}}fn1a{{.*}}[[STR1_VAR]]{{.*}}@{{.*}}fn1b{{.*}}[[STR1_VAR]]{{.*}}@{{.*}}fn1c{{.*}}[[STR1_VAR]]{{.*}}@{{.*}}fn2{{.*}}[[STR2_VAR]]
+#ifdef GOOD_ATTR
+// goodattr-no-diagnostics
+void fn1a() __attribute__((example)) {}
+[[example]] void fn1b() {}
+[[plugin::example]] void fn1c() {}
+void fn2() __attribute__((example("somestring", 1, 2.0))) {}
+// CHECK-COUNT-4: -AnnotateAttr 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -StringLiteral 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} 'const char [{{[0-9]+}}]' lvalue "somestring"
+// CHECK: -IntegerLiteral 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} 'int' 1
+// CHECK: -FloatingLiteral 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} 'double' 2.000000e+00
+#endif
 
-#ifdef BAD_ATTRIBUTE
+#ifdef BAD_ATTR
+int var1 __attribute__((example("otherstring"))) = 1; // badattr-warning {{'example' attribute only applies to functions}}
 class Example {
-  // BADATTRIBUTE: error: 'example' attribute only allowed at file scope
-  void __attribute__((example)) fn3();
+  void __attribute__((example)) fn3(); // badattr-error {{'example' attribute only allowed at file scope}}
 };
-// BADATTRIBUTE: error: 'example' attribute requires a string
-void fn4() __attribute__((example(123))) { }
-// BADATTRIBUTE: error: 'example' attribute takes no more than 1 argument
-void fn5() __attribute__((example("a","b"))) { }
+void fn4() __attribute__((example(123))) { } // badattr-error {{'example's first argument should be a string literal}}
+void fn5() __attribute__((example("a","b", 3, 4.0))) { } // badattr-error {{'example' attribute only allowed at most three arguments}}
 #endif