From 0982260d3b2708b0a624932ae9ffe33d367fd599 Mon Sep 17 00:00:00 2001 From: Petr Nohavica Date: Fri, 11 Jul 2025 19:10:36 +0200 Subject: [PATCH] Fix constructor parameters in inheritance hierarchies (#6036) (#6070) --- src/V3Class.cpp | 34 +++----- src/V3EmitCFunc.h | 65 ++++++++++----- src/V3EmitCHeaders.cpp | 14 ++-- test_regress/t/t_class_hier_construction.py | 18 +++++ test_regress/t/t_class_hier_construction.v | 88 +++++++++++++++++++++ 5 files changed, 174 insertions(+), 45 deletions(-) create mode 100755 test_regress/t/t_class_hier_construction.py create mode 100644 test_regress/t/t_class_hier_construction.v diff --git a/src/V3Class.cpp b/src/V3Class.cpp index a3a1a3aaf..1878b2582 100644 --- a/src/V3Class.cpp +++ b/src/V3Class.cpp @@ -57,28 +57,20 @@ class ClassVisitor final : public VNVisitor { // METHODS - bool recurseImplements(AstClass* nodep, bool setit) { - // Returns true to set useVirtualPublic(). - // If there's an implements of an interface class then we have - // multiple classes that point to same object, that need same - // VlClass (the diamond problem). C++ will require we use 'virtual - // public' for VlClass. So, we need the interface class, and all - // classes above, and any below using any implements to use - // 'virtual public' via useVirtualPublic(). - if (nodep->useVirtualPublic()) return true; // Short-circuit - if (nodep->isInterfaceClass()) setit = true; - for (const AstClassExtends* extp = nodep->extendsp(); extp; - extp = VN_AS(extp->nextp(), ClassExtends)) { - if (recurseImplements(extp->classp(), setit)) setit = true; - } - if (setit) { + void recurseImplements(AstClass* nodep) { + // In SystemVerilog, we have two inheritance chains: + // - extends of concrete clasess: mapped to non-virtual C++ inheritance + // as there is only single ancestor allowed + // - implements of concrete classes / extends of interface classes: mapped + // to virtual inheritance to allow diamond patterns with multiple ancestors + if (nodep->useVirtualPublic()) return; // Short-circuit to exit diamond cycles + if (nodep->isInterfaceClass()) { nodep->useVirtualPublic(true); - for (const AstClassExtends* extp = nodep->extendsp(); extp; - extp = VN_AS(extp->nextp(), ClassExtends)) { - (void)recurseImplements(extp->classp(), true); - } } - return setit; + for (const AstClassExtends* extp = nodep->extendsp(); extp; + extp = VN_AS(extp->nextp(), ClassExtends)) { + recurseImplements(extp->classp()); + } } // VISITORS @@ -89,7 +81,7 @@ class ClassVisitor final : public VNVisitor { nodep->name(m_prefix + nodep->name()); nodep->unlinkFrBack(); v3Global.rootp()->addModulesp(nodep); - (void)recurseImplements(nodep, false); + recurseImplements(nodep); // Make containing package // Note origName is the same as the class origName so errors look correct AstClassPackage* const packagep diff --git a/src/V3EmitCFunc.h b/src/V3EmitCFunc.h index 491e6c5a2..388c4e77c 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -261,33 +261,61 @@ public: if (const AstCNew* const cnewp = getSuperNewCallRecursep(nodep->nextp())) return cnewp; return nullptr; } - - void putConstructorSubinit(const AstClass* classp, AstCFunc* cfuncp, bool top, - std::set& doneClassesr) { - for (const AstClassExtends* extp = classp->extendsp(); extp; - extp = VN_AS(extp->nextp(), ClassExtends)) { - if (extp->classp()->useVirtualPublic()) { - // It's a c++ virtual class (diamond relation) - // Must get the subclasses initialized first - putConstructorSubinit(extp->classp(), cfuncp, false, doneClassesr); + void putConstructorSubinit(const AstClass* classp, AstCFunc* cfuncp) { + // Virtual bases in depth-first left-to-right order + std::vector virtualBases; + std::unordered_set doneClasses; + collectVirtualBasesRecursep(classp, virtualBases); + for (AstClass* vbase : virtualBases) { + if (doneClasses.count(vbase)) continue; + puts(doneClasses.empty() ? "" : "\n , "); + doneClasses.emplace(vbase); + puts(prefixNameProtect(vbase)); + if (constructorNeedsProcess(vbase)) { + puts("(vlProcess, vlSymsp)"); + } else { + puts("(vlSymsp)"); } - // Diamond pattern with same base class twice? - if (doneClassesr.find(extp->classp()) != doneClassesr.end()) continue; - puts(doneClassesr.empty() ? "" : "\n , "); - doneClassesr.emplace(extp->classp()); + } + const AstCNew* const superNewCallp = + getSuperNewCallRecursep(cfuncp->stmtsp()); + // Direct non-virtual bases in declaration order + for (const AstClassExtends* extp = classp->extendsp(); extp; + extp = VN_AS(extp->nextp(), ClassExtends)) { + if (extp->classp()->useVirtualPublic()) continue; + if (doneClasses.count(extp->classp())) continue; + puts(doneClasses.empty() ? "" : "\n , "); + doneClasses.emplace(extp->classp()); puts(prefixNameProtect(extp->classp())); if (constructorNeedsProcess(extp->classp())) { puts("(vlProcess, vlSymsp"); } else { puts("(vlSymsp"); } - if (top) { - const AstCNew* const superNewCallp = getSuperNewCallRecursep(cfuncp->stmtsp()); - UASSERT_OBJ(superNewCallp, cfuncp, "super.new call not found"); + // Handle super.new() args for the concrete parent + if (!extp->classp()->isInterfaceClass() && superNewCallp) { putCommaIterateNext(superNewCallp->argsp(), true); } puts(")"); - top = false; + } + } + void collectVirtualBasesRecursep(const AstClass* classp, + std::vector& virtualBases) { + std::set visited; + collectVirtualBasesRecursep(classp, virtualBases /*ref*/, visited /*ref*/); + } + void collectVirtualBasesRecursep(const AstClass* classp, + std::vector& virtualBases, + std::set& visited) { + if (visited.count(classp)) return; + visited.emplace(classp); + for (const AstClassExtends* extp = classp->extendsp(); extp; + extp = VN_AS(extp->nextp(), ClassExtends)) { + // Depth-first: recurse into this base first + collectVirtualBasesRecursep(extp->classp(), virtualBases, visited); + if (extp->classp()->useVirtualPublic()) { + virtualBases.push_back(extp->classp()); + } } } @@ -314,8 +342,7 @@ public: const AstClass* const classp = VN_CAST(nodep->scopep()->modp(), Class); if (nodep->isConstructor() && classp && classp->extendsp()) { puts("\n : "); - std::set doneClasses; - putConstructorSubinit(classp, nodep, true, doneClasses /*ref*/); + putConstructorSubinit(classp, nodep); } } puts(" {\n"); diff --git a/src/V3EmitCHeaders.cpp b/src/V3EmitCHeaders.cpp index f8ae0d50a..ddd73cf97 100644 --- a/src/V3EmitCHeaders.cpp +++ b/src/V3EmitCHeaders.cpp @@ -564,16 +564,20 @@ class EmitCHeader final : public EmitCConstInit { if (!VN_IS(modp, Class)) puts("alignas(VL_CACHE_LINE_BYTES) "); puts(prefixNameProtect(modp)); if (const AstClass* const classp = VN_CAST(modp, Class)) { - const string virtpub = classp->useVirtualPublic() ? "virtual public " : "public "; - puts(" : " + virtpub); + puts(" : "); if (classp->extendsp()) { + bool needComma = false; for (const AstClassExtends* extp = classp->extendsp(); extp; - extp = VN_AS(extp->nextp(), ClassExtends)) { + extp = VN_AS(extp->nextp(), ClassExtends)) { + if (needComma) puts(", "); + // Use virtual only for interfaces for class inheritance + // (extends) + puts(extp->classp()->useVirtualPublic() ? "virtual public " : "public "); putns(extp, prefixNameProtect(extp->classp())); - if (extp->nextp()) puts(", " + virtpub); + needComma = true; } } else { - puts("VlClass"); + puts("public virtual VlClass"); } } else { puts(" final : public VerilatedModule"); diff --git a/test_regress/t/t_class_hier_construction.py b/test_regress/t/t_class_hier_construction.py new file mode 100755 index 000000000..f989a35fb --- /dev/null +++ b/test_regress/t/t_class_hier_construction.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2025 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 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile() + +test.execute() + +test.passes() diff --git a/test_regress/t/t_class_hier_construction.v b/test_regress/t/t_class_hier_construction.v new file mode 100644 index 000000000..a3f60a566 --- /dev/null +++ b/test_regress/t/t_class_hier_construction.v @@ -0,0 +1,88 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2025 by Petr Nohavica +// SPDX-License-Identifier: CC0-1.0 + +`define stop $stop +`define checkh(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got='h%x exp='h%x\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got='%s' exp='%s'\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); + +interface class IBottomMid; + pure virtual function void moo(int i); +endclass + +interface class IBottom; + pure virtual function bit foo(); +endclass + +interface class IMid extends IBottomMid; + pure virtual function string bar(); +endclass + +class bottom_class implements IBottom; + string name; + + function new(string name); + this.name = name; + endfunction + + virtual function bit foo(); + $display("%s", name); + endfunction +endclass + +class middle_class extends bottom_class implements IMid, IBottom; + function new(string name); + super.new($sformatf("middle %0s", name)); + endfunction + + virtual function bit foo(); + $display("%s", name); + return 0; + endfunction + + virtual function void moo(int i); + $display("moo: %d", i); + endfunction + + virtual function string bar(); + return name; + endfunction +endclass + +class top_class extends middle_class; + int i; + function new(string name, int i); + super.new($sformatf("%0s %0d", name, i)); + this.i = i; + endfunction +endclass + +class sky_class extends top_class; + function new(string name); + super.new(name, 42); + endfunction +endclass + + +module t; + + initial begin + sky_class s = new("ahoj"); + bottom_class b = s; + top_class t = s; + IMid im; + + `checks( b.name, "middle ahoj 42" ); + `checks( s.name, "middle ahoj 42" ); + `checks( t.name, "middle ahoj 42" ); + `checkh( t.i, 42); + `checks(s.bar(), "middle ahoj 42"); + im = s; + im.moo(42); + + $finish; + end + +endmodule