From 018d994781fe41187306a04968214350e565ef11 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Thu, 4 Mar 2021 19:23:40 -0500 Subject: [PATCH] Convert VPI to singleton, part of (#2660). --- include/verilated.cpp | 21 ++++++++++++++ include/verilated_threads.cpp | 5 ++++ include/verilated_threads.h | 3 +- include/verilated_vpi.cpp | 54 ++++++++++++++++++++--------------- include/verilatedos.h | 4 ++- 5 files changed, 62 insertions(+), 25 deletions(-) diff --git a/include/verilated.cpp b/include/verilated.cpp index 6ecb1bf8f..34f715952 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -21,6 +21,27 @@ /// Code available from: https://verilator.org /// //========================================================================= +// Internal note: +// +// verilated.o may exist both in protect-lib (incrementally linked .a/.so) +// and the main module. Both refer the same instance of static +// variables/VL_THREAD_LOCAL in verilated.o such as Verilated, or +// VerilatedImpData. This is important to share that state, but the +// sharing may cause a double-free error when shutting down because the +// loader will insert a constructor/destructor at each reference to +// verilated.o, resulting in at runtime constructors/destructors being +// called multiple times. +// +// To avoid the trouble: +// * Statics declared inside functions. The compiler will wrap +// the construction in must-be-one-time checks. +// * Or, use only C++20 constinit types. (TODO: Make a VL_CONSTINIT). +// * Or, use types that are multi-constructor safe. +// * Or, the static should be of a union, which will avoid compiler +// construction, and appropriately check for duplicate construction. +// * Or, code is not linked in protected library. e.g. the VPI +// and DPI libraries are not needed there. +//========================================================================= #define VERILATOR_VERILATED_CPP_ diff --git a/include/verilated_threads.cpp b/include/verilated_threads.cpp index 54f029d11..580179bac 100644 --- a/include/verilated_threads.cpp +++ b/include/verilated_threads.cpp @@ -19,6 +19,11 @@ #include +//============================================================================= +// Globals + +// Internal note: Globals may multi-construct, see verilated.cpp top. + std::atomic VlMTaskVertex::s_yields; VL_THREAD_LOCAL VlThreadPool::ProfileTrace* VlThreadPool::t_profilep = nullptr; diff --git a/include/verilated_threads.h b/include/verilated_threads.h index 0a04ae380..f0b88bbd5 100644 --- a/include/verilated_threads.h +++ b/include/verilated_threads.h @@ -189,7 +189,7 @@ private: VerilatedMutex m_mutex; std::condition_variable_any m_cv; // Only notify the condition_variable if the worker is waiting - bool m_waiting VL_GUARDED_BY(m_mutex); + bool m_waiting VL_GUARDED_BY(m_mutex) = false; // Why a vector? We expect the pending list to be very short, typically // 0 or 1 or 2, so popping from the front shouldn't be @@ -262,6 +262,7 @@ class VlThreadPool final { // corrupting the profiling data. It's super cheap to append // a VlProfileRec struct on the end of a pre-allocated vector; // this is the only cost we pay in real-time during a profiling cycle. + // Internal note: Globals may multi-construct, see verilated.cpp top. static VL_THREAD_LOCAL ProfileTrace* t_profilep; ProfileSet m_allProfiles VL_GUARDED_BY(m_mutex); VerilatedMutex m_mutex; diff --git a/include/verilated_vpi.cpp b/include/verilated_vpi.cpp index 47de95749..7b25337e6 100644 --- a/include/verilated_vpi.cpp +++ b/include/verilated_vpi.cpp @@ -62,6 +62,7 @@ class VerilatedVpio VL_NOT_FINAL { static constexpr vluint32_t activeMagic() { return 0xfeed100f; } // MEM MANGLEMENT + // Internal note: Globals may multi-construct, see verilated.cpp top. static VL_THREAD_LOCAL vluint8_t* t_freeHead; public: @@ -504,17 +505,21 @@ class VerilatedVpiImp final { typedef std::list VpioCbList; typedef std::map, VerilatedVpiCbHolder> VpioTimedCbs; + // All only medium-speed, so use singleton function VpioCbList m_cbObjLists[CB_ENUM_MAX_VALUE]; // Callbacks for each supported reason VpioTimedCbs m_timedCbs; // Time based callbacks VerilatedVpiError* m_errorInfop = nullptr; // Container for vpi error info VerilatedAssertOneThread m_assertOne; ///< Assert only called from single thread vluint64_t m_nextCallbackId = 1; // Id to identify callback - static VerilatedVpiImp s_s; // Singleton + static VerilatedVpiImp& s() { // Singleton + static VerilatedVpiImp s_s; + return s_s; + } public: - static void assertOneCheck() { s_s.m_assertOne.check(); } - static vluint64_t nextCallbackId() { return ++s_s.m_nextCallbackId; } + static void assertOneCheck() { s().m_assertOne.check(); } + static vluint64_t nextCallbackId() { return ++s().m_nextCallbackId; } static void cbReasonAdd(vluint64_t id, const s_cb_data* cb_data_p) { // The passed cb_data_p was property of the user, so need to recreate @@ -525,20 +530,20 @@ public: cb_data_p->reason, id, cb_data_p->obj);); VerilatedVpioVar* varop = nullptr; if (cb_data_p->reason == cbValueChange) varop = VerilatedVpioVar::castp(cb_data_p->obj); - s_s.m_cbObjLists[cb_data_p->reason].emplace_back(id, cb_data_p, varop); + s().m_cbObjLists[cb_data_p->reason].emplace_back(id, cb_data_p, varop); } static void cbTimedAdd(vluint64_t id, const s_cb_data* cb_data_p, QData time) { // The passed cb_data_p was property of the user, so need to recreate VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: vpi_register_cb reason=%d id=%" VL_PRI64 "d delay=%" VL_PRI64 "u\n", cb_data_p->reason, id, time);); - s_s.m_timedCbs.emplace(std::piecewise_construct, + s().m_timedCbs.emplace(std::piecewise_construct, std::forward_as_tuple(std::make_pair(time, id)), std::forward_as_tuple(id, cb_data_p, nullptr)); } static void cbReasonRemove(vluint64_t id, vluint32_t reason) { // Id might no longer exist, if already removed due to call after event, or teardown - VpioCbList& cbObjList = s_s.m_cbObjLists[reason]; + VpioCbList& cbObjList = s().m_cbObjLists[reason]; // We do not remove it now as we may be iterating the list, // instead set to nullptr and will cleanup later for (auto& ir : cbObjList) { @@ -547,13 +552,13 @@ public: } static void cbTimedRemove(vluint64_t id, QData time) { // Id might no longer exist, if already removed due to call after event, or teardown - const auto it = s_s.m_timedCbs.find(std::make_pair(time, id)); - if (VL_LIKELY(it != s_s.m_timedCbs.end())) it->second.invalidate(); + const auto it = s().m_timedCbs.find(std::make_pair(time, id)); + if (VL_LIKELY(it != s().m_timedCbs.end())) it->second.invalidate(); } static void callTimedCbs() VL_MT_UNSAFE_ONE { assertOneCheck(); QData time = VL_TIME_Q(); - for (auto it = s_s.m_timedCbs.begin(); it != s_s.m_timedCbs.end();) { + for (auto it = s().m_timedCbs.begin(); it != s().m_timedCbs.end();) { if (VL_UNLIKELY(it->first.first <= time)) { VerilatedVpiCbHolder& ho = it->second; const auto last_it = it; @@ -564,19 +569,19 @@ public: ho.invalidate(); // Timed callbacks are one-shot (ho.cb_rtnp())(ho.cb_datap()); } - s_s.m_timedCbs.erase(last_it); + s().m_timedCbs.erase(last_it); } else { ++it; } } } static QData cbNextDeadline() { - const auto it = s_s.m_timedCbs.cbegin(); - if (VL_LIKELY(it != s_s.m_timedCbs.cend())) return it->first.first; + const auto it = s().m_timedCbs.cbegin(); + if (VL_LIKELY(it != s().m_timedCbs.cend())) return it->first.first; return ~0ULL; // maxquad } static bool callCbs(vluint32_t reason) VL_MT_UNSAFE_ONE { - VpioCbList& cbObjList = s_s.m_cbObjLists[reason]; + VpioCbList& cbObjList = s().m_cbObjLists[reason]; bool called = false; if (cbObjList.empty()) return called; const auto last = std::prev(cbObjList.end()); // prevent looping over newly added elements @@ -600,7 +605,7 @@ public: } static bool callValueCbs() VL_MT_UNSAFE_ONE { assertOneCheck(); - VpioCbList& cbObjList = s_s.m_cbObjLists[cbValueChange]; + VpioCbList& cbObjList = s().m_cbObjLists[cbValueChange]; bool called = false; typedef std::unordered_set VpioVarSet; VpioVarSet update; // set of objects to update after callbacks @@ -640,9 +645,17 @@ public: static VerilatedVpiError* error_info() VL_MT_UNSAFE_ONE; // getter for vpi error info }; -class VerilatedVpiError final { - //// Container for vpi error info +//====================================================================== +// Statics +// Internal note: Globals may multi-construct, see verilated.cpp top. +VL_THREAD_LOCAL vluint8_t* VerilatedVpio::t_freeHead = nullptr; + +//====================================================================== +// VerilatedVpiError +/// Internal container for vpi error info + +class VerilatedVpiError final { t_vpi_error_info m_errorInfo; bool m_flag = false; char m_buff[VL_VPI_LINE_SIZE_]; @@ -708,11 +721,6 @@ public: static const char* strFromVpiProp(PLI_INT32 vpiVal) VL_MT_SAFE; }; -//====================================================================== - -VerilatedVpiImp VerilatedVpiImp::s_s; // Singleton -VL_THREAD_LOCAL vluint8_t* VerilatedVpio::t_freeHead = nullptr; - //====================================================================== // VerilatedVpi implementation @@ -742,8 +750,8 @@ PLI_INT32 VerilatedVpioReasonCb::dovpi_remove_cb() { VerilatedVpiError* VerilatedVpiImp::error_info() VL_MT_UNSAFE_ONE { VerilatedVpiImp::assertOneCheck(); - if (VL_UNLIKELY(!s_s.m_errorInfop)) s_s.m_errorInfop = new VerilatedVpiError(); - return s_s.m_errorInfop; + if (VL_UNLIKELY(!s().m_errorInfop)) s().m_errorInfop = new VerilatedVpiError(); + return s().m_errorInfop; } //====================================================================== diff --git a/include/verilatedos.h b/include/verilatedos.h index 5b803212c..55be76d6e 100644 --- a/include/verilatedos.h +++ b/include/verilatedos.h @@ -39,7 +39,9 @@ # define VL_ATTR_PRINTF(fmtArgNum) __attribute__((format(printf, (fmtArgNum), (fmtArgNum) + 1))) # define VL_ATTR_PURE __attribute__((pure)) # define VL_ATTR_UNUSED __attribute__((unused)) -# define VL_ATTR_WEAK __attribute__((weak)) +# if !defined(_WIN32) && !defined(__MINGW32__) +# define VL_ATTR_WEAK __attribute__((weak)) +# endif # define VL_FUNC __func__ # if defined(__clang__) && defined(VL_THREADED) # define VL_ACQUIRE(...) __attribute__((acquire_capability(__VA_ARGS__)))