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-repl] Expose CreateExecutor() and ResetExecutor() in extended Interpreter interface #84460

Conversation

weliveindetail
Copy link
Contributor

IncrementalExecutor is an implementation detail of the Interpreter. In order to test extended features properly, we must be able to setup and tear down the executor manually.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

IncrementalExecutor is an implementation detail of the Interpreter. In order to test extended features properly, we must be able to setup and tear down the executor manually.


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

4 Files Affected:

  • (modified) clang/include/clang/Interpreter/Interpreter.h (+38-6)
  • (modified) clang/lib/Interpreter/Interpreter.cpp (+160-104)
  • (modified) clang/unittests/Interpreter/CMakeLists.txt (+2)
  • (added) clang/unittests/Interpreter/InterpreterExtensionsTest.cpp (+105)
diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h
index c8f932e95c4798..344f38da5ab21b 100644
--- a/clang/include/clang/Interpreter/Interpreter.h
+++ b/clang/include/clang/Interpreter/Interpreter.h
@@ -18,6 +18,7 @@
 #include "clang/AST/GlobalDecl.h"
 #include "clang/Interpreter/PartialTranslationUnit.h"
 #include "clang/Interpreter/Value.h"
+#include "clang/Sema/Ownership.h"
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ExecutionEngine/JITSymbol.h"
@@ -75,18 +76,26 @@ class IncrementalCompilerBuilder {
   llvm::StringRef CudaSDKPath;
 };
 
+/// Generate glue code between the Interpreter's built-in runtime and user code.
+class RuntimeInterfaceBuilder {
+public:
+  virtual ~RuntimeInterfaceBuilder() = default;
+
+  using TransformExprFunction = ExprResult(RuntimeInterfaceBuilder *Builder,
+                                           Expr *, ArrayRef<Expr *>);
+  virtual TransformExprFunction *getPrintValueTransformer() = 0;
+};
+
 /// Provides top-level interfaces for incremental compilation and execution.
 class Interpreter {
   std::unique_ptr<llvm::orc::ThreadSafeContext> TSCtx;
   std::unique_ptr<IncrementalParser> IncrParser;
   std::unique_ptr<IncrementalExecutor> IncrExecutor;
+  std::unique_ptr<RuntimeInterfaceBuilder> RuntimeIB;
 
   // An optional parser for CUDA offloading
   std::unique_ptr<IncrementalParser> DeviceParser;
 
-  Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
-
-  llvm::Error CreateExecutor();
   unsigned InitPTUSize = 0;
 
   // This member holds the last result of the value printing. It's a class
@@ -94,8 +103,33 @@ class Interpreter {
   // printing happens, it's in an invalid state.
   Value LastValue;
 
+  // Add a call to an Expr to report its result. We query the function from
+  // RuntimeInterfaceBuilder once and store it as a function pointer to avoid
+  // frequent virtual function calls.
+  RuntimeInterfaceBuilder::TransformExprFunction *AddPrintValueCall = nullptr;
+
+protected:
+  // Derived classes can make use an extended interface of the Interpreter.
+  // That's useful for testing and out-of-tree clients.
+  Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
+
+  // Create the internal IncrementalExecutor, or re-create it after calling
+  // ResetExecutor().
+  llvm::Error CreateExecutor();
+
+  // Delete the internal IncrementalExecutor. This causes a hard shutdown of the
+  // JIT engine. In particular, it doesn't run cleanup or destructors.
+  void ResetExecutor();
+
+  // Lazily construct the RuntimeInterfaceBuilder. The provided instance will be
+  // used for the entire lifetime of the interpreter. The default implementation
+  // targets the in-process __clang_Interpreter runtime. Override this to use a
+  // custom runtime.
+  virtual std::unique_ptr<RuntimeInterfaceBuilder> FindRuntimeInterface();
+
 public:
-  ~Interpreter();
+  virtual ~Interpreter();
+
   static llvm::Expected<std::unique_ptr<Interpreter>>
   create(std::unique_ptr<CompilerInstance> CI);
   static llvm::Expected<std::unique_ptr<Interpreter>>
@@ -143,8 +177,6 @@ class Interpreter {
 private:
   size_t getEffectivePTUSize() const;
 
-  bool FindRuntimeInterface();
-
   llvm::DenseMap<CXXRecordDecl *, llvm::orc::ExecutorAddr> Dtors;
 
   llvm::SmallVector<Expr *, 4> ValuePrintingInfo;
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 37696b28976428..0b4ef946de6ac4 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -36,9 +36,11 @@
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/ExecutionEngine/JITSymbol.h"
+#include "llvm/ExecutionEngine/Orc/JITTargetMachineBuilder.h"
 #include "llvm/ExecutionEngine/Orc/LLJIT.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
@@ -368,9 +370,22 @@ Interpreter::Parse(llvm::StringRef Code) {
   return IncrParser->Parse(Code);
 }
 
+static llvm::Expected<llvm::orc::JITTargetMachineBuilder>
+createJITTargetMachineBuilder(const std::string &TT) {
+  if (TT == llvm::sys::getProcessTriple())
+    return llvm::orc::JITTargetMachineBuilder::detectHost();
+  // FIXME: This can fail as well if the target is not registered! We just don't
+  // catch it yet.
+  return llvm::orc::JITTargetMachineBuilder(llvm::Triple(TT));
+}
+
 llvm::Error Interpreter::CreateExecutor() {
   const clang::TargetInfo &TI =
       getCompilerInstance()->getASTContext().getTargetInfo();
+  if (IncrExecutor)
+    return llvm::make_error<llvm::StringError>("Operation failed. "
+                                               "Execution engine exists",
+                                               std::error_code());
   llvm::Error Err = llvm::Error::success();
   auto Executor = std::make_unique<IncrementalExecutor>(*TSCtx, Err, TI);
   if (!Err)
@@ -379,6 +394,8 @@ llvm::Error Interpreter::CreateExecutor() {
   return Err;
 }
 
+void Interpreter::ResetExecutor() { IncrExecutor.reset(); }
+
 llvm::Error Interpreter::Execute(PartialTranslationUnit &T) {
   assert(T.TheModule);
   if (!IncrExecutor) {
@@ -507,9 +524,13 @@ static constexpr llvm::StringRef MagicRuntimeInterface[] = {
     "__clang_Interpreter_SetValueWithAlloc",
     "__clang_Interpreter_SetValueCopyArr", "__ci_newtag"};
 
-bool Interpreter::FindRuntimeInterface() {
+static std::unique_ptr<RuntimeInterfaceBuilder>
+createInProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &Ctx,
+                                       Sema &S);
+
+std::unique_ptr<RuntimeInterfaceBuilder> Interpreter::FindRuntimeInterface() {
   if (llvm::all_of(ValuePrintingInfo, [](Expr *E) { return E != nullptr; }))
-    return true;
+    return nullptr;
 
   Sema &S = getCompilerInstance()->getSema();
   ASTContext &Ctx = S.getASTContext();
@@ -528,120 +549,34 @@ bool Interpreter::FindRuntimeInterface() {
 
   if (!LookupInterface(ValuePrintingInfo[NoAlloc],
                        MagicRuntimeInterface[NoAlloc]))
-    return false;
+    return nullptr;
   if (!LookupInterface(ValuePrintingInfo[WithAlloc],
                        MagicRuntimeInterface[WithAlloc]))
-    return false;
+    return nullptr;
   if (!LookupInterface(ValuePrintingInfo[CopyArray],
                        MagicRuntimeInterface[CopyArray]))
-    return false;
+    return nullptr;
   if (!LookupInterface(ValuePrintingInfo[NewTag],
                        MagicRuntimeInterface[NewTag]))
-    return false;
-  return true;
+    return nullptr;
+
+  return createInProcessRuntimeInterfaceBuilder(*this, Ctx, S);
 }
 
 namespace {
 
-class RuntimeInterfaceBuilder
-    : public TypeVisitor<RuntimeInterfaceBuilder, Interpreter::InterfaceKind> {
-  clang::Interpreter &Interp;
+class InterfaceKindVisitor
+    : public TypeVisitor<InterfaceKindVisitor, Interpreter::InterfaceKind> {
+  friend class InProcessRuntimeInterfaceBuilder;
+
   ASTContext &Ctx;
   Sema &S;
   Expr *E;
   llvm::SmallVector<Expr *, 3> Args;
 
 public:
-  RuntimeInterfaceBuilder(clang::Interpreter &In, ASTContext &C, Sema &SemaRef,
-                          Expr *VE, ArrayRef<Expr *> FixedArgs)
-      : Interp(In), Ctx(C), S(SemaRef), E(VE) {
-    // The Interpreter* parameter and the out parameter `OutVal`.
-    for (Expr *E : FixedArgs)
-      Args.push_back(E);
-
-    // Get rid of ExprWithCleanups.
-    if (auto *EWC = llvm::dyn_cast_if_present<ExprWithCleanups>(E))
-      E = EWC->getSubExpr();
-  }
-
-  ExprResult getCall() {
-    QualType Ty = E->getType();
-    QualType DesugaredTy = Ty.getDesugaredType(Ctx);
-
-    // For lvalue struct, we treat it as a reference.
-    if (DesugaredTy->isRecordType() && E->isLValue()) {
-      DesugaredTy = Ctx.getLValueReferenceType(DesugaredTy);
-      Ty = Ctx.getLValueReferenceType(Ty);
-    }
-
-    Expr *TypeArg =
-        CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)Ty.getAsOpaquePtr());
-    // The QualType parameter `OpaqueType`, represented as `void*`.
-    Args.push_back(TypeArg);
-
-    // We push the last parameter based on the type of the Expr. Note we need
-    // special care for rvalue struct.
-    Interpreter::InterfaceKind Kind = Visit(&*DesugaredTy);
-    switch (Kind) {
-    case Interpreter::InterfaceKind::WithAlloc:
-    case Interpreter::InterfaceKind::CopyArray: {
-      // __clang_Interpreter_SetValueWithAlloc.
-      ExprResult AllocCall = S.ActOnCallExpr(
-          /*Scope=*/nullptr,
-          Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::WithAlloc],
-          E->getBeginLoc(), Args, E->getEndLoc());
-      assert(!AllocCall.isInvalid() && "Can't create runtime interface call!");
-
-      TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ty, SourceLocation());
-
-      // Force CodeGen to emit destructor.
-      if (auto *RD = Ty->getAsCXXRecordDecl()) {
-        auto *Dtor = S.LookupDestructor(RD);
-        Dtor->addAttr(UsedAttr::CreateImplicit(Ctx));
-        Interp.getCompilerInstance()->getASTConsumer().HandleTopLevelDecl(
-            DeclGroupRef(Dtor));
-      }
-
-      // __clang_Interpreter_SetValueCopyArr.
-      if (Kind == Interpreter::InterfaceKind::CopyArray) {
-        const auto *ConstantArrTy =
-            cast<ConstantArrayType>(DesugaredTy.getTypePtr());
-        size_t ArrSize = Ctx.getConstantArrayElementCount(ConstantArrTy);
-        Expr *ArrSizeExpr = IntegerLiteralExpr(Ctx, ArrSize);
-        Expr *Args[] = {E, AllocCall.get(), ArrSizeExpr};
-        return S.ActOnCallExpr(
-            /*Scope *=*/nullptr,
-            Interp
-                .getValuePrintingInfo()[Interpreter::InterfaceKind::CopyArray],
-            SourceLocation(), Args, SourceLocation());
-      }
-      Expr *Args[] = {
-          AllocCall.get(),
-          Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::NewTag]};
-      ExprResult CXXNewCall = S.BuildCXXNew(
-          E->getSourceRange(),
-          /*UseGlobal=*/true, /*PlacementLParen=*/SourceLocation(), Args,
-          /*PlacementRParen=*/SourceLocation(),
-          /*TypeIdParens=*/SourceRange(), TSI->getType(), TSI, std::nullopt,
-          E->getSourceRange(), E);
-
-      assert(!CXXNewCall.isInvalid() &&
-             "Can't create runtime placement new call!");
-
-      return S.ActOnFinishFullExpr(CXXNewCall.get(),
-                                   /*DiscardedValue=*/false);
-    }
-      // __clang_Interpreter_SetValueNoAlloc.
-    case Interpreter::InterfaceKind::NoAlloc: {
-      return S.ActOnCallExpr(
-          /*Scope=*/nullptr,
-          Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::NoAlloc],
-          E->getBeginLoc(), Args, E->getEndLoc());
-    }
-    default:
-      llvm_unreachable("Unhandled Interpreter::InterfaceKind");
-    }
-  }
+  InterfaceKindVisitor(ASTContext &Ctx, Sema &S, Expr *E)
+      : Ctx(Ctx), S(S), E(E) {}
 
   Interpreter::InterfaceKind VisitRecordType(const RecordType *Ty) {
     return Interpreter::InterfaceKind::WithAlloc;
@@ -713,8 +648,124 @@ class RuntimeInterfaceBuilder
     Args.push_back(CastedExpr.get());
   }
 };
+
+class InProcessRuntimeInterfaceBuilder : public RuntimeInterfaceBuilder {
+  Interpreter &Interp;
+  ASTContext &Ctx;
+  Sema &S;
+
+public:
+  InProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &C, Sema &S)
+      : Interp(Interp), Ctx(C), S(S) {}
+
+  TransformExprFunction *getPrintValueTransformer() override {
+    return &transformForValuePrinting;
+  }
+
+private:
+  static ExprResult transformForValuePrinting(RuntimeInterfaceBuilder *Builder,
+                                              Expr *E,
+                                              ArrayRef<Expr *> FixedArgs) {
+    auto *B = static_cast<InProcessRuntimeInterfaceBuilder *>(Builder);
+
+    // Get rid of ExprWithCleanups.
+    if (auto *EWC = llvm::dyn_cast_if_present<ExprWithCleanups>(E))
+      E = EWC->getSubExpr();
+
+    InterfaceKindVisitor Visitor(B->Ctx, B->S, E);
+
+    // The Interpreter* parameter and the out parameter `OutVal`.
+    for (Expr *E : FixedArgs)
+      Visitor.Args.push_back(E);
+
+    QualType Ty = E->getType();
+    QualType DesugaredTy = Ty.getDesugaredType(B->Ctx);
+
+    // For lvalue struct, we treat it as a reference.
+    if (DesugaredTy->isRecordType() && E->isLValue()) {
+      DesugaredTy = B->Ctx.getLValueReferenceType(DesugaredTy);
+      Ty = B->Ctx.getLValueReferenceType(Ty);
+    }
+
+    Expr *TypeArg = CStyleCastPtrExpr(B->S, B->Ctx.VoidPtrTy,
+                                      (uintptr_t)Ty.getAsOpaquePtr());
+    // The QualType parameter `OpaqueType`, represented as `void*`.
+    Visitor.Args.push_back(TypeArg);
+
+    // We push the last parameter based on the type of the Expr. Note we need
+    // special care for rvalue struct.
+    Interpreter::InterfaceKind Kind = Visitor.Visit(&*DesugaredTy);
+    switch (Kind) {
+    case Interpreter::InterfaceKind::WithAlloc:
+    case Interpreter::InterfaceKind::CopyArray: {
+      // __clang_Interpreter_SetValueWithAlloc.
+      ExprResult AllocCall = B->S.ActOnCallExpr(
+          /*Scope=*/nullptr,
+          B->Interp
+              .getValuePrintingInfo()[Interpreter::InterfaceKind::WithAlloc],
+          E->getBeginLoc(), Visitor.Args, E->getEndLoc());
+      assert(!AllocCall.isInvalid() && "Can't create runtime interface call!");
+
+      TypeSourceInfo *TSI =
+          B->Ctx.getTrivialTypeSourceInfo(Ty, SourceLocation());
+
+      // Force CodeGen to emit destructor.
+      if (auto *RD = Ty->getAsCXXRecordDecl()) {
+        auto *Dtor = B->S.LookupDestructor(RD);
+        Dtor->addAttr(UsedAttr::CreateImplicit(B->Ctx));
+        B->Interp.getCompilerInstance()->getASTConsumer().HandleTopLevelDecl(
+            DeclGroupRef(Dtor));
+      }
+
+      // __clang_Interpreter_SetValueCopyArr.
+      if (Kind == Interpreter::InterfaceKind::CopyArray) {
+        const auto *ConstantArrTy =
+            cast<ConstantArrayType>(DesugaredTy.getTypePtr());
+        size_t ArrSize = B->Ctx.getConstantArrayElementCount(ConstantArrTy);
+        Expr *ArrSizeExpr = IntegerLiteralExpr(B->Ctx, ArrSize);
+        Expr *Args[] = {E, AllocCall.get(), ArrSizeExpr};
+        return B->S.ActOnCallExpr(
+            /*Scope *=*/nullptr,
+            B->Interp
+                .getValuePrintingInfo()[Interpreter::InterfaceKind::CopyArray],
+            SourceLocation(), Args, SourceLocation());
+      }
+      Expr *Args[] = {
+          AllocCall.get(),
+          B->Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::NewTag]};
+      ExprResult CXXNewCall = B->S.BuildCXXNew(
+          E->getSourceRange(),
+          /*UseGlobal=*/true, /*PlacementLParen=*/SourceLocation(), Args,
+          /*PlacementRParen=*/SourceLocation(),
+          /*TypeIdParens=*/SourceRange(), TSI->getType(), TSI, std::nullopt,
+          E->getSourceRange(), E);
+
+      assert(!CXXNewCall.isInvalid() &&
+             "Can't create runtime placement new call!");
+
+      return B->S.ActOnFinishFullExpr(CXXNewCall.get(),
+                                      /*DiscardedValue=*/false);
+    }
+      // __clang_Interpreter_SetValueNoAlloc.
+    case Interpreter::InterfaceKind::NoAlloc: {
+      return B->S.ActOnCallExpr(
+          /*Scope=*/nullptr,
+          B->Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::NoAlloc],
+          E->getBeginLoc(), Visitor.Args, E->getEndLoc());
+    }
+    default:
+      llvm_unreachable("Unhandled Interpreter::InterfaceKind");
+    }
+  }
+};
 } // namespace
 
+static std::unique_ptr<RuntimeInterfaceBuilder>
+createInProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &Ctx,
+                                       Sema &S) {
+  return std::make_unique<InProcessRuntimeInterfaceBuilder>(Interp, Ctx, S);
+}
+
 // This synthesizes a call expression to a speciall
 // function that is responsible for generating the Value.
 // In general, we transform:
@@ -733,8 +784,13 @@ Expr *Interpreter::SynthesizeExpr(Expr *E) {
   Sema &S = getCompilerInstance()->getSema();
   ASTContext &Ctx = S.getASTContext();
 
-  if (!FindRuntimeInterface())
-    llvm_unreachable("We can't find the runtime iterface for pretty print!");
+  if (!RuntimeIB) {
+    RuntimeIB = FindRuntimeInterface();
+    AddPrintValueCall = RuntimeIB->getPrintValueTransformer();
+  }
+
+  assert(AddPrintValueCall &&
+         "We don't have a runtime interface for pretty print!");
 
   // Create parameter `ThisInterp`.
   auto *ThisInterp = CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)this);
@@ -743,9 +799,9 @@ Expr *Interpreter::SynthesizeExpr(Expr *E) {
   auto *OutValue = CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)&LastValue);
 
   // Build `__clang_Interpreter_SetValue*` call.
-  RuntimeInterfaceBuilder Builder(*this, Ctx, S, E, {ThisInterp, OutValue});
+  ExprResult Result =
+      AddPrintValueCall(RuntimeIB.get(), E, {ThisInterp, OutValue});
 
-  ExprResult Result = Builder.getCall();
   // It could fail, like printing an array type in C. (not supported)
   if (Result.isInvalid())
     return E;
diff --git a/clang/unittests/Interpreter/CMakeLists.txt b/clang/unittests/Interpreter/CMakeLists.txt
index 0ddedb283e07d1..498070b43d922e 100644
--- a/clang/unittests/Interpreter/CMakeLists.txt
+++ b/clang/unittests/Interpreter/CMakeLists.txt
@@ -4,12 +4,14 @@ set(LLVM_LINK_COMPONENTS
   OrcJIT
   Support
   TargetParser
+  TestingSupport
   )
 
 add_clang_unittest(ClangReplInterpreterTests
   IncrementalCompilerBuilderTest.cpp
   IncrementalProcessingTest.cpp
   InterpreterTest.cpp
+  InterpreterExtensionsTest.cpp
   CodeCompletionTest.cpp
   )
 target_link_libraries(ClangReplInterpreterTests PUBLIC
diff --git a/clang/unittests/Interpreter/InterpreterExtensionsTest.cpp b/clang/unittests/Interpreter/InterpreterExtensionsTest.cpp
new file mode 100644
index 00000000000000..8b88db1fcfede0
--- /dev/null
+++ b/clang/unittests/Interpreter/InterpreterExtensionsTest.cpp
@@ -0,0 +1,105 @@
+//===- unittests/Interpreter/InterpreterExtensionsTest.cpp ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for Clang's Interpreter library.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Interpreter/Interpreter.h"
+
+#include "clang/AST/Expr.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <system_error>
+
+using namespace clang;
+namespace {
+
+class TestCreateResetExecutor : public Interpreter {
+public:
+  TestCreateResetExecutor(std::unique_ptr<CompilerInstance> CI,
+                          llvm::Error &Err)
+      : Interpreter(std::move(CI), Err) {}
+
+  llvm::Error testCreateExecutor() {
+    return Interpreter::CreateExecutor();
+  }
+
+  void resetExecutor() { Interpreter::ResetExecutor(); }
+};
+
+TEST(InterpreterExtensionsTest, ExecutorCreateReset) {
+  clang::IncrementalCompilerBuilder CB;
+  llvm::Error ErrOut = llvm::Error::success();
+  TestCreateResetExecutor Interp(cantFail(CB.CreateCpp()), ErrOut);
+  cantFail(std::move(ErrOut));
+  cantFail(Interp.testCreateExecutor());
+  Interp.resetExecutor();
+  cantFail(Interp.testCreateExecutor());
+  EXPECT_THAT_ERROR(Interp.testCreateExecutor(),
+                    llvm::FailedWithMessage("Operation failed. "
+                                            "Execution engine exists"));
+}
+
+class RecordRuntimeIBMetrics : public Interpreter {
+  struct NoopRuntimeInterfaceBuilder : public RuntimeInterfaceBuilder {
+    NoopRuntimeInterfaceBuilder(Sema &S) : S(S) {}
+
+    TransformExprFunction *getPrintValueTransformer() override {
+      TransformerQueries += 1;
+      return &noop;
+    }
+
+    static ExprResult noop(RuntimeInterfaceBuilder *Builder, Expr *E,
+                           ArrayRef<Expr *> FixedArgs) {
+      auto *B = static_cast<NoopRuntimeInterfaceBuilder *>(Builder);
+      B->TransformedExprs += 1;
+      return B->S.A...
[truncated]

Copy link

github-actions bot commented Mar 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@weliveindetail
Copy link
Contributor Author

This PR was factored out for isolated review and to reduce the size of the follow-up patch in #84461. It depends on the previous change in #83126. Combined patch size will shrink significantly, once it landed (tonight hopefully). Until then please refer to the single commit 023cab5

@vgvassilev
Copy link
Contributor

I was currently touching this area to fix a problem of @jeaye reported on irc.

Do you mind incorporating this patch so that we avoid churn?

diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h
index 292fa566ae70..792978322349 100644
--- a/clang/include/clang/Interpreter/Interpreter.h
+++ b/clang/include/clang/Interpreter/Interpreter.h
@@ -83,7 +83,6 @@ class Interpreter {
 
   Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
 
-  llvm::Error CreateExecutor();
   unsigned InitPTUSize = 0;
 
   // This member holds the last result of the value printing. It's a class
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 9f97a3c6b0be..edc19b3a43e4 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -233,6 +233,26 @@ Interpreter::Interpreter(std::unique_ptr<CompilerInstance> CI,
   TSCtx = std::make_unique<llvm::orc::ThreadSafeContext>(std::move(LLVMCtx));
   IncrParser = std::make_unique<IncrementalParser>(*this, std::move(CI),
                                                    *TSCtx->getContext(), Err);
+  if (IncrParser->getCodeGen()) {
+    const clang::TargetInfo &TI =
+      getCompilerInstance()->getASTContext().getTargetInfo();
+    llvm::Error ExecErr = llvm::Error::success();
+    auto Executor = std::make_unique<IncrementalExecutor>(*TSCtx, ExecErr, TI);
+    if (ExecErr) {
+      Err = joinErrors(std::move(Err), std::move(ExecErr));
+      return;
+    }
+
+    IncrExecutor = std::move(Executor);
+
+    // Process the PTUs that came from initialization. For example -include will
+    // give us a header that's processed at initialization of the preprocessor.
+    for (auto &PTU : IncrParser->getPTUs())
+      if (auto ExecErr = Execute(PTU)) {
+        Err = joinErrors(std::move(Err), std::move(ExecErr));
+        return;
+      }
+  }
 }
 
 Interpreter::~Interpreter() {
@@ -327,10 +347,10 @@ CompilerInstance *Interpreter::getCompilerInstance() {
 }
 
 llvm::Expected<llvm::orc::LLJIT &> Interpreter::getExecutionEngine() {
-  if (!IncrExecutor) {
-    if (auto Err = CreateExecutor())
-      return std::move(Err);
-  }
+  if (!IncrExecutor)
+    return llvm::make_error<llvm::StringError>("Operation failed. "
+                                               "No execution engine",
+                                               std::error_code());
 
   return IncrExecutor->GetExecutionEngine();
 }
@@ -366,24 +386,8 @@ Interpreter::Parse(llvm::StringRef Code) {
   return IncrParser->Parse(Code);
 }
 
-llvm::Error Interpreter::CreateExecutor() {
-  const clang::TargetInfo &TI =
-      getCompilerInstance()->getASTContext().getTargetInfo();
-  llvm::Error Err = llvm::Error::success();
-  auto Executor = std::make_unique<IncrementalExecutor>(*TSCtx, Err, TI);
-  if (!Err)
-    IncrExecutor = std::move(Executor);
-
-  return Err;
-}
-
 llvm::Error Interpreter::Execute(PartialTranslationUnit &T) {
   assert(T.TheModule);
-  if (!IncrExecutor) {
-    auto Err = CreateExecutor();
-    if (Err)
-      return Err;
-  }
   // FIXME: Add a callback to retain the llvm::Module once the JIT is done.
   if (auto Err = IncrExecutor->addModule(T))
     return Err;
diff --git a/clang/test/Interpreter/execute.cpp b/clang/test/Interpreter/execute.cpp
index 6e73ed3927e8..36c74703cb85 100644
--- a/clang/test/Interpreter/execute.cpp
+++ b/clang/test/Interpreter/execute.cpp
@@ -7,6 +7,9 @@
 
 // RUN: cat %s | clang-repl | FileCheck %s
 // RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
+// RUN: clang-repl -Xcc -include -Xcc %s | FileCheck %s
+// RUN: clang-repl -Xcc -fsyntax-only -Xcc -include -Xcc %s
+
 extern "C" int printf(const char *, ...);
 int i = 42;
 auto r1 = printf("i = %d\n", i);
@@ -19,5 +22,3 @@ auto r2 = printf("S[f=%f, m=0x%llx]\n", s.f, reinterpret_cast<unsigned long long
 
 inline int foo() { return 42; }
 int r3 = foo();
-
-%quit

@weliveindetail
Copy link
Contributor Author

Do you mind incorporating this patch so that we avoid churn?

Sure. Essentially, this drops lazy creation of the executor and makes it dependent on the frontend action explicitly. Fine for me. We can still expose explicit setup/tear-down.

@weliveindetail
Copy link
Contributor Author

Do you mind incorporating this patch so that we avoid churn?

Sure. Essentially, this drops lazy creation of the executor and makes it dependent on the frontend action explicitly. Fine for me. We can still expose explicit setup/tear-down.

@vgvassilev I moved it into the separate review #84758, because it has unexpected side-effects.

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

clang/test/Interpreter/execute.cpp Outdated Show resolved Hide resolved
@vgvassilev
Copy link
Contributor

Do you mind incorporating this patch so that we avoid churn?

Sure. Essentially, this drops lazy creation of the executor and makes it dependent on the frontend action explicitly. Fine for me. We can still expose explicit setup/tear-down.

@vgvassilev I moved it into the separate review #84758, because it has unexpected side-effects.

Sorry about that...

@weliveindetail weliveindetail force-pushed the clang-repl-expose-create-reset-executor branch from fee6829 to a8c9cf8 Compare March 12, 2024 11:58
@weliveindetail weliveindetail merged commit bde7a6b into llvm:main Mar 12, 2024
4 checks passed
@weliveindetail weliveindetail deleted the clang-repl-expose-create-reset-executor branch March 12, 2024 13:11
@weliveindetail
Copy link
Contributor Author

Build bot https://lab.llvm.org/buildbot/#/builders/196/builds/46799 detected an issue with the test -- I will try and push a fix quickly:

tools/clang/unittests/Interpreter/CMakeFiles/ClangReplInterpreterTests.dir/InterpreterExtensionsTest.cpp.o:InterpreterExtensionsTest.cpp:function (anonymous namespace)::InterpreterExtensionsTest_ExecutorCreateReset_Test::TestBody(): error: undefined reference to 'llvm::detail::TakeError(llvm::Error)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

weliveindetail added a commit that referenced this pull request Mar 12, 2024
@vvereschaka
Copy link
Contributor

@weliveindetail ,
the Clang-Unit::ClangReplInterpreterTests.exe/InterpreterExtensionsTest/ExecutorCreateReset gets failed on the Clang Windows buidler

C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\clang\unittests\Interpreter\InterpreterExtensionsTest.cpp(51): error: Value of: llvm::detail::TakeError(Interp.testCreateExecutor())
Expected: failed with Error whose message has 1 element that is equal to "Operation failed. Execution engine exists"
  Actual: failed  (Unable to find target for this triple (no targets are registered)), whose element #0 doesn't match

would you take care of it?

@weliveindetail
Copy link
Contributor Author

Yes, I think I can fix it quickly

@weliveindetail
Copy link
Contributor Author

This should fix both: initializing the native target (your reported case) AND skipping the test on platforms that don't support JIT (other build bots complained).

@vvereschaka
Copy link
Contributor

Thank you @weliveindetail

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.

4 participants