From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 9E0B9469710 for ; Wed, 25 Nov 2020 12:52:24 +0300 (MSK) References: <3229ff8c69e84bd85798c79d382d35ba52640ce4.1605828734.git.artemreyt@tarantool.org> <20201123231440.GA17397@tarantool.org> From: Artem Message-ID: <4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org> Date: Wed, 25 Nov 2020 12:52:22 +0300 MIME-Version: 1.0 In-Reply-To: <20201123231440.GA17397@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Nikita, thanks for review! I considered your proposal to use stack `struct Select` variable  and make `memcpy` after filling that varible. It really looks better. I only changed naming (because, pNew means pointer and i had to sacrifice diff for better readability). diff --git a/src/box/sql/select.c b/src/box/sql/select.c index b0554a172..5d4b2f624 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -154,56 +154,52 @@ sqlSelectNew(Parse * pParse,      /* Parsing context */                  Expr * pLimit,         /* LIMIT value.  NULL means not used */                  Expr * pOffset)        /* OFFSET value.  NULL means no offset */  { -       Select *pNew;         Select standin;         sql *db = pParse->db; -       pNew = sqlDbMallocRawNN(db, sizeof(*pNew)); -       if (pNew == 0) { -               assert(db->mallocFailed); -               pNew = &standin; -       }         if (pEList == 0) {                 struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);                 if (expr == NULL)                         pParse->is_aborted = true;                 pEList = sql_expr_list_append(db, NULL, expr);         } -       pNew->pEList = pEList; -       pNew->op = TK_SELECT; -       pNew->selFlags = selFlags; -       pNew->iLimit = 0; -       pNew->iOffset = 0; +       standin.pEList = pEList; +       standin.op = TK_SELECT; +       standin.selFlags = selFlags; +       standin.iLimit = 0; +       standin.iOffset = 0;  #ifdef SQL_DEBUG -       pNew->zSelName[0] = 0; +       standin.zSelName[0] = 0;         if ((pParse->sql_flags & SQL_SelectTrace) != 0)                 sqlSelectTrace = 0xfff;         else                 sqlSelectTrace = 0;  #endif -       pNew->addrOpenEphm[0] = -1; -       pNew->addrOpenEphm[1] = -1; -       pNew->nSelectRow = 0; +       standin.addrOpenEphm[0] = -1; +       standin.addrOpenEphm[1] = -1; +       standin.nSelectRow = 0;         if (pSrc == 0)                 pSrc = sqlDbMallocZero(db, sizeof(*pSrc)); -       pNew->pSrc = pSrc; -       pNew->pWhere = pWhere; -       pNew->pGroupBy = pGroupBy; -       pNew->pHaving = pHaving; -       pNew->pOrderBy = pOrderBy; -       pNew->pPrior = 0; -       pNew->pNext = 0; -       pNew->pLimit = pLimit; -       pNew->pOffset = pOffset; -       pNew->pWith = 0; +       standin.pSrc = pSrc; +       standin.pWhere = pWhere; +       standin.pGroupBy = pGroupBy; +       standin.pHaving = pHaving; +       standin.pOrderBy = pOrderBy; +       standin.pPrior = 0; +       standin.pNext = 0; +       standin.pLimit = pLimit; +       standin.pOffset = pOffset; +       standin.pWith = 0;         assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted                || db->mallocFailed != 0); +       Select *pNew = sqlDbMallocRawNN(db, sizeof(*pNew));         if (db->mallocFailed) { -               clearSelect(db, pNew, pNew != &standin); -               pNew = 0; -       } else { -               assert(pNew->pSrc != 0 || pParse->is_aborted); +               clearSelect(db, &standin, 0); +               if (pNew != NULL) +                       sqlDbFree(db, pNew); +               return NULL;         } -       assert(pNew != &standin); +       assert(standin.pSrc != 0 || pParse->is_aborted); +       memcpy(pNew, &standin, sizeof(standin));         return pNew; } 24.11.2020 02:14, Nikita Pettik пишет: > On 20 Nov 05:04, Artem Starshov wrote: >> GCC 10 produces the following error: >> cc1: warning: function may return address of local variable [-Wreturn-local-addr] >> >> Fix it. >> >> Part-of #4966 >> --- >> src/box/sql/select.c | 54 +++++++++++++++++++++++--------------------- >> 1 file changed, 28 insertions(+), 26 deletions(-) >> >> diff --git a/src/box/sql/select.c b/src/box/sql/select.c >> index b0554a172..921fab7fc 100644 >> --- a/src/box/sql/select.c >> +++ b/src/box/sql/select.c >> @@ -154,13 +154,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ >> Expr * pLimit, /* LIMIT value. NULL means not used */ >> Expr * pOffset) /* OFFSET value. NULL means no offset */ >> { >> - Select *pNew; >> - Select standin; >> + Select *pNew; /* Pointer to allocated region by sqlDbMallocRawNN (0 if failed) */ >> + Select *pNewTmp; /* Pointer to work with */ >> + Select standin; /* If allocation failed, save it to pNewTmp for filling and futher cleaning */ > Unfortunatelly, part of SQL codebase is bad formatted. We strive to > follow original Tarantool codestyle when adding new code chunks. > So according to comments should not be placed on the lines containing code. > > /* comments */ > int x = 3; > /*comments */ > if (x == 3) { > ... > >> sql *db = pParse->db; >> pNew = sqlDbMallocRawNN(db, sizeof(*pNew)); >> - if (pNew == 0) { >> + pNewTmp = pNew; >> + if (pNewTmp == 0) { >> assert(db->mallocFailed); >> - pNew = &standin; >> + pNewTmp = &standin; >> } >> if (pEList == 0) { >> struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK); >> @@ -168,11 +170,11 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ >> pParse->is_aborted = true; >> pEList = sql_expr_list_append(db, NULL, expr); >> } >> - pNew->pEList = pEList; >> - pNew->op = TK_SELECT; >> - pNew->selFlags = selFlags; >> - pNew->iLimit = 0; >> - pNew->iOffset = 0; >> + pNewTmp->pEList = pEList; >> + pNewTmp->op = TK_SELECT; >> + pNewTmp->selFlags = selFlags; >> + pNewTmp->iLimit = 0; >> + pNewTmp->iOffset = 0; > Hm, why did you rename pNew -> pNewTmp instead of leaving pNew as is > and introduce new pointer which is returned from func? Sort of: > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 4b069addb..f4806a806 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -154,14 +154,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ > Expr * pLimit, /* LIMIT value. NULL means not used */ > Expr * pOffset) /* OFFSET value. NULL means no offset */ > { > - Select *pNew; > + Select *pNew, pNewTmp; > Select standin; > sql *db = pParse->db; > - pNew = sqlDbMallocRawNN(db, sizeof(*pNew)); > - if (pNew == 0) { > + pNewTmp = sqlDbMallocRawNN(db, sizeof(*pNew)); > + if (pNewTmp == 0) { > assert(db->mallocFailed); > - pNew = &standin; > + pNewTmp = &standin; > } > + pNew = pNewTmp; > if (pEList == 0) { > struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK); > if (expr == NULL) > > I mean it would be nice to avoid unnecessary diff. > > Alternatively, consider this refactoring: > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 4b069addb..5c829c932 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -154,14 +154,8 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ > Expr * pLimit, /* LIMIT value. NULL means not used */ > Expr * pOffset) /* OFFSET value. NULL means no offset */ > { > - Select *pNew; > - Select standin; > + Select pNew; > sql *db = pParse->db; > - pNew = sqlDbMallocRawNN(db, sizeof(*pNew)); > - if (pNew == 0) { > - assert(db->mallocFailed); > - pNew = &standin; > - } > if (pEList == 0) { > struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK); > if (expr == NULL) > @@ -197,13 +191,15 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ > pNew->pWith = 0; > assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted > || db->mallocFailed != 0); > + Select *select = sqlDbMallocRawNN(db, sizeof(*pNew)); > if (db->mallocFailed) { > - clearSelect(db, pNew, pNew != &standin); > - pNew = 0; > - } else { > - assert(pNew->pSrc != 0 || pParse->is_aborted); > + clearSelect(db, &pNew, 0); > + if (select != NULL) > + sqlDbFree(db, select); > + return NULL; > } > - assert(pNew != &standin); > + assert(pNew->pSrc != 0 || pParse->is_aborted); > + memcpy(select, &pNew, sizeof(pNew)); > return pNew; > } > > Here we fill in struct Select allocated on stack and in case of > sqlDbMallocZero/sqlDbMallocRawNN fail we simply don't copy it > on the structure allocated on heap. IMHO it looks better and will > work even if the next GCC version will contain more advanced pointer > aliasing machinery. > >> #ifdef SQL_DEBUG >> pNew->zSelName[0] = 0; >> if ((pParse->sql_flags & SQL_SelectTrace) != 0) >> @@ -180,30 +182,30 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ >> else >> sqlSelectTrace = 0; >> #endif >> - pNew->addrOpenEphm[0] = -1; >> - pNew->addrOpenEphm[1] = -1; >> - pNew->nSelectRow = 0; >> + pNewTmp->addrOpenEphm[0] = -1; >> + pNewTmp->addrOpenEphm[1] = -1; >> + pNewTmp->nSelectRow = 0; >> if (pSrc == 0) >> pSrc = sqlDbMallocZero(db, sizeof(*pSrc)); >> - pNew->pSrc = pSrc; >> - pNew->pWhere = pWhere; >> - pNew->pGroupBy = pGroupBy; >> - pNew->pHaving = pHaving; >> - pNew->pOrderBy = pOrderBy; >> - pNew->pPrior = 0; >> - pNew->pNext = 0; >> - pNew->pLimit = pLimit; >> - pNew->pOffset = pOffset; >> - pNew->pWith = 0; >> + pNewTmp->pSrc = pSrc; >> + pNewTmp->pWhere = pWhere; >> + pNewTmp->pGroupBy = pGroupBy; >> + pNewTmp->pHaving = pHaving; >> + pNewTmp->pOrderBy = pOrderBy; >> + pNewTmp->pPrior = 0; >> + pNewTmp->pNext = 0; >> + pNewTmp->pLimit = pLimit; >> + pNewTmp->pOffset = pOffset; >> + pNewTmp->pWith = 0; >> assert(pOffset == 0 || pLimit != 0 || pParse->is_aborted >> || db->mallocFailed != 0); >> if (db->mallocFailed) { >> - clearSelect(db, pNew, pNew != &standin); >> - pNew = 0; >> + clearSelect(db, pNewTmp, pNewTmp != &standin); >> + pNewTmp = 0; >> } else { >> - assert(pNew->pSrc != 0 || pParse->is_aborted); >> + assert(pNewTmp->pSrc != 0 || pParse->is_aborted); >> } >> - assert(pNew != &standin); >> + assert(pNewTmp != &standin); >> return pNew; >> } >> >> -- >> 2.28.0 >>