From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B599B6EC5F; Tue, 13 Apr 2021 23:42:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B599B6EC5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618346524; bh=pXVMX3hPxG45/VNir+/RaP2dvAdL64I5zqNnrUwAifU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=GO1tmLJkknm31yuxXgNly74pCov6/57ELGoexZ2hH4L60LnqhOX+OM7dbZGt4NpD5 2YUQpKY6PzTUAVgapoShC0Ezqu1bFEu5jvQ8aSund1yMVqNciNNvg1UVaG23Ijt1Rd ojq3ofOIzn2W26ewWJZwxbm7+xZQ9ow6EWynunwE= Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 484366EC5F for ; Tue, 13 Apr 2021 23:42:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 484366EC5F Received: by smtp17.mail.ru with esmtpa (envelope-from ) id 1lWPrF-0001ps-A5; Tue, 13 Apr 2021 23:42:01 +0300 Date: Tue, 13 Apr 2021 23:42:00 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org, tarantool-patches@dev.tarantool.org Message-ID: <20210413204200.GA21092@tarantool.org> References: <670c9835e020f49a67ee8ade81fbf59dd0062beb.1617984948.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <670c9835e020f49a67ee8ade81fbf59dd0062beb.1617984948.git.imeevma@gmail.com> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480B1C8842CE613979723F2FB4628545A35182A05F5380850409D1331F5B735B6FD1421F32AF2C1AFEFE6F8B774C308F9E010D59DE722B10A46 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73C696014E2DCCA1EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375E280A1EC162AD7D8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B25EF2AA7AE07D62C5C96F31CA5DB550B0F04B652EEC242312D2E47CDBA5A96583C09775C1D3CA48CF17B107DEF921CE79117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7ABB305BD10C6E5099FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7F841E3AC961D6469D32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C04E9B58D50EA91E7ACD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3727597FF642BA4D735872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D2538DECB10C4E1BE3BCB8CCF60F6B7FD3948E1CD14B953EB46D6FC5A650FFE185ED355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A517E98E0A24B2BB3B570A62048C8B2576BD1258245F87B639D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D343536C6A4332D8B8A02A72FE1E8204E871EB61E735C5E7C3EC553C9CAE5167468F76584A22F2975F31D7E09C32AA3244CF0104B0355505D20C298010C20FCBA1F05AB220A9D022EBCFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXEbzBYizdguuQ== X-Mailru-Sender: 5C3750E245F362008BC1685FEC6306ED70BD22D22A0282A21421F32AF2C1AFEF60497F7EF99AAC8F5105BD0848736F9966FEC6BF5C9C28D97E07721503EA2E00ED97202A5A4E92BF7402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 38/52] sql: move MEM flags to mem.c X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On new branch I dropped this commit. Please see my answer to "sql: introduce mem_is_*() functions()". On Fri, Apr 09, 2021 at 11:25:55PM +0300, Mergen Imeev via Tarantool-patches wrote: > Thank you for the review! My answers and new patch below. > > > On 30.03.2021 02:07, Vladislav Shpilevoy wrote: > > Thanks for the patch! > > > >> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > >> index 70224b55a..8b6f6749d 100644 > >> --- a/src/box/sql/mem.h > >> +++ b/src/box/sql/mem.h > >> @@ -563,13 +511,6 @@ columnNullValue(void); > >> > >> int sqlVdbeMemTooBig(Mem *); > >> > >> -/* Return TRUE if Mem X contains dynamically allocated content - anything > >> - * that needs to be deallocated to avoid a leak. > >> - */ > >> -#define VdbeMemDynamic(X) \ > >> - (((X)->flags&(MEM_Agg|MEM_Dyn|MEM_Frame))!=0) > > > > Why did you remove that? And why don't you have MEM_Agg|MEM_Frame in > > mem_is_dynamic()? > Function mem_is_dynamic() shows allocation type of MEM contains STRING or > VARBINARY. Macro VdbeMemDynamic() shows that MEM should be cleared before > changing. This macro is not needed now outside of mem.c since all settings there > should be done using mem_set_*() functions, which clear MEM before changing. > In mem.c this macro may be added in case we set MEM directly, however we inline > mem_set_*() only in cases when we know that there is no clearing needed. Not > sure that this macro is needed after all. > > There is one place where MEM is set without mem_set_*() outside of mem.c - in > function allocateCursor(). However, the way MEM is used here is so different > from normal way of using MEM, that I am not sure that VdbeMemDynamic() is > actually proper there. > > > New patch: > > commit 670c9835e020f49a67ee8ade81fbf59dd0062beb > Author: Mergen Imeev > Date: Wed Mar 17 10:19:57 2021 +0300 > > sql: move MEM flags to mem.c > > This patch moves MEM flags to mem.c. This allow us to have more control > over MEM state. > > Part of #5818 > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 2e147291f..52b1891aa 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -52,6 +52,57 @@ > static int > sqlVdbeMemGrow(struct Mem *pMem, int n, int preserve); > > +/* One or more of the following flags are set to indicate the validOK > + * representations of the value stored in the Mem struct. > + * > + * If the MEM_Null flag is set, then the value is an SQL NULL value. > + * No other flags may be set in this case. > + * > + * If the MEM_Str flag is set then Mem.z points at a string representation. > + * Usually this is encoded in the same unicode encoding as the main > + * database (see below for exceptions). If the MEM_Term flag is also > + * set, then the string is nul terminated. The MEM_Int and MEM_Real > + * flags may coexist with the MEM_Str flag. > + */ > +#define MEM_Null 0x0001 /* Value is NULL */ > +#define MEM_Str 0x0002 /* Value is a string */ > +#define MEM_Int 0x0004 /* Value is an integer */ > +#define MEM_Real 0x0008 /* Value is a real number */ > +#define MEM_Blob 0x0010 /* Value is a BLOB */ > +#define MEM_Bool 0x0020 /* Value is a bool */ > +#define MEM_UInt 0x0040 /* Value is an unsigned integer */ > +#define MEM_Frame 0x0080 /* Value is a VdbeFrame object */ > +#define MEM_Undefined 0x0100 /* Value is undefined */ > +#define MEM_Cleared 0x0200 /* NULL set by OP_Null, not from data */ > +#define MEM_TypeMask 0x83ff /* Mask of type bits */ > + > +/* Whenever Mem contains a valid string or blob representation, one of > + * the following flags must be set to determine the memory management > + * policy for Mem.z. The MEM_Term flag tells us whether or not the > + * string is \000 or \u0000 terminated > + */ > +#define MEM_Term 0x0400 /* String rep is nul terminated */ > +#define MEM_Dyn 0x0800 /* Need to call Mem.xDel() on Mem.z */ > +#define MEM_Static 0x1000 /* Mem.z points to a static string */ > +#define MEM_Ephem 0x2000 /* Mem.z points to an ephemeral string */ > +#define MEM_Agg 0x4000 /* Mem.z points to an agg function context */ > +#define MEM_Zero 0x8000 /* Mem.i contains count of 0s appended to blob */ > +#define MEM_Subtype 0x10000 /* Mem.eSubtype is valid */ > +#define MEM_Ptr 0x20000 /* Value is a generic pointer */ > + > +/** > + * In contrast to Mem_TypeMask, this one allows to get > + * pure type of memory cell, i.e. without _Dyn/_Zero and other > + * auxiliary flags. > + */ > +enum { > + MEM_PURE_TYPE_MASK = 0x7f > +}; > + > +static_assert(MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int | MEM_Real | > + MEM_Blob | MEM_Bool | MEM_UInt), > + "value of type mask must consist of corresponding to memory "\ > + "type bits"); > > bool > mem_is_null(const struct Mem *mem) > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index f17c4bb78..ce5076361 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -473,58 +473,6 @@ int > mem_compare(const struct Mem *left, const struct Mem *right, int *result, > enum field_type type, struct coll *coll); > > -/* One or more of the following flags are set to indicate the validOK > - * representations of the value stored in the Mem struct. > - * > - * If the MEM_Null flag is set, then the value is an SQL NULL value. > - * No other flags may be set in this case. > - * > - * If the MEM_Str flag is set then Mem.z points at a string representation. > - * Usually this is encoded in the same unicode encoding as the main > - * database (see below for exceptions). If the MEM_Term flag is also > - * set, then the string is nul terminated. The MEM_Int and MEM_Real > - * flags may coexist with the MEM_Str flag. > - */ > -#define MEM_Null 0x0001 /* Value is NULL */ > -#define MEM_Str 0x0002 /* Value is a string */ > -#define MEM_Int 0x0004 /* Value is an integer */ > -#define MEM_Real 0x0008 /* Value is a real number */ > -#define MEM_Blob 0x0010 /* Value is a BLOB */ > -#define MEM_Bool 0x0020 /* Value is a bool */ > -#define MEM_UInt 0x0040 /* Value is an unsigned integer */ > -#define MEM_Frame 0x0080 /* Value is a VdbeFrame object */ > -#define MEM_Undefined 0x0100 /* Value is undefined */ > -#define MEM_Cleared 0x0200 /* NULL set by OP_Null, not from data */ > -#define MEM_TypeMask 0x83ff /* Mask of type bits */ > - > -/* Whenever Mem contains a valid string or blob representation, one of > - * the following flags must be set to determine the memory management > - * policy for Mem.z. The MEM_Term flag tells us whether or not the > - * string is \000 or \u0000 terminated > - */ > -#define MEM_Term 0x0400 /* String rep is nul terminated */ > -#define MEM_Dyn 0x0800 /* Need to call Mem.xDel() on Mem.z */ > -#define MEM_Static 0x1000 /* Mem.z points to a static string */ > -#define MEM_Ephem 0x2000 /* Mem.z points to an ephemeral string */ > -#define MEM_Agg 0x4000 /* Mem.z points to an agg function context */ > -#define MEM_Zero 0x8000 /* Mem.i contains count of 0s appended to blob */ > -#define MEM_Subtype 0x10000 /* Mem.eSubtype is valid */ > -#define MEM_Ptr 0x20000 /* Value is a generic pointer */ > - > -/** > - * In contrast to Mem_TypeMask, this one allows to get > - * pure type of memory cell, i.e. without _Dyn/_Zero and other > - * auxiliary flags. > - */ > -enum { > - MEM_PURE_TYPE_MASK = 0x7f > -}; > - > -static_assert(MEM_PURE_TYPE_MASK == (MEM_Null | MEM_Str | MEM_Int | MEM_Real | > - MEM_Blob | MEM_Bool | MEM_UInt), > - "value of type mask must consist of corresponding to memory "\ > - "type bits"); > - > /** > * Simple type to str convertor. It is used to simplify > * error reporting. > @@ -555,7 +503,7 @@ registerTrace(int iReg, Mem *p); > * Return true if a memory cell is not marked as invalid. This macro > * is for use inside assert() statements only. > */ > -#define memIsValid(M) ((M)->flags & MEM_Undefined)==0 > +#define memIsValid(M) !mem_is_invalid(M) > #endif > > /** > @@ -589,7 +537,7 @@ int mem_apply_integer_type(struct Mem *); > int sqlVdbeMemStringify(struct Mem *); > int sqlVdbeMemNulTerminate(struct Mem *); > int sqlVdbeMemExpandBlob(struct Mem *); > -#define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0) > +#define ExpandBlob(P) (mem_is_zerobin(P)? sqlVdbeMemExpandBlob(P) : 0) > void sql_value_apply_type(struct Mem *val, enum field_type type); > > > @@ -700,13 +648,6 @@ columnNullValue(void); > > int sqlVdbeMemTooBig(Mem *); > > -/* Return TRUE if Mem X contains dynamically allocated content - anything > - * that needs to be deallocated to avoid a leak. > - */ > -#define VdbeMemDynamic(X) \ > - (((X)->flags&(MEM_Agg|MEM_Dyn|MEM_Frame))!=0) > - > - > int sqlMemCompare(const Mem *, const Mem *, const struct coll *); > > /** > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 4566606d7..71a827034 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -607,7 +607,6 @@ case OP_SetDiag: { /* jump */ > case OP_Gosub: { /* jump */ > assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor)); > pIn1 = &aMem[pOp->p1]; > - assert(VdbeMemDynamic(pIn1)==0); > memAboutToChange(p, pIn1); > mem_set_uint(pIn1, pOp - aOp); > REGISTER_TRACE(p, pOp->p1, pIn1); > @@ -649,7 +648,6 @@ case OP_InitCoroutine: { /* jump */ > assert(pOp->p2>=0 && pOp->p2nOp); > assert(pOp->p3>0 && pOp->p3nOp); > pOut = &aMem[pOp->p1]; > - assert(!VdbeMemDynamic(pOut)); > mem_set_uint(pOut, pOp->p3 - 1); > if (pOp->p2) goto jump_to_p2; > break; > @@ -691,7 +689,6 @@ case OP_EndCoroutine: { /* in1 */ > */ > case OP_Yield: { /* in1, jump */ > pIn1 = &aMem[pOp->p1]; > - assert(VdbeMemDynamic(pIn1)==0); > int pcDest = (int)pIn1->u.u; > mem_set_uint(pIn1, pOp - aOp); > REGISTER_TRACE(p, pOp->p1, pIn1);