Fix crash when undoing past the first print transaction.

include/cling/Interpreter/Interpreter.h

@@ -102,6 +102,18 @@ namespace cling {
       ~StateDebuggerRAII();
     };
 
+    ///\brief Merge all transactions between construction and destruction
+    /// Use Prev to merge the Transactions with the last Transaction (true)
+    /// or the first Transaction created after instantiation (false).
+    class TransactionMergerRAII {
+      IncrementalParser& m_IncrParser;
+      const Transaction* m_T;
+
+    public:
+      TransactionMergerRAII(Interpreter* Interp);
+      ~TransactionMergerRAII();
+    };
+
     ///\brief Describes the return result of the different routines that do the
     /// incremental compilation.
     ///
@@ -199,6 +211,7 @@ namespace cling {
 
     enum {
       kStdStringTransaction = 0, // Transaction known to contain std::string
+      kPrintValueTransaction, // Transaction that included RuntimePrintValue.h
       kNumTransactions
     };
     mutable const Transaction* m_CachedTrns[kNumTransactions] = {};
@@ -780,6 +793,15 @@ namespace cling {
                           [](const clang::PresumedLoc&) { return false;}) const;
 
     friend class runtime::internal::LifetimeHandler;
+
+    ///\brief Used by valuePrinterInternal::printValueInternal to mark when the
+    /// value printing headers were included.
+    ///
+    ///\returns reference to m_CachedTrns[kPrintValueTransaction]
+    ///
+    const Transaction*& printValueTransaction() {
+      return m_CachedTrns[kPrintValueTransaction];
+    }
   };
 } // namespace cling
 

include/cling/Interpreter/RuntimePrintValue.h

@@ -21,14 +21,20 @@
 namespace cling {
 
   class Value;
+  namespace valuePrinterInternal {
+    extern const char* const kEmptyCollection;
+    extern const char* const kUndefined;
+  }
 
   // General fallback - prints the address
   std::string printValue(const void *ptr);
 
   // Fallback for e.g. vector<bool>'s bit iterator:
   template <class T,
-    class = typename std::enable_if<!std::is_pointer<T>::value>::type>
-  std::string printValue(const T& val) { return "{not representable}"; }
+            class = typename std::enable_if<!std::is_pointer<T>::value>::type>
+  std::string printValue(const T& val) {
+    return valuePrinterInternal::kUndefined;
+  }
 
   // void pointer
   std::string printValue(const void **ptr);
@@ -114,10 +120,6 @@ namespace cling {
   // cling::Value
   std::string printValue(const Value *value);
 
-  namespace valuePrinterInternal {
-    extern const char* const kEmptyCollection;
-  }
-
   // Collections internal
   namespace collectionPrinterInternal {
 

lib/Interpreter/IncrementalParser.cpp

@@ -280,6 +280,65 @@ namespace cling {
     return m_Consumer->getTransaction();
   }
 
+  void
+  IncrementalParser::mergeTransactionsAfter(const Transaction *T) {
+    bool Found = false;
+    llvm::SmallVector<Transaction*, 8> Merge;
+    for (auto Itr = m_Transactions.rbegin(), End  = m_Transactions.rend();
+         Itr != End; ++Itr) {
+      Transaction *Cur = *Itr;
+      Merge.push_back(Cur);
+      if ((Found = (Cur == T)))
+        break;
+    }
+    if (!Found) {
+      clang::DiagnosticsEngine& Diags = m_Interpreter->getDiagnostics();
+      const auto ID = Diags.getCustomDiagID(clang::DiagnosticsEngine::Warning,
+                                            "Transaction was not found.");
+      Diags.Report(m_Interpreter->getSourceLocation(), ID);
+      return; // Unused: nullptr
+    }
+    if (Merge.empty()) {
+      // Trying to merge the last Transaction with nothing following.
+      return; // Unused: getCurrentTransaction();
+    }
+    assert(!T->isNestedTransaction() && "New parent cannot be a child.");
+
+    auto Itr = Merge.rbegin();
+    Transaction *Parent = *Itr;
+    ++Itr;
+    for (auto End = Merge.rend(); Itr != End; ++Itr) {
+      Transaction *CurChild = *Itr;
+      // Try to keep everything current
+      if (m_Consumer->getTransaction() == CurChild)
+        m_Consumer->setTransaction(Parent);
+
+      // Keep parents in place
+      while (CurChild->getParent())
+        CurChild = CurChild->getParent();
+
+      // FIXME: Propogate child's error state into parent. Note the CurChild
+      // error state will have to be saved here, as once addNestedTransaction
+      // has finished it can no longer be retrieved.
+      //
+      // Just force diag states to match until there's a decent test case
+      assert((CurChild->getIssuedDiags() != Transaction::kErrors ||
+              Parent->getIssuedDiags() == Transaction::kErrors)
+             && "Cannot merge transactions with different error states");
+
+      Parent->addNestedTransaction(CurChild);
+
+      // Move whatever the CurChild.Next is into Parent
+      Parent->setNext(const_cast<Transaction*>(CurChild->getNext()));
+      // And make sure the CurChild.Next is empty
+      CurChild->setNext(nullptr);
+
+      m_Transactions.pop_back();
+    }
+    assert(Parent->getNext() == nullptr && "Parent still has next");
+    return; // Unused: parent;
+  }
+
   SourceLocation IncrementalParser::getLastMemoryBufferEndLoc() const {
     const SourceManager& SM = getCI()->getSourceManager();
     SourceLocation Result = SM.getLocForStartOfFile(m_VirtualFileID);

lib/Interpreter/IncrementalParser.h

@@ -176,6 +176,13 @@ namespace cling {
       return m_Transactions.back();
     }
 
+    ///\brief Merge transactions after the one given.
+    ///
+    ///\param[in] T - transactions after which to merge
+    ///\returns the parent transaction of the merge.
+    ///
+    void mergeTransactionsAfter(const Transaction *T);
+
     ///\brief Returns the currently active transaction.
     ///
     const Transaction* getCurrentTransaction() const;

lib/Interpreter/Interpreter.cpp

@@ -141,6 +141,13 @@ namespace cling {
     }
   }
 
+  Interpreter::TransactionMergerRAII::TransactionMergerRAII(Interpreter* I) :
+    m_IncrParser(*I->m_IncrParser), m_T(m_IncrParser.getLastTransaction()) {}
+
+  Interpreter::TransactionMergerRAII::~TransactionMergerRAII() {
+    m_IncrParser.mergeTransactionsAfter(m_T);
+  }
+
   const Parser& Interpreter::getParser() const {
     return *m_IncrParser->getParser();
   }
@@ -1193,20 +1200,42 @@ namespace cling {
       return kSuccess;
     }
 
+    const Transaction *printTBefore = m_CachedTrns[kPrintValueTransaction];
+
     Value resultV;
     if (!V)
       V = &resultV;
     if (!lastT->getWrapperFD()) // no wrapper to run
       return Interpreter::kSuccess;
-    else if (RunFunction(lastT->getWrapperFD(), V) < kExeFirstError){
-      if (lastT->getCompilationOpts().ValuePrinting
-          != CompilationOptions::VPDisabled
-          && V->isValid()
-          // the !V->needsManagedAllocation() case is handled by
-          // dumpIfNoStorage.
-          && V->needsManagedAllocation())
-        V->dump();
-      return Interpreter::kSuccess;
+    else {
+      ExecutionResult rslt = RunFunction(lastT->getWrapperFD(), V);
+
+      // If print value Transaction has changed value, then lastT is now
+      // the transaction that holds the transaction(s) of loading up the
+      // value printer and must be recorded as such.
+      if (printTBefore != m_CachedTrns[kPrintValueTransaction]) {
+        assert(m_CachedTrns[kPrintValueTransaction] != nullptr
+               && "Value printer failed to load after success?");
+        // As the stack is unwound from recursive calls to EvaluateInternal
+        // we merge the subsequent transactions into the current one.
+        // Then m_CachedTrns[kPrintValue] is marked as this transaction as it
+        // is the first known Transaction that caused the printer to be loaded.
+        m_IncrParser->mergeTransactionsAfter(lastT);
+        m_CachedTrns[kPrintValueTransaction] = lastT;
+        assert(!m_CachedTrns[kPrintValueTransaction]->isNestedTransaction() &&
+               "Print value transaction is not topmost parent");
+      }
+
+      if ( rslt < kExeFirstError) {
+        if (lastT->getCompilationOpts().ValuePrinting
+            != CompilationOptions::VPDisabled
+            && V->isValid()
+            // the !V->needsManagedAllocation() case is handled by
+            // dumpIfNoStorage.
+            && V->needsManagedAllocation())
+          V->dump();
+        return Interpreter::kSuccess;
+      }
     }
     return Interpreter::kSuccess;
   }
@@ -1355,6 +1384,10 @@ namespace cling {
                       << i << " of " << numberOfTransactions << "\n";
         return;
       }
+      // Mark Interpreter as not having loaded 'RuntimePrintValue.h'
+      if (T == m_CachedTrns[kPrintValueTransaction])
+        m_CachedTrns[kPrintValueTransaction] = nullptr;
+
       unload(*T);
     }
   }

lib/Interpreter/Value.cpp

@@ -231,9 +231,15 @@ namespace cling {
   namespace valuePrinterInternal {
     std::string printTypeInternal(const Value& V);
     std::string printValueInternal(const Value& V);
+    extern const char* const kInvalid;
   } // end namespace valuePrinterInternal
 
   void Value::print(llvm::raw_ostream& Out, bool Escape) const {
+    if (!m_Interpreter) {
+      Out << valuePrinterInternal::kInvalid << "\n";
+      return;
+    }
+
     // Save the default type string representation so output can occur as one
     // operation (calling printValueInternal below may write to stderr).
     const std::string Type = valuePrinterInternal::printTypeInternal(*this);