[cling] teardown: correctly free frontend and retained JITLink allocations

interpreter/cling/lib/Interpreter/CIFactory.cpp

@@ -1622,7 +1622,9 @@ namespace {
     for (auto& E : moduleExtensions)
       FrontendOpts.ModuleFileExtensions.push_back(E);
 
-    FrontendOpts.DisableFree = true;
+    // The clang driver adds -disable-free to every cc1 line.
+    // Interpreter::ShutDown() should free the frontend.
+    FrontendOpts.DisableFree = false;
 
     // Set up compiler language and target
     if (!SetupCompiler(CI.get(), COpts, InitLang, InitTarget))

interpreter/cling/lib/Interpreter/IncrementalJIT.cpp

@@ -263,7 +263,7 @@ namespace {
     bool needsToReserveAllocationSpace() override { return true; }
   };
 
-  /// A JITLinkMemoryManager for Cling that never frees its allocations.
+  /// A JITLinkMemoryManager for Cling.
   class ClingJITLinkMemoryManager : public InProcessMemoryManager {
   public:
     using InProcessMemoryManager::InProcessMemoryManager;
@@ -273,16 +273,28 @@ namespace {
       // Disabled until CallFunc is informed about unloading, and can
       // re-generate the wrapper (if the decl is still available). See
       // https://github.com/root-project/root/issues/10898
-
-      // We still have to release the allocations which resets their addresses
-      // to FinalizedAlloc::InvalidAddr, or the assertion in ~FinalizedAlloc
-      // will be unhappy...
-      for (auto &Alloc : Allocs) {
-        Alloc.release();
-      }
-      // Pretend we successfully deallocated everything...
+      //
+      // Releasing the handles orphans each allocation's vector of JITLink
+      // dealloc actions. Retain them (required for CallFunc) and let the
+      // base class free everything when this manager is destroyed at
+      // interpreter teardown.
+      std::lock_guard<std::mutex> G(m_RetainedMutex);
+      for (auto& Alloc : Allocs)
+        m_Retained.push_back(std::move(Alloc));
       OnDeallocated(Error::success());
     }
+
+    ~ClingJITLinkMemoryManager() override {
+      if (!m_Retained.empty())
+        InProcessMemoryManager::deallocate(std::move(m_Retained),
+                                           [](Error Err) {
+                                             consumeError(std::move(Err));
+                                           });
+    }
+
+  private:
+    std::mutex m_RetainedMutex;
+    std::vector<FinalizedAlloc> m_Retained;
   };
 
   /// A DynamicLibrarySearchGenerator that uses ResourceTracker to remember

interpreter/cling/lib/Interpreter/Interpreter.cpp

@@ -379,27 +379,41 @@ namespace cling {
     }
   }
 
+
+// Temporary teardown tracing for the Windows CI crash in rootcling
+// (PR #23045). Remove before merge.
+#define CLING_TEARDOWN_TRACE(POINT)                                            \
+  do {                                                                         \
+    fprintf(stderr, "### CLING-TEARDOWN-TRACE: %s\n", POINT);                 \
+    fflush(stderr);                                                            \
+  } while (0)
   Interpreter::~Interpreter() {
+    CLING_TEARDOWN_TRACE("~Interpreter enter");
     // Do this first so m_StoredStates will be ignored if Interpreter::unload
     // is called later on.
     for (size_t i = 0, e = m_StoredStates.size(); i != e; ++i)
       delete m_StoredStates[i];
     m_StoredStates.clear();
+    CLING_TEARDOWN_TRACE("stored states cleared");
 
     if (m_Executor)
       m_Executor->shuttingDown();
+    CLING_TEARDOWN_TRACE("executor shuttingDown done");
 
     // LookupHelper's ~Parser needs the PP from IncrParser's CI, so do this
     // first:
     m_LookupHelper.reset();
+    CLING_TEARDOWN_TRACE("LookupHelper reset done");
 
     ShutDown();
+    CLING_TEARDOWN_TRACE("ShutDown returned");
 
     // We want to keep the callback alive during the shutdown of Sema, CodeGen
     // and the ASTContext. For that to happen we shut down the IncrementalParser
     // explicitly, before the implicit destruction (through the unique_ptr) of
     // the callbacks.
     m_IncrParser.reset(nullptr);
+    CLING_TEARDOWN_TRACE("IncrementalParser reset done; ~Interpreter body end");
   }
 
   Transaction* Interpreter::Initialize(bool NoRuntime, bool SyntaxOnly) {
@@ -587,22 +601,29 @@ namespace cling {
   }
 
   void Interpreter::ShutDown() {
+    CLING_TEARDOWN_TRACE("ShutDown enter");
     // Model the shutdown actions done in FrontendAction::EndSourceFile
     if (CompilerInstance* CI = getCIOrNull()) {
       CI->getDiagnostics().getClient()->EndSourceFile();
+      CLING_TEARDOWN_TRACE("diag client EndSourceFile done");
 
       if (CI->hasPreprocessor())
         CI->getPreprocessor().EndSourceFile();
+      CLING_TEARDOWN_TRACE("PP EndSourceFile done");
 
       bool DisableFree = CI->getFrontendOpts().DisableFree;
       if (DisableFree) {
         CI->resetAndLeakSema();
         CI->resetAndLeakASTContext();
         llvm::BuryPointer(CI->takeASTConsumer().get());
       } else {
+        CLING_TEARDOWN_TRACE("before setSema(nullptr)");
         CI->setSema(nullptr);
+        CLING_TEARDOWN_TRACE("before setASTContext(nullptr)");
         CI->setASTContext(nullptr);
+        CLING_TEARDOWN_TRACE("before setASTConsumer(nullptr)");
         CI->setASTConsumer(nullptr);
+        CLING_TEARDOWN_TRACE("frontend freed");
       }
 
       if (CI->getFrontendOpts().ShowStats) {
@@ -617,22 +638,23 @@ namespace cling {
       // Cleanup the output streams, and erase the output files if instructed by
       // the FrontendAction.
       bool shouldEraseOutputFiles = CI->getDiagnostics().hasErrorOccurred();
+      CLING_TEARDOWN_TRACE("before clearOutputFiles");
       CI->clearOutputFiles(/*EraseFiles=*/shouldEraseOutputFiles);
+      CLING_TEARDOWN_TRACE("clearOutputFiles done");
 
       LangOptions& LO = CI->getLangOpts();
       if (LO.getCompilingModule() != clang::LangOptions::CMK_None) {
         if (DisableFree) {
           CI->resetAndLeakPreprocessor();
           CI->resetAndLeakSourceManager();
           CI->resetAndLeakFileManager();
-        } else {
-          CI->setPreprocessor(nullptr);
-          CI->setSourceManager(nullptr);
-          CI->setFileManager(nullptr);
         }
+        // ~CompilerInstance destroys the Preprocessor, SourceManager and
+        // FileManager in the right order.
       }
 
       LO.setCompilingModule(clang::LangOptions::CMK_None);
+      CLING_TEARDOWN_TRACE("ShutDown exit");
     }
   }