From: Artem <artemreyt@tarantool.org>
To: Leonid Vasiliev <lvasiliev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 1/2] sql: fix build with GCC 10
Date: Wed, 9 Dec 2020 12:04:47 +0300 [thread overview]
Message-ID: <915844be-a960-d7af-3eed-acc0ea8f9872@tarantool.org> (raw)
In-Reply-To: <c8454129-2a89-b172-e64f-cbb36d04b980@tarantool.org>
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.
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;
>> }
>>
next prev parent reply other threads:[~2020-12-09 9:04 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-27 12:43 [Tarantool-patches] [PATCH v2 0/2] GCC 10 fix warnings Artem Starshov
2020-11-27 12:43 ` [Tarantool-patches] [PATCH v2 1/2] sql: fix build with GCC 10 Artem Starshov
2020-12-09 8:43 ` Leonid Vasiliev
2020-12-09 9:04 ` Artem [this message]
2020-12-09 10:07 ` Leonid Vasiliev
2020-11-27 12:43 ` [Tarantool-patches] [PATCH v2 2/2] bitset: replace zero-length array with flexible-array member Artem Starshov
2020-11-30 12:47 ` Aleksandr Lyapunov
2020-12-09 8:11 ` Leonid Vasiliev
2020-12-01 14:16 ` [Tarantool-patches] [PATCH v2 0/2] GCC 10 fix warnings Alexander V. Tikhonov
2020-12-02 0:27 ` Alexander V. Tikhonov
2020-12-16 11:47 ` Kirill Yukhin
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=915844be-a960-d7af-3eed-acc0ea8f9872@tarantool.org \
--to=artemreyt@tarantool.org \
--cc=lvasiliev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v2 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