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.

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

[SPIR-V] Silent compiler crash when using GetAttributeAtVertex #2955

Closed
Dredhog opened this issue Jun 8, 2020 · 6 comments · Fixed by #6459
Closed

[SPIR-V] Silent compiler crash when using GetAttributeAtVertex #2955

Dredhog opened this issue Jun 8, 2020 · 6 comments · Fixed by #6459
Assignees
Labels
spirv Work related to SPIR-V

Comments

@Dredhog
Copy link

Dredhog commented Jun 8, 2020

Title

Silent compiler crash when using GetAttributeAtVertex

Functional impact

Compiler crashes, no result gets printed to stdout.

Minimal repro steps

  1. Compile repro_shader.txt with dxc.exe -E frag -T ps_6_1 repro_shader.txt -spirv
struct PSInput
{
	float4 position : SV_POSITION;
	nointerpolation float3 color : COLOR;
};

cbuffer constants : register(b0)
{
	float4 g_constants;
}

float4 frag(PSInput input) : SV_TARGET
{
	uint cmp = (uint)(g_constants[0]);

	float colorAtV0 = GetAttributeAtVertex(input.color, 0)[cmp];
	float colorAtV1 = GetAttributeAtVertex(input.color, 1)[cmp];
	float colorAtV2 = GetAttributeAtVertex(input.color, 2)[cmp];

	return float4(colorAtV0, colorAtV1, colorAtV2, 0);
}

Expected result

The compiler outputs valid SPIR-V

Actual result

DXC crashes on null pointer dereference (see call_stack.txt)

Further technical details

DXC built from changeset 5ebc67c

@kuhar
Copy link
Collaborator

kuhar commented Jun 8, 2020

Thanks for the detailed bug report, @Dredhog! Looping in spir-v folks: @ehsannas @jaebaek.

@kuhar kuhar added the spirv Work related to SPIR-V label Jun 8, 2020
@sudonatalie sudonatalie self-assigned this Jul 6, 2021
sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this issue Jul 7, 2021
Fix a null pointer dereference which can occur when an unimplemented
intrinsic is used. The repro shader from issue microsoft#2955 (added as a unit
test) now fails with a useful message rather than a silent crash.
sudonatalie added a commit that referenced this issue Jul 9, 2021
Fix a null pointer dereference which can occur when an unimplemented
intrinsic is used. The repro shader from issue #2955 (added as a unit
test) now fails with a useful message rather than a silent crash.
@sudonatalie
Copy link
Collaborator

Closing this issue as the crash is resolved, and opened #3930 to track an actual implementation of the intrinsic in future.

@Keenuts
Copy link
Collaborator

Keenuts commented Nov 15, 2023

Seems like this issue is now present again.
Started https://github.com/microsoft/DirectXShaderCompiler/pull/6018/files but unsure about the result.

Also, tried to figure out the actual usages, and turns out the SPIR-V backend generated invalid code in a few cases instead of showing an error, or generating valid code.

@ShchchowAMD could you take a look? Seems like you implemented those features IIRC?

Another example which yields a validation error:

struct S {
  float4 a : COLOR;
};

float compute(float4 a) {
  return GetAttributeAtVertex(a, 0)[0];
}

float4 main(nointerpolation S s) : SV_TARGET
{
  return float4(0, 0, 0, compute(s.a));
}
dxc -E main -T ps_6_0 repro.hlsl -fcgl -spirv
fatal error: generated SPIR-V is invalid: Expected Constituent type to be equal to the corresponding member type of Result Type struct
  %22 = OpCompositeConstruct %S %21

note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

@ShchchowAMD
Copy link
Contributor

Just to double check I understand correct as I'm not familiar with this case:

a[0] is accessing first float channel of a, which means a[0] is eqaul to a.x.

I'll try to make a fix for this case if above explanation is expected.

Seems like previous implementation missed such case that using array-like bracket indexing to access a channel in vector.

@s-perron
Copy link
Collaborator

Just to double check I understand correct as I'm not familiar with this case:

a[0] is accessing first float channel of a, which means a[0] is eqaul to a.x.

I'll try to make a fix for this case if above explanation is expected.

Seems like previous implementation missed such case that using array-like bracket indexing to access a channel in vector.

I believe that is correct. You can see compare v.x and v[0] at https://godbolt.org/z/no8nvnaPz. They both have essentially the same access chain.

@s-perron
Copy link
Collaborator

@ShchchowAMD Thanks for looking into this.

sudonatalie added a commit that referenced this issue Mar 5, 2024
…ture inputs. (#6041)

**Fix for**

Fix issue under:
#2955
For structure type, when `nointerpolation` is decorated on a structure
input, this flag should be broadcast to its members.

PR (#6018) also
helps to resolve an issue when SPIRV backend generated variable has no
AST type.

**Test**
Please see same case under:
#6018

Besides, example as below should be invalid as the first parameter of
function `compute` should has only one spirv-type.

If its parameter type declaration has been expanded to an array
implicitly, it should not accept other interpolated inputs as its input
parameter.

```
struct S {
  float4 a : COLOR;
};

float compute(float4 a) {
  return GetAttributeAtVertex(a, 2)[0];
}

float4 main(nointerpolation S s, float4 b : COLOR2) : SV_TARGET
//float4 main(nointerpolation S s) : SV_TARGET
{
  return float4(0, 0, compute(b), compute(s.a));
  //return float4(0, 0, 0, compute(s.a));
}
```
I added an error report point in this commit and gets following reports:
```
1.hlsl:12:31: error: Current function could only use noninterpolated variable as input.
  return float4(0, 0, compute(b), compute(s.a));
                              ^
fatal error: generated SPIR-V is invalid: OpFunctionCall Argument <id> '37[%param_var_a_0]'s type does not match Function <id> '24[%_ptr_Function_v4float]'s parameter type.
  %45 = OpFunctionCall %float %compute %param_var_a_0

note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible
```
Any idea to let the error only reported before spirv validator?

**Ref**:
#2955
#6018

---------

Co-authored-by: Zhou, Shaochi(AMD) <shaozhou@amd.com>
Co-authored-by: Natalie Chouinard <sudonatalie@google.com>
Keenuts added a commit to Keenuts/DirectXShaderCompiler that referenced this issue Mar 26, 2024
Some SPIR-V types are not direct translations of an AST type, hence
they don't have an AST type attached to it.
The code didn't checked for this possibility when visiting all
variables.

Fixes microsoft#2955

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit to Keenuts/DirectXShaderCompiler that referenced this issue Mar 27, 2024
Some SPIR-V types are not direct translations of an AST type, hence
they don't have an AST type attached to it.
The code didn't checked for this possibility when visiting all
variables.

Fixes microsoft#2955

Signed-off-by: Nathan Gauër <brioche@google.com>
Keenuts added a commit that referenced this issue Mar 29, 2024
Some SPIR-V types are not direct translations of an AST type, hence they
don't have an AST type attached to it.
The code didn't checked for this possibility when visiting all
variables.

Fixes #2955

Signed-off-by: Nathan Gauër <brioche@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spirv Work related to SPIR-V
Projects
None yet
8 participants