Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
`DebugInfoManager::ConvertDebugGlobalToLocalVariable` incorrectly converts a debug global variable to a debug local variable.
## Summary

Hey all,

I believe this code in `DebugInfoManager::ConvertDebugGlobalToLocalVariable` incorrectly handles converting the operands from `DebugGlobalVariable` to `DebugLocalVariable`.

https://github.com/KhronosGroup/SPIRV-Tools/blob/edc68950bf725edc89b3e1974c533454cf2ae37c/source/opt/debug_info_manager.cpp#L775-L778

`NumInOperands` (typically returning 11) only represents the input operands count but `kDebugLocalVariableOperandFlagsIndex` is written in absolute operands (10).

Additionally, I think we want `i > kDebugLocalVariableOperandFlagsIndex` not `i >= kDebugLocalVariableOperandFlagsIndex` since we don't want to erase the flags operand.

I believe the correct code should look similar to:

```
for (uint32_t i = dbg_global_var->NumOperands() - 1; i > kDebugLocalVariableOperandFlagsIndex; --i) {
    dbg_global_var->RemoveOperand(i);
}
```

## Repro Steps

I've included an example case using Shaderplayground and DXC:

https://shader-playground.timjones.io/b7f995367a738273718f6813ec847b8e

`%37 = OpExtInst %void %1 DebugLocalVariable %14 %17 %23 %uint_6 %uint_20 %24 %uint_8 %uint_8`

Notice that `%37` represents the global `Pi` but notice the `args` operand is set to `%uint_8` even though this is a global and not the 8th argument in a function.

When we disable optimizations we see the original variable declaration before the conversion.

https://shader-playground.timjones.io/c9f52e4e48b2de44f65e185b3e800a78

`%48 = OpExtInst %void %1 DebugGlobalVariable %47 %19 %24 %uint_6 %uint_20 %25 %47 %Pi %uint_8`

## Conclusion

Hopefully this provides the information necessary. Let me know if there's anything else I can do to help.

Apologies for not directly submitting a patch. I don't have the appropriate testing environment set up.


Discussion / hints:

Fix patch:
diff --git a/source/opt/debug_info_manager.cpp b/source/opt/debug_info_manager.cpp
index 1e614c6ff3..24094b36ef 100644
--- a/source/opt/debug_info_manager.cpp
+++ b/source/opt/debug_info_manager.cpp
@@ -768,15 +768,29 @@ void DebugInfoManager::ConvertDebugGlobalToLocalVariable(
          local_var->opcode() == spv::Op::OpFunctionParameter);
 
   // Convert |dbg_global_var| to DebugLocalVariable
+  // All of the operands up to the scope operand are the same for the type
+  // instructions. The flag operand needs to move from operand
+  // kDebugGlobalVariableOperandFlagsIndex to
+  // kDebugLocalVariableOperandFlagsIndex. No other operands are needed to
+  // define the DebugLocalVariable.
+
+  // Modify the opcode.
   dbg_global_var->SetInOperand(kExtInstInstructionInIdx,
                                {CommonDebugInfoDebugLocalVariable});
+
+  // Move the flags operand.
   auto flags = dbg_global_var->GetSingleWordOperand(
       kDebugGlobalVariableOperandFlagsIndex);
-  for (uint32_t i = dbg_global_var->NumInOperands() - 1;
-       i >= kDebugLocalVariableOperandFlagsIndex; --i) {
+  dbg_global_var->SetOperand(kDebugLocalVariableOperandFlagsIndex, {flags});
+
+  // Remove the  extra operands. Starting at the end to avoid copying too much
+  // data.
+  for (uint32_t i = dbg_global_var->NumOperands() - 1;
+       i > kDebugLocalVariableOperandFlagsIndex; --i) {
     dbg_global_var->RemoveOperand(i);
   }
-  dbg_global_var->SetOperand(kDebugLocalVariableOperandFlagsIndex, {flags});
+
+  // Update the def-use manager.
   context()->ForgetUses(dbg_global_var);
   context()->AnalyzeUses(dbg_global_var);
 


Test patch:
diff --git a/test/opt/debug_info_manager_test.cpp b/test/opt/debug_info_manager_test.cpp
index 3df26a9765..9c75728030 100644
--- a/test/opt/debug_info_manager_test.cpp
+++ b/test/opt/debug_info_manager_test.cpp
@@ -799,6 +799,84 @@ void main(float in_var_color : COLOR) {
             7);
 }
 
+TEST(DebugInfoManager, ConvertGlobalToLocal) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "PSMain" %3
+               OpExecutionMode %2 OriginUpperLeft
+          %4 = OpString "C:\\local\\Temp\\2528091a-6811-4e62-9ed5-02f1547c2016.hlsl"
+          %5 = OpString "float"
+          %6 = OpString "Pi"
+      %float = OpTypeFloat 32
+%float_3_1415 = OpConstant %float 3.1415
+       %uint = OpTypeInt 32 0
+    %uint_32 = OpConstant %uint 32
+%_ptr_Private_float = OpTypePointer Private %float
+%_ptr_Function_float = OpTypePointer Function %float
+       %void = OpTypeVoid
+     %uint_3 = OpConstant %uint 3
+     %uint_0 = OpConstant %uint 0
+     %uint_4 = OpConstant %uint 4
+     %uint_1 = OpConstant %uint 1
+     %uint_5 = OpConstant %uint 5
+     %uint_8 = OpConstant %uint 8
+     %uint_6 = OpConstant %uint 6
+    %uint_20 = OpConstant %uint 20
+         %25 = OpTypeFunction %void
+    %uint_11 = OpConstant %uint 11
+          %3 = OpVariable %_ptr_Private_float Private
+          %8 = OpExtInst %void %1 DebugTypeBasic %5 %uint_32 %uint_3 %uint_0
+         %12 = OpExtInst %void %1 DebugSource %4
+         %13 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %12 %uint_5
+         %17 = OpExtInst %void %1 DebugGlobalVariable %6 %8 %12 %uint_6 %uint_20 %13 %6 %3 %uint_8
+          %2 = OpFunction %void None %25
+         %27 = OpLabel
+         %29 = OpVariable %_ptr_Function_float Function
+               OpStore %3 %float_3_1415
+         %28 = OpExtInst %void %1 DebugLine %12 %uint_11 %uint_11 %uint_1 %uint_1
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  std::unique_ptr<IRContext> context =
+      BuildModule(SPV_ENV_UNIVERSAL_1_1, nullptr, text,
+                  SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  auto* def_use_mgr = context->get_def_use_mgr();
+  auto* dbg_var = def_use_mgr->GetDef(17);
+  EXPECT_EQ(dbg_var->GetCommonDebugOpcode(),
+            OpenCLDebugInfo100DebugGlobalVariable);
+  EXPECT_EQ(dbg_var->NumInOperands(), 11);
+
+  std::vector<Operand> originalOperands;
+  for (uint32_t i = 0; i < dbg_var->NumInOperands(); ++i) {
+    originalOperands.emplace_back(dbg_var->GetInOperand((i)));
+  }
+
+  auto* local_var = def_use_mgr->GetDef(29);
+  auto* dbg_info_mgr = context->get_debug_info_mgr();
+  dbg_info_mgr->ConvertDebugGlobalToLocalVariable(dbg_var, local_var);
+
+  EXPECT_EQ(dbg_var->NumInOperands(), 9);
+
+  // This checks that the first two inoperands are correct.
+  EXPECT_EQ(dbg_var->GetCommonDebugOpcode(),
+            OpenCLDebugInfo100DebugLocalVariable);
+
+  // Then next 6 operands should be the same as the original instruction.
+  EXPECT_EQ(dbg_var->GetInOperand(2), originalOperands[2]);
+  EXPECT_EQ(dbg_var->GetInOperand(3), originalOperands[3]);
+  EXPECT_EQ(dbg_var->GetInOperand(4), originalOperands[4]);
+  EXPECT_EQ(dbg_var->GetInOperand(5), originalOperands[5]);
+  EXPECT_EQ(dbg_var->GetInOperand(6), originalOperands[6]);
+  EXPECT_EQ(dbg_var->GetInOperand(7), originalOperands[7]);
+
+  // The flags operand should have shifted because operand 8 and 9 in the global
+  // instruction are not relevant.
+  EXPECT_EQ(dbg_var->GetInOperand(8), originalOperands[10]);
+}
+
 }  // namespace
 }  // namespace analysis
 }  // namespace opt