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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

On 09.12.2020 12:04, Artem wrote:
> Hi, thanks for your letter!
> 
> Answering your question: we can't just return NULL, because this 
> function, oddly enough,
> 
> is responsible for clearing of its arguments in case of allocation 
> failure. In order to clean
> 
> them correctly we need to fill `Select` structure and invoke 
> `clearSelect` for it.
> 

Yep. You are right!
LGTM.

> 09.12.2020 11:43, Leonid Vasiliev пишет:
>> Hi! Thank you for the patch.
>> Looks correctly, but I have one question:
>>
>> On 27.11.2020 15:43, Artem Starshov via Tarantool-patches wrote:
>>> GCC 10 produces the following error:
>>> cc1: warning: function may return address of local variable 
>>> [-Wreturn-local-addr]
>>>
>>> Fix it.
>>>
>>> Part-of #4966
>>> ---
>>> Got LGTM from Nikita Pettik:
>>> https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020940.html 
>>>
>>>
>>>   src/box/sql/select.c | 56 ++++++++++++++++++++------------------------
>>>   1 file changed, 26 insertions(+), 30 deletions(-)
>>>
>>> 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) {
>>
>> 1) Why can't we return NULL if malloc has failed and don't work with
>> standin at all?
>>
>>> -        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;
>>>   }
>>>