Close trace on vl_fatal/vl_finish (#2414)

This is required to get the last bit of FST trace and close the FST file
properly on $stop or assertion failure.

Changes

@@ -9,6 +9,8 @@ The contributors that suggested a given feature are shown in []. Thanks!
 
 ****  With --bbox-unsup continue parsing on many (not all) UVM constructs.
 
+****  Flush FST trace on termination due to $stop or assertion failure.
+
 
 * Verilator 4.036 2020-06-06
 

include/verilated.cpp

@@ -31,6 +31,8 @@
 #include <cerrno>
 #include <sstream>
 #include <sys/stat.h>  // mkdir
+#include <list>
+#include <utility>
 
 // clang-format off
 #if defined(_WIN32) || defined(__MINGW32__)
@@ -59,7 +61,6 @@ typedef union {
 
 // Slow path variables
 VerilatedMutex Verilated::m_mutex;
-VerilatedVoidCb Verilated::s_flushCb = NULL;
 
 // Keep below together in one cache line
 Verilated::Serialized Verilated::s_s;
@@ -83,7 +84,8 @@ void vl_finish(const char* filename, int linenum, const char* hier) VL_MT_UNSAFE
     if (Verilated::gotFinish()) {
         VL_PRINTF(  // Not VL_PRINTF_MT, already on main thread
             "- %s:%d: Second verilog $finish, exiting\n", filename, linenum);
-        Verilated::flushCall();
+        Verilated::runFlushCallbacks();
+        Verilated::runExitCallbacks();
         exit(0);
     }
     Verilated::gotFinish(true);
@@ -93,7 +95,7 @@ void vl_finish(const char* filename, int linenum, const char* hier) VL_MT_UNSAFE
 #ifndef VL_USER_STOP  ///< Define this to override this function
 void vl_stop(const char* filename, int linenum, const char* hier) VL_MT_UNSAFE {
     Verilated::gotFinish(true);
-    Verilated::flushCall();
+    Verilated::runFlushCallbacks();
     vl_fatal(filename, linenum, hier, "Verilog $stop");
 }
 #endif
@@ -108,10 +110,15 @@ void vl_fatal(const char* filename, int linenum, const char* hier, const char* m
     } else {
         VL_PRINTF("%%Error: %s\n", msg);
     }
-    Verilated::flushCall();
+    Verilated::runFlushCallbacks();
 
     VL_PRINTF("Aborting...\n");  // Not VL_PRINTF_MT, already on main thread
-    Verilated::flushCall();  // Second flush in case VL_PRINTF does something needing a flush
+
+    // Second flush in case VL_PRINTF does something needing a flush
+    Verilated::runFlushCallbacks();
+
+    // Callbacks prior to termination
+    Verilated::runExitCallbacks();
     abort();
 }
 #endif
@@ -2242,29 +2249,60 @@ const char* Verilated::catName(const char* n1, const char* n2, const char* delim
     return strp;
 }
 
-void Verilated::flushCb(VerilatedVoidCb cb) VL_MT_SAFE {
-    const VerilatedLockGuard lock(m_mutex);
-    if (s_flushCb == cb) {  // Ok - don't duplicate
-    } else if (!s_flushCb) {
-        s_flushCb = cb;
-    } else {  // LCOV_EXCL_LINE
-        // Someday we may allow multiple callbacks ala atexit(), but until then
-        VL_FATAL_MT("unknown", 0, "",  // LCOV_EXCL_LINE
-                    "Verilated::flushCb called twice with different callbacks");
-    }
+//=========================================================================
+// Flush and exit callbacks
+
+// Keeping these out of class Verilated to avoid having to include <list>
+// in verilated.h (for compilation speed)
+typedef std::list<std::pair<Verilated::VoidPCb, void*> > VoidPCbList;
+static VoidPCbList g_flushCbs;
+static VoidPCbList g_exitCbs;
+
+static void addCb(Verilated::VoidPCb cb, void* datap, VoidPCbList& cbs) {
+    std::pair<Verilated::VoidPCb, void*> pair(cb, datap);
+    cbs.remove(pair);  // Just in case it's a duplicate
+    cbs.push_back(pair);
+}
+static void removeCb(Verilated::VoidPCb cb, void* datap, VoidPCbList& cbs) {
+    std::pair<Verilated::VoidPCb, void*> pair(cb, datap);
+    cbs.remove(pair);
+}
+static void runCallbacks(VoidPCbList& cbs) VL_MT_SAFE {
+    for (VoidPCbList::iterator it = cbs.begin(); it != cbs.end(); ++it) { it->first(it->second); }
 }
 
-// When running internal code coverage (gcc --coverage, as opposed to
-// verilator --coverage), dump coverage data to properly cover failing
-// tests.
-void Verilated::flushCall() VL_MT_SAFE {
+void Verilated::addFlushCb(VoidPCb cb, void* datap) VL_MT_SAFE {
     const VerilatedLockGuard lock(m_mutex);
-    if (s_flushCb) (*s_flushCb)();
+    addCb(cb, datap, g_flushCbs);
+}
+void Verilated::removeFlushCb(VoidPCb cb, void* datap) VL_MT_SAFE {
+    const VerilatedLockGuard lock(m_mutex);
+    removeCb(cb, datap, g_flushCbs);
+}
+void Verilated::runFlushCallbacks() VL_MT_SAFE {
+    const VerilatedLockGuard lock(m_mutex);
+    runCallbacks(g_flushCbs);
     fflush(stderr);
     fflush(stdout);
+    // When running internal code coverage (gcc --coverage, as opposed to
+    // verilator --coverage), dump coverage data to properly cover failing
+    // tests.
     VL_GCOV_FLUSH();
 }
 
+void Verilated::addExitCb(VoidPCb cb, void* datap) VL_MT_SAFE {
+    const VerilatedLockGuard lock(m_mutex);
+    addCb(cb, datap, g_exitCbs);
+}
+void Verilated::removeExitCb(VoidPCb cb, void* datap) VL_MT_SAFE {
+    const VerilatedLockGuard lock(m_mutex);
+    removeCb(cb, datap, g_exitCbs);
+}
+void Verilated::runExitCallbacks() VL_MT_SAFE {
+    const VerilatedLockGuard lock(m_mutex);
+    runCallbacks(g_exitCbs);
+}
+
 const char* Verilated::productName() VL_PURE { return VERILATOR_PRODUCT; }
 const char* Verilated::productVersion() VL_PURE { return VERILATOR_VERSION; }
 

include/verilated.h

@@ -84,10 +84,6 @@ typedef EData        WData;     ///< Verilated pack data, >64 bits, as an array
 typedef const WData* WDataInP;  ///< Array input to a function
 typedef WData* WDataOutP;  ///< Array output from a function
 
-typedef void (*VerilatedVoidCb)(void);
-
-class SpTraceVcd;
-class SpTraceVcdCFile;
 class VerilatedEvalMsgQueue;
 class VerilatedScopeNameMap;
 class VerilatedVar;
@@ -376,8 +372,6 @@ class Verilated {
     // Slow path variables
     static VerilatedMutex m_mutex;  ///< Mutex for s_s/s_ns members, when VL_THREADED
 
-    static VerilatedVoidCb s_flushCb;  ///< Flush callback function
-
     static struct Serialized {  // All these members serialized/deserialized
         // Fast path
         int s_debug;  ///< See accessors... only when VL_DEBUG set
@@ -501,9 +495,15 @@ public:
     static void profThreadsFilenamep(const char* flagp) VL_MT_SAFE;
     static const char* profThreadsFilenamep() VL_MT_SAFE { return s_ns.s_profThreadsFilenamep; }
 
-    /// Flush callback for VCD waves
-    static void flushCb(VerilatedVoidCb cb) VL_MT_SAFE;
-    static void flushCall() VL_MT_SAFE;
+    typedef void (*VoidPCb)(void*);  // Callback type for below
+    /// Callbacks to run on global flush
+    static void addFlushCb(VoidPCb cb, void* datap) VL_MT_SAFE;
+    static void removeFlushCb(VoidPCb cb, void* datap) VL_MT_SAFE;
+    static void runFlushCallbacks() VL_MT_SAFE;
+    /// Callbacks to run prior to termination
+    static void addExitCb(VoidPCb cb, void* datap) VL_MT_SAFE;
+    static void removeExitCb(VoidPCb cb, void* datap) VL_MT_SAFE;
+    static void runExitCallbacks() VL_MT_SAFE;
 
     /// Record command line arguments, for retrieval by $test$plusargs/$value$plusargs,
     /// and for parsing +verilator+ run-time arguments.

include/verilated_trace.h

@@ -158,6 +158,11 @@ private:
     // to access duck-typed functions to avoid a virtual function call.
     T_Derived* self() { return static_cast<T_Derived*>(this); }
 
+    // Flush any remaining data for this file
+    static void onFlush(void* selfp) VL_MT_UNSAFE_ONE;
+    // Close the file on termination
+    static void onExit(void* selfp) VL_MT_UNSAFE_ONE;
+
 #ifdef VL_TRACE_THREADED
     // Number of total trace buffers that have been allocated
     vluint32_t m_numTraceBuffers;

include/verilated_trace_imp.cpp

@@ -258,6 +258,19 @@ template <> void VerilatedTrace<VL_DERIVED_T>::flush() {
 #endif
 }
 
+//=============================================================================
+// Callbacks to run on global events
+
+template <> void VerilatedTrace<VL_DERIVED_T>::onFlush(void* selfp) {
+    // Note this calls 'flush' on the derived class
+    reinterpret_cast<VL_DERIVED_T*>(selfp)->flush();
+}
+
+template <> void VerilatedTrace<VL_DERIVED_T>::onExit(void* selfp) {
+    // Note this calls 'close' on the derived class
+    reinterpret_cast<VL_DERIVED_T*>(selfp)->close();
+}
+
 //=============================================================================
 // VerilatedTrace
 
@@ -282,6 +295,8 @@ VerilatedTrace<VL_DERIVED_T>::VerilatedTrace()
 
 template <> VerilatedTrace<VL_DERIVED_T>::~VerilatedTrace() {
     if (m_sigs_oldvalp) VL_DO_CLEAR(delete[] m_sigs_oldvalp, m_sigs_oldvalp = NULL);
+    Verilated::removeFlushCb(VerilatedTrace<VL_DERIVED_T>::onFlush, this);
+    Verilated::removeExitCb(VerilatedTrace<VL_DERIVED_T>::onExit, this);
 #ifdef VL_TRACE_THREADED
     close();
 #endif
@@ -318,6 +333,10 @@ template <> void VerilatedTrace<VL_DERIVED_T>::traceInit() VL_MT_UNSAFE {
     // holding previous signal values.
     if (!m_sigs_oldvalp) m_sigs_oldvalp = new vluint32_t[nextCode()];
 
+    // Set callback so flush/abort will flush this file
+    Verilated::addFlushCb(VerilatedTrace<VL_DERIVED_T>::onFlush, this);
+    Verilated::addExitCb(VerilatedTrace<VL_DERIVED_T>::onExit, this);
+
 #ifdef VL_TRACE_THREADED
     // Compute trace buffer size. we need to be able to store a new value for
     // each signal, which is 'nextCode()' entries after the init callbacks

include/verilated_vcd_c.cpp

@@ -66,47 +66,6 @@
 #include "verilated_trace_imp.cpp"
 #undef VL_DERIVED_T
 
-//=============================================================================
-// VerilatedVcdImp
-/// Base class to hold some static state
-/// This is an internally used class
-
-class VerilatedVcdSingleton {
-private:
-    typedef std::vector<VerilatedVcd*> VcdVec;
-    struct Singleton {
-        VerilatedMutex s_vcdMutex;  ///< Protect the singleton
-        VcdVec s_vcdVecp VL_GUARDED_BY(s_vcdMutex);  ///< List of all created traces
-    };
-    static Singleton& singleton() {
-        static Singleton s;
-        return s;
-    }
-
-public:
-    static void pushVcd(VerilatedVcd* vcdp) VL_EXCLUDES(singleton().s_vcdMutex) {
-        const VerilatedLockGuard lock(singleton().s_vcdMutex);
-        singleton().s_vcdVecp.push_back(vcdp);
-    }
-    static void removeVcd(const VerilatedVcd* vcdp) VL_EXCLUDES(singleton().s_vcdMutex) {
-        const VerilatedLockGuard lock(singleton().s_vcdMutex);
-        VcdVec::iterator pos
-            = find(singleton().s_vcdVecp.begin(), singleton().s_vcdVecp.end(), vcdp);
-        if (pos != singleton().s_vcdVecp.end()) { singleton().s_vcdVecp.erase(pos); }
-    }
-    static void flush_all() VL_EXCLUDES(singleton().s_vcdMutex) VL_MT_UNSAFE_ONE {
-        // Thread safety: Although this function is protected by a mutex so
-        // perhaps in the future we can allow tracing in separate threads,
-        // vcdp->flush() assumes call from single thread
-        const VerilatedLockGuard lock(singleton().s_vcdMutex);
-        for (VcdVec::const_iterator it = singleton().s_vcdVecp.begin();
-             it != singleton().s_vcdVecp.end(); ++it) {
-            VerilatedVcd* vcdp = *it;
-            vcdp->flush();
-        }
-    }
-};
-
 //=============================================================================
 //=============================================================================
 //=============================================================================
@@ -152,13 +111,7 @@ void VerilatedVcd::open(const char* filename) {
 
     // Set member variables
     m_filename = filename;  // "" is ok, as someone may overload open
-    VerilatedVcdSingleton::pushVcd(this);
 
-    // SPDIFF_OFF
-    // Set callback so an early exit will flush us
-    Verilated::flushCb(&flush_all);
-
-    // SPDIFF_ON
     openNext(m_rolloverMB != 0);
     if (!isOpen()) return;
 
@@ -266,7 +219,6 @@ VerilatedVcd::~VerilatedVcd() {
     if (m_wrBufp) VL_DO_CLEAR(delete[] m_wrBufp, m_wrBufp = NULL);
     deleteNameMap();
     if (m_filep && m_fileNewed) VL_DO_CLEAR(delete m_filep, m_filep = NULL);
-    VerilatedVcdSingleton::removeVcd(this);
 }
 
 void VerilatedVcd::closePrev() {
@@ -823,11 +775,6 @@ void VerilatedVcd::fullDouble(vluint32_t code, const double newval) {
 
 #endif  // VL_TRACE_VCD_OLD_API
 
-//======================================================================
-// Static members
-
-void VerilatedVcd::flush_all() VL_MT_UNSAFE_ONE { VerilatedVcdSingleton::flush_all(); }
-
 //======================================================================
 //======================================================================
 //======================================================================

include/verilated_vcd_c.h

@@ -105,9 +105,6 @@ private:
 
     void finishLine(vluint32_t code, char* writep);
 
-    /// Flush any remaining data from all files
-    static void flush_all() VL_MT_UNSAFE_ONE;
-
     // CONSTRUCTORS
     VL_UNCOPYABLE(VerilatedVcd);
 

include/verilated_vpi.cpp

@@ -2005,7 +2005,7 @@ PLI_INT32 vpi_mcd_vprintf(PLI_UINT32 mcd, PLI_BYTE8* format, va_list ap) {
 PLI_INT32 vpi_flush(void) {
     VerilatedVpiImp::assertOneCheck();
     _VL_VPI_ERROR_RESET();
-    Verilated::flushCall();
+    Verilated::runFlushCallbacks();
     return 0;
 }
 

src/V3EmitC.cpp

@@ -598,7 +598,7 @@ public:
     }
     virtual void visit(AstFFlush* nodep) VL_OVERRIDE {
         if (!nodep->filep()) {
-            puts("Verilated::flushCall();\n");
+            puts("Verilated::runFlushCallbacks();\n");
         } else {
             puts("if (");
             iterateAndNextNull(nodep->filep());

test_regress/t/t_trace_abort.out

@@ -0,0 +1,53 @@
+$version Generated by VerilatedVcd $end
+$date Wed Jun 10 17:27:15 2020
+ $end
+$timescale   1ps $end
+
+ $scope module top $end
+  $var wire  1 # clk $end
+  $scope module t $end
+   $var wire  1 # clk $end
+   $var wire  3 $ cyc [2:0] $end
+  $upscope $end
+ $upscope $end
+$enddefinitions $end
+
+
+#0
+0#
+b000 $
+#10
+1#
+b001 $
+#15
+0#
+#20
+1#
+b010 $
+#25
+0#
+#30
+1#
+b011 $
+#35
+0#
+#40
+1#
+b100 $
+#45
+0#
+#50
+1#
+b101 $
+#55
+0#
+#60
+1#
+b110 $
+#65
+0#
+#70
+1#
+b111 $
+#75
+0#

test_regress/t/t_trace_abort.pl

@@ -0,0 +1,24 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2020 by Geza Lore. 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_all => 1);
+
+compile(
+    verilator_flags2 => ['--cc --trace'],
+    );
+
+execute(
+    fails => 1,
+    );
+
+vcd_identical("$Self->{obj_dir}/simx.vcd", $Self->{golden_filename});
+
+ok(1);
+1;

test_regress/t/t_trace_abort.v

@@ -0,0 +1,21 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2020 by Geza Lore.
+// SPDX-License-Identifier: CC0-1.0
+
+module t (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+
+   input clk;
+   reg [2:0] cyc = 0;
+
+   always @(posedge clk) begin
+      cyc <= cyc + 3'd1;
+      // Exit via abort to make sure trace is flushed
+      if (&cyc) $stop;
+   end
+
+endmodule

test_regress/t/t_trace_abort_fst.out

@@ -0,0 +1,58 @@
+$date
+	Wed Jun 10 20:47:01 2020
+
+$end
+$version
+	fstWriter
+$end
+$timescale
+	1ps
+$end
+$scope module top $end
+$var wire 1 ! clk $end
+$scope module t $end
+$var wire 1 ! clk $end
+$var logic 3 " cyc $end
+$upscope $end
+$upscope $end
+$enddefinitions $end
+#0
+$dumpvars
+b000 "
+0!
+$end
+#10
+1!
+b001 "
+#15
+0!
+#20
+1!
+b010 "
+#25
+0!
+#30
+1!
+b011 "
+#35
+0!
+#40
+1!
+b100 "
+#45
+0!
+#50
+1!
+b101 "
+#55
+0!
+#60
+1!
+b110 "
+#65
+0!
+#70
+1!
+b111 "
+#75
+0!

test_regress/t/t_trace_abort_fst.pl

@@ -0,0 +1,26 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2020 by Geza Lore. 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_all => 1);
+
+top_filename("t/t_trace_abort.v");
+
+compile(
+    verilator_flags2 => ['--cc --trace-fst'],
+    );
+
+execute(
+    fails => 1,
+    );
+
+fst_identical("$Self->{obj_dir}/simx.fst", $Self->{golden_filename});
+
+ok(1);
+1;