From 649598c74c17c2ff889c32e1c6a463f1adc04a58 Mon Sep 17 00:00:00 2001 From: Adam Kostrzewski Date: Fri, 14 Aug 2026 15:23:44 +0200 Subject: [PATCH] Fix unintended side-effect insertion on associative array read (#8077) --- include/verilated_random.h | 7 +-- include/verilated_save.h | 2 +- include/verilated_types.h | 8 ++-- src/V3AstNodeExpr.h | 6 ++- src/V3AstNodes.cpp | 2 + src/V3EmitCFunc.cpp | 4 +- src/V3EmitCFunc.h | 6 ++- test_regress/t/t_assocsel_sideeffect.py | 18 +++++++ test_regress/t/t_assocsel_sideeffect.v | 62 +++++++++++++++++++++++++ test_regress/t/t_wildcard_sideeffect.py | 18 +++++++ test_regress/t/t_wildcard_sideeffect.v | 35 ++++++++++++++ 11 files changed, 153 insertions(+), 15 deletions(-) create mode 100755 test_regress/t/t_assocsel_sideeffect.py create mode 100644 test_regress/t/t_assocsel_sideeffect.v create mode 100755 test_regress/t/t_wildcard_sideeffect.py create mode 100644 test_regress/t/t_wildcard_sideeffect.v diff --git a/include/verilated_random.h b/include/verilated_random.h index bbaee8f0f..c37ae92c2 100644 --- a/include/verilated_random.h +++ b/include/verilated_random.h @@ -570,7 +570,8 @@ public: idxWidths.push_back(idx_width); indices.insert(indices.end(), integral_index.begin(), integral_index.end()); - record_arr_table(var.at(key), indexed_name, dimension - 1, indices, idxWidths); + record_arr_table(var.atWrite(key), indexed_name, dimension - 1, indices, + idxWidths); // Cleanup indices and widths idxWidths.pop_back(); @@ -644,7 +645,7 @@ public: std::string result = oss.str(); result.insert(result.begin(), int(idx_width / 4) - result.size(), '0'); - record_struct_arr(var.at(key), name + "." + result, dimension - 1, indices, + record_struct_arr(var.atWrite(key), name + "." + result, dimension - 1, indices, idxWidths); } } @@ -772,7 +773,7 @@ public: bool basicStdRandomization(VlAssocArray& value, size_t width) { T_Key key; for (int exists = value.first(key); exists; exists = value.next(key)) { - basicStdRandomization(value.at(key), width); + basicStdRandomization(value.atWrite(key), width); } return true; } diff --git a/include/verilated_save.h b/include/verilated_save.h index aa547af68..18ee06d33 100644 --- a/include/verilated_save.h +++ b/include/verilated_save.h @@ -327,7 +327,7 @@ VerilatedDeserialize& operator>>(VerilatedDeserialize& os, VlAssocArray> index; os >> value; - rhs.at(index) = value; + rhs.atWrite(index) = value; } return os; } diff --git a/include/verilated_types.h b/include/verilated_types.h index caa31f7e9..e96de5a29 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -1120,9 +1120,7 @@ public: return 1; } // Setting. Verilog: assoc[index] = v - // Can't just overload operator[] or provide a "at" reference to set, - // because we need to be able to insert only when the value is set - T_Value& at(const T_Key& index) { + T_Value& atWrite(const T_Key& index) { const auto it = m_map.find(index); if (it == m_map.end()) { std::pair pit = m_map.emplace(index, m_defaultValue); @@ -1138,7 +1136,7 @@ public: } // Setting as a chained operation VlAssocArray& set(const T_Key& index, const T_Value& value) { - at(index) = value; + atWrite(index) = value; return *this; } VlAssocArray& setDefault(const T_Value& value) { @@ -1395,7 +1393,7 @@ void VL_READMEM_N(bool hex, int bits, const std::string& filename, QData addr; std::string data; if (rmem.get(addr /*ref*/, data /*ref*/)) { - rmem.setData(&(obj.at(addr)), data); + rmem.setData(&(obj.atWrite(addr)), data); } else { break; } diff --git a/src/V3AstNodeExpr.h b/src/V3AstNodeExpr.h index 6fae7af60..5c3b8547c 100644 --- a/src/V3AstNodeExpr.h +++ b/src/V3AstNodeExpr.h @@ -4895,9 +4895,11 @@ public: bool cleanRhs() const override { return true; } bool sizeMattersLhs() const override { return false; } bool sizeMattersRhs() const override { return false; } - bool isGateOptimizable() const override { return false; } // AssocSel creates on miss + bool isGateOptimizable() const override { + return !isLValue(); // AssocSel creates on miss + } bool isPredictOptimizable() const override { return false; } - bool isPure() override { return false; } // AssocSel creates on miss + bool isPure() override { return !isLValue(); } // AssocSel creates on miss bool sameNode(const AstNode* /*samep*/) const override { return true; } int instrCount() const override { return widthInstrs(); } }; diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 249aacdd3..9f773e6aa 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -2053,6 +2053,8 @@ bool AstNodeExpr::isLValue() const { return varrefp->access().isWriteOrRW(); } else if (const AstMemberSel* const memberselp = VN_CAST(this, MemberSel)) { return memberselp->access().isWriteOrRW(); + } else if (const AstStructSel* const structselp = VN_CAST(this, StructSel)) { + return structselp->fromp()->isLValue(); } else if (const AstSel* const selp = VN_CAST(this, Sel)) { return selp->fromp()->isLValue(); } else if (const AstNodeSel* const nodeSelp = VN_CAST(this, NodeSel)) { diff --git a/src/V3EmitCFunc.cpp b/src/V3EmitCFunc.cpp index 6fa58ab8d..5aa4dca5e 100644 --- a/src/V3EmitCFunc.cpp +++ b/src/V3EmitCFunc.cpp @@ -485,7 +485,7 @@ void EmitCFunc::emitVarReset(const string& prefix, AstVar* varp, bool constructi const auto& mapr = initarp->map(); for (const auto& itr : mapr) { AstNode* const valuep = itr.second->valuep(); - emitSetVarConstant(newPrefix + ".at(" + cvtToStr(itr.first) + ")", + emitSetVarConstant(newPrefix + ".atWrite(" + cvtToStr(itr.first) + ")", VN_AS(valuep, Const)); } } else if (VN_IS(dtypep, WildcardArrayDType)) { @@ -496,7 +496,7 @@ void EmitCFunc::emitVarReset(const string& prefix, AstVar* varp, bool constructi const auto& mapr = initarp->map(); for (const auto& itr : mapr) { AstNode* const valuep = itr.second->valuep(); - emitSetVarConstant(newPrefix + ".at(" + cvtToStr(itr.first) + ")", + emitSetVarConstant(newPrefix + ".atWrite(" + cvtToStr(itr.first) + ")", VN_AS(valuep, Const)); } } else if (AstUnpackArrayDType* const adtypep = VN_CAST(dtypep, UnpackArrayDType)) { diff --git a/src/V3EmitCFunc.h b/src/V3EmitCFunc.h index 5e527870c..5529f4b8a 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -732,7 +732,8 @@ public: } void visit(AstAssocSel* nodep) override { iterateAndNextConstNull(nodep->fromp()); - putnbs(nodep, ".at("); + const std::string atFunc = nodep->isLValue() ? ".atWrite(" : ".at("; + putnbs(nodep, atFunc); AstAssocArrayDType* const adtypep = VN_AS(nodep->fromp()->dtypep()->skipRefp(), AssocArrayDType); UASSERT_OBJ(adtypep, nodep, "Associative select on non-associative type"); @@ -741,7 +742,8 @@ public: } void visit(AstWildcardSel* nodep) override { iterateAndNextConstNull(nodep->fromp()); - putnbs(nodep, ".at("); + const std::string atFunc = nodep->isLValue() ? ".atWrite(" : ".at("; + putnbs(nodep, atFunc); AstWildcardArrayDType* const adtypep = VN_AS(nodep->fromp()->dtypep()->skipRefp(), WildcardArrayDType); UASSERT_OBJ(adtypep, nodep, "Wildcard select on non-wildcard-associative type"); diff --git a/test_regress/t/t_assocsel_sideeffect.py b/test_regress/t/t_assocsel_sideeffect.py new file mode 100755 index 000000000..8a938befd --- /dev/null +++ b/test_regress/t/t_assocsel_sideeffect.py @@ -0,0 +1,18 @@ +#!/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('simulator') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_assocsel_sideeffect.v b/test_regress/t/t_assocsel_sideeffect.v new file mode 100644 index 000000000..0afc941a0 --- /dev/null +++ b/test_regress/t/t_assocsel_sideeffect.v @@ -0,0 +1,62 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// 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 Antmicro +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +// verilog_format: on + +module t; + + // dist + class Cls; + rand bit dict[int unsigned]; + function void call_rand; + void'(randomize() with { + dict[0] dist { + 1 :/ 1 + }; + }); + endfunction + endclass + + // foreach + int dict1d[int]; + int dict2d[int][string]; + int dict3d[int][string][int]; + Cls cls; + initial begin + // 1D + foreach (dict1d[b]) begin + $error(b); // should never reach + end + `checkd(dict1d.size(), 0); + + // 2D + foreach (dict2d[0][b]) begin + $error(b); // should never reach + end + `checkd(dict2d.size(), 0); + + // 3D + foreach (dict3d[0][i]) begin + foreach (dict3d[0][i][j]) begin + $error(i, j); // should never reach + end + end + `checkd(dict3d.size(), 0); + + cls = new; + cls.call_rand(); + `checkd(cls.dict.size(), 0); + + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule diff --git a/test_regress/t/t_wildcard_sideeffect.py b/test_regress/t/t_wildcard_sideeffect.py new file mode 100755 index 000000000..8a938befd --- /dev/null +++ b/test_regress/t/t_wildcard_sideeffect.py @@ -0,0 +1,18 @@ +#!/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('simulator') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_wildcard_sideeffect.v b/test_regress/t/t_wildcard_sideeffect.v new file mode 100644 index 000000000..2ddcf3293 --- /dev/null +++ b/test_regress/t/t_wildcard_sideeffect.v @@ -0,0 +1,35 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// 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 Antmicro +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +// verilog_format: on + +module t; + initial begin + int a; + int dict1d[*]; + int dict2d[*][*]; + int dict3d[*][*][*]; + int dictmix[int][*]; + + `checkd(dict1d[0], 0); + `checkd(dict1d.size(), 0); + `checkd(dict2d[0][0], 0); + `checkd(dict2d.size(), 0); + `checkd(dict3d[0][0][0], 0); + `checkd(dict3d.size(), 0); + `checkd(dictmix[0][0], 0); + `checkd(dictmix.size(), 0); + + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule