Add IMPLICITSTATIC warning when a ftask/function is implicitly static (#3839)

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.

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

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 == CMPCONST || m_e == COLONPLUS || m_e == IMPLICIT || m_e == IMPLICITSTATIC
-                || m_e == LITENDIAN || m_e == PINMISSING || m_e == REALCVT || m_e == UNSIGNED
+                || m_e == LATCH || m_e == LITENDIAN || m_e == PINMISSING || m_e == REALCVT
-                || m_e == WIDTH);
+                || m_e == UNSIGNED || m_e == WIDTH);
     }
     // Warnings that are style only
     bool styleError() const VL_MT_SAFE {

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 (!nodep->lifetime().isNone()) {
-                if (m_lifetime.isNone()) {
+                    m_lifetime = nodep->lifetime();
-                    // Automatic always until we support static
+                } else {
-                    m_lifetime = VLifetime::AUTOMATIC;
+                    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);
             }

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;

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;

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];

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
 

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;

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

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
 

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

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;

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

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

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

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];

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

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

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
 

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

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},
     );

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

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;