From 29fd2cf90dc4c7a07dac4a6ed2e12270e0992365 Mon Sep 17 00:00:00 2001 From: Aisha <87562264+aishasalim@users.noreply.github.com> Date: Mon, 3 Aug 2026 03:54:43 -0500 Subject: [PATCH] Add MULTIDRIVEN checks for clocking block outputs (#7987) --- docs/guide/warnings.rst | 13 +++ src/V3Undriven.cpp | 101 +++++++++++++++++- .../t/t_lint_multidriven_clocking_bad.out | 45 ++++++++ .../t/t_lint_multidriven_clocking_bad.py | 25 +++++ .../t/t_lint_multidriven_clocking_bad.v | 70 ++++++++++++ 5 files changed, 251 insertions(+), 3 deletions(-) create mode 100644 test_regress/t/t_lint_multidriven_clocking_bad.out create mode 100755 test_regress/t/t_lint_multidriven_clocking_bad.py create mode 100644 test_regress/t/t_lint_multidriven_clocking_bad.v diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 04a955e9c..0fcc0475b 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -1469,6 +1469,13 @@ List Of Warnings q <= d; end + A further case is when a signal named as a clocking block ``output`` is + also driven by a continuous assignment, or is named as an ``output`` of a + second clocking block. The clocking block drives the signal, so the design + and the testbench contend for it and the synchronous drive may be silently + lost. Declare the clocking block ``input`` if the intent is only to + observe the signal. + Ignoring this warning may hide clock domain crossing, timing, or portability bugs. It may also cause longer simulation runtimes due to reduced optimizations. @@ -1495,6 +1502,12 @@ List Of Warnings .. include:: ../../docs/gen/ex_MULTIDRIVENPROC_msg.rst + Also warns when a signal named as a clocking block ``output`` is driven + by a plain ``always`` block. Driving a signal from both a clocking block + and a plain ``always`` block is a deliberate idiom in some testbenches, + so it is reported as MULTIDRIVENPROC rather than under the on-by-default + :option:`MULTIDRIVEN`. + To fix, drive the signal from a single ``always`` block, or use ``always_ff``/``always_comb`` if the intent is a single specialized process. diff --git a/src/V3Undriven.cpp b/src/V3Undriven.cpp index 48895ba17..b4bfb98ba 100644 --- a/src/V3Undriven.cpp +++ b/src/V3Undriven.cpp @@ -48,6 +48,7 @@ class UndrivenVarEntry final { = nullptr; // always_comb of var if driven within always_comb, else nullptr const AstAlways* m_alwFFp = nullptr; // always_ff of var if driven within always_ff const AstAlways* m_alwPlainp = nullptr; // plain always of var if driven within plain always + const AstClocking* m_clockingp = nullptr; // clocking block of var if driven as output const AstNodeVarRef* m_nodep = nullptr; // varref if driven, else nullptr const AstNode* m_initStaticp = nullptr; // varref if in InitialStatic driven const AstNode* m_initialp = nullptr; // varref if driven in an explicit initial block @@ -64,7 +65,8 @@ class UndrivenVarEntry final { FLAG_DRIVEN_ALWCOMB = 2, // Whole signal has been driven from always_comb FLAG_DRIVEN_ALWFF = 3, // Whole signal has been driven from always_ff FLAG_DRIVEN_ALWPLAIN = 4, // Whole signal has been driven from a plain always - FLAGS_PER_BIT = 5 // Number of flags stored for each tracked bit + FLAG_DRIVEN_CLOCKING = 5, // Whole signal has been driven as a clocking block output + FLAGS_PER_BIT = 6 // Number of flags stored for each tracked bit }; public: @@ -164,6 +166,10 @@ public: m_wholeFlags[FLAG_DRIVEN_ALWPLAIN] = true; m_alwPlainp = alwPlainp; } + void drivenClockingWhole(const AstClocking* clockingp) { + m_wholeFlags[FLAG_DRIVEN_CLOCKING] = true; + m_clockingp = clockingp; + } const AstNode* initStaticp() const { return m_initStaticp; } void initStaticp(const AstNode* nodep) { m_initStaticp = nodep; } @@ -179,11 +185,13 @@ public: bool isDrivenAlwaysCombWhole() const { return m_wholeFlags[FLAG_DRIVEN_ALWCOMB]; } bool isDrivenAlwaysFFWhole() const { return m_wholeFlags[FLAG_DRIVEN_ALWFF]; } bool isDrivenAlwaysPlainWhole() const { return m_wholeFlags[FLAG_DRIVEN_ALWPLAIN]; } + bool isDrivenClockingWhole() const { return m_wholeFlags[FLAG_DRIVEN_CLOCKING]; } bool isFtaskDriven() const { return m_ftaskDriven; } const AstNodeVarRef* getNodep() const { return m_nodep; } const AstAlways* getAlwCombp() const { return m_alwCombp; } const AstAlways* getAlwFFp() const { return m_alwFFp; } const AstAlways* getAlwPlainp() const { return m_alwPlainp; } + const AstClocking* getClockingp() const { return m_clockingp; } void usedBit(int bit, int width, const AstNode* nodep) { UINFO(9, "set u[" << (bit + width - 1) << ":" << bit << "] " << m_varp->name()); for (int i = 0; i < width; i++) { @@ -384,6 +392,7 @@ class UndrivenVisitor final : public VNVisitorConst { const AstAlways* m_alwaysCombp = nullptr; // Current always if combo, otherwise nullptr const AstAlways* m_alwaysFFp = nullptr; // Current always if ff, otherwise nullptr const AstAlways* m_alwaysPlainp = nullptr; // Current always if plain (not comb/ff/latch) + const AstClocking* m_clockingp = nullptr; // Current clocking block, otherwise nullptr V3UndrivenCapture* const m_capturep = nullptr; // Capture object. 'nullptr' if disabled. @@ -431,6 +440,84 @@ class UndrivenVisitor final : public VNVisitorConst { true); // Complain just once for any usage } } + // A clocking block 'output' is an additional driver of the signal it names. + // The conflict is found from whichever driver is reached second. + bool clockingDrivesOther(const UndrivenVarEntry* entryp, bool otherWriteIsStaticInit) const { + const bool otherIsExplicitProc + = entryp->isDrivenAlwaysCombWhole() || entryp->isDrivenAlwaysFFWhole(); + return m_clockingp && !otherIsExplicitProc && !otherWriteIsStaticInit + && m_clockingp != entryp->getClockingp(); + } + bool otherDrivesClocking(const UndrivenVarEntry* entryp) const { + return !m_clockingp && !m_alwaysCombp && !m_alwaysFFp && !m_inInitialStatic + && entryp->isDrivenClockingWhole(); + } + // A continuous assignment or a second clocking block contending with a + // clocking block 'output' is an unambiguous driver conflict. + void warnClockingDriven(AstNodeVarRef* nodep, const UndrivenVarEntry* entryp, + const AstNode* otherWritep, bool otherWriteIsStaticInit) { + const AstClocking* const otherClockingp = entryp->getClockingp(); + if (clockingDrivesOther(entryp, otherWriteIsStaticInit)) { + if (otherClockingp) { + nodep->v3warn(MULTIDRIVEN, + "Variable written to in clocking block also written by another " + "clocking block" + << " (IEEE 1800-2023 14.3): " << nodep->prettyNameQ() << '\n' + << nodep->warnOther() << '\n' + << nodep->warnContextPrimary() << '\n' + << otherWritep->warnOther() + << "... Location of other clocking block output\n" + << otherWritep->warnContextSecondary()); + } else if (otherWritep == entryp->contAssignp()) { + nodep->v3warn(MULTIDRIVEN, + "Variable written to in clocking block also driven by continuous " + "assignment" + << " (IEEE 1800-2023 14.3): " << nodep->prettyNameQ() << '\n' + << nodep->warnOther() << '\n' + << nodep->warnContextPrimary() << '\n' + << otherWritep->warnOther() + << "... Location of continuous assignment\n" + << otherWritep->warnContextSecondary()); + } + } + if (otherDrivesClocking(entryp) && m_inContAssign) { + nodep->v3warn(MULTIDRIVEN, + "Variable driven by continuous assignment also written to in " + "clocking block" + << " (IEEE 1800-2023 14.3): " << nodep->prettyNameQ() << '\n' + << nodep->warnOther() << '\n' + << nodep->warnContextPrimary() << '\n' + << otherWritep->warnOther() + << "... Location of clocking block output\n" + << otherWritep->warnContextSecondary()); + } + } + // Driving a signal from both a clocking block and a plain process is a deliberate + // idiom in some testbenches, as with the plain always conflicts above. + void warnClockingDrivenProc(AstNodeVarRef* nodep, const UndrivenVarEntry* entryp, + const AstNode* otherWritep, bool otherWriteIsStaticInit) { + if (clockingDrivesOther(entryp, otherWriteIsStaticInit) && !entryp->getClockingp() + && otherWritep != entryp->contAssignp()) { + nodep->v3warn(MULTIDRIVENPROC, + "Variable written to in clocking block also written by another " + "process" + << " (IEEE 1800-2023 14.3): " << nodep->prettyNameQ() << '\n' + << nodep->warnOther() << '\n' + << nodep->warnContextPrimary() << '\n' + << otherWritep->warnOther() << "... Location of other write\n" + << otherWritep->warnContextSecondary()); + } + if (otherDrivesClocking(entryp) && !m_inContAssign) { + nodep->v3warn(MULTIDRIVENPROC, + "Variable written to in process also written to in clocking block" + << " (IEEE 1800-2023 14.3): " << nodep->prettyNameQ() << '\n' + << nodep->warnOther() << '\n' + << nodep->warnContextPrimary() << '\n' + << otherWritep->warnOther() + << "... Location of clocking block output\n" + << otherWritep->warnContextSecondary()); + } + } // VISITORS void visit(AstVar* nodep) override { @@ -551,10 +638,10 @@ class UndrivenVisitor final : public VNVisitorConst { // declaration's fileline, as v3warn suppression will check // the driving fileline and still warn even if the warning // was suppressed with lint_off at the declaration. + const bool otherWriteIsStaticInit + = nodep->varp()->hasUserInit() && otherWritep == entryp->initStaticp(); if (multidrivenCommon && !nodep->varp()->fileline()->warnIsOff(V3ErrorCode::MULTIDRIVEN)) { - const bool otherWriteIsStaticInit - = nodep->varp()->hasUserInit() && otherWritep == entryp->initStaticp(); if (m_alwaysCombp && (!entryp->isDrivenAlwaysCombWhole() || (m_alwaysCombp != entryp->getAlwCombp() @@ -602,6 +689,7 @@ class UndrivenVisitor final : public VNVisitorConst { << "... Location of always_ff write\n" << otherWritep->warnContextSecondary()); } + warnClockingDriven(nodep, entryp, otherWritep, otherWriteIsStaticInit); } if (multidrivenCommon && !nodep->varp()->fileline()->warnIsOff(V3ErrorCode::MULTIDRIVENPROC)) { @@ -634,6 +722,7 @@ class UndrivenVisitor final : public VNVisitorConst { << otherWritep->warnOther() << "... Location of other write\n" << otherWritep->warnContextSecondary()); } + warnClockingDrivenProc(nodep, entryp, otherWritep, otherWriteIsStaticInit); } if (!m_inInitialSetup || nodep->varp()->hasUserInit()) { // Else don't count default initialization as a driver to a net/variable @@ -646,6 +735,7 @@ class UndrivenVisitor final : public VNVisitorConst { if (m_alwaysCombp) entryp->drivenAlwaysCombWhole(m_alwaysCombp); if (m_alwaysFFp) entryp->drivenAlwaysFFWhole(m_alwaysFFp, nodep->varp()); if (m_alwaysPlainp) entryp->drivenAlwaysPlainWhole(m_alwaysPlainp); + if (m_clockingp) entryp->drivenClockingWhole(m_clockingp); } if (nodep->access().isWriteOrRW() && !VN_IS(nodep, VarXRef)) { // Ignoring xrefs as the initial and assignment to track might refer to two @@ -751,6 +841,11 @@ class UndrivenVisitor final : public VNVisitorConst { iterateChildrenConst(nodep); if (nodep->keyword() == VAlwaysKwd::ALWAYS_COMB) UINFO(9, " Done " << nodep); } + void visit(AstClocking* nodep) override { + VL_RESTORER(m_clockingp); + m_clockingp = nodep; + iterateChildrenConst(nodep); + } void visit(AstNodeFTaskRef* nodep) override { VL_RESTORER(m_inFTaskRef); diff --git a/test_regress/t/t_lint_multidriven_clocking_bad.out b/test_regress/t/t_lint_multidriven_clocking_bad.out new file mode 100644 index 000000000..d0bc0f772 --- /dev/null +++ b/test_regress/t/t_lint_multidriven_clocking_bad.out @@ -0,0 +1,45 @@ +%Warning-MULTIDRIVEN: t/t_lint_multidriven_clocking_bad.v:21:10: Variable driven by continuous assignment also written to in clocking block (IEEE 1800-2023 14.3): 'early_assign' + : ... note: In instance 't' + t/t_lint_multidriven_clocking_bad.v:21:10: + 21 | assign early_assign = 1'b1; + | ^~~~~~~~~~~~ + t/t_lint_multidriven_clocking_bad.v:17:12: ... Location of clocking block output + 17 | output early_assign; + | ^~~~~~~~~~~~ + ... For warning description see https://verilator.org/warn/MULTIDRIVEN?v=latest + ... Use "/* verilator lint_off MULTIDRIVEN */" and lint_on around source to disable this message. +%Warning-MULTIDRIVENPROC: t/t_lint_multidriven_clocking_bad.v:22:25: Variable written to in process also written to in clocking block (IEEE 1800-2023 14.3): 'early_always' + : ... note: In instance 't' + t/t_lint_multidriven_clocking_bad.v:22:25: + 22 | always @(posedge clk) early_always <= 1'b1; + | ^~~~~~~~~~~~ + t/t_lint_multidriven_clocking_bad.v:18:12: ... Location of clocking block output + 18 | output early_always; + | ^~~~~~~~~~~~ + ... For warning description see https://verilator.org/warn/MULTIDRIVENPROC?v=latest + ... Use "/* verilator lint_off MULTIDRIVENPROC */" and lint_on around source to disable this message. +%Warning-MULTIDRIVEN: t/t_lint_multidriven_clocking_bad.v:45:12: Variable written to in clocking block also driven by continuous assignment (IEEE 1800-2023 14.3): 'late_assign' + : ... note: In instance 't' + t/t_lint_multidriven_clocking_bad.v:45:12: + 45 | output late_assign; + | ^~~~~~~~~~~ + t/t_lint_multidriven_clocking_bad.v:27:10: ... Location of continuous assignment + 27 | assign late_assign = 1'b1; + | ^~~~~~~~~~~ +%Warning-MULTIDRIVENPROC: t/t_lint_multidriven_clocking_bad.v:46:12: Variable written to in clocking block also written by another process (IEEE 1800-2023 14.3): 'late_always' + : ... note: In instance 't' + t/t_lint_multidriven_clocking_bad.v:46:12: + 46 | output late_always; + | ^~~~~~~~~~~ + t/t_lint_multidriven_clocking_bad.v:32:25: ... Location of other write + 32 | always @(posedge clk) late_always <= 1'b1; + | ^~~~~~~~~~~ +%Warning-MULTIDRIVEN: t/t_lint_multidriven_clocking_bad.v:53:12: Variable written to in clocking block also written by another clocking block (IEEE 1800-2023 14.3): 'dual_clocking' + : ... note: In instance 't' + t/t_lint_multidriven_clocking_bad.v:53:12: + 53 | output dual_clocking; + | ^~~~~~~~~~~~~ + t/t_lint_multidriven_clocking_bad.v:47:12: ... Location of other clocking block output + 47 | output dual_clocking; + | ^~~~~~~~~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_multidriven_clocking_bad.py b/test_regress/t/t_lint_multidriven_clocking_bad.py new file mode 100755 index 000000000..b835bea38 --- /dev/null +++ b/test_regress/t/t_lint_multidriven_clocking_bad.py @@ -0,0 +1,25 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# 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-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('linter') + +# A clocking block 'output' drives the signal it names. Against a continuous +# assignment or a second clocking block that is an unambiguous driver conflict +# (MULTIDRIVEN, on by default); against a plain always block it is a common +# testbench idiom, so it is only reported under MULTIDRIVENPROC, enabled here +# explicitly. Signals driven by a single clocking block, only read by one, named +# by two clockvars of one clocking block, or carrying just a declaration +# initializer must not warn at all. +test.lint(fails=True, + verilator_flags2=['-Wwarn-MULTIDRIVENPROC'], + expect_filename=test.golden_filename) + +test.passes() diff --git a/test_regress/t/t_lint_multidriven_clocking_bad.v b/test_regress/t/t_lint_multidriven_clocking_bad.v new file mode 100644 index 000000000..f2ac7a326 --- /dev/null +++ b/test_regress/t/t_lint_multidriven_clocking_bad.v @@ -0,0 +1,70 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Aisha Salimgereyeva +// SPDX-License-Identifier: CC0-1.0 + +module t ( + input wire clk +); + + // Clocking block declared before the drivers it contends with, so the + // conflict is found on reaching the design driver. + logic early_assign; + logic early_always; + + clocking cb_early @(posedge clk); + output early_assign; + output early_always; + endclocking + + assign early_assign = 1'b1; + always @(posedge clk) early_always <= 1'b1; + + // Continuous assignment against a clocking block output: an unambiguous + // driver conflict, reported by MULTIDRIVEN. + logic late_assign; + assign late_assign = 1'b1; + + // Plain always block against a clocking block output: a deliberate testbench + // idiom, so reported only by the off-by-default MULTIDRIVENPROC. + logic late_always; + always @(posedge clk) late_always <= 1'b1; + + // Driven by two clocking blocks, reported by MULTIDRIVEN. + logic dual_clocking; + + // Driven only by a clocking block: legal, must not warn. + logic clocking_only; + + // Read by a clocking block rather than driven: legal, must not warn. + logic observed; + always_ff @(posedge clk) observed <= ~observed; + + clocking cb_late @(posedge clk); + output late_assign; + output late_always; + output dual_clocking; + output clocking_only; + input observed; + endclocking + + clocking cb_dual @(posedge clk); + output dual_clocking; + endclocking + + // Declaration initializer alongside a clocking block output: the initializer + // is not a competing driver, must not warn. + logic decl_init = 1'b0; + + // One signal named by two clockvars of the same clocking block: still a + // single driver, must not warn. + logic aliased; + + clocking cb_alias @(posedge clk); + output decl_init; + output cv_a = aliased; + output cv_b = aliased; + endclocking + +endmodule