From 1946df8bc8028be45988c140892d7d434de4fd4f Mon Sep 17 00:00:00 2001 From: Eunseo Song <162149585+eunseo9311@users.noreply.github.com> Date: Fri, 27 Mar 2026 12:13:26 +0900 Subject: [PATCH] Fix type check for bitwise operators on unpacked arrays (#5664) Reject non-packed array operands (unpacked, dynamic, queue, associative) in bitwise AND, OR, and XOR at the type-checking stage (V3Width) instead of letting them reach V3DfgPeephole where they cause an internal error. Add AstNodeDType::isNonPackedArray() helper and replace all existing VN_IS(*, UnpackArrayDType) || VN_IS(*, DynArrayDType) || ... patterns across the codebase to use it. Closes #5664 --- src/V3AstNodeDType.h | 2 ++ src/V3AstNodes.cpp | 5 +++++ src/V3Const.cpp | 6 ++---- src/V3EmitCHeaders.cpp | 4 +--- src/V3Randomize.cpp | 20 ++++++------------- src/V3Width.cpp | 25 ++++++++++++++++-------- test_regress/t/t_bitwise_array_bad.out | 10 ++++++++++ test_regress/t/t_bitwise_array_bad.py | 16 +++++++++++++++ test_regress/t/t_bitwise_array_bad.v | 15 ++++++++++++++ test_regress/t/t_mem_slice_dtype_bad.out | 17 +++++++++++++--- 10 files changed, 88 insertions(+), 32 deletions(-) create mode 100644 test_regress/t/t_bitwise_array_bad.out create mode 100644 test_regress/t/t_bitwise_array_bad.py create mode 100644 test_regress/t/t_bitwise_array_bad.v diff --git a/src/V3AstNodeDType.h b/src/V3AstNodeDType.h index 608ab7e1b..e1e7ba0c1 100644 --- a/src/V3AstNodeDType.h +++ b/src/V3AstNodeDType.h @@ -125,6 +125,8 @@ public: virtual AstNodeDType* subDTypep() const VL_MT_STABLE { return nullptr; } virtual AstNodeDType* subDType2p() const VL_MT_STABLE { return nullptr; } virtual bool isAggregateType() const { return false; } + // True for unpacked, dynamic, queue, and associative arrays (not packed arrays) + bool isNonPackedArray() const; virtual bool isFourstate() const; // Ideally an IEEE $typename virtual string prettyDTypeName(bool) const { return prettyTypeName(); } diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 417ffa5b6..3b1570b9b 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -1013,6 +1013,11 @@ bool AstNodeDType::similarDType(const AstNodeDType* samep) const { bool AstNodeDType::isFourstate() const { return basicp() && basicp()->isFourstate(); } +bool AstNodeDType::isNonPackedArray() const { + return VN_IS(this, UnpackArrayDType) || VN_IS(this, DynArrayDType) + || VN_IS(this, QueueDType) || VN_IS(this, AssocArrayDType); +} + class AstNodeDType::CTypeRecursed final { public: string m_type; // The base type, e.g.: "Foo_t"s diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 2409ec165..e95a70699 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -2397,8 +2397,7 @@ class ConstVisitor final : public VNVisitor { const AstNodeDType* const srcDTypep = srcp->dtypep()->skipRefp(); // Handle unpacked/queue/dynarray source -> queue/dynarray dest via // CvtArrayToArray (StreamL reverses, so reverse=true) - if ((VN_IS(srcDTypep, UnpackArrayDType) || VN_IS(srcDTypep, QueueDType) - || VN_IS(srcDTypep, DynArrayDType)) + if (srcDTypep->isNonPackedArray() && (VN_IS(dstDTypep, QueueDType) || VN_IS(dstDTypep, DynArrayDType))) { int blockSize = 1; if (const AstConst* const constp @@ -2461,8 +2460,7 @@ class ConstVisitor final : public VNVisitor { origSrcp = cvtp->fromp(); } const AstNodeDType* const origSrcDTypep = origSrcp->dtypep()->skipRefp(); - if (VN_IS(origSrcDTypep, UnpackArrayDType) || VN_IS(origSrcDTypep, QueueDType) - || VN_IS(origSrcDTypep, DynArrayDType)) { + if (origSrcDTypep->isNonPackedArray()) { int srcElementBits = 0; if (const AstNodeDType* const elemDtp = origSrcDTypep->subDTypep()) { srcElementBits = elemDtp->width(); diff --git a/src/V3EmitCHeaders.cpp b/src/V3EmitCHeaders.cpp index 77124ad91..2fc364bbc 100644 --- a/src/V3EmitCHeaders.cpp +++ b/src/V3EmitCHeaders.cpp @@ -280,9 +280,7 @@ class EmitCHeader final : public EmitCConstInit { enum class AttributeType { Width, Dimension }; // Get member attribute based on type int getNodeAttribute(const AstMemberDType* itemp, AttributeType type) { - const bool isArrayType - = VN_IS(itemp->dtypep(), UnpackArrayDType) || VN_IS(itemp->dtypep(), DynArrayDType) - || VN_IS(itemp->dtypep(), QueueDType) || VN_IS(itemp->dtypep(), AssocArrayDType); + const bool isArrayType = itemp->dtypep()->isNonPackedArray(); switch (type) { case AttributeType::Width: { if (isArrayType) { diff --git a/src/V3Randomize.cpp b/src/V3Randomize.cpp index 65825503c..661f69d13 100644 --- a/src/V3Randomize.cpp +++ b/src/V3Randomize.cpp @@ -1136,8 +1136,7 @@ class ConstraintExprVisitor final : public VNVisitor { AstClassRefDType* elemClassRefDtp = nullptr; { AstNodeDType* varDtp = varp->dtypep()->skipRefp(); - if (VN_IS(varDtp, DynArrayDType) || VN_IS(varDtp, QueueDType) - || VN_IS(varDtp, UnpackArrayDType) || VN_IS(varDtp, AssocArrayDType)) { + if (varDtp->isNonPackedArray()) { AstNodeDType* const elemDtp = varDtp->subDTypep()->skipRefp(); elemClassRefDtp = VN_CAST(elemDtp, ClassRefDType); if (elemClassRefDtp) { @@ -1209,9 +1208,7 @@ class ConstraintExprVisitor final : public VNVisitor { AstVar* const memberVarp = VN_CAST(mnodep, Var); if (!memberVarp || !memberVarp->rand().isRandomizable()) continue; AstNodeDType* const memberDtp = memberVarp->dtypep()->skipRefp(); - if (VN_IS(memberDtp, ClassRefDType) || VN_IS(memberDtp, DynArrayDType) - || VN_IS(memberDtp, QueueDType) || VN_IS(memberDtp, UnpackArrayDType) - || VN_IS(memberDtp, AssocArrayDType)) + if (VN_IS(memberDtp, ClassRefDType) || memberDtp->isNonPackedArray()) continue; const int memberWidth = memberDtp->width(); @@ -1281,9 +1278,7 @@ class ConstraintExprVisitor final : public VNVisitor { VAccess::READWRITE}, VCMethod::RANDOMIZER_WRITE_VAR}; uint32_t dimension = 0; - if (VN_IS(varp->dtypep(), UnpackArrayDType) || VN_IS(varp->dtypep(), DynArrayDType) - || VN_IS(varp->dtypep(), QueueDType) - || VN_IS(varp->dtypep(), AssocArrayDType)) { + if (varp->dtypep()->isNonPackedArray()) { const std::pair dims = varp->dtypep()->dimensions(/*includeBasic=*/true); const uint32_t unpackedDimensions = dims.second; @@ -1316,8 +1311,7 @@ class ConstraintExprVisitor final : public VNVisitor { methodp->addPinsp(varRefp); } AstNodeDType* tmpDtypep = varp->dtypep(); - while (VN_IS(tmpDtypep, UnpackArrayDType) || VN_IS(tmpDtypep, DynArrayDType) - || VN_IS(tmpDtypep, QueueDType) || VN_IS(tmpDtypep, AssocArrayDType)) + while (tmpDtypep->isNonPackedArray()) tmpDtypep = tmpDtypep->subDTypep(); const size_t width = tmpDtypep->width(); methodp->addPinsp( @@ -3369,8 +3363,7 @@ class RandomizeVisitor final : public VNVisitor { dtypep->findBasicDType(VBasicDTypeKwd::UINT32)}; }; AstNodeExpr* tempElementp = nullptr; - while (VN_IS(tempDTypep, DynArrayDType) || VN_IS(tempDTypep, UnpackArrayDType) - || VN_IS(tempDTypep, AssocArrayDType) || VN_IS(tempDTypep, QueueDType)) { + while (tempDTypep->isNonPackedArray()) { AstVar* const newRandLoopIndxp = createLoopIndex(tempDTypep); randLoopIndxp = AstNode::addNext(randLoopIndxp, newRandLoopIndxp); AstNodeExpr* const tempExprp = tempElementp ? tempElementp : exprp; @@ -4396,8 +4389,7 @@ class RandomizeVisitor final : public VNVisitor { } AstNodeDType* tmpDtypep = arrVarp->dtypep(); - while (VN_IS(tmpDtypep, UnpackArrayDType) || VN_IS(tmpDtypep, DynArrayDType) - || VN_IS(tmpDtypep, QueueDType) || VN_IS(tmpDtypep, AssocArrayDType)) + while (tmpDtypep->isNonPackedArray()) tmpDtypep = tmpDtypep->subDTypep(); const size_t width = tmpDtypep->width(); diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 84810a942..0e830692b 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -3328,8 +3328,7 @@ class WidthVisitor final : public VNVisitor { // Similar logic in V3Case return irangep->newAndFromInside(exprp, irangep->lhsp()->unlinkFrBack(), irangep->rhsp()->unlinkFrBack()); - } else if (VN_IS(itemDtp, UnpackArrayDType) || VN_IS(itemDtp, DynArrayDType) - || VN_IS(itemDtp, QueueDType)) { + } else if (itemDtp->isNonPackedArray()) { // Unsupported in parameters AstNodeExpr* const inewp = new AstCMethodHard{nodep->fileline(), itemp->unlinkFrBack(), VCMethod::ARRAY_INSIDE, exprp}; @@ -5975,12 +5974,8 @@ class WidthVisitor final : public VNVisitor { const AstNodeDType* const rhsDtp = nodep->rhsp()->dtypep()->skipRefp(); // Only check if number of states match for unpacked array to unpacked array // assignments - const bool lhsIsUnpackArray - = VN_IS(lhsDtp, UnpackArrayDType) || VN_IS(lhsDtp, DynArrayDType) - || VN_IS(lhsDtp, QueueDType) || VN_IS(lhsDtp, AssocArrayDType); - const bool rhsIsUnpackArray - = VN_IS(rhsDtp, UnpackArrayDType) || VN_IS(rhsDtp, DynArrayDType) - || VN_IS(rhsDtp, QueueDType) || VN_IS(rhsDtp, AssocArrayDType); + const bool lhsIsUnpackArray = lhsDtp->isNonPackedArray(); + const bool rhsIsUnpackArray = rhsDtp->isNonPackedArray(); if (lhsIsUnpackArray && rhsIsUnpackArray) { if (lhsDtp->isFourstate() != rhsDtp->isFourstate()) { nodep->v3error("Assignment between 2-state and 4-state types requires " @@ -7918,6 +7913,20 @@ class WidthVisitor final : public VNVisitor { // Determine expression widths only relying on what's in the subops userIterateAndNext(nodep->lhsp(), WidthVP{CONTEXT_DET, PRELIM}.p()); userIterateAndNext(nodep->rhsp(), WidthVP{CONTEXT_DET, PRELIM}.p()); + // Bitwise operators are not defined for non-packed arrays + { + const AstNodeDType* const lhsDtp = nodep->lhsp()->dtypep()->skipRefp(); + const AstNodeDType* const rhsDtp = nodep->rhsp()->dtypep()->skipRefp(); + if (lhsDtp->isNonPackedArray() || rhsDtp->isNonPackedArray()) { + const AstNodeDType* const badDtp + = lhsDtp->isNonPackedArray() ? lhsDtp : rhsDtp; + nodep->v3error("Operator " << nodep->prettyTypeName() + << " not defined for " + << badDtp->prettyDTypeName(false) << " operands"); + nodep->dtypeSetBit(); + return; + } + } checkCvtUS(nodep->lhsp(), false); checkCvtUS(nodep->rhsp(), false); const int width = std::max(nodep->lhsp()->width(), nodep->rhsp()->width()); diff --git a/test_regress/t/t_bitwise_array_bad.out b/test_regress/t/t_bitwise_array_bad.out new file mode 100644 index 000000000..62da23ff0 --- /dev/null +++ b/test_regress/t/t_bitwise_array_bad.out @@ -0,0 +1,10 @@ +%Error: t/t_bitwise_array_bad.v:13:19: Operator OR not defined for logic[1:0]$[1:0] operands + : ... note: In instance 't' + 13 | assign o = arr | 1; + | ^ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Error: t/t_bitwise_array_bad.v:14:20: Operator OR not defined for logic[1:0]$[1:0] operands + : ... note: In instance 't' + 14 | assign o2 = arr | i + 1; + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_bitwise_array_bad.py b/test_regress/t/t_bitwise_array_bad.py new file mode 100644 index 000000000..38cf36b43 --- /dev/null +++ b/test_regress/t/t_bitwise_array_bad.py @@ -0,0 +1,16 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# 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-FileCopyrightText: 2026 Wilson Snyder +# 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_bitwise_array_bad.v b/test_regress/t/t_bitwise_array_bad.v new file mode 100644 index 000000000..8d7c28683 --- /dev/null +++ b/test_regress/t/t_bitwise_array_bad.v @@ -0,0 +1,15 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2024 Antmicro +// SPDX-License-Identifier: CC0-1.0 + +module t( + input [1:0] i, + output [1:0] o, + output [1:0] o2 +); + wire [1:0] arr [1:0]; + assign o = arr | 1; + assign o2 = arr | i + 1; +endmodule diff --git a/test_regress/t/t_mem_slice_dtype_bad.out b/test_regress/t/t_mem_slice_dtype_bad.out index 9b3748af6..56bb1240e 100644 --- a/test_regress/t/t_mem_slice_dtype_bad.out +++ b/test_regress/t/t_mem_slice_dtype_bad.out @@ -1,5 +1,16 @@ -%Error: t/t_mem_slice_dtype_bad.v:22:43: ADD unexpected in assignment to unpacked array - 22 | completed_cnt[id] <= completed_cnt_dp + 1; - | ^ +%Error: t/t_mem_slice_dtype_bad.v:37:25: Operator AND not defined for logic[39:0]$[1:0] operands + : ... note: In instance 't' + 37 | assert ((addr[id] & way_mask) == 0); + | ^ ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Warning-WIDTHTRUNC: t/t_mem_slice_dtype_bad.v:37:25: Operator AND expects 32 or 1 bits on the LHS, but LHS's ARRAYSEL generates 40 bits. + : ... note: In instance 't' + 37 | assert ((addr[id] & way_mask) == 0); + | ^ + ... For warning description see https://verilator.org/warn/WIDTHTRUNC?v=latest + ... Use "/* verilator lint_off WIDTHTRUNC */" and lint_on around source to disable this message. +%Warning-WIDTHTRUNC: t/t_mem_slice_dtype_bad.v:37:25: Operator AND expects 32 or 1 bits on the RHS, but RHS's VARREF 'way_mask' generates 40 bits. + : ... note: In instance 't' + 37 | assert ((addr[id] & way_mask) == 0); + | ^ %Error: Exiting due to