Fix removal of callbacks no longer in current list (#5851) (#5852)

docs/CONTRIBUTORS

@@ -65,6 +65,7 @@ Garrett Smith
 Geza Lore
 Gianfranco Costamagna
 Gijs Burghoorn
+Gilberto Abram
 Glen Gibb
 Gökçe Aydos
 Graham Rushton

include/verilated_vpi.cpp

@@ -873,6 +873,7 @@ class VerilatedVpiImp final {
     // All only medium-speed, so use singleton function
     // Callbacks that are past or at current timestamp
     std::array<VpioCbList, CB_ENUM_MAX_VALUE> m_cbCurrentLists;
+    VpioCbList m_cbCallList;  // List of callbacks currently being called by callCbs
     VpioFutureCbs m_futureCbs;  // Time based callbacks for future timestamps
     VpioFutureCbs m_nextCbs;  // cbNextSimTime callbacks
     std::list<VerilatedVpiPutHolder> m_inertialPuts;  // Pending vpi puts due to vpiInertialDelay
@@ -925,7 +926,13 @@ public:
         for (auto& ir : s().m_cbCurrentLists[reason]) {
             if (ir.id() == id) {
                 ir.invalidate();
-                return;  // Once found, it won't also be in m_futureCbs
+                return;  // Once found, it won't also be in m_cbCallList, m_futureCbs, or m_nextCbs
+            }
+        }
+        for (auto& ir : s().m_cbCallList) {
+            if (ir.id() == id) {
+                ir.invalidate();
+                return;  // Once found, it won't also be in m_futureCbs or m_nextCbs
             }
         }
         {  // Remove from cbFuture queue
@@ -985,10 +992,9 @@ public:
         moveFutureCbs();
         if (s().m_cbCurrentLists[reason].empty()) return false;
         // Iterate on old list, making new list empty, to prevent looping over newly added elements
-        VpioCbList cbObjList;
+        std::swap(s().m_cbCurrentLists[reason], s().m_cbCallList);
-        std::swap(s().m_cbCurrentLists[reason], cbObjList);
         bool called = false;
-        for (VerilatedVpiCbHolder& ihor : cbObjList) {
+        for (VerilatedVpiCbHolder& ihor : s().m_cbCallList) {
             // cbReasonRemove sets to nullptr, so we know on removal the old end() will still exist
             if (VL_LIKELY(!ihor.invalid())) {  // Not deleted earlier
                 VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: reason_callback reason=%d id=%" PRId64 "\n",
@@ -998,6 +1004,7 @@ public:
                 called = true;
             }
         }
+        s().m_cbCallList.clear();
         return called;
     }
     static bool callValueCbs() VL_MT_UNSAFE_ONE {

test_regress/t/t_vpi_time_cb_c.cpp

@@ -94,13 +94,27 @@ static int _time_cb2(p_cb_data cb_data) {
     return 0;
 }
 
+static vpiHandle callback_handles3[2] = {NULL, NULL};
+
+static int _time_cb3(p_cb_data cb_data) {
+    size_t cb_id = (size_t) cb_data->user_data;
+    size_t cb_id_other = cb_id ? 0 : 1;
+    TEST_VERBOSE_PRINTF("time_cb_3: %d\n", (int)cb_id);
+    TEST_CHECK_NZ(callback_handles3[cb_id]);
+    TEST_CHECK_NZ(callback_handles3[cb_id_other]);
+    vpi_remove_cb(callback_handles3[cb_id_other]);
+    callback_handles3[0] = callback_handles3[1] = NULL;
+    return 0;
+}
+
 extern "C" void dpii_init() {
     TEST_VERBOSE_PRINTF("-dpii_init()\n");
 
-    t_cb_data cb_data_n1, cb_data_n2;
+    t_cb_data cb_data_n1, cb_data_n2, cb_data_n3;
     bzero(&cb_data_n1, sizeof(cb_data_n1));
     bzero(&cb_data_n2, sizeof(cb_data_n2));
-    s_vpi_time t1, t2;
+    bzero(&cb_data_n2, sizeof(cb_data_n3));
+    s_vpi_time t1, t2, t3;
 
     cb_data_n1.reason = cbAfterDelay;
     t1.type = vpiSimTime;
@@ -118,6 +132,17 @@ extern "C" void dpii_init() {
     cb_data_n2.time = &t2;
     cb_data_n2.cb_rtn = _time_cb2;
     TestVpiHandle cb_data_n2_h = vpi_register_cb(&cb_data_n2);
+
+    cb_data_n3.reason = cbAfterDelay;
+    t3.type = vpiSimTime;
+    t3.high = 0;
+    t3.low = 5;
+    cb_data_n3.time = &t3;
+    cb_data_n3.cb_rtn = _time_cb3;
+    cb_data_n3.user_data = (PLI_BYTE8*) 0;
+    callback_handles3[0] = vpi_register_cb(&cb_data_n3);
+    cb_data_n3.user_data = (PLI_BYTE8*) 1;
+    callback_handles3[1] = vpi_register_cb(&cb_data_n3);
 }
 
 extern "C" void dpii_final() {