From 5b196f1941dd56516ba094540866fc575fa7e717 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Fri, 1 Apr 2016 10:19:25 -0400 Subject: [PATCH 1/9] Fix validation of array length. --- source/opcode.cpp | 3 +-- source/validate_id.cpp | 3 +-- test/ValidateID.cpp | 12 ++++++++++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/source/opcode.cpp b/source/opcode.cpp index 2421610a59..61ba9a4762 100644 --- a/source/opcode.cpp +++ b/source/opcode.cpp @@ -448,13 +448,12 @@ int32_t spvOpcodeIsConstant(const SpvOp opcode) { case SpvOpConstant: case SpvOpConstantComposite: case SpvOpConstantSampler: - // case SpvOpConstantNull: case SpvOpConstantNull: case SpvOpSpecConstantTrue: case SpvOpSpecConstantFalse: case SpvOpSpecConstant: case SpvOpSpecConstantComposite: - // case SpvOpSpecConstantOp: + case SpvOpSpecConstantOp: return true; default: return false; diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 513f2575cc..b8510598ff 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -278,8 +278,7 @@ bool idUsage::isValid(const spv_instruction_t* inst, } auto lengthIndex = 3; auto length = usedefs_.FindDef(inst->words[lengthIndex]); - if (!length.first || (SpvOpConstant != length.second.opcode && - SpvOpSpecConstant != length.second.opcode)) { + if (!length.first || !spvOpcodeIsConstant(length.second.opcode)) { DIAG(lengthIndex) << "OpTypeArray Length '" << inst->words[lengthIndex] << "' is not a scalar constant type."; return false; diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index e29672fb2e..f6c0a4cb09 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -371,6 +371,7 @@ TEST_F(ValidateID, OpTypeArrayGood) { %3 = OpTypeArray %1 %2)"; CHECK(spirv, SPV_SUCCESS); } + TEST_F(ValidateID, OpTypeArrayElementTypeBad) { const char* spirv = R"( %1 = OpTypeInt 32 0 @@ -378,6 +379,7 @@ TEST_F(ValidateID, OpTypeArrayElementTypeBad) { %3 = OpTypeArray %2 %2)"; CHECK(spirv, SPV_ERROR_INVALID_ID); } + TEST_F(ValidateID, OpTypeArrayLengthBad) { const char* spirv = R"( %1 = OpTypeInt 32 0 @@ -386,6 +388,16 @@ TEST_F(ValidateID, OpTypeArrayLengthBad) { CHECK(spirv, SPV_ERROR_INVALID_ID); } +TEST_F(ValidateID, OpTypeArrayLengthSpecConstOp) { + const char* spirv = R"( +%i32 = OpTypeInt 32 1 +%c1 = OpConstant %i32 1 +%c2 = OpConstant %i32 2 +%len = OpSpecConstantOp %i32 IAdd %c1 %c2 +%ary = OpTypeArray %i32 %len)"; + CHECK(spirv, SPV_SUCCESS); +} + TEST_F(ValidateID, OpTypeRuntimeArrayGood) { const char* spirv = R"( %1 = OpTypeInt 32 0 From 353376d69fc000c66fb76edf8fe1e58138ddfe49 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Fri, 1 Apr 2016 17:19:23 -0400 Subject: [PATCH 2/9] Clean up 32/64 bit array-length calculation. --- source/validate_id.cpp | 60 ++++++++++++++++++++----------- test/ValidateID.cpp | 82 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 117 insertions(+), 25 deletions(-) diff --git a/source/validate_id.cpp b/source/validate_id.cpp index b8510598ff..ceb36b6161 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -264,6 +264,30 @@ bool idUsage::isValid(const spv_instruction_t*, return true; } +// True if the integer constant is > 0. constWords are words of the +// constant-defining instruction (either OpConstant or +// OpSpecConstant). typeWords are the words of the constant's-type-defining +// OpTypeInt. +bool aboveZero(const std::vector& constWords, + const std::vector& typeWords) { + const auto width = typeWords[2]; + const bool is_signed = typeWords[3]; + if (width == 64) { + if (is_signed) { + int64_t value = constWords[3] | (int64_t{constWords[4]} << 32); + return value > 0; + } else { + uint64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); + return value > 0; + } + } else { // Per spec, must be 32 bits or less, sign-extended. + if (is_signed) + return (constWords[3] > 0) && !(constWords[3] >> 31); + else + return constWords[3] > 0; + } +} + template <> bool idUsage::isValid(const spv_instruction_t* inst, const spv_opcode_desc) { @@ -293,27 +317,23 @@ bool idUsage::isValid(const spv_instruction_t* inst, << "' is not a constant integer type."; return false; } - if (4 == constInst.size()) { - spvCheck(1 > constInst[3], DIAG(lengthIndex) - << "OpTypeArray Length '" - << inst->words[lengthIndex] - << "' value must be at least 1."; - return false); - } else if (5 == constInst.size()) { - uint64_t value = constInst[3] | ((uint64_t)constInst[4]) << 32; - bool signedness = constResultType.second.words[3] != 0; - if (signedness) { - spvCheck(1 > (int64_t)value, DIAG(lengthIndex) - << "OpTypeArray Length '" - << inst->words[lengthIndex] - << "' value must be at least 1."; - return false); - } else { - spvCheck(1 > value, DIAG(lengthIndex) << "OpTypeArray Length '" - << inst->words[lengthIndex] - << "' value must be at least 1."; - return false); + + switch (length.second.opcode) { + case SpvOpSpecConstant: + case SpvOpConstant: + if (aboveZero(length.second.words, constResultType.second.words)) break; + // Else fall through! + case SpvOpConstantNull: { + DIAG(lengthIndex) << "OpTypeArray Length '" + << inst->words[lengthIndex] + << "' default value must be at least 1."; + return false; } + case SpvOpSpecConstantOp: + // Assume it's OK, rather than try to evaluate the operation. + break; + default: + assert(0 && "bug in spvOpcodeIsConstant() or result type isn't int"); } return true; } diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index f6c0a4cb09..29f72af447 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -24,6 +24,7 @@ // TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE // MATERIALS OR THE USE OR OTHER DEALINGS IN THE MATERIALS. +#include #include #include "UnitSPIRV.h" @@ -36,6 +37,9 @@ namespace { +using std::ostringstream; +using std::string; + class ValidateID : public ::testing::Test { public: virtual void TearDown() { spvBinaryDestroy(binary); } @@ -78,7 +82,7 @@ const char kOpenCLMemoryModel64[] = R"( if (error) { \ spvDiagnosticPrint(diagnostic); \ spvDiagnosticDestroy(diagnostic); \ - ASSERT_EQ(SPV_SUCCESS, error); \ + ASSERT_EQ(SPV_SUCCESS, error) << shader; \ } \ spv_result_t result = spvValidate(context, get_const_binary(), &diagnostic); \ if (SPV_SUCCESS != result) { \ @@ -380,14 +384,82 @@ TEST_F(ValidateID, OpTypeArrayElementTypeBad) { CHECK(spirv, SPV_ERROR_INVALID_ID); } -TEST_F(ValidateID, OpTypeArrayLengthBad) { +// Signed or unsigned. +enum Signed { kSigned, kUnsigned }; + +// Creates an assembly snippet declaring OpTypeArray with the given length. +string MakeArrayLength(const string& len, Signed isSigned, int width) { + ostringstream ss; + ss << " %t = OpTypeInt " << width << (isSigned == kSigned ? " 1" : " 0") + << " %l = OpConstant %t " << len << " %a = OpTypeArray %t %l"; + return ss.str(); +} + +TEST_F(ValidateID, OpTypeArrayLength0) { + CHECK(MakeArrayLength("0", kSigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength0U) { + CHECK(MakeArrayLength("0", kUnsigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLengthNegative1) { + CHECK(MakeArrayLength("-1", kSigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLengthNegative2) { + CHECK(MakeArrayLength("-2", kSigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLengthNegative123) { + CHECK(MakeArrayLength("-123", kSigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLengthNegativeMax) { + CHECK(MakeArrayLength("0x80000000", kSigned, 32), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64Bit0) { + CHECK(MakeArrayLength("0", kSigned, 64), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64Bit0U) { + CHECK(MakeArrayLength("0", kUnsigned, 64), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64BitNegative1) { + CHECK(MakeArrayLength("-1", kSigned, 64), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64BitNegative2) { + CHECK(MakeArrayLength("-2", kSigned, 64), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64BitNegative123) { + CHECK(MakeArrayLength("-123", kSigned, 64), SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLength64BitNegativeMax) { + CHECK(MakeArrayLength("0x8000000000000000", kSigned, 64), + SPV_ERROR_INVALID_ID); +} + +TEST_F(ValidateID, OpTypeArrayLengthNull) { const char* spirv = R"( -%1 = OpTypeInt 32 0 -%2 = OpConstant %1 0 -%3 = OpTypeArray %1 %2)"; +%i32 = OpTypeInt 32 1 +%len = OpConstantNull %i32 +%ary = OpTypeArray %i32 %len)"; CHECK(spirv, SPV_ERROR_INVALID_ID); } +TEST_F(ValidateID, OpTypeArrayLengthSpecConst) { + const char* spirv = R"( +%i32 = OpTypeInt 32 1 +%len = OpSpecConstant %i32 2 +%ary = OpTypeArray %i32 %len)"; + CHECK(spirv, SPV_SUCCESS); +} + TEST_F(ValidateID, OpTypeArrayLengthSpecConstOp) { const char* spirv = R"( %i32 = OpTypeInt 32 1 From fb304aec8a8ad0ae9fd66b470fc163b07a5b3b21 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Sat, 2 Apr 2016 21:42:26 -0400 Subject: [PATCH 3/9] Fix double-word value conversion. Use uint64_t instead of int64_t to shift left a SPIR-V word representing high-order bits of a signed 64-bit value. That way shifting left by 32 bits is well defined. --- source/validate_id.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/validate_id.cpp b/source/validate_id.cpp index ceb36b6161..66ed4f5841 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -274,7 +274,7 @@ bool aboveZero(const std::vector& constWords, const bool is_signed = typeWords[3]; if (width == 64) { if (is_signed) { - int64_t value = constWords[3] | (int64_t{constWords[4]} << 32); + int64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); return value > 0; } else { uint64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); From cb59937237c76be7d3910ac49a4f14e4c3ab26ff Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 10:35:03 -0400 Subject: [PATCH 4/9] DRY aboveZero() and handle widths between 32 & 64. --- source/validate_id.cpp | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 66ed4f5841..68f8ee7ca6 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -272,19 +272,15 @@ bool aboveZero(const std::vector& constWords, const std::vector& typeWords) { const auto width = typeWords[2]; const bool is_signed = typeWords[3]; - if (width == 64) { - if (is_signed) { - int64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); - return value > 0; - } else { - uint64_t value = constWords[3] | (uint64_t{constWords[4]} << 32); - return value > 0; - } - } else { // Per spec, must be 32 bits or less, sign-extended. - if (is_signed) - return (constWords[3] > 0) && !(constWords[3] >> 31); - else - return constWords[3] > 0; + const auto loWord = constWords[3]; + if (width > 32) { + // The spec currently doesn't allow integers wider than 64 bits. + const auto hiWord = constWords[4]; // Must exist, per spec. + if (is_signed && (hiWord >> 31)) return false; + return loWord | hiWord; + } else { + if (is_signed && (loWord >> 31)) return false; + return loWord > 0; } } From 52eda2762e5d950a41cfaf45391fdf36e48330ee Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 14:37:08 -0400 Subject: [PATCH 5/9] Parameterize OpArrayLength tests and cover many widths. --- include/spirv-tools/libspirv.h | 5 +- test/ValidateID.cpp | 91 ++++++++++++++++++++-------------- 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/include/spirv-tools/libspirv.h b/include/spirv-tools/libspirv.h index 7de0750f3b..57515a2b1d 100644 --- a/include/spirv-tools/libspirv.h +++ b/include/spirv-tools/libspirv.h @@ -383,10 +383,11 @@ spv_result_t spvBinaryToText(const spv_const_context context, // pointer. void spvBinaryDestroy(spv_binary binary); -// Validates a SPIR-V binary for correctness. +// Validates a SPIR-V binary for correctness. Any errors will be written into +// *diagnostic. spv_result_t spvValidate(const spv_const_context context, const spv_const_binary binary, - spv_diagnostic* pDiagnostic); + spv_diagnostic* diagnostic); // Creates a diagnostic object. The position parameter specifies the location in // the text/binary stream. The message parameter, copied into the diagnostic diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index 29f72af447..076b86388f 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -27,7 +27,7 @@ #include #include -#include "UnitSPIRV.h" +#include "TestFixture.h" // NOTE: The tests in this file are ONLY testing ID usage, there for the input // SPIR-V does not follow the logical layout rules from the spec in all cases in @@ -37,8 +37,10 @@ namespace { +using ::testing::ValuesIn; using std::ostringstream; using std::string; +using std::vector; class ValidateID : public ::testing::Test { public: @@ -390,59 +392,76 @@ enum Signed { kSigned, kUnsigned }; // Creates an assembly snippet declaring OpTypeArray with the given length. string MakeArrayLength(const string& len, Signed isSigned, int width) { ostringstream ss; + ss << kGLSL450MemoryModel; ss << " %t = OpTypeInt " << width << (isSigned == kSigned ? " 1" : " 0") << " %l = OpConstant %t " << len << " %a = OpTypeArray %t %l"; return ss.str(); } -TEST_F(ValidateID, OpTypeArrayLength0) { - CHECK(MakeArrayLength("0", kSigned, 32), SPV_ERROR_INVALID_ID); -} - -TEST_F(ValidateID, OpTypeArrayLength0U) { - CHECK(MakeArrayLength("0", kUnsigned, 32), SPV_ERROR_INVALID_ID); -} - -TEST_F(ValidateID, OpTypeArrayLengthNegative1) { - CHECK(MakeArrayLength("-1", kSigned, 32), SPV_ERROR_INVALID_ID); -} - -TEST_F(ValidateID, OpTypeArrayLengthNegative2) { - CHECK(MakeArrayLength("-2", kSigned, 32), SPV_ERROR_INVALID_ID); -} - -TEST_F(ValidateID, OpTypeArrayLengthNegative123) { - CHECK(MakeArrayLength("-123", kSigned, 32), SPV_ERROR_INVALID_ID); -} +// Tests OpTypeArray. Parameter is the width (in bits) of the array-length's +// type. +class OpTypeArrayLengthTest + : public spvtest::TextToBinaryTestBase<::testing::TestWithParam> { + protected: + OpTypeArrayLengthTest() + : position_{0, 0, 0}, diagnostic_(spvDiagnosticCreate(&position_, "")) {} + + ~OpTypeArrayLengthTest() { spvDiagnosticDestroy(diagnostic_); } + + // Runs spvValidate() on v, printing any errors via spvDiagnosticPrint(). + spv_result_t Val(const SpirvVector& v) { + spv_const_binary_t binary{v.data(), v.size()}; + const auto status = spvValidate(context, &binary, &diagnostic_); + if (status != SPV_SUCCESS) { + spvDiagnosticPrint(diagnostic_); + } + return status; + } + + private: + spv_position_t position_; // For creating diagnostic_. + spv_diagnostic diagnostic_; +}; -TEST_F(ValidateID, OpTypeArrayLengthNegativeMax) { - CHECK(MakeArrayLength("0x80000000", kSigned, 32), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, Length0) { + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("0", kSigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64Bit0) { - CHECK(MakeArrayLength("0", kSigned, 64), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, Length0U) { + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("0", kUnsigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64Bit0U) { - CHECK(MakeArrayLength("0", kUnsigned, 64), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, LengthNegative1) { + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-1", kSigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64BitNegative1) { - CHECK(MakeArrayLength("-1", kSigned, 64), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, LengthNegative2) { + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-2", kSigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64BitNegative2) { - CHECK(MakeArrayLength("-2", kSigned, 64), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, LengthNegative123) { + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-123", kSigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64BitNegative123) { - CHECK(MakeArrayLength("-123", kSigned, 64), SPV_ERROR_INVALID_ID); +TEST_P(OpTypeArrayLengthTest, LengthNegativeMax) { + const string neg_max = "0x8" + string(GetParam() / 4 - 1, '0'); + EXPECT_EQ( + SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength(neg_max, kSigned, GetParam())))); } -TEST_F(ValidateID, OpTypeArrayLength64BitNegativeMax) { - CHECK(MakeArrayLength("0x8000000000000000", kSigned, 64), - SPV_ERROR_INVALID_ID); -} +INSTANTIATE_TEST_CASE_P(Widths, OpTypeArrayLengthTest, + ValuesIn(vector{8, 16, 32, 48, 64})); TEST_F(ValidateID, OpTypeArrayLengthNull) { const char* spirv = R"( From 02a31a6189398423488a75628e11227baa7d16e7 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 14:45:38 -0400 Subject: [PATCH 6/9] Add test cases for positive length. --- test/ValidateID.cpp | 74 ++++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index 076b86388f..7208d4bba9 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -423,41 +423,47 @@ class OpTypeArrayLengthTest spv_diagnostic diagnostic_; }; -TEST_P(OpTypeArrayLengthTest, Length0) { +TEST_P(OpTypeArrayLengthTest, LengthPositive) { + const int width = GetParam(); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("1", kSigned, width)))); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("1", kUnsigned, width)))); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("2", kSigned, width)))); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("2", kUnsigned, width)))); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("55", kSigned, width)))); + EXPECT_EQ(SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("55", kUnsigned, width)))); + const string fpad(width / 4 - 1, 'F'); EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength("0", kSigned, GetParam())))); -} - -TEST_P(OpTypeArrayLengthTest, Length0U) { - EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength("0", kUnsigned, GetParam())))); -} - -TEST_P(OpTypeArrayLengthTest, LengthNegative1) { - EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength("-1", kSigned, GetParam())))); -} - -TEST_P(OpTypeArrayLengthTest, LengthNegative2) { - EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength("-2", kSigned, GetParam())))); -} - -TEST_P(OpTypeArrayLengthTest, LengthNegative123) { - EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength("-123", kSigned, GetParam())))); -} - -TEST_P(OpTypeArrayLengthTest, LengthNegativeMax) { - const string neg_max = "0x8" + string(GetParam() / 4 - 1, '0'); - EXPECT_EQ( - SPV_ERROR_INVALID_ID, - Val(CompileSuccessfully(MakeArrayLength(neg_max, kSigned, GetParam())))); + SPV_SUCCESS, + Val(CompileSuccessfully(MakeArrayLength("0x7" + fpad, kSigned, width)))); + EXPECT_EQ(SPV_SUCCESS, Val(CompileSuccessfully( + MakeArrayLength("0xF" + fpad, kUnsigned, width)))); +} + +TEST_P(OpTypeArrayLengthTest, LengthZero) { + const int width = GetParam(); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("0", kSigned, width)))); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("0", kUnsigned, width)))); +} + +TEST_P(OpTypeArrayLengthTest, LengthNegative) { + const int width = GetParam(); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-1", kSigned, width)))); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-2", kSigned, width)))); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength("-123", kSigned, width)))); + const string neg_max = "0x8" + string(width / 4 - 1, '0'); + EXPECT_EQ(SPV_ERROR_INVALID_ID, + Val(CompileSuccessfully(MakeArrayLength(neg_max, kSigned, width)))); } INSTANTIATE_TEST_CASE_P(Widths, OpTypeArrayLengthTest, From 8bca0d07da38830f2ade1419a7645e14ac4c4b77 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 14:51:54 -0400 Subject: [PATCH 7/9] Add a TODO for removing CHECK. --- test/ValidateID.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index 7208d4bba9..9a2b3f09b0 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -75,6 +75,8 @@ const char kOpenCLMemoryModel64[] = R"( OpMemoryModel Physical64 OpenCL )"; +// TODO(dekimir): this can be removed by adding a method to ValidateID akin to +// OpTypeArrayLengthTest::Val(). #define CHECK(str, expected) \ spv_diagnostic diagnostic; \ spv_context context = spvContextCreate(SPV_ENV_UNIVERSAL_1_0); \ From 91355b501452f8211446e4206c4c6f3f84e51c4f Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 15:10:03 -0400 Subject: [PATCH 8/9] Avoid "auto" in code that depends on correct type. --- source/validate_id.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/validate_id.cpp b/source/validate_id.cpp index 68f8ee7ca6..4efcc7efce 100644 --- a/source/validate_id.cpp +++ b/source/validate_id.cpp @@ -270,12 +270,12 @@ bool idUsage::isValid(const spv_instruction_t*, // OpTypeInt. bool aboveZero(const std::vector& constWords, const std::vector& typeWords) { - const auto width = typeWords[2]; + const uint32_t width = typeWords[2]; const bool is_signed = typeWords[3]; - const auto loWord = constWords[3]; + const uint32_t loWord = constWords[3]; if (width > 32) { // The spec currently doesn't allow integers wider than 64 bits. - const auto hiWord = constWords[4]; // Must exist, per spec. + const uint32_t hiWord = constWords[4]; // Must exist, per spec. if (is_signed && (hiWord >> 31)) return false; return loWord | hiWord; } else { From 671c7644ea6e6e02dbbb46afb872365446c09167 Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Mon, 4 Apr 2016 15:14:57 -0400 Subject: [PATCH 9/9] Rename "binary" to "cbinary" in Val(). --- test/ValidateID.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/ValidateID.cpp b/test/ValidateID.cpp index 9a2b3f09b0..03a7ce3f0f 100644 --- a/test/ValidateID.cpp +++ b/test/ValidateID.cpp @@ -412,8 +412,8 @@ class OpTypeArrayLengthTest // Runs spvValidate() on v, printing any errors via spvDiagnosticPrint(). spv_result_t Val(const SpirvVector& v) { - spv_const_binary_t binary{v.data(), v.size()}; - const auto status = spvValidate(context, &binary, &diagnostic_); + spv_const_binary_t cbinary{v.data(), v.size()}; + const auto status = spvValidate(context, &cbinary, &diagnostic_); if (status != SPV_SUCCESS) { spvDiagnosticPrint(diagnostic_); }