Add ENUMVALUE warning when value misused for enum (#726).

This commit is contained in:
Wilson Snyder 2022-11-12 20:11:05 -05:00
parent 4f50073feb
commit d25834e57b
7 changed files with 99 additions and 16 deletions

View File

@ -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]

View File

@ -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

View File

@ -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",

View File

@ -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

View File

@ -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

View File

@ -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);

View File

@ -21,6 +21,7 @@ module t;
struct_t str;
e = ONE;
e = $random() == 0 ? ONE : TWO;
e = e_t'(1);
e = e;