KhronosGroup / KhronosGroup/SPIRV-Tools

Regression in validation of Image Operand Lod in combination with MS image

Open
#4,424 2 comments 0 reactions 0 assignees View on GitHub
component:validation
Dominant language
C++
Stars
1.4k
Forks
709
Avg merge
1d 22h
Merged PRs (30d)
28

Description

Take the following (I think invalid) SPIR-V:

```swift
; SPIR-V
; Version: 1.0
; Generator: Google ANGLE Shader Compiler; 0
; Bound: 79
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %17 "main" %21
OpExecutionMode %17 LocalSize 4 1 1
OpSource GLSL 450
OpName %5 "_uimageIn"
OpName %8 "_udataOut"
OpMemberName %8 0 "_udata"
OpName %10 ""
OpName %12 "ANGLEUniformBlock"
OpMemberName %12 0 "acbBufferOffsets"
OpName %14 "ANGLEUniforms"
OpName %17 "main"
OpName %21 "gl_GlobalInvocationID"
OpName %31 "_usampleIndex"
OpName %40 "_ucolor"
OpName %46 "_uunnormalized"
OpName %49 "_uoutIndex"
OpDecorate %5 DescriptorSet 2
OpDecorate %5 Binding 0
OpDecorate %7 ArrayStride 4
OpMemberDecorate %8 0 Offset 0
OpDecorate %8 BufferBlock
OpDecorate %10 DescriptorSet 3
OpDecorate %10 Binding 0
OpMemberDecorate %12 0 Offset 0
OpDecorate %12 Block
OpDecorate %14 DescriptorSet 0
OpDecorate %14 Binding 0
OpDecorate %21 BuiltIn GlobalInvocationId
%1 = OpTypeFloat 32
%2 = OpTypeImage %1 2D 0 0 1 1 Unknown
%3 = OpTypeSampledImage %2
%6 = OpTypeInt 32 0
%7 = OpTypeRuntimeArray %6
%8 = OpTypeStruct %7
%11 = OpTypeVector %6 4
%12 = OpTypeStruct %11
%15 = OpTypeVoid
%19 = OpTypeVector %6 3
%22 = OpTypeInt 32 1
%23 = OpConstant %6 0
%28 = OpConstant %22 4
%32 = OpTypeVector %22 2
%33 = OpConstantNull %32
%34 = OpTypeVector %1 4
%41 = OpConstant %1 255
%56 = OpConstant %22 1
%59 = OpConstant %6 1
%63 = OpConstant %22 2
%66 = OpConstant %6 2
%70 = OpConstant %22 3
%73 = OpConstant %6 3
%4 = OpTypePointer UniformConstant %3
%9 = OpTypePointer Uniform %8
%13 = OpTypePointer Uniform %12
%77 = OpTypePointer Private %19
%20 = OpTypePointer Input %19
%78 = OpTypePointer Private %6
%24 = OpTypePointer Input %6
%30 = OpTypePointer Function %22
%39 = OpTypePointer Function %34
%45 = OpTypePointer Function %11
%51 = OpTypePointer Function %6
%54 = OpTypePointer Uniform %6
%16 = OpTypeFunction %15
%5 = OpVariable %4 UniformConstant
%10 = OpVariable %9 Uniform
%14 = OpVariable %13 Uniform
%21 = OpVariable %20 Input
%17 = OpFunction %15 None %16
%18 = OpLabel
%31 = OpVariable %30 Function
%40 = OpVariable %39 Function
%46 = OpVariable %45 Function
%49 = OpVariable %30 Function
%25 = OpAccessChain %24 %21 %23
%26 = OpLoad %6 %25
%27 = OpBitcast %22 %26
%29 = OpSMod %22 %27 %28
OpStore %31 %29
%35 = OpLoad %3 %5
%36 = OpLoad %22 %31
%37 = OpImage %2 %35
%38 = OpImageFetch %34 %37 %33 Lod|Sample %23 %36
OpStore %40 %38
%42 = OpLoad %34 %40
%43 = OpVectorTimesScalar %34 %42 %41
%44 = OpConvertFToU %11 %43
OpStore %46 %44
%47 = OpLoad %22 %31
%48 = OpIMul %22 %47 %28
OpStore %49 %48
%50 = OpLoad %22 %49
%52 = OpAccessChain %51 %46 %23
%53 = OpLoad %6 %52
%55 = OpAccessChain %54 %10 %23 %50
OpStore %55 %53
%57 = OpLoad %22 %49
%58 = OpIAdd %22 %57 %56
%60 = OpAccessChain %51 %46 %59
%61 = OpLoad %6 %60
%62 = OpAccessChain %54 %10 %23 %58
OpStore %62 %61
%64 = OpLoad %22 %49
%65 = OpIAdd %22 %64 %63
%67 = OpAccessChain %51 %46 %66
%68 = OpLoad %6 %67
%69 = OpAccessChain %54 %10 %23 %65
OpStore %69 %68
%71 = OpLoad %22 %49
%72 = OpIAdd %22 %71 %70
%74 = OpAccessChain %51 %46 %73
%75 = OpLoad %6 %74
%76 = OpAccessChain %54 %10 %23 %72
OpStore %76 %75
OpReturn
OpFunctionEnd
```

It includes the following instruction:

```swift
%38 = OpImageFetch %34 %37 %33 Lod|Sample %23 %36
```

When validating this SPIR-V from the command line, I get a validation error:

```bash
$ spirv-val --version
SPIRV-Tools v2020.7-dev unknown hash, 2020-12-08T22:07:00
$ spirv-val the_shader
error: line 85: Image Operand Lod requires 'MS' parameter to be 0
%38 = OpImageFetch %v4float %37 %33 Lod|Sample %uint_0 %36
```

However, when validating it (with the API) using ToT, I don't get a validation error. This regression seems to have been introduced by 305caff2ebb135b688a476233ce1873efee032bb which contains:

```diff
- if (info.multisampled != 0) {
- return _.diag(SPV_ERROR_INVALID_DATA, inst)
- << "Image Operand Lod requires 'MS' parameter to be 0";
- }
+ // Multisampled is already checked.
```

and also removes a test for this!

```diff
-TEST_F(ValidateImage, LodMultisampled) {
- const std::string body = R"(
-%img = OpLoad %type_image_f32_2d_0011 %uniform_image_f32_2d_0011
-%sampler = OpLoad %type_sampler %uniform_sampler
-%simg = OpSampledImage %type_sampled_image_f32_2d_0011 %img %sampler
-%res1 = OpImageSampleExplicitLod %f32vec4 %simg %f32vec2_00 Lod %f32_0)";
-
- CompileSuccessfully(GenerateShaderCode(body).c_str());
- ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Image Operand Lod requires 'MS' parameter to be 0"));
-}
```

I only caught this error because the invalid SPIR-V is making Intel/mesa crash.

Contributor guide

Open the contributing guide

Research direction

Start with the validator logic for Image Operand Lod and the ValidateImage LodMultisampled test referenced in the issue. Reproduce the failure with the supplied SPIR-V or spirv-val, then restore validation that rejects a nonzero MS parameter with Lod and add or restore the regression test. Done means the invalid module is rejected through both the command-line tool and API validation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, devtools
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.