Fix lint of case statements with enum and wildcard bits (#4464) (#4487)

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; }

src/V3Case.cpp

@@ -139,32 +139,42 @@ private:
     std::array<AstNode*, 1 << CASE_OVERLAP_WIDTH> m_valueItem;
 
     // METHODS
-    bool caseIsEnumComplete(AstCase* nodep, uint32_t numCases) {
+    //! Determine whether we should check case items are complete
-        // Return true if case is across an enum, and every value in the case
+    //! @return  Enum's dtype if should check, nullptr if shouldn't
-        // statement corresponds to one of the enum values
+    const AstEnumDType* getEnumCompletionCheckDType(const AstCase* const nodep) {
-        if (!nodep->uniquePragma() && !nodep->unique0Pragma()) return false;
+        if (!nodep->uniquePragma() && !nodep->unique0Pragma()) return nullptr;
-        AstEnumDType* const enumDtp
+        const AstEnumDType* const enumDtp
             = VN_CAST(nodep->exprp()->dtypep()->skipRefToEnump(), EnumDType);
-        if (!enumDtp) return false;  // Case isn't enum
+        if (!enumDtp) return nullptr;  // Case isn't enum
-        AstBasicDType* const basicp = enumDtp->subDTypep()->basicp();
+        const AstBasicDType* const basicp = enumDtp->subDTypep()->basicp();
-        if (!basicp) return false;  // Not simple type (perhaps IEEE illegal)
+        if (!basicp) return nullptr;  // Not simple type (perhaps IEEE illegal)
-        if (basicp->width() > 32) return false;
+        if (basicp->width() > 32) return nullptr;
-        // Find all case values into a set
+        return enumDtp;
-        std::set<uint32_t> 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
+    //! @return  True if case items are complete, false if there are uncovered enums
-        std::set<uint32_t> enumSet;
+    bool checkCaseEnumComplete(const AstCase* const nodep, const AstEnumDType* const dtype) {
-        for (AstEnumItem* itemp = enumDtp->itemsp(); itemp;
+        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();
+            V3Number nummask{itemp, econstp->width()};
-            // UINFO(9, "Complete enum item " << val << ": " << itemp << endl);
+            nummask.opBitsNonX(econstp->num());
-            enumSet.emplace(val);
+            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<AstNode*, AstCaseItem*> 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) {
+                        if (!reportedOverlap && overlappedCondp) {
-                            icondp->v3warn(CASEOVERLAP, "Case values overlap (example pattern 0x"
+                            std::ostringstream examplePattern;
-                                                            << std::hex << firstOverlap << ")");
+                            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,11 +279,21 @@ 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)) {
+        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]) {
+                    if (!m_valueItem[i]) {  // has uncovered case
                         nodep->v3warn(CASEINCOMPLETE, "Case values incompletely covered "
                                                       "(example pattern 0x"
                                                           << std::hex << i << ")");
@@ -264,6 +302,7 @@ private:
                     }
                 }
             }
+        }
 
         if (m_caseItems <= 3
             // Avoid e.g. priority expanders from going crazy in expansion
@@ -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)) {
+            if (AstNode* const condp = m_valueItem[i]) {
-                m_valueItem[i] = itemp->stmtsp();
+                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 {
+    void visit(AstAlways* nodep) override {
-        if (VN_IS(nodep, Always)) m_alwaysp = nodep;
+        VL_RESTORER(m_alwaysp)
+        m_alwaysp = nodep;
         iterateChildren(nodep);
     }
+    void visit(AstNode* nodep) override { iterateChildren(nodep); }
 
 public:
     // CONSTRUCTORS

src/verilog.y

@@ -3527,7 +3527,7 @@ statement_item<nodep>:          // 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);

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;
+        S0, S2: $stop;
-        S1: $finish;
+        S1: v++;
+      endcase
+      unique case (state)
         S2: $stop;
+        default: v++;
       endcase
    end
 endmodule

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;

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

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

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

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;

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

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

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

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;

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

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

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