Fix silent stale-build corruption on module interface changes #28

Merged

catbot merged 3 commits from claude/issue-27 into master

2026-07-30 17:45:19 +00:00

catbot commented

2026-07-30 17:45:14 +00:00

Member

Root cause

GetInterfacesAndImplementations scans a TU's import X; statements when the source is declared. A name matching neither a module in the Configuration nor one reachable through dependencies was dropped on the floor — leaving that TU with no staleness edge to the interface it consumes.

dependencies is routinely assigned after the scan. AddTest is exactly that shape: it resolves tests/<name>/main.cpp and only then returns a builder whose .Dependencies() supplies the library (Crafter.Build-Test.cpp:439 vs :484). So every test that imported a dependency's module had no edge to it at all. A member added to the interface rebuilt the library, relinked the test, and kept the test's object exactly as it was — which is why the original report saw the corruption in test executables specifically, and why ShouldInteropCurlHTTP1 passed: it drives the listener with curl and never constructs the mismatched object.

Nothing fails to link when a member is added — every signature and mangled name is unchanged — so there was no diagnostic of any kind.

Reproduced first, in the reporter's shape

A library whose interface gains a std::string, plus a consumer declared in the AddTest order. Before the fix the second build printed Linking SizeCheck with no Compiling main.cpp, and the binary reported test sees sizeof(Widget)=72 lib says=96. The same edit after the fix recompiles the consumer and the sizes agree.

The fix

Unresolved names are kept on the partition/implementation as pendingImports, and Configuration::ResolvePendingImports() retries them against the dependency DAG as it stands. Build() calls it immediately before comparing mtimes, so the window closes for every caller rather than only those that declare in the right order; TestBuilder::Dependencies also calls it so the Configuration is coherent for anyone inspecting it beforehand.

On the issue's suggested directions: this takes option 1's spirit at the point where it costs nothing — the edge is established before the answer is computed. It does not add option 2's post-hoc consistency check, because after this the tracking has the information it needs and a check would only restate it. Option 3 is included.

Also in this PR — the same theme elsewhere

Project args are now part of the variant identity. Flags ApplyStandardArgs doesn't itself interpret typically decide what gets compiled or bundled (--no-webgpu dropping entries from cfg.files). Both settings previously shared one bin dir and interleaved their outputs, leaving a bundle matching neither. Sorted and deduplicated so flag order doesn't split the cache; inherited by test Configurations. Verified: crafter-build and crafter-build -- --no-webgpu now land in separate directories for both the exe and the lib.

The host PCM cache is invalidated by source content, not mtime. <cache>/crafter.build/<target>-<march>/ is shared by every crafter-build on the machine, and an mtime cannot distinguish "this PCM is newer than my source" from "this PCM was built from different sources that happen to be newer". Two checkouts of different versions silently compiled their project.cpp against each other's declarations. I hit this for real while establishing the baseline for this PR — a master worktree poisoned the cache for the main checkout, and the resulting crashes cost a while to attribute. Content stamping also covers what a per-file mtime never could: the cached PCMs import each other, so a change to :Interface invalidates :Clang's PCM with Crafter.Build-Clang.cppm untouched.

crafter-build clean removes the project's bin/ and build/ trees. It deliberately does not load project.cpp — cleaning is most often reached when something is already wrong, and a clean that first needs the project to compile is useless exactly then.

Tests

crafter-build test — 18 passed, 0 failed.

Three new tests, each verified to fail when its fix is reverted:

tests/IncrementalInterfaceChange — the interface member change end to end. Asserts the pending import is recorded, that ResolvePendingImports places it and is idempotent, that AddTest(...).Dependencies() wires it up, and — without calling ResolvePendingImports itself, so the guarantee under test is Build()'s — that the consumer object is recompiled and the binary agrees on the layout. With the fix's call sites neutered it reports 4 failures including second pass agrees on the layout (exit=1 output='24 48').
tests/HostCacheSourceStamp — asserts the cache notices a change the mtime rule provably cannot see (different content, deliberately backdated), and that unchanged sources are still a no-op.
tests/CleanProject — clean works against an uncompilable project, is idempotent, leaves unrelated files alone, and resolves relative to the project file rather than the cwd.

crafter-build lint and crafter-build format --check are clean (41 files, 15 rules).

The issue's three-step reproduction now gives identical results for the incremental and clean builds.

Two environment notes, not repo changes

Worth recording, since a human reproducing this locally may not need them:

llvm-ar is absent from this container, so the LTO static-archive step fails. I shimmed it to GNU ar (which here has the LLVM plugin) to run the suite. Unrelated to this PR, but it will block crafter-build test on any static-lib project in this image.
The container's installed /usr/local/share/crafter-build predates the Lint module, so tests/ConcurrentCacheRace — which hardcodes that path — fails on unmodified master too. Pointing CRAFTER_BUILD_HOME at this checkout's share/crafter-build gives it a complete module set; with that it passes both here and on master.

No UI surface changed, so no screenshots.

Resolves #27

Adding a data member to a class in a module interface did not rebuild every object compiled against the old layout. The build succeeded, and the resulting mixed-layout binary surfaced later as a SIGSEGV in a destructor. ## Root cause `GetInterfacesAndImplementations` scans a TU's `import X;` statements when the source is declared. A name matching neither a module in the Configuration nor one reachable through `dependencies` was **dropped on the floor** — leaving that TU with no staleness edge to the interface it consumes. `dependencies` is routinely assigned *after* the scan. `AddTest` is exactly that shape: it resolves `tests/<name>/main.cpp` and only then returns a builder whose `.Dependencies()` supplies the library (Crafter.Build-Test.cpp:439 vs :484). So every test that imported a dependency's module had no edge to it at all. A member added to the interface rebuilt the library, relinked the test, and kept the test's object exactly as it was — which is why the original report saw the corruption in test executables specifically, and why `ShouldInteropCurlHTTP1` passed: it drives the listener with `curl` and never constructs the mismatched object. Nothing fails to link when a member is added — every signature and mangled name is unchanged — so there was no diagnostic of any kind. ### Reproduced first, in the reporter's shape A library whose interface gains a `std::string`, plus a consumer declared in the `AddTest` order. Before the fix the second build printed `Linking SizeCheck` with no `Compiling main.cpp`, and the binary reported `test sees sizeof(Widget)=72 lib says=96`. The same edit after the fix recompiles the consumer and the sizes agree. ## The fix Unresolved names are kept on the partition/implementation as `pendingImports`, and `Configuration::ResolvePendingImports()` retries them against the dependency DAG as it stands. `Build()` calls it immediately before comparing mtimes, so the window closes for **every** caller rather than only those that declare in the right order; `TestBuilder::Dependencies` also calls it so the Configuration is coherent for anyone inspecting it beforehand. On the issue's suggested directions: this takes option 1's spirit at the point where it costs nothing — the edge is established before the answer is computed. It does not add option 2's post-hoc consistency check, because after this the tracking has the information it needs and a check would only restate it. Option 3 is included. ## Also in this PR — the same theme elsewhere **Project args are now part of the variant identity.** Flags `ApplyStandardArgs` doesn't itself interpret typically decide what gets compiled or bundled (`--no-webgpu` dropping entries from `cfg.files`). Both settings previously shared one bin dir and interleaved their outputs, leaving a bundle matching neither. Sorted and deduplicated so flag order doesn't split the cache; inherited by test Configurations. Verified: `crafter-build` and `crafter-build -- --no-webgpu` now land in separate directories for both the exe and the lib. **The host PCM cache is invalidated by source content, not mtime.** `<cache>/crafter.build/<target>-<march>/` is shared by every crafter-build on the machine, and an mtime cannot distinguish "this PCM is newer than my source" from "this PCM was built from *different* sources that happen to be newer". Two checkouts of different versions silently compiled their `project.cpp` against each other's declarations. I hit this for real while establishing the baseline for this PR — a `master` worktree poisoned the cache for the main checkout, and the resulting crashes cost a while to attribute. Content stamping also covers what a per-file mtime never could: the cached PCMs import each other, so a change to `:Interface` invalidates `:Clang`'s PCM with `Crafter.Build-Clang.cppm` untouched. **`crafter-build clean`** removes the project's `bin/` and `build/` trees. It deliberately does not load `project.cpp` — cleaning is most often reached when something is already wrong, and a clean that first needs the project to compile is useless exactly then. ## Tests `crafter-build test` — **18 passed, 0 failed**. Three new tests, each verified to fail when its fix is reverted: - `tests/IncrementalInterfaceChange` — the interface member change end to end. Asserts the pending import is recorded, that `ResolvePendingImports` places it and is idempotent, that `AddTest(...).Dependencies()` wires it up, and — without calling `ResolvePendingImports` itself, so the guarantee under test is `Build()`'s — that the consumer object is recompiled and the binary agrees on the layout. With the fix's call sites neutered it reports 4 failures including `second pass agrees on the layout (exit=1 output='24 48')`. - `tests/HostCacheSourceStamp` — asserts the cache notices a change the mtime rule provably cannot see (different content, deliberately backdated), and that unchanged sources are still a no-op. - `tests/CleanProject` — clean works against an uncompilable project, is idempotent, leaves unrelated files alone, and resolves relative to the project file rather than the cwd. `crafter-build lint` and `crafter-build format --check` are clean (41 files, 15 rules). The issue's three-step reproduction now gives identical results for the incremental and clean builds. ## Two environment notes, not repo changes Worth recording, since a human reproducing this locally may not need them: - `llvm-ar` is absent from this container, so the LTO static-archive step fails. I shimmed it to GNU `ar` (which here has the LLVM plugin) to run the suite. Unrelated to this PR, but it will block `crafter-build test` on any static-lib project in this image. - The container's installed `/usr/local/share/crafter-build` predates the Lint module, so `tests/ConcurrentCacheRace` — which hardcodes that path — fails on unmodified `master` too. Pointing `CRAFTER_BUILD_HOME` at this checkout's `share/crafter-build` gives it a complete module set; with that it passes both here and on `master`. No UI surface changed, so no screenshots. Resolves #27

catbot added 3 commits

2026-07-30 17:45:14 +00:00

fix: re-resolve module imports before checking staleness 13697cd026

Adding a data member to a class in a module interface did not rebuild every
object compiled against the old layout. The build succeeded with no error or
warning and the resulting binary mixed both layouts, surfacing later as a
SIGSEGV in a destructor.

GetInterfacesAndImplementations scans a TU's `import X;` statements when the
source is declared. An import that matches neither a module in the
Configuration nor one reachable through `dependencies` was dropped on the
floor, leaving that TU with no staleness edge to the interface it consumes.
`dependencies` is frequently assigned *after* the scan — AddTest does exactly
that, resolving tests/<name>/main.cpp and only then returning a builder whose
.Dependencies() supplies the library — so consumers of a dependency's modules
routinely carried no edge at all. A layout change then rebuilt the library,
relinked the consumer, and kept the consumer's object as it was.

Unresolved names are now remembered on the partition/implementation as
pendingImports, and Configuration::ResolvePendingImports retries them against
the dependency DAG as it stands. Build() calls it immediately before comparing
mtimes, which closes the window for every caller rather than only the ones that
declare in the right order; TestBuilder::Dependencies also calls it so the
Configuration is coherent for anyone inspecting it before the build.

Resolves #27

fix: key the host PCM cache on source content, add clean, hash project args e8fde57582

Three follow-ons to the stale-build report, all cases of an identity not
capturing something that changes the output.

The host PCM cache under <cache>/crafter.build/<target>-<march>/ is shared by
every crafter-build on the machine, and freshness was a per-file mtime
comparison. That cannot tell "this PCM is newer than my source" from "this PCM
was built from different sources that happen to be newer", so a package install
and a working checkout — or two checkouts of different versions — silently
compiled their project.cpp against each other's declarations. Invalidation now
keys on a stamp over the bytes of every module source, which also covers the
case one file's mtime never could: the cached PCMs import each other, so a
change to :Interface invalidates :Clang's PCM with Crafter.Build-Clang.cppm
untouched.

Project args ApplyStandardArgs does not itself interpret are now folded into
VariantId. Such a flag typically decides what gets compiled or bundled — the
report's example is --no-webgpu dropping entries from cfg.files — and without it
both settings shared one bin dir and interleaved their outputs there, leaving a
bundle matching neither. Sorted and deduplicated so flag order doesn't split the
cache, and inherited by test Configurations.

`crafter-build clean` removes the project's bin/ and build/ trees. It
deliberately does not load project.cpp: cleaning is most often reached when
something is already wrong, and a clean that first needs the project to compile
is useless exactly then.

docs: incrementality, variant identity and clean in the README

CI / build-test-release (pull_request) Failing after 6m2s

Details

2fbcb6fbf3

catbot merged commit e8f7bb12a8 into master

2026-07-30 17:45:19 +00:00

catbot deleted branch claude/issue-27

2026-07-30 17:45:19 +00:00

catbot referenced this pull request from a commit

2026-07-30 17:45:20 +00:00

Merge pull request 'Fix silent stale-build corruption on module interface changes' (#28) from claude/issue-27 into master