fix(verilog): break and continue were no-ops; lower them to guard flags - #3014
Merged
Conversation
The Verilog emitter wrote `disable fork;` for `break` and `/* continue */;` for `continue`. `disable fork` kills processes spawned by a `fork` in the current scope, and the token `fork` occurs nowhere in the generated corpus except inside that very line -- so all of them were no-ops: the loop ran to completion and later iterations overwrote whatever the jump was meant to preserve. Both lowerings parse. iverilog accepts them, yosys accepts them, the seal hashes are stable over them. Every instrument in this repository asked whether the output PARSES, so the defect was invisible for as long as the emitter has existed. Population, over every .t27 in the tree and not just specs/: 17 jump sites in 8 files -- 16 break, 1 continue. Two of them live in compiler/cli/gen.t27, outside specs/, which is why a specs-scoped census reports 15 in 7. The lowering is a flag per loop that needs one, allocated only when the body actually contains the jump: `reg __t27_brk_N` persists and joins the loop CONDITION, `reg __t27_cnt_N` is cleared at the top of each iteration and gates only the iteration tail. The scan for a jump stops at a nested loop, so an inner break binds to the inner loop. Measured: * a probe of 8 declared tests, expected values confirmed by the Zig backend (8/8) -- an oracle independent of the Verilog emitter. Under icarus-simulate: 1 of 8 before, 8 of 8 after; the one that passed before is the jump-free control * 581 of 650 specs generate Verilog, before and after * 7 of 581 generated files differ * yosys read_verilog -sv -DSIMULATION + hierarchy on those 7: +2 (specs/ar/asp_solver, specs/compiler/lexer go FAIL -> PASS), 0 lost. The other 574 are byte-identical, so their verdicts cannot move * iverilog: unchanged on all 7, so the corpus acceptance columns do not move * mutation: 6 of 6 killed. The sixth binds break to the OUTERMOST enclosing loop and survived five tests plus a sixth that COUNTED declarations -- only asserting that the flag a break sets is the flag its loop declared kills it `return` inside a loop is the other half of the defect and is deliberately not here: __t27_ret already exists and is set correctly, and what is missing is that no loop tests it. That repair moves loops containing no jump at all, so it gets its own measurement -- #2989. Adds `tri jumps census`: names every break/continue site in the generated Verilog and says what it lowered to -- a flag, a no-op, or a refusal -- and asserts the pairing a count cannot see. Closes #2988 Refs #2989 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .claude/skills/ci-gates/SKILL.md
Owner
Author
|
Renumbered to Worth recording as a footnote to 439 itself. My resolver did |
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
…oved `coverage` was red on the first CI round: 7 generated Verilog files change, and this corpus twins its seals, so that is 19 `.trinity/seals/*.json`. Re-sealing is a statement that the new output is the one we want, so the acceptance reading first, on an idle machine, whole corpus, both binaries: generates Verilog 581 -> 581 Zig AND Verilog accept 258 -> 258 ALL FOUR accept 184 -> 184 Identical to the digit. That is expected and was predicted before the run: `iverilog` was measured directly on each of the 7 changed files and its verdict does not move on any of them, and the other 574 files are byte-identical, so no acceptance column can move. `tri seals drift` reads 0 after the fix; it read 19 before. ci-gates 441 gains the part that was missing: I had the 7-file delta and still shipped without a reseal, because I never converted it into the unit the seal gate counts in. Refs #2988 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
PR DashboardGenerated at: 2026-09-03 10:14:55 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
# Conflicts: # .claude/skills/ci-gates/SKILL.md
Contributor
PR DashboardGenerated at: 2026-09-03 10:30:23 UTC
Summary
Seal Status
|
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Owner
Author
|
Renumbered again: Resolved the same way as the first: rebuilt from the two authoritative sides ( The convention question this raises is a decision, not a fix, and it is on the dashboard: assign section numbers at merge rather than at writing, or accept the collisions. It is cheap for an append-only skill file and it will not be cheap for |
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
breakwasdisable fork;, and the corpus has noforkThe Verilog emitter wrote
disable fork;forbreakand/* continue */;forcontinue.disable forkkills processes spawned by aforkin the current scope, and the tokenforkoccurs nowhere in the generated corpus except inside that very line. All of them were no-ops: the loop ran to completion, and later iterations overwrote whatever the jump was meant to preserve.Both lowerings parse.
iverilog -g2012accepts them,yosysaccepts them, the seal hashes are stable over them. Every instrument this repository owns asked whether the output parses, so this was invisible for as long as the emitter has existed.Population
Over every
.t27in the tree, not justspecs/:break->disable fork;continue->/* continue */;Two of them are in
compiler/cli/gen.t27, outsidespecs/. A census scoped tospecs/reports 15 in 7 -- which is the number I published, twice.Measured
icarus-simulate: 1 of 8 before, 8 of 8 after. The one that passed before is the jump-free controlyosys read_verilog -sv -DSIMULATION+hierarchyon those 7: +2 (specs/ar/asp_solver,specs/compiler/lexergo FAIL -> PASS), 0 lost. The other 574 are byte-identical, so their verdicts cannot moveiverilog: unchanged on all 7 (all seven already fail for other reasons), so the corpus acceptance columns do not movebreakto the OUTERMOST enclosing loop; it survived five tests and a sixth that counted declarations, because the count is still one flag per loop. Only asserting that the flag abreaksets is the flag its loop declared kills itThe lowering
One flag per loop that needs one, allocated only when the body actually contains the jump -- which is why the delta is 7 files and not 581.
reg __t27_brk_Npersists and joins the loop conditionreg __t27_cnt_Nis cleared at the top of each iteration and gates only the iteration tailbreakbinds to the inner loopdisablewas the other candidate and is not this: it cannot expresscontinuewithout a block per iteration, anddisable <named block>is a parse error underyosysin all three modes this repo usesScope
returninside a loop is the other half and is not here.__t27_retalready exists and is set correctly; what is missing is that no loop tests it. That repair moves loops containing no jump at all, so it gets its own measurement -- #2989.Instrument
tri jumps censusnames everybreak/continuesite in the generated Verilog and says what it lowered to.It also asserts the pairing -- every flag a loop declares must be written somewhere -- because a count of declarations cannot see a jump bound to the wrong loop.
Skill
ci-gates441-443: a marker is only yours if the base count is zero (I read 535 pre-existing// NOT LOWERED BY THIS BACKENDlines as mine); a two-part construct needs its halves asserted as a pair; and when only 7 files' bytes moved, the expensive ruler only needs those 7 -- provided you say that is what you did.Closes #2988
Refs #2989