Fix void-cast queue pop_front or pop_back (#3542) (#3364)

Fix compile error for queue method usage, if it is the
first statement in a block of code, and the return
value is not used. Example:

>  if (foo)
>    void'(bar.pop_front());

src/V3Ast.h

@@ -1509,6 +1509,16 @@ public:
     AstNode* firstAbovep() const {  // Returns nullptr when second or later in list
         return ((backp() && backp()->nextp() != this) ? backp() : nullptr);
     }
+    // isFirstInMyListOfStatements(n) -- implemented by child classes:
+    // AstNodeBlock, AstCaseItem, AstNodeIf, AstNodeFTask, and possibly others.
+    virtual bool isFirstInMyListOfStatements(AstNode* n) const { return false; }
+    // isStandaloneBodyStmt == Do we need a ; on generated cpp for this node?
+    bool isStandaloneBodyStmt() {
+        return (!firstAbovep()  // we're 2nd or later in the list, so yes need ;
+
+                // If we're first in the list, check what backp() thinks of us:
+                || (backp() && backp()->isFirstInMyListOfStatements(this)));
+    }
     uint8_t brokenState() const { return m_brokenState; }
     void brokenState(uint8_t value) { m_brokenState = value; }
 
@@ -2566,6 +2576,7 @@ public:
     AstNode* stmtsp() const { return op1p(); }  // op1 = List of statements
     void addStmtsp(AstNode* nodep) { addNOp1p(nodep); }
     bool unnamed() const { return m_unnamed; }
+    bool isFirstInMyListOfStatements(AstNode* nodep) const override { return nodep == stmtsp(); }
 };
 
 class AstNodePreSel VL_NOT_FINAL : public AstNode {
@@ -2711,6 +2722,9 @@ public:
     VBranchPred branchPred() const { return m_branchPred; }
     void isBoundsCheck(bool flag) { m_isBoundsCheck = flag; }
     bool isBoundsCheck() const { return m_isBoundsCheck; }
+    bool isFirstInMyListOfStatements(AstNode* n) const override {
+        return n == ifsp() || n == elsesp();
+    }
 };
 
 class AstNodeCase VL_NOT_FINAL : public AstNodeStmt {
@@ -3237,6 +3251,7 @@ public:
     bool isVirtual() const { return m_virtual; }
     void lifetime(const VLifetime& flag) { m_lifetime = flag; }
     VLifetime lifetime() const { return m_lifetime; }
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == stmtsp(); }
 };
 
 class AstNodeFTaskRef VL_NOT_FINAL : public AstNodeStmt {

src/V3AstNodes.h

@@ -3586,6 +3586,7 @@ public:
     void addStmtp(AstNode* nodep) { addOp2p(nodep); }
     // Special accessors
     bool isJustOneBodyStmt() const { return bodysp() && !bodysp()->nextp(); }
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstAssign final : public AstNodeAssign {
@@ -4009,6 +4010,7 @@ public:
     void condsp(AstNode* nodep) { setOp1p(nodep); }
     void addBodysp(AstNode* newp) { addOp2p(newp); }
     bool isDefault() const { return condsp() == nullptr; }
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstSFormatF final : public AstNode {
@@ -4694,6 +4696,7 @@ public:
     virtual bool isGateOptimizable() const override { return false; }
     virtual int instrCount() const override { return INSTR_COUNT_BRANCH; }
     virtual bool same(const AstNode* /*samep*/) const override { return true; }
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstRepeat final : public AstNodeStmt {
@@ -4711,6 +4714,7 @@ public:
     }  // Not relevant - converted to FOR
     virtual int instrCount() const override { return INSTR_COUNT_BRANCH; }
     virtual bool same(const AstNode* /*samep*/) const override { return true; }
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstWait final : public AstNodeStmt {
@@ -4722,6 +4726,7 @@ public:
     }
     ASTNODE_NODE_FUNCS(Wait)
     AstNode* bodysp() const { return op3p(); }  // op3 = body of loop
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstWhile final : public AstNodeStmt {
@@ -4748,6 +4753,7 @@ public:
     virtual void addBeforeStmt(AstNode* newp, AstNode* belowp) override;
     // Stop statement searchback here
     virtual void addNextStmt(AstNode* newp, AstNode* belowp) override;
+    bool isFirstInMyListOfStatements(AstNode* n) const override { return n == bodysp(); }
 };
 
 class AstBreak final : public AstNodeStmt {

src/V3Width.cpp

@@ -3090,7 +3090,9 @@ private:
             newp = new AstCMethodHard(nodep->fileline(), nodep->fromp()->unlinkFrBack(),
                                       nodep->name());
             newp->dtypeFrom(adtypep->subDTypep());
-            if (!nodep->firstAbovep()) newp->makeStatement();
+            // Because queue methods pop_front() or pop_back() can be void cast,
+            // they use makeStatement to check if they need the c++ ";" added.
+            if (nodep->isStandaloneBodyStmt()) newp->makeStatement();
         } else if (nodep->name() == "push_back" || nodep->name() == "push_front") {
             methodOkArguments(nodep, 1, 1);
             methodCallLValueRecurse(nodep, nodep->fromp(), VAccess::WRITE);

test_regress/t/t_void_queue_ops.pl

@@ -0,0 +1,21 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2003 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(
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_void_queue_ops.v

@@ -0,0 +1,190 @@
+module t
+  (/*AUTOARG*/
+   // Inputs
+   clk
+   );
+
+  input clk;
+
+  int   cyc = 0;
+
+
+  // Test for https://github.com/verilator/verilator/issues/3364
+  // Make sure all SV queue API is supported and verilator can generate
+  // compile-able C++ models for it.
+
+  // simple queue
+  logic [31:0] my_int_queue [$];
+
+  // On the functions and tasks, the my_int_queue.pop_[front|back]() call will
+  // have nodep->firstAbovep() != nullptr. Because the pop_front or pop_back is
+  // the first node on the "list".
+  // To fix this, V3Width.cpp will not use firstAbovep(), and instead us
+  // isStandalongStmt() -- which checks if the pop_front or pop_back is
+  // 2nd or later, or if it's first in the list that it's in a "block" of code.
+  // For functions/tasks, that is checked with:
+  // VN_IS(backp(), NodeFTask)=True, so even though
+  function automatic void f_pop_back__my_int_queue();
+    void'(my_int_queue.pop_back());
+  endfunction : f_pop_back__my_int_queue
+
+  function automatic void f_pop_front__my_int_queue();
+    void'(my_int_queue.pop_front());
+  endfunction : f_pop_front__my_int_queue
+
+  task automatic t_pop_back__my_int_queue();
+    void'(my_int_queue.pop_back());
+  endtask : t_pop_back__my_int_queue
+
+  task automatic t_pop_front__my_int_queue();
+    void'(my_int_queue.pop_front());
+  endtask : t_pop_front__my_int_queue
+
+
+  task automatic do_random_queue_operation();
+    bit [7:0] rand_op;
+    int       rand_index;
+    logic [31:0] item;
+
+
+    rand_op = 8'($urandom_range(32, 0));
+    case(rand_op)
+    8'd0: ; // nop
+
+    // pushes (2x of these)
+    8'd1, 8'd2: my_int_queue.push_back($urandom);
+    8'd3, 8'd4: my_int_queue.push_front($urandom);
+
+    // delete:
+    8'd5: my_int_queue.delete();
+
+    // insert(index, item):
+    8'd6: begin
+      rand_index = $urandom_range(my_int_queue.size());
+      my_int_queue.insert(rand_index, item);
+    end
+
+    // shuffle
+    8'd7: my_int_queue.shuffle();
+
+    // Various pops for rand_op >= 8:
+    // pops to var
+    // V3Width debug -- firstAbovep()=ASSIGN (which I guess does the ; for us
+    //                                        so we don't need the queue op to
+    //                                        do it.)
+    // isStandalongStmt() will ignore ASSIGN, return false (NodeAssign is
+    // child of AstNodeStmt)
+    8'd8: if (my_int_queue.size() > 0) item = my_int_queue.pop_front();
+    8'd9: if (my_int_queue.size() > 0) item = my_int_queue.pop_back();
+
+    // pops to the void
+    // V3Width debug -- firstAbovep()=IF
+    // This is fixed with isStandalongStmt() -- VN_IS(backp(), NodeIf)=True
+    8'd10: if (my_int_queue.size() > 0) void'(my_int_queue.pop_front());
+    8'd11: if (my_int_queue.size() > 0) void'(my_int_queue.pop_back());
+
+    // pop result to the lhs of a condition, and do something with it.
+    8'd12:
+      if (my_int_queue.size() > 0)
+        // V3Width debug -- firstAbovep()=LTE (good we don't want a ; here)
+        if (my_int_queue.pop_front() <= 2022)
+          my_int_queue.push_front(3022); // living in the year 3022.
+
+    // pop result to the rhs of a condition, and do something with it.
+    8'd13:
+      if (my_int_queue.size() > 0)
+        // V3Width debug -- firstAbovep()=GT (good we don't want a ; here)
+        if (4022 > my_int_queue.pop_front())
+          my_int_queue.push_front(3023); // living in the year 3023.
+
+    // pops to the void after yet another case:
+    // V3Width debug -- firstAbovep()=CASEITEM (not a nullptr)
+    // This is fixed with isStandalongStmt() -- VN_IS(backp(), CaseItem)=True
+    8'd14:
+      case (my_int_queue.size() > 0)
+      0: ;
+      1: void'(my_int_queue.pop_front());
+      default: ;
+      endcase // case (my_int_queue.size() > 0)
+
+    // V3Width debug -- firstAbovep()=CASEITEM (not a nullptr)
+    // backp()->nextp()=CASEITEM (different one)
+    // This is fixed with isStandalongStmt() -- VN_IS(backp(), CaseItem)=True
+    8'd15:
+      case (my_int_queue.size() > 0)
+      0: ;
+      1: void'(my_int_queue.pop_back());
+      default;
+      endcase // case (my_int_queue.size() > 0)
+
+    // pops in a function or task
+    8'd16: if (my_int_queue.size() > 0) f_pop_back__my_int_queue();
+    8'd17: if (my_int_queue.size() > 0) f_pop_front__my_int_queue();
+    8'd18: if (my_int_queue.size() > 0) t_pop_back__my_int_queue();
+    8'd19: if (my_int_queue.size() > 0) t_pop_front__my_int_queue();
+
+    // But what if we put some dummy code before the pop_back() or pop_front():
+    8'd20: begin
+      if (my_int_queue.size() > 0) begin
+        ; // dummy line
+        // V3Width debug -- firstAbovep()=BEGIN (is not nullptr).
+        // This is fixed with isStandalongStmt() -- VN_IS(backp(), NodeIf)=True
+        void'(my_int_queue.pop_back());
+      end
+    end
+    8'd21: begin
+      automatic int temp_int = 0;
+      if (my_int_queue.size() > 0) begin
+        temp_int = 5; // dummy line
+        // V3Width debug -- firstAbovep()=nullptr (good)
+        void'(my_int_queue.pop_back());
+      end
+    end
+    8'd22: begin
+      if (my_int_queue.size() > 0) begin
+        automatic int some_temp_dummy_int;
+        some_temp_dummy_int = 42;
+        // V3Width debug -- firstAbovep()=nullptr (good)
+        void'(my_int_queue.pop_back());
+      end
+    end
+    8'd23: begin
+      if (my_int_queue.size() > 0) begin
+        // no dummy here, just a 'begin' helper before it.
+        // V3Width debug -- firstAbovep()=BEGIN (is not nullptr).
+        // This is fixed with isStandalongStmt() -- VN_IS(backp(), NodeIf)=True
+        void'(my_int_queue.pop_back());
+      end
+    end
+
+    // What about an if of something else, followed by a pop_front?
+    8'd24: begin
+      automatic int temp_int = 0;
+      if (my_int_queue.size() == 0) begin // dummy
+        temp_int = 1000;
+      end
+      void'(my_int_queue.pop_front()); // firstAbovep() should be nullptr here.
+    end
+
+
+    default: ; // nop
+    endcase // case (rand_op)
+
+  endtask : do_random_queue_operation
+
+
+
+  always @ (posedge clk) begin : main
+    cyc <= cyc + 1;
+
+    do_random_queue_operation();
+
+    if (cyc > 100) begin
+      $write("*-* All Finished *-*\n");
+      $finish();
+    end
+  end
+
+
+
+endmodule : t