Updates from feedback ; move location of loading function ; Use gold file for all tests

Signed-off-by: Matthew Ballance <matt.ballance@gmail.com>

bin/verilator

@@ -653,7 +653,7 @@ description of these arguments.
      +verilator+solver+file+<filename>     Set random solver log filename
      +verilator+V                          Show verbose version and config
      +verilator+version                    Show version and exit
-     +verilator+vpi+<library>[:<bootstrap>]  Load VPI shared library (requires --vpi --main)
+     +verilator+vpi+<library>[:<bootstrap>]  Load VPI shared library
      +verilator+wno+unsatconstr+<value>    Disable constraint warnings
 
 

docs/guide/exe_sim.rst

@@ -146,14 +146,12 @@ Options:
    :vlopt:`--binary`). ``<library>`` is the path to the shared library. If
    ``:<bootstrap>`` is given, that named no-argument function is called;
    otherwise the library's ``vlog_startup_routines`` array (IEEE 1800 Section 37) is
-   invoked. May be repeated to load multiple libraries. For signals to be
-   accessible by name, also Verilate with :vlopt:`--public-flat-rw`. If passed
-   to a model not compiled with :vlopt:`--vpi`, a warning is emitted and the
-   argument is ignored.
+   invoked. May be repeated to load multiple libraries.
 
    Runtime loading is supported on POSIX platforms only (it relies on the
    executable exporting its VPI symbols to the loaded library); on Windows the
-   argument is rejected and the VPI code must instead be linked into the model.
+   argument is rejected and the VPI code must instead be statically linked
+   into the model.
 
 .. option:: +verilator+wno+unsatconstr+<value>
 

include/verilated.cpp

@@ -89,6 +89,12 @@
 # include <unistd.h>
 # define _VL_HAVE_GETRLIMIT
 #endif
+#if VM_VPI
+# include <cstring>
+# ifndef _WIN32
+#  include <dlfcn.h>  // Used by vl_load_vpi_libs
+# endif
+#endif
 
 #include "verilated_threads.h"
 // clang-format on
@@ -260,6 +266,58 @@ void VL_WARN_MT(const char* filename, int linenum, const char* hier, const char*
     }});
 }
 
+//===========================================================================
+// Runtime VPI shared library loading (--vpi)
+
+#if VM_VPI
+void vl_load_vpi_libs(int argc, char** argv) VL_MT_UNSAFE {
+    const char* prefix = "+verilator+vpi+";
+    const size_t pfx_len = std::strlen(prefix);
+    for (int i = 1; i < argc; ++i) {
+        if (std::strncmp(argv[i], prefix, pfx_len) != 0) continue;
+        const std::string arg(argv[i] + pfx_len);
+        if (arg.empty()) continue;
+#ifdef _WIN32
+        VL_FATAL_MT("", 0, "",
+                    "+verilator+vpi+: runtime VPI library loading is not supported on"
+                    " Windows; link the VPI code into the model instead");
+#else
+        using vlog_startup_t = void (*)();
+        // Split <lib>:<bootstrap> on the last ':'
+        const std::string::size_type colon_pos = arg.rfind(':');
+        const bool has_entry = (colon_pos != std::string::npos);
+        const std::string libpath = has_entry ? arg.substr(0, colon_pos) : arg;
+        const std::string entry_name = has_entry ? arg.substr(colon_pos + 1) : std::string{};
+        void* handle = dlopen(libpath.c_str(), RTLD_LAZY);
+        if (!handle)
+            // The library path is stable; the dlerror() text is platform-specific, so put it on
+            // a separate "- " line (test golden files strip "- " lines, keeping output portable).
+            VL_FATAL_MT(
+                "", 0, "",
+                (std::string{"Cannot load VPI library: "} + libpath + "\n- dlerror: " + dlerror())
+                    .c_str());
+        if (has_entry) {
+            vlog_startup_t bsp = reinterpret_cast<vlog_startup_t>(dlsym(handle, entry_name.c_str()));
+            if (!bsp)
+                VL_FATAL_MT("", 0, "",
+                            (std::string{"Cannot find VPI bootstrap '"} + entry_name
+                             + "' in: " + libpath)
+                                .c_str());
+            bsp();
+        } else {
+            vlog_startup_t* routinesp
+                = reinterpret_cast<vlog_startup_t*>(dlsym(handle, "vlog_startup_routines"));
+            if (!routinesp)
+                VL_FATAL_MT(
+                    "", 0, "",
+                    (std::string{"Cannot find 'vlog_startup_routines' in: "} + libpath).c_str());
+            for (int j = 0; routinesp[j]; ++j) routinesp[j]();
+        }
+#endif
+    }
+}
+#endif
+
 //===========================================================================
 // Debug prints
 
@@ -3552,7 +3610,7 @@ void VerilatedContextImp::commandArgVl(const std::string& arg) {
 #if !VM_VPI
             VL_WARN_MT(
                 "COMMAND_LINE", 0, "",
-                ("+verilator+vpi+ ignored: simulation was not compiled with --vpi (" + arg + ")")
+                ("+verilator+vpi+ ignored: simulation was not compiled with --vpi '" + arg + "'")
                     .c_str());
 #endif
         } else if (arg == "+verilator+V") {

include/verilated.mk.in

@@ -80,9 +80,6 @@ UNAME_S := $(shell uname -s)
 ######################################################################
 # C Preprocessor flags
 
-# Default for makefiles generated before --vpi tracking was added
-VM_VPI ?= 0
-
 # Add -MMD -MP if you're using a recent version of GCC.
 VK_CPPFLAGS_ALWAYS += \
   -MMD \

include/verilated_funcs.h

@@ -74,6 +74,11 @@ extern void VL_FATAL_MT(const char* filename, int linenum, const char* hier,
 extern void VL_WARN_MT(const char* filename, int linenum, const char* hier,
                        const char* msg) VL_MT_SAFE;
 
+#if VM_VPI
+/// Load VPI shared libraries requested via +verilator+vpi+<lib>[:<bootstrap>].
+extern void vl_load_vpi_libs(int argc, char** argv) VL_MT_UNSAFE;
+#endif
+
 /// Print a string, multithread safe. Eventually VL_PRINTF will get called.
 extern void VL_PRINTF_MT(const char* formatp, ...) VL_ATTR_PRINTF(1) VL_MT_SAFE;
 

src/V3EmitCMain.cpp

@@ -55,13 +55,7 @@ private:
         // Heavily commented output, as users are likely to look at or copy this code
 
         puts("#include \"verilated.h\"\n");
-        if (v3Global.opt.vpi()) {
-            puts("#include \"verilated_vpi.h\"\n");
-            puts("#ifndef _WIN32\n");
-            puts("# include <dlfcn.h>  // For runtime +verilator+vpi+ library loading\n");
-            puts("#endif\n");
-            puts("#include <cstring>\n");
-        }
+        if (v3Global.opt.vpi()) puts("#include \"verilated_vpi.h\"\n");
         puts("#include \"" + EmitCUtil::topClassName() + ".h\"\n");
         if (v3Global.opt.debugRuntimeTimeout()) {
             puts("\n");
@@ -73,56 +67,6 @@ private:
             puts("// User VPI code adds to this array to get startup main() callbacks\n");
             puts("extern \"C\" void (*vlog_startup_routines[])() VL_ATTR_WEAK;\n");
             puts("\n");
-
-            // Runtime loader for VPI shared libraries requested via +verilator+vpi+<lib>.
-            // POSIX only: relies on the executable exporting its VPI symbols.
-            puts(
-                "// Load VPI shared libraries requested via +verilator+vpi+<lib>[:<bootstrap>]\n");
-            puts("static void vl_load_vpi_libs(int argc, char** argv) {\n");
-            puts(/**/ "const char* prefix = \"+verilator+vpi+\";\n");
-            puts(/**/ "const size_t pfx_len = std::strlen(prefix);\n");
-            puts(/**/ "for (int i = 1; i < argc; ++i) {\n");
-            puts(/****/ "if (std::strncmp(argv[i], prefix, pfx_len) != 0) continue;\n");
-            puts(/****/ "const std::string arg(argv[i] + pfx_len);\n");
-            puts(/****/ "if (arg.empty()) continue;\n");
-            puts("#ifdef _WIN32\n");
-            puts(/****/ "VL_FATAL_MT(\"\", 0, \"\","
-                        " \"+verilator+vpi+: runtime VPI library loading is not supported on\"\n");
-            puts(/******/ "\" Windows; link the VPI code into the model instead\");\n");
-            puts("#else\n");
-            puts(/****/ "using vlog_startup_t = void (*)();\n");
-            puts(/****/ "// Split <lib>:<bootstrap> on the last ':'\n");
-            puts(/****/ "const std::string::size_type colon_pos = arg.rfind(':');\n");
-            puts(/****/ "const bool has_entry = (colon_pos != std::string::npos);\n");
-            puts(/****/ "const std::string libpath = has_entry ? arg.substr(0, colon_pos) : "
-                        "arg;\n");
-            puts(/****/ "const std::string entry_name\n");
-            puts(/******/ "= has_entry ? arg.substr(colon_pos + 1) : std::string{};\n");
-            puts(/****/ "void* handle = dlopen(libpath.c_str(), RTLD_LAZY);\n");
-            puts(/****/ "if (!handle)\n");
-            puts(/******/ "VL_FATAL_MT(\"\", 0, \"\","
-                          " (std::string{\"Cannot load VPI library: \"} + dlerror()).c_str());\n");
-            puts(/****/ "if (has_entry) {\n");
-            puts(/******/ "vlog_startup_t bsp = reinterpret_cast<vlog_startup_t>("
-                          "dlsym(handle, entry_name.c_str()));\n");
-            puts(/******/ "if (!bsp)\n");
-            puts(/********/ "VL_FATAL_MT(\"\", 0, \"\", (std::string{\"Cannot find VPI bootstrap "
-                            "'\"}\n");
-            puts(/**********/ "+ entry_name + \"' in: \" + libpath).c_str());\n");
-            puts(/******/ "bsp();\n");
-            puts(/****/ "} else {\n");
-            puts(/******/ "vlog_startup_t* routinesp = reinterpret_cast<vlog_startup_t*>(\n");
-            puts(/********/ "dlsym(handle, \"vlog_startup_routines\"));\n");
-            puts(/******/ "if (!routinesp)\n");
-            puts(/********/ "VL_FATAL_MT(\"\", 0, \"\","
-                            " (std::string{\"Cannot find 'vlog_startup_routines' in: \"}\n");
-            puts(/**********/ "+ libpath).c_str());\n");
-            puts(/******/ "for (int j = 0; routinesp[j]; ++j) routinesp[j]();\n");
-            puts(/****/ "}\n");
-            puts("#endif\n");
-            puts(/**/ "}\n");
-            puts("}\n");
-            puts("\n");
         }
 
         puts("//======================\n\n");

test_regress/t/t_flag_main_vpi.cpp

@@ -62,7 +62,7 @@ static PLI_INT32 start_of_sim_cb(p_cb_data /*cb_data*/) {
 }
 
 static PLI_INT32 end_of_sim_cb(p_cb_data /*cb_data*/) {
-    vpi_printf(const_cast<char*>("- VPI end of simulation\n"));
+    vpi_printf(const_cast<char*>("VPI end of simulation\n"));
     return 0;
 }
 

test_regress/t/t_flag_main_vpi.out

@@ -1 +1,2 @@
-- VPI end of simulation
+*-* All Finished *-*
+VPI end of simulation

test_regress/t/t_flag_main_vpi.py

@@ -24,6 +24,10 @@ test.file_grep(mk, r'LDFLAGS \+= -rdynamic')
 test.file_grep(mk, r'LDLIBS \+= -ldl')
 
 # Run the generated binary; load the VPI library via the +verilator+vpi+ plusarg.
-test.execute(all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so"], check_finished=True)
+# The VPI library's output (observed 'count' reaching MAX_TICKS, then end-of-sim) is
+# checked against the golden .out file.
+test.execute(all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so"],
+             check_finished=True,
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_badentry.out

@@ -0,0 +1,2 @@
+%Error: Cannot find VPI bootstrap 'no_such_fn' in: obj_vlt/t_flag_main_vpi_badentry/libvpi.so
+Aborting...

test_regress/t/t_flag_main_vpi_badentry.py

@@ -19,8 +19,7 @@ test.pli_filename = 't/t_flag_main_vpi.cpp'
 test.compile(make_pli=True, verilator_flags2=["--binary --vpi --public-flat-rw"])
 
 test.execute(fails=True,
-             all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so:no_such_fn"])
-
-test.file_grep(test.run_log_filename, r"Cannot find VPI bootstrap 'no_such_fn'")
+             all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so:no_such_fn"],
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_badlib.out

@@ -0,0 +1,2 @@
+%Error: Cannot load VPI library: obj_vlt/t_flag_main_vpi_badlib/nonexistent.so
+Aborting...

test_regress/t/t_flag_main_vpi_badlib.py

@@ -17,8 +17,10 @@ test.top_filename = 't/t_flag_main_vpi.v'
 
 test.compile(verilator_flags2=["--binary --vpi --public-flat-rw"])
 
-test.execute(fails=True, all_run_flags=["+verilator+vpi+" + test.obj_dir + "/nonexistent.so"])
-
-test.file_grep(test.run_log_filename, r'Cannot load VPI library')
+# The fatal names the (stable, relative) library path; the platform-specific dlerror()
+# detail is emitted on a "- " line, which golden comparison strips, so the .out is portable.
+test.execute(fails=True,
+             all_run_flags=["+verilator+vpi+" + test.obj_dir + "/nonexistent.so"],
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_bootstrap.out

@@ -0,0 +1,2 @@
+*-* All Finished *-*
+VPI end of simulation

test_regress/t/t_flag_main_vpi_bootstrap.py

@@ -19,6 +19,7 @@ test.pli_filename = 't/t_flag_main_vpi.cpp'
 test.compile(make_pli=True, verilator_flags2=["--binary --vpi --public-flat-rw"])
 
 test.execute(all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so:my_vpi_bootstrap"],
-             check_finished=True)
+             check_finished=True,
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_lib2.cpp

@@ -17,7 +17,7 @@
 
 #include "vpi_user.h"
 
-static void lib2_startup() { vpi_printf(const_cast<char*>("- second VPI library loaded\n")); }
+static void lib2_startup() { vpi_printf(const_cast<char*>("second VPI library loaded\n")); }
 
 // IEEE 1800 section 37: vlog_startup_routines[] -- null-terminated array of startup functions
 extern "C" {

test_regress/t/t_flag_main_vpi_multi.out

@@ -0,0 +1,3 @@
+second VPI library loaded
+*-* All Finished *-*
+VPI end of simulation

test_regress/t/t_flag_main_vpi_multi.py

@@ -36,8 +36,7 @@ test.execute(all_run_flags=[
     "+verilator+vpi+" + test.obj_dir + "/libvpi.so",
     "+verilator+vpi+" + test.obj_dir + "/libvpi2.so"
 ],
-             check_finished=True)
-
-test.file_grep(test.run_log_filename, r'- second VPI library loaded')
+             check_finished=True,
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_noarray.out

@@ -0,0 +1,2 @@
+%Error: Cannot find 'vlog_startup_routines' in: obj_vlt/t_flag_main_vpi_noarray/libvpi.so
+Aborting...

test_regress/t/t_flag_main_vpi_noarray.py

@@ -18,8 +18,8 @@ test.pli_filename = 't/t_flag_main_vpi_noarray.cpp'
 
 test.compile(make_pli=True, verilator_flags2=["--binary --vpi --public-flat-rw"])
 
-test.execute(fails=True, all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so"])
-
-test.file_grep(test.run_log_filename, r"Cannot find 'vlog_startup_routines'")
+test.execute(fails=True,
+             all_run_flags=["+verilator+vpi+" + test.obj_dir + "/libvpi.so"],
+             expect_filename=test.golden_filename)
 
 test.passes()

test_regress/t/t_flag_main_vpi_nowarn.out

@@ -0,0 +1,3 @@
+%Warning: COMMAND_LINE:0: +verilator+vpi+ ignored: simulation was not compiled with --vpi '+verilator+vpi+/nonexistent.so'
+[0] Hello
+*-* All Finished *-*

test_regress/t/t_flag_main_vpi_nowarn.py

@@ -15,11 +15,10 @@ test.scenarios('vlt')
 # Passing +verilator+vpi+ at runtime should emit a warning, not load anything.
 test.compile(top_filename='t/t_flag_main.v', verilator_flags2=["--binary"])
 
-test.execute(all_run_flags=["+verilator+vpi+/nonexistent.so"], check_finished=True)
-
-test.file_grep(
-    test.run_log_filename,
-    r'%Warning: COMMAND_LINE:0: \+verilator\+vpi\+ ignored: simulation was not compiled with --vpi'
-)
+# Without --vpi there is no loader, so the plusarg is ignored with a warning (checked via
+# the golden .out).  The plusarg value is fixed, so the warning text is portable.
+test.execute(all_run_flags=["+verilator+vpi+/nonexistent.so"],
+             check_finished=True,
+             expect_filename=test.golden_filename)
 
 test.passes()