[Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10
Nikita Pettik
korablev at tarantool.org
Wed Nov 25 14:30:22 MSK 2020
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
> > >
More information about the Tarantool-patches
mailing list