Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No validation against OpStore being used to store to an object with OpTypeImage. #4796

Closed
manon-traverse opened this issue May 10, 2022 · 1 comment · Fixed by #5368
Closed

No validation against OpStore being used to store to an object with OpTypeImage. #4796

manon-traverse opened this issue May 10, 2022 · 1 comment · Fixed by #5368
Assignees
@alan-baker
Labels
component:validation

Comments

@manon-traverse
Copy link

We found some invalid SPIR-V being generated by DXC. It turns out that spirv-val does not catch the invalid SPIR-V either.
I have attached an example shader to illustrate the issue.

bin_lines.cs.hlsl#main#SPIR-V#cs_6_6.txt

%type_3d_image = OpTypeImage %uint 3D 2 0 0 2 R32ui
...
%_ptr_Function_type_3d_image = OpTypePointer Function %type_3d_image
...
%131 = OpAccessChain %_ptr_UniformConstant_type_3d_image %g_rwTexture3d %130
%132 = OpLoad %type_3d_image %131
...
`%69 = OpVariable %_ptr_Function_type_3d_image Function`
...
OpStore %69 %132

This contradicts Vulkan's spec, https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-StandaloneSpirv-OpTypeImage-04661 :
"Objects of types OpTypeImage, OpTypeSampler, OpTypeSampledImage, and arrays of these types must not be stored to or modified"
@manon-traverse manon-traverse changed the title No validation against OpStore being used to store to an objecth with OpTypeImage. No validation against OpStore being used to store to an object with OpTypeImage. May 10, 2022
This was referenced May 10, 2022
Closed
Open
@sjfricke sjfricke mentioned this issue Jun 27, 2022
Merged
@MarijnS95
Copy link
Contributor

The same should probably also be applied for acceleration structures, which seems to have been added to a more recent VUID that superseeds 04661:

https://registry.khronos.org/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-StandaloneSpirv-OpTypeImage-06924

Objects of types OpTypeImage, OpTypeSampler, OpTypeSampledImage, OpTypeAccelerationStructureKHR, and arrays of these types must not be stored to or modified

svenvh added a commit to svenvh/SPIRV-Tools that referenced this issue Aug 9, 2023
@svenvh
spirv-val: Disallow stores according to VUID 06924
c25ec50 
Ensure that the validator rejects stores to objects of types
`OpTypeImage`, `OpTypeSampler`, `OpTypeSampledImage`,
`OpTypeAccelerationStructureKHR`, and arrays of these types, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Fixes KhronosGroup#4796

Signed-off-by: Sven van Haastregt <[email protected]>
@svenvh svenvh mentioned this issue Aug 9, 2023
Merged
svenvh added a commit to svenvh/SPIRV-Tools that referenced this issue Apr 19, 2024
@svenvh
spirv-val: Disallow stores according to VUID 06924
6871edd 
Ensure that the validator rejects stores to objects of types
`OpTypeSampledImage` and `OpTypeAccelerationStructureKHR`, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Stores to objects of types `OpTypeImage` and `OpTypeSampler` should be
rejected too, but HLSL-to-SPIR-V translation by glslang and shaderc
seems to result in some violations; so leave validation of those as
future work.

Contributes to KhronosGroup#4796

Signed-off-by: Sven van Haastregt <[email protected]>
svenvh added a commit to svenvh/SPIRV-Tools that referenced this issue Sep 20, 2024
@svenvh
spirv-val: Disallow stores according to VUID 06924
b81dfd2 
Ensure that the validator rejects stores to objects of types
`OpTypeSampledImage` and `OpTypeAccelerationStructureKHR`, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Stores to objects of types `OpTypeImage` and `OpTypeSampler` should be
rejected too, but HLSL-to-SPIR-V translation by glslang and shaderc
seems to result in some violations; so leave validation of those as
future work.

Contributes to KhronosGroup#4796

Signed-off-by: Sven van Haastregt <[email protected]>
svenvh added a commit to svenvh/SPIRV-Tools that referenced this issue Sep 20, 2024
@svenvh
spirv-val: Disallow stores according to VUID 06924
8308e38 
Ensure that the validator rejects stores to objects of types
`OpTypeImage`, `OpTypeSampler`, `OpTypeSampledImage`,
`OpTypeAccelerationStructureKHR`, and arrays of these types, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Guard the check behind the before_hlsl_legalization option, as
sometimes we may have temporaries or local variables that are expected
to get optimized away.

Fixes KhronosGroup#4796

Signed-off-by: Sven van Haastregt <[email protected]>
Change-Id: Ie035c01c5f94e7bdfc16b5c6c85705f302b7bda3
alan-baker pushed a commit that referenced this issue Sep 23, 2024
@svenvh
spirv-val: Disallow stores according to VUID 06924 (#5368)
7ba72f1 
Ensure that the validator rejects stores to objects of types
`OpTypeImage`, `OpTypeSampler`, `OpTypeSampledImage`,
`OpTypeAccelerationStructureKHR`, and arrays of these types, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Guard the check behind the before_hlsl_legalization option, as
sometimes we may have temporaries or local variables that are expected
to get optimized away.

Fixes #4796


Change-Id: Ie035c01c5f94e7bdfc16b5c6c85705f302b7bda3

Signed-off-by: Sven van Haastregt <[email protected]>
Keenuts pushed a commit to Keenuts/SPIRV-Tools that referenced this issue Nov 12, 2024
@svenvh @Keenuts
spirv-val: Disallow stores according to VUID 06924 (KhronosGroup#5368)
e944e83 
Ensure that the validator rejects stores to objects of types
`OpTypeImage`, `OpTypeSampler`, `OpTypeSampledImage`,
`OpTypeAccelerationStructureKHR`, and arrays of these types, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Guard the check behind the before_hlsl_legalization option, as
sometimes we may have temporaries or local variables that are expected
to get optimized away.

Fixes KhronosGroup#4796


Change-Id: Ie035c01c5f94e7bdfc16b5c6c85705f302b7bda3

Signed-off-by: Sven van Haastregt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alan-baker alan-baker

Labels
component:validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

spirv-val: Disallow stores according to VUID 06924 svenvh/SPIRV-Tools
4 participants
@MarijnS95 @dneto0 @alan-baker @manon-traverse