Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

marsupial
Copy link
Contributor

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.

Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

///\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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand your argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

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

Interpreter::TransactionMerge::TransactionMerge(Interpreter *interp,
bool prev) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for prev

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for prev

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

const Transaction *prntT = m_CachedTrns[kPrintValueTransaction];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to printTBefore?

// 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] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"unwound"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

merge.pop_back();
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

const Transaction*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,2 @@

ERROR;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@marsupial marsupial force-pushed the PR/6 branch 8 times, most recently from 75ba416 to 1e519a0 Compare June 30, 2017 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants