Re: [Tarantool-patches] [PATCH 1/2] sql: fix build with GCC 10

[Artem <artemreyt@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).


Here's commit diff on github:

https://github.com/tarantool/tarantool/commit/d452cfb5be5512450ff12f416c28419c25552dd7

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