From 167a30be1cc4156954babb1f25ed82faf2df7609 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krzysztof=20Boro=C5=84ski?=
 <94375110+kboronski-ant@users.noreply.github.com>
Date: Tue, 23 May 2023 15:01:57 +0200
Subject: [PATCH] Fix deep traversal of class inheritance timing (#4216)

---
 src/V3Timing.cpp                     | 52 ++++++++++++++++++----------
 test_regress/t/t_suspendable_deep.pl | 18 ++++++++++
 test_regress/t/t_suspendable_deep.v  | 35 +++++++++++++++++++
 3 files changed, 87 insertions(+), 18 deletions(-)
 create mode 100755 test_regress/t/t_suspendable_deep.pl
 create mode 100644 test_regress/t/t_suspendable_deep.v

diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp
index a49761636..184ba25d8 100644
--- a/src/V3Timing.cpp
+++ b/src/V3Timing.cpp
@@ -57,6 +57,8 @@
 #include "V3SenTree.h"
 #include "V3UniqueNames.h"
 
+#include <queue>
+
 VL_DEFINE_DEBUG_FUNCTIONS;
 
 // ######################################################################
@@ -143,26 +145,40 @@ private:
         if (!m_classp) return;
         // If class method (possibly overrides another method)
         if (!m_classp->user1SetOnce()) m_classp->repairCache();
+
         // Go over overridden functions
-        for (auto* cextp = m_classp->extendsp(); cextp;
-             cextp = VN_AS(cextp->nextp(), ClassExtends)) {
-            // TODO: It is possible that a methods the same name in the base class is not
-            // actually overridden by our method. If this causes a problem, traverse to the
-            // root of the inheritance hierarchy and check if the original method is virtual or
-            // not.
-            if (!cextp->classp()->user1SetOnce()) cextp->classp()->repairCache();
-            if (auto* const overriddenp
-                = VN_CAST(cextp->classp()->findMember(nodep->name()), CFunc)) {
-                if (overriddenp->user2()) {  // If it's suspendable
-                    nodep->user2(true);  // Then we are also suspendable
-                    // As both are suspendable already, there is no need to add it as our
-                    // dependency or self to its dependencies
+
+        std::queue<AstClassExtends*> extends;
+        if (m_classp->extendsp()) extends.push(m_classp->extendsp());
+
+        while (!extends.empty()) {
+            AstClassExtends* ext_list = extends.front();
+            extends.pop();
+
+            for (AstClassExtends* cextp = ext_list; cextp;
+                 cextp = VN_AS(cextp->nextp(), ClassExtends)) {
+                // TODO: It is possible that a methods the same name in the base class is not
+                // actually overridden by our method. If this causes a problem, traverse to
+                // the root of the inheritance hierarchy and check if the original method is
+                // virtual or not.
+                if (!cextp->classp()->user1SetOnce()) cextp->classp()->repairCache();
+                if (AstCFunc* const overriddenp
+                    = VN_CAST(cextp->classp()->findMember(nodep->name()), CFunc)) {
+                    if (overriddenp->user2()) {  // If it's suspendable
+                        nodep->user2(true);  // Then we are also suspendable
+                        // As both are suspendable already, there is no need to add it as our
+                        // dependency or self to its dependencies
+                    } else {
+                        // Make a dependency cycle, as being suspendable should propagate both
+                        // up and down the inheritance tree
+                        TimingDependencyVertex* const overriddenVxp
+                            = getDependencyVertex(overriddenp);
+                        new V3GraphEdge{&m_depGraph, vxp, overriddenVxp, 1};
+                        new V3GraphEdge{&m_depGraph, overriddenVxp, vxp, 1};
+                    }
                 } else {
-                    // Make a dependency cycle, as being suspendable should propagate both up
-                    // and down the inheritance tree
-                    TimingDependencyVertex* const overriddenVxp = getDependencyVertex(overriddenp);
-                    new V3GraphEdge{&m_depGraph, vxp, overriddenVxp, 1};
-                    new V3GraphEdge{&m_depGraph, overriddenVxp, vxp, 1};
+                    AstClassExtends* more_extends = cextp->classp()->extendsp();
+                    if (more_extends) extends.push(more_extends);
                 }
             }
         }
diff --git a/test_regress/t/t_suspendable_deep.pl b/test_regress/t/t_suspendable_deep.pl
new file mode 100755
index 000000000..e7824bce9
--- /dev/null
+++ b/test_regress/t/t_suspendable_deep.pl
@@ -0,0 +1,18 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2023 by Wilson Snyder. 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(
+    verilator_flags2 => ["--timing"],
+    );
+
+ok(1);
+1;
diff --git a/test_regress/t/t_suspendable_deep.v b/test_regress/t/t_suspendable_deep.v
new file mode 100644
index 000000000..32ecd0a4c
--- /dev/null
+++ b/test_regress/t/t_suspendable_deep.v
@@ -0,0 +1,35 @@
+// DESCRIPTION: Verilator: Verilog Test module for specialized type default values
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2023 by Antmicro.
+// SPDX-License-Identifier: CC0-1.0
+
+`timescale 1ns/1ns
+
+event evt;
+
+class Baz;
+  virtual task do_something(); endtask
+endclass
+
+class Foo extends Baz;
+endclass
+
+class Bar extends Foo;
+  virtual task do_something();
+    @evt $display("Hello");
+  endtask
+endclass
+
+module top();
+  initial begin
+    Bar bar;
+    bar = new;
+
+    fork
+      #10 bar.do_something();
+      #20 $display("world!");
+      #10 ->evt;
+    join
+  end
+endmodule