diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..17ede4bfb --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,98 @@ + + +# Verilator Coding Guidelines + +When to check: read this file before making any change in this repository, then read the directory guide for the area being edited. + +## Topic guides + +- [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 + +## 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. + +- Run a single test from the repository root: `test_regress/t/t_.py`; run the full regression with `make test`. + +## Error classification + +Choose the diagnostic API by what went wrong; the choice 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 | +| `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. + +## 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. + +- Keep lists sorted: lexer/parser tokens, option declarations, enum values, configure feature lists, documented option lists. + +- `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. diff --git a/docs/AGENTS.md b/docs/AGENTS.md new file mode 100644 index 000000000..ee4ae5249 --- /dev/null +++ b/docs/AGENTS.md @@ -0,0 +1,38 @@ + + +# docs/ Guidelines -- Verilator documentation (*.rst) + +When to check: before editing anything under `docs/`. +Read the repository-root [AGENTS.md](../AGENTS.md) first for process and cross-cutting rules. + +## Writing rules + +- Rewrap paragraphs after editing -- keep consistent line length in `*.rst` files. + +- Document only stable, implemented features -- never planned or in-development capabilities; prevents user confusion and support burden. + +- Explain WHAT and WHEN, not HOW -- conceptual purpose and practical use cases over implementation mechanics; describe behavior ("optimized as pure", not "treated as pure") and clarify ambiguous referents ("in the internals of Verilator"). + +- Keep terminology consistent -- one term per concept; update docs when renaming code constructs; spell out full terms, avoiding abbreviations like "sim"/"sims". + +- Use "how many" for countable nouns (threads, tasks, workers) and "how much" for uncountable quantities. + +- Mark internal or experimental features "for internal use only" -- prevents user dependence and forced deprecation cycles later. + +- Use specific IEEE references: `IEEE {number}-{year}` plus the section (e.g. `Annex I`) -- a vague "IEEE spec requires" is unverifiable. + +- Document all flags with descriptions, not just syntax. + +## reStructuredText mechanics + +- Use the `:vlopt:` role for Verilator option references -- makes cross-references clickable and consistent. + +- Escape angle brackets (`\<`, `\>`) in link targets -- prevents broken links to command-line options. + +- Generate documentation examples with `test.extract` from `test_regress` test files -- examples stay synced with actually tested behavior. + +## Hard constraint + +- Never edit `docs/CONTRIBUTORS` -- only humans may, after reading and agreeing to its statement; remind the user instead. diff --git a/src/AGENTS.md b/src/AGENTS.md new file mode 100644 index 000000000..f5f5cec94 --- /dev/null +++ b/src/AGENTS.md @@ -0,0 +1,297 @@ + + +# src/ Coding Guidelines -- 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. + +## 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. + +## 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`: + + ```cpp + // BAD: redundant double check + if (VN_IS(nodep, VarRef)) { AstVarRef* const refp = VN_AS(nodep, VarRef); } + // GOOD: single conditional cast + 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. + +## 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. + +## 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. + +- 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. + +## 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. + +## 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. + +## 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. + +## 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/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/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") | diff --git a/test_regress/AGENTS.md b/test_regress/AGENTS.md new file mode 100644 index 000000000..1c8d91c4b --- /dev/null +++ b/test_regress/AGENTS.md @@ -0,0 +1,146 @@ + + +# test_regress/ Guidelines -- 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. + +## 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. + +## 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. + +## 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. + +## 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: + + ```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. + +## 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. + +## 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.