diff --git a/src/V3Ast.h b/src/V3Ast.h index 0e6508036..b925d7f26 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1702,7 +1702,7 @@ public: const char* typeName() const VL_MT_SAFE { return type().ascii(); } // See also prettyTypeName AstNode* nextp() const VL_MT_STABLE { return m_nextp; } AstNode* backp() const VL_MT_STABLE { return m_backp; } - AstNode* abovep() const; // Parent node above, only when no nextp() as otherwise slow + AstNode* abovep() const; // Get parent node above, only for list head and tail AstNode* op1p() const VL_MT_STABLE { return m_op1p; } AstNode* op2p() const VL_MT_STABLE { return m_op2p; } AstNode* op3p() const VL_MT_STABLE { return m_op3p; } diff --git a/src/V3Case.cpp b/src/V3Case.cpp index c8cd473c2..8297c1b69 100644 --- a/src/V3Case.cpp +++ b/src/V3Case.cpp @@ -139,32 +139,42 @@ private: std::array m_valueItem; // METHODS - bool caseIsEnumComplete(AstCase* nodep, uint32_t numCases) { - // Return true if case is across an enum, and every value in the case - // statement corresponds to one of the enum values - if (!nodep->uniquePragma() && !nodep->unique0Pragma()) return false; - AstEnumDType* const enumDtp + //! Determine whether we should check case items are complete + //! @return Enum's dtype if should check, nullptr if shouldn't + const AstEnumDType* getEnumCompletionCheckDType(const AstCase* const nodep) { + if (!nodep->uniquePragma() && !nodep->unique0Pragma()) return nullptr; + const AstEnumDType* const enumDtp = VN_CAST(nodep->exprp()->dtypep()->skipRefToEnump(), EnumDType); - if (!enumDtp) return false; // Case isn't enum - AstBasicDType* const basicp = enumDtp->subDTypep()->basicp(); - if (!basicp) return false; // Not simple type (perhaps IEEE illegal) - if (basicp->width() > 32) return false; - // Find all case values into a set - std::set caseSet; - for (uint32_t i = 0; i < numCases; ++i) { // All case items - if (m_valueItem[i]) caseSet.emplace(i); - } - // Find all enum values into a set - std::set enumSet; - for (AstEnumItem* itemp = enumDtp->itemsp(); itemp; + if (!enumDtp) return nullptr; // Case isn't enum + const AstBasicDType* const basicp = enumDtp->subDTypep()->basicp(); + if (!basicp) return nullptr; // Not simple type (perhaps IEEE illegal) + if (basicp->width() > 32) return nullptr; + return enumDtp; + } + //! @return True if case items are complete, false if there are uncovered enums + bool checkCaseEnumComplete(const AstCase* const nodep, const AstEnumDType* const dtype) { + const uint32_t numCases = 1UL << m_caseWidth; + for (AstEnumItem* itemp = dtype->itemsp(); itemp; itemp = VN_AS(itemp->nextp(), EnumItem)) { AstConst* const econstp = VN_AS(itemp->valuep(), Const); - const uint32_t val = econstp->toUInt(); - // UINFO(9, "Complete enum item " << val << ": " << itemp << endl); - enumSet.emplace(val); + V3Number nummask{itemp, econstp->width()}; + nummask.opBitsNonX(econstp->num()); + const uint32_t mask = nummask.toUInt(); + V3Number numval{itemp, econstp->width()}; + numval.opBitsOne(econstp->num()); + const uint32_t val = numval.toUInt(); + + for (uint32_t i = 0; i < numCases; ++i) { + if ((i & mask) == val) { + if (!m_valueItem[i]) { + nodep->v3warn(CASEINCOMPLETE, "Enum item " << itemp->prettyNameQ() + << " not covered by case\n"); + return false; // enum has uncovered value by case items + } + } + } } - // If sets match, all covered - return (caseSet == enumSet); + return true; // enum is fully covered } bool isCaseTreeFast(AstCase* nodep) { int width = 0; @@ -193,6 +203,8 @@ private: // We can cheat and use uint32_t's because we only support narrow case's bool reportedOverlap = false; bool reportedSubcase = false; + bool hasDefaultCase = false; + std::map caseItemMap; // case condition -> case item for (AstCaseItem* itemp = nodep->itemsp(); itemp; itemp = VN_AS(itemp->nextp(), CaseItem)) { for (AstNode* icondp = itemp->condsp(); icondp; icondp = icondp->nextp()) { @@ -202,6 +214,7 @@ private: if (neverItem(nodep, iconstp)) { // X in casez can't ever be executed } else { + const bool isCondWildcard = iconstp->num().isAnyXZ(); V3Number nummask{itemp, iconstp->width()}; nummask.opBitsNonX(iconstp->num()); const uint32_t mask = nummask.toUInt(); @@ -210,16 +223,17 @@ private: const uint32_t val = numval.toUInt(); uint32_t firstOverlap = 0; - bool foundOverlap = false; + AstNode* overlappedCondp = nullptr; bool foundHit = false; for (uint32_t i = 0; i < numCases; ++i) { if ((i & mask) == val) { if (!m_valueItem[i]) { - m_valueItem[i] = itemp; + m_valueItem[i] = icondp; + caseItemMap[icondp] = itemp; foundHit = true; - } else if (!foundOverlap) { + } else if (!overlappedCondp) { firstOverlap = i; - foundOverlap = true; + overlappedCondp = m_valueItem[i]; m_caseNoOverlapsAllCovered = false; } } @@ -227,9 +241,19 @@ private: if (!nodep->priorityPragma()) { // If this case statement doesn't have the priority // keyword, we want to warn on any overlap. - if (!reportedOverlap && foundOverlap) { - icondp->v3warn(CASEOVERLAP, "Case values overlap (example pattern 0x" - << std::hex << firstOverlap << ")"); + if (!reportedOverlap && overlappedCondp) { + std::ostringstream examplePattern; + if (isCondWildcard) { + examplePattern << " (example pattern 0x" << std::hex + << firstOverlap << ")"; + } + icondp->v3warn(CASEOVERLAP, + "Case conditions overlap" + << examplePattern.str() << "\n" + << icondp->warnContextPrimary() << '\n' + << overlappedCondp->warnOther() + << "... Location of overlapping condition\n" + << overlappedCondp->warnContextSecondary()); reportedOverlap = true; } } else { @@ -240,7 +264,11 @@ private: if (!reportedSubcase && !foundHit) { icondp->v3warn(CASEOVERLAP, "Case item ignored: every matching value is covered " - "by an earlier item"); + "by an earlier condition\n" + << icondp->warnContextPrimary() << '\n' + << overlappedCondp->warnOther() + << "... Location of previous condition\n" + << overlappedCondp->warnContextPrimary()); reportedSubcase = true; } } @@ -251,17 +279,28 @@ private: for (uint32_t i = 0; i < numCases; ++i) { if (!m_valueItem[i]) m_valueItem[i] = itemp; } + caseItemMap[itemp] = itemp; + hasDefaultCase = true; } } - if (!caseIsEnumComplete(nodep, numCases)) { - for (uint32_t i = 0; i < numCases; ++i) { - if (!m_valueItem[i]) { - nodep->v3warn(CASEINCOMPLETE, "Case values incompletely covered " - "(example pattern 0x" - << std::hex << i << ")"); + if (!hasDefaultCase) { + const AstEnumDType* const dtype = getEnumCompletionCheckDType(nodep); + if (dtype) { + if (!checkCaseEnumComplete(nodep, dtype)) { + // checkCaseEnumComplete has already warned of incompletion m_caseNoOverlapsAllCovered = false; return false; } + } else { + for (uint32_t i = 0; i < numCases; ++i) { + if (!m_valueItem[i]) { // has uncovered case + nodep->v3warn(CASEINCOMPLETE, "Case values incompletely covered " + "(example pattern 0x" + << std::hex << i << ")"); + m_caseNoOverlapsAllCovered = false; + return false; + } + } } } @@ -274,8 +313,10 @@ private: // Convert valueItem from AstCaseItem* to the expression // Not done earlier, as we may now have a nullptr because it's just a ";" NOP branch for (uint32_t i = 0; i < numCases; ++i) { - if (AstCaseItem* const itemp = VN_AS(m_valueItem[i], CaseItem)) { - m_valueItem[i] = itemp->stmtsp(); + if (AstNode* const condp = m_valueItem[i]) { + AstCaseItem* caseItemp = caseItemMap[condp]; + UASSERT(caseItemp, "caseItemp should exist"); + m_valueItem[i] = caseItemp->stmtsp(); } } return true; // All is fine @@ -543,10 +584,12 @@ private: } } //-------------------- - void visit(AstNode* nodep) override { - if (VN_IS(nodep, Always)) m_alwaysp = nodep; + void visit(AstAlways* nodep) override { + VL_RESTORER(m_alwaysp) + m_alwaysp = nodep; iterateChildren(nodep); } + void visit(AstNode* nodep) override { iterateChildren(nodep); } public: // CONSTRUCTORS diff --git a/src/verilog.y b/src/verilog.y index 16dc3c644..34410ffdc 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3527,7 +3527,7 @@ statement_item: // IEEE: statement_item { $$ = nullptr; BBUNSUP($4, "Unsupported: matches (for tagged union)"); } | unique_priorityE caseStart caseAttrE yINSIDE case_insideListE yENDCASE { $$ = $2; if ($5) $2->addItemsp($5); - if (!$2->caseSimple()) $2->v3error("Illegal to have inside on a casex/casez"); + if (!$2->caseSimple()) $4->v3error("Illegal to have inside on a casex/casez"); $2->caseInsideSet(); if ($1 == uniq_UNIQUE) $2->uniquePragma(true); if ($1 == uniq_UNIQUE0) $2->unique0Pragma(true); diff --git a/test_regress/t/t_case_enum_complete.v b/test_regress/t/t_case_enum_complete.v index 7ee354cfd..cd0d02f92 100644 --- a/test_regress/t/t_case_enum_complete.v +++ b/test_regress/t/t_case_enum_complete.v @@ -8,13 +8,18 @@ module t; enum logic [2:0] {S0, S1, S2} state; + int v = 0; + initial begin state = S1; unique case (state) - S0: $stop; - S1: $finish; + S0, S2: $stop; + S1: v++; + endcase + unique case (state) S2: $stop; + default: v++; endcase end endmodule diff --git a/test_regress/t/t_case_enum_complete_wildcard.pl b/test_regress/t/t_case_enum_complete_wildcard.pl new file mode 100755 index 000000000..59837c4db --- /dev/null +++ b/test_regress/t/t_case_enum_complete_wildcard.pl @@ -0,0 +1,18 @@ +#!/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 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( + verilator_flags2 => ["--lint-only -Wwarn-CASEINCOMPLETE"], + ); + +ok(1); +1; diff --git a/test_regress/t/t_case_enum_complete_wildcard.v b/test_regress/t/t_case_enum_complete_wildcard.v new file mode 100644 index 000000000..0beb75014 --- /dev/null +++ b/test_regress/t/t_case_enum_complete_wildcard.v @@ -0,0 +1,74 @@ +// DESCRIPTION: Verilator: SystemVerilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Anthony Donlon. +// SPDX-License-Identifier: CC0-1.0 + +// Fix bug4464 + +module t; + + enum logic [1:0] { + S00 = 'b00, + S01 = 'b01, + S10 = 'b10, + + S0X = 2'b0?, + SX0 = 2'b?0 + } state; + + int v = 0; + + initial begin + state = S01; + unique case (state) + S00: $stop; + S01: v++; + S10: $stop; + endcase + unique case (state) + S00: $stop; + default: v++; // default + endcase + unique case (state) + 2'd0: $stop; + 2'd1: v++; + 2'd2: $stop; + endcase + unique case (state) + 2'd0: $stop; + 2'd1: v++; + 2'd2: $stop; + 2'd3: $stop; // extra case + endcase + + unique case (state) inside + 2'd0: $stop; + 2'd1: v++; + [2'd2:2'd3]: $stop; + endcase + unique case (state) inside + [S00:S10]: v++; + endcase + + unique casez (state) + S10: $stop; + S0X: v++; // fully covered + endcase + unique casez (state) + S10: $stop; + S0X: v++; + 2'b11: $stop; // extra case + endcase + unique casez (state) + S0X: v++; + default: $stop; + endcase + + case (state) + S00: $stop; + S01: v++; + S10, 2'b11: $stop; + endcase + end +endmodule diff --git a/test_regress/t/t_case_enum_incomplete_bad.out b/test_regress/t/t_case_enum_incomplete_bad.out index 4a88a83dc..f7713d73b 100644 --- a/test_regress/t/t_case_enum_incomplete_bad.out +++ b/test_regress/t/t_case_enum_incomplete_bad.out @@ -1,4 +1,4 @@ -%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_bad.v:14:14: Case values incompletely covered (example pattern 0x1) +%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_bad.v:14:14: Enum item 'S1' not covered by case 14 | unique case (state) | ^~~~ ... For warning description see https://verilator.org/warn/CASEINCOMPLETE?v=latest diff --git a/test_regress/t/t_case_enum_incomplete_wildcard_bad.out b/test_regress/t/t_case_enum_incomplete_wildcard_bad.out new file mode 100644 index 000000000..18337e745 --- /dev/null +++ b/test_regress/t/t_case_enum_incomplete_wildcard_bad.out @@ -0,0 +1,15 @@ +%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_wildcard_bad.v:26:16: Enum item 'S10' not covered by case + 26 | unique case (state) + | ^~~~ + ... For warning description see https://verilator.org/warn/CASEINCOMPLETE?v=latest + ... Use "/* verilator lint_off CASEINCOMPLETE */" and lint_on around source to disable this message. +%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_wildcard_bad.v:30:16: Enum item 'S00' not covered by case + 30 | unique case (state) + | ^~~~ +%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_wildcard_bad.v:35:16: Enum item 'S10' not covered by case + 35 | unique casez (state) + | ^~~~~ +%Warning-CASEINCOMPLETE: t/t_case_enum_incomplete_wildcard_bad.v:40:9: Case values incompletely covered (example pattern 0x3) + 40 | case (state) + | ^~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_case_enum_incomplete_wildcard_bad.pl b/test_regress/t/t_case_enum_incomplete_wildcard_bad.pl new file mode 100755 index 000000000..1d5773686 --- /dev/null +++ b/test_regress/t/t_case_enum_incomplete_wildcard_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 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( + verilator_flags2 => ['--assert'], + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_case_enum_incomplete_wildcard_bad.v b/test_regress/t/t_case_enum_incomplete_wildcard_bad.v new file mode 100644 index 000000000..e0e6965ba --- /dev/null +++ b/test_regress/t/t_case_enum_incomplete_wildcard_bad.v @@ -0,0 +1,46 @@ +// DESCRIPTION: Verilator: SystemVerilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Anthony Donlon. +// SPDX-License-Identifier: CC0-1.0 + +module t; + t1 i_t1(); +endmodule + +module t1; + + enum logic [1:0] { + S00 = 'b00, + S01 = 'b01, + S10 = 'b10, + + SX0 = 2'b?0, + S0X = 'b0? + } state; + + int v = 0; + + initial begin + state = S10; + unique case (state) + S00: $stop; + 2'b01: $stop; + endcase + unique case (state) + 2'd2: v++; + 2'd1: $stop; + endcase + + unique casez (state) + S0X: $stop; + 2'b11: $stop; + endcase + + case (state) + S00: $stop; + S01: $stop; + S10: v++; + endcase + end +endmodule diff --git a/test_regress/t/t_case_inside_bad.out b/test_regress/t/t_case_inside_bad.out index b683c04e3..2be995ce2 100644 --- a/test_regress/t/t_case_inside_bad.out +++ b/test_regress/t/t_case_inside_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_case_inside_bad.v:9:7: Illegal to have inside on a casex/casez +%Error: t/t_case_inside_bad.v:9:20: Illegal to have inside on a casex/casez 9 | casex (1'bx) inside - | ^~~~~ + | ^~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_case_overlap_bad.out b/test_regress/t/t_case_overlap_bad.out new file mode 100644 index 000000000..3362705db --- /dev/null +++ b/test_regress/t/t_case_overlap_bad.out @@ -0,0 +1,33 @@ +%Warning-CASEOVERLAP: t/t_case_overlap_bad.v:20:21: Case conditions overlap (example pattern 0x6) + 20 | 3'b11?, 3'b???: v++; + | ^~~~~~ + t/t_case_overlap_bad.v:20:13: ... Location of overlapping condition + 20 | 3'b11?, 3'b???: v++; + | ^~~~~~ + ... For warning description see https://verilator.org/warn/CASEOVERLAP?v=latest + ... Use "/* verilator lint_off CASEOVERLAP */" and lint_on around source to disable this message. +%Warning-CASEOVERLAP: t/t_case_overlap_bad.v:25:13: Case conditions overlap + 25 | 3'b001, 3'b000: $stop; + | ^~~~~~ + t/t_case_overlap_bad.v:24:13: ... Location of overlapping condition + 24 | 3'b00?: $stop; + | ^~~~~~ +%Warning-CASEOVERLAP: t/t_case_overlap_bad.v:30:13: Case conditions overlap (example pattern 0x7) + 30 | 3'b11?: $stop; + | ^~~~~~ + t/t_case_overlap_bad.v:29:13: ... Location of overlapping condition + 29 | 3'b111, 3'b0??: v++; + | ^~~~~~ +%Warning-CASEOVERLAP: t/t_case_overlap_bad.v:35:13: Case conditions overlap + 35 | 3'b001: $stop; + | ^~~~~~ + t/t_case_overlap_bad.v:34:21: ... Location of overlapping condition + 34 | 3'b000, 3'b001, 3'b010, 3'b011: v++; + | ^~~~~~ +%Warning-CASEOVERLAP: t/t_case_overlap_bad.v:40:13: Case conditions overlap + 40 | 3'b011: $stop; + | ^~~~~~ + t/t_case_overlap_bad.v:39:37: ... Location of overlapping condition + 39 | 3'b000, 3'b001, 3'b010, 3'b011: v++; + | ^~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_case_overlap_bad.pl b/test_regress/t/t_case_overlap_bad.pl new file mode 100755 index 000000000..a60503a1f --- /dev/null +++ b/test_regress/t/t_case_overlap_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 2003 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_case_overlap_bad.v b/test_regress/t/t_case_overlap_bad.v new file mode 100644 index 000000000..39cf8d4c6 --- /dev/null +++ b/test_regress/t/t_case_overlap_bad.v @@ -0,0 +1,44 @@ +// DESCRIPTION: Verilator: SystemVerilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Anthony Donlon. +// SPDX-License-Identifier: CC0-1.0 + +module t; + t1 i_t1(); +endmodule + +module t1; + + int v = 0; + + logic [2:0] state; + + initial begin + state = 2; + casez (state) + 3'b11?, 3'b???: v++; + default; + endcase + casez (state) + 3'b00?: $stop; + 3'b001, 3'b000: $stop; + default; + endcase + casez (state) + 3'b111, 3'b0??: v++; + 3'b11?: $stop; + default; + endcase + casez (state) + 3'b000, 3'b001, 3'b010, 3'b011: v++; + 3'b001: $stop; + default; + endcase + casez (state) + 3'b000, 3'b001, 3'b010, 3'b011: v++; + 3'b011: $stop; + default; + endcase + end +endmodule diff --git a/test_regress/t/t_param_scope_bad.out b/test_regress/t/t_param_scope_bad.out index eddd36bd1..babd1fe25 100644 --- a/test_regress/t/t_param_scope_bad.out +++ b/test_regress/t/t_param_scope_bad.out @@ -1,6 +1,9 @@ -%Warning-CASEOVERLAP: t/t_param_scope_bad.v:28:9: Case values overlap (example pattern 0x2) +%Warning-CASEOVERLAP: t/t_param_scope_bad.v:28:9: Case conditions overlap 28 | 2'h2: $stop; | ^~~~ + t/t_param_scope_bad.v:27:9: ... Location of overlapping condition + 27 | CASEVAL: ; + | ^~~~~~~ ... For warning description see https://verilator.org/warn/CASEOVERLAP?v=latest ... Use "/* verilator lint_off CASEOVERLAP */" and lint_on around source to disable this message. %Error: Exiting due to diff --git a/test_regress/t/t_priority_case.out b/test_regress/t/t_priority_case.out index 468773b6c..6a725e733 100644 --- a/test_regress/t/t_priority_case.out +++ b/test_regress/t/t_priority_case.out @@ -1,9 +1,15 @@ -%Warning-CASEOVERLAP: t/t_priority_case.v:34:7: Case item ignored: every matching value is covered by an earlier item +%Warning-CASEOVERLAP: t/t_priority_case.v:34:7: Case item ignored: every matching value is covered by an earlier condition 34 | 2'b ?1: out1 = 3'd1; + | ^~~~~~ + t/t_priority_case.v:33:7: ... Location of previous condition + 33 | 2'b ?1: out1 = 3'd0; | ^~~~~~ ... For warning description see https://verilator.org/warn/CASEOVERLAP?v=latest ... Use "/* verilator lint_off CASEOVERLAP */" and lint_on around source to disable this message. -%Warning-CASEOVERLAP: t/t_priority_case.v:44:7: Case item ignored: every matching value is covered by an earlier item +%Warning-CASEOVERLAP: t/t_priority_case.v:44:7: Case item ignored: every matching value is covered by an earlier condition 44 | 2'b ?1: out1 = 3'd1; + | ^~~~~~ + t/t_priority_case.v:43:7: ... Location of previous condition + 43 | 2'b ?1: out1 = 3'd0; | ^~~~~~ %Error: Exiting due to