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.

Sign up for GitHub

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

Jump to bottom

spirv-fuzz: Invalidation when indexing into a structure #3671

Closed
andreperezmaselco opened this issue Aug 7, 2020 · 1 comment · Fixed by #3672
Closed

spirv-fuzz: Invalidation when indexing into a structure #3671

andreperezmaselco opened this issue Aug 7, 2020 · 1 comment · Fixed by #3672
Labels
component:fuzzer Relates to the spirv-fuzz tool

Comments

@andreperezmaselco
Copy link
Collaborator

spirv-val invalidated a fuzzed shader because the result id of an OpConstantNull instruction was being used to index into a structure. According to SPIR-V specification, when indexing into a structure, each index must be the result id of an OpConstant instruction.

error: line 79: The <id> passed to OpInBoundsAccessChain to index into a structure must be an OpConstant.
  %399 = OpConstantNull %uint

Debugging spirv-fuzz, I found that TransformationReplaceIdWithSynonym checks only if the index instruction is OpAccessChain, while the same is true for OpInBoundsAccessChain and OpPtrAccessChain.
@andreperezmaselco andreperezmaselco mentioned this issue Aug 7, 2020
Merged
@paulthomson paulthomson added the component:fuzzer Relates to the spirv-fuzz tool label Aug 11, 2020
@paulthomson
Copy link
Contributor

paulthomson commented Aug 12, 2020 •
edited
Loading

TransformationReplaceIdWithSynonym is careful to avoid replacing id uses that index into a struct with synonyms because the indices must only be OpConstant instructions. However, the check only considered OpAccessChain instructions, even though the same restriction applies to OpInBoundsAccessChain, OpPtrAccessChain, etc.

paulthomson pushed a commit that referenced this issue Aug 12, 2020
@andreperezmaselco
spirv-fuzz: consider additional access chain instructions (#3672)
2641d33 
`TransformationReplaceIdWithSynonym` is careful to avoid replacing id uses that index into a struct with synonyms because the indices must only be `OpConstant` instructions. However, the check only considered `OpAccessChain` instructions, even though the same restriction applies to `OpInBoundsAccessChain`, `OpPtrAccessChain`, etc. 

This change extends the check to include all access chain instructions.

Fixes #3671.
dnovillo pushed a commit to dnovillo/SPIRV-Tools that referenced this issue Aug 19, 2020
@andreperezmaselco @dnovillo
spirv-fuzz: consider additional access chain instructions (KhronosGro…
d5d1cc9 
…up#3672)

`TransformationReplaceIdWithSynonym` is careful to avoid replacing id uses that index into a struct with synonyms because the indices must only be `OpConstant` instructions. However, the check only considered `OpAccessChain` instructions, even though the same restriction applies to `OpInBoundsAccessChain`, `OpPtrAccessChain`, etc. 

This change extends the check to include all access chain instructions.

Fixes KhronosGroup#3671.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component:fuzzer Relates to the spirv-fuzz tool
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

spirv-fuzz: consider additional access chain instructions andreperezmaselco/SPIRV-Tools
2 participants
@paulthomson @andreperezmaselco