--- a/include/verilated.cpp
+VL_THREAD struct Verilated::VerilatedCommandArgs Verilated::s_args = {0, NULL};
 
This doesn't need to be per-thread as there's only one command line.

+++ b/include/verilated.h
@@ -236,6 +236,11 @@ private:
+    static struct VerilatedCommandArgs {
+	int          argc;
+        const char** argv;
+    } s_args;

Because this is under Verilated:: you should drop the Verilated prefix.

I was also thinking it needs to be save-restored (serialized) but the
assumption is that the restore is allowed to pass different arguments so
that's ok.  Perhaps add a comment mentioning that.

@@ -275,6 +280,7 @@ public:
+    static struct VerilatedCommandArgs* getCommandArgs() {return &s_args;}

use:

static VerilatedCommandArgs* getCommandArgs() {return &s_args;}

+++ b/include/verilated_vpi.h
@@ -41,7 +39,7 @@
 
 // Not supported yet
 #define _VL_VPI_UNIMP() \
-    vl_fatal(__FILE__,__LINE__,"",Verilated::catName("Unsupported VPI function: ",VL_FUNC))
+    VerilatedVpiError::vpi_unsupported(); vl_fatal(__FILE__,__LINE__,"",Verilated::catName("Unsupported VPI function: ",VL_FUNC));
 
Might as well clean up further and move the vl_fatal into vpi_unsupported
and have vpi_unsupported take the file and line.

@@ -250,13 +248,21 @@ struct VerilatedVpiTimedCbsCmp {
 class VerilatedVpi {
      enum { CB_ENUM_MAX_VALUE = cbAtEndOfSimTime+1 };	// Maxium callback reason
      typedef set<VerilatedVpioCb*> VpioCbSet;
      typedef set<pair<QData,VerilatedVpioCb*>,VerilatedVpiTimedCbsCmp > VpioTimedCbs;
 
+    struct product_info {
+	PLI_BYTE8* product;
+        PLI_BYTE8* version;
+    };

I know VPI isn't "const correct" but prefer verilator to be where possible
so please add a "const" here which unfortunately you'll need to cast-away
when reading in vpi_get_vlog_info.

      VpioCbSet		m_cbObjSets[CB_ENUM_MAX_VALUE];	// Callbacks for each supported reason
      VpioTimedCbs	m_timedCbs;	// Time based callbacks
+    VerilatedVpiError  *m_error_infop;
 
Nit, put the * next to the class:

VerilatedVpiError*  m_error_infop;

@@ -338,0 +344,0 @@ public:

+    static struct product_info* get_product_info() {
+        // how to import version info here?
+        // static const char* _package_string = PACKAGE_STRING;
+	static struct product_info _product_info = {(PLI_BYTE8*)"Verilator", (PLI_BYTE8*)"3.844 devel"};
+        return &_product_info;
+    };

I'd get rid of this and make a Verilated::productName() and
Verilated::productVersion() which VPI just calls.  Of couse that just
pushes the problem over ;)

In verilated.cpp roughly

const char* Verilated::productVersion() { return VERILATOR_VERSION; }

#include "verilated_config.h" at top of verilated.h

Add a verilat_config.h.in which is similar to src/config_build.h.in:

    // standard header....

    #define VERILATOR_VERSION	@PACKAGE_STRING@

Edit configure.ac:

    AC_CONFIG_FILES(Makefile src/Makefile src/Makefile_obj include/verilated.mk include/verilated_config.h)

Be sure to check that the generated code is sane.  Add a "clean" rule to
remove include/verilated_config.h.  Add a .gitignore rule for
include/verilated_config.h.

@@ -338,8 +344,107 @@ public:

+struct VerilatedVpiError {
+    //// Container for vpi error info
+    t_vpi_error_info m_error_info;
+    bool             m_flag;
+    char             m_buff[1024];

Use a #define VERILATOR_VPI_LINE_SIZE 8192

(1024 too likely to be hit regurally and makes the arbitrary size more obvious)

+    VerilatedVpiError() : m_flag(false) {
+	m_error_info.product = VerilatedVpi::get_product_info()->product;
+    }

+    void pli(PLI_INT32 level, string file, PLI_INT32 line, string message, ...) {
+        va_list args;
+        m_error_info.state = vpiPLI;
+        va_start(args, message);
+        vsnprintf(m_buff, sizeof(m_buff), message.c_str(), args);
+        va_end(args);
+        set(level, (PLI_BYTE8*)m_buff, (PLI_BYTE8*)file.c_str(), line);
+    }
+    void pli(PLI_INT32 level, PLI_BYTE8 *code, PLI_BYTE8 *file, PLI_INT32 line, string message, ...) {
+        va_list args;
+        m_error_info.state = vpiPLI;
+        va_start(args, message);
+        vsnprintf(m_buff, sizeof(m_buff), message.c_str(), args);
+        va_end(args);
+        set(level, (PLI_BYTE8*)message.c_str(), code, file, line);
+    }
+    void set(PLI_INT32 level, PLI_BYTE8 *message, PLI_BYTE8 *file, PLI_INT32 line) {
+	m_flag=true;
+        m_error_info.level = level;
+        m_error_info.message = message;
+        m_error_info.file = file;
+        m_error_info.line = line;
+        m_error_info.code = NULL;
+        do_callbacks();
+    }
+    void set(PLI_INT32 level, PLI_BYTE8 *message, PLI_BYTE8 *code, PLI_BYTE8 *file, PLI_INT32 line) {
+	set(level, message, file, line);
+        m_error_info.code = code;
+        do_callbacks();
+    }


'set' is a usually a type.  I'd suggest pliError(...) and setError(...) or similar.
I think setError() should be private.

@@ -571,20 +678,24 @@ PLI_BYTE8 *vpi_get_str(PLI_INT32 property, vpiHandle object) {
 
 // delay processing
 
+void vpi_put_delays(vpiHandle object, p_vpi_delay delay_p) {
+     VerilatedVpi::error_info()->pli(vpiError, __FILE__, __LINE__, " %s is currently unimplemented", VL_FUNC);
+    _VL_VPI_UNIMP(); return;
+}
 
Here and elsewhere I don't think you need to call
VerilatedVpi::error_info()->pli... as you put this into VL_VPI_UNIMP.


@@ -624,3 +735,4 @@ void vpi_get_value(vpiHandle object, p_vpi_value value_p) {
 		return;
 	    }
 	    default: {
+                 VerilatedVpi::error_info()->pli(vpiError, __FILE__, __LINE__, "Unsupported format (%s) for %s in %s", VerilatedVpiError::str_from_vpiVal(value_p->format), vop->fullname(), VL_FUNC);

This is ugly to read and I suspect we may change it later so, I suggest making:

#define _VL_VPI_ERROR VerilatedVpi::error_info()->pliError
#define _VL_VPI_WARN  VerilatedVpi::error_info()->pliWarning
#define _VL_VPI_ERROR_RESET VerilatedVpi::error_info()->reset

Then this becomes

_VL_VPI_ERROR(__FILE__, __LINE__, "Unsupported format (%s) for %s in %s", VerilatedVpiError::str_from_vpiVal(value_p->format), vop->fullname(), VL_FUNC);

Ideally the first parameters could be implied but this is a bit of work as
we can't use defines with arbitrary numbers of parameters as that isn't
portable.


+	} else if (value_p->format == vpiStringVal) {
+	    static VL_THREAD char out[1+VL_MULS_MAX_WORDS*4];
+	    value_p->value.str = out;
+	    switch (vop->varp()->vltype()) {
+	    case VLVT_UINT8:
+	    case VLVT_WDATA:
+            {
+		int bytes = VL_BYTES_I(vop->varp()->range().bits());
+		CData* datap = ((CData*)(vop->varDatap()));
+                int i;
+		for (i=0; i<bytes; i++) {
+                    char val = datap[bytes-i-1];
+     	            out[i] = val?val:' ';
+		}
+                out[i]=0; // NULL terminate
+		return;
+	    }

Need to check for "out" buffer overrun. as VL_MULS_MAX_WORDS only applies
to multiplies not everything.

     void VerilatedVpiError::do_callbacks() {
         if (getError()->level < vpiError) return; // Don't do anything if not an error
         VerilatedVpi::callCbs(cbPLIError);
#ifdef VL_VPI_FATAL_ON_UNSUPPORTED
         vpi_unsupported();
#endif
     }

--- a/include/verilated_vpi.h

+#define _VL_VPI_ERROR_SET_ \
+        va_list args; \
+        va_start(args, message); \
+        vsnprintf(m_buff, sizeof(m_buff), message.c_str(), args); \
+        va_end(args);

Please wrap this in a 'do { ... } while (0)' and drop trailing _ so it can
be called with a trailing semicolon so that emacs indents don't get upset.

+class VerilatedVpiError {
+...
+    VerilatedVpiError() : m_flag(false) {

When you make a constructor always make a destructor even if empty.  (See "Exceptional C++")

+    static const char* str_from_vpiVal(PLI_INT32 vpiVal) {
+	static const char *names[] = {
+	    "*undefined*",
+	    "vpiBinStrVal",

First the mix of underscores and studlyCaps kills me :) I'm inconsistent
but usually avoid underscores in Verilator-internal method names.

I think these tables or the whole function definitions should be in
verilated_vpi.cpp as there's no reason to burden every header include with
compiling the function.

Also I presume you made them from the spec, but it seems a bit error prone;
if this stuff changed a lot I would script it, but it's so static it isn't
worth it.  Perhaps a test for at least the last value in each table would
be good, as otherwise one deletion in the beginning screws everything.

+++ b/test_regress/t/t_vpi_var.cpp
+#define DEBUG if (0) printf

Don't ever define DEBUG unless it's for assert.h's usage, call it TEST_MSG or something.