Fix crash if transactions are unloaded past the first printing of an expression.

Fix crash when RuntimePrintValue.h cannot be loaded.
Merge value-printing transactions with the transaction that invoked it.

The latter fixes poor functionality in the .undo system where the user must have
knowledge of how many transactions are added when an expression is printed.

Author: marsupial

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
 

lib/Interpreter/IncrementalParser.cpp

@@ -278,6 +278,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/ValuePrinter.cpp

@@ -863,12 +863,30 @@ namespace cling {
 
     std::string printValueInternal(const Value &V) {
       assert(V.getInterpreter() && "Invalid cling::Value");
-      static bool includedRuntimePrintValue = false; // initialized only once as a static function variable
+
       // Include "RuntimePrintValue.h" only on the first printing.
       // This keeps the interpreter lightweight and reduces the startup time.
-      if (!includedRuntimePrintValue) {
-        V.getInterpreter()->declare("#include \"cling/Interpreter/RuntimePrintValue.h\"");
-        includedRuntimePrintValue = true;
+      // But user can undo past the transaction that invoked this, so whether
+      // we are first or not is known by the interpreter.
+      //
+      // Additionally add any transactions that occur in this scope as
+      // children to the transaction that invoked us
+
+      Interpreter* Interp = V.getInterpreter();
+      Interpreter::TransactionMergerRAII M(Interp);
+      const Transaction*& T = Interp->printValueTransaction();
+      if (!T) {
+        // DiagnosticErrorTrap Trap(Interp->getSema().getDiagnostics());
+        Interp->declare("#include \"cling/Interpreter/RuntimePrintValue.h\"",
+                        const_cast<Transaction**>(&T));
+        if (!T) {
+          // Should also check T->getIssuedDiags() == Transaction::kErrors)?
+          
+          // It's redundant, but nicer to see the error at the bottom.
+          // if (!Trap.hasErrorOccurred())
+          cling::errs() << "RuntimePrintValue.h could not be loaded.\n";
+          return kUndefined;
+        }
       }
       return printUnpackedClingValue(V);
     }

test/Prompt/ValuePrinter/UndoPrint.C

@@ -0,0 +1,67 @@
+//------------------------------------------------------------------------------
+// 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 -Xclang -verify 2>&1 | FileCheck %s
+// Test undoPrinter
+
+.stats undo
+//      CHECK: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+
+const char *message = "winkey";
+
+message
+// CHECK-NEXT: (const char *) "winkey"
+
+.undo
+
+// Make sure we can still print
+message
+// CHECK-NEXT: (const char *) "winkey"
+
+.undo
+.stats undo
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+
+message
+// CHECK-NEXT: (const char *) "winkey"
+
+.undo // print message
+.undo // decalre message
+.stats undo
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+
+#include "cling/Interpreter/Interpreter.h"
+
+gCling->echo("1;");
+// CHECK-NEXT: (int) 1
+
+.stats undo
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `      <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `      <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `      <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+
+.undo
+.stats undo
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: `   <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+// CHECK-NEXT: <cling::Transaction* 0x{{[0-9a-f]+}} isEmpty=0 isCommitted=1>
+
+gCling->echo("1;");
+// CHECK-NEXT: (int) 1
+
+// expected-no-diagnostics
+.q