-
Notifications
You must be signed in to change notification settings - Fork 538
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
Basic SSA Validation #27
Changes from 3 commits
7acf07c
d6aaa17
8033cdb
da35d28
b0a5367
3949bf4
70b6cf0
3017901
73bfa70
9691d3c
fc71d20
9f8db87
92d6b47
ad5add6
0fbc35c
2b61dbd
be947ef
89c4a21
ee4c90a
71b49c4
64190ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,10 @@ | |
#include <string.h> | ||
|
||
#include <vector> | ||
#include <iterator> | ||
#include <set> | ||
|
||
using std::vector; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The using declarations should come after all #includes. Otherwise you might mess up the type mappings seen by those other includes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
|
||
#include "binary.h" | ||
#include "diagnostic.h" | ||
|
@@ -80,6 +84,7 @@ spv_result_t spvValidateOperandValue(const spv_operand_type_t type, | |
spv_diagnostic* pDiagnostic) { | ||
switch (type) { | ||
case SPV_OPERAND_TYPE_ID: | ||
case SPV_OPERAND_TYPE_TYPE_ID: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also: SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID, SPV_OPERAND_TYPE_SCOPE_ID. Since you started working on this, I've reorganized the enumerants in spv_operand_type_t so these groupings are easier to see. |
||
case SPV_OPERAND_TYPE_RESULT_ID: { | ||
// NOTE: ID's are validated in SPV_VALIDATION_LEVEL_1, this is | ||
// SPV_VALIDATION_LEVEL_0 | ||
|
@@ -166,14 +171,15 @@ spv_result_t spvValidateBasic(const spv_instruction_t* pInsts, | |
// of the parse. | ||
spv_operand_type_t type = spvBinaryOperandInfo( | ||
word, index, opcodeEntry, operandTable, &operandEntry); | ||
|
||
if (SPV_OPERAND_TYPE_LITERAL_STRING == type) { | ||
spvCheckReturn(spvValidateOperandsString( | ||
words + index, wordCount - index, position, pDiagnostic)); | ||
// NOTE: String literals are always at the end of Opcodes | ||
break; | ||
} else if (SPV_OPERAND_TYPE_LITERAL_INTEGER == type) { | ||
spvCheckReturn(spvValidateOperandsLiteral( | ||
words + index, wordCount - index, 2, position, pDiagnostic)); | ||
// spvCheckReturn(spvValidateOperandsNumber( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you plan on re-enabling this code, then use: Otherwise just delete the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
// words + index, wordCount - index, 2, position, pDiagnostic)); | ||
} else { | ||
spvCheckReturn(spvValidateOperandValue(type, word, operandTable, | ||
position, pDiagnostic)); | ||
|
@@ -261,6 +267,143 @@ spv_result_t spvValidateIDs(const spv_instruction_t* pInsts, | |
return SPV_SUCCESS; | ||
} | ||
|
||
// TODO(umar): Validate header | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A subtle thing: the Id bound should be validated also. But you can only do that after you've seen all the instructions in the module. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI. The binary parser validates the magic word, and the length of the header, but nothing else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added notes da35d28 |
||
spv_result_t setHeader(void* user_data, spv_endianness_t endian, uint32_t magic, | ||
uint32_t version, uint32_t generator, uint32_t id_bound, | ||
uint32_t reserved) { | ||
(void)user_data; | ||
(void)endian; | ||
(void)version; | ||
(void)generator; | ||
(void)id_bound; | ||
(void)reserved; | ||
return SPV_SUCCESS; | ||
} | ||
|
||
// TODO(umar): Move this class to another file | ||
class validation_state_t { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Class names in CamelCase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
const uint32_t* binary; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the "binary" member is only used by the binary() method, which itself is never used. Remove it, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a relic which was based on an older version of spy_parsed_instruction_t struct which did not have the words pointer. Fixed da35d28 |
||
spv_diagnostic* diagnostic; | ||
const uint32_t* program_counter; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the program counter? What's it counting? I presume it's referencing someone else's storage? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From usage, seems this should be instruction_words_ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was a reference to the program counter register in a CPU. Its probably confusing in this context. I can change it to |
||
|
||
std::set<uint32_t> declared_ids; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer std::unordered_set. Regular sets are usually implemented as trees of nodes which use up a lot of space and are slow since there's so much pointer chasing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, these are class data members, so they should go at the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
std::set<uint32_t> forward_ids; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From usage, this isn't really forward_ids. It's unresolved_forward_ids. That is, it's the set of Ids for which we've seen a forward declaration but no definition. I think the name needs to be fixed, and we need a comment to describe it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Fixed da35d28 |
||
|
||
public: | ||
validation_state_t(const uint32_t* binary, spv_diagnostic* diag) | ||
: binary(binary), diagnostic(diag), program_counter(binary) {} | ||
|
||
void set_instruction_words(const uint32_t * const words) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much as I dislike camelcase (and I really do), convention is to use camelcase for method names. |
||
program_counter = words; | ||
} | ||
|
||
spv_result_t declare_id(uint32_t id) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All methods should have a description before their declaration. E.g. for this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, aren't these definitions, not declarations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Fixed da35d28 |
||
if (declared_ids.find(id) == std::end(declared_ids)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd usually write declared_ids.end(). Seems more direct. ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
declared_ids.insert(id); | ||
} else { | ||
return diag(SPV_ERROR_INVALID_ID) | ||
<< "ID cannot be assigned multiple times"; | ||
} | ||
return SPV_SUCCESS; | ||
} | ||
|
||
spv_result_t forward_declare_id(uint32_t id) { | ||
forward_ids.insert(id); | ||
return SPV_SUCCESS; | ||
} | ||
|
||
spv_result_t remove_if_fwd_declared(uint32_t id) { | ||
forward_ids.erase(id); | ||
return SPV_SUCCESS; | ||
} | ||
|
||
size_t forward_declared_id_count() { return forward_ids.size(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unresolved_forward_id_count(), or num_unresolved_forward_ids() |
||
|
||
bool check_declared_id(uint32_t id) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "check" is ambiguous. Prefer "isDefinedId" |
||
return declared_ids.find(id) != std::end(declared_ids); | ||
} | ||
|
||
const uint32_t* get_binary() const { return binary; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used? Remove it, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
|
||
const uint32_t* get_instruction_ptr() { return program_counter; } | ||
|
||
libspirv::DiagnosticStream diag(spv_result_t error_code) { | ||
return libspirv::DiagnosticStream({0, 0, (uint64_t)(program_counter - binary)}, | ||
diagnostic, error_code); | ||
} | ||
}; | ||
|
||
spv_result_t SSAPass(validation_state_t& _, bool can_have_forward_declared_ids, | ||
const spv_parsed_instruction_t* inst) { | ||
const uint32_t* inst_ptr = _.get_instruction_ptr(); | ||
|
||
for (int i = 0; i < inst->num_operands; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and by extension, i should probably be unsigned. |
||
spv_parsed_operand_t operand = inst->operands[i]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
spv_operand_type_t type = operand.type; | ||
const uint32_t* operand_ptr = inst_ptr + operand.offset; | ||
|
||
auto ret = SPV_ERROR_INTERNAL; | ||
switch (type) { | ||
case SPV_OPERAND_TYPE_RESULT_ID: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for the braces since you're not defining or declaring variables. But you can keep if you like. |
||
_.remove_if_fwd_declared(*operand_ptr); | ||
ret = _.declare_id(*operand_ptr); | ||
break; | ||
} | ||
case SPV_OPERAND_TYPE_TYPE_ID: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two other ID kinds you have to check: SPV_OPERAND_TYPE_MEMORY_SEMANTICS_ID, and SPV_OPERAND_TYPE_SCOPE_ID. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
case SPV_OPERAND_TYPE_ID: { | ||
if (can_have_forward_declared_ids) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For OpBranchConditional, this is not quite right. The condition Id must have been defined by now. It's the true branch and false branch that could be forward references. The general point is that whether an Id can be forward declared is a function of both opcode and argument index. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 3949bf4 |
||
if (_.check_declared_id(*operand_ptr)) { | ||
ret = SPV_SUCCESS; | ||
} else { | ||
ret = _.forward_declare_id(*operand_ptr); | ||
} | ||
} else { | ||
if (_.check_declared_id(*operand_ptr)) { | ||
ret = SPV_SUCCESS; | ||
} else { | ||
ret = _.diag(SPV_ERROR_INVALID_ID) << "ID " << *operand_ptr | ||
<< " has not been declared"; | ||
} | ||
} | ||
break; | ||
} | ||
default: | ||
ret = SPV_SUCCESS; | ||
break; | ||
} | ||
if (SPV_SUCCESS != ret) { | ||
return ret; | ||
} | ||
} | ||
return SPV_SUCCESS; | ||
} | ||
|
||
spv_result_t pushInstructions(void* user_data, | ||
const spv_parsed_instruction_t* const inst) { | ||
validation_state_t& _ = *(reinterpret_cast<validation_state_t*>(user_data)); | ||
_.set_instruction_words(inst->words); | ||
|
||
const uint32_t* inst_ptr = _.get_instruction_ptr(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, inst_ptr is just inst->words. No need to go through a method to get it out again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed da35d28 |
||
|
||
vector<SpvOp_> instructions_with_forward_ids = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's cleaner and likely faster to use a separate method bool InstructionAllowsForwardIdReferences(SpvOp) that just has a switch statement with the positive cases, a default that breaks out, and a default return of false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a placeholder. I have updated this to return a functor based off of the opcode and return true or false based on the index of the operand. See 3949bf4 |
||
SpvOpExecutionMode, SpvOpEntryPoint, SpvOpPhi, SpvOpFunctionCall, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OpSwitch is another branching instruction. But its selector Id must be defined (can't be a forward reference). Its default Id and case label Ids can be forward references. |
||
SpvOpName, SpvOpMemberName, SpvOpDecorate, SpvOpMemberDecorate, | ||
SpvOpGroupDecorate}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed 3949bf4 |
||
|
||
bool can_have_forward_declared_ids = false; | ||
for (auto& instruction : instructions_with_forward_ids) { | ||
can_have_forward_declared_ids |= inst->opcode == instruction; | ||
} | ||
|
||
auto err = SSAPass(_, can_have_forward_declared_ids, inst); | ||
|
||
if (err != SPV_SUCCESS) { | ||
return err; | ||
} | ||
|
||
return SPV_SUCCESS; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe L398 - L404 can be combined into one instruction |
||
} | ||
|
||
spv_result_t spvValidate(const spv_const_context context, | ||
const spv_const_binary binary, const uint32_t options, | ||
spv_diagnostic* pDiagnostic) { | ||
|
@@ -279,6 +422,19 @@ spv_result_t spvValidate(const spv_const_context context, | |
return SPV_ERROR_INVALID_BINARY; | ||
} | ||
|
||
validation_state_t vstate(binary->code, pDiagnostic); | ||
auto err = spvBinaryParse(context, &vstate, binary->code, binary->wordCount, setHeader, | ||
pushInstructions, pDiagnostic); | ||
|
||
if (vstate.forward_declared_id_count() > 0) { | ||
// TODO(umar): print undefined IDs | ||
return vstate.diag(SPV_ERROR_INVALID_ID) | ||
<< "Some forward referenced IDs have not be defined. \n"; | ||
} | ||
if (err) { | ||
return err; | ||
} | ||
|
||
// NOTE: Copy each instruction for easier processing | ||
std::vector<spv_instruction_t> instructions; | ||
uint64_t index = SPV_INDEX_INSTRUCTION; | ||
|
@@ -293,19 +449,6 @@ spv_result_t spvValidate(const spv_const_context context, | |
index += wordCount; | ||
} | ||
|
||
if (spvIsInBitfield(SPV_VALIDATE_BASIC_BIT, options)) { | ||
position.index = SPV_INDEX_INSTRUCTION; | ||
// TODO: Imcomplete implementation | ||
spvCheckReturn(spvValidateBasic( | ||
instructions.data(), instructions.size(), context->opcode_table, | ||
context->operand_table, &position, pDiagnostic)); | ||
} | ||
|
||
if (spvIsInBitfield(SPV_VALIDATE_LAYOUT_BIT, options)) { | ||
position.index = SPV_INDEX_INSTRUCTION; | ||
// TODO: spvBinaryValidateLayout | ||
} | ||
|
||
if (spvIsInBitfield(SPV_VALIDATE_ID_BIT, options)) { | ||
position.index = SPV_INDEX_INSTRUCTION; | ||
spvCheckReturn( | ||
|
@@ -314,10 +457,5 @@ spv_result_t spvValidate(const spv_const_context context, | |
context->ext_inst_table, &position, pDiagnostic)); | ||
} | ||
|
||
if (spvIsInBitfield(SPV_VALIDATE_RULES_BIT, options)) { | ||
position.index = SPV_INDEX_INSTRUCTION; | ||
// TODO: Specified validation rules... | ||
} | ||
|
||
return SPV_SUCCESS; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1216,12 +1216,11 @@ bool idUsage::isValid<SpvOpFunctionParameter>(const spv_instruction_t* inst, | |
<< inst->words[resultTypeIndex] | ||
<< "' is not defined."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI. The spvValidateIDs function in validate.cpp needs to be reimplemented around the spvBinaryParse and spv_parsed_instruction_t because the technique of using spvBinaryOperandInfo function is not expressive enough to describe all the types of instructions, and the fact that they can be variable length. For example OpSwitch has variable number of operands, and some of them are literal numbers, and others are label Ids. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my thought. I didn't want to break all of the ValidateID unit tests in this commit. That is something I have been prototyping in another branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
return false); | ||
auto function = inst - 1; | ||
auto function = inst; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems kind of pointless. Just go ahead and rename the parameter inst to function if you like. |
||
// NOTE: Find OpFunction & ensure OpFunctionParameter is not out of place. | ||
size_t paramIndex = 0; | ||
while (firstInst != function) { | ||
spvCheck(SpvOpFunction != function->opcode && | ||
SpvOpFunctionParameter != function->opcode, | ||
while (firstInst != --function) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is dangerous. A malicious input will cause this code to read out of bounds. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed using assertion in da35d28 |
||
spvCheck(SpvOpFunction != function->opcode && SpvOpFunctionParameter != function->opcode, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If possible, I'd suggest to remove |
||
DIAG(0) << "OpFunctionParameter is not preceded by OpFunction or " | ||
"OpFunctionParameter sequence."; | ||
return false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sort these. Not a big deal, we can come back to this. The rest of the code is not fully compliant either.
For reference: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now
clang-format
can do this automatically for you, I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed da35d28