You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/11/06 00:06:24 UTC
[2/3] impala git commit: IMPALA-5031: Make UBSAN-friendly arithmetic
generic
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 <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/78b6f1db
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/78b6f1db
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/78b6f1db
Branch: refs/heads/master
Commit: 78b6f1db69eafaf01408fda444e53513200852f3
Parents: 691f9d9
Author: Jim Apple <jb...@apache.org>
Authored: Sat Oct 27 07:49:23 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Nov 5 23:30:52 2018 +0000
----------------------------------------------------------------------
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/78b6f1db/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/78b6f1db/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 {