Got incorrect warning message in latest releases version 4.202

[yanjiuntw]

Hello,

I use latest releases version to check lint of my verilog file, but I got a incorrect message.

What 'verilator --version' are you using? Did you try it with the git master version?

$ verilator --version
Verilator 4.202 2021-04-24 rev v4.202

Can you attach an example that shows the issue? (Must be openly licensed, ideally in test_regress format.)

Here is my verilog file content:

// test.v
module test (
  input      [2:0] a,
  input      [7:0] c,

  output reg [7:0] b
);

  integer i;

  always @ (*)
  begin
    case(a)
      {3'b000}:
        b = 8'd1;
      {3'b001}:
        for(i=0;i<4;i=i+1) b[i*2+:2] = {2{c[i]}};
      {3'b010}:
        b = 8'd3;
      {3'b011}:
        b = 8'd4;
      default: b = 0;
    endcase
  end

endmodule

And I use this command run the test, and got a Warning-LATCH message:

$ verilator --lint-only -Wall test.v
%Warning-LATCH: test.v:10:3: Latch inferred for signal 'b' (not all control paths of combinational always assign a value)
                           : ... Suggest use of always_latch for intentional latches
   10 |   always @ (*)
      |   ^~~~~~
                ... For warning description see https://verilator.org/warn/LATCH?v=4.202
                ... Use "/* verilator lint_off LATCH */" and lint_on around source to disable this message.
%Error: Exiting due to 1 warning(s)

Would you be willing to try to fix Verilator yourself with assistance?

I will try if I can understand what wrong.

[udif]

Out of curiosity, and taking advantage of a fast 8-core machine and git bisect, I found that the warnings started appearing on:

commit a11700271fc0c681bb1869bf6a299ff594c4028a (refs/bisect/bad)
Author: Julien Margetts <56540603+margej@users.noreply.github.com>
Date:   Tue Jan 5 19:26:01 2021 +0000

    Add LATCH and NOLATCH warnings (#1609) (#2740).

I don't plan doing any further work on this, but it was easy for me to at least help in locating where the issue started.
judging by the commit description, it's not a regression but rather a new feature that never worked for this testcase.

[wsnyder]

Makes sense. I suspect it gets confused by the bit select range. @margej mind taking and look and making a proposal to fix it?

[margej]

Sure, I'll take a look

[margej]

Latch detection graph does not detect assignment to 'b' in the 3rd if (as commented below). This is probably because the assignments to b here depend upon the current value of b:

        // Body
        if ((4U & (IData)(vlTOPp->a))) {
            vlTOPp->b = 0U;
        } else {
            if ((2U & (IData)(vlTOPp->a))) {
                vlTOPp->b = ((1U & (IData)(vlTOPp->a))
                              ? 4U : 3U);
            } else {
                if ((1U & (IData)(vlTOPp->a))) { // Assignment to b NOT detected here
                    vlTOPp->b = ((0xf0U & (IData)(vlTOPp->b)) 
                                 | ((4U & ((IData)(vlTOPp->c) 
                                           << 1U)) 
                                    | (1U & (IData)(vlTOPp->c))));
                    vlTOPp->b = ((0xfU & (IData)(vlTOPp->b)) 
                                 | ((0x40U & ((IData)(vlTOPp->c) 
                                              << 3U)) 
                                    | (0x10U & 
                                       ((IData)(vlTOPp->c) 
                                        << 2U))));
                } else {
                    vlTOPp->b = 1U;
                }
            }
        }

[margej]

The highlighted 'if' node should be green

[margej]

The empty math node visitor 'optimisation' was preventing the graph pass reaching the varRef visitors when part select was used. The following patch seems to resolve the issue:

--- a/src/V3Active.cpp
+++ b/src/V3Active.cpp
@@ -317,8 +317,6 @@ private:
         iterateAndNextNull(nodep->elsesp());
         m_graph.currentp(parentp);
     }
-    // Empty visitors, speed things up
-    virtual void visit(AstNodeMath* nodep) {}
     //--------------------
     virtual void visit(AstNode* nodep) { iterateChildren(nodep); }

[wsnyder]

Can you please convert that to a pull, along with a test, then we can merge it. Thanks for debugging etc!

[wsnyder]

Oh, #2948

[margej]

Sure, I'll add the test case provided by @yanjiuntw above (when I remind myself how to add a test)

[margej]

'fix' seems to have lifted the lid on a bunch more false latch detections, but for some reason I can run any tests locally
Even 'make test' fails (as does almost every test in make test_regress with the same sort of error) with:

Archive ar -rcs Vt_a1_first_cc__ALL.a Vt_a1_first_cc__ALL.o
ar: @Vt_a1_first_cc__ALL.a.tmp: No such file or directory
g++    Vt_a1_first_cc__main.o verilated.o verilated_vcd_c.o Vt_a1_first_cc__ALL.a      -o Vt_a1_first_cc
g++: error: Vt_a1_first_cc__ALL.a: No such file or directory
make[1]: *** [Vt_a1_first_cc] Error 1

Clearly something wrong with my setup, but any pointers on what might be wrong would be gratefully received ?
This is with an unmodified checkout of tag v4.202 btw

[margej]

I just realised I've been posting all my comments on the PR, not this issue, so 1, the above was resolved by #2921
2. Please see #2948 (comment) for question on resolving new test failure

[margej]

Re: Issues on PR #2948 I need to distinguish AstIf nodes used for array bounds checking from those present in the RTL. It appears these are generated in V3Unknown.

My hope is I can flag them at creation so they can be later ignored for the purposes of latch detection

[wsnyder]

Thanks for your work on this.