IMPALA-5031: Make UBSAN-friendly arithmetic generic ArithmeticUtil::AsUnsigned() makes it possible to do arithmetic on signed integers in a way that does not invoke undefined behavior, but it only works on integers. This patch adds ArithmeticUtil::Compute(), which dispatches (at compile time) to the normal arithmetic evaluation method if the type of the values is a floating point type, but uses AsUnsigned() if the type of the values is an integral type.
Change-Id: I73bec71e59c5a921003d0ebca52a1d4e49bbef66 Reviewed-on: http://gerrit.cloudera.org:8080/11810 Reviewed-by: Jim Apple <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/e0642c91 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/e0642c91 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/e0642c91 Branch: refs/heads/branch-3.1.0 Commit: e0642c9191e082fde4cc92ad754c9f643e20e248 Parents: 9bdb73b Author: Jim Apple <[email protected]> Authored: Sat Oct 27 07:49:23 2018 -0700 Committer: Zoltan Borok-Nagy <[email protected]> Committed: Tue Nov 13 12:50:23 2018 +0100 ---------------------------------------------------------------------- be/src/exprs/expr-test.cc | 6 ++--- be/src/util/arithmetic-util.h | 51 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/e0642c91/be/src/exprs/expr-test.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc index c926ff0..dd6f1b3 100644 --- a/be/src/exprs/expr-test.cc +++ b/be/src/exprs/expr-test.cc @@ -943,11 +943,11 @@ class ExprTest : public testing::Test { string a_str = LiteralToString<Result>(cast_a); string b_str = LiteralToString<Result>(cast_b); TestValue(a_str + " + " + b_str, expected_type, - static_cast<Result>(cast_a + cast_b)); + ArithmeticUtil::Compute<std::plus>(cast_a, cast_b)); TestValue(a_str + " - " + b_str, expected_type, - static_cast<Result>(cast_a - cast_b)); + ArithmeticUtil::Compute<std::minus>(cast_a, cast_b)); TestValue(a_str + " * " + b_str, expected_type, - static_cast<Result>(cast_a * cast_b)); + ArithmeticUtil::Compute<std::multiplies>(cast_a, cast_b)); TestValue(a_str + " / " + b_str, TYPE_DOUBLE, static_cast<double>(a) / static_cast<double>(b)); } http://git-wip-us.apache.org/repos/asf/impala/blob/e0642c91/be/src/util/arithmetic-util.h ---------------------------------------------------------------------- diff --git a/be/src/util/arithmetic-util.h b/be/src/util/arithmetic-util.h index 93a0039..4ab60cd 100644 --- a/be/src/util/arithmetic-util.h +++ b/be/src/util/arithmetic-util.h @@ -99,6 +99,57 @@ class ArithmeticUtil { const auto a = ToUnsigned(x), b = ToUnsigned(y); return ToSigned(Operator<UnsignedType<T>>()(a, b)); } + + // Compute() is meant to be used like AsUnsigned(), but when the template context does + // not enforce that the type T is integral. For floating point types, it performs + // Operator (i.e. std::plus) without modification, and for integral types it calls + // AsUnsigned(). + // + // It is needed because AsUnsigned<std::plus>(1.0, 2.0) does not compile, since + // UnsignedType<float> is not a valid type. In contrast, Compute<std::plus>(1.0, 2.0) + // does compile and performs the usual addition on 1.0 and 2.0 to produce 3.0. + template <template <typename> class Operator, typename T> + static T Compute(T x, T y) { + return OperateOn<T>::template Compute<Operator>(x, y); + } + + private: + // Ring and OperateOn are used for compile-time dispatching on how Compute() should + // perform an arithmetic operation: as an unsigned integer operation, as a + // floating-point operation, or not at all. + // + // For example, OperatorOn<int>::Compute<std::plus> is really just an alias for + // AsUnsigned<std::plus, int>, while OperatorOn<float>::Compute<std::plus> is really + // just an alias for the usual addition operator on floats. + enum class Ring { INTEGER, FLOAT, NEITHER }; + + template <typename T, + Ring R = std::is_integral<T>::value ? + Ring::INTEGER : + (std::is_floating_point<T>::value ? Ring::FLOAT : Ring::NEITHER)> + struct OperateOn; +}; + +template <typename T> +struct ArithmeticUtil::OperateOn<T, ArithmeticUtil::Ring::FLOAT> { + template <template <typename> class Operator> + static T Compute(T a, T b) { + return Operator<T>()(a, b); + } +}; + +template <typename T> +struct ArithmeticUtil::OperateOn<T, ArithmeticUtil::Ring::INTEGER> { + template <template <typename> class Operator> + static T Compute(T x, T y) { + return AsUnsigned<Operator>(x, y); + } +}; + +template <typename T> +struct ArithmeticUtil::OperateOn<T, ArithmeticUtil::Ring::NEITHER> { + template <template <typename> class Operator> + static T Compute(T x, T y) = delete; }; class ArithmeticUtilTest {
