Add SIDEEFFECT warning on mishandled side effect cases (#487 partial)

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).

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*/`

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_op1p) newp->op1p(this->m_op1p->cloneTreeIterList(needPure));
-    if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList());
+    if (this->m_op2p) newp->op2p(this->m_op2p->cloneTreeIterList(needPure));
-    if (this->m_op3p) newp->op3p(this->m_op3p->cloneTreeIterList());
+    if (this->m_op3p) newp->op3p(this->m_op3p->cloneTreeIterList(needPure));
-    if (this->m_op4p) newp->op4p(this->m_op4p->cloneTreeIterList());
+    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;
     }

src/V3Ast.h

@@ -1713,8 +1713,8 @@ class AstNode VL_NOT_FINAL {
     }
 
 private:
-    AstNode* cloneTreeIter();
+    AstNode* cloneTreeIter(bool needPure);
-    AstNode* cloneTreeIterList();
+    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* cloneTree(bool cloneNextLink, bool needPure = false);  // Not const, as sets clonep() on original nodep
-    AstNode* cloneTreePure(bool cloneNextLink) { return cloneTree(cloneNextLink); }
+    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

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 {

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;

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)

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

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

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;

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

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

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" -