From bbdc2cce08a26751bcfef865a6ea31523fd7f069 Mon Sep 17 00:00:00 2001 From: em2machine <92717390+em2machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:33:17 +0200 Subject: [PATCH] Internals: fixDeadRefs, findLiveCloneOf, TypeTableDeadRefVisitor removal (#8492 partial) (#8537) --- src/V3LinkDotIfaceCapture.cpp | 231 +----------------- src/V3LinkDotIfaceCapture.h | 14 +- src/V3Param.cpp | 4 +- test_regress/t/t_iface_capture_debug.py | 2 +- .../t/t_paramgraph_comined_iface_stats.py | 1 - ..._paramgraph_iface_template_nested_stats.py | 1 - ...t_paramgraph_nested_iface_typedef_stats.py | 1 - 7 files changed, 11 insertions(+), 243 deletions(-) diff --git a/src/V3LinkDotIfaceCapture.cpp b/src/V3LinkDotIfaceCapture.cpp index 14c81c316..cde4975f9 100644 --- a/src/V3LinkDotIfaceCapture.cpp +++ b/src/V3LinkDotIfaceCapture.cpp @@ -168,7 +168,7 @@ AstParamTypeDType* V3LinkDotIfaceCapture::findParamTypeInModule(AstNodeModule* m bool V3LinkDotIfaceCapture::retargetRefToModule(AstRefDType* refp, AstNodeModule* targetModp) { const VIfaceCaptureTag* const tagp = refp->captureTagp(); UASSERT_OBJ(tagp, refp, "Retarget of a reference that was not captured"); - if (!targetModp) return false; + UASSERT_OBJ(targetModp, refp, "Retarget to a null module"); if (tagp->m_kind == VIfaceCaptureTag::Kind::PARAM_TYPE) { AstParamTypeDType* const paramTypep = findParamTypeInModule(targetModp, refp->name()); @@ -201,41 +201,12 @@ void V3LinkDotIfaceCapture::retargetRefToTypedef(AstRefDType* refp, AstTypedef* } } -bool V3LinkDotIfaceCapture::isCloneOfModule(const AstNodeModule* modp, - const AstNodeModule* templateModp) { - if (!modp || !templateModp || modp == templateModp) return false; - // Only a module that was actually copied has copies, which keeps a copy from - // matching another copy of the same original. - if (!templateModp->parameterizedTemplate()) return false; - // A copy keeps the name the module was written with, whatever it was renamed to. - return modp->origName() == templateModp->origName(); -} - -AstNodeModule* V3LinkDotIfaceCapture::findCloneViaHierarchy(AstNodeModule* containingModp, - AstNodeModule* deadTargetModp, - int depth) { - if (depth > 20) return nullptr; // Safety limit - for (AstNode* stmtp = containingModp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { - if (AstCell* const cellp = VN_CAST(stmtp, Cell)) { - AstNodeModule* const cellModp = cellp->modp(); - if (!cellModp || cellModp->dead()) continue; - if (isCloneOfModule(cellModp, deadTargetModp)) return cellModp; - // Recurse into sub-cells - AstNodeModule* const found - = findCloneViaHierarchy(cellModp, deadTargetModp, depth + 1); - if (found) return found; - } - } - return nullptr; -} - namespace { using LiveNodes = std::unordered_set; // A scoped snapshot of every node currently in the tree. V3Broken::isLinkable() // cannot serve this role: its table is populated only while V3Broken::brokenAll() -// runs and is cleared before it returns, and brokenAll() would itself assert on -// the dangling cross-links this pass has yet to repair. +// runs and is cleared before it returns. LiveNodes collectLiveNodes() { LiveNodes liveNodes; v3Global.rootp()->foreach([&](AstNode* nodep) { liveNodes.insert(nodep); }); @@ -255,84 +226,12 @@ AstNodeModule* findOwnerModuleIfLive(AstNode* nodep, const LiveNodes& liveNodes) return findOwnerModuleImpl(nodep, &liveNodes); } -// Shared by the callers that all asked this the same way. -AstNodeModule* dyingOwnerOf(AstNode* nodep, const LiveNodes& liveNodes) { - if (!nodep || !liveNodes.count(nodep)) return nullptr; - AstNodeModule* const ownerp = findOwnerModuleIfLive(nodep, liveNodes); - return (ownerp && ownerp->dead()) ? ownerp : nullptr; -} - bool moduleMatchesOwner(const AstNodeModule* modp, const string& ownerName) { if (!modp || ownerName.empty()) return false; return modp->name() == ownerName || modp->origName() == ownerName; } } // namespace -int V3LinkDotIfaceCapture::fixDeadRefs(AstRefDType* refp, AstNodeModule* containingModp, - const char* location, const LiveNodes& liveNodes) { - int fixed = 0; - - // Check both links, a reference may only have one of them. - AstTypedef* const oldTypedefp = refp->typedefp(); - AstNodeModule* deadModp = dyingOwnerOf(oldTypedefp, liveNodes); - if (!deadModp) deadModp = dyingOwnerOf(refp->refDTypep(), liveNodes); - - if (deadModp && containingModp) { - // The module we are in can be the copy we want. - AstNodeModule* const cloneModp = isCloneOfModule(containingModp, deadModp) - ? containingModp - : findCloneViaHierarchy(containingModp, deadModp); - if (cloneModp) { - // A reference without a typedef still has its own name. - const string& tdName = oldTypedefp ? oldTypedefp->name() : refp->name(); - // Use the same retargets as the ledger so nothing is left behind. - if (AstTypedef* const newTdp = findTypedefInModule(cloneModp, tdName)) { - UINFO(9, "iface capture finalizeCapture (" << location << "): fixing refp=" << refp - << " dead=" << deadModp->name() - << " -> " << cloneModp->name()); - retargetRefToTypedef(refp, newTdp); - ++fixed; - } - } - } - - // Only worth checking when there is no typedef, as that is read first. - AstNodeDType* const oldRefDTypep = refp->refDTypep(); - if (!refp->typedefp() && oldRefDTypep && liveNodes.count(oldRefDTypep)) { - AstNodeModule* const targetModp = findOwnerModuleIfLive(oldRefDTypep, liveNodes); - UASSERT_OBJ(!targetModp || !targetModp->dead(), refp, - "refDTypep of '" << refp->prettyNameQ() << "' points to dead module '" - << (targetModp ? targetModp->name() : "") << "'"); - } - - // dtypep (checked later by V3Broken) likewise never points at a dead module. - AstNodeDType* const oldDTypep = refp->dtypep(); - if (oldDTypep && liveNodes.count(oldDTypep)) { - AstNodeModule* const dtOwnerp = findOwnerModuleIfLive(oldDTypep, liveNodes); - UASSERT_OBJ(!dtOwnerp || !dtOwnerp->dead(), refp, - "dtypep of '" << refp->prettyNameQ() << "' points to dead module '" - << (dtOwnerp ? dtOwnerp->name() : "") << "'"); - } - - return fixed; -} - -AstNodeModule* V3LinkDotIfaceCapture::findLiveCloneOf(AstNodeModule* deadTargetModp, - AstNodeModule** containerp) { - for (AstNode* np = v3Global.rootp()->modulesp(); np; np = np->nextp()) { - if (AstNodeModule* const modp = VN_CAST(np, NodeModule)) { - if (modp->dead()) continue; - AstNodeModule* const found = findCloneViaHierarchy(modp, deadTargetModp); - if (found) { - if (containerp) *containerp = modp; - return found; - } - } - } - if (containerp) *containerp = nullptr; - return nullptr; -} - AstNodeModule* V3LinkDotIfaceCapture::findOwnerModule(AstNode* nodep) { return findOwnerModuleImpl(nodep, nullptr); } @@ -358,8 +257,9 @@ void V3LinkDotIfaceCapture::dumpEntries(const string& label) { void V3LinkDotIfaceCapture::tag(AstRefDType* refp, const AstNodeModule* capturedInp, VIfaceCaptureTag::Kind kind, const string& cellPath, const string& ownerModName) { - if (refp->captureTagp()) return; // First capture wins + UASSERT_OBJ(!refp->captureTagp(), refp, "Reference captured twice"); UASSERT_OBJ(capturedInp, refp, "Captured reference is not in a module"); + UASSERT_OBJ(!cellPath.empty(), refp, "Captured reference has no cell path"); s_tags.emplace_back(VIfaceCaptureTag{kind, cellPath, ownerModName, capturedInp->origName()}); refp->captureTagp(&s_tags.back()); } @@ -477,10 +377,6 @@ void V3LinkDotIfaceCapture::captureTypedefContext(AstRefDType* refp, const char* << " cell=" << ifaceCellp << " cellPath='" << cellPath << "'" << " mod=" << (ifaceCellp->modp() ? ifaceCellp->modp()->name() : "") << " dotPos=" << dotPos); - // Note: the enclosingVar walk + promoteVarToParamType callback was removed. - // It supported 'localparam xyz_t = iface.rq_t;' without the 'type' keyword, - // which was never valid SystemVerilog. CI-CD with v3fatalSrc asserts on - // the promoteVarCb path and replaceRef confirmed this was dead code. } void V3LinkDotIfaceCapture::addParamType(AstRefDType* refp, const string& cellPath, @@ -497,8 +393,8 @@ void V3LinkDotIfaceCapture::addParamType(AstRefDType* refp, const string& cellPa << " ownerModp=" << (ownerModp ? ownerModp->name() : "") << " paramTypep=" << paramTypep << " paramTypeOwnerModName='" << ptOwnerName << "'"); - if (paramTypep) { - UINFO(9, "addParamType: paramTypep subDTypep chain:"); + UINFO(9, "addParamType: paramTypep subDTypep chain:"); + if (debug()) paramTypep->foreach([&](AstRefDType* innerRefp) { UINFO(9, " inner RefDType: " @@ -506,103 +402,9 @@ void V3LinkDotIfaceCapture::addParamType(AstRefDType* refp, const string& cellPa << (innerRefp->refDTypep() ? " refDTypep->name=" : "") << (innerRefp->refDTypep() ? innerRefp->refDTypep()->prettyTypeName() : "")); }); - } tag(refp, ownerModp, VIfaceCaptureTag::Kind::PARAM_TYPE, cellPath, ptOwnerName); } -// Visitor that fixes dead references in the global type table. -// -// When interface templates are cloned, REFDTYPEs in the global type table may -// still point to the dead template module. This visitor traverses the type -// table and redirects those references to the appropriate live clone. -// -// Handles both AstRefDType (direct typedef references) and AstMemberDType -// (struct/union member types) in a single traversal for efficiency. -class TypeTableDeadRefVisitor final : public VNVisitor { - const LiveNodes& m_liveNodes; - int m_fixed = 0; - - void visit(AstRefDType* refp) override { - iterateChildren(refp); - // For type table entries, find the first live module that contains - // a cell hierarchy leading to the dead target - AstNodeModule* containingModp = nullptr; - // Either (or both) links may point to a dead module. - AstNodeModule* deadTargetModp = dyingOwnerOf(refp->typedefp(), m_liveNodes); - if (!deadTargetModp) deadTargetModp = dyingOwnerOf(refp->refDTypep(), m_liveNodes); - if (deadTargetModp) { - V3LinkDotIfaceCapture::findLiveCloneOf(deadTargetModp, &containingModp); - } - m_fixed - += V3LinkDotIfaceCapture::fixDeadRefs(refp, containingModp, "type table", m_liveNodes); - } - - void visit(AstMemberDType* memberp) override { - iterateChildren(memberp); - if (!memberp->dtypep()) return; - UASSERT_OBJ(m_liveNodes.count(memberp->dtypep()), memberp, - "MemberDType has a dangling dtypep"); - AstNodeModule* const dtOwnerp = findOwnerModuleIfLive(memberp->dtypep(), m_liveNodes); - if (!dtOwnerp || !dtOwnerp->dead()) return; - // Try to find the clone of the dead module - AstNodeModule* const cloneModp = V3LinkDotIfaceCapture::findLiveCloneOf(dtOwnerp); - if (cloneModp) { - // Find matching type by name in the clone - const string& dtName = memberp->dtypep()->prettyName(); - // Try typedef children - for (AstNode* sp = cloneModp->stmtsp(); sp; sp = sp->nextp()) { - if (AstTypedef* const tdp = VN_CAST(sp, Typedef)) { - if (tdp->subDTypep() && tdp->subDTypep()->prettyName() == dtName) { - UINFO(9, "iface capture type table MEMBERDTYPE fixup (via typedef): " - << memberp->name() << " dtypep " << dtOwnerp->name() << " -> " - << cloneModp->name()); - memberp->dtypep(tdp->subDTypep()); - ++m_fixed; - return; - } - } - } - } - // One of the above fixup paths (prettyName or typedef) always succeeds - // when cloneModp is found. If this fires, either cloneModp is null - // (findLiveCloneOf failed - check that the dead template has a clone) - // or the dtype name doesn't match any statement in the clone (check - // memberp->dtypep()->prettyName() against cloneModp's statements). - v3fatalSrc("MemberDType fixup: could not fix member '" - << memberp->name() << "' dtypep points to dead " << dtOwnerp->name() - << " cloneModp=" << (cloneModp ? cloneModp->name() : "")); - } - - void visit(AstNode* nodep) override { iterateChildren(nodep); } - -public: - int fixed() const { return m_fixed; } - TypeTableDeadRefVisitor(AstNode* nodep, const LiveNodes& liveNodes) - : m_liveNodes{liveNodes} { - iterate(nodep); - } -}; - -int V3LinkDotIfaceCapture::fixDeadRefsInTypeTable(const LiveNodes& liveNodes) { - if (!v3Global.rootp()->typeTablep()) return 0; - const TypeTableDeadRefVisitor visitor{v3Global.rootp()->typeTablep(), liveNodes}; - return visitor.fixed(); -} - -int V3LinkDotIfaceCapture::fixDeadRefsInModules(const LiveNodes& liveNodes) { - int fixed = 0; - for (AstNode* nodep = v3Global.rootp()->modulesp(); nodep; nodep = nodep->nextp()) { - if (AstNodeModule* const modp = VN_CAST(nodep, NodeModule)) { - if (modp->dead()) continue; - const string modName = modp->name(); - modp->foreach([&](AstRefDType* refp) { - fixed += fixDeadRefs(refp, modp, modName.c_str(), liveNodes); - }); - } - } - return fixed; -} - int V3LinkDotIfaceCapture::resolveCapturedRefs() { int fixed = 0; @@ -630,12 +432,6 @@ int V3LinkDotIfaceCapture::resolveCapturedRefs() { if (moduleMatchesOwner(ownerModp, tagp->m_ownerModName)) { correctModp = ownerModp; } else { - // A non-matching owner always carries a cell path to the target owner. - UASSERT_OBJ(!tagp->m_cellPath.empty(), refp, - "captured ref '" - << refp->prettyNameQ() << "' owner '" << ownerModp->prettyNameQ() - << "' does not match target owner '" << tagp->m_ownerModName - << "' and has no cell path"); correctModp = followCellPath(ownerModp, tagp->m_cellPath); UINFO(9, " followCellPath('" << ownerModp->name() << "', '" << tagp->m_cellPath @@ -735,30 +531,19 @@ void V3LinkDotIfaceCapture::finalizeIfaceCapture() { if (!v3Global.rootp()) return; clearModuleCache(); // Ensure fresh view after all cloning/widthing - // Snapshot the tree to guard every legacy cross-link inspection below. - const LiveNodes liveNodes = collectLiveNodes(); - // Resolve live captured refs from stable path metadata before inspecting // any inherited target pointers, which may refer to replaced template nodes. const int capturedFixed = resolveCapturedRefs(); UINFO(4, "finalizeIfaceCapture: structurally resolved " << capturedFixed << " captured refs"); - const int typeTableFixed = fixDeadRefsInTypeTable(liveNodes); - const int moduleFixed = fixDeadRefsInModules(liveNodes); - UINFO(4, "finalizeIfaceCapture: fixed " << typeTableFixed << " in type table, " << moduleFixed - << " in modules (dead refs)"); - if (debug() >= 9) dumpEntries("after finalizeIfaceCapture"); // Emit statistics for --stats V3Stats::addStat("IfaceCapture, Captured refs", s_tags.size()); - V3Stats::addStat("IfaceCapture, Dead refs fixed in type table", typeTableFixed); - V3Stats::addStat("IfaceCapture, Dead refs fixed in modules", moduleFixed); V3Stats::addStat("IfaceCapture, Captured refs resolved", capturedFixed); - // Independent debug-only audit of the repairs above; kept separate from the - // repair traversal so it can catch omissions in that repair. - if (debug() >= 9) verifyNoDeadRefs(liveNodes); + // No reference may be left pointing into a module about to be removed + if (v3Global.opt.debugCheck()) verifyNoDeadRefs(collectLiveNodes()); // Drop every tag before its record is freed. v3Global.rootp()->foreach([](AstRefDType* refp) { refp->captureTagp(nullptr); }); reset(); diff --git a/src/V3LinkDotIfaceCapture.h b/src/V3LinkDotIfaceCapture.h index 4b26110b2..7313bf799 100644 --- a/src/V3LinkDotIfaceCapture.h +++ b/src/V3LinkDotIfaceCapture.h @@ -44,8 +44,6 @@ public: }; class V3LinkDotIfaceCapture final { - friend class TypeTableDeadRefVisitor; - static std::deque s_tags; // Owns every tag; a deque keeps addresses stable static bool s_enabled; @@ -54,24 +52,14 @@ class V3LinkDotIfaceCapture final { static void reset(); static void clearModuleCache(); static AstIfaceRefDType* ifaceRefFromVarDType(AstNodeDType* dtypep); - // True if this module is a copy of that one. - static bool isCloneOfModule(const AstNodeModule* modp, const AstNodeModule* templateModp); // Point a reference at a typedef and fix its other links. static void retargetRefToTypedef(AstRefDType* refp, AstTypedef* typedefp); // Same, for a parameter type. static void retargetRefToParamType(AstRefDType* refp, AstParamTypeDType* paramTypep); - static AstNodeModule* findCloneViaHierarchy(AstNodeModule* containingModp, - AstNodeModule* deadTargetModp, int depth = 0); - static AstNodeModule* findLiveCloneOf(AstNodeModule* deadTargetModp, - AstNodeModule** containerp = nullptr); - static int fixDeadRefs(AstRefDType* refp, AstNodeModule* containingModp, const char* location, - const std::unordered_set& liveNodes); - // Tag a reference captured in capturedInp, unless it is already tagged + // Tag a reference captured in capturedInp static void tag(AstRefDType* refp, const AstNodeModule* capturedInp, VIfaceCaptureTag::Kind kind, const string& cellPath, const string& ownerModName); - static int fixDeadRefsInTypeTable(const std::unordered_set& liveNodes); - static int fixDeadRefsInModules(const std::unordered_set& liveNodes); static int resolveCapturedRefs(); static void verifyNoDeadRefs(const std::unordered_set& liveNodes); diff --git a/src/V3Param.cpp b/src/V3Param.cpp index ddafa94e5..d1ffeef34 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -858,7 +858,7 @@ class ParamProcessor final { // References in a class nested in newModp (e.g. a covergroup) are included. newModp->foreach([&](AstRefDType* refp) { const VIfaceCaptureTag* const tagp = V3LinkDotIfaceCapture::captureTag(refp); - if (!tagp || tagp->m_cellPath.empty()) return; + if (!tagp) return; AstNodeModule* const correctModp = V3LinkDotIfaceCapture::followCellPath(newModp, tagp->m_cellPath); @@ -2183,8 +2183,6 @@ public: // reference the old template's types so that $bits(iface_typedef) // evaluates with the clone's widths. void retargetIfaceRefs(AstNodeModule* parentModp, const string& cellName) { - // A template's references are retargeted in each of its clones instead. - if (parentModp->hasGParam()) return; AstNodeModule* const correctModp = V3LinkDotIfaceCapture::followCellPath(parentModp, cellName); if (!correctModp || correctModp->dead() || correctModp->parameterizedTemplate()) return; diff --git a/test_regress/t/t_iface_capture_debug.py b/test_regress/t/t_iface_capture_debug.py index b9e6450b6..369e0248f 100755 --- a/test_regress/t/t_iface_capture_debug.py +++ b/test_regress/t/t_iface_capture_debug.py @@ -10,7 +10,7 @@ import vltest_bootstrap test.scenarios('vlt') -test.top_filename = "t/t_iface_typedef_bits_uaf.v" +test.top_filename = "t/t_paramgraph_iface_template_nested.v" test.lint( # Check we can dump the interface capture ledger diff --git a/test_regress/t/t_paramgraph_comined_iface_stats.py b/test_regress/t/t_paramgraph_comined_iface_stats.py index 86886766e..3b1e78f7f 100755 --- a/test_regress/t/t_paramgraph_comined_iface_stats.py +++ b/test_regress/t/t_paramgraph_comined_iface_stats.py @@ -21,7 +21,6 @@ test.compile(v_flags2=["--binary --stats"]) test.file_grep(test.stats, r'IfaceCapture, Captured refs\s+(\d+)', 8) test.file_grep(test.stats, r'IfaceCapture, Ledger fixups in V3Param\s+(\d+)', 8) test.file_grep(test.stats, r'IfaceCapture, Captured refs resolved\s+(\d+)', 10) -test.file_grep(test.stats, r'IfaceCapture, Dead refs fixed in modules\s+(\d+)', 0) test.execute() diff --git a/test_regress/t/t_paramgraph_iface_template_nested_stats.py b/test_regress/t/t_paramgraph_iface_template_nested_stats.py index ce023feaf..a18443a7a 100755 --- a/test_regress/t/t_paramgraph_iface_template_nested_stats.py +++ b/test_regress/t/t_paramgraph_iface_template_nested_stats.py @@ -21,7 +21,6 @@ test.compile(v_flags2=["--binary --stats"]) test.file_grep(test.stats, r'IfaceCapture, Captured refs\s+(\d+)', 11) test.file_grep(test.stats, r'IfaceCapture, Ledger fixups in V3Param\s+(\d+)', 5) test.file_grep(test.stats, r'IfaceCapture, Captured refs resolved\s+(\d+)', 18) -test.file_grep(test.stats, r'IfaceCapture, Dead refs fixed in modules\s+(\d+)', 0) test.execute() diff --git a/test_regress/t/t_paramgraph_nested_iface_typedef_stats.py b/test_regress/t/t_paramgraph_nested_iface_typedef_stats.py index 76188c9f4..36e0092ef 100755 --- a/test_regress/t/t_paramgraph_nested_iface_typedef_stats.py +++ b/test_regress/t/t_paramgraph_nested_iface_typedef_stats.py @@ -21,7 +21,6 @@ test.compile(v_flags2=["--binary --stats"]) test.file_grep(test.stats, r'IfaceCapture, Captured refs\s+(\d+)', 8) test.file_grep(test.stats, r'IfaceCapture, Ledger fixups in V3Param\s+(\d+)', 8) test.file_grep(test.stats, r'IfaceCapture, Captured refs resolved\s+(\d+)', 14) -test.file_grep(test.stats, r'IfaceCapture, Dead refs fixed in modules\s+(\d+)', 0) test.execute()