Skip to content

Add command line option for separate debug info#7255

Closed
jarcherNV wants to merge 7 commits intoshader-slang:masterfrom
jarcherNV:jarcher-dev-isseu-7098-alt
Closed

Add command line option for separate debug info#7255
jarcherNV wants to merge 7 commits intoshader-slang:masterfrom
jarcherNV:jarcher-dev-isseu-7098-alt

Conversation

@jarcherNV
Copy link
Copy Markdown
Collaborator

Add command line arg -separate-debug-info which, if provided, produces both a .spv and a .dbg.spv file. The .dbg.spv file contains full debug info and the .spv file has all debug info stripped out.

@jarcherNV jarcherNV requested a review from a team as a code owner May 27, 2025 21:18
@jarcherNV
Copy link
Copy Markdown
Collaborator Author

jarcherNV commented May 27, 2025

This is an alternative to #7178

Note that unlike 7178, this PR does not work for two reasons:

  1. spirv-opt crashes when trying to strip debug and non-semantic instructions. I have a fix for this locally, currently in review at Ignore cleared debug instructions KhronosGroup/SPIRV-Tools#6154
  2. spirv-opt does not strip out all debug info, so the test I have added fails as the debug strings remain in the SPIRV output and consequently the CHECK-NOT fails as it finds itself in the comments of one of the debug strings.

Edit: I have fixed the problem from (2), but we will still need the upstream fix for (1).

@jarcherNV jarcherNV force-pushed the jarcher-dev-isseu-7098-alt branch from dc7f9db to 963afcc Compare May 27, 2025 21:42
@jarcherNV jarcherNV added the pr: non-breaking PRs without breaking changes label May 27, 2025
@jarcherNV jarcherNV force-pushed the jarcher-dev-isseu-7098-alt branch from 963afcc to c5d27d1 Compare May 27, 2025 23:27
Add command line arg -separate-debug-info which, if provided, produces
both a .spv and a .dbg.spv file. The .dbg.spv file contains full debug
info and the .spv file has all debug info stripped out.

This patch also updates external/spirv-tools to point at the slang
fork which has a patch needed for this to work. We will change this
back when the patch is upstreamed.
@jarcherNV jarcherNV force-pushed the jarcher-dev-isseu-7098-alt branch from c5d27d1 to 328dec2 Compare May 28, 2025 23:18
@jarcherNV
Copy link
Copy Markdown
Collaborator Author

I have updated the patch to point spirv-tools at the Slang fork, which has the needed fix for this to work. We will need to change this back once the patch is upstreamed (assuming all the tests pass).

@jarcherNV
Copy link
Copy Markdown
Collaborator Author

/format

@slangbot
Copy link
Copy Markdown
Contributor

🌈 Formatted, please merge the changes from this PR

@jarcherNV
Copy link
Copy Markdown
Collaborator Author

/regenerate-cmdline-ref

@slangbot
Copy link
Copy Markdown
Contributor

🌈 Regenerated command line reference, please merge the changes from this PR

@jarcherNV
Copy link
Copy Markdown
Collaborator Author

After some recent discussion with Aftermath team it sounds like we may want to use the #7178 PR instead. I will leave this open for now in case we change our minds, but likely to close without submitting.

@jarcherNV jarcherNV marked this pull request as draft May 30, 2025 23:16
@csyonghe
Copy link
Copy Markdown
Collaborator

csyonghe commented Jun 3, 2025

We should also consider adding a unit test to look for the debug identifier in the spirv that has the debug info.

@jarcherNV
Copy link
Copy Markdown
Collaborator Author

We have decided not to use this approach so I am closing this PR.

@jarcherNV jarcherNV closed this Jun 5, 2025
@jarcherNV jarcherNV deleted the jarcher-dev-isseu-7098-alt branch June 5, 2025 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants