SQLite Forum

warning: function may return address of local variable [-Wreturn-local-addr]
Login

warning: function may return address of local variable [-Wreturn-local-addr]

(1) By Adam S Levy (alaskanarcher) on 2020-05-25 00:29:18 [link]

I am getting the following warning when compiling the last two official release amalgamations using CGo:

```
$ go build
# crawshaw.io/sqlite
In file included from ./static.go:19:
././c/sqlite3.c: In function 'sqlite3SelectNew':
././c/sqlite3.c:128048:10: warning: function may return address of local variable [-Wreturn-local-addr]
128048 |   return pNew;
       |          ^~~~
././c/sqlite3.c:128008:10: note: declared here
128008 |   Select standin;
       |          ^~~~~~~
```

The relevant amalgamation code is below.

Clearly the standin variable is protected from being exposed using the assert at the bottom. But perhaps this should be made into a global to avoid the warning.

```
/*
** Allocate a new Select structure and return a pointer to that
** structure.
*/
SQLITE_PRIVATE Select *sqlite3SelectNew(
  Parse *pParse,        /* Parsing context */
  ExprList *pEList,     /* which columns to include in the result */
  SrcList *pSrc,        /* the FROM clause -- which tables to scan */
  Expr *pWhere,         /* the WHERE clause */
  ExprList *pGroupBy,   /* the GROUP BY clause */
  Expr *pHaving,        /* the HAVING clause */
  ExprList *pOrderBy,   /* the ORDER BY clause */
  u32 selFlags,         /* Flag parameters, such as SF_Distinct */
  Expr *pLimit          /* LIMIT value.  NULL means not used */
){
  Select *pNew;
  Select standin;
  pNew = sqlite3DbMallocRawNN(pParse->db, sizeof(*pNew) );
  if( pNew==0 ){
    assert( pParse->db->mallocFailed );
    pNew = &standin;
  }
  if( pEList==0 ){
    pEList = sqlite3ExprListAppend(pParse, 0,
                                   sqlite3Expr(pParse->db,TK_ASTERISK,0));
  }
  pNew->pEList = pEList;
  pNew->op = TK_SELECT;
  pNew->selFlags = selFlags;
  pNew->iLimit = 0;
  pNew->iOffset = 0;
  pNew->selId = ++pParse->nSelect;
  pNew->addrOpenEphm[0] = -1;
  pNew->addrOpenEphm[1] = -1;
  pNew->nSelectRow = 0;
  if( pSrc==0 ) pSrc = sqlite3DbMallocZero(pParse->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->pWith = 0;
#ifndef SQLITE_OMIT_WINDOWFUNC
  pNew->pWin = 0;
  pNew->pWinDefn = 0;
#endif
  if( pParse->db->mallocFailed ) {
    clearSelect(pParse->db, pNew, pNew!=&standin);
    pNew = 0;
  }else{
    assert( pNew->pSrc!=0 || pParse->nErr>0 );
  }
  assert( pNew!=&standin );
  return pNew;
}
```

(2) By Richard Hipp (drh) on 2020-05-25 01:02:09 in reply to 1 [link]

> perhaps this should be made into a global to avoid the warning.

The "`standin`" local variable is a structure used to clean up and free
prior memory allocations associated with the parse tree after a malloc()
failure while trying to acquire space to build a "`Select`" object.

  *  We cannot use a global for this, as that would cause problems if
     two or more threads all suffer a malloc() failure at about the
     same time.

  *  We cannot malloc for space.  The whole reason for using "`standin`"
     in the first place is that malloc isn't working.

Hence "`standin`" must be a stack variable.

As you observe, the logic of the overall system and especially the
assert() on the previous line demonstrate that the value returned
cannot be the stack variable.

This problem only arises because SQLite is very careful to not leak
memory nor crash following a malloc() failure.  We test that 
using simulated malloc() failures.

Perhaps you should bring this false-positive warning message to the
attention of the people who maintain the CGo compiler?

(3) By Richard Hipp (drh) on 2020-05-25 01:39:44 in reply to 2 [link]

On second thought, there is a way to work around this false-positive warning
(I think - I don't have a convenient way to test it.)  See my proposed
work-around on the [cgo-warning-workaround branch][1].

This work-around makes SQLite very slightly larger and slower.
So then the question becomes:  Do we allow SQLite to grow slightly larger
and slower on billions of devices all over the world in order to silence
a false-positive compiler warning from CGo?  To be fair, we have already
done that for MSVC.  But MSVC seems like a more important compiler than CGo.
I think I'll leave the patch on the branch while we ponder this question.

[1]: https://www.sqlite.org/src/vdiff?branch=cgo-warning-workaround&dc=20

(6) By Adam S Levy (alaskanarcher) on 2020-05-25 19:29:49 in reply to 3 [link]

Thanks for your thoughtful response.

I don't fully understand why the standin is necessary but I absolutely trust your expertise. The warning can be tolerated. It doesn't stop or hurt the build. It just will confuse Golang users of the package. 

I think I will just apply your patch in the amalgamation that the Golang package builds, since if size and speed is the ultimate priority for a user, Golang is probably the wrong language choice anyway.

(7) By Adam S Levy (alaskanarcher) on 2020-05-25 19:37:56 in reply to 6 [link]

BTW I tried your patch and it does silence the warning. Thank you.

(8) By kyle on 2020-05-25 21:09:41 in reply to 3 [link]

CGo isn't the compiler for the c code in this case, just a wrapper for CC, which is gcc in Adam's case.  I've tried to replicate the warning with a few gcc versions and the amalgamation, but cannot.

(9) By Mike (mike.mcternan) on 2020-05-26 06:20:03 in reply to 3 [link]

Your work-around works for me too.

I see this with gcc 10.1, as shipped with Fedora 32, when compiling at -O2 and above:

<code>
$ gcc -v
Using built-in specs.
COLLECT_GCC=/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/10/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.1.1 20200507 (Red Hat 10.1.1-1) (GCC) 
$ $ gcc -O2 -c sqlite3.c 
sqlite3.c: In function ‘sqlite3SelectNew’:
sqlite3.c:128048:10: warning: function may return address of local variable [-Wreturn-local-addr]
128048 |   return pNew;
       |          ^~~~
sqlite3.c:128008:10: note: declared here
128008 |   Select standin;
       |          ^~~~~~~
</code>

This was with sqlite-amalgamation-3310100, but fixed by your work-around at -O2 and -Wall -O3.

Note that I did try other workarounds, but couldn't get any to work.

Specifically I think the problem in the original code is that gcc can't track <code>pParse->db->mallocFailed</code>, either having to assume it could be aliased or modified by intermediate function calls.

Replacing the condition above the call to <code>clearSelect()</code> with <code>if ( `pNew==&standin || pParse->db->mallocFailed` )</code> should be robust (and cheap), but still doesn't fix the warning .


I'm not sure how much overhead your workaround adds (it looks fairly cheap to my eyes, but I am not a compiler!) but I'd guess the warning will become a lot more prevalent now GCC 10 is shipping with distros.

(10) By Mike (mike.mcternan) on 2020-05-30 07:32:58 in reply to 3 [link]

I see this made it back to trunk already, but just as an addendum, I roughly checked the overhead of the fix, using gcc (GCC) 10.1.1 20200507 (Red Hat 10.1.1-1) at -O2:
 

| text     | data    | bss  | dec     | hex  | filename |
---------------------------------------------------------
| 798015   | 8448    | 1784 | 808247 | c5537 | sqlite-amalgamation-3310100-fixed.o |
| 798015   | 8448    | 1784 | 808247 | c5537 | sqlite-amalgamation-3310100.o |
| 802126   | 8512    | 1784 | 812422 | c6586 | sqlite-amalgamation-3320100-fixed.o |
| 802126   | 8512    | 1784 | 812422 | c6586 | sqlite-amalgamation-3320100.o |

It doesn't look any different with out without the workaround applied to the last pair of releases.  Phew!

(11) By Richard Hipp (drh) on 2020-05-30 10:06:04 in reply to 10 [link]

I measure size and performance using -Os with GCC 5.4.0.  Your mileage may vary.

(16) By Mike (mike.mcternan) on 2020-05-31 03:03:53 in reply to 11 [link]

Okay, last one on this topic for me, but I'm getting better mileage with -Os and the fix:

gcc version 10.1.1 20200507 (Red Hat 10.1.1-1) (GCC)
-Os

|  text  |   data  |   bss  |   dec |    hex |filename |
|--------|---------|--------|-------|--------|---------|
| 530810 |   8448  |  1784  |541042 |  84172 |sqlite-amalgamation-3310100-fixed.o |
| 530826 |   8448  |  1784  |541058 |  84182 |sqlite-amalgamation-3310100.o |
| 534732 |   8512  |  1784  |545028 |  85104 |sqlite-amalgamation-3320100-fixed.o |
| 534748 |   8512  |  1784  |545044 |  85114 |sqlite-amalgamation-3320100.o       |

Thank you for your work on this, and sqlite as a whole.

(12) By anonymous on 2020-05-30 20:26:42 in reply to 3 [link]

Why not just declare `standin` static, properly initialized?

You don't intend to return its value, just testing the address in assert. This should silence the warning.

(13) By Richard Damon (RichardDamon) on 2020-05-30 21:25:51 in reply to 12 [link]

One thought is that wouldn't be thread-safe if two threads both hit the condition. The memory IS used internally to complete the operation, it just isn't returned.

(14) By anonymous on 2020-05-30 23:03:09 in reply to 13 [link]

From the code it looks like the variable's **address** value is used as a sentinel; it's read-only, used as a const value. As such it is thread-safe.

Moreover, static variable's address will be const by definition.

(15) By Richard Hipp (drh) on 2020-05-30 23:54:41 in reply to 14 [link]

A parse tree is being constructed.  The job of the sqlite3SelectNew() routine
is to takes a bunch of subtrees as input (the pararameters to the
sqlite3SelectNew() call), allocate a new SELECT object,
attach the subtress to the Select object, then returns the new Select
object.  Ownership of the subtrees passes from the caller into the new
Select object.

If an out-of-memory (OOM) error occurs, sqlite3SelectNew() should return NULL.
But before doing so, it also has to free all of the substructure that was
passed in as parameters (because it owns it).  The easiest way to delete all
that substructure is to load it all into a Select object, and invoke the
destructor on that Select object.  But if the OOM occurred while allocating
the Select object itself, what can you use to load the substructure into?

Solution:  Load the substructure into a Select object on the stack, and
call the destructor for that Stack object instead.  The destructor in this
case is the clearSelect() routine.  clearSelect() always frees the substructure,
but it only frees the Select object itself if the second parameter is true.
That parameter is false when we are deleting the stack-based Select object,
since obviously it would be a problem if you tried to free an object on the
stack.

So the "`standin`" variable is used (though rarely - only when an OOM occurs),
and it could in theory be used simultaneously by two different threads.  So
it cannot be static.

(17) By anonymous on 2020-05-31 03:08:24 in reply to 15 [link]

Thank you for the explanation. I can see that the current version (using pAllocated) in this case is the most easiest fix to silence the eager compiler.

Looking closely into the details, it seems that the whole `sqlite3SelectNew()` function is executed under an active mutex, since it calls [`sqlite3DbMallocRawNN()`](https://www.sqlite.org/src/file?udc=1&ln=588-635&ci=1cb248a3fc4c35c5&name=src%2Fmalloc.c) which expects `assert( sqlite3_mutex_held(db->mutex) );`.

Does this mean the whole function is effectively single-threaded?

(18) By Keith Medcalf (kmedcalf) on 2020-05-31 05:15:31 in reply to 17 [link]

No.

SQLite3 is multiple-entrant (can be executed on multiple threads simultaneously) but is only singly-entrant per connection.

In other words, the sqlite3SelectNew() function will only be executed on one thread per connection, however, multiple connections may execute the function concurrently.  The mutex protects simultaneous access to the connection and is not a single execution path (a critical section).

(20) By anonymous on 2020-07-10 21:39:01 in reply to 2

Hi, I have the same warning with the latest SQLite & GCC 10.

> As you observe, the logic of the overall system and especially the assert() on the previous line demonstrate that the value returned cannot be the stack variable.

But assert does nothing when NDEBUG is defined, and this is exactly what SQLite does by default:

```
** Setting NDEBUG makes the code smaller and faster by disabling the
** assert() statements in the code.  So we want the default action
** to be for NDEBUG to be set and NDEBUG to be undefined only if SQLITE_DEBUG
** is set.  Thus NDEBUG becomes an opt-in rather than an opt-out
** feature.
*/
#if !defined(NDEBUG) && !defined(SQLITE_DEBUG)
# define NDEBUG 1
#endif
#if defined(NDEBUG) && defined(SQLITE_DEBUG)
# undef NDEBUG
#endif
```

Is it possible to silence it with something like:

```diff
   assert( pNew!=&standin );
+  pNew = NULL;
```
?

Thanks.

(21) By Richard Hipp (drh) on 2020-07-10 21:53:08 in reply to 20 [link]

See <https://www.sqlite.org/src/info/04e1edd8e5821a37> for the actual
work-around.  The [prerelease snapshot][1] contains the work-around.

I consider this to be a bug in GCC-10.  But I have worked around the bug
nevertheless.  It isn't the first compiler bug to be worked around (some prior
bugs were quite a bit more serious) and it probably won't be the last.

[1]: https://sqlite.org/download.html

(4) By kyle on 2020-05-25 17:11:58 in reply to 1 [link]

Adam, can you give some more information on what compiler cgo is using and which flags?  I couldn't reproduce on my local machine.  The full output of `go env` would help.

(5) By Adam S Levy (alaskanarcher) on 2020-05-25 19:21:26 in reply to 4 [link]

```
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/aslevy/.cache/go-build"
GOENV="/home/aslevy/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/aslevy/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/aslevy/repos/go/AdamSLevy/flagbind/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build049582596=/tmp/go-build -gno-record-gcc-switches"
```

Please also note that if you are building the master branch of `crawshaw.io/sqlite` I pushed a small patch to silence the warning, but from Richard Hipp's response I think that my patch may be foolish. If you want to reproduce it try commit `1afc5f0`  on the repo at [https://github.com/crawshaw/sqlite/](https://github.com/crawshaw/sqlite/)

(19) By Chris Carson (chriscarson) on 2020-06-20 10:38:27 in reply to 1 [link]

Very helpful thread.  For me, just turning off the category of gcc warnings is an effective workaround.


export CGO_CFLAGS="-g -O2 -Wno-return-local-addr"