[Tarantool-patches] [PATCH 02/11] util: introduce double_compare_nint64()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Utility module (util.h and util.c) offers a function
double_compare_uint64() to compare double and uint64_t in a
reliable way, without undefined behaviour, without losses, even if
values are about 2^53 - 2^64.

There was no a similar function to compare double and int64_t,
which is needed when the right value is negative. To workaround it
the right value (int64_t) was multiplied by -1 when it was
negative to be able to use it in double_compare_uint64(). This led
to undefined behaviour, when right value was INT64_MIN, because
expression (uint64_t)-value did not help. Firstly -value was
calculated, and then it was cast to uint64_t. The first step is
detected by the sanitizer as undefined behaviour.

Not counting, that it was slower than straightforward comparison
of negative int64_t and a double value. Because involved
additional negation.

Besides, there was an expression:

    assert((uint64_t)(double)rhs == rhs || rhs > (uint64_t)EXP2_53);

Rhs is a uint64_t. And there was hidden an undefined behaviour,
when rhs is UINT64_MAX. The problem is that UINT64_MAX is
2^64 - 1. And it can't be stored in a double value without
precision loss. It is rounded up to 2^64 = UINT64_MAX + 1. This
is what happens in "(double)rhs" expression. And when it is
cast back to uint64_t: "(uint64_t)(double)rhs", it explodes.

The patch fixes it by simply changing check order. If rhs is
bigger than 2^53 (below this border all the integers can be
represented), then the cast is not done.

Part of #4609
---
 src/box/sql/vdbeaux.c    | 16 ++++++++--------
 src/box/tuple_compare.cc |  9 ++-------
 src/lib/core/util.c      | 22 +++++++++++++++++++++-
 src/trivia/util.h        | 22 ++++++++++++++++++++++
 4 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 71f694b20..bae1a5352 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2876,8 +2876,8 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 		}
 		if ((f1 & MEM_Int) != 0) {
 			if ((f2 & MEM_Real) != 0) {
-				return double_compare_uint64(-pMem2->u.r,
-							     -pMem1->u.i, 1);
+				return double_compare_nint64(pMem2->u.r,
+							     pMem1->u.i, -1);
 			} else {
 				return -1;
 			}
@@ -2894,8 +2894,8 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 		}
 		if ((f1 & MEM_Real) != 0) {
 			if ((f2 & MEM_Int) != 0) {
-				return double_compare_uint64(-pMem1->u.r,
-							     -pMem2->u.i, -1);
+				return double_compare_nint64(pMem1->u.r,
+							     pMem2->u.i, 1);
 			} else if ((f2 & MEM_UInt) != 0) {
 				return double_compare_uint64(pMem1->u.r,
 							     pMem2->u.u, 1);
@@ -3073,8 +3073,8 @@ sqlVdbeCompareMsgpack(const char **key1,
 					rc = +1;
 				}
 			} else if (pKey2->flags & MEM_Real) {
-				rc = double_compare_uint64(-pKey2->u.r,
-							   -mem1.u.i, 1);
+				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
+							   -1);
 			} else if ((pKey2->flags & MEM_Null) != 0) {
 				rc = 1;
 			} else if ((pKey2->flags & MEM_Bool) != 0) {
@@ -3092,8 +3092,8 @@ sqlVdbeCompareMsgpack(const char **key1,
 			mem1.u.r = mp_decode_double(&aKey1);
  do_float:
 			if ((pKey2->flags & MEM_Int) != 0) {
-				rc = double_compare_uint64(-mem1.u.r,
-							   -pKey2->u.i, -1);
+				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
+							   1);
 			} else if (pKey2->flags & MEM_UInt) {
 				rc = double_compare_uint64(mem1.u.r,
 							   pKey2->u.u, 1);
diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
index bc01fe068..d059c709f 100644
--- a/src/box/tuple_compare.cc
+++ b/src/box/tuple_compare.cc
@@ -168,13 +168,8 @@ static int
 mp_compare_double_any_int(double lhs, const char *rhs, enum mp_type rhs_type,
 			  int k)
 {
-	if (rhs_type == MP_INT) {
-		int64_t v = mp_decode_int(&rhs);
-		if (v < 0) {
-			return double_compare_uint64(-lhs, (uint64_t)-v, -k);
-		}
-		return double_compare_uint64(lhs, (uint64_t)v, k);
-	}
+	if (rhs_type == MP_INT)
+		return double_compare_int64(lhs, mp_decode_int(&rhs), k);
 	assert(rhs_type == MP_UINT);
 	return double_compare_uint64(lhs, mp_decode_uint(&rhs), k);
 }
diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index ffa1d2b51..a65bc496c 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -327,6 +327,7 @@ fpconv_check()
 }
 
 #define EXP2_53 9007199254740992.0      /* 2.0 ^ 53 */
+#define EXP2_63 9223372036854775808.0   /* 2.0 ^ 63 */
 #define EXP2_64 1.8446744073709552e+19  /* 2.0 ^ 64 */
 
 int
@@ -378,7 +379,7 @@ double_compare_uint64(double lhs, uint64_t rhs, int k)
 		 * rounding happens.
 		 */
 		assert(lhs < EXP2_53);
-		assert((uint64_t)(double)rhs == rhs || rhs > (uint64_t)EXP2_53);
+		assert(rhs > (uint64_t)EXP2_53 || (uint64_t)(double)rhs == rhs);
 		return k*COMPARE_RESULT(lhs, (double)rhs);
 	}
 	/*
@@ -387,3 +388,22 @@ double_compare_uint64(double lhs, uint64_t rhs, int k)
 	 */
 	return -k;
 }
+
+int
+double_compare_nint64(double lhs, int64_t rhs, int k)
+{
+	assert(rhs < 0);
+	assert(k==1 || k==-1);
+	if (lhs <= -EXP2_53) {
+		assert((int64_t)-EXP2_63 == INT64_MIN);
+		if (lhs < -EXP2_63)
+			return -k;
+		assert((double)(int64_t)lhs == lhs);
+		return k*COMPARE_RESULT((int64_t)lhs, rhs);
+	}
+	if (!isnan(lhs)) {
+		assert(rhs < (int64_t)-EXP2_53 || (int64_t)(double)rhs == rhs);
+		return k*COMPARE_RESULT(lhs, (double)rhs);
+	}
+	return -k;
+}
diff --git a/src/trivia/util.h b/src/trivia/util.h
index 8a3d22b38..973c9df33 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -512,6 +512,28 @@ json_escape(char *buf, int size, const char *data);
 int
 double_compare_uint64(double lhs, uint64_t rhs, int k);
 
+/**
+ * The same as double_compare_uint64(), but for negative int64_t
+ * value. To avoid unnecessary negation for cast to uint64_t to
+ * be able to use the other function, and to avoid the undefined
+ * behaviour in it, because "(uint64_t)-value" is UB, if value is
+ * INT64_MIN.
+ */
+int
+double_compare_nint64(double lhs, int64_t rhs, int k);
+
+/**
+ * A shortcut to automatically choose a needed double vs
+ * int64/uint64 function.
+ */
+static inline int
+double_compare_int64(double lhs, int64_t rhs, int k)
+{
+	if (rhs >= 0)
+		return double_compare_uint64(lhs, (uint64_t)rhs, k);
+	return double_compare_nint64(lhs, rhs, k);
+}
+
 #if !defined(__cplusplus) && !defined(static_assert)
 # define static_assert _Static_assert
 #endif
-- 
2.21.1 (Apple Git-122.3)