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 0A59224F0D for ; Fri, 5 Jul 2019 12:36:05 -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 250ggBcyV6P7 for ; Fri, 5 Jul 2019 12:36:04 -0400 (EDT) Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 B7B0024EED for ; Fri, 5 Jul 2019 12:36:04 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints From: "n.pettik" In-Reply-To: <46ec2553-ae16-9a9f-8b85-882c2adc6031@tarantool.org> Date: Fri, 5 Jul 2019 19:36:02 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7AA862F1-50A9-40E4-A054-D42836D125A5@tarantool.org> 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> 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 > On 2 Jul 2019, at 00:53, Vladislav Shpilevoy = wrote: >=20 > Thanks for the fixes! >=20 >>>> @@ -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 =3D pIn1->u.i; >>>> - if (x<=3D0 || sqlAddInt64(&x, pIn3->u.i > 0 ? pIn3->u.i : 0)) { >>>> + int64_t rhs =3D pIn3->flags & MEM_Int ? 0 : pIn3->u.u; >>>> + bool unused; >>>> + if ((x =3D=3D 0 || pIn1->flags & MEM_Int) || >>>> + sql_add_int(x, pIn1->flags & MEM_Int, rhs, false, >>>=20 >>> 14. If you get to this line, then (pIn1->flags & MEM_Int) is already >>> 0 and can be inlined. >>=20 >> Wait, why? If x =3D=3D 0 then pIn1->flags =3D=3D MEM_UInt - >> we consider 0 as an unsigned value. >=20 > Because you can only get to sql_add_int(), if x !=3D 0 and > pIn1->flags & MEM_Int =3D=3D 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 >=3D 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 =3D &aMem[pOp->p1]; pIn3 =3D &aMem[pOp->p3]; pOut =3D out2Prerelease(p, pOp); - assert((pIn1->flags & (MEM_Int | MEM_UInt)) !=3D 0); - assert((pIn3->flags & (MEM_Int | MEM_UInt)) !=3D 0); - x =3D pIn1->u.i; - int64_t rhs =3D pIn3->flags & MEM_Int ? 0 : pIn3->u.u; + assert((pIn1->flags & MEM_UInt) !=3D 0); + assert((pIn3->flags & MEM_UInt) !=3D 0); + uint64_t x =3D pIn1->u.u; + uint64_t rhs =3D pIn3->u.u; bool unused; - if ((x =3D=3D 0 || pIn1->flags & MEM_Int) || - sql_add_int(x, pIn1->flags & MEM_Int, rhs, false, - (int64_t *) &x, &unused) !=3D 0) { + if (x =3D=3D 0 || sql_add_int(x, false, rhs, false, (int64_t *) = &x, + &unused) !=3D 0) { > Please, inline the value (false), and add a test, which would fail, if > I put here true. >=20 >> if (is_lhs_neg) { >> uint64_t u_lhs =3D (uint64_t) (-lhs); >> uint64_t u_rhs =3D is_rhs_neg ? (uint64_t) (-rhs) : >> (uint64_t) rhs; >> uint64_t u_res =3D u_lhs % u_rhs; >> if (u_res > (uint64_t) INT64_MAX + 1) >> return -1; >=20 > 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 =3D (uint64_t) (-lhs); uint64_t u_res =3D u_lhs % u_rhs; - if (u_res > (uint64_t) INT64_MAX + 1) - return -1; *res =3D -u_res; *is_res_neg =3D true; return 0; > Consider new fixes below, and on the branch > in a separate commit. Applied.