From 1bb6433649d205ad19ce277452ddfed99787fe0d Mon Sep 17 00:00:00 2001
From: Geza Lore <gezalore@gmail.com>
Date: Mon, 27 Jun 2022 14:16:20 +0100
Subject: [PATCH] Improve worker thread shutdown.

Always ensure worker thread task queue is drained before shutting down.
---
 include/verilated_threads.cpp | 21 ++++++++-------------
 include/verilated_threads.h   |  6 ++++--
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/include/verilated_threads.cpp b/include/verilated_threads.cpp
index dbdd3330a..a78ea9ae6 100644
--- a/include/verilated_threads.cpp
+++ b/include/verilated_threads.cpp
@@ -50,31 +50,26 @@ VlMTaskVertex::VlMTaskVertex(uint32_t upstreamDepCount)
 VlWorkerThread::VlWorkerThread(uint32_t threadId, VerilatedContext* contextp,
                                VlExecutionProfiler* profilerp, VlStartWorkerCb startCb)
     : m_ready_size{0}
-    , m_exiting{false}
     , m_cthread{startWorker, this, threadId, profilerp, startCb}
     , m_contextp{contextp} {}
 
 VlWorkerThread::~VlWorkerThread() {
-    m_exiting.store(true, std::memory_order_release);
-    wakeUp();
+    shutdown();
     // The thread should exit; join it.
     m_cthread.join();
 }
 
+void VlWorkerThread::shutdownTask(void*, bool) {
+    // Deliberately empty, we use the address of this function as a magic number
+}
+
 void VlWorkerThread::workerLoop() {
     ExecRec work;
-    work.m_fnp = nullptr;
 
     while (true) {
-        if (VL_LIKELY(!work.m_fnp)) dequeWork(&work);
-
-        // Do this here, not above, to avoid a race with the destructor.
-        if (VL_UNLIKELY(m_exiting.load(std::memory_order_acquire))) break;
-
-        if (VL_LIKELY(work.m_fnp)) {
-            work.m_fnp(work.m_selfp, work.m_evenCycle);
-            work.m_fnp = nullptr;
-        }
+        dequeWork(&work);
+        if (VL_UNLIKELY(work.m_fnp == shutdownTask)) break;
+        work.m_fnp(work.m_selfp, work.m_evenCycle);
     }
 }
 
diff --git a/include/verilated_threads.h b/include/verilated_threads.h
index 805a73d2d..eeb8f9342 100644
--- a/include/verilated_threads.h
+++ b/include/verilated_threads.h
@@ -165,7 +165,6 @@ private:
     // Store the size atomically, so we can spin wait
     std::atomic<size_t> m_ready_size;
 
-    std::atomic<bool> m_exiting;  // Worker thread should exit
     std::thread m_cthread;  // Underlying C++ thread record
     VerilatedContext* const m_contextp;  // Context for spawned thread
 
@@ -198,7 +197,6 @@ public:
         m_ready.erase(m_ready.begin());
         m_ready_size.fetch_sub(1, std::memory_order_relaxed);
     }
-    inline void wakeUp() { addTask(nullptr, nullptr, false); }
     inline void addTask(VlExecFnp fnp, VlSelfP selfp, bool evenCycle)
         VL_MT_SAFE_EXCLUDES(m_mutex) {
         bool notify;
@@ -210,6 +208,10 @@ public:
         }
         if (notify) m_cv.notify_one();
     }
+
+    inline void shutdown() { addTask(shutdownTask, nullptr, false); }
+    static void shutdownTask(void*, bool);
+
     void workerLoop();
     static void startWorker(VlWorkerThread* workerp, uint32_t threadId,
                             VlExecutionProfiler* profilerp, VlStartWorkerCb startCb);