diff --git a/Changes b/Changes index 13e49197e..96b17667d 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,7 @@ Verilator 5.003 devel * Deprecate --no-threads; use --threads 1 for single threaded (#3703). [Kamil Rakoczy, Antmicro Ltd] * Support named properties (#3667). [Ryszard Rozak, Antmicro Ltd] * Support randcase. +* Add ENUMVALUE warning when value misused for enum (#726). * Internal AST improvements, also affect XML format (#3721). [Geza Lore] * Change ENDLABEL from warning into an error. * Fix return type of $countbits functions to int (#3725). [Ryszard Rozak, Antmicro Ltd] diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 3a5f625de..b91ed900a 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -572,6 +572,40 @@ List Of Warnings missing." +.. option:: ENUMVALUE + + Error that an enum data type value is being assigned from another data + type that is not implicitly assignment compatible with that enumerated + type. This error is required by IEEE, but it may be disabled. + + Faulty example: + + .. code-block:: sv + :linenos: + :emphasize-lines: 2 + + typedef enum { ZERO } e_t; + initial e_t en = 0; //<--- Warning + + The ideal repair is to use the enumeration value's mnemonic: + + .. code-block:: sv + :linenos: + :emphasize-lines: 2 + + typedef enum { ZERO } e_t; + initial e_t en = ZERO; //<--- Repaired + + Or, alternatively use a static cast: + + .. code-block:: sv + :linenos: + :emphasize-lines: 2 + + typedef enum { ZERO } e_t; + initial e_t en = e_t'(0); //<--- Repaired + + .. option:: EOFNEWLINE Warns that a file does not end in a newline. POSIX defines that a line diff --git a/src/V3Error.h b/src/V3Error.h index f22107cd8..47e222dd7 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -56,6 +56,7 @@ public: I_TIMING, // Enable timing from /*verilator timing_on/off*/ // Error codes: E_ENCAPSULATED, // Error: local/protected violation + E_ENUMVALUE, // Error: enum type needs explicit cast E_PORTSHORT, // Error: Output port is connected to a constant, electrical short E_UNSUPPORTED, // Error: Unsupported (generally) E_TASKNSVAR, // Error: Task I/O not simple @@ -169,7 +170,7 @@ public: // Boolean " I_CELLDEFINE", " I_COVERAGE", " I_TRACING", " I_LINT", " I_UNUSED", " I_DEF_NETTYPE_WIRE", " I_TIMING", // Errors - "ENCAPSULATED", "PORTSHORT", "UNSUPPORTED", "TASKNSVAR", "NEEDTIMINGOPT", "NOTIMING", + "ENCAPSULATED", "ENUMVALUE", "PORTSHORT", "UNSUPPORTED", "TASKNSVAR", "NEEDTIMINGOPT", "NOTIMING", // Warnings " EC_FIRST_WARN", "ALWCOMBORDER", "ASSIGNDLY", "ASSIGNIN", "BADSTDPRAGMA", diff --git a/src/V3Width.cpp b/src/V3Width.cpp index d0c2c9d03..1b8db1311 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -216,6 +216,7 @@ private: // STATE WidthVP* m_vup = nullptr; // Current node state const AstCell* m_cellp = nullptr; // Current cell for arrayed instantiations + const AstEnumItem* m_enumItemp = nullptr; // Current enum item const AstNodeFTask* m_ftaskp = nullptr; // Current function/task const AstNodeProcedure* m_procedurep = nullptr; // Current final/always const AstWith* m_withp = nullptr; // Current 'with' statement @@ -468,7 +469,10 @@ private: // the size of this subexpression only. // Second call (final()) m_vup->width() is probably the expression size, so // the expression includes the size of the output too. - if (nodep->thenp()->isDouble() || nodep->elsep()->isDouble()) { + if (nodep->thenp()->dtypep()->skipRefp() == nodep->elsep()->dtypep()->skipRefp()) { + // TODO might need a broader equation, use the Castable function? + nodep->dtypeFrom(nodep->thenp()->dtypep()); + } else if (nodep->thenp()->isDouble() || nodep->elsep()->isDouble()) { nodep->dtypeSetDouble(); } else if (nodep->thenp()->isString() || nodep->elsep()->isString()) { nodep->dtypeSetString(); @@ -1757,31 +1761,34 @@ private: const uint64_t maxval = enumMaxValue(nodep, enumDtp); const bool assoc = maxval > ENUM_LOOKUP_BITS; AstNode* testp = nullptr; + FileLine* const fl_novalue = new FileLine{fl}; + fl_novalue->warnOff(V3ErrorCode::E_ENUMVALUE, true); if (assoc) { AstVar* const varp = enumVarp(enumDtp, VAttrType::ENUM_VALID, true, 0); - testp = new AstAssocSel{fl, newVarRefDollarUnit(varp), + testp = new AstAssocSel{fl_novalue, newVarRefDollarUnit(varp), nodep->fromp()->cloneTree(false)}; } else { const int selwidth = V3Number::log2b(maxval) + 1; // Width to address a bit AstVar* const varp = enumVarp(enumDtp, VAttrType::ENUM_VALID, false, (1ULL << selwidth) - 1); - FileLine* const fl_nowarn = new FileLine{fl}; - fl_nowarn->warnOff(V3ErrorCode::WIDTH, true); + FileLine* const fl_nowidth = new FileLine{fl}; + fl_nowidth->warnOff(V3ErrorCode::WIDTH, true); testp = new AstCond{ fl, - new AstGt{fl_nowarn, nodep->fromp()->cloneTree(false), - new AstConst{fl_nowarn, AstConst::Unsized64{}, maxval}}, + new AstGt{fl_nowidth, nodep->fromp()->cloneTree(false), + new AstConst{fl_nowidth, AstConst::Unsized64{}, maxval}}, new AstConst{fl, AstConst::BitFalse{}}, new AstArraySel{ fl, newVarRefDollarUnit(varp), - new AstSel{fl, nodep->fromp()->cloneTree(false), 0, selwidth}}}; + new AstSel{fl_novalue, nodep->fromp()->cloneTree(false), 0, selwidth}}}; } - newp = new AstCond{fl, testp, - new AstExprStmt{fl, - new AstAssign{fl, nodep->top()->unlinkFrBack(), - nodep->fromp()->unlinkFrBack()}, - new AstConst{fl, AstConst::Signed32{}, 1}}, - new AstConst{fl, AstConst::Signed32{}, 0}}; + newp = new AstCond{ + fl, testp, + new AstExprStmt{fl, + new AstAssign{fl_novalue, nodep->top()->unlinkFrBack(), + nodep->fromp()->unlinkFrBack()}, + new AstConst{fl, AstConst::Signed32{}, 1}}, + new AstConst{fl, AstConst::Signed32{}, 0}}; } else if (castable == COMPATIBLE) { nodep->v3warn(CASTCONST, "$cast will always return one as " << toDtp->prettyDTypeNameQ() @@ -1893,7 +1900,8 @@ private: } else if (!basicp->isSigned() && nodep->fromp()->isSigned()) { newp = new AstUnsigned(nodep->fileline(), nodep->fromp()->unlinkFrBack()); } else { - // Can just remove cast + // Can just remove cast, but need extend placeholder + // so we can avoid warning message } } else if (VN_IS(toDtp, ClassRefDType)) { // Can just remove cast @@ -2192,6 +2200,8 @@ private: } void visit(AstEnumItem* nodep) override { UINFO(5, " ENUMITEM " << nodep << endl); + VL_RESTORER(m_enumItemp); + m_enumItemp = nodep; AstNodeDType* const vdtypep = m_vup->dtypep(); UASSERT_OBJ(vdtypep, nodep, "ENUMITEM not under ENUM"); nodep->dtypep(vdtypep); @@ -2752,6 +2762,7 @@ private: 0); // Spec doesn't say what to do } else { newp = VN_AS(itemp->valuep()->cloneTree(false), Const); // A const + newp->dtypeFrom(adtypep); // To prevent a later E_ENUMVALUE } } else if (nodep->name() == "last") { const AstEnumItem* itemp = adtypep->itemsp(); @@ -2761,6 +2772,7 @@ private: 0); // Spec doesn't say what to do } else { newp = VN_AS(itemp->valuep()->cloneTree(false), Const); // A const + newp->dtypeFrom(adtypep); // To prevent a later E_ENUMVALUE } } UASSERT_OBJ(newp, nodep, "Enum method (perhaps enum item) not const"); @@ -2808,6 +2820,7 @@ private: AstVar* const varp = enumVarp(adtypep, attrType, true, 0); AstNode* const newp = new AstAssocSel{nodep->fileline(), newVarRefDollarUnit(varp), nodep->fromp()->unlinkFrBack()}; + newp->dtypeFrom(adtypep); // To prevent a later E_ENUMVALUE nodep->replaceWith(newp); } else { const int selwidth = V3Number::log2b(msbdim) + 1; // Width to address a bit @@ -2818,6 +2831,7 @@ private: // We return "random" values if outside the range, which is fine // as next/previous on illegal values just need something good out new AstSel(nodep->fileline(), nodep->fromp()->unlinkFrBack(), 0, selwidth)); + newp->dtypeFrom(adtypep); // To prevent a later E_ENUMVALUE nodep->replaceWith(newp); } VL_DO_DANGLING(nodep->deleteTree(), nodep); @@ -6101,6 +6115,20 @@ private: const AstBasicDType* const expBasicp = expDTypep->basicp(); const AstBasicDType* const underBasicp = underp->dtypep()->basicp(); if (expBasicp && underBasicp) { + if (const AstEnumDType* const expEnump + = VN_CAST(expDTypep->skipRefToEnump(), EnumDType)) { + const auto castable = computeCastable(expEnump, underp->dtypep(), underp); + if (castable != COMPATIBLE && castable != ENUM_IMPLICIT && !VN_IS(underp, Cast) + && !VN_IS(underp, CastDynamic) && !m_enumItemp && warnOn) { + nodep->v3warn(E_ENUMVALUE, + "Illegal implicit conversion to enum " + << expDTypep->prettyDTypeNameQ() << " from " + << underp->dtypep()->prettyDTypeNameQ() + << " (IEEE 1800-2017 6.19.3)\n" + << nodep->warnMore() + << "... Suggest use enum's mnemonic, or static cast"); + } + } AstNodeDType* subDTypep = expDTypep; // We then iterate FINAL before width fixes, as if the under-operation // is e.g. an ADD, the ADD will auto-adjust to the proper data type diff --git a/test_regress/t/t_enum_type_bad.out b/test_regress/t/t_enum_type_bad.out new file mode 100644 index 000000000..26fe75f83 --- /dev/null +++ b/test_regress/t/t_enum_type_bad.out @@ -0,0 +1,17 @@ +%Error-ENUMVALUE: t/t_enum_type_bad.v:28:9: Illegal implicit conversion to enum 't.e_t' from 'logic[31:0]' (IEEE 1800-2017 6.19.3) + : ... In instance t + : ... Suggest use enum's mnemonic, or static cast + 28 | e = 1; + | ^ + ... For error description see https://verilator.org/warn/ENUMVALUE?v=latest +%Error-ENUMVALUE: t/t_enum_type_bad.v:29:9: Illegal implicit conversion to enum 't.o_t' from 't.e_t' (IEEE 1800-2017 6.19.3) + : ... In instance t + : ... Suggest use enum's mnemonic, or static cast + 29 | o = e; + | ^ +%Error-ENUMVALUE: t/t_enum_type_bad.v:35:9: Illegal implicit conversion to enum 't.o_t' from 'ENUMDTYPE 't.e_t'' (IEEE 1800-2017 6.19.3) + : ... In instance t + : ... Suggest use enum's mnemonic, or static cast + 35 | o = str.m_e; + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_enum_type_bad.pl b/test_regress/t/t_enum_type_bad.pl index aa2cd3195..a083f46f5 100755 --- a/test_regress/t/t_enum_type_bad.pl +++ b/test_regress/t/t_enum_type_bad.pl @@ -11,7 +11,8 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(vlt => 1); lint( - fails => 0, # But should fail + fails => 1, + expect_filename => $Self->{golden_filename}, ); ok(1); diff --git a/test_regress/t/t_enum_type_bad.v b/test_regress/t/t_enum_type_bad.v index 038646ca6..d46fb9d67 100644 --- a/test_regress/t/t_enum_type_bad.v +++ b/test_regress/t/t_enum_type_bad.v @@ -21,6 +21,7 @@ module t; struct_t str; e = ONE; + e = $random() == 0 ? ONE : TWO; e = e_t'(1); e = e;