KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.96k stars 819 forks source link

Invalid SPIR-V can be generated: OpLoad a pointer to a OpTypeRuntimeArray #2733

Open khyperia opened 3 years ago

khyperia commented 3 years ago

Heya! I've minimized down a repro down to a frankly bizarre looking shader: the below generates an instruction of the form %15 = OpLoad %_runtimearr_float %14. This is illegal: the SPIR-V spec states, in the OpLoad instruction, "Result Type is the type of the loaded object. It must be a type with fixed size; i.e., it must not be, nor include, any OpTypeRuntimeArray types."

Unfortunately, spirv-val does not catch this case, and the resulting SPIR-V module passes spirv-val. I have reported the bug here.

#version 450
layout(binding = 0) buffer stuff {
    float variable_array[];
};
void main() {
    variable_array = variable_array;
}

Compiled via: glslangValidator -V test.glsl.frag. Also reproduces with shaderc, via glslc test.glsl.frag -o glslc.spv (I believe glslc uses glslang?)

Generated SPIR-V, click to expand ```spir-v ; SPIR-V ; Version: 1.0 ; Generator: Khronos Glslang Reference Front End; 10 ; Bound: 17 ; Schema: 0 OpCapability Shader %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %main "main" OpExecutionMode %main OriginLowerLeft OpSource GLSL 450 OpName %main "main" OpName %stuff "stuff" OpMemberName %stuff 0 "variable_array" OpName %_ "" OpDecorate %_runtimearr_float ArrayStride 4 OpMemberDecorate %stuff 0 Offset 0 OpDecorate %stuff BufferBlock OpDecorate %_ DescriptorSet 0 OpDecorate %_ Binding 0 %void = OpTypeVoid %3 = OpTypeFunction %void %float = OpTypeFloat 32 %_runtimearr_float = OpTypeRuntimeArray %float %stuff = OpTypeStruct %_runtimearr_float %_ptr_Uniform_stuff = OpTypePointer Uniform %stuff %_ = OpVariable %_ptr_Uniform_stuff Uniform %int = OpTypeInt 32 1 %int_0 = OpConstant %int 0 %_ptr_Uniform__runtimearr_float = OpTypePointer Uniform %_runtimearr_float %main = OpFunction %void None %3 %5 = OpLabel %14 = OpAccessChain %_ptr_Uniform__runtimearr_float %_ %int_0 %15 = OpLoad %_runtimearr_float %14 %16 = OpAccessChain %_ptr_Uniform__runtimearr_float %_ %int_0 OpStore %16 %15 OpReturn OpFunctionEnd ```

To be honest, this is mostly an academic issue, I was doing research for the rust-gpu project and stumbled across this miscompilation, I didn't run into this in the "real world". However, I believe that compilers should always either output valid code, or produce an error, so I'm reporting this bug~

Thanks for your time!

greg-lunarg commented 3 years ago

Unfortunately, glslangValidator is caught in the middle here. As far as I can tell, the array assignment above is legal GLSL. Which means the solution is for glslangValidator to generate a loop over the length of the array and copy by element.

Since this is not from a real-life shader, I do not anticipate fixing this in the near future. If someone really needs this, ping me.

greg-lunarg commented 3 years ago

@gnl21 SPIR-V disallows load and store of runtime arrays. Thinking about this more, trying to implement this in glslangValidator would be very problematic. It seems like a much better solution to disallow whole-array operations on runtime arrays in GLSL as is done for SPIR-V. Given these are disallowed in SPIR-V, I am guessing we will get very little push back here from IHVs.

Could this be done?

gnl21 commented 3 years ago

I don't think this was ever intended to be allowed in GLSL, but I can't find anything specifically disallowing it in the spec either. Could someone open a GLSL issue to track this and get it approved by the working group?

The right solution here seems to be to disallow arrays whose size is not known to the compiler from being used in most expressions. They are allowed to be indexed with the subscript operator, but everything else should require a known size. (Assignment may be the only other thing that is allowed for them at present, I'm not sure).

greg-lunarg commented 3 years ago

I will open an issue. Thanks!