Fix VPI callback list iteration (#2644)

The end iterator always points to an element past the end of the list.
When new elements are added to the back of the list, they are inserted
before the end iterator.
Instead, track the last element in the list at the start of processing
and stop after it's been processed.

include/verilated_vpi.cpp

@@ -475,16 +475,21 @@ public:
     static bool callCbs(vluint32_t reason) VL_MT_UNSAFE_ONE {
         VpioCbList& cbObjList = s_s.m_cbObjLists[reason];
         bool called = false;
-        const auto end = cbObjList.end();  // prevent looping over newly added elements
-        for (auto it = cbObjList.begin(); it != end;) {
+        if (cbObjList.empty()) return called;
+        const auto last = std::prev(cbObjList.end());  // prevent looping over newly added elements
+        for (auto it = cbObjList.begin(); true;) {
+            // cbReasonRemove sets to nullptr, so we know on removal the old end() will still exist
+            bool was_last = it == last;
             if (VL_UNLIKELY(!*it)) {  // Deleted earlier, cleanup
                 it = cbObjList.erase(it);
+                if (was_last) break;
                 continue;
             }
             VerilatedVpioCb* vop = *it++;
             VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: reason_callback %d %p\n", reason, vop););
             (vop->cb_rtnp())(vop->cb_datap());
             called = true;
+            if (was_last) break;
         }
         return called;
     }
@@ -494,10 +499,14 @@ public:
         bool called = false;
         typedef std::set<VerilatedVpioVar*> VpioVarSet;
         VpioVarSet update;  // set of objects to update after callbacks
-        const auto end = cbObjList.end();  // prevent looping over newly added elements
-        for (auto it = cbObjList.begin(); it != end;) {
+        if (cbObjList.empty()) return called;
+        const auto last = std::prev(cbObjList.end());  // prevent looping over newly added elements
+        for (auto it = cbObjList.begin(); true;) {
+            // cbReasonRemove sets to nullptr, so we know on removal the old end() will still exist
+            bool was_last = it == last;
             if (VL_UNLIKELY(!*it)) {  // Deleted earlier, cleanup
                 it = cbObjList.erase(it);
+                if (was_last) break;
                 continue;
             }
             VerilatedVpioCb* vop = *it++;
@@ -516,6 +525,7 @@ public:
                     called = true;
                 }
             }
+            if (was_last) break;
         }
         for (const auto& ip : update) { memcpy(ip->prevDatap(), ip->varDatap(), ip->entSize()); }
         return called;

test_regress/t/t_vpi_cb_iter.cpp

@@ -0,0 +1,202 @@
+// -*- mode: C++; c-file-style: "cc-mode" -*-
+//*************************************************************************
+//
+// Copyright 2020 by Wilson Snyder and Marlon James. 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
+//
+//*************************************************************************
+
+#include "Vt_vpi_cb_iter.h"
+#include "verilated.h"
+#include "verilated_vpi.h"
+
+#include <cstdlib>
+#include <cstdio>
+#include <cstring>
+#include <iostream>
+#include <vector>
+
+#include "TestSimulator.h"
+#include "TestVpi.h"
+
+#include "vpi_user.h"
+
+bool got_error = false;
+
+vpiHandle vh_value_cb = 0;
+vpiHandle vh_rw_cb = 0;
+
+unsigned int last_value_cb_time = 0;
+unsigned int last_rw_cb_time = 0;
+
+unsigned int main_time = 0;
+
+#ifdef TEST_VERBOSE
+bool verbose = true;
+#else
+bool verbose = false;
+#endif
+
+#define CHECK_RESULT_NZ(got) \
+    if (!(got)) { \
+        printf("%%Error: %s:%d: GOT = NULL  EXP = !NULL\n", __FILE__, __LINE__); \
+        got_error = true; \
+    }
+
+// Use cout to avoid issues with %d/%lx etc
+#define CHECK_RESULT_NE(got, exp) \
+    if ((got) == (exp)) { \
+        std::cout << std::dec << "%Error: " << __FILE__ << ":" << __LINE__ << ": GOT = " << (got) \
+                  << "   EXP = !" << (exp) << std::endl; \
+        got_error = true; \
+    }
+
+// Use cout to avoid issues with %d/%lx etc
+#define CHECK_RESULT(got, exp) \
+    if ((got) != (exp)) { \
+        std::cout << std::dec << "%Error: " << __FILE__ << ":" << __LINE__ << ": GOT = " << (got) \
+                  << "   EXP = " << (exp) << std::endl; \
+        got_error = true; \
+    }
+static void reregister_value_cb();
+static void reregister_rw_cb();
+
+static int the_value_callback(p_cb_data cb_data) {
+    reregister_value_cb();
+    return 0;
+}
+
+static int the_rw_callback(p_cb_data cb_data) {
+    reregister_rw_cb();
+    return 0;
+}
+
+static void reregister_value_cb() {
+    if (vh_value_cb) {
+        if (verbose) { vpi_printf(const_cast<char*>("- Removing cbValueChange callback\n")); }
+        int ret = vpi_remove_cb(vh_value_cb);
+        CHECK_RESULT(ret, 1);
+
+        if (verbose) {
+            vpi_printf(const_cast<char*>("- last_value_cb_time %d , main_time %d\n"),
+                       last_value_cb_time, main_time);
+        }
+        CHECK_RESULT_NE(main_time, last_value_cb_time);
+        last_value_cb_time = main_time;
+    }
+    if (verbose) { vpi_printf(const_cast<char*>("- Registering cbValueChange callback\n")); }
+    t_cb_data cb_data_testcase;
+    bzero(&cb_data_testcase, sizeof(cb_data_testcase));
+    cb_data_testcase.cb_rtn = the_value_callback;
+    cb_data_testcase.reason = cbValueChange;
+
+    vpiHandle vh1 = VPI_HANDLE("count");
+    CHECK_RESULT_NZ(vh1);
+
+    s_vpi_value v;
+    v.format = vpiSuppressVal;
+
+    cb_data_testcase.obj = vh1;
+    cb_data_testcase.value = &v;
+
+    vh_value_cb = vpi_register_cb(&cb_data_testcase);
+    CHECK_RESULT_NZ(vh_value_cb);
+}
+
+static void reregister_rw_cb() {
+    if (vh_rw_cb) {
+        if (verbose) { vpi_printf(const_cast<char*>("- Removing cbReadWriteSynch callback\n")); }
+        int ret = vpi_remove_cb(vh_rw_cb);
+        CHECK_RESULT(ret, 1);
+
+        if (verbose) {
+            vpi_printf(const_cast<char*>("- last_rw_cb_time %d , main_time %d\n"), last_rw_cb_time,
+                       main_time);
+        }
+        CHECK_RESULT_NE(main_time, last_rw_cb_time);
+        last_rw_cb_time = main_time;
+    }
+    if (verbose) { vpi_printf(const_cast<char*>("- Registering cbReadWriteSynch callback\n")); }
+    t_cb_data cb_data_testcase;
+    bzero(&cb_data_testcase, sizeof(cb_data_testcase));
+    cb_data_testcase.cb_rtn = the_rw_callback;
+    cb_data_testcase.reason = cbReadWriteSynch;
+
+    vh_rw_cb = vpi_register_cb(&cb_data_testcase);
+    CHECK_RESULT_NZ(vh_rw_cb);
+}
+
+static int the_filler_callback(p_cb_data cb_data) { return 0; }
+
+static void register_filler_cb() {
+    if (verbose) {
+        vpi_printf(const_cast<char*>("- Registering filler cbReadWriteSynch callback\n"));
+    }
+    t_cb_data cb_data_1;
+    bzero(&cb_data_1, sizeof(cb_data_1));
+    cb_data_1.cb_rtn = the_filler_callback;
+    cb_data_1.reason = cbReadWriteSynch;
+
+    vpiHandle vh1 = vpi_register_cb(&cb_data_1);
+    CHECK_RESULT_NZ(vh1);
+
+    if (verbose) {
+        vpi_printf(const_cast<char*>("- Registering filler cbValueChange callback\n"));
+    }
+    t_cb_data cb_data_2;
+    bzero(&cb_data_2, sizeof(cb_data_2));
+    cb_data_2.cb_rtn = the_filler_callback;
+    cb_data_2.reason = cbValueChange;
+
+    vpiHandle vh2 = VPI_HANDLE("count");
+    CHECK_RESULT_NZ(vh2);
+
+    s_vpi_value v;
+    v.format = vpiSuppressVal;
+
+    cb_data_2.obj = vh2;
+    cb_data_2.value = &v;
+
+    vpiHandle vh3 = vpi_register_cb(&cb_data_2);
+    CHECK_RESULT_NZ(vh3);
+}
+
+double sc_time_stamp() { return main_time; }
+
+int main(int argc, char** argv, char** env) {
+    double sim_time = 100;
+    Verilated::commandArgs(argc, argv);
+    Verilated::debug(0);
+
+    VM_PREFIX* topp = new VM_PREFIX("");  // Note null name - we're flattening it out
+
+    reregister_value_cb();
+    CHECK_RESULT_NZ(vh_value_cb);
+    reregister_rw_cb();
+    CHECK_RESULT_NZ(vh_rw_cb);
+    register_filler_cb();
+
+    topp->eval();
+    topp->clk = 0;
+
+    while (sc_time_stamp() < sim_time && !Verilated::gotFinish()) {
+        main_time += 1;
+        if (verbose) { VL_PRINTF("Sim Time %d got_error %d\n", main_time, got_error); }
+        topp->clk = !topp->clk;
+        topp->eval();
+        VerilatedVpi::callValueCbs();
+        VerilatedVpi::callCbs(cbReadWriteSynch);
+        if (got_error) { vl_stop(__FILE__, __LINE__, "TOP-cpp"); }
+    }
+
+    if (!Verilated::gotFinish()) {
+        vl_fatal(__FILE__, __LINE__, "main", "%Error: Timeout; never got a $finish");
+    }
+    topp->final();
+
+    VL_DO_DANGLING(delete topp, topp);
+    exit(0L);
+}

test_regress/t/t_vpi_cb_iter.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 Wilson Snyder and Marlon James. 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 => 1);
+
+compile(
+    make_top_shell => 0,
+    make_main => 0,
+    verilator_flags2 => ["--exe --vpi $Self->{t_dir}/$Self->{name}.cpp"],
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_vpi_cb_iter.v

@@ -0,0 +1,29 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2020 Wilson Snyder and Marlon James.
+// SPDX-License-Identifier: CC0-1.0
+
+
+module t (/*AUTOARG*/
+   // Inputs
+   input clk
+   );
+
+   reg [31:0]     count    /*verilator public_flat_rd */;
+
+   // Test loop
+   initial begin
+      count = 0;
+   end
+
+   always @(posedge clk) begin
+      count <= count + 2;
+
+      if (count == 10) begin
+        $write("*-* All Finished *-*\n");
+        $finish;
+      end
+   end
+
+endmodule : t