From 8478dfca563f8b0748422ecea4eafb91ac36a88c Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Fri, 2 Sep 2022 23:01:00 +0000 Subject: [PATCH] tint: improve error message for const eval operator overflow Bug: tint:1581 Change-Id: I112c7981d6b1998dd8133d1f80409fdc12a16a21 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101042 Reviewed-by: Ben Clayton Kokoro: Kokoro Commit-Queue: Antonio Maiorano --- src/tint/resolver/const_eval.cc | 83 ++++++++++++++-------------- src/tint/resolver/const_eval_test.cc | 67 ++++++++++------------ 2 files changed, 71 insertions(+), 79 deletions(-) diff --git a/src/tint/resolver/const_eval.cc b/src/tint/resolver/const_eval.cc index d363b67d50..5b3cee23a9 100644 --- a/src/tint/resolver/const_eval.cc +++ b/src/tint/resolver/const_eval.cc @@ -140,6 +140,14 @@ inline bool IsPositiveZero(T value) { return Number(value) == Number(0); // Considers sign bit } +template +std::string OverflowErrorMessage(NumberT lhs, const char* op, NumberT rhs) { + std::stringstream ss; + ss << "'" << lhs.value << " " << op << " " << rhs.value << "' cannot be represented as '" + << FriendlyName() << "'"; + return ss.str(); +} + /// Constant inherits from sem::Constant to add an private implementation method for conversion. struct Constant : public sem::Constant { /// Convert attempts to convert the constant value to the given type. On error, Convert() @@ -515,28 +523,26 @@ ConstEval::ConstEval(ProgramBuilder& b) : builder(b) {} template utils::Result ConstEval::Add(NumberT a, NumberT b) { - using T = UnwrapNumber; - auto add_values = [](T lhs, T rhs) { - if constexpr (std::is_integral_v && std::is_signed_v) { - // Ensure no UB for signed overflow - using UT = std::make_unsigned_t; - return static_cast(static_cast(lhs) + static_cast(rhs)); - } else { - return lhs + rhs; - } - }; NumberT result; if constexpr (std::is_same_v || std::is_same_v) { // Check for over/underflow for abstract values if (auto r = CheckedAdd(a, b)) { result = r->value; } else { - AddError("'" + std::to_string(add_values(a.value, b.value)) + - "' cannot be represented as '" + FriendlyName() + "'", - *current_source); + AddError(OverflowErrorMessage(a, "+", b), *current_source); return utils::Failure; } } else { + using T = UnwrapNumber; + auto add_values = [](T lhs, T rhs) { + if constexpr (std::is_integral_v && std::is_signed_v) { + // Ensure no UB for signed overflow + using UT = std::make_unsigned_t; + return static_cast(static_cast(lhs) + static_cast(rhs)); + } else { + return lhs + rhs; + } + }; result = add_values(a.value, b.value); } return result; @@ -545,27 +551,25 @@ utils::Result ConstEval::Add(NumberT a, NumberT b) { template utils::Result ConstEval::Mul(NumberT a, NumberT b) { using T = UnwrapNumber; - auto mul_values = [](T lhs, T rhs) { // - if constexpr (std::is_integral_v && std::is_signed_v) { - // For signed integrals, avoid C++ UB by multiplying as unsigned - using UT = std::make_unsigned_t; - return static_cast(static_cast(lhs) * static_cast(rhs)); - } else { - return lhs * rhs; - } - }; NumberT result; if constexpr (std::is_same_v || std::is_same_v) { // Check for over/underflow for abstract values if (auto r = CheckedMul(a, b)) { result = r->value; } else { - AddError("'" + std::to_string(mul_values(a.value, b.value)) + - "' cannot be represented as '" + FriendlyName() + "'", - *current_source); + AddError(OverflowErrorMessage(a, "*", b), *current_source); return utils::Failure; } } else { + auto mul_values = [](T lhs, T rhs) { + if constexpr (std::is_integral_v && std::is_signed_v) { + // For signed integrals, avoid C++ UB by multiplying as unsigned + using UT = std::make_unsigned_t; + return static_cast(static_cast(lhs) * static_cast(rhs)); + } else { + return lhs * rhs; + } + }; result = mul_values(a.value, b.value); } return result; @@ -966,37 +970,32 @@ ConstEval::ConstantResult ConstEval::OpPlus(const sem::Type*, return r; } -ConstEval::ConstantResult ConstEval::OpMinus(const sem::Type* ty, +ConstEval::ConstantResult ConstEval::OpMinus(const sem::Type*, utils::VectorRef args, const Source& source) { auto transform = [&](const sem::Constant* c0, const sem::Constant* c1) { auto create = [&](auto i, auto j) -> const Constant* { using NumberT = decltype(i); - using T = UnwrapNumber; - - auto subtract_values = [](T lhs, T rhs) { - if constexpr (std::is_integral_v && std::is_signed_v) { - // Ensure no UB for signed underflow - using UT = std::make_unsigned_t; - return static_cast(static_cast(lhs) - static_cast(rhs)); - } else { - return lhs - rhs; - } - }; - NumberT result; if constexpr (std::is_same_v || std::is_same_v) { // Check for over/underflow for abstract values if (auto r = CheckedSub(i, j)) { result = r->value; } else { - AddError("'" + std::to_string(subtract_values(i.value, j.value)) + - "' cannot be represented as '" + - ty->FriendlyName(builder.Symbols()) + "'", - source); + AddError(OverflowErrorMessage(i, "-", j), source); return nullptr; } } else { + using T = UnwrapNumber; + auto subtract_values = [](T lhs, T rhs) { + if constexpr (std::is_integral_v && std::is_signed_v) { + // Ensure no UB for signed underflow + using UT = std::make_unsigned_t; + return static_cast(static_cast(lhs) - static_cast(rhs)); + } else { + return lhs - rhs; + } + }; result = subtract_values(i.value, j.value); } return CreateElement(builder, c0->Type(), result); diff --git a/src/tint/resolver/const_eval_test.cc b/src/tint/resolver/const_eval_test.cc index a83cfa1bff..ff455dc49f 100644 --- a/src/tint/resolver/const_eval_test.cc +++ b/src/tint/resolver/const_eval_test.cc @@ -3515,7 +3515,6 @@ struct OverflowCase { ast::BinaryOp op; Types lhs; Types rhs; - std::string overflowed_result; }; static std::ostream& operator<<(std::ostream& o, const OverflowCase& c) { @@ -3539,35 +3538,32 @@ TEST_P(ResolverConstEvalBinaryOpTest_Overflow, Test) { }, c.lhs); - EXPECT_THAT(r()->error(), HasSubstr("1:1 error: '" + c.overflowed_result + - "' cannot be represented as '" + type_name + "'")); + EXPECT_THAT(r()->error(), HasSubstr("1:1 error: '")); + EXPECT_THAT(r()->error(), HasSubstr("' cannot be represented as '" + type_name + "'")); } INSTANTIATE_TEST_SUITE_P( Test, ResolverConstEvalBinaryOpTest_Overflow, - testing::Values( // - // scalar-scalar add - OverflowCase{ast::BinaryOp::kAdd, Val(AInt::Highest()), Val(1_a), "-9223372036854775808"}, - OverflowCase{ast::BinaryOp::kAdd, Val(AInt::Lowest()), Val(-1_a), "9223372036854775807"}, - OverflowCase{ast::BinaryOp::kAdd, Val(AFloat::Highest()), Val(AFloat::Highest()), "inf"}, - OverflowCase{ast::BinaryOp::kAdd, Val(AFloat::Lowest()), Val(AFloat::Lowest()), "-inf"}, + testing::Values( + + // scalar-scalar add + OverflowCase{ast::BinaryOp::kAdd, Val(AInt::Highest()), Val(1_a)}, + OverflowCase{ast::BinaryOp::kAdd, Val(AInt::Lowest()), Val(-1_a)}, + OverflowCase{ast::BinaryOp::kAdd, Val(AFloat::Highest()), Val(AFloat::Highest())}, + OverflowCase{ast::BinaryOp::kAdd, Val(AFloat::Lowest()), Val(AFloat::Lowest())}, // scalar-scalar subtract - OverflowCase{ast::BinaryOp::kSubtract, Val(AInt::Lowest()), Val(1_a), - "9223372036854775807"}, - OverflowCase{ast::BinaryOp::kSubtract, Val(AInt::Highest()), Val(-1_a), - "-9223372036854775808"}, - OverflowCase{ast::BinaryOp::kSubtract, Val(AFloat::Highest()), Val(AFloat::Lowest()), - "inf"}, - OverflowCase{ast::BinaryOp::kSubtract, Val(AFloat::Lowest()), Val(AFloat::Highest()), - "-inf"}, + OverflowCase{ast::BinaryOp::kSubtract, Val(AInt::Lowest()), Val(1_a)}, + OverflowCase{ast::BinaryOp::kSubtract, Val(AInt::Highest()), Val(-1_a)}, + OverflowCase{ast::BinaryOp::kSubtract, Val(AFloat::Highest()), Val(AFloat::Lowest())}, + OverflowCase{ast::BinaryOp::kSubtract, Val(AFloat::Lowest()), Val(AFloat::Highest())}, // scalar-scalar multiply - OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Highest()), Val(2_a), "-2"}, - OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Lowest()), Val(-2_a), "0"}, + OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Highest()), Val(2_a)}, + OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Lowest()), Val(-2_a)}, // scalar-vector multiply - OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Highest()), Vec(2_a, 1_a), "-2"}, - OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Lowest()), Vec(-2_a, 1_a), "0"}, + OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Highest()), Vec(2_a, 1_a)}, + OverflowCase{ast::BinaryOp::kMultiply, Val(AInt::Lowest()), Vec(-2_a, 1_a)}, // vector-matrix multiply @@ -3577,8 +3573,7 @@ INSTANTIATE_TEST_SUITE_P( OverflowCase{ast::BinaryOp::kMultiply, // Vec(AFloat::Highest(), 1.0_a), // Mat({2.0_a, 1.0_a}, // - {1.0_a, 1.0_a}), // - "inf"}, + {1.0_a, 1.0_a})}, // Overflow from second multiplication of dot product of vector and matrix column 0 // i.e. (v[0] * m[0][0] + v[1] * m[0][1]) @@ -3586,8 +3581,7 @@ INSTANTIATE_TEST_SUITE_P( OverflowCase{ast::BinaryOp::kMultiply, // Vec(1.0_a, AFloat::Highest()), // Mat({1.0_a, 2.0_a}, // - {1.0_a, 1.0_a}), // - "inf"}, + {1.0_a, 1.0_a})}, // Overflow from addition of dot product of vector and matrix column 0 // i.e. (v[0] * m[0][0] + v[1] * m[0][1]) @@ -3595,8 +3589,7 @@ INSTANTIATE_TEST_SUITE_P( OverflowCase{ast::BinaryOp::kMultiply, // Vec(AFloat::Highest(), AFloat::Highest()), // Mat({1.0_a, 1.0_a}, // - {1.0_a, 1.0_a}), // - "inf"}, + {1.0_a, 1.0_a})}, // matrix-matrix multiply @@ -3607,8 +3600,7 @@ INSTANTIATE_TEST_SUITE_P( Mat({AFloat::Highest(), 1.0_a}, // {1.0_a, 1.0_a}), // Mat({2.0_a, 1.0_a}, // - {1.0_a, 1.0_a}), // - "inf"}, + {1.0_a, 1.0_a})}, // Overflow from second multiplication of dot product of lhs row 0 and rhs column 0 // i.e. m1[0][0] * m2[0][0] + m1[0][1] * m[1][0] @@ -3617,8 +3609,7 @@ INSTANTIATE_TEST_SUITE_P( Mat({1.0_a, AFloat::Highest()}, // {1.0_a, 1.0_a}), // Mat({1.0_a, 1.0_a}, // - {2.0_a, 1.0_a}), // - "inf"}, + {2.0_a, 1.0_a})}, // Overflow from addition of dot product of lhs row 0 and rhs column 0 // i.e. m1[0][0] * m2[0][0] + m1[0][1] * m[1][0] @@ -3627,8 +3618,7 @@ INSTANTIATE_TEST_SUITE_P( Mat({AFloat::Highest(), 1.0_a}, // {AFloat::Highest(), 1.0_a}), // Mat({1.0_a, 1.0_a}, // - {1.0_a, 1.0_a}), // - "inf"} + {1.0_a, 1.0_a})} )); @@ -3636,26 +3626,29 @@ TEST_F(ResolverConstEvalTest, BinaryAbstractAddOverflow_AInt) { GlobalConst("c", Add(Source{{1, 1}}, Expr(AInt::Highest()), 1_a)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "1:1 error: '-9223372036854775808' cannot be represented as 'abstract-int'"); + "1:1 error: '9223372036854775807 + 1' cannot be represented as 'abstract-int'"); } TEST_F(ResolverConstEvalTest, BinaryAbstractAddUnderflow_AInt) { GlobalConst("c", Add(Source{{1, 1}}, Expr(AInt::Lowest()), -1_a)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "1:1 error: '9223372036854775807' cannot be represented as 'abstract-int'"); + "1:1 error: '-9223372036854775808 + -1' cannot be represented as 'abstract-int'"); } TEST_F(ResolverConstEvalTest, BinaryAbstractAddOverflow_AFloat) { GlobalConst("c", Add(Source{{1, 1}}, Expr(AFloat::Highest()), AFloat::Highest())); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "1:1 error: 'inf' cannot be represented as 'abstract-float'"); + EXPECT_EQ(r()->error(), + "1:1 error: '1.79769e+308 + 1.79769e+308' cannot be represented as 'abstract-float'"); } TEST_F(ResolverConstEvalTest, BinaryAbstractAddUnderflow_AFloat) { GlobalConst("c", Add(Source{{1, 1}}, Expr(AFloat::Lowest()), AFloat::Lowest())); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "1:1 error: '-inf' cannot be represented as 'abstract-float'"); + EXPECT_EQ( + r()->error(), + "1:1 error: '-1.79769e+308 + -1.79769e+308' cannot be represented as 'abstract-float'"); } // Mixed AInt and AFloat args to test implicit conversion to AFloat