[PATCH v4 0/3] MR504: ci: Validate the SPIR-V code.
In order to validate SPIR-V code we have to fix a little bug that causes us to emit invalid SPIR-V code in some cases. That's technically a bug, so it should be good to fix it in 1.10; unfortunately the way the bug is fixed right now is not ideal, because in some cases it causes many push constants to be wasted, potentially reaching the Vulkan implementation limit, so some application that used to work might stop working. Eventually we should implement a better allocator for push constants, and include an alternative path if the Vulkan implementation doesn't offer enough push constants. -- v4: ci: Build vkd3d with SPIRV-Tools. vkd3d-shader/spirv: Honor force_validation after emitting SPIR-V code. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/504
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 38 +++++++++++++++++++----- libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index dc3d96313..bb5d5701f 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -133,7 +133,7 @@ static void vkd3d_spirv_dump(const struct vkd3d_shader_code *spirv, } } -static void vkd3d_spirv_validate(const struct vkd3d_shader_code *spirv, +static bool vkd3d_spirv_validate(struct vkd3d_string_buffer *buffer, const struct vkd3d_shader_code *spirv, enum vkd3d_shader_spirv_environment environment) { spv_diagnostic diagnostic = NULL; @@ -145,12 +145,13 @@ static void vkd3d_spirv_validate(const struct vkd3d_shader_code *spirv, if ((ret = spvValidateBinary(context, spirv->code, spirv->size / sizeof(uint32_t), &diagnostic))) { - FIXME("Failed to validate SPIR-V binary, ret %d.\n", ret); - FIXME("Diagnostic message: %s.\n", debugstr_a(diagnostic->error)); + vkd3d_string_buffer_printf(buffer, "%s", diagnostic->error); } spvDiagnosticDestroy(diagnostic); spvContextDestroy(context); + + return !ret; } #else @@ -2393,6 +2394,8 @@ struct spirv_compiler struct ssa_register_info *ssa_register_info; unsigned int ssa_register_count; + + uint64_t config_flags; }; static bool is_in_default_phase(const struct spirv_compiler *compiler) @@ -2448,7 +2451,8 @@ static void spirv_compiler_destroy(struct spirv_compiler *compiler) static struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *shader_version, struct vkd3d_shader_desc *shader_desc, const struct vkd3d_shader_compile_info *compile_info, const struct vkd3d_shader_scan_descriptor_info1 *scan_descriptor_info, - struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location) + struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location, + bool config_flags) { const struct shader_signature *patch_constant_signature = &shader_desc->patch_constant_signature; const struct shader_signature *output_signature = &shader_desc->output_signature; @@ -2465,6 +2469,7 @@ static struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_ve memset(compiler, 0, sizeof(*compiler)); compiler->message_context = message_context; compiler->location = *location; + compiler->config_flags = config_flags; if ((target_info = vkd3d_find_struct(compile_info->next, SPIRV_TARGET_INFO))) { @@ -9939,11 +9944,28 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, if (!vkd3d_spirv_compile_module(builder, spirv, spirv_compiler_get_entry_point_name(compiler))) return VKD3D_ERROR; - if (TRACE_ON()) + if (TRACE_ON() || parser->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION) { enum vkd3d_shader_spirv_environment environment = spirv_compiler_get_target_environment(compiler); - vkd3d_spirv_dump(spirv, environment); - vkd3d_spirv_validate(spirv, environment); + struct vkd3d_string_buffer buffer; + + if (TRACE_ON()) + vkd3d_spirv_dump(spirv, environment); + + vkd3d_string_buffer_init(&buffer); + if (!vkd3d_spirv_validate(&buffer, spirv, environment)) + { + FIXME("Failed to validate SPIR-V binary.\n"); + vkd3d_shader_trace_text(buffer.buffer, buffer.content_size); + + if (compiler->config_flags & VKD3D_SHADER_CONFIG_FLAG_FORCE_VALIDATION) + { + spirv_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_INVALID_SHADER, + "Execution generated an invalid shader, failing compilation:\n%s", + buffer.buffer); + } + } + vkd3d_string_buffer_cleanup(&buffer); } if (compiler->failed) @@ -9970,7 +9992,7 @@ int spirv_compile(struct vkd3d_shader_parser *parser, int ret; if (!(spirv_compiler = spirv_compiler_create(&parser->shader_version, &parser->shader_desc, - compile_info, scan_descriptor_info, message_context, &parser->location))) + compile_info, scan_descriptor_info, message_context, &parser->location, parser->config_flags))) { ERR("Failed to create SPIR-V compiler.\n"); return VKD3D_ERROR; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 102d49a38..936e89305 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -96,6 +96,7 @@ enum vkd3d_shader_error VKD3D_SHADER_ERROR_SPV_INVALID_TYPE = 2006, VKD3D_SHADER_ERROR_SPV_INVALID_HANDLER = 2007, VKD3D_SHADER_ERROR_SPV_NOT_IMPLEMENTED = 2008, + VKD3D_SHADER_ERROR_SPV_INVALID_SHADER = 2009, VKD3D_SHADER_WARNING_SPV_INVALID_SWIZZLE = 2300, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/504
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- gitlab/build-linux | 2 +- gitlab/build.yml | 4 ++-- gitlab/image.docker | 31 ++++++++++++++++++++++++++++++- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/gitlab/build-linux b/gitlab/build-linux index 4a8c793b7..d7e7573f1 100755 --- a/gitlab/build-linux +++ b/gitlab/build-linux @@ -12,7 +12,7 @@ rm -fr build mkdir build cd build export LD_LIBRARY_PATH=/usr/local/lib -if ../configure --enable-demos DXCOMPILER_LIBS="-L/usr/local/lib" CFLAGS="-g -O2 -Werror" CPPFLAGS="-DVKD3D_SHADER_UNSUPPORTED_DXIL" && \ +if ../configure --enable-demos --with-spirv-tools DXCOMPILER_LIBS="-L/usr/local/lib" CFLAGS="-g -O2 -Werror" CPPFLAGS="-DVKD3D_SHADER_UNSUPPORTED_DXIL" && \ make -j$(nproc) ; then make -j$(nproc) check || \ touch ../tests_failed diff --git a/gitlab/build.yml b/gitlab/build.yml index 9695a3042..9a19316b5 100644 --- a/gitlab/build.yml +++ b/gitlab/build.yml @@ -48,7 +48,7 @@ build-radv-32: - amd-gpu variables: VK_LOADER_DRIVERS_SELECT: 'radeon_*' - CC: 'gcc -m32' + CC: 'i686-linux-gnu-gcc' VKD3D_SHADER_CONFIG: 'force_validation' build-llvmpipe-32: @@ -58,7 +58,7 @@ build-llvmpipe-32: - 2 variables: VK_LOADER_DRIVERS_SELECT: 'lvp_*' - CC: 'gcc -m32' + CC: 'i686-linux-gnu-gcc' VKD3D_SHADER_CONFIG: 'force_validation' build-crosstest: diff --git a/gitlab/image.docker b/gitlab/image.docker index 9b7b17b09..270c509ba 100644 --- a/gitlab/image.docker +++ b/gitlab/image.docker @@ -2,6 +2,12 @@ WORKDIR /tmp +# Package spirv-tools from Debian has two problems for us: first, it +# doesn't have shared libraries; second, it's not multiarch. So we +# have to rebuild it with some tweaks. In order to make it multiarch +# we also have to drop the executables, but we don't care about those +# anyway. + RUN export DEBIAN_FRONTEND=noninteractive; \ echo 'path-exclude=/usr/share/doc/*' > /etc/dpkg/dpkg.cfg.d/99-exclude-cruft && \ echo 'path-exclude=/usr/share/locale/*' >> /etc/dpkg/dpkg.cfg.d/99-exclude-cruft && \ @@ -11,9 +17,10 @@ RUN export DEBIAN_FRONTEND=noninteractive; \ echo 'exit 101' >> /usr/sbin/policy-rc.d && \ chmod +x /usr/sbin/policy-rc.d && \ dpkg --add-architecture i386 && \ + sed -i -e 's|Types: deb|Types: deb deb-src|g' /etc/apt/sources.list.d/* && \ apt-get update && \ apt-get dist-upgrade -y && \ - apt-get install -y build-essential pkg-config gcc-multilib gcc-mingw-w64 \ + apt-get install -y build-essential pkg-config gcc-mingw-w64 crossbuild-essential-i386 \ autoconf automake libtool flex bison curl \ git ca-certificates rsync \ doxygen doxygen-latex graphviz \ @@ -27,6 +34,8 @@ RUN export DEBIAN_FRONTEND=noninteractive; \ libxcb-util-dev libxcb-util-dev:i386 \ libxcb-icccm4-dev libxcb-icccm4-dev:i386 \ libxcb-keysyms1-dev libxcb-keysyms1-dev:i386 && \ + apt-get build-dep -y spirv-tools && \ + apt-get build-dep -y -ai386 spirv-tools && \ git clone --depth 1 --branch wine-3.21 https://gitlab.winehq.org/wine/wine.git && \ cd wine && \ mkdir build && \ @@ -36,6 +45,26 @@ RUN export DEBIAN_FRONTEND=noninteractive; \ cp tools/widl/widl /usr/local/bin && \ cd ../.. && \ rm -rf wine && \ + apt-get source spirv-tools && \ + cd spirv-tools-* && \ + sed -i -e 's|-DBUILD_SHARED_LIBS=OFF|-DBUILD_SHARED_LIBS=ON|g' debian/rules && \ + sed -i -e 's|dh_install$|dh_install \&\& rm debian/spirv-tools/usr/bin/*|g' debian/rules && \ + sed -i '/Architecture: any/a Multi-Arch: same' debian/control && \ + dpkg-buildpackage -uc -us && \ + cd .. && \ + rm -f spirv-tools-dbgsym_*.deb && \ + dpkg -i spirv-tools_*.deb && \ + rm -fr * && \ + apt-get source spirv-tools && \ + cd spirv-tools-* && \ + sed -i -e 's|-DBUILD_SHARED_LIBS=OFF|-DBUILD_SHARED_LIBS=ON|g' debian/rules && \ + sed -i -e 's|dh_install$|dh_install \&\& rm debian/spirv-tools/usr/bin/*|g' debian/rules && \ + sed -i '/Architecture: any/a Multi-Arch: same' debian/control && \ + CONFIG_SITE=/etc/dpkg-cross/cross-config.amd64 DEB_BUILD_OPTIONS=nocheck dpkg-buildpackage -ai386 -Pcross,nocheck -uc -us && \ + cd .. && \ + rm -f spirv-tools-dbgsym_*.deb && \ + dpkg -i spirv-tools_*.deb && \ + rm -fr * && \ apt-get clean && \ curl -L -s https://github.com/microsoft/DirectXShaderCompiler/releases/download/v1.7.23... | tar zx -C /usr/local ./lib/libdxcompiler.so ./lib/libdxil.so && \ groupadd host-render -g 800 && \ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/504
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Because that's the granularity of Constant Buffer accesses in SM4. This commit requires using more push constants, but without it the generated SPIR-V can be invalid. --- libs/vkd3d/state.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d/state.c b/libs/vkd3d/state.c index fc3187f4b..9a039452c 100644 --- a/libs/vkd3d/state.c +++ b/libs/vkd3d/state.c @@ -515,7 +515,7 @@ static HRESULT d3d12_root_signature_init_push_constants(struct d3d12_root_signat assert(p->ShaderVisibility <= D3D12_SHADER_VISIBILITY_PIXEL); push_constants[p->ShaderVisibility].stageFlags = use_vk_heaps ? VK_SHADER_STAGE_ALL : stage_flags_from_visibility(p->ShaderVisibility); - push_constants[p->ShaderVisibility].size += p->u.Constants.Num32BitValues * sizeof(uint32_t); + push_constants[p->ShaderVisibility].size += align(p->u.Constants.Num32BitValues, 4) * sizeof(uint32_t); } if (push_constants[D3D12_SHADER_VISIBILITY_ALL].size) { @@ -564,7 +564,7 @@ static HRESULT d3d12_root_signature_init_push_constants(struct d3d12_root_signat idx = push_constant_count == 1 ? 0 : p->ShaderVisibility; offset = push_constants_offset[idx]; - push_constants_offset[idx] += p->u.Constants.Num32BitValues * sizeof(uint32_t); + push_constants_offset[idx] += align(p->u.Constants.Num32BitValues, 4) * sizeof(uint32_t); root_signature->parameters[i].parameter_type = p->ParameterType; root_constant->stage_flags = push_constant_count == 1 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/504
```diff @@ -2448,7 +2451,8 @@ static void spirv_compiler_destroy(struct spirv_compiler *compiler) static struct spirv_compiler *spirv_compiler_create(const struct vkd3d_shader_version *shader_version, struct vkd3d_shader_desc *shader_desc, const struct vkd3d_shader_compile_info *compile_info, const struct vkd3d_shader_scan_descriptor_info1 *scan_descriptor_info, - struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location) + struct vkd3d_shader_message_context *message_context, const struct vkd3d_shader_location *location, + bool config_flags) { const struct shader_signature *patch_constant_signature = &shader_desc->patch_constant_signature; const struct shader_signature *output_signature = &shader_desc->output_signature; ```
Should that be "uint64_t config_flags"? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/504#note_55720
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)