From 53b8a5b0274cd5e3969e2145d8a20d506b879cc5 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sun, 21 Sep 2025 09:41:58 -0400 Subject: [PATCH] Add error on zero/negative unpacked dimensions (#1642). --- Changes | 1 + src/V3AstNodeOther.h | 14 ++++++++++---- src/V3AstNodes.cpp | 2 ++ src/V3Width.cpp | 14 +++++++++++--- src/verilog.y | 2 +- test_regress/t/t_constraint_json_only.out | 2 +- test_regress/t/t_dump_json.out | 14 +++++++------- test_regress/t/t_fuzz_eqne_bad.out | 6 +++++- test_regress/t/t_json_only_debugcheck.out | 6 +++--- test_regress/t/t_json_only_tag.out | 2 +- test_regress/t/t_lint_range_negative_bad.out | 10 ++++++++++ test_regress/t/t_lint_range_negative_bad.py | 16 ++++++++++++++++ test_regress/t/t_lint_range_negative_bad.v | 15 +++++++++++++++ test_regress/t/t_sys_file_basic.v | 2 +- test_regress/t/t_var_port_json_only.out | 4 ++-- 15 files changed, 86 insertions(+), 24 deletions(-) create mode 100644 test_regress/t/t_lint_range_negative_bad.out create mode 100755 test_regress/t/t_lint_range_negative_bad.py create mode 100644 test_regress/t/t_lint_range_negative_bad.v diff --git a/Changes b/Changes index 1fbcb5859..48a4c4e25 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,7 @@ Verilator 5.041 devel **Other:** +* Add error on zero/negative unpacked dimensions (#1642). [Stefan Wallentowitz] * Add error on non-packed struct randc (#5999). [Seth Pellegrino] * Add configure `--enable-asan` to compile verilator_bin with the address sanitizer (#6404). [Geza Lore] * Add $(LDFLAGS) and $(LIBS) to when building shared libraries (#6425) (#6426). [Ahmed El-Mahmoudy] diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index c22ffcb79..71b95d8e7 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -2720,7 +2720,7 @@ public: // === AstNodeRange === class AstBracketRange final : public AstNodeRange { - // Parser only concept "[lhsp]", an AstUnknownRange, QueueRange or Range, + // Parser only concept C-style "[lhsp]", an AstUnknownRange, QueueRange or Range, // unknown until lhsp type is determined // @astgen op1 := elementsp : AstNode public: @@ -2740,9 +2740,11 @@ class AstRange final : public AstNodeRange { // Range specification, for use under variables and cells // @astgen op1 := leftp : AstNodeExpr // @astgen op2 := rightp : AstNodeExpr + const bool m_fromBracket = false; // From C-style '[x]' declaration public: - AstRange(FileLine* fl, AstNodeExpr* leftp, AstNodeExpr* rightp) - : ASTGEN_SUPER_Range(fl) { + AstRange(FileLine* fl, AstNodeExpr* leftp, AstNodeExpr* rightp, bool fromBracket = false) + : ASTGEN_SUPER_Range(fl) + , m_fromBracket{fromBracket} { this->leftp(leftp); this->rightp(rightp); } @@ -2766,7 +2768,11 @@ public: void dump(std::ostream& str) const override; void dumpJson(std::ostream& str) const override; virtual string emitC() { V3ERROR_NA_RETURN(""); } - bool sameNode(const AstNode* /*samep*/) const override { return true; } + bool sameNode(const AstNode* samep) const override { + const AstRange* const asamep = VN_DBG_AS(samep, Range); + return fromBracket() == asamep->fromBracket(); + } + bool fromBracket() const { return m_fromBracket; } }; class AstUnsizedRange final : public AstNodeRange { // Unsized range specification, for open arrays diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 82fbbb14b..22f818699 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -2217,10 +2217,12 @@ void AstNodeRange::dump(std::ostream& str) const { this->AstNode::dump(str); } void AstNodeRange::dumpJson(std::ostream& str) const { dumpJsonGen(str); } void AstRange::dump(std::ostream& str) const { this->AstNodeRange::dump(str); + if (fromBracket()) str << " [FB]"; if (ascending()) str << " [ASCENDING]"; } void AstRange::dumpJson(std::ostream& str) const { dumpJsonBoolFunc(str, ascending); + dumpJsonBoolFunc(str, fromBracket); dumpJsonGen(str); } void AstParamTypeDType::dump(std::ostream& str) const { diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 680483b72..a9696b748 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -961,7 +961,14 @@ class WidthVisitor final : public VNVisitor { if (m_vup->prelim()) { // Don't need to iterate because V3Const already constified const int width = nodep->elementsConst(); - if (width > (1 << 28)) { + if (nodep->fromBracket() && nodep->leftConst() > nodep->rightConst()) { + // From a C-Style '[size]' declaration, due to range handling a '[-1]' + // will get transformed to look like a '[0:1]', instead report as error + // now we know the datatype is used + nodep->v3error("Size of range is '[" + << (-width + 2) + << "]', must be positive integer (IEEE 1800-2023 7.4.2)"); + } else if (width > (1 << 28)) { nodep->v3error("Width of bit range is huge; vector of over 1 billion bits: 0x" << std::hex << width << std::dec); } @@ -1921,7 +1928,7 @@ class WidthVisitor final : public VNVisitor { // Cleanup array size userIterateAndNext(nodep->rangep(), WidthVP{SELF, BOTH}.p()); nodep->dtypep(nodep); // The array itself, not subDtype - if (auto* const adtypep = VN_CAST(nodep, UnpackArrayDType)) { + if (AstUnpackArrayDType* const adtypep = VN_CAST(nodep, UnpackArrayDType)) { // Historically array elements have width of the ref type not the full array nodep->widthFromSub(nodep->subDTypep()); if (nodep->subDTypep()->skipRefp()->isCompound()) adtypep->isCompound(true); @@ -1964,7 +1971,8 @@ class WidthVisitor final : public VNVisitor { nodep->fileline(), VFlagChildDType{}, childp, new AstRange{nodep->fileline(), new AstConst(elementsp->fileline(), 0), new AstSub{elementsNewFl, VN_AS(elementsp, NodeExpr), - new AstConst(elementsNewFl, 1)}}}; + new AstConst(elementsNewFl, 1)}, + true}}; } nodep->replaceWith(newp); VL_DO_DANGLING(nodep->deleteTree(), nodep); diff --git a/src/verilog.y b/src/verilog.y index 4d49ea9b2..c4d72502d 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3189,7 +3189,7 @@ instRangeList: instRange: '[' constExpr ']' - { $$ = new AstRange{$1, new AstConst{$1, 0}, new AstSub{$1, $2, new AstConst{$1, 1}}}; } + { $$ = new AstRange{$1, new AstConst{$1, 0}, new AstSub{$1, $2, new AstConst{$1, 1}}, true}; } | '[' constExpr ':' constExpr ']' { $$ = new AstRange{$1, $2, $4}; } ; diff --git a/test_regress/t/t_constraint_json_only.out b/test_regress/t/t_constraint_json_only.out index 11f9f908f..3735055c5 100644 --- a/test_regress/t/t_constraint_json_only.out +++ b/test_regress/t/t_constraint_json_only.out @@ -63,7 +63,7 @@ {"type":"BASICDTYPE","name":"bit","addr":"(U)","loc":"d,11:9,11:12","dtypep":"(U)","keyword":"bit","generic":true,"rangep": []}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(Y)","loc":"d,15:18,15:19","dtypep":"(Y)","isCompound":false,"declRange":"[0:1]","generic":false,"refDTypep":"(Q)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(PB)","loc":"d,15:18,15:19","ascending":true, + {"type":"RANGE","name":"","addr":"(PB)","loc":"d,15:18,15:19","ascending":true,"fromBracket":true, "leftp": [ {"type":"CONST","name":"32'h0","addr":"(QB)","loc":"d,15:19,15:20","dtypep":"(OB)"} ], diff --git a/test_regress/t/t_dump_json.out b/test_regress/t/t_dump_json.out index 6a165f488..3ff9c864e 100644 --- a/test_regress/t/t_dump_json.out +++ b/test_regress/t/t_dump_json.out @@ -18,7 +18,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(P)","loc":"e,14:4,14:7","dtypep":"(P)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(Q)","loc":"e,14:8,14:9","ascending":false, + {"type":"RANGE","name":"","addr":"(Q)","loc":"e,14:8,14:9","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh3f","addr":"(R)","loc":"e,14:9,14:11","dtypep":"(S)"} ], @@ -31,7 +31,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(V)","loc":"e,15:4,15:7","dtypep":"(V)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(W)","loc":"e,15:8,15:9","ascending":false, + {"type":"RANGE","name":"","addr":"(W)","loc":"e,15:8,15:9","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh3f","addr":"(X)","loc":"e,15:9,15:11","dtypep":"(S)"} ], @@ -44,7 +44,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(AB)","loc":"e,18:9,18:10","dtypep":"(AB)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(BB)","loc":"e,18:9,18:10","ascending":false, + {"type":"RANGE","name":"","addr":"(BB)","loc":"e,18:9,18:10","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh1f","addr":"(CB)","loc":"e,18:10,18:12","dtypep":"(DB)"} ], @@ -73,7 +73,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(MB)","loc":"e,22:9,22:10","dtypep":"(MB)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(NB)","loc":"e,22:9,22:10","ascending":false, + {"type":"RANGE","name":"","addr":"(NB)","loc":"e,22:9,22:10","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh1f","addr":"(OB)","loc":"e,22:10,22:12","dtypep":"(DB)"} ], @@ -119,7 +119,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(FC)","loc":"e,33:9,33:10","dtypep":"(FC)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(GC)","loc":"e,33:9,33:10","ascending":false, + {"type":"RANGE","name":"","addr":"(GC)","loc":"e,33:9,33:10","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh3f","addr":"(HC)","loc":"e,33:10,33:12","dtypep":"(S)"} ], @@ -444,7 +444,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(GH)","loc":"e,79:10,79:11","dtypep":"(GH)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(HH)","loc":"e,79:10,79:11","ascending":false, + {"type":"RANGE","name":"","addr":"(HH)","loc":"e,79:10,79:11","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh1f","addr":"(IH)","loc":"e,79:11,79:13","dtypep":"(DB)"} ], @@ -457,7 +457,7 @@ "childDTypep": [ {"type":"BASICDTYPE","name":"logic","addr":"(LH)","loc":"e,80:11,80:14","dtypep":"(LH)","keyword":"logic","generic":false, "rangep": [ - {"type":"RANGE","name":"","addr":"(MH)","loc":"e,80:15,80:16","ascending":false, + {"type":"RANGE","name":"","addr":"(MH)","loc":"e,80:15,80:16","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"?32?sh1f","addr":"(NH)","loc":"e,80:16,80:18","dtypep":"(DB)"} ], diff --git a/test_regress/t/t_fuzz_eqne_bad.out b/test_regress/t/t_fuzz_eqne_bad.out index 72d8f23be..5411009a1 100644 --- a/test_regress/t/t_fuzz_eqne_bad.out +++ b/test_regress/t/t_fuzz_eqne_bad.out @@ -1,8 +1,12 @@ +%Error: t/t_fuzz_eqne_bad.v:9:9: Size of range is '[0]', must be positive integer (IEEE 1800-2023 7.4.2) + : ... note: In instance 't' + 9 | reg a[0]; + | ^ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. %Error: t/t_fuzz_eqne_bad.v:12:19: Comparison requires matching data types : ... note: In instance 't' : ... Left-hand data type: 'logic$[0:-1]' : ... Right-hand data type: 'logic' 12 | initial c = (a != &b); | ^~ - ... 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_json_only_debugcheck.out b/test_regress/t/t_json_only_debugcheck.out index 2a84b6f06..f003a9509 100644 --- a/test_regress/t/t_json_only_debugcheck.out +++ b/test_regress/t/t_json_only_debugcheck.out @@ -2944,7 +2944,7 @@ {"type":"BASICDTYPE","name":"string","addr":"(RB)","loc":"d,28:4,28:10","dtypep":"(RB)","keyword":"string","generic":true,"rangep": []}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(IC)","loc":"d,17:12,17:16","dtypep":"(IC)","isCompound":false,"declRange":"[7:0]","generic":false,"refDTypep":"(ERB)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(LRB)","loc":"d,17:12,17:16","ascending":false, + {"type":"RANGE","name":"","addr":"(LRB)","loc":"d,17:12,17:16","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"32'h7","addr":"(MRB)","loc":"d,17:12,17:16","dtypep":"(NC)"} ], @@ -2954,7 +2954,7 @@ ]}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(CJ)","loc":"d,17:12,17:16","dtypep":"(CJ)","isCompound":false,"declRange":"[7:0]","generic":false,"refDTypep":"(ERB)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(ORB)","loc":"d,17:12,17:16","ascending":false, + {"type":"RANGE","name":"","addr":"(ORB)","loc":"d,17:12,17:16","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"32'h7","addr":"(PRB)","loc":"d,17:12,17:16","dtypep":"(NC)"} ], @@ -2964,7 +2964,7 @@ ]}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(OM)","loc":"d,17:12,17:16","dtypep":"(OM)","isCompound":true,"declRange":"[7:0]","generic":false,"refDTypep":"(RB)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(RRB)","loc":"d,17:12,17:16","ascending":false, + {"type":"RANGE","name":"","addr":"(RRB)","loc":"d,17:12,17:16","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"32'h7","addr":"(SRB)","loc":"d,17:12,17:16","dtypep":"(NC)"} ], diff --git a/test_regress/t/t_json_only_tag.out b/test_regress/t/t_json_only_tag.out index f75d0525c..722936103 100644 --- a/test_regress/t/t_json_only_tag.out +++ b/test_regress/t/t_json_only_tag.out @@ -79,7 +79,7 @@ {"type":"REFDTYPE","name":"my_struct","addr":"(WB)","loc":"d,31:4,31:13","dtypep":"(K)","generic":false,"typedefp":"UNLINKED","refDTypep":"(K)","classOrPackagep":"UNLINKED","typeofp": [],"classOrPackageOpp": [],"paramsp": []}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(Q)","loc":"d,31:26,31:27","dtypep":"(Q)","isCompound":false,"declRange":"[0:1]","generic":false,"refDTypep":"(WB)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(XB)","loc":"d,31:26,31:27","ascending":true, + {"type":"RANGE","name":"","addr":"(XB)","loc":"d,31:26,31:27","ascending":true,"fromBracket":true, "leftp": [ {"type":"CONST","name":"32'h0","addr":"(YB)","loc":"d,31:27,31:28","dtypep":"(S)"} ], diff --git a/test_regress/t/t_lint_range_negative_bad.out b/test_regress/t/t_lint_range_negative_bad.out new file mode 100644 index 000000000..ffbe1b72b --- /dev/null +++ b/test_regress/t/t_lint_range_negative_bad.out @@ -0,0 +1,10 @@ +%Error: t/t_lint_range_negative_bad.v:8:16: Size of range is '[0]', must be positive integer (IEEE 1800-2023 7.4.2) + : ... note: In instance 't' + 8 | int array_bad[0]; + | ^ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Error: t/t_lint_range_negative_bad.v:9:17: Size of range is '[-1]', must be positive integer (IEEE 1800-2023 7.4.2) + : ... note: In instance 't' + 9 | int array2_bad[-1]; + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_range_negative_bad.py b/test_regress/t/t_lint_range_negative_bad.py new file mode 100755 index 000000000..55203b6c9 --- /dev/null +++ b/test_regress/t/t_lint_range_negative_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_lint_range_negative_bad.v b/test_regress/t/t_lint_range_negative_bad.v new file mode 100644 index 000000000..300efb1ec --- /dev/null +++ b/test_regress/t/t_lint_range_negative_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 Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t; + int array_bad[0]; // <--- Error: Must be positive size + int array2_bad[-1]; // <--- Error: Must be positive size + sub #(1) u_sub(); +endmodule + +module sub #(parameter SIZE=0); + int ignore[SIZE]; +endmodule diff --git a/test_regress/t/t_sys_file_basic.v b/test_regress/t/t_sys_file_basic.v index 762c765d9..1ee489e88 100644 --- a/test_regress/t/t_sys_file_basic.v +++ b/test_regress/t/t_sys_file_basic.v @@ -13,7 +13,7 @@ module t; integer file; - integer file_a[0]; + integer file_a[1]; integer chars; reg [1*8:1] letterl; diff --git a/test_regress/t/t_var_port_json_only.out b/test_regress/t/t_var_port_json_only.out index 603f2a87b..e5be3bfd0 100644 --- a/test_regress/t/t_var_port_json_only.out +++ b/test_regress/t/t_var_port_json_only.out @@ -71,7 +71,7 @@ "typesp": [ {"type":"UNPACKARRAYDTYPE","name":"","addr":"(RB)","loc":"d,58:34,58:35","dtypep":"(RB)","isCompound":false,"declRange":"[5:0]","generic":false,"refDTypep":"(J)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(TB)","loc":"d,58:34,58:35","ascending":false, + {"type":"RANGE","name":"","addr":"(TB)","loc":"d,58:34,58:35","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"32'sh5","addr":"(UB)","loc":"d,58:35,58:36","dtypep":"(VB)"} ], @@ -82,7 +82,7 @@ {"type":"BASICDTYPE","name":"logic","addr":"(J)","loc":"d,58:41,58:56","dtypep":"(J)","keyword":"logic","generic":true,"rangep": []}, {"type":"UNPACKARRAYDTYPE","name":"","addr":"(CB)","loc":"d,40:36,40:37","dtypep":"(CB)","isCompound":false,"declRange":"[5:0]","generic":false,"refDTypep":"(J)","childDTypep": [], "rangep": [ - {"type":"RANGE","name":"","addr":"(XB)","loc":"d,40:36,40:37","ascending":false, + {"type":"RANGE","name":"","addr":"(XB)","loc":"d,40:36,40:37","ascending":false,"fromBracket":false, "leftp": [ {"type":"CONST","name":"32'sh5","addr":"(YB)","loc":"d,40:37,40:38","dtypep":"(VB)"} ],