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

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?

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.

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.

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.

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

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.

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:

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

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

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

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

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.

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.

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.

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.

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() which expects assert( sqlite3_mutex_held(db->mutex) );.

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

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

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:

   assert( pNew!=&standin );
+  pNew = NULL;

?

Thanks.

See https://www.sqlite.org/src/info/04e1edd8e5821a37 for the actual work-around. The prerelease snapshot 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.

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.

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

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"