From 423c119c004b01786f36517a6db880586e02b7e6 Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Sun, 13 Sep 2026 08:58:09 -0600 Subject: [PATCH 1/3] vulkan-shaders-gen: retry an empty compile, and never declare what is not defined A shader whose compile produces no SPIR-V becomes an undefined reference at link, twenty minutes later, naming a symbol in vendored code with nothing earlier in the log to explain it: ld.bfd: ggml-vulkan.cpp:3964: undefined reference to `matmul_id_subgroup_nvfp4_f32_aligned_f16acc_cm1_len' write_output_files() emits the declaration before reading the artefact, then skips the definition if the file is empty, silently. The symbol is left declared and never defined. Compile success was also judged by stderr alone, with no exit code, so a shader that merely warns is discarded while one that reports nothing and writes nothing is accepted. Success is now judged by the artefact: the SPIR-V must exist and be non-empty. An empty result is retried up to three times with short backoff before giving up. This has been seen in CI with no accompanying diagnostic and does not reproduce locally, so it appears environmental and rare; retrying costs milliseconds on a genuinely broken shader and saves a build that would otherwise fail at link for no visible reason. After the last attempt the shader is named and generation fails. Verified both ways against an injected failure. A transient -- first attempt empty, later attempts normal -- recovers and the build links. A permanent one reports: shader matmul_id_subgroup_nvfp4_f32_aligned_f16acc_cm1 produced no SPIR-V; retrying (2/3) shader matmul_id_subgroup_nvfp4_f32_aligned_f16acc_cm1 produced no SPIR-V; retrying (3/3) cannot compile matmul_id_subgroup_nvfp4_f32_aligned_f16acc_cm1 after 3 attempts shader generation failed; see errors above and stops at generation instead of at link. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EkxqpYvUbjCpRDnFiNiVfx --- .../vulkan-shaders/vulkan-shaders-gen.cpp | 66 ++++++++++++++++--- 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp index 40c223470..319bb7e62 100644 --- a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp +++ b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -34,6 +35,9 @@ std::mutex lock; std::vector> shader_fnames; +// Set when a shader yields no SPIR-V, so the build stops at generation rather +// than at a link error that points nowhere useful. +bool generation_failed = false; std::locale c_locale("C"); std::string GLSLC = "glslc"; @@ -324,6 +328,14 @@ compile_count_guard acquire_compile_slot() { return compile_count_guard(&compile_count, &decrement_compile_count); } +// A shader is usable only if its SPIR-V exists and is non-empty. A zero-byte +// file is what an interrupted or silently-failed compile leaves behind. +static bool spv_is_usable(const std::string & path) { + std::error_code ec; + const auto size = std::filesystem::file_size(path, ec); + return !ec && size > 0; +} + void string_to_spv_func(std::string name, std::string in_path, std::string out_path, std::map defines, bool coopmat, bool dep_file, compile_count_guard slot) { std::string target_env = (name.find("_cm2") != std::string::npos) ? "--target-env=vulkan1.3" : "--target-env=vulkan1.2"; @@ -365,22 +377,49 @@ void string_to_spv_func(std::string name, std::string in_path, std::string out_p std::string stdout_str, stderr_str; try { - // std::cout << "Executing command: "; - // for (const auto& part : cmd) { - // std::cout << part << " "; - // } - // std::cout << std::endl; + // Success is judged by the artefact, not by stderr. Judging by stderr + // discards a shader over a warning, and misses the case that matters + // more: a compile that reports nothing and writes nothing. + // + // An empty result is retried rather than accepted. It has been seen in + // CI with no accompanying diagnostic, and retrying costs milliseconds + // on a genuinely broken shader while saving a build that would + // otherwise fail much later at link, naming a symbol whose absence has + // no visible cause. + constexpr int max_attempts = 3; + bool produced = false; + + for (int attempt = 1; attempt <= max_attempts && !produced; ++attempt) { + stdout_str.clear(); + stderr_str.clear(); + execute_command(cmd, stdout_str, stderr_str); + produced = spv_is_usable(out_path); + + if (!produced && attempt < max_attempts) { + std::cerr << "shader " << name << " produced no SPIR-V; retrying (" + << (attempt + 1) << "/" << max_attempts << ")" << std::endl; + std::this_thread::sleep_for(std::chrono::milliseconds(100 * attempt)); + } + } - execute_command(cmd, stdout_str, stderr_str); - if (!stderr_str.empty()) { - std::cerr << "cannot compile " << name << "\n\n"; + if (!produced) { + std::cerr << "cannot compile " << name << " after " << max_attempts + << " attempts\n\n"; for (const auto& part : cmd) { std::cerr << part << " "; } std::cerr << "\n\n" << stderr_str << std::endl; + generation_failed = true; return; } + if (!stderr_str.empty()) { + // Diagnostics alongside a usable artefact are warnings. Keeping the + // shader is the point: dropping it is what leaves a declaration + // with no definition. + std::cerr << "warnings compiling " << name << ":\n" << stderr_str << std::endl; + } + if (dep_file) { // replace .spv output path with the embed .cpp path which is used as output in CMakeLists.txt std::string dep = read_binary_file(target_cpp + ".d", true); @@ -1050,6 +1089,12 @@ void write_output_files() { if (input_filepath != "") { std::string data = read_binary_file(path); if (data.empty()) { + // The declaration above is already written, so skipping the + // definition leaves a symbol declared and never defined, which + // surfaces much later as an undefined reference at link. + std::cerr << "ERROR: shader '" << name << "' produced no SPIR-V (" + << path << ")\n"; + generation_failed = true; continue; } @@ -1196,5 +1241,10 @@ int main(int argc, char** argv) { write_output_files(); + if (generation_failed) { + std::cerr << "shader generation failed; see errors above" << std::endl; + return EXIT_FAILURE; + } + return EXIT_SUCCESS; } From f031d289b5eaec9f9c48de4e0a5db2c79c09e158 Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Sun, 20 Sep 2026 13:27:44 -0600 Subject: [PATCH 2/3] vulkan-shaders-gen: believe the exit code, and make the failure flag atomic Review feedback on #540. Two holes in the previous commit: Judging success by the artefact alone means a compile that fails and leaves an earlier build's .spv in place is credited with that stale file. Reproduced against a fake glslc that exits 0, prints nothing and writes nothing, with a 26-byte stale artefact already present: the generator exited 0 and embedded the stale bytes as the shader. execute_command discarded the exit status at the syscall -- waitpid with a null status, and no GetExitCodeProcess on Win32 -- so no caller could have checked it. It now returns the status, and a non-zero exit or any stderr fails the shader immediately, without retrying; a compiler that reports failure is believed the first time. Only a silent empty result is retried, and out_path is removed before each attempt so no stale file can stand in for a missing one. generation_failed was a plain bool written from the compile threads. It is std::atomic now, and the catch handler sets it too: an exception there leaves the shader out of shader_fnames, which is the same undefined reference by another route. Verified standalone against fake compilers over add.comp's 16 variants: healthy exits 0 clean; a non-zero exit fails at once naming the code; a transient first-attempt-empty recovers with all 16 definitions emitted; a permanent empty and the stale-file case both stop at generation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TMmMgd5xNGnjsQgybuUiK3 --- .../vulkan-shaders/vulkan-shaders-gen.cpp | 62 ++++++++++++------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp index 319bb7e62..21103ea03 100644 --- a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp +++ b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -35,9 +36,10 @@ std::mutex lock; std::vector> shader_fnames; -// Set when a shader yields no SPIR-V, so the build stops at generation rather -// than at a link error that points nowhere useful. -bool generation_failed = false; +// Set when a shader fails to compile or yields no SPIR-V, so the build stops at +// generation rather than at a link error that points nowhere useful. Written +// from the compile threads, so it has to be atomic. +std::atomic generation_failed{false}; std::locale c_locale("C"); std::string GLSLC = "glslc"; @@ -82,7 +84,7 @@ enum MatMulIdType { namespace { -void execute_command(std::vector& command, std::string& stdout_str, std::string& stderr_str) { +int execute_command(std::vector& command, std::string& stdout_str, std::string& stderr_str) { #ifdef _WIN32 HANDLE stdout_read, stdout_write; HANDLE stderr_read, stderr_write; @@ -131,8 +133,11 @@ void execute_command(std::vector& command, std::string& stdout_str, CloseHandle(stdout_read); CloseHandle(stderr_read); WaitForSingleObject(pi.hProcess, INFINITE); + DWORD exit_code = 1; + GetExitCodeProcess(pi.hProcess, &exit_code); CloseHandle(pi.hProcess); CloseHandle(pi.hThread); + return (int)exit_code; #else int stdout_pipe[2]; int stderr_pipe[2]; @@ -179,7 +184,9 @@ void execute_command(std::vector& command, std::string& stdout_str, close(stdout_pipe[0]); close(stderr_pipe[0]); - waitpid(pid, nullptr, 0); + int status = 0; + waitpid(pid, &status, 0); + return WIFEXITED(status) ? WEXITSTATUS(status) : -1; } #endif } @@ -377,24 +384,31 @@ void string_to_spv_func(std::string name, std::string in_path, std::string out_p std::string stdout_str, stderr_str; try { - // Success is judged by the artefact, not by stderr. Judging by stderr - // discards a shader over a warning, and misses the case that matters - // more: a compile that reports nothing and writes nothing. - // - // An empty result is retried rather than accepted. It has been seen in - // CI with no accompanying diagnostic, and retrying costs milliseconds - // on a genuinely broken shader while saving a build that would - // otherwise fail much later at link, naming a symbol whose absence has - // no visible cause. + // A compile can report success and still leave no SPIR-V behind. That + // has been observed in CI, and because the generated header declares + // every shader unconditionally, the gap only surfaces at link as an + // undefined reference to a generated symbol, long after the cause is + // visible. Judge by the exit code first, then by the artefact, and + // retry an empty result before giving up. constexpr int max_attempts = 3; + int exit_code = 0; bool produced = false; for (int attempt = 1; attempt <= max_attempts && !produced; ++attempt) { stdout_str.clear(); stderr_str.clear(); - execute_command(cmd, stdout_str, stderr_str); - produced = spv_is_usable(out_path); + // Drop any earlier artefact first, so a compile that reports + // success without writing cannot be credited to a stale file. + std::error_code ec; + std::filesystem::remove(out_path, ec); + + exit_code = execute_command(cmd, stdout_str, stderr_str); + if (exit_code != 0 || !stderr_str.empty()) { + break; + } + + produced = spv_is_usable(out_path); if (!produced && attempt < max_attempts) { std::cerr << "shader " << name << " produced no SPIR-V; retrying (" << (attempt + 1) << "/" << max_attempts << ")" << std::endl; @@ -402,9 +416,8 @@ void string_to_spv_func(std::string name, std::string in_path, std::string out_p } } - if (!produced) { - std::cerr << "cannot compile " << name << " after " << max_attempts - << " attempts\n\n"; + if (exit_code != 0 || !stderr_str.empty()) { + std::cerr << "cannot compile " << name << " (exit code " << exit_code << ")\n\n"; for (const auto& part : cmd) { std::cerr << part << " "; } @@ -413,11 +426,11 @@ void string_to_spv_func(std::string name, std::string in_path, std::string out_p return; } - if (!stderr_str.empty()) { - // Diagnostics alongside a usable artefact are warnings. Keeping the - // shader is the point: dropping it is what leaves a declaration - // with no definition. - std::cerr << "warnings compiling " << name << ":\n" << stderr_str << std::endl; + if (!produced) { + std::cerr << "cannot compile " << name << ": no SPIR-V produced after " + << max_attempts << " attempts (" << out_path << ")" << std::endl; + generation_failed = true; + return; } if (dep_file) { @@ -436,6 +449,7 @@ void string_to_spv_func(std::string name, std::string in_path, std::string out_p shader_fnames.push_back(std::make_pair(name, out_path)); } catch (const std::exception& e) { std::cerr << "Error executing command for " << name << ": " << e.what() << std::endl; + generation_failed = true; } } From 8c11f8bcd4d345f19d07539917919d59e83d1116 Mon Sep 17 00:00:00 2001 From: Chris Thompson Date: Sun, 20 Sep 2026 13:27:44 -0600 Subject: [PATCH 3/3] vulkan-shaders-gen: stop before writing a partial header Backported from the shape llama.cpp master already has: the failure check belongs before write_output_files(), not after it. A partial header and source carry fresh timestamps, so a second build finds them newer than their inputs, skips regeneration and links the gap instead. The check after write_output_files() stays, because the embed step has its own way to fail -- a shader that compiled but whose artefact is unreadable by the time it is read back. llama.cpp sets its flag there and then returns EXIT_SUCCESS without re-reading it, so that one is detected and dropped upstream; keeping both checks here closes it. execute_command is now byte-identical to llama.cpp master, which should make the next ggml sync of this file a clean one. Verified with ci/run-local.sh linux-vulkan on a real glslc: 1932 shader symbols declared, every one defined, binary links with no undefined generated symbols. Against the fake-compiler matrix, no output file is written on any failure path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TMmMgd5xNGnjsQgybuUiK3 --- .../ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp index 21103ea03..3047769f7 100644 --- a/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp +++ b/external/ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp @@ -1253,8 +1253,18 @@ int main(int argc, char** argv) { process_shaders(); + // Stop before writing anything. A partial header and source carry fresh + // timestamps, so a second build would find them newer than their inputs + // and link the gap rather than regenerate it. + if (generation_failed) { + std::cerr << "shader generation failed; see errors above" << std::endl; + return EXIT_FAILURE; + } + write_output_files(); + // The embed step has its own way to fail: a shader that compiled but whose + // artefact is unreadable by the time it is read back. if (generation_failed) { std::cerr << "shader generation failed; see errors above" << std::endl; return EXIT_FAILURE;