From 77cc93c1763b5fed763d4a3a6d7d2890c90b3af1 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 15 Mar 2025 21:57:10 -0400 Subject: [PATCH] Fix detecting bad datatype on $countones (#5674). --- Changes | 1 + src/V3Width.cpp | 37 +++++++++++++++++++----- test_regress/t/t_math_countbits2_bad.out | 17 +++++++++++ test_regress/t/t_math_countbits2_bad.py | 16 ++++++++++ test_regress/t/t_math_countbits2_bad.v | 23 +++++++++++++++ 5 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 test_regress/t/t_math_countbits2_bad.out create mode 100755 test_regress/t/t_math_countbits2_bad.py create mode 100644 test_regress/t/t_math_countbits2_bad.v diff --git a/Changes b/Changes index 80261cb9c..c6f01da2e 100644 --- a/Changes +++ b/Changes @@ -25,6 +25,7 @@ Verilator 5.035 devel * Add `--make json` to enable integration with non-make/cmake build systems (#5799). [Andrew Voznytsa] * Add empty veriuser.h for legacy compatibility. * Optimize automatic splitting of some packed variables (#5843). [Geza Lore] +* Fix detecting bad datatype on $countones (#5674). [Shou-Li Hsu] * Fix foreach of associative array inside a constraint block (#5727) (#5841). [Yilou Wang] * Fix reset of automatic function variables (#5747). [Augustin Fabre] * Fix invalid code motion over branches (#5811) (#5814). [Geza Lore] diff --git a/src/V3Width.cpp b/src/V3Width.cpp index bbc943afc..7fe325b49 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -623,8 +623,8 @@ class WidthVisitor final : public VNVisitor { iterateCheckString(nodep, "RHS", nodep->rhsp(), BOTH); nodep->dtypeSetString(); } else { - iterateCheckSizedSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); - iterateCheckSizedSelf(nodep, "RHS", nodep->rhsp(), SELF, BOTH); + iterateCheckSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); + iterateCheckSelf(nodep, "RHS", nodep->rhsp(), SELF, BOTH); if (m_streamConcat) { packIfUnpacked(nodep->lhsp()); @@ -831,7 +831,7 @@ class WidthVisitor final : public VNVisitor { if (vdtypep && vdtypep->isString()) { iterateCheckString(nodep, "LHS", nodep->srcp(), BOTH); } else { - iterateCheckSizedSelf(nodep, "LHS", nodep->srcp(), SELF, BOTH); + iterateCheckSelf(nodep, "LHS", nodep->srcp(), SELF, BOTH); } if ((vdtypep && vdtypep->isString()) || nodep->srcp()->isString()) { @@ -898,9 +898,9 @@ class WidthVisitor final : public VNVisitor { VL_RESTORER(m_streamConcat); if (m_vup->prelim()) { m_streamConcat = true; - iterateCheckSizedSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); + iterateCheckSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); m_streamConcat = false; - iterateCheckSizedSelf(nodep, "RHS", nodep->rhsp(), SELF, BOTH); + iterateCheckSelf(nodep, "RHS", nodep->rhsp(), SELF, BOTH); V3Const::constifyParamsEdit(nodep->rhsp()); // rhsp may change if (const AstConst* const constp = VN_CAST(nodep->rhsp(), Const)) { if (constp->toUInt() == 0) nodep->v3error("Slice size cannot be zero."); @@ -3390,7 +3390,7 @@ class WidthVisitor final : public VNVisitor { } else if (nodep->name() == "exists") { // function int exists(input index) // IEEE really should have made this a "bit" return methodOkArguments(nodep, 1, 1); - iterateCheckSizedSelf(nodep, "argument", methodArg(nodep, 0), SELF, BOTH); + iterateCheckSelf(nodep, "argument", methodArg(nodep, 0), SELF, BOTH); AstNodeExpr* const index_exprp = methodCallWildcardIndexExpr(nodep, adtypep); newp = new AstCMethodHard{nodep->fileline(), nodep->fromp()->unlinkFrBack(), "exists", index_exprp->unlinkFrBack()}; @@ -3403,7 +3403,7 @@ class WidthVisitor final : public VNVisitor { "clear"}; newp->dtypeSetVoid(); } else { - iterateCheckSizedSelf(nodep, "argument", methodArg(nodep, 0), SELF, BOTH); + iterateCheckSelf(nodep, "argument", methodArg(nodep, 0), SELF, BOTH); AstNodeExpr* const index_exprp = methodCallWildcardIndexExpr(nodep, adtypep); newp = new AstCMethodHard{nodep->fileline(), nodep->fromp()->unlinkFrBack(), "erase", index_exprp->unlinkFrBack()}; @@ -5879,7 +5879,7 @@ class WidthVisitor final : public VNVisitor { if (VN_IS(propStmtp, Var)) { userIterate(propStmtp, nullptr); } else if (VN_IS(propStmtp, PropSpec)) { - iterateCheckSizedSelf(nodep, "PropSpec", propStmtp, SELF, BOTH); + iterateCheckSelf(nodep, "PropSpec", propStmtp, SELF, BOTH); } else { propStmtp->v3fatal("Invalid statement under AstProperty"); } @@ -7067,6 +7067,22 @@ class WidthVisitor final : public VNVisitor { } (void)underp; // cppcheck } + void iterateCheckSelf(AstNode* nodep, const char* side, AstNode* underp, Determ determ, + Stage stage) { + // Coerce child to any data type; child is self-determined + // i.e. isolated from expected type. + // e.g. nodep=CONCAT, underp=lhs in CONCAT(lhs,rhs) + UASSERT_OBJ(determ == SELF, nodep, "Bad call"); + UASSERT_OBJ(stage == FINAL || stage == BOTH, nodep, "Bad call"); + // underp may change as a result of replacement + if (stage & PRELIM) { + underp = userIterateSubtreeReturnEdits(underp, WidthVP{SELF, PRELIM}.p()); + } + underp = VN_IS(underp, NodeExpr) ? checkCvtUS(VN_AS(underp, NodeExpr)) : underp; + AstNodeDType* const expDTypep = underp->dtypep(); + underp = iterateCheck(nodep, side, underp, SELF, FINAL, expDTypep, EXTEND_EXP); + (void)underp; // cppcheck + } void iterateCheckSizedSelf(AstNode* nodep, const char* side, AstNode* underp, Determ determ, Stage stage) { // Coerce child to any sized-number data type; child is self-determined @@ -7081,6 +7097,11 @@ class WidthVisitor final : public VNVisitor { underp = VN_IS(underp, NodeExpr) ? checkCvtUS(VN_AS(underp, NodeExpr)) : underp; AstNodeDType* const expDTypep = underp->dtypep(); underp = iterateCheck(nodep, side, underp, SELF, FINAL, expDTypep, EXTEND_EXP); + AstNodeDType* const checkDtp = expDTypep->skipRefToEnump(); + if (!checkDtp->isIntegralOrPacked()) { + nodep->v3error("Expected numeric type, but got a " << checkDtp->prettyDTypeNameQ() + << " data type"); + } (void)underp; // cppcheck } void iterateCheckAssign(AstNode* nodep, const char* side, AstNode* rhsp, Stage stage, diff --git a/test_regress/t/t_math_countbits2_bad.out b/test_regress/t/t_math_countbits2_bad.out new file mode 100644 index 000000000..fd042e2f0 --- /dev/null +++ b/test_regress/t/t_math_countbits2_bad.out @@ -0,0 +1,17 @@ +%Error: t/t_math_countbits2_bad.v:15:15: Expected numeric type, but got a 'logic$[0:3]' data type + : ... note: In instance 't' + 15 | count = $countones(my_vec); + | ^~~~~~~~~~ +%Error: t/t_math_countbits2_bad.v:16:15: Expected numeric type, but got a 'logic$[0:3]' data type + : ... note: In instance 't' + 16 | count = $countbits(my_vec, '0); + | ^~~~~~~~~~ +%Error: t/t_math_countbits2_bad.v:17:14: Expected numeric type, but got a 'logic$[0:3]' data type + : ... note: In instance 't' + 17 | bool = $onehot(my_vec); + | ^~~~~~~ +%Error: t/t_math_countbits2_bad.v:18:14: Expected numeric type, but got a 'logic$[0:3]' data type + : ... note: In instance 't' + 18 | bool = $onehot0(my_vec); + | ^~~~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_math_countbits2_bad.py b/test_regress/t/t_math_countbits2_bad.py new file mode 100755 index 000000000..55203b6c9 --- /dev/null +++ b/test_regress/t/t_math_countbits2_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_math_countbits2_bad.v b/test_regress/t/t_math_countbits2_bad.v new file mode 100644 index 000000000..bbbb2a215 --- /dev/null +++ b/test_regress/t/t_math_countbits2_bad.v @@ -0,0 +1,23 @@ +// 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; + + logic my_vec [4]; + logic bool; + int count; + + initial begin + my_vec = '{1, 0, 1, 0}; + count = $countones(my_vec); // Bad, must be bit vector + count = $countbits(my_vec, '0); // Bad, must be bit vector + bool = $onehot(my_vec); // Bad, must be bit vector + bool = $onehot0(my_vec); // Bad, must be bit vector + bool = $isunknown(my_vec); // Bad, must be bit vector + $stop; + end + +endmodule