diff --git a/Changes b/Changes index 73f5a63f5..cc313c39f 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,7 @@ Verilator 5.017 devel **Minor:** +* Add SIDEEFFECT warning on mishandled side effect cases. * Add trace() API even when Verilated without --trace (#4462). [phelter] * Add warning on interface instantiation without parens (#4094). [Gökçe Aydos] * Support randc (#4349). diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 4588fd28b..2691e9acf 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -1514,6 +1514,39 @@ List Of Warnings modeled values, or DPI calls. +.. option:: SIDEEFFECT + + Warns that an expression has a side effect that might not properly be + executed by Verilator. + + This often represents a bug in Verilator, as opposed to a bad code + construct, however the Verilog code can typically be changed to avoid + the warning. + + Faulty example: + + .. code-block:: sv + :linenos: + :emphasize-lines: 1 + + x = y[a++]; + + This example warns because Verilator does not currently handle side + effects inside array subscripts; the a++ may be executed multiple times. + + Rewrite the code to avoid expression side effects, typically by using a + temporary: + + .. code-block:: sv + :linenos: + + temp = a++; + x = y[temp]; + + Ignoring this warning may make Verilator simulations differ from other + simulators. + + .. option:: SPLITVAR Warns that a variable with a :option:`/*verilator&32;split_var*/` diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index e85a2f980..30ebd67cf 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -801,26 +801,33 @@ void AstNode::swapWith(AstNode* bp) { //====================================================================== // Clone -AstNode* AstNode::cloneTreeIter() { +AstNode* AstNode::cloneTreeIter(bool needPure) { // private: Clone single node and children + if (VL_UNLIKELY(needPure && !isPure())) { + this->v3warn(SIDEEFFECT, + "Expression side effect may be mishandled\n" + << this->warnMore() + << "... Suggest use a temporary variable in place of this expression"); + // this->v3fatalSrc("cloneTreePure debug backtrace"); // Comment in to debug where caused it + } AstNode* const newp = this->clone(); - if (this->m_op1p) newp->op1p(this->m_op1p->cloneTreeIterList()); - if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList()); - if (this->m_op3p) newp->op3p(this->m_op3p->cloneTreeIterList()); - if (this->m_op4p) newp->op4p(this->m_op4p->cloneTreeIterList()); + if (this->m_op1p) newp->op1p(this->m_op1p->cloneTreeIterList(needPure)); + if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList(needPure)); + if (this->m_op3p) newp->op3p(this->m_op3p->cloneTreeIterList(needPure)); + if (this->m_op4p) newp->op4p(this->m_op4p->cloneTreeIterList(needPure)); newp->m_iterpp = nullptr; newp->clonep(this); // Save pointers to/from both to simplify relinking. this->clonep(newp); // Save pointers to/from both to simplify relinking. return newp; } -AstNode* AstNode::cloneTreeIterList() { +AstNode* AstNode::cloneTreeIterList(bool needPure) { // private: Clone list of nodes, set m_headtailp AstNode* newheadp = nullptr; AstNode* newtailp = nullptr; // Audited to make sure this is never nullptr for (AstNode* oldp = this; oldp; oldp = oldp->m_nextp) { - AstNode* const newp = oldp->cloneTreeIter(); + AstNode* const newp = oldp->cloneTreeIter(needPure); newp->m_headtailp = nullptr; newp->m_backp = newtailp; if (newtailp) newtailp->m_nextp = newp; @@ -832,14 +839,14 @@ AstNode* AstNode::cloneTreeIterList() { return newheadp; } -AstNode* AstNode::cloneTree(bool cloneNextLink) { +AstNode* AstNode::cloneTree(bool cloneNextLink, bool needPure) { debugTreeChange(this, "-cloneThs: ", __LINE__, cloneNextLink); cloneClearTree(); AstNode* newp; if (cloneNextLink && this->m_nextp) { - newp = cloneTreeIterList(); + newp = cloneTreeIterList(needPure); } else { - newp = cloneTreeIter(); + newp = cloneTreeIter(needPure); newp->m_nextp = nullptr; newp->m_headtailp = newp; } diff --git a/src/V3Ast.h b/src/V3Ast.h index 7fb97634d..402f4b8eb 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1713,8 +1713,8 @@ class AstNode VL_NOT_FINAL { } private: - AstNode* cloneTreeIter(); - AstNode* cloneTreeIterList(); + AstNode* cloneTreeIter(bool needPure); + AstNode* cloneTreeIterList(bool needPure); void checkTreeIter(const AstNode* prevBackp) const VL_MT_STABLE; bool gateTreeIter() const; static bool sameTreeIter(const AstNode* node1p, const AstNode* node2p, bool ignNext, @@ -2074,8 +2074,8 @@ public: AstNode* belowp); // When calling, "this" is second argument // METHODS - Iterate on a tree - AstNode* cloneTree(bool cloneNextLink); // Not const, as sets clonep() on original nodep // - AstNode* cloneTreePure(bool cloneNextLink) { return cloneTree(cloneNextLink); } + AstNode* cloneTree(bool cloneNextLink, bool needPure = false); // Not const, as sets clonep() on original nodep + AstNode* cloneTreePure(bool cloneNextLink) { return cloneTree(cloneNextLink, true); } bool gateTree() { return gateTreeIter(); } // Is tree isGateOptimizable? inline bool sameTree(const AstNode* node2p) const; // Does tree of this == node2p? // Does tree of this == node2p?, not allowing non-isGateOptimizable diff --git a/src/V3Error.h b/src/V3Error.h index 87c6c0bad..3513c3902 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -136,6 +136,7 @@ public: RISEFALLDLY, // Unsupported: rise/fall/turn-off delays SELRANGE, // Selection index out of range SHORTREAL, // Shortreal not supported + SIDEEFFECT, // Sideeffect ignored SPLITVAR, // Cannot split the variable STATICVAR, // Static variable declared in a loop with a declaration assignment STMTDLY, // Delayed statement @@ -203,7 +204,8 @@ public: "MULTIDRIVEN", "MULTITOP", "NEWERSTD", "NOLATCH", "NULLPORT", "PINCONNECTEMPTY", "PINMISSING", "PINNOCONNECT", "PINNOTFOUND", "PKGNODECL", "PROCASSWIRE", "PROFOUTOFDATE", "PROTECTED", "RANDC", "REALCVT", "REDEFMACRO", "RISEFALLDLY", - "SELRANGE", "SHORTREAL", "SPLITVAR", "STATICVAR", "STMTDLY", "SYMRSVDWORD", "SYNCASYNCNET", + "SELRANGE", "SHORTREAL", "SIDEEFFECT", "SPLITVAR", + "STATICVAR", "STMTDLY", "SYMRSVDWORD", "SYNCASYNCNET", "TICKCOUNT", "TIMESCALEMOD", "UNDRIVEN", "UNOPT", "UNOPTFLAT", "UNOPTTHREADS", "UNPACKED", "UNSIGNED", "UNUSEDGENVAR", "UNUSEDPARAM", "UNUSEDSIGNAL", @@ -231,7 +233,8 @@ public: } // Warnings to mention manual bool mentionManual() const VL_MT_SAFE { - return (m_e == EC_FATALSRC || m_e == SYMRSVDWORD || m_e == ZERODLY || pretendError()); + return (m_e == EC_FATALSRC || m_e == SIDEEFFECT || m_e == SYMRSVDWORD || m_e == ZERODLY + || pretendError()); } // Warnings that are lint only bool lintError() const VL_MT_SAFE { diff --git a/src/V3LinkInc.cpp b/src/V3LinkInc.cpp index 49b775ab9..4e2381ff7 100644 --- a/src/V3LinkInc.cpp +++ b/src/V3LinkInc.cpp @@ -220,16 +220,6 @@ private: } void prepost_stmt_visit(AstNodeTriop* nodep) { iterateChildren(nodep); - - // Currently we can't reference the target, so we just copy the AST both for read and - // write, but doing so would double any side-effects, so as a safety measure all - // statements which could have side-effects are banned at the moment. - if (!nodep->rhsp()->isPure()) { - nodep->rhsp()->v3warn(E_UNSUPPORTED, - "Unsupported: Inc/Dec of expression with side-effects"); - return; - } - AstConst* const constp = VN_AS(nodep->lhsp(), Const); UASSERT_OBJ(nodep, constp, "Expecting CONST"); AstConst* const newconstp = constp->cloneTree(true); @@ -250,16 +240,6 @@ private: } void prepost_expr_visit(AstNodeTriop* nodep) { iterateChildren(nodep); - - // Currently we can't reference the target, so we just copy the AST both for read and - // write, but doing so would double any side-effects, so as a safety measure all - // statements which could have side-effects are banned at the moment. - if (!nodep->rhsp()->isPure()) { - nodep->rhsp()->v3warn(E_UNSUPPORTED, - "Unsupported: Inc/Dec of expression with side-effects"); - return; - } - if (m_unsupportedHere) { nodep->v3warn(E_UNSUPPORTED, "Unsupported: Incrementation in this context."); return; diff --git a/test_regress/t/t_altera_lpm.v b/test_regress/t/t_altera_lpm.v index 0a3671e49..95e16b454 100644 --- a/test_regress/t/t_altera_lpm.v +++ b/test_regress/t/t_altera_lpm.v @@ -2557,12 +2557,14 @@ module lpm_clshift ( // ALWAYS CONSTRUCT BLOCK always @(data or i_direction or distance) begin + // verilator lint_off SIDEEFFECT if ((lpm_shifttype == "LOGICAL") || (lpm_shifttype == "UNUSED")) {tmp_overflow, tmp_underflow, tmp_result} = LogicShift(data, distance, i_direction); else if (lpm_shifttype == "ARITHMETIC") {tmp_overflow, tmp_underflow, tmp_result} = ArithShift(data, distance, i_direction); else if (lpm_shifttype == "ROTATE") {tmp_overflow, tmp_underflow, tmp_result} = RotateShift(data, distance, i_direction); + // verilator lint_on SIDEEFFECT end always @(posedge i_clock or posedge i_aclr) diff --git a/test_regress/t/t_expr_incr_unsup.out b/test_regress/t/t_expr_incr_unsup.out index 6f899d2ac..3f28464b5 100644 --- a/test_regress/t/t_expr_incr_unsup.out +++ b/test_regress/t/t_expr_incr_unsup.out @@ -1,5 +1,11 @@ -%Error-UNSUPPORTED: t/t_expr_incr_unsup.v:17:34: Unsupported: Inc/Dec of expression with side-effects +%Warning-SIDEEFFECT: t/t_expr_incr_unsup.v:17:34: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression 17 | $display("Value: %d", arr[postincrement_i()]++); | ^ - ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest + ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest + ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message. +%Warning-SIDEEFFECT: t/t_expr_incr_unsup.v:17:35: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 17 | $display("Value: %d", arr[postincrement_i()]++); + | ^~~~~~~~~~~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_lint_sideeffect_bad.out b/test_regress/t/t_lint_sideeffect_bad.out new file mode 100644 index 000000000..7b2dbd3f4 --- /dev/null +++ b/test_regress/t/t_lint_sideeffect_bad.out @@ -0,0 +1,24 @@ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:19:31: Expression side effect may be mishandled + : ... note: In instance 't' + : ... Suggest use a temporary variable in place of this expression + 19 | $display("0x%8x", array[$c(0) +: 32]); + | ^~ + ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest + ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message. +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:12:13: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 12 | case ($c("1")) + | ^~ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:13:10: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 13 | 1: $stop; + | ^ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:14:10: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 14 | 2: $stop; + | ^ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:15:10: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 15 | 3: $stop; + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_sideeffect_bad.pl b/test_regress/t/t_lint_sideeffect_bad.pl new file mode 100755 index 000000000..a82cf66cb --- /dev/null +++ b/test_regress/t/t_lint_sideeffect_bad.pl @@ -0,0 +1,19 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2008 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(linter => 1); + +lint( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + +ok(1); +1; diff --git a/test_regress/t/t_lint_sideeffect_bad.v b/test_regress/t/t_lint_sideeffect_bad.v new file mode 100644 index 000000000..d153b846e --- /dev/null +++ b/test_regress/t/t_lint_sideeffect_bad.v @@ -0,0 +1,22 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t (/*AUTOARG*/); + + logic [63:0] array = 64'hfeedf00d12345678; + + initial begin + case ($c("1")) + 1: $stop; + 2: $stop; + 3: $stop; + default: $stop; + endcase + + $display("0x%8x", array[$c(0) +: 32]); + end + +endmodule diff --git a/test_regress/t/t_stmt_incr_unsup.out b/test_regress/t/t_stmt_incr_unsup.out index 9bc34d883..c7c313981 100644 --- a/test_regress/t/t_stmt_incr_unsup.out +++ b/test_regress/t/t_stmt_incr_unsup.out @@ -1,5 +1,11 @@ -%Error-UNSUPPORTED: t/t_stmt_incr_unsup.v:17:12: Unsupported: Inc/Dec of expression with side-effects +%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:12: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression 17 | arr[postincrement_i()]++; | ^ - ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest + ... For warning description see https://verilator.org/warn/SIDEEFFECT?v=latest + ... Use "/* verilator lint_off SIDEEFFECT */" and lint_on around source to disable this message. +%Warning-SIDEEFFECT: t/t_stmt_incr_unsup.v:17:13: Expression side effect may be mishandled + : ... Suggest use a temporary variable in place of this expression + 17 | arr[postincrement_i()]++; + | ^~~~~~~~~~~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_sv_cpu_code/ports.sv b/test_regress/t/t_sv_cpu_code/ports.sv index da9dbc25b..5cfeeccbb 100644 --- a/test_regress/t/t_sv_cpu_code/ports.sv +++ b/test_regress/t/t_sv_cpu_code/ports.sv @@ -112,10 +112,12 @@ module ports // - Setup read multiplexer - for ( j = PORTID_A; j <= PORTID_D; j++ ) begin + // verilator lint_off SIDEEFFECT if ( padsif.IsPort( j ) ) data[j] = '{ port[j].dir, port[j].out, 8'h00, 8'h00 }; else data[j] = '{ 8'h00, 8'h00, 8'h00, 8'h00 }; + // verilator lint_on SIDEEFFECT end // - Connect "genbusif" -