From 8868d459a295b0b72bbc1925b7ae1ccaabca5295 Mon Sep 17 00:00:00 2001 From: Szymon Gizler Date: Thu, 28 Aug 2025 15:03:46 +0200 Subject: [PATCH] Fix broken support of unassigned virtual interfaces (#6253) (#6338) * Fix broken support of unassigned virtual interfaces Unassigned virtual interface support added by #6245 is broken - PR marks dead module as alive - we can't do that as once a module is dead it needs to remain dead because earlier steps (e.g. port resolution) have already been skipped. This commit handles unassigned virtual interfaces at the beginning of first pass of LinkDot (so it is never marked as dead, and no linking steps are getting skipped). Fixes #6253. * Apply suggestions from code review Co-authored-by: Wilson Snyder * Apply 'make format' * Revert add of redundant iterateChildren() call * Add original test case --------- Co-authored-by: Wilson Snyder Co-authored-by: github action --- src/V3LinkDot.cpp | 63 ++++++++++++++----- .../t/t_interface_virtual_missing_bad.out | 5 ++ .../t/t_interface_virtual_missing_bad.py | 16 +++++ .../t/t_interface_virtual_missing_bad.v | 15 +++++ test_regress/t/t_interface_virtual_unused.v | 2 +- test_regress/t/t_interface_virtual_unused2.py | 18 ++++++ test_regress/t/t_interface_virtual_unused2.v | 27 ++++++++ test_regress/t/t_interface_virtual_unused3.py | 16 +++++ test_regress/t/t_interface_virtual_unused3.v | 20 ++++++ 9 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 test_regress/t/t_interface_virtual_missing_bad.out create mode 100755 test_regress/t/t_interface_virtual_missing_bad.py create mode 100644 test_regress/t/t_interface_virtual_missing_bad.v create mode 100755 test_regress/t/t_interface_virtual_unused2.py create mode 100644 test_regress/t/t_interface_virtual_unused2.v create mode 100755 test_regress/t/t_interface_virtual_unused3.py create mode 100644 test_regress/t/t_interface_virtual_unused3.v diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index f6e8573fd..4afa67720 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -533,22 +533,9 @@ public: continue; } else if (ifacerefp->ifaceViaCellp()->dead() || !existsNodeSym(ifacerefp->ifaceViaCellp())) { - // virtual interface never assigned any actual interface - ifacerefp->ifacep()->dead(false); - varp->dtypep(ifacerefp->dtypep()); - // Create dummy cell to keep the virtual interface alive - // (later stages assume that non-dead interface has associated cell) - AstCell* const ifacecellp - = new AstCell{ifacerefp->ifacep()->fileline(), - ifacerefp->ifacep()->fileline(), - ifacerefp->ifacep()->name() + "__02E" + varp->name(), - ifacerefp->ifaceName(), - nullptr, - nullptr, - nullptr}; - ifacecellp->modp(ifacerefp->ifacep()); - VSymEnt* const symp = new VSymEnt{&m_syms, ifacecellp}; - ifacerefp->ifacep()->user1p(symp); + UASSERT_OBJ(false, ifacerefp->ifaceViaCellp(), + "Interface referenced by virtual iface is dead. " + "Dead interfaces have required linking steps skipped"); } VSymEnt* const ifaceSymp = getNodeSym(ifacerefp->ifaceViaCellp()); VSymEnt* ifOrPortSymp = ifaceSymp; @@ -874,6 +861,14 @@ LinkDotState* LinkDotState::s_errorThisp = nullptr; //====================================================================== class LinkDotFindVisitor final : public VNVisitor { + // NODE STATE + // Cleared on global + // *::user1p() -> See LinkDotState + // *::user2p() -> See LinkDotState + // *::user3() // bool. Node stored in m_virtualIfaces + // *::user4() -> See LinkDotState + const VNUser3InUse m_inuser3; + // STATE LinkDotState* const m_statep; // State to pass between visitors, including symbol table AstNodeModule* m_classOrPackagep = nullptr; // Current package @@ -891,6 +886,8 @@ class LinkDotFindVisitor final : public VNVisitor { int m_modBlockNum = 0; // Begin block number in module, 0=none seen int m_modWithNum = 0; // With block number, 0=none seen int m_modArgNum = 0; // Arg block number for randomize(), 0=none seen + std::vector m_virtIfaces; // interfaces used as virtual, + // needed for handleUnvisitedVirtIfaces() // METHODS void makeImplicitNew(AstClass* nodep) { @@ -986,6 +983,14 @@ class LinkDotFindVisitor final : public VNVisitor { } void visit(AstTypeTable*) override {} // FindVisitor:: void visit(AstConstPool*) override {} // FindVisitor:: + void visit(AstIfaceRefDType* nodep) override { + if (m_statep->forPrimary() && nodep->isVirtual() && nodep->ifacep() + && !nodep->ifacep()->user3()) { + m_virtIfaces.push_back(nodep->ifacep()); + nodep->ifacep()->user3(true); + } + iterateChildren(nodep); + } void visit(AstNodeModule* nodep) override { // FindVisitor:: // Called on top module from Netlist, other modules from the cell creating them, // and packages @@ -1821,12 +1826,36 @@ class LinkDotFindVisitor final : public VNVisitor { void visit(AstNode* nodep) override { iterateChildren(nodep); } // FindVisitor:: + void handleUnvisitedVirtIfaces() { + AstNodeModule* const top = v3Global.rootp()->modulesp(); + m_curSymp = m_statep->getNodeSym(top); + for (AstIface* ifacep : m_virtIfaces) { + if (!ifacep->user4()) { // virtual interface never assigned any actual interface + // Create dummy cell to keep the virtual interface alive + // (later stages assume that non-dead interface has associated cell) + AstCell* const ifacecellp = new AstCell{ifacep->fileline(), + ifacep->fileline(), + "__VdummyInstOf" + ifacep->name(), + ifacep->name(), + nullptr, + nullptr, + nullptr}; + ifacecellp->modp(ifacep); + top->addStmtsp(ifacecellp); + + iterate(ifacecellp); + } + } + } + public: // CONSTRUCTORS LinkDotFindVisitor(AstNetlist* rootp, LinkDotState* statep) : m_statep{statep} { UINFO(4, __FUNCTION__ << ": "); iterate(rootp); + + if (!m_virtIfaces.empty()) handleUnvisitedVirtIfaces(); } ~LinkDotFindVisitor() override = default; }; @@ -2877,7 +2906,7 @@ class LinkDotResolveVisitor final : public VNVisitor { void visit(AstTypeTable*) override {} void visit(AstConstPool*) override {} void visit(AstNodeModule* nodep) override { - if (nodep->dead() || !m_statep->existsNodeSym(nodep)) return; + if (nodep->dead()) return; LINKDOT_VISIT_START(); UINFO(8, indent() << "visit " << nodep); VL_RESTORER(m_genericIfaceModule); diff --git a/test_regress/t/t_interface_virtual_missing_bad.out b/test_regress/t/t_interface_virtual_missing_bad.out new file mode 100644 index 000000000..c8d485e15 --- /dev/null +++ b/test_regress/t/t_interface_virtual_missing_bad.out @@ -0,0 +1,5 @@ +%Error: t/t_interface_virtual_missing_bad.v:9:12: Cannot find file containing interface: 'foo' + 9 | virtual foo vif; + | ^~~ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Error: Exiting due to diff --git a/test_regress/t/t_interface_virtual_missing_bad.py b/test_regress/t/t_interface_virtual_missing_bad.py new file mode 100755 index 000000000..55203b6c9 --- /dev/null +++ b/test_regress/t/t_interface_virtual_missing_bad.py @@ -0,0 +1,16 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('linter') + +test.lint(fails=True, expect_filename=test.golden_filename) + +test.passes() diff --git a/test_regress/t/t_interface_virtual_missing_bad.v b/test_regress/t/t_interface_virtual_missing_bad.v new file mode 100644 index 000000000..321539f28 --- /dev/null +++ b/test_regress/t/t_interface_virtual_missing_bad.v @@ -0,0 +1,15 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Antmicro. +// SPDX-License-Identifier: CC0-1.0 + +module t (/*AUTOARG*/); + + virtual foo vif; + + initial begin + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_interface_virtual_unused.v b/test_regress/t/t_interface_virtual_unused.v index c0c7d185f..df22a29fd 100644 --- a/test_regress/t/t_interface_virtual_unused.v +++ b/test_regress/t/t_interface_virtual_unused.v @@ -6,7 +6,7 @@ // See also t_interface_virtual.v -interface QBus; +interface QBus(); endinterface module t (/*AUTOARG*/); diff --git a/test_regress/t/t_interface_virtual_unused2.py b/test_regress/t/t_interface_virtual_unused2.py new file mode 100755 index 000000000..f989a35fb --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused2.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_interface_virtual_unused2.v b/test_regress/t/t_interface_virtual_unused2.v new file mode 100644 index 000000000..81502e650 --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused2.v @@ -0,0 +1,27 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Antmicro. +// SPDX-License-Identifier: CC0-1.0 + +interface QBus(input logic k); + logic data; +endinterface + +class cls; + virtual QBus vif1; + + function foo(virtual QBus vif2); + vif2.data = 1; + endfunction +endclass + +module t (/*AUTOARG*/); + + cls bar; + + initial begin + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_interface_virtual_unused3.py b/test_regress/t/t_interface_virtual_unused3.py new file mode 100755 index 000000000..147fe6faf --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused3.py @@ -0,0 +1,16 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile() + +test.passes() diff --git a/test_regress/t/t_interface_virtual_unused3.v b/test_regress/t/t_interface_virtual_unused3.v new file mode 100644 index 000000000..fbac6d425 --- /dev/null +++ b/test_regress/t/t_interface_virtual_unused3.v @@ -0,0 +1,20 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +interface stream_ifc #( +) ( + input logic clk_i +); +endinterface + +package pkg; + class stream_driver #(); + virtual stream_ifc stream; + endclass +endpackage + +module t; +endmodule