From bdfb2e8db16bf59dd6bc9b1f4009d30a11510888 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Thu, 1 Oct 2026 08:04:56 -0400 Subject: [PATCH] Fix --hierarchical parameter specialization name collisions (#8555) (#8565) Fixes #8565. --- Changes | 1 + src/V3Param.cpp | 64 +++++++------ test_regress/t/t_class_param_enum_bad.out | 2 +- test_regress/t/t_hier_block_param_name.py | 18 ++++ test_regress/t/t_hier_block_param_name.v | 111 ++++++++++++++++++++++ test_regress/t/t_mailbox_bad.out | 2 +- test_regress/t/t_param_array_sparse.py | 18 ++++ test_regress/t/t_param_array_sparse.v | 46 +++++++++ test_regress/t/t_timing_at_dtype_bad.out | 4 +- 9 files changed, 231 insertions(+), 35 deletions(-) create mode 100755 test_regress/t/t_hier_block_param_name.py create mode 100644 test_regress/t/t_hier_block_param_name.v create mode 100755 test_regress/t/t_param_array_sparse.py create mode 100644 test_regress/t/t_param_array_sparse.v diff --git a/Changes b/Changes index db12ebcd3..b4813dbcf 100644 --- a/Changes +++ b/Changes @@ -154,6 +154,7 @@ Verilator 5.053 devel * Fix stack depth calculations over calls (#8544). [Nick Brereton] * Fix cascading errors after unsupported class dot reference in parameter value (#8551). [Martijn Wobbes, Antmicro Ltd.] * Fix vpi_put_value crash on NULL value pointer (#8553). [Nick Brereton] +* Fix --hierarchical parameter specialization name collisions (#8555) (#8565). [Marco Bartoli] * Fix dehash of a suffix after a hashed name (#8556). [Ethan Sifferman] diff --git a/src/V3Param.cpp b/src/V3Param.cpp index d1ffeef34..a5e9794d9 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -57,7 +57,6 @@ #include "V3Case.h" #include "V3Const.h" #include "V3EmitV.h" -#include "V3Hasher.h" #include "V3LinkDotIfaceCapture.h" #include "V3MemberMap.h" #include "V3Os.h" @@ -72,6 +71,7 @@ #include #include #include +#include #include #include @@ -281,7 +281,7 @@ class ParamProcessor final { std::map m_longMap; // Hash of very long names to unique identity number - int m_longId = 0; + std::set m_longSuffixes; // Digest suffixes used by m_longMap // All module names that are loaded from source code // Generated modules by this visitor is not included @@ -290,8 +290,13 @@ class ParamProcessor final { CloneMap m_originalParams; // Map between parameters of copied parameteized classes and their // original nodes - std::map m_valueMap; // Hash of node hash to param value - int m_nextValue = 1; // Next value to use in m_valueMap + std::map m_valueNames; // Parameter value text to its name + std::set m_valueSuffixes; // Digest suffixes used by m_valueNames + // Number of digest hex digits in a name. With 8 digits, a collision between names made in + // different runs, such as those of different hierarchical blocks, is unlikely. Within one + // run, digestSuffix resolves a collision by lengthening the later text's suffix, which can + // then differ between runs. + static constexpr std::string::size_type DIGEST_DIGITS = 8; const AstNodeModule* m_modp = nullptr; // Current module being processed @@ -392,11 +397,12 @@ class ParamProcessor final { key += "] "; key += paramValueString(dtypep->subDTypep()); } else if (const AstInitArray* const initp = VN_CAST(nodep, InitArray)) { + // Include the indices and the default, as with a default the map may be sparse key += "{"; - for (auto it : initp->map()) { - key += paramValueString(it.second->valuep()); - key += ","; + for (const auto& it : initp->map()) { + key += cvtToStr(it.first) + ":" + paramValueString(it.second->valuep()) + ","; } + if (initp->defaultp()) key += "default:" + paramValueString(initp->defaultp()) + ","; key += "}"; } else if (const AstConsPackUOrStruct* const structp = VN_CAST(nodep, ConsPackUOrStruct)) { key += "{"; @@ -446,43 +452,39 @@ class ParamProcessor final { return key; } + // Return a name suffix for 'text' from its SHA-512 digest. Hierarchical blocks are + // Verilated in separate runs, where a counter would restart, but the digest is the same in + // every run. Use the shortest digest prefix of at least DIGEST_DIGITS digits that is not + // already in 'usedr', and add it to 'usedr'. + static string digestSuffix(const string& text, std::set& usedr) { + const string hex = VHashSha512{text}.digestHex(); + // Force collisions of the prefixes -- for testing only + string::size_type digits = v3Global.opt.debugCollision() ? 1 : DIGEST_DIGITS; + while (digits < hex.size() && !usedr.insert(hex.substr(0, digits)).second) ++digits; + return hex.substr(0, digits); + } string paramValueNumber(AstNode* nodep) { - // For type parameters (NodeDType), use only the string representation for hashing. - // Using V3Hasher::uncachedHash includes AST node pointer which differs for equivalent - // types represented by different AST nodes (e.g., parameterized class specializations). - // For value parameters, we can still use the AST hash for better collision resistance. // All call sites resolve through skipRefToNonRefp() or pass non-DType // nodes, so nodep should never be a bare RefDType here. if (VN_IS(nodep, RefDType)) { // LCOV_EXCL_LINE nodep->v3fatalSrc("Unexpected RefDType in paramValueNumber"); // LCOV_EXCL_LINE } - const string paramStr = paramValueString(nodep); - V3Hash hash; - if (VN_IS(nodep, NodeDType)) { - // Type parameter: use only string-based hash for type equivalence - hash = V3Hash{paramStr}; - } else { - // Value parameter: use AST hash + string for better collision resistance - hash = V3Hasher::uncachedHash(nodep) + paramStr; - } - // Force hash collisions -- for testing only - // cppcheck-suppress unreadVariable - if (VL_UNLIKELY(v3Global.opt.debugCollision())) hash = V3Hash{paramStr}; - int num; - const auto pair = m_valueMap.emplace(hash, 0); - if (pair.second) pair.first->second = m_nextValue++; - num = pair.first->second; - return "z"s + cvtToStr(num); + // Name the value by its text, which is the same for equal values or types in every run. + // V3Hasher is unsuitable, as it hashes node pointers, which can differ for equal types. + // V3Hash of a string is unsuitable, as std::hash varies between C++ libraries. + const string text = paramValueString(nodep); + const auto pair = m_valueNames.emplace(text, ""); + if (pair.second) pair.first->second = "z" + digestSuffix(text, m_valueSuffixes); + return pair.first->second; } string moduleCalcName(const AstNodeModule* srcModp, const string& longname) { string newname = longname; if (longname.length() > 30) { const auto pair = m_longMap.emplace(longname, ""); if (pair.second) { - newname = srcModp->name(); // We use all upper case above, so lower here can't conflict - newname += "__pi" + cvtToStr(++m_longId); - pair.first->second = newname; + pair.first->second + = srcModp->name() + "__pi" + digestSuffix(longname, m_longSuffixes); } newname = pair.first->second; } diff --git a/test_regress/t/t_class_param_enum_bad.out b/test_regress/t/t_class_param_enum_bad.out index 1bd914c75..ff73ba725 100644 --- a/test_regress/t/t_class_param_enum_bad.out +++ b/test_regress/t/t_class_param_enum_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_class_param_enum_bad.v:25:40: Assign RHS expects a CLASSREFDTYPE 'Converter__Tz2', got CLASSREFDTYPE 'Converter__Tz1' +%Error: t/t_class_param_enum_bad.v:25:40: Assign RHS expects a CLASSREFDTYPE 'Converter__Tz7d09f080', got CLASSREFDTYPE 'Converter__Tza32c8cf8' : ... note: In instance 't' 25 | automatic Converter #(bit) conv2 = conv1; | ^~~~~ diff --git a/test_regress/t/t_hier_block_param_name.py b/test_regress/t/t_hier_block_param_name.py new file mode 100755 index 000000000..6042640d3 --- /dev/null +++ b/test_regress/t/t_hier_block_param_name.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('vlt') + +test.compile(verilator_flags2=['--binary', '--hierarchical']) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_hier_block_param_name.v b/test_regress/t/t_hier_block_param_name.v new file mode 100644 index 000000000..05e555cd6 --- /dev/null +++ b/test_regress/t/t_hier_block_param_name.v @@ -0,0 +1,111 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// Specializations of a parameterized class are distinct types, but for matching ones, and +// $typename returns their names (IEEE 1800-2023 8.25, 20.6.1). Hierarchical block 'hb' and the +// parent are Verilated in runs of their own, which each name the specializations they elaborate: +// a specialization has one name in both, and distinct ones distinct names. hb elaborates two +// specializations of each class, and passes their names to the parent, which elaborates the +// second of those. + +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got='%s' exp='%s'\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +// A type name as characters, the first the most significant, for a port to pass +localparam int NAME_CHARS = 64; + +function automatic logic [8*NAME_CHARS-1:0] to_bits(string name); + to_bits = '0; + for (int i = 0; i < name.len() && i < NAME_CHARS; ++i) to_bits[8*(NAME_CHARS-1-i)+:8] = name[i]; +endfunction + +function automatic string to_name(logic [8*NAME_CHARS-1:0] name_bits); + to_name = ""; + for (int i = 0; i < NAME_CHARS; ++i) begin + if (name_bits[8*(NAME_CHARS-1-i)+:8] == 0) break; + to_name = {to_name, string'(name_bits[8*(NAME_CHARS-1-i)+:8])}; + end +endfunction + +// Of a type parameter +class TypeParam #( + type T = int +); + T v; +endclass + +// Of a parameter not of 32 bits +class ByteParam #( + bit [7:0] B = 8'd1 +); + bit [7:0] v = B; +endclass + +// Of parameters whose values make a long name +class LongParameters #( + int FIRST_PARAMETER = 1, + int SECOND_PARAMETER = 2 +); + int v = FIRST_PARAMETER + SECOND_PARAMETER; +endclass + +module hb ( + output logic [8*NAME_CHARS-1:0] type_a_name, + output logic [8*NAME_CHARS-1:0] type_b_name, + output logic [8*NAME_CHARS-1:0] byte_a_name, + output logic [8*NAME_CHARS-1:0] byte_b_name, + output logic [8*NAME_CHARS-1:0] long_a_name, + output logic [8*NAME_CHARS-1:0] long_b_name +); + /*verilator hier_block*/ + TypeParam #(byte) type_a; + TypeParam #(shortint) type_b; + ByteParam #(8'd2) byte_a; + ByteParam #(8'd3) byte_b; + LongParameters #(1111111, 2222222) long_a; + LongParameters #(3333333, 4444444) long_b; + + // In a procedure, as the handles are dynamic (IEEE 1800-2023 6.21) + initial begin + type_a_name = to_bits($typename(type_a)); + type_b_name = to_bits($typename(type_b)); + byte_a_name = to_bits($typename(byte_a)); + byte_b_name = to_bits($typename(byte_b)); + long_a_name = to_bits($typename(long_a)); + long_b_name = to_bits($typename(long_b)); + end +endmodule + +module t; + logic [8*NAME_CHARS-1:0] type_a_name; + logic [8*NAME_CHARS-1:0] type_b_name; + logic [8*NAME_CHARS-1:0] byte_a_name; + logic [8*NAME_CHARS-1:0] byte_b_name; + logic [8*NAME_CHARS-1:0] long_a_name; + logic [8*NAME_CHARS-1:0] long_b_name; + TypeParam #(shortint) type_b; + ByteParam #(8'd3) byte_b; + LongParameters #(3333333, 4444444) long_b; + + hb h (.*); + + // Without $finish, which could come before hb's names: the simulation ends, and runs the final + // blocks, once time zero has run + final begin + // A specialization of both has one name in both + `checks(to_name(type_b_name), $typename(type_b)); + `checks(to_name(byte_b_name), $typename(byte_b)); + `checks(to_name(long_b_name), $typename(long_b)); + // Distinct specializations have distinct names + `checkd(to_name(type_a_name) != $typename(type_b), 1); + `checkd(to_name(byte_a_name) != $typename(byte_b), 1); + `checkd(to_name(long_a_name) != $typename(long_b), 1); + $write("*-* All Finished *-*\n"); + end +endmodule diff --git a/test_regress/t/t_mailbox_bad.out b/test_regress/t/t_mailbox_bad.out index e0d32b705..8e638ed3c 100644 --- a/test_regress/t/t_mailbox_bad.out +++ b/test_regress/t/t_mailbox_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_mailbox_bad.v:12:11: Class method 'bad_method' not found in class 'mailbox__Tz1' +%Error: t/t_mailbox_bad.v:12:11: Class method 'bad_method' not found in class 'mailbox__Tz6d8d043b' : ... note: In instance 't' 12 | if (m.bad_method() != 0) $stop; | ^~~~~~~~~~ diff --git a/test_regress/t/t_param_array_sparse.py b/test_regress/t/t_param_array_sparse.py new file mode 100755 index 000000000..46d1fe4c0 --- /dev/null +++ b/test_regress/t/t_param_array_sparse.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(verilator_flags2=['--binary']) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_param_array_sparse.v b/test_regress/t/t_param_array_sparse.v new file mode 100644 index 000000000..866ed4720 --- /dev/null +++ b/test_regress/t/t_param_array_sparse.v @@ -0,0 +1,46 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// Unpacked array parameter values with equal elements at different indices are different +// values, so the instances given them must not share a module specialization. + +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +typedef int arr_t[4]; + +// Sets one element only, so the constant value has that element and a default for the others +function automatic arr_t one_hot(int idx); + one_hot[idx] = 5; +endfunction + +module sub #( + parameter arr_t ARR = '{default: 0} +) ( + output arr_t o +); + assign o = ARR; +endmodule + +module t; + arr_t o1; + arr_t o2; + + sub #(.ARR(one_hot(1))) u1 (.o(o1)); + sub #(.ARR(one_hot(2))) u2 (.o(o2)); + + // Without $finish, which could come before the outputs: the simulation ends, and runs the final + // blocks, once time zero has run + final begin + `checkd(o1[1], 5); + `checkd(o1[2], 0); + `checkd(o2[1], 0); + `checkd(o2[2], 5); + $write("*-* All Finished *-*\n"); + end +endmodule diff --git a/test_regress/t/t_timing_at_dtype_bad.out b/test_regress/t/t_timing_at_dtype_bad.out index b4eb22942..121cba106 100644 --- a/test_regress/t/t_timing_at_dtype_bad.out +++ b/test_regress/t/t_timing_at_dtype_bad.out @@ -1,11 +1,11 @@ %Error-UNSUPPORTED: t/t_timing_at_dtype_bad.v:20:12: Unsupported: Cannot detect changes on expression of complex type 'int$[$]' - : ... note: In instance 't::any_monitor__Tz1_TBz1' + : ... note: In instance 't::any_monitor__pif994bd0b' : ... May be caused by combinational cycles reported with UNOPTFLAT 20 | @req; | ^~~ ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest %Error-UNSUPPORTED: t/t_timing_at_dtype_bad.v:23:12: Unsupported: Cannot detect changes on expression of complex type 'int$[$]' - : ... note: In instance 't::any_monitor__Tz1_TBz1' + : ... note: In instance 't::any_monitor__pif994bd0b' : ... May be caused by combinational cycles reported with UNOPTFLAT 23 | @rsp; | ^~~