spirv-val: Validate outter descriptor array index

[spencer-lunarg]

I found out that spirv-val wasn't validating access chain OOB for the outer array of a descriptor array

// buffer array
layout(set=0, binding=0) buffer foo { int x; } var[3];
var[3].x = 3;

// image array
layout(set = 0, binding = 0) uniform sampler sampler_array[2];
sampler2D(texture_image, sampler_array[2])

[spencer-lunarg]

moved string logic to error message time as assume most OpAccessChain are valid and don't need to spend time building this string

[spencer-lunarg]

while the OpAccessChain indexes are required to be 32-bit int scalar but can be negative

I realize from the failing spirv-opt tests is the OpTypeArray length operand can be a 64-bit length

[spencer-lunarg]

@alan-baker working more into this, I found the line below makes it seem like it is legal to have a -1 access chain index?

So this means if I have a

layout(set=0, binding=0) buffer foo { int x[3]; } var;

These are valid?

%int_n1 = OpConstant %int -1
%int_5 = OpConstant %int 5

%ac = OpAccessChain %ptr %var %int_0 %int_n1
// or
%ac = OpAccessChain %sb_ptr %var %int_0 %int_5

(I realize this code should be checking 64-bit, I am adding that)

[s-perron]

Are we sure we want this in the validator? Having an OpAccessChain that is oob is not invalid, it is undefined behaviour, if it is executed.

This is important because someone might have code that is guarded by a conditional statement that will only execute it when it is allowed. This seems like a better fit for the linter.

[spencer-lunarg]

@s-perron I guess the issue was someone had the shader in #5468

and it seemed like something that could be caught statically so it was not be validated in the validation layers SPIR-V runtime

We are currently checking if some of the OpAccessChain is being accessed correctly in spirv-val, so it was strange it was not for all aspects

[s-perron]

We are currently checking if some of the OpAccessChain is being accessed correctly in spirv-val

That surprises me because I thought we had agreed it was not a validation error.

[alan-baker]

We are currently checking if some of the OpAccessChain is being accessed correctly in spirv-val

That surprises me because I thought we had agreed it was not a validation error.

When I look at the code we currently on check OpTypeStruct indexing. That is specifically called out as must (validity) in SPIR-V.

As I mentioned in the other issue, I would be ok with Vulkan specific checks that error on guaranteed undefined behaviour. I agree with Steven that that means you might have to limit where those checks are applied (e.g. always executed).

[spencer-lunarg]

When I look at the code we currently on check OpTypeStruct indexing. That is specifically called out as must (validity) in SPIR-V.

Ok, good to know!

I agree with Steven that that means you might have to limit where those checks are applied (e.g. always executed).

Ok, let me take another stab at this, while we do plan to have GPU Validation on OOB, there is a large amount of trivial cases that could be caught, happy to guard (and comment in the code) the restriction of the checking

[spencer-lunarg]

After spending more time on this, decided that

1. GLSL/HLSL/Slang don't even allow you do do this, so it would be someone code gen going bad
2. I prototyped this in VVL and it was quite a lot of code/overhead for something that is not likely to hit
3. GPU-AV will catch this anyway just by nature of it being OOB

closing and just gonna keep this as a runtime check