From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 286C92DAFE for ; Tue, 11 Jun 2019 17:11:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ftXA0vOAUu2v for ; Tue, 11 Jun 2019 17:11:29 -0400 (EDT) Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6F6FF2DA45 for ; Tue, 11 Jun 2019 17:11:28 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints References: <7d8e776bc82cb9d25460c2104e687540526aa7cd.1559919361.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9233795e-a77c-565a-9bd5-3712499e7fce@tarantool.org> Date: Tue, 11 Jun 2019 23:11:38 +0200 MIME-Version: 1.0 In-Reply-To: <7d8e776bc82cb9d25460c2104e687540526aa7cd.1559919361.git.korablev@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Nikita Pettik Thanks for the patch! See 14 comments below, review fixes at the end of the email, and in a separate commit on the branch. On 07/06/2019 18:37, Nikita Pettik wrote: > Let's patch internal VDBE routines which add, subtract, multiply, divide > and calculate the remainder of division to allow them take operands of > unsigned type. In this respect, each operator now accepts signs of both > operands and return sign of result. > > Part of #3810 > Part of #4015 > --- > src/box/sql/func.c | 14 ++- > src/box/sql/sqlInt.h | 35 +++++- > src/box/sql/util.c | 233 +++++++++++++++++++++++++++---------- > src/box/sql/vdbe.c | 77 +++++++++--- > test/sql-tap/func.test.lua | 28 +++-- > test/sql/integer-overflow.result | 15 ++- > test/sql/integer-overflow.test.lua | 3 + > 7 files changed, 309 insertions(+), 96 deletions(-) > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index f4c1cbcca..457c9b92b 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -1620,9 +1622,13 @@ sum_step(struct sql_context *context, int argc, sql_value **argv) > p->cnt++; > if (type == MP_INT || type == MP_UINT) { > int64_t v = sql_value_int64(argv[0]); > - p->rSum += v; > + if (type == MP_INT) > + p->rSum += v; > + else > + p->rSum += (uint64_t) v; > if ((p->approx | p->overflow) == 0 && > - sqlAddInt64(&p->iSum, v) != 0) { > + sql_add_int(p->iSum, p->is_neg, v, v < 0, &p->iSum, 1. As I understand, now sql_value_int64() returns an integer buffer, and its result can't be compared with 0. How do you do it here with 'v'? > + &p->is_neg) != 0) { > p->overflow = 1; > } > } else { > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index da17e24ed..c0e2ab699 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -4461,10 +4461,37 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, const char *); > Expr *sqlExprSkipCollate(Expr *); > int sqlCheckIdentifierName(Parse *, char *); > void sqlVdbeSetChanges(sql *, int); > -int sqlAddInt64(i64 *, i64); > -int sqlSubInt64(i64 *, i64); > -int sqlMulInt64(i64 *, i64); > -int sqlAbsInt32(int); > + > +/** > + * Attempt to add, subtract, multiply or get the remainder of > + * 64-bit integer values. There functions are able to operate > + * on signed as well as unsigned integers. If result of operation > + * is greater 0, then it is assumed to be unsigned and can take > + * values in range up to 2^64 - 1. If the result is negative, > + * then its minimum value is -2^63. > + * Return 0 on success. Or if the operation would have resulted > + * in an overflow, return -1. > + */ > +int > +sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg); > + > +int > +sql_sub_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg); > + > +int > +sql_mul_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg); > + > +int > +sql_div_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg); > + > +int > +sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg); 2. That function is usually called 'mod', not 'rem'. > + > u8 sqlGetBoolean(const char *z, u8); > > const void *sqlValueText(sql_value *); > diff --git a/src/box/sql/util.c b/src/box/sql/util.c > index 2e7c298e1..ee6a83ad5 100644 > --- a/src/box/sql/util.c > +++ b/src/box/sql/util.c > @@ -1194,89 +1194,196 @@ sqlSafetyCheckSickOrOk(sql * db) > } > } > > -/* > - * Attempt to add, substract, or multiply the 64-bit signed value iB against > - * the other 64-bit signed integer at *pA and store the result in *pA. > - * Return 0 on success. Or if the operation would have resulted in an > - * overflow, leave *pA unchanged and return 1. > - */ > int > -sqlAddInt64(i64 * pA, i64 iB) > +sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg) > { > - i64 iA = *pA; > - testcase(iA == 0); > - testcase(iA == 1); > - testcase(iB == -1); > - testcase(iB == 0); > - if (iB >= 0) { > - testcase(iA > 0 && LARGEST_INT64 - iA == iB); > - testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1); > - if (iA > 0 && LARGEST_INT64 - iA < iB) > - return 1; > - } else { > - testcase(iA < 0 && -(iA + LARGEST_INT64) == iB + 1); > - testcase(iA < 0 && -(iA + LARGEST_INT64) == iB + 2); > - if (iA < 0 && -(iA + LARGEST_INT64) > iB + 1) > - return 1; > + /* Addition of two negative integers. */ > + if (is_lhs_neg && is_rhs_neg) { > + assert(lhs < 0 && rhs < 0); > + /* This is the same as (lhs + rhs) < INT64_MIN */ > + if (-(lhs + INT64_MAX) > rhs + 1) > + return -1; 3. Why so complex? Why not 'lhs < INT64_MIN - rhs'? I did it, and the tests pass. If they should not, then please, provide a test. > + *is_res_neg = true; > + *res = lhs + rhs; > + return 0; > } > - *pA += iB; > + /* Both are unsigned integers. */ > + if (!is_lhs_neg && !is_rhs_neg) { > + uint64_t u_lhs = (uint64_t) lhs; > + uint64_t u_rhs = (uint64_t) rhs; > + /* This is the same as (lhs + rhs) > UINT64_MAX */ > + if (UINT64_MAX - u_lhs < u_rhs) > + return -1; > + *is_res_neg = false; > + *res = lhs + rhs; > + return 0; > + } > + /* > + * Make sure we've got only one combination of > + * positive and negative operands. > + */ > + if (is_lhs_neg) { > + SWAP(is_lhs_neg, is_rhs_neg); > + SWAP(lhs, rhs); > + } > + assert(! is_lhs_neg && is_rhs_neg); 4. You have spent more code lines on the SWAP and its explanation, than could on avoidance of this SWAP. In addition, SWAP version is slower obviously. - /* - * Make sure we've got only one combination of - * positive and negative operands. - */ - if (is_lhs_neg) { - SWAP(is_lhs_neg, is_rhs_neg); - SWAP(lhs, rhs); - } - assert(! is_lhs_neg && is_rhs_neg); - *is_res_neg = (uint64_t)(-rhs) > (uint64_t) lhs; + if (is_rhs_neg) + *is_res_neg = (uint64_t)(-rhs) > (uint64_t) lhs; + else + *is_res_neg = (uint64_t)(-lhs) > (uint64_t) rhs; *res = lhs + rhs; return 0; > + *is_res_neg = (uint64_t)(-rhs) > (uint64_t) lhs; > + *res = lhs + rhs; > return 0; > } > > int > -sqlSubInt64(i64 * pA, i64 iB) > +sql_sub_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg) > { > - testcase(iB == SMALLEST_INT64 + 1); > - if (iB == SMALLEST_INT64) { > - testcase((*pA) == (-1)); > - testcase((*pA) == 0); > - if ((*pA) >= 0) > - return 1; > - *pA -= iB; > + if (!is_lhs_neg && !is_rhs_neg) { > + uint64_t u_lhs = (uint64_t) lhs; > + uint64_t u_rhs = (uint64_t) rhs; > + /* > + * Unsigned arithmetic has no overflows, so to > + * check if lhs is less than rhs we can compare > + * result of their subtraction with lhs. 5. Why not to compare just the values? They are of the same sign, use '<'. > + */ > + *is_res_neg = u_lhs < (uint64_t)(u_lhs - u_rhs); > + if (! *is_res_neg) { > + *res = u_lhs - u_rhs; > + return 0; > + } > + if (u_rhs > (uint64_t) INT64_MAX + 1 && > + u_lhs < (uint64_t)(u_rhs - INT64_MAX - 1)) 6. Too complex. You wanted to check u_lhs - u_rhs < INT64_MIN. Then check it, in a one comparison. u_lhs - u_rhs < INT64_MIN But u_lhs is < u_rhs, so swap them and change the signs: u_rhs - u_lhs > (uint64_t) INT64_MAX + 1 No overflows are possible here. > + return -1; > + *res = lhs - rhs; > return 0; > - } else { > - return sqlAddInt64(pA, -iB); > } > + if (is_rhs_neg) { > + return sql_add_int(lhs, is_lhs_neg, -rhs, false, res, > + is_res_neg); > + } > + assert(is_lhs_neg && !is_rhs_neg); > + /* > + * (lhs - rhs) < 0, lhs < 0, rhs > 0: in this case their > + * difference must be less than INT64_MIN. > + */ > + if ((uint64_t) -lhs + (uint64_t) rhs > (uint64_t) INT64_MAX + 1) > + return -1; > + *is_res_neg = true; > + *res = lhs - rhs; > + return 0; > } > > int > -sqlMulInt64(i64 * pA, i64 iB) > +sql_mul_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg) > { > - i64 iA = *pA; > - if (iB > 0) { > - if (iA > LARGEST_INT64 / iB) > - return 1; > - if (iA < SMALLEST_INT64 / iB) > - return 1; > - } else if (iB < 0) { > - if (iA > 0) { > - if (iB < SMALLEST_INT64 / iA) > - return 1; > - } else if (iA < 0) { > - if (iB == SMALLEST_INT64) > - return 1; > - if (iA == SMALLEST_INT64) > - return 1; > - if (-iA > LARGEST_INT64 / -iB) > - return 1; > - } > + if (lhs == 0 || rhs == 0) { > + *res = 0; > + *is_res_neg = false; > + return 0; > + } > + /* > + * Multiplication of integers with same sign leads to > + * unsigned result. > + */ > + if (! (is_lhs_neg ^ is_rhs_neg)) { 7. Dear God, what is it? What about '!=' operator? Is it disabled, or !^ is faster? > + uint64_t u_res = is_lhs_neg ? > + (uint64_t) (-lhs) * (uint64_t) (-rhs) : > + (uint64_t) lhs * (uint64_t) (rhs); > + /* > + * Overflow detection is quite primitive due to > + * the absence of overflow with unsigned values: > + * lhs * rhs == res --> rhs == res / lhs; > + * If this predicate is false, then result was > + * reduced modulo UINT_MAX + 1. > + */ > + if ((is_lhs_neg && u_res / (uint64_t) (-lhs) != > + (uint64_t) (-rhs)) || > + (!is_lhs_neg && u_res / (uint64_t) lhs != (uint64_t) rhs)) > + return -1; > + *is_res_neg = false; > + *res = u_res; > + return 0; > + } > + /* > + * Make sure we've got only one combination of > + * positive and negative operands. > + */ > + if (is_lhs_neg) { > + SWAP(is_lhs_neg, is_rhs_neg); > + SWAP(lhs, rhs); > } > - *pA = iA * iB; > + assert(! is_lhs_neg && is_rhs_neg); > + uint64_t u_rhs = (uint64_t) (-rhs); > + uint64_t u_res = u_rhs * (uint64_t) lhs; > + if (u_res / u_rhs != (uint64_t) lhs || > + u_rhs * (uint64_t) lhs > (uint64_t) INT64_MAX + 1) 8. You have already calculated that multiplication, it is u_res. > + return -1; > + *is_res_neg = true; > + *res = lhs * rhs; > + return 0; > +} > + > +int > +sql_div_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg) > +{ > + if (lhs == 0) { > + *res = 0; > + *is_res_neg = false; > + return 0; > + } > + /* > + * The only possible overflow situations is when operands > + * of different signs and result turns out to be less > + * than INT64_MIN. > + */ > + if (is_lhs_neg ^ is_rhs_neg) { 9. 😢 > + uint64_t u_res = is_lhs_neg ? > + (uint64_t) (-lhs) / (uint64_t) rhs : > + (uint64_t) lhs / (uint64_t) (-rhs); > + if (u_res > (uint64_t) INT64_MAX + 1) > + return -1; > + *is_res_neg = u_res != 0 ? true : false; 10. Does not it look tautological to you? 'if then true else false'. > + *res = -u_res; > + return 0; > + } > + *is_res_neg = false; > + /* > + * Another one special case: INT64_MIN / -1 > + * Signed division leads to program termination due > + * to overflow. > + */ > + if (is_lhs_neg && lhs == INT64_MIN && rhs == -1) { > + *res = (uint64_t) INT64_MAX + 1; > + return 0; > + } > + *res = is_lhs_neg ? (uint64_t) (lhs / rhs) : 11. Why do you need that cast? *res is int64_t, so it makes no sense to cast the rvalue to uint64_t. > + (uint64_t) lhs / (uint64_t) rhs; > return 0; > } > > -/* > - * Compute the absolute value of a 32-bit signed integer, of possible. Or > - * if the integer has a value of -2147483648, return +2147483647 > - */ > int > -sqlAbsInt32(int x) > +sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, > + int64_t *res, bool *is_res_neg) > { > - if (x >= 0) > - return x; > - if (x == (int)0x80000000) > - return 0x7fffffff; > - return -x; > + if (is_lhs_neg) { > + uint64_t u_lhs = (uint64_t) (-lhs); > + uint64_t u_res = u_lhs % (uint64_t) rhs; 12. Why do you ignore sign of 'rhs'? You can't just cast it, if it is negative. > + if (u_res > (uint64_t) INT64_MAX + 1) > + return -1; > + *res = -u_res; > + *is_res_neg = true; > + return 0; > + } > + /* > + * While calculating remainder we always ignore sign of > + * rhs - it doesn't affect the result. > + * */ > + uint64_t u_lhs = (uint64_t) lhs; > + uint64_t u_rhs = is_rhs_neg ? (uint64_t) (-rhs) : (uint64_t) rhs; > + *res = u_lhs % u_rhs; > + *is_res_neg = false; > + return 0; > } > > /* > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index d141397a0..9c28b9131 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1621,27 +1621,48 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > (type2 & (MEM_Int | MEM_UInt)) !=0) { > iA = pIn1->u.i; > iB = pIn2->u.i; > + bool is_lhs_neg = pIn1->flags & MEM_Int; > + bool is_rhs_neg = pIn2->flags & MEM_Int; > + bool is_res_neg; > bIntint = 1; > switch( pOp->opcode) { > - case OP_Add: if (sqlAddInt64(&iB,iA)) goto integer_overflow; break; > - case OP_Subtract: if (sqlSubInt64(&iB,iA)) goto integer_overflow; break; > - case OP_Multiply: if (sqlMulInt64(&iB,iA)) goto integer_overflow; break; > + case OP_Add: { > + if (sql_add_int(iA, is_lhs_neg, iB, is_rhs_neg, > + (int64_t *) &iB, &is_res_neg) != 0) 13. Why do you need all these casts to (int64_t *)? These variables already are signed 64 bit integers. > + goto integer_overflow; > + break; > + } > + case OP_Subtract: { > + if (sql_sub_int(iB, is_rhs_neg, iA, is_lhs_neg, > + (int64_t *) &iB, &is_res_neg) != 0) > + goto integer_overflow; > + break; > + } > + case OP_Multiply: { > + if (sql_mul_int(iA, is_lhs_neg, iB, is_rhs_neg, > + (int64_t *) &iB, &is_res_neg) != 0) > + goto integer_overflow; > + break; > + } > case OP_Divide: { > if (iA == 0) > goto division_by_zero; > - if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow; > - iB /= iA; > + if (sql_div_int(iB, is_rhs_neg, iA, is_lhs_neg, > + (int64_t *) &iB, &is_res_neg) != 0) > + goto integer_overflow; > break; > } > default: { > if (iA == 0) > goto division_by_zero; > if (iA==-1) iA = 1; > - iB %= iA; > + if (sql_rem_int(iB, is_rhs_neg, iA, is_lhs_neg, > + (int64_t *) &iB, &is_res_neg) != 0) > + goto integer_overflow; > break; > } > } > - mem_set_int(pOut, iB, iB < 0); > + mem_set_int(pOut, iB, is_res_neg); > } else { > bIntint = 0; > if (sqlVdbeRealValue(pIn1, &rA) != 0) { > @@ -1883,13 +1904,13 @@ case OP_ShiftRight: { /* same as TK_RSHIFT, in1, in2, out3 */ > break; > } > bool unused; > - if (sqlVdbeIntValue(pIn2, &iA, &unused) != 0) { > + if (sqlVdbeIntValue(pIn2, (int64_t *) &iA, &unused) != 0) { > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > sql_value_text(pIn2), "integer"); > rc = SQL_TARANTOOL_ERROR; > goto abort_due_to_error; > } > - if (sqlVdbeIntValue(pIn1, &iB, &unused) != 0) { > + if (sqlVdbeIntValue(pIn1, (int64_t *) &iB, &unused) != 0) { > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > sql_value_text(pIn1), "integer"); > rc = SQL_TARANTOOL_ERROR; > @@ -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. > + (int64_t *) &x, &unused) != 0) { > /* If the LIMIT is less than or equal to zero, loop forever. This > * is documented. But also, if the LIMIT+OFFSET exceeds 2^63 then > * also loop forever. This is undocumented. In fact, one could argue Consider my review fixes here and on the branch in a separate commit. These changes and the comments are arguable, probably I am wrong somewhere. ============================================================= diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index c0e2ab699..de63d4a6f 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4489,7 +4489,7 @@ sql_div_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, int64_t *res, bool *is_res_neg); int -sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, +sql_mod_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, int64_t *res, bool *is_res_neg); u8 sqlGetBoolean(const char *z, u8); diff --git a/src/box/sql/util.c b/src/box/sql/util.c index 0688087d4..1cba4142b 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -1194,8 +1194,8 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, if (is_lhs_neg && is_rhs_neg) { assert(lhs < 0 && rhs < 0); /* This is the same as (lhs + rhs) < INT64_MIN */ - if (-(lhs + INT64_MAX) > rhs + 1) - return -1; + if (lhs < INT64_MIN - rhs) + return -1; *is_res_neg = true; *res = lhs + rhs; return 0; @@ -1211,16 +1211,10 @@ sql_add_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, *res = lhs + rhs; return 0; } - /* - * Make sure we've got only one combination of - * positive and negative operands. - */ - if (is_lhs_neg) { - SWAP(is_lhs_neg, is_rhs_neg); - SWAP(lhs, rhs); - } - assert(! is_lhs_neg && is_rhs_neg); - *is_res_neg = (uint64_t)(-rhs) > (uint64_t) lhs; + if (is_rhs_neg) + *is_res_neg = (uint64_t)(-rhs) > (uint64_t) lhs; + else + *is_res_neg = (uint64_t)(-lhs) > (uint64_t) rhs; *res = lhs + rhs; return 0; } @@ -1232,19 +1226,14 @@ sql_sub_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, if (!is_lhs_neg && !is_rhs_neg) { uint64_t u_lhs = (uint64_t) lhs; uint64_t u_rhs = (uint64_t) rhs; - /* - * Unsigned arithmetic has no overflows, so to - * check if lhs is less than rhs we can compare - * result of their subtraction with lhs. - */ - *is_res_neg = u_lhs < (uint64_t)(u_lhs - u_rhs); - if (! *is_res_neg) { + if (u_lhs >= u_rhs) { + *is_res_neg = false; *res = u_lhs - u_rhs; return 0; } - if (u_rhs > (uint64_t) INT64_MAX + 1 && - u_lhs < (uint64_t)(u_rhs - INT64_MAX - 1)) + if ((uint64_t) INT64_MAX + 1 < u_rhs - u_lhs) return -1; + *is_res_neg = true; *res = lhs - rhs; return 0; } @@ -1277,7 +1266,7 @@ sql_mul_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, * Multiplication of integers with same sign leads to * unsigned result. */ - if (! (is_lhs_neg ^ is_rhs_neg)) { + if (is_lhs_neg == is_rhs_neg) { uint64_t u_res = is_lhs_neg ? (uint64_t) (-lhs) * (uint64_t) (-rhs) : (uint64_t) lhs * (uint64_t) (rhs); @@ -1308,7 +1297,7 @@ sql_mul_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, uint64_t u_rhs = (uint64_t) (-rhs); uint64_t u_res = u_rhs * (uint64_t) lhs; if (u_res / u_rhs != (uint64_t) lhs || - u_rhs * (uint64_t) lhs > (uint64_t) INT64_MAX + 1) + u_res > (uint64_t) INT64_MAX + 1) return -1; *is_res_neg = true; *res = lhs * rhs; @@ -1329,13 +1318,13 @@ sql_div_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, * of different signs and result turns out to be less * than INT64_MIN. */ - if (is_lhs_neg ^ is_rhs_neg) { + if (is_lhs_neg != is_rhs_neg) { uint64_t u_res = is_lhs_neg ? (uint64_t) (-lhs) / (uint64_t) rhs : (uint64_t) lhs / (uint64_t) (-rhs); if (u_res > (uint64_t) INT64_MAX + 1) return -1; - *is_res_neg = u_res != 0 ? true : false; + *is_res_neg = u_res != 0; *res = -u_res; return 0; } @@ -1349,13 +1338,12 @@ sql_div_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, *res = (uint64_t) INT64_MAX + 1; return 0; } - *res = is_lhs_neg ? (uint64_t) (lhs / rhs) : - (uint64_t) lhs / (uint64_t) rhs; + *res = is_lhs_neg ? lhs / rhs : (uint64_t) lhs / (uint64_t) rhs; return 0; } int -sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, +sql_mod_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg, int64_t *res, bool *is_res_neg) { if (is_lhs_neg) { diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 4379bd123..7103db1c9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1619,19 +1619,19 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ switch( pOp->opcode) { case OP_Add: { if (sql_add_int(iA, is_lhs_neg, iB, is_rhs_neg, - (int64_t *) &iB, &is_res_neg) != 0) + &iB, &is_res_neg) != 0) goto integer_overflow; break; } case OP_Subtract: { if (sql_sub_int(iB, is_rhs_neg, iA, is_lhs_neg, - (int64_t *) &iB, &is_res_neg) != 0) + &iB, &is_res_neg) != 0) goto integer_overflow; break; } case OP_Multiply: { if (sql_mul_int(iA, is_lhs_neg, iB, is_rhs_neg, - (int64_t *) &iB, &is_res_neg) != 0) + &iB, &is_res_neg) != 0) goto integer_overflow; break; } @@ -1639,7 +1639,7 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ if (iA == 0) goto division_by_zero; if (sql_div_int(iB, is_rhs_neg, iA, is_lhs_neg, - (int64_t *) &iB, &is_res_neg) != 0) + &iB, &is_res_neg) != 0) goto integer_overflow; break; } @@ -1647,8 +1647,8 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ if (iA == 0) goto division_by_zero; if (iA==-1) iA = 1; - if (sql_rem_int(iB, is_rhs_neg, iA, is_lhs_neg, - (int64_t *) &iB, &is_res_neg) != 0) + if (sql_mod_int(iB, is_rhs_neg, iA, is_lhs_neg, + &iB, &is_res_neg) != 0) goto integer_overflow; break; } @@ -1895,13 +1895,13 @@ case OP_ShiftRight: { /* same as TK_RSHIFT, in1, in2, out3 */ break; } bool unused; - if (sqlVdbeIntValue(pIn2, (int64_t *) &iA, &unused) != 0) { + if (sqlVdbeIntValue(pIn2, &iA, &unused) != 0) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_text(pIn2), "integer"); rc = SQL_TARANTOOL_ERROR; goto abort_due_to_error; } - if (sqlVdbeIntValue(pIn1, (int64_t *) &iB, &unused) != 0) { + if (sqlVdbeIntValue(pIn1, &iB, &unused) != 0) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_text(pIn1), "integer"); rc = SQL_TARANTOOL_ERROR; @@ -5165,9 +5165,8 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ x = pIn1->u.i; 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, - (int64_t *) &x, &unused) != 0) { + if (x == 0 || (pIn1->flags & MEM_Int) != 0 || + sql_add_int(x, false, rhs, false, &x, &unused) != 0) { /* If the LIMIT is less than or equal to zero, loop forever. This * is documented. But also, if the LIMIT+OFFSET exceeds 2^63 then * also loop forever. This is undocumented. In fact, one could argue