KhronosGroup / KhronosGroup/glslang

Wrong `NonSemantic.Shader.DebugInfo` source location for included struct

Open
#4,394 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
3.6k
Forks
989
Avg merge
1d 2h
Merged PRs (30d)
31

Description

Hi,

When a struct is defined in an included file and used in a push constant range (or presumably any other block), the `DebugTypeMember` instructions for that struct's members reference the correct lines and columns from the included file, but the wrong `DebugSource` instruction. The `DebugSource` instruction used is the one which references the file where the push constant range is defined (instead of where the struct is defined).
The `DebugTypeComposite` instruction for the struct itself references the source location of the push constant range (both file and line).

This issue manifests itself later as SPIR-V validation errors, e.g.
```
error: line 55: NonSemantic.Shader.DebugInfo.100 DebugTypeMember: operand Column End (11) is larger then Line 2 column length of 1 found in the DebugSource text
%32 = OpExtInst %void %1 DebugTypeMember %33 %30 %19 %uint_2 %uint_11 %uint_0 %uint_0 %uint_3
```

Here's a simple example:
`my_struct.glsl`:
```glsl
struct MyStruct {
float x;
};
```
`shader.comp`:
```glsl
#version 460

#extension GL_GOOGLE_include_directive : require

#include "my_struct.glsl"

layout(push_constant, std430) uniform PushConstRange {
MyStruct s;
};

void main() {}
```

```shell
glslang -o shader.comp.spv -gVS -Od --target-env vulkan1.3 shader.comp
spirv-val shader.comp.spv # fails
```

```llvm
%2 = OpString "shader.comp"
%3 = OpString "./my_struct.glsl"
%29 = OpString "float"
%33 = OpString "x"
%35 = OpString "MyStruct"
%19 = OpExtInst %void %1 DebugSource %2 %20
%30 = OpExtInst %void %1 DebugTypeBasic %29 %uint_32 %uint_3 %uint_0
%32 = OpExtInst %void %1 DebugTypeMember %33 %30 %19 %uint_2 %uint_11 %uint_0 %uint_0 %uint_3
%34 = OpExtInst %void %1 DebugTypeComposite %35 %uint_1 %19 %uint_7 %uint_0 %22 %35 %uint_0 %uint_3 %32
```

The issue is similar to #3862, but that deals with built-in variables, not included files. The same workaround works though (just zero out `Line` and `Column` in the `DebugTypeMember` instruction).
A better workaround I found for the `DebugTypeMember` issue is to simply pass the correct source ID as part of `StructMemberDebugInfo`:
```diff
diff --git a/SPIRV/SpvBuilder.cpp b/SPIRV/SpvBuilder.cpp
@@ -1300,7 +1300,7 @@
type->addIdOperand(getStringId(debugTypeLoc.name)); // name id
type->addIdOperand(debugTypeLoc.debugTypeOverride != 0 ? debugTypeLoc.debugTypeOverride
: getDebugType(memberType)); // type id
- type->addIdOperand(makeDebugSource(currentFileId)); // source id
+ type->addIdOperand(makeDebugSource(debugTypeLoc.sourceIdOverride ? debugTypeLoc.sourceIdOverride : currentFileId)); // source id
type->addIdOperand(makeUintConstant(debugTypeLoc.line)); // line id TODO: currentLine is always zero
type->addIdOperand(makeUintConstant(debugTypeLoc.column)); // TODO: column id
type->addIdOperand(makeUintConstant(0)); // TODO: offset id
diff --git a/SPIRV/GlslangToSpv.cpp b/SPIRV/GlslangToSpv.cpp
@@ -6412,6 +6412,9 @@
debugInfo.name = glslangMember.type->getFieldName();
debugInfo.line = glslangMember.loc.line;
debugInfo.column = glslangMember.loc.column;
+ if (const char *const filename = glslangMember.loc.getFilename()) {
+ debugInfo.sourceIdOverride = builder.getStringId(filename);
+ }

// Per the GLSL spec, bool variables inside of a uniform or buffer block are generated as uint.
// But for debug info, we want to represent them as bool because that is the original type in
diff --git a/SPIRV/SpvBuilder.h b/SPIRV/SpvBuilder.h
@@ -88,6 +88,7 @@
int column {0};
// Set if the caller knows a better debug type than what is associated with the functional SPIR-V type.
spv::Id debugTypeOverride {0};
+ spv::Id sourceIdOverride {0};
};

class Builder {
```
Of course neither workaround fixes the `DebugTypeComposite` instruction.

Contributor guide

Open the contributing guide

Research direction

Reproduce the issue with the shown glslang and spirv-val commands, then inspect SPIRV/SpvBuilder.cpp, SPIRV/GlslangToSpv.cpp, and SPIRV/SpvBuilder.h around StructMemberDebugInfo and DebugTypeComposite generation. Ensure included-struct members and the composite use the included file's source location, then confirm the generated SPIR-V passes spirv-val.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.