diff --git a/AGENTS.md b/AGENTS.md index 17ede4bfb..013026920 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,97 +2,127 @@ SPDX-FileCopyrightText: 2026-2026 Wilson Snyder SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 --> -# Verilator Coding Guidelines +# Verilator Guidelines for AI Coding Agents -When to check: read this file before making any change in this repository, then read the directory guide for the area being edited. +This file has two parts. **Orientation** gets you productive in the codebase from +a cold start. **Before you open a PR** is the checklist of conventions reviewers +otherwise have to enforce by hand -- read it before submitting any change. -## Topic guides +Then read the directory guide for the area you are editing: -- [src/AGENTS.md](src/AGENTS.md) -- compiler C++ sources: style, AST, visitors, parser, file-specific rules -- [test_regress/AGENTS.md](test_regress/AGENTS.md) -- regression tests: naming, drivers, golden files, Verilog style -- [docs/AGENTS.md](docs/AGENTS.md) -- documentation (`*.rst`) writing rules -- `docs/internals.rst` -- architecture, AST, and pass overview -- `docs/CONTRIBUTING.rst` -- contribution process +- [src/AGENTS.md](src/AGENTS.md) -- compiler C++ sources: AST, visitors, passes, parser, style +- [test_regress/AGENTS.md](test_regress/AGENTS.md) -- regression tests: harness, drivers, golden files +- [docs/AGENTS.md](docs/AGENTS.md) -- documentation (`*.rst`) -## Build and test +--- -- Build in the source tree: `autoconf && ./configure && make -j8`; developers should configure with `--enable-ccwarn` to stop the build on new warnings. +# Orientation -- Run a single test from the repository root: `test_regress/t/t_.py`; run the full regression with `make test`. +## What Verilator is -## Error classification +Verilator is a *compiler*, not an interpreter. It translates synthesizable (and +much behavioral) SystemVerilog into a cycle-accurate C++ model that you then +compile and run. Almost every decision is made at compile ("verilation") time; +the generated C++ just advances state each evaluation. Optimize for verilation- +time work over runtime work. -Choose the diagnostic API by what went wrong; the choice determines which test must accompany the change. +## The pipeline is the spine + +A run is an ordered sequence of passes over one shared AST (abstract syntax +tree). In source order: + +| Stage | What it does | Key files | +|---|---|---| +| Preprocess + parse | Lex and parse text into a raw AST -- builds nodes only, no semantic checks | `verilog.l`, `verilog.y` | +| Link / elaborate | Resolve names, scopes, parameters; instantiate the hierarchy | `V3LinkParse`, `V3LinkDot`, `V3Param` | +| Width / type | Assign and check data types and bit widths | `V3Width` | +| Transform / optimize / schedule | Constant fold, lower language features, schedule events | `V3Const`, `V3Randomize`, `V3Assert*`, `V3Sched`, `V3Timing`, `V3Dfg` | +| Emit | Lower the final AST to generated C++ | `V3EmitC*` | +| Runtime | Library the generated model links against | `include/verilated*` | + +`docs/internals.rst` is the authoritative architecture and pass reference. Read +it before any non-trivial change. + +## Where to make a change + +Map the symptom to the pass that owns it, then start by reading that pass's +top-of-file comment. + +| Symptom or feature area | Start in | +|---|---| +| Type/width error, "what type is this", implicit conversion | `V3Width` | +| Name/scope/parameter resolution ("Can't find...", hierarchy) | `V3LinkDot`, `V3Param` | +| `randomize` / `constraint` / `rand` / `randc` | `V3Randomize` | +| `assert` / `property` / `sequence` / `cover` | `V3Assert`, `V3AssertPre`, `V3AssertNfa` | +| `fork` / timing / `#delay` / NBA / event scheduling | `V3Sched`, `V3Timing`, `V3Fork` | +| Syntax wrongly accepted or rejected | `verilog.y`, `verilog.l` | +| Wrong generated C++ | `V3EmitC*` | +| Runtime model behavior | `include/verilated*` | + +## Build and run a test + +- Build in the source tree: `autoconf && ./configure && make -j8`. Configure with + `--enable-ccwarn` so a new compiler warning stops the build. +- Run one test from the repository root: `test_regress/t/t_.py`. +- Run the full regression: `make test`. + +--- + +# Before you open a PR + +## Scope and process + +- [ ] Searched open PRs and issues -- duplicating in-flight work wastes review time. +- [ ] Fixed the general root cause, not just the reported case -- if it also + affects other modules/classes/interfaces, cover them or expect rejection. +- [ ] PR is single-purpose. Refactors, drive-by fixes found along the way, and new + features each go in separate PRs; land standalone cleanups first. +- [ ] Every bug fix has a test that fails *without* the fix; include the issue's + own reproducer when possible. +- [ ] New code aims for 100% line coverage; branch coverage far below line coverage + signals guards callers never violate -- justify or remove them. +- [ ] Ran `make format` (clang-format) and `make cppcheck`; self-reviewed the diff + for leftover debug code, stale comments, and copy-paste errors. +- [ ] Did not edit `docs/CONTRIBUTORS` (humans only) or `Changes` (maintainer + updates it near release). + +## Pick the right diagnostic (and its required test) + +The API you choose determines which test must accompany the change. | API | Output | Meaning | Required test | |---|---|---|---| -| `v3error("...")` | `%Error:` | User wrote invalid SystemVerilog (IEEE violation) | `t_*_bad*.v` + `.out` golden | -| `v3error("Unsupported: ...")` | `%Error-UNSUPPORTED:` | Legal SystemVerilog that Verilator does not yet support | `t_*_unsup*.v` + `.out` golden | +| `v3error("...")` | `%Error:` | User wrote invalid SystemVerilog | `t_*_bad*.v` + `.out` golden | +| `v3error("Unsupported: ...")` | `%Error-UNSUPPORTED:` | Legal SV that Verilator does not yet support | `t_*_unsup*.v` + `.out` golden | | `v3warn(CODE, "...")` | `%Warning-CODE:` | Legal but suspicious code | warning test + `.out` golden | | `v3fatalSrc("...")` | `%Error: Internal Error` | Should-never-happen internal assertion | none -- not user-triggerable | -- Include the IEEE reference in errors about spec-defined restrictions: `(IEEE 1800-2023 XX.X)` -- makes the restriction verifiable. - -- Every `v3error`/`v3warn` message needs a test in `test_regress/t/` -- enforced by the warn-coverage distribution test; `v3fatalSrc` is exempt. - -- Reserve "Unsupported:" for not-yet-implemented features, never for user mistakes; when partially implementing a feature, keep errors on the still-unsupported sub-features. - -- On error paths, clean up or replace invalid AST (e.g. with `AstConst::BitFalse`) so later passes do not crash after the error. - -- Update `docs/guide/warnings.rst` when adding or changing warnings -- documentation for every warning is enforced by a distribution test. - -## Process - -- Search existing open pull requests and issues for overlapping work before starting -- duplicating an open PR wastes both author and reviewer time. - -- Cite IEEE 1800-2023 with section numbers in error messages, code comments, and PR text -- verify against the actual standard text, not recalled knowledge. - -- Do not edit `docs/CONTRIBUTORS` -- only humans may edit it; remind the user to read and agree to the statement in it. - -- Do not edit the `Changes` file unless explicitly asked -- the maintainer updates it near release time; contributor edits cause merge conflicts. - -- Fix the general case, not just the reported case -- if the root cause also affects modules/classes/interfaces beyond the trigger scenario, cover them or expect the fix to be rejected. - -- Keep changes single-purpose: refactoring, drive-by bug fixes found along the way, and new features belong in separate PRs; land standalone cleanups first. - -- Split large features into a chain of small, independently mergeable PRs, each adding one dimension of complexity -- keeps review scope bounded. - -- Do not expand scope opportunistically -- even an IEEE-justified diagnostic tightening reclassifies user-facing behavior, re-goldens existing tests, and widens the review blast radius; file it as its own PR. - -- When changing a warning's or error's classification, update the warning documentation and regression expectations in the same change -- diagnostic class and suppressibility are part of the user-facing contract. - -- Very large PRs include an explicit risk summary: what semantics changed, what stayed invariant, and what validation proves safety -- review bandwidth drops sharply as changed-file count grows. - -- Build, export, or reporting features must show that a downstream consumer can actually parse the produced output -- format-only checks miss consumer-visible breakage. - -- After a framework PR lands, plan a follow-up that simplifies and unifies what review revealed -- a negative line count in the follow-up is a sign of maturity, not failure. - -- Every bug fix includes a test that fails without the fix; include the issue's own reproducer as a regression test when possible. - -- Aim for 100% line coverage on new code and justify or remove uncovered branches -- branch coverage markedly below line coverage signals guards that callers never violate; request a CI coverage report and check it. - -- Run `make format` (clang-format) and `make cppcheck` before pushing; self-review the diff for leftover debug code, stale comments, and copy-paste errors. - -- Apply new code patterns globally or not at all -- do not introduce a one-off convention in a single file. - -## Commits - -- Commit messages are one short line referencing issue and PR numbers: `Short description (#issue) (#pr)`. - -- Do not add Co-Authored-By or any AI attribution to commits. +- Every `v3error`/`v3warn` needs a test in `test_regress/t/` -- enforced by the + warn-coverage distribution test. `v3fatalSrc` is exempt. +- Reserve "Unsupported:" for not-yet-implemented features, never for user mistakes. +- When an error enforces a spec-defined restriction, cite the clause + (`IEEE 1800-2023 11.4.7`) so it is verifiable. Update `docs/guide/warnings.rst` + when adding or changing a warning. +- On error paths, clean up or replace invalid AST (e.g. `AstConst::BitFalse`) so + later passes do not crash after the error. ## Cross-cutting code rules -- No non-ASCII characters in C++ sources or headers -- use `--` not em dashes and plain `'` not smart quotes, at write time, not just when CI complains. +- [ ] No non-ASCII characters in C++ sources or headers -- `--` not em dashes, + plain `'` not smart quotes. At write time, not when CI complains. +- [ ] Lists stay sorted: lexer/parser tokens, option declarations, enum values, + configure feature lists, documented option lists. +- [ ] `bin/` scripts are Python (distributed cross-platform); `nodist/` may use + bash and platform-specific code (developer-only, not packaged). +- [ ] Runtime code in `include/` targets C++14 (`--no-timing` builds must work); + C++20 only in timing code paths. +- [ ] In `include/` public headers, prefix public classes with `Verilated`/`Vl` + and document the API with `///` comments. +- [ ] A new code pattern is applied globally or not at all -- no one-off + convention in a single file. -- Keep lists sorted: lexer/parser tokens, option declarations, enum values, configure feature lists, documented option lists. +## Commits -- `bin/` scripts must be Python -- they are distributed cross-platform; `nodist/` may use bash and platform-specific code (developer-only, not packaged). - -- Runtime code in `include/` targets C++14 (`--no-timing` builds must work); C++20 features are allowed only in timing code paths. - -- In `include/` public headers, prefix public classes and types with `Verilated`/`Vl` and use `///` comments for API documentation -- prevents collisions with user code and feeds doc generation. - -- Apply the same const, naming, and portability discipline in infrastructure and test code as in core passes -- review standards are not relaxed for helper code. - -- Keep deprecated command-line options active with warning-only handlers (`v3warn(DEPRECATED, ...)`) -- removing an option outright breaks existing user scripts. +- Subject line is short and imperative and conventionally ends with the PR number: + `Support property case (#7721)`. A body is optional and common for non-trivial + changes. diff --git a/src/AGENTS.md b/src/AGENTS.md index f5f5cec94..08a9ce964 100644 --- a/src/AGENTS.md +++ b/src/AGENTS.md @@ -2,76 +2,84 @@ SPDX-FileCopyrightText: 2026-2026 Wilson Snyder SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 --> -# src/ Coding Guidelines -- Verilator compiler sources +# src/ -- Verilator compiler sources -When to check: before editing any C++ source or header under `src/`, including astgen inputs and the parser/lexer (`verilog.y`, `verilog.l`). -Read the repository-root [AGENTS.md](../AGENTS.md) first for process and cross-cutting rules. +Covers all C++ under `src/`, including astgen inputs and the parser/lexer +(`verilog.y`, `verilog.l`). Read the repository-root [AGENTS.md](../AGENTS.md) +first. This file has two parts: **Orientation** explains the AST and pass model; +**Before you open a PR** is the style and correctness checklist. + +--- + +# Orientation: the AST and the visitor model + +- **Everything is an `AstNode`.** Each construct is an `Ast*` subclass (`AstAdd`, + `AstVar`, `AstIf`). The design under analysis is one tree, with statement lists + threaded by sibling `nextp()` links. +- **Children sit in numbered slots `op1p()`..`op4p()`, accessed by name.** Use the + named accessors (`lhsp()`, `condp()`, `thensp()`), not the raw slots -- the + numbering is an implementation detail. +- **`astgen` generates node boilerplate.** Declare children and cross-node + pointers with `@astgen op` / `@astgen ptr` annotations in the `V3AstNode*.h` + headers; it emits accessors, clone, and broken-check code. Do not hand-write + what astgen can generate. +- **A pass is a visitor.** Convention: a class with a private constructor and a + static `apply()` entry point, named after its file (`TimingSuspendableVisitor` + in `V3Timing.cpp`). It walks the tree through `visit(AstFoo*)` handlers and + `iterateChildren()`. To understand a pass, read its top-of-file comment first -- + every `.cpp` opens with one describing the algorithm. +- **Scratch state lives on nodes.** Passes stash data in `user1p()`..`user5p()` + (and `user1()`..`user5()`), claimed for the pass lifetime with a `VNUser*InUse` + guard. Save and restore visitor members across recursion with `VL_RESTORER`. +- **Three downcasts, three null behaviors:** `VN_IS` returns bool, `VN_CAST` + returns nullptr on mismatch, `VN_AS` asserts the type. `V3Broken` re-validates + tree invariants between passes, so trust resolved pointers (`dtypep()`, + `varp()`) instead of adding defensive null checks for impossible cases. +- `docs/internals.rst` is the authoritative reference for the AST, the pass list, + and node lifetime. + +--- + +# Before you open a PR ## Code style -- Mark every variable, parameter, and pointer `const` where possible -- one missing `const` found in review triggers a rescan of the whole change. - -- Pointer variables take a `p` suffix and pointer locals are doubly const where possible: `AstVar* const varp` -- non-pointers must not use the `p` suffix; names describe content, not type. - -- Do not use `auto` except for iterators or genuinely unwieldy types -- explicit types aid comprehension. - -- Use pre-increment (`++i`), never post-increment -- faster for non-trivial types, always correct. - -- Use brace initialization for all node and struct construction: `new AstIf{fl, condp, thenp, elsep}` -- not parentheses. - -- No C-style casts -- use `static_cast` for non-AST types and `VN_AS`/`VN_CAST` for AST downcasts. - -- Use `static constexpr` for compile-time constants -- not `#define` or file-scope `const`. - -- Mark every `class`/`struct` `final` or `VL_NOT_FINAL` -- including nested and private helper structs; a distribution test scans all definitions. - -- Keep functions under roughly 100-150 lines -- split larger ones into helpers with clear contracts; thread shared state through a context struct rather than long parameter lists. - -- Keep headers lean: move implementation to `.cpp` files; convert large lambdas into named member functions or file-local `static` functions -- lambdas are opaque in stack traces and block reuse. - -- Start every new `.cpp` file with a top-of-file comment explaining the algorithm. - -- Use `//` line comments; write comments as capitalized sentences without "I"/"we"/"our" -- write for an unknown future reader. - -- Use section markers in class declarations: `// MEMBERS`, `// CONSTRUCTORS`, `// METHODS`. - -- Categorize visitor state comments: "STATE - across all visitors", "STATE - Statistic tracking", "STATE - for current visit position (use VL_RESTORER)". - -- Start continuation lines with the operator (`&&` at line start, not line end). - -- No `using namespace` -- expand types where used; prefix non-namespaced symbols with `VL`/`Vl`. - -- Prefer semantic predicates over enum comparisons: `varp->isClassMember()`, not `varp->varType() == VVarType::MEMBER` -- survives enum changes and reads as intent. - -- Getter is the member name minus `m_`; setter is the same name taking an argument; a setter that only ever sets true uses a `set` prefix with no argument. - -- `new*` functions return a `new` object; `make*` functions do something more complex -- pick the prefix accordingly. - -- Use `T_`/`N_` prefixes for template type/value parameters (`T_Func`, `N_Depth`) -- distinguishes them from concrete types. - -- Name compiler-created temporaries with a `__V` prefix plus a context suffix (`__VInside`, `__VCase`), no redundant "Tmp"; use a `vl_` prefix for runtime utility functions. - -- Use `VL_*` macros from `verilatedos.h` (`VL_WORDS_I`, `VL_MASK_I`, `VL_BIT`) for bit/word math -- no hand-rolled shifts; do not include `` directly, `verilatedos.h` provides it. - -- Use the `"text"s` literal suffix for `std::string` concatenation chains -- cleaner than multi-line `+=` sequences. - -- Wrap multi-statement macros in `do { ... } while (0)` -- single-statement behavior in all control-flow contexts. - -- Convert pointers to integers via `uintptr_t`: `static_cast(reinterpret_cast(p))` -- portable across 32/64-bit platforms. - -- Preserve `splitCheck()` in emit code paths for functions that can grow arbitrarily large -- a build safeguard against oversized generated code; refactor around it, never remove it. - -- Extract a shared helper when two flows build the same logical structure -- parallel implementations drift and create omission bugs. - -- Remove commented-out code -- version control preserves history. - -- Replace magic numbers with named `static constexpr` constants for thresholds, limits, and weights -- self-documenting and centrally tunable. +- Mark every variable, parameter, and pointer `const` where possible. +- Pointer variables take a `p` suffix and pointer locals are doubly const where + possible: `AstVar* const varp`; non-pointers never use the `p` suffix. +- Do not use `auto` except for iterators or genuinely unwieldy types. +- Use pre-increment (`++i`), never post-increment. +- Brace-initialize node and struct construction: `new AstIf{fl, condp, thenp, elsep}`. +- No C-style casts -- `static_cast` for non-AST types, `VN_AS`/`VN_CAST` for + AST downcasts. +- `static constexpr` for compile-time constants, not `#define` or file-scope const. +- Mark every `class`/`struct` `final` or `VL_NOT_FINAL` -- a distribution test + scans all definitions. +- Keep functions under roughly 100-150 lines; thread shared state through a + context struct rather than long parameter lists. +- Keep headers lean: move implementation to `.cpp`; convert large lambdas into + named member functions -- lambdas are opaque in stack traces and block reuse. +- Start every new `.cpp` with a top-of-file comment explaining the algorithm. +- Comments are capitalized sentences written for an unknown future reader, without + "I"/"we"/"our"; remove commented-out code -- version control preserves history. +- Start continuation lines with the operator (`&&` at line start). +- No `using namespace`; prefix non-namespaced symbols with `VL`/`Vl`. +- Prefer semantic predicates over enum comparisons: `varp->isClassMember()`, not + `varp->varType() == VVarType::MEMBER`. +- `new*` functions return a `new` object; `make*` functions do something more + complex -- pick the prefix accordingly. +- Name compiler-created temporaries with a `__V` prefix plus a context suffix + (`__VInside`, `__VCase`); runtime utility functions use a `vl_` prefix. +- Use `VL_*` bit/word macros from `verilatedos.h` (`VL_WORDS_I`, `VL_MASK_I`); do + not include `` directly. +- Replace magic numbers with named `static constexpr` constants. ## AST construction and manipulation -- Build logic as AST nodes, never as raw C text in `AstCStmt` -- later passes (V3Name, `--protect`) rename AST identifiers but cannot see into raw strings. - -- Know the three cast forms and their null-safety: `VN_IS` returns false for nullptr, `VN_CAST` returns nullptr on mismatch, `VN_AS` asserts the type. Never pair `VN_IS` with `VN_AS` on the same node -- use a single `VN_CAST`: +- Build logic as AST nodes, never as raw C text in `AstCStmt` -- later passes + (V3Name, `--protect`) rename AST identifiers but cannot see into raw strings. +- Know the cast forms (above) and never pair `VN_IS` with `VN_AS` on the same + node -- use a single `VN_CAST`: ```cpp // BAD: redundant double check @@ -80,218 +88,139 @@ Read the repository-root [AGENTS.md](../AGENTS.md) first for process and cross-c if (const AstVarRef* const refp = VN_CAST(nodep, VarRef)) { ... } ``` -- Trust tree invariants: `dtypep()`, `subDTypep()`, `varp()` results are never null after the resolving pass -- defensive null checks for impossible cases hide bugs and create uncoverable code; V3Broken validates the tree. - -- Use `UASSERT_OBJ(cond, nodep, "...")` over `UASSERT` whenever a node is in scope -- the node supplies the error location; pass the condition directly, never `UASSERT_OBJ(false, ...)` inside an `if`. - -- Use `v3fatalSrc("...")` for unreachable code paths -- never a silent `if (!ptr) return;`. - -- Use `VL_DO_DANGLING(pushDeletep(nodep), nodep)` instead of `deleteTree()` in visitors -- deferred deletion is safe against re-entry and unlinking order, and keeps nodes visible for debugging; `deleteTree()` only for fresh nodes that never entered the tree. - -- Every new AST member variable needs both `dump()` and `dumpJson()` support -- never wrap these in `LCOV_EXCL`; cover them naturally by adding a construct exercising the node to the tree-dump test (`test_regress/t/t_debug_emitv.v`), which dumps every node at every stage. - -- Override `isSame()` to include any new semantically meaningful field added to a node. - -- Let astgen generate child accessors via `@astgen op` annotations -- do not write manual accessors; use `@astgen ptr` (with `Optional[_]` for nullable) for cross-node pointers instead of hand-coded `brokenGen`/`cloneRelinkGen`; make generated virtual methods abstract (`= 0`), not empty defaults. - -- Pointers to nodes outside op1p-op4p require a `broken()` override and `cloneRelink()` support -- avoid storing out-of-tree node pointers when possible. - -- When caching derived data on AST nodes, add a `broken()` check that recomputes and verifies the cache. - -- Never allocate AstNode objects on the stack or by value -- always pointers; leak checking complains otherwise. - -- `AstSelBit` exists only during parse -- later passes use `AstArraySel`/`AstSel`. - -- Use `nodep->foreach(...)` for simple traversals and named accessors (`lhsp()`, `fromp()`) over `op1p()`..`op4p()` -- but verify traversal order is preserved when converting manual iteration. - -- `addNext()` walks to the list tail itself -- do not pre-walk `nextp()` chains before calling it. - -- Prefer `AstForeach` over generating unrolled loop bodies for array iteration -- constant-size generated code instead of O(N), uniform across array types; wrap the body in `AstBegin` for scope isolation. - -- Always `skipRefp()` when comparing or resolving dtypes -- missing it breaks typedef support; prove the paths with typedef tests. - -- Use `num().isOpaque()` rather than spelling out `isDouble() || isString()` when guarding V3Number comparisons against non-integer types. - -- Use `FileLine::operatorCompare` for source-position ordering -- never hand-roll filename/lineno comparisons; it covers file, line, column, last-line, and last-column. - -- Prefer a virtual `isSomething()` predicate on the base node class over long `VN_IS(x, A) || VN_IS(x, B) || ...` chains at call sites -- a single place to extend when subclasses are added. - -- Never identify compiler-generated constructs by name-pattern matching -- add an attribute flag to the node (with dump/dumpJson support); magic-name matching breaks with escaped identifiers. - -- Use `V3Number` arithmetic for `AstConst` values wider than 32 bits -- `1 << i` silently overflows at `i >= 32`. - -- Use `VMemberMap`/`findMember()` for name lookups in structs, unions, classes, modules, and packages -- O(1) versus quadratic repeated linear scans. - -- Never emit raw source filenames or identifiers in generated code -- pass them through `protect()`/`putsQuoted` so `--protect` mode does not leak source. +- Use `UASSERT_OBJ(cond, nodep, "...")` over `UASSERT` when a node is in scope; + use `v3fatalSrc("...")` for unreachable paths, never a silent `if (!ptr) return;`. +- Use `VL_DO_DANGLING(pushDeletep(nodep), nodep)` instead of `deleteTree()` in + visitors -- deferred deletion is safe against re-entry and unlinking order. + `deleteTree()` is only for fresh nodes that never entered the tree. +- Every new AST member needs both `dump()` and `dumpJson()` support -- never wrap + these in `LCOV_EXCL`; cover them by adding a construct to `t_debug_emitv.v`. + Override `isSame()` to include any new semantically meaningful field. +- Pointers to nodes outside op1p-op4p require a `broken()` override and + `cloneRelink()` support -- avoid storing out-of-tree node pointers when possible. +- Never allocate AstNode objects on the stack or by value -- always pointers. +- Prefer `nodep->foreach(...)` and named accessors over `op1p()`..`op4p()`, but + verify traversal order is preserved when converting manual iteration. +- Prefer `AstForeach` over generating unrolled loop bodies -- constant-size code + instead of O(N); wrap the body in `AstBegin` for scope isolation. +- Always `skipRefp()` when comparing or resolving dtypes -- missing it breaks + typedef support; prove the paths with typedef tests. +- Use `num().isOpaque()` rather than `isDouble() || isString()` when guarding + V3Number comparisons against non-integer types. +- Use `FileLine::operatorCompare` for source-position ordering -- never hand-roll + filename/lineno comparisons. +- Identify compiler-generated constructs by an attribute flag on the node (with + dump support), never by name-pattern matching -- magic names break with escaped + identifiers. +- Use `V3Number` arithmetic for `AstConst` values wider than 32 bits -- `1 << i` + silently overflows at `i >= 32`. +- Use `VMemberMap`/`findMember()` for name lookups in structs, classes, modules, + and packages -- O(1) versus quadratic linear scans. +- Never emit raw source filenames or identifiers in generated code -- pass them + through `protect()`/`putsQuoted` so `--protect` does not leak source. ## Visitors and passes -- Use VL_RESTORER on every visitor member that a `visit()` modifies before iterating children -- even when nesting "cannot happen" today; nested classes/modules eventually appear and it documents intent. - -- Every pass using `userNp()` needs a `VNUserNInUse` guard, and the visitor header documents which user fields it uses -- undeclared use causes silent cross-pass conflicts. - -- Use `iterateAndNextNull()` rather than `iterate()` -- the null-safe form prevents copy-paste errors during refactors. - -- Derive read-only visitors from `VNVisitorConst` and use `iterateChildrenConst` -- better performance when the tree is not modified. - -- Visitors are algorithms: private constructor plus a static `apply()` entry point, class name prefixed with the source file name (`TimingSuspendableVisitor` in `V3Timing.cpp`). - -- Reset per-module visitor state in `visit(AstNodeModule*)` -- including numeric ID counters, to keep generated names stable. - -- Avoid `backp()` -- it returns parent or prior sibling depending on position, breaks on complex expressions, and causes O(n^2) hunts in loops; build maps or capture context during forward traversal. - -- Capture first-occurrence module state inside the node's own `visit()` handler, not via a `foreach` pre-scan from `visit(AstNodeModule)` -- the visitor already walks every node, and source order matches IEEE declaration-before-use rules. - -- When raw node pointers key a map or set, erase entries when the node is deleted -- allocators reuse addresses, so stale entries eventually alias new nodes. - -- Derive graph-shaped passes from V3Graph (`V3GraphVertex`/`V3GraphEdge`) -- never roll your own Node/Edge classes; V3Graph provides dump, color, rank, topological sort, and reachability for free. - -- When a change outgrows local rewrites, create a dedicated pass instead of growing an existing one -- keeps pass invariants clear and reduces coupling regressions. - -- Do not overload established internal terminology for new concepts -- if a term has a stable meaning (e.g. "pre" triggers in the scheduler), pick a distinct name. - -- State explicitly how side effects are preserved in optimizations involving purity, expression lifting, or simplification -- purity assumptions for functions, DPI, and methods are easy to misclassify. - -- Use `forall` only for simple predicates -- it is cheaper than a visit-everything visitor but more expensive than a visitor that can shortcut subtrees. - -- Avoid global numbering or shared mutable state when a module-local or pass-local alternative exists -- global coupling amplifies diff noise and is harder to reason about. - -- Leave explicit comments at the contract points where astgen or other generated metadata drives runtime behavior -- missing registration silently degrades type safety. +- `VL_RESTORER` on every visitor member a `visit()` modifies before iterating + children -- even when nesting "cannot happen" today. +- Every pass using `userNp()` needs a `VNUserNInUse` guard, and the header + documents which user fields it uses. +- Use `iterateAndNextNull()` rather than `iterate()` -- the null-safe form + prevents copy-paste errors during refactors. +- Derive read-only visitors from `VNVisitorConst` with `iterateChildrenConst`. +- Reset per-module visitor state in `visit(AstNodeModule*)`, including numeric ID + counters, to keep generated names stable. +- Capture first-occurrence module state inside the node's own `visit()` handler, + not via a `foreach` pre-scan from `visit(AstNodeModule)` -- source order already + matches IEEE declaration-before-use. +- Avoid `backp()` -- it returns parent or prior sibling depending on position and + causes O(n^2) hunts; build maps or capture context during forward traversal. +- When raw node pointers key a map or set, erase entries when the node is deleted + -- allocators reuse addresses, so stale entries alias new nodes. +- Derive graph-shaped passes from V3Graph (`V3GraphVertex`/`V3GraphEdge`) -- it + gives dump, color, rank, topological sort, and reachability for free. +- When a change outgrows local rewrites, create a dedicated pass instead of + growing an existing one. +- State explicitly how side effects are preserved in optimizations involving + purity, expression lifting, or simplification. ## Errors and warnings -- Append `nodep->prettyNameQ()` for user-facing names; use `name()` only in UINFO/debug output -- users see pretty names, `.tree` debuggers see internal ones. +(See the root checklist for choosing the diagnostic API and its required test.) -- Enclose specific values and variable names in single quotes: `'value'`. - -- End messages with periods, never exclamation marks; do not write "Error:" in the text -- the macro already prints the prefix. - -- Include a suggestion via `warnMore()` where possible (e.g. "Suggest use 'function automatic'"); the first location uses `warnContextPrimary()`, later ones `warnContextSecondary()`. - -- State what was attempted and what was found: "Instance attempts to connect to 'PARAM' as a parameter, but it is a variable". - -- Reference named constants or option names in messages, not magic numbers; capitalize acronyms (UDP, not udp); use formal modal verbs (must/must not). - -- Name warning codes object-first and short (`ASCRANGE`, not `RANGEASC`); rename via `renamedTo()` so old suppressions keep working; when splitting a warning, keep the old name as a meta-control. - -- Set warning suppression on `AstVar`, not `AstVarRef` -- VarRefs get recreated and lose `warnIsOff`. - -- `v3warn` checks `warnIsOff` internally -- no redundant guard needed. - -- User errors use `v3error` (non-fatal), not `v3fatal`; move fatal aborts out of visitors to the top level so AST dumps still occur on failure. - -- "Unsupported:" messages must describe the specific unsupported context ("Unsupported: sequence referenced outside assertion property"), not just the construct name -- each message must be distinct. - -- New warnings must not fire on previously-clean parameterized code -- constant-folded loops should not trigger lint; over-permissive suppression beats false positives, and survey other tools before adding a warning. - -- Internally constructed AST that uses warning-prone language features must suppress its own warnings via `fileline()->warnOff(...)`. - -- Never call bare `abort()` -- print a searchable message first (`vlAbortOrExit()`); mark unreachable code `VL_UNCOVERABLE`. - -- Defer `abortIfErrors()` to pipeline exit points, not after every operation -- users see all errors in one compile attempt. - -- When an assertion fires, fix the root-cause initialization -- never weaken the assertion. - -- When replacing or refactoring a pass, keep existing user-facing error strings (or provide direct equivalents) -- `.out` goldens and user documentation depend on the wording. +- Append `nodep->prettyNameQ()` for user-facing names; use `name()` only in + debug/UINFO output. Enclose specific values in single quotes: `'value'`. +- End messages with periods, never exclamation marks; do not write "Error:" in the + text -- the macro prints the prefix. +- State what was attempted and what was found: "Instance attempts to connect to + 'PARAM' as a parameter, but it is a variable". Add a `warnMore()` suggestion + where possible. +- Name warning codes object-first and short (`ASCRANGE`, not `RANGEASC`); rename + via `renamedTo()` so old suppressions keep working. +- Set warning suppression on `AstVar`, not `AstVarRef` -- VarRefs get recreated + and lose `warnIsOff`. +- "Unsupported:" messages describe the specific unsupported context, not just the + construct name -- each message must be distinct. +- When replacing or refactoring a pass, keep existing user-facing error strings -- + `.out` goldens and documentation depend on the wording. ## Performance and memory -- O(n^2) is never acceptable -- build maps for batch lookups instead of per-item scans; any quadratic-looking loop needs explicit complexity justification in comments. - -- Prefer `std::map` for per-module structures (many small instances) -- older libc++ allocates dozens of KB per unordered container; use `unordered_map` only for one-per-netlist data. - -- Never let `unordered_*` iteration order reach generated output -- for stable deduplication use an `unordered_set` for membership plus a `vector` for order, pushing only on successful insertion. - -- Prefer `emplace` over `insert`; check the returned `.second` instead of a separate `find()` -- avoids double lookups. - -- `reserve()` strings and vectors when the size is estimable. - -- Add no new static or global mutable data -- statics are being eliminated for future parallelism. - -- Process wide data word-by-word (`VL_MEMSET_W`, `VL_MEMCPY_W`), never bit-by-bit or byte-by-byte loops -- compilers do not optimize byte-wise sign fill. - -- No exceptions in verilated runtime code -- use error returns or assertions; exceptions add overhead. - -- Move string parsing to verilation time -- never parse strings during simulation; emit structured data or compile-time hints instead. - -- Do not add vtables to high-frequency runtime objects (8 bytes per instance); guard optional runtime features behind `hasClasses()`/`hasEvents()`-style checks. - -- Functions called per-cycle must avoid mutexes -- use atomics or lockless designs. - -- Each full-netlist visitor costs minutes on large designs -- merge visitors where possible, or collect into vectors and process afterwards. - -- Count what every new pass does via V3Stats -- stats become deterministic regression anchors in tests. - -- Use Verilator's data types for model data (`CData`/`SData`/`IData`/`QData`/`VlWide`), not `size_t` -- fixed widths across platforms; `size_t` only answers "how big is this container". - -- Emit no runtime loops in generated C++ -- either expand loops at verilation time or call a single runtime function. - -- Wrap unlikely hot-path branches (error checks, bounds checks) in `VL_UNLIKELY`/`VL_LIKELY` -- guides the compiler to optimize the common case. - -- Use generation counters (alongside `user*` fields) to invalidate caches instead of clearing them between passes. - -- Reducing memory footprint often beats reducing instruction count -- smaller per-element data wins through cache effects. - -- The `inline` keyword alone does nothing for member functions defined in headers -- use `VL_ATTR_ALWINLINE` where forced inlining matters, and weigh macro versus function overhead in `-O0` debug builds. +- O(n^2) is never acceptable -- build maps for batch lookups; any quadratic loop + needs explicit justification in a comment. +- Prefer `std::map` for per-module structures (many small instances); use + `unordered_map` only for one-per-netlist data, and never let `unordered_*` + iteration order reach generated output. +- Prefer `emplace` over `insert` and check the returned `.second` instead of a + separate `find()`. `reserve()` strings and vectors when the size is estimable. +- Add no new static or global mutable data -- statics are being eliminated for + future parallelism. +- Use Verilator's fixed-width data types for model data (`CData`/`SData`/`IData`/ + `QData`/`VlWide`), not `size_t`. Process wide data word-by-word + (`VL_MEMSET_W`, `VL_MEMCPY_W`), never bit-by-bit. +- No exceptions in verilated runtime code; do string parsing at verilation time, + never during simulation. +- Wrap unlikely hot-path branches in `VL_UNLIKELY`/`VL_LIKELY`. +- Count what every new pass does via V3Stats -- stats become deterministic + regression anchors. ## Thread safety -- Annotate with the three-level hierarchy `VL_PURE` > `VL_MT_SAFE` > `VL_MT_STABLE` -- VL_PURE has no side effects and may call only VL_PURE; VL_MT_SAFE is safe under locks; VL_MT_STABLE is safe only while tree topology is stable; VL_MT_SAFE must not call VL_MT_STABLE. - -- Annotations must match the implementation -- a function that calls non-safe functions cannot itself be marked safe. - -- Never include `verilated.h` in the compiler itself -- use `verilatedos.h` or create a new header. - -- Annotate mutex-protected members with `VL_GUARDED_BY` and document mutex acquisition ordering -- prevents priority inversion. - -- `++` on shared state and `empty()` on standard containers are not thread-safe -- use atomics or hold the lock. - -- `_exit()` is acceptable only from non-main threads in multithreaded mode; the main thread and single-threaded code use `std::exit()`, and dump gcov on any non-exit termination path. - -- Prefer has-a over is-a for thread-safe classes: a guarded class wrapping the unguarded internal one, with the guarded version as the default public API. +- Annotate with the hierarchy `VL_PURE` > `VL_MT_SAFE` > `VL_MT_STABLE`: PURE has + no side effects and calls only PURE; MT_SAFE is safe under locks; MT_STABLE is + safe only while tree topology is stable. Annotations must match the + implementation. +- Never include `verilated.h` in the compiler itself -- use `verilatedos.h`. +- Annotate mutex-protected members with `VL_GUARDED_BY` and document acquisition + ordering. `++` on shared state and container `empty()` are not thread-safe. ## Parser and lexer (verilog.y, verilog.l) -- Preserve IEEE Appendix A BNF comments (`// IEEE: {rule}`) mapping grammar rules to the standard; explain any split/combined rules, and comment explicitly when accepting syntax beyond IEEE as an extension. - -- The parser only builds AST nodes: defer semantic validation, `VN_IS` checks, and context-dependent logic to V3LinkParse/V3Width and later passes. - -- Represent hierarchical paths as structured nodes (`AstDot`/parse-ref chains via the existing `idDotted` production), never concatenated strings -- preserves per-segment FileLine. - -- Prefer tightening a grammar rule's operand type over a runtime cast-chain-plus-`v3fatalSrc` guard in a later visitor -- illegal operands then fail with a clean syntax error; the visitor guard becomes defense-in-depth. - -- Solve ambiguities with token-pipeline look-ahead (`tokenPipeScan*`) rather than limiting grammar rules; name tokens for semantic meaning, not one syntax case. - -- Mark unsupported grammar rules with the `//UNSUP` comment convention. - -- Extract repeated grammar action sequences into helper functions in `V3ParseGrammar` at the top of `verilog.y`. - -- Format grammar rules with the opening `{` at column 56 for short rules, column 24 for long rules, contents indented two more spaces -- match surrounding rules. - -- Keep lexer rules matching only semantic token content, excluding trailing whitespace; when adding lexer states or tokens, update every cross-cutting section (comments, preprocessor directives, `/*verilator*/` meta-comments). - -- Sort token declarations alphabetically by string literal; sort `yD_*` productions by token name. - -- `m_modp` can be null on parse errors, and nested modules break simple tracking -- assert non-null via helpers that carry fileline context. - -- Add a test for every `|` alternative and optional clause of a new or changed grammar rule -- untested alternatives are where parse regressions hide. +- Preserve IEEE Appendix A BNF comments (`// IEEE: {rule}`); comment explicitly + when accepting syntax beyond IEEE as an extension. +- The parser only builds AST nodes -- defer semantic validation, `VN_IS` checks, + and context-dependent logic to V3LinkParse/V3Width and later passes. +- Represent hierarchical paths as structured nodes (`AstDot`/parse-ref chains via + `idDotted`), never concatenated strings -- preserves per-segment FileLine. +- Prefer tightening a grammar rule's operand type over a runtime cast-chain guard + in a later visitor -- illegal operands then fail with a clean syntax error. +- Solve ambiguities with token-pipeline look-ahead (`tokenPipeScan*`) rather than + limiting grammar rules; mark unsupported rules with `//UNSUP`. +- Sort token declarations alphabetically by string literal; sort `yD_*` + productions by token name. +- Add a test for every `|` alternative and optional clause of a new or changed + grammar rule -- untested alternatives are where parse regressions hide. ## File-specific rules | File | Rule | |---|---| -| `configure.ac` | Keep build-time flags (sanitizers, debug) orthogonal to runtime flags and document which is which -- tool-compile options and generated-code options must be independently controllable | -| `src/V3Options.cpp` | Chain `.notNeededForRerun()` onto `DECL_OPTION()` for options that do not affect semantic output -- rerun logic then skips them | +| `src/V3Options.cpp` | Chain `.notNeededForRerun()` onto `DECL_OPTION()` for options that do not affect semantic output | | `src/V3Ast.cpp` | For composite types (queues, dynamic arrays) use `computeCastableImp()` on subtypes -- shallow `width()`/`similarDType()` checks miss nested incompatibility | -| `src/V3AstNode*.h` | File DESCRIPTION names the node category; every node class gets a what-construct comment and every member (especially node pointers) a semantic-purpose comment; put enum type definitions in `V3NodeAttr.h`, not `V3AstNodeOther.h` | +| `src/V3AstNode*.h` | Every node class gets a what-construct comment and every member a semantic-purpose comment; put enum type definitions in `V3NodeAttr.h` | | `src/V3AstNodeExpr.h` | `CCast` is only for basic C types (char/short/int/QData) -- never 4-state logic or structs | -| `src/V3AstNodeOther.h` | `cloneRelink` must propagate all stateful flags (e.g. `maybePointedTo`) and update internal references -- otherwise clones keep stale pointers into the original tree | -| `src/V3AstNodes.cpp` | Override `name()` in subclasses that store names or `.tree` dumps lose them; in JSON dumps store derived type properties (e.g. `width`) once on the dtype node, not per node | -| `src/V3Const.cpp` | Check `keepIfEmpty` before removing empty functions -- flagged functions must survive for codegen/linking/side effects | -| `src/V3Coverage.cpp` | Instrumentation contexts are opt-in (allowlist of known-safe contexts), never blocklist -- blocklists silently break when new contexts appear | -| `src/V3Inline.cpp` | Preserve `VarXRef::varp()` during passes -- intermediate pin-reconnection passes need it before V3LinkDot re-resolves | -| `src/V3LinkJump.cpp` | Track concurrent constructs (forks) path-sensitively with scope-local flags -- global flags over-conservatively block safe control-flow optimizations | -| `src/V3LinkLValue.cpp` | Propagate assignment attributes from original variables when processing aliases -- aliases are references, not new assignments | -| `src/V3Localize.cpp` | Mark scope-specific optimization restrictions on `AstVarScope`, not `AstVar` -- preserves per-scope granularity | -| `src/V3Sched*.cpp` | Every change needs a test proving necessity; isolate unrelated scheduler changes into separate PRs -- high-risk, hard-to-debug area | -| `src/V3SchedTrigger.cpp` | Name trigger eval helpers `_eval_triggers_{qualifier}__` (qualifier after "triggers") | +| `src/V3AstNodeOther.h` | `cloneRelink` must propagate all stateful flags (e.g. `maybePointedTo`) and update internal references | +| `src/V3Const.cpp` | Check `keepIfEmpty` before removing empty functions -- flagged functions must survive for codegen/side effects | +| `src/V3Coverage.cpp` | Instrumentation contexts are opt-in (allowlist), never blocklist -- blocklists silently break when new contexts appear | +| `src/V3Inline.cpp` | Preserve `VarXRef::varp()` during passes -- pin-reconnection needs it before V3LinkDot re-resolves | +| `src/V3Sched*.cpp` | Every change needs a test proving necessity; isolate unrelated scheduler changes into separate PRs -- high-risk area | diff --git a/test_regress/AGENTS.md b/test_regress/AGENTS.md index 1c8d91c4b..05833d460 100644 --- a/test_regress/AGENTS.md +++ b/test_regress/AGENTS.md @@ -2,145 +2,127 @@ SPDX-FileCopyrightText: 2026-2026 Wilson Snyder SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 --> -# test_regress/ Guidelines -- regression tests +# test_regress/ -- regression tests -When to check: before adding or editing any test under `test_regress/` (`.v` sources, `.py` drivers, `.out` golden files). -Read the repository-root [AGENTS.md](../AGENTS.md) first for process and cross-cutting rules. +Covers `.v`/`.sv` sources, `.py` drivers, and `.out` golden files under +`test_regress/`. Read the repository-root [AGENTS.md](../AGENTS.md) first. This +file has two parts: **Orientation** explains how the harness runs a test; +**Before you open a PR** is the test-authoring checklist. + +--- + +# Orientation: how the harness works + +- **A test is a `.v`/`.sv` source plus a matching `.py` driver in `t/`.** The + driver calls `test.compile()`, `test.execute()`, and/or `test.lint()`. Without a + `.py` the source never runs and is dead code giving false coverage confidence. +- **Golden output lives in `.out` files**, compared via `expect_filename`. + Generate them with `HARNESS_UPDATE_GOLDEN=1 python3 t/.py` -- never + hand-write them; the harness normalizes paths, version markers, and line numbers + that hand edits get wrong. +- **Run one test from the repository root:** `test_regress/t/t_.py`. When + running from a checkout, set `VERILATOR_ROOT` to the checkout root first so the + harness compiles the generated C++ against the right headers. +- **`test.compile()` defaults are richer than they look:** the vlt driver + auto-injects `--exe` and a generated main, and `assert property` fires its + action blocks without `--assert` -- try the simple form before adding flags. +- **The `t_dist_*` tests enforce repository conventions** (file headers, sorted + lists, warning documentation, ASCII-only) -- run the relevant ones before + submitting. + +--- + +# Before you open a PR ## Naming -- Name tests `t_{category}_{description}` in snake_case; the first word groups the category (`t_lint_unused_func_bad`, not `t_unused_func_lint_bad`) -- category prefixes make related tests findable and runnable together. - -- Use the `_bad` suffix when the test code is illegal SystemVerilog, `_unsup` when the code is legal but Verilator does not yet support it, `_off` for disabled-behavior tests -- never `_fail`. - -- Keep filenames under roughly 30-35 characters, abbreviating where needed -- concise names stay readable in test output. +- Name tests `t_{category}_{description}` in snake_case; the first word groups the + category (`t_lint_unused_func_bad`, not `t_unused_func_lint_bad`) so related + tests are findable and runnable together. +- Use `_bad` when the code is illegal SystemVerilog, `_unsup` when it is legal but + unsupported, `_off` for disabled-behavior tests -- never `_fail`. +- Keep filenames under roughly 30-35 characters. ## Files and drivers -- Every `.v` file needs a matching `.py` driver -- without one the test never runs and becomes dead code giving false coverage confidence. - -- Drivers must actually call `test.compile()` and `test.execute()` (or `test.lint()` for lint-only tests) -- a driver that sets up but never runs hides coverage gaps silently. - -- Copy the license header from an existing file: `.v` tests carry the Creative Commons Public Domain (CC0) notice with matching SPDX tags; `.py` drivers carry the standard driver header -- distribution tests check headers on every committed file. - -- Use `--binary` instead of `--exe --main --timing` (or subsets of them) -- `--binary` implies the others; shorter and clearer. - -- Error tests use `test.compile(fails=True, expect_filename=test.golden_filename)` (or `test.lint(...)`) and must not call `test.execute()` -- compilation is supposed to fail. - -- Use `test.lint()` rather than `test.compile()` for static-analysis-only tests -- clearer intent, no needless simulation. - -- `test.compile()` defaults are richer than they look: the vlt driver auto-injects `--exe` and a generated main, so bare `test.compile()` plus `test.execute()` produces a runnable binary, and `assert property` fires its action blocks without `--assert` -- try the simple form before adding flags. - -- Use `.out` golden files with `expect_filename` instead of inline `expect` regex strings -- goldens are diffable and maintainable in version control. - -- Prefer inline assertion macros for simple deterministic values; use `test.execute(expect_filename=test.golden_filename)` only for complex or multi-line output. - -- Use `test.glob_one()`/`test.glob_some()` instead of `glob.glob()` with manual count checks -- clearer assertions and better error messages. - -- Use `test.timeout()` for time budgets, not manual `time.time()` measurements -- the framework handles machine-speed variation. - -- Coverage tests must run `verilator_coverage` and verify individual bins and points, not just aggregate percentages -- aggregate-only checks mask per-bin regressions. - -- Add override/suppression test cases when introducing a new suppressible warning code -- proves users can actually turn it off. - -- Verify external tool availability with a minimal functional command, not `--version` -- proves the tool works for its purpose, not just that the binary exists. - -- Assert optimization stats with exact expected counts: `test.file_grep(test.stats, r'Optimizations, ...\s+(\d+)', N)` -- presence-only matches miss algorithmic drift; use an expected count of 0 (not `file_grep_not`) to prove a stat is exactly zero. - -- Add a `-fno-inline` driver variant (reusing the same `.v` via `test.top_filename`) for scope-dependent features -- module inlining changes scope handling. - -- Add a `_protect_ids` variant when a feature emits user identifiers or filenames -- verifies nothing from the source leaks in `--protect-ids` mode. - -- Use conservative thread counts (2 or fewer) in multithreaded tests -- CI machines run many tests in parallel and contention causes flakiness. - -- Extend existing test files with related cases instead of creating many single-purpose files; keep one test concept per file when concepts genuinely differ. - -- Keep drivers minimal -- test logic and descriptions belong in the `.v`/`.cpp` files, common patterns in `driver.py` functions. - -- Place a single support/config file directly in `t/` with a descriptive name -- a subdirectory holding one file is clutter. - -- Use `test.files_identical_sorted()` for output whose ordering is non-deterministic -- `expect_filename` comparison fails on ordering noise. - -- When running tests from a checkout, set `VERILATOR_ROOT` to the checkout root before invoking `python3 t/.py` -- the harness compiles generated C++ against the headers it finds there. - -- The `t_dist_*` tests enforce repository conventions (headers, sorted lists, warning documentation) -- run the relevant ones before submitting. +- Every `.v` needs a matching `.py` driver that actually calls `test.compile()` + and `test.execute()` (or `test.lint()` for static-analysis-only tests) -- a + driver that sets up but never runs hides coverage gaps silently. +- Copy the license header from an existing file: `.v` tests carry the CC0 notice, + `.py` drivers carry the standard driver header -- distribution tests check + headers on every committed file. +- Use `--binary` instead of `--exe --main --timing` -- it implies the others. +- Error tests use `test.compile(fails=True, expect_filename=test.golden_filename)` + (or `test.lint(...)`) and must not call `test.execute()`. +- Use `.out` golden files with `expect_filename` instead of inline `expect` + regex strings -- goldens are diffable and maintainable. +- Use `test.glob_one()`/`test.glob_some()` and `test.timeout()` instead of raw + `glob.glob()` and manual `time.time()` measurements. +- Coverage tests run `verilator_coverage` and verify individual bins and points, + not just aggregate percentages. +- Assert optimization stats with exact expected counts: + `test.file_grep(test.stats, r'Optimizations, ...\s+(\d+)', N)`. +- Add a `_protect_ids` variant when a feature emits user identifiers or filenames; + use conservative thread counts (2 or fewer) in multithreaded tests. +- Extend existing test files with related cases instead of creating many + single-purpose files; keep drivers minimal -- test logic belongs in the `.v`. ## Golden .out files -- Never hand-write or hand-edit `.out` goldens -- generate them with `HARNESS_UPDATE_GOLDEN=1 python3 t/.py`; the harness handles path normalization, version markers, and line-number masking that hand edits get wrong. - -- When a feature lands, remove its now-supported entries from `t_*_unsup.v`/`t_*_bad.v` in the same change and regenerate the goldens -- stale entries no longer error, and reviewers will flag them. +- Never hand-write or hand-edit `.out` goldens -- regenerate with + `HARNESS_UPDATE_GOLDEN=1`. +- When a feature lands, remove its now-supported entries from `t_*_unsup.v` / + `t_*_bad.v` in the same change and regenerate the goldens -- stale entries no + longer error and reviewers will flag them. ## Verilog style -- Use 2-space indentation and no tabs -- matches `nodist/verilog_format` and common online-editor settings. - -- Declarations are flush-left with a single space between type and name; never column-align declarations: +- 2-space indentation, no tabs. +- Declarations are flush-left with a single space between type and name; never + column-align: ```systemverilog // WRONG: column-aligned bit [63:0] crc = 64'h5aef0c8d_d70a4497; int cyc = 0; - // RIGHT: flush-left bit [63:0] crc = 64'h5aef0c8d_d70a4497; int cyc = 0; ``` -- Run `nodist/verilog_format` on new `.v` files; wrap test macro definitions in `// verilog_format: off`/`on` so the formatter does not split them. - -- Use `$display("%0d", ...)` not `%d` -- avoids leading-space padding in output. - -- Wrap Verilator-specific test code (e.g. `$c`) in `` `ifdef VERILATOR`` -- keeps tests portable to other simulators. - -- Use inline `// verilator lint_off WARNCODE` directives (unquoted code names) instead of `-Wno-*` driver flags, and only suppress a warning when that warning is itself under test -- fix root causes otherwise. - -- Use only IEEE 1800-compliant constructs that other simulators also accept -- tests must validate standard behavior, not Verilator's parser leniency. - -- Omit optional end labels on `endmodule`/`endclass`/`endtask`/`endfunction` -- matches the prevailing test suite style. +- Run `nodist/verilog_format` on new `.v` files; wrap macro definitions in + `// verilog_format: off`/`on` so the formatter does not split them. +- Use `$display("%0d", ...)` not `%d` -- avoids leading-space padding. +- Wrap Verilator-specific test code (e.g. `$c`) in `` `ifdef VERILATOR ``. +- Use inline `// verilator lint_off WARNCODE` only when that warning is itself + under test -- fix root causes otherwise. +- Use only IEEE 1800-compliant constructs other simulators also accept -- tests + validate standard behavior, not Verilator's parser leniency. +- Omit optional end labels on `endmodule`/`endclass`/`endtask`/`endfunction`. ## Self-checking -- Use the `checkh`/`checkd`/`checks` assertion macros, not manual `if`/`$display`/`$stop` sequences -- standard macros keep assertion style uniform across the suite. - -- Note `checkh` prints with `%p`, which renders integers as hex (`'h11` for 17) -- use `checkd` for integer comparisons and reserve `checkh` for structs/arrays where `%p` shows the assignment-pattern shape. - -- Use the `` `stop`` macro rather than direct `$stop` -- allows centralized control of stop behavior. - -- Verify against CRC-checked or generated sequences rather than single inline spot checks -- sequence verification catches edge cases that spot checks miss. - -- Exercise runtime behavior over multiple clock cycles, not just initial values -- initial blocks are optimized differently from dynamic data flow. - -- Drive logic with variable inputs (counters, CRC registers) so constant folding and dead-code elimination cannot evaluate the logic under test at compile time. - -- Loop randomization/constraint tests 10-20 iterations and check that values actually differ across iterations -- for variables wider than 1 bit, 10 identical draws is effectively impossible. - -- Use the minimum iteration count that reliably detects the bug, not an arbitrary large number -- keeps the regression suite fast. - -- For constraint-enforcement tests, pick a narrow value space (e.g. `bit [3:0]` with 4-5 elements) so a violation is probable per run -- and verify the test fails on an unfixed tree before submitting. +- Use the `checkh`/`checkd`/`checks` assertion macros, not manual + `if`/`$display`/`$stop` sequences. Note `checkh` prints with `%p`, which renders + integers as hex -- use `checkd` for integer comparisons. +- Use the `` `stop `` macro rather than direct `$stop`. +- Drive logic with runtime-varying inputs (counters, CRC/LFSR registers) so + constant folding cannot pre-evaluate the logic under test; check behavior across + multiple clock cycles, not just initial values. +- For a test whose pass/fail depends on varying or random values, loop enough + iterations that the values demonstrably differ and size the value space so a + failure is probable per run, then confirm the test fails on an un-fixed tree + before submitting. ## Test design -- Use non-power-of-2, non-word-aligned widths (e.g. 7, 15, 31, 33, 63, 65, 95) -- exposes masking and word-boundary bugs in the under-32, 33-64, and over-64 bit representations that widths like 32/64/128 hide. - -- Test both `[high:low]` and `[low:high]` orderings plus non-zero bounds like `[3:1]` and `[5:4]` -- catches endianness and offset bugs that zero-based ranges miss. - -- Use different ranges for each axis of multidimensional arrays (`[2:1][3:1] arr [5:4][6:3]`) -- uniform dimensions hide iteration and offset-calculation bugs. - -- When adding type support, test all basic types (chandle, string, real, ...) and typedef-wrapped variants -- proves each either works or produces a clear error, and exercises the typedef-resolution paths. - -- Test constructs in all control-flow contexts (loop conditions, increments, `if`, `case`) and test sibling operators in the same optimization domain -- shared compiler code paths break together. - -- Include the issue's own reproducer as a committed test, and verify the test fails without the fix -- prevents false-positive regressions. - -- Use DPI-C imports for external C functions, not VPI or `$c`, where the test allows -- keeps tests portable across simulators. - -- Instantiate parameterized interfaces multiple times with varying parameters -- single-instance tests miss instance-specific connection bugs. - -- Test dual-use methods both as void statements and in value-returning contexts -- catches bugs where return values are ignored or misused. - -- Test constraints involving array methods with all comparison operators (`==`, `!=`, `<`, `>`, `<=`, `>=`) -- rarely used operators regress silently. - -- Assert non-blocking-assignment results in the cycle immediately after they take effect, before later overwrites, and use indices that change post-NBA -- verifies NBA timing and index capture, not just the final value. - -- Keep standard idioms like `do ... while (0)` macro wrapping in test code even if a compiler pass mishandles them -- fix the compiler, not the test. +- Use non-power-of-2, non-word-aligned widths (7, 15, 31, 33, 63, 65, 95) -- + exposes masking and word-boundary bugs that 32/64/128 hide. +- Test both `[high:low]` and `[low:high]` orderings plus non-zero bounds like + `[3:1]`; use different ranges for each axis of multidimensional arrays. +- When adding type support, test all basic types (chandle, string, real) and + typedef-wrapped variants. +- Include the issue's own reproducer as a committed test, and verify it fails + without the fix. +- Assert non-blocking-assignment results in the cycle immediately after they take + effect, before later overwrites, using indices that change post-NBA.