ConstantExpr cached APValues if present for constant evaluation

[wchilders]

Summary

This patch allows the constant evaluator to use APValueResults from ConstantExprs.

There are some outstanding concerns I'll mark with inline comments.

[wchilders]

clang/lib/AST/Expr.cpp

2727

IgnoreParensSingleStep for some reason has been unwrapping ConstantExprs. This results in the constant evaluator, removing the ConstantExpr, and reevaluating the expression. There are no observed downsides to removing this condition, in the test suite, however, it's strange enough to note.

clang/lib/AST/ExprConstant.cpp

7354

This doesn't seem to be the right answer, and ConstantExprs don't have LValue APValues, at least, not that are reaching this case. We had a previous implementation that also, kind of punted on this issue with an override in TemporaryExprEvaluator: https://gitlab.com/lock3/clang/-/blob/9fbaeea06fc567ac472264bec2a72661a1e06c73/clang/lib/AST/ExprConstant.cpp#L9753

[rsmith]

clang/lib/AST/Expr.cpp

2727

We sadly don't really have any good, rigorous definitions of what the various Ignore* functions do. But the general idea of IgnoreParens is to ignore syntax with no semantic effect, and ConstantExpr doesn't describe syntax, so it probably shouldn't be stepped over here. (It is ignored by IgnoreImplicit, which is generally -- mostly -- trying to ignore semantics with no syntactic representation, which seems appropriate. It might make sense for IgnoreImpCasts to not step over FullExprs, but I expect that change would break some testcases.)

clang/lib/AST/ExprConstant.cpp

7354

The base class version seems appropriate to me, even for this case. We eventually want to use ConstantExpr to store the evaluated initializer value of a constexpr variable (replacing the existing special-case caching on VarDecl) and the like, not only for immediate invocations, and once we start doing that for reference variables we'll have glvalue ConstantExprs.

Is there some circumstance under which a glvalue ConstantExpr's APValue result is not of kind LValue?

[wchilders]

clang/lib/AST/ExprConstant.cpp

7354

Is there some circumstance under which a glvalue ConstantExpr's APValue result is not of kind LValue?

Good question, the Sema/switch.c test fails if you assert(Result.isLValue()).

ConstantExpr 0x555562ab8760 'const int' lvalue Int: 3
`-DeclRefExpr 0x555562ab8740 'const int' lvalue Var 0x555562ab8230 'a' 'const int'

With an attributed line no. 359: https://github.com/llvm/llvm-project/blob/4b0029995853fe37d1dc95ef96f46697c743fcad/clang/test/Sema/switch.c#L359

The offending RValue is created in SemaExpr.cpp here: https://github.com/llvm/llvm-project/blob/f87563661d6661fd4843beb8f391acd077b08dbe/clang/lib/Sema/SemaExpr.cpp#L15190

The issue stems from evaluating this as an RValue to produce an integer, then caching that RValue in an lvalue constant expression. Do you have any suggestions? Perhaps an LValue to RValue conversion should be performed on the expression if it folds correctly, so that the ConstantExpr is actually an RValue?

[rsmith]

clang/lib/AST/ExprConstant.cpp

7354

I think we should be inserting the lvalue-to-rvalue conversion before we even try evaluating the expression. Does this go wrong along both the C++11 and pre-C++11 codepaths? (They do rather different conversions to the expression.) In any case, we're likely missing a call to DefaultLvalueConversion on whichever side is failing.

Judging by the fact that struct X { void f() { switch (0) case f: ; } }; misses the "non-static member function must be called" diagnostic only in C++98 mode, I'd imagine it's just the pre-C++11 codepath that's broken here.

[wchilders]

Updated to assume LValue ConstantExprs have LValue APValues, with verification in debug mode. Additionally, added a missing LValue -> RValue conversion for VerifyIntegerConstantExpression.

[wchilders]

clang/lib/AST/ExprConstant.cpp

7354

This is a C test, the C++ language level is actually irrelevant as best I can tell. I added the missing LValue -> RValue conversion so that the APValue is generated correctly. With that things function as you'd expect, I added an assertion to the LValue base evaluator to detect this case early in development, otherwise, the logic is now shared across evaluator classes.

[rsmith]

clang/lib/AST/ExprConstant.cpp

7350–7354

I think this override is now fully redundant and can be removed: the isLValue() assert is reached anyway when DerivedSuccess calls LValueExprEvaluatorBase::Success which calls LValue::setFrom.

[wchilders]

Dropped the override for constexpr evaluator, LValue evaluation base class

[wchilders]

clang/lib/AST/ExprConstant.cpp

7350–7354

Good catch, updated the patch to drop this. :)

[wchilders]

I should note, I don't have commit access, so I'm unable to commit this myself. Attribution would be Wyatt Childers <[e-mail]>

clang/include/clang/AST/Expr.h

APValue::ValueKind getResultAPValueKind() const {
     return static_cast<APValue::ValueKind>(ConstantExprBits.APValueKind);
   }
   ResultStorageKind getResultStorageKind() const {
     return static_cast<ResultStorageKind>(ConstantExprBits.ResultKind);
   }
   bool isImmediateInvocation() const {
     return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+    return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;
   // Iterators
   child_range children() { return child_range(&SubExpr, &SubExpr+1); }
   const_child_range children() const {
     return const_child_range(&SubExpr, &SubExpr + 1);
   }

clang/lib/AST/Expr.cpp

case ConstantExpr::RSK_Int64:
     return llvm::APSInt(llvm::APInt(ConstantExprBits.BitWidth, Int64Result()),
                         ConstantExprBits.IsUnsigned);
   default:
     llvm_unreachable("invalid Accessor");
   }
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
     return APValueResult();
   case ConstantExpr::RSK_Int64:
     return APValue(
         llvm::APSInt(llvm::APInt(ConstantExprBits.BitWidth, Int64Result()),
                      ConstantExprBits.IsUnsigned));
   case ConstantExpr::RSK_None:
if (!GSE->isResultDependent())
       return GSE->getResultExpr();
   }
 
   else if (auto *CE = dyn_cast<ChooseExpr>(E)) {
     if (!CE->isConditionDependent())
       return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast<ConstantExpr>(E))
-    return CE->getSubExpr();
-
   return E;
 }
 
 static Expr *IgnoreNoopCastsSingleStep(const ASTContext &Ctx, Expr *E) {
   if (auto *CE = dyn_cast<CastExpr>(E)) {
     // We ignore integer <-> casts that are of the same width, ptr<->ptr and
     // ptr<->int casts of the same width. We also ignore all identity casts.
     Expr *SubExpr = CE->getSubExpr();
     bool IsIdentityCast =

clang/lib/AST/ExprConstant.cpp

public:
 
   bool VisitStmt(const Stmt *) {
     llvm_unreachable("Expression evaluator should not be called on stmts");
   }
   bool VisitExpr(const Expr *E) {
     return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-    { return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+    if (E->hasAPValueResult())
+      return DerivedSuccess(E->getAPValueResult(), E);
+
+    return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
     { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
     { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryPlus(const UnaryOperator *E)
     { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitChooseExpr(const ChooseExpr *E)
     { return StmtVisitorTy::Visit(E->getChosenSubExpr()); }
LValueExprEvaluatorBase(EvalInfo &Info, LValue &Result, bool InvalidBaseOK)
       : ExprEvaluatorBaseTy(Info), Result(Result),
         InvalidBaseOK(InvalidBaseOK) {}
 
   bool Success(const APValue &V, const Expr *E) {
     Result.setFrom(this->Info.Ctx, V);
     return true;
   }
 
   bool VisitMemberExpr(const MemberExpr *E) {
     // Handle non-static data members.
     QualType BaseTy;
     bool EvalOK;
     if (E->isArrow()) {
       EvalOK = evaluatePointer(E->getBase(), Result);
       BaseTy = E->getBase()->getType()->castAs<PointerType>()->getPointeeType();
     } else if (E->getBase()->isRValue()) {
       assert(E->getBase()->getType()->isRecordType());
       EvalOK = EvaluateTemporary(E->getBase(), Result, this->Info);
       BaseTy = E->getBase()->getType();
     } else {
       EvalOK = this->Visit(E->getBase());

clang/lib/Sema/SemaExpr.cpp

if (!E->getType()->isIntegralOrUnscopedEnumerationType())
       return ExprError();
   } else if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
     // An ICE must be of integral or unscoped enumeration type.
     if (!Diagnoser.Suppress)
       Diagnoser.diagnoseNotICE(*this, DiagLoc, E->getSourceRange());
     return ExprError();
   }
 
+  ExprResult RValueExpr = DefaultLvalueConversion(E);
+  if (RValueExpr.isInvalid())
+    return ExprError();
+
+  E = RValueExpr.get();
+
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
     if (Result)
       *Result = E->EvaluateKnownConstIntCheckOverflow(Context);
     if (!isa<ConstantExpr>(E))
       E = ConstantExpr::Create(Context, E);
     return E;