Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DebugInfoManager::ConvertDebugGlobalToLocalVariable incorrectly converts a debug global variable to a debug local variable. #5776

Closed
AlexSabourinDev opened this issue Aug 27, 2024 · 0 comments · Fixed by #5803

Comments

@AlexSabourinDev
Copy link

Summary

Hey all,

I believe this code in DebugInfoManager::ConvertDebugGlobalToLocalVariable incorrectly handles converting the operands from DebugGlobalVariable to DebugLocalVariable.

for (uint32_t i = dbg_global_var->NumInOperands() - 1;
i >= kDebugLocalVariableOperandFlagsIndex; --i) {
dbg_global_var->RemoveOperand(i);
}

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.

@s-perron s-perron self-assigned this Sep 10, 2024
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Sep 12, 2024
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Sep 12, 2024
The code converting the global to local was generating an extra operand
that was incorrect. Fixed the code, and added a unit test.

Fixes KhronosGroup#5776
s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Sep 16, 2024
The code converting the global to local was generating an extra operand
that was incorrect. Fixed the code, and added a unit test.

Fixes KhronosGroup#5776
Keenuts pushed a commit to Keenuts/SPIRV-Tools that referenced this issue Nov 12, 2024
…onosGroup#5803)

The code converting the global to local was generating an extra operand
that was incorrect. Fixed the code, and added a unit test.

Fixes KhronosGroup#5776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants