From 8d7a331d4054fbadbcacd2fecfa3c393878b6c1d Mon Sep 17 00:00:00 2001 From: Matthew Ballance Date: Tue, 7 Jul 2026 01:35:49 +0000 Subject: [PATCH] Prevent internal error for coverpoints that references a covergroup formal parameter Signed-off-by: Matthew Ballance --- src/V3Covergroup.cpp | 59 +++++++++++++------ .../t/t_covergroup_embedded_unsup.out | 8 ++- test_regress/t/t_covergroup_embedded_unsup.v | 42 ++++++++++--- 3 files changed, 83 insertions(+), 26 deletions(-) diff --git a/src/V3Covergroup.cpp b/src/V3Covergroup.cpp index f243b0c33..201564747 100644 --- a/src/V3Covergroup.cpp +++ b/src/V3Covergroup.cpp @@ -1692,21 +1692,23 @@ class FunctionalCoverageVisitor final : public VNVisitor { } // VISITORS - AstNode* findEnclosingMemberRef(AstClass* cgClassp) { + AstNode* findUnsupportedCoverpointRef(AstClass* cgClassp) { // An embedded covergroup is lowered into a sibling AstClass that has no handle to - // the enclosing object. A coverpoint/iff/cross expression that references a - // (non-static) member of the enclosing class therefore emits C++ that accesses the - // member as if it were static ("invalid use of non-static data member"). Detect - // such references so the caller can skip lowering with a clean warning instead of - // producing uncompilable code. Returns the first offending node, or nullptr. + // the enclosing object. Two coverpoint/iff/cross shapes cannot be lowered and would + // otherwise emit uncompilable C++ or crash V3Broken; detect them so the caller can + // skip lowering with a clean COVERIGN warning. Returns the first offending node + // (or nullptr), preferring the enclosing-member case for message clarity. // Collect the covergroup class's own member variables (sample/constructor args); // references to those are legitimate. std::set ownVars; for (AstNode* itemp = cgClassp->membersp(); itemp; itemp = itemp->nextp()) { if (const AstVar* const varp = VN_CAST(itemp, Var)) ownVars.insert(varp); } + // Pass 1: a (non-static) member of the enclosing class reached with no handle. The + // member would be emitted as if it were static ("invalid use of non-static data + // member"). AstNode* offenderp = nullptr; - const auto scan = [&](AstNode* rootp) { + const auto scanEnclosing = [&](AstNode* rootp) { rootp->foreach([&](AstVarRef* refp) { if (offenderp) return; const AstVar* const varp = refp->varp(); // Always set post-LinkDot @@ -1716,8 +1718,26 @@ class FunctionalCoverageVisitor final : public VNVisitor { if (varp->isClassMember() && !ownVars.count(varp)) offenderp = refp; }); }; - for (AstCoverpoint* cpp : m_coverpoints) scan(cpp); - for (AstCoverCross* crossp : m_coverCrosses) scan(crossp); + for (AstCoverpoint* cpp : m_coverpoints) scanEnclosing(cpp); + for (AstCoverCross* crossp : m_coverCrosses) scanEnclosing(crossp); + if (offenderp) return offenderp; + // Pass 2: the coverpoint expression dereferences a class handle to reach a member, + // e.g. a parameterized covergroup 'covergroup cg(cls st); coverpoint st.test;'. The + // lowered covergroup class cannot resolve the handle argument, so lowering leaves a + // dangling VarRef and aborts in V3Broken ("Broken link ... VARREF"). AstMemberSel is + // class-handle member access only (packed-struct selects use AstStructSel and options + // use AstCoverOption), so scanning the value/iff expression will not flag supported + // coverpoints. + const auto scanHandleDeref = [&](AstNode* rootp) { + if (!rootp) return; + rootp->foreach([&](AstMemberSel* selp) { + if (!offenderp) offenderp = selp; + }); + }; + for (AstCoverpoint* cpp : m_coverpoints) { + scanHandleDeref(cpp->exprp()); + scanHandleDeref(cpp->iffp()); + } return offenderp; } @@ -1802,15 +1822,20 @@ class FunctionalCoverageVisitor final : public VNVisitor { iterateChildren(nodep); // Option B safety net for embedded covergroups: if a coverpoint/iff/cross - // references a member of the enclosing class, lowering would emit uncompilable - // C++ (no handle to the enclosing instance). Skip this covergroup with a clean - // warning rather than crashing the C++ compile. (Full support - an enclosing - // back-pointer - is the planned follow-up.) - if (AstNode* const offenderp = findEnclosingMemberRef(nodep)) { + // references a member of the enclosing class (no handle to the enclosing + // instance), or dereferences a class-handle argument of a parameterized + // covergroup, lowering would emit uncompilable C++ or crash V3Broken with a + // dangling VarRef. Skip this covergroup with a clean warning rather than + // failing. (Full support - an enclosing back-pointer and handle-argument + // lowering - is the planned follow-up.) + if (AstNode* const offenderp = findUnsupportedCoverpointRef(nodep)) { + const bool viaHandle = VN_IS(offenderp, MemberSel); offenderp->v3warn(COVERIGN, - "Unsupported: 'covergroup' coverpoint referencing enclosing " - "class member; ignoring covergroup " - << nodep->prettyNameQ()); + "Unsupported: 'covergroup' coverpoint " + << (viaHandle ? "dereferencing a class handle member " + "(parameterized covergroup)" + : "referencing enclosing class member") + << "; ignoring covergroup " << nodep->prettyNameQ()); for (AstCoverpoint* cpp : m_coverpoints) { VL_DO_DANGLING(pushDeletep(cpp->unlinkFrBack()), cpp); } diff --git a/test_regress/t/t_covergroup_embedded_unsup.out b/test_regress/t/t_covergroup_embedded_unsup.out index 93cf7a3a5..3c09541ed 100644 --- a/test_regress/t/t_covergroup_embedded_unsup.out +++ b/test_regress/t/t_covergroup_embedded_unsup.out @@ -1,7 +1,11 @@ -%Warning-COVERIGN: t/t_covergroup_embedded_unsup.v:27:34: Unsupported: 'covergroup' coverpoint referencing enclosing class member; ignoring covergroup '__vlAnonCG_cov_trans' +%Warning-COVERIGN: t/t_covergroup_embedded_unsup.v:31:34: Unsupported: 'covergroup' coverpoint referencing enclosing class member; ignoring covergroup '__vlAnonCG_cov_trans' : ... note: In instance 't' - 27 | trans_start_addr: coverpoint trans_collected.addr {option.auto_bin_max = 16;} + 31 | trans_start_addr: coverpoint trans_collected.addr {option.auto_bin_max = 16;} | ^~~~~~~~~~~~~~~ ... For warning description see https://verilator.org/warn/COVERIGN?v=latest ... Use "/* verilator lint_off COVERIGN */" and lint_on around source to disable this message. +%Warning-COVERIGN: t/t_covergroup_embedded_unsup.v:54:23: Unsupported: 'covergroup' coverpoint dereferencing a class handle member (parameterized covergroup); ignoring covergroup '__vlAnonCG_cov_param' + : ... note: In instance 't' + 54 | cp: coverpoint st.test; + | ^~~~ %Error: Exiting due to diff --git a/test_regress/t/t_covergroup_embedded_unsup.v b/test_regress/t/t_covergroup_embedded_unsup.v index a35ffb840..5e3297dcf 100644 --- a/test_regress/t/t_covergroup_embedded_unsup.v +++ b/test_regress/t/t_covergroup_embedded_unsup.v @@ -5,13 +5,17 @@ // SPDX-FileCopyrightText: 2026 Wilson Snyder // SPDX-License-Identifier: CC0-1.0 -// Test the graceful-degradation safety net for embedded covergroups (the dominant -// UVM pattern: a covergroup declared inside a class whose coverpoints reference the -// enclosing object's members). Such a covergroup is lowered into a sibling class -// with no handle to the enclosing instance, so emitting it would produce -// uncompilable C++ ("invalid use of non-static data member"). Until the enclosing -// back-pointer feature exists, Verilator must emit a clean COVERIGN warning and skip -// lowering the covergroup, rather than crashing the C++ compile. +// Test the graceful-degradation safety net for covergroups that cannot yet be lowered. +// Two unsupported shapes must degrade to a clean COVERIGN warning (rather than emitting +// uncompilable C++ or crashing) until full support exists: +// 1. Embedded covergroup (the dominant UVM pattern): coverpoints reference members of +// the enclosing object. The covergroup is lowered into a sibling class with no +// handle to the enclosing instance, so emitting it would produce uncompilable C++ +// ("invalid use of non-static data member"). +// 2. Parameterized covergroup: a coverpoint dereferences a class-handle argument +// ('coverpoint st.test'). The lowered class cannot resolve the handle argument, so +// lowering leaves a dangling VarRef and aborts in V3Broken ("Broken link ... VARREF"). +// See https://github.com/verilator/verilator/issues/7853 class ubus_transfer; bit [15:0] addr; @@ -35,10 +39,34 @@ class ubus_master_monitor; endfunction endclass +class coverage_state; + bit [3:0] test; + bit [3:0] test2; +endclass + +class parameterized_monitor; + coverage_state cs; + + // Parameterized covergroup: the coverpoints dereference the class-handle argument 'st'. + // Two handle-dereferencing coverpoints ensure the safety net reports only the first + // offender (a second AstMemberSel is seen with the offender already latched). + covergroup cov_param(coverage_state st); + cp: coverpoint st.test; + cp2: coverpoint st.test2; + endgroup + + function new(); + cs = new; + cov_param = new(cs); + endfunction +endclass + module t; ubus_master_monitor m; + parameterized_monitor p; initial begin m = new; + p = new; $write("*-* All Finished *-*\n"); $finish; end