[PATCH] D111548: [Clang] Add the `annotate_type` attribute
Martin Böhme via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 20 04:32:12 PDT 2022
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 just keep it in here, if that's OK?
>
> That's fine by me. If you intend to keep it as it is though, you should probably add a FIXME comment about potentially using the other function instead of doing this the hard way.
> That's fine by me. If you intend to keep it as it is though, you should probably add a FIXME comment about potentially using the other function instead of doing this the hard way.
Makes sense -- I've done the latter.
================
Comment at: clang/lib/Sema/SemaType.cpp:8106
+ if (PA.getNumArgs() < 1)
+ // Diagnostic is produced in ProcessDeclAttribute().
+ return;
----------------
aaron.ballman wrote:
> Wha?
>
> Do we really get into `ProcessDeclAttribute()` for something like:
> ```
> template <typename Ty> void func();
> void foo() {
> func<int [[clang::annotate_type()]]>();
> }
> ```
> (My intuition was that this needs to be diagnosed here -- see `HandleVectorSizeAttr()` -- but I could be remembering type attribute processing somewhat wrong.)
Ah -- true, we don't get into `ProcessDeclAttribute()` for the example you gave. (I have added this example to the tests.)
So I've realized that I do need to emit the diagnostic here to make sure that I emit it in all cases (and not just those seen by `ProcessDeclAttribute()`. To prevent `ProcessDeclAttribute()` from emitting a duplicate diagnostic in the cases where it sees the attribute, I've set the `HasCustomParsing` flag for the attribute. This seems appropriate anyhow.
================
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;
----------------
xbolva00 wrote:
> Is it possible to typedef int [[clang::annotate_type("bar")]] mymagicint_t ?
Yes -- I've added a test.
================
Comment at: clang/test/Sema/annotate-type.c:23
+ int * [[clang::annotate_type(1)]] z3; // expected-error {{'annotate_type' attribute requires a string}}
+ int * [[clang::annotate_type()]] z4; // expected-error {{'annotate_type' attribute takes at least 1 argument}}
+ int * [[clang::annotate_type]] z5; // expected-error {{'annotate_type' attribute takes at least 1 argument}}
----------------
aaron.ballman wrote:
> I'd also like to see an example like:
> ```
> int *[[clang::annotate_type("second arg is a problem", int)]] zN;
> ```
> to demonstrate that we don't hit the assert added in `HandleAnnotateTypeAttr()`.
Done (below).
I've also added another test case where the second argument is an identifier, not a keyword.
================
Comment at: clang/test/SemaCXX/annotate-type.cpp:33
+template <>
+struct S2<int [[clang::annotate_type("foo")]]> {}; // expected-error {{redefinition of 'S2<int>'}}
----------------
aaron.ballman wrote:
> Another thing we should probably test is parameter pack expansion that's supported by the declaration version of this. e.g.,
> ```
> template <int... Is> void variadic_nttp() {
> [[clang::annotate("D", Is...)]] int val1; // This works today
> int [[clang::annotate_type(D, Is...)]] val2; // I'd expect this to work as well
> }
>
> int main() {
> variadic_nttp<1, 2, 3>();
> }
> ```
> If it doesn't work yet, I think it's fine to do the implementation work in a follow-up, but we should document the difference so user's aren't caught by surprise.
I've added a test (and set the `AcceptsExprPack` flag so the attribute does actually accept parameter packs, just like `annotate`).
================
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:
> > > 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 know whether we're parsing a statement or a declaration. As a result, we need to be able to specify `AppertainsToUnknown` and would need to add additional logic deeper in the call stack to produce diagnostics once we know for sure whether we're dealing with a statement or declaration. (This isn't yet implemented.) This makes the solution less elegant that it initially seems.
- There's a tension between this new functionality and the existing `ParsedAttr::diagnoseAppertainsTo()`. However, we unfortunately can't extend the existing `ParsedAttr::diagnoseAppertainsTo()` because it is called from `ProcessDeclAttribute()` and (despite the name) that function is also processes some legitimate type attributes.
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