From 766ad14ae0957c1893321da7f06df0272fa0245b Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Thu, 8 Jul 2021 14:58:57 +0100 Subject: [PATCH] Check function locals are referenced only when in scope V3Broken now checks that AstVar nodes referenced in an AstCFunc are either external, or appear in the tree before the reference, and are in scope. Fix V3Begin to move lifted AstVars to beginning of FTask, rather than end, which trips the above check. --- src/V3Begin.cpp | 45 ++++++++++++++-------- src/V3Broken.cpp | 97 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 17 deletions(-) diff --git a/src/V3Begin.cpp b/src/V3Begin.cpp index 9c1e3674a..5bee0a7f4 100644 --- a/src/V3Begin.cpp +++ b/src/V3Begin.cpp @@ -62,6 +62,7 @@ private: BeginState* m_statep; // Current global state AstNodeModule* m_modp = nullptr; // Current module AstNodeFTask* m_ftaskp = nullptr; // Current function/task + AstNode* m_liftedp = nullptr; // Local nodes we are lifting into m_ftaskp string m_namedScope; // Name of begin blocks above us string m_unnamedScope; // Name of begin blocks, including unnamed blocks int m_ifDepth = 0; // Current if depth @@ -74,6 +75,21 @@ private: return a + "__DOT__" + b; } + void liftNode(AstNode* nodep) { + nodep->unlinkFrBack(); + if (m_ftaskp) { + // AstBegin under ftask, just move into the ftask + if (!m_liftedp) { + m_liftedp = nodep; + } else { + m_liftedp->addNext(nodep); + } + } else { + // Move to module + m_modp->addStmtp(nodep); + } + } + // VISITORS virtual void visit(AstNodeModule* nodep) override { VL_RESTORER(m_modp); @@ -101,7 +117,17 @@ private: m_namedScope = ""; m_unnamedScope = ""; m_ftaskp = nodep; + m_liftedp = nullptr; iterateChildren(nodep); + if (m_liftedp) { + // Place lifted nodes at beginning of stmtsp, so Var nodes appear before referenced + if (AstNode* const stmtsp = nodep->stmtsp()) { + stmtsp->unlinkFrBackWithNext(); + m_liftedp->addNext(stmtsp); + } + nodep->addStmtsp(m_liftedp); + m_liftedp = nullptr; + } m_ftaskp = nullptr; } } @@ -157,13 +183,8 @@ private: // Rename it nodep->name(dot(m_unnamedScope, nodep->name())); m_statep->userMarkChanged(nodep); - // Move to module - nodep->unlinkFrBack(); - if (m_ftaskp) { - m_ftaskp->addStmtsp(nodep); // Begins under funcs just move into the func - } else { - m_modp->addStmtp(nodep); - } + // Move it under enclosing tree + liftNode(nodep); } } virtual void visit(AstTypedef* nodep) override { @@ -171,14 +192,8 @@ private: // Rename it nodep->name(dot(m_unnamedScope, nodep->name())); m_statep->userMarkChanged(nodep); - // Move to module - nodep->unlinkFrBack(); - // Begins under funcs just move into the func - if (m_ftaskp) { - m_ftaskp->addStmtsp(nodep); - } else { - m_modp->addStmtp(nodep); - } + // Move it under enclosing tree + liftNode(nodep); } } virtual void visit(AstCell* nodep) override { diff --git a/src/V3Broken.cpp b/src/V3Broken.cpp index 3205ce912..b9bae7707 100644 --- a/src/V3Broken.cpp +++ b/src/V3Broken.cpp @@ -18,6 +18,7 @@ // Entire netlist // Mark all nodes // Check all links point to marked nodes +// Check local variables in CFuncs appear before they are referenced // //************************************************************************* @@ -33,6 +34,7 @@ #include #include +#include //###################################################################### @@ -240,13 +242,23 @@ public: class BrokenCheckVisitor final : public AstNVisitor { bool m_inScope = false; // Under AstScope + // Current CFunc, if any + const AstCFunc* m_cfuncp = nullptr; + // All local variables declared in current function + std::unordered_set m_localVars; + // Variable references in current function that do not reference an in-scope local + std::unordered_map m_suspectRefs; + // Local variables declared in the scope of the current statement + std::vector> m_localsStack; + private: static void checkWidthMin(const AstNode* nodep) { UASSERT_OBJ(nodep->width() == nodep->widthMin() || v3Global.widthMinUsage() != VWidthMinUsage::MATCHES_WIDTH, nodep, "Width != WidthMin"); } - void processAndIterate(AstNode* nodep) { + + void processEnter(AstNode* nodep) { BrokenTable::setUnder(nodep, true); const char* whyp = nodep->broken(); UASSERT_OBJ(!whyp, nodep, @@ -270,8 +282,30 @@ private: if (const AstNodeDType* dnodep = VN_CAST(nodep, NodeDType)) checkWidthMin(dnodep); } checkWidthMin(nodep); + } + void processExit(AstNode* nodep) { BrokenTable::setUnder(nodep, false); } + void processAndIterate(AstNode* nodep) { + processEnter(nodep); iterateChildrenConst(nodep); - BrokenTable::setUnder(nodep, false); + processExit(nodep); + } + void processAndIterateList(AstNode* nodep) { + while (nodep) { + processAndIterate(nodep); + nodep = nodep->nextp(); + }; + } + void pushLocalScope() { + if (m_cfuncp) m_localsStack.emplace_back(); + } + void popLocalScope() { + if (m_cfuncp) m_localsStack.pop_back(); + } + bool isInScopeLocal(const AstVar* varp) const { + for (const auto& set : m_localsStack) { + if (set.count(varp)) return true; + } + return false; } virtual void visit(AstNodeAssign* nodep) override { processAndIterate(nodep); @@ -295,6 +329,65 @@ private: UASSERT_OBJ( !(v3Global.assertScoped() && m_inScope && nodep->varp() && !nodep->varScopep()), nodep, "VarRef missing VarScope pointer"); + if (m_cfuncp) { + // Check if variable is an in-scope local, otherwise mark as suspect + if (const AstVar* const varp = nodep->varp()) { + if (!isInScopeLocal(varp)) { + // This only stores the first ref for each Var, which is what we want + m_suspectRefs.emplace(varp, nodep); + } + } + } + } + virtual void visit(AstCFunc* nodep) override { + UASSERT_OBJ(!m_cfuncp, nodep, "Nested AstCFunc"); + m_cfuncp = nodep; + m_localVars.clear(); + m_suspectRefs.clear(); + m_localsStack.clear(); + pushLocalScope(); + + processAndIterate(nodep); + + // Check suspect references are all to non-locals + for (const auto& pair : m_suspectRefs) { + UASSERT_OBJ(m_localVars.count(pair.first) == 0, pair.second, + "Local variable not in scope where referenced: " << pair.first); + } + + m_cfuncp = nullptr; + } + virtual void visit(AstNodeIf* nodep) override { + // Each branch is a separate local variable scope + pushLocalScope(); + processEnter(nodep); + processAndIterate(nodep->condp()); + if (AstNode* const ifsp = nodep->ifsp()) { + pushLocalScope(); + processAndIterateList(ifsp); + popLocalScope(); + } + if (AstNode* const elsesp = nodep->elsesp()) { + pushLocalScope(); + processAndIterateList(elsesp); + popLocalScope(); + } + processExit(nodep); + popLocalScope(); + } + virtual void visit(AstNodeStmt* nodep) override { + // For local variable checking act as if any statement introduces a new scope. + // This is aggressive but conservatively correct. + pushLocalScope(); + processAndIterate(nodep); + popLocalScope(); + } + virtual void visit(AstVar* nodep) override { + processAndIterate(nodep); + if (m_cfuncp) { + m_localVars.insert(nodep); + m_localsStack.back().insert(nodep); + } } virtual void visit(AstNode* nodep) override { // Process not just iterate