From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 03E76469710 for ; Wed, 25 Nov 2020 14:30:22 +0300 (MSK) Date: Wed, 25 Nov 2020 11:30:22 +0000 From: Nikita Pettik Message-ID: <20201125113021.GB23894@tarantool.org> References: <3229ff8c69e84bd85798c79d382d35ba52640ce4.1605828734.git.artemreyt@tarantool.org> <20201123231440.GA17397@tarantool.org> <4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org> 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: Artem Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko On 25 Nov 12:52, Artem wrote: > 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). As you wish. Anyway, LGTM > 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 > > >