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 8492B6EC63; Fri, 9 Apr 2021 23:26:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8492B6EC63 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618000014; bh=nPMR58w0he5CFbPBcLSkGdpV24j5tRHsNeuno5G2dbI=; h=To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=QG34/DkOHZ6Ljpwyp+hlORrlAr3GYJ9JR+CAQdvFeLcRlTbk8VrY02o1UmKHvqnnz C48QxUc4ukta5r7tmzrbWE8sbQXgUBk1QK142qyGkUWsHS5RDlib3tPAUje3WsEpCD dvMAjcJbTfHdk4ci5pXHsoYGFJQUB6GW3IcmeTtc= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 D725A6C7D1 for ; Fri, 9 Apr 2021 23:25:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D725A6C7D1 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lUxhU-000566-0u; Fri, 09 Apr 2021 23:25:56 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 23:25:55 +0300 Message-Id: <670c9835e020f49a67ee8ade81fbf59dd0062beb.1617984948.git.imeevma@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E748094FADAEB10E66ADA4C48BE3C291E66DA182A05F5380850401665F948D965ABED51E6F93F353B35E641C245A09493D3CDBFCC9D277DA8D332 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE766DBE83FD69AB645EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D636C6B786B38B6A8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B23D16406E76B1511C79304A93BA50B14F3999716826408816D2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE78C592797616C97AB9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735E4A630A5B664A4FFC4224003CC83647689D4C264860C145E X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4FAB01D746736EDE8CEF82190F3E1D9BBC9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EE19B6E2433CA093E7C5EBB26CF551246C5C58D85F969C78636C4C038EC76A6B4DF324C33BE246F51D7E09C32AA3244C3680DEF6252B59A1E6671F684ED87CC38A6D4CC6FBFAC251FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4Qti+To2evnjw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638226C0A460C7D5DA85171A7D40D52B5711083D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [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: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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);