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 EB98F228AD for ; Mon, 1 Jul 2019 10:21:12 -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 TQ_qvzELL9w0 for ; Mon, 1 Jul 2019 10:21:12 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 67886220EC for ; Mon, 1 Jul 2019 10:21:12 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints From: "n.pettik" In-Reply-To: <6f7ec2c9-c325-601a-234f-97d6c6ced195@tarantool.org> Date: Mon, 1 Jul 2019 17:21:10 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <0e009036b6c6ce9e2d1f2ff66063291cb60e1387.1559919361.git.korablev@tarantool.org> <6f7ec2c9-c325-601a-234f-97d6c6ced195@tarantool.org> 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 Cc: Vladislav Shpilevoy >> diff --git a/src/box/sql/func.c b/src/box/sql/func.c >> index bb7405e68..f4c1cbcca 100644 >> --- a/src/box/sql/func.c >> +++ b/src/box/sql/func.c >> @@ -111,6 +111,7 @@ typeofFunc(sql_context * context, int NotUsed, = sql_value ** argv) >> UNUSED_PARAMETER(NotUsed); >> switch (sql_value_type(argv[0])) { >> case MP_INT: >> + case MP_UINT: >=20 > 1. Why on MP_UINT do you return 'integer' instead of > 'unsigned=E2=80=99? It is fixed in the next patch (and the rest of built-in functions). >> z =3D "integer"; >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 6ecdb26fc..d141397a0 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -900,8 +904,8 @@ case OP_InitCoroutine: { /* jump */ >> assert(pOp->p3>=3D0 && pOp->p3nOp); >> pOut =3D &aMem[pOp->p1]; >> assert(!VdbeMemDynamic(pOut)); >> - pOut->u.i =3D pOp->p3 - 1; >> - pOut->flags =3D MEM_Int; >> + pOut->u.u =3D pOp->p3 - 1; >=20 > 2. Why are you sure, that p3 >=3D 1? According to > the assertion above it is >=3D 0, but we don't know > anything about p3 >=3D 1. P3 is an address of opcode. OP_Init features 0 address (always), ergo other instructions have addresses >=3D 1. >> @@ -1099,7 +1108,7 @@ case OP_Bool: { /* out2 */ >> case OP_Int64: { /* out2 */ >> pOut =3D out2Prerelease(p, pOp); >> assert(pOp->p4.pI64!=3D0); >> - pOut->u.i =3D *pOp->p4.pI64; >> + mem_set_int(pOut, *pOp->p4.pI64, *pOp->p4.pI64 < 0); >=20 > 3. Sorry, but such places are hell. You pass separately a value > and a flag if it is negative. Please, introduce separate functions >=20 > /* Set a value, check sign inside. */ > mem_set_i64(int64_t value); >=20 > /* Set a non-negative value. */ > mem_set_u64(uint64_t value); >=20 > /* Set a big integer with sign passed separately. */ > mem_set_int(uint64_t value, bool is_negative); >=20 > In most places you will use mem_set_i64 and mem_set_u64. It > will look shorter and somewhere work faster. Done. Note that I=E2=80=99ve also rebased onto the fresh master, so real diff may turn out to be slightly different. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 602b39eb9..6d6300f97 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -313,7 +313,7 @@ mem_apply_type(struct Mem *record, enum field_type = type) if ((record->flags & MEM_Real) =3D=3D MEM_Real) { int64_t i =3D (int64_t) record->u.r; if (i =3D=3D record->u.r) - mem_set_int(record, i, i < 0); + mem_set_i64(record, i); return 0; } return sqlVdbeMemIntegerify(record, false); @@ -898,7 +898,7 @@ case OP_Gosub: { /* jump */ pIn1 =3D &aMem[pOp->p1]; assert(VdbeMemDynamic(pIn1)=3D=3D0); memAboutToChange(p, pIn1); - mem_set_int(pIn1, pOp - aOp, false); + mem_set_u64(pIn1, pOp - aOp); REGISTER_TRACE(p, pOp->p1, pIn1); =20 /* Most jump operations do a goto to this spot in order to = update @@ -939,7 +939,7 @@ case OP_InitCoroutine: { /* jump */ assert(pOp->p3>=3D0 && pOp->p3nOp); pOut =3D &aMem[pOp->p1]; assert(!VdbeMemDynamic(pOut)); - mem_set_int(pOut, pOp->p3 - 1, false); + mem_set_u64(pOut, pOp->p3 - 1); if (pOp->p2) goto jump_to_p2; break; } @@ -982,7 +982,7 @@ case OP_Yield: { /* in1, jump */ pIn1 =3D &aMem[pOp->p1]; assert(VdbeMemDynamic(pIn1)=3D=3D0); int pcDest =3D (int)pIn1->u.u; - mem_set_int(pIn1, pOp - aOp, false); + mem_set_u64(pIn1, pOp - aOp); REGISTER_TRACE(p, pOp->p1, pIn1); pOp =3D &aOp[pcDest]; break; @@ -1064,7 +1064,7 @@ case OP_Integer: { /* out2 */ if (pOp->p1 < 0) pOut->u.i =3D pOp->p1; else - mem_set_int(pOut, pOp->p1, false); + mem_set_u64(pOut, pOp->p1); break; } =20 @@ -1089,7 +1089,7 @@ case OP_Bool: { /* out2 */ case OP_Int64: { /* out2 */ pOut =3D out2Prerelease(p, pOp); assert(pOp->p4.pI64!=3D0); - mem_set_int(pOut, *pOp->p4.pI64, *pOp->p4.pI64 < 0); + mem_set_i64(pOut, *pOp->p4.pI64); break; } =20 @@ -1173,7 +1173,7 @@ case OP_NextAutoincValue: { goto abort_due_to_error; =20 pOut =3D out2Prerelease(p, pOp); - mem_set_int(pOut, value, value < 0); + mem_set_i64(pOut, value); =20 break; } @@ -1605,7 +1605,7 @@ case OP_Remainder: { /* same as TK_REM, = in1, in2, out3 */ break; } } - mem_set_int(pOut, iB, iB < 0); + mem_set_i64(pOut, iB); } else { bIntint =3D 0; if (sqlVdbeRealValue(pIn1, &rA) !=3D 0) { @@ -1870,7 +1870,7 @@ case OP_ShiftRight: { /* same as = TK_RSHIFT, in1, in2, out3 */ memcpy(&iA, &uA, sizeof(iA)); } } - mem_set_int(pOut, iA, iA < 0); + mem_set_i64(pOut, iA); break; } =20 @@ -2437,7 +2437,7 @@ case OP_BitNot: { /* same as = TK_BITNOT, in1, out2 */ sql_value_text(pIn1), "integer"); goto abort_due_to_error; } - mem_set_int(pOut, ~i, ~i < 0); + mem_set_i64(pOut, ~i); } break; } @@ -2798,7 +2798,7 @@ case OP_Count: { /* out2 */ nEntry =3D tarantoolsqlEphemeralCount(pCrsr); } pOut =3D out2Prerelease(p, pOp); - mem_set_int(pOut, nEntry, false); + mem_set_u64(pOut, nEntry); break; } =20 @@ -3603,7 +3603,7 @@ case OP_Sequence: { /* out2 */ assert(p->apCsr[pOp->p1]!=3D0); pOut =3D out2Prerelease(p, pOp); int64_t seq_val =3D p->apCsr[pOp->p1]->seqCount++; - mem_set_int(pOut, seq_val, false); + mem_set_u64(pOut, seq_val); break; } =20 @@ -3619,8 +3619,7 @@ case OP_NextSequenceId: { uint64_t id; tarantoolSqlNextSeqId(&id); id++; - pOut->u.u =3D id; - pOut->flags =3D MEM_UInt; + mem_set_u64(pOut, id); break; } =20 @@ -3650,7 +3649,7 @@ case OP_NextIdEphemeral: { goto abort_due_to_error; } pOut =3D &aMem[pOp->p2]; - mem_set_int(pOut, rowid, false); + mem_set_u64(pOut, rowid); break; } =20 @@ -4904,9 +4903,9 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ * it would take nearly 300 years to actually reach the = limit. So * looping forever is a reasonable approximation. */ - mem_set_int(pOut, -1, true); + mem_set_i64(pOut, -1); } else { - mem_set_int(pOut, x, false); + mem_set_u64(pOut, x); } break; } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 8df34bb27..b83926a66 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -470,6 +470,21 @@ mem_set_bool(struct Mem *mem, bool value); void mem_set_ptr(struct Mem *mem, void *ptr); =20 +/** + * Set integer value. Depending on its sign MEM_Int (in case + * of negative value) or MEM_UInt flag is set. + */ +void +mem_set_i64(struct Mem *mem, int64_t value); + +/** Set unsigned value and MEM_UInt flag. */ +void +mem_set_u64(struct Mem *mem, uint64_t value); + +/** + * Set integer value. According to is_neg flag value is considered + * to be signed or unsigned. + */ void mem_set_int(struct Mem *mem, int64_t value, bool is_neg); =20 diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 57bf25ed8..705e869bc 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -322,7 +322,7 @@ sql_result_double(sql_context * pCtx, double rVal) void sql_result_int(sql_context * pCtx, int iVal) { - mem_set_int(pCtx->pOut, iVal, iVal < 0); + mem_set_i64(pCtx->pOut, iVal); } =20 void @@ -334,7 +334,7 @@ sql_result_bool(struct sql_context *ctx, bool value) void sql_result_int64(sql_context * pCtx, i64 iVal) { - mem_set_int(pCtx->pOut, iVal, iVal < 0); + mem_set_i64(pCtx->pOut, iVal); } =20 void @@ -998,7 +998,7 @@ sql_bind_int64(sql_stmt * pStmt, int i, sql_int64 = iValue) if (vdbeUnbind(p, i) !=3D 0) return -1; int rc =3D sql_bind_type(p, i, "INTEGER"); - mem_set_int(&p->aVar[i - 1], iValue, iValue < 0); + mem_set_i64(&p->aVar[i - 1], iValue); return rc; } =20 diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 60a4e8669..d5ecd8f43 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1430,7 +1430,7 @@ sqlVdbeList(Vdbe * p) } if (p->explain =3D=3D 1) { assert(i >=3D 0); - mem_set_int(pMem, i, false); + mem_set_u64(pMem, i); =20 pMem++; =20 @@ -1463,13 +1463,13 @@ sqlVdbeList(Vdbe * p) } } =20 - mem_set_int(pMem, pOp->p1, pOp->p1 < 0); + mem_set_i64(pMem, pOp->p1); pMem++; =20 - mem_set_int(pMem, pOp->p2, pOp->p2 < 0); + mem_set_i64(pMem, pOp->p2); pMem++; =20 - mem_set_int(pMem, pOp->p3, pOp->p3 < 0); + mem_set_i64(pMem, pOp->p3); pMem++; =20 if (sqlVdbeMemClearAndResize(pMem, 256)) { diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 51d4787f7..5387c6f33 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -798,6 +798,25 @@ mem_set_bool(struct Mem *mem, bool value) mem->flags =3D MEM_Bool; } =20 +void +mem_set_i64(struct Mem *mem, int64_t value) +{ + if (VdbeMemDynamic(mem)) + sqlVdbeMemSetNull(mem); + mem->u.i =3D value; + int flag =3D value < 0 ? MEM_Int : MEM_UInt; + MemSetTypeFlag(mem, flag); +} + +void +mem_set_u64(struct Mem *mem, uint64_t value) +{ + if (VdbeMemDynamic(mem)) + sqlVdbeMemSetNull(mem); + mem->u.u =3D value; + MemSetTypeFlag(mem, MEM_UInt); +} + void mem_set_int(struct Mem *mem, int64_t value, bool is_neg) { @@ -1376,8 +1395,7 @@ valueFromExpr(sql * db, /* The database = connection */ if (pVal =3D=3D 0) goto no_mem; if (ExprHasProperty(pExpr, EP_IntValue)) { - mem_set_int(pVal, (i64) pExpr->u.iValue * = negInt, - pExpr->u.iValue * negInt < 0); + mem_set_i64(pVal, (i64) pExpr->u.iValue * = negInt); } else { zVal =3D sqlMPrintf(db, "%s%s", zNeg, = pExpr->u.zToken); >> @@ -1609,7 +1617,8 @@ case OP_Remainder: { /* same as = TK_REM, in1, in2, out3 */ >> pOut =3D &aMem[pOp->p3]; >> flags =3D pIn1->flags | pIn2->flags; >> if ((flags & MEM_Null)!=3D0) goto arithmetic_result_is_null; >> - if ((type1 & type2 & MEM_Int)!=3D0) { >> + if ((type1 & (MEM_Int | MEM_UInt)) !=3D0 && >> + (type2 & (MEM_Int | MEM_UInt)) !=3D0) { >> iA =3D pIn1->u.i; >> iB =3D pIn2->u.i; >=20 > 4. How can you access pIn1/2->u.i if the types can be MEM_UInt? There=E2=80=99s no opportunity to operate on values in the range [INT64_MAX + 1, UINT64_MAX] in scope of this patch. So, basically it is safe to use values as integers members of union. It is fixed in the next patch as well as other arithmetic operations. >> bIntint =3D 1; >> @@ -2485,14 +2490,14 @@ case OP_BitNot: { /* same as = TK_BITNOT, in1, out2 */ >> sqlVdbeMemSetNull(pOut); >> if ((pIn1->flags & MEM_Null)=3D=3D0) { >> int64_t i; >> - if (sqlVdbeIntValue(pIn1, &i) !=3D 0) { >> + bool is_neg; >> + if (sqlVdbeIntValue(pIn1, &i, &is_neg) !=3D 0) { >> diag_set(ClientError, ER_SQL_TYPE_MISMATCH, >> sql_value_text(pIn1), "integer"); >> rc =3D SQL_TARANTOOL_ERROR; >> goto abort_due_to_error; >> } >> - pOut->flags =3D MEM_Int; >> - pOut->u.i =3D ~i; >> + mem_set_int(pOut, ~i, ~i < 0); >=20 > 5. Why do you compare 'i' with 0? What if it was a big > unsigned value? See answer above: in this patch =E2=80=9Ci=E2=80=9D can=E2=80=99t be = =E2=80=9Cbig unsigned value=E2=80=9D. > You have 'is_neg' flag for this, don't you? >=20 >> } >> break; >> } >> @@ -3507,12 +3515,16 @@ case OP_SeekGT: { /* jump, in3 */ >> * the seek, so convert it. >> */ >> pIn3 =3D &aMem[reg_ipk]; >> - if ((pIn3->flags & (MEM_Int|MEM_Real|MEM_Str))=3D=3DMEM_St= r) { >> + if ((pIn3->flags & = (MEM_Int|MEM_UInt|MEM_Real|MEM_Str))=3D=3DMEM_Str) { >=20 > 6. Please, remind me how is it possible, that a value is at the > same time string and number? Why not to check for flags & MEM_Str !=3D = 0 > only? It definitely could be some time ago (when string was converted to a = number). I do not remember reason of that behaviour. Now it seems that this = situation doesn=E2=80=99t appear anymore, so we really can simplify this check. = Fixed by applying your diff. >> @@ -3814,10 +3828,10 @@ case OP_Sequence: { /* out2 */ >> */ >> case OP_NextSequenceId: { >> pOut =3D &aMem[pOp->p2]; >> - tarantoolSqlNextSeqId((uint64_t *) &pOut->u.i); >> - >> - pOut->u.i +=3D 1; >> - pOut->flags =3D MEM_Int; >> + int64_t id; >> + tarantoolSqlNextSeqId((uint64_t *) &id); >> + id++; >> + mem_set_int(pOut, id, id < 0); >=20 > 7. tarantoolSqlNextSeqId returns _sequence.id field. The > field has unsigned type. How is it possible, that it is < 0? No way. Fixed: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7e7c12ee4..47f558011 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3828,10 +3828,11 @@ case OP_Sequence: { /* out2 */ */ case OP_NextSequenceId: { pOut =3D &aMem[pOp->p2]; - int64_t id; - tarantoolSqlNextSeqId((uint64_t *) &id); + uint64_t id; + tarantoolSqlNextSeqId(&id); id++; - mem_set_int(pOut, id, id < 0); + mem_set_u64(pOut, id); break; } >> @@ -5079,10 +5092,17 @@ case OP_FkIfZero: { /* jump */ >> */ >> case OP_IfPos: { /* jump, in1 */ >> pIn1 =3D &aMem[pOp->p1]; >> - assert(pIn1->flags&MEM_Int); >> - VdbeBranchTaken( pIn1->u.i>0, 2); >> - if (pIn1->u.i>0) { >> - pIn1->u.i -=3D pOp->p3; >> + assert((pIn1->flags & (MEM_Int | MEM_UInt)) !=3D 0); >> + if ((pIn1->flags & MEM_UInt) !=3D 0 && pIn1->u.u !=3D 0) { >> + assert(pOp->p3 >=3D 0); >> + uint64_t res =3D pIn1->u.u - (uint64_t) pOp->p3; >> + /* >> + * To not bother setting integer flag in case >> + * result of subtraction is negative, just >> + * use saturated arithmetic. >> + */ >> + res &=3D -(res <=3D pIn1->u.u); >=20 > 8. I do not understand. Why do you compare res and pIn1->u.u? > You compare 'u.u' and 'u.u - p3'. Obvioiusly, the latter is > smaller. Probably, this is because you rely on 'res' being > overflowed will be bigger than 'u.u=E2=80=99? Yep. >=20 > In such a case, I think it would be cleaner to compare 'u.u' and > 'p3'. >=20 > res &=3D -(u.u > p3); Sorry, can=E2=80=99t understand what you mean or how it should be implemented. This is common way to implement saturated subtraction. > Additionally, now this opcode's comment is wrong - P1 is not equal > to P1 - P3 from this moment. Ok, fixed a bit comment: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index b93e12b9a..fcc3b250c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -5115,7 +5115,8 @@ case OP_FkIfZero: { /* jump */ * * Register P1 must contain an integer. * If the value of register P1 is 1 or greater, subtract P3 from the - * value in P1 and jump to P2. + * value in P1 and jump to P2. If the result of subtraction is + * negative, then register P1 will hold 0. * * If the initial value of register P1 is less than 1, then the * value is unchanged and control passes through to the next = instruction. >> @@ -5111,10 +5131,10 @@ case OP_OffsetLimit: { /* in1, out2, in3 = */ >> pIn1 =3D &aMem[pOp->p1]; >> pIn3 =3D &aMem[pOp->p3]; >> pOut =3D out2Prerelease(p, pOp); >> - assert(pIn1->flags & MEM_Int); >> - assert(pIn3->flags & MEM_Int); >> + assert(pIn1->flags & (MEM_Int | MEM_UInt)); >> + assert(pIn3->flags & (MEM_Int | MEM_UInt)); >> x =3D pIn1->u.i; >> - if (x<=3D0 || sqlAddInt64(&x, pIn3->u.i>0?pIn3->u.i:0)) { >> + if (x<=3D0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) { >=20 > 9. Why do you touch pIn3.u.i without checking its type? In case it > is a big unsigned, you will get 'u.i < 0'. Additionally, as I = understand, > now no one should treat 'u.i' as an unsigned. It is supposed to be > negative always. Due to the reason I=E2=80=99ve already mentioned: in this patch = there=E2=80=99s no chance that values in range [INT64_MAX + 1, =E2=80=A6] get to VDBE. >> /* 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 >> @@ -5124,8 +5144,9 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ >> * looping forever is a reasonable approximation. >> */ >> pOut->u.i =3D -1; >> + pOut->flags =3D MEM_Int; >> } else { >> - pOut->u.i =3D x; >> + mem_set_int(pOut, x, x < 0); >=20 > 10. Here 'x' is always >=3D 0. Fixed: @@ -5178,7 +5179,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ */ mem_set_int(pOut, -1, true); } else { - mem_set_int(pOut, x, x < 0); + mem_set_u64(pOut, x,); } break; } >> @@ -5140,7 +5161,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ >> */ >> case OP_IfNotZero: { /* jump, in1 */ >> pIn1 =3D &aMem[pOp->p1]; >> - assert(pIn1->flags&MEM_Int); >> + assert(pIn1->flags&(MEM_Int|MEM_UInt)); >> VdbeBranchTaken(pIn1->u.i<0, 2); >> if (pIn1->u.i) { >> if (pIn1->u.i>0) pIn1->u.i--; >=20 > 11. You can't treat 'u.i' as a positive value without > checking the flags. P1 is LIMIT counter (according to the only usage of this opcode) and it can be only positive value. It is checked in computeLimitRegisters(). I=E2=80=99ve fixed an assertion: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index a08194178..f4d311927 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -5149,7 +5149,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ */ case OP_IfNotZero: { /* jump, in1 */ pIn1 =3D &aMem[pOp->p1]; - assert((pIn1->flags & (MEM_Int | MEM_UInt)) !=3D 0); + assert((pIn1->flags & MEM_UInt) !=3D 0); - VdbeBranchTaken(pIn1->u.i<0, 2); - if (pIn1->u.i) { - if (pIn1->u.i>0) pIn1->u.i--; + VdbeBranchTaken(pIn1->u.u<0, 2); + if (pIn1->u.u) { + if (pIn1->u.u > 0) pIn1->u.u--; >> @@ -5157,7 +5178,7 @@ case OP_IfNotZero: { /* jump, in1 */ >> */ >> case OP_DecrJumpZero: { /* jump, in1 */ >> pIn1 =3D &aMem[pOp->p1]; >> - assert(pIn1->flags&MEM_Int); >> + assert((pIn1->flags&(MEM_Int|MEM_UInt)) !=3D 0); >> if (pIn1->u.i>SMALLEST_INT64) pIn1->u.i--; >=20 > 12. The same. The same is here: P1 is the LIMIT counter, so I=E2=80=99ve fixed an assertion: @@ -5166,7 +5166,7 @@ case OP_IfNotZero: { /* jump, in1 */ */ case OP_DecrJumpZero: { /* jump, in1 */ pIn1 =3D &aMem[pOp->p1]; - assert((pIn1->flags & (MEM_Int | MEM_UInt)) !=3D 0); + assert((pIn1->flags & MEM_UInt) !=3D 0); - if (pIn1->u.i>SMALLEST_INT64) pIn1->u.i--; + if (pIn1->u.i>SMALLEST_INT64) pIn1->u.u--; VdbeBranchTaken(pIn1->u.i=3D=3D0, 2); - if (pIn1->u.i=3D=3D0) goto jump_to_p2; + if (pIn1->u.u =3D=3D 0) goto jump_to_p2; break; } >> @@ -5396,12 +5417,12 @@ case OP_Init: { /* jump */ >> case OP_IncMaxid: { >> assert(pOp->p1 > 0); >> pOut =3D &aMem[pOp->p1]; >> - >> - rc =3D tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i); >> + int64_t id; >> + rc =3D tarantoolsqlIncrementMaxid((uint64_t*) &id); >=20 > 13. Id is always non-negative. Fixed: @@ -5417,12 +5418,13 @@ case OP_Init: { /* jump */ case OP_IncMaxid: { assert(pOp->p1 > 0); pOut =3D &aMem[pOp->p1]; - int64_t id; - rc =3D tarantoolsqlIncrementMaxid((uint64_t*) &id); + uint64_t id; + rc =3D tarantoolsqlIncrementMaxid(&id); if (rc!=3DSQL_OK) { goto abort_due_to_error; } - mem_set_int(pOut, id, id < 0); + mem_set_u64(pOut, id); break; } >> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h >> index a3100e513..abed46486 100644 >> --- a/src/box/sql/vdbeInt.h >> +++ b/src/box/sql/vdbeInt.h >> @@ -259,9 +261,14 @@ struct Mem { >> * auxiliary flags. >> */ >> enum { >> - MEM_PURE_TYPE_MASK =3D 0x3f >> + MEM_PURE_TYPE_MASK =3D 0x7f >> }; >>=20 >> +static_assert((int) MEM_PURE_TYPE_MASK =3D=3D (MEM_Null | MEM_Str | = MEM_Int | >=20 > 14. Why do you need the cast to 'int=E2=80=99? Well, in fact I don=E2=80=99t need to do that. Fixed by applying your = diff. >=20 >> + MEM_Real | MEM_Blob | = MEM_Bool | >> + MEM_UInt), >> + "value of type mask must consist of corresponding to = memory type bits"); >=20 > 15. Out of 80. Same. >> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c >> index e480ae720..393782c23 100644 >> --- a/src/box/sql/vdbeapi.c >> +++ b/src/box/sql/vdbeapi.c >> @@ -233,7 +234,8 @@ sql_int64 >> sql_value_int64(sql_value * pVal) >> { >> int64_t i =3D 0; >> - sqlVdbeIntValue((Mem *) pVal, &i); >> + bool is_neg; >> + sqlVdbeIntValue((Mem *) pVal, &i, &is_neg); >> return i; >=20 > 16. The result integer is invalid in case it was a > big unsigned value. In the next patches I=E2=80=99m introducing sql_value_uint(), so it=E2=80=99s OK. However, I=E2=80=99d like to rename is_neg to unused to underline that the usage is correct: diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 3c0a9cc59..5ba7981ec 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -234,8 +234,8 @@ sql_int64 sql_value_int64(sql_value * pVal) { int64_t i =3D 0; - bool is_neg; - sqlVdbeIntValue((Mem *) pVal, &i, &is_neg); + bool unused; + sqlVdbeIntValue((Mem *) pVal, &i, &unused); return i; } >> @@ -385,7 +388,7 @@ sql_result_error(sql_context * pCtx, const char = *z, int n) >> void >> sql_result_int(sql_context * pCtx, int iVal) >> { >> - sqlVdbeMemSetInt64(pCtx->pOut, (i64) iVal); >> + mem_set_int(pCtx->pOut, (i64) iVal, iVal < 0); >=20 > 17. int to int64 cast works implicitly. You don't need an > explicit cast. Ok, fixed in your diff. >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index 25d4cd759..5a71e1801 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -1639,6 +1641,8 @@ sqlVdbeList(Vdbe * p) >> if (p->explain =3D=3D 1) { >> pMem->flags =3D MEM_Int; >> pMem->u.i =3D i; /* Program counter */ >> + mem_set_int(pMem, i, i < 0); >> + >> pMem++; >=20 > 18. Stray empty line + you do mem_set_int() twice - once you > call the function, and second you set flag and value manually. > Besides, program counter is always > 0. Fixed: diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 5a71e1801..de5fa5047 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1639,9 +1639,8 @@ sqlVdbeList(Vdbe * p) pOp =3D &apSub[j]->aOp[i]; } if (p->explain =3D=3D 1) { - pMem->flags =3D MEM_Int; - pMem->u.i =3D i; /* Program counter */ - mem_set_int(pMem, i, i < 0); + assert(i >=3D 0); + mem_set_int(pMem, i, false); =20 pMem++; >> @@ -3395,10 +3403,23 @@ sqlMemCompare(const Mem * pMem1, const Mem * = pMem2, const struct coll * pColl) >> return -1; >> } >> } >> + if ((f1 & MEM_UInt) !=3D 0) { >> + if ((f2 & MEM_Real) !=3D 0) { >> + return sqlIntFloatCompare(pMem1->u.i, >> + pMem2->u.r); >> + } else if ((f2 & MEM_Int) !=3D 0) { >> + return +1; >> + } else { >> + return -1; >> + } >> + } >> if ((f1 & MEM_Real) !=3D 0) { >> if ((f2 & MEM_Int) !=3D 0) { >> return -sqlIntFloatCompare(pMem2->u.i, >> = pMem1->u.r); >> + } else if ((f2 & MEM_UInt) !=3D 0) { >> + return -sqlIntFloatCompare(pMem2->u.u, >=20 > 19. You treat 'u.u' here as int64_t. Why? Same reason and fixed in the next patch. >> @@ -3563,13 +3584,14 @@ sqlVdbeCompareMsgpack(const char **key1, >> mem1.u.r =3D (double)v; >> goto do_float; >> } >> - mem1.u.i =3D v; >> + mem1.u.u =3D v; >> goto do_int; >> } >> case MP_INT:{ >> mem1.u.i =3D mp_decode_int(&aKey1); >> do_int: >> - if (pKey2->flags & MEM_Int) { >> + if ((pKey2->flags & MEM_Int) || >> + (pKey2->flags & MEM_UInt)) { >> if (mem1.u.i < pKey2->u.i) { >=20 > 20. How can you touch pKey2->u.i, if you don't know > if it is MEM_Int or MEM_UInt? The only thing you know is > that it is an integer. You don't know which. It is fixed in the next patch. >> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c >> index 3a361d066..25119ff16 100644 >> --- a/src/box/sql/vdbemem.c >> +++ b/src/box/sql/vdbemem.c >> @@ -418,7 +422,7 @@ sqlVdbeMemRelease(Mem * p) >> * return the closest available 64-bit signed integer. >> */ >> static int >> -doubleToInt64(double r, int64_t *i) >> +doubleToInt64(double r, int64_t *i, bool *is_neg) >=20 > 21. I understand 'is_neg' in the most of other places, > but why here? In a caller function you can always check > 'r < 0' if necessary. Fair, removed: diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 48cb556e9..a8ac9c806 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -422,7 +422,7 @@ sqlVdbeMemRelease(Mem * p) * return the closest available 64-bit signed integer. */ static int -doubleToInt64(double r, int64_t *i, bool *is_neg) +doubleToInt64(double r, int64_t *i) { /* * Many compilers we encounter do not define constants for the @@ -433,7 +433,6 @@ doubleToInt64(double r, int64_t *i, bool *is_neg) */ static const int64_t maxInt =3D LARGEST_INT64; static const int64_t minInt =3D SMALLEST_INT64; - *is_neg =3D r < 0.f; if (r <=3D (double)minInt) { *i =3D minInt; return -1; @@ -472,7 +471,9 @@ sqlVdbeIntValue(Mem * pMem, int64_t *i, bool = *is_neg) *is_neg =3D false; return 0; } else if (flags & MEM_Real) { - return doubleToInt64(pMem->u.r, i, is_neg); + *is_neg =3D pMem->u.r < 0; + return doubleToInt64(pMem->u.r, i); } else if (flags & (MEM_Str)) { assert(pMem->z || pMem->n =3D=3D 0); if (sql_atoi64(pMem->z, i, is_neg, pMem->n) =3D=3D 0) @@ -500,8 +501,7 @@ sqlVdbeRealValue(Mem * pMem, double *v) } else if (pMem->flags & MEM_UInt) { *v =3D (double)pMem->u.u; return 0; - } - else if (pMem->flags & MEM_Str) { + } else if (pMem->flags & MEM_Str) { if (sqlAtoF(pMem->z, v, pMem->n)) return 0; } @@ -530,9 +530,8 @@ mem_apply_integer_type(Mem *pMem) assert(pMem->flags & MEM_Real); assert(EIGHT_BYTE_ALIGNMENT(pMem)); =20 - bool is_neg; - if ((rc =3D doubleToInt64(pMem->u.r, (int64_t *) &ix, &is_neg)) = =3D=3D 0) - mem_set_int(pMem, ix, is_neg); + if ((rc =3D doubleToInt64(pMem->u.r, (int64_t *) &ix)) =3D=3D 0) + mem_set_int(pMem, ix, pMem->u.r <=3D -1); return rc; } =20 >> @@ -454,20 +458,24 @@ doubleToInt64(double r, int64_t *i) >> * If pMem represents a string value, its encoding might be changed. >> */ >> int >> -sqlVdbeIntValue(Mem * pMem, int64_t *i) >> +sqlVdbeIntValue(Mem * pMem, int64_t *i, bool *is_neg) >> { >> int flags; >> assert(EIGHT_BYTE_ALIGNMENT(pMem)); >> flags =3D pMem->flags; >> if (flags & MEM_Int) { >> *i =3D pMem->u.i; >> + *is_neg =3D true; >> + return 0; >> + } else if (flags & MEM_UInt) { >> + *i =3D pMem->u.u; >> + *is_neg =3D false; >> return 0; >> } else if (flags & MEM_Real) { >> - return doubleToInt64(pMem->u.r, i); >> + return doubleToInt64(pMem->u.r, i, is_neg); >> } else if (flags & (MEM_Str)) { >> assert(pMem->z || pMem->n =3D=3D 0); >> - bool is_neg; >> - if (sql_atoi64(pMem->z, (int64_t *)i, &is_neg, pMem->n) = =3D=3D 0) >> + if (sql_atoi64(pMem->z, (int64_t *)i, is_neg, pMem->n) = =3D=3D 0) >=20 > 22. I do not understand these casts. 'i' is already int64_t *. Why do > you cast it? Indeed, there=E2=80=99s no need to cast here. Removed in the previous = patch. >> @@ -489,7 +497,11 @@ sqlVdbeRealValue(Mem * pMem, double *v) >> } else if (pMem->flags & MEM_Int) { >> *v =3D (double)pMem->u.i; >> return 0; >> - } else if (pMem->flags & MEM_Str) { >> + } else if (pMem->flags & MEM_UInt) { >> + *v =3D (double)pMem->u.u; >> + return 0; >> + } >> + else if (pMem->flags & MEM_Str) { >=20 > 23. Stray line wrap. Fixed: diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 48cb556e9..241a8f78a 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -500,8 +500,7 @@ sqlVdbeRealValue(Mem * pMem, double *v) } else if (pMem->flags & MEM_UInt) { *v =3D (double)pMem->u.u; return 0; - } - else if (pMem->flags & MEM_Str) { + } else if (pMem->flags & MEM_Str) { if (sqlAtoF(pMem->z, v, pMem->n)) return 0; } >> @@ -777,42 +792,29 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n) >> pMem->z =3D 0; >> } >>=20 >> -/* >> - * The pMem is known to contain content that needs to be destroyed = prior >> - * to a value change. So invoke the destructor, then set the value = to >> - * a 64-bit integer. >> - */ >> -static SQL_NOINLINE void >> -vdbeReleaseAndSetInt64(Mem * pMem, i64 val) >> +void >> +mem_set_bool(struct Mem *mem, bool value) >=20 > 24. Please, move this function back below mem_set_int to > avoid diff padding out. But it is not used anymore, so compilation would result in error. > 25. I was confused by several places where you use Mem.u.i not > as a negative integer, but as a buffer of both integers. To avoid > further confusion I think we should add another alias to Mem.u - > 'ib' - integer buffer. It should be uint64_t, but due to its name > it will be clear that in places of its usage we just want to take > raw bytes of the stored integer, separately from its sign. >=20 > Besides, in a couple of places 'ib' usage will be faster. Sounds extremely dubious. > 1) sqlVdbeMemCast - here the only thing which matters if 'ib' is 0. > Now you check it twice - for 'u.u' and 'u.i'. >=20 > 2) OP_SeekGT, here: >=20 > if ((pIn3->flags & MEM_Int) =3D=3D MEM_Int) { > i =3D pIn3->u.i; > is_neg =3D true; > } else if ((pIn3->flags & MEM_UInt) =3D=3D MEM_UInt) { > i =3D pIn3->u.u; > is_neg =3D false; >=20 > In this place you could write 'i =3D pIn3->u.ib; is_neg =3D flags =3D=3D= MEM_Int;=E2=80=99. I don=E2=80=99t see difference between =E2=80=9Ci=E2=80=9D and =E2=80=9Cib= =E2=80=9D. IMHO it would only complicate code maintenance. > Please, consider my review fixes here and on the branch in a separate = commit: Applied.