From a509b6a21c2e4949d733c96e3286770f43999391 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Mon, 8 Feb 2016 22:15:44 -0500 Subject: [PATCH] Internals: Fix compares to null, ongoing part of bug1030. No functional change intended. --- src/V3Ast.cpp | 59 +++++++++++++++++++++++++++------------------- src/V3Ast.h | 9 ++++--- src/V3AstNodes.cpp | 3 +++ src/V3Const.cpp | 18 +++++--------- src/V3Inst.cpp | 3 ++- src/V3Slice.cpp | 3 +-- src/V3Unroll.cpp | 9 +++---- src/V3Width.cpp | 3 +-- src/verilog.y | 3 +-- 9 files changed, 58 insertions(+), 52 deletions(-) diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index 8ce029816..3bc51653f 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -235,20 +235,20 @@ inline void AstNode::debugTreeChange(const char* prefix, int lineno, bool next) #endif } -AstNode* AstNode::addNext(AstNode* newp) { +AstNode* AstNode::addNext(AstNode* nodep, AstNode* newp) { // Add to m_nextp, returns this - UASSERT(newp,"Null item passed to addNext\n"); - this->debugTreeChange("-addNextThs: ", __LINE__, false); + UDEBUGONLY(if (!newp) nodep->v3fatalSrc("Null item passed to addNext");); + nodep->debugTreeChange("-addNextThs: ", __LINE__, false); newp->debugTreeChange("-addNextNew: ", __LINE__, true); - if (!this) { + if (!nodep) { // verilog.y and lots of other places assume this return (newp); } else { // Find end of old list - AstNode* oldtailp = this; + AstNode* oldtailp = nodep; if (oldtailp->m_nextp) { if (oldtailp->m_headtailp) { oldtailp = oldtailp->m_headtailp; // This=beginning of list, jump to end - UASSERT(!oldtailp->m_nextp, "Node had next, but headtail says it shouldn't"); + UDEBUGONLY(if (oldtailp->m_nextp) nodep->v3fatalSrc("Node had next, but headtail says it shouldn't");); } else { // Though inefficent, we are occasionally passed a addNext in the middle of a list. while (oldtailp->m_nextp != NULL) oldtailp = oldtailp->m_nextp; @@ -267,20 +267,20 @@ AstNode* AstNode::addNext(AstNode* newp) { newp->editCountInc(); if (oldtailp->m_iterpp) *(oldtailp->m_iterpp) = newp; // Iterate on new item } - this->debugTreeChange("-addNextOut:", __LINE__, true); - return this; + nodep->debugTreeChange("-addNextOut:", __LINE__, true); + return nodep; } -AstNode* AstNode::addNextNull(AstNode* newp) { - if (!newp) return this; - return addNext(newp); +AstNode* AstNode::addNextNull(AstNode* nodep, AstNode* newp) { + if (!newp) return nodep; + return addNext(nodep, newp); } void AstNode::addNextHere(AstNode* newp) { // Add to m_nextp on exact node passed, not at the end. // This could be at head, tail, or both (single) // New could be head of single node, or list - UDEBUGONLY(UASSERT(dynamic_cast(this),"Null base node");); + UDEBUGONLY(UASSERT(dynamic_cast(this),"this should not be NULL");); UASSERT(newp,"Null item passed to addNext"); UASSERT(newp->backp()==NULL,"New node (back) already assigned?"); this->debugTreeChange("-addHereThs: ", __LINE__, false); @@ -605,6 +605,7 @@ void AstNode::swapWith (AstNode* bp) { // Clone AstNode* AstNode::cloneTreeIter() { + // private: Clone single node and children AstNode* newp = this->clone(); if (this->m_op1p) newp->op1p(this->m_op1p->cloneTreeIterList()); if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList()); @@ -617,9 +618,10 @@ AstNode* AstNode::cloneTreeIter() { } AstNode* AstNode::cloneTreeIterList() { - // Clone list of nodes, set m_headtailp + // private: Clone list of nodes, set m_headtailp AstNode* newheadp = NULL; AstNode* newtailp = NULL; + // Audited to make sure this is never NULL for (AstNode* oldp = this; oldp; oldp=oldp->m_nextp) { AstNode* newp = oldp->cloneTreeIter(); newp->m_headtailp = NULL; @@ -634,7 +636,7 @@ AstNode* AstNode::cloneTreeIterList() { } AstNode* AstNode::cloneTree(bool cloneNextLink) { - if (!this) return NULL; + if (!this) return NULL; // verilog.y relies on this this->debugTreeChange("-cloneThs: ", __LINE__, cloneNextLink); cloneClearTree(); AstNode* newp; @@ -655,6 +657,7 @@ AstNode* AstNode::cloneTree(bool cloneNextLink) { // Delete void AstNode::deleteNode() { + // private: Delete single node. Publicly call deleteTree() instead. UASSERT(m_backp==NULL,"Delete called on node with backlink still set\n"); editCountInc(); // Change links of old node so we coredump if used @@ -674,6 +677,8 @@ AstNode::~AstNode() { } void AstNode::deleteTreeIter() { + // private: Delete list of nodes. Publicly call deleteTree() instead. + // Audited to make sure this is never NULL for (AstNode* nodep=this, *nnextp; nodep; nodep=nnextp) { nnextp = nodep->m_nextp; // MUST be depth first! @@ -690,7 +695,6 @@ void AstNode::deleteTreeIter() { void AstNode::deleteTree() { // deleteTree always deletes the next link, because you must have called // unlinkFromBack or unlinkFromBackWithNext as appropriate before calling this. - if (!this) return; UASSERT(m_backp==NULL,"Delete called on node with backlink still set\n"); this->debugTreeChange("-delTree: ", __LINE__, true); this->editCountInc(); @@ -721,12 +725,10 @@ void AstNode::operator delete(void* objp, size_t size) { void AstNode::iterateChildren(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->iterateAndNext(v, vup); if (m_op2p) m_op2p->iterateAndNext(v, vup); if (m_op3p) m_op3p->iterateAndNext(v, vup); @@ -755,7 +757,7 @@ void AstNode::iterateAndNext(AstNVisitor& v, AstNUser* vup) { #ifdef VL_DEBUG // Otherwise too hot of a function for debug if (VL_UNLIKELY(nodep && !nodep->m_backp)) nodep->v3fatalSrc("iterateAndNext node has no back"); #endif - while (nodep) { + while (nodep) { // effectively: if (!this) return; // Callers rely on this 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 @@ -776,6 +778,7 @@ void AstNode::iterateAndNext(AstNVisitor& v, AstNUser* vup) { } void AstNode::iterateListBackwards(AstNVisitor& v, AstNUser* vup) { + UDEBUGONLY(UASSERT(dynamic_cast(this),"this should not be NULL");); AstNode* nodep=this; while (nodep->m_nextp) nodep=nodep->m_nextp; while (nodep) { @@ -795,8 +798,8 @@ void AstNode::iterateChildrenBackwards(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; ) { + if (!this) return; // A few cases could be cleaned up, but want symmetry with iterateAndNext + for (AstNode* nodep=this; nodep; ) { // effectively: if (!this) return; // Callers rely on this AstNode* nnextp = nodep->m_nextp; ASTNODE_PREFETCH(nnextp); nodep->accept(v, vup); @@ -843,6 +846,7 @@ AstNode* AstNode::acceptSubtreeReturnEdits(AstNVisitor& v, AstNUser* vup) { //====================================================================== void AstNode::cloneRelinkTree() { + // private: Cleanup clone() operation on whole tree. Publicly call cloneTree() instead. for (AstNode* nodep=this; nodep; nodep=nodep->m_nextp) { if (m_dtypep && m_dtypep->clonep()) { m_dtypep = m_dtypep->clonep()->castNodeDType(); @@ -859,7 +863,7 @@ void AstNode::cloneRelinkTree() { // Comparison bool AstNode::gateTreeIter() { - // Return true if the two trees are identical + // private: Return true if the two trees are identical if (!isGateOptimizable()) return false; if (m_op1p && !m_op1p->gateTreeIter()) return false; if (m_op2p && !m_op2p->gateTreeIter()) return false; @@ -869,7 +873,7 @@ bool AstNode::gateTreeIter() { } bool AstNode::sameTreeIter(AstNode* node1p, AstNode* node2p, bool ignNext, bool gateOnly) { - // Return true if the two trees are identical + // private: Return true if the two trees are identical if (!node1p && !node2p) return true; if (!node1p || !node2p) return false; if (node1p->type() != node2p->type() @@ -906,6 +910,7 @@ V3Hash::V3Hash(const string& name) { // Debugging void AstNode::checkTreeIter(AstNode* backp) { + // private: Check a tree and children if (backp != this->backp()) { this->v3fatalSrc("Back node inconsistent"); } @@ -921,7 +926,8 @@ void AstNode::checkTreeIter(AstNode* backp) { } void AstNode::checkTreeIterList(AstNode* backp) { - // Check a (possible) list of nodes, this is always the head of the list + // private: Check a (possible) list of nodes, this is always the head of the list + // Audited to make sure this is never NULL AstNode* headp = this; AstNode* tailp = this; for (AstNode* nodep=headp; nodep; nodep=nodep->nextp()) { @@ -1008,6 +1014,7 @@ void AstNode::dumpTree(ostream& os, const string& indent, int maxDepth) { } void AstNode::dumpTreeAndNext(ostream& os, const string& indent, int maxDepth) { + // Audited to make sure this is never NULL for (AstNode* nodep=this; nodep; nodep=nodep->nextp()) { nodep->dumpTree(os, indent, maxDepth); } @@ -1042,7 +1049,11 @@ void AstNode::dumpTreeFile(const string& filename, bool append, bool doDump) { } void AstNode::v3errorEnd(ostringstream& str) const { - if (!this || !m_fileline) { + if (!dynamic_cast(this)) { + // No known cases cause this, but better than a core dump + if (debug()) UINFO(0, "-node: NULL. Please report this along with a --gdbbt backtrace as a Verilator bug.\n"); + V3Error::v3errorEnd(str); + } else if (!m_fileline) { V3Error::v3errorEnd(str); } else { ostringstream nsstr; diff --git a/src/V3Ast.h b/src/V3Ast.h index e44ade4b8..28d14395f 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -861,7 +861,8 @@ public: } #include "V3Ast__gen_visitor.h" // From ./astgen // Things like: - // virtual void visit(type*) = 0; + // virtual void visit(AstBreak* nodep, AstNUser* vup) { visit((AstNodeStmt*)(nodep),vup); } + // virtual void visit(AstNodeStmt* nodep, AstNUser* vup) { visit((AstNode*)(nodep),vup); } }; //###################################################################### @@ -1211,8 +1212,10 @@ public: void dumpGdbHeader() const; // METHODS - Tree modifications - AstNode* addNext(AstNode* newp); // Returns this, adds to end of list - AstNode* addNextNull(AstNode* newp); // Returns this, adds to end of list, NULL is OK + static AstNode* addNext(AstNode* nodep, AstNode* newp); // Returns nodep, adds newp to end of nodep's list + static AstNode* addNextNull(AstNode* nodep, AstNode* newp); // Returns nodep, adds newp (maybe NULL) to end of nodep's list + inline AstNode* addNext(AstNode* newp) { return addNext(this, newp); } + inline AstNode* addNextNull(AstNode* newp) { return addNextNull(this, newp); } void addNextHere(AstNode* newp); // Adds after speced node void addPrev(AstNode* newp) { replaceWith(newp); newp->addNext(this); } void addHereThisAsNext(AstNode* newp); // Adds at old place of this, this becomes next diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index ec43e8881..3b6294958 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -386,6 +386,7 @@ AstNodeDType* AstNodeDType::dtypeDimensionp(int dimension) { // TODO this function should be removed in favor of recursing the dtype(), // as that allows for more complicated data types. int dim = 0; + UDEBUGONLY(UASSERT(dynamic_cast(this),"this should not be NULL");); for (AstNodeDType* dtypep=this; dtypep; ) { dtypep = dtypep->skipRefp(); // Skip AstRefDType/AstTypedef, or return same node if (AstNodeArrayDType* adtypep = dtypep->castNodeArrayDType()) { @@ -420,6 +421,7 @@ AstNodeDType* AstNodeDType::dtypeDimensionp(int dimension) { uint32_t AstNodeDType::arrayUnpackedElements() { uint32_t entries=1; + UDEBUGONLY(UASSERT(dynamic_cast(this),"this should not be NULL");); for (AstNodeDType* dtypep=this; dtypep; ) { dtypep = dtypep->skipRefp(); // Skip AstRefDType/AstTypedef, or return same node if (AstUnpackArrayDType* adtypep = dtypep->castUnpackArrayDType()) { @@ -438,6 +440,7 @@ pair AstNodeDType::dimensions(bool includeBasic) { // How many array dimensions (packed,unpacked) does this Var have? uint32_t packed = 0; uint32_t unpacked = 0; + UDEBUGONLY(UASSERT(dynamic_cast(this),"this should not be NULL");); for (AstNodeDType* dtypep=this; dtypep; ) { dtypep = dtypep->skipRefp(); // Skip AstRefDType/AstTypedef, or return same node if (AstNodeArrayDType* adtypep = dtypep->castNodeArrayDType()) { diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 1524d389a..90d5069d4 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1037,10 +1037,8 @@ private: AstNodeAssign* asn2ap=nodep->cloneType(lc2p, sel2p)->castNodeAssign(); asn1ap->dtypeFrom(sel1p); asn2ap->dtypeFrom(sel2p); - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn1ap); - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn2ap); + newp = AstNode::addNext(newp, asn1ap); + newp = AstNode::addNext(newp, asn2ap); } else { if (!m_modp) nodep->v3fatalSrc("Not under module"); // We could create just one temp variable, but we'll get better optimization @@ -1070,14 +1068,10 @@ private: asn2ap->dtypeFrom(temp2p); asn2bp->dtypeFrom(temp2p); // This order matters - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn1ap); - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn2ap); - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn1bp); - // cppcheck-suppress nullPointer // addNext deals with it - newp = newp->addNext(asn2bp); + newp = AstNode::addNext(newp, asn1ap); + newp = AstNode::addNext(newp, asn2ap); + newp = AstNode::addNext(newp, asn1bp); + newp = AstNode::addNext(newp, asn2bp); } if (debug()>=9 && newp) newp->dumpTreeAndNext(cout," _new: "); nodep->addNextHere(newp); diff --git a/src/V3Inst.cpp b/src/V3Inst.cpp index 4e32fd695..08738f661 100644 --- a/src/V3Inst.cpp +++ b/src/V3Inst.cpp @@ -220,8 +220,9 @@ private: ifaceVarp->unlinkFrBack(); pushDeletep(ifaceVarp); VL_DANGLING(ifaceVarp); } nodep->unlinkFrBack(); pushDeletep(nodep); VL_DANGLING(nodep); + } else { + nodep->iterateChildren(*this); } - nodep->iterateChildren(*this); } virtual void visit(AstVar* nodep, AstNUser*) { diff --git a/src/V3Slice.cpp b/src/V3Slice.cpp index a61bdfefd..31946d5f5 100644 --- a/src/V3Slice.cpp +++ b/src/V3Slice.cpp @@ -423,8 +423,7 @@ class SliceVisitor : public AstNVisitor { AstNode* lhsp = new AstArraySel(nodep->fileline(), nodep->lhsp()->cloneTree(false), index++); - // cppcheck-suppress nullPointer - newp = newp->addNext(nodep->cloneType(lhsp, subp->unlinkFrBack())); + newp = AstNode::addNext(newp, nodep->cloneType(lhsp, subp->unlinkFrBack())); } //if (debug()>=9) newp->dumpTreeAndNext(cout, "-InitArrayOut: "); nodep->replaceWith(newp); diff --git a/src/V3Unroll.cpp b/src/V3Unroll.cpp index 0ca42dafc..083633362 100644 --- a/src/V3Unroll.cpp +++ b/src/V3Unroll.cpp @@ -280,18 +280,15 @@ private: } if (precondsp) { precondsp->unlinkFrBackWithNext(); - // cppcheck-suppress nullPointer // addNextNull deals with it - stmtsp = stmtsp->addNextNull(precondsp); + stmtsp = AstNode::addNextNull(stmtsp, precondsp); } if (bodysp) { bodysp->unlinkFrBackWithNext(); - // cppcheck-suppress nullPointer // addNextNull deals with it - stmtsp = stmtsp->addNextNull(bodysp); // Maybe null if no body + stmtsp = AstNode::addNextNull(stmtsp, bodysp); // Maybe null if no body } if (incp && !nodep->castGenFor()) { // Generates don't need to increment loop index incp->unlinkFrBackWithNext(); - // cppcheck-suppress nullPointer // addNextNull deals with it - stmtsp = stmtsp->addNextNull(incp); // Maybe null if no body + stmtsp = AstNode::addNextNull(stmtsp, incp); // Maybe null if no body } // Mark variable to disable some later warnings m_forVarp->usedLoopIdx(true); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 08f8a792b..4a59173b8 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -2243,8 +2243,7 @@ private: argp->unlinkFrBackWithNext(&handle); // Format + additional args, if any AstNode* argsp = NULL; while (AstArg* nextargp = argp->nextp()->castArg()) { - // cppcheck-suppress nullPointer - argsp = argsp->addNext(nextargp->exprp()->unlinkFrBackWithNext()); // Expression goes to SFormatF + argsp = AstNode::addNext(argsp, nextargp->exprp()->unlinkFrBackWithNext()); // Expression goes to SFormatF nextargp->unlinkFrBack()->deleteTree(); // Remove the call's Arg wrapper } string format; diff --git a/src/verilog.y b/src/verilog.y index a446874a4..a3b365cd4 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3692,8 +3692,7 @@ void V3ParseGrammar::argWrapList(AstNodeFTaskRef* nodep) { while (nodep->pinsp()) { AstNode* exprp = nodep->pinsp()->unlinkFrBack(); // addNext can handle nulls: - // cppcheck-suppress nullPointer - outp = outp->addNext(new AstArg(exprp->fileline(), "", exprp)); + outp = AstNode::addNext(outp, new AstArg(exprp->fileline(), "", exprp)); } if (outp) nodep->addPinsp(outp); }