From 06661ab6760cc619ebf029783785774d1cc6d216 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Boro=C5=84ski?= <94375110+kboronski-ant@users.noreply.github.com> Date: Tue, 28 Feb 2023 21:21:58 +0100 Subject: [PATCH] Disallow ++/-- over expressions with potential side effects (#3976). --- docs/CONTRIBUTORS | 1 + src/V3Ast.cpp | 10 ++++++++++ src/V3Ast.h | 2 ++ src/V3AstNodeExpr.h | 1 + src/V3AstNodes.cpp | 6 ++++++ src/V3Const.cpp | 11 +---------- src/V3LinkInc.cpp | 18 ++++++++++++++++++ test_regress/t/t_expr_incr_unsup.out | 5 +++++ test_regress/t/t_expr_incr_unsup.pl | 19 +++++++++++++++++++ test_regress/t/t_expr_incr_unsup.v | 19 +++++++++++++++++++ test_regress/t/t_func_tasknsvar_bad.out | 2 +- test_regress/t/t_stmt_incr_unsup.out | 5 +++++ test_regress/t/t_stmt_incr_unsup.pl | 19 +++++++++++++++++++ test_regress/t/t_stmt_incr_unsup.v | 20 ++++++++++++++++++++ 14 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 test_regress/t/t_expr_incr_unsup.out create mode 100755 test_regress/t/t_expr_incr_unsup.pl create mode 100644 test_regress/t/t_expr_incr_unsup.v create mode 100644 test_regress/t/t_stmt_incr_unsup.out create mode 100755 test_regress/t/t_stmt_incr_unsup.pl create mode 100644 test_regress/t/t_stmt_incr_unsup.v diff --git a/docs/CONTRIBUTORS b/docs/CONTRIBUTORS index ecde8cdba..791205edf 100644 --- a/docs/CONTRIBUTORS +++ b/docs/CONTRIBUTORS @@ -78,6 +78,7 @@ Keith Colbert Kevin Kiningham Kritik Bhimani Krzysztof Bieganski +Krzysztof BoroĊ„ski Kuba Ober Larry Doolittle Ludwig Rogiers diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index aea94b9aa..db5722c64 100644 --- a/src/V3Ast.cpp +++ b/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 diff --git a/src/V3Ast.h b/src/V3Ast.h index ffdc57ce4..eb7d345f7 100644 --- a/src/V3Ast.h +++ b/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; } diff --git a/src/V3AstNodeExpr.h b/src/V3AstNodeExpr.h index e2217ca1d..3376c64b9 100644 --- a/src/V3AstNodeExpr.h +++ b/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(""); } diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 25cc94a67..d44850903 100644 --- a/src/V3AstNodes.cpp +++ b/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 { diff --git a/src/V3Const.cpp b/src/V3Const.cpp index ed390e107..b1575caec 100644 --- a/src/V3Const.cpp +++ b/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 { diff --git a/src/V3LinkInc.cpp b/src/V3LinkInc.cpp index fdd8dd70d..4a2bb0c1c 100644 --- a/src/V3LinkInc.cpp +++ b/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."); diff --git a/test_regress/t/t_expr_incr_unsup.out b/test_regress/t/t_expr_incr_unsup.out new file mode 100644 index 000000000..6f899d2ac --- /dev/null +++ b/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 diff --git a/test_regress/t/t_expr_incr_unsup.pl b/test_regress/t/t_expr_incr_unsup.pl new file mode 100755 index 000000000..35d749208 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_expr_incr_unsup.v b/test_regress/t/t_expr_incr_unsup.v new file mode 100644 index 000000000..9d6647cbf --- /dev/null +++ b/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 diff --git a/test_regress/t/t_func_tasknsvar_bad.out b/test_regress/t/t_func_tasknsvar_bad.out index 230364bae..d4c3ed0ed 100644 --- a/test_regress/t/t_func_tasknsvar_bad.out +++ b/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. diff --git a/test_regress/t/t_stmt_incr_unsup.out b/test_regress/t/t_stmt_incr_unsup.out new file mode 100644 index 000000000..9bc34d883 --- /dev/null +++ b/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 diff --git a/test_regress/t/t_stmt_incr_unsup.pl b/test_regress/t/t_stmt_incr_unsup.pl new file mode 100755 index 000000000..35d749208 --- /dev/null +++ b/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; diff --git a/test_regress/t/t_stmt_incr_unsup.v b/test_regress/t/t_stmt_incr_unsup.v new file mode 100644 index 000000000..e1682331a --- /dev/null +++ b/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