From 899e7bacb2749676ccf53ea8ae851ae7412c07b9 Mon Sep 17 00:00:00 2001 From: Marlon James Date: Tue, 17 Nov 2020 14:19:51 -0800 Subject: [PATCH] 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 | 18 ++- test_regress/t/t_vpi_cb_iter.cpp | 202 +++++++++++++++++++++++++++++++ test_regress/t/t_vpi_cb_iter.pl | 24 ++++ test_regress/t/t_vpi_cb_iter.v | 29 +++++ 4 files changed, 269 insertions(+), 4 deletions(-) create mode 100644 test_regress/t/t_vpi_cb_iter.cpp create mode 100755 test_regress/t/t_vpi_cb_iter.pl create mode 100644 test_regress/t/t_vpi_cb_iter.v diff --git a/include/verilated_vpi.cpp b/include/verilated_vpi.cpp index 43e6feecd..e9a64241a 100644 --- a/include/verilated_vpi.cpp +++ b/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 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; diff --git a/test_regress/t/t_vpi_cb_iter.cpp b/test_regress/t/t_vpi_cb_iter.cpp new file mode 100644 index 000000000..f9dd322d4 --- /dev/null +++ b/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 +#include +#include +#include +#include + +#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("- Removing cbValueChange callback\n")); } + int ret = vpi_remove_cb(vh_value_cb); + CHECK_RESULT(ret, 1); + + if (verbose) { + vpi_printf(const_cast("- 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("- 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("- Removing cbReadWriteSynch callback\n")); } + int ret = vpi_remove_cb(vh_rw_cb); + CHECK_RESULT(ret, 1); + + if (verbose) { + vpi_printf(const_cast("- 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("- 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("- 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("- 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); +} diff --git a/test_regress/t/t_vpi_cb_iter.pl b/test_regress/t/t_vpi_cb_iter.pl new file mode 100755 index 000000000..0d8f23641 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_vpi_cb_iter.v b/test_regress/t/t_vpi_cb_iter.v new file mode 100644 index 000000000..9c4d0fa91 --- /dev/null +++ b/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