glslang icon indicating copy to clipboard operation
glslang copied to clipboard

Attempting to generate debug info using non-semantic shader debug information results in "Id is 0"

Open danginsburg opened this issue 1 year ago • 9 comments

I am trying to generate shader debug information with slang (https://github.com/shader-slang/slang) which uses glslang under the hood. It's just been updated to commit 4f3ae4b (https://github.com/KhronosGroup/glslang/commit/8c8c6027937faad014feb583b05f345353d5ed6d) and this bug also reproduces with Vulkan SDK 1.3.246.1. glslangValidator:

Glslang Version: 11:12.1.0
ESSL Version: OpenGL ES GLSL 3.20 glslang Khronos. 12.1.0
GLSL Version: 4.60 glslang Khronos. 12.1.0
SPIR-V Version 0x00010600, Revision 1
GLSL.std.450 Version 100, Revision 1
Khronos Tool ID 8
SPIR-V Generator Version 11
GL_KHR_vulkan_glsl version 100
ARB_GL_gl_spirv version 100

To reproduce, with the attached shader, run:

glslangvalidator -gVS -S frag -Os -V 1.glsl

It gives the following error:

error: input:0:0:205: Id is 0

The error goes away if -Os is not used or -gV[S]is not used. So it's a bug with optimization and shader debug info. It could be spirv-opt related, but thought I'd start here.

Shader attached:

1.zip

danginsburg avatar Aug 11 '23 13:08 danginsburg

This appears to be triggered by the buffer_reference in the shader, here is a more stripped down version. If you comment out the buffer_reference the error does not occur:

#version 450
#extension GL_EXT_buffer_reference : require
#extension GL_ARB_enhanced_layouts : require

layout(buffer_reference, std430, buffer_reference_align = 16) readonly buffer BufferPointer_BDATest_t_0_1
{
    vec4 _data;
};

layout(binding = 0)
layout(std140) uniform _S2
{
    BufferPointer_BDATest_t_0_1 g_vBDAValue_0;
} ExternalTestCB1_0;

layout(location = 0) out vec4 _S5;


void main()
{
    _S5 = vec4(0,0,0,0);
    return;
}


danginsburg avatar Aug 11 '23 14:08 danginsburg

I'll take a look at this today. Given it's triggered only by -Os, I would suspect this is a SPIRV-Tools bug though.

arcady-lunarg avatar Aug 11 '23 15:08 arcady-lunarg

Actually, this is a bug in glslang's generation of debuginfo for buffer references, with a debug build and no -Os, I get an assertion failure.

glslangValidator: /home/arcady/glslang/SPIRV/SpvBuilder.cpp:990: spv::Id spv::Builder::makeMemberDebugType(const spv::Id, const spv::Builder::DebugTypeLoc &): Assertion `debugId[memberType] != 0' failed.

arcady-lunarg avatar Aug 11 '23 15:08 arcady-lunarg

In fact, I think the actual bug here is that no debug info is emitted for the buffer reference pointer type at all, so when a struct has a buffer reference member, the code that emits debug info for the struct's members tries to find the type of that member, can't find it, and asserts (in debug builds) or emits a 0 (in release builds).

arcady-lunarg avatar Aug 11 '23 21:08 arcady-lunarg

The original author generated SPIR-V for buffer references so that it would work if the buffer reference was recursive or not. So he always generated the pointer type after the struct that contained it.

I am guessing since the pointer type is not generated at the time the struct type is generated, the debug info for the pointer type is not generated at the time the debug info for the struct is being generated.

Since SPIR-V allows forward references in struct types, this policy works when generating the core SPIR-V. But since (currently) there is no way to do forward references for NonSemantic.Shader.DebugInfo, this won't fly.

The shader of this issue does not require forward references for either the core SPIR-V or the NonSemantic.Shader.DebugInfo, so there is no reason the NonSemantic.Shader.DebugInfo for this shader cannot be generated as the specs exist today. We will just have to add extra logic to glslangValidator to do it. Specifically, we will need glslangValidator to generate the debug info for the pointer type before the debug info for the struct type that contains it, when possible.

We still won't be able to generate NonSemantic.Shader.DebugInfo for truly recursive (linked-list) structs until the current plans for the SPIR-V and NonSemantic.Shader.DebugInfo specs https://github.com/KhronosGroup/SPIRV-Registry/issues/203 are completed.

greg-lunarg avatar Aug 15 '23 23:08 greg-lunarg

@greg-lunarg @arcady-lunarg

So the original kernel in the @danginsburg now compiles fine and doesn't produce errors, even when running it through spirv-val.

However, a slight variation of this kernel also "compiles", but generates invalid SPIRV:

#version 450
#extension GL_EXT_buffer_reference : require
#extension GL_ARB_enhanced_layouts : require

layout(buffer_reference, std430, buffer_reference_align = 16) buffer BufferPointer_BDATest_t_0_1
{
    vec4 _data;
};

layout(binding = 0)
layout(std140) uniform _S2
{
    BufferPointer_BDATest_t_0_1 g_vBDAValue_0;
} ExternalTestCB1_0;

void myFunc(BufferPointer_BDATest_t_0_1 pointer)
{
    pointer._data = vec4(0);
}

void main()
{
    myFunc(ExternalTestCB1_0.g_vBDAValue_0);
    return;
}

when compiling that with -gVS (and -V), it generates a SPIRV blob that doesn't pass through spirv-val: error: line 69: Id is 0

Here's the disassembly, generated by spirv-dis:

; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 11
; Bound: 91
; Schema: 0
               OpCapability Shader
               OpCapability PhysicalStorageBufferAddresses
               OpExtension "SPV_KHR_non_semantic_info"
               OpExtension "SPV_KHR_physical_storage_buffer"
          %1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
          %3 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel PhysicalStorageBuffer64 GLSL450
               OpEntryPoint GLCompute %14 "main"
               OpExecutionMode %14 LocalSize 1 1 1
          %2 = OpString "myshader.comp"
          %8 = OpString "uint"
         %18 = OpString "float"
         %25 = OpString "_data"
         %27 = OpString "// OpModuleProcessed client vulkan100
// OpModuleProcessed target-env vulkan1.0
// OpModuleProcessed entry-point main
#line 1
#version 450
#extension GL_EXT_buffer_reference : require
#extension GL_ARB_enhanced_layouts : require

layout(buffer_reference, std430, buffer_reference_align = 16) buffer BufferPointer_BDATest_t_0_1
{
    vec4 _data;
};

layout(binding = 0)
layout(std140) uniform _S2
{
    BufferPointer_BDATest_t_0_1 g_vBDAValue_0;
} ExternalTestCB1_0;

void myFunc(BufferPointer_BDATest_t_0_1 pointer)
{
    pointer._data = vec4(0);
}

void main()
{
    myFunc(ExternalTestCB1_0.g_vBDAValue_0);
    return;
}
"
         %31 = OpString "BufferPointer_BDATest_t_0_1"
         %42 = OpString "myFunc"
         %46 = OpString "pointer"
         %51 = OpString "main"
         %59 = OpString "int"
         %73 = OpString "_S2"
         %79 = OpString "ExternalTestCB1_0"
               OpSourceExtension "GL_ARB_enhanced_layouts"
               OpSourceExtension "GL_EXT_buffer_reference"
               OpName %14 "main"
               OpName %23 "BufferPointer_BDATest_t_0_1"
               OpMemberName %23 0 "_data"
               OpName %40 "myFunc(1;"
               OpName %39 "pointer"
               OpName %71 "_S2"
               OpMemberName %71 0 "g_vBDAValue_0"
               OpName %77 "ExternalTestCB1_0"
               OpName %81 "param"
               OpMemberDecorate %23 0 Offset 0
               OpDecorate %23 Block
               OpDecorate %39 AliasedPointer
               OpMemberDecorate %71 0 Offset 0
               OpDecorate %71 Block
               OpDecorate %77 DescriptorSet 0
               OpDecorate %77 Binding 0
               OpDecorate %81 AliasedPointer
          %4 = OpTypeVoid
          %5 = OpTypeFunction %4
          %7 = OpTypeInt 32 0
         %10 = OpConstant %7 32
         %11 = OpConstant %7 6
         %12 = OpConstant %7 0
          %9 = OpExtInst %4 %1 DebugTypeBasic %8 %10 %11 %12
         %13 = OpConstant %7 3
          %6 = OpExtInst %4 %1 DebugTypeFunction %13 %4
               OpTypeForwardPointer %16 PhysicalStorageBuffer
         %17 = OpTypeFloat 32
         %19 = OpExtInst %4 %1 DebugTypeBasic %18 %10 %13 %12
         %20 = OpTypeVector %17 4
         %21 = OpConstant %7 4
         %22 = OpExtInst %4 %1 DebugTypeVector %19 %21
         %23 = OpTypeStruct %20
         %26 = OpExtInst %4 %1 DebugSource %2 %27
         %28 = OpConstant %7 7
         %29 = OpConstant %7 10
         %24 = OpExtInst %4 %1 DebugTypeMember %25 %22 %26 %28 %29 %12 %12 %13
         %32 = OpConstant %7 1
         %34 = OpConstant %7 2
         %33 = OpExtInst %4 %1 DebugCompilationUnit %32 %21 %26 %34
         %30 = OpExtInst %4 %1 DebugTypeComposite %31 %32 %26 %12 %12 %33 %31 %12 %13 %24
         %16 = OpTypePointer PhysicalStorageBuffer %23
         %35 = OpTypePointer Function %16
         %36 = OpExtInst %4 %1 DebugInfoNone
         %37 = OpTypeFunction %4 %35

I did some brief debugging myself in glslang to find out what is going on. It seems to be happening around Builder::makeForwardPointer() area and how it ends up interacting with the debugId table. If you compile in debug, then an assert is eventually hit in addIdOperand() that checks that the supplied id is not 0.

So with glslang compiled as debug, it flat out crashes, and when compiled in release, it seems ends up outputting a 0 id somewhere in the spirv.

Is this enough information to go on? Should I post a separate issue for this?

RikoOphorst avatar Mar 20 '24 17:03 RikoOphorst

Pinging @jeremy-lunarg as well, I don't know how visible this comment is 😅

RikoOphorst avatar Mar 21 '24 11:03 RikoOphorst

There is going to be a more comprehensive solution to this soon, but it requires a new SPIR-V extension which is currently making its way through the approval process. I am going to take a look at your example to see if it can be fixed easily to not emit a 0 Id with the current scheme, and to make sure that it will work with the new solution with the new extension.

arcady-lunarg avatar Mar 21 '24 18:03 arcady-lunarg

Thanks a lot, I hope it's trivial enough to find a workaround! Also, is there an approximate ETA for that new SPIR-V extension to make it's way out into the wild?

RikoOphorst avatar Mar 22 '24 20:03 RikoOphorst