From 467c911dc58de1fb88e59fef4f8b4af5aa607883 Mon Sep 17 00:00:00 2001 From: araq Date: Tue, 25 Aug 2026 21:00:13 +0200 Subject: [PATCH] IC testing: compare against `nim c`, and run the real corpus under `nim ic` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every invariant `tests/ic` checked was IC-against-IC — clean == incremental, a no-op edit changes nothing, a body edit moves no interface cookie. An IC that is *consistently* wrong satisfies all of them, and that is exactly how two silent miscompilations survived. `koch bootic` has the same blind spot: it proves the compiler reproduces itself, not that it agrees with the reference backend. Two mechanisms, at different scales. **The oracle, in the metamorphic runner.** Every successful step is now also compiled with `nim c` and run, and the two outputs must agree. Unlike the hand-written `expect:` strings this needs no foresight from the test author: it compares everything the program does, not only what someone thought to print, which is what a silently-skipped destructor evades. `no-oracle` opts out. The format also grew the expressiveness the recent bug hunt showed was missing — every one of these described a state the suite could not reach: * `#!DELETE ` removes a module. Deleting a still-imported file moves no mtime, so nothing re-fires. * `#!FLAGS ` changes the compiler switches between steps. A config change is not a file either. * `fails: ` asserts that BOTH compilers reject the program with that text. Previously every step had to succeed, so the whole error path — and recovery from it — was untested. Six regression tests cover the eight bugs the last round fixed. `testament r ` now dispatches metamorphic tests like `testament cat ic`. **`testament --ic` runs the whole corpus through the incremental compiler**, so IC inherits ~10k programs with expected output instead of 30 bespoke tests. Two things had to change for that to mean anything: * `nimcacheDir` now keys on the `matrix:` entry too. Two matrix variants of one file are two different compilations; sharing a cache meant each run invalidated what the previous left — harmless for a backend that caches only object files, useless for an incremental one. * About half the corpus overrides the command wholesale (`cmd: "nim c --gc:arc $file"`), bypassing both `$target` and `$options`. Those are rewritten to `ic` and given a private cache. **Warm cache, hastur-style.** A generated warmup program pulling in `system` and the most-imported stdlib modules is compiled once per distinct compile configuration into `nimcache/ic_warmup_`, and each test's empty cache is seeded from it with mtimes preserved (nifmake compares output-mtime > input-mtime, so stamping the copies "now" re-fires the whole graph). Only program-independent artifacts are copied: the frontend NIFs and cookies plus the per-module `lower`/`cg` output. The `.c`/`.o` are left behind on purpose — the merge decision is whole-program, so they are re-rendered for every program anyway. `tests/destructor` (97 runs): `nim c` 35s cold / 32s warm; `nim ic` ~3m30 cold / **9.8s warm**. Compiler changes this required or uncovered: * `merge` read the live-module list from a manifest the driver writes instead of globbing `*.c.nif` off the nimcache. Globbing absorbed artifacts belonging to any other program sharing the directory — which is what made a prefilled cache produce undefined symbols at link. * The build-arg signature no longer includes `--icproject:`/`--icPreparsedConfig:` (they name where a build lives, not what it produces, so two caches holding identical artifacts got different signatures). The precompiled config still counts, by content hash, minus its `(nimcache …)` line. * `.s.deps` seeding is speculative and runs before the prune, so a sidecar entry that has gone stale (an import that a `when` no longer takes) can be dropped instead of lingering forever; a pruned module's scan artifacts are deleted so an edit-accumulated cache still matches a clean one. * A failed nifmake run no longer prints an `Error:` of its own. The children have already reported; adding a build-system status as the LAST error hid the compiler's real message from anything reading the final error — every reject-style test under `nim ic` said "nifmake failed with exit code: 1". * `--mm:hooks` fed the mm mode to an on/off switch and failed outright with "'on' or 'off' expected, but 'hooks' found". Pre-existing and unrelated to IC; only reachable through the explicit switch, since `--newruntime` sets `selectedGC` directly. Running `tests/destructor` under `--ic` currently leaves 10 failures. They are genuine IC defects, not harness noise (all 97 pass under `nim c`) — the clearest is `tglobaldestructor`: `graph.globalDestructors` is accumulated while injecting destructors into a module's top level, but the main module's `cg` — which emits the teardown — is a different process, so a module-level `var` with a `=destroy` is never destroyed. Same shape as the init/datInit metas, and it wants the same fix: record it in the `.c.nif` head. Validation: `koch bootic` reaches its byte-identical fixed point; `tests/ic` is 36/36; arc, destructor, macros, template, iter, closure, ccg, codegen, types and effects pass under `nim c`, with generics showing only its pre-existing failure. Co-Authored-By: Claude Opus 5 --- compiler/cnif.nim | 7 ++ compiler/commands.nim | 6 +- compiler/deps.nim | 93 ++++++++++++++++++-- compiler/nifbackend.nim | 15 +++- doc/ic.md | 59 +++++++++++++ testament/categories.nim | 106 +++++++++++++++++++++- testament/testament.nim | 141 ++++++++++++++++++++++++++++-- tests/ic/tconfig_invalidation.nim | 34 +++++++ tests/ic/tdead_when_import.nim | 37 ++++++++ tests/ic/tdeleted_module.nim | 26 ++++++ tests/ic/tdestructor_fidelity.nim | 68 ++++++++++++++ tests/ic/tdiscovered_import.nim | 38 ++++++++ tests/ic/terror_recovery.nim | 32 +++++++ 13 files changed, 641 insertions(+), 21 deletions(-) create mode 100644 tests/ic/tconfig_invalidation.nim create mode 100644 tests/ic/tdead_when_import.nim create mode 100644 tests/ic/tdeleted_module.nim create mode 100644 tests/ic/tdestructor_fidelity.nim create mode 100644 tests/ic/tdiscovered_import.nim create mode 100644 tests/ic/terror_recovery.nim diff --git a/compiler/cnif.nim b/compiler/cnif.nim index 0caebd8682..3799ad8d27 100644 --- a/compiler/cnif.nim +++ b/compiler/cnif.nim @@ -585,6 +585,13 @@ proc computeMergeDecision*(files: openArray[string]): MergeDecision = if d in result.live: inc result.liveDefs const MergeDecisionFile* = "ic.backend.merge.nif" +const LiveModulesFile* = "ic.backend.live.txt" + ## One `.c.nif` path per line: exactly the artifacts of the modules the CURRENT + ## build graph considers live. The `merge` stage reads this instead of globbing + ## `*.c.nif` off the nimcache, so a leftover artifact from an unrelated build + ## that happens to share the cache directory cannot be merged in (which is what + ## made a shared prebuilt cache unusable: merge picked owners in modules the + ## program does not import, and the link then wanted their objects). ## Fixed name of the merge stage's output in the nimcache, read by `emit`. proc writeMergeDecision*(outfile: string; d: MergeDecision) = diff --git a/compiler/commands.nim b/compiler/commands.nim index 597063a680..1182a4a377 100644 --- a/compiler/commands.nim +++ b/compiler/commands.nim @@ -627,7 +627,11 @@ proc processMemoryManagementOption(switch, arg: string, pass: TCmdLinePass, conf.selectedGC = gcHooks defineSymbol(conf.symbols, "gchooks") incl conf.globalOptions, optSeqDestructors - processOnOffSwitchG(conf, {optSeqDestructors}, arg, pass, info) + # (The `arg` here is the mm MODE — "hooks" — so feeding it to an on/off + # switch made `--mm:hooks` fail outright with "'on' or 'off' expected, but + # 'hooks' found". The `incl` above is what that call was meant to do. + # Reachable only via the explicit switch: `--newruntime` sets + # `selectedGC` directly, which is why this stayed hidden.) if pass in {passCmd2, passPP}: defineSymbol(conf.symbols, "nimSeqsV2") of "go": diff --git a/compiler/deps.nim b/compiler/deps.nim index dcfaf78018..a812d949b2 100644 --- a/compiler/deps.nim +++ b/compiler/deps.nim @@ -11,6 +11,7 @@ ## This enables incremental and parallel compilation using the `m` switch. import std / [os, tables, sets, times, osproc, algorithm, strtabs, strutils, syncio] +from std/sha1 import secureHash, `$` import options, msgs, lineinfos, pathutils, condsyms, modulepaths, extccomp, cnif, platform @@ -805,6 +806,15 @@ proc pruneDeadSpeculative(c: var DepContext) = var cascaded = 0 for i in 0 ..< n: if not alive[i]: + # Drop the scan artifacts of a module that just left the graph. `nifler` + # ran on it during `traverseDeps` (that is how we learned it cannot + # build), and leaving its `.p.nif`/`.deps.nif` behind makes an + # edit-accumulated cache differ from a clean one for no reason. Re-running + # nifler if it ever comes back costs a single parse. + for f in c.nodes[i].files: + removeFile(c.parsedFile(f)) + removeFile(c.depsFile(f)) + removeFile(c.parsedFile(f).changeFileExt("") & ".deps.nif") if c.nodes[i].missingImport.len > 0: rawMessage(c.config, hintSuccess, "ic: skipping " & c.nodes[i].files[0].nimFile & @@ -983,12 +993,37 @@ proc configSignatureFile(c: DepContext; forwardedArgs: seq[string]): string = ## OLD configuration. Reify the configuration as a FILE and make every rule ## that consumes it an input, so a config change moves an mtime like any edit. ## Written `OnlyIfChanged` so a genuine no-op run stays a no-op. + ## + ## Deliberately EXCLUDES the two per-build path switches (`--icproject:`, + ## `--icPreparsedConfig:`): they name where this build lives, not what it + ## produces, so including them made the signature differ between two caches + ## holding byte-identical artifacts — which defeats prefilling a test's cache + ## from a shared warm one (every rule would re-fire on the rewritten + ## signature). The precompiled config still counts, by CONTENT: a `nim.cfg` + ## edit changes the artifact, hence the hash, hence every rule. result = getNimcacheDir(c.config).string / "ic_build_args.txt" var content = "" for p in c.config.searchPaths: content.add "--path:" & p.string & "\n" for a in forwardedArgs: + if a.startsWith("--icproject:") or a.startsWith("--icPreparsedConfig:"): + continue content.add a & "\n" + if c.config.icPreparsedConfig.len > 0 and fileExists(c.config.icPreparsedConfig): + # Hash the precompiled config MINUS its `(nimcache "...")` entry — the one + # line in the artifact that records where this build's cache lives rather + # than what the config says. Everything else is genuinely config-derived, so + # two builds with the same `nim.cfg`/`config.nims` hash the same no matter + # which directory they run in. + var normalized = "" + try: + for line in lines(c.config.icPreparsedConfig): + if "(nimcache " in line: continue + normalized.add line + normalized.add '\n' + except IOError, OSError: + normalized = c.config.icPreparsedConfig + content.add "config:" & $secureHash(normalized) & "\n" if not fileExists(result) or readFile(result) != content: writeFile(result, content) @@ -1383,13 +1418,29 @@ proc generateBackendBuildFile(c: DepContext; forwardedArgs: seq[string]): string outputStr cnifFiles[i] b.endTree() - # merge: read every `.c.nif`, write the ownership/liveness decision. + # merge: read the live modules' `.c.nif`, write the ownership/liveness + # decision. The list is handed over as a FILE (`LiveModulesFile`) because the + # merge child is a separate process that never sees the build file: without it + # merge globbed `*.c.nif` off the nimcache and so silently absorbed artifacts + # belonging to some other program that shares the directory. + let liveFile = nimcache / LiveModulesFile + block: + var manifest = "" + for i in 0 ..< c.nodes.len: + if live[i]: + manifest.add cnifFiles[i] + manifest.add "\n" + # OnlyIfChanged: its mtime is a merge input, so rewriting it every run would + # re-fire merge (and, through the decision, every `emit`) on a no-op build. + if not fileExists(liveFile) or readFile(liveFile) != manifest: + writeFile(liveFile, manifest) b.addTree "do" b.addIdent "nim_nifc" b.withTree "args": b.addStrLit "--icBackendStage:merge" for i in 0 ..< c.nodes.len: if live[i]: inputStr cnifFiles[i] + inputStr liveFile outputStr mergeFile b.endTree() @@ -1443,7 +1494,17 @@ proc deriveFromSemDeps(c: var DepContext): bool = ## Editing `dyn.nim` then changed nothing at all: the build silently reused the ## `.s.bif` from the run that discovered it. Seeding from the sidecars makes ## the discovery stick across runs. + ## + ## The edges are recorded SPECULATIVELY: a sidecar says what the module + ## imported the last time it was semmed, which is a statement about the past. + ## Flip a `when`, or delete an `import`, and a module that is no longer reached + ## would otherwise linger in the graph forever (and fail to build, if what it + ## imports is gone). Marking the edge speculative lets `pruneDeadSpeculative` + ## drop such a leftover, while a genuinely-needed macro import — which compiles + ## fine — stays. result = false + inc c.speculating + defer: dec c.speculating let n0 = c.nodes.len # snapshot: new nodes are traversed as they're added for ni in 0 ..< n0: for p in readSemDeps(c, c.nodes[ni].files[0]): @@ -1463,7 +1524,7 @@ proc deriveFromSemDeps(c: var DepContext): bool = traverseDeps(c, pair, newNode) result = true if idx != ni and idx notin c.nodes[ni].deps: - c.nodes[ni].deps.add idx + addDepEdge(c, c.nodes[ni], idx) result = true proc commandIc*(conf: ConfigRef; frontendOnly = false) = @@ -1563,16 +1624,17 @@ proc commandIc*(conf: ConfigRef; frontendOnly = false) = # Process dependencies traverseDeps(c, rootPair, rootNode) + # Re-apply what earlier runs discovered post-sem (macro-generated imports), + # so those modules keep their rules on a warm build instead of vanishing from + # the graph until the next failure. No-op on a cold cache. Runs BEFORE the + # prune so a sidecar entry that has since gone stale is prunable too. + discard deriveFromSemDeps(c) + # Modules that only a `when` the scanner cannot decide pulls in, and that # import something not installed, are dead in this configuration; scheduling # them would fail the build over code the classic compiler never reads. pruneDeadSpeculative(c) - # Re-apply what earlier runs discovered post-sem (macro-generated imports), - # so those modules keep their rules on a warm build instead of vanishing from - # the graph until the next failure. No-op on a cold cache. - discard deriveFromSemDeps(c) - # Discovery via `.s.deps`: imports GENERATED by macros (chronicles builds # `import chronicles/textlines` via parseStmt from the chronicles_sinks # define) are invisible to the static scanner. Each `nim m` records the @@ -1645,7 +1707,18 @@ proc commandIc*(conf: ConfigRef; frontendOnly = false) = if rounds <= 20: discovered = deriveFromSemDeps(c) if not discovered: - rawMessage(conf, errGenerated, "nifmake failed with exit code: " & $exitCode) + # The children have already printed the real diagnostics. Adding an + # `Error:` line of our own here made a build-system status the LAST error + # in the stream, hiding the compiler's own message from anything that + # reads the final error (testament's `errormsg:`, editors, CI log + # scrapers) — every `reject`-style test under `nim ic` reported + # "nifmake failed with exit code: 1" instead of what the compiler said. + # The non-zero exit is what signals failure; this line is context. + rawMessage(conf, hintExecuting, + "nifmake reported failures (exit code " & $exitCode & ")") + # Fail the run without printing an `Error:` of our own (see above): the + # exit code is derived from `errorCounter`. + inc conf.errorCounter break # Phase 2 — backend (whole-program `nim nifc`), run once over the now-final @@ -1660,6 +1733,8 @@ proc commandIc*(conf: ConfigRef; frontendOnly = false) = rawMessage(conf, hintExecuting, cmd) let exitCode = execShellCmd(cmd) if exitCode != 0: - rawMessage(conf, errGenerated, "nifmake (backend) failed with exit code: " & $exitCode) + rawMessage(conf, hintExecuting, + "nifmake reported backend failures (exit code " & $exitCode & ")") + inc conf.errorCounter else: rawMessage(conf, errGenerated, "nim ic not available in bootstrap build") diff --git a/compiler/nifbackend.nim b/compiler/nifbackend.nim index b77f3cdbd3..ede4550735 100644 --- a/compiler/nifbackend.nim +++ b/compiler/nifbackend.nim @@ -724,8 +724,19 @@ proc generateMergeStage(g: ModuleGraph) = ## in-process first-claimant/DCE coordination. let nimcache = getNimcacheDir(g.config).string var files: seq[string] = @[] - for artifact in walkFiles(nimcache / "*.c.nif"): - files.add artifact + # The driver lists the live modules' artifacts explicitly (deps.nim's + # `writeLiveModules`); only fall back to globbing when that manifest is + # absent (a cache written by an older compiler). Globbing merges whatever + # `.c.nif` happens to sit in the directory, which is wrong the moment the + # cache is shared with another program — see `LiveModulesFile`. + let manifest = nimcache / LiveModulesFile + if fileExists(manifest): + for line in lines(manifest): + let p = line.strip() + if p.len > 0: files.add p + else: + for artifact in walkFiles(nimcache / "*.c.nif"): + files.add artifact sort files let decision = computeMergeDecision(files) if decision.broken: diff --git a/doc/ic.md b/doc/ic.md index 1b619aa263..d17f457347 100644 --- a/doc/ic.md +++ b/doc/ic.md @@ -405,3 +405,62 @@ See also - NIF format spec: [nifspec/doc/nif-spec.md](../nifspec/doc/nif-spec.md) - NIFC (C-like target) spec: dist/nimony/doc/nifc-spec.md + +Testing IC +========== + +Two mechanisms, at very different scales. + +**`tests/ic` — metamorphic tests.** A `t*.nim` whose body contains `#? metamorphic` +drives a sequence of cross-module edits through `nim ic` in one fixed build +directory (see `testament/categories.nim`, `runMetamorphicIcTest`). Directives: + +| directive | effect | +| --------- | ------ | +| ``#!FILE `` | (re)write a module in the virtual file system | +| ``#!DELETE `` | remove a module, from the vfs and from disk | +| ``#!FLAGS `` | change the compiler switches from here on | +| ``#!STEP `` | materialise the files, build, run, check | + +Step attributes: ``expect: ``, ``fails: `` (BOTH compilers must +reject it, with that text), ``noop``, ``body-edit``, ``iface-edit``, +``modules: ``, ``clean``, ``no-oracle``. + +Every successful step is **also compiled with `nim c` and run, and the two +outputs must agree**. That oracle is the only check in the suite that is not +IC-against-IC: `clean == incremental`, `noop changes nothing` and the cookie +invariants are all satisfied by an IC that is *consistently* wrong, which is how +two silent miscompilations survived (a NIF-loaded module's `sfInjectDestructors` +was lost, so top-level destructors were never injected; `nfFirstWrite`/`nfLastRead` +had nowhere to live on a serialized sym node, so every first assignment to a +destructor-bearing local became `=sink` over zeroed memory). `koch bootic` has the +same blind spot — it proves the compiler reproduces *itself*. + +**`testament --ic` — the whole corpus.** Compiles every C-target test with +`nim ic` instead of `nim c`, so IC inherits the existing ~10k programs and their +expected output instead of the handful written for it by hand. Tests that +override the command (`cmd: "nim c --gc:arc $file"`) are rewritten too, and get a +private nimcache; without one they would share a cache and thrash it. + +To keep that affordable, testament borrows nimony's hastur model +(`warmupSharedCache` + `prefillFromWarmup`): a generated warmup program pulling in +`system` and the most-imported stdlib modules is compiled once per distinct +compile configuration into `nimcache/ic_warmup_`, and each test's empty +cache is seeded from it with **mtimes preserved** (nifmake compares +output-mtime > input-mtime, so stamping the copies "now" would re-fire the whole +graph). Only program-independent artifacts are copied — the frontend NIFs and +cookies plus the per-module `lower`/`cg` outputs. The `.c`/`.o` are deliberately +left behind: the merge decision (which module owns each emit-everywhere +definition) is whole-program, so those are re-rendered for every program anyway. + +Measured on `tests/destructor` (97 test runs, 32-core box): + +| | cold | warm | +| - | ---- | ---- | +| `nim c` | 35s | 32s | +| `nim ic` | ~3m30 | **9.8s** | + +The warm number is the developer loop and it is 3.2x faster than the classic +backend; the cold number is paid once per configuration and then cached on disk. +The disk cost is real and worth knowing: ~3.4 GB of nimcache for that one +category. diff --git a/testament/categories.nim b/testament/categories.nim index 7ed98b8aa9..377955feb5 100644 --- a/testament/categories.nim +++ b/testament/categories.nim @@ -516,7 +516,15 @@ proc testNimblePackages(r: var TResults; cat: Category; packageFilter: string) = # accumulated file set is materialised before each `#!STEP`. A `#!STEP`'s # attributes are `;`-separated, each either `key: value` or a bare flag: # expect: noop body-edit iface-edit modules: clean +# fails: no-oracle # The last step always also runs the clean==incremental check. +# +# Every successful step is ALSO compiled with `nim c` and run, and the two +# outputs must agree (`no-oracle` opts out). This is the only check in the suite +# that is not IC-against-IC; without it a consistently wrong IC passes +# everything. `#!DELETE ` removes a module, `#!FLAGS ` changes +# the compiler switches from that point on, and `fails: ` asserts that +# BOTH compilers reject the program with that text. type MetamorphicError = object of CatchableError resultKind: TResultEnum @@ -590,16 +598,34 @@ proc runMetamorphicIcTest(r: var TResults; file: string; cat: Category; options: let buildDir = (file.changeFileExt("") & "_mm").absolutePath let nc = buildDir / "nc" let bin = buildDir / "prog".addFileExt(ExeExt) + # The ORACLE: the same sources compiled by the classic backend. Every + # invariant this runner checked before was IC-against-IC (clean == incremental, + # no-op changes nothing, ...), which a *consistently* wrong IC satisfies + # perfectly — that is how a whole class of silent miscompilations (top-level + # destructors never injected; `nfFirstWrite`/`nfLastRead` dropped by the + # serializer, so every first assignment to a destructor-bearing local became + # `=sink` over zeroed memory) stayed invisible. `nim c` is the reference the + # suite was missing. + let ncRef = buildDir / "ncref" + let binRef = buildDir / "progref".addFileExt(ExeExt) removeDir(buildDir) createDir(buildDir) + # Extra switches for both compilers, settable per step via `#!FLAGS`. + var extraFlags: seq[string] = @[] + template compileIc(): untyped = - execCmdEx2(compilerPrefix, ["ic", "--hint:Conf:off", "--warnings:off", - "--nimcache:" & nc, "--out:" & bin, "main.nim"], + execCmdEx2(compilerPrefix, @["ic", "--hint:Conf:off", "--warnings:off", + "--nimcache:" & nc, "--out:" & bin] & extraFlags & @["main.nim"], + workingDir = buildDir) + + template compileRef(): untyped = + execCmdEx2(compilerPrefix, @["c", "--hint:Conf:off", "--warnings:off", + "--nimcache:" & ncRef, "--out:" & binRef] & extraFlags & @["main.nim"], workingDir = buildDir) # Parse the source into a flat op list: ("file", name, content) | ("step", attrs, ""). - type OpKind = enum opFile, opStep + type OpKind = enum opFile, opStep, opDelete, opFlags type Op = object kind: OpKind a, b: string @@ -615,6 +641,18 @@ proc runMetamorphicIcTest(r: var TResults; file: string; cat: Category; options: if s.startsWith("#!FILE"): flushFile() curName = s["#!FILE".len .. ^1].strip + elif s.startsWith("#!DELETE"): + # Remove a module from the virtual file system AND from disk. Deleting a + # still-imported file moves no mtime, so nothing in an mtime-keyed build + # re-fires: `nim ic` used to relink a stale binary where `nim c` reports + # `cannot open file`. Untestable until the format could express it. + flushFile() + ops.add Op(kind: opDelete, a: s["#!DELETE".len .. ^1].strip) + elif s.startsWith("#!FLAGS"): + # Change the compiler switches for the following steps. Config changes + # are not files, so an mtime-keyed build cannot see them either. + flushFile() + ops.add Op(kind: opFlags, a: s["#!FLAGS".len .. ^1].strip) elif s.startsWith("#!STEP"): flushFile() ops.add Op(kind: opStep, a: s["#!STEP".len .. ^1].strip) @@ -630,11 +668,21 @@ proc runMetamorphicIcTest(r: var TResults; file: string; cat: Category; options: var prevSnap = initTable[string, string]() var prevBin = "" var stepIdx = 0 + var deleted: seq[string] = @[] try: for o in ops: - if o.kind == opFile: + case o.kind + of opFile: vfs[o.a] = o.b continue + of opDelete: + vfs.del o.a + deleted.add o.a + continue + of opFlags: + extraFlags = o.a.splitWhitespace() + continue + of opStep: discard inc stepIdx let where = "step " & $stepIdx # Parse step attributes. @@ -646,8 +694,36 @@ proc runMetamorphicIcTest(r: var TResults; file: string; cat: Category; options: if c >= 0: attrs[p[0 ..< c].strip] = p[c+1 .. ^1].strip else: attrs[p] = "" + for fn in deleted: + removeFile(buildDir / fn) + deleted.setLen 0 for fn, content in vfs: writeFile(buildDir / fn, content) let (_, cout, ccode) = compileIc() + + # `fails: ` — the build MUST fail, with that text in its output. + # Without this every step had to succeed, so the whole error path was + # untested: a `nim m` that errored still wrote its `.s.bif`, nifmake then + # saw the rule satisfied, and the NEXT run reported success for a program + # that does not compile. + if "fails" in attrs: + if ccode == 0: + mmRaise(reBuildFailed, "a failed build", where & ": `nim ic` unexpectedly succeeded") + let want = attrs["fails"] + if want.len > 0 and want notin cout: + mmRaise(reOutputsDiffer, want, where & ": error text did not contain it:\n" & cout) + # The oracle must reject it too, else the test is asserting an IC-only + # error rather than a real one. + let (_, refOut, refCode) = compileRef() + if refCode == 0: + mmRaise(reBuildFailed, "`nim c` to fail too", + where & ": `nim ic` failed but `nim c` accepted the program:\n" & cout) + if want.len > 0 and want notin refOut: + mmRaise(reOutputsDiffer, want, + where & ": `nim c` failed differently:\n" & refOut) + prevSnap = snapshotDir(nc) + prevBin = "" + continue + if ccode != 0: mmRaise(reBuildFailed, "", where & ": `nim ic` failed:\n" & cout) let (_, rout, rcode) = execCmdEx2(bin.absolutePath, [], workingDir = buildDir) @@ -658,6 +734,22 @@ proc runMetamorphicIcTest(r: var TResults; file: string; cat: Category; options: if rout.strip == want.strip: discard else: mmRaise(reOutputsDiffer, want, where & " output:\n" & rout.strip) + # ORACLE: same sources through the classic backend, same observable + # behaviour. Unlike `expect:` this needs no foresight from the test author — + # it compares everything the program does, not only what someone thought to + # print, which is exactly what a silently-skipped destructor evades. + block oracle: + if "no-oracle" in attrs: break oracle + let (_, refCout, refCcode) = compileRef() + if refCcode != 0: + mmRaise(reBuildFailed, "", where & ": `nim c` (oracle) failed:\n" & refCout) + let (_, refRout, refRcode) = execCmdEx2(binRef.absolutePath, [], + workingDir = buildDir) + if refRout.strip != rout.strip or refRcode != rcode: + mmRaise(reOutputsDiffer, "`nim c` output:\n" & refRout.strip, + where & ": `nim ic` disagrees with `nim c`\n ic (exit " & $rcode & + "):\n" & rout.strip & "\n c (exit " & $refRcode & "):\n" & refRout.strip) + let snap = snapshotDir(nc) let binBytes = stableBinary(bin) if stepIdx > 1: @@ -755,6 +847,12 @@ proc processSingleTest(r: var TResults, cat: Category, options, test: string, ta let target = if cat.string.normalize == "js": targetJS else: targetC targets = {target} doAssert fileExists(test), test & " test does not exist" + # `testament r ` must dispatch metamorphic IC tests the same way + # `testament cat ic` does, otherwise a single-test run tries to parse the + # header as an ordinary spec and rejects it. + if isMetamorphicIcTest(readFile(test)): + runMetamorphicIcTest(r, test, cat, options) + return testSpec r, makeTest(test, options, cat), targets proc isJoinableSpec(spec: TSpec): bool = diff --git a/testament/testament.nim b/testament/testament.nim index 73af243c57..6b199eca37 100644 --- a/testament/testament.nim +++ b/testament/testament.nim @@ -12,7 +12,7 @@ import std/[strutils, pegs, os, osproc, streams, json, parseopt, browsers, terminal, exitprocs, - algorithm, times, intsets, macros] + algorithm, times, intsets, macros, tables] import backend, specs, azure, htmlgen @@ -35,6 +35,12 @@ var simulate = false var optVerbose = false var useMegatest = true var valgrindEnabled = true +var useIc = false + ## `--ic`: compile every C-target test with `nim ic` instead of `nim c`, so the + ## incremental compiler inherits the whole existing corpus (~10k programs with + ## expected output) instead of the handful of tests written for it by hand. + ## Every invariant the `tests/ic` suite checks is IC-against-IC; this is the + ## part that compares IC against the reference backend at scale. proc verboseCmd(cmd: string) = if optVerbose: @@ -58,6 +64,7 @@ Arguments: Options: --print print results to the console --verbose print commands (compiling and running tests) + --ic compile C-target tests with `nim ic` (incremental) --simulate see what tests would be run but don't run them (for debugging) --failing only show failing/ignored tests --targets:"c cpp js objc" run tests for specified targets (default: c) @@ -155,23 +162,146 @@ proc execCmdEx2(command: string, args: openArray[string]; workingDir: string = " if result.exitCode != -1: break close(p) -proc nimcacheDir(filename, options: string, target: TTarget): string = +proc nimcacheDir(filename, options: string, target: TTarget, + extraOptions = ""): string = ## Give each test a private nimcache dir so they don't clobber each other's. - let hashInput = options & $target + ## `extraOptions` (a `matrix:` entry) is part of the key: two matrix variants + ## of one file are two different compilations, and sharing a cache between them + ## means each run invalidates what the previous left. Harmless for the classic + ## backend, which caches only object files, but it makes an incremental cache + ## useless — every variant re-sems the world every time. + let hashInput = options & extraOptions & $target result = "nimcache" / (filename & '_' & hashInput.getMD5) +const icWarmupSource = """ +# Generated by testament for `--ic`. Compiling this once fills a shared IC cache +# with `system` and the stdlib modules the test corpus imports most, so each +# test's own cold build starts from precompiled NIFs instead of re-semming the +# world. Mirrors nimony's hastur `tools/warmup.nim` + `prefillFromWarmup`. +import std/[assertions, macros, strutils, tables, os, typetraits, sequtils, + sugar, math, options, times, json, sets, algorithm, hashes, + strformat, parseutils, streams, unicode] + +proc icWarmupAnchor*(): int = + # Reference a few generic instantiations the corpus leans on so their + # `.c.nif` artifacts are precompiled too, not just the modules' interfaces. + var t = initTable[string, int]() + t["a"] = 1 + var s = @[1, 2, 3] + s.sort() + result = s.len + t.len + "x".repeat(2).len +""" + +var icWarmupCaches: Table[string, string] + ## Compile-config key -> shared warm IC cache (or "" when unavailable). +var buildingIcWarmup = false + proc prepareTestCmd(cmdTemplate, filename, options, nimcache: string, target: TTarget, extraOptions = ""): string = var options = target.defaultOptions & ' ' & options if nimcache.len > 0: options.add(" --nimCache:$#" % nimcache.quoteShell) options.add ' ' & extraOptions + # `--ic` swaps the C target's command; every other target is left alone (the + # incremental compiler has a C backend only). + let targetCmd = + if useIc and target == targetC: "ic" else: targetToCmd[target] # we avoid using `parseCmdLine` which is buggy, refs bug #14343 - result = cmdTemplate % ["target", targetToCmd[target], + result = cmdTemplate % ["target", targetCmd, "options", options, "file", filename.quoteShell, "filedir", filename.getFileDir(), "nim", compilerPrefix] + if useIc and target == targetC: + # Roughly half the corpus overrides the command wholesale (`cmd: "nim c + # --gc:arc $file"`), which neither goes through `$target` nor picks up + # `$options` — so those tests would silently keep using the classic backend + # and share one nimcache. Rewrite the compile verb and give them a private + # cache, which is what makes them incremental at all. + let prefix = compilerPrefix & " c " + if result.startsWith(prefix): + result = compilerPrefix & " ic " & result[prefix.len .. ^1] + if nimcache.len > 0 and "--nimCache:" notin result and "--nimcache:" notin result: + # Must land BEFORE the project file: anything after it is swallowed into + # `config.arguments`, and a non-empty `arguments` without `--run` is a hard + # error ("arguments can only be given if the '--run' option is selected"). + let fileArg = filename.quoteShell + let at = result.find(fileArg) + let switch = "--nimCache:" & nimcache.quoteShell & " " + if at >= 0: result = result[0 ..< at] & switch & result[at .. ^1] + else: result.add " " & switch + +proc icWarmupCache(cmdTemplate, filename, options: string, target: TTarget, + extraOptions: string): string = + ## The shared warm cache for this test's exact compile configuration, built on + ## first use and kept in `nimcache/` across runs. Keyed by the switches AND the + ## test's directory, because both decide what the artifacts contain: the + ## switches through `-d:`/`--mm:` etc., the directory through the `nim.cfg` / + ## `config.nims` it inherits. A cache built under a different configuration + ## would just be invalidated wholesale on first use, which is worse than none. + if buildingIcWarmup: return "" + let dir = filename.getFileDir() + let key = options & extraOptions & $target & dir + if icWarmupCaches.hasKey(key): return icWarmupCaches[key] + result = "nimcache" / ("ic_warmup_" & key.getMD5) + icWarmupCaches[key] = result + if dirExists(result / "ic.version"): return # already built by an earlier run + if fileExists(result / "ic.version"): return + # The warmup must live in the test's own directory so it inherits the same + # config files; a stray `.nim` there is not picked up as a test (testament + # only collects `t*.nim`). The name must be a valid Nim identifier. + let src = dir / "icwarmup_generated.nim" + try: + writeFile(src, icWarmupSource) + except IOError, OSError: + icWarmupCaches[key] = "" + return "" + buildingIcWarmup = true + let cmd = prepareTestCmd(cmdTemplate, src, options, result, target, extraOptions) + let (outp, code) = execCmdEx(cmd) + buildingIcWarmup = false + try: removeFile(src) + except OSError: discard + if code != 0: + # Non-fatal: without a warm cache every test just pays its own cold build. + if optVerbose: echo "ic warmup failed: ", cmd, "\n", outp + icWarmupCaches[key] = "" + return "" + +proc prefillIcCache(warmup, nimcache: string) = + ## Seed a test's empty cache from the shared warm one. Only the artifacts that + ## do NOT depend on which program is being built are copied: the frontend NIFs + ## and cookies, plus the per-module `lower`/`cg` outputs. The `.c`/`.o` are + ## deliberately left out — the merge decision (who owns each emit-everywhere + ## definition) is whole-program, so those get re-rendered for every program + ## anyway and copying them is pure I/O. + ## + ## Mtimes are preserved, and that is load-bearing: nifmake decides staleness by + ## output-mtime > input-mtime, so stamping every prefilled file with "now" + ## would scramble the DAG ordering the warmup established and re-fire the + ## whole graph — exactly what the copy is meant to avoid. + if warmup.len == 0 or not dirExists(warmup): return + if dirExists(nimcache): return # the test already has its own cache + const wanted = [".p.nif", ".p.deps.nif", ".deps.nif", ".s.bif", ".iface.bif", + ".impl.bif", ".edges.bif", ".s.deps.bif", ".t.bif", ".c.nif"] + try: + createDir(nimcache) + for path in walkFiles(warmup / "*"): + let name = path.extractFilename + var take = name == "ic.version" or name == "ic_build_args.txt" + if not take: + for ext in wanted: + if name.endsWith(ext): take = true; break + if not take: continue + let dst = nimcache / name + copyFile(path, dst) + try: setLastModificationTime(dst, getLastModificationTime(path)) + except OSError, IOError: discard + except OSError, IOError: + discard # best effort; a cold build still works proc callNimCompiler(cmdTemplate, filename, options, nimcache: string, target: TTarget, extraOptions = ""): TSpec = + if useIc and target == targetC and nimcache.len > 0 and not buildingIcWarmup: + prefillIcCache(icWarmupCache(cmdTemplate, filename, options, target, extraOptions), + nimcache) result = TSpec(cmd: prepareTestCmd(cmdTemplate, filename, options, nimcache, target, extraOptions)) verboseCmd(result.cmd) @@ -590,7 +720,7 @@ proc targetHelper(r: var TResults, test: TTest, expected: TSpec, extraOptions: s inc count echo "testSpec count: ", count, " expected: ", expected else: - let nimcache = nimcacheDir(test.name, test.options, target) + let nimcache = nimcacheDir(test.name, test.options, target, extraOptions) var testClone = test let target = changeTarget(extraOptions, target) testSpecHelper(r, testClone, expected, target, extraOptions, nimcache) @@ -691,6 +821,7 @@ proc main() = case p.key.normalize of "print": optPrintResults = true of "verbose": optVerbose = true + of "ic": useIc = true of "failing": optFailing = true of "pedantic": discard # deadcode refs https://github.com/nim-lang/Nim/issues/16731 of "targets": diff --git a/tests/ic/tconfig_invalidation.nim b/tests/ic/tconfig_invalidation.nim new file mode 100644 index 0000000000..8f2f81f197 --- /dev/null +++ b/tests/ic/tconfig_invalidation.nim @@ -0,0 +1,34 @@ +discard """ + description: '''IC: changing the compiler switches must invalidate the cache''' +""" + +#? metamorphic + +# nifmake decides staleness from file mtimes and never looks at a rule's command +# line, so `-d:` / `--mm:` / `--opt:` changes re-generated the build file with +# the new switches and re-fired nothing: a silently stale binary built with the +# PREVIOUS configuration. And switches given only on the driver's command line +# never reached the per-module children at all, because they replay the +# project's config files rather than the driver's argv. + +#!FILE cfg.nim +const Mode* {.strdefine.} = "plain" + +proc describe*(): string = + when Mode == "loud": "LOUD" + elif Mode == "quiet": "quiet" + else: "plain" + +#!FILE main.nim +import cfg +echo describe() +#!STEP expect: plain + +#!FLAGS -d:Mode=loud +#!STEP expect: LOUD + +#!FLAGS -d:Mode=quiet +#!STEP expect: quiet + +#!FLAGS +#!STEP expect: plain diff --git a/tests/ic/tdead_when_import.nim b/tests/ic/tdead_when_import.nim new file mode 100644 index 0000000000..477a11197f --- /dev/null +++ b/tests/ic/tdead_when_import.nim @@ -0,0 +1,37 @@ +discard """ + description: '''IC: an import under an undecidable `when` must not be compiled''' +""" + +#? metamorphic + +# `when SomeStrdefineConst == "x": import y` is `cvUnknown` to the dependency +# scanner, which conservatively keeps the edge — right for an edge, but it also +# gave `y` its own `nim m` rule. `nim c` never looks at that file, so a build +# died on a package the user never installed because they never selected that +# backend. Selecting it must still produce the honest error. + +#!FILE needsmissing.nim +import pkg/definitely_not_an_installed_package +proc unreachable*(): string = "never" + +#!FILE guarded.nim +const Backend* {.strdefine.} = "plain" + +when Backend == "fancy": + import ./needsmissing + +proc pick*(): string = + when Backend == "fancy": unreachable() + else: "plain" + +#!FILE main.nim +import guarded +echo pick() +#!STEP expect: plain + +# selecting the branch that really does need the missing package must report it +#!FLAGS -d:Backend=fancy +#!STEP fails: cannot open file + +#!FLAGS +#!STEP expect: plain diff --git a/tests/ic/tdeleted_module.nim b/tests/ic/tdeleted_module.nim new file mode 100644 index 0000000000..c9ad4dcbca --- /dev/null +++ b/tests/ic/tdeleted_module.nim @@ -0,0 +1,26 @@ +discard """ + description: '''IC: deleting a still-imported module must be an error''' +""" + +#? metamorphic + +# Deleting a file moves no mtime, so nothing in an mtime-keyed build re-fires: +# `nim ic` relinked a stale binary while `nim c` reported `cannot open file`. +# The dependency scan is the only part of the pipeline that looks at import +# paths at all, so that is where the vanished module has to be noticed. + +#!FILE helper.nim +proc help*(): string = "helped" + +#!FILE main.nim +import helper +echo help() +#!STEP expect: helped + +#!DELETE helper.nim +#!STEP fails: cannot open file + +# putting it back recovers +#!FILE helper.nim +proc help*(): string = "back" +#!STEP expect: back diff --git a/tests/ic/tdestructor_fidelity.nim b/tests/ic/tdestructor_fidelity.nim new file mode 100644 index 0000000000..12340f0e54 --- /dev/null +++ b/tests/ic/tdestructor_fidelity.nim @@ -0,0 +1,68 @@ +discard """ + description: '''IC vs `nim c`: destructor injection and move analysis must agree''' +""" + +#? metamorphic + +# Two whole classes of IC miscompilation are invisible to any IC-vs-IC check, +# because IC was *consistently* wrong: warm == cold == not what `nim c` does. +# The oracle is what catches them. +# +# * `sfInjectDestructors` lives on the MODULE symbol, which the NIF loader +# rebuilds from scratch — so `genTopLevelStmt` skipped the destructor pass +# entirely and a module-level `block: let h = ...` never ran `=destroy`. +# * `nfFirstWrite`/`nfLastRead` sit on `nkSym` nodes, which serialize as bare +# NIF `SymUse` tokens with nowhere to put node flags — so the frontend's move +# analysis never reached the backend and EVERY first assignment to a +# destructor-bearing local became `=sink` over still-zeroed memory. + +#!FILE res.nim +var log*: seq[string] + +type R* = object + tag*: string + +proc `=destroy`*(r: R) = log.add "d(" & r.tag & ")" +proc `=copy`*(d: var R, s: R) = (log.add "c(" & s.tag & ")"; d.tag = s.tag) + +proc mk*(t: string): R = R(tag: t) +proc mkVia*(t: string): R = (result = R(tag: t)) +proc consume*(r: sink R): string = "u:" & r.tag + +#!FILE main.nim +import res + +# in a proc: worked before +proc inProc() = + let a = mk("proc") + discard a +inProc() + +# module top level: the pass was skipped wholesale +block: + let t = mk("toplevel") + discard t + +for i in 0 .. 1: + let l = mk("loop" & $i) + discard l + +# every `result` shape: each must construct in place, not `=sink` over zeroes +block: + let x = mk("direct") + let y = mkVia("via") + discard x + discard y + +# last read is a move, a re-read is a copy +proc moves(): string = + var m = mk("moved") + result = consume(m) +proc copies(): string = + var k = mk("kept") + result = consume(k) & "/" & k.tag +discard moves() +discard copies() + +echo log +#!STEP expect: @["d(proc)", "d(toplevel)", "d(loop0)", "d(loop1)", "d(via)", "d(direct)", "d(moved)", "c(kept)", "d(kept)", "d(kept)"] diff --git a/tests/ic/tdiscovered_import.nim b/tests/ic/tdiscovered_import.nim new file mode 100644 index 0000000000..e7dcd4d496 --- /dev/null +++ b/tests/ic/tdiscovered_import.nim @@ -0,0 +1,38 @@ +discard """ + description: '''IC: a macro-generated import stays in the graph across runs''' +""" + +#? metamorphic + +# The static scanner cannot see `parseStmt("import dyn")`. The discovery +# fixpoint recovers it — but only ran AFTER a failure, and the graph is +# re-derived statically on every run, so on a warm build the discovered module +# had no nifler/`nim m` rule at all: editing it changed nothing, forever. + +#!FILE dyn.nim +proc hidden*(): string = "first" + +#!FILE gen.nim +import std/macros + +macro generatedImport(): untyped = + parseStmt("import dyn") + +generatedImport() + +proc reveal*(): string = hidden() + +#!FILE main.nim +import gen +echo reveal() +#!STEP expect: first + +# the warm build must see this edit +#!FILE dyn.nim +proc hidden*(): string = "second" +#!STEP expect: second + +# and again, to prove it is not a one-shot recovery +#!FILE dyn.nim +proc hidden*(): string = "third" +#!STEP expect: third diff --git a/tests/ic/terror_recovery.nim b/tests/ic/terror_recovery.nim new file mode 100644 index 0000000000..c38b1f5034 --- /dev/null +++ b/tests/ic/terror_recovery.nim @@ -0,0 +1,32 @@ +discard """ + description: '''IC: a failed `nim m` must not poison the cache''' +""" + +#? metamorphic + +# A `nim m` that errored still wrote its `.s.bif` and cookie sidecars. nifmake +# then saw the rule satisfied (outputs newer than inputs) and the NEXT run +# reported success for a program that does not compile — linking a binary +# generated from error-bearing AST, or crashing codegen outright. Expressing +# this needs a step that is allowed to FAIL and a following step that recovers. + +#!FILE dep.nim +proc value*(): int = 41 + +#!FILE main.nim +import dep +echo value() + 1 +#!STEP expect: 42 + +# introduce a real error +#!FILE dep.nim +proc value*(): int = undefinedThing() + 1 +#!STEP fails: undeclared identifier: 'undefinedThing' + +# ... and again: the second run must NOT decide the rule is up to date. +#!STEP fails: undeclared identifier: 'undefinedThing' + +# fixing it must rebuild rather than serve the poisoned artifact +#!FILE dep.nim +proc value*(): int = 100 +#!STEP expect: 101