From 4fdaa463285a3ea59fab5dc3b2dad8394dbc97a8 Mon Sep 17 00:00:00 2001 From: Anthony Donlon <4056887+donlon@users.noreply.github.com> Date: Sun, 15 Oct 2023 18:01:32 +0100 Subject: [PATCH] Fix using functions/tasks following class definition inside module (#4553) --- src/V3Global.h | 6 ++-- src/V3LinkDot.cpp | 50 +++++++++++++-------------- test_regress/t/t_class_nested_link.pl | 21 +++++++++++ test_regress/t/t_class_nested_link.v | 45 ++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 27 deletions(-) create mode 100755 test_regress/t/t_class_nested_link.pl create mode 100644 test_regress/t/t_class_nested_link.v diff --git a/src/V3Global.h b/src/V3Global.h index a25a3f274..126392ad9 100644 --- a/src/V3Global.h +++ b/src/V3Global.h @@ -41,12 +41,13 @@ class V3HierBlockPlan; //====================================================================== // Restorer -/// Save a given variable's value on the stack, restoring it at -/// end-of-stope. +/// Save a given variable's value on the stack, restoring it at end-of-scope. // Object must be named, or it will not persist until end-of-scope. // Constructor needs () or GCC 4.8 false warning. #define VL_RESTORER(var) \ const VRestorer::type> restorer_##var(var); +/// Get the copy of the variable previously saved by VL_RESTORER() +#define VL_RESTORER_PREV(var) restorer_##var.saved() // Object used by VL_RESTORER. This object must be an auto variable, not // allocated on the heap or otherwise. @@ -60,6 +61,7 @@ public: : m_ref{permr} , m_saved{permr} {} ~VRestorer() { m_ref = m_saved; } + const T& saved() const { return m_saved; } VL_UNCOPYABLE(VRestorer); }; diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 1d8cbce88..3b129d741 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -1050,12 +1050,11 @@ class LinkDotFindVisitor final : public VNVisitor { } else { VL_RESTORER(m_blockp); VL_RESTORER(m_curSymp); - VSymEnt* const oldCurSymp = m_curSymp; { m_blockp = nodep; m_curSymp = m_statep->insertBlock(m_curSymp, nodep->name(), nodep, m_classOrPackagep); - m_curSymp->fallbackp(oldCurSymp); + m_curSymp->fallbackp(VL_RESTORER_PREV(m_curSymp)); // Iterate iterateChildren(nodep); } @@ -1440,12 +1439,11 @@ class LinkDotFindVisitor final : public VNVisitor { void visit(AstForeach* nodep) override { // Symbol table needs nodep->name() as the index variable's name VL_RESTORER(m_curSymp); - VSymEnt* const oldCurSymp = m_curSymp; { ++m_modWithNum; m_curSymp = m_statep->insertBlock(m_curSymp, "__Vforeach" + cvtToStr(m_modWithNum), nodep, m_classOrPackagep); - m_curSymp->fallbackp(oldCurSymp); + m_curSymp->fallbackp(VL_RESTORER_PREV(m_curSymp)); // DOT(x, SELLOOPVARS(var, loops)) -> SELLOOPVARS(DOT(x, var), loops) if (AstDot* const dotp = VN_CAST(nodep->arrayp(), Dot)) { if (AstSelLoopVars* const loopvarsp = VN_CAST(dotp->rhsp(), SelLoopVars)) { @@ -1520,12 +1518,11 @@ class LinkDotFindVisitor final : public VNVisitor { // Symbol table needs nodep->name() as the index variable's name // Iteration will pickup the AstVar we made under AstWith VL_RESTORER(m_curSymp); - VSymEnt* const oldCurSymp = m_curSymp; { ++m_modWithNum; m_curSymp = m_statep->insertBlock(m_curSymp, "__Vwith" + cvtToStr(m_modWithNum), nodep, m_classOrPackagep); - m_curSymp->fallbackp(oldCurSymp); + m_curSymp->fallbackp(VL_RESTORER_PREV(m_curSymp)); UASSERT_OBJ(nodep->indexArgRefp(), nodep, "Missing lambda argref"); UASSERT_OBJ(nodep->valueArgRefp(), nodep, "Missing lambda argref"); // Insert argref's name into symbol table @@ -1927,11 +1924,10 @@ class LinkDotIfaceVisitor final : public VNVisitor { // Modport: Remember its name for later resolution UINFO(5, " fiv: " << nodep << endl); VL_RESTORER(m_curSymp); - VSymEnt* const oldCurSymp = m_curSymp; { // Create symbol table for the vars m_curSymp = m_statep->insertBlock(m_curSymp, nodep->name(), nodep, nullptr); - m_curSymp->fallbackp(oldCurSymp); + m_curSymp->fallbackp(VL_RESTORER_PREV(m_curSymp)); iterateChildren(nodep); } } @@ -2461,7 +2457,8 @@ private: m_ds.m_dotErr = true; } else { const auto classp = VN_AS(classSymp->nodep(), Class); - if (!classp->extendsp()) { + const auto cextp = classp->extendsp(); + if (!cextp) { nodep->v3error("'super' used on non-extended class (IEEE 1800-2017 8.15)"); m_ds.m_dotErr = true; } else { @@ -2469,8 +2466,6 @@ private: && m_extendsParam.find(classp) != m_extendsParam.end()) { m_ds.m_unresolvedClass = true; } else { - const auto cextp = classp->extendsp(); - UASSERT_OBJ(cextp, nodep, "Bad super extends link"); const auto baseClassp = cextp->classp(); UASSERT_OBJ(baseClassp, nodep, "Bad superclass"); m_ds.m_dotSymp = m_statep->getNodeSym(baseClassp); @@ -3151,12 +3146,16 @@ private: } else { string baddot; VSymEnt* okSymp = nullptr; - VSymEnt* dotSymp = nodep->dotted().empty() - ? m_ds.m_dotSymp // Non-'super.' dotted reference - : m_curSymp; // Start search at dotted point - // of same name under a subtask isn't a relevant hit however a - // function under a begin/end is. So we want begins, but not - // the function + VSymEnt* dotSymp; + if (nodep->dotted().empty()) { + // Non-'super.' dotted reference + dotSymp = m_ds.m_dotSymp; + } else { + // Start search at module, as a variable of same name under a subtask isn't a + // relevant hit however a function under a begin/end is. So we want begins, but + // not the function + dotSymp = m_curSymp; + } if (nodep->classOrPackagep()) { // Look only in specified package dotSymp = m_statep->getNodeSym(nodep->classOrPackagep()); UINFO(8, " Override classOrPackage " << dotSymp << endl); @@ -3335,7 +3334,7 @@ private: void visit(AstNodeBlock* nodep) override { UINFO(5, " " << nodep << endl); checkNoDot(nodep); - VSymEnt* const oldCurSymp = m_curSymp; + VL_RESTORER(m_curSymp); { if (nodep->name() != "") { m_ds.m_dotSymp = m_curSymp = m_statep->getNodeSym(nodep); @@ -3343,7 +3342,7 @@ private: } iterateChildren(nodep); } - m_ds.m_dotSymp = m_curSymp = oldCurSymp; + m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); UINFO(5, " cur=se" << cvtToHex(m_curSymp) << endl); } void visit(AstNodeFTask* nodep) override { @@ -3368,7 +3367,7 @@ private: nodep->v3error("Definition not found for extern " + nodep->prettyNameQ()); } } - VSymEnt* const oldCurSymp = m_curSymp; + VL_RESTORER(m_curSymp); { m_ftaskp = nodep; m_ds.m_dotSymp = m_curSymp = m_statep->getNodeSym(nodep); @@ -3383,28 +3382,28 @@ private: } } } - m_ds.m_dotSymp = m_curSymp = oldCurSymp; + m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); m_ftaskp = nullptr; } void visit(AstForeach* nodep) override { UINFO(5, " " << nodep << endl); checkNoDot(nodep); - VSymEnt* const oldCurSymp = m_curSymp; + VL_RESTORER(m_curSymp); { m_ds.m_dotSymp = m_curSymp = m_statep->getNodeSym(nodep); iterateChildren(nodep); } - m_ds.m_dotSymp = m_curSymp = oldCurSymp; + m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); } void visit(AstWith* nodep) override { UINFO(5, " " << nodep << endl); checkNoDot(nodep); - VSymEnt* const oldCurSymp = m_curSymp; + VL_RESTORER(m_curSymp); { m_ds.m_dotSymp = m_curSymp = m_statep->getNodeSym(nodep); iterateChildren(nodep); } - m_ds.m_dotSymp = m_curSymp = oldCurSymp; + m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); } void visit(AstLambdaArgRef* nodep) override { UINFO(5, " " << nodep << endl); @@ -3582,6 +3581,7 @@ private: } } } + m_ds.m_dotSymp = VL_RESTORER_PREV(m_curSymp); } void visit(AstRefDType* nodep) override { // Resolve its reference diff --git a/test_regress/t/t_class_nested_link.pl b/test_regress/t/t_class_nested_link.pl new file mode 100755 index 000000000..1aa73f80a --- /dev/null +++ b/test_regress/t/t_class_nested_link.pl @@ -0,0 +1,21 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 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 + +scenarios(simulator => 1); + +compile( + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_class_nested_link.v b/test_regress/t/t_class_nested_link.v new file mode 100644 index 000000000..def276cc9 --- /dev/null +++ b/test_regress/t/t_class_nested_link.v @@ -0,0 +1,45 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Anthony Donlon. +// SPDX-License-Identifier: CC0-1.0 + +/// Test for bug4553 + +// bit0: 'new' called +// bit1: 'myfunc' called +// bit2: 'myfunc' in class called +int calls = 0; + +module t; + // int calls = 0; // TODO: Error: Internal Error: Can't locate varref scope + + function void myfunc(); + calls |= 32'b10; + endfunction : myfunc + + class Cls #(int A = 0); + function new(); + calls |= 32'b1; + endfunction : new + function void myfunc(); + calls |= 32'b100; + endfunction : myfunc + endclass + + Cls #(100) cls; + + // this block is following the definition of Cls + initial begin + cls = new; + myfunc(); + + if (calls != 32'b011) begin + $write("calls: %0b\n", calls); + $stop; + end + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule