[clang][bytecode] Allow continuing when discarded MemberExpr Base fails (llvm#107231)

tbaederr · web-flow · commit 83fea8b809b2 · 2024-09-07T09:33:27.000+02:00
We don't need the value in this case, since we're discarding it anyway.
Allow continuing the interpretation but note the side effect.
diff --git a/clang/lib/AST/ByteCode/ByteCodeEmitter.h b/clang/lib/AST/ByteCode/ByteCodeEmitter.h
@@ -46,7 +46,7 @@ class ByteCodeEmitter {
 
   /// Methods implemented by the compiler.
   virtual bool visitFunc(const FunctionDecl *E) = 0;
-  virtual bool visitExpr(const Expr *E) = 0;
+  virtual bool visitExpr(const Expr *E, bool DestroyToplevelScope) = 0;
   virtual bool visitDeclAndReturn(const VarDecl *E, bool ConstantContext) = 0;
 
   /// Emits jumps.
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -1874,8 +1874,12 @@ bool Compiler<Emitter>::VisitMemberExpr(const MemberExpr *E) {
     return false;
   }
 
-  if (!isa<FieldDecl>(Member))
-    return this->discard(Base) && this->visitDeclRef(Member, E);
+  if (!isa<FieldDecl>(Member)) {
+    if (!this->discard(Base) && !this->emitSideEffect(E))
+      return false;
+
+    return this->visitDeclRef(Member, E);
+  }
 
   if (Initializing) {
     if (!this->delegate(Base))
@@ -2673,8 +2677,14 @@ bool Compiler<Emitter>::VisitCXXConstructExpr(const CXXConstructExpr *E) {
       if (!this->emitCallVar(Func, VarArgSize, E))
         return false;
     } else {
-      if (!this->emitCall(Func, 0, E))
+      if (!this->emitCall(Func, 0, E)) {
+        // When discarding, we don't need the result anyway, so clean up
+        // the instance dup we did earlier in case surrounding code wants
+        // to keep evaluating.
+        if (DiscardResult)
+          (void)this->emitPopPtr(E);
         return false;
+      }
     }
 
     if (DiscardResult)
@@ -3723,20 +3733,29 @@ const Function *Compiler<Emitter>::getFunction(const FunctionDecl *FD) {
   return Ctx.getOrCreateFunction(FD);
 }
 
-template <class Emitter> bool Compiler<Emitter>::visitExpr(const Expr *E) {
+template <class Emitter>
+bool Compiler<Emitter>::visitExpr(const Expr *E, bool DestroyToplevelScope) {
   LocalScope<Emitter> RootScope(this);
+
+  auto maybeDestroyLocals = [&]() -> bool {
+    if (DestroyToplevelScope)
+      return RootScope.destroyLocals();
+    return true;
+  };
+
   // Void expressions.
   if (E->getType()->isVoidType()) {
     if (!visit(E))
       return false;
-    return this->emitRetVoid(E) && RootScope.destroyLocals();
+    return this->emitRetVoid(E) && maybeDestroyLocals();
   }
 
   // Expressions with a primitive return type.
   if (std::optional<PrimType> T = classify(E)) {
     if (!visit(E))
       return false;
-    return this->emitRet(*T, E) && RootScope.destroyLocals();
+
+    return this->emitRet(*T, E) && maybeDestroyLocals();
   }
 
   // Expressions with a composite return type.
@@ -3754,10 +3773,10 @@ template <class Emitter> bool Compiler<Emitter>::visitExpr(const Expr *E) {
     // We are destroying the locals AFTER the Ret op.
     // The Ret op needs to copy the (alive) values, but the
     // destructors may still turn the entire expression invalid.
-    return this->emitRetValue(E) && RootScope.destroyLocals();
+    return this->emitRetValue(E) && maybeDestroyLocals();
   }
 
-  RootScope.destroyLocals();
+  (void)maybeDestroyLocals();
   return false;
 }
 
diff --git a/clang/lib/AST/ByteCode/Compiler.h b/clang/lib/AST/ByteCode/Compiler.h
@@ -222,7 +222,7 @@ class Compiler : public ConstStmtVisitor<Compiler<Emitter>, bool>,
 
 protected:
   bool visitStmt(const Stmt *S);
-  bool visitExpr(const Expr *E) override;
+  bool visitExpr(const Expr *E, bool DestroyToplevelScope) override;
   bool visitFunc(const FunctionDecl *F) override;
 
   bool visitDeclAndReturn(const VarDecl *VD, bool ConstantContext) override;
diff --git a/clang/lib/AST/ByteCode/Context.cpp b/clang/lib/AST/ByteCode/Context.cpp
@@ -70,13 +70,16 @@ bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
   return true;
 }
 
-bool Context::evaluate(State &Parent, const Expr *E, APValue &Result) {
+bool Context::evaluate(State &Parent, const Expr *E, APValue &Result,
+                       ConstantExprKind Kind) {
   ++EvalID;
   bool Recursing = !Stk.empty();
   size_t StackSizeBefore = Stk.size();
   Compiler<EvalEmitter> C(*this, *P, Parent, Stk);
 
-  auto Res = C.interpretExpr(E);
+  auto Res = C.interpretExpr(E, /*ConvertResultToRValue=*/false,
+                             /*DestroyToplevelScope=*/Kind ==
+                                 ConstantExprKind::ClassTemplateArgument);
   if (Res.isInvalid()) {
     C.cleanup();
     Stk.clearTo(StackSizeBefore);
diff --git a/clang/lib/AST/ByteCode/Context.h b/clang/lib/AST/ByteCode/Context.h
@@ -52,7 +52,8 @@ class Context final {
   bool evaluateAsRValue(State &Parent, const Expr *E, APValue &Result);
 
   /// Like evaluateAsRvalue(), but does no implicit lvalue-to-rvalue conversion.
-  bool evaluate(State &Parent, const Expr *E, APValue &Result);
+  bool evaluate(State &Parent, const Expr *E, APValue &Result,
+                ConstantExprKind Kind);
 
   /// Evaluates a toplevel initializer.
   bool evaluateAsInitializer(State &Parent, const VarDecl *VD, APValue &Result);
diff --git a/clang/lib/AST/ByteCode/EvalEmitter.cpp b/clang/lib/AST/ByteCode/EvalEmitter.cpp
@@ -38,13 +38,14 @@ EvalEmitter::~EvalEmitter() {
 void EvalEmitter::cleanup() { S.cleanup(); }
 
 EvaluationResult EvalEmitter::interpretExpr(const Expr *E,
-                                            bool ConvertResultToRValue) {
+                                            bool ConvertResultToRValue,
+                                            bool DestroyToplevelScope) {
   S.setEvalLocation(E->getExprLoc());
   this->ConvertResultToRValue = ConvertResultToRValue && !isa<ConstantExpr>(E);
   this->CheckFullyInitialized = isa<ConstantExpr>(E);
   EvalResult.setSource(E);
 
-  if (!this->visitExpr(E)) {
+  if (!this->visitExpr(E, DestroyToplevelScope)) {
     // EvalResult may already have a result set, but something failed
     // after that (e.g. evaluating destructors).
     EvalResult.setInvalid();
diff --git a/clang/lib/AST/ByteCode/EvalEmitter.h b/clang/lib/AST/ByteCode/EvalEmitter.h
@@ -35,7 +35,8 @@ class EvalEmitter : public SourceMapper {
   using Local = Scope::Local;
 
   EvaluationResult interpretExpr(const Expr *E,
-                                 bool ConvertResultToRValue = false);
+                                 bool ConvertResultToRValue = false,
+                                 bool DestroyToplevelScope = false);
   EvaluationResult interpretDecl(const VarDecl *VD, bool CheckFullyInitialized);
 
   /// Clean up all resources.
@@ -54,7 +55,7 @@ class EvalEmitter : public SourceMapper {
   LabelTy getLabel();
 
   /// Methods implemented by the compiler.
-  virtual bool visitExpr(const Expr *E) = 0;
+  virtual bool visitExpr(const Expr *E, bool DestroyToplevelScope) = 0;
   virtual bool visitDeclAndReturn(const VarDecl *VD, bool ConstantContext) = 0;
   virtual bool visitFunc(const FunctionDecl *F) = 0;
 
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
@@ -221,17 +221,17 @@ static void popArg(InterpState &S, const Expr *Arg) {
   TYPE_SWITCH(Ty, S.Stk.discard<T>());
 }
 
-void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC) {
+void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC,
+                              const Function *Func) {
   assert(S.Current);
-  const Function *CurFunc = S.Current->getFunction();
-  assert(CurFunc);
+  assert(Func);
 
-  if (CurFunc->isUnevaluatedBuiltin())
+  if (Func->isUnevaluatedBuiltin())
     return;
 
   // Some builtin functions require us to only look at the call site, since
   // the classified parameter types do not match.
-  if (unsigned BID = CurFunc->getBuiltinID();
+  if (unsigned BID = Func->getBuiltinID();
       BID && S.getASTContext().BuiltinInfo.hasCustomTypechecking(BID)) {
     const auto *CE =
         cast<CallExpr>(S.Current->Caller->getExpr(S.Current->getRetPC()));
@@ -242,7 +242,7 @@ void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC) {
     return;
   }
 
-  if (S.Current->Caller && CurFunc->isVariadic()) {
+  if (S.Current->Caller && Func->isVariadic()) {
     // CallExpr we're look for is at the return PC of the current function, i.e.
     // in the caller.
     // This code path should be executed very rarely.
@@ -259,8 +259,8 @@ void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC) {
     } else
       assert(false && "Can't get arguments from that expression type");
 
-    assert(NumArgs >= CurFunc->getNumWrittenParams());
-    NumVarArgs = NumArgs - (CurFunc->getNumWrittenParams() +
+    assert(NumArgs >= Func->getNumWrittenParams());
+    NumVarArgs = NumArgs - (Func->getNumWrittenParams() +
                             isa<CXXOperatorCallExpr>(CallSite));
     for (unsigned I = 0; I != NumVarArgs; ++I) {
       const Expr *A = Args[NumArgs - 1 - I];
@@ -270,7 +270,8 @@ void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC) {
 
   // And in any case, remove the fixed parameters (the non-variadic ones)
   // at the end.
-  S.Current->popArgs();
+  for (PrimType Ty : Func->args_reverse())
+    TYPE_SWITCH(Ty, S.Stk.discard<T>());
 }
 
 bool CheckExtern(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
@@ -1036,6 +1037,12 @@ bool CallVar(InterpState &S, CodePtr OpPC, const Function *Func,
 
 bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
           uint32_t VarArgSize) {
+  assert(Func);
+  auto cleanup = [&]() -> bool {
+    cleanupAfterFunctionCall(S, OpPC, Func);
+    return false;
+  };
+
   if (Func->hasThisPointer()) {
     size_t ArgSize = Func->getArgSize() + VarArgSize;
     size_t ThisOffset = ArgSize - (Func->hasRVO() ? primSize(PT_Ptr) : 0);
@@ -1052,22 +1059,22 @@ bool Call(InterpState &S, CodePtr OpPC, const Function *Func,
       assert(ThisPtr.isZero());
     } else {
       if (!CheckInvoke(S, OpPC, ThisPtr))
-        return false;
+        return cleanup();
     }
   }
 
   if (!CheckCallable(S, OpPC, Func))
-    return false;
+    return cleanup();
 
   // FIXME: The isConstructor() check here is not always right. The current
   // constant evaluator is somewhat inconsistent in when it allows a function
   // call when checking for a constant expression.
   if (Func->hasThisPointer() && S.checkingPotentialConstantExpression() &&
       !Func->isConstructor())
-    return false;
+    return cleanup();
 
   if (!CheckCallDepth(S, OpPC))
-    return false;
+    return cleanup();
 
   auto NewFrame = std::make_unique<InterpFrame>(S, Func, OpPC, VarArgSize);
   InterpFrame *FrameBefore = S.Current;
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
@@ -281,7 +281,8 @@ enum class ArithOp { Add, Sub };
 // Returning values
 //===----------------------------------------------------------------------===//
 
-void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC);
+void cleanupAfterFunctionCall(InterpState &S, CodePtr OpPC,
+                              const Function *Func);
 
 template <PrimType Name, class T = typename PrimConv<Name>::T>
 bool Ret(InterpState &S, CodePtr &PC, APValue &Result) {
@@ -302,7 +303,7 @@ bool Ret(InterpState &S, CodePtr &PC, APValue &Result) {
   assert(S.Current);
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller)
-    cleanupAfterFunctionCall(S, PC);
+    cleanupAfterFunctionCall(S, PC, S.Current->getFunction());
 
   if (InterpFrame *Caller = S.Current->Caller) {
     PC = S.Current->getRetPC();
@@ -322,7 +323,7 @@ inline bool RetVoid(InterpState &S, CodePtr &PC, APValue &Result) {
   assert(S.Current->getFrameOffset() == S.Stk.size() && "Invalid frame");
 
   if (!S.checkingPotentialConstantExpression() || S.Current->Caller)
-    cleanupAfterFunctionCall(S, PC);
+    cleanupAfterFunctionCall(S, PC, S.Current->getFunction());
 
   if (InterpFrame *Caller = S.Current->Caller) {
     PC = S.Current->getRetPC();
@@ -2658,6 +2659,9 @@ inline bool Unsupported(InterpState &S, CodePtr OpPC) {
 
 /// Do nothing and just abort execution.
 inline bool Error(InterpState &S, CodePtr OpPC) { return false; }
+inline bool SideEffect(InterpState &S, CodePtr OpPC) {
+  return S.noteSideEffect();
+}
 
 /// Same here, but only for casts.
 inline bool InvalidCast(InterpState &S, CodePtr OpPC, CastKind Kind,
diff --git a/clang/lib/AST/ByteCode/InterpFrame.cpp b/clang/lib/AST/ByteCode/InterpFrame.cpp
@@ -96,11 +96,6 @@ void InterpFrame::destroy(unsigned Idx) {
   }
 }
 
-void InterpFrame::popArgs() {
-  for (PrimType Ty : Func->args_reverse())
-    TYPE_SWITCH(Ty, S.Stk.discard<T>());
-}
-
 template <typename T>
 static void print(llvm::raw_ostream &OS, const T &V, ASTContext &ASTCtx,
                   QualType Ty) {
diff --git a/clang/lib/AST/ByteCode/InterpFrame.h b/clang/lib/AST/ByteCode/InterpFrame.h
@@ -46,9 +46,6 @@ class InterpFrame final : public Frame {
   void destroy(unsigned Idx);
   void initScope(unsigned Idx);
 
-  /// Pops the arguments off the stack.
-  void popArgs();
-
   /// Describes the frame with arguments for diagnostic purposes.
   void describe(llvm::raw_ostream &OS) const override;
 
diff --git a/clang/lib/AST/ByteCode/InterpState.h b/clang/lib/AST/ByteCode/InterpState.h
@@ -68,6 +68,9 @@ class InterpState final : public State, public SourceMapper {
   bool keepEvaluatingAfterFailure() const override {
     return Parent.keepEvaluatingAfterFailure();
   }
+  bool keepEvaluatingAfterSideEffect() const override {
+    return Parent.keepEvaluatingAfterSideEffect();
+  }
   bool checkingPotentialConstantExpression() const override {
     return Parent.checkingPotentialConstantExpression();
   }
@@ -83,6 +86,7 @@ class InterpState final : public State, public SourceMapper {
     Parent.setFoldFailureDiagnostic(Flag);
   }
   bool hasPriorDiagnostic() override { return Parent.hasPriorDiagnostic(); }
+  bool noteSideEffect() override { return Parent.noteSideEffect(); }
 
   /// Reports overflow and return true if evaluation should continue.
   bool reportOverflow(const Expr *E, const llvm::APSInt &Value);
diff --git a/clang/lib/AST/ByteCode/Opcodes.td b/clang/lib/AST/ByteCode/Opcodes.td
@@ -732,6 +732,7 @@ def Flip : Opcode {
 def Invalid : Opcode {}
 def Unsupported : Opcode {}
 def Error : Opcode {}
+def SideEffect : Opcode {}
 def InvalidCast : Opcode {
   let Args = [ArgCastKind, ArgBool];
 }
diff --git a/clang/lib/AST/ByteCode/State.h b/clang/lib/AST/ByteCode/State.h
@@ -62,6 +62,7 @@ class State {
   virtual bool checkingPotentialConstantExpression() const = 0;
   virtual bool noteUndefinedBehavior() = 0;
   virtual bool keepEvaluatingAfterFailure() const = 0;
+  virtual bool keepEvaluatingAfterSideEffect() const = 0;
   virtual Frame *getCurrentFrame() = 0;
   virtual const Frame *getBottomFrame() const = 0;
   virtual bool hasActiveDiagnostic() = 0;
@@ -71,6 +72,7 @@ class State {
   virtual ASTContext &getASTContext() const = 0;
   virtual bool hasPriorDiagnostic() = 0;
   virtual unsigned getCallStackDepth() = 0;
+  virtual bool noteSideEffect() = 0;
 
 public:
   State() = default;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
@@ -1222,7 +1222,7 @@ namespace {
   public:
     /// Should we continue evaluation after encountering a side-effect that we
     /// couldn't model?
-    bool keepEvaluatingAfterSideEffect() {
+    bool keepEvaluatingAfterSideEffect() const override {
       switch (EvalMode) {
       case EM_IgnoreSideEffects:
         return true;
@@ -1240,7 +1240,7 @@ namespace {
 
     /// Note that we have had a side-effect, and determine whether we should
     /// keep evaluating.
-    bool noteSideEffect() {
+    bool noteSideEffect() override {
       EvalStatus.HasSideEffects = true;
       return keepEvaluatingAfterSideEffect();
     }
@@ -16281,7 +16281,7 @@ bool Expr::EvaluateAsConstantExpr(EvalResult &Result, const ASTContext &Ctx,
   Info.InConstantContext = true;
 
   if (Info.EnableNewConstInterp) {
-    if (!Info.Ctx.getInterpContext().evaluate(Info, this, Result.Val))
+    if (!Info.Ctx.getInterpContext().evaluate(Info, this, Result.Val, Kind))
       return false;
     return CheckConstantExpression(Info, getExprLoc(),
                                    getStorageType(Ctx, this), Result.Val, Kind);
diff --git a/clang/test/AST/ByteCode/codegen.cpp b/clang/test/AST/ByteCode/codegen.cpp

Original file line number	Diff line number	Diff line change
`@@ -96,11 +96,6 @@ void InterpFrame::destroy(unsigned Idx) {`
`96`	`96`	`}`
`97`	`97`	`}`
`98`	`98`
`99`		`-void InterpFrame::popArgs() {`
`100`		`- for (PrimType Ty : Func->args_reverse())`
`101`		`- TYPE_SWITCH(Ty, S.Stk.discard<T>());`
`102`		`-}`
`103`		`-`
`104`	`99`	`template <typename T>`
`105`	`100`	`static void print(llvm::raw_ostream &OS, const T &V, ASTContext &ASTCtx,`
`106`	`101`	`QualType Ty) {`