[Tarantool-patches] [PATCH 0/2] GCC 10 warnings

[Tarantool-patches] [PATCH 0/2] GCC 10 warnings

[Artem Starshov]

Issue: https://github.com/tarantool/tarantool/issues/4966
Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-4966-gcc10-warns

Artem Starshov (2):
  sql: fix build with GCC 10
  bitset: fix GCC 10 build

 cmake/compiler.cmake | 14 ++++++++++++
 src/box/sql/select.c | 54 +++++++++++++++++++++++---------------------
 2 files changed, 42 insertions(+), 26 deletions(-)

-- 
2.28.0

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

[Artem Starshov]

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 */
 	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;
 #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

[Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build

[Artem Starshov]

GCC 10 reports: "warning: writing 1 byte into
a region of size 0 [-Wstringop-overflow=]"

It's false positive. In order to fix build
i tried to add ignoring gcc -Wstringop-overflow via
preprocessor directive `pragma GCC ignored`.
But it doesn't work, so added -Wnostringop-overflow
flag in complier.cmake for gcc version equal or greather 10.


Explaining of false positive:
The problem is:
In file included from /root/ttar/src/lib/bitset/bitset.h:45,
                 from /root/ttar/src/lib/bitset/bitset.c:32:
In function ‘bit_set’,
    inlined from ‘tt_bitset_set’ at /root/ttar/src/lib/bitset/bitset.c:112:14:
/root/ttar/src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  230 |  ldata[chunk] |= (1UL << offset);
      |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

I made a research and found out that if to explicitly init bitset->realloc = realloc
than a warning is still triggers, but if not to call rb_tree functions (tt_bitset_pages_search and
tt_bitset_pages_insert) it's ok. But these functions don't allocate or free a memory, so they can't
influence on memory region which `page` variable points to.

Part-of #4966
---
 cmake/compiler.cmake | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index db2ae6227..5db636812 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -430,3 +430,17 @@ else()
     set(CMAKE_HOST_C_COMPILER ${CMAKE_C_COMPILER})
     set(CMAKE_HOST_CXX_COMPILER ${CMAKE_CXX_COMPILER})
 endif()
+
+if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10)
+# In file included from src/lib/bitset/bitset.h:45,
+#                  from src/lib/bitset/bitset.c:32:
+# In function ‘bit_set’,
+#     inlined from ‘tt_bitset_set’ at src/lib/bitset/bitset.c:111:14:
+# src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
+#   230 |  ldata[chunk] |= (1UL << offset);
+#       |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
+#
+# false positive for gcc version 10
+# macro 'GCC diagnostic ignored "-Wstringop-overflow"' doesn't help
+    add_compile_flags("C" "-Wno-stringop-overflow")
+endif()
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build

[Aleksandr Lyapunov]

Hi! Thanks for the patch!
I have the following thoughts:
1. It's not obvious for me that if a function does not allocate of free 
memory
then it can't have influence on memory access. For example, deletion 
from tree
can break tree structure and returned pages are not correct page anymore.
I think we should dig more.
2. I'm sure we should not switch off the check for entire project 
because of one file.
At least we should use smth like set_source_file_properties

On 11/20/20 5:23 AM, Artem Starshov wrote:
> GCC 10 reports: "warning: writing 1 byte into
> a region of size 0 [-Wstringop-overflow=]"
>
> It's false positive. In order to fix build
> i tried to add ignoring gcc -Wstringop-overflow via
> preprocessor directive `pragma GCC ignored`.
> But it doesn't work, so added -Wnostringop-overflow
> flag in complier.cmake for gcc version equal or greather 10.
>
>
> Explaining of false positive:
> The problem is:
> In file included from /root/ttar/src/lib/bitset/bitset.h:45,
>                   from /root/ttar/src/lib/bitset/bitset.c:32:
> In function ‘bit_set’,
>      inlined from ‘tt_bitset_set’ at /root/ttar/src/lib/bitset/bitset.c:112:14:
> /root/ttar/src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>    230 |  ldata[chunk] |= (1UL << offset);
>        |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>
> I made a research and found out that if to explicitly init bitset->realloc = realloc
> than a warning is still triggers, but if not to call rb_tree functions (tt_bitset_pages_search and
> tt_bitset_pages_insert) it's ok. But these functions don't allocate or free a memory, so they can't
> influence on memory region which `page` variable points to.
>
> Part-of #4966
> ---
>   cmake/compiler.cmake | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
> index db2ae6227..5db636812 100644
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -430,3 +430,17 @@ else()
>       set(CMAKE_HOST_C_COMPILER ${CMAKE_C_COMPILER})
>       set(CMAKE_HOST_CXX_COMPILER ${CMAKE_CXX_COMPILER})
>   endif()
> +
> +if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10)
> +# In file included from src/lib/bitset/bitset.h:45,
> +#                  from src/lib/bitset/bitset.c:32:
> +# In function ‘bit_set’,
> +#     inlined from ‘tt_bitset_set’ at src/lib/bitset/bitset.c:111:14:
> +# src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
> +#   230 |  ldata[chunk] |= (1UL << offset);
> +#       |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
> +#
> +# false positive for gcc version 10
> +# macro 'GCC diagnostic ignored "-Wstringop-overflow"' doesn't help
> +    add_compile_flags("C" "-Wno-stringop-overflow")
> +endif()

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

[Artem]

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

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

[Artem]

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


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

Re: [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build

[Artem]

[-- Attachment #1: Type: text/plain, Size: 4582 bytes --]

Hi, thanks for review!

See my 2 comments below.


Branch: 
https://github.com/tarantool/tarantool/tree/artemreyt/gh-4966-gcc10-warns

Issue: https://github.com/tarantool/tarantool/issues/4966

21.11.2020 14:29, Aleksandr Lyapunov пишет:
> Hi! Thanks for the patch!
> I have the following thoughts:
> 1. It's not obvious for me that if a function does not allocate of 
> free memory
> then it can't have influence on memory access. For example, deletion 
> from tree
> can break tree structure and returned pages are not correct page anymore.
> I think we should dig more.

1. They may be not correct pages in a tree, but in this function 
(tt_bitset_set) it doesn't matter

for displayed warning.

The most important thing - is it enough memory was allocated for 
returned page AFTER

(uint8_t *) page + sizeof(tt_bitset_page). I will add a definition of 
tt_bitset_page for visibility:

struct tt_bitset_page {
    size_t first_pos; rb_node(struct tt_bitset_page)node; size_t cardinality; uint8_t data[0]; };

This is not trivial problem. I made a new ticket for this:

https://github.com/tarantool/tarantool/issues/5564

> 2. I'm sure we should not switch off the check for entire project 
> because of one file.
> At least we should use smth like set_source_file_properties
>
2. I agree with that, so i switched off the check only for bitset.c 
referred to the created ticket:

diff --git a/src/lib/bitset/CMakeLists.txt b/src/lib/bitset/CMakeLists.txt
index 1339a02ef..0aba49a47 100644
--- a/src/lib/bitset/CMakeLists.txt
+++ b/src/lib/bitset/CMakeLists.txt
@@ -6,6 +6,11 @@ set(lib_sources
      index.c
  )

+if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION 
VERSION_GREATER_EQUAL 10)
+    # gh-5564: bitset: writing 1 byte into a region of size 0 (gcc 10)

+ set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/bitset.c
+            PROPERTIES COMPILE_OPTIONS -Wno-stringop-overflow)
+endif()
  set_source_files_compile_flags(${lib_sources})
  add_library(bitset STATIC ${lib_sources})
  target_link_libraries(bitset bit)

> On 11/20/20 5:23 AM, Artem Starshov wrote:
>> GCC 10 reports: "warning: writing 1 byte into
>> a region of size 0 [-Wstringop-overflow=]"
>>
>> It's false positive. In order to fix build
>> i tried to add ignoring gcc -Wstringop-overflow via
>> preprocessor directive `pragma GCC ignored`.
>> But it doesn't work, so added -Wnostringop-overflow
>> flag in complier.cmake for gcc version equal or greather 10.
>>
>>
>> Explaining of false positive:
>> The problem is:
>> In file included from /root/ttar/src/lib/bitset/bitset.h:45,
>>                   from /root/ttar/src/lib/bitset/bitset.c:32:
>> In function ‘bit_set’,
>>      inlined from ‘tt_bitset_set’ at 
>> /root/ttar/src/lib/bitset/bitset.c:112:14:
>> /root/ttar/src/lib/bit/bit.h:230:15: warning: writing 1 byte into a 
>> region of size 0 [-Wstringop-overflow=]
>>    230 |  ldata[chunk] |= (1UL << offset);
>>        |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>>
>> I made a research and found out that if to explicitly init 
>> bitset->realloc = realloc
>> than a warning is still triggers, but if not to call rb_tree 
>> functions (tt_bitset_pages_search and
>> tt_bitset_pages_insert) it's ok. But these functions don't allocate 
>> or free a memory, so they can't
>> influence on memory region which `page` variable points to.
>>
>> Part-of #4966
>> ---
>>   cmake/compiler.cmake | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
>> index db2ae6227..5db636812 100644
>> --- a/cmake/compiler.cmake
>> +++ b/cmake/compiler.cmake
>> @@ -430,3 +430,17 @@ else()
>>       set(CMAKE_HOST_C_COMPILER ${CMAKE_C_COMPILER})
>>       set(CMAKE_HOST_CXX_COMPILER ${CMAKE_CXX_COMPILER})
>>   endif()
>> +
>> +if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION 
>> VERSION_GREATER_EQUAL 10)
>> +# In file included from src/lib/bitset/bitset.h:45,
>> +#                  from src/lib/bitset/bitset.c:32:
>> +# In function ‘bit_set’,
>> +#     inlined from ‘tt_bitset_set’ at src/lib/bitset/bitset.c:111:14:
>> +# src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of 
>> size 0 [-Wstringop-overflow=]
>> +#   230 |  ldata[chunk] |= (1UL << offset);
>> +#       |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>> +#
>> +# false positive for gcc version 10
>> +# macro 'GCC diagnostic ignored "-Wstringop-overflow"' doesn't help
>> +    add_compile_flags("C" "-Wno-stringop-overflow")
>> +endif()

[-- Attachment #2: Type: text/html, Size: 8139 bytes --]

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

[Nikita Pettik]

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