SPIRV: don't generate invalid debug instructions (#5397)

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: https://github.com/KhronosGroup/SPIRV-Registry/issues/203

---------

Signed-off-by: Nathan Gauër <[email protected]>

tools/clang/lib/SPIRV/DebugTypeVisitor.cpp

@@ -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)) {
@@ -174,6 +182,7 @@ void DebugTypeVisitor::lowerDebugTypeMembers(
       }
     }
   }
+#endif
 }
 
 SpirvDebugTypeTemplate *DebugTypeVisitor::lowerDebugTypeTemplate(

tools/clang/lib/SPIRV/SpirvEmitter.cpp

@@ -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;
 

tools/clang/test/CodeGenSPIRV/rich.debug.member.function.param.hlsl

@@ -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"
@@ -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
 

tools/clang/test/CodeGenSPIRV/rich.debug.type.composite.hlsl

@@ -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"
@@ -31,7 +38,10 @@ 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
@@ -39,8 +49,11 @@ struct foo {
 // 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;

tools/clang/test/CodeGenSPIRV/rich.debug.type.composite.warning.hlsl

@@ -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

tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp

@@ -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");
 }