From b752faa10713e0b6b017b67e6b2a6a05d8b37343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Boro=C5=84ski?= <94375110+kboronski-ant@users.noreply.github.com> Date: Fri, 11 Aug 2023 18:28:37 +0200 Subject: [PATCH] Fix jumping over object initialization (#4411) --- src/V3AstNodeOther.h | 10 +++++ src/V3Const.cpp | 7 +++ src/V3EmitCFunc.h | 5 +++ src/V3Timing.cpp | 20 +++++++++ .../t/t_jumps_uninit_destructor_call.pl | 23 ++++++++++ .../t/t_jumps_uninit_destructor_call.v | 45 +++++++++++++++++++ 6 files changed, 110 insertions(+) create mode 100755 test_regress/t/t_jumps_uninit_destructor_call.pl create mode 100644 test_regress/t/t_jumps_uninit_destructor_call.v diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index dc5f038e1..fb3c31789 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -699,6 +699,16 @@ public: && finalsp() == nullptr; } }; +class AstCLocalScope final : public AstNode { + // Pack statements into an unnamed scope when generating C++ + // @astgen op1 := stmtsp : List[AstNode] +public: + AstCLocalScope(FileLine* fl, AstNode* stmtsp) + : ASTGEN_SUPER_CLocalScope(fl) { + this->addStmtsp(stmtsp); + } + ASTGEN_MEMBERS_AstCLocalScope; +}; class AstCUse final : public AstNode { // C++ use of a class or #include; indicates need of forward declaration // Parents: NODEMODULE diff --git a/src/V3Const.cpp b/src/V3Const.cpp index c8448ff6c..8eca1a198 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -2288,6 +2288,13 @@ private: iterateChildren(nodep); } } + void visit(AstCLocalScope* nodep) override { + iterateChildren(nodep); + if (!nodep->stmtsp()) { + nodep->unlinkFrBack(); + VL_DO_DANGLING(nodep->deleteTree(), nodep); + } + } void visit(AstScope* nodep) override { // No ASSIGNW removals under scope, we've long eliminated INITIALs VL_RESTORER(m_wremove); diff --git a/src/V3EmitCFunc.h b/src/V3EmitCFunc.h index ba7c10e54..351fd956a 100644 --- a/src/V3EmitCFunc.h +++ b/src/V3EmitCFunc.h @@ -883,6 +883,11 @@ public: iterateAndNextConstNull(nodep->endStmtsp()); puts("}\n"); } + void visit(AstCLocalScope* nodep) override { + puts("{\n"); + iterateAndNextConstNull(nodep->stmtsp()); + puts("}\n"); + } void visit(AstJumpGo* nodep) override { puts("goto __Vlabel" + cvtToStr(nodep->labelp()->blockp()->labelNum()) + ";\n"); } diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index ab099d647..0cca7835a 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -52,6 +52,7 @@ #include "V3Ast.h" #include "V3Const.h" #include "V3EmitV.h" +#include "V3Global.h" #include "V3Graph.h" #include "V3MemberMap.h" #include "V3SenExprBuilder.h" @@ -388,6 +389,7 @@ private: AstNode* m_procp = nullptr; // NodeProcedure/CFunc/Begin we're under double m_timescaleFactor = 1.0; // Factor to scale delays by int m_forkCnt = 0; // Number of forks inside a module + bool m_underJumpBlock = false; // True if we are inside of a jump-block // Unique names V3UniqueNames m_contAssignVarNames{"__VassignWtmp"}; // Names for temp AssignW vars @@ -594,6 +596,17 @@ private: m_netlistp->typeTablep()->addTypesp(m_forkDtp); return m_forkDtp; } + // Move `insertBeforep` into `AstCLocalScope` if necessary to avoid jumping over + // a variable initialization that whould be inserted before `insertBeforep`. All + // access to this variable shoule be contained within returned `AstCLocalScope`. + AstCLocalScope* addCLocalScope(FileLine* const flp, AstNode* const insertBeforep) const { + if (!insertBeforep || !m_underJumpBlock) return nullptr; + VNRelinker handle; + insertBeforep->unlinkFrBack(&handle); + AstCLocalScope* const cscopep = new AstCLocalScope{flp, insertBeforep}; + handle.relink(cscopep); + return cscopep; + } // Create a temp variable and optionally put it before the specified node (mark local if so) AstVarScope* createTemp(FileLine* const flp, const std::string& name, AstNodeDType* const dtypep, AstNode* const insertBeforep = nullptr) { @@ -625,6 +638,7 @@ private: FileLine* const flp = forkp->fileline(); // If we're in a function, insert the sync var directly before the fork AstNode* const insertBeforep = m_classp ? forkp : nullptr; + addCLocalScope(flp, insertBeforep); AstVarScope* forkVscp = createTemp(flp, forkp->name() + "__sync", getCreateForkSyncDTypep(), insertBeforep); unsigned joinCount = 0; // Needed for join counter @@ -691,6 +705,11 @@ private: new AstCStmt{nodep->fileline(), "vlProcess->state(VlProcess::FINISHED);\n"}); } } + void visit(AstJumpBlock* nodep) override { + VL_RESTORER(m_underJumpBlock); + m_underJumpBlock = true; + visit(static_cast(nodep)); + } void visit(AstAlways* nodep) override { if (nodep->user1SetOnce()) return; VL_RESTORER(m_procp); @@ -908,6 +927,7 @@ private: // Insert new vars before the timing control if we're in a function; in a process we can't // do that. These intra-assignment vars will later be passed to forked processes by value. AstNode* const insertBeforep = m_classp ? controlp : nullptr; + addCLocalScope(flp, insertBeforep); // Function for replacing values with intermediate variables const auto replaceWithIntermediate = [&](AstNodeExpr* const valuep, const std::string& name) { diff --git a/test_regress/t/t_jumps_uninit_destructor_call.pl b/test_regress/t/t_jumps_uninit_destructor_call.pl new file mode 100755 index 000000000..b8493bd06 --- /dev/null +++ b/test_regress/t/t_jumps_uninit_destructor_call.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( + verilator_flags2 => ["--exe --main --timing"], + make_main => 0, + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_jumps_uninit_destructor_call.v b/test_regress/t/t_jumps_uninit_destructor_call.v new file mode 100644 index 000000000..9354f989e --- /dev/null +++ b/test_regress/t/t_jumps_uninit_destructor_call.v @@ -0,0 +1,45 @@ +// 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 +class Foo; + string arra[2]; + string arrb[string]; + + function new(); + arra = '{"baz", "boo"}; + arrb = '{"baz": "inga!", "boo": "..."}; + endfunction + + task automatic return_before_fork(bit b); + if (b) begin + // This is going to translate to a `goto` statement. + return; + end + // This will instantiate a `VlForkSync` object (local to the call, as we are under a class) + fork + #10 $display("forked process"); + join + // This is where we jump to from the aforementioned goto. If we don't wrap the `VlForkSync` + // object in a scope that ends before that point, we will end up calling its destructor + // without having it initialized first. + endtask + task automatic return_before_select(bit b, int idx); + if (b) return; // goto + // This will create two temporary strings used to select from `arrb` and assign to it. + arrb[arra[idx]] = #10 "yah!"; + // jump here + endtask +endclass + +module t(); + initial begin + Foo foo; + foo = new; + foo.return_before_fork(1'b1); + foo.return_before_select(1'b1, 1); + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule