From e21198cb2dfd9963f62685805e792a32b982fbe2 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Fri, 3 Mar 2023 21:06:52 -0500 Subject: [PATCH] Add warning on missing class reference #() --- src/V3AstNodeOther.h | 4 +++ src/V3LinkDot.cpp | 10 ++++++- src/V3LinkParse.cpp | 1 + src/verilog.y | 28 +++++++++---------- test_regress/t/t_class_param.v | 4 +-- test_regress/t/t_class_param_bad_paren.out | 5 ++++ test_regress/t/t_class_param_bad_paren.pl | 19 +++++++++++++ test_regress/t/t_class_param_bad_paren.v | 31 ++++++++++++++++++++++ 8 files changed, 85 insertions(+), 17 deletions(-) create mode 100644 test_regress/t/t_class_param_bad_paren.out create mode 100755 test_regress/t/t_class_param_bad_paren.pl create mode 100644 test_regress/t/t_class_param_bad_paren.v diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index fe0594267..87bad25ba 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -210,6 +210,7 @@ private: bool m_modTrace : 1; // Tracing this module bool m_inLibrary : 1; // From a library, no error if not used, never top level bool m_dead : 1; // LinkDot believes is dead; will remove in Dead visitors + bool m_hasGParam : 1; // Has global parameter (for link) bool m_hierBlock : 1; // Hierarchical Block marked by HIER_BLOCK pragma bool m_internal : 1; // Internally created bool m_recursive : 1; // Recursive module @@ -223,6 +224,7 @@ protected: , m_modTrace{false} , m_inLibrary{false} , m_dead{false} + , m_hasGParam{false} , m_hierBlock{false} , m_internal{false} , m_recursive{false} @@ -250,6 +252,8 @@ public: bool modTrace() const { return m_modTrace; } void dead(bool flag) { m_dead = flag; } bool dead() const { return m_dead; } + void hasGParam(bool flag) { m_hasGParam = flag; } + bool hasGParam() const { return m_hasGParam; } void hierBlock(bool flag) { m_hierBlock = flag; } bool hierBlock() const { return m_hierBlock; } void internal(bool flag) { m_internal = flag; } diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 69b2e83bf..db591012f 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -2227,6 +2227,7 @@ private: } } } + // VISITs void visit(AstNetlist* nodep) override { // Recurse..., backward as must do packages before using packages @@ -2822,8 +2823,15 @@ private: UINFO(4, "(Backto) Link ClassOrPackageRef: " << nodep << endl); iterateChildren(nodep); } + if (m_statep->forPrimary() && VN_IS(nodep->classOrPackagep(), Class) && !nodep->paramsp() + && nodep->classOrPackagep()->hasGParam() + // Don't warn on typedefs, which are hard to know if there's a param somewhere buried + && VN_IS(nodep->classOrPackageNodep(), Class)) { + nodep->v3error("Reference to parameterized class without #() (IEEE 1800-2017 8.25.1)\n" + << nodep->warnMore() << "... Suggest use '" + << nodep->classOrPackageNodep()->prettyName() << "#()'"); + } } - void visit(AstVarRef* nodep) override { // VarRef: Resolve its reference // ParseRefs are used the first pass (forPrimary) so we shouldn't get can't find diff --git a/src/V3LinkParse.cpp b/src/V3LinkParse.cpp index 7368edb62..c0984fe98 100644 --- a/src/V3LinkParse.cpp +++ b/src/V3LinkParse.cpp @@ -225,6 +225,7 @@ private: if (nodep->lifetime().isNone() && nodep->varType() != VVarType::PORT) { nodep->lifetime(m_lifetime); } + if (nodep->isGParam() && m_modp) m_modp->hasGParam(true); if (nodep->isParam() && !nodep->valuep() && nodep->fileline()->language() < V3LangCode::L1800_2009) { nodep->v3error("Parameter requires default value, or use IEEE 1800-2009 or later."); diff --git a/src/verilog.y b/src/verilog.y index 56f0deb55..a5728f0f7 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -1394,7 +1394,7 @@ parameter_value_assignmentE: // IEEE: [ parameter_value_assignment ] ; parameter_value_assignment: // IEEE: parameter_value_assignment - '#' '(' cellparamList ')' { $$ = $3; } + '#' '(' cellparamListE ')' { $$ = $3; } // // Parentheses are optional around a single parameter | '#' yaINTNUM { $$ = new AstPin{$2, 1, "", new AstConst{$2, *$2}}; } | '#' yaFLOATNUM { $$ = new AstPin{$2, 1, "", @@ -1408,7 +1408,7 @@ parameter_value_assignment: // IEEE: parameter_value_assignment parameter_value_assignmentClass: // IEEE: [ parameter_value_assignment ] (for classes) // // Like parameter_value_assignment, but for classes only, which always have #() - '#' '(' cellparamList ')' { $$ = $3; } + '#' '(' cellparamListE ')' { $$ = $3; } ; parameter_port_listE: // IEEE: parameter_port_list + empty == parameter_value_assignment @@ -3160,11 +3160,11 @@ instnameList: ; instnameParen: - id instRangeListE '(' cellpinList ')' + id instRangeListE '(' cellpinListE ')' { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, $4, $2); } | id instRangeListE { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, nullptr, $2); } - //UNSUP instRangeListE '(' cellpinList ')' { UNSUP } // UDP + //UNSUP instRangeListE '(' cellpinListE ')' { UNSUP } // UDP // // Adding above and switching to the Verilog-Perl syntax // // causes a shift conflict due to use of idClassSel inside exprScope. // // It also breaks allowing "id foo;" instantiation syntax. @@ -3187,22 +3187,22 @@ instRange: { $$ = new AstRange{$1, $2, $4}; } ; -cellparamList: - { GRAMMARP->pinPush(); } cellparamItList { $$ = $2; GRAMMARP->pinPop(CRELINE()); } +cellparamListE: + { GRAMMARP->pinPush(); } cellparamItListE { $$ = $2; GRAMMARP->pinPop(CRELINE()); } ; -cellpinList: - {VARRESET_LIST(UNKNOWN);} cellpinItList { $$ = $2; VARRESET_NONLIST(UNKNOWN); } +cellpinListE: + { VARRESET_LIST(UNKNOWN); } cellpinItListE { $$ = $2; VARRESET_NONLIST(UNKNOWN); } ; -cellparamItList: // IEEE: list_of_parameter_assignmente +cellparamItListE: // IEEE: list_of_parameter_assignmente cellparamItemE { $$ = $1; } - | cellparamItList ',' cellparamItemE { $$ = addNextNull($1, $3); } + | cellparamItListE ',' cellparamItemE { $$ = addNextNull($1, $3); } ; -cellpinItList: // IEEE: list_of_port_connections +cellpinItListE: // IEEE: list_of_port_connections cellpinItemE { $$ = $1; } - | cellpinItList ',' cellpinItemE { $$ = addNextNull($1, $3); } + | cellpinItListE ',' cellpinItemE { $$ = addNextNull($1, $3); } ; cellparamItemE: // IEEE: named_parameter_assignment + empty @@ -6602,8 +6602,8 @@ checker_generate_item: // ==IEEE: checker_generate_item //UNSUPchecker_instantiation: //UNSUP // // Only used for procedural_assertion_item's //UNSUP // // Version in concurrent_assertion_item looks like etcInst -//UNSUP // // Thus instead of *_checker_port_connection we can use etcInst's cellpinList -//UNSUP id/*checker_identifier*/ id '(' cellpinList ')' ';' { } +//UNSUP // // Thus instead of *_checker_port_connection we can use etcInst's cellpinListE +//UNSUP id/*checker_identifier*/ id '(' cellpinListE ')' ';' { } //UNSUP ; //********************************************************************** diff --git a/test_regress/t/t_class_param.v b/test_regress/t/t_class_param.v index 7028eb98e..d0fc116f7 100644 --- a/test_regress/t/t_class_param.v +++ b/test_regress/t/t_class_param.v @@ -210,11 +210,11 @@ module t (/*AUTOARG*/); if(dict_op.get("abcd") != 1) $stop; if (getter1.get_1() != 1) $stop; - if (Getter1::get_1() != 1) $stop; + if (Getter1#()::get_1() != 1) $stop; if (getter1_param_1.get_1() != 1) $stop; if (getter2.get_2() != 2) $stop; - if (Getter2::get_2() != 2) $stop; + if (Getter2#()::get_2() != 2) $stop; if (Getter2#(2)::get_2() != 2) $stop; $write("*-* All Finished *-*\n"); diff --git a/test_regress/t/t_class_param_bad_paren.out b/test_regress/t/t_class_param_bad_paren.out new file mode 100644 index 000000000..8de97f3d9 --- /dev/null +++ b/test_regress/t/t_class_param_bad_paren.out @@ -0,0 +1,5 @@ +%Error: t/t_class_param_bad_paren.v:28:11: Reference to parameterized class without #() (IEEE 1800-2017 8.25.1) + : ... Suggest use 'Cls#()' + 28 | if (Cls::OTHER != 12) $stop; + | ^~~ +%Error: Exiting due to diff --git a/test_regress/t/t_class_param_bad_paren.pl b/test_regress/t/t_class_param_bad_paren.pl new file mode 100755 index 000000000..44783b1f6 --- /dev/null +++ b/test_regress/t/t_class_param_bad_paren.pl @@ -0,0 +1,19 @@ +#!/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(linter => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_class_param_bad_paren.v b/test_regress/t/t_class_param_bad_paren.v new file mode 100644 index 000000000..e81209326 --- /dev/null +++ b/test_regress/t/t_class_param_bad_paren.v @@ -0,0 +1,31 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +class Cls #(int PARAM = 1); + parameter OTHER = 12; +endclass + +class Other extends Cls#(); // Ok +endclass + +class OtherMaybe extends Cls; // Questionable but others do not warn +endclass + +module t (/*AUTOARG*/); + + typedef Cls#(2) Cls2_t; // Ok + typedef Cls ClsNone_t; // Ok + + Cls c; // Ok + + initial begin + if (Cls#()::OTHER != 12) $stop; // Ok + if (Cls2_t::OTHER != 12) $stop; // ok + + if (Cls::OTHER != 12) $stop; // Bad #() required + end + +endmodule