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

OpDecorate should not allow an arbitrary number of arguments #34

Closed
dneto0 opened this issue Nov 30, 2015 · 2 comments
Closed

OpDecorate should not allow an arbitrary number of arguments #34

dneto0 opened this issue Nov 30, 2015 · 2 comments
Assignees
@dneto0

Comments

@dneto0
Copy link
Collaborator

dneto0 commented Nov 30, 2015

The following should assemble, but should not disassemble.

   OpDecorate %1 LinkageAttributes "abc" !1 !0

The !1 is 1, which is interpreted as linkage attribute "Import". The instruction ends up having 6 words, the last being value 0.
However, the binary parser (and disassembler) should reject this input because that last !0 word is extraneous.

The root cause is that the grammar in opcode.inc has a last operand class of OperandVariableLiterals, so that expands to zero or more literals. Instead that should be dropped, and we should allow processing of the decoration enum word to add just the right number and type of operands to the operand pattern.
@dneto0 dneto0 self-assigned this Nov 30, 2015
@dneto0
Copy link
Collaborator Author

dneto0 commented Nov 30, 2015

Credit to @dmircevski for starting the conversation about using short string inputs. The linkage attributes decoration is the only case where a string literal comes before yet another operand. Making a test case to cover that exposed this issue.

@dneto0
Copy link
Collaborator Author

dneto0 commented Nov 30, 2015

Even less exotic test case. This should not assemble, but it does:

│OpDecorate %1 LinkageAttributes "abc" Import 1 2 3

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Nov 30, 2015
@dneto0
OpDecorate should not accept any number of literal operands.
bd88a10 
This is a grammar fix.  The Decoration operand of OpDecorate (and
OpMemberDecorate) determines the remaining operands.  Don't just
allow any number of literal numbers as operands.

(The OperandVariableLiterals operand class as the last member
of the OpDecorate and OpMemberDecorate entries in in opcode.inc is
an artifact of how the spec generates the opcode descriptions. It's
not suitable for parsing those instructions.)

Fixes KhronosGroup#34
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Nov 30, 2015
@dneto0
OpDecorate should not accept any number of literal operands.
2005b50 
This is a grammar fix.  The Decoration operand of OpDecorate (and
OpMemberDecorate) determines the remaining operands.  Don't just
allow any number of literal numbers as operands.

(The OperandVariableLiterals operand class as the last member
of the OpDecorate and OpMemberDecorate entries in in opcode.inc is
an artifact of how the spec generates the opcode descriptions. It's
not suitable for parsing those instructions.)

Fixes KhronosGroup#34
@dneto0 dneto0 closed this as completed in 39fa148 Dec 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dneto0 dneto0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dneto0