From 5aa12e9b511e2fbd1338c7f242da313ad39355c9 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sun, 15 May 2022 10:50:44 -0400 Subject: [PATCH] Add assert when VerilatedContext is mis-deleted (#3121). --- Changes | 1 + docs/guide/connecting.rst | 3 +++ examples/make_tracing_c/sim_main.cpp | 2 ++ include/verilated.cpp | 14 ++++++++++- include/verilated.h | 7 ++++++ test_regress/driver.pl | 1 + test_regress/t/t_wrapper_del_context_bad.cpp | 22 +++++++++++++++++ test_regress/t/t_wrapper_del_context_bad.out | 2 ++ test_regress/t/t_wrapper_del_context_bad.pl | 25 ++++++++++++++++++++ test_regress/t/t_wrapper_del_context_bad.v | 9 +++++++ 10 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 test_regress/t/t_wrapper_del_context_bad.cpp create mode 100644 test_regress/t/t_wrapper_del_context_bad.out create mode 100755 test_regress/t/t_wrapper_del_context_bad.pl create mode 100644 test_regress/t/t_wrapper_del_context_bad.v diff --git a/Changes b/Changes index 85b3f62bb..615f7088f 100644 --- a/Changes +++ b/Changes @@ -14,6 +14,7 @@ Verilator 4.223 devel **Minor:** * Support compile time trace signal selection with tracing_on/off (#3323). [Shunyao CAD] +* Add assert when VerilatedContext is mis-deleted (#3121). [Ruptert Swarbrick] * Fix hang with large case statement optimization (#3405). [Mike Urbach] * Fix 'with' operator with type casting (#3387). [xiak95] diff --git a/docs/guide/connecting.rst b/docs/guide/connecting.rst index 8259447b3..edfc3d67b 100644 --- a/docs/guide/connecting.rst +++ b/docs/guide/connecting.rst @@ -110,6 +110,9 @@ model. Here is a simple example: Verilated::commandArgs(argc, argv); // Remember args top = new Vtop; // Create model + // Do not instead make Vtop as a file-scope static + // variable, as the "C++ static initialization order fiasco" + // may cause a crash top->reset_l = 0; // Set some inputs diff --git a/examples/make_tracing_c/sim_main.cpp b/examples/make_tracing_c/sim_main.cpp index 04181850a..06d399901 100644 --- a/examples/make_tracing_c/sim_main.cpp +++ b/examples/make_tracing_c/sim_main.cpp @@ -34,6 +34,8 @@ int main(int argc, char** argv, char** env) { // Using unique_ptr is similar to // "VerilatedContext* contextp = new VerilatedContext" then deleting at end. const std::unique_ptr contextp{new VerilatedContext}; + // Do not instead make Vtop as a file-scope static variable, as the + // "C++ static initialization order fiasco" may cause a crash // Set debug level, 0 is off, 9 is highest presently used // May be overridden by commandArgs argument parsing diff --git a/include/verilated.cpp b/include/verilated.cpp index 782b8cf39..1f6fe3b4c 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -2289,7 +2289,17 @@ VerilatedContext::VerilatedContext() } // Must declare here not in interface, as otherwise forward declarations not known -VerilatedContext::~VerilatedContext() {} +VerilatedContext::~VerilatedContext() { + checkMagic(this); + m_magic = 0x1; // Arbitrary but 0x1 is what Verilator src uses for a deleted pointer +} + +void VerilatedContext::checkMagic(const VerilatedContext* contextp) { + if (VL_UNLIKELY(!contextp || contextp->m_magic != MAGIC)) { + VL_FATAL_MT("", 0, "", // LCOV_EXCL_LINE + "Attempt to create model using a bad/deleted VerilatedContext pointer"); + } +} VerilatedContext::Serialized::Serialized() { m_timeunit = VL_TIME_UNIT; // Initial value until overriden by _Vconfigure @@ -2657,6 +2667,7 @@ const VerilatedScopeNameMap* VerilatedContext::scopeNameMap() VL_MT_SAFE { VerilatedSyms::VerilatedSyms(VerilatedContext* contextp) : _vm_contextp__(contextp ? contextp : Verilated::threadContextp()) { + VerilatedContext::checkMagic(_vm_contextp__); Verilated::threadContextp(_vm_contextp__); #ifdef VL_THREADED __Vm_evalMsgQp = new VerilatedEvalMsgQueue; @@ -2664,6 +2675,7 @@ VerilatedSyms::VerilatedSyms(VerilatedContext* contextp) } VerilatedSyms::~VerilatedSyms() { + VerilatedContext::checkMagic(_vm_contextp__); #ifdef VL_THREADED delete __Vm_evalMsgQp; #endif diff --git a/include/verilated.h b/include/verilated.h index 889f59c7b..aa1a582be 100644 --- a/include/verilated.h +++ b/include/verilated.h @@ -374,6 +374,10 @@ protected: // List of free descriptors in the MCT region [4, 32) std::vector m_fdFreeMct VL_GUARDED_BY(m_fdMutex); + // Magic to check for bad construction + static constexpr uint64_t MAGIC = 0xC35F9A6E5298EE6EULL; // SHA256 "VerilatedContext" + uint64_t m_magic = MAGIC; + private: // CONSTRUCTORS VL_UNCOPYABLE(VerilatedContext); @@ -535,6 +539,9 @@ public: // But for internal use only // Internal: Serialization setup static constexpr size_t serialized1Size() VL_PURE { return sizeof(m_s); } void* serialized1Ptr() VL_MT_UNSAFE { return &m_s; } + + // Internal: Check magic number + static void checkMagic(const VerilatedContext* contextp); }; //=========================================================================== diff --git a/test_regress/driver.pl b/test_regress/driver.pl index 3f4db9538..ffcfac4a8 100755 --- a/test_regress/driver.pl +++ b/test_regress/driver.pl @@ -1892,6 +1892,7 @@ sub _make_main { $fh->print(" if (save_time && ${time} == save_time) {\n"); $fh->print(" save_model(\"$self->{obj_dir}/saved.vltsv\");\n"); $fh->print(" printf(\"Exiting after save_model\\n\");\n"); + $fh->print(" topp.reset(nullptr);\n"); $fh->print(" return 0;\n"); $fh->print(" }\n"); } diff --git a/test_regress/t/t_wrapper_del_context_bad.cpp b/test_regress/t/t_wrapper_del_context_bad.cpp new file mode 100644 index 000000000..e6e33decd --- /dev/null +++ b/test_regress/t/t_wrapper_del_context_bad.cpp @@ -0,0 +1,22 @@ +// +// DESCRIPTION: Verilator: Verilog Multiple Model Test Module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +#include + +#include VM_PREFIX_INCLUDE + +int main(int argc, char** argv, char** env) { + // Create contexts + VerilatedContext* contextp{new VerilatedContext}; + + delete contextp; // Test mistake - deleting contextp + + // instantiate verilated design + std::unique_ptr topp{new VM_PREFIX{contextp, "TOP"}}; + + return 0; +} diff --git a/test_regress/t/t_wrapper_del_context_bad.out b/test_regress/t/t_wrapper_del_context_bad.out new file mode 100644 index 000000000..6344c8d00 --- /dev/null +++ b/test_regress/t/t_wrapper_del_context_bad.out @@ -0,0 +1,2 @@ +%Error: Attempt to create model using a bad/deleted VerilatedContext pointer +Aborting... diff --git a/test_regress/t/t_wrapper_del_context_bad.pl b/test_regress/t/t_wrapper_del_context_bad.pl new file mode 100755 index 000000000..82a4ea9cf --- /dev/null +++ b/test_regress/t/t_wrapper_del_context_bad.pl @@ -0,0 +1,25 @@ +#!/usr/bin/env perl +if (!$::Driver) { use strict; use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Multiple Model Test Module +# +# Copyright 2020-2021 by Andreas Kuster. 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 + +scenarios(vlt => 1); + +compile( + make_top_shell => 0, + make_main => 0, + verilator_flags2 => ["--exe $Self->{t_dir}/$Self->{name}.cpp"], + ); + +execute( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_wrapper_del_context_bad.v b/test_regress/t/t_wrapper_del_context_bad.v new file mode 100644 index 000000000..f4ae87f02 --- /dev/null +++ b/test_regress/t/t_wrapper_del_context_bad.v @@ -0,0 +1,9 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module top; + initial $finish; +endmodule