[PATCH] D76420: Prevent IR-gen from emitting consteval declarations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:1062
+    return ConstantExprBits.APValueKind != APValue::None &&
+           ConstantExprBits.APValueKind != APValue::Indeterminate;
   }
----------------
Why do we need to treat indeterminate values specially here? Are we using that value inappropriately in `ExprConstant`?


================
Comment at: clang/lib/AST/ExprConstant.cpp:6807-6808
 
+    llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
     return StmtVisitorTy::Visit(E->getSubExpr());
   }
----------------
I don't think this is really right, but perhaps the difference isn't observable right now. What I'm thinking of is a case like this:

```
consteval int do_stuff() {
  __builtin_produce_diagnostic("hello world\n");
  return 42;
}
constexpr int f() {
  int n = do_stuff();
  return n;
}
int k = f();
```

Here, I would expect that when we process the immediate invocation of `do_stuff()` in `f`, we would immediately evaluate it, including issuing its diagnostic. And then for all subsequent calls to `f()`, we would never re-evaluate it.

I can see a couple of ways this could work:

 * Whenever we create a `ConstantExpr`, we always evaluate it and fill in the `APValue` result; it's never absent except perhaps in a window of time between forming that AST node and deciding for sure that we want to keep it (for nested immediate invocation handling).
 * Whenever constant evaluation meets a `ConstantExpr` that doesn't have an associated result yet, it triggers that result to be computed and cached, as a separate evaluation.

I think the first of those two approaches is much better: lazily evaluating the `ConstantExpr` will require us to save update records if we're writing an AST file, and will mean we don't always have an obvious point where the side-effects from builtin consteval functions (eg, reflection-driven actions) happen.

So I think the right thing to do is probably to say that a `ConstantExpr` that hasn't yet had its `APValue` result filled in is non-constant for now, and to ensure that everywhere that creates a `ConstantExpr` always eventually either removes it again or fills in the result.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10772-10777
 bool IntExprEvaluator::VisitConstantExpr(const ConstantExpr *E) {
   llvm::SaveAndRestore<bool> InConstantContext(Info.InConstantContext, true);
-  if (E->getResultAPValueKind() != APValue::None)
+  if (E->hasAPValueResult())
     return Success(E->getAPValueResult(), E);
   return ExprEvaluatorBaseTy::VisitConstantExpr(E);
 }
----------------
This override looks equivalent to the base class version. (The only difference appears to be whether `IsConstantContext` is set during the call to `Success`, but I don't think `Success` cares about that flag.) Can you remove this override?


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1364-1365
+llvm::Constant *ConstantEmitter::tryEmitConstantExpr(const ConstantExpr *CE) {
+  if (!CE->isImmediateInvocation())
+    return nullptr;
+  const Expr *Inner = CE->getSubExpr()->IgnoreImplicit();
----------------
I'm fine with having this check for now, but eventually I think we should do this for all `ConstantExpr`s, regardless of whether they're immediate invocations.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1374
+      emitAbstract(CE->getBeginLoc(), CE->getAPValueResult(), RetType);
+  return Res;
+}
----------------
Can we assert that we succeeded here? This emission should never fail.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:464-470
+      QualType RetType = cast<CallExpr>(E->getSubExpr()->IgnoreImplicit())
+                             ->getCallReturnType(CGF.getContext());
+      if (RetType->isReferenceType()) {
+        return CGF.Builder.CreateLoad(Address(
+            Result, CGF.getContext().getTypeAlignInChars(
+                        cast<ReferenceType>(RetType)->getPointeeType())));
+      }
----------------
OK, so this is presumably handling the case where `ScalarExprEmitter` is used to emit an lvalue expression, under the understanding that when it reaches the eventual lvalue a load will be implicitly generated.

Looking for a `CallExpr` that returns a reference type is not the best way to handle this. It's brittle (it would break if `tryEmitConstantExpr` starts emitting more kinds of `ConstantExpr` or if we start supporting more kinds of immediate invocations) and we don't need to perform such a subtle check: instead, please just check whether `E` is an lvalue, and perform a load if so.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1095
+  if (const auto *EWC = dyn_cast_or_null<ExprWithCleanups>(RV)) {
+    enterFullExpression(EWC);
+    RV = EWC->getSubExpr();
----------------
Presumably it's OK to skip this for `ConstantExpr` because by definition the `ConstantExpr` node isn't tracking any block cleanups. If that's the case, should we rename this to `enterExprWithCleanups` and change it to take an `ExprWithCleanups*` rather than a `FullExpr*`?


================
Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:1
-// RUN: %clang_cc1 -std=c++2a -fsyntax-only -Wno-unused-value %s -verify
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -Wno-unused-value %s -verify > /dev/null
 
----------------
Use `-emit-llvm-only` instead of `-emit-llvm > /dev/null` if you want to make sure that emission succeeds but don't want to check the IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76420