From cff37f077502ed361d3eadf5f8225146df782067 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 1 Jul 2023 08:31:53 -0400 Subject: [PATCH] Add GENUNNAMED lint warning. Also fix generate-for blocks with empty statements getting lost. --- Changes | 1 + docs/guide/exe_verilator.rst | 4 +- docs/guide/warnings.rst | 46 +++++++++++++++++++++++ src/V3Error.h | 12 +++--- src/V3LinkParse.cpp | 19 ++++++++-- src/verilog.y | 8 ++-- test_regress/t/t_lint_declfilename_bbox.v | 2 +- test_regress/t/t_lint_genunnamed_bad.out | 21 +++++++++++ test_regress/t/t_lint_genunnamed_bad.pl | 20 ++++++++++ test_regress/t/t_lint_genunnamed_bad.v | 36 ++++++++++++++++++ test_regress/t/t_lint_unused_bad.v | 7 +++- 11 files changed, 159 insertions(+), 17 deletions(-) create mode 100644 test_regress/t/t_lint_genunnamed_bad.out create mode 100755 test_regress/t/t_lint_genunnamed_bad.pl create mode 100644 test_regress/t/t_lint_genunnamed_bad.v diff --git a/Changes b/Changes index fa45b7f1a..e3086370a 100644 --- a/Changes +++ b/Changes @@ -15,6 +15,7 @@ Verilator 5.013 devel * Deprecation planned for 32-bit pointer -m32 mode (#4268). * Support some stream operations on queues (#4292). [Ryszard Rozak, Antmicro Ltd] +* Add GENUNNAMED lint warning. [Srinivasan Venkataramanan, Deepa Palaniappan] * Fix 'VlForkSync' redeclaration (#4277). [Krzysztof Bieganski, Antmicro Ltd] * Fix processes that can outlive their parents (#4253). [Krzysztof Boronski, Antmicro Ltd] * Fix duplicate fork names (#4295). [Ryszard Rozak, Antmicro Ltd] diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index a4d0af876..faede475a 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -1539,7 +1539,7 @@ Summary: Disable all code style related warning messages (note that by default, they are already disabled). This is equivalent to ``-Wno-DECLFILENAME -Wno-DEFPARAM - -Wno-EOFNEWLINE -Wno-IMPORTSTAR -Wno-INCABSPATH -Wno-PINCONNECTEMPTY + -Wno-EOFNEWLINE -Wno-GENUNNAMED -Wno-IMPORTSTAR -Wno-INCABSPATH -Wno-PINCONNECTEMPTY -Wno-PINNOCONNECT -Wno-SYNCASYNCNET -Wno-UNDRIVEN -Wno-UNUSEDGENVAR -Wno-UNUSEDPARAM -Wno-UNUSEDSIGNAL -Wno-VARHIDDEN``. @@ -1571,7 +1571,7 @@ Summary: Enable all code style-related warning messages. This is equivalent to ``-Wwarn ASSIGNDLY -Wwarn-DECLFILENAME -Wwarn-DEFPARAM -Wwarn-EOFNEWLINE - -Wwarn-INCABSPATH -Wwarn-PINNOCONNECT -Wwarn-SYNCASYNCNET -Wwarn-UNDRIVEN + -Wwarn-GENUNNAMED -Wwarn-INCABSPATH -Wwarn-PINNOCONNECT -Wwarn-SYNCASYNCNET -Wwarn-UNDRIVEN -Wwarn-UNUSEDGENVAR -Wwarn-UNUSEDPARAM -Wwarn-UNUSEDSIGNAL -Wwarn-VARHIDDEN``. .. option:: --x-assign 0 diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 69b4d80dd..94af80a31 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -670,6 +670,52 @@ List Of Warnings used as a clock. +.. option:: GENUNNAMED + + Warns that a generate block was unnamed and "genblk" will be used per + IEEE. + + The potential issue is that adding additional generate blocks will + renumber the assigned names, which may cause evental problems with + synthesis constraints or other tools that depend on hierarchical paths + remaining consistend. + + Blocks that are empty may not be reported with this warning, as no + scopes are created for empty blocks, so there is no harm in having them + unnamed. + + Disabled by default as this is a code-style warning; it will simulate + correctly. + + .. code-block:: sv + :linenos: + :emphasize-lines: 2 + + generate + if (PARAM == 1) begin //<--- Warning + end + + Results in: + + .. code-block:: + + %Warning-GENUNNAMED: example.v:2:9: Unnamed generate block (IEEE 1800-2017 27.6) + + To fix this assign a label (often with the naming convention prefix of + 'gen_' or 'g_'), for example: + + .. code-block:: sv + :linenos: + :emphasize-lines: 2 + + generate + if (PARAM == 1) begin : gen_param_1 //<--- Repaired + end + + Other tools with similar warnings: Verible's generate-label, "All + generate block statements must have a label." + + .. option:: HIERBLOCK Warns that the top module is marked as a hierarchy block by the diff --git a/src/V3Error.h b/src/V3Error.h index 335549dfa..264f8c0c1 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -99,6 +99,7 @@ public: ENUMVALUE, // Error: enum type needs explicit cast EOFNEWLINE, // End-of-file missing newline GENCLK, // Generated Clock. Historical, never issued. + GENUNNAMED, // Generate unnamed, without label HIERBLOCK, // Ignored hierarchical block setting IFDEPTH, // If statements too deep IGNOREDRETURN, // Ignoring return value (function as task) @@ -192,7 +193,8 @@ public: "CASEINCOMPLETE", "CASEOVERLAP", "CASEWITHX", "CASEX", "CASTCONST", "CDCRSTLOGIC", "CLKDATA", "CMPCONST", "COLONPLUS", "COMBDLY", "CONSTRAINTIGN", "CONTASSREG", "DECLFILENAME", "DEFPARAM", "DEPRECATED", - "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "HIERBLOCK", + "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "GENUNNAMED", + "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE", @@ -243,10 +245,10 @@ public: bool styleError() const VL_MT_SAFE { return (m_e == ASSIGNDLY // More than style, but for backward compatibility || m_e == BLKSEQ || m_e == DEFPARAM || m_e == DECLFILENAME || m_e == EOFNEWLINE - || m_e == IMPORTSTAR || m_e == INCABSPATH || m_e == PINCONNECTEMPTY - || m_e == PINNOCONNECT || m_e == SYNCASYNCNET || m_e == UNDRIVEN - || m_e == UNUSEDGENVAR || m_e == UNUSEDPARAM || m_e == UNUSEDSIGNAL - || m_e == VARHIDDEN); + || m_e == GENUNNAMED || m_e == IMPORTSTAR || m_e == INCABSPATH + || m_e == PINCONNECTEMPTY || m_e == PINNOCONNECT || m_e == SYNCASYNCNET + || m_e == UNDRIVEN || m_e == UNUSEDGENVAR || m_e == UNUSEDPARAM + || m_e == UNUSEDSIGNAL || m_e == VARHIDDEN); } // Warnings that are unused only bool unusedError() const VL_MT_SAFE { diff --git a/src/V3LinkParse.cpp b/src/V3LinkParse.cpp index 9da829f30..e434a7177 100644 --- a/src/V3LinkParse.cpp +++ b/src/V3LinkParse.cpp @@ -580,14 +580,25 @@ private: // The genblk name will get attached to the if true/false LOWER begin block(s) const bool nestedIf = nestedIfBegin(nodep); // It's not FOR(BEGIN(...)) but we earlier changed it to BEGIN(FOR(...)) + int assignGenBlkNum = -1; if (nodep->genforp()) { ++m_genblkNum; - if (nodep->name() == "") nodep->name("genblk" + cvtToStr(m_genblkNum)); + if (nodep->name() == "") assignGenBlkNum = m_genblkNum; + } else if (nodep->generate() && nodep->name() == "" && assignGenBlkNum == -1 + && (VN_IS(backp, CaseItem) || VN_IS(backp, GenIf)) && !nestedIf) { + assignGenBlkNum = m_genblkAbove; } - if (nodep->generate() && nodep->name() == "" - && (VN_IS(backp, CaseItem) || VN_IS(backp, GenIf)) && !nestedIf) { - nodep->name("genblk" + cvtToStr(m_genblkAbove)); + if (assignGenBlkNum != -1) { + nodep->name("genblk" + cvtToStr(assignGenBlkNum)); + if (nodep->stmtsp()) { + nodep->v3warn(GENUNNAMED, + "Unnamed generate block " + << nodep->prettyNameQ() << " (IEEE 1800-2017 27.6)" + << nodep->warnMore() + << "... Suggest assign a label with 'begin : gen_'"); + } } + if (nodep->name() != "") { VL_RESTORER(m_genblkAbove); VL_RESTORER(m_genblkNum); diff --git a/src/verilog.y b/src/verilog.y index c7d17f3fd..0b8dd10e5 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -2753,12 +2753,14 @@ genItemBegin: // IEEE: part of generate_block { $$ = new AstBegin{$1, *$1, $4, true, false}; GRAMMARP->endLabel($6, *$1, $6); } | id yP_COLON__BEGIN yBEGIN yEND endLabelE - { $$ = nullptr; GRAMMARP->endLabel($5, *$1, $5); } + { $$ = new AstBegin{$1, *$1, nullptr, true, false}; + GRAMMARP->endLabel($5, *$1, $5); } | yBEGIN ':' idAny ~c~genItemList yEND endLabelE { $$ = new AstBegin{$3, *$3, $4, true, false}; GRAMMARP->endLabel($6, *$3, $6); } | yBEGIN ':' idAny yEND endLabelE - { $$ = nullptr; GRAMMARP->endLabel($5, *$3, $5); } + { $$ = new AstBegin{$3, *$3, nullptr, true, false}; + GRAMMARP->endLabel($5, *$3, $5); } ; c_genItemBegin: // IEEE: part of generate_block (for checkers) @@ -5186,7 +5188,7 @@ let_port_list: // IEEE: let_port_list ; let_port_item: // IEEE: let_port_Item - // // IEEE: Expanded let_formal_type + // // IEEE: Expanded let_formal_type yUNTYPED idAny/*formal_port_identifier*/ variable_dimensionListE exprEqE { $$ = nullptr; BBUNSUP($1, "Unsupported: let untyped ports"); } | data_type id/*formal_port_identifier*/ variable_dimensionListE exprEqE diff --git a/test_regress/t/t_lint_declfilename_bbox.v b/test_regress/t/t_lint_declfilename_bbox.v index c9bea6e23..5809d71df 100644 --- a/test_regress/t/t_lint_declfilename_bbox.v +++ b/test_regress/t/t_lint_declfilename_bbox.v @@ -6,7 +6,7 @@ module t_lint_declfilename_bbox (); parameter IN = 0; - if (IN) begin + if (IN) begin : gen_hasbbox // Should not warn, see bug2430 BLACKBOXED bboxed (); end diff --git a/test_regress/t/t_lint_genunnamed_bad.out b/test_regress/t/t_lint_genunnamed_bad.out new file mode 100644 index 000000000..2989ed1aa --- /dev/null +++ b/test_regress/t/t_lint_genunnamed_bad.out @@ -0,0 +1,21 @@ +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:14:6: Unnamed generate block 'genblk2' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 14 | begin + | ^~~~~ + ... For warning description see https://verilator.org/warn/GENUNNAMED?v=latest + ... Use "/* verilator lint_off GENUNNAMED */" and lint_on around source to disable this message. +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:18:6: Unnamed generate block 'genblk2' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 18 | begin + | ^~~~~ +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:22:4: Unnamed generate block 'genblk3' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 22 | for (genvar v = 0; v < P; ++v) ; + | ^~~ +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:24:4: Unnamed generate block 'genblk4' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 24 | for (genvar v = 0; v < P; ++v) + | ^~~ +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:30:9: Unnamed generate block 'genblk5' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 30 | 1: initial begin end + | ^~~~~~~ +%Warning-GENUNNAMED: t/t_lint_genunnamed_bad.v:31:9: Unnamed generate block 'genblk5' (IEEE 1800-2017 27.6) : ... Suggest assign a label with 'begin : gen_' + 31 | 2: begin + | ^~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_genunnamed_bad.pl b/test_regress/t/t_lint_genunnamed_bad.pl new file mode 100755 index 000000000..4ad25735e --- /dev/null +++ b/test_regress/t/t_lint_genunnamed_bad.pl @@ -0,0 +1,20 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003-2009 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, + verilator_flags2 => ["--lint-only -Wall -Wno-DECLFILENAME"], + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_genunnamed_bad.v b/test_regress/t/t_lint_genunnamed_bad.v new file mode 100644 index 000000000..44c701abb --- /dev/null +++ b/test_regress/t/t_lint_genunnamed_bad.v @@ -0,0 +1,36 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t (/*AUTOARG*/); + + parameter P = 1; + + if (P) ; + + if (P) + begin + initial $display; + end + else + begin + initial $display; + end + + for (genvar v = 0; v < P; ++v) ; + + for (genvar v = 0; v < P; ++v) + begin + initial $display; + end + + case (P) + 1: initial begin end + 2: begin + initial begin end + end + endcase + +endmodule diff --git a/test_regress/t/t_lint_unused_bad.v b/test_regress/t/t_lint_unused_bad.v index cb2908289..cc837c4ec 100644 --- a/test_regress/t/t_lint_unused_bad.v +++ b/test_regress/t/t_lint_unused_bad.v @@ -71,8 +71,11 @@ module sub; end generate - if (0) - for (ok_gv = 0; ok_gv < 1; ++ok_gv) begin end + if (0) begin : gen_gv_if0 + for (ok_gv = 0; ok_gv < 1; ++ok_gv) + begin : gen_gv_if0_for + end + end endgenerate endmodule