From 9a23711ff94b988e4358c258c747a79dd25d9a59 Mon Sep 17 00:00:00 2001 From: Christian Hecken <104357222+Christian-Hecken@users.noreply.github.com> Date: Sun, 7 Dec 2025 21:42:29 +0100 Subject: [PATCH] Fix stale pointers in VerilatedImpData::m_hierMap (#6726) --- include/verilated.cpp | 2 + include/verilated.h | 1 + include/verilated_imp.h | 5 ++ src/V3EmitCSyms.cpp | 5 ++ test_regress/t/t_vpi_hierarchy_clear.cpp | 64 ++++++++++++++++++++++++ test_regress/t/t_vpi_hierarchy_clear.py | 20 ++++++++ test_regress/t/t_vpi_hierarchy_clear.v | 24 +++++++++ 7 files changed, 121 insertions(+) create mode 100644 test_regress/t/t_vpi_hierarchy_clear.cpp create mode 100755 test_regress/t/t_vpi_hierarchy_clear.py create mode 100644 test_regress/t/t_vpi_hierarchy_clear.v diff --git a/include/verilated.cpp b/include/verilated.cpp index c9529811e..d94b0d24b 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -3584,6 +3584,8 @@ void VerilatedHierarchy::remove(const VerilatedScope* fromp, const VerilatedScop VerilatedImp::hierarchyRemove(fromp, top); } +void VerilatedHierarchy::clear() { VerilatedImp::hierarchyClear(); } + //=========================================================================== // VerilatedOneThreaded:: Methods diff --git a/include/verilated.h b/include/verilated.h index e41f66dfc..77c655689 100644 --- a/include/verilated.h +++ b/include/verilated.h @@ -743,6 +743,7 @@ class VerilatedHierarchy final { public: static void add(const VerilatedScope* fromp, const VerilatedScope* top); static void remove(const VerilatedScope* fromp, const VerilatedScope* top); + static void clear(); }; //=========================================================================== diff --git a/include/verilated_imp.h b/include/verilated_imp.h index 603f896c4..45a15eeaf 100644 --- a/include/verilated_imp.h +++ b/include/verilated_imp.h @@ -524,6 +524,11 @@ public: const auto it = find(scopes.begin(), scopes.end(), top); if (it != scopes.end()) scopes.erase(it); } + static void hierarchyClear() VL_MT_SAFE { + const VerilatedLockGuard lock{s().m_hierMapMutex}; + VerilatedHierarchyMap& map = s().m_hierMap; + map.clear(); + } static const VerilatedHierarchyMap* hierarchyMap() VL_MT_SAFE_POSTINIT { // Thread save only assuming this is called only after model construction completed return &s().m_hierMap; diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index d5553504f..2d67b19d8 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -626,6 +626,11 @@ void EmitCSyms::emitScopeHier(std::vector& stmts, bool destroy) { stmts.emplace_back("__Vhier." + method + "(" + fromId + ", " + toId + ");"); } } + + if (destroy) { + stmts.emplace_back("// Clear keys from hierarchy map after values have been removed"); + stmts.emplace_back("__Vhier.clear();"); + } } std::vector EmitCSyms::getSymCtorStmts() { diff --git a/test_regress/t/t_vpi_hierarchy_clear.cpp b/test_regress/t/t_vpi_hierarchy_clear.cpp new file mode 100644 index 000000000..a304477e4 --- /dev/null +++ b/test_regress/t/t_vpi_hierarchy_clear.cpp @@ -0,0 +1,64 @@ +// ====================================================================== +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty. +// SPDX-License-Identifier: CC0-1.0 +// ====================================================================== + +// DESCRIPTION: Scope hierarchy map clearing upon VerilatedModel destruction +// +// VerilatedImp::s() is a VerilatedImpData singleton that contains an m_hierMap +// whose keys are pointers to VerilatedScope objects. Because it is a +// singleton, it is not automatically destroyed together with the +// VerilatedModel, so this test checks that the VerilatedSyms destructor that is +// invoked upon the VerilatedModel's destruction clears the keys from the map. + +// Workaround to be able to include verilated_imp.h, needed to directly test the hierarchy map +#define VERILATOR_VERILATED_CPP_ +#include "verilated_imp.h" +#include + +#include "TestVpi.h" +#include "Vt_vpi_hierarchy_clear.h" +#include "Vt_vpi_hierarchy_clear__Syms.h" + +#include + +int main(int argc, char** argv) { + const std::unique_ptr contextp{new VerilatedContext}; + contextp->commandArgs(argc, argv); + + // Vt_vpi_hierarchy_clear::vlSymsp is private, so create a dummy syms object just to be able to + // construct the VerilatedScope objects + std::unique_ptr dummySymsp + = std::make_unique(contextp.get(), "dummySymsObject", + nullptr); + std::unique_ptr additionalScope + = std::make_unique(dummySymsp.get(), "t.additional", "additional", + "Additional", -12, VerilatedScope::SCOPE_MODULE); + + { + // Construct and destroy + const std::unique_ptr topp{ + new VM_PREFIX{contextp.get(), + // Note null name - we're flattening it out + ""}}; + + // Insert additional scope into map for the first topp + VerilatedImp::hierarchyAdd(additionalScope.get(), nullptr); + const bool scopeFound = (VerilatedImp::hierarchyMap()->find(additionalScope.get()) + != VerilatedImp::hierarchyMap()->end()); + CHECK_RESULT_NZ(scopeFound); // NOLINT(concurrency-mt-unsafe) + } + + // Test second construction + const std::unique_ptr topp{new VM_PREFIX{contextp.get(), + // Note null name - we're flattening it out + ""}}; + + // Do not insert additionalScope this time, so it should not be present in the map any more + const bool scopeFound = (VerilatedImp::hierarchyMap()->find(additionalScope.get()) + != VerilatedImp::hierarchyMap()->end()); + CHECK_RESULT_Z(scopeFound); // NOLINT(concurrency-mt-unsafe) + + return 0; +} diff --git a/test_regress/t/t_vpi_hierarchy_clear.py b/test_regress/t/t_vpi_hierarchy_clear.py new file mode 100755 index 000000000..c850792e2 --- /dev/null +++ b/test_regress/t/t_vpi_hierarchy_clear.py @@ -0,0 +1,20 @@ +#!/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('vlt_all') + +test.compile(make_top_shell=False, + make_main=False, + verilator_flags2=["--exe --vpi", test.pli_filename]) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_vpi_hierarchy_clear.v b/test_regress/t/t_vpi_hierarchy_clear.v new file mode 100644 index 000000000..190886548 --- /dev/null +++ b/test_regress/t/t_vpi_hierarchy_clear.v @@ -0,0 +1,24 @@ +// ====================================================================== +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty. +// SPDX-License-Identifier: CC0-1.0 +// ====================================================================== + +module t ( + clk +); + + input clk; + + Sub sub (.clk(clk)); + +endmodule + +module Sub ( + clk +); + + // this comment should get ignored for public-ignore + input clk /* verilator public_flat_rw */; + +endmodule