Add Cpp::TakeInterpreter

Pops interpreter without destroying it.
Destruction to be handled by the caller.
This is to be used along with UserExternalInterpreter, where libCppInterOp is not owning the interpreter instance.

Author: Vipul-Cariappa

include/CppInterOp/CppInterOp.h

@@ -702,6 +702,11 @@ CreateInterpreter(const std::vector<const char*>& Args = {},
 ///\returns false on failure or if \c I is not tracked in the stack.
 CPPINTEROP_API bool DeleteInterpreter(TInterp_t I = nullptr);
 
+/// Take ownership of an interpreter instance.
+///\param[in] I - the interpreter to be taken, if nullptr, returns the last.
+///\returns nullptr on failure or if \c I is not tracked in the stack.
+CPPINTEROP_API TInterp_t TakeInterpreter(TInterp_t I = nullptr);
+
 /// Activates an instance of an interpreter to handle subsequent API requests
 ///\param[in] I - the interpreter to be activated.
 ///\returns false on failure.

lib/CppInterOp/CppInterOp.cpp

@@ -54,6 +54,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -138,16 +139,16 @@ struct InterpreterInfo {
   InterpreterInfo& operator=(const InterpreterInfo&) = delete;
 };
 
-// NOLINTBEGIN
+// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables)
 // std::deque avoids relocations and calling the dtor of InterpreterInfo.
-static llvm::ManagedStatic<std::deque<std::shared_ptr<InterpreterInfo>>>
+static llvm::ManagedStatic<std::deque<std::unique_ptr<InterpreterInfo>>>
     sInterpreters;
 static llvm::ManagedStatic<
-    std::unordered_map<clang::ASTContext*, std::weak_ptr<InterpreterInfo>>>
+    std::unordered_map<clang::ASTContext*, InterpreterInfo*>>
     sInterpreterASTMap;
 static std::recursive_mutex InterpreterStackLock;
 static std::recursive_mutex LLVMLock;
-// NOLINTEND
+// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables)
 
 static InterpreterInfo& getInterpInfo() {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
@@ -161,17 +162,20 @@ static InterpreterInfo& getInterpInfo(const clang::Decl* D) {
     return getInterpInfo();
   if (sInterpreters->size() == 1)
     return *sInterpreters->back();
-  return *(*sInterpreterASTMap)[&D->getASTContext()].lock();
+  return *(*sInterpreterASTMap)[&D->getASTContext()];
 }
 static InterpreterInfo& getInterpInfo(const void* D) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
+  QualType QT = QualType::getFromOpaquePtr(D);
+  if (auto* D = QT->getAsTagDecl())
+    return getInterpInfo(D);
   if (!D)
     return getInterpInfo();
   if (sInterpreters->size() == 1)
     return *sInterpreters->back();
   for (auto& item : *sInterpreterASTMap) {
     if (item.first->getAllocator().identifyObject(D))
-      return *item.second.lock();
+      return *item.second;
   }
   llvm_unreachable(
       "This pointer does not belong to any interpreter instance.\n");
@@ -189,7 +193,7 @@ static compat::Interpreter& getInterp(const clang::Decl* D) {
     return getInterp();
   if (sInterpreters->size() == 1)
     return *sInterpreters->back()->Interpreter;
-  return *(*sInterpreterASTMap)[&D->getASTContext()].lock()->Interpreter;
+  return *(*sInterpreterASTMap)[&D->getASTContext()]->Interpreter;
 }
 static compat::Interpreter& getInterp(const void* D) {
   return *getInterpInfo(D).Interpreter;
@@ -3338,6 +3342,8 @@ static std::string MakeResourcesPath() {
 TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
                             const std::vector<const char*>& GpuArgs /*={}*/) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
+
   std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr);
   std::string ResourceDir = MakeResourcesPath();
   std::vector<const char*> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
@@ -3414,42 +3420,73 @@ TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
   )");
 
   sInterpreters->emplace_back(
-      std::make_shared<InterpreterInfo>(I, /*Owned=*/true));
+      std::make_unique<InterpreterInfo>(I, /*Owned=*/true));
   sInterpreterASTMap->insert(
       {&sInterpreters->back()->Interpreter->getSema().getASTContext(),
-       sInterpreters->back()});
+       sInterpreters->back().get()});
 
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
   return I;
 }
 
+static inline auto find_interpreter_in_stack(TInterp_t I) {
+  return std::find_if(
+      sInterpreters->begin(), sInterpreters->end(),
+      [&I](const auto& Info) { return Info->Interpreter == I; });
+}
+
+static inline auto find_interpreter_in_map(InterpreterInfo* I) {
+  return std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
+                      [&](const auto& Item) { return Item.second == I; });
+}
+
 bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
 
   if (!I) {
-    auto foundAST =
-        std::find_if(sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
-                     [](const auto& Item) {
-                       return Item.second.lock() == sInterpreters->back();
-                     });
+    auto foundAST = find_interpreter_in_map(sInterpreters->back().get());
+    assert(foundAST != sInterpreterASTMap->end());
     sInterpreterASTMap->erase(foundAST);
     sInterpreters->pop_back();
     return true;
   }
 
-  auto found =
-      std::find_if(sInterpreters->begin(), sInterpreters->end(),
-                   [&I](const auto& Info) { return Info->Interpreter == I; });
+  auto found = find_interpreter_in_stack(I);
   if (found == sInterpreters->end())
     return false; // failure
 
-  auto foundAST = std::find_if(
-      sInterpreterASTMap->begin(), sInterpreterASTMap->end(),
-      [&found](const auto& Item) { return Item.second.lock() == *found; });
+  auto foundAST = find_interpreter_in_map((*found).get());
+  assert(foundAST != sInterpreterASTMap->end());
   sInterpreterASTMap->erase(foundAST);
   sInterpreters->erase(found);
   return true;
 }
 
+TInterp_t TakeInterpreter(TInterp_t I /*=nullptr*/) {
+  std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
+
+  if (!I) {
+    auto foundAST = find_interpreter_in_map(sInterpreters->back().get());
+    sInterpreterASTMap->erase(foundAST);
+    InterpreterInfo* res = sInterpreters->back().release();
+    sInterpreters->pop_back();
+    return res->Interpreter;
+  }
+
+  auto found = find_interpreter_in_stack(I);
+  if (found == sInterpreters->end())
+    return nullptr; // failure
+
+  auto foundAST = find_interpreter_in_map((*found).get());
+  sInterpreterASTMap->erase(foundAST);
+  InterpreterInfo* res = (*found).release();
+  sInterpreters->erase(found);
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
+  return res->Interpreter;
+}
+
 bool ActivateInterpreter(TInterp_t I) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
 
@@ -3477,10 +3514,13 @@ TInterp_t GetInterpreter() {
 
 void UseExternalInterpreter(TInterp_t I) {
   std::lock_guard<std::recursive_mutex> Lock(InterpreterStackLock);
-  assert(sInterpreters->empty() && "sInterpreter already in use!");
   sInterpreters->emplace_back(
-      std::make_shared<InterpreterInfo>(static_cast<compat::Interpreter*>(I),
+      std::make_unique<InterpreterInfo>(static_cast<compat::Interpreter*>(I),
                                         /*isOwned=*/false));
+  sInterpreterASTMap->insert(
+      {&sInterpreters->back()->Interpreter->getSema().getASTContext(),
+       sInterpreters->back().get()});
+  assert(sInterpreters->size() == sInterpreterASTMap->size());
 }
 
 void AddSearchPath(const char* dir, bool isUser, bool prepend) {
@@ -3734,9 +3774,9 @@ std::string ObjToString(const char* type, void* obj) {
   return getInterp().toString(type, obj);
 }
 
-Decl* InstantiateTemplate(TemplateDecl* TemplateD,
-                          TemplateArgumentListInfo& TLI, Sema& S,
-                          bool instantiate_body) {
+static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
+                                 TemplateArgumentListInfo& TLI, Sema& S,
+                                 bool instantiate_body) {
   LOCK(getInterpInfo());
   // This is not right but we don't have a lot of options to choose from as a
   // template instantiation requires a valid source location.

lib/CppInterOp/CppInterOpInterpreter.h

@@ -140,12 +140,11 @@ namespace Cpp {
 /// CppInterOp Interpreter
 ///
 class Interpreter {
-private:
   std::unique_ptr<clang::Interpreter> inner;
 
+public:
   Interpreter(std::unique_ptr<clang::Interpreter> CI) : inner(std::move(CI)) {}
 
-public:
   static std::unique_ptr<Interpreter>
   create(int argc, const char* const* argv, const char* llvmdir = nullptr,
          const std::vector<std::shared_ptr<clang::ModuleFileExtension>>&

unittests/CppInterOp/InterpreterTest.cpp

@@ -24,6 +24,7 @@
 #include "gtest/gtest.h"
 
 #include <algorithm>
+#include <utility>
 
 using ::testing::StartsWith;
 
@@ -150,6 +151,8 @@ TEST(InterpreterTest, Process) {
   EXPECT_EQ(Res, CXError_Success);
   clang_Value_dispose(CXV);
   clang_Interpreter_dispose(CXI);
+  auto* OldI = Cpp::TakeInterpreter();
+  EXPECT_EQ(OldI, I);
 }
 
 TEST(InterpreterTest, EmscriptenExceptionHandling) {
@@ -329,7 +332,9 @@ if (llvm::sys::RunningOnValgrind())
   // Create the interpreter instance.
   std::unique_ptr<clang::Interpreter> I =
       ExitOnErr(clang::Interpreter::create(std::move(CI)));
-  auto ExtInterp = I.get();
+
+  auto CPPI = Cpp::Interpreter(std::move(I));
+  auto* ExtInterp = &CPPI;
 #endif // CPPINTEROP_USE_REPL
 
 #ifdef CPPINTEROP_USE_CLING
@@ -346,12 +351,9 @@ if (llvm::sys::RunningOnValgrind())
 
   EXPECT_NE(ExtInterp, nullptr);
 
-#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-#ifndef _WIN32 // Windows seems to fail to die...
-    EXPECT_DEATH(Cpp::UseExternalInterpreter(ExtInterp), "sInterpreter already in use!");
-#endif // _WIN32
-#endif
-  EXPECT_TRUE(Cpp::GetInterpreter()) << "External Interpreter not set";
+  Cpp::UseExternalInterpreter(ExtInterp);
+  EXPECT_EQ(ExtInterp, Cpp::GetInterpreter());
+  EXPECT_EQ(ExtInterp, Cpp::TakeInterpreter(ExtInterp));
 
 #ifndef CPPINTEROP_USE_CLING
   I.release();
@@ -366,14 +368,15 @@ TEST(InterpreterTest, MultipleInterpreter) {
 #if CLANG_VERSION_MAJOR < 20 && defined(EMSCRIPTEN)
   GTEST_SKIP() << "Test fails for Emscipten LLVM 20 builds";
 #endif
-
-  EXPECT_TRUE(Cpp::CreateInterpreter());
+  auto* I = Cpp::CreateInterpreter();
+  EXPECT_TRUE(I);
   Cpp::Declare(R"(
   void f() {}
   )");
   Cpp::TCppScope_t f = Cpp::GetNamed("f");
 
-  EXPECT_TRUE(Cpp::CreateInterpreter());
+  auto* I2 = Cpp::CreateInterpreter();
+  EXPECT_TRUE(I2);
   Cpp::Declare(R"(
   void ff() {}
   )");