From c3be3c80501bec8d4c3b7f5256890332df62d410 Mon Sep 17 00:00:00 2001 From: Marco Bartoli Date: Wed, 29 Jul 2026 15:49:34 +0200 Subject: [PATCH] Fix nested named fork disable propagation (Part 4 of #7857) (#8001) --- src/V3LinkJump.cpp | 71 ++++----- src/V3Timing.cpp | 15 +- test_regress/t/t_disable_fork_deep.py | 18 +++ test_regress/t/t_disable_fork_deep.v | 217 ++++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 39 deletions(-) create mode 100755 test_regress/t/t_disable_fork_deep.py create mode 100644 test_regress/t/t_disable_fork_deep.v diff --git a/src/V3LinkJump.cpp b/src/V3LinkJump.cpp index ff0e0f2d7..4e80c1c96 100644 --- a/src/V3LinkJump.cpp +++ b/src/V3LinkJump.cpp @@ -68,6 +68,7 @@ class LinkJumpVisitor final : public VNVisitor { "__VprocessQueue"}; // Names for queues needed for 'disable' handling std::unordered_map m_taskDisableQueues; // Per-task process queues std::unordered_map m_beginDisableQueues; // Per-begin process queues + std::unordered_map m_forkDisableQueues; // Per-fork process queues std::unordered_map m_taskDisableBegins; // Per-task process wrappers std::unordered_map @@ -310,6 +311,21 @@ class LinkJumpVisitor final : public VNVisitor { m_beginDisableQueues.emplace(beginp, processQueuep); return processQueuep; } + AstVar* getOrCreateForkDisableQueuep(AstFork* const forkp, FileLine* const fl) { + const auto it = m_forkDisableQueues.find(forkp); + if (it != m_forkDisableQueues.end()) return it->second; + + AstVar* const processQueuep = getProcessQueuep(forkp, fl); + prependForkBranchQueuePushes(forkp, processQueuep, fl, true); + // Disabling a fork must also terminate detached descendants created by nested forks + // under each branch, so track nested fork branch processes in the same queue. + for (AstBegin* branchp = forkp->forksp(); branchp; + branchp = VN_AS(branchp->nextp(), Begin)) { + prependNestedForkBranchQueuePushes(branchp, processQueuep, fl, true); + } + m_forkDisableQueues.emplace(forkp, processQueuep); + return processQueuep; + } AstStmtExpr* insertKillStmtp(AstDisable* const nodep, AstVar* const processQueuep) { AstStmtExpr* const killStmtp = getQueueKillStmtp(nodep->fileline(), processQueuep); nodep->addNextHere(killStmtp); @@ -319,13 +335,13 @@ class LinkJumpVisitor final : public VNVisitor { AstJumpBlock* const jmpBlockp = getJumpBlock(targetp, false); killStmtp->addNextHere(new AstJumpGo{killStmtp->fileline(), jmpBlockp}); } - void handleDisableOnFork(AstDisable* const nodep, const std::vector& forks) { - // The support utilizes the process::kill()` method. For each `disable` a queue of - // processes is declared. At the beginning of each fork that can be disabled, its process - // handle is pushed to the queue. `disable` statement is replaced with calling `kill()` - // method on each element of the queue. + void handleDisableOnFork(AstDisable* const nodep) { + // The support utilizes the process::kill()` method. For each disabled fork a queue of + // processes is declared. At the beginning of each fork branch (and each nested fork + // branch) its process handle is pushed to the queue. The `disable` statement is replaced + // with calling `kill()` on each element of the queue. FileLine* const fl = nodep->fileline(); - AstNode* const targetp = nodep->targetp(); + AstFork* const targetp = VN_AS(nodep->targetp(), Fork); if (m_ftaskp) { if (!m_ftaskp->exists( [targetp](const AstNodeBlock* blockp) -> bool { return blockp == targetp; })) { @@ -334,33 +350,18 @@ class LinkJumpVisitor final : public VNVisitor { } } - AstPackage* const topPkgp = v3Global.rootp()->dollarUnitPkgAddp(); - AstVar* const processQueuep = getProcessQueuep(targetp, fl); - AstVarRef* const queueWriteRefp - = new AstVarRef{fl, topPkgp, processQueuep, VAccess::WRITE}; - AstStmtExpr* pushCurrentProcessp = getQueuePushProcessSelfp(queueWriteRefp); - - for (AstBegin* const beginp : forks) { - if (pushCurrentProcessp->backp()) { - pushCurrentProcessp = pushCurrentProcessp->cloneTree(false); - } - prependStmtsp(beginp, pushCurrentProcessp); - } + AstVar* const processQueuep = getOrCreateForkDisableQueuep(targetp, fl); AstStmtExpr* const killStmtp = insertKillStmtp(nodep, processQueuep); - // 'process::kill' does not immediately kill the current process - // executing the disable statement (because it's in the running state). - // If the disable statement is indeed executed by a process under the - // target AstFork, then jump to the end of that fork branch. - if (VN_IS(targetp, Fork)) { - AstNodeBlock* forkBranchp = nullptr; - for (AstNodeBlock* const blockp : vlstd::reverse_view(m_blockStack)) { - if (blockp == targetp) { - addJumpAfterKill(killStmtp, VN_AS(forkBranchp, Begin)); - break; - } - forkBranchp = blockp; - } + // 'process::kill' does not immediately kill the current process executing the disable + // statement (because it's in the running state). If the disable runs under the target + // fork, jump to the end of the innermost enclosing fork branch (the branch that holds the + // running process) so statements after the disable do not execute. Targeting the innermost + // branch keeps the jump inside a single emitted coroutine even when the disable sits in a + // nested sub-fork; targeting the outer target fork's branch would cross a coroutine + // boundary once forks are split into separate functions. + if (AstBegin* const branchp = innerForkBranchp(targetp)) { + addJumpAfterKill(killStmtp, branchp); } } // VISITORS @@ -553,12 +554,8 @@ class LinkJumpVisitor final : public VNVisitor { if (it != m_taskDisableBegins.end()) jumpTargetp = it->second; addJumpAfterKill(killStmtp, jumpTargetp); } - } else if (AstFork* const forkp = VN_CAST(targetp, Fork)) { - std::vector forks; - for (AstBegin* itemp = forkp->forksp(); itemp; itemp = VN_AS(itemp->nextp(), Begin)) { - forks.push_back(itemp); - } - handleDisableOnFork(nodep, forks); + } else if (VN_IS(targetp, Fork)) { + handleDisableOnFork(nodep); } else if (AstBegin* const beginp = VN_CAST(targetp, Begin)) { if (existsBlockAbove(beginp->name())) { if (!beginp->user3()) { diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index bbb1227c5..385bf8c8a 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -749,10 +749,21 @@ class TimingControlVisitor final : public VNVisitor { addDebugInfo(donep); beginp->addStmtsp(donep->makeStmt()); } - static bool hasDisableQueuePushSelfPrefix(const AstBegin* const beginp) { + static bool hasDisableQueuePushSelfPrefix(AstBegin* const beginp) { // LinkJump prepends disable-by-name registration as: // __VprocessQueue_*.push_back(std::process::self()) - return beginp->stmtsp() && beginp->stmtsp()->isDisableQueuePushSelfStmt(); + // V3LiftExpr lifts std::process::self() into an assignment to a temporary, then V3Task + // lowers that assignment's function call into a leading comment and AstStmtExpr. Thus the + // push_back is no longer necessarily the first statement. Scan across this generated + // prefix for it, stopping at the fork-start sentinel or the branch body. Registrations for + // nested forks live in sub-blocks and so are not in this statement list. + for (AstNode* stmtp = beginp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { + if (VN_IS(stmtp, Comment)) continue; + AstStmtExpr* const stmtExprp = VN_CAST(stmtp, StmtExpr); + if (!stmtExprp) break; + if (stmtExprp->isDisableQueuePushSelfStmt()) return true; + } + return false; } // Register a callback so killing a process-backed fork branch decrements the join counter void addForkOnKill(AstBegin* const beginp, AstVarScope* const forkVscp) const { diff --git a/test_regress/t/t_disable_fork_deep.py b/test_regress/t/t_disable_fork_deep.py new file mode 100755 index 000000000..6fe7d000c --- /dev/null +++ b/test_regress/t/t_disable_fork_deep.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# 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-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile(verilator_flags2=["--binary"]) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_disable_fork_deep.v b/test_regress/t/t_disable_fork_deep.v new file mode 100644 index 000000000..fa905d716 --- /dev/null +++ b/test_regress/t/t_disable_fork_deep.v @@ -0,0 +1,217 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// Disabling a named fork from a deeply nested sub-fork branch must terminate +// every process spawned within that fork, at all nesting levels, including +// branches suspended on a delay. Previously this crashed Verilator: the bail-out +// jump emitted for the disable targeted the outer fork branch and crossed a +// coroutine boundary once the nested forks were split into separate functions. +// Even disregarding the crash, processes of nested sub-forks were not registered +// for the kill, so a nested sibling survived and its $stop fired. +// +// The disable must also let the process that owns the disabled fork's 'join' +// resume after the join (IEEE 1800-2023 9.6.2): once the block is terminated, +// execution continues at the point after it. The *_resumed bits below latch that +// the parent ran past its join; a hang there leaves the bit clear and is caught +// at the end. A fork branch that itself holds a nested fork is killed while +// suspended at the inner join, so it must register a kill hook on its parent +// fork or the parent join counter is never decremented and the parent hangs. + +// verilog_format: off +`define stop $stop +`define checkt(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0t exp=%0t (%t !== %t)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +module t; + + bit d2_resumed = 1'b0; + bit d3_resumed = 1'b0; + bit d2a_resumed = 1'b0; + bit d2r_resumed = 1'b0; + bit ja1_resumed = 1'b0; + int prefix_queue[$]; + + // An ordinary queue push at the start of a fork branch must not be mistaken + // for compiler-generated disable queue registration. + initial begin + fork + begin + prefix_queue.push_back(1); + end + join + end + + // fork..join nested two levels: disabling the outer named fork from inside the + // inner fork must kill the inner delayed sibling and the outer delayed sibling. + initial begin + fork : fork_d2 + begin + fork + begin + `checkt($time, 0); + disable fork_d2; + $stop; + end + begin + `checkt($time, 0); + #1 $stop; + end + join + end + begin + `checkt($time, 0); + #1 $stop; + end + join + `checkt($time, 0); + d2_resumed = 1'b1; + end + + // fork..join nested three levels: disabling the outermost named fork from the + // innermost branch must kill the delayed siblings at every level. + initial begin + fork : fork_d3 + begin + fork + begin + fork + begin + `checkt($time, 0); + disable fork_d3; + $stop; + end + begin + `checkt($time, 0); + #1 $stop; + end + join + end + begin + `checkt($time, 0); + #1 $stop; + end + join + end + begin + `checkt($time, 0); + #1 $stop; + end + join + `checkt($time, 0); + d3_resumed = 1'b1; + end + + // fork..join_any nested two levels. + initial begin + fork : fork_d2a + begin + fork + begin + `checkt($time, 0); + disable fork_d2a; + $stop; + end + begin + `checkt($time, 0); + #1 $stop; + end + join_any + end + begin + `checkt($time, 0); + #1 $stop; + end + join_any + `checkt($time, 0); + d2a_resumed = 1'b1; + end + + // fork..join_none nested two levels. + initial begin + fork : fork_d2n + begin + fork + begin + `checkt($time, 0); + disable fork_d2n; + $stop; + end + begin + `checkt($time, 0); + #1 $stop; + end + join_none + end + begin + `checkt($time, 0); + #1 $stop; + end + join_none + end + + // fork..join nested two levels, but the disabling branch comes after the + // delayed sibling (in source order) at both levels. + initial begin + fork : fork_d2r + begin + `checkt($time, 0); + #1 $stop; + end + begin + fork + begin + `checkt($time, 0); + #1 $stop; + end + begin + `checkt($time, 0); + disable fork_d2r; + $stop; + end + join + end + join + `checkt($time, 0); + d2r_resumed = 1'b1; + end + + // fork..join_any whose only branch holds the nested fork that gets disabled. + // join_any needs one branch to complete, but the only branch is killed while + // suspended at its inner join, so the parent resumes only if that branch + // decrements the join counter on kill. + initial begin + fork : fork_ja1 + begin + fork + begin + `checkt($time, 0); + disable fork_ja1; + $stop; + end + begin + `checkt($time, 0); + #1 $stop; + end + join + end + join_any + `checkt($time, 0); + ja1_resumed = 1'b1; + end + + initial begin + #10; + `checkd(d2_resumed, 1'b1); + `checkd(d3_resumed, 1'b1); + `checkd(d2a_resumed, 1'b1); + `checkd(d2r_resumed, 1'b1); + `checkd(ja1_resumed, 1'b1); + `checkd(prefix_queue.size(), 1); + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule