From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 30E3145C304 for ; Wed, 9 Dec 2020 13:08:03 +0300 (MSK) References: <915844be-a960-d7af-3eed-acc0ea8f9872@tarantool.org> From: Leonid Vasiliev Message-ID: <2ec2d319-aec0-e28c-54b2-d1522ae07eaf@tarantool.org> Date: Wed, 9 Dec 2020 13:07:06 +0300 MIME-Version: 1.0 In-Reply-To: <915844be-a960-d7af-3eed-acc0ea8f9872@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 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 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; >>>   } >>>