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,19 @@ 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 TransactionMerge {
+      IncrementalParser& m_IncrParser;
+      const Transaction* m_Current;
+      const bool m_Prev;
+
+    public:
+      TransactionMerge(Interpreter* Interp, bool Prev);
+      ~TransactionMerge();
+    };
+
     ///\brief Describes the return result of the different routines that do the
     /// incremental compilation.
     ///
@@ -199,6 +212,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 +794,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

@@ -280,6 +280,55 @@ namespace cling {
     return m_Consumer->getTransaction();
   }
 
+  const Transaction*
+  IncrementalParser::mergeTransactionsAfter(const Transaction *T, bool Prev) {
+    llvm::SmallVector<Transaction*, 8> merge;
+    for (std::deque<Transaction*>::reverse_iterator itr = m_Transactions.rbegin(),
+            end  = m_Transactions.rend(); itr != end; ++itr) {
+      Transaction *cur = *itr;
+      merge.push_back(cur);
+      if (cur==T) {
+        if (!Prev)
+          merge.pop_back();
+        break;
+      }
+    }
+    if (merge.size()>1) {
+      llvm::SmallVector<Transaction*, 8>::reverse_iterator itr = merge.rbegin();
+      Transaction *parent = *itr;
+      ++itr;
+      for (llvm::SmallVector<Transaction*, 8>::reverse_iterator end = merge.rend();
+           itr != end; ++itr) {
+        Transaction *cur = *itr;
+        // Try to keep everything current
+        if (m_Consumer->getTransaction()==cur)
+          m_Consumer->setTransaction(parent);
+        if (cur==parent->getNext()) // Make sure parent->next isn't a child
+          parent->setNext(const_cast<Transaction*>(cur->getNext()));
+        while (cur->getParent())   // Keep parents in place
+          cur = cur->getParent();
+
+        // Lets just make them match until there's a decent test case
+        assert((cur->getIssuedDiags() != Transaction::kErrors ||
+               parent->getIssuedDiags() == Transaction::kErrors)
+              && "Cannot merge transactions with different error states");
+        parent->addNestedTransaction(cur);
+
+        // TODO: Propogate child's error state into parent.
+        //
+        // Once addNestedTransaction finishes we can't get the child's errors
+        // const Transaction::IssuedDiags childErr = cur->getIssuedDiags();
+        // parent->addNestedTransaction(cur);
+        // if (issued < parent->getIssuedDiags())
+        //   parent->setIssuedDiags(childErr);
+
+        m_Transactions.pop_back();
+      }
+      return parent;
+    }
+    return getCurrentTransaction();
+  }
+  
   SourceLocation IncrementalParser::getLastMemoryBufferEndLoc() const {
     const SourceManager& SM = getCI()->getSourceManager();
     SourceLocation Result = SM.getLocForStartOfFile(m_VirtualFileID);

lib/Interpreter/IncrementalParser.h

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

lib/Interpreter/Interpreter.cpp

@@ -141,6 +141,16 @@ namespace cling {
     }
   }
 
+  Interpreter::TransactionMerge::TransactionMerge(Interpreter *interp,
+                                                  bool prev) :
+    m_IncrParser(*interp->m_IncrParser),
+    m_Current(m_IncrParser.getLastTransaction()), m_Prev(prev) {
+  }
+
+  Interpreter::TransactionMerge::~TransactionMerge() {
+    m_IncrParser.mergeTransactionsAfter(m_Current, m_Prev);
+  }
+
   const Parser& Interpreter::getParser() const {
     return *m_IncrParser->getParser();
   }
@@ -1193,20 +1203,40 @@ namespace cling {
       return kSuccess;
     }
 
+    const Transaction *prntT = 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 ( prntT != m_CachedTrns[kPrintValueTransaction] ) {
+        assert(m_CachedTrns[kPrintValueTransaction] != nullptr
+               && "Value printer failed to load after success?");
+        // As the stack is unwinded 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, true);
+        m_CachedTrns[kPrintValueTransaction] = lastT;
+      }
+
+      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 +1385,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::TransactionMerge M(Interp, true);
+      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