[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D108479#3140638 <https://reviews.llvm.org/D108479#3140638>, @samitolvanen wrote:

> In D108479#3133578 <https://reviews.llvm.org/D108479#3133578>, @rjmccall wrote:
>
>> If we do need to support constant expressions of this
>
> Yes, we need this also in constant expressions.

Okay.  I assume just static initializers, and not things like template arguments?

>> I think we should have the constant evaluator produce an expression rather than an l-value, the way we do with some other builtin calls.  That should stop the comparison problem more generally.
>
> Sure, I can take a look at how that would work. Basically, in `PointerExprEvaluator::VisitBuiltinCallExpr` we should not evaluate the l-value and just leave it at `Result.set(E)`?

Yes, exactly.  Since the builtin already requires a constant operand in non-dependent contexts, that should be enough.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:199
+/// Check that the argument to __builtin_function_start is a function.
+static bool SemaBuiltinSymbolAddress(Sema &S, CallExpr *TheCall) {
+  if (checkArgCount(S, TheCall, 1))
----------------
Please update the function name here.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:208
+    if (UnaryOp->getOpcode() == UnaryOperator::Opcode::UO_AddrOf)
+      E = UnaryOp->getSubExpr();
+
----------------
It would be more general to allow any expression that we can constant-evaluate to a specific function / member function reference.  That allows callers to do stuff like `__builtin_function_start((int (A::*)() const) &A::x)` to resolve overloaded function references.

You should delay this check if the operand is value-dependent.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:221
+  if (ResultType.isNull())
+    return true;
+
----------------
I think we decided that the result type should be `void*`.  Are there other semantic checks from `CheckAddressOfOperand` that still meaningfully apply?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108479