Fix TransactionPool placement new and delete.

TransactionPool was using placement new and delete improperly which could lead to Transaction::~Transaction running twice.
Make Transaction constructor private to make sure TransactionPool has allocated all Transactions it contains.
Remove "*Very useful for debugging purposes" leak on every allocation.

include/cling/Interpreter/Transaction.h

@@ -39,6 +39,7 @@ namespace llvm {
 
 namespace cling {
   class IncrementalExecutor;
+  class TransactionPool;
 
   ///\brief Contains information about the consumed input at once.
   ///
@@ -184,14 +185,20 @@ namespace cling {
     ///
     clang::FileID m_BufferFID;
 
-  public:
-
+    ///\brief This is all to support allocation via TransactionPool.
+    /// There is currently no way for TransactionPool to mark if a Transaction
+    /// originated there or elsewhere, so if this interface is needed
+    /// then TransactionPool will have to be rewritten (or even removed
+    /// as the performance gains seem negligable)
+    ///
+    friend class TransactionPool;
     Transaction(clang::Sema& S);
     Transaction(const CompilationOptions& Opts, clang::Sema& S);
+    ~Transaction();
 
     void Initialize(clang::Sema& S);
 
-    ~Transaction();
+  public:
 
     enum State {
       kCollecting,
@@ -511,7 +518,6 @@ namespace cling {
 
     void printStructureBrief(size_t nindent = 0) const;
 
-    friend class TransactionPool;
   private:
     bool comesFromASTReader(clang::DeclGroupRef DGR) const;
   };

lib/Interpreter/IncrementalParser.cpp

@@ -187,7 +187,7 @@ namespace cling {
   void
   IncrementalParser::Initialize(llvm::SmallVectorImpl<ParseResultTransaction>&
                                 result, bool isChildInterpreter) {
-    m_TransactionPool.reset(new TransactionPool(getCI()->getSema()));
+    m_TransactionPool.reset(new TransactionPool);
     if (hasCodeGenerator()) {
       getCodeGenerator()->Initialize(getCI()->getASTContext());
       m_BackendPasses.reset(new BackendPasses(getCI()->getCodeGenOpts(),
@@ -261,17 +261,16 @@ namespace cling {
   }
 
   IncrementalParser::~IncrementalParser() {
-    const Transaction* T = getFirstTransaction();
-    const Transaction* nextT = 0;
+    Transaction* T = const_cast<Transaction*>(getFirstTransaction());
     while (T) {
       assert((T->getState() == Transaction::kCommitted
               || T->getState() == Transaction::kRolledBackWithErrors
               || T->getState() == Transaction::kNumStates // reset from the pool
               || T->getState() == Transaction::kRolledBack)
              && "Not committed?");
-      nextT = T->getNext();
-      delete T;
-      T = nextT;
+      const Transaction* nextT = T->getNext();
+      m_TransactionPool->releaseTransaction(T, false);
+      T = const_cast<Transaction*>(nextT);
     }
   }
 
@@ -287,7 +286,7 @@ namespace cling {
   Transaction* IncrementalParser::beginTransaction(const CompilationOptions&
                                                    Opts) {
     Transaction* OldCurT = m_Consumer->getTransaction();
-    Transaction* NewCurT = m_TransactionPool->takeTransaction();
+    Transaction* NewCurT = m_TransactionPool->takeTransaction(m_CI->getSema());
     NewCurT->setCompilationOpts(Opts);
     // If we are in the middle of transaction and we see another begin
     // transaction - it must be nested transaction.

lib/Interpreter/TransactionPool.h

@@ -30,62 +30,48 @@ namespace cling {
     //
     llvm::SmallVector<Transaction*, POOL_SIZE>  m_Transactions;
 
-    ///\brief The Sema required by cling::Transactions' ctor.
-    ///
-    clang::Sema& m_Sema;
-
-    // We need to free them in blocks.
-    //
-    //llvm::SmallVector<Transaction*, 64> m_TransactionBlocks;
-#ifndef NDEBUG
-    bool m_Debug;
-#endif
-
-  private:
   public:
-    TransactionPool(clang::Sema& S) : m_Sema(S) {
-#ifndef NDEBUG
-      m_Debug = false;
-#endif
-    }
-
+    TransactionPool() {}
     ~TransactionPool() {
-      for (size_t i = 0, e = m_Transactions.size(); i < e; ++i)
-        delete m_Transactions[i];
+      // Only free the memory as anything put in m_Transactions will have
+      // already been destructed in releaseTransaction
+      for (Transaction* T : m_Transactions)
+        ::operator delete(T);
     }
 
-    Transaction* takeTransaction() {
-      if (m_Transactions.empty())
-        return new Transaction(m_Sema);
-      Transaction* T = new (m_Transactions.pop_back_val()) Transaction(m_Sema);
-#ifndef NDEBUG
-      // *Very useful for debugging purposes and setting breakpoints in gdb.
-      if (m_Debug)
-        T = new Transaction(m_Sema);
-#endif
+    Transaction* takeTransaction(clang::Sema& S) {
+      Transaction *T;
+      if (kDebugMode || m_Transactions.empty()) {
+        T = (Transaction*) ::operator new(sizeof(Transaction));
+        new(T) Transaction(S);
+      } else
+        T = new (m_Transactions.pop_back_val()) Transaction(S);
 
-      T->m_State = Transaction::kCollecting;
       return T;
     }
 
-    void releaseTransaction(Transaction* T) {
-      assert((T->getState() == Transaction::kCompleted ||
-              T->getState() == Transaction::kRolledBack)
-             && "Transaction must completed!");
+    void releaseTransaction(Transaction* T, bool reuse = true) {
+      if (reuse) {
+        assert((T->getState() == Transaction::kCompleted ||
+                T->getState() == Transaction::kRolledBack)
+               && "Transaction must completed!");
+      }
 
       // Tell the parent that T is gone.
       if (T->getParent())
         T->getParent()->removeNestedTransaction(T);
 
-      if (m_Transactions.size() == POOL_SIZE) {
-        // don't overflow the pool
-        delete T;
-        return;
-      }
-      T->m_State = Transaction::kNumStates;
       T->~Transaction();
-      m_Transactions.push_back(T);
+
+      // don't overflow the pool
+      if (reuse && (m_Transactions.size() < POOL_SIZE)) {
+        T->m_State = Transaction::kNumStates;
+        m_Transactions.push_back(T);
+      }
+      else
+       ::operator delete(T);
     }
+
 #undef POOL_SIZE
 #undef TRANSACTIONS_IN_BLOCK
   };

test/CodeUnloading/TransactionPool.C

@@ -0,0 +1,15 @@
+//------------------------------------------------------------------------------
+// CLING - the C++ LLVM-based InterpreterG :)
+//
+// This file is dual-licensed: you can choose to license it under the University
+// of Illinois Open Source License or the GNU Lesser General Public License. See
+// LICENSE.TXT for details.
+//------------------------------------------------------------------------------
+
+// RUN: cat %s | %cling -I %S -Xclang -verify
+// Test transactionPoolReuse
+
+.undo
+
+//expected-no-diagnostics
+.q