From 9d7428b052dbc73b45bcb7c3c7919bbbadd6f287 Mon Sep 17 00:00:00 2001 From: alan-baker Date: Wed, 2 Oct 2019 21:12:57 -0400 Subject: [PATCH] Validate physical storage buffer restrictions (#2930) * Physical storage buffer cannot be used with OpConstantNull, OpPtrEqual, OpPtrNotEqual or OpPtrDiff * new tests * see also #2929 --- source/val/validate_constants.cpp | 6 +- source/val/validate_memory.cpp | 5 +- test/opt/inst_buff_addr_check_test.cpp | 4 +- test/val/val_constants_test.cpp | 20 ++++++ test/val/val_memory_test.cpp | 90 ++++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/source/val/validate_constants.cpp b/source/val/validate_constants.cpp index 95ec1680d..dea95c8a2 100644 --- a/source/val/validate_constants.cpp +++ b/source/val/validate_constants.cpp @@ -304,7 +304,6 @@ bool IsTypeNullable(const std::vector& instruction, case SpvOpTypeBool: case SpvOpTypeInt: case SpvOpTypeFloat: - case SpvOpTypePointer: case SpvOpTypeEvent: case SpvOpTypeDeviceEvent: case SpvOpTypeReserveId: @@ -325,6 +324,11 @@ bool IsTypeNullable(const std::vector& instruction, } return true; } + case SpvOpTypePointer: + if (instruction[2] == SpvStorageClassPhysicalStorageBuffer) { + return false; + } + return true; default: return false; } diff --git a/source/val/validate_memory.cpp b/source/val/validate_memory.cpp index 59cdbb360..bff8b20ee 100644 --- a/source/val/validate_memory.cpp +++ b/source/val/validate_memory.cpp @@ -1590,8 +1590,8 @@ spv_result_t ValidatePtrComparison(ValidationState_t& _, << "Operand type must be a pointer"; } + SpvStorageClass sc = op1_type->GetOperandAs(1u); if (_.addressing_model() == SpvAddressingModelLogical) { - SpvStorageClass sc = op1_type->GetOperandAs(1u); if (sc != SpvStorageClassWorkgroup && sc != SpvStorageClassStorageBuffer) { return _.diag(SPV_ERROR_INVALID_ID, inst) << "Invalid pointer storage class"; @@ -1602,6 +1602,9 @@ spv_result_t ValidatePtrComparison(ValidationState_t& _, << "Workgroup storage class pointer requires VariablePointers " "capability to be specified"; } + } else if (sc == SpvStorageClassPhysicalStorageBuffer) { + return _.diag(SPV_ERROR_INVALID_ID, inst) + << "Cannot use a pointer in the PhysicalStorageBuffer storage class"; } return SPV_SUCCESS; diff --git a/test/opt/inst_buff_addr_check_test.cpp b/test/opt/inst_buff_addr_check_test.cpp index 837c8166b..4a1e5e6fb 100644 --- a/test/opt/inst_buff_addr_check_test.cpp +++ b/test/opt/inst_buff_addr_check_test.cpp @@ -608,10 +608,12 @@ OpReturn OpFunctionEnd )"; + // Issue #2929: cannot use OpConstantNull of a physical storage buffer + // pointer. SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + new_funcs, true, - true, 7u, 23u, 2u); + false, 7u, 23u, 2u); } } // namespace diff --git a/test/val/val_constants_test.cpp b/test/val/val_constants_test.cpp index 2499f5ca4..301539d98 100644 --- a/test/val/val_constants_test.cpp +++ b/test/val/val_constants_test.cpp @@ -458,6 +458,26 @@ OpMemoryModel Logical GLSL450 EXPECT_EQ(SPV_SUCCESS, ValidateInstructions()); } +TEST_F(ValidateConstant, NullPhysicalStorageBuffer) { + std::string spirv = R"( +OpCapability Shader +OpCapability PhysicalStorageBufferAddresses +OpCapability Linkage +OpExtension "SPV_KHR_physical_storage_buffer" +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpName %ptr "ptr" +%int = OpTypeInt 32 0 +%ptr = OpTypePointer PhysicalStorageBuffer %int +%null = OpConstantNull %ptr +)"; + + CompileSuccessfully(spirv); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("OpConstantNull Result Type '1[%ptr]' cannot have " + "a null value")); +} + } // namespace } // namespace val } // namespace spvtools diff --git a/test/val/val_memory_test.cpp b/test/val/val_memory_test.cpp index 858ef8ddc..17f26eec3 100644 --- a/test/val/val_memory_test.cpp +++ b/test/val/val_memory_test.cpp @@ -4386,6 +4386,96 @@ OpFunctionEnd "typed as OpTypeStruct, or an array of this type")); } +TEST_F(ValidateMemory, PhysicalStorageBufferPtrEqual) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +%void = OpTypeVoid +%bool = OpTypeBool +%long = OpTypeInt 64 0 +%long_0 = OpConstant %long 0 +%ptr_pssbo_long = OpTypePointer PhysicalStorageBuffer %long +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%conv = OpConvertUToPtr %ptr_pssbo_long %long_0 +%eq = OpPtrEqual %bool %conv %conv +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_5); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_5)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "Cannot use a pointer in the PhysicalStorageBuffer storage class")); +} + +TEST_F(ValidateMemory, PhysicalStorageBufferPtrNotEqual) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +%void = OpTypeVoid +%bool = OpTypeBool +%long = OpTypeInt 64 0 +%long_0 = OpConstant %long 0 +%ptr_pssbo_long = OpTypePointer PhysicalStorageBuffer %long +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%conv = OpConvertUToPtr %ptr_pssbo_long %long_0 +%neq = OpPtrNotEqual %bool %conv %conv +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_5); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_5)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "Cannot use a pointer in the PhysicalStorageBuffer storage class")); +} + +TEST_F(ValidateMemory, PhysicalStorageBufferPtrDiff) { + const std::string spirv = R"( +OpCapability Shader +OpCapability Int64 +OpCapability PhysicalStorageBufferAddresses +OpCapability VariablePointers +OpMemoryModel PhysicalStorageBuffer64 GLSL450 +OpEntryPoint GLCompute %main "main" +OpExecutionMode %main LocalSize 1 1 1 +%void = OpTypeVoid +%long = OpTypeInt 64 0 +%long_0 = OpConstant %long 0 +%ptr_pssbo_long = OpTypePointer PhysicalStorageBuffer %long +%void_fn = OpTypeFunction %void +%main = OpFunction %void None %void_fn +%entry = OpLabel +%conv = OpConvertUToPtr %ptr_pssbo_long %long_0 +%diff = OpPtrDiff %long %conv %conv +OpReturn +OpFunctionEnd +)"; + + CompileSuccessfully(spirv, SPV_ENV_UNIVERSAL_1_5); + EXPECT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_UNIVERSAL_1_5)); + EXPECT_THAT( + getDiagnosticString(), + HasSubstr( + "Cannot use a pointer in the PhysicalStorageBuffer storage class")); +} + } // namespace } // namespace val } // namespace spvtools