Skip to content

Commit

Permalink
SPIRV: don't generate invalid debug instructions (microsoft#5397)
Browse files Browse the repository at this point in the history
When generating debug instructions for a function, each one is linked to
its scope.
In case of member functions, this scope is the class.

When declaring a class, all its member, including functions must be
declared.

This cycle requires a forward reference, which is allowed by the debug
instruction spec, but not by parents: NonSemantic & SPIR-V specs. This
is a spec issue we have to fix. In the meantime, we decided to emit a
warning, and generate slightly worse debug instructions.

Context: KhronosGroup/SPIRV-Registry#203

---------

Signed-off-by: Nathan Gauër <[email protected]>
  • Loading branch information
Keenuts authored Jul 17, 2023
1 parent 68c7d38 commit f4dcc6e
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 5 deletions.
9 changes: 9 additions & 0 deletions tools/clang/lib/SPIRV/DebugTypeVisitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,14 @@ void DebugTypeVisitor::lowerDebugTypeMembers(
assert(false && "Uknown DeclContext for DebugTypeMember generation");
}

// Note:
// The NonSemantic.Shader.DebugInfo.100 way to define member functions
// breaks both the NonSemantic and SPIR-V specification. Until this is
// resolved, we cannot emit debug instructions for member functions without
// creating invalid forward references.
//
// See https://github.com/KhronosGroup/SPIRV-Registry/issues/203
#if 0
// Push member functions to DebugTypeComposite Members operand.
for (auto *subDecl : decl->decls()) {
if (const auto *methodDecl = dyn_cast<FunctionDecl>(subDecl)) {
Expand All @@ -174,6 +182,7 @@ void DebugTypeVisitor::lowerDebugTypeMembers(
}
}
}
#endif
}

SpirvDebugTypeTemplate *DebugTypeVisitor::lowerDebugTypeTemplate(
Expand Down
6 changes: 6 additions & 0 deletions tools/clang/lib/SPIRV/SpirvEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,12 @@ void SpirvEmitter::HandleTranslationUnit(ASTContext &context) {
if (context.getDiagnostics().hasErrorOccurred())
return;

if (spirvOptions.debugInfoRich) {
emitWarning("Member functions will not be linked to their class in the debug information. "
"See https://github.com/KhronosGroup/SPIRV-Registry/issues/203",
{});
}

TranslationUnitDecl *tu = context.getTranslationUnitDecl();
uint32_t numEntryPoints = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ struct foo {
bool c;
};

// Note:
// For member functions, the scope in the debug info should be the encapsulating composite.
// Doing this (as specified in the NonSemantic.Shader.DebugInfo.100) would break the SPIR-V and NonSemantic
// spec. For this reason, DXC is emitting the wrong scope.
// See https://github.com/KhronosGroup/SPIRV-Registry/issues/203

// CHECK: [[set:%\d+]] = OpExtInstImport "OpenCL.DebugInfo.100"

// CHECK: [[fooName:%\d+]] = OpString "foo"
Expand All @@ -31,19 +37,26 @@ struct foo {
// CHECK: [[arg:%\d+]] = OpString "arg"

// CHECK: [[bool:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic {{%\d+}} %uint_32 Boolean
// CHECK: [[unit:%\d+]] = OpExtInst %void [[set]] DebugCompilationUnit 1 4 {{%\d+}} HLSL
// CHECK: [[foo:%\d+]] = OpExtInst %void [[set]] DebugTypeComposite [[fooName]] Structure {{%\d+}} 3 8

// CHECK: [[float:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic {{%\d+}} %uint_32 Float
// CHECK: [[int:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic {{%\d+}} %uint_32 Signed

// CHECK: [[func1Type:%\d+]] = OpExtInst %void [[set]] DebugTypeFunction FlagIsProtected|FlagIsPrivate [[int]] [[foo]] [[int]] [[float]] [[bool]]
// CHECK: [[f1:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func1]] [[func1Type]] {{%\d+}} 12 3 [[foo]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1

// CHECK-NOT: [[f1:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func1]] [[func1Type]] {{%\d+}} 12 3 [[foo]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1
// CHECK: [[f1:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func1]] [[func1Type]] {{%\d+}} 12 3 [[unit]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1

// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[arg2]] {{%\d+}} {{%\d+}} 12 40 [[f1]] FlagIsLocal 4
// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[arg1]] {{%\d+}} {{%\d+}} 12 29 [[f1]] FlagIsLocal 3
// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[arg0]] {{%\d+}} {{%\d+}} 12 17 [[f1]] FlagIsLocal 2
// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[this]] [[foo]] {{%\d+}} 12 3 [[f1]] FlagArtificial|FlagObjectPointer 1
// CHECK: [[func0Type:%\d+]] = OpExtInst %void [[set]] DebugTypeFunction FlagIsProtected|FlagIsPrivate %void [[foo]] [[float]]
// CHECK: [[f0:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func0]] [[func0Type]] {{%\d+}} 6 3 [[foo]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0

// CHECK-NOT: [[f0:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func0]] [[func0Type]] {{%\d+}} 6 3 [[foo]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0
// CHECK: [[f0:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func0]] [[func0Type]] {{%\d+}} 6 3 [[unit]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0

// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[arg]] {{%\d+}} {{%\d+}} 6 20 [[f0]] FlagIsLocal 2
// CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugLocalVariable [[this]] [[foo]] {{%\d+}} 6 3 [[f0]] FlagArtificial|FlagObjectPointer 1

Expand Down
19 changes: 16 additions & 3 deletions tools/clang/test/CodeGenSPIRV/rich.debug.type.composite.hlsl
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,14 @@ struct foo {
bool c;
};

// Note:
// For member functions, the scope in the debug info should be the encapsulating composite.
// Doing this (as specified in the NonSemantic.Shader.DebugInfo.100) would break the SPIR-V and NonSemantic
// spec. For this reason, DXC is emitting the wrong scope.
// See https://github.com/KhronosGroup/SPIRV-Registry/issues/203

// CHECK: [[set:%\d+]] = OpExtInstImport "OpenCL.DebugInfo.100"

// CHECK: [[bool_name:%\d+]] = OpString "bool"
// CHECK: [[foo:%\d+]] = OpString "foo"
// CHECK: [[c_name:%\d+]] = OpString "c"
Expand All @@ -31,16 +38,22 @@ struct foo {
// CHECK: [[func0:%\d+]] = OpString "foo.func0"

// CHECK: [[bool:%\d+]] = OpExtInst %void %1 DebugTypeBasic [[bool_name]] %uint_32 Boolean
// CHECK: [[parent:%\d+]] = OpExtInst %void [[set]] DebugTypeComposite [[foo]] Structure {{%\d+}} 3 8 {{%\d+}} {{%\d+}} %uint_192 FlagIsProtected|FlagIsPrivate [[a:%\d+]] [[b:%\d+]] [[c:%\d+]] [[f0:%\d+]] [[f1:%\d+]]
// CHECK: [[unit:%\d+]] = OpExtInst %void [[set]] DebugCompilationUnit 1 4 {{%\d+}} HLSL

// CHECK-NOT: [[parent:%\d+]] = OpExtInst %void [[set]] DebugTypeComposite [[foo]] Structure {{%\d+}} 3 8 {{%\d+}} {{%\d+}} %uint_192 FlagIsProtected|FlagIsPrivate [[a:%\d+]] [[b:%\d+]] [[c:%\d+]] {{%\d+}} {{%\d+}}
// CHECK: [[parent:%\d+]] = OpExtInst %void [[set]] DebugTypeComposite [[foo]] Structure {{%\d+}} 3 8 {{%\d+}} {{%\d+}} %uint_192 FlagIsProtected|FlagIsPrivate [[a:%\d+]] [[b:%\d+]] [[c:%\d+]]

// CHECK: [[c]] = OpExtInst %void [[set]] DebugTypeMember [[c_name]] [[bool]] {{%\d+}} 19 8 [[parent]] %uint_160 %uint_32 FlagIsProtected|FlagIsPrivate
// CHECK: [[float:%\d+]] = OpExtInst %void %1 DebugTypeBasic [[float_name]] %uint_32 Float
// CHECK: [[v4f:%\d+]] = OpExtInst %void %1 DebugTypeVector [[float]] 4
// CHECK: [[b]] = OpExtInst %void [[set]] DebugTypeMember [[b_name]] [[v4f]] {{%\d+}} 10 10 [[parent]] %uint_32 %uint_128 FlagIsProtected|FlagIsPrivate
// CHECK: [[int:%\d+]] = OpExtInst %void %1 DebugTypeBasic [[int_name]] %uint_32 Signed
// CHECK: [[a]] = OpExtInst %void [[set]] DebugTypeMember [[a_name]] [[int]] {{%\d+}} 4 7 [[parent]] %uint_0 %uint_32 FlagIsProtected|FlagIsPrivate
// CHECK: [[f1]] = OpExtInst %void [[set]] DebugFunction [[func1]] {{%\d+}} {{%\d+}} 12 3 [[parent]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1
// CHECK: [[f0]] = OpExtInst %void [[set]] DebugFunction [[func0]] {{%\d+}} {{%\d+}} 6 3 [[parent]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0

// CHECK-NOT: [[f1:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func1]] {{%\d+}} {{%\d+}} 12 3 [[parent]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1
// CHECK-NOT: [[f0:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func0]] {{%\d+}} {{%\d+}} 6 3 [[parent]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0
// CHECK: [[f1:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func1]] {{%\d+}} {{%\d+}} 12 3 [[unit]] {{%\d+}} FlagIsProtected|FlagIsPrivate 12 %foo_func1
// CHECK: [[f0:%\d+]] = OpExtInst %void [[set]] DebugFunction [[func0]] {{%\d+}} {{%\d+}} 6 3 [[unit]] {{%\d+}} FlagIsProtected|FlagIsPrivate 6 %foo_func0

float4 main(float4 color : COLOR) : SV_TARGET {
foo a;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// RUN: %dxc -T ps_6_0 -E main -fspv-debug=rich

struct foo {
void method() { }
};

float4 main(float4 color : COLOR) : SV_TARGET {
return color;
}

// CHECK: warning: Member functions will not be linked to their class in the debug information. See https://github.com/KhronosGroup/SPIRV-Registry/issues/203
3 changes: 3 additions & 0 deletions tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3026,6 +3026,9 @@ TEST_F(FileTest, DISABLED_RichDebugInfoMemberFunctionWithoutCall) {
TEST_F(FileTest, RichDebugInfoTypeComposite) {
runFileTest("rich.debug.type.composite.hlsl");
}
TEST_F(FileTest, RichDebugInfoTypeCompositeEmitsWarning) {
runFileTest("rich.debug.type.composite.warning.hlsl", Expect::Warning);
}
TEST_F(FileTest, RichDebugInfoTypeCompositeEmpty) {
runFileTest("rich.debug.type.composite.empty.hlsl");
}
Expand Down

0 comments on commit f4dcc6e

Please sign in to comment.