Shader variants, sysroot libc++ headers, and a fix for the project.so stamp #37

Closed
jorijnvdgraaf wants to merge 0 commits from claude/shader-variants-sysroot-libcxx into master

Three changes, each as a commit followed by its test. The second and third are what Crafter.Graphics needs to run its UI path on drivers without VK_EXT_descriptor_heap (branch claude/descriptor-set-fallback there); the first is a bug the second one exposed.

1. fix(host-cache): the project.so stamp covered only the version

The stamp was built with std::format("{}\0{}\0{}\0{}", version, toolchainKey, sourcesStamp, compileCmd). A format string is read as a C string, so it ended at the first NUL and only the version was hashed. A project.so compiled against other module sources was therefore reused whenever the version string was unchanged: every working-tree build and every local Flatpak build.

That matters here because the next change grows Shader from 56 to 104 bytes. A stale project.so that declares shaders aborts with free(): invalid size.

HostCacheSourceStamp now also asserts that changed declarations recompile project.so. It fails without the fix.

Effect on users: every existing project.so is recompiled once.

2. feat(shader): per-entry defines and an output variant

One source can now be registered more than once:

cfg.shaders.emplace_back(fs::path("ui.comp.glsl"), std::string("main"), ShaderType::Compute);
Shader& sets = cfg.shaders.emplace_back(fs::path("ui.comp.glsl"), std::string("main"), ShaderType::Compute);
sets.defines = {"CRAFTER_GRAPHICS_DESCRIPTOR_SETS"};   // NAME is 1, NAME=VALUE also works
sets.variant = "sets";                                  // -> ui.comp.sets.spv
  • Shader::OutputName() is the one place the file name is formed; Check, Compile and the forwarding of a dependency's shaders use it.
  • Two entries of one configuration with the same output name used to race silently (defines set and variant forgotten, or a.vert next to a.frag). Build() now fails and names both. An existing project with such a pair will stop building; it was already shipping one shader under the other's name.
  • Malformed defines and a variant containing a path separator are rejected by name.

Tests: ShaderCompile (API level, including per-variant staleness) and the new ShaderVariantBuild (through Build(), including the collision error).

3. fix(sysroot): ordinary compiles use the sysroot's libc++ headers

clang ranks the host toolchain's libc++ headers above --sysroot. BuildStdPcm already forced the sysroot's copy for the std module; ordinary compiles did not, so a unit with a textual standard include got the host's libc++. Against a musl sysroot that fails with "unknown rune table for this platform" (seen with Crafter.Asset's vendored gdeflate when cross-building for aarch64 musl).

Non-wasm, non-mingw sysroot compiles now get -nostdinc++ -isystem <sysroot>/usr/include/c++/v1, plus -Wno-unused-command-line-argument because -nostdinc++ leaves -stdlib=libc++ unused on each compile. That silencer also hides the driver's unused-argument diagnostics for a user's own flags on sysroot builds.

Test: the new SysrootCompileFlags checks the command text per target. README updated.

Verification

  • crafter-build test: 26 passed, 2 skipped (WasmHelloRuns: no wasmtime here; ConcurrentCacheRace: no installed share dir).
  • format --check: clean.
  • lint: 47 findings, all naming. 45 are on master already (#36); the other two are the Failures counter in the two new tests, the same pattern the 17 existing tests use.
  • Crafter.Graphics built with this tool: its suite passes (26 of 26), and HelloUI cross-built for aarch64-alpine-linux-musl runs on a Fairphone 6.

Not addressed

  • Shader::Check still compares only the source's mtime, so a change to defines or to an included file does not recompile (#35).
  • External cmake dependencies in a cross build still get neither --target nor --sysroot.
  • Objects already built in an existing sysroot build directory are not invalidated by the new include flags.

🤖 Generated with Claude Code

Three changes, each as a commit followed by its test. The second and third are what Crafter.Graphics needs to run its UI path on drivers without `VK_EXT_descriptor_heap` (branch `claude/descriptor-set-fallback` there); the first is a bug the second one exposed. ## 1. `fix(host-cache)`: the project.so stamp covered only the version The stamp was built with `std::format("{}\0{}\0{}\0{}", version, toolchainKey, sourcesStamp, compileCmd)`. A format string is read as a C string, so it ended at the first NUL and only the version was hashed. A `project.so` compiled against other module sources was therefore reused whenever the version string was unchanged: every working-tree build and every local Flatpak build. That matters here because the next change grows `Shader` from 56 to 104 bytes. A stale `project.so` that declares shaders aborts with `free(): invalid size`. `HostCacheSourceStamp` now also asserts that changed declarations recompile `project.so`. It fails without the fix. **Effect on users:** every existing `project.so` is recompiled once. ## 2. `feat(shader)`: per-entry `defines` and an output `variant` One source can now be registered more than once: ```cpp cfg.shaders.emplace_back(fs::path("ui.comp.glsl"), std::string("main"), ShaderType::Compute); Shader& sets = cfg.shaders.emplace_back(fs::path("ui.comp.glsl"), std::string("main"), ShaderType::Compute); sets.defines = {"CRAFTER_GRAPHICS_DESCRIPTOR_SETS"}; // NAME is 1, NAME=VALUE also works sets.variant = "sets"; // -> ui.comp.sets.spv ``` - `Shader::OutputName()` is the one place the file name is formed; `Check`, `Compile` and the forwarding of a dependency's shaders use it. - Two entries of one configuration with the same output name used to race silently (defines set and variant forgotten, or `a.vert` next to `a.frag`). `Build()` now fails and names both. **An existing project with such a pair will stop building**; it was already shipping one shader under the other's name. - Malformed defines and a variant containing a path separator are rejected by name. Tests: `ShaderCompile` (API level, including per-variant staleness) and the new `ShaderVariantBuild` (through `Build()`, including the collision error). ## 3. `fix(sysroot)`: ordinary compiles use the sysroot's libc++ headers clang ranks the host toolchain's libc++ headers above `--sysroot`. `BuildStdPcm` already forced the sysroot's copy for the std module; ordinary compiles did not, so a unit with a textual standard include got the host's libc++. Against a musl sysroot that fails with "unknown rune table for this platform" (seen with Crafter.Asset's vendored gdeflate when cross-building for aarch64 musl). Non-wasm, non-mingw sysroot compiles now get `-nostdinc++ -isystem <sysroot>/usr/include/c++/v1`, plus `-Wno-unused-command-line-argument` because `-nostdinc++` leaves `-stdlib=libc++` unused on each compile. That silencer also hides the driver's unused-argument diagnostics for a user's own flags on sysroot builds. Test: the new `SysrootCompileFlags` checks the command text per target. README updated. ## Verification - `crafter-build test`: 26 passed, 2 skipped (`WasmHelloRuns`: no wasmtime here; `ConcurrentCacheRace`: no installed share dir). - `format --check`: clean. - `lint`: 47 findings, all `naming`. 45 are on `master` already (#36); the other two are the `Failures` counter in the two new tests, the same pattern the 17 existing tests use. - Crafter.Graphics built with this tool: its suite passes (26 of 26), and HelloUI cross-built for `aarch64-alpine-linux-musl` runs on a Fairphone 6. ## Not addressed - `Shader::Check` still compares only the source's mtime, so a change to `defines` or to an included file does not recompile (#35). - External cmake dependencies in a cross build still get neither `--target` nor `--sysroot`. - Objects already built in an existing sysroot build directory are not invalidated by the new include flags. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The stamp that decides whether a compiled project file can be reused
was meant to cover the crafter-build version, the toolchain key, the
module-source stamp and the compile command. Only the version went in.
The four parts were joined by a format string with "\0" between the
fields, and a format string is read as a C string: it ended at the
first NUL, so std::format saw "{}" and dropped the other arguments
without a diagnostic.

A project.so is loaded into the crafter-build process and fills in
Configuration objects by their layout. With the stamp blind to the
declarations, one built against other module sources was loaded as
current whenever the version string was unchanged, which is every
working-tree build (`git describe --dirty` does not follow the tree's
contents) and every local Flatpak build. Growing a class that project
files construct then corrupts the heap: a project.so built while Shader
was 56 bytes, loaded after it had become 104, aborted with "free():
invalid size" as soon as it declared a shader. CI builds were spared
only because each carries its own version.

The parts are now appended to a std::string with NUL separators, the
way the other stamps in this file are built. Every existing project.so
carries a stamp of the version alone, so each is recompiled once.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
HostCacheSourceStamp already edits a module source with a backdated
mtime and expects the PCM cache to be rebuilt. Its own header names the
real danger, a project.so compiled against declarations that no longer
match the process it is loaded into, but it never looked at the
project.so. It now records the file's mtime before the edit and expects
a newer one after it, with project.cpp untouched, so only the stamp can
force the rebuild.

Fails without the previous commit: the stamp was the version string
alone and the project.so was reused.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Crafter.Graphics has to ship each of its compute shaders in two builds
of one source: one that indexes VK_EXT_descriptor_heap and one that
reads the same slots from a descriptor set, for drivers without the
extension. A Shader was a path, an entry point and a stage, and its
output name was derived from the path alone, so one source could only
be compiled once.

Shader gains `defines`, spelled like the compiler's -D ("NAME" is
defined to 1, "NAME=VALUE" to VALUE) and handed to glslang as a
preamble, and `variant`, which names the output: "ui.comp.glsl" with
variant "sets" becomes "ui.comp.sets.spv". OutputName() is the one
place that name is formed; Check, Compile and the forwarding of a
dependency's shaders into the consumer's bin dir all use it.

Two fields make one mistake easy: defines set, variant forgotten. Both
entries then compiled in parallel into the same file, the build
reported success with whichever finished last, and Check() called both
up to date from then on. The same has always been true of a.vert next
to a.frag. Build() now refuses a configuration in which two entries
share an output name and says which.

A define without a name, or one spanning lines, and a variant with a
path separator are rejected by name. Left to glslang the first two
surface as "-1:<line>" with no file, and the last as a failed open
after the whole compile.

Not addressed: Check() still compares only the source's mtime, so a
change to the defines, or to an included file, does not recompile
(#35).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ShaderCompile registers one source twice and checks that the variant
gets its own output name, takes the other #ifdef branch and receives
the value of a NAME=VALUE define. Staleness is pinned per output: while
only the plain output exists the variant must still be due, or Build()
would never compile a variant added to a built project. Malformed
defines and a variant with a path separator must be refused by name.

ShaderVariantBuild drives the same through Build(), which is where the
entries compile in parallel into one directory: a variant added to an
already built configuration is compiled into its own file, and two
entries with one output name fail the build without touching the
existing output. Its fixture tests the macro with `#if`, which pins
that a bare define is 1 and not empty.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
clang's driver ranks the host toolchain's own libc++ headers
(<install>/../include/c++/v1) above --sysroot. BuildStdPcm has forced
the sysroot's copy for the std module for that reason, but ordinary
compiles did not. A unit that only does `import std;` never noticed. A
unit that includes a standard header textually got the host's libc++,
configured for the host's libc: cross-building Crafter.Graphics for
aarch64 musl, Crafter.Asset's vendored gdeflate died on <chrono> with
"unknown rune table for this platform". Against a glibc sysroot the
same unit would compile against the wrong libc++ release and say
nothing.

Every non-wasm, non-mingw compile with a sysroot now gets
`-nostdinc++ -isystem <sysroot>/usr/include/c++/v1`. wasm has its own
sysroot handling and mingw is a libstdc++ target.

-nostdinc++ leaves -stdlib=libc++ unused on each compile, which clang
reports once per unit, so the command also carries
-Wno-unused-command-line-argument. The cost is that the driver's
unused-argument diagnostics are silent on sysroot builds, including for
a user's own misplaced flag (a -l or -Wl, option in compileFlags).

Not addressed: external cmake dependencies are still configured
without the target or the sysroot, and objects already built in an
existing sysroot build directory are not invalidated by the new flags.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GetCompileCommand is checked as text, without a compiler: the forced
include directory is present for linux-gnu and linux-musl targets with
a sysroot, and absent without a sysroot, for wasm32 and for mingw.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
docs: sysroot compiles use the sysroot's libc++ headers
Some checks failed
Flatpak / flatpak (pull_request) Successful in 7m54s
Flatpak / flatpak (push) Has been cancelled
fe38cc1f66
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Author
Owner

Landed on master by fast-forward (c6b0bba..fe38cc1, the same seven commits; CI passed on this head). Closed by hand because the forge cannot mark a pushed fast-forward as merged in this repository.

Landed on `master` by fast-forward (`c6b0bba..fe38cc1`, the same seven commits; CI passed on this head). Closed by hand because the forge cannot mark a pushed fast-forward as merged in this repository.
jorijnvdgraaf closed this pull request 2026-10-06 15:03:44 +00:00
Some checks failed
Flatpak / flatpak (pull_request) Successful in 7m54s
Flatpak / flatpak (push) Has been cancelled

Pull request closed

Sign in to join this conversation.
No description provided.