From 9c2ead90d5d820a15557e3151e0d5111e34a698e Mon Sep 17 00:00:00 2001 From: Krzysztof Bieganski Date: Thu, 29 Sep 2022 00:54:18 +0200 Subject: [PATCH] Add custom memory management for verilated classes (#3595) This change introduces a custom reference-counting pointer class that allows creating such pointers from 'this'. This lets us keep the receiver object around even if all references to it outside of a class method no longer exist. Useful for coroutine methods, which may outlive all external references to the object. The deletion of objects is deferred until the next time slot. This is to make clearing the triggered flag on named events in classes safe (otherwise freed memory could be accessed). --- include/verilated.cpp | 15 +++ include/verilated_types.h | 154 +++++++++++++++++++++++++- src/V3EmitCFunc.h | 4 +- src/V3EmitCHeaders.cpp | 4 +- src/V3EmitCImp.cpp | 2 + src/V3EmitCModel.cpp | 1 + src/V3EmitCSyms.cpp | 1 + src/V3Global.h | 3 + src/V3SchedTiming.cpp | 1 + src/V3Timing.cpp | 2 + src/verilog.y | 3 +- test_regress/t/t_class_member_sens.pl | 22 ++++ test_regress/t/t_class_member_sens.v | 30 +++++ test_regress/t/t_timing_class.v | 8 +- 14 files changed, 242 insertions(+), 8 deletions(-) create mode 100755 test_regress/t/t_class_member_sens.pl create mode 100644 test_regress/t/t_class_member_sens.v diff --git a/include/verilated.cpp b/include/verilated.cpp index 1b5dab84b..4370a5b75 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -3095,3 +3095,18 @@ void VerilatedAssertOneThread::fatal_different() VL_MT_SAFE { #endif //=========================================================================== +// VlDeleter:: Methods + +void VlDeleter::deleteAll() { + while (true) { + VerilatedLockGuard lock{m_mutex}; + if (m_newGarbage.empty()) break; + VerilatedLockGuard deleteLock{m_deleteMutex}; + std::swap(m_newGarbage, m_toDelete); + lock.unlock(); // So destuctors can enqueue new objects + for (VlClass* const objp : m_toDelete) delete objp; + m_toDelete.clear(); + } +} + +//=========================================================================== diff --git a/include/verilated_types.h b/include/verilated_types.h index bdcc0cdad..b39b3e2ae 100644 --- a/include/verilated_types.h +++ b/include/verilated_types.h @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -1003,12 +1004,161 @@ std::string VL_TO_STRING(const VlUnpacked& obj) { return obj.to_string(); } +class VlClass; // See below + +//=================================================================== +// Class providing delayed deletion of garbage objects. Objects get deleted only when 'deleteAll()' +// is called, or the deleter itself is destroyed. + +class VlDeleter final { + // MEMBERS + // Queue of new objects that should be deleted + std::vector m_newGarbage VL_GUARDED_BY(m_mutex); + // Queue of objects currently being deleted (only for deleteAll()) + std::vector m_toDelete VL_GUARDED_BY(m_deleteMutex); + mutable VerilatedMutex m_mutex; // Mutex protecting the 'new garbage' queue + mutable VerilatedMutex m_deleteMutex; // Mutex protecting the delete queue + +public: + // CONSTRUCTOR + VlDeleter() = default; + ~VlDeleter() { deleteAll(); } + +private: + VL_UNCOPYABLE(VlDeleter); + +public: + // METHODS + // Adds a new object to the 'new garbage' queue. + void put(VlClass* const objp) VL_MT_SAFE { + const VerilatedLockGuard lock{m_mutex}; + m_newGarbage.push_back(objp); + } + + // Deletes all queued garbage objects. + void deleteAll() VL_MT_SAFE; +}; + +//=================================================================== +// Base class for all verilated classes. Includes a reference counter, and a pointer to the deleter +// object that should destroy it after the counter reaches 0. This allows for easy construction of +// VlClassRefs from 'this'. Also declares a virtual constructor, so that the object can be deleted +// using a base pointer. + +class VlClass VL_NOT_FINAL { + // TYPES + template + friend class VlClassRef; // Needed for access to the ref counter and deleter + + // MEMBERS + std::atomic m_counter{0}; // Reference count for this object + VlDeleter* m_deleter = nullptr; // The deleter that will delete this object + + // METHODS + // Atomically increments the reference counter + void refCountInc() VL_MT_SAFE { ++m_counter; } + // Atomically decrements the reference counter. Assuming VlClassRef semantics are sound, it + // should never get called at m_counter == 0. + void refCountDec() VL_MT_SAFE { + if (!--m_counter) m_deleter->put(this); + } + +public: + // CONSTRUCTORS + VlClass() = default; + VlClass(const VlClass& copied) {} + virtual ~VlClass() {} +}; + //=================================================================== // Verilog class reference container // There are no multithreaded locks on this; the base variable must // be protected by other means -#define VlClassRef std::shared_ptr +template +class VlClassRef final { +private: + // TYPES + template + friend class VlClassRef; // Needed for template copy/move assignments + + // MEMBERS + T_Class* m_objp = nullptr; // Object pointed to + + // METHODS + // Increase reference counter with null check + void refCountInc() const VL_MT_SAFE { + if (m_objp) m_objp->refCountInc(); + } + // Decrease reference counter with null check + void refCountDec() const VL_MT_SAFE { + if (m_objp) m_objp->refCountDec(); + } + +public: + // CONSTRUCTORS + VlClassRef() = default; + template + VlClassRef(VlDeleter& deleter, T_Args&&... args) + : m_objp{new T_Class{std::forward(args)...}} { + m_objp->m_deleter = &deleter; + refCountInc(); + } + VlClassRef(T_Class* objp) + : m_objp{objp} { + refCountInc(); + } + VlClassRef(const VlClassRef& copied) + : m_objp{copied.m_objp} { + refCountInc(); + } + VlClassRef(VlClassRef&& moved) + : m_objp{std::exchange(moved.m_objp, nullptr)} {} + ~VlClassRef() { refCountDec(); } + + // METHODS + // Copy and move assignments + VlClassRef& operator=(const VlClassRef& copied) { + refCountDec(); + m_objp = copied.m_objp; + refCountInc(); + return *this; + } + VlClassRef& operator=(VlClassRef&& moved) { + refCountDec(); + m_objp = std::exchange(moved.m_objp, nullptr); + return *this; + } + template + VlClassRef& operator=(const VlClassRef& copied) { + refCountDec(); + m_objp = copied.m_objp; + refCountInc(); + return *this; + } + template + VlClassRef& operator=(VlClassRef&& moved) { + refCountDec(); + m_objp = std::exchange(moved.m_objp, nullptr); + return *this; + } + // Dynamic caster + template + VlClassRef dynamicCast() const { + return dynamic_cast(m_objp); + } + // Dereference operators + T_Class& operator*() const { return *m_objp; } + T_Class* operator->() const { return m_objp; } + // For 'if (ptr)...' + operator bool() const { return m_objp; } +}; + +#define VL_NEW(Class, ...) \ + VlClassRef { vlSymsp->__Vm_deleter, __VA_ARGS__ } + +#define VL_KEEP_THIS \ + VlClassRef::type> __Vthisref { this } template // T typically of type VlClassRef inline T VL_NULL_CHECK(T t, const char* filename, int linenum) { @@ -1018,7 +1168,7 @@ inline T VL_NULL_CHECK(T t, const char* filename, int linenum) { template static inline bool VL_CAST_DYNAMIC(VlClassRef in, VlClassRef& outr) { - VlClassRef casted = std::dynamic_pointer_cast(in); + VlClassRef casted = in.template dynamicCast(); if (VL_LIKELY(casted)) { outr = casted; return true; diff --git a/src/V3EmitCFunc.h b/src/V3EmitCFunc.h index e9b63f591..8369386bd 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -419,7 +419,7 @@ public: } void visit(AstCNew* nodep) override { bool comma = false; - puts("std::make_shared<" + prefixNameProtect(nodep->dtypep()) + ">("); + puts("VL_NEW(" + prefixNameProtect(nodep->dtypep()) + ", "); puts("vlSymsp"); // TODO make this part of argsp, and eliminate when unnecessary if (nodep->argsp()) comma = true; for (AstNode* subnodep = nodep->argsp(); subnodep; subnodep = subnodep->nextp()) { @@ -1054,7 +1054,7 @@ public: puts(")"); } void visit(AstNewCopy* nodep) override { - puts("std::make_shared<" + prefixNameProtect(nodep->dtypep()) + ">("); + puts("VL_NEW(" + prefixNameProtect(nodep->dtypep()) + ", "); puts("*"); // i.e. make into a reference iterateAndNextNull(nodep->rhsp()); puts(")"); diff --git a/src/V3EmitCHeaders.cpp b/src/V3EmitCHeaders.cpp index f03598b58..d9197cd0c 100644 --- a/src/V3EmitCHeaders.cpp +++ b/src/V3EmitCHeaders.cpp @@ -256,9 +256,11 @@ class EmitCHeader final : public EmitCConstInit { puts("\nclass "); puts(prefixNameProtect(modp)); if (const AstClass* const classp = VN_CAST(modp, Class)) { + puts(" : public "); if (classp->extendsp()) { - puts(" : public "); puts(prefixNameProtect(classp->extendsp()->classp())); + } else { + puts("VlClass"); } } else { puts(" final : public VerilatedModule"); diff --git a/src/V3EmitCImp.cpp b/src/V3EmitCImp.cpp index 5e9c3367f..0c0cdd379 100644 --- a/src/V3EmitCImp.cpp +++ b/src/V3EmitCImp.cpp @@ -75,6 +75,7 @@ class EmitCGatherDependencies final : VNVisitor { iterateChildrenConst(nodep); } void visit(AstCNew* nodep) override { + addSymsDependency(); addDTypeDependency(nodep->dtypep()); iterateChildrenConst(nodep); } @@ -83,6 +84,7 @@ class EmitCGatherDependencies final : VNVisitor { iterateChildrenConst(nodep); } void visit(AstNewCopy* nodep) override { + addSymsDependency(); addDTypeDependency(nodep->dtypep()); iterateChildrenConst(nodep); } diff --git a/src/V3EmitCModel.cpp b/src/V3EmitCModel.cpp index 5430337d2..0be57f9c9 100644 --- a/src/V3EmitCModel.cpp +++ b/src/V3EmitCModel.cpp @@ -387,6 +387,7 @@ class EmitCModel final : public EmitCFunc { if (v3Global.opt.trace()) puts("vlSymsp->__Vm_activity = true;\n"); if (v3Global.hasEvents()) puts("vlSymsp->clearTriggeredEvents();\n"); + if (v3Global.hasClasses()) puts("vlSymsp->__Vm_deleter.deleteAll();\n"); puts("if (VL_UNLIKELY(!vlSymsp->__Vm_didInit)) {\n"); puts("vlSymsp->__Vm_didInit = true;\n"); diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index 62f2ba8b4..752fe77c6 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -446,6 +446,7 @@ void EmitCSyms::emitSymHdr() { " ///< Used by trace routines when tracing multiple models\n"); } if (v3Global.hasEvents()) puts("std::vector __Vm_triggeredEvents;\n"); + if (v3Global.hasClasses()) puts("VlDeleter __Vm_deleter;\n"); puts("bool __Vm_didInit = false;\n"); if (v3Global.opt.mtasks()) { diff --git a/src/V3Global.h b/src/V3Global.h index 05f290c5f..0f1baaa3b 100644 --- a/src/V3Global.h +++ b/src/V3Global.h @@ -106,6 +106,7 @@ class V3Global final { bool m_needTraceDumper = false; // Need __Vm_dumperp in symbols bool m_dpi = false; // Need __Dpi include files bool m_hasEvents = false; // Design uses SystemVerilog named events + bool m_hasClasses = false; // Design uses SystemVerilog classes bool m_usesTiming = false; // Design uses timing constructs bool m_hasForceableSignals = false; // Need to apply V3Force pass bool m_hasSCTextSections = false; // Has `systemc_* sections that need to be emitted @@ -149,6 +150,8 @@ public: void dpi(bool flag) { m_dpi = flag; } bool hasEvents() const { return m_hasEvents; } void setHasEvents() { m_hasEvents = true; } + bool hasClasses() const { return m_hasClasses; } + void setHasClasses() { m_hasClasses = true; } bool usesTiming() const { return m_usesTiming; } void setUsesTiming() { m_usesTiming = true; } bool hasForceableSignals() const { return m_hasForceableSignals; } diff --git a/src/V3SchedTiming.cpp b/src/V3SchedTiming.cpp index cc426ddd2..5423ec629 100644 --- a/src/V3SchedTiming.cpp +++ b/src/V3SchedTiming.cpp @@ -347,6 +347,7 @@ void transformForks(AstNetlist* const netlistp) { nodep->replaceWith(callp); // If we're in a class, add a vlSymsp arg if (m_inClass) { + newfuncp->addInitsp(new AstCStmt{nodep->fileline(), "VL_KEEP_THIS;\n"}); newfuncp->argTypes(EmitCBaseVisitor::symClassVar()); callp->argTypes("vlSymsp"); } diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index 8e5661ef9..5ebb7ea7b 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -393,6 +393,8 @@ private: } if (nodep->user2() && !nodep->isCoroutine()) { // If first marked as suspendable nodep->rtnType("VlCoroutine"); + // If in a class, create a shared pointer to 'this' + if (m_classp) nodep->addInitsp(new AstCStmt{nodep->fileline(), "VL_KEEP_THIS;\n"}); // Revisit dependent nodes if needed for (V3GraphEdge* edgep = vxp->inBeginp(); edgep; edgep = edgep->inNextp()) { auto* const depVxp = static_cast(edgep->fromp()); diff --git a/src/verilog.y b/src/verilog.y index f98715141..86ecb7604 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -6202,7 +6202,8 @@ classFront: // IEEE: part of class_declaration { $$ = new AstClass($2, *$4); $$->isVirtual($1); $$->lifetime($3); - SYMP->pushNew($$); } + SYMP->pushNew($$); + v3Global.setHasClasses(); } // // IEEE: part of interface_class_declaration | yINTERFACE yCLASS lifetimeE idAny/*class_identifier*/ { $$ = new AstClass($2, *$4); diff --git a/test_regress/t/t_class_member_sens.pl b/test_regress/t/t_class_member_sens.pl new file mode 100755 index 000000000..cf6d969a8 --- /dev/null +++ b/test_regress/t/t_class_member_sens.pl @@ -0,0 +1,22 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 by Antmicro Ltd. 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( + sanitize => 1, + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_class_member_sens.v b/test_regress/t/t_class_member_sens.v new file mode 100644 index 000000000..f178f995b --- /dev/null +++ b/test_regress/t/t_class_member_sens.v @@ -0,0 +1,30 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + class EventClass; + event e; + endclass + + EventClass ec = new; + int cyc = 0; + + always @ec.e ec = new; + + always @(posedge clk) begin + cyc <= cyc + 1; + if (cyc == 1) ->ec.e; + else if (cyc == 2) begin + $write("*-* All Finished *-*\n"); + $finish; + end + end +endmodule diff --git a/test_regress/t/t_timing_class.v b/test_regress/t/t_timing_class.v index 4b8480ced..b79139153 100644 --- a/test_regress/t/t_timing_class.v +++ b/test_regress/t/t_timing_class.v @@ -84,6 +84,7 @@ module t; logic y; task do_assign; y = #10 x; + `WRITE_VERBOSE(("Did assignment with delay\n")); endtask endclass @@ -121,7 +122,10 @@ module t; if ($time != 80) $stop; if (event_trig_count != 2) $stop; if (dAsgn.y != 1) $stop; - $write("*-* All Finished *-*\n"); + // Test if the object is deleted before do_assign finishes: + fork dAsgn.do_assign; join_none + #5 dAsgn = null; + #15 $write("*-* All Finished *-*\n"); $finish; end @@ -162,5 +166,5 @@ module t; if (fc.done != 4 || $time != 70) $stop; end - initial #81 $stop; // timeout + initial #101 $stop; // timeout endmodule