[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints

["n.pettik" <korablev@tarantool.org>]

> On 2 Jul 2019, at 00:53, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the fixes!
> 
>>>> @@ -5134,7 +5175,11 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
>>>> 	assert(pIn1->flags & (MEM_Int | MEM_UInt));
>>>> 	assert(pIn3->flags & (MEM_Int | MEM_UInt));
>>>> 	x = pIn1->u.i;
>>>> -	if (x<=0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) {
>>>> +	int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
>>>> +	bool unused;
>>>> +	if ((x == 0 || pIn1->flags & MEM_Int) ||
>>>> +	    sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
>>> 
>>> 14. If you get to this line, then (pIn1->flags & MEM_Int) is already
>>> 0 and can be inlined.
>> 
>> Wait, why? If x == 0 then pIn1->flags == MEM_UInt -
>> we consider 0 as an unsigned value.
> 
> Because you can only get to sql_add_int(), if x != 0 and
> pIn1->flags & MEM_Int == 0. It is the C standard. In an
> expression (a || b) 'b' is executed iff 'a' is false.

My bad, never mind.

> Looks like that place is not tested at all. The tests pass
> regardless of how I call sql_add_int: with pIn1->flags & MEM_Int -> false
> or true.

In fact, the reason is the same as for OP_DecrJumpZero and
OP_IfNotZero: P1 is a limit counter and P3 is an offset counter.
Hence, both are >= 0, it is checked before these opcodes are
executed. Added fix to the previous patch:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index f864ef950..81005d14a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4929,18 +4929,16 @@ case OP_IfPos: {        /* jump, in1 */
  * Otherwise, r[P2] is set to the sum of r[P1] and r[P3].
  */
 case OP_OffsetLimit: {    /* in1, out2, in3 */
-       i64 x;
        pIn1 = &aMem[pOp->p1];
        pIn3 = &aMem[pOp->p3];
        pOut = out2Prerelease(p, pOp);
-       assert((pIn1->flags & (MEM_Int | MEM_UInt)) != 0);
-       assert((pIn3->flags & (MEM_Int | MEM_UInt)) != 0);
-       x = pIn1->u.i;
-       int64_t rhs = pIn3->flags & MEM_Int ? 0 : pIn3->u.u;
+       assert((pIn1->flags & MEM_UInt) != 0);
+       assert((pIn3->flags & MEM_UInt) != 0);
+       uint64_t x = pIn1->u.u;
+       uint64_t rhs = pIn3->u.u;
        bool unused;
-       if ((x == 0 || pIn1->flags & MEM_Int) ||
-           sql_add_int(x, pIn1->flags & MEM_Int, rhs, false,
-                       (int64_t *) &x, &unused) != 0) {
+       if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x,
+                                 &unused) != 0) {

> Please, inline the value (false), and add a test, which would fail, if
> I put here true.
> 
>> 	if (is_lhs_neg) {
>> 		uint64_t u_lhs = (uint64_t) (-lhs);
>> 		uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) :
>> 				 (uint64_t) rhs;
>> 		uint64_t u_res = u_lhs % u_rhs;
>> 		if (u_res > (uint64_t) INT64_MAX + 1)
>> 			return -1;
> 
> Please, add a test for this error. I've removed that check,
> and the tests passed.

Indeed, this check is redundant: we are ignoring the sign
of rhs, so if lhs is negative then the result is negative as
well. Hence, it is always less than INT64_MAX:

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 1bdaa24e5..161c1f607 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -1106,8 +1106,6 @@ sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
        if (is_lhs_neg) {
                uint64_t u_lhs = (uint64_t) (-lhs);
                uint64_t u_res = u_lhs % u_rhs;
-               if (u_res > (uint64_t) INT64_MAX + 1)
-                       return -1;
                *res = -u_res;
                *is_res_neg = true;
                return 0;

> Consider new fixes below, and on the branch
> in a separate commit.

Applied.