This allows using annotation in a much more contexts than it currently has.
especially when annotation with template or constexpr.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/CodeGen/CodeGenModule.cpp | ||
---|---|---|
2338 | Exprs.empty() would be more concise. | |
2354–2356 | You can return the expression directly, which removes a somewhat questionable use of auto. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
3683 | Just curious, but is the ?: actually necessary? I would have assumed that an empty ArrayRef would return nullptr from data(). | |
3688 | auto * since the type is spelled out in the initialization. Also, I think this is unsafe -- it looks like during template instantiation, any arguments that have a substitution failure will come through as nullptr, and this will crash. What should happen on template instantiation failure for these arguments? I think the whole attribute should probably be dropped with appropriate diagnostics rather than emitting a partial attribute, but perhaps you have different ideas. | |
3691 | This is unfortunate -- you should be passing in the ParsedAttr object so that the diagnostic engine can take care of properly formatting the diagnostic. For instance, this will incorrectly name the attribute 'annotate' instead of clang::annotate' if that's how the user spelled the attribute. I don't recall whether it will work on an AttributeCommonInfo though. | |
3694 | E->getDependence() smells a bit fishy because this will early bail on any kind of dependency, including an error dependency. Do you mean to care only about type and/or value dependence here? | |
3704–3705 | Under what circumstances would we want the constant expressions to be lvalues? I would have guessed you would want to call Expr::EvaluateAsConstantExpr() instead of either of these. | |
3730 | Hmm, I think you should assert that you never get an identifier at this point -- the parser should be treating all of these as expressions and not simple identifiers. | |
clang/test/SemaCXX/attr-annotate.cpp | ||
1 | Do you mean to emit llvm here? I think that should probably be -fsyntax-only |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3683 | maybe it was(in a previous version of the code) but not anymore since there is always at least one argument. | |
3688 | When template instantation fails nullptr is put in the Expr arguments and AddAnnotationAttr will emit a diagnostic and not associate the attribut to the declaration. | |
3704–3705 | Expr::EvaluateAsConstantExpr will evaluate expression in there value category. const int g_i = 0; [[clang::annotate("test", g_i)]] void t() {} // annotation carries a pointer/reference on g_i [[clang::annotate("test", (int)g_i)]] void t1() {} // annotation carries the value 0 [[clang::annotate("test", g_i + 0)]] void t1() {} // annotation carries the value 0 with EvaluateAsRValue in all of the cases above the annotation will carry the value 0. optionally we could wrap expression with an LValue to RValue cast and call EvaluateAsConstantExpr this should also work. | |
clang/test/SemaCXX/attr-annotate.cpp | ||
1 | The reason i put an -emit-llvm is to also test the asserts in IRgen on more complex code. |
I didn't say this before, but yeah, I agree -- this is really quite neat, thank you for working on it!
clang/include/clang/Sema/ParsedAttr.h | ||
---|---|---|
1077 | as only argument -> as its only argument | |
1081 | The comment can probably be re-flowed so "ambiguity" is on the preceding line? | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
3688 | Great, thank you for the fix! | |
3693 | What is the rationale for bailing out when the constant expression has an APValue result? | |
3704–3705 | Thank you for the explanation. I think wrapping with an lvalue to rvalue conversion may make more sense -- EvaluateAsRValue() tries really hard to get an rvalue out of something even if the standard says that's not okay. It'll automatically apply the lvalue to rvalue conversion if appropriate, but it'll also do more than that (such as evaluating side effects). | |
clang/test/SemaCXX/attr-annotate.cpp | ||
1 | Tests in sema shouldn't typically run codegen, so I'd rather this was -fsyntax-only and test the assertions more explicitly in a codegen-specific test. |
Great feature for all the weird pieces of hardware all around the world! :-)
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
2229–2233 | Curious reindenting with mix-and-match of spaces & tabulations? | |
clang/lib/CodeGen/CodeGenModule.cpp | ||
2387–2391 | Same curious reindenting. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
3688 | Perhaps a few comments on this clarification would be useful in the code for the plain mortals... | |
3704–3705 | This needs some motivating comments and explanations in the code. |
@erichkeane @alexandre.isoard @Naghasan: any feedback in the context of SYCL & HLS C++ about this feature extension?
clang/lib/CodeGen/CodeGenFunction.cpp | ||
---|---|---|
2229–2233 | these are not tabs i believe this is just how phabricator shows indentation changes. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
3693 | the intent was that during nested template instantiation arguement will be evaluated as soon as they are neither value nor type dependent and the result will be stored in the ConstantExpr. if an arguement has already been evaluated in a previous instantiation we don't want to evaluate it again. | |
3704–3705 | i changed to use EvaluateAsConstantExpr + LValueToRvalue cast.
they are tested not as expressively as above but they are tested.
yes this is how it is working.
(int&)g_i has the salve value category and same type as g_i it just has a noop cast around it. |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3704–3705 | I see. Thanks for the clarification. |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3693 | Ah, thank you for the explanation and change. | |
3696–3697 | Should this move up above the point where we're checking whether the expression has an array type? | |
3706 | I'm surprised that the presence of notes alone would mean the attribute argument has an error and should be skipped. Are there circumstances under which Result is true but Notes is non-empty? | |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
195 | No else after a return. | |
clang/test/SemaCXX/attr-annotate.cpp | ||
97 | Some more tests for various constant expression situations that may be weird: int n = 10; int vla[n]; [[clang::annotate("vlas are awful", sizeof(vla[++n]))] int i = 0; // reject, the sizeof is not unevaluated [[clang::annotate("_Generic selection expression should be fine", _Generic(n, int : 0, default : 1))]] int j = 0; // second arg should resolve to 0 fine void designator(); [[clang::annotate("function designators?", designator)]] int k = 0; // Should work? |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3706 | AFAIK the difference is observable in code like: constexpr int foldable_but_invalid() { int *A = new int(0); return *A; } [[clang::annotate("", foldable_but_invalid())]] void f1() {} foldable_but_invalid retruns a constant but any constant evaluation of this function is invalid because it doesn't desallocate A. i think it is desirable that attributes don't diverge from the language mode. | |
clang/test/SemaCXX/attr-annotate.cpp | ||
97 | added CK_FunctionToPointerDecay cast when needed |
LGTM aside from a request for a comment to be added. Thank you!
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
3706 | Ah, thank you for explaining that! Can you add a comment to the code to make that clear for others who may run across it? |
He means this review comment he made requesting a comment in the code: >Ah, thank you for explaining that! Can you add a comment to the code to make that clear for others who may run across it?
Oh gosh no! I just meant I was asking for a comment to be added in SemaDeclAttr.td before you commit. Sorry for the confusion! :-)
Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt
Please take a look, and revert for now if it takes a while to fix.
This also causes the AnnotateFunctions example to no longer build:
/home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp: In member function ‘virtual bool {anonymous}::AnnotateFunctionsConsumer::HandleTopLevelDecl(clang::DeclGroupRef)’: /home/jb/work/llvm-project/clang/examples/AnnotateFunctions/AnnotateFunctions.cpp:36:70: error: no matching function for call to ‘clang::AnnotateAttr::CreateImplicit(clang::ASTContext&, const char [19])’ "example_annotation")); ^
it looks like it needs to be adjusted for the extra variadic arg that the attribute now has.
@Tyker This is causing another build failure in another example:
[2858/4034] Building CXX object tools/clang/examples/Attribute/CMakeFiles/Attribute.dir/Attribute.cpp.o /Attribute.cpp:73:16: error: no matching function for call to 'Create' D->addAttr(AnnotateAttr::Create(S.Context, "example(" + Str.str() + ")", ^~~~~~~~~~~~~~~~~~~~ tools/clang/include/clang/AST/Attrs.inc:885:24: note: candidate function not viable: requires at least 4 arguments, but 3 were provided static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, Expr * *Args, unsigned ArgsSize, const AttributeCommonInfo &CommonInfo = {SourceRange{}}); ^ tools/clang/include/clang/AST/Attrs.inc:887:24: note: candidate function not viable: requires 6 arguments, but 3 were provided static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, Expr * *Args, unsigned ArgsSize, SourceRange Range, AttributeCommonInfo::Syntax Syntax); ^ 1 error generated.
Please build with -DCLANG_BUILD_EXAMPLES=ON to check if all bugs are gone.
The lang ref needs to reflect the new type https://llvm.org/docs/LangRef.html#llvm-var-annotation-intrinsic
Please also review the auto-upgrade patch for this: https://reviews.llvm.org/D95993
as only argument -> as its only argument