vulkan-shaders-gen : fail the build when a shader fails to compile - #24450
Conversation
vulkan-shaders-gen did not detect shader-compile subprocess failures, so a broken libggml-vulkan could be produced while the build reported success and the breakage only surfaced at run time. execute_command() discarded the child exit code (POSIX waitpid passed nullptr for status; the Windows branch never called GetExitCodeProcess) and string_to_spv decided success only from whether stderr was empty, so a non-zero exit with empty stderr, or a subprocess that failed to launch, was treated as success. Return the child exit code from execute_command() (WEXITSTATUS on POSIX, GetExitCodeProcess on Windows), treat a non-zero exit or non-empty stderr or a launch exception as a failure, and record it in an atomic flag. main() checks the flag after process_shaders() and returns EXIT_FAILURE before writing the output files, so the build stops instead of emitting a broken backend. Fixes ggml-org#24393 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
|
Hi @liminfei-amd, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
Closing this to comply with the new-contributor one-open-PR limit (keeping #24237 open). I also acknowledge the AI-assistance disclosure point — I will revisit this build-robustness fix as a clearly human-authored change later. Thanks! |
|
You can keep it open, the PRs are aimed at different backends and you're just waiting for feedback. The AI rule is also more about disclosure and that you really understand what you are submitting, not about rewriting it manually. The only thing you should really do is use the actual PR template. |
0cc4m
left a comment
There was a problem hiding this comment.
It works as intended on Linux. @jeffbolznv Can you check on Windows?
| return WIFEXITED(status) ? WEXITSTATUS(status) : -1; | ||
| } | ||
| #endif | ||
| return -1; |
| std::cerr << part << " "; | ||
| } | ||
| std::cerr << "\n\n" << stderr_str << std::endl; | ||
| compile_failed.store(true); |
There was a problem hiding this comment.
It's not necessary to use .store/.load, you can just access it as if it were a normal bool.
Yes, it works on windows too. |
…le return Address review feedback on ggml-org#24450: - Access the std::atomic<bool> compile_failed directly (= / implicit bool) instead of .store()/.load(); the flag stays atomic because the worker threads in process_shaders() set it concurrently. - Remove the unreachable trailing return -1 in execute_command(): on POSIX the child _exit()s after execvp and the parent returns (fork()<0 throws); on Windows the block returns the exit code. Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
|
Thanks for the review! Addressed both points in a follow-up commit:
Rebuilt clean (no new warnings, no |
* vulkan-shaders-gen: fail the build when a shader fails to compile vulkan-shaders-gen did not detect shader-compile subprocess failures, so a broken libggml-vulkan could be produced while the build reported success and the breakage only surfaced at run time. execute_command() discarded the child exit code (POSIX waitpid passed nullptr for status; the Windows branch never called GetExitCodeProcess) and string_to_spv decided success only from whether stderr was empty, so a non-zero exit with empty stderr, or a subprocess that failed to launch, was treated as success. Return the child exit code from execute_command() (WEXITSTATUS on POSIX, GetExitCodeProcess on Windows), treat a non-zero exit or non-empty stderr or a launch exception as a failure, and record it in an atomic flag. main() checks the flag after process_shaders() and returns EXIT_FAILURE before writing the output files, so the build stops instead of emitting a broken backend. Fixes ggml-org#24393 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> * vulkan-shaders-gen: simplify compile_failed access and drop unreachable return Address review feedback on ggml-org#24450: - Access the std::atomic<bool> compile_failed directly (= / implicit bool) instead of .store()/.load(); the flag stays atomic because the worker threads in process_shaders() set it concurrently. - Remove the unreachable trailing return -1 in execute_command(): on POSIX the child _exit()s after execvp and the parent returns (fork()<0 throws); on Windows the block returns the exit code. Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> --------- Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
* vulkan-shaders-gen: fail the build when a shader fails to compile vulkan-shaders-gen did not detect shader-compile subprocess failures, so a broken libggml-vulkan could be produced while the build reported success and the breakage only surfaced at run time. execute_command() discarded the child exit code (POSIX waitpid passed nullptr for status; the Windows branch never called GetExitCodeProcess) and string_to_spv decided success only from whether stderr was empty, so a non-zero exit with empty stderr, or a subprocess that failed to launch, was treated as success. Return the child exit code from execute_command() (WEXITSTATUS on POSIX, GetExitCodeProcess on Windows), treat a non-zero exit or non-empty stderr or a launch exception as a failure, and record it in an atomic flag. main() checks the flag after process_shaders() and returns EXIT_FAILURE before writing the output files, so the build stops instead of emitting a broken backend. Fixes ggml-org#24393 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> * vulkan-shaders-gen: simplify compile_failed access and drop unreachable return Address review feedback on ggml-org#24450: - Access the std::atomic<bool> compile_failed directly (= / implicit bool) instead of .store()/.load(); the flag stays atomic because the worker threads in process_shaders() set it concurrently. - Remove the unreachable trailing return -1 in execute_command(): on POSIX the child _exit()s after execvp and the parent returns (fork()<0 throws); on Windows the block returns the exit code. Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> --------- Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
* vulkan-shaders-gen: fail the build when a shader fails to compile vulkan-shaders-gen did not detect shader-compile subprocess failures, so a broken libggml-vulkan could be produced while the build reported success and the breakage only surfaced at run time. execute_command() discarded the child exit code (POSIX waitpid passed nullptr for status; the Windows branch never called GetExitCodeProcess) and string_to_spv decided success only from whether stderr was empty, so a non-zero exit with empty stderr, or a subprocess that failed to launch, was treated as success. Return the child exit code from execute_command() (WEXITSTATUS on POSIX, GetExitCodeProcess on Windows), treat a non-zero exit or non-empty stderr or a launch exception as a failure, and record it in an atomic flag. main() checks the flag after process_shaders() and returns EXIT_FAILURE before writing the output files, so the build stops instead of emitting a broken backend. Fixes ggml-org#24393 Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> * vulkan-shaders-gen: simplify compile_failed access and drop unreachable return Address review feedback on ggml-org#24450: - Access the std::atomic<bool> compile_failed directly (= / implicit bool) instead of .store()/.load(); the flag stays atomic because the worker threads in process_shaders() set it concurrently. - Remove the unreachable trailing return -1 in execute_command(): on POSIX the child _exit()s after execvp and the parent returns (fork()<0 throws); on Windows the block returns the exit code. Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com> --------- Signed-off-by: liminfei-amd <91481003+liminfei-amd@users.noreply.github.com>
voxtype-vulkan fails to link on CI with hundreds of undefined matmul_id_*
and matmul_*_cm1_* symbols, breaking the Home martin@{bane,ravi,skrye,tanis,
zannah} jobs and blocking the all-builds-green auto-merge gate.
The root cause is upstream, in ggml's vulkan-shaders-gen, vendored via
whisper-rs-sys 0.15.0 inside the voxtype flake input. The generator
discards the glslc child exit status and only inspects stderr, so a
child that dies without printing anything counts as a success. The
shader is then absent from the generated source while the header still
declares it, and the breakage only surfaces at link time. fork() also
fails transiently with EAGAIN or ENOMEM on small CI runners, which
takes the same silent path. That is why the identical derivation builds
on a workstation but not on a GitHub runner, and why three CI jobs of
the same derivation dropped different symbol sets.
- Add overlays/patches/vulkan-shaders-gen-fail-and-retry.patch, carrying
the exit-code check from llama.cpp PR 24450, a retry with backoff on
fork() EAGAIN/ENOMEM, and a report of any shader left without SPIR-V
- Apply it via postPatch on voxtype-vulkan-unwrapped in overlays/default.nix
- Re-point the upstream symlinkJoin wrapper at the patched build, so
upstream's runtime dependency list is not duplicated
Verified with `just eval`, `just format`, and
`nix build .#nixosConfigurations.bane.pkgs.voxtype-vulkan`, which builds
cleanly with the patch applied and the crate test suite passing.
Refs: ggml-org/llama.cpp#24393
Refs: ggml-org/llama.cpp#20868
Refs: ggml-org/llama.cpp#24450
Refs: peteonrails/voxtype#550
voxtype-vulkan fails to link on CI with hundreds of undefined matmul_id_*
and matmul_*_cm1_* symbols, breaking the Home martin@{bane,ravi,skrye,tanis,
zannah} jobs and blocking the all-builds-green auto-merge gate.
The root cause is upstream, in ggml's vulkan-shaders-gen, vendored via
whisper-rs-sys 0.15.0 inside the voxtype flake input. The generator
discards the glslc child exit status and only inspects stderr, so a
child that dies without printing anything counts as a success. The
shader is then absent from the generated source while the header still
declares it, and the breakage only surfaces at link time. fork() also
fails transiently with EAGAIN or ENOMEM on small CI runners, which
takes the same silent path. That is why the identical derivation builds
on a workstation but not on a GitHub runner, and why three CI jobs of
the same derivation dropped different symbol sets.
- Add overlays/patches/vulkan-shaders-gen-fail-and-retry.patch, carrying
the exit-code check from llama.cpp PR 24450, a retry with backoff on
fork() EAGAIN/ENOMEM, and a report of any shader left without SPIR-V
- Apply it via postPatch on voxtype-vulkan-unwrapped in overlays/default.nix
- Re-point the upstream symlinkJoin wrapper at the patched build, so
upstream's runtime dependency list is not duplicated
Verified with `just eval`, `just format`, and
`nix build .#nixosConfigurations.bane.pkgs.voxtype-vulkan`, which builds
cleanly with the patch applied and the crate test suite passing.
Refs: ggml-org/llama.cpp#24393
Refs: ggml-org/llama.cpp#20868
Refs: ggml-org/llama.cpp#24450
Refs: peteonrails/voxtype#550
Overview
vulkan-shaders-genignores shader-compile subprocess failures, so a brokenlibggml-vulkancan be produced while the build reports success — the breakage onlysurfaces at run time. This PR makes the generator fail the build loudly instead:
execute_command()now returns the child exit code (WIFEXITED ? WEXITSTATUS : -1on POSIX;
GetExitCodeProcessafter the wait on Windows) — previously the status was discarded.string_to_spv()treatsexit_code != 0 || !stderr_str.empty()(and the launch-exceptioncatch) as failure and records it in astd::atomic<bool>.main()checks that flag afterprocess_shaders()and returnsEXIT_FAILUREbeforewrite_output_files(), so CMake stops instead of linking a silently-broken backend.Build-time only; no runtime or shader-output change. Fixes #24393.
Additional information
Why the old check missed it: success was decided solely by
if (!stderr_str.empty()), so acompiler that exits non-zero with empty stderr — or a subprocess that fails to launch
(
execvp->_exit(EXIT_FAILURE), empty stderr) — was treated as success and the shader registered.Validation (off-device, build-time logic): passes
g++ -std=c++17 -fsyntax-only. A small standaloneharness using the same fork/execvp/waitpid pattern confirms the previous stderr-only check reports
success for both a silent non-zero exit (
sh -c 'exit 3') and a missing-binary launch failure,whereas the exit-code check flags both and still passes a clean compile. Both the POSIX and Windows
exit-code paths are covered. No GPU required.
Requirements
paths and drafting wording). I authored and reviewed the change, understand it fully, and can explain
every line; the design and the validation harness are mine. No AI-written code was submitted without
manual review.