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 B4A6E2535D for ; Wed, 10 Jul 2019 18:48:00 -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 4Lb58oy2umkp for ; Wed, 10 Jul 2019 18:48:00 -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 695C52531E for ; Wed, 10 Jul 2019 18:48:00 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints References: <7d8e776bc82cb9d25460c2104e687540526aa7cd.1559919361.git.korablev@tarantool.org> <9233795e-a77c-565a-9bd5-3712499e7fce@tarantool.org> <53B73D9B-740B-469F-99DC-FF2FA14E16BA@tarantool.org> <46ec2553-ae16-9a9f-8b85-882c2adc6031@tarantool.org> <7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 11 Jul 2019 00:49:21 +0200 MIME-Version: 1.0 In-Reply-To: <7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the fixes! On 05/07/2019 18:36, n.pettik wrote: > > >> On 2 Jul 2019, at 00:53, Vladislav Shpilevoy 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. >> I did this: @@ -4943,7 +4943,7 @@ case OP_OffsetLimit: { /* in1, out2, in3 */ uint64_t x = pIn1->u.u; uint64_t rhs = pIn3->u.u; bool unused; - if (x == 0 || sql_add_int(x, false, rhs, false, (int64_t *) &x, + if (x == 0 || sql_add_int(x, true, rhs, false, (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 And the tests passed. Looks like a test was not added.