From 472226422e0b6dc7df282dca5c1bdd17401eaed0 Mon Sep 17 00:00:00 2001 From: "Matias N. Goldberg" Date: Sat, 29 Jul 2023 18:28:33 -0300 Subject: [PATCH] Fix uninitialized variable ending up sent to Vulkan The first time a shader is compiled Godot performs the following: ```cpp for (uint32_t i = 0; i < SHADER_STAGE_MAX; i++) { if (spirv_data.push_constant_stages_mask.has_flag((ShaderStage)(1 << i))) { binary_data.push_constant_vk_stages_mask |= shader_stage_masks[i]; } } ``` However binary_data.push_constant_vk_stages_mask is never initialized to 0 and thus contains garbage data or'ed with the good data. This value is used by push constants (and many other things) thus it can be a big deal. Fortunately because the relevant flags are always guaranteed to be set (but not guaranteed to be unset), the damage is restricted to: 1. Performance (unnecessary flushing & over-excessive barriers) 2. Overwriting push descriptors already set (this would be serious, doesn't seem to be an issue) 3. Driver implementations going crazy when they see bits set they don't expect (unknown if this is an issue) This uninitialized value is later saved into the binary cache. Valgrind is able to detect this bug on the first run, but not on the subsequent ones because they data comes from a file. cache_file_version has been bumped to force rebuild of all cached shaders. Because the ones generated so far are compromised. --- drivers/vulkan/rendering_device_vulkan.cpp | 2 +- servers/rendering/renderer_rd/shader_rd.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/vulkan/rendering_device_vulkan.cpp b/drivers/vulkan/rendering_device_vulkan.cpp index be6f8f35807..cd18d848379 100644 --- a/drivers/vulkan/rendering_device_vulkan.cpp +++ b/drivers/vulkan/rendering_device_vulkan.cpp @@ -4687,7 +4687,7 @@ Vector RenderingDeviceVulkan::shader_compile_binary_from_spirv(const Ve "Number of uniform sets is larger than what is supported by the hardware (" + itos(limits.maxBoundDescriptorSets) + ")."); // Collect reflection data into binary data. - RenderingDeviceVulkanShaderBinaryData binary_data; + RenderingDeviceVulkanShaderBinaryData binary_data{}; Vector> uniform_info; // Set bindings. Vector specialization_constants; { diff --git a/servers/rendering/renderer_rd/shader_rd.cpp b/servers/rendering/renderer_rd/shader_rd.cpp index c85ece6366d..eaa84b76140 100644 --- a/servers/rendering/renderer_rd/shader_rd.cpp +++ b/servers/rendering/renderer_rd/shader_rd.cpp @@ -382,7 +382,7 @@ String ShaderRD::_version_get_sha1(Version *p_version) const { } static const char *shader_file_header = "GDSC"; -static const uint32_t cache_file_version = 2; +static const uint32_t cache_file_version = 3; bool ShaderRD::_load_from_cache(Version *p_version) { String sha1 = _version_get_sha1(p_version);