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

spirv-val: Check DebugLine Line/Column are valid #5986

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spencer-lunarg
Copy link
Contributor

I had to raise issue in Glslang because it would generate Line/Column that didn't actually point to any valid spot in the DebugSource's text

This adds some validation where if there is a DebugSource with Text then any Line/Column must be correct, otherwise every other consume tool (like Validation Layers) need to do this before trying to print out the snippet in the source code

@spencer-lunarg spencer-lunarg force-pushed the spencer-lunarg-debug-info-lines branch from d543267 to 5af732a Compare February 4, 2025 19:46
@spencer-lunarg
Copy link
Contributor Author

It seems these tests are showing the bug I found in GLSL (KhronosGroup/glslang#3863), where things like https://github.com/KhronosGroup/glslang/blob/main/Test/baseResults/spv.debuginfo.implicit_br.glsl.frag.out#L16 are adding extra things to the source, but not updating the Line number to match it

Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok for the most part. Prefer making the accessor more robust generally.

For my own sake, I think DebugSourceContinued can't ever be used as a 'Source' operand so all the lines and columns are relative to a DebugSource instruction.

@spencer-lunarg
Copy link
Contributor Author

For my own sake, I think DebugSourceContinued can't ever be used as a 'Source' operand so all the lines and columns are relative to a DebugSource instruction.

That is correct, we will always reference the DebugSource, but the Text operand in DebugSource might be incomplete, so we need to build it up to find the DebugSourceContinued after it

Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, but I've restarted the formatting bot.

@spencer-lunarg
Copy link
Contributor Author

@alan-baker what did we want to do with Glslang failing, should I try to push and get those fixes in first and then rebase this? (I know we have a protocol for these situations, I just haven't had to do it in awhile)

@alan-baker
Copy link
Contributor

@alan-baker what did we want to do with Glslang failing, should I try to push and get those fixes in first and then rebase this? (I know we have a protocol for these situations, I just haven't had to do it in awhile)

You can either fix glslang first if you want or merge this and update glslang to have failing tests until it is fixed. If glslang can be updated first, that's preferable.

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

Successfully merging this pull request may close these issues.

2 participants