microsoft / DirectXShaderCompiler

This repo hosts the source for the DirectX Shader Compiler which is based on LLVM/Clang.
Other
3.11k stars 697 forks source link

Internal compiler error on shader validation #3835

Open Gordon-F opened 3 years ago

Gordon-F commented 3 years ago

Reproduces on latest release.

struct gl_PerVertex {
    float4 gl_Position : SV_Position;
    float gl_PointSize : PSIZE;
    float gl_ClipDistance[1] : SV_ClipDistance;
    float gl_CullDistance[1] : SV_CullDistance;
};

struct type10 {
    float2 member : LOC0;
    float4 gl_Position1 : SV_Position;
    float gl_PointSize1 : PSIZE;
    float gl_ClipDistance1[1] : SV_ClipDistance;
    float gl_CullDistance1[1] : SV_CullDistance;
};

static float2 v_uv;
static float2 a_uv;
static gl_PerVertex perVertexStruct;
static float2 a_pos;

struct VertexInput {
    float2 a_uv2 : LOC1;
    float2 a_pos2 : LOC0;
};

void main()
{
    float2 _expr12 = a_uv;
    v_uv = _expr12;
    float2 _expr13 = a_pos;
    perVertexStruct.gl_Position = float4(_expr13.x, _expr13.y, 0.0, 1.0);
    return;
}

type10 vert_main(VertexInput vertexinput)
{
    a_uv = vertexinput.a_uv2;
    a_pos = vertexinput.a_pos2;
    main();
    float2 _expr10 = v_uv;
    float4 _expr11 = perVertexStruct.gl_Position;
    float _expr12 = perVertexStruct.gl_PointSize;
    float _expr13[] = perVertexStruct.gl_ClipDistance;
    float _expr14[] = perVertexStruct.gl_CullDistance;
    const type10 type10_ = { _expr10, _expr11, _expr12, _expr13, _expr14 };
    return type10_;
}
./dxc.exe ./shader.hlsl -T vs_5_0 -E vert_main -Wno-parentheses-equality -Zi -Qembed_debug
Internal compiler error: access violation. Attempted to read from address 0x0000000000000009

The shader is not valid, generated by work in progress hlsl-out backend in naga, but dxc should be able to parse it without any ICE.

pow2clk commented 3 months ago

Stull repros: https://godbolt.org/z/e9afnMKh3

damyanp commented 2 months ago

In triage today we were unable to figure out why this shader is invalid.

llvm-beanz commented 2 months ago

FXC compiles this fine: https://shader-playground.timjones.io/2948b7c0adb063dc3ffe0da392078058

tex3d commented 2 months ago

This is a problem likely caused by some custom assignment handling in SemaHLSL circumventing the usual path in clang that would update the incomplete decl type.

It causes this assert during clang CodeGen:

  assert(!isIncompleteType() && "This doesn't make sense for incomplete types");

under Ty->isConstantSizeType() called from CodeGenFunction::EmitAutoVarAlloca for float _expr13[], where the type is incomplete, since it hasn't been set properly from the assignment initialization.

BTW: the shader is invalid because only the gl_Position field of perVertexStruct is initialized, and the other uninitialized fields are used. FXC just outputs zero for these uninitialized values, with no warning. Initializing these fields does not make the assert go away.

tex3d commented 2 months ago

Note: C++ wouldn't actually support this syntax, requiring {} for aggregate initializer. HLSL supports array assignment initialization, unlike C++, but breaks in the case where the assignee decl is an incomplete array type. A much more minimal case can be constructed that causes the assert to fire, but does not cause a crash on release (resulting in bad codegen instead).

It is somewhat important to be aware of this case so we can more tightly define what HLSL does for new implementation. Here are some options:

We should also decide whether we would like to deprecate this kind of array assignment in a future language version (since it's unsupported in C++). This assignment makes sense in HLSL because arrays are always by-value aggregates instead of decaying to a pointer.

tex3d commented 2 months ago

Apparently, FXC allows and handles this kind of initialization properly, so we could say that the HLSL language can and should support it. Then we could leave this in DXC's backlog as something that could be fixed maybe one day by someone who cares about this scenario.

Here is a minimal repro that:

int array1[1];

int main() : OUT {
    int error_expr[] = array1;
    return error_expr[0];
}