Fix crash when undoing past the first print transaction. #160

marsupial · 2017-06-23T18:06:15Z

The use of a static makes this crash, but also makes it so a second interpreter in a session would never work. It also merges all Transactions generated by printing, so a statement become atomic for undo rather than a series unrelated Transactions (which abstracts the implementation away from the user a bit better).

Was

// Startup transactions
<cling::Transaction* 0x7fae5385a800 isEmpty=0 isCommitted=1> 
`   <cling::Transaction* 0x7fae538ec000 isEmpty=0 isCommitted=1> 
// Trigger {} Tr{}
<cling::Transaction* 0x7fae53858a00 isEmpty=0 isCommitted=1> 
<cling::Transaction* 0x7fae54154200 isEmpty=0 isCommitted=1> 
<cling::Transaction* 0x7fae53939000 isEmpty=0 isCommitted=1> 
<cling::Transaction* 0x7fae550e7400 isEmpty=0 isCommitted=1>

Becomes

// Startup transactions
<cling::Transaction* 0x7f9fd104ac00 isEmpty=0 isCommitted=1> 
`   <cling::Transaction* 0x7f9fd10d1400 isEmpty=0 isCommitted=1> 
// Trigger {} Tr{}
<cling::Transaction* 0x7f9fd11c0200 isEmpty=0 isCommitted=1> 
`   <cling::Transaction* 0x7f9fd2028000 isEmpty=0 isCommitted=1> 
`   <cling::Transaction* 0x7f9fd1481200 isEmpty=0 isCommitted=1> 
`   <cling::Transaction* 0x7f9fd219c000 isEmpty=0 isCommitted=1>

This includes and is dependent on #157 & #159.

Axel-Naumann

Very nice, I like the approach! I believe most of my comments are on implementation details, nothing fundamental.

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/IncrementalParser.h

+    ///\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);


I don't like that prev boolean as an interface. Can we change the interface into mergeNextTransactions(Transaction* into); that merges transactions after the argument into the argument? And then either pass T or T->getNext()?

To me that couples the interfaces in a way that is undesirable. That is: to use the function (or wrapper class) one would also need to know an implementation detail of Transaction. A simple bool avoids this.

I do not understand your argument.

To use this functionality I don't think a requirement should be an #include "Transaction.h" and the caller knowing how Transactions are implemented.

The difference is more complicated than T or T->getNext(). As Previous get's the Transaction before what was passed.

Okay, we don't agree. Let's try from a different angle: do you actually need that argument? All calls seem to pass true. Can we just remove the argument and rename this to mergeTransactionsInfoPrevious(const Transaction *T)?

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/Interpreter.cpp

@@ -141,6 +141,16 @@ namespace cling {
    }
  }

+  Interpreter::TransactionMerge::TransactionMerge(Interpreter *interp,
+                                                  bool prev) :


ditto for prev

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/Interpreter.cpp

@@ -1193,20 +1203,40 @@ namespace cling {
      return kSuccess;
    }

+    const Transaction *prntT = m_CachedTrns[kPrintValueTransaction];


Rename to printTBefore?

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/Interpreter.cpp

+      // 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] ) {


No space after '(' and before ')', please.

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/Interpreter.cpp

+      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


"As the stack is unwinded from recursive calls to EvaluateInternal" are you saying that m_CachedTrns must be a top-most transaction? (I believe so - unload() can only unload top-most transactions.) If so, could you state that explicitly?

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/IncrementalParser.cpp

+          merge.pop_back();
+        break;
+      }
+    }


If T is not a top-most transaction, or unloaded, or rolled back, then it will not be part of m_Transactions. In that case, your code currently merges all transactions into the first (if I read your code correctly).

You should at least assert that you've seen T in m_Transactions. I'd even simply use find() instead of building the auxiliary SmallVector merge.

No it checks merge.size()>1 which I'll change to !merge.empty().
The auxiliary vector is used for keeping and inspecting the existing parent structure.

To be clearer "keeping and inspecting the existing parent structure," means that the Transactions must be cached as their real pointer values first, as iterators will be invalidated when fiddling with the nesting structure.

I don't think your comment addresses my concern:

If I call IncrementalParser::mergeTransactionsAfter((Transaction*)0x123, prev) it will merge all transactions into the first. Or crash. Probably depending on the value of prev. I don't see how !merge.empty() prevents that, as merge will contain all transactions.

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/IncrementalParser.cpp

+        // 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


patent->getNext() must point to the first transaction to be merged. You should probably assert that, and then set it to nullptr, because after mergeTransactionsAfter() is done the parent aught to not have a subsequent transaction anymore.

I tried to make it more apparent, but considering the case:

<cling::Transaction* A isEmpty=0 isCommitted=1> <cling::Transaction* B isEmpty=0 isCommitted=1> <cling::Transaction* C isEmpty=0 isCommitted=1> <cling::Transaction* D isEmpty=0 isCommitted=1> <cling::Transaction* E isEmpty=0 isCommitted=1> Interp->mergeTransactionsAfter(A)

As each lower Transaction is put into Parent(A), Parent should absorb whatever next the Child had, until finally reaching E where it's next is nullptr.

Let me try again. In the loop you do

Parent->setNext(const_cast<Transaction*>(CurChild->getNext()));

In the end you do

assert(Parent->getNext() == nullptr && "Parent still has next");

Why do you update Parent->getName() during the loop?

Because as it loops it is stealing next from whatever it is absorbing.

<cling::Transaction* A isEmpty=0 isCommitted=1> <cling::Transaction* B isEmpty=0 isCommitted=1> <cling::Transaction* C isEmpty=0 isCommitted=1> <cling::Transaction* D isEmpty=0 isCommitted=1> <cling::Transaction* E isEmpty=0 isCommitted=1> A.Next = B, B.Next = C, C.Next = D, D.Next = NULL Interp->mergeTransactionsAfter(A): Make B a child: A.Next = B.Next, B.Next = 0 Make C a child: A.Next = C.Next, C.Next = 0 Make D a child: A.Next = D.Next, D.Next = 0 Make D a child: A.Next = E.Next, E.Next = 0

If an exception is thrown before all children could be nested, then it should hopefully be in a state that is valid.

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/IncrementalParser.cpp

+          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


Are you saying `m_Transaction" contains nested transactions? IIRC that would be a bug... I.e. this should probably be an assert.

Axel-Naumann · 2017-06-27T09:42:42Z

lib/Interpreter/IncrementalParser.cpp

@@ -280,6 +280,55 @@ namespace cling {
    return m_Consumer->getTransaction();
  }

+  const Transaction*


The return type seems unused. Can we make it void until we need something?

Axel-Naumann · 2017-06-27T09:42:42Z

test/ErrorRecovery/ABI/cling/Interpreter/RuntimePrintValue.h

@@ -0,0 +1,2 @@
+
+ERROR;


:-) Could you maybe add a comment that this is meant to simulate a parsing error of cling's original RuntimePrintValue.h?

…tring for undefined values.

…esizer::Transform.

…nd report errors.

…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.

… be loaded.

…PrintValue.h.

marsupial force-pushed the marsupial:PR/6 branch from 8af6636 to 7d7ae03 Jun 25, 2017

Axel-Naumann requested changes Jun 27, 2017

View changes

marsupial force-pushed the marsupial:PR/6 branch 8 times, most recently from 75ba416 to 1e519a0 Jun 27, 2017

marsupial added 10 commits Jan 27, 2017

Report executePrintValue errors with DiagnosticsEngine.

5c08f31

Fix crash when dumping an un-evaluated cling::Value and standardize s…

Loading status checks…

849b9fd

…tring for undefined values.

Propagate errors and report them a bit better in ValueExtractionSynth…

80c87cc

…esizer::Transform.

Fix crash in ValueExtractionSynthesizer::FindAndCacheRuntimeDecls() a…

d84b494

…nd report errors.

Report ValueExtractionSynthesizer errors with DiagnosticsEngine.

07279aa

Use DiagnosticsEngine to report error when RuntimePrintValue.h cannot…

32000ba

… be loaded.

Check if CLING_RUNTIME_PRINT_VALUE_H defined before including Runtime…

41fc3b7

…PrintValue.h.

Fix undo value printing tests for CERN/master.

9a6782d

Partially revert "Fix test for CERN/master."

Loading status checks…

501aad9

marsupial force-pushed the marsupial:PR/6 branch from 1e519a0 to 501aad9 Jul 7, 2017

Oct	NOV	Dec
	19
2019	2020	2021

root-project / cling

Fix crash when undoing past the first print transaction. #160

Fix crash when undoing past the first print transaction. #160

marsupial commented Jun 23, 2017

Axel-Naumann left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

root-project / cling

Join GitHub today

GitHub is where the world builds software

Fix crash when undoing past the first print transaction. #160

Fix crash when undoing past the first print transaction. #160

Conversation

marsupial commented Jun 23, 2017

Axel-Naumann left a comment

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 29, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 29, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

marsupial Jun 27, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 29, 2017 Member

This comment has been minimized.

marsupial Jun 29, 2017 Author Contributor

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

This comment has been minimized.

Axel-Naumann Jun 27, 2017 Member

Essential cookies

Always active

Analytics cookies

Axel-Naumann Jun 27, 2017
Member

marsupial Jun 27, 2017
Author Contributor

Axel-Naumann Jun 27, 2017
Member

marsupial Jun 27, 2017
Author Contributor

Axel-Naumann Jun 29, 2017
Member

Axel-Naumann Jun 27, 2017
Member

marsupial Jun 27, 2017
Author Contributor

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member

marsupial Jun 27, 2017
Author Contributor

marsupial Jun 27, 2017
Author Contributor

Axel-Naumann Jun 29, 2017
Member

Axel-Naumann Jun 27, 2017
Member

marsupial Jun 27, 2017
Author Contributor

Axel-Naumann Jun 29, 2017
Member

marsupial Jun 29, 2017
Author Contributor

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member

Axel-Naumann Jun 27, 2017
Member