chore/litgen: safely refresh continuous checks by default - #2302
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This PR adds a -update mode to litgen that regenerates only failing CHECK groups in place, plus several new IR-generalization regexes (symbol/cgo hashes, numeric globals, metadata IDs, setjmp/longjmp, pthread opaque sizes, test-case path prefixes). The change is well-structured and the new -update path has good test coverage (TestUpdateSourceChecks_*).
Findings below are non-blocking. The most actionable are the missing docs for the new flag and a couple of correctness/robustness edge cases in the generalization logic. Details are in inline comments; a few cross-cutting notes:
-updateflag is undocumented.dev/README.mddescribes thelitgendefault (full-rewrite) behavior but never mentions the new-updateflag or how it differs (preserves passing groups, regenerates only failing groups at their existing byte range, never adds new function/global checks). Please document it in thelitgensection.updateSourceChecksuses O(G×F) FileCheck subprocess spawns. EachmatchCheckTextcall spawns an external FileCheck process + temp file, andfindFunctionForCheckGroupruns it once per IR function per check group. For a large golden (G groups × F IR functions) this becomes thousands of process launches. Acceptable for a dev-only tool, but the quadratic factor is avoidable — the definition-line match infindFunctionForCheckGroupcould be a direct symbol/string comparison againstfn.symbol/fn.lines[0]instead of shelling out to FileCheck per function.
| line = numericGlobalRE.ReplaceAllString(line, `@{{[0-9]+}}`) | ||
| line = metadataIDRE.ReplaceAllString(line, `!{{[0-9]+}}`) | ||
| line = generalizePlatformIR(line) | ||
| line = strings.ReplaceAll(line, "[[", `{{\[\[}}`) |
There was a problem hiding this comment.
Order-dependent [[ escaping can corrupt already-generated FileCheck patterns.
By this point scrubIRLine has already injected patterns that each contain a single [ — {{[0-9]+}} (numeric globals / metadata), {{[0-9a-f]+}} (cgo hash), {{[-A-Za-z0-9_]+}} (symbol hash). The blind whole-line strings.ReplaceAll(line, "[[", ...) escapes any run of two [, so if an injected [ ends up adjacent to a literal [ from the original IR, the intended {{[...}} regex gets rewritten into {{\[\[}} and the pattern is corrupted.
Safer to escape [[ on the raw IR line before injecting any FileCheck patterns, rather than after. The existing test (TestGeneralizeIRLine_EscapesFileCheckSyntaxAndCgoHash) only covers a standalone [[ with no adjacent injected pattern, so this edge case is untested.
There was a problem hiding this comment.
Fixed in 2032f29. Raw [[ is escaped before generated regex substitutions, and the regression test covers adjacency between an injected character class and a literal bracket.
| } | ||
| var matched *irFunction | ||
| for i := range funcs { | ||
| if len(funcs[i].lines) == 0 || matchCheckText(definitionCheck, strings.Join(funcs[i].lines, "\n")) != nil { |
There was a problem hiding this comment.
This runs a full FileCheck subprocess (via matchCheckText → temp file + filecheck.Match → exec.Command(...).Run()) once per IR function, for every check group — i.e. O(groups × funcs) process spawns and temp files per -update run. It also re-computes strings.Join(funcs[i].lines, "\n") for every function on every group.
Since the goal here is only to identify which function a CHECK group belongs to via its single define line, a direct comparison against fn.symbol / fn.lines[0] would remove the FileCheck spawn from this inner loop entirely. Non-blocking (dev-only tool), but worth it if goldens grow.
There was a problem hiding this comment.
Addressed in 2032f29. Canonical generated definition checks now use a precomputed direct index; FileCheck iteration remains only as a compatibility fallback for intentionally loose hand-written definitions and changed signatures.
| return "", false, fmt.Errorf("%s: no checks generated for %q", srcPath, fn.symbol) | ||
| } | ||
| if hasDefinition { | ||
| // Keep the existing definition directive byte-for-byte. It may be |
There was a problem hiding this comment.
The comment says the definition directive is kept "byte-for-byte", but firstDefinitionCheck (line 204) does strings.TrimLeft(line, " \t"), and the block is then re-emitted through formatDirectiveBlock(indent, ...) which reapplies the group's computed indent. So the directive content is preserved verbatim, but leading whitespace is re-normalized — not byte-identical if the original indentation differed. Consider rewording to something like "keep the existing definition directive's content verbatim (leading indentation is re-normalized)".
There was a problem hiding this comment.
Fixed in 2032f29. The inaccurate byte-for-byte wording was removed; the current comment only promises that hand-written checks are kept verbatim and validated when they are not regenerated.
| } | ||
|
|
||
| var pthreadOpaqueSizes = []pthreadOpaqueSize{ | ||
| {"MutexAttr", regexp.MustCompile(`\[(?:4|8|16) x i8\]`), `[{{(4|8|16)}} x i8]`}, |
There was a problem hiding this comment.
The disambiguation between Mutex/MutexAttr, Cond/CondAttr, RWLock/RWLockAttr in generalizePlatformIR (line 706) relies on two independent, undocumented invariants: (1) the Attr variants must be ordered before their prefixes in this slice, and (2) the trailing " in strings.Contains(line, ".../sync."+typeName+") is load-bearing (it prevents Mutex from matching MutexAttr). A future reorder or a dropped trailing quote would silently apply the wrong size set. Worth a comment documenting both, and adding MutexAttr/RWLockAttr/CondAttr cases to TestGeneralizePlatformIR (currently only Mutex is covered).
There was a problem hiding this comment.
Fixed in 2032f29. Pthread types are extracted with an exact type-name regexp and looked up in a map, so correctness no longer depends on prefix ordering or a trailing quote. MutexAttr, RWLockAttr, and CondAttr cases are covered.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
changed the title
1 participant
Summary
CHECK/CHECK-LABEL+CHECK-NEXT/CHECK-EMPTYsnapshots in place by default, including snapshots that still pass-forcefallbacknest/swiftself, setjmp/longjmp names, and pthread/jmp-buffer layoutsBenefits
Compiler changes can refresh stale continuous IR snapshots without discarding hand-written test intent or requiring manual cleanup. Regenerating passing snapshots also detects stale but overly broad checks, while deterministic platform generalization keeps reviews reproducible across Linux and macOS.
Validation
go test ./chore/litgen -count=1 -cover(78.3%)cl/_test*/**files to its base, and ran default litgen on the 62 changed LIT sources-forcecl/_testltoremains intentionally excluded from bulk refreshes because its CHECKs inspect post-LTO IR while litgen produces package IR before that transform.