Fix false IMPLICITSTATIC on package functions.

Changes

@@ -28,6 +28,7 @@ Verilator 5.011 devel
 * Fix linking AstRefDType if it has parameterized class ref (#4164) (#4170). [Ryszard Rozak, Antmicro Ltd]
 * Fix crash caused by $display() optimization (#4165) (#4166). [Tudor Timi]
 * Fix detection of wire/reg duplicates.
+* Fix false IMPLICITSTATIC on package functions.
 
 
 Verilator 5.010 2023-04-30

docs/guide/warnings.rst

@@ -769,11 +769,14 @@ List Of Warnings
    then automatic as static prevents the function from being reentrant,
    which may be a source of bugs, and/or performance issues.
 
-   If the function does not require static behavior, change it to "function
+   If the function is in a module, and does not require static behavior,
-   automatic".
+   change it to "function automatic".
 
-   If the function requires static behavior, change it to "function
+   If the function is in a module, and requires static behavior, change it
-   static".
+   to "function static".
+
+   If the function is in a package, it defaults to static, and label the
+   function's variables as static.
 
    Ignoring this warning will only suppress the lint check; it will
    simulate correctly.

src/V3LinkParse.cpp

@@ -124,13 +124,6 @@ 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 {
@@ -156,12 +149,30 @@ private:
                         // DPI-imported functions and properties don't have lifetime specifiers
                         m_lifetime = VLifetime::NONE;
                     }
-                    if (m_lifetime.isStatic() && hasStaticDeclAssignments(nodep)) {
+                    for (AstNode* itemp = nodep->stmtsp(); itemp; itemp = itemp->nextp()) {
-                        nodep->v3warn(
+                        AstVar* const varp = VN_CAST(itemp, Var);
-                            IMPLICITSTATIC,
+                        if (varp && varp->valuep() && varp->lifetime().isNone()
-                            "Function/task's lifetime implicitly set to static\n"
+                            && m_lifetime.isStatic() && !varp->isIO()) {
-                                << nodep->warnMore()
+                            if (VN_IS(m_modp, Module)) {
-                                << "... Suggest use 'function automatic' or 'function static'");
+                                nodep->v3warn(IMPLICITSTATIC,
+                                              "Function/task's lifetime implicitly set to static\n"
+                                                  << nodep->warnMore()
+                                                  << "... Suggest use 'function automatic' or "
+                                                     "'function static'\n"
+                                                  << nodep->warnContextPrimary() << '\n'
+                                                  << varp->warnOther()
+                                                  << "... Location of implicit static variable\n"
+                                                  << varp->warnContextSecondary() << '\n'
+                                                  << "... Suggest use 'function automatic' or "
+                                                     "'function static'");
+                            } else {
+                                varp->v3warn(IMPLICITSTATIC,
+                                             "Variable's lifetime implicitly set to static\n"
+                                                 << nodep->warnMore()
+                                                 << "... Suggest use 'static' before "
+                                                    "variable declaration'");
+                            }
+                        }
                     }
                     nodep->lifetime(m_lifetime);
                 }

test_regress/t/t_func_no_lifetime_bad.out

@@ -1,11 +1,27 @@
-%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:7:14: Function/task's lifetime implicitly set to static
+%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:29:17: Function/task's lifetime implicitly set to static
-                                                        : ... Suggest use 'function automatic' or 'function static'
+                                                         : ... Suggest use 'function automatic' or 'function static'
-    7 | function int f_implicit_static();
+   29 |    function int f_implicit_static();
-      |              ^~~~~~~~~~~~~~~~~
+      |                 ^~~~~~~~~~~~~~~~~
+                         t/t_func_no_lifetime_bad.v:30:11: ... Location of implicit static variable
+   30 |       int cnt = 0;
+      |           ^~~
+... Suggest use 'function automatic' or 'function 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
+%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:34:9: Function/task's lifetime implicitly set to static
                                                         : ... Suggest use 'function automatic' or 'function static'
-   12 | task t_implicit_static();
+   34 |    task t_implicit_static();
-      |      ^~~~~~~~~~~~~~~~~
+      |         ^~~~~~~~~~~~~~~~~
+                         t/t_func_no_lifetime_bad.v:35:11: ... Location of implicit static variable
+   35 |       int cnt = 0;
+      |           ^~~
+... Suggest use 'function automatic' or 'function static'
+%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:9:8: Variable's lifetime implicitly set to static
+                                                        : ... Suggest use 'static' before variable declaration'
+    9 |    int cnt = 0;   
+      |        ^~~
+%Warning-IMPLICITSTATIC: t/t_func_no_lifetime_bad.v:15:8: Variable's lifetime implicitly set to static
+                                                        : ... Suggest use 'static' before variable declaration'
+   15 |    int cnt = 0;   
+      |        ^~~
 %Error: Exiting due to

test_regress/t/t_func_no_lifetime_bad.v

@@ -4,22 +4,38 @@
 // any use, without warranty, 2023 by Antmicro Ltd.
 // SPDX-License-Identifier: CC0-1.0
 
-function int f_implicit_static();
+// Not legal to put "static" here, so no warning
-   int cnt = 0;
+function int f_dunit_static();
+   int cnt = 0;  // Ok to require "static" here somehday
    return ++cnt;
 endfunction
 
-task t_implicit_static();
+// Not legal to put "static" here, so no warning
-   int cnt = 0;
+task t_dunit_static();
+   int cnt = 0;  // Ok to require "static" here somehday
    $display("%d", ++cnt);
 endtask
 
+task t_dunit_static_ok(input int in_ok = 1);
+   static int cnt_ok = 0;  // No warning here
+   $display("%d", ++cnt_ok);
+endtask
 
 module t (/*AUTOARG*/
    // Inputs
    clk
    );
 
+   function int f_implicit_static();
+      int cnt = 0;
+      return ++cnt;
+   endfunction
+
+   task t_implicit_static();
+      int cnt = 0;
+      $display("%d", ++cnt);
+   endtask
+
    // verilator lint_off IMPLICITSTATIC
    function int f_implicit_but_lint_off();
       int cnt = 0;
@@ -30,6 +46,10 @@ module t (/*AUTOARG*/
 
    int   a, b;
    initial begin
+      a = f_dunit_static();
+      t_dunit_static();
+      t_dunit_static_ok();
+
       a = f_implicit_static();
       t_implicit_static();
       b = f_implicit_but_lint_off();