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

Martin Böhme via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 11 08:10:08 PDT 2022


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

In D111548#3439039 <https://reviews.llvm.org/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 `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.


================
Comment at: clang/unittests/AST/AttrTest.cpp:39
 
+const FunctionDecl *getFunctionNode(clang::ASTUnit *AST,
+                                    const std::string &Name) {
----------------
aaron.ballman wrote:
> 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 = &S::mem; // in C++ mode
> ```
> Rather than doing the AST tests this way, you could add an -ast-dump to clang\test\AST and test the AST more directly.
I think a problem with this might be that `TypePrinter` can't print the arguments of the attribute, and I want to test those?

> 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.
Done. I've also introduced a test helper `AssertAnnotatedAs` to keep the tests as compact as I can.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111548



More information about the cfe-commits mailing list