diff --git a/src/V3Ast.h b/src/V3Ast.h index 9f8d12b91..9a26091ea 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -24,6 +24,7 @@ #include "V3FileLine.h" #include "V3Number.h" #include "V3Global.h" +#include "V3Broken.h" #include #include @@ -1335,21 +1336,29 @@ class AstNode VL_NOT_FINAL { // ^ ASTNODE_PREFETCH depends on above ordering of members // AstType is 2 bytes, so we can stick another 6 bytes after it to utilize what would - // otherwise be padding (on a 64-bit system). We stick the attribute flags and the clone - // count here. + // otherwise be padding (on a 64-bit system). We stick the attribute flags, broken state, + // and the clone count here. struct { bool didWidth : 1; // Did V3Width computation bool doingWidth : 1; // Inside V3Width bool protect : 1; // Protect name if protection is on - uint16_t unused : 13; // Space for more flags here (there must be 16 bits in total) + // Space for more flags here (there must be 8 bits in total) + uint8_t unused : 5; } m_flags; // Attribute flags + // State variable used by V3Broken for consistency checking. The top bit of this is byte is a + // flag, representing V3Broken is currently proceeding under this node. The bottom 7 bits are + // a generation number. This is hot when --debug-checks so we access as a whole to avoid bit + // field masking resulting in unnecessary read-modify-write ops. + uint8_t m_brokenState = 0; + int m_cloneCnt; // Sequence number for when last clone was made #if defined(__x86_64__) && defined(__gnu_linux__) // Only assert this on known platforms, as it only affects performance, not correctness - static_assert(sizeof(m_type) + sizeof(m_flags) + sizeof(m_cloneCnt) <= sizeof(void*), + static_assert(sizeof(m_type) + sizeof(m_flags) + sizeof(m_brokenState) + sizeof(m_cloneCnt) + <= sizeof(void*), "packing requires padding"); #endif @@ -1466,9 +1475,14 @@ public: AstNode* firstAbovep() const { // Returns nullptr when second or later in list return ((backp() && backp()->nextp() != this) ? backp() : nullptr); } - bool brokeExists() const; - bool brokeExistsAbove() const; - bool brokeExistsBelow() const; + uint8_t brokenState() const { return m_brokenState; } + void brokenState(uint8_t value) { m_brokenState = value; } + + // Used by AstNode::broken() + bool brokeExists() const { return V3Broken::isLinkable(this); } + bool brokeExistsAbove() const { return brokeExists() && (m_brokenState >> 7); } + bool brokeExistsBelow() const { return brokeExists() && !(m_brokenState >> 7); } + // Note: brokeExistsBelow is not quite precise, as it is true for sibling nodes as well // CONSTRUCTORS virtual ~AstNode() = default; diff --git a/src/V3Broken.cpp b/src/V3Broken.cpp index b9bae7707..49190f2eb 100644 --- a/src/V3Broken.cpp +++ b/src/V3Broken.cpp @@ -32,202 +32,129 @@ // This visitor does not edit nodes, and is called at error-exit, so should use constant iterators #include "V3AstConstOnly.h" -#include #include #include //###################################################################### +// Generation counter for AstNode::m_brokenState -class BrokenTable VL_NOT_FINAL : public AstNVisitor { - // Table of brokenExists node pointers +static class BrokenCntGlobal { + // This is a 7 bit generation counter, stored in the bottom 7 bits of AstNode::m_brokenState, + // used to mark a node as being present under the root AstNetlist in the current traversal. A + // value 0 is invalid, as the AstNode constructor uses that to initialize m_brokenState + static constexpr uint8_t MIN_VALUE = 1; + static constexpr uint8_t MAX_VALUE = 127; + + uint8_t m_count = MIN_VALUE; + +public: + uint8_t get() { + UASSERT(MIN_VALUE <= m_count && m_count <= MAX_VALUE, "Invalid generation number"); + return m_count; + } + + void inc() { + ++m_count; + if (m_count > MAX_VALUE) m_count = MIN_VALUE; + } +} s_brokenCntGlobal; + +//###################################################################### +// Table of allocated AstNode pointers + +static class AllocTable final { private: // MEMBERS - // For each node, we keep if it exists or not. - using NodeMap = std::unordered_map; // Performance matters (when --debug) - static NodeMap s_nodes; // Set of all nodes that exist - // BITMASK - enum { FLAG_ALLOCATED = 0x01 }; // new() and not delete()ed - enum { FLAG_IN_TREE = 0x02 }; // Is in netlist tree - enum { FLAG_LINKABLE = 0x04 }; // Is in netlist tree, can be linked to - enum { FLAG_LEAKED = 0x08 }; // Known to have been leaked - enum { FLAG_UNDER_NOW = 0x10 }; // Is in tree as parent of current node + std::unordered_set m_allocated; // Set of all nodes allocated but not freed public: // METHODS - static void deleted(const AstNode* nodep) { - // Called by operator delete on any node - only if VL_LEAK_CHECKS - if (debug() >= 9) cout << "-nodeDel: " << cvtToHex(nodep) << endl; - const auto iter = s_nodes.find(nodep); - UASSERT_OBJ(!(iter == s_nodes.end() || !(iter->second & FLAG_ALLOCATED)), - reinterpret_cast(nodep), - "Deleting AstNode object that was never tracked or already deleted"); - if (iter != s_nodes.end()) s_nodes.erase(iter); - } -#if defined(__GNUC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 4 - // GCC 4.4.* compiler warning bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=39390 -#pragma GCC diagnostic ignored "-Wstrict-aliasing" -#endif - static void addNewed(const AstNode* nodep) { + void addNewed(const AstNode* nodep) { // Called by operator new on any node - only if VL_LEAK_CHECKS - if (debug() >= 9) cout << "-nodeNew: " << cvtToHex(nodep) << endl; - const auto iter = s_nodes.find(nodep); - UASSERT_OBJ(!(iter != s_nodes.end() && (iter->second & FLAG_ALLOCATED)), nodep, - "Newing AstNode object that is already allocated"); - if (iter == s_nodes.end()) { - const int flags = FLAG_ALLOCATED; // This int needed to appease GCC 4.1.2 - s_nodes.emplace(nodep, flags); + // LCOV_EXCL_START + if (VL_UNCOVERABLE(!m_allocated.emplace(nodep).second)) { + nodep->v3fatalSrc("Newing AstNode object that is already allocated"); } + // LCOV_EXCL_STOP } - static void setUnder(const AstNode* nodep, bool flag) { - // Called by BrokenCheckVisitor when each node entered/exited - if (!okIfLinkedTo(nodep)) return; - const auto iter = s_nodes.find(nodep); - if (iter != s_nodes.end()) { - iter->second &= ~FLAG_UNDER_NOW; - if (flag) iter->second |= FLAG_UNDER_NOW; + void deleted(const AstNode* nodep) { + // Called by operator delete on any node - only if VL_LEAK_CHECKS + // LCOV_EXCL_START + if (VL_UNCOVERABLE(m_allocated.erase(nodep) == 0)) { + nodep->v3fatalSrc("Deleting AstNode object that was not allocated or already freed"); } + // LCOV_EXCL_STOP } - static void addInTree(AstNode* nodep, bool linkable) { -#ifndef VL_LEAK_CHECKS - // cppcheck-suppress knownConditionTrueFalse - if (!linkable) return; // save some time, else the map will get huge! -#endif - const auto iter = s_nodes.find(nodep); - if (VL_UNCOVERABLE(iter == s_nodes.end())) { -#ifdef VL_LEAK_CHECKS - nodep->v3fatalSrc("AstNode is in tree, but not allocated"); -#endif - } else { -#ifdef VL_LEAK_CHECKS - UASSERT_OBJ(iter->second & FLAG_ALLOCATED, nodep, - "AstNode is in tree, but not allocated"); -#endif - UASSERT_OBJ(!(iter->second & FLAG_IN_TREE), nodep, - "AstNode is already in tree at another location"); - } - const int or_flags = FLAG_IN_TREE | (linkable ? FLAG_LINKABLE : 0); - if (iter == s_nodes.end()) { - s_nodes.emplace(nodep, or_flags); - } else { - iter->second |= or_flags; - } - } - static bool isAllocated(const AstNode* nodep) { - // Some generic node has a pointer to this node. Is it allocated? - // Use this when might not be in tree; otherwise use okIfLinkedTo(). -#ifdef VL_LEAK_CHECKS - const auto iter = s_nodes.find(nodep); - if (iter == s_nodes.end()) return false; - if (!(iter->second & FLAG_ALLOCATED)) return false; -#endif - return true; - } - static bool okIfLinkedTo(const AstNode* nodep) { - // Some node in tree has a pointer to this node. Is it kosher? - const auto iter = s_nodes.find(nodep); - if (iter == s_nodes.end()) return false; -#ifdef VL_LEAK_CHECKS - if (!(iter->second & FLAG_ALLOCATED)) return false; -#endif - if (!(iter->second & FLAG_IN_TREE)) return false; - if (!(iter->second & FLAG_LINKABLE)) return false; - return true; - } - static bool okIfAbove(const AstNode* nodep) { - // Must be linked to and below current node - if (!okIfLinkedTo(nodep)) return false; - const auto iter = s_nodes.find(nodep); - if (iter == s_nodes.end()) return false; - if ((iter->second & FLAG_UNDER_NOW)) return false; - return true; - } - static bool okIfBelow(const AstNode* nodep) { - // Must be linked to and below current node - if (!okIfLinkedTo(nodep)) return false; - const auto iter = s_nodes.find(nodep); - if (iter == s_nodes.end()) return false; - if (!(iter->second & FLAG_UNDER_NOW)) return false; - return true; - } - static void prepForTree() { -#ifndef VL_LEAK_CHECKS - s_nodes.clear(); -#else - for (NodeMap::iterator it = s_nodes.begin(); it != s_nodes.end(); ++it) { - it->second &= ~FLAG_IN_TREE; - it->second &= ~FLAG_LINKABLE; - } -#endif - } - static void doneWithTree() { - for (int backs = 0; backs < 2; - backs++) { // Those with backp() are probably under one leaking without - for (NodeMap::iterator it = s_nodes.begin(); it != s_nodes.end(); ++it) { - // LCOV_EXCL_START - if (VL_UNCOVERABLE((it->second & FLAG_ALLOCATED) && !(it->second & FLAG_IN_TREE) - && !(it->second & FLAG_LEAKED) - && (it->first->backp() ? backs == 1 : backs == 0))) { + bool isAllocated(const AstNode* nodep) const { return m_allocated.count(nodep) != 0; } + void checkForLeaks() { + if (!v3Global.opt.debugCheck()) return; + const uint8_t brokenCntCurrent = s_brokenCntGlobal.get(); + + // Those with backp() are probably under a parent that was leaked and has no backp() + for (const bool withBack : {false, true}) { + for (const AstNode* const nodep : m_allocated) { + // LCOV_EXCL_START + // Most likely not leaked, so check that first + if (VL_UNCOVERABLE(nodep->brokenState() != brokenCntCurrent)) { + const bool hasBack = nodep->backp() != nullptr; + if (hasBack != withBack) continue; // Use only AstNode::dump instead of the virtual one, as there // may be varp() and other cross links that are bad. - if (v3Global.opt.debugCheck()) { - // When get this message, find what forgot to delete the - // node by running GDB, where for node "" use: - // watch AstNode::s_editCntGbl==#### - // run - // bt - std::cerr << "%Error: LeakedNode" - << (it->first->backp() ? "Back: " : ": "); - AstNode* rawp - = const_cast(static_cast(it->first)); - rawp->AstNode::dump(std::cerr); - std::cerr << endl; - V3Error::incErrors(); - } - it->second |= FLAG_LEAKED; + // When get this message, find what forgot to delete the + // node by running GDB, where for node "" use: + // watch AstNode::s_editCntGbl==#### + // run + // bt + std::cerr << "%Error: LeakedNode" << (withBack ? "with back pointer: " : ": "); + nodep->AstNode::dump(std::cerr); + std::cerr << endl; + V3Error::incErrors(); } // LCOV_EXCL_STOP } } } +} s_allocTable; - // CONSTRUCTORS - BrokenTable() = default; - virtual ~BrokenTable() override = default; -}; - -BrokenTable::NodeMap BrokenTable::s_nodes; - -bool AstNode::brokeExists() const { - // Called by node->broken() routines to do table lookup - return BrokenTable::okIfLinkedTo(this); -} -bool AstNode::brokeExistsAbove() const { - // Called by node->broken() routines to do table lookup - return BrokenTable::okIfBelow(this); -} -bool AstNode::brokeExistsBelow() const { - // Called by node->broken() routines to do table lookup - return BrokenTable::okIfAbove(this); -} +void V3Broken::addNewed(const AstNode* nodep) { s_allocTable.addNewed(nodep); } +void V3Broken::deleted(const AstNode* nodep) { s_allocTable.deleted(nodep); } //###################################################################### +// Table of AstNode pointers that can be linked to via member pointers + +static class LinkableTable final { +private: + // MEMBERS + std::unordered_set m_linkable; // Set of all nodes allocated but not freed + +public: + // METHODS + void clear() { m_linkable.clear(); } + inline void addLinkable(const AstNode* nodep) { m_linkable.emplace(nodep); } + inline bool isLinkable(const AstNode* nodep) const { return m_linkable.count(nodep) != 0; } +} s_linkableTable; + +bool V3Broken::isLinkable(const AstNode* nodep) { return s_linkableTable.isLinkable(nodep); } + +//###################################################################### +// Mark every node in the tree class BrokenMarkVisitor final : public AstNVisitor { - // Mark every node in the tree private: - // NODE STATE - // Nothing! // This may be called deep inside other routines - // // so userp and friends may not be used - // METHODS - void processAndIterate(AstNode* nodep) { - BrokenTable::addInTree(nodep, nodep->maybePointedTo()); - iterateChildrenConst(nodep); - } + const uint8_t m_brokenCntCurrent = s_brokenCntGlobal.get(); + // VISITORS virtual void visit(AstNode* nodep) override { - // Process not just iterate - processAndIterate(nodep); +#ifdef VL_LEAK_CHECKS + UASSERT_OBJ(s_allocTable.isAllocated(nodep), nodep, + "AstNode is in tree, but not allocated"); +#endif + UASSERT_OBJ(nodep->brokenState() != m_brokenCntCurrent, nodep, + "AstNode is already in tree at another location"); + if (nodep->maybePointedTo()) s_linkableTable.addLinkable(nodep); + nodep->brokenState(m_brokenCntCurrent); + iterateChildrenConst(nodep); } public: @@ -237,11 +164,15 @@ public: }; //###################################################################### -// Broken state, as a visitor of each AstNode +// Check every node in tree class BrokenCheckVisitor final : public AstNVisitor { bool m_inScope = false; // Under AstScope + // Constants for marking we are under/not under a node + const uint8_t m_brokenCntCurrentNotUnder = s_brokenCntGlobal.get(); // Top bit is clear + const uint8_t m_brokenCntCurrentUnder = m_brokenCntCurrentNotUnder | 0x80; // Top bit is set + // Current CFunc, if any const AstCFunc* m_cfuncp = nullptr; // All local variables declared in current function @@ -259,7 +190,7 @@ private: } void processEnter(AstNode* nodep) { - BrokenTable::setUnder(nodep, true); + nodep->brokenState(m_brokenCntCurrentUnder); const char* whyp = nodep->broken(); UASSERT_OBJ(!whyp, nodep, "Broken link in node (or something without maybePointedTo): " << whyp); @@ -283,7 +214,7 @@ private: } checkWidthMin(nodep); } - void processExit(AstNode* nodep) { BrokenTable::setUnder(nodep, false); } + void processExit(AstNode* nodep) { nodep->brokenState(m_brokenCntCurrentNotUnder); } void processAndIterate(AstNode* nodep) { processEnter(nodep); iterateChildrenConst(nodep); @@ -401,7 +332,7 @@ public: }; //###################################################################### -// Broken class functions +// Broken check entry point void V3Broken::brokenAll(AstNetlist* nodep) { // UINFO(9, __FUNCTION__ << ": " << endl); @@ -411,24 +342,23 @@ void V3Broken::brokenAll(AstNetlist* nodep) { UINFO(1, "Broken called under broken, skipping recursion.\n"); // LCOV_EXCL_LINE } else { inBroken = true; - BrokenTable::prepForTree(); BrokenMarkVisitor mvisitor(nodep); BrokenCheckVisitor cvisitor(nodep); - BrokenTable::doneWithTree(); + s_allocTable.checkForLeaks(); + s_linkableTable.clear(); + s_brokenCntGlobal.inc(); inBroken = false; } } -void V3Broken::addNewed(AstNode* nodep) { BrokenTable::addNewed(nodep); } -void V3Broken::deleted(AstNode* nodep) { BrokenTable::deleted(nodep); } -bool V3Broken::isAllocated(AstNode* nodep) { return BrokenTable::isAllocated(nodep); } +//###################################################################### +// Self test + void V3Broken::selfTest() { - // Warmup addNewed and deleted for coverage, as otherwise only with VL_LEAK_CHECKS - FileLine* fl = new FileLine(FileLine::commandLineFilename()); - auto* newp = new AstBegin(fl, "[EditWrapper]", nullptr); -#ifndef VL_LEAK_CHECKS + // Exercise addNewed and deleted for coverage, as otherwise only used with VL_LEAK_CHECKS + FileLine* const fl = new FileLine(FileLine::commandLineFilename()); + const AstNode* const newp = new AstBegin(fl, "[EditWrapper]", nullptr); addNewed(newp); deleted(newp); -#endif VL_DO_DANGLING(delete newp, newp); } diff --git a/src/V3Broken.h b/src/V3Broken.h index 16103baa0..94387ef06 100644 --- a/src/V3Broken.h +++ b/src/V3Broken.h @@ -20,17 +20,18 @@ #include "config_build.h" #include "verilatedos.h" -#include "V3Error.h" -#include "V3Ast.h" - //============================================================================ +// Forward declare so we can include this in V3Ast.h +class AstNode; +class AstNetlist; + class V3Broken final { public: static void brokenAll(AstNetlist* nodep); - static void addNewed(AstNode* nodep); - static void deleted(AstNode* nodep); - static bool isAllocated(AstNode* nodep); + static bool isLinkable(const AstNode* nodep); + static void addNewed(const AstNode* nodep); + static void deleted(const AstNode* nodep); static void selfTest(); };