spirv-val: Not catching OOB access into an UniformConstant array

[spencer-lunarg]

If trying to index into an sampler array were we know it is OOB, spirv-val doesn't detect it

// what glsl would look like
layout (location = 0) out vec4 out_color;
layout (binding = 0) uniform sampler2D textures[2];

void main() {
    out_color = texture(textures[2], vec2(0.0));
}

               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %out_color
               OpExecutionMode %main OriginUpperLeft
               OpDecorate %out_color Location 0
               OpDecorate %textures DescriptorSet 0
               OpDecorate %textures Binding 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
    %v4float = OpTypeVector %float 4
%_ptr_Output_v4float = OpTypePointer Output %v4float
  %out_color = OpVariable %_ptr_Output_v4float Output
         %10 = OpTypeImage %float 2D 0 0 0 1 Unknown
         %11 = OpTypeSampledImage %10
        %int = OpTypeInt 32 1
%num_textures = OpConstant %int 2
%_arr_11_num_textures = OpTypeArray %11 %num_textures
%_ptr_UniformConstant__arr_11_num_textures = OpTypePointer UniformConstant %_arr_11_num_textures
   %textures = OpVariable %_ptr_UniformConstant__arr_11_num_textures UniformConstant
      %int_2 = OpConstant %int 2
%_ptr_UniformConstant_11 = OpTypePointer UniformConstant %11
    %v2float = OpTypeVector %float 2
    %float_0 = OpConstant %float 0
         %23 = OpConstantComposite %v2float %float_0 %float_0
       %main = OpFunction %void None %3
          %5 = OpLabel
         %19 = OpAccessChain %_ptr_UniformConstant_11 %textures %int_2
         %20 = OpLoad %11 %19
         %24 = OpImageSampleImplicitLod %v4float %20 %23
               OpStore %out_color %24
               OpReturn
               OpFunctionEnd

[alan-baker]

SPIR-V phrases that as undefined behaviour and not a validation rule. So technically the module is valid, but the behaviour is unreliable. This is another example where a best effort linter (as in #5455) is preferable to validation in my opinion.

[spencer-lunarg]

SPIR-V phrases that as undefined behaviour and not a validation rule

Where is the line in the spec about this?

Also do you mean undefined value? At least from a Vulkan Point of View, Undefined behaviour == Validation (VU)

[alan-baker]

It comes from OpAccessChain

if indexing into a vector, array, or matrix, with the result type being a logical pointer type, causes undefined behavior if not in bounds.

SPIR-V distinguishes between static and runtime differently than Vulkan. See https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_validity_and_defined_behavior.

[spencer-lunarg]

Ok, this was educational

would you agree that this should be then caught at runtime from the Vulkan Validation Layer if used in Vulkan, or do we expect all implementations to be able to handle this?

If first one, will move issue to VVL, if second, want to get CTS written

[alan-baker]

do we expect all implementations to be able to handle this?

I'm not certain what that means. I think crashing is within bounds of undefined behavior. You could try to validate this in VVL, but unless it's added to GPUAV it won't catch everything. Maybe that's ok though.

[spencer-lunarg]

You could try to validate this in VVL, but unless it's added to GPUAV it won't catch everything.

We already have GPU-AV for OOB access like this for descriptor indexing, I guess not fully understanding why we aren't able to statically detect textures[2] is OOB from the array size in VVL and mark this as invalid without GPU-AV

[alan-baker]

I guess not fully understanding why we aren't able to statically detect textures[2] is OOB from the array size in VVL and mark this as invalid without GPU-AV

You can in this case for sure, but not non-constant cases.

[spencer-lunarg]

ok yes, agree, if it is not a OpConstant it will have to be deferred to GPU-AV time (which I have plans to add)

as for the static, OpConstant case, would you agree that could live in spirv-val under a IsEnv(Vulkan) check?

[alan-baker]

ok yes, agree, if it is not a OpConstant it will have to be deferred to GPU-AV time (which I have plans to add)

as for the static, OpConstant case, would you agree that could live in spirv-val under a IsEnv(Vulkan) check?

Based on your earlier comment about how Vulkan treats UB, I think so.

Things to watch out for:

• That rule only applies to logical pointers (so no physical storage buffer)
• Indexes could be 64-bit values (there appears to be a bug where the struct validation assumes 32-bit)
• Spec constants shouldn't assume default values.