Disallow ++/-- over expressions with potential side effects (#3976).

docs/CONTRIBUTORS

@@ -78,6 +78,7 @@ Keith Colbert
 Kevin Kiningham
 Kritik Bhimani
 Krzysztof Bieganski
+Krzysztof Boroński
 Kuba Ober
 Larry Doolittle
 Ludwig Rogiers

src/V3Ast.cpp

@@ -1263,6 +1263,16 @@ void AstNode::dumpTreeDotFile(const string& filename, bool append, bool doDump)
     }
 }
 
+bool AstNode::isTreePureRecurse() const {
+    // Should memoize this if call commonly
+    if (!this->isPure()) return false;
+    if (this->op1p() && !this->op1p()->isTreePureRecurse()) return false;
+    if (this->op2p() && !this->op2p()->isTreePureRecurse()) return false;
+    if (this->op3p() && !this->op3p()->isTreePureRecurse()) return false;
+    if (this->op4p() && !this->op4p()->isTreePureRecurse()) return false;
+    return true;
+}
+
 void AstNode::v3errorEndFatal(std::ostringstream& str) const VL_REQUIRES(V3Error::s().m_mutex) {
     v3errorEnd(str);
     assert(0);  // LCOV_EXCL_LINE

src/V3Ast.h

@@ -1926,6 +1926,8 @@ public:
     void dumpTreeDot(std::ostream& os = std::cout) const;
     void dumpTreeDotFile(const string& filename, bool append = false, bool doDump = true);
 
+    bool isTreePureRecurse() const;
+
     // METHODS - queries
     // Changes control flow, disable some optimizations
     virtual bool isBrancher() const { return false; }

src/V3AstNodeExpr.h

@@ -249,6 +249,7 @@ public:
     void classOrPackagep(AstNodeModule* nodep) { m_classOrPackagep = nodep; }
     bool pli() const { return m_pli; }
     void pli(bool flag) { m_pli = flag; }
+    bool isPure() const override;
 
     string emitVerilog() final override { V3ERROR_NA_RETURN(""); }
     string emitC() final override { V3ERROR_NA_RETURN(""); }

src/V3AstNodes.cpp

@@ -60,6 +60,12 @@ void AstNodeFTaskRef::cloneRelink() {
     }
 }
 
+bool AstNodeFTaskRef::isPure() const {
+    // TODO: For non-DPI functions we could traverse the AST of function's body to determine
+    // pureness.
+    return this->taskp() && this->taskp()->dpiImport() && this->taskp()->pure();
+}
+
 bool AstNodeFTaskRef::isGateOptimizable() const { return m_taskp && m_taskp->isGateOptimizable(); }
 
 const char* AstNodeVarRef::broken() const {

src/V3Const.cpp

@@ -1384,15 +1384,6 @@ private:
         // but for now can disable en masse until V3Purify takes effect.
         return m_doShort || VN_IS(nodep, VarRef) || VN_IS(nodep, Const);
     }
-    bool isTreePureRecurse(AstNode* nodep) {
-        // Should memoize this if call commonly
-        if (!nodep->isPure()) return false;
-        if (nodep->op1p() && !isTreePureRecurse(nodep->op1p())) return false;
-        if (nodep->op2p() && !isTreePureRecurse(nodep->op2p())) return false;
-        if (nodep->op3p() && !isTreePureRecurse(nodep->op3p())) return false;
-        if (nodep->op4p() && !isTreePureRecurse(nodep->op4p())) return false;
-        return true;
-    }
 
     // Extraction checks
     bool warnSelect(AstSel* nodep) {
@@ -2954,7 +2945,7 @@ private:
                 }
                 VL_DO_DANGLING(nodep->deleteTree(), nodep);
             } else if (!afterComment(nodep->thensp()) && !afterComment(nodep->elsesp())) {
-                if (!isTreePureRecurse(nodep->condp())) {
+                if (!nodep->condp()->isTreePureRecurse()) {
                     // Condition has side effect - leave - perhaps in
                     // future simplify to remove all but side effect terms
                 } else {

src/V3LinkInc.cpp

@@ -201,6 +201,15 @@ 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()->isTreePureRecurse()) {
+            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);
@@ -222,6 +231,15 @@ 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()->isTreePureRecurse()) {
+            nodep->rhsp()->v3warn(E_UNSUPPORTED,
+                                  "Unsupported: Inc/Dec of expression with side-effects");
+            return;
+        }
+
         const AstNodeVarRef* varrefp = nullptr;
         if (m_unsupportedHere || !(varrefp = VN_CAST(nodep->rhsp(), VarRef))) {
             nodep->v3warn(E_UNSUPPORTED, "Unsupported: Incrementation in this context.");

test_regress/t/t_expr_incr_unsup.out

@@ -0,0 +1,5 @@
+%Error-UNSUPPORTED: t/t_expr_incr_unsup.v:17:34: Unsupported: Inc/Dec of expression with side-effects
+   17 |         $display("Value: %d", arr[postincrement_i()]++);
+      |                                  ^
+                    ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest
+%Error: Exiting due to

test_regress/t/t_expr_incr_unsup.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 2003-2009 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_expr_incr_unsup.v

@@ -0,0 +1,19 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2022 Krzysztof Boronski.
+// SPDX-License-Identifier: CC0-1.0
+
+int i = 0;
+
+function int postincrement_i;
+    return i++;
+endfunction
+
+module t;
+    initial begin
+        int arr [1:0] = {0, 0};
+        i = 0;
+        $display("Value: %d", arr[postincrement_i()]++);
+    end
+endmodule

test_regress/t/t_func_tasknsvar_bad.out

@@ -2,7 +2,7 @@
    16 |       foo(bus_we_select_from[2]);   
       |                             ^
                   ... For error description see https://verilator.org/warn/TASKNSVAR?v=latest
-%Error: Internal Error: t/t_func_tasknsvar_bad.v:10:7: ../V3Broken.cpp:#: Broken link in node (or something without maybePointedTo): 'm_varp && !m_varp->brokeExists()' @ ../V3AstNodes.cpp:66
+%Error: Internal Error: t/t_func_tasknsvar_bad.v:10:7: ../V3Broken.cpp:#: Broken link in node (or something without maybePointedTo): 'm_varp && !m_varp->brokeExists()' @ ../V3AstNodes.cpp:72
    10 |       sig = '1;
       |       ^~~
                         ... See the manual at https://verilator.org/verilator_doc.html for more assistance.

test_regress/t/t_stmt_incr_unsup.out

@@ -0,0 +1,5 @@
+%Error-UNSUPPORTED: t/t_stmt_incr_unsup.v:17:12: Unsupported: Inc/Dec of expression with side-effects
+   17 |         arr[postincrement_i()]++;
+      |            ^
+                    ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest
+%Error: Exiting due to

test_regress/t/t_stmt_incr_unsup.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 2003-2009 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_stmt_incr_unsup.v

@@ -0,0 +1,20 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2022 Krzysztof Boronski.
+// SPDX-License-Identifier: CC0-1.0
+
+int i = 0;
+
+function int postincrement_i;
+    return i++;
+endfunction
+
+module t;
+    initial begin
+        int arr [1:0] = {0, 0};
+        i = 0;
+        arr[postincrement_i()]++;
+        $display("Value: %d", i);
+    end
+endmodule