Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: Artem <artemreyt@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Alexander Turenko <alexander.turenko@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10
Date: Wed, 25 Nov 2020 11:30:22 +0000	[thread overview]
Message-ID: <20201125113021.GB23894@tarantool.org> (raw)
In-Reply-To: <4ee13a26-9079-9a3e-9bb9-68854fef6c43@tarantool.org>

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
> > > 

  reply	other threads:[~2020-11-25 11:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-20  2:23 [Tarantool-patches] [PATCH 0/2] GCC 10 warnings Artem Starshov
2020-11-20  2:23 ` [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10 Artem Starshov
     [not found]   ` <20201123231440.GA17397@tarantool.org>
2020-11-25  9:25     ` Artem
2020-11-25  9:52     ` Artem
2020-11-25 11:30       ` Nikita Pettik [this message]
2020-11-20  2:23 ` [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build Artem Starshov
2020-11-21 11:29   ` Aleksandr Lyapunov
2020-11-25 11:29     ` Artem

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201125113021.GB23894@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=artemreyt@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox