From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Date: Fri, 5 Jul 2019 19:36:02 +0300 [thread overview] Message-ID: <7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org> (raw) In-Reply-To: <46ec2553-ae16-9a9f-8b85-882c2adc6031@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.
next prev parent reply other threads:[~2019-07-05 16:36 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-07 15:37 [tarantool-patches] [PATCH 0/6] Introduce UNSIGNED type in SQL Nikita Pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 1/6] sql: refactor sql_atoi64() Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:20 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:32 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:33 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik [this message] 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 12:24 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 4/6] sql: make built-in functions operate on unsigned values Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 5/6] sql: introduce extended range for INTEGER type Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-24 15:59 ` Konstantin Osipov 2019-07-24 16:54 ` n.pettik 2019-07-24 17:09 ` Konstantin Osipov 2019-06-07 15:37 ` [tarantool-patches] [PATCH 6/6] sql: allow to specify UNSIGNED column type Nikita Pettik 2019-07-01 21:53 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-11 21:25 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-07-18 20:56 ` n.pettik 2019-07-18 21:08 ` Vladislav Shpilevoy 2019-07-18 21:13 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-22 19:17 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-17 0:54 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-08-06 19:36 ` n.pettik 2019-07-24 13:01 ` [tarantool-patches] Re: [PATCH 0/6] Introduce UNSIGNED type in SQL Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox