[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.
Closed by commit rG665da187ccf3: [Clang] Add the `annotate_type` attribute 
(authored by mboehme).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked an inline comment as done.
mboehme added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:349
 
+- Added the `clang::annotate_type` attribute, which can be used to add
+  annotations to types (see documentation for details).

aaron.ballman wrote:
> 
Thanks for catching -- fixed. (I'm too used to Markdown syntax...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 436764.
mboehme marked an inline comment as done.
mboehme added a comment.

Fixed syntax error in ReleaseNotes.rst.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for this! I did find one small formatting issue in the release 
notes, but you can fix that up when landing.




Comment at: clang/docs/ReleaseNotes.rst:349
 
+- Added the `clang::annotate_type` attribute, which can be used to add
+  annotations to types (see documentation for details).




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked an inline comment as done.
mboehme added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6501
+
+The attribute does not have any effect on the semantics of C++ code, neither
+type checking rules, nor runtime semantics. In particular:

aaron.ballman wrote:
> (Since this is for C as well as C++)
Good point -- done.



Comment at: clang/lib/Parse/ParseDecl.cpp:3187-3193
+  // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though 
they
+  // are type attributes, because we historically haven't allowed these
+  // to be used as type attributes in C++11 / C2x syntax.
+  if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_LifetimeBound 
&&
+  PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
+continue;
+  Diag(PA.getLoc(), diag::err_attribute_not_type_attr) << PA;

aaron.ballman wrote:
> Should this be done as part of D126061 instead?
Thanks for raising this question!

In this patch, I had originally allowed all type attributes to be placed on the 
decl-specifier-seq because a) I needed this to be true for `annotate_type` so 
that I could test the attribute in this position, and b) there didn't appear to 
be any harm in allowing any type attribute instead of just `annotate_type`.

However, subsequent discussion on https://reviews.llvm.org/D126061 has made it 
clear that not all type attributes should actually be allowed on the 
decl-specifier-seq -- at least not initially, since they won't currently work 
correctly in that context (see extensive discussion on that other patch). So 
I've made the code here more restrictive to allow only `annotate_type` on the 
decl-specifier-seq (which is all that I need here).

https://reviews.llvm.org/D126061 now contains the change that allows type 
attributes on the decl-specifier-seq more generally, which is fine because it 
also contains additional code to disable this behavior again for the specific 
attributes that need it.

Given that I'm planning to land this patch and https://reviews.llvm.org/D126061 
at the same time, it's not _super_ critical where we make this change, but a) 
the way it's done now makes the patches more logically cohesive, and b) there's 
always a chance that a patch may need to be reverted, and having the right 
changes together in one patch will be important then.

Thanks again for bringing this to my attention!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 436732.
mboehme marked 2 inline comments as done.
mboehme added a comment.

Instead of allowing all type attributes on the decl-specifier-seq, allow only
`annotate_type` for now. The more general change will be done in
https://reviews.llvm.org/D126061.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 436713.
mboehme added a comment.

Change attribute documentation so it's not specific to C++.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+AssertAnnotatedAs(Func->getParamDecl(1)->getTypeSourceInfo()->getTypeLoc(),
+  

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I really only had one salient question (and a tiny nit), so I think this is 
almost good to go.




Comment at: clang/include/clang/Basic/AttrDocs.td:6501
+
+The attribute does not have any effect on the semantics of C++ code, neither
+type checking rules, nor runtime semantics. In particular:

(Since this is for C as well as C++)



Comment at: clang/lib/Parse/ParseDecl.cpp:3187-3193
+  // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though 
they
+  // are type attributes, because we historically haven't allowed these
+  // to be used as type attributes in C++11 / C2x syntax.
+  if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_LifetimeBound 
&&
+  PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
+continue;
+  Diag(PA.getLoc(), diag::err_attribute_not_type_attr) << PA;

Should this be done as part of D126061 instead?



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> mboehme wrote:
> > mboehme wrote:
> > > mboehme wrote:
> > > > rsmith wrote:
> > > > > mboehme wrote:
> > > > > > rsmith wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > > Can you also add some test coverage for 
> > > > > > > > > > > > > > > > > > > applying the attribute to a declaration 
> > > > > > > > > > > > > > > > > > > instead of a type or not giving it any 
> > > > > > > > > > > > > > > > > > > arguments? Also, should test arguments 
> > > > > > > > > > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > > > > > > > > > I've added tests as you suggested, though I 
> > > > > > > > > > > > > > > > > > put most of them in Sema/annotate-type.c, 
> > > > > > > > > > > > > > > > > > as they're not specific to C++.
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > Thanks for prompting me to do this -- the 
> > > > > > > > > > > > > > > > > > tests caused me to notice and fix a number 
> > > > > > > > > > > > > > > > > > of bugs.
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > I haven't managed to diagnose the case when 
> > > > > > > > > > > > > > > > > > the attribute appears at the beginning of 
> > > > > > > > > > > > > > > > > > the declaration. It seems to me that, at 
> > > > > > > > > > > > > > > > > > the point where I've added the check, this 
> > > > > > > > > > > > > > > > > > case is indistinguishable from an attribute 
> > > > > > > > > > > > > > > > > > that appears on the type. In both cases, 
> > > > > > > > > > > > > > > > > > the `TAL` is `TAL_DeclSpec`, and the 
> > > > > > > > > > > > > > > > > > attribute is contained in 
> > > > > > > > > > > > > > > > > > `DeclSpec::getAttributes()`. This is 
> > > > > > > > > > > > > > > > > > because `Parser::ParseSimpleDeclaration` 
> > > > > > > > > > > > > > > > > > forwards the declaration attributes to the 
> > > > > > > > > > > > > > > > > > `DeclSpec`:
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > I believe if I wanted to prohibit this 
> > > > > > > > > > > > > > > > > > case, I would need to add a check to 
> > > > > > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > > > > > > > > > > prohibit any occurrences of `annotate_type` 
> > > > > > > > > > > > > > > > > > there. However, this seems the wrong place 
> > > > > > > > > > > > > > > > > > to do this because it doesn't contain any 
> > > > > > > > > > > > > > > > > > specific processing for other attributes.
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > I've noticed that Clang also doesn't 
> > > > > > > > > > > > > > > > > > prohibit other type attributes (even ones 
> > > > > > > > > > > > > > > > > > with C++ 11 syntax) from being applied to 
> > > > > > > > > > > > > > > > > > declarations. For example: 
> > > > > > > > > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > How do you think I should proceed here? I 
> > > > > > > > > > > > > > > > > > think the underlying issue is that Clang 
> > > > > > > > > > > > > > > > > > doesn't always 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

Is there anything else that needs to happen on this change before it's ready to 
land?

There is some breakage in the pre-merge checks, but it looks as if it's 
pre-existing breakage. See the comment-only change here that looks as if it 
exhibits similar breakage:

https://reviews.llvm.org/D127494




Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> mboehme wrote:
> > mboehme wrote:
> > > rsmith wrote:
> > > > mboehme wrote:
> > > > > rsmith wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > mboehme wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > mboehme wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > > Can you also add some test coverage for 
> > > > > > > > > > > > > > > > > > applying the attribute to a declaration 
> > > > > > > > > > > > > > > > > > instead of a type or not giving it any 
> > > > > > > > > > > > > > > > > > arguments? Also, should test arguments 
> > > > > > > > > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > > > > > > > > I've added tests as you suggested, though I 
> > > > > > > > > > > > > > > > > put most of them in Sema/annotate-type.c, as 
> > > > > > > > > > > > > > > > > they're not specific to C++.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > Thanks for prompting me to do this -- the 
> > > > > > > > > > > > > > > > > tests caused me to notice and fix a number of 
> > > > > > > > > > > > > > > > > bugs.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > I haven't managed to diagnose the case when 
> > > > > > > > > > > > > > > > > the attribute appears at the beginning of the 
> > > > > > > > > > > > > > > > > declaration. It seems to me that, at the 
> > > > > > > > > > > > > > > > > point where I've added the check, this case 
> > > > > > > > > > > > > > > > > is indistinguishable from an attribute that 
> > > > > > > > > > > > > > > > > appears on the type. In both cases, the `TAL` 
> > > > > > > > > > > > > > > > > is `TAL_DeclSpec`, and the attribute is 
> > > > > > > > > > > > > > > > > contained in `DeclSpec::getAttributes()`. 
> > > > > > > > > > > > > > > > > This is because 
> > > > > > > > > > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > I believe if I wanted to prohibit this case, 
> > > > > > > > > > > > > > > > > I would need to add a check to 
> > > > > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > > > > > > > > > prohibit any occurrences of `annotate_type` 
> > > > > > > > > > > > > > > > > there. However, this seems the wrong place to 
> > > > > > > > > > > > > > > > > do this because it doesn't contain any 
> > > > > > > > > > > > > > > > > specific processing for other attributes.
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > I've noticed that Clang also doesn't prohibit 
> > > > > > > > > > > > > > > > > other type attributes (even ones with C++ 11 
> > > > > > > > > > > > > > > > > syntax) from being applied to declarations. 
> > > > > > > > > > > > > > > > > For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > How do you think I should proceed here? I 
> > > > > > > > > > > > > > > > > think the underlying issue is that Clang 
> > > > > > > > > > > > > > > > > doesn't always distinguish cleanly between 
> > > > > > > > > > > > > > > > > declaration attributes and type attributes, 
> > > > > > > > > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > > > > > > > > > How do you think I should proceed here? I 
> > > > > > > > > > > > > > > > > think the underlying issue is that Clang 
> > > > > > > > > > > > > > > > > doesn't always distinguish cleanly between 
> > > > > > > > > > > > > > > > > declaration attributes and type attributes, 
> > > > > > > > > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > This is a general issue with attribute 
> > > > > > > > > > > > > > > > processing. I would imagine that 
> > > > > > > > > > > > > > > > SemaDeclAttr.cpp should be able to diagnose 
> > > > > > > > > > > > > > > > that case when the attribute only applies to 
> > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 435524.
mboehme added a comment.

Rebased to a newer base revision.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+AssertAnnotatedAs(Func->getParamDecl(1)->getTypeSourceInfo()->getTypeLoc(),
+  "bar", IntTL);
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> mboehme wrote:
> > rsmith wrote:
> > > mboehme wrote:
> > > > rsmith wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > Can you also add some test coverage for 
> > > > > > > > > > > > > > > > > applying the attribute to a declaration 
> > > > > > > > > > > > > > > > > instead of a type or not giving it any 
> > > > > > > > > > > > > > > > > arguments? Also, should test arguments which 
> > > > > > > > > > > > > > > > > are not a constant expression.
> > > > > > > > > > > > > > > > I've added tests as you suggested, though I put 
> > > > > > > > > > > > > > > > most of them in Sema/annotate-type.c, as 
> > > > > > > > > > > > > > > > they're not specific to C++.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Thanks for prompting me to do this -- the tests 
> > > > > > > > > > > > > > > > caused me to notice and fix a number of bugs.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > I haven't managed to diagnose the case when the 
> > > > > > > > > > > > > > > > attribute appears at the beginning of the 
> > > > > > > > > > > > > > > > declaration. It seems to me that, at the point 
> > > > > > > > > > > > > > > > where I've added the check, this case is 
> > > > > > > > > > > > > > > > indistinguishable from an attribute that 
> > > > > > > > > > > > > > > > appears on the type. In both cases, the `TAL` 
> > > > > > > > > > > > > > > > is `TAL_DeclSpec`, and the attribute is 
> > > > > > > > > > > > > > > > contained in `DeclSpec::getAttributes()`. This 
> > > > > > > > > > > > > > > > is because `Parser::ParseSimpleDeclaration` 
> > > > > > > > > > > > > > > > forwards the declaration attributes to the 
> > > > > > > > > > > > > > > > `DeclSpec`:
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > I believe if I wanted to prohibit this case, I 
> > > > > > > > > > > > > > > > would need to add a check to 
> > > > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > > > > > > > > prohibit any occurrences of `annotate_type` 
> > > > > > > > > > > > > > > > there. However, this seems the wrong place to 
> > > > > > > > > > > > > > > > do this because it doesn't contain any specific 
> > > > > > > > > > > > > > > > processing for other attributes.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > I've noticed that Clang also doesn't prohibit 
> > > > > > > > > > > > > > > > other type attributes (even ones with C++ 11 
> > > > > > > > > > > > > > > > syntax) from being applied to declarations. For 
> > > > > > > > > > > > > > > > example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > How do you think I should proceed here? I think 
> > > > > > > > > > > > > > > > the underlying issue is that Clang doesn't 
> > > > > > > > > > > > > > > > always distinguish cleanly between declaration 
> > > > > > > > > > > > > > > > attributes and type attributes, and fixing this 
> > > > > > > > > > > > > > > > might be nontrivial.
> > > > > > > > > > > > > > > > How do you think I should proceed here? I think 
> > > > > > > > > > > > > > > > the underlying issue is that Clang doesn't 
> > > > > > > > > > > > > > > > always distinguish cleanly between declaration 
> > > > > > > > > > > > > > > > attributes and type attributes, and fixing this 
> > > > > > > > > > > > > > > > might be nontrivial.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > This is a general issue with attribute 
> > > > > > > > > > > > > > > processing. I would imagine that SemaDeclAttr.cpp 
> > > > > > > > > > > > > > > should be able to diagnose that case when the 
> > > > > > > > > > > > > > > attribute only applies to types and not 
> > > > > > > > > > > > > > > declarations, but it'd take some investigation 
> > > > > > > > > > > > > > > for me to be sure.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Because this issue isn't new to your situation, 
> > > > > > > > > > > > > > > I'd recommend filing an issue about the general 
> > > > > > > > > > > > > > > problem and then we can solve that later.
> > > > > > > > > > > > > > I've done some more investigation myself, and I 
> > > > > > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> rsmith wrote:
> > mboehme wrote:
> > > rsmith wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > mboehme wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > mboehme wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > Can you also add some test coverage for 
> > > > > > > > > > > > > > > > applying the attribute to a declaration instead 
> > > > > > > > > > > > > > > > of a type or not giving it any arguments? Also, 
> > > > > > > > > > > > > > > > should test arguments which are not a constant 
> > > > > > > > > > > > > > > > expression.
> > > > > > > > > > > > > > > I've added tests as you suggested, though I put 
> > > > > > > > > > > > > > > most of them in Sema/annotate-type.c, as they're 
> > > > > > > > > > > > > > > not specific to C++.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Thanks for prompting me to do this -- the tests 
> > > > > > > > > > > > > > > caused me to notice and fix a number of bugs.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I haven't managed to diagnose the case when the 
> > > > > > > > > > > > > > > attribute appears at the beginning of the 
> > > > > > > > > > > > > > > declaration. It seems to me that, at the point 
> > > > > > > > > > > > > > > where I've added the check, this case is 
> > > > > > > > > > > > > > > indistinguishable from an attribute that appears 
> > > > > > > > > > > > > > > on the type. In both cases, the `TAL` is 
> > > > > > > > > > > > > > > `TAL_DeclSpec`, and the attribute is contained in 
> > > > > > > > > > > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > > > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I believe if I wanted to prohibit this case, I 
> > > > > > > > > > > > > > > would need to add a check to 
> > > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > > > > > > > prohibit any occurrences of `annotate_type` 
> > > > > > > > > > > > > > > there. However, this seems the wrong place to do 
> > > > > > > > > > > > > > > this because it doesn't contain any specific 
> > > > > > > > > > > > > > > processing for other attributes.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I've noticed that Clang also doesn't prohibit 
> > > > > > > > > > > > > > > other type attributes (even ones with C++ 11 
> > > > > > > > > > > > > > > syntax) from being applied to declarations. For 
> > > > > > > > > > > > > > > example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > How do you think I should proceed here? I think 
> > > > > > > > > > > > > > > the underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > > > distinguish cleanly between declaration 
> > > > > > > > > > > > > > > attributes and type attributes, and fixing this 
> > > > > > > > > > > > > > > might be nontrivial.
> > > > > > > > > > > > > > > How do you think I should proceed here? I think 
> > > > > > > > > > > > > > > the underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > > > distinguish cleanly between declaration 
> > > > > > > > > > > > > > > attributes and type attributes, and fixing this 
> > > > > > > > > > > > > > > might be nontrivial.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This is a general issue with attribute processing. 
> > > > > > > > > > > > > > I would imagine that SemaDeclAttr.cpp should be 
> > > > > > > > > > > > > > able to diagnose that case when the attribute only 
> > > > > > > > > > > > > > applies to types and not declarations, but it'd 
> > > > > > > > > > > > > > take some investigation for me to be sure.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Because this issue isn't new to your situation, I'd 
> > > > > > > > > > > > > > recommend filing an issue about the general problem 
> > > > > > > > > > > > > > and then we can solve that later.
> > > > > > > > > > > > > I've done some more investigation myself, and I think 
> > > > > > > > > > > > > I've come up with a solution; actually, two potential 
> > > > > > > > > > > > > solutions. I have draft patches for both of these; I 
> > > > > > > > > > > > > wanted to run these by you here first, so I 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

rsmith wrote:
> mboehme wrote:
> > rsmith wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > Can you also add some test coverage for applying 
> > > > > > > > > > > > > > > the attribute to a declaration instead of a type 
> > > > > > > > > > > > > > > or not giving it any arguments? Also, should test 
> > > > > > > > > > > > > > > arguments which are not a constant expression.
> > > > > > > > > > > > > > I've added tests as you suggested, though I put 
> > > > > > > > > > > > > > most of them in Sema/annotate-type.c, as they're 
> > > > > > > > > > > > > > not specific to C++.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Thanks for prompting me to do this -- the tests 
> > > > > > > > > > > > > > caused me to notice and fix a number of bugs.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I haven't managed to diagnose the case when the 
> > > > > > > > > > > > > > attribute appears at the beginning of the 
> > > > > > > > > > > > > > declaration. It seems to me that, at the point 
> > > > > > > > > > > > > > where I've added the check, this case is 
> > > > > > > > > > > > > > indistinguishable from an attribute that appears on 
> > > > > > > > > > > > > > the type. In both cases, the `TAL` is 
> > > > > > > > > > > > > > `TAL_DeclSpec`, and the attribute is contained in 
> > > > > > > > > > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I believe if I wanted to prohibit this case, I 
> > > > > > > > > > > > > > would need to add a check to 
> > > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and prohibit 
> > > > > > > > > > > > > > any occurrences of `annotate_type` there. However, 
> > > > > > > > > > > > > > this seems the wrong place to do this because it 
> > > > > > > > > > > > > > doesn't contain any specific processing for other 
> > > > > > > > > > > > > > attributes.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I've noticed that Clang also doesn't prohibit other 
> > > > > > > > > > > > > > type attributes (even ones with C++ 11 syntax) from 
> > > > > > > > > > > > > > being applied to declarations. For example: 
> > > > > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > > distinguish cleanly between declaration attributes 
> > > > > > > > > > > > > > and type attributes, and fixing this might be 
> > > > > > > > > > > > > > nontrivial.
> > > > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > > distinguish cleanly between declaration attributes 
> > > > > > > > > > > > > > and type attributes, and fixing this might be 
> > > > > > > > > > > > > > nontrivial.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This is a general issue with attribute processing. I 
> > > > > > > > > > > > > would imagine that SemaDeclAttr.cpp should be able to 
> > > > > > > > > > > > > diagnose that case when the attribute only applies to 
> > > > > > > > > > > > > types and not declarations, but it'd take some 
> > > > > > > > > > > > > investigation for me to be sure.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Because this issue isn't new to your situation, I'd 
> > > > > > > > > > > > > recommend filing an issue about the general problem 
> > > > > > > > > > > > > and then we can solve that later.
> > > > > > > > > > > > I've done some more investigation myself, and I think 
> > > > > > > > > > > > I've come up with a solution; actually, two potential 
> > > > > > > > > > > > solutions. I have draft patches for both of these; I 
> > > > > > > > > > > > wanted to run these by you here first, so I haven't 
> > > > > > > > > > > > opened a bug yet.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'd appreciate your input on how you'd prefer me to 
> > > > > > > > > > > > proceed with this. I do 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> rsmith wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > mboehme wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > mboehme wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > Can you also add some test coverage for applying 
> > > > > > > > > > > > > > the attribute to a declaration instead of a type or 
> > > > > > > > > > > > > > not giving it any arguments? Also, should test 
> > > > > > > > > > > > > > arguments which are not a constant expression.
> > > > > > > > > > > > > I've added tests as you suggested, though I put most 
> > > > > > > > > > > > > of them in Sema/annotate-type.c, as they're not 
> > > > > > > > > > > > > specific to C++.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Thanks for prompting me to do this -- the tests 
> > > > > > > > > > > > > caused me to notice and fix a number of bugs.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I haven't managed to diagnose the case when the 
> > > > > > > > > > > > > attribute appears at the beginning of the 
> > > > > > > > > > > > > declaration. It seems to me that, at the point where 
> > > > > > > > > > > > > I've added the check, this case is indistinguishable 
> > > > > > > > > > > > > from an attribute that appears on the type. In both 
> > > > > > > > > > > > > cases, the `TAL` is `TAL_DeclSpec`, and the attribute 
> > > > > > > > > > > > > is contained in `DeclSpec::getAttributes()`. This is 
> > > > > > > > > > > > > because `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I believe if I wanted to prohibit this case, I would 
> > > > > > > > > > > > > need to add a check to 
> > > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and prohibit 
> > > > > > > > > > > > > any occurrences of `annotate_type` there. However, 
> > > > > > > > > > > > > this seems the wrong place to do this because it 
> > > > > > > > > > > > > doesn't contain any specific processing for other 
> > > > > > > > > > > > > attributes.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I've noticed that Clang also doesn't prohibit other 
> > > > > > > > > > > > > type attributes (even ones with C++ 11 syntax) from 
> > > > > > > > > > > > > being applied to declarations. For example: 
> > > > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > > 
> > > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > distinguish cleanly between declaration attributes 
> > > > > > > > > > > > > and type attributes, and fixing this might be 
> > > > > > > > > > > > > nontrivial.
> > > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > > distinguish cleanly between declaration attributes 
> > > > > > > > > > > > > and type attributes, and fixing this might be 
> > > > > > > > > > > > > nontrivial.
> > > > > > > > > > > > 
> > > > > > > > > > > > This is a general issue with attribute processing. I 
> > > > > > > > > > > > would imagine that SemaDeclAttr.cpp should be able to 
> > > > > > > > > > > > diagnose that case when the attribute only applies to 
> > > > > > > > > > > > types and not declarations, but it'd take some 
> > > > > > > > > > > > investigation for me to be sure.
> > > > > > > > > > > > 
> > > > > > > > > > > > Because this issue isn't new to your situation, I'd 
> > > > > > > > > > > > recommend filing an issue about the general problem and 
> > > > > > > > > > > > then we can solve that later.
> > > > > > > > > > > I've done some more investigation myself, and I think 
> > > > > > > > > > > I've come up with a solution; actually, two potential 
> > > > > > > > > > > solutions. I have draft patches for both of these; I 
> > > > > > > > > > > wanted to run these by you here first, so I haven't 
> > > > > > > > > > > opened a bug yet.
> > > > > > > > > > > 
> > > > > > > > > > > I'd appreciate your input on how you'd prefer me to 
> > > > > > > > > > > proceed with this. I do think it makes sense to do this 
> > > > > > > > > > > work now because otherwise, people will start putting 
> > > > > > > > > > > `annotate_type` in places where it doesn't belong, and 
> > > > > > > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

rsmith wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > mboehme wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > Can you also add some test coverage for applying the 
> > > > > > > > > > > > > attribute to a declaration instead of a type or not 
> > > > > > > > > > > > > giving it any arguments? Also, should test arguments 
> > > > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > > > I've added tests as you suggested, though I put most of 
> > > > > > > > > > > > them in Sema/annotate-type.c, as they're not specific 
> > > > > > > > > > > > to C++.
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks for prompting me to do this -- the tests caused 
> > > > > > > > > > > > me to notice and fix a number of bugs.
> > > > > > > > > > > > 
> > > > > > > > > > > > I haven't managed to diagnose the case when the 
> > > > > > > > > > > > attribute appears at the beginning of the declaration. 
> > > > > > > > > > > > It seems to me that, at the point where I've added the 
> > > > > > > > > > > > check, this case is indistinguishable from an attribute 
> > > > > > > > > > > > that appears on the type. In both cases, the `TAL` is 
> > > > > > > > > > > > `TAL_DeclSpec`, and the attribute is contained in 
> > > > > > > > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > > > 
> > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > > 
> > > > > > > > > > > > I believe if I wanted to prohibit this case, I would 
> > > > > > > > > > > > need to add a check to 
> > > > > > > > > > > > `Parser::ParseStatementOrDeclaration` and prohibit any 
> > > > > > > > > > > > occurrences of `annotate_type` there. However, this 
> > > > > > > > > > > > seems the wrong place to do this because it doesn't 
> > > > > > > > > > > > contain any specific processing for other attributes.
> > > > > > > > > > > > 
> > > > > > > > > > > > I've noticed that Clang also doesn't prohibit other 
> > > > > > > > > > > > type attributes (even ones with C++ 11 syntax) from 
> > > > > > > > > > > > being applied to declarations. For example: 
> > > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > > 
> > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > distinguish cleanly between declaration attributes and 
> > > > > > > > > > > > type attributes, and fixing this might be nontrivial.
> > > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > > underlying issue is that Clang doesn't always 
> > > > > > > > > > > > distinguish cleanly between declaration attributes and 
> > > > > > > > > > > > type attributes, and fixing this might be nontrivial.
> > > > > > > > > > > 
> > > > > > > > > > > This is a general issue with attribute processing. I 
> > > > > > > > > > > would imagine that SemaDeclAttr.cpp should be able to 
> > > > > > > > > > > diagnose that case when the attribute only applies to 
> > > > > > > > > > > types and not declarations, but it'd take some 
> > > > > > > > > > > investigation for me to be sure.
> > > > > > > > > > > 
> > > > > > > > > > > Because this issue isn't new to your situation, I'd 
> > > > > > > > > > > recommend filing an issue about the general problem and 
> > > > > > > > > > > then we can solve that later.
> > > > > > > > > > I've done some more investigation myself, and I think I've 
> > > > > > > > > > come up with a solution; actually, two potential solutions. 
> > > > > > > > > > I have draft patches for both of these; I wanted to run 
> > > > > > > > > > these by you here first, so I haven't opened a bug yet.
> > > > > > > > > > 
> > > > > > > > > > I'd appreciate your input on how you'd prefer me to proceed 
> > > > > > > > > > with this. I do think it makes sense to do this work now 
> > > > > > > > > > because otherwise, people will start putting 
> > > > > > > > > > `annotate_type` in places where it doesn't belong, and then 
> > > > > > > > > > we'll have to keep supporting that in the future.
> > > > > > > > > > 
> > > > > > > > > > **My preferred solution**
> > > > > > > > > > 
> > > > > > > > > > https://reviews.llvm.org/D124081
> > > > > > > > > > 
> > > > > > > > > > This adds a function 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > mboehme wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > mboehme wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > Can you also add some test coverage for applying the 
> > > > > > > > > > > > attribute to a declaration instead of a type or not 
> > > > > > > > > > > > giving it any arguments? Also, should test arguments 
> > > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > > I've added tests as you suggested, though I put most of 
> > > > > > > > > > > them in Sema/annotate-type.c, as they're not specific to 
> > > > > > > > > > > C++.
> > > > > > > > > > > 
> > > > > > > > > > > Thanks for prompting me to do this -- the tests caused me 
> > > > > > > > > > > to notice and fix a number of bugs.
> > > > > > > > > > > 
> > > > > > > > > > > I haven't managed to diagnose the case when the attribute 
> > > > > > > > > > > appears at the beginning of the declaration. It seems to 
> > > > > > > > > > > me that, at the point where I've added the check, this 
> > > > > > > > > > > case is indistinguishable from an attribute that appears 
> > > > > > > > > > > on the type. In both cases, the `TAL` is `TAL_DeclSpec`, 
> > > > > > > > > > > and the attribute is contained in 
> > > > > > > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > > > > > > > > > attributes to the `DeclSpec`:
> > > > > > > > > > > 
> > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > > 
> > > > > > > > > > > I believe if I wanted to prohibit this case, I would need 
> > > > > > > > > > > to add a check to `Parser::ParseStatementOrDeclaration` 
> > > > > > > > > > > and prohibit any occurrences of `annotate_type` there. 
> > > > > > > > > > > However, this seems the wrong place to do this because it 
> > > > > > > > > > > doesn't contain any specific processing for other 
> > > > > > > > > > > attributes.
> > > > > > > > > > > 
> > > > > > > > > > > I've noticed that Clang also doesn't prohibit other type 
> > > > > > > > > > > attributes (even ones with C++ 11 syntax) from being 
> > > > > > > > > > > applied to declarations. For example: 
> > > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > > 
> > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > > > cleanly between declaration attributes and type 
> > > > > > > > > > > attributes, and fixing this might be nontrivial.
> > > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > > > cleanly between declaration attributes and type 
> > > > > > > > > > > attributes, and fixing this might be nontrivial.
> > > > > > > > > > 
> > > > > > > > > > This is a general issue with attribute processing. I would 
> > > > > > > > > > imagine that SemaDeclAttr.cpp should be able to diagnose 
> > > > > > > > > > that case when the attribute only applies to types and not 
> > > > > > > > > > declarations, but it'd take some investigation for me to be 
> > > > > > > > > > sure.
> > > > > > > > > > 
> > > > > > > > > > Because this issue isn't new to your situation, I'd 
> > > > > > > > > > recommend filing an issue about the general problem and 
> > > > > > > > > > then we can solve that later.
> > > > > > > > > I've done some more investigation myself, and I think I've 
> > > > > > > > > come up with a solution; actually, two potential solutions. I 
> > > > > > > > > have draft patches for both of these; I wanted to run these 
> > > > > > > > > by you here first, so I haven't opened a bug yet.
> > > > > > > > > 
> > > > > > > > > I'd appreciate your input on how you'd prefer me to proceed 
> > > > > > > > > with this. I do think it makes sense to do this work now 
> > > > > > > > > because otherwise, people will start putting `annotate_type` 
> > > > > > > > > in places where it doesn't belong, and then we'll have to 
> > > > > > > > > keep supporting that in the future.
> > > > > > > > > 
> > > > > > > > > **My preferred solution**
> > > > > > > > > 
> > > > > > > > > https://reviews.llvm.org/D124081
> > > > > > > > > 
> > > > > > > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls 
> > > > > > > > > it in all places where we should reject attributes that can't 
> > > > > > > > > be put on declarations. (As part of this 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 428015.
mboehme added a comment.

Move some tests here that I originally added to 
https://reviews.llvm.org/D124919.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+AssertAnnotatedAs(Func->getParamDecl(1)->getTypeSourceInfo()->getTypeLoc(),
+  "bar", 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D111548#3483326 , @xbolva00 wrote:

>>> The intent is to allow adding arbitrary annotations to types for use in 
>>> static analysis tools
>
> This patch should not land until we see some real use cases to justify new 
> hundreds of lines of code. We should more careful and take maintenance cost 
> really into account and not merge every experimental cool research extensions 
> automatically.
>
> Please answer points in “ Contributing Extensions to Clang “
> https://clang.llvm.org/get_involved.html

Sorry, I had missed this. The justification is from the original RFC on adding 
lifetime annotation checking to the clang static analyzer 
(https://discourse.llvm.org/t/rfc-new-attribute-annotate-type-iteration-2). I 
think this code has utility as a general-purpose mechanism in the same manner 
that the existing declaration annotation attribute does. As attribute code 
owner, I think this meets the bar for a contribution as an extension.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D111548#3483326 , @xbolva00 wrote:

> Please answer points in “ Contributing Extensions to Clang “
> https://clang.llvm.org/get_involved.html

I've added these to the RFC:

https://discourse.llvm.org/t/rfc-new-attribute-annotate-type-iteration-2/61378/9?u=martinboehme


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > mboehme wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > Can you also add some test coverage for applying the 
> > > > > > > > > > > attribute to a declaration instead of a type or not 
> > > > > > > > > > > giving it any arguments? Also, should test arguments 
> > > > > > > > > > > which are not a constant expression.
> > > > > > > > > > I've added tests as you suggested, though I put most of 
> > > > > > > > > > them in Sema/annotate-type.c, as they're not specific to 
> > > > > > > > > > C++.
> > > > > > > > > > 
> > > > > > > > > > Thanks for prompting me to do this -- the tests caused me 
> > > > > > > > > > to notice and fix a number of bugs.
> > > > > > > > > > 
> > > > > > > > > > I haven't managed to diagnose the case when the attribute 
> > > > > > > > > > appears at the beginning of the declaration. It seems to me 
> > > > > > > > > > that, at the point where I've added the check, this case is 
> > > > > > > > > > indistinguishable from an attribute that appears on the 
> > > > > > > > > > type. In both cases, the `TAL` is `TAL_DeclSpec`, and the 
> > > > > > > > > > attribute is contained in `DeclSpec::getAttributes()`. This 
> > > > > > > > > > is because `Parser::ParseSimpleDeclaration` forwards the 
> > > > > > > > > > declaration attributes to the `DeclSpec`:
> > > > > > > > > > 
> > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > > 
> > > > > > > > > > I believe if I wanted to prohibit this case, I would need 
> > > > > > > > > > to add a check to `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > > prohibit any occurrences of `annotate_type` there. However, 
> > > > > > > > > > this seems the wrong place to do this because it doesn't 
> > > > > > > > > > contain any specific processing for other attributes.
> > > > > > > > > > 
> > > > > > > > > > I've noticed that Clang also doesn't prohibit other type 
> > > > > > > > > > attributes (even ones with C++ 11 syntax) from being 
> > > > > > > > > > applied to declarations. For example: 
> > > > > > > > > > https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > > 
> > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > > cleanly between declaration attributes and type attributes, 
> > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > > cleanly between declaration attributes and type attributes, 
> > > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > 
> > > > > > > > > This is a general issue with attribute processing. I would 
> > > > > > > > > imagine that SemaDeclAttr.cpp should be able to diagnose that 
> > > > > > > > > case when the attribute only applies to types and not 
> > > > > > > > > declarations, but it'd take some investigation for me to be 
> > > > > > > > > sure.
> > > > > > > > > 
> > > > > > > > > Because this issue isn't new to your situation, I'd recommend 
> > > > > > > > > filing an issue about the general problem and then we can 
> > > > > > > > > solve that later.
> > > > > > > > I've done some more investigation myself, and I think I've come 
> > > > > > > > up with a solution; actually, two potential solutions. I have 
> > > > > > > > draft patches for both of these; I wanted to run these by you 
> > > > > > > > here first, so I haven't opened a bug yet.
> > > > > > > > 
> > > > > > > > I'd appreciate your input on how you'd prefer me to proceed 
> > > > > > > > with this. I do think it makes sense to do this work now 
> > > > > > > > because otherwise, people will start putting `annotate_type` in 
> > > > > > > > places where it doesn't belong, and then we'll have to keep 
> > > > > > > > supporting that in the future.
> > > > > > > > 
> > > > > > > > **My preferred solution**
> > > > > > > > 
> > > > > > > > https://reviews.llvm.org/D124081
> > > > > > > > 
> > > > > > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it 
> > > > > > > > in all places where we should reject attributes that can't be 
> > > > > > > > put on declarations. (As part of this work, I noticed that 
> > > > > > > > `gsl::suppress` should be a `DeclOrStmtAttr`, not just a 
> > > > > > > > `StmtAttr`.)
> > > > > > > > 
> > > > > > > > Advantages:
> > > > > > > > - Not very invasive.
> > > > > > > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: rjmccall.
aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > mboehme wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Can you also add some test coverage for applying the 
> > > > > > > > > > attribute to a declaration instead of a type or not giving 
> > > > > > > > > > it any arguments? Also, should test arguments which are not 
> > > > > > > > > > a constant expression.
> > > > > > > > > I've added tests as you suggested, though I put most of them 
> > > > > > > > > in Sema/annotate-type.c, as they're not specific to C++.
> > > > > > > > > 
> > > > > > > > > Thanks for prompting me to do this -- the tests caused me to 
> > > > > > > > > notice and fix a number of bugs.
> > > > > > > > > 
> > > > > > > > > I haven't managed to diagnose the case when the attribute 
> > > > > > > > > appears at the beginning of the declaration. It seems to me 
> > > > > > > > > that, at the point where I've added the check, this case is 
> > > > > > > > > indistinguishable from an attribute that appears on the type. 
> > > > > > > > > In both cases, the `TAL` is `TAL_DeclSpec`, and the attribute 
> > > > > > > > > is contained in `DeclSpec::getAttributes()`. This is because 
> > > > > > > > > `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > > > > > > > attributes to the `DeclSpec`:
> > > > > > > > > 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > > 
> > > > > > > > > I believe if I wanted to prohibit this case, I would need to 
> > > > > > > > > add a check to `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > > prohibit any occurrences of `annotate_type` there. However, 
> > > > > > > > > this seems the wrong place to do this because it doesn't 
> > > > > > > > > contain any specific processing for other attributes.
> > > > > > > > > 
> > > > > > > > > I've noticed that Clang also doesn't prohibit other type 
> > > > > > > > > attributes (even ones with C++ 11 syntax) from being applied 
> > > > > > > > > to declarations. For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > > 
> > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > cleanly between declaration attributes and type attributes, 
> > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > > How do you think I should proceed here? I think the 
> > > > > > > > > underlying issue is that Clang doesn't always distinguish 
> > > > > > > > > cleanly between declaration attributes and type attributes, 
> > > > > > > > > and fixing this might be nontrivial.
> > > > > > > > 
> > > > > > > > This is a general issue with attribute processing. I would 
> > > > > > > > imagine that SemaDeclAttr.cpp should be able to diagnose that 
> > > > > > > > case when the attribute only applies to types and not 
> > > > > > > > declarations, but it'd take some investigation for me to be 
> > > > > > > > sure.
> > > > > > > > 
> > > > > > > > Because this issue isn't new to your situation, I'd recommend 
> > > > > > > > filing an issue about the general problem and then we can solve 
> > > > > > > > that later.
> > > > > > > I've done some more investigation myself, and I think I've come 
> > > > > > > up with a solution; actually, two potential solutions. I have 
> > > > > > > draft patches for both of these; I wanted to run these by you 
> > > > > > > here first, so I haven't opened a bug yet.
> > > > > > > 
> > > > > > > I'd appreciate your input on how you'd prefer me to proceed with 
> > > > > > > this. I do think it makes sense to do this work now because 
> > > > > > > otherwise, people will start putting `annotate_type` in places 
> > > > > > > where it doesn't belong, and then we'll have to keep supporting 
> > > > > > > that in the future.
> > > > > > > 
> > > > > > > **My preferred solution**
> > > > > > > 
> > > > > > > https://reviews.llvm.org/D124081
> > > > > > > 
> > > > > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it 
> > > > > > > in all places where we should reject attributes that can't be put 
> > > > > > > on declarations. (As part of this work, I noticed that 
> > > > > > > `gsl::suppress` should be a `DeclOrStmtAttr`, not just a 
> > > > > > > `StmtAttr`.)
> > > > > > > 
> > > > > > > Advantages:
> > > > > > > - Not very invasive.
> > > > > > > Disadvantages:
> > > > > > > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` 
> > > > > > > everywhere that non-declaration attributes should be rejected. 
> > > > > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked an inline comment as done.
mboehme added a comment.

In D111548#3483326 , @xbolva00 wrote:

> This patch should not land until we see some real use cases to justify new 
> hundreds of lines of code. We should more careful and take maintenance cost 
> really into account and not merge every experimental cool research extensions 
> automatically.

I assume you've seen the RFC 
 for the 
primary use case (Rust-style lifetime annotations for C++) that motivates this 
change?

There's a bit of a chicken-and-egg problem here: We obviously can't submit code 
for the lifetime analysis tooling before the type annotations it needs are 
supported. We're conscious that it's not desirable to add features merely for 
the purpose of an experimental tool that may never be stabilized. This is why 
we've consciously steered away from adding attributes that are specific to our 
use case and are instead proposing a general-purpose type annotation attribute.

> Please answer points in “ Contributing Extensions to Clang “
> https://clang.llvm.org/get_involved.html

This makes sense. I'll add an appendix answering these points to the RFC for 
this change 
.
 I probably won't get round to doing this today, but I did want to let you know 
that I've seen your comment. I'll ping this comment again when I've added the 
appendix to the RFC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 3 inline comments as done.
mboehme added a comment.

In D111548#3483275 , @erichkeane 
wrote:

> I don't really know how useful this ends up being, these attributes (since 
> they are part of `AttributedType` end up disappearing pretty quickly/easily.  
> Anything that causes canonicalization will cause these to go away,

This is true, but I also think we don't have much choice in the matter if we 
don't want these attributes to affect the C++ type system. We discuss this at 
length in the RFC:

https://discourse.llvm.org/t/rfc-new-attribute-annotate-type-iteration-2/61378#rationale-5

I go into more depth in this comment:

https://discourse.llvm.org/t/rfc-new-attribute-annotate-type-iteration-2/61378/4?u=martinboehme

However, it is possible for a static analysis to propagate the annotations 
through typedefs, template arguments and such, and we do so in the lifetime 
static analysis (see also below).

> they don't apply to references to these types, etc.

Not sure what you mean here -- do you mean that the annotations don't propagate 
through typedefs? As noted, it's possible for a static analysis tool to perform 
this propagation, and the discussion here is again relevant:

https://discourse.llvm.org/t/rfc-new-attribute-annotate-type-iteration-2/61378/4?u=martinboehme

> But I otherwise don't have concerns.
>
> HOWEVER, have you implemented the lifetime static analysis that you're 
> wanting to do on top of this already

Yes, we have. We are able to propagate lifetime annotations through non-trivial 
C++ constructs, including template arguments. Based on what our current 
prototype can do, we're confident these annotations will do everything we need 
them to. We also believe they are general enough to be useful for other types 
of static analysis.




Comment at: clang/lib/AST/TypePrinter.cpp:1689
+// the type.
+OS << " [[clang::annotate_type]]";
+return;

erichkeane wrote:
> My opinion (though not attached to it) would be `clang::annotate_type(...)` 
> or, `clang::annotate_type()` or something like that.
Good idea -- this makes it more obvious that the arguments were omitted. 
Changed!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 2 inline comments as done.
mboehme added inline comments.



Comment at: clang/unittests/AST/AttrTest.cpp:89
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+)cpp");
+

aaron.ballman wrote:
> The formatting looks a bit off for this in terms of indentation.
Fixed.



Comment at: clang/unittests/AST/AttrTest.cpp:159
+__auto_type [[clang::annotate_type("auto")]] auto_var = 1;
+)c",
+ {"-fdouble-square-bracket-attributes"},

aaron.ballman wrote:
> Formatting looks a bit off here in terms of the indentation for this.
Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 426990.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+  )cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > mboehme wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Can you also add some test coverage for applying the 
> > > > > > > > > attribute to a declaration instead of a type or not giving it 
> > > > > > > > > any arguments? Also, should test arguments which are not a 
> > > > > > > > > constant expression.
> > > > > > > > I've added tests as you suggested, though I put most of them in 
> > > > > > > > Sema/annotate-type.c, as they're not specific to C++.
> > > > > > > > 
> > > > > > > > Thanks for prompting me to do this -- the tests caused me to 
> > > > > > > > notice and fix a number of bugs.
> > > > > > > > 
> > > > > > > > I haven't managed to diagnose the case when the attribute 
> > > > > > > > appears at the beginning of the declaration. It seems to me 
> > > > > > > > that, at the point where I've added the check, this case is 
> > > > > > > > indistinguishable from an attribute that appears on the type. 
> > > > > > > > In both cases, the `TAL` is `TAL_DeclSpec`, and the attribute 
> > > > > > > > is contained in `DeclSpec::getAttributes()`. This is because 
> > > > > > > > `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > > > > > > attributes to the `DeclSpec`:
> > > > > > > > 
> > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > > 
> > > > > > > > I believe if I wanted to prohibit this case, I would need to 
> > > > > > > > add a check to `Parser::ParseStatementOrDeclaration` and 
> > > > > > > > prohibit any occurrences of `annotate_type` there. However, 
> > > > > > > > this seems the wrong place to do this because it doesn't 
> > > > > > > > contain any specific processing for other attributes.
> > > > > > > > 
> > > > > > > > I've noticed that Clang also doesn't prohibit other type 
> > > > > > > > attributes (even ones with C++ 11 syntax) from being applied to 
> > > > > > > > declarations. For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > > 
> > > > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > > > declaration attributes and type attributes, and fixing this 
> > > > > > > > might be nontrivial.
> > > > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > > > declaration attributes and type attributes, and fixing this 
> > > > > > > > might be nontrivial.
> > > > > > > 
> > > > > > > This is a general issue with attribute processing. I would 
> > > > > > > imagine that SemaDeclAttr.cpp should be able to diagnose that 
> > > > > > > case when the attribute only applies to types and not 
> > > > > > > declarations, but it'd take some investigation for me to be sure.
> > > > > > > 
> > > > > > > Because this issue isn't new to your situation, I'd recommend 
> > > > > > > filing an issue about the general problem and then we can solve 
> > > > > > > that later.
> > > > > > I've done some more investigation myself, and I think I've come up 
> > > > > > with a solution; actually, two potential solutions. I have draft 
> > > > > > patches for both of these; I wanted to run these by you here first, 
> > > > > > so I haven't opened a bug yet.
> > > > > > 
> > > > > > I'd appreciate your input on how you'd prefer me to proceed with 
> > > > > > this. I do think it makes sense to do this work now because 
> > > > > > otherwise, people will start putting `annotate_type` in places 
> > > > > > where it doesn't belong, and then we'll have to keep supporting 
> > > > > > that in the future.
> > > > > > 
> > > > > > **My preferred solution**
> > > > > > 
> > > > > > https://reviews.llvm.org/D124081
> > > > > > 
> > > > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in 
> > > > > > all places where we should reject attributes that can't be put on 
> > > > > > declarations. (As part of this work, I noticed that `gsl::suppress` 
> > > > > > should be a `DeclOrStmtAttr`, not just a `StmtAttr`.)
> > > > > > 
> > > > > > Advantages:
> > > > > > - Not very invasive.
> > > > > > Disadvantages:
> > > > > > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` 
> > > > > > everywhere that non-declaration attributes should be rejected. (Not 
> > > > > > sure if I've identified all of those places yet?)
> > > > > > 
> > > > > > **Alternative solution**
> > > > > > 
> > > > > > https://reviews.llvm.org/D124083
> > > > > > 
> > > > > > This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` 
> > 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 426977.
mboehme added a comment.

Rebased to a more recent base change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+)cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> The intent is to allow adding arbitrary annotations to types for use in 
>> static analysis tools

This patch should not land until we see some real use cases to justify new 
hundreds of lines of code. We should more careful and take maintanence cost 
really into account and not merge every experimental cool research extensions 
automatically.

Please answer points in “ Contributing Extensions to Clang “
https://clang.llvm.org/get_involved.html


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't really know how useful this ends up being, these attributes (since they 
are part of `AttributedType` end up disappearing pretty quickly/easily.  
Anything that causes canonicalization will cause these to go away, they don't 
apply to references to these types, etc.  But I otherwise don't have concerns.

HOWEVER, have you implemented the lifetime static analysis that you're wanting 
to do on top of this already? If you haven't, you might just find that your 
implementation/idea here won't work, and that you've already committed 
sufficiently to a design that doesn't work for it.




Comment at: clang/lib/AST/TypePrinter.cpp:1689
+// the type.
+OS << " [[clang::annotate_type]]";
+return;

My opinion (though not attached to it) would be `clang::annotate_type(...)` or, 
`clang::annotate_type()` or something like that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added subscribers: rsmith, erichkeane.
aaron.ballman added a comment.

Adding in @erichkeane just in case he spots anything I've missed.




Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > mboehme wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Can you also add some test coverage for applying the attribute 
> > > > > > > > to a declaration instead of a type or not giving it any 
> > > > > > > > arguments? Also, should test arguments which are not a constant 
> > > > > > > > expression.
> > > > > > > I've added tests as you suggested, though I put most of them in 
> > > > > > > Sema/annotate-type.c, as they're not specific to C++.
> > > > > > > 
> > > > > > > Thanks for prompting me to do this -- the tests caused me to 
> > > > > > > notice and fix a number of bugs.
> > > > > > > 
> > > > > > > I haven't managed to diagnose the case when the attribute appears 
> > > > > > > at the beginning of the declaration. It seems to me that, at the 
> > > > > > > point where I've added the check, this case is indistinguishable 
> > > > > > > from an attribute that appears on the type. In both cases, the 
> > > > > > > `TAL` is `TAL_DeclSpec`, and the attribute is contained in 
> > > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > > `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > > > > > attributes to the `DeclSpec`:
> > > > > > > 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > > 
> > > > > > > I believe if I wanted to prohibit this case, I would need to add 
> > > > > > > a check to `Parser::ParseStatementOrDeclaration` and prohibit any 
> > > > > > > occurrences of `annotate_type` there. However, this seems the 
> > > > > > > wrong place to do this because it doesn't contain any specific 
> > > > > > > processing for other attributes.
> > > > > > > 
> > > > > > > I've noticed that Clang also doesn't prohibit other type 
> > > > > > > attributes (even ones with C++ 11 syntax) from being applied to 
> > > > > > > declarations. For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > > 
> > > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > > declaration attributes and type attributes, and fixing this might 
> > > > > > > be nontrivial.
> > > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > > declaration attributes and type attributes, and fixing this might 
> > > > > > > be nontrivial.
> > > > > > 
> > > > > > This is a general issue with attribute processing. I would imagine 
> > > > > > that SemaDeclAttr.cpp should be able to diagnose that case when the 
> > > > > > attribute only applies to types and not declarations, but it'd take 
> > > > > > some investigation for me to be sure.
> > > > > > 
> > > > > > Because this issue isn't new to your situation, I'd recommend 
> > > > > > filing an issue about the general problem and then we can solve 
> > > > > > that later.
> > > > > I've done some more investigation myself, and I think I've come up 
> > > > > with a solution; actually, two potential solutions. I have draft 
> > > > > patches for both of these; I wanted to run these by you here first, 
> > > > > so I haven't opened a bug yet.
> > > > > 
> > > > > I'd appreciate your input on how you'd prefer me to proceed with 
> > > > > this. I do think it makes sense to do this work now because 
> > > > > otherwise, people will start putting `annotate_type` in places where 
> > > > > it doesn't belong, and then we'll have to keep supporting that in the 
> > > > > future.
> > > > > 
> > > > > **My preferred solution**
> > > > > 
> > > > > https://reviews.llvm.org/D124081
> > > > > 
> > > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in 
> > > > > all places where we should reject attributes that can't be put on 
> > > > > declarations. (As part of this work, I noticed that `gsl::suppress` 
> > > > > should be a `DeclOrStmtAttr`, not just a `StmtAttr`.)
> > > > > 
> > > > > Advantages:
> > > > > - Not very invasive.
> > > > > Disadvantages:
> > > > > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` everywhere 
> > > > > that non-declaration attributes should be rejected. (Not sure if I've 
> > > > > identified all of those places yet?)
> > > > > 
> > > > > **Alternative solution**
> > > > > 
> > > > > https://reviews.llvm.org/D124083
> > > > > 
> > > > > This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` and 
> > > > > other "parse 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > mboehme wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Can you also add some test coverage for applying the attribute to 
> > > > > > > a declaration instead of a type or not giving it any arguments? 
> > > > > > > Also, should test arguments which are not a constant expression.
> > > > > > I've added tests as you suggested, though I put most of them in 
> > > > > > Sema/annotate-type.c, as they're not specific to C++.
> > > > > > 
> > > > > > Thanks for prompting me to do this -- the tests caused me to notice 
> > > > > > and fix a number of bugs.
> > > > > > 
> > > > > > I haven't managed to diagnose the case when the attribute appears 
> > > > > > at the beginning of the declaration. It seems to me that, at the 
> > > > > > point where I've added the check, this case is indistinguishable 
> > > > > > from an attribute that appears on the type. In both cases, the 
> > > > > > `TAL` is `TAL_DeclSpec`, and the attribute is contained in 
> > > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > > `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > > > > attributes to the `DeclSpec`:
> > > > > > 
> > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > > 
> > > > > > I believe if I wanted to prohibit this case, I would need to add a 
> > > > > > check to `Parser::ParseStatementOrDeclaration` and prohibit any 
> > > > > > occurrences of `annotate_type` there. However, this seems the wrong 
> > > > > > place to do this because it doesn't contain any specific processing 
> > > > > > for other attributes.
> > > > > > 
> > > > > > I've noticed that Clang also doesn't prohibit other type attributes 
> > > > > > (even ones with C++ 11 syntax) from being applied to declarations. 
> > > > > > For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > > 
> > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > declaration attributes and type attributes, and fixing this might 
> > > > > > be nontrivial.
> > > > > > How do you think I should proceed here? I think the underlying 
> > > > > > issue is that Clang doesn't always distinguish cleanly between 
> > > > > > declaration attributes and type attributes, and fixing this might 
> > > > > > be nontrivial.
> > > > > 
> > > > > This is a general issue with attribute processing. I would imagine 
> > > > > that SemaDeclAttr.cpp should be able to diagnose that case when the 
> > > > > attribute only applies to types and not declarations, but it'd take 
> > > > > some investigation for me to be sure.
> > > > > 
> > > > > Because this issue isn't new to your situation, I'd recommend filing 
> > > > > an issue about the general problem and then we can solve that later.
> > > > I've done some more investigation myself, and I think I've come up with 
> > > > a solution; actually, two potential solutions. I have draft patches for 
> > > > both of these; I wanted to run these by you here first, so I haven't 
> > > > opened a bug yet.
> > > > 
> > > > I'd appreciate your input on how you'd prefer me to proceed with this. 
> > > > I do think it makes sense to do this work now because otherwise, people 
> > > > will start putting `annotate_type` in places where it doesn't belong, 
> > > > and then we'll have to keep supporting that in the future.
> > > > 
> > > > **My preferred solution**
> > > > 
> > > > https://reviews.llvm.org/D124081
> > > > 
> > > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in all 
> > > > places where we should reject attributes that can't be put on 
> > > > declarations. (As part of this work, I noticed that `gsl::suppress` 
> > > > should be a `DeclOrStmtAttr`, not just a `StmtAttr`.)
> > > > 
> > > > Advantages:
> > > > - Not very invasive.
> > > > Disadvantages:
> > > > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` everywhere 
> > > > that non-declaration attributes should be rejected. (Not sure if I've 
> > > > identified all of those places yet?)
> > > > 
> > > > **Alternative solution**
> > > > 
> > > > https://reviews.llvm.org/D124083
> > > > 
> > > > This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` and 
> > > > other "parse attributes" functions that call it. This parameter 
> > > > specifies whether the attributes in the place where they're currently 
> > > > being parsed appertain to a declaration, statement or type. If 
> > > > `ParseCXX11Attributes()` encounters an attribute that is not compatible 
> > > > with `appertainsTo`, it outputs a diagnostic.
> > > > 
> > > > Advantages:
> > > > - Every call to 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > mboehme wrote:
> > > > > aaron.ballman wrote:
> > > > > > Can you also add some test coverage for applying the attribute to a 
> > > > > > declaration instead of a type or not giving it any arguments? Also, 
> > > > > > should test arguments which are not a constant expression.
> > > > > I've added tests as you suggested, though I put most of them in 
> > > > > Sema/annotate-type.c, as they're not specific to C++.
> > > > > 
> > > > > Thanks for prompting me to do this -- the tests caused me to notice 
> > > > > and fix a number of bugs.
> > > > > 
> > > > > I haven't managed to diagnose the case when the attribute appears at 
> > > > > the beginning of the declaration. It seems to me that, at the point 
> > > > > where I've added the check, this case is indistinguishable from an 
> > > > > attribute that appears on the type. In both cases, the `TAL` is 
> > > > > `TAL_DeclSpec`, and the attribute is contained in 
> > > > > `DeclSpec::getAttributes()`. This is because 
> > > > > `Parser::ParseSimpleDeclaration` forwards the declaration attributes 
> > > > > to the `DeclSpec`:
> > > > > 
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > > 
> > > > > I believe if I wanted to prohibit this case, I would need to add a 
> > > > > check to `Parser::ParseStatementOrDeclaration` and prohibit any 
> > > > > occurrences of `annotate_type` there. However, this seems the wrong 
> > > > > place to do this because it doesn't contain any specific processing 
> > > > > for other attributes.
> > > > > 
> > > > > I've noticed that Clang also doesn't prohibit other type attributes 
> > > > > (even ones with C++ 11 syntax) from being applied to declarations. 
> > > > > For example: https://godbolt.org/z/Yj1zWY7nn
> > > > > 
> > > > > How do you think I should proceed here? I think the underlying issue 
> > > > > is that Clang doesn't always distinguish cleanly between declaration 
> > > > > attributes and type attributes, and fixing this might be nontrivial.
> > > > > How do you think I should proceed here? I think the underlying issue 
> > > > > is that Clang doesn't always distinguish cleanly between declaration 
> > > > > attributes and type attributes, and fixing this might be nontrivial.
> > > > 
> > > > This is a general issue with attribute processing. I would imagine that 
> > > > SemaDeclAttr.cpp should be able to diagnose that case when the 
> > > > attribute only applies to types and not declarations, but it'd take 
> > > > some investigation for me to be sure.
> > > > 
> > > > Because this issue isn't new to your situation, I'd recommend filing an 
> > > > issue about the general problem and then we can solve that later.
> > > I've done some more investigation myself, and I think I've come up with a 
> > > solution; actually, two potential solutions. I have draft patches for 
> > > both of these; I wanted to run these by you here first, so I haven't 
> > > opened a bug yet.
> > > 
> > > I'd appreciate your input on how you'd prefer me to proceed with this. I 
> > > do think it makes sense to do this work now because otherwise, people 
> > > will start putting `annotate_type` in places where it doesn't belong, and 
> > > then we'll have to keep supporting that in the future.
> > > 
> > > **My preferred solution**
> > > 
> > > https://reviews.llvm.org/D124081
> > > 
> > > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in all 
> > > places where we should reject attributes that can't be put on 
> > > declarations. (As part of this work, I noticed that `gsl::suppress` 
> > > should be a `DeclOrStmtAttr`, not just a `StmtAttr`.)
> > > 
> > > Advantages:
> > > - Not very invasive.
> > > Disadvantages:
> > > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` everywhere that 
> > > non-declaration attributes should be rejected. (Not sure if I've 
> > > identified all of those places yet?)
> > > 
> > > **Alternative solution**
> > > 
> > > https://reviews.llvm.org/D124083
> > > 
> > > This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` and 
> > > other "parse attributes" functions that call it. This parameter specifies 
> > > whether the attributes in the place where they're currently being parsed 
> > > appertain to a declaration, statement or type. If 
> > > `ParseCXX11Attributes()` encounters an attribute that is not compatible 
> > > with `appertainsTo`, it outputs a diagnostic.
> > > 
> > > Advantages:
> > > - Every call to `ParseCXX11Attributes()` _has_ to specify `appertainsTo` 
> > > -- so there's no risk of missing some case where we have to output 
> > > diagnostics
> > > Disadvantages:
> > > - This change is _much_ more invasive.
> > > - It's 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 5 inline comments as done.
mboehme added inline comments.



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > aaron.ballman wrote:
> > > > > Can you also add some test coverage for applying the attribute to a 
> > > > > declaration instead of a type or not giving it any arguments? Also, 
> > > > > should test arguments which are not a constant expression.
> > > > I've added tests as you suggested, though I put most of them in 
> > > > Sema/annotate-type.c, as they're not specific to C++.
> > > > 
> > > > Thanks for prompting me to do this -- the tests caused me to notice and 
> > > > fix a number of bugs.
> > > > 
> > > > I haven't managed to diagnose the case when the attribute appears at 
> > > > the beginning of the declaration. It seems to me that, at the point 
> > > > where I've added the check, this case is indistinguishable from an 
> > > > attribute that appears on the type. In both cases, the `TAL` is 
> > > > `TAL_DeclSpec`, and the attribute is contained in 
> > > > `DeclSpec::getAttributes()`. This is because 
> > > > `Parser::ParseSimpleDeclaration` forwards the declaration attributes to 
> > > > the `DeclSpec`:
> > > > 
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > > 
> > > > I believe if I wanted to prohibit this case, I would need to add a 
> > > > check to `Parser::ParseStatementOrDeclaration` and prohibit any 
> > > > occurrences of `annotate_type` there. However, this seems the wrong 
> > > > place to do this because it doesn't contain any specific processing for 
> > > > other attributes.
> > > > 
> > > > I've noticed that Clang also doesn't prohibit other type attributes 
> > > > (even ones with C++ 11 syntax) from being applied to declarations. For 
> > > > example: https://godbolt.org/z/Yj1zWY7nn
> > > > 
> > > > How do you think I should proceed here? I think the underlying issue is 
> > > > that Clang doesn't always distinguish cleanly between declaration 
> > > > attributes and type attributes, and fixing this might be nontrivial.
> > > > How do you think I should proceed here? I think the underlying issue is 
> > > > that Clang doesn't always distinguish cleanly between declaration 
> > > > attributes and type attributes, and fixing this might be nontrivial.
> > > 
> > > This is a general issue with attribute processing. I would imagine that 
> > > SemaDeclAttr.cpp should be able to diagnose that case when the attribute 
> > > only applies to types and not declarations, but it'd take some 
> > > investigation for me to be sure.
> > > 
> > > Because this issue isn't new to your situation, I'd recommend filing an 
> > > issue about the general problem and then we can solve that later.
> > I've done some more investigation myself, and I think I've come up with a 
> > solution; actually, two potential solutions. I have draft patches for both 
> > of these; I wanted to run these by you here first, so I haven't opened a 
> > bug yet.
> > 
> > I'd appreciate your input on how you'd prefer me to proceed with this. I do 
> > think it makes sense to do this work now because otherwise, people will 
> > start putting `annotate_type` in places where it doesn't belong, and then 
> > we'll have to keep supporting that in the future.
> > 
> > **My preferred solution**
> > 
> > https://reviews.llvm.org/D124081
> > 
> > This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in all 
> > places where we should reject attributes that can't be put on declarations. 
> > (As part of this work, I noticed that `gsl::suppress` should be a 
> > `DeclOrStmtAttr`, not just a `StmtAttr`.)
> > 
> > Advantages:
> > - Not very invasive.
> > Disadvantages:
> > - Need to make sure we call `DiagnoseCXX11NonDeclAttrs()` everywhere that 
> > non-declaration attributes should be rejected. (Not sure if I've identified 
> > all of those places yet?)
> > 
> > **Alternative solution**
> > 
> > https://reviews.llvm.org/D124083
> > 
> > This adds an `appertainsTo` parameter to `ParseCXX11Attributes()` and other 
> > "parse attributes" functions that call it. This parameter specifies whether 
> > the attributes in the place where they're currently being parsed appertain 
> > to a declaration, statement or type. If `ParseCXX11Attributes()` encounters 
> > an attribute that is not compatible with `appertainsTo`, it outputs a 
> > diagnostic.
> > 
> > Advantages:
> > - Every call to `ParseCXX11Attributes()` _has_ to specify `appertainsTo` -- 
> > so there's no risk of missing some case where we have to output diagnostics
> > Disadvantages:
> > - This change is _much_ more invasive.
> > - It's not always clear what value to specify for `appertainsTo`. The 
> > poster child for this is `Parser::ParseStatementOrDeclaration()`: As the 
> > name says, we don't yet 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:3184
+continue;
+  // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though 
they
+  // are type attributes, because we historically haven't allowed these

mboehme wrote:
> aaron.ballman wrote:
> > I think this is a FIXME situation -- our policy is to diagnose attributes 
> > that are being ignored, and it seems pretty important to (someday, not part 
> > of this patch) diagnose these. Especially the lifetime bound attribute 
> > given that it's intended to help harden code.
> Maybe I should clarify what's happening here: We _do_ produce diagnostics 
> (errors) for AT_LifetimeBound and AT_AnyX86NoCfCheck, both before and after 
> this patch. (For these attributes, the code falls through to the 
> `Diag(PA.getLoc(), diag::err_attribute_not_type_attr)` call below.)
> 
> Before this patch, we would reject (produce errors for) _all_ C++11 
> attributes here. Now, we only reject non-type attributes, and in addition, we 
> also reject AT_LifetimeBound and AT_AnyX86NoCfCheck (even though they are 
> type attributes) because we historically haven't allowed them to be used in 
> this way. There are tests for this behavior, so it seemed important to 
> preserve it.
Oh derp, that's my mistake, I read the `!=` logic wrong. Thank you for the 
clarification!



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > Can you also add some test coverage for applying the attribute to a 
> > > > declaration instead of a type or not giving it any arguments? Also, 
> > > > should test arguments which are not a constant expression.
> > > I've added tests as you suggested, though I put most of them in 
> > > Sema/annotate-type.c, as they're not specific to C++.
> > > 
> > > Thanks for prompting me to do this -- the tests caused me to notice and 
> > > fix a number of bugs.
> > > 
> > > I haven't managed to diagnose the case when the attribute appears at the 
> > > beginning of the declaration. It seems to me that, at the point where 
> > > I've added the check, this case is indistinguishable from an attribute 
> > > that appears on the type. In both cases, the `TAL` is `TAL_DeclSpec`, and 
> > > the attribute is contained in `DeclSpec::getAttributes()`. This is 
> > > because `Parser::ParseSimpleDeclaration` forwards the declaration 
> > > attributes to the `DeclSpec`:
> > > 
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L1851
> > > 
> > > I believe if I wanted to prohibit this case, I would need to add a check 
> > > to `Parser::ParseStatementOrDeclaration` and prohibit any occurrences of 
> > > `annotate_type` there. However, this seems the wrong place to do this 
> > > because it doesn't contain any specific processing for other attributes.
> > > 
> > > I've noticed that Clang also doesn't prohibit other type attributes (even 
> > > ones with C++ 11 syntax) from being applied to declarations. For example: 
> > > https://godbolt.org/z/Yj1zWY7nn
> > > 
> > > How do you think I should proceed here? I think the underlying issue is 
> > > that Clang doesn't always distinguish cleanly between declaration 
> > > attributes and type attributes, and fixing this might be nontrivial.
> > > How do you think I should proceed here? I think the underlying issue is 
> > > that Clang doesn't always distinguish cleanly between declaration 
> > > attributes and type attributes, and fixing this might be nontrivial.
> > 
> > This is a general issue with attribute processing. I would imagine that 
> > SemaDeclAttr.cpp should be able to diagnose that case when the attribute 
> > only applies to types and not declarations, but it'd take some 
> > investigation for me to be sure.
> > 
> > Because this issue isn't new to your situation, I'd recommend filing an 
> > issue about the general problem and then we can solve that later.
> I've done some more investigation myself, and I think I've come up with a 
> solution; actually, two potential solutions. I have draft patches for both of 
> these; I wanted to run these by you here first, so I haven't opened a bug yet.
> 
> I'd appreciate your input on how you'd prefer me to proceed with this. I do 
> think it makes sense to do this work now because otherwise, people will start 
> putting `annotate_type` in places where it doesn't belong, and then we'll 
> have to keep supporting that in the future.
> 
> **My preferred solution**
> 
> https://reviews.llvm.org/D124081
> 
> This adds a function `DiagnoseCXX11NonDeclAttrs()` and calls it in all places 
> where we should reject attributes that can't be put on declarations. (As part 
> of this work, I noticed that `gsl::suppress` should be a `DeclOrStmtAttr`, 
> not just a `StmtAttr`.)
> 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 5 inline comments as done.
mboehme added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > mboehme wrote:
> > > > xazax.hun wrote:
> > > > > Would it make sense to print something without the arguments? I 
> > > > > wonder which behavior would be less confusing.
> > > > TBH I'm not sure. Is TypePrinter used only to produce debug output or 
> > > > is it required that the output of TypePrinter will parse again 
> > > > correctly?
> > > > 
> > > > The reason I'm asking is because `annotate_type` has a mandatory first 
> > > > argument; if we need the output to be parseable, we would have to print 
> > > > some dummy string, e.g. 
> > > > `[[clang::annotate_type("arguments_omitted")]]`. This seems a bit 
> > > > strange, but maybe it's still worth doing. OTOH, if the output is used 
> > > > only for debugging, I guess we could print something like 
> > > > `[[clang::annotate_type(...)]]`, which doesn't parse but by that very 
> > > > nature makes it clear that we don't have all the information here.
> > > > 
> > > > I guess both of these options could have limited use -- they would at 
> > > > least make it clear that there is an annotation on the type, though 
> > > > without the arguments, we can't know what the actual annotation is.
> > > > 
> > > > Would appreciate some more input on the wider context here.
> > > Yeah, the trouble is that you need a `TypeLoc` in order to get back to 
> > > the actual attribute information that stores the arguments. But the type 
> > > printer is printing types not specific instances of types at the location 
> > > they're used.
> > > 
> > > The goal with the pretty printers is to print something back that the 
> > > user actually wrote, but it's not always possible and so this is sort of 
> > > a best-effort.
> > So what's your position -- should I not print the attribute at all (which 
> > is what I'm doing right now) or should I print it as something like 
> > `[[clang::annotate_type(...)]]`?
> I think we need to print *something* here because the type printer is used by 
> `QualType::getAsStringInternal()` and `QualType::print()` which are used to 
> produce diagnostics when giving a `QualType` argument. So I would  print 
> `[[clang::annotate]]` as a stop-gap with a FIXME comment that it would be 
> nice to print the arguments here some day.
Makes sense -- done!



Comment at: clang/lib/Parse/ParseDecl.cpp:3184
+continue;
+  // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though 
they
+  // are type attributes, because we historically haven't allowed these

aaron.ballman wrote:
> I think this is a FIXME situation -- our policy is to diagnose attributes 
> that are being ignored, and it seems pretty important to (someday, not part 
> of this patch) diagnose these. Especially the lifetime bound attribute given 
> that it's intended to help harden code.
Maybe I should clarify what's happening here: We _do_ produce diagnostics 
(errors) for AT_LifetimeBound and AT_AnyX86NoCfCheck, both before and after 
this patch. (For these attributes, the code falls through to the 
`Diag(PA.getLoc(), diag::err_attribute_not_type_attr)` call below.)

Before this patch, we would reject (produce errors for) _all_ C++11 attributes 
here. Now, we only reject non-type attributes, and in addition, we also reject 
AT_LifetimeBound and AT_AnyX86NoCfCheck (even though they are type attributes) 
because we historically haven't allowed them to be used in this way. There are 
tests for this behavior, so it seemed important to preserve it.



Comment at: clang/lib/Sema/SemaAttr.cpp:396-408
+if (E->getType()->isArrayType())
+  E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
+clang::CK_ArrayToPointerDecay)
+  .get();
+if (E->getType()->isFunctionType())
+  E = ImplicitCastExpr::Create(Context,
+   Context.getPointerType(E->getType()),

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > This seems an awful lot like doing 
> > > `DefaultFunctionArrayLValueConversion()` here -- can you call that to do 
> > > the heavy lifting?
> > > 
> > > Oh, I see we're just shuffling code around. Feel free to ignore or make 
> > > as an NFC change if you'd prefer.
> > By "NFC change", you mean I could submit this separately as an NFC change?
> > 
> > Since this change is already part of this patch, I think I'd prefer to just 
> > keep it in here, if that's OK?
> > By "NFC change", you mean I could submit this separately as an NFC change?
> 
> Yup!
> 
> > Since this change is already part of this patch, I think I'd prefer to 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 423856.
mboehme marked 2 inline comments as done.
mboehme added a comment.

Changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/attr-annotate-type.c
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+)cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/Sema/annotate-type.c:6
+void foo(float * [[clang::annotate_type("foo")]] a) {
+  int [[clang::annotate_type("bar")]] x1;
+  int * [[clang::annotate_type("bar")]] x2;

Is it possible to typedef int [[clang::annotate_type("bar")]]  mymagicint_t ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6409-6411
+- The attribute will not cause any additional data to be output to LLVM IR,
+  object files or debug information. It is only intended to be consumed using
+  Clang APIs by source-level static analysis tools such as Clang-Tidy.

mboehme wrote:
> aaron.ballman wrote:
> > It seems to me that because we found reasons for `annotate` to be lowered 
> > to LLVM IR, we'll find reasons to lower `annotate_type` eventually. I worry 
> > that if we document this, we'll never be able to modify the attribute to 
> > output information to LLVM IR because people will rely on it not doing so 
> > and be broken by a change. Are we sure we want to commit to this as part of 
> > the design?
> Good point. I've done what I think you're implying, namely that we should 
> remove the comments saying that the attribute won't have any any effect on 
> the LLVM IR?
Heh, yes, you interpreted my concerns correctly, thank you!



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > xazax.hun wrote:
> > > > Would it make sense to print something without the arguments? I wonder 
> > > > which behavior would be less confusing.
> > > TBH I'm not sure. Is TypePrinter used only to produce debug output or is 
> > > it required that the output of TypePrinter will parse again correctly?
> > > 
> > > The reason I'm asking is because `annotate_type` has a mandatory first 
> > > argument; if we need the output to be parseable, we would have to print 
> > > some dummy string, e.g. `[[clang::annotate_type("arguments_omitted")]]`. 
> > > This seems a bit strange, but maybe it's still worth doing. OTOH, if the 
> > > output is used only for debugging, I guess we could print something like 
> > > `[[clang::annotate_type(...)]]`, which doesn't parse but by that very 
> > > nature makes it clear that we don't have all the information here.
> > > 
> > > I guess both of these options could have limited use -- they would at 
> > > least make it clear that there is an annotation on the type, though 
> > > without the arguments, we can't know what the actual annotation is.
> > > 
> > > Would appreciate some more input on the wider context here.
> > Yeah, the trouble is that you need a `TypeLoc` in order to get back to the 
> > actual attribute information that stores the arguments. But the type 
> > printer is printing types not specific instances of types at the location 
> > they're used.
> > 
> > The goal with the pretty printers is to print something back that the user 
> > actually wrote, but it's not always possible and so this is sort of a 
> > best-effort.
> So what's your position -- should I not print the attribute at all (which is 
> what I'm doing right now) or should I print it as something like 
> `[[clang::annotate_type(...)]]`?
I think we need to print *something* here because the type printer is used by 
`QualType::getAsStringInternal()` and `QualType::print()` which are used to 
produce diagnostics when giving a `QualType` argument. So I would  print 
`[[clang::annotate]]` as a stop-gap with a FIXME comment that it would be nice 
to print the arguments here some day.



Comment at: clang/lib/Parse/ParseDecl.cpp:3184
+continue;
+  // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though 
they
+  // are type attributes, because we historically haven't allowed these

I think this is a FIXME situation -- our policy is to diagnose attributes that 
are being ignored, and it seems pretty important to (someday, not part of this 
patch) diagnose these. Especially the lifetime bound attribute given that it's 
intended to help harden code.



Comment at: clang/lib/Sema/SemaAttr.cpp:396-408
+if (E->getType()->isArrayType())
+  E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
+clang::CK_ArrayToPointerDecay)
+  .get();
+if (E->getType()->isFunctionType())
+  E = ImplicitCastExpr::Create(Context,
+   Context.getPointerType(E->getType()),

mboehme wrote:
> aaron.ballman wrote:
> > This seems an awful lot like doing `DefaultFunctionArrayLValueConversion()` 
> > here -- can you call that to do the heavy lifting?
> > 
> > Oh, I see we're just shuffling code around. Feel free to ignore or make as 
> > an NFC change if you'd prefer.
> By "NFC change", you mean I could submit this separately as an NFC change?
> 
> Since this change is already part of this patch, I think I'd prefer to just 
> keep it in here, if that's OK?
> By "NFC change", you mean I could submit this separately as an NFC change?

Yup!

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 4 inline comments as done.
mboehme added a comment.

In D111548#3439039 , @aaron.ballman 
wrote:

> Also, you should add a release note about the new attribute

Done.

and check the Precommit CI pipeline failure out:

> Failed Tests (1):
>
>   Clang :: CodeGenCXX/annotate-type.cpp

Fixed.




Comment at: clang/include/clang/Basic/AttrDocs.td:6409-6411
+- The attribute will not cause any additional data to be output to LLVM IR,
+  object files or debug information. It is only intended to be consumed using
+  Clang APIs by source-level static analysis tools such as Clang-Tidy.

aaron.ballman wrote:
> It seems to me that because we found reasons for `annotate` to be lowered to 
> LLVM IR, we'll find reasons to lower `annotate_type` eventually. I worry that 
> if we document this, we'll never be able to modify the attribute to output 
> information to LLVM IR because people will rely on it not doing so and be 
> broken by a change. Are we sure we want to commit to this as part of the 
> design?
Good point. I've done what I think you're implying, namely that we should 
remove the comments saying that the attribute won't have any any effect on the 
LLVM IR?



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

aaron.ballman wrote:
> mboehme wrote:
> > xazax.hun wrote:
> > > Would it make sense to print something without the arguments? I wonder 
> > > which behavior would be less confusing.
> > TBH I'm not sure. Is TypePrinter used only to produce debug output or is it 
> > required that the output of TypePrinter will parse again correctly?
> > 
> > The reason I'm asking is because `annotate_type` has a mandatory first 
> > argument; if we need the output to be parseable, we would have to print 
> > some dummy string, e.g. `[[clang::annotate_type("arguments_omitted")]]`. 
> > This seems a bit strange, but maybe it's still worth doing. OTOH, if the 
> > output is used only for debugging, I guess we could print something like 
> > `[[clang::annotate_type(...)]]`, which doesn't parse but by that very 
> > nature makes it clear that we don't have all the information here.
> > 
> > I guess both of these options could have limited use -- they would at least 
> > make it clear that there is an annotation on the type, though without the 
> > arguments, we can't know what the actual annotation is.
> > 
> > Would appreciate some more input on the wider context here.
> Yeah, the trouble is that you need a `TypeLoc` in order to get back to the 
> actual attribute information that stores the arguments. But the type printer 
> is printing types not specific instances of types at the location they're 
> used.
> 
> The goal with the pretty printers is to print something back that the user 
> actually wrote, but it's not always possible and so this is sort of a 
> best-effort.
So what's your position -- should I not print the attribute at all (which is 
what I'm doing right now) or should I print it as something like 
`[[clang::annotate_type(...)]]`?



Comment at: clang/lib/Sema/SemaAttr.cpp:396-408
+if (E->getType()->isArrayType())
+  E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
+clang::CK_ArrayToPointerDecay)
+  .get();
+if (E->getType()->isFunctionType())
+  E = ImplicitCastExpr::Create(Context,
+   Context.getPointerType(E->getType()),

aaron.ballman wrote:
> This seems an awful lot like doing `DefaultFunctionArrayLValueConversion()` 
> here -- can you call that to do the heavy lifting?
> 
> Oh, I see we're just shuffling code around. Feel free to ignore or make as an 
> NFC change if you'd prefer.
By "NFC change", you mean I could submit this separately as an NFC change?

Since this change is already part of this patch, I think I'd prefer to just 
keep it in here, if that's OK?



Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

aaron.ballman wrote:
> Can you also add some test coverage for applying the attribute to a 
> declaration instead of a type or not giving it any arguments? Also, should 
> test arguments which are not a constant expression.
I've added tests as you suggested, though I put most of them in 
Sema/annotate-type.c, as they're not specific to C++.

Thanks for prompting me to do this -- the tests caused me to notice and fix a 
number of bugs.

I haven't managed to diagnose the case when the attribute appears at the 
beginning of the declaration. It seems to me that, at the point where I've 
added the check, this case is indistinguishable from an attribute that appears 
on the type. In both cases, the `TAL` is 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 421930.
mboehme marked 2 inline comments as done.
mboehme added a comment.

Various changes in response to review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,154 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::ast_matchers::varDecl;
+using clang::tooling::buildASTFromCode;
+using clang::tooling::buildASTFromCodeWithArgs;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(ASTUnit *AST, const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+const VarDecl *getVariableNode(ASTUnit *AST, const std::string ) {
+  auto Result = match(varDecl(hasName(Name)).bind("var"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("var");
+}
+
+template 
+void AssertAnnotatedAs(TypeLoc TL, llvm::StringRef annotation,
+   ModifiedTypeLoc ,
+   const AnnotateTypeAttr **AnnotateOut = nullptr) {
+  const auto AttributedTL = TL.getAs();
+  ASSERT_FALSE(AttributedTL.isNull());
+  ModifiedTL = AttributedTL.getModifiedLoc().getAs();
+  ASSERT_TRUE(ModifiedTL);
+
+  ASSERT_NE(AttributedTL.getAttr(), nullptr);
+  const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+  ASSERT_NE(Annotate, nullptr);
+  EXPECT_EQ(Annotate->getAnnotation(), annotation);
+  if (AnnotateOut) {
+*AnnotateOut = Annotate;
+  }
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+
+int [[clang::annotate_type("int")]] * [[clang::annotate_type("ptr")]]
+  array[10] [[clang::annotate_type("arr")]];
+
+void (* [[clang::annotate_type("funcptr")]] fp)(void);
+
+struct S { int mem; };
+int [[clang::annotate_type("int")]]
+S::* [[clang::annotate_type("ptr_to_mem")]] ptr_to_member = ::mem;
+)cpp");
+
+  {
+const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+// First parameter.
+const auto PointerTL = Func->getParamDecl(0)
+   ->getTypeSourceInfo()
+   ->getTypeLoc()
+   .getAs();
+ASSERT_FALSE(PointerTL.isNull());
+PointerTypeLoc PointerPointerTL;
+const AnnotateTypeAttr *Annotate;
+AssertAnnotatedAs(PointerTL.getPointeeLoc(), "foo", PointerPointerTL,
+  );
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], AST->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], AST->getASTContext())
+  .size(),
+  1);
+
+// Second parameter.
+BuiltinTypeLoc IntTL;
+

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Also, you should add a release note about the new attribute and check the 
Precommit CI pipeline failure out:

Failed Tests (1):

  Clang :: CodeGenCXX/annotate-type.cpp
   


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6409-6411
+- The attribute will not cause any additional data to be output to LLVM IR,
+  object files or debug information. It is only intended to be consumed using
+  Clang APIs by source-level static analysis tools such as Clang-Tidy.

It seems to me that because we found reasons for `annotate` to be lowered to 
LLVM IR, we'll find reasons to lower `annotate_type` eventually. I worry that 
if we document this, we'll never be able to modify the attribute to output 
information to LLVM IR because people will rely on it not doing so and be 
broken by a change. Are we sure we want to commit to this as part of the design?



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

mboehme wrote:
> xazax.hun wrote:
> > Would it make sense to print something without the arguments? I wonder 
> > which behavior would be less confusing.
> TBH I'm not sure. Is TypePrinter used only to produce debug output or is it 
> required that the output of TypePrinter will parse again correctly?
> 
> The reason I'm asking is because `annotate_type` has a mandatory first 
> argument; if we need the output to be parseable, we would have to print some 
> dummy string, e.g. `[[clang::annotate_type("arguments_omitted")]]`. This 
> seems a bit strange, but maybe it's still worth doing. OTOH, if the output is 
> used only for debugging, I guess we could print something like 
> `[[clang::annotate_type(...)]]`, which doesn't parse but by that very nature 
> makes it clear that we don't have all the information here.
> 
> I guess both of these options could have limited use -- they would at least 
> make it clear that there is an annotation on the type, though without the 
> arguments, we can't know what the actual annotation is.
> 
> Would appreciate some more input on the wider context here.
Yeah, the trouble is that you need a `TypeLoc` in order to get back to the 
actual attribute information that stores the arguments. But the type printer is 
printing types not specific instances of types at the location they're used.

The goal with the pretty printers is to print something back that the user 
actually wrote, but it's not always possible and so this is sort of a 
best-effort.



Comment at: clang/lib/Sema/SemaAttr.cpp:396-408
+if (E->getType()->isArrayType())
+  E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
+clang::CK_ArrayToPointerDecay)
+  .get();
+if (E->getType()->isFunctionType())
+  E = ImplicitCastExpr::Create(Context,
+   Context.getPointerType(E->getType()),

This seems an awful lot like doing `DefaultFunctionArrayLValueConversion()` 
here -- can you call that to do the heavy lifting?

Oh, I see we're just shuffling code around. Feel free to ignore or make as an 
NFC change if you'd prefer.



Comment at: clang/lib/Sema/SemaType.cpp:8102
+static void HandleAnnotateTypeAttr(TypeProcessingState ,
+   QualType , const ParsedAttr ) {
+  Sema  = State.getSema();

Just to avoid re-using a type name as a declaration.



Comment at: clang/lib/Sema/SemaType.cpp:8117-8119
+  if (!S.ConstantFoldAttrArgs(Attr, Args)) {
+return;
+  }





Comment at: clang/test/SemaCXX/annotate-type.cpp:2
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {

Can you also add some test coverage for applying the attribute to a declaration 
instead of a type or not giving it any arguments? Also, should test arguments 
which are not a constant expression.



Comment at: clang/unittests/AST/AttrTest.cpp:39
 
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+const std::string ) {

Rather than doing the AST tests this way, you could add an `-ast-dump` to 
`clang\test\AST` and test the AST more directly. Coverage is roughly the same, 
but the AST test may be easier to read.

Other AST things to test: more complicated types that have the attribute to 
ensure it shows up in the correct places and can be written on deduced types.
```
int [[attr]] * [[attr]] ident[10] [[attr]];
void (* [[attr]] fp)(void);
__auto_type [[attr]] oooh = 12; // in C mode

struct S { int mem; };
int [[attr]] S::* [[attr]] oye = ::mem; // in C++ mode
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 2 inline comments as done.
mboehme added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6387
+additional information specific to the annotation category. The optional
+arguments must be constant expressions of arbitrary type.
+

xazax.hun wrote:
> Do we want to mention that it is not actually part of the type system (i.e., 
> annotated and unannotated types are considered the same)?
Good point -- I've added some relevant parts from the RFC below.



Comment at: clang/include/clang/Basic/AttrDocs.td:6393
+
+  int* [[clang::annotate("category1", "foo", 1)]] 
f(int[[clang::annotate("category2"]] *);
+

xazax.hun wrote:
> Missing closing paren for the second annotate. 
Oops -- thanks for catching. Fixed.



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

xazax.hun wrote:
> Would it make sense to print something without the arguments? I wonder which 
> behavior would be less confusing.
TBH I'm not sure. Is TypePrinter used only to produce debug output or is it 
required that the output of TypePrinter will parse again correctly?

The reason I'm asking is because `annotate_type` has a mandatory first 
argument; if we need the output to be parseable, we would have to print some 
dummy string, e.g. `[[clang::annotate_type("arguments_omitted")]]`. This seems 
a bit strange, but maybe it's still worth doing. OTOH, if the output is used 
only for debugging, I guess we could print something like 
`[[clang::annotate_type(...)]]`, which doesn't parse but by that very nature 
makes it clear that we don't have all the information here.

I guess both of these options could have limited use -- they would at least 
make it clear that there is an annotation on the type, though without the 
arguments, we can't know what the actual annotation is.

Would appreciate some more input on the wider context here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 421506.
mboehme added a comment.

Documentation: Added discussion of type system implications and fixed syntax
error in example.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,81 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::tooling::buildASTFromCode;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+)cpp");
+
+  const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+  {
+TypeLoc Parm0TL = Func->getParamDecl(0)->getTypeSourceInfo()->getTypeLoc();
+const auto PointerTL = Parm0TL.getAs();
+ASSERT_FALSE(PointerTL.isNull());
+const auto AttributedTL =
+PointerTL.getPointeeLoc().getAs();
+ASSERT_FALSE(AttributedTL.isNull());
+ASSERT_NE(AttributedTL.getAttr(), nullptr);
+ASSERT_TRUE(AttributedTL.getModifiedLoc().getAs());
+
+const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+ASSERT_NE(Annotate, nullptr);
+EXPECT_EQ(Annotate->getAnnotation(), "foo");
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], Func->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], Func->getASTContext())
+  .size(),
+  1);
+  }
+
+  {
+TypeLoc Parm1TL = Func->getParamDecl(1)->getTypeSourceInfo()->getTypeLoc();
+const auto AttributedTL = Parm1TL.getAs();
+ASSERT_FALSE(AttributedTL.isNull());
+ASSERT_TRUE(AttributedTL.getModifiedLoc().getType() ==
+Func->getASTContext().IntTy);
+
+const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+ASSERT_NE(Annotate, nullptr);
+EXPECT_EQ(Annotate->getAnnotation(), "bar");
+  }
+}
+
 } // namespace
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
+  void f() [[clang::annotate_type("foo")]];
+};
+
+template  struct is_same {
+  static constexpr bool value = false;
+};
+
+template  struct is_same {
+  static constexpr bool value = true;
+};
+
+static_assert(is_same::value);
+static_assert(is_same::value);
+static_assert(is_same::value);
+
+// Cannot overload on types that only differ by `annotate_type` attribute.
+void f(int) {} // expected-note {{previous definition is here}}
+void f(int [[clang::annotate_type("foo")]]) {} // expected-error {{redefinition of 'f'}}
+
+// Cannot specialize on types that only differ by 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:6387
+additional information specific to the annotation category. The optional
+arguments must be constant expressions of arbitrary type.
+

Do we want to mention that it is not actually part of the type system (i.e., 
annotated and unannotated types are considered the same)?



Comment at: clang/include/clang/Basic/AttrDocs.td:6393
+
+  int* [[clang::annotate("category1", "foo", 1)]] 
f(int[[clang::annotate("category2"]] *);
+

Missing closing paren for the second annotate. 



Comment at: clang/lib/AST/TypePrinter.cpp:1686
+  // would require retrieving the attribute arguments, which we don't have 
here.
+  if (T->getAttrKind() == attr::AnnotateType)
+return;

Would it make sense to print something without the arguments? I wonder which 
behavior would be less confusing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 421159.
mboehme added a comment.

Add documentation and a unit test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp
  clang/unittests/AST/AttrTest.cpp

Index: clang/unittests/AST/AttrTest.cpp
===
--- clang/unittests/AST/AttrTest.cpp
+++ clang/unittests/AST/AttrTest.cpp
@@ -7,7 +7,10 @@
 //===--===//
 
 #include "clang/AST/Attr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/AttrKinds.h"
+#include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -15,10 +18,81 @@
 
 namespace {
 
+using clang::ast_matchers::constantExpr;
+using clang::ast_matchers::equals;
+using clang::ast_matchers::functionDecl;
+using clang::ast_matchers::has;
+using clang::ast_matchers::hasDescendant;
+using clang::ast_matchers::hasName;
+using clang::ast_matchers::integerLiteral;
+using clang::ast_matchers::match;
+using clang::ast_matchers::selectFirst;
+using clang::ast_matchers::stringLiteral;
+using clang::tooling::buildASTFromCode;
+
 TEST(Attr, Doc) {
   EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
   testing::HasSubstr("The compiler must emit the definition even "
  "if it appears to be unused"));
 }
 
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+const std::string ) {
+  auto Result =
+  match(functionDecl(hasName(Name)).bind("fn"), AST->getASTContext());
+  EXPECT_EQ(Result.size(), 1u);
+  return Result[0].getNodeAs("fn");
+}
+
+TEST(Attr, AnnotateType) {
+
+  // Test that the AnnotateType attribute shows up in the right places and that
+  // it stores its arguments correctly.
+
+  auto AST = buildASTFromCode(R"cpp(
+void f(int* [[clang::annotate_type("foo", "arg1", 2)]] *,
+   int [[clang::annotate_type("bar")]]);
+)cpp");
+
+  const FunctionDecl *Func = getFunctionNode(AST.get(), "f");
+
+  {
+TypeLoc Parm0TL = Func->getParamDecl(0)->getTypeSourceInfo()->getTypeLoc();
+const auto PointerTL = Parm0TL.getAs();
+ASSERT_FALSE(PointerTL.isNull());
+const auto AttributedTL =
+PointerTL.getPointeeLoc().getAs();
+ASSERT_FALSE(AttributedTL.isNull());
+ASSERT_NE(AttributedTL.getAttr(), nullptr);
+ASSERT_TRUE(AttributedTL.getModifiedLoc().getAs());
+
+const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+ASSERT_NE(Annotate, nullptr);
+EXPECT_EQ(Annotate->getAnnotation(), "foo");
+
+EXPECT_EQ(Annotate->args_size(), 2);
+const auto *StringLit = selectFirst(
+"str", match(constantExpr(hasDescendant(stringLiteral().bind("str"))),
+ *Annotate->args_begin()[0], Func->getASTContext()));
+ASSERT_NE(StringLit, nullptr);
+EXPECT_EQ(StringLit->getString(), "arg1");
+EXPECT_EQ(match(constantExpr(has(integerLiteral(equals(2)).bind("int"))),
+*Annotate->args_begin()[1], Func->getASTContext())
+  .size(),
+  1);
+  }
+
+  {
+TypeLoc Parm1TL = Func->getParamDecl(1)->getTypeSourceInfo()->getTypeLoc();
+const auto AttributedTL = Parm1TL.getAs();
+ASSERT_FALSE(AttributedTL.isNull());
+ASSERT_TRUE(AttributedTL.getModifiedLoc().getType() ==
+Func->getASTContext().IntTy);
+
+const auto *Annotate = dyn_cast(AttributedTL.getAttr());
+ASSERT_NE(Annotate, nullptr);
+EXPECT_EQ(Annotate->getAnnotation(), "bar");
+  }
+}
+
 } // namespace
Index: clang/test/SemaCXX/annotate-type.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
+  void f() [[clang::annotate_type("foo")]];
+};
+
+template  struct is_same {
+  static constexpr bool value = false;
+};
+
+template  struct is_same {
+  static constexpr bool value = true;
+};
+
+static_assert(is_same::value);
+static_assert(is_same::value);
+static_assert(is_same::value);
+
+// Cannot overload on types that only differ by `annotate_type` attribute.
+void f(int) {} // expected-note {{previous definition is here}}
+void f(int [[clang::annotate_type("foo")]]) {} // expected-error {{redefinition of 'f'}}
+
+// Cannot specialize on types that only differ by `annotate_type` attribute.
+template  struct S2 {};
+
+template <> 

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 419642.
mboehme added a comment.
Herald added a project: All.

Uploading newest version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGenCXX/annotate-type.cpp
  clang/test/Sema/annotate-type.c
  clang/test/SemaCXX/annotate-type.cpp

Index: clang/test/SemaCXX/annotate-type.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/annotate-type.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify
+
+struct S1 {
+  void f() [[clang::annotate_type("foo")]];
+};
+
+template  struct is_same {
+  static constexpr bool value = false;
+};
+
+template  struct is_same {
+  static constexpr bool value = true;
+};
+
+static_assert(is_same::value);
+static_assert(is_same::value);
+static_assert(is_same::value);
+
+// Cannot overload on types that only differ by `annotate_type` attribute.
+void f(int) {} // expected-note {{previous definition is here}}
+void f(int [[clang::annotate_type("foo")]]) {
+} // expected-error {{redefinition of 'f'}}
+
+// Cannot specialize on types that only differ by `annotate_type` attribute.
+template  struct S2 {};
+
+template <> struct S2 {}; // expected-note {{previous definition is here}}
+
+template <>
+struct S2{}; // expected-error {{redefinition of 'S2'}}
Index: clang/test/Sema/annotate-type.c
===
--- /dev/null
+++ clang/test/Sema/annotate-type.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -fsyntax-only -fdouble-square-bracket-attributes -verify
+
+void foo(float * [[clang::annotate_type("foo")]] a) {
+  int [[clang::annotate_type("bar")]] x1;
+  int * [[clang::annotate_type("bar")]] x2;
+  int * [[clang::annotate_type(1)]] x3; // expected-error {{'annotate_type' attribute requires a string}}
+  int * [[clang::annotate_type("bar", 1)]] x4;
+
+  // GNU spelling is not supported
+  int __attribute__((annotate_type("bar"))) y1; // expected-warning {{unknown attribute 'annotate_type' ignored}}
+  int * __attribute__((annotate_type("bar"))) y2; // expected-warning {{unknown attribute 'annotate_type' ignored}}
+}
Index: clang/test/CodeGenCXX/annotate-type.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/annotate-type.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only %s -emit-llvm -o
+// - | FileCheck %s
+
+// Test that `annotate_type` does not affect mangled names.
+
+int *[[clang::annotate_type("foo")]] f(int *[[clang::annotate_type("foo")]],
+   int [[clang::annotate_type("foo")]]) {
+  return nullptr;
+}
+// CHECK: @_Z1fPii
+
+template  struct S {};
+
+S
+g(S) {
+  return {};
+}
+// CHECK: @_Z1g1SIPiE()
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -8098,6 +8098,30 @@
 CurType = T;
 }
 
+static void HandleAnnotateTypeAttr(TypeProcessingState ,
+   QualType , const ParsedAttr ) {
+  Sema  = State.getSema();
+
+  // Make sure that there is a string literal as the annotation's first
+  // argument.
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str))
+return;
+
+  llvm::SmallVector Args;
+  Args.reserve(Attr.getNumArgs() - 1);
+  for (unsigned Idx = 1; Idx < Attr.getNumArgs(); Idx++) {
+assert(!Attr.isArgIdent(Idx));
+Args.push_back(Attr.getArgAsExpr(Idx));
+  }
+  if (!S.ConstantFoldAttrArgs(Attr, Args)) {
+return;
+  }
+  auto *AnnotateTypeAttr =
+  AnnotateTypeAttr::Create(S.Context, Str, Args.data(), Args.size(), Attr);
+  CurType = State.getAttributedType(AnnotateTypeAttr, CurType, CurType);
+}
+
 static void HandleLifetimeBoundAttr(TypeProcessingState ,
 QualType ,
 ParsedAttr ) {
@@ -8158,10 +8182,11 @@
   if (!IsTypeAttr)
 continue;
 }
-  } else if (TAL != TAL_DeclChunk && !isAddressSpaceKind(attr)) {
+  } else if (TAL != TAL_DeclChunk && !isAddressSpaceKind(attr) &&
+ attr.getKind() != ParsedAttr::AT_AnnotateType) {
 // Otherwise, only consider type processing for a C++11 attribute if
 // it's actually been applied to a type.
-// We also allow C++11 address_space and
+// We also allow C++11 address_space and annotate_type and
 // OpenCL language address space attributes to pass through.
 continue;
   }
@@ -8359,6 +8384,11 @@
   attr.setUsedAsTypeAttr();
   

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme created this revision.
mboehme added a project: clang.
Herald added a reviewer: aaron.ballman.
mboehme requested review of this revision.
Herald added a subscriber: cfe-commits.

This is an analog to the `annotate` attribute but for types. The intent is to 
allow adding arbitrary annotations to types for use in static analysis tools.

DO NOT REVIEW YET

I'm uploading this for discussion on cfe-dev. Will remove this note once the 
change is ready for review.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111548

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/annotate-type.c

Index: clang/test/Sema/annotate-type.c
===
--- /dev/null
+++ clang/test/Sema/annotate-type.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -fdouble-square-bracket-attributes -verify
+
+void foo(float *__attribute__((annotate_type("foo"))) a) {
+  // Only the GNU spelling of the attribute can be applied to decl specifiers.
+  int __attribute__((annotate_type("bar"))) w;
+  int [[clang::annotate_type("bar")]] w2; // expected-error {{'annotate_type' attribute cannot be applied to types}}
+
+  int *__attribute__((annotate_type("bar"))) x;
+  int *[[clang::annotate_type("bar")]] x2;
+
+  int *__attribute__((annotate_type(1))) y; // expected-error {{'annotate_type' attribute requires a string}}
+  int *[[clang::annotate_type(1)]] y2;  // expected-error {{'annotate_type' attribute requires a string}}
+
+  int *__attribute__((annotate_type("bar", 1))) z;
+  int *[[clang::annotate_type("bar", 1)]] z2;
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -8036,6 +8036,30 @@
 CurType = T;
 }
 
+static void HandleAnnotateTypeAttr(TypeProcessingState ,
+   QualType , const ParsedAttr ) {
+  Sema  = State.getSema();
+
+  // Make sure that there is a string literal as the annotation's first
+  // argument.
+  StringRef Str;
+  if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str))
+return;
+
+  llvm::SmallVector Args;
+  Args.reserve(Attr.getNumArgs() - 1);
+  for (unsigned Idx = 1; Idx < Attr.getNumArgs(); Idx++) {
+assert(!Attr.isArgIdent(Idx));
+Args.push_back(Attr.getArgAsExpr(Idx));
+  }
+  if (!S.ConstantFoldAttrArgs(Attr, Args)) {
+return;
+  }
+  auto *AnnotateTypeAttr =
+  AnnotateTypeAttr::Create(S.Context, Str, Args.data(), Args.size(), Attr);
+  CurType = State.getAttributedType(AnnotateTypeAttr, CurType, CurType);
+}
+
 static void HandleLifetimeBoundAttr(TypeProcessingState ,
 QualType ,
 ParsedAttr ) {
@@ -8298,6 +8322,11 @@
   attr.setUsedAsTypeAttr();
   break;
 }
+case ParsedAttr::AT_AnnotateType: {
+  HandleAnnotateTypeAttr(state, type, attr);
+  attr.setUsedAsTypeAttr();
+  break;
+}
 }
 
 // Handle attributes that are defined in a macro. We do not want this to be
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -3863,48 +3863,10 @@
 void Sema::AddAnnotationAttr(Decl *D, const AttributeCommonInfo ,
  StringRef Str, MutableArrayRef Args) {
   auto *Attr = AnnotateAttr::Create(Context, Str, Args.data(), Args.size(), CI);
-  llvm::SmallVector Notes;
-  for (unsigned Idx = 0; Idx < Attr->args_size(); Idx++) {
-Expr * = Attr->args_begin()[Idx];
-assert(E && "error are handled before");
-if (E->isValueDependent() || E->isTypeDependent())
-  continue;
-
-if (E->getType()->isArrayType())
-  E = ImpCastExprToType(E, Context.getPointerType(E->getType()),
-clang::CK_ArrayToPointerDecay)
-  .get();
-if (E->getType()->isFunctionType())
-  E = ImplicitCastExpr::Create(Context,
-   Context.getPointerType(E->getType()),
-   clang::CK_FunctionToPointerDecay, E, nullptr,
-   VK_PRValue, FPOptionsOverride());
-if (E->isLValue())
-  E = ImplicitCastExpr::Create(Context, E->getType().getNonReferenceType(),
-   clang::CK_LValueToRValue, E, nullptr,
-   VK_PRValue, FPOptionsOverride());
-
-Expr::EvalResult Eval;
-Notes.clear();
-Eval.Diag = 
-
-bool Result =
-E->EvaluateAsConstantExpr(Eval, Context);
-
-/// Result means the expression can be folded to a constant.
-/// Note.empty() means the expression is a valid constant expression in the
-/// current language mode.
-