fix(verilog): a range is not a bound -- 36 -> 0 in simulation, 5 -> 0 synthesizable - #3005
Merged
Merged
Conversation
`gen_verilog_for_stmt` wrote the whole iterable where the loop's comparison
belongs, so `for (0..1000) |_| { … }` became
for (i = 0; i < (0 .. 1000); i = i + 1) begin
and iverilog answers `syntax error`. Measured by regenerating the corpus with
the stock compiler and with this one, counting files whose `for (` line carries
a `..`: 36 -> 0 in the simulation path, 5 -> 0 synthesizable. 581 specs generate
either way; `iverilog -g2012` accepts 380 before and 380 after; 5 synthesizable
and 129 simulation files change, and their seals are re-sealed here.
#2849 fixed exactly this in the C emitter and wrote the trap down in its own
comment -- the range is an `ExprBinary` whose `extra_op` is "..", NOT the
`ExprRange` variant, which is declared in NodeKind and constructed nowhere. The
repair did not travel. Rust tests the same shape; Zig writes the range verbatim
and is correct, because Zig has ranges.
Two things worth keeping from getting it wrong first:
* The first version renamed a `_` capture to `__t27_i`, copying C. C declares
its counter in the `for` header; here the declaration is hoisted by
`collect_fn_loop_vars`, so the rename produced
`register `__t27_i' unknown in …`. The probe caught it; the corpus could not
have, because all 36 carriers already failed to elaborate on the very defect
being repaired. The rename is gone.
* The guard has three clauses and the first test suite reached two. `for (data)
|x|` iterates an identifier with zero children, so a mutant keeping only
`children.len() == 2` left it untouched. `for (lo + hi) |x|` is the two-child
non-range that separates them; with it, mutation is 4 of 4.
Out of scope and stated in the issue: the same literal reaches SLICE positions,
`expr[(0 .. idx)]`, 15 sites in 7 files, unchanged. A range in an index is a
part-select and a different repair.
Closes #2997
Refs #2849, #2998
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-09-03 07:30:23 UTC
Summary
Seal Status
|
gHashTag
added a commit
that referenced
this pull request
Sep 3, 2026
…and a verification is dated (#3012) 434. Section 428 named `$?`. The trap fired again through `&&`: git apply --check "$patch" | head -3 && echo " APPLIES" printed APPLIES for two patches that do not apply, because `head` succeeded. The real answer -- `error: bootstrap/stage0/FROZEN_HASH: patch does not apply` -- went past on the piped line. `$?` was never the subject: `&&`, `||`, `if`, `while`, `until` and `set -e` all read the LAST command of a pipeline. A command whose exit code you care about does not go in a pipeline. 435. I published that two externally-authored patches apply cleanly and their FROZEN_HASH is valid, hedged correctly that any edit to compiler.rs would end that, and then ended it myself four hours later by merging #3005 -- which rewrote the seal from fd842146… to 1b52250f…. A hedge moves the work to a reader who may never come. Name the commit rather than the branch, and when you merge something touching the same files, go back and say so: you are the one person holding both facts at once. The tell was a review agent reporting that the brief's premise -- "compiler.rs is byte-identical between X and master" -- had expired during its own run. `tri skill check`: 399 sections, no number used twice. Refs #2988, #3005
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.
gen_verilog_for_stmtwrote the whole iterable where the loop's comparisonbelongs:
iveriloganswerssyntax error. Source isfor (0..1000) |_| { … }.Measured, by regenerating the corpus with each binary
Counting files whose
for (line carries a..— the exact defect, not a loosegrep for the characters:
iverilog -g2012acceptsAcceptance does not move, and that is expected: a syntax error in a file that
already failed elaboration for other reasons does not flip it. Seals for the
changed files are re-sealed in this commit, with that reading as the reason.
The repair did not travel, and the trap was already written down
#2849 fixed exactly this in the C emitter and left the reason in its own comment:
the range is an
ExprBinarywhoseextra_opis"..", not theExprRangevariant, which is declared in
NodeKindand constructed nowhere. Rust tests thesame shape. Zig writes the range verbatim and is correct, because Zig has ranges.
Verilog was the one site that never got it.
Two things worth keeping from getting it wrong first
The rename that broke it. My first version also renamed a
_capture to__t27_i, copying C. C declares its counter in theforheader; here thedeclaration is hoisted by
collect_fn_loop_vars, so the rename producedregister__t27_i' unknown in …`. The probe caught it and the corpus couldnot have — all 36 carriers already failed to elaborate on the very defect
being repaired, so no acceptance number could move in either direction. The
rename is gone; the counter is whatever was declared, and a test now asserts
exactly that.
The guard has three clauses and the first suite reached two.
for (data) |x|iterates an identifier with zero children, so a mutant keeping only
children.len() == 2left it untouched and survived.for (lo + hi) |x|is thetwo-child binary that is not a range; with it, mutation is 4 of 4 —
never-a-range, always-a-range, start/end swapped, start forced to zero.
Honesty
t27c icarus-simulateand PASS. Before this change the probe did notelaborate at all.
iverilogexists rather than matching aphrase in whatever the failure printed (ci-gates 426), and skips loudly.
rustfmt --checkoncompiler.rs: 315 complaints on master and 315 here.The new test file is clean.
for (i = 0; i < (0 .. 1000); …)-- 38 specs, and 10 in synthesizable RTL #2997 says so: the same literal reaches slice positions —expr[(0 .. idx)],line[(SSE_DATA_PREFIX_len .. line_len)]— 15 sites in7 files, unchanged. A range in an index is a part-select and a different
repair.
for (x = 0; x < data; x = x + 1), comparing an index against an array. Thatis pre-existing and this change deliberately does not touch it.
Closes #2997
Refs #2849, #2998
🤖 Generated with Claude Code