Internals: Make const iterator to fix missed-edits on dump. Merge from bug683 branch.

src/V3Ast.cpp

@@ -239,6 +239,9 @@ inline void AstNode::debugTreeChange(const char* prefix, int lineno, bool next)
     //if (debug()) cout<<"-treeChange: V3Ast.cpp:"<<lineno<<" Tree Change for "<<prefix<<": "<<(void*)this<<" <e"<<AstNode::s_editCntGbl<<">"<<endl;
     //if (debug()) {
     //	cout<<"-treeChange: V3Ast.cpp:"<<lineno<<" Tree Change for "<<prefix<<endl;
+    //	// Commenting out the section below may crash, as the tree state
+    //	// between edits is not always consistent for printing
+    //	cout<<"-treeChange: V3Ast.cpp:"<<lineno<<" Tree Change for "<<prefix<<endl;
     //	v3Global.rootp()->dumpTree(cout,"-treeChange: ");
     //	if (next||1) this->dumpTreeAndNext(cout, prefix);
     //	else this->dumpTree(cout, prefix);
@@ -561,7 +564,7 @@ void AstNode::relink(AstNRelinker* linkerp) {
     if (linkerp->m_iterpp) {
 	// If we're iterating over a next() link, we need to follow links off the
 	// NEW node.  Thus we pass iteration information via a pointer in the node.
-	// This adds a unfortunate 4 bytes to every AstNode, but is faster than passing
+	// This adds a unfortunate hot 8 bytes to every AstNode, but is faster than passing
 	// across every function.
 	// If anyone has a cleaner way, I'd be grateful.
 	*(linkerp->m_iterpp) = newp;
@@ -750,6 +753,20 @@ void AstNode::iterateChildren(AstNVisitor& v, AstNUser* vup) {
     if (m_op4p) m_op4p->iterateAndNext(v, vup);
 }
 
+void AstNode::iterateChildrenConst(AstNVisitor& v, AstNUser* vup) {
+    // This is a very hot function
+    if (!this) return;
+    ASTNODE_PREFETCH(m_op1p);
+    ASTNODE_PREFETCH(m_op2p);
+    ASTNODE_PREFETCH(m_op3p);
+    ASTNODE_PREFETCH(m_op4p);
+    // if () not needed since iterateAndNext accepts null this, but faster with it.
+    if (m_op1p) m_op1p->iterateAndNextConst(v, vup);
+    if (m_op2p) m_op2p->iterateAndNextConst(v, vup);
+    if (m_op3p) m_op3p->iterateAndNextConst(v, vup);
+    if (m_op4p) m_op4p->iterateAndNextConst(v, vup);
+}
+
 void AstNode::iterateAndNext(AstNVisitor& v, AstNUser* vup) {
     // This is a very hot function
     // IMPORTANT: If you replace a node that's the target of this iterator,
@@ -764,16 +781,18 @@ void AstNode::iterateAndNext(AstNVisitor& v, AstNUser* vup) {
     while (nodep) {
 	AstNode* niterp = nodep;  // This address may get stomped via m_iterpp if the node is edited
 	ASTNODE_PREFETCH(nodep->m_nextp);
+	// Desirable check, but many places where multiple iterations are OK
+	//if (VL_UNLIKELY(niterp->m_iterpp)) niterp->v3fatalSrc("IterateAndNext under iterateAndNext may miss edits");
 	// cppcheck-suppress nullPointer
 	niterp->m_iterpp = &niterp;
 	niterp->accept(v, vup);
 	// accept may do a replaceNode and change niterp on us...
 	//if (niterp != nodep) UINFO(1,"iterateAndNext edited "<<(void*)nodep<<" now into "<<(void*)niterp<<endl);  // niterp maybe NULL, so need cast
-	if (!niterp) return;
+	if (!niterp) return;  // Perhaps node deleted inside accept
 	niterp->m_iterpp = NULL;
-	if (VL_UNLIKELY(niterp!=nodep)) { // Edited it
+	if (VL_UNLIKELY(niterp!=nodep)) { // Edited node inside accept
 	    nodep = niterp;
-	} else {  // Same node, just loop
+	} else {  // Unchanged node, just continue loop
 	    nodep = niterp->m_nextp;
 	}
     }
@@ -799,11 +818,12 @@ void AstNode::iterateChildrenBackwards(AstNVisitor& v, AstNUser* vup) {
     this->op4p()->iterateListBackwards(v,vup);
 }
 
-void AstNode::iterateAndNextIgnoreEdit(AstNVisitor& v, AstNUser* vup) {
+void AstNode::iterateAndNextConst(AstNVisitor& v, AstNUser* vup) {
     // Keep following the current list even if edits change it
     if (!this) return;
     for (AstNode* nodep=this; nodep; ) {
 	AstNode* nnextp = nodep->m_nextp;
+	ASTNODE_PREFETCH(nnextp);
 	nodep->accept(v, vup);
 	nodep = nnextp;
     }

src/V3Ast.h

@@ -1186,9 +1186,11 @@ public:
     virtual void accept(AstNVisitor& v, AstNUser* vup=NULL) = 0;
     void	iterate(AstNVisitor& v, AstNUser* vup=NULL) { this->accept(v,vup); } 	  // Does this; excludes following this->next
     void	iterateAndNext(AstNVisitor& v, AstNUser* vup=NULL);
-    void	iterateAndNextIgnoreEdit(AstNVisitor& v, AstNUser* vup=NULL);
+    void	iterateAndNextConst(AstNVisitor& v, AstNUser* vup=NULL);
+    void	iterateAndNextIgnoreEdit(AstNVisitor& v, AstNUser* vup=NULL) { iterateAndNextConst(v, vup); }
     void	iterateChildren(AstNVisitor& v, AstNUser* vup=NULL);  // Excludes following this->next
     void	iterateChildrenBackwards(AstNVisitor& v, AstNUser* vup=NULL);  // Excludes following this->next
+    void	iterateChildrenConst(AstNVisitor& v, AstNUser* vup=NULL);  // Excludes following this->next
     AstNode*	acceptSubtreeReturnEdits(AstNVisitor& v, AstNUser* vup=NULL);  // Return edited nodep; see comments in V3Ast.cpp
 
     // CONVERSION

src/V3AstConstOnly.h

@@ -0,0 +1,33 @@
+// -*- mode: C++; c-file-style: "cc-mode" -*-
+//*************************************************************************
+// DESCRIPTION: Verilator: Ast node structure
+//
+// Code available from: http://www.veripool.org/verilator
+//
+//*************************************************************************
+//
+// Copyright 2003-2014 by Wilson Snyder.  This program is free software; you can
+// redistribute it and/or modify it under the terms of either the GNU
+// Lesser General Public License Version 3 or the Perl Artistic License
+// Version 2.0.
+//
+// Verilator is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+//*************************************************************************
+
+#ifndef _V3ASTCONSTONLY_H_
+#define _V3ASTCONSTONLY_H_ 1
+
+// Include only in visitors that do not not edit nodes, so should use constant iterators
+#define iterateAndNext   error_use_iterateAndNextConst
+#define iterateChildren  error_use_iterateChildrenConst
+
+#define addNext          error_no_addNext_in_ConstOnlyVisitor
+#define replaceWith      error_no_replaceWith_in_ConstOnlyVisitor
+#define deleteTree       error_no_deleteTree_in_ConstOnlyVisitor
+#define unlinkFrBack     error_no_unlinkFrBack_in_ConstOnlyVisitor
+
+#endif // Guard

src/V3Broken.cpp

@@ -37,6 +37,9 @@
 #include "V3Broken.h"
 #include "V3Ast.h"
 
+// This visitor does not edit nodes, and is called at error-exit, so should use constant iterators
+#include "V3AstConstOnly.h"
+
 //######################################################################
 
 class BrokenTable : public AstNVisitor {
@@ -185,7 +188,7 @@ private:
     // VISITORS
     virtual void visit(AstNode* nodep, AstNUser*) {
 	BrokenTable::addInTree(nodep, nodep->maybePointedTo());
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
     }
 public:
     // CONSTUCTORS
@@ -228,7 +231,7 @@ private:
 		nodep->v3fatalSrc("Width != WidthMin");
 	    }
 	}
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
 	BrokenTable::setUnder(nodep,false);
     }
 public:

src/V3Stats.cpp

@@ -31,6 +31,9 @@
 #include "V3Ast.h"
 #include "V3File.h"
 
+// This visitor does not edit nodes, and is called at error-exit, so should use constant iterators
+#include "V3AstConstOnly.h"
+
 //######################################################################
 // Stats class functions
 
@@ -74,14 +77,14 @@ private:
     virtual void visit(AstNodeModule* nodep, AstNUser*) {
 	allNodes(nodep);
 	if (!m_fast) {
-	    nodep->iterateChildren(*this);
+	    nodep->iterateChildrenConst(*this);
 	} else {
 	    for (AstNode* searchp = nodep->stmtsp(); searchp; searchp=searchp->nextp()) {
 		if (AstCFunc* funcp = searchp->castCFunc()) {
 		    if (funcp->name() == "_eval") {
 			m_instrs=0;
 			m_counting = true;
-			funcp->iterateChildren(*this);
+			funcp->iterateChildrenConst(*this);
 			m_counting = false;
 		    }
 		}
@@ -90,7 +93,7 @@ private:
     }
     virtual void visit(AstVar* nodep, AstNUser*) {
 	allNodes(nodep);
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
 	if (m_counting && nodep->dtypep()) {
 	    if (nodep->isUsedClock()) ++m_statVarClock;
 	    if (nodep->dtypeSkipRefp()->castUnpackArrayDType()) ++m_statVarArray;
@@ -103,7 +106,7 @@ private:
     }
     virtual void visit(AstVarScope* nodep, AstNUser*) {
 	allNodes(nodep);
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
 	if (m_counting) {
 	    if (nodep->varp()->dtypeSkipRefp()->castBasicDType()) {
 		m_statVarScpBytes += nodep->varp()->dtypeSkipRefp()->widthTotalBytes();
@@ -114,13 +117,13 @@ private:
 	UINFO(4,"   IF "<<nodep<<endl);
 	allNodes(nodep);
 	// Condition is part of PREVIOUS block
-	nodep->condp()->iterateAndNext(*this);
+	nodep->condp()->iterateAndNextConst(*this);
 	// Track prediction
 	if (m_counting) {
 	    ++m_statPred[nodep->branchPred()];
 	}
 	if (!m_fast) {
-	    nodep->iterateChildren(*this);
+	    nodep->iterateChildrenConst(*this);
 	} else {
 	    // See which path we want to take
 	    bool takeElse = false;
@@ -135,11 +138,11 @@ private:
 		m_counting = false;
 		// Check if
 		m_instrs = 0;
-		nodep->ifsp()->iterateAndNext(*this);
+		nodep->ifsp()->iterateAndNextConst(*this);
 		double instrIf = m_instrs;
 		// Check else
 		m_instrs = 0;
-		nodep->elsesp()->iterateAndNext(*this);
+		nodep->elsesp()->iterateAndNextConst(*this);
 		double instrElse = m_instrs;
 		// Max of if or else condition
 		takeElse = (instrElse > instrIf);
@@ -150,9 +153,9 @@ private:
 	    // Count the block
 	    if (m_counting) {
 		if (takeElse) {
-		    nodep->elsesp()->iterateAndNext(*this);
+		    nodep->elsesp()->iterateAndNextConst(*this);
 		} else {
-		    nodep->ifsp()->iterateAndNext(*this);
+		    nodep->ifsp()->iterateAndNextConst(*this);
 		}
 	    }
 	}
@@ -163,7 +166,7 @@ private:
     virtual void visit(AstCCall* nodep, AstNUser*) {
 	//UINFO(4,"  CCALL "<<nodep<<endl);
 	allNodes(nodep);
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
 	if (m_fast) {
 	    // Enter the function and trace it
 	    nodep->funcp()->accept(*this);
@@ -172,12 +175,12 @@ private:
     virtual void visit(AstCFunc* nodep, AstNUser*) {
 	m_cfuncp = nodep;
 	allNodes(nodep);
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
 	m_cfuncp = NULL;
     }
     virtual void visit(AstNode* nodep, AstNUser*) {
 	allNodes(nodep);
-	nodep->iterateChildren(*this);
+	nodep->iterateChildrenConst(*this);
     }
 public:
     // CONSTRUCTORS