diff options
author | Alan Baker <alanbaker@google.com> | 2018-08-03 14:50:14 -0400 |
---|---|---|
committer | Alan Baker <alanbaker@google.com> | 2018-08-07 19:01:58 -0400 |
commit | 3a20879f4d0af1cc336ef3294d67bf199796be5d (patch) | |
tree | 1d73d635cf476833dd5263f7c11560e3c542bdd0 | |
parent | 2896b8f0e5f85a726d724d46c0641529280a7a31 (diff) |
Fixes #1800
* Refactored duplication of code between OpCopyMemory and
OpCopyMemorySized validation
* Fixed some bugs in OpCopyMemorySized validation
* Replaced asserts with checks
* Added new tests
-rw-r--r-- | source/val/validate_memory.cpp | 153 | ||||
-rw-r--r-- | test/val/val_id_test.cpp | 288 |
2 files changed, 346 insertions, 95 deletions
diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp index 55060921..18c3cca0 100644 --- a/source/val/validate_memory.cpp +++ b/source/val/validate_memory.cpp @@ -316,96 +316,87 @@ spv_result_t ValidateCopyMemory(ValidationState_t& _, const Instruction& inst) { } const auto target_pointer_type = _.FindDef(target->type_id()); - assert(target_pointer_type); - const auto target_type = _.FindDef(target_pointer_type->words()[3]); - assert(target_type); - const auto source_pointer_type = _.FindDef(source->type_id()); - assert(source_pointer_type); - const auto source_type = _.FindDef(source_pointer_type->words()[3]); - assert(source_type); - if (target_type->id() != source_type->id()) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemory Target <id> '" << _.getIdName(source_id) - << "'s type does not match Source <id> '" - << _.getIdName(source_type->id()) << "'s type."; - } - return SPV_SUCCESS; -} - -spv_result_t ValidateCopyMemorySized(ValidationState_t& _, - const Instruction& inst) { - const auto target_index = 0; - const auto target_id = inst.GetOperandAs<uint32_t>(target_index); - const auto target = _.FindDef(target_id); - if (!target) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "Target operand <id> '" << _.getIdName(target_id) - << "' is not defined."; - } - - const auto source_index = 1; - const auto source_id = inst.GetOperandAs<uint32_t>(source_index); - const auto source = _.FindDef(source_id); - if (!source) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "Source operand <id> '" << _.getIdName(source_id) - << "' is not defined."; - } - - const auto size_index = 2; - const auto size_id = inst.GetOperandAs<uint32_t>(size_index); - const auto size = _.FindDef(size_id); - if (!size) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "Size operand <id> '" << _.getIdName(size_id) - << "' is not defined."; - } - - const auto target_pointer_type = _.FindDef(target->type_id()); if (!target_pointer_type || - SpvOpTypePointer != target_pointer_type->opcode()) { + target_pointer_type->opcode() != SpvOpTypePointer) { return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemorySized Target <id> '" << _.getIdName(target_id) + << "Target operand <id> '" << _.getIdName(target_id) << "' is not a pointer."; } + const auto source_pointer_type = _.FindDef(source->type_id()); if (!source_pointer_type || - SpvOpTypePointer != source_pointer_type->opcode()) { + source_pointer_type->opcode() != SpvOpTypePointer) { return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemorySized Source <id> '" << _.getIdName(source_id) + << "Source operand <id> '" << _.getIdName(source_id) << "' is not a pointer."; } - switch (size->opcode()) { - // TODO: The following opcode's are assumed to be valid, refer to the - // following bug https://cvs.khronos.org/bugzilla/show_bug.cgi?id=13871 for - // clarification - case SpvOpConstant: - case SpvOpSpecConstant: { - auto size_type = _.FindDef(size->type_id()); - assert(size_type); - if (SpvOpTypeInt != size_type->opcode()) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id) - << "'s type is not an integer type."; - } - } break; - case SpvOpVariable: { - auto pointer_type = _.FindDef(size->type_id()); - assert(pointer_type); - auto size_type = _.FindDef(pointer_type->type_id()); - if (!size_type || SpvOpTypeInt != size_type->opcode()) { - return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id) - << "'s variable type is not an integer type."; - } - } break; - default: + + if (inst.opcode() == SpvOpCopyMemory) { + const auto target_type = + _.FindDef(target_pointer_type->GetOperandAs<uint32_t>(2)); + if (!target_type || target_type->opcode() == SpvOpTypeVoid) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Target operand <id> '" << _.getIdName(target_id) + << "' cannot be a void pointer."; + } + + const auto source_type = + _.FindDef(source_pointer_type->GetOperandAs<uint32_t>(2)); + if (!source_type || source_type->opcode() == SpvOpTypeVoid) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Source operand <id> '" << _.getIdName(source_id) + << "' cannot be a void pointer."; + } + + if (target_type->id() != source_type->id()) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Target <id> '" << _.getIdName(source_id) + << "'s type does not match Source <id> '" + << _.getIdName(source_type->id()) << "'s type."; + } + } else { + const auto size_id = inst.GetOperandAs<uint32_t>(2); + const auto size = _.FindDef(size_id); + if (!size) { return _.diag(SPV_ERROR_INVALID_ID, &inst) - << "OpCopyMemorySized Size <id> '" << _.getIdName(size_id) - << "' is not a constant or variable."; + << "Size operand <id> '" << _.getIdName(size_id) + << "' is not defined."; + } + + const auto size_type = _.FindDef(size->type_id()); + if (!_.IsIntScalarType(size_type->id())) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Size operand <id> '" << _.getIdName(size_id) + << "' must be a scalar integer type."; + } + + bool is_zero = true; + switch (size->opcode()) { + case SpvOpConstantNull: + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Size operand <id> '" << _.getIdName(size_id) + << "' cannot be a constant zero."; + case SpvOpConstant: + if (size_type->word(3) == 1 && + size->word(size->words().size() - 1) & 0x80000000) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Size operand <id> '" << _.getIdName(size_id) + << "' cannot have the sign bit set to 1."; + } + for (size_t i = 3; is_zero && i < size->words().size(); ++i) { + is_zero &= (size->word(i) == 0); + } + if (is_zero) { + return _.diag(SPV_ERROR_INVALID_ID, &inst) + << "Size operand <id> '" << _.getIdName(size_id) + << "' cannot be a constant zero."; + } + break; + default: + // Cannot infer any other opcodes. + break; + } } - // TODO: Check that consant is a least size 1, see the same bug as above for - // clarification? return SPV_SUCCESS; } @@ -576,10 +567,8 @@ spv_result_t ValidateMemoryInstructions(ValidationState_t& _, if (auto error = ValidateStore(_, *inst)) return error; break; case SpvOpCopyMemory: - if (auto error = ValidateCopyMemory(_, *inst)) return error; - break; case SpvOpCopyMemorySized: - if (auto error = ValidateCopyMemorySized(_, *inst)) return error; + if (auto error = ValidateCopyMemory(_, *inst)) return error; break; case SpvOpAccessChain: case SpvOpInBoundsAccessChain: diff --git a/test/val/val_id_test.cpp b/test/val/val_id_test.cpp index 6bdba1d4..6d483274 100644 --- a/test/val/val_id_test.cpp +++ b/test/val/val_id_test.cpp @@ -41,20 +41,31 @@ std::string kOpCapabilitySetup = R"( OpCapability Shader OpCapability Linkage OpCapability Addresses - OpCapability Pipes - OpCapability LiteralSampler - OpCapability DeviceEnqueue - OpCapability Vector16 OpCapability Int8 OpCapability Int16 OpCapability Int64 OpCapability Float64 + OpCapability LiteralSampler + OpCapability Pipes + OpCapability DeviceEnqueue + OpCapability Vector16 )"; std::string kGLSL450MemoryModel = kOpCapabilitySetup + R"( OpMemoryModel Logical GLSL450 )"; +std::string kNoKernelGLSL450MemoryModel = R"( + OpCapability Shader + OpCapability Linkage + OpCapability Addresses + OpCapability Int8 + OpCapability Int16 + OpCapability Int64 + OpCapability Float64 + OpMemoryModel Logical GLSL450 +)"; + std::string kOpenCLMemoryModel32 = R"( OpCapability Addresses OpCapability Linkage @@ -2584,6 +2595,49 @@ TEST_F(ValidateIdWithMessage, OpCopyMemoryGood) { CompileSuccessfully(spirv.c_str()); EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); } + +TEST_F(ValidateIdWithMessage, OpCopyMemoryNonPointerTarget) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpTypePointer Uniform %2 +%4 = OpTypeFunction %1 %2 %3 +%5 = OpFunction %1 None %4 +%6 = OpFunctionParameter %2 +%7 = OpFunctionParameter %3 +%8 = OpLabel +OpCopyMemory %6 %7 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Target operand <id> '6' is not a pointer.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemoryNonPointerSource) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpTypePointer Uniform %2 +%4 = OpTypeFunction %1 %2 %3 +%5 = OpFunction %1 None %4 +%6 = OpFunctionParameter %2 +%7 = OpFunctionParameter %3 +%8 = OpLabel +OpCopyMemory %7 %6 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Source operand <id> '6' is not a pointer.")); +} + TEST_F(ValidateIdWithMessage, OpCopyMemoryBad) { std::string spirv = kGLSL450MemoryModel + R"( %1 = OpTypeVoid @@ -2604,11 +2658,54 @@ TEST_F(ValidateIdWithMessage, OpCopyMemoryBad) { CompileSuccessfully(spirv.c_str()); EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("OpCopyMemory Target <id> '5's type does not match " + HasSubstr("Target <id> '5's type does not match " "Source <id> '2's type.")); } -// TODO: OpCopyMemorySized +TEST_F(ValidateIdWithMessage, OpCopyMemoryVoidTarget) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpTypePointer Uniform %1 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFunction %1 %2 %3 +%6 = OpFunction %1 None %5 +%7 = OpFunctionParameter %3 +%8 = OpFunctionParameter %4 +%9 = OpLabel +OpCopyMemory %7 %8 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Target operand <id> '7' cannot be a void pointer.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemoryVoidSource) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpTypePointer Uniform %1 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFunction %1 %2 %3 +%6 = OpFunction %1 None %5 +%7 = OpFunctionParameter %3 +%8 = OpFunctionParameter %4 +%9 = OpLabel +OpCopyMemory %8 %7 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Source operand <id> '7' cannot be a void pointer.")); +} + TEST_F(ValidateIdWithMessage, OpCopyMemorySizedGood) { std::string spirv = kGLSL450MemoryModel + R"( %1 = OpTypeVoid @@ -2644,7 +2741,7 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedTargetBad) { CompileSuccessfully(spirv.c_str()); EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("OpCopyMemorySized Target <id> '9' is not a pointer.")); + HasSubstr("Target operand <id> '9' is not a pointer.")); } TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSourceBad) { std::string spirv = kGLSL450MemoryModel + R"( @@ -2663,7 +2760,7 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSourceBad) { CompileSuccessfully(spirv.c_str()); EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); EXPECT_THAT(getDiagnosticString(), - HasSubstr("OpCopyMemorySized Source <id> '6' is not a pointer.")); + HasSubstr("Source operand <id> '6' is not a pointer.")); } TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeBad) { std::string spirv = kGLSL450MemoryModel + R"( @@ -2682,9 +2779,9 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeBad) { OpFunctionEnd)"; CompileSuccessfully(spirv.c_str()); EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); - EXPECT_THAT(getDiagnosticString(), - HasSubstr("OpCopyMemorySized Size <id> '6's variable type is not " - "an integer type.")); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Size operand <id> '6' must be a scalar integer type.")); } TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeTypeBad) { std::string spirv = kGLSL450MemoryModel + R"( @@ -2707,8 +2804,173 @@ TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeTypeBad) { EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); EXPECT_THAT( getDiagnosticString(), - HasSubstr( - "OpCopyMemorySized Size <id> '9's type is not an integer type.")); + HasSubstr("Size operand <id> '9' must be a scalar integer type.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNull) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpConstantNull %2 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Size operand <id> '3' cannot be a constant zero.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantZero) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpConstant %2 0 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Size operand <id> '3' cannot be a constant zero.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantZero64) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 64 0 +%3 = OpConstant %2 0 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Size operand <id> '3' cannot be a constant zero.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNegative) { + const std::string spirv = kNoKernelGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 1 +%3 = OpConstant %2 -1 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Size operand <id> '3' cannot have the sign bit set to 1.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeConstantNegative64) { + const std::string spirv = kNoKernelGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 64 1 +%3 = OpConstant %2 -1 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr("Size operand <id> '3' cannot have the sign bit set to 1.")); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeUnsignedNegative) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 32 0 +%3 = OpConstant %2 2147483648 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); +} + +TEST_F(ValidateIdWithMessage, OpCopyMemorySizedSizeUnsignedNegative64) { + const std::string spirv = kGLSL450MemoryModel + R"( +%1 = OpTypeVoid +%2 = OpTypeInt 64 0 +%3 = OpConstant %2 9223372036854775808 +%4 = OpTypePointer Uniform %2 +%5 = OpTypeFloat 32 +%6 = OpTypePointer UniformConstant %5 +%7 = OpTypeFunction %1 %4 %6 +%8 = OpFunction %1 None %7 +%9 = OpFunctionParameter %4 +%10 = OpFunctionParameter %6 +%11 = OpLabel +OpCopyMemorySized %9 %10 %3 +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); } const char kDeeplyNestedStructureSetup[] = R"( |