Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][Interp] Add an EvaluationResult class #71315

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

tbaederr
Copy link
Contributor

@tbaederr tbaederr commented Nov 5, 2023

Add an EvaluationResult class. This contains the result either as a Pointer or as a APValue.

This way, we can inspect the result of the evaluation and diagnose problems with it (e.g. uninitialized fields in global initializers or pointers pointing to things they shouldn't point to).

Other changes:

  1. Move the code from EvalEmitter::emitRetValue to Pointer::toRValue.
  2. To the implicit lvalue-to-rvalue ourselves instead of doing it in ExprConstant.cpp.
  3. Remove the awkward CheckGlobalCtor opcode and do it in evaluateAsInitializer instead.
  4. Add a new Context::evaluate() function which is similar to `evaluateAsRValue() but without the lvalue-to-rvalue conversion.
  5. This patch also contains changes from [clang] Use new interpreter in EvaluateAsConstantExpr if requested #70763
  6. To make the changes in EvalEmitter::interpret{Expr,Decl} easier, this also removes the BailLocation handling. It was untested anyway and I we would towards never needing it anyway.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 5, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 5, 2023

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Add an EvaluationResult class. This contains the result either as a Pointer or as a APValue.

This way, we can inspect the result of the evaluation and diagnose problems with it (e.g. uninitialized fields in global initializers or pointers pointing to things they shouldn't point to).

Other changes:

  1. Move the code from EvalEmitter::emitRetValue to Pointer::toRValue.
  2. To the implicit lvalue-to-rvalue ourselves instead of doing it in ExprConstant.cpp.
  3. Remove the awkward CheckGlobalCtor opcode and do it in evaluateAsInitializer instead.
  4. Add a new Context::evaluate() function which is similar to `evaluateAsRValue() but without the lvalue-to-rvalue conversion.
  5. This patch also contains changes from [clang] Use new interpreter in EvaluateAsConstantExpr if requested #70763
  6. To make the changes in EvalEmitter::interpret{Expr,Decl} easier, this also removes the BailLocation handling. It was untested anyway and I we would towards never needing it anyway.

Patch is 43.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71315.diff

19 Files Affected:

  • (modified) clang/lib/AST/CMakeLists.txt (+1)
  • (modified) clang/lib/AST/ExprConstant.cpp (+20-7)
  • (modified) clang/lib/AST/Interp/ByteCodeEmitter.cpp (+2-11)
  • (modified) clang/lib/AST/Interp/ByteCodeEmitter.h (+1-4)
  • (modified) clang/lib/AST/Interp/ByteCodeExprGen.cpp (+2-1)
  • (modified) clang/lib/AST/Interp/ByteCodeExprGen.h (+7-5)
  • (modified) clang/lib/AST/Interp/Context.cpp (+73-33)
  • (modified) clang/lib/AST/Interp/Context.h (+3)
  • (modified) clang/lib/AST/Interp/EvalEmitter.cpp (+42-111)
  • (modified) clang/lib/AST/Interp/EvalEmitter.h (+6-5)
  • (added) clang/lib/AST/Interp/EvaluationResult.cpp (+196)
  • (added) clang/lib/AST/Interp/EvaluationResult.h (+111)
  • (modified) clang/lib/AST/Interp/Interp.cpp (-92)
  • (modified) clang/lib/AST/Interp/Interp.h (+6-10)
  • (modified) clang/lib/AST/Interp/Opcodes.td (-2)
  • (modified) clang/lib/AST/Interp/Pointer.cpp (+110-22)
  • (modified) clang/lib/AST/Interp/Pointer.h (+2-1)
  • (modified) clang/test/AST/Interp/literals.cpp (+1-3)
  • (modified) clang/test/AST/Interp/records.cpp (-1)
diff --git a/clang/lib/AST/CMakeLists.txt b/clang/lib/AST/CMakeLists.txt
index fe3f8c485ec1c56..ebcb3952198a5b5 100644
--- a/clang/lib/AST/CMakeLists.txt
+++ b/clang/lib/AST/CMakeLists.txt
@@ -75,6 +75,7 @@ add_clang_library(clangAST
   Interp/Function.cpp
   Interp/InterpBuiltin.cpp
   Interp/Floating.cpp
+  Interp/EvaluationResult.cpp
   Interp/Interp.cpp
   Interp/InterpBlock.cpp
   Interp/InterpFrame.cpp
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index b6b1e6617dffaa9..d6e223e77d6f1c3 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -15418,11 +15418,13 @@ static bool EvaluateAsRValue(EvalInfo &Info, const Expr *E, APValue &Result) {
   if (Info.EnableNewConstInterp) {
     if (!Info.Ctx.getInterpContext().evaluateAsRValue(Info, E, Result))
       return false;
-  } else {
-    if (!::Evaluate(Result, Info, E))
-      return false;
+    return CheckConstantExpression(Info, E->getExprLoc(), E->getType(), Result,
+                                   ConstantExprKind::Normal);
   }
 
+  if (!::Evaluate(Result, Info, E))
+    return false;
+
   // Implicit lvalue-to-rvalue cast.
   if (E->isGLValue()) {
     LValue LV;
@@ -15650,6 +15652,13 @@ bool Expr::EvaluateAsConstantExpr(EvalResult &Result, const ASTContext &Ctx,
   EvalInfo Info(Ctx, Result, EM);
   Info.InConstantContext = true;
 
+  if (Info.EnableNewConstInterp) {
+    if (!Info.Ctx.getInterpContext().evaluate(Info, this, Result.Val))
+      return false;
+    return CheckConstantExpression(Info, getExprLoc(),
+                                   getStorageType(Ctx, this), Result.Val, Kind);
+  }
+
   // The type of the object we're initializing is 'const T' for a class NTTP.
   QualType T = getType();
   if (Kind == ConstantExprKind::ClassTemplateArgument)
@@ -15662,10 +15671,10 @@ bool Expr::EvaluateAsConstantExpr(EvalResult &Result, const ASTContext &Ctx,
   APValue::LValueBase Base(&BaseMTE);
 
   Info.setEvaluatingDecl(Base, Result.Val);
-  LValue LVal;
-  LVal.set(Base);
 
   {
+    LValue LVal;
+    LVal.set(Base);
     // C++23 [intro.execution]/p5
     // A full-expression is [...] a constant-expression
     // So we need to make sure temporary objects are destroyed after having
@@ -15723,10 +15732,16 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
   Info.setEvaluatingDecl(VD, Value);
   Info.InConstantContext = IsConstantInitialization;
 
+  SourceLocation DeclLoc = VD->getLocation();
+  QualType DeclTy = VD->getType();
+
   if (Info.EnableNewConstInterp) {
     auto &InterpCtx = const_cast<ASTContext &>(Ctx).getInterpContext();
     if (!InterpCtx.evaluateAsInitializer(Info, VD, Value))
       return false;
+
+    return CheckConstantExpression(Info, DeclLoc, DeclTy, Value,
+                                   ConstantExprKind::Normal);
   } else {
     LValue LVal;
     LVal.set(VD);
@@ -15744,8 +15759,6 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
       llvm_unreachable("Unhandled cleanup; missing full expression marker?");
   }
 
-  SourceLocation DeclLoc = VD->getLocation();
-  QualType DeclTy = VD->getType();
   return CheckConstantExpression(Info, DeclLoc, DeclTy, Value,
                                  ConstantExprKind::Normal) &&
          CheckMemoryLeaks(Info);
diff --git a/clang/lib/AST/Interp/ByteCodeEmitter.cpp b/clang/lib/AST/Interp/ByteCodeEmitter.cpp
index c8abb7c17a38ba2..d3a9583bee59e59 100644
--- a/clang/lib/AST/Interp/ByteCodeEmitter.cpp
+++ b/clang/lib/AST/Interp/ByteCodeEmitter.cpp
@@ -19,8 +19,7 @@
 using namespace clang;
 using namespace clang::interp;
 
-Expected<Function *>
-ByteCodeEmitter::compileFunc(const FunctionDecl *FuncDecl) {
+Function *ByteCodeEmitter::compileFunc(const FunctionDecl *FuncDecl) {
   // Set up argument indices.
   unsigned ParamOffset = 0;
   SmallVector<PrimType, 8> ParamTypes;
@@ -108,10 +107,6 @@ ByteCodeEmitter::compileFunc(const FunctionDecl *FuncDecl) {
 
   // Compile the function body.
   if (!IsEligibleForCompilation || !visitFunc(FuncDecl)) {
-    // Return a dummy function if compilation failed.
-    if (BailLocation)
-      return llvm::make_error<ByteCodeGenError>(*BailLocation);
-
     Func->setIsFullyCompiled(true);
     return Func;
   }
@@ -171,11 +166,7 @@ int32_t ByteCodeEmitter::getOffset(LabelTy Label) {
   return 0ull;
 }
 
-bool ByteCodeEmitter::bail(const SourceLocation &Loc) {
-  if (!BailLocation)
-    BailLocation = Loc;
-  return false;
-}
+bool ByteCodeEmitter::bail(const SourceLocation &Loc) { return false; }
 
 /// Helper to write bytecode and bail out if 32-bit offsets become invalid.
 /// Pointers will be automatically marshalled as 32-bit IDs.
diff --git a/clang/lib/AST/Interp/ByteCodeEmitter.h b/clang/lib/AST/Interp/ByteCodeEmitter.h
index 5520f8c3006106f..4ef36d784dc4d59 100644
--- a/clang/lib/AST/Interp/ByteCodeEmitter.h
+++ b/clang/lib/AST/Interp/ByteCodeEmitter.h
@@ -17,7 +17,6 @@
 #include "PrimType.h"
 #include "Program.h"
 #include "Source.h"
-#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace interp {
@@ -32,7 +31,7 @@ class ByteCodeEmitter {
 
 public:
   /// Compiles the function into the module.
-  llvm::Expected<Function *> compileFunc(const FunctionDecl *FuncDecl);
+  Function *compileFunc(const FunctionDecl *FuncDecl);
 
 protected:
   ByteCodeEmitter(Context &Ctx, Program &P) : Ctx(Ctx), P(P) {}
@@ -81,8 +80,6 @@ class ByteCodeEmitter {
   LabelTy NextLabel = 0;
   /// Offset of the next local variable.
   unsigned NextLocalOffset = 0;
-  /// Location of a failure.
-  std::optional<SourceLocation> BailLocation;
   /// Label information for linker.
   llvm::DenseMap<LabelTy, unsigned> LabelOffsets;
   /// Location of label relocations.
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 485893d58f487ae..a27293566c26368 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -2143,7 +2143,8 @@ bool ByteCodeExprGen<Emitter>::visitDecl(const VarDecl *VD) {
     return this->emitRet(*VarT, VD);
   }
 
-  return this->emitRetValue(VD);
+  // Return non-primitive values as pointers here.
+  return this->emitRet(PT_Ptr, VD);
 }
 
 template <class Emitter>
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 83986d3dd579ed6..4d9cd84b714247e 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -129,7 +129,13 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
 
   /// Classifies a type.
   std::optional<PrimType> classify(const Expr *E) const {
-    return E->isGLValue() ? PT_Ptr : classify(E->getType());
+    if (E->isGLValue()) {
+      if (E->getType()->isFunctionType())
+        return PT_FnPtr;
+      return PT_Ptr;
+    }
+
+    return classify(E->getType());
   }
   std::optional<PrimType> classify(QualType Ty) const {
     return Ctx.classify(Ty);
@@ -184,10 +190,6 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
     if (!visitInitializer(Init))
       return false;
 
-    if ((Init->getType()->isArrayType() || Init->getType()->isRecordType()) &&
-        !this->emitCheckGlobalCtor(Init))
-      return false;
-
     return this->emitPopPtr(Init);
   }
 
diff --git a/clang/lib/AST/Interp/Context.cpp b/clang/lib/AST/Interp/Context.cpp
index cb96e56fb5e1ad8..b1b0a5d101b3767 100644
--- a/clang/lib/AST/Interp/Context.cpp
+++ b/clang/lib/AST/Interp/Context.cpp
@@ -30,18 +30,8 @@ Context::~Context() {}
 bool Context::isPotentialConstantExpr(State &Parent, const FunctionDecl *FD) {
   assert(Stk.empty());
   Function *Func = P->getFunction(FD);
-  if (!Func || !Func->hasBody()) {
-    if (auto R = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD)) {
-      Func = *R;
-    } else {
-      handleAllErrors(R.takeError(), [&Parent](ByteCodeGenError &Err) {
-        Parent.FFDiag(Err.getRange().getBegin(),
-                      diag::err_experimental_clang_interp_failed)
-            << Err.getRange();
-      });
-      return false;
-    }
-  }
+  if (!Func || !Func->hasBody())
+    Func = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD);
 
   APValue DummyResult;
   if (!Run(Parent, Func, DummyResult)) {
@@ -54,36 +44,90 @@ bool Context::isPotentialConstantExpr(State &Parent, const FunctionDecl *FD) {
 bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
   assert(Stk.empty());
   ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
-  if (Check(Parent, C.interpretExpr(E))) {
-    assert(Stk.empty());
-#ifndef NDEBUG
-    // Make sure we don't rely on some value being still alive in
-    // InterpStack memory.
+
+  auto Res = C.interpretExpr(E);
+
+  if (Res.isInvalid()) {
     Stk.clear();
+    return false;
+  }
+
+  assert(Stk.empty());
+#ifndef NDEBUG
+  // Make sure we don't rely on some value being still alive in
+  // InterpStack memory.
+  Stk.clear();
 #endif
-    return true;
+
+  // Implicit lvalue-to-rvalue conversion.
+  if (E->isGLValue()) {
+    std::optional<APValue> RValueResult = Res.toRValue();
+    if (!RValueResult) {
+      return false;
+    }
+    Result = *RValueResult;
+  } else {
+    Result = Res.toAPValue();
   }
 
+  return true;
+}
+
+bool Context::evaluate(State &Parent, const Expr *E, APValue &Result) {
+  assert(Stk.empty());
+  ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
+
+  auto Res = C.interpretExpr(E);
+  if (Res.isInvalid()) {
+    Stk.clear();
+    return false;
+  }
+
+  assert(Stk.empty());
+#ifndef NDEBUG
+  // Make sure we don't rely on some value being still alive in
+  // InterpStack memory.
   Stk.clear();
-  return false;
+#endif
+  Result = Res.toAPValue();
+  return true;
 }
 
 bool Context::evaluateAsInitializer(State &Parent, const VarDecl *VD,
                                     APValue &Result) {
   assert(Stk.empty());
   ByteCodeExprGen<EvalEmitter> C(*this, *P, Parent, Stk, Result);
-  if (Check(Parent, C.interpretDecl(VD))) {
-    assert(Stk.empty());
-#ifndef NDEBUG
-    // Make sure we don't rely on some value being still alive in
-    // InterpStack memory.
+
+  auto Res = C.interpretDecl(VD);
+  if (Res.isInvalid()) {
     Stk.clear();
-#endif
-    return true;
+    return false;
   }
 
+  assert(Stk.empty());
+#ifndef NDEBUG
+  // Make sure we don't rely on some value being still alive in
+  // InterpStack memory.
   Stk.clear();
-  return false;
+#endif
+
+  // Ensure global variables are fully initialized.
+  if (shouldBeGloballyIndexed(VD) && !Res.isInvalid() &&
+      (VD->getType()->isRecordType() || VD->getType()->isArrayType())) {
+    assert(Res.isLValue());
+
+    if (!Res.checkFullyInitialized(C.getState()))
+      return false;
+
+    // lvalue-to-rvalue conversion.
+    std::optional<APValue> RValueResult = Res.toRValue();
+    if (!RValueResult)
+      return false;
+    Result = *RValueResult;
+
+  } else
+    Result = Res.toAPValue();
+  return true;
 }
 
 const LangOptions &Context::getLangOpts() const { return Ctx.getLangOpts(); }
@@ -231,12 +275,8 @@ const Function *Context::getOrCreateFunction(const FunctionDecl *FD) {
     return Func;
 
   if (!Func || WasNotDefined) {
-    if (auto R = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD))
-      Func = *R;
-    else {
-      llvm::consumeError(R.takeError());
-      return nullptr;
-    }
+    if (auto F = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD))
+      Func = F;
   }
 
   return Func;
diff --git a/clang/lib/AST/Interp/Context.h b/clang/lib/AST/Interp/Context.h
index 7649caef2242816..ab83a8d13224670 100644
--- a/clang/lib/AST/Interp/Context.h
+++ b/clang/lib/AST/Interp/Context.h
@@ -51,6 +51,9 @@ class Context final {
   /// Evaluates a toplevel expression as an rvalue.
   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);
+
   /// Evaluates a toplevel initializer.
   bool evaluateAsInitializer(State &Parent, const VarDecl *VD, APValue &Result);
 
diff --git a/clang/lib/AST/Interp/EvalEmitter.cpp b/clang/lib/AST/Interp/EvalEmitter.cpp
index 9bc42057c5f5782..458fd7a21754672 100644
--- a/clang/lib/AST/Interp/EvalEmitter.cpp
+++ b/clang/lib/AST/Interp/EvalEmitter.cpp
@@ -19,7 +19,7 @@ using namespace clang::interp;
 
 EvalEmitter::EvalEmitter(Context &Ctx, Program &P, State &Parent,
                          InterpStack &Stk, APValue &Result)
-    : Ctx(Ctx), P(P), S(Parent, P, Stk, Ctx, this), Result(Result) {
+    : Ctx(Ctx), P(P), S(Parent, P, Stk, Ctx, this), EvalResult(&Ctx) {
   // Create a dummy frame for the interpreter which does not have locals.
   S.Current =
       new InterpFrame(S, /*Func=*/nullptr, /*Caller=*/nullptr, CodePtr());
@@ -33,20 +33,22 @@ EvalEmitter::~EvalEmitter() {
   }
 }
 
-llvm::Expected<bool> EvalEmitter::interpretExpr(const Expr *E) {
-  if (this->visitExpr(E))
-    return true;
-  if (BailLocation)
-    return llvm::make_error<ByteCodeGenError>(*BailLocation);
-  return false;
+EvaluationResult EvalEmitter::interpretExpr(const Expr *E) {
+  EvalResult.setSource(E);
+
+  if (!this->visitExpr(E))
+    EvalResult.setInvalid();
+
+  return std::move(this->EvalResult);
 }
 
-llvm::Expected<bool> EvalEmitter::interpretDecl(const VarDecl *VD) {
-  if (this->visitDecl(VD))
-    return true;
-  if (BailLocation)
-    return llvm::make_error<ByteCodeGenError>(*BailLocation);
-  return false;
+EvaluationResult EvalEmitter::interpretDecl(const VarDecl *VD) {
+  EvalResult.setSource(VD);
+
+  if (!this->visitDecl(VD))
+    EvalResult.setInvalid();
+
+  return std::move(this->EvalResult);
 }
 
 void EvalEmitter::emitLabel(LabelTy Label) {
@@ -77,11 +79,7 @@ Scope::Local EvalEmitter::createLocal(Descriptor *D) {
   return {Off, D};
 }
 
-bool EvalEmitter::bail(const SourceLocation &Loc) {
-  if (!BailLocation)
-    BailLocation = Loc;
-  return false;
-}
+bool EvalEmitter::bail(const SourceLocation &Loc) { return false; }
 
 bool EvalEmitter::jumpTrue(const LabelTy &Label) {
   if (isActive()) {
@@ -116,104 +114,37 @@ template <PrimType OpType> bool EvalEmitter::emitRet(const SourceInfo &Info) {
   if (!isActive())
     return true;
   using T = typename PrimConv<OpType>::T;
-  return ReturnValue<T>(S.Stk.pop<T>(), Result);
+  EvalResult.setValue(S.Stk.pop<T>().toAPValue());
+  return true;
+}
+
+template <> bool EvalEmitter::emitRet<PT_Ptr>(const SourceInfo &Info) {
+  if (!isActive())
+    return true;
+  EvalResult.setPointer(S.Stk.pop<Pointer>());
+  return true;
+}
+template <> bool EvalEmitter::emitRet<PT_FnPtr>(const SourceInfo &Info) {
+  if (!isActive())
+    return true;
+  EvalResult.setFunctionPointer(S.Stk.pop<FunctionPointer>());
+  return true;
 }
 
-bool EvalEmitter::emitRetVoid(const SourceInfo &Info) { return true; }
+bool EvalEmitter::emitRetVoid(const SourceInfo &Info) {
+  EvalResult.setValid();
+  return true;
+}
 
 bool EvalEmitter::emitRetValue(const SourceInfo &Info) {
-  // Method to recursively traverse composites.
-  std::function<bool(QualType, const Pointer &, APValue &)> Composite;
-  Composite = [this, &Composite](QualType Ty, const Pointer &Ptr, APValue &R) {
-    if (const auto *AT = Ty->getAs<AtomicType>())
-      Ty = AT->getValueType();
-
-    if (const auto *RT = Ty->getAs<RecordType>()) {
-      const auto *Record = Ptr.getRecord();
-      assert(Record && "Missing record descriptor");
-
-      bool Ok = true;
-      if (RT->getDecl()->isUnion()) {
-        const FieldDecl *ActiveField = nullptr;
-        APValue Value;
-        for (const auto &F : Record->fields()) {
-          const Pointer &FP = Ptr.atField(F.Offset);
-          QualType FieldTy = F.Decl->getType();
-          if (FP.isActive()) {
-            if (std::optional<PrimType> T = Ctx.classify(FieldTy)) {
-              TYPE_SWITCH(*T, Ok &= ReturnValue<T>(FP.deref<T>(), Value));
-            } else {
-              Ok &= Composite(FieldTy, FP, Value);
-            }
-            break;
-          }
-        }
-        R = APValue(ActiveField, Value);
-      } else {
-        unsigned NF = Record->getNumFields();
-        unsigned NB = Record->getNumBases();
-        unsigned NV = Ptr.isBaseClass() ? 0 : Record->getNumVirtualBases();
-
-        R = APValue(APValue::UninitStruct(), NB, NF);
-
-        for (unsigned I = 0; I < NF; ++I) {
-          const Record::Field *FD = Record->getField(I);
-          QualType FieldTy = FD->Decl->getType();
-          const Pointer &FP = Ptr.atField(FD->Offset);
-          APValue &Value = R.getStructField(I);
-
-          if (std::optional<PrimType> T = Ctx.classify(FieldTy)) {
-            TYPE_SWITCH(*T, Ok &= ReturnValue<T>(FP.deref<T>(), Value));
-          } else {
-            Ok &= Composite(FieldTy, FP, Value);
-          }
-        }
-
-        for (unsigned I = 0; I < NB; ++I) {
-          const Record::Base *BD = Record->getBase(I);
-          QualType BaseTy = Ctx.getASTContext().getRecordType(BD->Decl);
-          const Pointer &BP = Ptr.atField(BD->Offset);
-          Ok &= Composite(BaseTy, BP, R.getStructBase(I));
-        }
-
-        for (unsigned I = 0; I < NV; ++I) {
-          const Record::Base *VD = Record->getVirtualBase(I);
-          QualType VirtBaseTy = Ctx.getASTContext().getRecordType(VD->Decl);
-          const Pointer &VP = Ptr.atField(VD->Offset);
-          Ok &= Composite(VirtBaseTy, VP, R.getStructBase(NB + I));
-        }
-      }
-      return Ok;
-    }
-
-    if (Ty->isIncompleteArrayType()) {
-      R = APValue(APValue::UninitArray(), 0, 0);
-      return true;
-    }
-
-    if (const auto *AT = Ty->getAsArrayTypeUnsafe()) {
-      const size_t NumElems = Ptr.getNumElems();
-      QualType ElemTy = AT->getElementType();
-      R = APValue(APValue::UninitArray{}, NumElems, NumElems);
-
-      bool Ok = true;
-      for (unsigned I = 0; I < NumElems; ++I) {
-        APValue &Slot = R.getArrayInitializedElt(I);
-        const Pointer &EP = Ptr.atIndex(I);
-        if (std::optional<PrimType> T = Ctx.classify(ElemTy)) {
-          TYPE_SWITCH(*T, Ok &= ReturnValue<T>(EP.deref<T>(), Slot));
-        } else {
-          Ok &= Composite(ElemTy, EP.narrow(), Slot);
-        }
-      }
-      return Ok;
-    }
-    llvm_unreachable("invalid value to return");
-  };
-
-  // Return the composite type.
   const auto &Ptr = S.Stk.pop<Pointer>();
-  return Composite(Ptr.getType(), Ptr, Result);
+  if (std::optional<APValue> APV = Ptr.toRValue(S.getCtx())) {
+    EvalResult.setValue(*APV);
+    return true;
+  }
+
+  EvalResult.setInvalid();
+  return false;
 }
 
 bool EvalEmitter::emitGetPtrLocal(uint32_t I, const SourceInfo &Info) {
diff --git a/clang/lib/AST/Interp/EvalEmitter.h b/clang/lib/AST/Interp/EvalEmitter.h
index 5a9be18c34a03bc..e2516056ff5d2ca 100644
--- a/clang/lib/AST/Interp/EvalEmitter.h
+++ b/clang/lib/AST/Interp/EvalEmitter.h
@@ -13,6 +13,7 @@
 #ifndef LLVM_CLANG_AST_INTERP_EVALEMITTER_H
 #define LLVM_CLANG_AST_INTERP_EVALEMITTER_H
 
+#include "EvaluationResult.h"
 #include "InterpState.h"
 #include "PrimType.h"
 #include "Source.h"
@@ -33,8 +34,10 @@ class EvalEmitter : public SourceMapper {
   using AddrTy = uintptr_t;
   using Local = Scope::Local;
 
-  llvm::Expected<bool> interpretExpr(const Expr *E);
-  llvm::Expected<bool> interpretDecl(const VarDecl *VD);
+  EvaluationResult interpretExpr(const Expr *E);
+  EvaluationResult interpretDecl(const VarDecl *VD);
+
+  InterpState &getState() { return S; }
 
 protected:
   EvalEmitter(Context &Ctx, Program &P, State &Parent, InterpStack &Stk,
@@ -86,7 +89,7 @@ class EvalEmitter : public SourceMapper {
   /// Callee evaluation state.
   InterpState S;
   /// Location to write the result to.
-  APValue &Result;
+  EvaluationResult EvalResult;
 
   /// Temporaries which require storage.
   llvm::DenseMap<unsigned, std::unique_ptr<char[]>> Locals;
@@ -100,8 +103,6 @@ class EvalEmitter : public SourceMapper {
   // The emitter always tracks the current instruction and sets OpPC to a token
   // value which is mapped to the location of the opcode being evaluated.
   CodePtr OpPC;
-  /// Location of a failure.
-  std::optional<SourceLocation> BailLocation;
   /// Location of the current instruction.
   SourceInfo CurrentSource;
 
diff --git a/clang/lib/AST/Interp/EvaluationResult.cpp b/cl...
[truncated]

@tbaederr
Copy link
Contributor Author

Ping

1 similar comment
@tbaederr
Copy link
Contributor Author

Ping

@tbaederr
Copy link
Contributor Author

tbaederr commented Dec 8, 2023

Ping

@tbaederr
Copy link
Contributor Author

Ping

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly reasonable to me.

BailLocation = Loc;
return false;
}
bool ByteCodeEmitter::bail(const SourceLocation &Loc) { return false; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have future plans for this function (aka, should we remove it entirely now)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, probably. There is some utility to it, i.e. I can set a debugger breakpoint on it, but if it just returns false, that doesn't work anyway.

Comment on lines +122 to +126
// lvalue-to-rvalue conversion.
std::optional<APValue> RValueResult = Res.toRValue();
if (!RValueResult)
return false;
Result = *RValueResult;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably turn this into a function rather than reimplement it in different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two versions are slightly different: in evaluateInitializer(), we always do the conversion for global variables, but in the version in evaluateAsRValue only for GLValues. I could make the EvaluationResult API a littler easier to use for the cases in Context though, i.e. bool toRValue(APValue &Result) instead of returning the std::optional.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that seems fine to do in a follow-up.

return false;
}

assert(Stk.empty());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be asserting that we have an rvalue as opposed to an lvalue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have either here and just return what we have.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay!

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM though we can clean some bits up in follow-ups

@tbaederr tbaederr merged commit 4e7cf1b into llvm:main Jan 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants