[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

Great feature for all the weird pieces of hardware all around the world! :-)



================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2229-2233
+  SmallVector<llvm::Value *, 5> Args = {
+      AnnotatedVal,
+      Builder.CreateBitCast(CGM.EmitAnnotationString(AnnotationStr), Int8PtrTy),
+      Builder.CreateBitCast(CGM.EmitAnnotationUnit(Location), Int8PtrTy),
+      CGM.EmitAnnotationLineNo(Location),
----------------
Curious reindenting with mix-and-match of spaces & tabulations?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2386-2390
+      llvm::ConstantExpr::getBitCast(ASZeroGV, Int8PtrTy),
+      llvm::ConstantExpr::getBitCast(AnnoGV, Int8PtrTy),
+      llvm::ConstantExpr::getBitCast(UnitGV, Int8PtrTy),
+      LineNoCst,
+      Args,
----------------
Same curious reindenting.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3688
+    Expr *&E = Attr->args_begin()[Idx];
+    ConstantExpr *CE = dyn_cast<ConstantExpr>(E);
+    if (!E) {
----------------
aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > `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.
> > 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.
> Great, thank you for the fix!
Perhaps a few comments on this clarification would be useful in the code for the plain mortals...


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3704-3705
+      Result = E->EvaluateAsRValue(Eval, Context, true);
+    else
+      Result = E->EvaluateAsLValue(Eval, Context, true);
+
----------------
aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > 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.
> > Expr::EvaluateAsConstantExpr will evaluate expression in there value category.
> > this can be quite surprising in some situations like:
> > ```
> > 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.
> 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).
This needs some motivating comments and explanations in the code.
Also the variations on `g_i` annotation above are quite interesting and should appear in the unit tests. I am unsure they are now.
Perhaps more interesting with other values than `0` in the example. :-)
I guess we can still use `[[clang::annotate("test", &g_i)]] void t() {}` to have a pointer on `g_i` and `(int&)g_i` for reference? To add to the unit tests if not already checked.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645