From 7da43d83028d38ea07e57f14e37e2aa956681e87 Mon Sep 17 00:00:00 2001 From: Marco Bartoli Date: Fri, 14 Aug 2026 02:41:39 +0200 Subject: [PATCH] Support class handle covergroup arguments (#8071) --- src/V3Covergroup.cpp | 30 ++++-- test_regress/t/t_covergroup_args.out | 2 + test_regress/t/t_covergroup_args.v | 17 ++++ test_regress/t/t_covergroup_embedded.out | 98 +++++++++++++++++++ test_regress/t/t_covergroup_embedded.v | 82 ++++++++++++++++ .../t/t_covergroup_embedded_unsup.out | 10 +- test_regress/t/t_covergroup_embedded_unsup.py | 2 +- test_regress/t/t_covergroup_embedded_unsup.v | 79 +++------------ 8 files changed, 241 insertions(+), 79 deletions(-) diff --git a/src/V3Covergroup.cpp b/src/V3Covergroup.cpp index dc1b5a2e7..c49e3294e 100644 --- a/src/V3Covergroup.cpp +++ b/src/V3Covergroup.cpp @@ -1959,6 +1959,7 @@ class FunctionalCoverageVisitor final : public VNVisitor { class FormalRefVisitor final : public VNVisitor { const std::set& m_constructorArgs; + const std::map& m_replacements; AstMemberSel* m_memberSelp = nullptr; AstNode* m_offenderp = nullptr; @@ -1969,7 +1970,11 @@ class FunctionalCoverageVisitor final : public VNVisitor { iterateChildren(nodep); } void visit(AstVarRef* nodep) override { - if (m_memberSelp && m_constructorArgs.count(nodep->varp())) { + if (!m_memberSelp) return; + const auto it = m_replacements.find(nodep->varp()); + if (it != m_replacements.end()) { + nodep->varp(it->second); + } else if (m_constructorArgs.count(nodep->varp())) { m_offenderp = m_memberSelp; } } @@ -1978,8 +1983,10 @@ class FunctionalCoverageVisitor final : public VNVisitor { } public: - explicit FormalRefVisitor(const std::set& constructorArgs) - : m_constructorArgs{constructorArgs} {} + FormalRefVisitor(const std::set& constructorArgs, + const std::map& replacements) + : m_constructorArgs{constructorArgs} + , m_replacements{replacements} {} void scan(AstNode* nodep) { if (nodep && !m_offenderp) iterate(nodep); } @@ -1988,12 +1995,19 @@ class FunctionalCoverageVisitor final : public VNVisitor { AstNode* findUnsupportedFormalRef() { std::set constructorArgs; + std::map replacements; for (AstNode* stmtp = m_constructorp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { - if (const AstVar* const varp = VN_CAST(stmtp, Var)) { - if (varp->isIO()) constructorArgs.insert(varp); - } + AstVar* const varp = VN_CAST(stmtp, Var); + if (!varp || !varp->isIO()) continue; + constructorArgs.insert(varp); + if (!VN_IS(varp->dtypep()->skipRefp(), ClassRefDType)) continue; + AstVar* const memberp + = VN_CAST(m_memberMap.findMember(m_covergroupp, varp->name()), Var); + UASSERT_OBJ(memberp && memberp->isClassMember(), varp, + "Covergroup constructor argument missing persistent member"); + replacements.emplace(varp, memberp); } - FormalRefVisitor visitor{constructorArgs}; + FormalRefVisitor visitor{constructorArgs, replacements}; for (AstCoverpoint* const cpp : m_coverpoints) { visitor.scan(cpp->exprp()); visitor.scan(cpp->iffp()); @@ -2167,7 +2181,7 @@ class FunctionalCoverageVisitor final : public VNVisitor { if (AstNode* const offenderp = findUnsupportedFormalRef()) { offenderp->v3warn(COVERIGN, "Unsupported: 'covergroup' coverpoint dereferencing a " - "class handle member; ignoring covergroup " + "non-class constructor argument; ignoring covergroup " << nodep->prettyNameQ()); deleteCoverageItems(); if (embeddedEventForkp) { diff --git a/test_regress/t/t_covergroup_args.out b/test_regress/t/t_covergroup_args.out index 80f671977..69ff229d6 100644 --- a/test_regress/t/t_covergroup_args.out +++ b/test_regress/t/t_covergroup_args.out @@ -1,3 +1,5 @@ +__vlAnonCG_cg.__Vcoverpoint0.auto_0: 1 +__vlAnonCG_cg.__Vcoverpoint0.auto_1: 0 cg.cp1.hi: 0 cg.cp1.lo: 1 cg_clocked.cp_clocked.hi: 0 diff --git a/test_regress/t/t_covergroup_args.v b/test_regress/t/t_covergroup_args.v index fee8f4542..6837fb0af 100644 --- a/test_regress/t/t_covergroup_args.v +++ b/test_regress/t/t_covergroup_args.v @@ -9,6 +9,21 @@ class PlainClass; int x; endclass +class CoverageState; + bit test; +endclass + +class Coverage; + covergroup cg(CoverageState st); + coverpoint st.test; + endgroup + CoverageState state; + function new(); + state = new(); + cg = new(state); + endfunction +endclass + // Top-level (file-scope) covergroup declared outside any module covergroup cg_toplevel; cp_tl: coverpoint 0; @@ -39,6 +54,7 @@ module t; cg cov2 = new(69); cg_clocked cov_clocked = new(10); cg_samp cov_samp = new; + Coverage cov = new; PlainClass plain_inst = new; // Non-covergroup class instance - must not affect covergroup coverage function void x(); @@ -82,6 +98,7 @@ module t; // default 0) b3 would never be hit. cov_samp.sample(2'd0); cov_samp.sample(2'd3); + cov.cg.sample(); $finish; end diff --git a/test_regress/t/t_covergroup_embedded.out b/test_regress/t/t_covergroup_embedded.out index 1ede10263..aa3c1c6e9 100644 --- a/test_regress/t/t_covergroup_embedded.out +++ b/test_regress/t/t_covergroup_embedded.out @@ -58,6 +58,104 @@ __vlAnonCG_clock_cg.cp_clocked.hi: 8 __vlAnonCG_clock_cg.cp_clocked.lo: 8 __vlAnonCG_copy_cg.cp_copy.hi: 0 __vlAnonCG_copy_cg.cp_copy.lo: 1 +__vlAnonCG_cov_mixed.cp.auto_0: 0 +__vlAnonCG_cov_mixed.cp.auto_1: 0 +__vlAnonCG_cov_mixed.cp.auto_10: 0 +__vlAnonCG_cov_mixed.cp.auto_11: 0 +__vlAnonCG_cov_mixed.cp.auto_12: 0 +__vlAnonCG_cov_mixed.cp.auto_13: 0 +__vlAnonCG_cov_mixed.cp.auto_14: 0 +__vlAnonCG_cov_mixed.cp.auto_15: 16 +__vlAnonCG_cov_mixed.cp.auto_2: 0 +__vlAnonCG_cov_mixed.cp.auto_3: 0 +__vlAnonCG_cov_mixed.cp.auto_4: 0 +__vlAnonCG_cov_mixed.cp.auto_5: 0 +__vlAnonCG_cov_mixed.cp.auto_6: 0 +__vlAnonCG_cov_mixed.cp.auto_7: 0 +__vlAnonCG_cov_mixed.cp.auto_8: 0 +__vlAnonCG_cov_mixed.cp.auto_9: 0 +__vlAnonCG_cov_param.cp.auto_0: 1 +__vlAnonCG_cov_param.cp.auto_1: 1 +__vlAnonCG_cov_param.cp.auto_10: 1 +__vlAnonCG_cov_param.cp.auto_11: 1 +__vlAnonCG_cov_param.cp.auto_12: 1 +__vlAnonCG_cov_param.cp.auto_13: 1 +__vlAnonCG_cov_param.cp.auto_14: 1 +__vlAnonCG_cov_param.cp.auto_15: 1 +__vlAnonCG_cov_param.cp.auto_2: 1 +__vlAnonCG_cov_param.cp.auto_3: 1 +__vlAnonCG_cov_param.cp.auto_4: 1 +__vlAnonCG_cov_param.cp.auto_5: 1 +__vlAnonCG_cov_param.cp.auto_6: 1 +__vlAnonCG_cov_param.cp.auto_7: 1 +__vlAnonCG_cov_param.cp.auto_8: 1 +__vlAnonCG_cov_param.cp.auto_9: 1 +__vlAnonCG_cov_param.cp2.auto_0: 1 +__vlAnonCG_cov_param.cp2.auto_1: 1 +__vlAnonCG_cov_param.cp2.auto_10: 1 +__vlAnonCG_cov_param.cp2.auto_11: 1 +__vlAnonCG_cov_param.cp2.auto_12: 1 +__vlAnonCG_cov_param.cp2.auto_13: 1 +__vlAnonCG_cov_param.cp2.auto_14: 1 +__vlAnonCG_cov_param.cp2.auto_15: 1 +__vlAnonCG_cov_param.cp2.auto_2: 1 +__vlAnonCG_cov_param.cp2.auto_3: 1 +__vlAnonCG_cov_param.cp2.auto_4: 1 +__vlAnonCG_cov_param.cp2.auto_5: 1 +__vlAnonCG_cov_param.cp2.auto_6: 1 +__vlAnonCG_cov_param.cp2.auto_7: 1 +__vlAnonCG_cov_param.cp2.auto_8: 1 +__vlAnonCG_cov_param.cp2.auto_9: 1 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_0_x_auto_0 [cross]: 8 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_0_x_auto_1 [cross]: 8 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_10_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_10_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_11_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_11_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_12_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_12_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_13_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_13_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_14_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_14_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_15_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_15_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_1_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_1_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_2_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_2_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_3_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_3_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_4_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_4_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_5_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_5_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_6_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_6_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_7_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_7_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_8_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_8_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_9_x_auto_0 [cross]: 0 +__vlAnonCG_cov_trans.trans_addr_x_dir.auto_9_x_auto_1 [cross]: 0 +__vlAnonCG_cov_trans.trans_dir.auto_0: 8 +__vlAnonCG_cov_trans.trans_dir.auto_1: 8 +__vlAnonCG_cov_trans.trans_start_addr.auto_0: 16 +__vlAnonCG_cov_trans.trans_start_addr.auto_1: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_10: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_11: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_12: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_13: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_14: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_15: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_2: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_3: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_4: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_5: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_6: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_7: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_8: 0 +__vlAnonCG_cov_trans.trans_start_addr.auto_9: 0 __vlAnonCG_derived_cg.cp_cross.hi_x_hi [cross]: 0 __vlAnonCG_derived_cg.cp_cross.hi_x_lo [cross]: 8 __vlAnonCG_derived_cg.cp_cross.lo_x_hi [cross]: 8 diff --git a/test_regress/t/t_covergroup_embedded.v b/test_regress/t/t_covergroup_embedded.v index a78cab6ff..2b99e5a65 100644 --- a/test_regress/t/t_covergroup_embedded.v +++ b/test_regress/t/t_covergroup_embedded.v @@ -69,6 +69,79 @@ class Monitor; endfunction endclass +class UbusTransfer; + bit [15:0] addr; + bit read_write; +endclass + +class UbusMasterMonitor; + UbusTransfer trans_collected; + + covergroup cov_trans; + trans_start_addr: coverpoint trans_collected.addr {option.auto_bin_max = 16;} + trans_dir: coverpoint trans_collected.read_write; + trans_addr_x_dir: cross trans_start_addr, trans_dir; + endgroup + + function new(); + trans_collected = new; + cov_trans = new; + endfunction + + function void observe(bit [15:0] addr, bit read_write); + trans_collected.addr = addr; + trans_collected.read_write = read_write; + cov_trans.sample(); + endfunction +endclass + +class CoverageState; + bit [3:0] test; + bit [3:0] test2; +endclass + +class ParameterizedMonitor; + CoverageState state; + bit clk; + + covergroup cov_param(CoverageState st) @(posedge clk); + cp: coverpoint st.test; + cp2: coverpoint st.test2; + endgroup + + function new(); + state = new; + cov_param = new(state); + endfunction + + function void observe(bit [3:0] test, bit [3:0] test2); + state.test = test; + state.test2 = test2; + clk = 0; + clk = 1; + endfunction +endclass + +class MixedMonitor; + bit [3:0] local_value; + CoverageState state; + + covergroup cov_mixed(CoverageState st); + cp: coverpoint local_value + st.test; + endgroup + + function new(); + state = new; + cov_mixed = new(state); + endfunction + + function void observe(bit [3:0] local_value, bit [3:0] test); + this.local_value = local_value; + state.test = test; + cov_mixed.sample(); + endfunction +endclass + class BranchMonitor; bit [2:0] value; @@ -471,6 +544,9 @@ endclass module t; Monitor mon; + UbusMasterMonitor ubus_mon; + ParameterizedMonitor parameterized_arg_mon; + MixedMonitor mixed_arg_mon; BranchMonitor branch_a; BranchMonitor branch_b; DerivedMonitor derived; @@ -500,6 +576,9 @@ module t; initial begin mon = new; + ubus_mon = new; + parameterized_arg_mon = new; + mixed_arg_mon = new; branch_a = new(1); branch_b = new(0); derived = new; @@ -525,6 +604,9 @@ module t; for (i = 0; i < 16; ++i) begin mon.observe(i[3:0], i[7:0] * 17, i[1:0], i[3:0]); + ubus_mon.observe(i[15:0], i[0]); + parameterized_arg_mon.observe(i[3:0], 15 - i[3:0]); + mixed_arg_mon.observe(i[3:0], 15 - i[3:0]); derived.observe(i[3:0]); leaf.observe(i[3:0]); parameterized.observe(i[3:0]); diff --git a/test_regress/t/t_covergroup_embedded_unsup.out b/test_regress/t/t_covergroup_embedded_unsup.out index 3d5ecc1fb..18fc4eb92 100644 --- a/test_regress/t/t_covergroup_embedded_unsup.out +++ b/test_regress/t/t_covergroup_embedded_unsup.out @@ -1,11 +1,7 @@ -%Warning-COVERIGN: t/t_covergroup_embedded_unsup.v:45:23: Unsupported: 'covergroup' coverpoint dereferencing a class handle member; ignoring covergroup '__vlAnonCG_cov_param' +%Warning-COVERIGN: t/t_covergroup_embedded_unsup.v:19:24: Unsupported: 'covergroup' coverpoint dereferencing a non-class constructor argument; ignoring covergroup '__vlAnonCG_cov_interface' : ... note: In instance 't' - 45 | cp: coverpoint st.test; - | ^~~~ + 19 | cp: coverpoint vif.test; + | ^~~~ ... 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:61:37: Unsupported: 'covergroup' coverpoint dereferencing a class handle member; ignoring covergroup '__vlAnonCG_cov_mixed' - : ... note: In instance 't' - 61 | cp: coverpoint local_value + st.test; - | ^~~~ %Error: Exiting due to diff --git a/test_regress/t/t_covergroup_embedded_unsup.py b/test_regress/t/t_covergroup_embedded_unsup.py index 18c00bc38..77a0ac64b 100755 --- a/test_regress/t/t_covergroup_embedded_unsup.py +++ b/test_regress/t/t_covergroup_embedded_unsup.py @@ -11,6 +11,6 @@ import vltest_bootstrap test.scenarios('vlt') -test.lint(verilator_flags2=['--timing'], expect_filename=test.golden_filename, fails=True) +test.lint(expect_filename=test.golden_filename, fails=True) test.passes() diff --git a/test_regress/t/t_covergroup_embedded_unsup.v b/test_regress/t/t_covergroup_embedded_unsup.v index 5a1fb6a9d..f2ce549b7 100644 --- a/test_regress/t/t_covergroup_embedded_unsup.v +++ b/test_regress/t/t_covergroup_embedded_unsup.v @@ -5,77 +5,30 @@ // SPDX-FileCopyrightText: 2026 Wilson Snyder // SPDX-License-Identifier: CC0-1.0 -// Test that unsupported coverpoint reference styles are properly flagged as COVERIGN. +// Class-handle covergroup constructor arguments are rebound to persistent +// covergroup members. Virtual-interface arguments are not rebound, so +// dereferencing one after construction is unsupported and the covergroup must +// be ignored with COVERIGN. -class ubus_transfer; - bit [15:0] addr; - bit read_write; -endclass +interface CoverageInterface; + bit test; +endinterface -class ubus_master_monitor; - ubus_transfer trans_collected; - - // Coverpoints reference 'trans_collected', a member of the enclosing class. - // A cross is included so the safety-net cleanup also exercises cross removal. - covergroup cov_trans; - trans_start_addr: coverpoint trans_collected.addr {option.auto_bin_max = 16;} - trans_dir: coverpoint trans_collected.read_write; - trans_addr_x_dir : cross trans_start_addr, trans_dir; +class InterfaceArgumentMonitor; + covergroup cov_interface(virtual CoverageInterface vif); + cp: coverpoint vif.test; endgroup - function new(); - trans_collected = new; - cov_trans = new; - endfunction -endclass - -class coverage_state; - bit [3:0] test; - bit [3:0] test2; -endclass - -class parameterized_monitor; - coverage_state cs; - bit clk; - - // 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) @(posedge clk); - cp: coverpoint st.test; - cp2: coverpoint st.test2; - endgroup - - function new(); - cs = new; - cov_param = new(cs); - endfunction -endclass - -class mixed_monitor; - bit [3:0] local_value; - coverage_state cs; - - // The formal-handle guard must still apply when the expression also has an enclosing member. - covergroup cov_mixed(coverage_state st); - cp: coverpoint local_value + st.test; - endgroup - - function new(); - cs = new; - cov_mixed = new(cs); + function new(virtual CoverageInterface vif); + cov_interface = new(vif); endfunction endclass module t; - ubus_master_monitor m; - parameterized_monitor p; - mixed_monitor q; + CoverageInterface coverage_interface(); + InterfaceArgumentMonitor mon; + initial begin - m = new; - p = new; - q = new; - $write("*-* All Finished *-*\n"); - $finish; + mon = new(coverage_interface); end endmodule