From 4784daa7dc9ce61f8d92b99492a600c3a252bb55 Mon Sep 17 00:00:00 2001 From: Ryszard Rozak Date: Thu, 5 Jan 2023 23:42:05 +0100 Subject: [PATCH] Add IMPLICITSTATIC warning when a ftask/function is implicitly static (#3839) --- docs/guide/exe_verilator.rst | 2 +- docs/guide/warnings.rst | 21 ++++++++++++ src/V3Error.h | 9 +++--- src/V3LinkParse.cpp | 31 +++++++++++++++--- test_regress/t/t_array_query_with.v | 6 ++-- test_regress/t/t_assoc2.v | 2 +- test_regress/t/t_dynarray_param.v | 4 +-- test_regress/t/t_func_bad.v | 2 +- test_regress/t/t_func_const.v | 2 +- test_regress/t/t_func_default_warn.v | 2 +- test_regress/t/t_func_named.v | 2 +- test_regress/t/t_func_no_lifetime_bad.out | 11 +++++++ test_regress/t/t_func_no_lifetime_bad.pl | 23 +++++++++++++ test_regress/t/t_func_no_lifetime_bad.v | 39 +++++++++++++++++++++++ test_regress/t/t_func_under.v | 2 +- test_regress/t/t_jumps_do_while.v | 22 ++++++------- test_regress/t/t_param_array3.v | 2 +- test_regress/t/t_param_func2.v | 4 +-- test_regress/t/t_stream_integer_type.v | 2 +- test_regress/t/t_trace_string.v | 2 +- test_regress/t/t_unpacked_str_init2.v | 4 +-- test_regress/t/t_var_static.pl | 1 + test_regress/t/t_var_static_param.out | 4 +-- test_regress/t/t_var_static_param.v | 1 + 24 files changed, 160 insertions(+), 40 deletions(-) create mode 100644 test_regress/t/t_func_no_lifetime_bad.out create mode 100755 test_regress/t/t_func_no_lifetime_bad.pl create mode 100644 test_regress/t/t_func_no_lifetime_bad.v diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index e89d3986b..e610fa926 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -1502,7 +1502,7 @@ Summary: Disable all lint-related warning messages, and all style warnings. This is equivalent to ``-Wno-ALWCOMBORDER -Wno-BSSPACE -Wno-CASEINCOMPLETE -Wno-CASEOVERLAP -Wno-CASEX -Wno-CASTCONST -Wno-CASEWITHX -Wno-CMPCONST -Wno-COLONPLUS - -Wno-IMPLICIT -Wno-LITENDIAN -Wno-PINCONNECTEMPTY + -Wno-IMPLICIT -Wno-IMPLICITSTATIC -Wno-LITENDIAN -Wno-PINCONNECTEMPTY -Wno-PINMISSING -Wno-SYNCASYNCNET -Wno-UNDRIVEN -Wno-UNSIGNED -Wno-UNUSEDGENVAR -Wno-UNUSEDPARAM -Wno-UNUSEDSIGNAL -Wno-WIDTH`` plus the list shown for Wno-style. diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 2daff53a6..7f0011153 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -727,6 +727,27 @@ List Of Warnings implicit definition of wire '...'". +.. option:: IMPLICITSTATIC + + Warns that the lifetime of a task or a function was not provided and so + was implicitly set to static. The warning is suppressed when no + variables inside the task or a function are assigned to. + + This is a warning because the static default differs from C++, differs + from class member function/tasks. Static is a more dangerous default + then automatic as static prevents the function from being reinterant, + which may be a source of bugs, and/or performance issues. + + If the function does not require static behavior, change it to "function + automatic". + + If the function requires static behavior, change it to "function + static". + + Ignoring this warning will only suppress the lint check; it will + simulate correctly. + + .. option:: IMPORTSTAR .. TODO better example diff --git a/src/V3Error.h b/src/V3Error.h index ead674249..1dc8e2487 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -96,6 +96,7 @@ public: IGNOREDRETURN, // Ignoring return value (function as task) IMPERFECTSCH, // Imperfect schedule (disabled by default). Historical, never issued. IMPLICIT, // Implicit wire + IMPLICITSTATIC, // Implicit static function IMPORTSTAR, // Import::* in $unit IMPURE, // Impure function not being inlined INCABSPATH, // Include has absolute path @@ -180,7 +181,7 @@ public: "DECLFILENAME", "DEFPARAM", "DEPRECATED", "ENCAPSULATED", "ENDLABEL", "ENUMVALUE", "EOFNEWLINE", "GENCLK", "HIERBLOCK", "IFDEPTH", "IGNOREDRETURN", - "IMPERFECTSCH", "IMPLICIT", "IMPORTSTAR", "IMPURE", + "IMPERFECTSCH", "IMPLICIT", "IMPLICITSTATIC", "IMPORTSTAR", "IMPURE", "INCABSPATH", "INFINITELOOP", "INITIALDLY", "INSECURE", "LATCH", "LITENDIAN", "MINTYPMAXDLY", "MODDUP", "MULTIDRIVEN", "MULTITOP","NOLATCH", "NULLPORT", "PINCONNECTEMPTY", @@ -220,9 +221,9 @@ public: bool lintError() const VL_MT_SAFE { return (m_e == ALWCOMBORDER || m_e == BSSPACE || m_e == CASEINCOMPLETE || m_e == CASEOVERLAP || m_e == CASEWITHX || m_e == CASEX || m_e == CASTCONST - || m_e == CMPCONST || m_e == COLONPLUS || m_e == IMPLICIT || m_e == LATCH - || m_e == LITENDIAN || m_e == PINMISSING || m_e == REALCVT || m_e == UNSIGNED - || m_e == WIDTH); + || m_e == CMPCONST || m_e == COLONPLUS || m_e == IMPLICIT || m_e == IMPLICITSTATIC + || m_e == LATCH || m_e == LITENDIAN || m_e == PINMISSING || m_e == REALCVT + || m_e == UNSIGNED || m_e == WIDTH); } // Warnings that are style only bool styleError() const VL_MT_SAFE { diff --git a/src/V3LinkParse.cpp b/src/V3LinkParse.cpp index 4875aefd7..8aa45b271 100644 --- a/src/V3LinkParse.cpp +++ b/src/V3LinkParse.cpp @@ -123,6 +123,13 @@ private: && (VN_IS(nodep->stmtsp(), GenIf)) // Begin has if underneath && !nodep->stmtsp()->nextp()); // Has only one item } + bool hasStaticDeclAssignments(AstNodeFTask* nodep) { + for (const AstNode* itemp = nodep->stmtsp(); itemp; itemp = itemp->nextp()) { + const AstVar* const varp = VN_CAST(itemp, Var); + if (varp && varp->valuep() && !varp->lifetime().isAutomatic()) return true; + } + return false; + } // VISITs void visit(AstNodeFTask* nodep) override { @@ -133,10 +140,26 @@ private: VL_RESTORER(m_lifetime); { m_ftaskp = nodep; - m_lifetime = nodep->lifetime(); - if (m_lifetime.isNone()) { - // Automatic always until we support static - m_lifetime = VLifetime::AUTOMATIC; + if (!nodep->lifetime().isNone()) { + m_lifetime = nodep->lifetime(); + } else { + const AstClassOrPackageRef* const classPkgRefp + = VN_AS(nodep->classOrPackagep(), ClassOrPackageRef); + if (classPkgRefp && VN_IS(classPkgRefp->classOrPackageNodep(), Class)) { + // Class methods are automatic by default + m_lifetime = VLifetime::AUTOMATIC; + } else if (nodep->dpiImport()) { + // DPI-imported function don't have lifetime specifiers + m_lifetime = VLifetime::NONE; + } + if (m_lifetime.isStatic() && hasStaticDeclAssignments(nodep)) { + nodep->v3warn( + IMPLICITSTATIC, + "Function/task's lifetime implicitly set to static\n" + << nodep->warnMore() + << "... Suggest use 'function automatic' or 'function static'"); + } + nodep->lifetime(m_lifetime); } iterateChildren(nodep); } diff --git a/test_regress/t/t_array_query_with.v b/test_regress/t/t_array_query_with.v index 423670b00..bb4fa6137 100644 --- a/test_regress/t/t_array_query_with.v +++ b/test_regress/t/t_array_query_with.v @@ -19,19 +19,19 @@ module t (/*AUTOARG*/ return found.size() == 1; endfunction - function bit test_find_index; + function static bit test_find_index; int q[$] = {1, 2, 3, 4}; int found[$] = q.find_index(x) with (x <= 2); return found.size() == 2; endfunction - function bit test_find_first_index; + function static bit test_find_first_index; int q[] = {1, 2, 3, 4, 5, 6}; int first_even_idx[$] = q.find_first_index(x) with (x % 2 == 0); return first_even_idx[0] == 1; endfunction - function bit test_sort; + function automatic bit test_sort; int q[] = {-5, 2, -3, 0, 4}; q.sort(x) with (x >= 0 ? x : -x); return q[1] == 2; diff --git a/test_regress/t/t_assoc2.v b/test_regress/t/t_assoc2.v index 5b94ca3b9..7ceff9c25 100644 --- a/test_regress/t/t_assoc2.v +++ b/test_regress/t/t_assoc2.v @@ -21,7 +21,7 @@ module t (/*AUTOARG*/ // associative array of an associative array logic [31:0] a [logic [31:0]][logic [63:0]]; - task disp(); + task static disp(); int i = 60; imap[i++] = 600; imap[i++] = 601; diff --git a/test_regress/t/t_dynarray_param.v b/test_regress/t/t_dynarray_param.v index 72098b871..e16635584 100644 --- a/test_regress/t/t_dynarray_param.v +++ b/test_regress/t/t_dynarray_param.v @@ -12,7 +12,7 @@ module t(/*AUTOARG*/); typedef int calc_sums_t [3:0]; localparam int SUMS_ARRAY [3:0] = calc_sums_array(SIZES, 4); - function calc_sums_t calc_sums_array(int s[3:0], int n); + function automatic calc_sums_t calc_sums_array(int s[3:0], int n); int sum = 0; for (int ii = 0; ii < n; ++ii) begin sum = sum + s[ii]; @@ -23,7 +23,7 @@ module t(/*AUTOARG*/); `ifndef VERILATOR localparam int SUMS_DYN [3:0] = calc_sums_dyn(SIZES, 4); `endif - function calc_sums_t calc_sums_dyn(int s[], int n); + function automatic calc_sums_t calc_sums_dyn(int s[], int n); int sum = 0; for (int ii = 0; ii < n; ++ii) begin sum = sum + s[ii]; diff --git a/test_regress/t/t_func_bad.v b/test_regress/t/t_func_bad.v index d16ecd5e2..835ab2b45 100644 --- a/test_regress/t/t_func_bad.v +++ b/test_regress/t/t_func_bad.v @@ -34,7 +34,7 @@ module t; hasout = 0; endfunction - function int f( int j = 1, int dup = 0 ); + function automatic int f( int j = 1, int dup = 0 ); return (j<<16) | dup; endfunction diff --git a/test_regress/t/t_func_const.v b/test_regress/t/t_func_const.v index 2955bf3d2..2d8487195 100644 --- a/test_regress/t/t_func_const.v +++ b/test_regress/t/t_func_const.v @@ -102,7 +102,7 @@ module t; endcase endfunction - function integer f_return(input [31:0] a); + function automatic integer f_return(input [31:0] a); integer out = 2; while (1) begin out = out+1; diff --git a/test_regress/t/t_func_default_warn.v b/test_regress/t/t_func_default_warn.v index bf7ff2eff..fcc89134c 100644 --- a/test_regress/t/t_func_default_warn.v +++ b/test_regress/t/t_func_default_warn.v @@ -5,7 +5,7 @@ // without warranty, 2015 by Todd Strader. // SPDX-License-Identifier: CC0-1.0 -function logic foo +function automatic logic foo ( // Intentionally provide a non-width'ed default value // This should warn, not error out diff --git a/test_regress/t/t_func_named.v b/test_regress/t/t_func_named.v index 2ecd570d7..353232dd2 100644 --- a/test_regress/t/t_func_named.v +++ b/test_regress/t/t_func_named.v @@ -6,7 +6,7 @@ module t (/*AUTOARG*/); - function int f( int j = 1, int s = 0 ); + function automatic int f( int j = 1, int s = 0 ); return (j<<16) | s; endfunction diff --git a/test_regress/t/t_func_no_lifetime_bad.out b/test_regress/t/t_func_no_lifetime_bad.out new file mode 100644 index 000000000..ac2132066 --- /dev/null +++ b/test_regress/t/t_func_no_lifetime_bad.out @@ -0,0 +1,11 @@ +%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:7:14: Function/task's lifetime implicitly set to static + : ... Suggest use 'function automatic' or 'function static' + 7 | function int f_implicit_static(); + | ^~~~~~~~~~~~~~~~~ + ... For warning description see https://verilator.org/warn/IMPLICITSTATIC?v=latest + ... Use "/* verilator lint_off IMPLICITSTATIC */" and lint_on around source to disable this message. +%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:12:6: Function/task's lifetime implicitly set to static + : ... Suggest use 'function automatic' or 'function static' + 12 | task t_implicit_static(); + | ^~~~~~~~~~~~~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_func_no_lifetime_bad.pl b/test_regress/t/t_func_no_lifetime_bad.pl new file mode 100755 index 000000000..bd07fc421 --- /dev/null +++ b/test_regress/t/t_func_no_lifetime_bad.pl @@ -0,0 +1,23 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2023 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(simulator => 1); + +compile( + fails => $Self->{vlt_all}, + expect_filename => $Self->{golden_filename}, + ); + +execute( + check_finished => 1, + ) if !$Self->{vlt_all}; + +ok(1); +1; diff --git a/test_regress/t/t_func_no_lifetime_bad.v b/test_regress/t/t_func_no_lifetime_bad.v new file mode 100644 index 000000000..e77979cf3 --- /dev/null +++ b/test_regress/t/t_func_no_lifetime_bad.v @@ -0,0 +1,39 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +function int f_implicit_static(); + int cnt = 0; + return ++cnt; +endfunction + +task t_implicit_static(); + int cnt = 0; + $display("%d", ++cnt); +endtask + + +module t (/*AUTOARG*/ + // Inputs + clk + ); + + // verilator lint_off IMPLICITSTATIC + function int f_implicit_but_lint_off(); + int cnt = 0; + return ++cnt; + endfunction + + input clk; + + int a, b; + initial begin + a = f_implicit_static(); + t_implicit_static(); + b = f_implicit_but_lint_off(); + $stop; + end + +endmodule diff --git a/test_regress/t/t_func_under.v b/test_regress/t/t_func_under.v index f4d8f0155..dabe8f2b9 100644 --- a/test_regress/t/t_func_under.v +++ b/test_regress/t/t_func_under.v @@ -12,7 +12,7 @@ module t (/*AUTOARG*/ reg [3:0] counter = 0; integer l2; - function log2 (input [3:0] x); + function automatic log2 (input [3:0] x); integer log2 = (x < 2) ? 1 : (x < 4) ? 2 : (x < 8) ? 3 : 4; endfunction always @(posedge clk) begin diff --git a/test_regress/t/t_jumps_do_while.v b/test_regress/t/t_jumps_do_while.v index 217e46b3e..0f9705487 100644 --- a/test_regress/t/t_jumps_do_while.v +++ b/test_regress/t/t_jumps_do_while.v @@ -10,7 +10,7 @@ module t (/*AUTOARG*/ input clk; - function bit test_1; + function automatic bit test_1; int iterations = 0; do begin iterations++; @@ -20,7 +20,7 @@ module t (/*AUTOARG*/ return iterations == 1; endfunction - function bit test_2; + function automatic bit test_2; int iterations = 0; do begin break; @@ -37,7 +37,7 @@ module t (/*AUTOARG*/ return 1'b1; endfunction - function bit test_4; + function automatic bit test_4; int incr = 0; do begin incr++; @@ -48,7 +48,7 @@ module t (/*AUTOARG*/ return incr == 1; endfunction - function bit test_5; + function automatic bit test_5; int incr = 0; do begin do @@ -62,7 +62,7 @@ module t (/*AUTOARG*/ return incr == 10; endfunction - function bit test_6; + function automatic bit test_6; int incr = 0; do begin do begin @@ -78,7 +78,7 @@ module t (/*AUTOARG*/ return incr == 10; endfunction - function bit test_7; + function automatic bit test_7; int incr = 0; do begin do begin @@ -95,7 +95,7 @@ module t (/*AUTOARG*/ return incr == 2; endfunction - function bit test_8; + function automatic bit test_8; int incr = 0; do begin incr++; @@ -106,7 +106,7 @@ module t (/*AUTOARG*/ return incr == 1; endfunction - function bit test_9; + function automatic bit test_9; int incr = 0; do begin incr++; @@ -117,7 +117,7 @@ module t (/*AUTOARG*/ return incr == 5; endfunction - function bit test_10; + function automatic bit test_10; do begin continue; end @@ -125,7 +125,7 @@ module t (/*AUTOARG*/ return 1'b1; endfunction - function bit test_11; + function automatic bit test_11; int incr = 0; do begin do @@ -139,7 +139,7 @@ module t (/*AUTOARG*/ return incr == 12; endfunction - function bit test_12; + function automatic bit test_12; int incr = 0; do begin do begin diff --git a/test_regress/t/t_param_array3.v b/test_regress/t/t_param_array3.v index fb6926365..3d9d98fb1 100644 --- a/test_regress/t/t_param_array3.v +++ b/test_regress/t/t_param_array3.v @@ -8,7 +8,7 @@ module t; parameter int SIZES [3:0] = '{1,2,3,4}; typedef int calc_sums_t [3:0]; - function calc_sums_t calc_sums; + function static calc_sums_t calc_sums; int sum = 0; for (int i=0; i<4; i++) begin sum = sum + SIZES[i]; diff --git a/test_regress/t/t_param_func2.v b/test_regress/t/t_param_func2.v index 432d2914d..1a4630d77 100644 --- a/test_regress/t/t_param_func2.v +++ b/test_regress/t/t_param_func2.v @@ -29,7 +29,7 @@ endmodule module sub; parameter WIDTH = 1; - function [WIDTH-1:0] orer; + function automatic [WIDTH-1:0] orer; input [WIDTH-1:0] in; // IEEE provices no way to override this parameter, basically it's a localparam parameter MASK_W = WIDTH - 2; @@ -39,7 +39,7 @@ module sub; // verilator lint_on WIDTH endfunction - function [WIDTH-1:0] orer2; + function automatic [WIDTH-1:0] orer2; input [WIDTH-1:0] in; // Same param names as other function to check we disambiguate // IEEE provices no way to override this parameter, basically it's a localparam diff --git a/test_regress/t/t_stream_integer_type.v b/test_regress/t/t_stream_integer_type.v index 2b606829f..bb7c94533 100644 --- a/test_regress/t/t_stream_integer_type.v +++ b/test_regress/t/t_stream_integer_type.v @@ -236,7 +236,7 @@ module t (/*AUTOARG*/ end endfunction : print_data_error - function void print_all_data (string name = ""); + function static void print_all_data (string name = ""); foreach (byte_in[i]) $display(" %s byte_in[%0d]=%0h, byte_out=%0h ", name, i, byte_in[i], byte_out[i]); $display(" %s packed_data_32=%0h, packed_data_32_ref=%0h", name, packed_data_32, packed_data_32_ref); diff --git a/test_regress/t/t_trace_string.v b/test_regress/t/t_trace_string.v index cb48d4770..065a7c2f9 100644 --- a/test_regress/t/t_trace_string.v +++ b/test_regress/t/t_trace_string.v @@ -22,7 +22,7 @@ module t (/*AUTOARG*/ localparam string REGX [0:31] = '{"zero", "ra", "sp", "gp", "tp", "t0", "t1", "t2", "s0/fp", "s1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "s2", "s3", "s4", "s5", "s6", "s7", "s8", "s9", "s10", "s11", "t3", "t4", "t5", "t6"}; - function string regx (logic [5-1:0] r, bit abi=1'b0); + function automatic string regx (logic [5-1:0] r, bit abi=1'b0); regx = abi ? REGX[r] : $sformatf("x%0d", r); endfunction: regx diff --git a/test_regress/t/t_unpacked_str_init2.v b/test_regress/t/t_unpacked_str_init2.v index 9481aafb8..3d31cb54d 100644 --- a/test_regress/t/t_unpacked_str_init2.v +++ b/test_regress/t/t_unpacked_str_init2.v @@ -14,12 +14,12 @@ module t (/*AUTOARG*/); "s3", "s4", "s5", "s6", "s7", "s8", "s9", "s10", "s11", "t3", "t4", "t5", "t6"}; - function string reg_x (logic [4:0] r, bit abi=1'b0); + function automatic string reg_x (logic [4:0] r, bit abi=1'b0); reg_x = abi ? REG_X[r] : $sformatf("x%0d", r); endfunction // the issue is triggered by a second function containing a case statement - function string f2 (logic [4:0] r, bit abi=0); + function automatic string f2 (logic [4:0] r, bit abi=0); case (r) 5'd0: f2 = $sformatf("nop"); 5'd1: f2 = $sformatf("reg %s", reg_x(r[4:0], abi)); diff --git a/test_regress/t/t_var_static.pl b/test_regress/t/t_var_static.pl index 8d48ddb75..dc589151c 100755 --- a/test_regress/t/t_var_static.pl +++ b/test_regress/t/t_var_static.pl @@ -11,6 +11,7 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(simulator => 1); compile( + verilator_flags2 => ['-Wno-IMPLICITSTATIC'], fails => $Self->{vlt_all}, # Verilator unsupported, bug546 expect_filename => $Self->{golden_filename}, ); diff --git a/test_regress/t/t_var_static_param.out b/test_regress/t/t_var_static_param.out index ac3c941a9..2a08df190 100644 --- a/test_regress/t/t_var_static_param.out +++ b/test_regress/t/t_var_static_param.out @@ -1,6 +1,6 @@ -%Error-UNSUPPORTED: t/t_var_static_param.v:32:18: Unsupported: 'static' function/task variables +%Error-UNSUPPORTED: t/t_var_static_param.v:33:18: Unsupported: 'static' function/task variables : ... In instance t.subb - 32 | static int st = 2; st += P; return st; + 33 | static int st = 2; st += P; return st; | ^~ ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest %Error: Exiting due to diff --git a/test_regress/t/t_var_static_param.v b/test_regress/t/t_var_static_param.v index 89f381d98..83972619f 100644 --- a/test_regress/t/t_var_static_param.v +++ b/test_regress/t/t_var_static_param.v @@ -27,6 +27,7 @@ endmodule module sub; parameter P = 1; + // verilator lint_off IMPLICITSTATIC function int f_no_st (); // This static is unique within each parameterized module static int st = 2; st += P; return st;