Destroy the interpreters when they are owned.

This should fix a longstanding issue with resource deallocation in CppInterOp.

Author: vgvassilev

lib/CppInterOp/CppInterOp.cpp

@@ -48,10 +48,12 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/raw_os_ostream.h"
 
 #include <algorithm>
 #include <cassert>
+#include <deque>
 #include <iterator>
 #include <map>
 #include <memory>
@@ -84,20 +86,47 @@ using namespace std;
 struct InterpreterInfo {
   compat::Interpreter* Interpreter = nullptr;
   bool isOwned = true;
+  InterpreterInfo(compat::Interpreter* I, bool O)
+      : Interpreter(I), isOwned(O) {}
+
+  // Enable move constructors.
+  InterpreterInfo(InterpreterInfo&& other) noexcept
+      : Interpreter(other.Interpreter), isOwned(other.isOwned) {
+    other.Interpreter = nullptr;
+    other.isOwned = false;
+  }
+  InterpreterInfo& operator=(InterpreterInfo&& other) noexcept {
+    if (this != &other) {
+      // Delete current resource if owned
+      if (isOwned)
+        delete Interpreter;
+
+      Interpreter = other.Interpreter;
+      isOwned = other.isOwned;
+
+      other.Interpreter = nullptr;
+      other.isOwned = false;
+    }
+    return *this;
+  }
 
-  // Valgrind complains about __cxa_pure_virtual called when deleting
-  // llvm::SectionMemoryManager::~SectionMemoryManager as part of the dtor
-  // chain of the Interpreter.
-  // This might fix the issue https://reviews.llvm.org/D107087
-  // FIXME: For now we just leak the Interpreter.
-  ~InterpreterInfo() = default;
+  ~InterpreterInfo() {
+    if (isOwned)
+      delete Interpreter;
+  }
+
+  // Disable copy semantics (to avoid accidental double deletes)
+  InterpreterInfo(const InterpreterInfo&) = delete;
+  InterpreterInfo& operator=(const InterpreterInfo&) = delete;
 };
-static llvm::SmallVector<InterpreterInfo, 8> sInterpreters;
+
+// std::deque avoids relocations and calling the dtor of InterpreterInfo.
+static llvm::ManagedStatic<std::deque<InterpreterInfo>> sInterpreters;
 
 static compat::Interpreter& getInterp() {
-  assert(!sInterpreters.empty() &&
+  assert(!sInterpreters->empty() &&
          "Interpreter instance must be set before calling this!");
-  return *sInterpreters.back().Interpreter;
+  return *sInterpreters->back().Interpreter;
 }
 static clang::Sema& getSema() { return getInterp().getCI()->getSema(); }
 static clang::ASTContext& getASTContext() { return getSema().getASTContext(); }
@@ -2934,52 +2963,52 @@ TInterp_t CreateInterpreter(const std::vector<const char*>& Args /*={}*/,
     llvm::cl::ParseCommandLineOptions(NumArgs + 1, Args.get());
   }
 
-  sInterpreters.push_back({I, /*isOwned=*/true});
+  sInterpreters->emplace_back(I, /*isOwned=*/true);
 
   return I;
 }
 
 bool DeleteInterpreter(TInterp_t I /*=nullptr*/) {
   if (!I) {
-    sInterpreters.pop_back();
+    sInterpreters->pop_back();
     return true;
   }
 
-  auto* found =
-      std::find_if(sInterpreters.begin(), sInterpreters.end(),
+  auto found =
+      std::find_if(sInterpreters->begin(), sInterpreters->end(),
                    [&I](const auto& Info) { return Info.Interpreter == I; });
-  if (found == sInterpreters.end())
+  if (found == sInterpreters->end())
     return false; // failure
 
-  sInterpreters.erase(found);
+  sInterpreters->erase(found);
   return true;
 }
 
 bool ActivateInterpreter(TInterp_t I) {
   if (!I)
     return false;
 
-  auto* found =
-      std::find_if(sInterpreters.begin(), sInterpreters.end(),
+  auto found =
+      std::find_if(sInterpreters->begin(), sInterpreters->end(),
                    [&I](const auto& Info) { return Info.Interpreter == I; });
-  if (found == sInterpreters.end())
+  if (found == sInterpreters->end())
     return false;
 
-  if (std::next(found) != sInterpreters.end()) // if not already last element.
-    std::rotate(found, found + 1, sInterpreters.end());
+  if (std::next(found) != sInterpreters->end()) // if not already last element.
+    std::rotate(found, found + 1, sInterpreters->end());
 
   return true; // success
 }
 
 TInterp_t GetInterpreter() {
-  if (sInterpreters.empty())
+  if (sInterpreters->empty())
     return nullptr;
-  return sInterpreters.back().Interpreter;
+  return sInterpreters->back().Interpreter;
 }
 
 void UseExternalInterpreter(TInterp_t I) {
-  assert(sInterpreters.empty() && "sInterpreter already in use!");
-  sInterpreters.push_back(
+  assert(sInterpreters->empty() && "sInterpreter already in use!");
+  sInterpreters->push_back(
       {static_cast<compat::Interpreter*>(I), /*isOwned=*/false});
 }