From 21e85f87bc9e0426ec9dcb1622644ec523d51868 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Tue, 23 Jan 2024 19:36:11 -0500 Subject: [PATCH] Fix compilation error on multi-inherited interface class usage (#4819). --- Changes | 1 + src/V3AstNodeOther.h | 15 +++++------ src/V3AstNodes.cpp | 1 + src/V3Class.cpp | 27 +++++++++++++++++++ src/V3EmitCFunc.h | 43 ++++++++++++++++++++++--------- src/V3EmitCHeaders.cpp | 9 +++++-- src/V3Task.cpp | 8 +----- test_regress/t/t_class_imp2.pl | 21 +++++++++++++++ test_regress/t/t_class_imp2.v | 47 ++++++++++++++++++++++++++++++++++ 9 files changed, 143 insertions(+), 29 deletions(-) create mode 100755 test_regress/t/t_class_imp2.pl create mode 100644 test_regress/t/t_class_imp2.v diff --git a/Changes b/Changes index 26be978b1..5dd935a56 100644 --- a/Changes +++ b/Changes @@ -29,6 +29,7 @@ Verilator 5.021 devel * Fix delays using wrong timeunit when modules inlined (#4806). [Paul Wright] * Fix warnings in verilated_sc_trace.h for Clang. (#4807) (#4827). [Anthony Donlon] * Fix null pointer dereference (#4810) (#4825). [Adrian Sampson] +* Fix compilation error on multi-inherited interface class usage (#4819). * Fix maybe-uninitialized compiler warning (#4820) (#4822). [Larry Doolittle] * Fix mis-splitting of dump control functions (#4821). [Fan Shupei] * Fix wrong utimes() parameter (#4829). [Szymon Gizler] diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 2ca075540..cdb1e39b9 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -593,7 +593,6 @@ class AstCFunc final : public AstNode { string m_cname; // C name, for dpiExports string m_rtnType; // void, bool, or other return type string m_argTypes; // Argument types - string m_baseCtors; // Base class constructor string m_ifdef; // #ifdef symbol around this function VBoolOrUnknown m_isConst; // Function is declared const (*this not changed) bool m_isStatic : 1; // Function is static (no need for a 'this' pointer) @@ -656,8 +655,7 @@ public: bool same(const AstNode* samep) const override { const AstCFunc* const asamep = VN_DBG_AS(samep, CFunc); return ((isTrace() == asamep->isTrace()) && (rtnTypeVoid() == asamep->rtnTypeVoid()) - && (argTypes() == asamep->argTypes()) && (baseCtors() == asamep->baseCtors()) - && isLoose() == asamep->isLoose() + && (argTypes() == asamep->argTypes()) && isLoose() == asamep->isLoose() && (!(dpiImportPrototype() || dpiExportImpl()) || name() == asamep->name())); } // @@ -689,8 +687,6 @@ public: void funcPublic(bool flag) { m_funcPublic = flag; } void argTypes(const string& str) { m_argTypes = str; } string argTypes() const { return m_argTypes; } - void baseCtors(const string& str) { m_baseCtors = str; } - string baseCtors() const { return m_baseCtors; } void ifdef(const string& str) { m_ifdef = str; } string ifdef() const { return m_ifdef; } bool isConstructor() const { return m_isConstructor; } @@ -2255,8 +2251,9 @@ class AstClass final : public AstNodeModule { bool m_extended = false; // Is extension or extended by other classes bool m_interfaceClass = false; // Interface class bool m_needRNG = false; // Need RNG, uses srandom/randomize - bool m_virtual = false; // Virtual class bool m_parameterized = false; // Parameterized class + bool m_useVirtualPublic = false; // Subclasses need virtual public as uses interface class + bool m_virtual = false; // Virtual class public: AstClass(FileLine* fl, const string& name) @@ -2274,12 +2271,14 @@ public: void isExtended(bool flag) { m_extended = flag; } bool isInterfaceClass() const { return m_interfaceClass; } void isInterfaceClass(bool flag) { m_interfaceClass = flag; } + bool isParameterized() const { return m_parameterized; } + void isParameterized(bool flag) { m_parameterized = flag; } bool isVirtual() const { return m_virtual; } void isVirtual(bool flag) { m_virtual = flag; } bool needRNG() const { return m_needRNG; } void needRNG(bool flag) { m_needRNG = flag; } - bool isParameterized() const { return m_parameterized; } - void isParameterized(bool flag) { m_parameterized = flag; } + bool useVirtualPublic() const { return m_useVirtualPublic; } + void useVirtualPublic(bool flag) { m_useVirtualPublic = flag; } // Return true if this class is an extension of base class (SLOW) // Accepts nullptrs static bool isClassExtendedFrom(const AstClass* refClassp, const AstClass* baseClassp); diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 77e812c64..7b00c3bb1 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -1406,6 +1406,7 @@ void AstClass::dump(std::ostream& str) const { if (isExtended()) str << " [EXT]"; if (isInterfaceClass()) str << " [IFCCLS]"; if (isVirtual()) str << " [VIRT]"; + if (useVirtualPublic()) str << " [VIRPUB]"; } void AstClassExtends::dump(std::ostream& str) const { this->AstNode::dump(str); diff --git a/src/V3Class.cpp b/src/V3Class.cpp index 54ac0677b..c7f167ed8 100644 --- a/src/V3Class.cpp +++ b/src/V3Class.cpp @@ -49,12 +49,39 @@ 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) { + nodep->useVirtualPublic(true); + for (const AstClassExtends* extp = nodep->extendsp(); extp; + extp = VN_AS(extp->nextp(), ClassExtends)) { + (void)recurseImplements(extp->classp(), true); + } + } + return setit; + } + + // VISITORS + void visit(AstClass* nodep) override { if (nodep->user1SetOnce()) return; // Move this class nodep->name(m_prefix + nodep->name()); nodep->unlinkFrBack(); v3Global.rootp()->addModulesp(nodep); + (void)recurseImplements(nodep, false); // 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 7c85832c7..b68dc0c83 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -248,6 +248,32 @@ public: return nullptr; } + void putConstructorSubinit(const AstClass* classp, AstCFunc* cfuncp, bool top, bool& firstr) { + 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, firstr); + } + puts(firstr ? "" : "\n, "); + firstr = false; + 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"); + putCommaIterateNext(superNewCallp->argsp(), true); + } + puts(")"); + top = false; + } + } + // VISITORS using EmitCConstInit::visit; void visit(AstCFunc* nodep) override { @@ -265,20 +291,13 @@ public: if (nodep->isInline()) puts("VL_INLINE_OPT "); emitCFuncHeader(nodep, m_modp, /* withScope: */ true); - if (!nodep->baseCtors().empty()) { - puts(": "); - puts(nodep->baseCtors()); + if (nodep->isConstructor()) { const AstClass* const classp = VN_CAST(nodep->scopep()->modp(), Class); - // Find call to super.new to get the arguments - if (classp && constructorNeedsProcess(classp)) { - puts("(vlProcess, vlSymsp"); - } else { - puts("(vlSymsp"); + if (nodep->isConstructor() && classp && classp->extendsp()) { + puts("\n : "); + bool first = true; + putConstructorSubinit(classp, nodep, true, first /*ref*/); } - const AstCNew* const superNewCallp = getSuperNewCallRecursep(nodep->stmtsp()); - UASSERT_OBJ(superNewCallp, nodep, "super.new call not found"); - putCommaIterateNext(superNewCallp->argsp(), true); - puts(")"); } puts(" {\n"); diff --git a/src/V3EmitCHeaders.cpp b/src/V3EmitCHeaders.cpp index 795193e5a..499f1e6d4 100644 --- a/src/V3EmitCHeaders.cpp +++ b/src/V3EmitCHeaders.cpp @@ -316,9 +316,14 @@ 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)) { - puts(" : public "); + const string virtpub = classp->useVirtualPublic() ? "virtual public " : "public "; + puts(" : " + virtpub); if (classp->extendsp()) { - puts(prefixNameProtect(classp->extendsp()->classp())); + for (const AstClassExtends* extp = classp->extendsp(); extp; + extp = VN_AS(extp->nextp(), ClassExtends)) { + puts(prefixNameProtect(extp->classp())); + if (extp->nextp()) puts(", " + virtpub); + } } else { puts("VlClass"); } diff --git a/src/V3Task.cpp b/src/V3Task.cpp index 7c589b7b9..5c1708fdf 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -1223,13 +1223,7 @@ class TaskVisitor final : public VNVisitor { } cfuncp->isVirtual(nodep->isVirtual()); cfuncp->dpiPure(nodep->dpiPure()); - if (nodep->name() == "new") { - cfuncp->isConstructor(true); - AstClass* const classp = m_statep->getClassp(nodep); - if (classp->extendsp()) { - cfuncp->baseCtors(EmitCBase::prefixNameProtect(classp->extendsp()->classp())); - } - } + if (nodep->name() == "new") cfuncp->isConstructor(true); if (cfuncp->dpiExportImpl()) cfuncp->cname(nodep->cname()); if (!nodep->dpiImport() && !nodep->taskPublic()) { diff --git a/test_regress/t/t_class_imp2.pl b/test_regress/t/t_class_imp2.pl new file mode 100755 index 000000000..e64ab41be --- /dev/null +++ b/test_regress/t/t_class_imp2.pl @@ -0,0 +1,21 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2024 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(simulator => 1); + +compile( + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_class_imp2.v b/test_regress/t/t_class_imp2.v new file mode 100644 index 000000000..36d3922a5 --- /dev/null +++ b/test_regress/t/t_class_imp2.v @@ -0,0 +1,47 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2024 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t(/*AUTOARG*/); + + interface class Courier; + pure virtual function void deliver(); + endclass + class Person implements Courier; + virtual function void deliver(); + $display("slow delivery"); + endfunction + endclass + + interface class Seats; + pure virtual function int seats(); + endclass + + class Vehicle; + endclass + + class Car extends Vehicle implements Courier, Seats; + virtual function void deliver(); + $display("fast delivery"); + endfunction + virtual function int seats(); return 4; endfunction + endclass + + class MetaCar extends Car; + endclass + + function void do_delivery(Courier courier); + courier.deliver(); + endfunction + + initial begin + MetaCar car; + car = new(); + do_delivery(car); + if (car.seats() != 4) $stop; + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule