From ad343f8260cf429e786e73cace8ecf5e96654937 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Tue, 26 Sep 2023 22:15:48 -0400 Subject: [PATCH] Add warning on interface instantiation without parens (#4094). --- Changes | 1 + src/V3AstNodeOther.h | 4 ++++ src/V3LinkCells.cpp | 6 ++++++ src/verilog.y | 7 ++++--- test_regress/t/t_fuzz_genintf_bad.v | 2 +- .../t/t_interface_paren_missing_bad.out | 5 +++++ .../t/t_interface_paren_missing_bad.pl | 19 +++++++++++++++++++ .../t/t_interface_paren_missing_bad.v | 15 +++++++++++++++ test_regress/t/t_interface_virtual.v | 2 +- test_regress/t/t_interface_virtual_bad.v | 4 ++-- 10 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 test_regress/t/t_interface_paren_missing_bad.out create mode 100755 test_regress/t/t_interface_paren_missing_bad.pl create mode 100644 test_regress/t/t_interface_paren_missing_bad.v diff --git a/Changes b/Changes index 283efe579..6c60e88e3 100644 --- a/Changes +++ b/Changes @@ -14,6 +14,7 @@ Verilator 5.017 devel **Minor:** * Add trace() API even when Verilated without --trace (#4462). [phelter] +* Add warning on interface instantiation without parens (#4094). [Gökçe Aydos] * Support randc (#4349). * Support resizing function call inout arguments (#4467). diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 2908bbe95..ee393ad9d 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -776,6 +776,7 @@ class AstCell final : public AstNode { string m_modName; // Module the cell instances AstNodeModule* m_modp = nullptr; // [AfterLink] Pointer to module instanced bool m_hasIfaceVar : 1; // True if a Var has been created for this cell + bool m_hasNoParens : 1; // Instantiation has no parenthesis bool m_recursive : 1; // Self-recursive module bool m_trace : 1; // Trace this cell public: @@ -787,6 +788,7 @@ public: , m_origName{instName} , m_modName{modName} , m_hasIfaceVar{false} + , m_hasNoParens{false} , m_recursive{false} , m_trace{true} { this->addPinsp(pinsp); @@ -810,6 +812,8 @@ public: void modp(AstNodeModule* nodep) { m_modp = nodep; } bool hasIfaceVar() const { return m_hasIfaceVar; } void hasIfaceVar(bool flag) { m_hasIfaceVar = flag; } + bool hasNoParens() const { return m_hasNoParens; } + void hasNoParens(bool flag) { m_hasNoParens = flag; } void trace(bool flag) { m_trace = flag; } bool isTrace() const { return m_trace; } void recursive(bool flag) { m_recursive = flag; } diff --git a/src/V3LinkCells.cpp b/src/V3LinkCells.cpp index d98a9158b..4d18030eb 100644 --- a/src/V3LinkCells.cpp +++ b/src/V3LinkCells.cpp @@ -455,6 +455,12 @@ private: nodep->addNextHere(varp); nodep->hasIfaceVar(true); } + if (nodep->hasNoParens()) { + nodep->v3error("Interface instantiation " + << nodep->prettyNameQ() << " requires parenthesis\n" + << nodep->warnMore() << "... Suggest use '" << nodep->prettyName() + << "()'"); + } } if (nodep->modp()) { // iterateChildren(nodep); diff --git a/src/verilog.y b/src/verilog.y index 39f2e31ff..6d06f969e 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -133,7 +133,7 @@ public: return new AstText{fileline, newtext}; } AstNode* createCellOrIfaceRef(FileLine* fileline, const string& name, AstPin* pinlistp, - AstNodeRange* rangelistp) { + AstNodeRange* rangelistp, bool parens) { // Must clone m_instParamp as may be comma'ed list of instances VSymEnt* const foundp = SYMP->symCurrentp()->findIdFallback(name); if (foundp && VN_IS(foundp->nodep(), Port)) { @@ -155,6 +155,7 @@ public: pinlistp, (GRAMMARP->m_instParamp ? GRAMMARP->m_instParamp->cloneTree(true) : nullptr), GRAMMARP->scrubRange(rangelistp)}; + nodep->hasNoParens(!parens); nodep->trace(GRAMMARP->allTracingOn(fileline)); return nodep; } @@ -3187,9 +3188,9 @@ instnameList: instnameParen: id instRangeListE '(' cellpinListE ')' - { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, $4, $2); } + { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, $4, $2, true); } | id instRangeListE - { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, nullptr, $2); } + { $$ = GRAMMARP->createCellOrIfaceRef($1, *$1, nullptr, $2, false); } //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. diff --git a/test_regress/t/t_fuzz_genintf_bad.v b/test_regress/t/t_fuzz_genintf_bad.v index 53df6dea8..99fad227c 100644 --- a/test_regress/t/t_fuzz_genintf_bad.v +++ b/test_regress/t/t_fuzz_genintf_bad.v @@ -18,7 +18,7 @@ module m1 endmodule module t; - intf ifs; + intf ifs(); m1 m0( j.e(0), diff --git a/test_regress/t/t_interface_paren_missing_bad.out b/test_regress/t/t_interface_paren_missing_bad.out new file mode 100644 index 000000000..161dad3fc --- /dev/null +++ b/test_regress/t/t_interface_paren_missing_bad.out @@ -0,0 +1,5 @@ +%Error: t/t_interface_paren_missing_bad.v:13:9: Interface instantiation 'intf_i' requires parenthesis + : ... Suggest use 'intf_i()' + 13 | intf intf_i; + | ^~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_interface_paren_missing_bad.pl b/test_regress/t/t_interface_paren_missing_bad.pl new file mode 100755 index 000000000..b564041ff --- /dev/null +++ b/test_regress/t/t_interface_paren_missing_bad.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 2023 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(vlt => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_interface_paren_missing_bad.v b/test_regress/t/t_interface_paren_missing_bad.v new file mode 100644 index 000000000..c1a9edae4 --- /dev/null +++ b/test_regress/t/t_interface_paren_missing_bad.v @@ -0,0 +1,15 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// Interface instantiation without paranthesis +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Goekce Aydos. +// SPDX-License-Identifier: CC0-1.0 + +interface intf; +endinterface + +module t; + intf intf_i; + initial $finish; +endmodule diff --git a/test_regress/t/t_interface_virtual.v b/test_regress/t/t_interface_virtual.v index 878df0995..0e05cf47e 100644 --- a/test_regress/t/t_interface_virtual.v +++ b/test_regress/t/t_interface_virtual.v @@ -25,7 +25,7 @@ endclass module t (/*AUTOARG*/); - PBus ia, ib; + PBus ia(), ib(); virtual PBus va, vb; virtual PBus.phy pa, pb; Cls ca, cb; diff --git a/test_regress/t/t_interface_virtual_bad.v b/test_regress/t/t_interface_virtual_bad.v index 3c4f690da..55c99e285 100644 --- a/test_regress/t/t_interface_virtual_bad.v +++ b/test_regress/t/t_interface_virtual_bad.v @@ -19,8 +19,8 @@ typedef virtual PBus vpbus_t; module t (/*AUTOARG*/); - PBus p8; - QBus q8; + PBus p8(); + QBus q8(); vpbus_t v8; virtual PBus.phy v8_phy; logic data;