Cling: prevent double release of Transactions, take 2

interpreter/cling/include/cling/Interpreter/Interpreter.h

@@ -46,13 +46,13 @@ namespace clang {
   class NamedDecl;
   class Parser;
   class Preprocessor;
+  class PresumedLoc;
   class QualType;
   class RecordDecl;
   class Sema;
   class SourceLocation;
   class SourceManager;
   class Type;
-  class PresumedLoc;
 }
 
 namespace cling {
@@ -67,13 +67,13 @@ namespace cling {
   class ClangInternalState;
   class CompilationOptions;
   class DynamicLibraryManager;
+  class IncrementalCUDADeviceCompiler;
   class IncrementalExecutor;
   class IncrementalParser;
   class InterpreterCallbacks;
   class LookupHelper;
-  class Value;
   class Transaction;
-  class IncrementalCUDADeviceCompiler;
+  class Value;
 
   ///\brief Class that implements the interpreter-like behavior. It manages the
   /// incremental compilation.

interpreter/cling/include/cling/Interpreter/Transaction.h

@@ -139,6 +139,11 @@ namespace cling {
 
     unsigned m_IssuedDiags : 2;
 
+    ///\brief the Transaction is currently being unloaded. Currently,
+    /// used for ensuring system consistency when unloading transactions.
+    ///
+    bool m_Unloading : 1;
+
     ///\brief Options controlling the transformers and code generator.
     ///
     CompilationOptions m_Opts;
@@ -308,6 +313,8 @@ namespace cling {
       m_State = val;
     }
 
+    void setUnloading() { m_Unloading = true; }
+
     IssuedDiags getIssuedDiags() const {
       return static_cast<IssuedDiags>(getTopmostParent()->m_IssuedDiags);
     }

interpreter/cling/lib/Interpreter/Interpreter.cpp

@@ -1472,6 +1472,7 @@ namespace cling {
   }
 
   void Interpreter::unload(Transaction& T) {
+    T.setUnloading();
     // Clear any stored states that reference the llvm::Module.
     // Do it first in case
     auto Module = T.getModule();

interpreter/cling/lib/Interpreter/LookupHelper.cpp

@@ -722,7 +722,11 @@ namespace cling {
                         // in invalid state. We should be unloading all of them, i.e. inload the
                         // current (possibly nested) transaction.
                         auto *T = const_cast<Transaction*>(m_Interpreter->getCurrentTransaction());
-                        m_Interpreter->unload(*T);
+                        // Must not unload the Transaction, which might delete
+                        // it: the RAII above still points to it! Instead, just
+                        // mark it as "erroneous" which causes the RAII to
+                        // unload it in due time.
+                        T->setIssuedDiags(Transaction::kErrors);
                         *setResultType = nullptr;
                         return 0;
                       }

interpreter/cling/lib/Interpreter/Transaction.cpp

@@ -39,6 +39,7 @@ namespace cling {
     m_NestedTransactions.reset(0);
     m_Parent = 0;
     m_State = kCollecting;
+    m_Unloading = false;
     m_IssuedDiags = kNone;
     m_Opts = CompilationOptions();
     m_DefinitionShadowNS = 0;
@@ -94,6 +95,7 @@ namespace cling {
   }
 
   void Transaction::addNestedTransaction(Transaction* nested) {
+    assert(!m_Unloading && "Must not nest within unloading transaction");
     // Create lazily the list
     if (!m_NestedTransactions)
       m_NestedTransactions.reset(new NestedTransactions());
@@ -357,6 +359,8 @@ namespace cling {
     }
     cling::log() << indent << " state: " << stateNames[getState()]
                  << " decl groups, ";
+    if (m_Unloading)
+      cling::log() << "currently unloading, ";
     if (hasNestedTransactions())
       cling::log() << m_NestedTransactions->size();
     else

interpreter/cling/lib/Interpreter/TransactionPool.h

@@ -26,8 +26,7 @@ namespace cling {
 #else
       kDebugMode           = 1, // Always use a new Transaction
 #endif
-      kTransactionsInBlock = 8,
-      kPoolSize            = 2 * kTransactionsInBlock
+      kPoolSize            = 16
     };
 
     // It is twice the size of the block because there might be easily around 8
@@ -48,10 +47,9 @@ namespace cling {
 
     Transaction* takeTransaction(clang::Sema& S) {
       Transaction *T;
-      if (kDebugMode || m_Transactions.empty()) {
-        T = (Transaction*) ::operator new(sizeof(Transaction));
-        new(T) Transaction(S);
-      } else
+      if (kDebugMode || m_Transactions.empty())
+        T = new Transaction(S);
+      else
         T = new (m_Transactions.pop_back_val()) Transaction(S);
 
       return T;
@@ -60,10 +58,12 @@ namespace cling {
     // Transaction T must be from call to TransactionPool::takeTransaction
     //
     void releaseTransaction(Transaction* T, bool reuse = true) {
+      assert((m_Transactions.empty() || m_Transactions.back() != T) \
+             && "Transaction already in pool");
       if (reuse) {
         assert((T->getState() == Transaction::kCompleted ||
                 T->getState() == Transaction::kRolledBack)
-               && "Transaction must completed!");
+               && "Transaction must be completed!");
         // Force reuse to off when not in Debug mode
         if (kDebugMode)
           reuse = false;
@@ -73,15 +73,14 @@ namespace cling {
       if (T->getParent())
         T->getParent()->removeNestedTransaction(T);
 
-      T->~Transaction();
-
       // don't overflow the pool
       if (reuse && (m_Transactions.size() < kPoolSize)) {
         T->m_State = Transaction::kNumStates;
+        T->~Transaction();
         m_Transactions.push_back(T);
       }
       else
-       ::operator delete(T);
+       delete T;
     }
   };