From 71198438ace07dccfc2955cf45a1a2d02696f286 Mon Sep 17 00:00:00 2001 From: Sarah Date: Fri, 16 Jul 2021 13:38:05 +0000 Subject: [PATCH] validation: textureSample*() offset argument must be literal or const_expr Bug: tint:939 Change-Id: I4ba44902948e38aa9ce3e9ecae9b3b4b593bd93d Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/57660 Kokoro: Kokoro Reviewed-by: Ben Clayton Commit-Queue: Sarah Mashayekhi --- src/ast/intrinsic_texture_helper_test.cc | 6 +- src/resolver/intrinsic_validation_test.cc | 228 +++++++++++++++--- src/resolver/resolver.cc | 58 ++++- src/resolver/resolver.h | 2 + .../generator_impl_intrinsic_texture_test.cc | 6 +- .../generator_impl_intrinsic_texture_test.cc | 6 +- .../spirv/builder_intrinsic_texture_test.cc | 15 +- 7 files changed, 275 insertions(+), 46 deletions(-) diff --git a/src/ast/intrinsic_texture_helper_test.cc b/src/ast/intrinsic_texture_helper_test.cc index 20c5f4c8ef..2fd64062fc 100644 --- a/src/ast/intrinsic_texture_helper_test.cc +++ b/src/ast/intrinsic_texture_helper_test.cc @@ -1367,7 +1367,7 @@ std::vector TextureOverloadCase::ValidCases() { b->vec2(1.f, 2.f), // coords b->vec2(3.f, 4.f), // ddx b->vec2(5.f, 6.f), // ddy - b->vec2(7, 8)); // offset + b->vec2(7, 7)); // offset }, }, { @@ -1413,7 +1413,7 @@ std::vector TextureOverloadCase::ValidCases() { 3, // array_index b->vec2(4.f, 5.f), // ddx b->vec2(6.f, 7.f), // ddy - b->vec2(8, 9)); // offset + b->vec2(6, 7)); // offset }, }, { @@ -1455,7 +1455,7 @@ std::vector TextureOverloadCase::ValidCases() { b->vec3(1.f, 2.f, 3.f), // coords b->vec3(4.f, 5.f, 6.f), // ddx b->vec3(7.f, 8.f, 9.f), // ddy - b->vec3(10, 11, 12)); // offset + b->vec3(0, 1, 2)); // offset }, }, { diff --git a/src/resolver/intrinsic_validation_test.cc b/src/resolver/intrinsic_validation_test.cc index bb003b5f9e..fef6915eea 100644 --- a/src/resolver/intrinsic_validation_test.cc +++ b/src/resolver/intrinsic_validation_test.cc @@ -12,40 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "src/resolver/resolver.h" - -#include "gmock/gmock.h" -#include "src/ast/assignment_statement.h" -#include "src/ast/bitcast_expression.h" -#include "src/ast/break_statement.h" -#include "src/ast/call_statement.h" -#include "src/ast/continue_statement.h" -#include "src/ast/if_statement.h" #include "src/ast/intrinsic_texture_helper_test.h" -#include "src/ast/loop_statement.h" -#include "src/ast/return_statement.h" -#include "src/ast/stage_decoration.h" -#include "src/ast/struct_block_decoration.h" -#include "src/ast/switch_statement.h" -#include "src/ast/unary_op_expression.h" -#include "src/ast/variable_decl_statement.h" #include "src/resolver/resolver_test_helper.h" -#include "src/sem/call.h" -#include "src/sem/function.h" -#include "src/sem/member_accessor_expression.h" -#include "src/sem/sampled_texture_type.h" -#include "src/sem/statement.h" -#include "src/sem/variable.h" - -using ::testing::ElementsAre; -using ::testing::HasSubstr; namespace tint { namespace resolver { namespace { -using IntrinsicType = sem::IntrinsicType; - using ResolverIntrinsicValidationTest = ResolverTest; TEST_F(ResolverIntrinsicValidationTest, InvalidPipelineStageDirect) { @@ -90,6 +63,207 @@ TEST_F(ResolverIntrinsicValidationTest, InvalidPipelineStageIndirect) { 7:8 note: called by entry point 'main')"); } +namespace TextureSamplerOffset { + +using TextureOverloadCase = ast::intrinsic::test::TextureOverloadCase; +using ValidTextureOverload = ast::intrinsic::test::ValidTextureOverload; +using TextureKind = ast::intrinsic::test::TextureKind; +using TextureDataType = ast::intrinsic::test::TextureDataType; +using u32 = ProgramBuilder::u32; +using i32 = ProgramBuilder::i32; +using f32 = ProgramBuilder::f32; + +static std::vector ValidCases() { + std::vector cases; + for (auto c : TextureOverloadCase::ValidCases()) { + if (std::string(c.function).find("textureSample") == 0) { + if (std::string(c.description).find("offset ") != std::string::npos) { + cases.push_back(c); + } + } + } + return cases; +} + +struct OffsetCase { + bool is_valid; + int32_t x; + int32_t y; + int32_t z; + int32_t illegal_value = 0; +}; + +static std::vector OffsetCases() { + return { + {true, 0, 1, 2}, // + {true, 7, -8, 7}, // + {false, 10, 10, 20, 10}, // + {false, -9, 0, 0, -9}, // + {false, 0, 8, 0, 8}, // + }; +} + +using IntrinsicTextureSamplerValidationTest = + ResolverTestWithParam>; +TEST_P(IntrinsicTextureSamplerValidationTest, ConstExpr) { + auto& p = GetParam(); + auto param = std::get<0>(p); + auto offset = std::get<1>(p); + param.buildTextureVariable(this); + param.buildSamplerVariable(this); + + auto args = param.args(this); + // Make Resolver visit the Node about to be removed + WrapInFunction(args.back()); + args.pop_back(); + if (NumCoordinateAxes(param.texture_dimension) == 2) { + args.push_back( + Construct(Source{{12, 34}}, ty.vec2(), offset.x, offset.y)); + } else if (NumCoordinateAxes(param.texture_dimension) == 3) { + args.push_back(Construct(Source{{12, 34}}, ty.vec3(), offset.x, + offset.y, offset.z)); + } + + auto* call = Call(param.function, args); + Func("func", {}, ty.void_(), {Ignore(call)}, + {create(ast::PipelineStage::kFragment)}); + + if (offset.is_valid) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()); + std::stringstream err; + err << "12:34 error: each offset component of '" << param.function + << "' must be at least -8 and at most 7. found: '" + << std::to_string(offset.illegal_value) << "'"; + EXPECT_EQ(r()->error(), err.str()); + } +} + +TEST_P(IntrinsicTextureSamplerValidationTest, ConstExprOfConstExpr) { + auto& p = GetParam(); + auto param = std::get<0>(p); + auto offset = std::get<1>(p); + param.buildTextureVariable(this); + param.buildSamplerVariable(this); + + auto args = param.args(this); + // Make Resolver visit the Node about to be removed + WrapInFunction(args.back()); + args.pop_back(); + if (NumCoordinateAxes(param.texture_dimension) == 2) { + args.push_back(Construct(Source{{12, 34}}, ty.vec2(), + Construct(ty.i32(), offset.x), offset.y)); + } else if (NumCoordinateAxes(param.texture_dimension) == 3) { + args.push_back(Construct(Source{{12, 34}}, ty.vec3(), offset.x, + Construct(ty.vec2(), offset.y, offset.z))); + } + auto* call = Call(param.function, args); + Func("func", {}, ty.void_(), {Ignore(call)}, + {create(ast::PipelineStage::kFragment)}); + if (offset.is_valid) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()); + std::stringstream err; + err << "12:34 error: each offset component of '" << param.function + << "' must be at least -8 and at most 7. found: '" + << std::to_string(offset.illegal_value) << "'"; + EXPECT_EQ(r()->error(), err.str()); + } +} + +TEST_P(IntrinsicTextureSamplerValidationTest, EmptyVectorConstructor) { + auto& p = GetParam(); + auto param = std::get<0>(p); + param.buildTextureVariable(this); + param.buildSamplerVariable(this); + + auto args = param.args(this); + // Make Resolver visit the Node about to be removed + WrapInFunction(args.back()); + args.pop_back(); + if (NumCoordinateAxes(param.texture_dimension) == 2) { + args.push_back(Construct(Source{{12, 34}}, ty.vec2())); + } else if (NumCoordinateAxes(param.texture_dimension) == 3) { + args.push_back(Construct(Source{{12, 34}}, ty.vec3())); + } + + auto* call = Call(param.function, args); + Func("func", {}, ty.void_(), {Ignore(call)}, + {create(ast::PipelineStage::kFragment)}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_P(IntrinsicTextureSamplerValidationTest, GlobalConst) { + auto& p = GetParam(); + auto param = std::get<0>(p); + auto offset = std::get<1>(p); + param.buildTextureVariable(this); + param.buildSamplerVariable(this); + + auto args = param.args(this); + // Make Resolver visit the Node about to be removed + WrapInFunction(args.back()); + args.pop_back(); + GlobalConst("offset_2d", ty.vec2(), vec2(offset.x, offset.y)); + GlobalConst("offset_3d", ty.vec3(), + vec3(offset.x, offset.y, offset.z)); + if (NumCoordinateAxes(param.texture_dimension) == 2) { + args.push_back(Expr(Source{{12, 34}}, "offset_2d")); + } else if (NumCoordinateAxes(param.texture_dimension) == 3) { + args.push_back(Expr(Source{{12, 34}}, "offset_3d")); + } + + auto* call = Call(param.function, args); + Func("func", {}, ty.void_(), {Ignore(call)}, + {create(ast::PipelineStage::kFragment)}); + EXPECT_FALSE(r()->Resolve()); + std::stringstream err; + err << "12:34 error: '" << param.function + << "' offset parameter must be provided as" + << " a literal or const_expr expression"; + EXPECT_EQ(r()->error(), err.str()); +} + +TEST_P(IntrinsicTextureSamplerValidationTest, ScalarConst) { + auto& p = GetParam(); + auto param = std::get<0>(p); + auto offset = std::get<1>(p); + param.buildTextureVariable(this); + param.buildSamplerVariable(this); + auto* x = Const("x", ty.i32(), Construct(ty.i32(), offset.x)); + + auto args = param.args(this); + // Make Resolver visit the Node about to be removed + WrapInFunction(args.back()); + args.pop_back(); + if (NumCoordinateAxes(param.texture_dimension) == 2) { + args.push_back(Construct(Source{{12, 34}}, ty.vec2(), x, offset.y)); + } else if (NumCoordinateAxes(param.texture_dimension) == 3) { + args.push_back( + Construct(Source{{12, 34}}, ty.vec3(), x, offset.y, offset.z)); + } + + auto* call = Call(param.function, args); + Func("func", {}, ty.void_(), {Decl(x), Ignore(call)}, + {create(ast::PipelineStage::kFragment)}); + EXPECT_FALSE(r()->Resolve()); + std::stringstream err; + err << "12:34 error: '" << param.function + << "' offset parameter must be provided as" + << " a literal or const_expr expression"; + EXPECT_EQ(r()->error(), err.str()); +} + +INSTANTIATE_TEST_SUITE_P(IntrinsicTextureSamplerValidationTest, + IntrinsicTextureSamplerValidationTest, + testing::Combine(testing::ValuesIn(ValidCases()), + testing::ValuesIn(OffsetCases()))); +} // namespace TextureSamplerOffset + } // namespace } // namespace resolver } // namespace tint diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 8b00eced0d..604972b085 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -2431,13 +2431,67 @@ bool Resolver::IntrinsicCall(ast::CallExpression* call, AddWarning("use of deprecated intrinsic", call->source()); } - builder_->Sem().Add( - call, builder_->create(call, result, current_statement_)); + auto* out = builder_->create(call, result, current_statement_); + builder_->Sem().Add(call, out); SetExprInfo(call, result->ReturnType()); current_function_->intrinsic_calls.emplace_back( IntrinsicCallInfo{call, result}); + if (IsTextureIntrinsic(intrinsic_type) && + !ValidateTextureIntrinsicFunction(call, out)) { + return false; + } + + return true; +} + +bool Resolver::ValidateTextureIntrinsicFunction( + const ast::CallExpression* ast_call, + const sem::Call* sem_call) { + auto* intrinsic = sem_call->Target()->As(); + if (!intrinsic) { + return false; + } + std::string func_name = intrinsic->str(); + auto index = + sem::IndexOf(intrinsic->Parameters(), sem::ParameterUsage::kOffset); + if (index > -1) { + auto* param = ast_call->params()[index]; + if (param->Is()) { + auto values = ConstantValueOf(param); + if (!values.IsValid()) { + AddError("'" + func_name + + "' offset parameter must be provided as a literal or " + "const_expr expression", + param->source()); + return false; + } + if (!values.Type()->Is() || + !values.ElementType()->Is()) { + TINT_ICE(Resolver, diagnostics_) + << "failed to resolve '" + func_name + "' offset parameter type"; + return false; + } + for (auto offset : values.Elements()) { + auto offset_value = offset.i32; + if (offset_value < -8 || offset_value > 7) { + AddError("each offset component of '" + func_name + + "' must be at least -8 and at most 7. " + "found: '" + + std::to_string(offset_value) + "'", + param->source()); + return false; + } + } + } else { + AddError("'" + func_name + + "' offset parameter must be provided as a literal or " + "const_expr expression", + param->source()); + return false; + } + } return true; } diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index 5c45a7be89..ce82686770 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -323,6 +323,8 @@ class Resolver { bool ValidateArrayConstructor(const ast::TypeConstructorExpression* ctor, const sem::Array* arr_type); bool ValidateTypeDecl(const ast::TypeDecl* named_type) const; + bool ValidateTextureIntrinsicFunction(const ast::CallExpression* ast_call, + const sem::Call* sem_call); bool ValidateNoDuplicateDecorations(const ast::DecorationList& decorations); // sem::Struct is assumed to have at least one member bool ValidateStorageClassLayout(const sem::Struct* type, diff --git a/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc b/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc index e898665daa..d5171b6212 100644 --- a/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc +++ b/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc @@ -254,15 +254,15 @@ ExpectedResult expected_texture_overload( case ValidTextureOverload::kSampleGrad2dF32: return R"(tint_symbol.SampleGrad(tint_symbol_1, float2(1.0f, 2.0f), float2(3.0f, 4.0f), float2(5.0f, 6.0f));)"; case ValidTextureOverload::kSampleGrad2dOffsetF32: - return R"(tint_symbol.SampleGrad(tint_symbol_1, float2(1.0f, 2.0f), float2(3.0f, 4.0f), float2(5.0f, 6.0f), int2(7, 8));)"; + return R"(tint_symbol.SampleGrad(tint_symbol_1, float2(1.0f, 2.0f), float2(3.0f, 4.0f), float2(5.0f, 6.0f), int2(7, 7));)"; case ValidTextureOverload::kSampleGrad2dArrayF32: return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, float(3)), float2(4.0f, 5.0f), float2(6.0f, 7.0f));)"; case ValidTextureOverload::kSampleGrad2dArrayOffsetF32: - return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, float(3)), float2(4.0f, 5.0f), float2(6.0f, 7.0f), int2(8, 9));)"; + return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, float(3)), float2(4.0f, 5.0f), float2(6.0f, 7.0f), int2(6, 7));)"; case ValidTextureOverload::kSampleGrad3dF32: return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, 3.0f), float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f));)"; case ValidTextureOverload::kSampleGrad3dOffsetF32: - return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, 3.0f), float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f), int3(10, 11, 12));)"; + return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, 3.0f), float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f), int3(0, 1, 2));)"; case ValidTextureOverload::kSampleGradCubeF32: return R"(tint_symbol.SampleGrad(tint_symbol_1, float3(1.0f, 2.0f, 3.0f), float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f));)"; case ValidTextureOverload::kSampleGradCubeArrayF32: diff --git a/src/writer/msl/generator_impl_intrinsic_texture_test.cc b/src/writer/msl/generator_impl_intrinsic_texture_test.cc index aba0aacf17..c4af75b04c 100644 --- a/src/writer/msl/generator_impl_intrinsic_texture_test.cc +++ b/src/writer/msl/generator_impl_intrinsic_texture_test.cc @@ -153,15 +153,15 @@ std::string expected_texture_overload( case ValidTextureOverload::kSampleGrad2dF32: return R"(texture.sample(sampler, float2(1.0f, 2.0f), gradient2d(float2(3.0f, 4.0f), float2(5.0f, 6.0f))))"; case ValidTextureOverload::kSampleGrad2dOffsetF32: - return R"(texture.sample(sampler, float2(1.0f, 2.0f), gradient2d(float2(3.0f, 4.0f), float2(5.0f, 6.0f)), int2(7, 8)))"; + return R"(texture.sample(sampler, float2(1.0f, 2.0f), gradient2d(float2(3.0f, 4.0f), float2(5.0f, 6.0f)), int2(7, 7)))"; case ValidTextureOverload::kSampleGrad2dArrayF32: return R"(texture.sample(sampler, float2(1.0f, 2.0f), 3, gradient2d(float2(4.0f, 5.0f), float2(6.0f, 7.0f))))"; case ValidTextureOverload::kSampleGrad2dArrayOffsetF32: - return R"(texture.sample(sampler, float2(1.0f, 2.0f), 3, gradient2d(float2(4.0f, 5.0f), float2(6.0f, 7.0f)), int2(8, 9)))"; + return R"(texture.sample(sampler, float2(1.0f, 2.0f), 3, gradient2d(float2(4.0f, 5.0f), float2(6.0f, 7.0f)), int2(6, 7)))"; case ValidTextureOverload::kSampleGrad3dF32: return R"(texture.sample(sampler, float3(1.0f, 2.0f, 3.0f), gradient3d(float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f))))"; case ValidTextureOverload::kSampleGrad3dOffsetF32: - return R"(texture.sample(sampler, float3(1.0f, 2.0f, 3.0f), gradient3d(float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f)), int3(10, 11, 12)))"; + return R"(texture.sample(sampler, float3(1.0f, 2.0f, 3.0f), gradient3d(float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f)), int3(0, 1, 2)))"; case ValidTextureOverload::kSampleGradCubeF32: return R"(texture.sample(sampler, float3(1.0f, 2.0f, 3.0f), gradientcube(float3(4.0f, 5.0f, 6.0f), float3(7.0f, 8.0f, 9.0f))))"; case ValidTextureOverload::kSampleGradCubeArrayF32: diff --git a/src/writer/spirv/builder_intrinsic_texture_test.cc b/src/writer/spirv/builder_intrinsic_texture_test.cc index 2197133bd9..8cfcc72b48 100644 --- a/src/writer/spirv/builder_intrinsic_texture_test.cc +++ b/src/writer/spirv/builder_intrinsic_texture_test.cc @@ -2070,14 +2070,13 @@ OpCapability SampledCubeArray %25 = OpTypeInt 32 1 %24 = OpTypeVector %25 2 %26 = OpConstant %25 7 -%27 = OpConstant %25 8 -%28 = OpConstantComposite %24 %26 %27 +%27 = OpConstantComposite %24 %26 %26 )", R"( %10 = OpLoad %7 %5 %11 = OpLoad %3 %1 %13 = OpSampledImage %12 %11 %10 -%8 = OpImageSampleExplicitLod %9 %13 %17 Grad|ConstOffset %20 %23 %28 +%8 = OpImageSampleExplicitLod %9 %13 %17 Grad|ConstOffset %20 %23 %27 )", R"( )"}; @@ -2141,8 +2140,8 @@ OpCapability SampledCubeArray %26 = OpConstant %4 7 %27 = OpConstantComposite %21 %25 %26 %28 = OpTypeVector %18 2 -%29 = OpConstant %18 8 -%30 = OpConstant %18 9 +%29 = OpConstant %18 6 +%30 = OpConstant %18 7 %31 = OpConstantComposite %28 %29 %30 )", R"( @@ -2216,9 +2215,9 @@ OpCapability SampledCubeArray %26 = OpConstantComposite %14 %23 %24 %25 %28 = OpTypeInt 32 1 %27 = OpTypeVector %28 3 -%29 = OpConstant %28 10 -%30 = OpConstant %28 11 -%31 = OpConstant %28 12 +%29 = OpConstant %28 0 +%30 = OpConstant %28 1 +%31 = OpConstant %28 2 %32 = OpConstantComposite %27 %29 %30 %31 )", R"(