[clang][frontend] Add support for attribute plugins for statement attributes

[ericastor]

We already have support for declaration attributes; this is just a matter of extending the plugin infrastructure to cover one more case.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Eric Astor (ericastor)

Changes

We already have support for declaration attributes; this is just a matter of extending the plugin infrastructure to cover one more case.

---

Full diff: https://github.com/llvm/llvm-project/pull/110334.diff

5 Files Affected:

• (modified) clang/docs/ClangPlugins.rst (+12-5)
• (modified) clang/examples/Attribute/Attribute.cpp (+49)
• (modified) clang/include/clang/Basic/ParsedAttrInfo.h (+10)
• (modified) clang/lib/Sema/SemaStmtAttr.cpp (+4)
• (modified) clang/test/Frontend/plugin-attribute.cpp (+33-7)

diff --git a/clang/docs/ClangPlugins.rst b/clang/docs/ClangPlugins.rst
index 001e66e434efb1..92e41fb5877fe8 100644
--- a/clang/docs/ClangPlugins.rst
+++ b/clang/docs/ClangPlugins.rst
@@ -92,11 +92,6 @@ The members of ``ParsedAttrInfo`` that a plugin attribute must define are:
    attribute, each of which consists of an attribute syntax and how the
    attribute name is spelled for that syntax. If the syntax allows a scope then
    the spelling must be "scope::attr" if a scope is present or "::attr" if not.
- * ``handleDeclAttribute``, which is the function that applies the attribute to
-   a declaration. It is responsible for checking that the attribute's arguments
-   are valid, and typically applies the attribute by adding an ``Attr`` to the
-   ``Decl``. It returns either ``AttributeApplied``, to indicate that the
-   attribute was successfully applied, or ``AttributeNotApplied`` if it wasn't.
 
 The members of ``ParsedAttrInfo`` that may need to be defined, depending on the
 attribute, are:
@@ -105,6 +100,18 @@ attribute, are:
    arguments to the attribute.
  * ``diagAppertainsToDecl``, which checks if the attribute has been used on the
    right kind of declaration and issues a diagnostic if not.
+ * ``handleDeclAttribute``, which is the function that applies the attribute to
+   a declaration. It is responsible for checking that the attribute's arguments
+   are valid, and typically applies the attribute by adding an ``Attr`` to the
+   ``Decl``. It returns either ``AttributeApplied``, to indicate that the
+   attribute was successfully applied, or ``AttributeNotApplied`` if it wasn't.
+ * ``diagAppertainsToStmt``, which checks if the attribute has been used on the
+   right kind of statement and issues a diagnostic if not.
+ * ``handleStmtAttribute``, which is the function that applies the attribute to
+   a statement. It is responsible for checking that the attribute's arguments
+   are valid, and typically applies the attribute by adding an ``Attr`` to the
+   ``Stmt``. It returns either ``AttributeApplied``, to indicate that the
+   attribute was successfully applied, or ``AttributeNotApplied`` if it wasn't.
  * ``diagLangOpts``, which checks if the attribute is permitted for the current
    language mode and issues a diagnostic if not.
  * ``existsInTarget``, which checks if the attribute is permitted for the given
diff --git a/clang/examples/Attribute/Attribute.cpp b/clang/examples/Attribute/Attribute.cpp
index 9d6cf9ae36c6a6..07dd19321195c8 100644
--- a/clang/examples/Attribute/Attribute.cpp
+++ b/clang/examples/Attribute/Attribute.cpp
@@ -94,6 +94,55 @@ struct ExampleAttrInfo : public ParsedAttrInfo {
     }
     return AttributeApplied;
   }
+
+  bool diagAppertainsToStmt(Sema &S, const ParsedAttr &Attr,
+                            const Stmt *St) const override {
+    // This attribute appertains to for loop statements only.
+    if (!isa<ForStmt>(St)) {
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
+          << Attr << Attr.isRegularKeywordAttribute() << "for loop statements";
+      return false;
+    }
+    return true;
+  }
+
+  AttrHandling handleStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &Attr,
+                                   class Attr *&Result) const override {
+    // 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 accepts 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) {
+      auto *Arg0 = Attr.getArgAsExpr(0);
+      StringLiteral *Literal =
+          dyn_cast<StringLiteral>(Arg0->IgnoreParenCasts());
+      if (!Literal) {
+        unsigned ID = S.getDiagnostics().getCustomDiagID(
+            DiagnosticsEngine::Error, "first argument to the 'example' "
+                                      "attribute must 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));
+      }
+      Result = AnnotateAttr::Create(S.Context, "example", ArgsBuf.data(),
+                                    ArgsBuf.size(), Attr.getRange());
+    } else {
+      // Attach an annotate attribute to the Decl.
+      Result = AnnotateAttr::Create(S.Context, "example", nullptr, 0,
+                                    Attr.getRange());
+    }
+    return AttributeApplied;
+  }
 };
 
 } // namespace
diff --git a/clang/include/clang/Basic/ParsedAttrInfo.h b/clang/include/clang/Basic/ParsedAttrInfo.h
index 537d8f3391d589..fab5c6f1377d27 100644
--- a/clang/include/clang/Basic/ParsedAttrInfo.h
+++ b/clang/include/clang/Basic/ParsedAttrInfo.h
@@ -24,6 +24,7 @@
 
 namespace clang {
 
+class Attr;
 class Decl;
 class LangOptions;
 class ParsedAttr;
@@ -154,6 +155,15 @@ struct ParsedAttrInfo {
                                            const ParsedAttr &Attr) const {
     return NotHandled;
   }
+  /// If this ParsedAttrInfo knows how to handle this ParsedAttr applied to this
+  /// Stmt then do so (referencing the resulting Attr in Result) and return
+  /// either AttributeApplied if it was applied or AttributeNotApplied if it
+  /// wasn't. Otherwise return NotHandled.
+  virtual AttrHandling handleStmtAttribute(Sema &S, Stmt *St,
+                                           const ParsedAttr &Attr,
+                                           class Attr *&Result) const {
+    return NotHandled;
+  }
 
   static const ParsedAttrInfo &get(const AttributeCommonInfo &A);
   static ArrayRef<const ParsedAttrInfo *> getAllBuiltin();
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index b9b3b4063bc383..3ebd1148e8bbfb 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -680,6 +680,10 @@ static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
   case ParsedAttr::AT_NoConvergent:
     return handleNoConvergentAttr(S, St, A, Range);
   default:
+    if (Attr *AT = nullptr; A.getInfo().handleStmtAttribute(S, St, A, AT) !=
+                            ParsedAttrInfo::NotHandled) {
+      return AT;
+    }
     // N.B., ClangAttrEmitter.cpp emits a diagnostic helper that ensures a
     // declaration attribute is not written on a statement, but this code is
     // needed for attributes in Attr.td that do not list any subjects.
diff --git a/clang/test/Frontend/plugin-attribute.cpp b/clang/test/Frontend/plugin-attribute.cpp
index 1c5a2440b26888..2e9d171a0095a9 100644
--- a/clang/test/Frontend/plugin-attribute.cpp
+++ b/clang/test/Frontend/plugin-attribute.cpp
@@ -4,11 +4,33 @@
 // REQUIRES: plugins, examples
 //--- good_attr.cpp
 // expected-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"
+void fn1a() __attribute__((example)) {
+  __attribute__((example)) for (int i = 0; i < 10; ++i) {}
+}
+[[example]] void fn1b() {
+  [[example]] for (int i = 0; i < 10; ++i) {}
+}
+[[plugin::example]] void fn1c() {
+  [[plugin::example]] for (int i = 0; i < 10; ++i) {}
+}
+void fn2() __attribute__((example("somestring", 1, 2.0))) {
+  __attribute__((example("abc", 3, 4.0))) for (int i = 0; i < 10; ++i) {}
+}
+// CHECK: -AttributedStmt 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}}
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AttributedStmt 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}}
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AttributedStmt 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}}
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}} "example"
+// CHECK: -AttributedStmt 0x{{[0-9a-z]+}} {{<line:[0-9]+:[0-9]+(, col:[0-9]+)?>}}
+// CHECK: -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 "abc"
+// CHECK: -IntegerLiteral 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} 'int' 3
+// CHECK: -FloatingLiteral 0x{{[0-9a-z]+}} {{<col:[0-9]+(, col:[0-9]+)?>}} 'double' 4.000000e+00
+// CHECK: -AnnotateAttr 0x{{[0-9a-z]+}} {{<line:[0-9]+:[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
@@ -18,5 +40,9 @@ int var1 __attribute__((example("otherstring"))) = 1; // expected-warning {{'exa
 class Example {
   void __attribute__((example)) fn3(); // expected-error {{'example' attribute only allowed at file scope}}
 };
-void fn4() __attribute__((example(123))) { } // expected-error {{first argument to the 'example' attribute must be a string literal}}
-void fn5() __attribute__((example("a","b", 3, 4.0))) { } // expected-error {{'example' attribute only accepts at most three arguments}}
+void fn4() __attribute__((example(123))) { // expected-error {{first argument to the 'example' attribute must be a string literal}}
+  __attribute__((example("somestring"))) while (true); // expected-warning {{'example' attribute only applies to for loop statements}}
+}
+void fn5() __attribute__((example("a","b", 3, 4.0))) { // expected-error {{'example' attribute only accepts at most three arguments}}
+  __attribute__((example("a","b", 3, 4.0))) for (int i = 0; i < 10; ++i) {} // expected-error {{'example' attribute only accepts at most three arguments}}
+}

[ilya-biryukov]

This looks like a relatively straightforward extension in line with the current Clang interfaces. Therefore, I feel we should accept this.

However, please wait for feedback from Clang owners to make sure this does not go against some non-obvious trade-offs or future plans that Clang has.

cc @AaronBallman, @cor3ntin

[Sirraide]

Looks ok to me, but I’ll leave it to @erichkeane to approve this as he’s the attributes code owner.

[erichkeane]

So the problem with statement attributes is that we don't really have a good way to do instantiation of them on a template, which is why we held off on this in the first place. The infrastructure for instantiation DOES happen on decl attributes automatically anyway, so that made it easier to do.

So please ensure there is some sort of hooks in the statement attributes to ensure that they get a chance to instantatiate themselves (and add tests with templates in them!).

[ericastor]

So the problem with statement attributes is that we don't really have a good way to do instantiation of them on a template, which is why we held off on this in the first place. The infrastructure for instantiation DOES happen on decl attributes automatically anyway, so that made it easier to do.

So please ensure there is some sort of hooks in the statement attributes to ensure that they get a chance to instantatiate themselves (and add tests with templates in them!).

This is a fair point... how are standard statement attributes handled inside of templates? (Assuming clang supports that.)

[erichkeane]

So the problem with statement attributes is that we don't really have a good way to do instantiation of them on a template, which is why we held off on this in the first place. The infrastructure for instantiation DOES happen on decl attributes automatically anyway, so that made it easier to do.
So please ensure there is some sort of hooks in the statement attributes to ensure that they get a chance to instantatiate themselves (and add tests with templates in them!).

This is a fair point... how are standard statement attributes handled inside of templates? (Assuming clang supports that.)

All our handling of attributes is pretty manual for anything with arguments. We have a bit of an automatic handling for many attributes, but arguments aren't. I think there is a section in SemaInstantiateDecl.cpp that handles a lot of it (InstantiateAttrs) and you can see there is a LOT of manual handling there

[ericastor]

So... I think this turned out to be surprisingly easy. After all, we don't actually allow custom attributes to create entirely new attributes in the AST, since they can't create new legal AST entries... instead, the example creates AnnotateAttrs that can then be observed & acted on by the compiler-extending plugin.

It looks like reaching the same level of support just required adding AnnotateAttr support to the template instantiation logic. Am I missing anything else? @erichkeane

[erichkeane]

This seems reasonable? I actually didn't know we weren't letting them create new attributes, and were just using AnnotateAttr in the AST. I think @AaronBallman should do 1 run through this as he understands the plugins better than I, but this otherwise LGTM.

[erichkeane]

we probably want to transform the annotation as well? or does StringArgument not work for dependent values?

[ericastor]

I think StringArgument only handles StringRefs? I don't see a way it could be given a dependent value.

[AaronBallman]

LGTM, though we should probably add a release note to clang/docs/ReleaseNotes.rst so users know that the plugin system got a bit more functionality.