diff --git a/Changes b/Changes index 98e5eda6e..999fe55e6 100644 --- a/Changes +++ b/Changes @@ -21,6 +21,7 @@ Verilator 5.041 devel * Add $(LDFLAGS) and $(LIBS) to when building shared libraries (#6425) (#6426). [Ahmed El-Mahmoudy] * Add ASSIGNEQEXPR when use `=` inside expressions (#5567). [Ethan Sifferman] * Add IMPLICITSTATIC also on procedure variables. +* Add error on function invoking task. * Deprecate sensitivity list on public_flat_rw attributes (#6443). [Geza Lore] * Deprecate clocker attribute and --clk option (#6463). [Geza Lore] * Support modports referencing clocking blocks (#4555) (#6436). [Ryszard Rozak, Antmicro Ltd.] diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index d7d707d56..04fdc630c 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -132,6 +132,8 @@ class AstNodeFTask VL_NOT_FINAL : public AstNode { bool m_recursive : 1; // Recursive or part of recursion bool m_static : 1; // Static method in class bool m_underGenerate : 1; // Under generate (for warning) + bool m_verilogFunction : 1; // Declared by user as function (versus internal-made) + bool m_verilogTask : 1; // Declared by user as task (versus internal-made) bool m_virtual : 1; // Virtual method in class bool m_needProcess : 1; // Needs access to VlProcess of the caller VBaseOverride m_baseOverride; // BaseOverride (inital/final/extends) @@ -162,6 +164,8 @@ protected: , m_recursive{false} , m_static{false} , m_underGenerate{false} + , m_verilogFunction{false} + , m_verilogTask{false} , m_virtual{false} , m_needProcess{false} { addStmtsp(stmtsp); @@ -227,6 +231,10 @@ public: void isStatic(bool flag) { m_static = flag; } bool underGenerate() const { return m_underGenerate; } void underGenerate(bool flag) { m_underGenerate = flag; } + bool verilogFunction() const { return m_verilogFunction; } + void verilogFunction(bool flag) { m_verilogFunction = flag; } + bool verilogTask() const { return m_verilogTask; } + void verilogTask(bool flag) { m_verilogTask = flag; } bool isVirtual() const { return m_virtual; } void isVirtual(bool flag) { m_virtual = flag; } bool needProcess() const { return m_needProcess; } diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 9e85e993c..a73ff7c01 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -2901,6 +2901,8 @@ void AstNodeFTask::dump(std::ostream& str) const { if (recursive()) str << " [RECURSIVE]"; if (taskPublic()) str << " [PUBLIC]"; if (isStatic()) str << " [STATIC]"; + if (verilogTask()) str << " [VTASK]"; + if (verilogFunction()) str << " [VFUNC]"; if ((dpiImport() || dpiExport()) && cname() != name()) str << " [c=" << cname() << "]"; } bool AstNodeFTask::isPure() { diff --git a/src/V3WidthCommit.cpp b/src/V3WidthCommit.cpp index bc75e3e0d..00cf1f659 100644 --- a/src/V3WidthCommit.cpp +++ b/src/V3WidthCommit.cpp @@ -45,11 +45,13 @@ class WidthCommitVisitor final : public VNVisitor { const VNUser2InUse m_inuser2; // STATE - AstNodeModule* m_modp = nullptr; + AstNodeFTask* m_ftaskp = nullptr; // Current function/task + AstNodeModule* m_modp = nullptr; // Current module std::string m_contNba; // In continuous- or non-blocking assignment bool m_dynsizedelem = false; // Writing a dynamically-sized array element, not the array itself VMemberMap m_memberMap; // Member names cached for fast lookup + bool m_taskRefWarn = true; // Allow task reference warnings bool m_underSel = false; // Under AstMemberSel or AstSel bool m_underAlwaysClocked = false; // Under always with sequential SenTree std::vector m_virtualNewsp; // Instantiations of virtual classes @@ -216,6 +218,13 @@ private: iterateChildren(nodep); editDType(nodep); } + void visit(AstFork* nodep) override { + VL_RESTORER(m_taskRefWarn); + // fork..join_any is allowed to call tasks, and UVM does this + if (nodep->joinType().joinNone()) m_taskRefWarn = false; + iterateChildren(nodep); + editDType(nodep); + } void visit(AstSenTree* nodep) override { if (nodep->sensesp() && nodep->sensesp()->isComboStar()) { UASSERT_OBJ(!nodep->sensesp()->nextp(), nodep, "Shouldn't be senitems after .*"); @@ -317,6 +326,8 @@ private: void visit(AstNodeFTask* nodep) override { if (!nodep->taskPublic() && !nodep->dpiExport() && !nodep->dpiImport()) m_tasksp.push_back(nodep); + VL_RESTORER(m_ftaskp); + m_ftaskp = nodep; iterateChildren(nodep); editDType(nodep); { @@ -419,6 +430,17 @@ private: iterateChildren(nodep); editDType(nodep); classEncapCheck(nodep, nodep->taskp(), VN_CAST(nodep->classOrPackagep(), Class)); + if (nodep->taskp() && nodep->taskp()->verilogTask() && m_ftaskp + && m_ftaskp->verilogFunction() && m_taskRefWarn) { + nodep->v3error("Functions cannot invoke tasks (IEEE 1800-2023 13.4)\n" + << nodep->warnContextPrimary() << "\n" + << nodep->warnMore() << "... Suggest make caller 'function " + << m_ftaskp->prettyName() << "' a task\n" + << m_ftaskp->warnContextSecondary() << "\n" + << nodep->warnMore() << "... Or, suggest make called 'task " + << nodep->taskp()->prettyName() << "' a function void\n" + << nodep->taskp()->warnContextSecondary()); + } if (nodep->taskp()) nodep->taskp()->user2(1); if (AstNew* const newp = VN_CAST(nodep, New)) { if (!VN_IS(newp->backp(), Assign)) return; diff --git a/src/verilog.y b/src/verilog.y index 96290e020..1fe51788c 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -4546,14 +4546,17 @@ lifetime: // ==IEEE: lifetime taskId: id - { $$ = new AstTask{$$, *$1, nullptr}; } + { $$ = new AstTask{$$, *$1, nullptr}; + $$->verilogTask(true); } // | id/*interface_identifier*/ '.' idAny { $$ = new AstTask{$$, *$3, nullptr}; + $$->verilogTask(true); BBUNSUP($2, "Unsupported: Out of block function declaration"); } // | packageClassScope id { $$ = new AstTask{$$, *$2, nullptr}; + $$->verilogTask(true); $$->classOrPackagep($1); $$->classMethod(true); } ; @@ -4584,18 +4587,23 @@ funcId: // IEEE: function_data_type_or_implicit + part o $$->fvarp(GRAMMARP->createArray(refp, $4, true)); } // // To verilator tasks are the same as void functions (we separately detect time passing) | yVOID taskId - { $$ = $2; } + { $$ = $2; // Internals represent it as a task, not a function (TODO cleanup) + $$->verilogTask(false); + $$->verilogFunction(true); } ; funcIdNew: // IEEE: from class_constructor_declaration yNEW__ETC { $$ = new AstFunc{$1, "new", nullptr, nullptr}; + $$->verilogFunction(true); $$->isConstructor(true); } | yNEW__PAREN { $$ = new AstFunc{$1, "new", nullptr, nullptr}; + $$->verilogFunction(true); $$->isConstructor(true); } | packageClassScopeNoId yNEW__PAREN { $$ = new AstFunc{$2, "new", nullptr, nullptr}; + $$->verilogFunction(true); $$->classOrPackagep($1); $$->isConstructor(true); $$->classMethod(true); } @@ -4605,16 +4613,19 @@ fIdScoped: // IEEE: part of function_body_declaration/task_ // // IEEE: [ interface_identifier '.' | class_scope ] function_identifier id { $$ = $1; - $$ = new AstFunc{$$, *$1, nullptr, nullptr}; } + $$ = new AstFunc{$$, *$1, nullptr, nullptr}; + $$->verilogFunction(true); } // | id/*interface_identifier*/ '.' idAny { $$ = $1; $$ = new AstFunc{$$, *$1, nullptr, nullptr}; + $$->verilogFunction(true); BBUNSUP($2, "Unsupported: Out of block function declaration"); } // | packageClassScope id { $$ = $1; $$ = new AstFunc{$$, *$2, nullptr, nullptr}; + $$->verilogFunction(true); $$->classMethod(true); $$->classOrPackagep($1); } ; diff --git a/test_regress/t/t_func_task_bad.out b/test_regress/t/t_func_task_bad.out index 2cb9809f4..3d3dbfb9a 100644 --- a/test_regress/t/t_func_task_bad.out +++ b/test_regress/t/t_func_task_bad.out @@ -1,10 +1,10 @@ -%Error: t/t_func_task_bad.v:10:11: Cannot call a task/void-function as a function: 'task_as_func' - : ... note: In instance 't' - 10 | if (task_as_func(1'b0)) $stop; - | ^~~~~~~~~~~~ - ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. -%Error: t/t_func_task_bad.v:10:7: Logical operator IF expects a non-complex data type on the If. +%Error: t/t_func_task_bad.v:10:9: Cannot call a task/void-function as a function: 'a_task' : ... note: In instance 't' - 10 | if (task_as_func(1'b0)) $stop; - | ^~ + 10 | if (a_task(1'b0)) $stop; + | ^~~~~~ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Error: t/t_func_task_bad.v:10:5: Logical operator IF expects a non-complex data type on the If. + : ... note: In instance 't' + 10 | if (a_task(1'b0)) $stop; + | ^~ %Error: Exiting due to diff --git a/test_regress/t/t_func_task_bad.v b/test_regress/t/t_func_task_bad.v index dcf2f685d..9f0e668b3 100644 --- a/test_regress/t/t_func_task_bad.v +++ b/test_regress/t/t_func_task_bad.v @@ -6,12 +6,12 @@ module t; - initial begin - if (task_as_func(1'b0)) $stop; - end + initial begin + if (a_task(1'b0)) $stop; // <--- Bad: Calling task _as_ function + end - task task_as_func; - input ign; - endtask + task a_task; + input ign; + endtask endmodule diff --git a/test_regress/t/t_func_task_bad2.out b/test_regress/t/t_func_task_bad2.out new file mode 100644 index 000000000..174c5e0f4 --- /dev/null +++ b/test_regress/t/t_func_task_bad2.out @@ -0,0 +1,12 @@ +%Error: t/t_func_task_bad2.v:14:5: Functions cannot invoke tasks (IEEE 1800-2023 13.4) + : ... note: In instance 't' + 14 | a_task(1'b0); + | ^~~~~~ + : ... Suggest make caller 'function func_calls_task' a task + 13 | function void func_calls_task; + | ^~~~~~~~~~~~~~~ + : ... Or, suggest make called 'task a_task' a function void + 9 | task a_task; + | ^~~~~~ + ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. +%Error: Exiting due to diff --git a/test_regress/t/t_func_task_bad2.py b/test_regress/t/t_func_task_bad2.py new file mode 100755 index 000000000..5df24a780 --- /dev/null +++ b/test_regress/t/t_func_task_bad2.py @@ -0,0 +1,16 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2024 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 + +import vltest_bootstrap + +test.scenarios('linter') + +test.lint(verilator_flags2=['--timing'], fails=True, expect_filename=test.golden_filename) + +test.passes() diff --git a/test_regress/t/t_func_task_bad2.v b/test_regress/t/t_func_task_bad2.v new file mode 100644 index 000000000..61d06bfe6 --- /dev/null +++ b/test_regress/t/t_func_task_bad2.v @@ -0,0 +1,23 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2011 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t; + + task a_task; + input ign; + endtask + + function void func_calls_task; + a_task(1'b0); // <--- Bad: Calling task _from_ function + endfunction + + function void func_ok; + fork + a_task(1'b0); // ok, and done in UVM + join_none + endfunction + +endmodule diff --git a/test_regress/t/t_opt_dead_task.v b/test_regress/t/t_opt_dead_task.v index fe9e1aeba..cf9f77a6d 100644 --- a/test_regress/t/t_opt_dead_task.v +++ b/test_regress/t/t_opt_dead_task.v @@ -4,39 +4,41 @@ // any use, without warranty, 2025 by Antmicro. // SPDX-License-Identifier: CC0-1.0 -module t (input clk); - // AstLet and AstProperty are also NodeFTasks, but lets are substituted earlier and properties should be "used" by their asserts so also not deadified - let nclk = ~clk; - assert property(@(posedge clk)1); +module t ( + input clk +); + // AstLet and AstProperty are also NodeFTasks, but lets are substituted earlier and properties should be "used" by their asserts so also not deadified + let nclk = ~clk; + assert property (@(posedge clk) 1); - function void livefunc(); - endfunction - task livetask; - endtask - // Tasks/functions that are called somewhere will not be deadified - initial begin - livefunc(); - livetask(); - $finish; - end + function void livefunc(); + endfunction + task livetask; + endtask + // Tasks/functions that are called somewhere will not be deadified + initial begin + livefunc(); + livetask(); + $finish; + end - // These should be deadified - function deadfunc(); - deeptask2(); - endfunction - task deadtask; - deeptask1(); - endtask - // A chain of dead tasks calling each other to ensure V3Dead can remove chained dead tasks - task deeptask1; - deeptask2(); - endtask - task deeptask2; - deeptask3(); - endtask - task deeptask3; - deeptask4(); - endtask - task deeptask4; - endtask + // These should be deadified + task deadfunc(); + deeptask2(); + endtask + task deadtask; + deeptask1(); + endtask + // A chain of dead tasks calling each other to ensure V3Dead can remove chained dead tasks + task deeptask1; + deeptask2(); + endtask + task deeptask2; + deeptask3(); + endtask + task deeptask3; + deeptask4(); + endtask + task deeptask4; + endtask endmodule diff --git a/test_regress/t/t_var_in_fork.v b/test_regress/t/t_var_in_fork.v index 9aaa3f3c7..d4a10079e 100644 --- a/test_regress/t/t_var_in_fork.v +++ b/test_regress/t/t_var_in_fork.v @@ -14,7 +14,7 @@ module t(); class Foo; - function void do_something(int captured_var); + task do_something(int captured_var); fork begin int my_var; @@ -23,7 +23,7 @@ module t(); my_other_var = captured_var; /* Capture the same value "twice" */ my_var++; static_var++; /* Write to a value with static lifetime (valid) */ - $display("Vars in forked process: %d %d", my_var, my_other_var); + $display("Vars in forked process: %0d %0d", my_var, my_other_var); if (my_var != 2) $stop; if (my_other_var != 1) @@ -32,7 +32,7 @@ module t(); end join_none $display("Leaving fork's parent"); - endfunction + endtask endclass initial begin @@ -44,14 +44,14 @@ module t(); end always @(evt) begin - $display("Static variable: %d", static_var); + $display("Static variable: %0d", static_var); if (static_var != 1) $stop; fork begin automatic int my_auto_var = 0; my_auto_var++; - $display("Automatic variable in fork: %d", my_auto_var); + $display("Automatic variable in fork: %0d", my_auto_var); if (my_auto_var != 1) $stop; end join_none